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

[Brijesh Singh <brijesh.singh@amd.com>]

On 5/25/21 6:11 AM, Borislav Petkov wrote:
> 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?

Okay, I will see what I can do to avoid the forward declarations.


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

I am not ignoring your valuable feedback; I am sorry if I came across
like that.

In this particular case, the snp_register_ghcb() is shared between the
decompress and main kernel. The variable data->snp_ghcb_registered is
not visible in the decompressed path, so I choose not to set it to true
inside the function itself.


> Thx.
>