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@xxxxxxx>
> ---
> 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)
--
