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 <[email protected]> > --- > 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) --
