| Message ID | 20170724200303.12197-12-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
On 09/13/2017 08:37 AM, Borislav Petkov wrote: ... >> + 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: > Actually, I see both approaches used in svm.c but I am flexible to go with either ways. lets wait for Paolo and Radim comments. -Brijesh
On Wed, Sep 13, 2017 at 10:14:20AM -0500, Brijesh Singh wrote: > Actually, I see both approaches used in svm.c but I am flexible to go with > either ways. lets wait for Paolo and Radim comments. Frankly, if you're going to access those fields only in svm.c, those accessors look like an overkill to me. But maybe there's a higher strategy behind it...
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; +} + +static inline unsigned int sev_get_handle(struct kvm *kvm) +{ + return to_sev_info(kvm)->handle; +} + +static inline bool sev_guest(struct kvm *kvm) +{ + return to_sev_info(kvm)->active; +} + +static inline int sev_get_asid(struct kvm *kvm) +{ + return to_sev_info(kvm)->asid; +} + +static inline int sev_get_fd(struct kvm *kvm) +{ + return to_sev_info(kvm)->sev_fd; +} + +static inline void sev_set_asid(struct kvm *kvm, int asid) +{ + to_sev_info(kvm)->asid = asid; +} + +static inline void sev_set_handle(struct kvm *kvm, unsigned int handle) +{ + to_sev_info(kvm)->handle = handle; +} + +static inline void sev_set_fd(struct kvm *kvm, int fd) +{ + to_sev_info(kvm)->sev_fd = fd; +} + static inline void mark_all_dirty(struct vmcb *vmcb) { vmcb->control.clean = 0;
The patch adds a new member (sev_info) in 'struct kvm_arch', and setter/getter functions for the sev_info field. 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(+)