linux-kernel - Re: [RFC Part2 PATCH v3 11/26] KVM: X86: Extend struct kvm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170913133729.dtae2a3kidoigri5@pd.tnic>
Date:   Wed, 13 Sep 2017 15:37:29 +0200
From:   Borislav Petkov <bp@...e.de>
To:     Brijesh Singh <brijesh.singh@....com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, kvm@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Joerg Roedel <joro@...tes.org>,
        "Michael S . Tsirkin" <mst@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        \"Radim Krčmář\" <rkrcmar@...hat.com>,
        Tom Lendacky <thomas.lendacky@....com>
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@....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)
--