Re: [PATCH Part1 RFC v2 13/20] x86/sev: Register GHCB memory when SEV-SNP is active

From: Borislav Petkov <bp@alien8.de>
To: Brijesh Singh <brijesh.singh@amd.com>
Cc: x86@kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, tglx@linutronix.de, jroedel@suse.de,
	thomas.lendacky@amd.com, pbonzini@redhat.com, mingo@redhat.com,
	dave.hansen@intel.com, rientjes@google.com, seanjc@google.com,
	peterz@infradead.org, hpa@zytor.com, tony.luck@intel.com
Subject: Re: [PATCH Part1 RFC v2 13/20] x86/sev: Register GHCB memory when SEV-SNP is active
Date: Tue, 25 May 2021 13:11:59 +0200	[thread overview]
Message-ID: <YKzbfwD6nHL7ChcJ@zn.tnic> (raw)
In-Reply-To: <20210430121616.2295-14-brijesh.singh@amd.com>

On Fri, Apr 30, 2021 at 07:16:09AM -0500, Brijesh Singh wrote:
> The SEV-SNP guest is required to perform GHCB GPA registration. This is
> because the hypervisor may prefer that a guest use a consistent and/or
> specific GPA for the GHCB associated with a vCPU. For more information,
> see the GHCB specification section GHCB GPA Registration.
> 
> During the boot, init_ghcb() allocates a per-cpu GHCB page. On very first
> VC exception, the exception handler switch to using the per-cpu GHCB page
> allocated during the init_ghcb(). The GHCB page must be registered in
> the current vcpu context.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  arch/x86/kernel/sev.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index 8c8c939a1754..e6819f170ec4 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -88,6 +88,13 @@ struct sev_es_runtime_data {
>  	 * is currently unsupported in SEV-ES guests.
>  	 */
>  	unsigned long dr7;
> +
> +	/*
> +	 * SEV-SNP requires that the GHCB must be registered before using it.
> +	 * The flag below will indicate whether the GHCB is registered, if its
> +	 * not registered then sev_es_get_ghcb() will perform the registration.
> +	 */
> +	bool snp_ghcb_registered;
>  };
>  
>  struct ghcb_state {
> @@ -100,6 +107,9 @@ DEFINE_STATIC_KEY_FALSE(sev_es_enable_key);
>  /* Needed in vc_early_forward_exception */
>  void do_early_exception(struct pt_regs *regs, int trapnr);
>  
> +/* Defined in sev-shared.c */
> +static void snp_register_ghcb(unsigned long paddr);

Can we get rid of those forward declarations pls? Due to sev-shared.c
this file is starting to spawn those and that's ugly.

Either through a code reorg or even defining a sev-internal.h header
which contains all those so that they don't pollute the code?

Thx.

> +
>  static void __init setup_vc_stacks(int cpu)
>  {
>  	struct sev_es_runtime_data *data;
> @@ -218,6 +228,12 @@ static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state)
>  		data->ghcb_active = true;
>  	}
>  
> +	/* SEV-SNP guest requires that GHCB must be registered before using it. */
> +	if (sev_snp_active() && !data->snp_ghcb_registered) {
> +		snp_register_ghcb(__pa(ghcb));
> +		data->snp_ghcb_registered = true;
> +	}

More missed review from last time:

"This needs to be set to true in the function itself, in the success
case."

Can you please be more careful and go through all review comments so
that I don't have to do the same work twice?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette