Messages in this thread | | | Date | Wed, 13 Sep 2017 15:37:29 +0200 | From | Borislav Petkov <> | Subject | Re: [RFC Part2 PATCH v3 11/26] KVM: X86: Extend struct kvm_arch to include SEV information |
| |
On Mon, Jul 24, 2017 at 03:02:48PM -0500, Brijesh Singh wrote: > The patch adds a new member (sev_info) in 'struct kvm_arch', and
Never say "This patch" in a commit message of a patch. It is tautologically useless.
> setter/getter functions for the sev_info field.
Also, I can see what the patch does from the hunk below. What is more important to explain in the commit message is *why* you're doing what you're doing.
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/include/asm/kvm_host.h | 9 +++++++++ > arch/x86/kvm/svm.c | 45 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 54 insertions(+) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 4295f82..150177e 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -742,6 +742,13 @@ enum kvm_irqchip_mode { > KVM_IRQCHIP_SPLIT, /* created with KVM_CAP_SPLIT_IRQCHIP */ > }; > > +struct kvm_sev_info { > + bool active; /* SEV enabled guest */ > + unsigned int handle; /* firmware handle */ > + unsigned int asid; /* asid for this guest */ > + int sev_fd; /* SEV device fd */ > +}; > + > struct kvm_arch { > unsigned int n_used_mmu_pages; > unsigned int n_requested_mmu_pages; > @@ -829,6 +836,8 @@ struct kvm_arch { > > bool x2apic_format; > bool x2apic_broadcast_quirk_disabled; > + > + struct kvm_sev_info sev_info; > }; > > struct kvm_vm_stat { > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 256c9df..2a5a03a 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -322,6 +322,51 @@ enum { > /* Secure Encrypted Virtualization */ > static unsigned int max_sev_asid; > > +static inline struct kvm_sev_info *to_sev_info(struct kvm *kvm) > +{ > + return &kvm->arch.sev_info; > +} > + > +static inline void sev_set_active(struct kvm *kvm) > +{ > + to_sev_info(kvm)->active = true; > +}
Is this the accepted way to do this in KVM land or can you simply access all members directly:
kvm->arch.sev_info.<member>
Because I see stuff like that:
static void kvm_gen_update_masterclock(struct kvm *kvm) { ...
struct kvm_arch *ka = &kvm->arch;
spin_lock(&ka->pvclock_gtod_sync_lock);
and
struct kvm_lapic *apic = svm->vcpu.arch.apic;
...
kvm_lapic_reg_write(apic, APIC_ICR2, icrh);
so why do you need the accessors?
-- Regards/Gruss, Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --
| |