Re: [PATCH 05/11] x86/sev: Perform PVALIDATE using the SVSM when not at VMPL0

[Tom Lendacky <thomas.lendacky@amd.com>]

On 1/26/24 18:59, Dionna Amalie Glaze wrote:
> On Fri, Jan 26, 2024 at 2:18 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> The PVALIDATE instruction can only be performed at VMPL0. An SVSM will
>> be present when running at VMPL1 or a lower privilege level.
>>
>> When an SVSM is present, use the SVSM_CORE_PVALIDATE call to perform
>> memory validation instead of issuing the PVALIDATE instruction directly.
>>
>> The validation of a single 4K page is now explicitly identified as such
>> in the function name, pvalidate_4k_page(). The pvalidate_pages() function
>> is used for validating 1 or more pages at either 4K or 2M in size. Each
>> function, however, determines whether it can issue the PVALIDATE directly
>> or whether the SVSM needs to be invoked.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   arch/x86/boot/compressed/sev.c |  42 +++++++-
>>   arch/x86/include/asm/sev.h     |  22 +++++
>>   arch/x86/kernel/sev-shared.c   | 176 ++++++++++++++++++++++++++++++++-
>>   arch/x86/kernel/sev.c          |  25 +++--
>>   4 files changed, 247 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
>> index 5d2403914ceb..3fbb614c31e0 100644
>> --- a/arch/x86/boot/compressed/sev.c
>> +++ b/arch/x86/boot/compressed/sev.c

>> +static int svsm_protocol(struct svsm_call *call)
>> +{
>> +       struct ghcb *ghcb;
>> +       int ret;
>> +
>> +       if (boot_ghcb)
>> +               ghcb = boot_ghcb;
>> +       else
>> +               ghcb = NULL;
>> +
>> +       do {
>> +               ret = ghcb ? __svsm_ghcb_protocol(ghcb, call)
>> +                          : __svsm_msr_protocol(call);
>> +       } while (ret == SVSM_ERR_BUSY);
> 
> Should this loop forever or eventually give up and panic?

The question becomes how many times before giving up. This would likely 
only happen if the hypervisor is causing an issue for the SVSM, so it 
would be similar to a DoS. I'm open to suggestions, though.

On a side not, that does remind that this should also be checking for 
SVSM_ERR_INCOMPLETE.

> 

>>   void snp_set_page_private(unsigned long paddr)
>> @@ -261,6 +289,12 @@ void sev_es_shutdown_ghcb(void)
>>          if (!sev_es_check_cpu_features())
>>                  error("SEV-ES CPU Features missing.");
>>
>> +       /*
>> +        * Ensure that the boot GHCB isn't used for the PVALIDATE when running
> 
> Why the definite article? Which PVALIDATE is this referring to?

I'll clarify the comment, but it is specifically relates to the 
set_page_encrypted() for the boot_ghcb_page that immediately follows. 
Since the GHCB page is being changed to encrypted, we need to use the MSR 
protocol by zeroing out the boot_ghcb variable. The comment should have 
said for the Page State Change and not PVALIDATE.

Thanks,
Tom

>