Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751380AbdIMNhp (ORCPT <rfc822;w@1wt.eu>);
        Wed, 13 Sep 2017 09:37:45 -0400
Received: from mx2.suse.de ([195.135.220.15]:43742 "EHLO mx1.suse.de"
        rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP
        id S1750999AbdIMNhm (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 13 Sep 2017 09:37:42 -0400
Date: Wed, 13 Sep 2017 15:37:29 +0200
From: Borislav Petkov <bp@suse.de>
To: Brijesh Singh <brijesh.singh@amd.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, kvm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, Joerg Roedel <joro@8bytes.org>,
        "Michael S . Tsirkin" <mst@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        =?utf-8?B?XCJSYWRpbSBLcsSNbcOhxZlcIg==?= <rkrcmar@redhat.com>,
        Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [RFC Part2 PATCH v3 11/26] KVM: X86: Extend struct kvm_arch to
 include SEV information
Message-ID: <20170913133729.dtae2a3kidoigri5@pd.tnic>
References: <20170724200303.12197-1-brijesh.singh@amd.com>
 <20170724200303.12197-12-brijesh.singh@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20170724200303.12197-12-brijesh.singh@amd.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2521
Lines: 95

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