[v2,17/20] x86: efistub: Check SEV/SNP support while running in the firmware

  The decompressor executes in an environment with little or no access to
a console, and without any ability to return an error back to the caller
(the bootloader). So the only recourse we have when the SEV/SNP context
is not quite what the kernel expects is to terminate the guest entirely.

This is a bit harsh, and also unnecessary when booting via the EFI stub,
given that it provides all the support that SEV guests need to probe the
underlying platform.

So let's do the SEV initialization and SNP feature check before calling
ExitBootServices(), and simply return with an error if the SNP feature
mask is not as expected.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/sev.c          | 12 ++++++++----
 arch/x86/include/asm/sev.h              |  4 ++++
 drivers/firmware/efi/libstub/x86-stub.c | 17 +++++++++++++++++
 3 files changed, 29 insertions(+), 4 deletions(-)
  

On 5/8/23 02:03, Ard Biesheuvel wrote:
> The decompressor executes in an environment with little or no access to
> a console, and without any ability to return an error back to the caller
> (the bootloader). So the only recourse we have when the SEV/SNP context
> is not quite what the kernel expects is to terminate the guest entirely.
> 
> This is a bit harsh, and also unnecessary when booting via the EFI stub,
> given that it provides all the support that SEV guests need to probe the
> underlying platform.
> 
> So let's do the SEV initialization and SNP feature check before calling
> ExitBootServices(), and simply return with an error if the SNP feature
> mask is not as expected.

My SEV-ES / SEV-SNP guests started crashing when I applied this patch.
Turns out that sev_es_negotiate_protocol() used to be called when no #VC
exceptions were being generated before a valid GHCB was setup. Because
of that the current GHCB MSR value was not saved/restored. But now,
sev_es_negotiate_protocol() is called earlier in the boot process and
there are still messages being issued by UEFI, e.g.:

SetUefiImageMemoryAttributes - 0x000000007F6D7000 - 0x0000000000006000 (0x0000000000000008)

Similarly, get_hv_features() didn't worry about saving the GHCB MSR value
and an SNP guest crashed after fixing sev_es_negotiate_protocol().

The following changes got me past everything:

diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 3a5b0c9c4fcc..23450628d41c 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -106,15 +106,19 @@ static void __noreturn sev_es_terminate(unsigned int set, unsigned int reason)
   */
  static u64 get_hv_features(void)
  {
-	u64 val;
+	u64 val, save;
  
  	if (ghcb_version < 2)
  		return 0;
  
+	save = sev_es_rd_ghcb_msr();
+
  	sev_es_wr_ghcb_msr(GHCB_MSR_HV_FT_REQ);
  	VMGEXIT();
-
  	val = sev_es_rd_ghcb_msr();
+
+	sev_es_wr_ghcb_msr(save);
+
  	if (GHCB_RESP_CODE(val) != GHCB_MSR_HV_FT_RESP)
  		return 0;
  
@@ -139,13 +143,17 @@ static void snp_register_ghcb_early(unsigned long paddr)
  
  static bool sev_es_negotiate_protocol(void)
  {
-	u64 val;
+	u64 val, save;
+
+	save = sev_es_rd_ghcb_msr();
  
  	/* Do the GHCB protocol version negotiation */
  	sev_es_wr_ghcb_msr(GHCB_MSR_SEV_INFO_REQ);
  	VMGEXIT();
  	val = sev_es_rd_ghcb_msr();
  
+	sev_es_wr_ghcb_msr(save);
+
  	if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP)
  		return false;
  

Thanks,
Tom

> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   arch/x86/boot/compressed/sev.c          | 12 ++++++++----
>   arch/x86/include/asm/sev.h              |  4 ++++
>   drivers/firmware/efi/libstub/x86-stub.c | 17 +++++++++++++++++
>   3 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index 014b89c890887b9a..19c40873fdd209b5 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -315,20 +315,24 @@ static void enforce_vmpl0(void)
>    */
>   #define SNP_FEATURES_PRESENT (0)
>   
> +u64 snp_get_unsupported_features(void)
> +{
> +	if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
> +		return 0;
> +	return sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
> +}
> +
>   void snp_check_features(void)
>   {
>   	u64 unsupported;
>   
> -	if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
> -		return;
> -
>   	/*
>   	 * Terminate the boot if hypervisor has enabled any feature lacking
>   	 * guest side implementation. Pass on the unsupported features mask through
>   	 * EXIT_INFO_2 of the GHCB protocol so that those features can be reported
>   	 * as part of the guest boot failure.
>   	 */
> -	unsupported = sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
> +	unsupported = snp_get_unsupported_features();
>   	if (unsupported) {
>   		if (ghcb_version < 2 || (!boot_ghcb && !early_setup_ghcb()))
>   			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 13dc2a9d23c1eb25..bf27b91644d0da5a 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -157,6 +157,7 @@ static __always_inline void sev_es_nmi_complete(void)
>   		__sev_es_nmi_complete();
>   }
>   extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
> +extern void sev_enable(struct boot_params *bp);
>   
>   static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs)
>   {
> @@ -202,12 +203,14 @@ void snp_set_wakeup_secondary_cpu(void);
>   bool snp_init(struct boot_params *bp);
>   void __init __noreturn snp_abort(void);
>   int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio);
> +u64 snp_get_unsupported_features(void);
>   #else
>   static inline void sev_es_ist_enter(struct pt_regs *regs) { }
>   static inline void sev_es_ist_exit(void) { }
>   static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
>   static inline void sev_es_nmi_complete(void) { }
>   static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; }
> +static inline void sev_enable(struct boot_params *bp) { }
>   static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
>   static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
>   static inline void setup_ghcb(void) { }
> @@ -225,6 +228,7 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
>   {
>   	return -ENOTTY;
>   }
> +static inline u64 snp_get_unsupported_features(void) { return 0; }
>   #endif
>   
>   #endif
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index ce8434fce0c37982..33d11ba78f1d8c4f 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -15,6 +15,7 @@
>   #include <asm/setup.h>
>   #include <asm/desc.h>
>   #include <asm/boot.h>
> +#include <asm/sev.h>
>   
>   #include "efistub.h"
>   
> @@ -714,6 +715,22 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
>   			  &p->efi->efi_memmap, &p->efi->efi_memmap_hi);
>   	p->efi->efi_memmap_size		= map->map_size;
>   
> +	/*
> +	 * Call the SEV init code while still running with the firmware's
> +	 * GDT/IDT, so #VC exceptions will be handled by EFI.
> +	 */
> +	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
> +		u64 unsupported;
> +
> +		sev_enable(p->boot_params);
> +		unsupported = snp_get_unsupported_features();
> +		if (unsupported) {
> +			efi_err("Unsupported SEV-SNP features detected: 0x%llx\n",
> +				unsupported);
> +			return EFI_UNSUPPORTED;
> +		}
> +	}
> +
>   	return EFI_SUCCESS;
>   }
>
  

On Thu, 18 May 2023 at 22:16, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 5/8/23 02:03, Ard Biesheuvel wrote:
> > The decompressor executes in an environment with little or no access to
> > a console, and without any ability to return an error back to the caller
> > (the bootloader). So the only recourse we have when the SEV/SNP context
> > is not quite what the kernel expects is to terminate the guest entirely.
> >
> > This is a bit harsh, and also unnecessary when booting via the EFI stub,
> > given that it provides all the support that SEV guests need to probe the
> > underlying platform.
> >
> > So let's do the SEV initialization and SNP feature check before calling
> > ExitBootServices(), and simply return with an error if the SNP feature
> > mask is not as expected.
>
> My SEV-ES / SEV-SNP guests started crashing when I applied this patch.
> Turns out that sev_es_negotiate_protocol() used to be called when no #VC
> exceptions were being generated before a valid GHCB was setup. Because
> of that the current GHCB MSR value was not saved/restored. But now,
> sev_es_negotiate_protocol() is called earlier in the boot process and
> there are still messages being issued by UEFI, e.g.:
>
> SetUefiImageMemoryAttributes - 0x000000007F6D7000 - 0x0000000000006000 (0x0000000000000008)
>
> Similarly, get_hv_features() didn't worry about saving the GHCB MSR value
> and an SNP guest crashed after fixing sev_es_negotiate_protocol().
>

Thanks for the clarification

So the underlying assumption here is that performing these checks
before ExitBootServices() is better because we can still return to the
bootloader, which -like GRUB does- could simply attempt booting the
next kernel in the list.

I was obviously unaware of the complication you are hitting here. So I
wonder what your take is on this: should we defer this check until
after ExitBootServices(), and crash and burn like before if the test
fails? Or is the change below reasonable in your opinion, and we can
incorporate it? Or is there a third option, i.e., is there a SEV
specific EFI protocol that the stub should be using to establish
whether the underlying platform meets the kernel's expectations?


> The following changes got me past everything:
>
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index 3a5b0c9c4fcc..23450628d41c 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -106,15 +106,19 @@ static void __noreturn sev_es_terminate(unsigned int set, unsigned int reason)
>    */
>   static u64 get_hv_features(void)
>   {
> -       u64 val;
> +       u64 val, save;
>
>         if (ghcb_version < 2)
>                 return 0;
>
> +       save = sev_es_rd_ghcb_msr();
> +
>         sev_es_wr_ghcb_msr(GHCB_MSR_HV_FT_REQ);
>         VMGEXIT();
> -
>         val = sev_es_rd_ghcb_msr();
> +
> +       sev_es_wr_ghcb_msr(save);
> +
>         if (GHCB_RESP_CODE(val) != GHCB_MSR_HV_FT_RESP)
>                 return 0;
>
> @@ -139,13 +143,17 @@ static void snp_register_ghcb_early(unsigned long paddr)
>
>   static bool sev_es_negotiate_protocol(void)
>   {
> -       u64 val;
> +       u64 val, save;
> +
> +       save = sev_es_rd_ghcb_msr();
>
>         /* Do the GHCB protocol version negotiation */
>         sev_es_wr_ghcb_msr(GHCB_MSR_SEV_INFO_REQ);
>         VMGEXIT();
>         val = sev_es_rd_ghcb_msr();
>
> +       sev_es_wr_ghcb_msr(save);
> +
>         if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP)
>                 return false;
>
>
> Thanks,
> Tom
>
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >   arch/x86/boot/compressed/sev.c          | 12 ++++++++----
> >   arch/x86/include/asm/sev.h              |  4 ++++
> >   drivers/firmware/efi/libstub/x86-stub.c | 17 +++++++++++++++++
> >   3 files changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> > index 014b89c890887b9a..19c40873fdd209b5 100644
> > --- a/arch/x86/boot/compressed/sev.c
> > +++ b/arch/x86/boot/compressed/sev.c
> > @@ -315,20 +315,24 @@ static void enforce_vmpl0(void)
> >    */
> >   #define SNP_FEATURES_PRESENT (0)
> >
> > +u64 snp_get_unsupported_features(void)
> > +{
> > +     if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
> > +             return 0;
> > +     return sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
> > +}
> > +
> >   void snp_check_features(void)
> >   {
> >       u64 unsupported;
> >
> > -     if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
> > -             return;
> > -
> >       /*
> >        * Terminate the boot if hypervisor has enabled any feature lacking
> >        * guest side implementation. Pass on the unsupported features mask through
> >        * EXIT_INFO_2 of the GHCB protocol so that those features can be reported
> >        * as part of the guest boot failure.
> >        */
> > -     unsupported = sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
> > +     unsupported = snp_get_unsupported_features();
> >       if (unsupported) {
> >               if (ghcb_version < 2 || (!boot_ghcb && !early_setup_ghcb()))
> >                       sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> > index 13dc2a9d23c1eb25..bf27b91644d0da5a 100644
> > --- a/arch/x86/include/asm/sev.h
> > +++ b/arch/x86/include/asm/sev.h
> > @@ -157,6 +157,7 @@ static __always_inline void sev_es_nmi_complete(void)
> >               __sev_es_nmi_complete();
> >   }
> >   extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
> > +extern void sev_enable(struct boot_params *bp);
> >
> >   static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs)
> >   {
> > @@ -202,12 +203,14 @@ void snp_set_wakeup_secondary_cpu(void);
> >   bool snp_init(struct boot_params *bp);
> >   void __init __noreturn snp_abort(void);
> >   int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio);
> > +u64 snp_get_unsupported_features(void);
> >   #else
> >   static inline void sev_es_ist_enter(struct pt_regs *regs) { }
> >   static inline void sev_es_ist_exit(void) { }
> >   static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
> >   static inline void sev_es_nmi_complete(void) { }
> >   static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; }
> > +static inline void sev_enable(struct boot_params *bp) { }
> >   static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
> >   static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
> >   static inline void setup_ghcb(void) { }
> > @@ -225,6 +228,7 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
> >   {
> >       return -ENOTTY;
> >   }
> > +static inline u64 snp_get_unsupported_features(void) { return 0; }
> >   #endif
> >
> >   #endif
> > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > index ce8434fce0c37982..33d11ba78f1d8c4f 100644
> > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > @@ -15,6 +15,7 @@
> >   #include <asm/setup.h>
> >   #include <asm/desc.h>
> >   #include <asm/boot.h>
> > +#include <asm/sev.h>
> >
> >   #include "efistub.h"
> >
> > @@ -714,6 +715,22 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
> >                         &p->efi->efi_memmap, &p->efi->efi_memmap_hi);
> >       p->efi->efi_memmap_size         = map->map_size;
> >
> > +     /*
> > +      * Call the SEV init code while still running with the firmware's
> > +      * GDT/IDT, so #VC exceptions will be handled by EFI.
> > +      */
> > +     if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
> > +             u64 unsupported;
> > +
> > +             sev_enable(p->boot_params);
> > +             unsupported = snp_get_unsupported_features();
> > +             if (unsupported) {
> > +                     efi_err("Unsupported SEV-SNP features detected: 0x%llx\n",
> > +                             unsupported);
> > +                     return EFI_UNSUPPORTED;
> > +             }
> > +     }
> > +
> >       return EFI_SUCCESS;
> >   }
> >
  

On 5/18/23 17:27, Ard Biesheuvel wrote:
> On Thu, 18 May 2023 at 22:16, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 5/8/23 02:03, Ard Biesheuvel wrote:
>>> The decompressor executes in an environment with little or no access to
>>> a console, and without any ability to return an error back to the caller
>>> (the bootloader). So the only recourse we have when the SEV/SNP context
>>> is not quite what the kernel expects is to terminate the guest entirely.
>>>
>>> This is a bit harsh, and also unnecessary when booting via the EFI stub,
>>> given that it provides all the support that SEV guests need to probe the
>>> underlying platform.
>>>
>>> So let's do the SEV initialization and SNP feature check before calling
>>> ExitBootServices(), and simply return with an error if the SNP feature
>>> mask is not as expected.
>>
>> My SEV-ES / SEV-SNP guests started crashing when I applied this patch.
>> Turns out that sev_es_negotiate_protocol() used to be called when no #VC
>> exceptions were being generated before a valid GHCB was setup. Because
>> of that the current GHCB MSR value was not saved/restored. But now,
>> sev_es_negotiate_protocol() is called earlier in the boot process and
>> there are still messages being issued by UEFI, e.g.:
>>
>> SetUefiImageMemoryAttributes - 0x000000007F6D7000 - 0x0000000000006000 (0x0000000000000008)
>>
>> Similarly, get_hv_features() didn't worry about saving the GHCB MSR value
>> and an SNP guest crashed after fixing sev_es_negotiate_protocol().
>>
> 
> Thanks for the clarification
> 
> So the underlying assumption here is that performing these checks
> before ExitBootServices() is better because we can still return to the
> bootloader, which -like GRUB does- could simply attempt booting the
> next kernel in the list.
> 
> I was obviously unaware of the complication you are hitting here. So I
> wonder what your take is on this: should we defer this check until
> after ExitBootServices(), and crash and burn like before if the test
> fails? Or is the change below reasonable in your opinion, and we can

Deferring the checks is probably the safest thing to do, since that would 
match the way things are done today and known to work. I'm not sure what 
other things might pop up if we stay with this approach, for example, page 
state change calls using the GHCB MSR protocol that also don't 
save/restore the MSR value.

It is possible to audit these areas and stay with this approach, but I'm 
wondering if that wouldn't be better done as a separate patch series.

Adding @Joerg for any additional thoughts he might have around this area, too.

> incorporate it? Or is there a third option, i.e., is there a SEV
> specific EFI protocol that the stub should be using to establish
> whether the underlying platform meets the kernel's expectations?

There isn't currently an EFI protocol do that.

Thanks,
Tom

> 
> 
>> The following changes got me past everything:
>>
>> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
>> index 3a5b0c9c4fcc..23450628d41c 100644
>> --- a/arch/x86/kernel/sev-shared.c
>> +++ b/arch/x86/kernel/sev-shared.c
>> @@ -106,15 +106,19 @@ static void __noreturn sev_es_terminate(unsigned int set, unsigned int reason)
>>     */
>>    static u64 get_hv_features(void)
>>    {
>> -       u64 val;
>> +       u64 val, save;
>>
>>          if (ghcb_version < 2)
>>                  return 0;
>>
>> +       save = sev_es_rd_ghcb_msr();
>> +
>>          sev_es_wr_ghcb_msr(GHCB_MSR_HV_FT_REQ);
>>          VMGEXIT();
>> -
>>          val = sev_es_rd_ghcb_msr();
>> +
>> +       sev_es_wr_ghcb_msr(save);
>> +
>>          if (GHCB_RESP_CODE(val) != GHCB_MSR_HV_FT_RESP)
>>                  return 0;
>>
>> @@ -139,13 +143,17 @@ static void snp_register_ghcb_early(unsigned long paddr)
>>
>>    static bool sev_es_negotiate_protocol(void)
>>    {
>> -       u64 val;
>> +       u64 val, save;
>> +
>> +       save = sev_es_rd_ghcb_msr();
>>
>>          /* Do the GHCB protocol version negotiation */
>>          sev_es_wr_ghcb_msr(GHCB_MSR_SEV_INFO_REQ);
>>          VMGEXIT();
>>          val = sev_es_rd_ghcb_msr();
>>
>> +       sev_es_wr_ghcb_msr(save);
>> +
>>          if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP)
>>                  return false;
>>
>>
>> Thanks,
>> Tom
>>
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>>    arch/x86/boot/compressed/sev.c          | 12 ++++++++----
>>>    arch/x86/include/asm/sev.h              |  4 ++++
>>>    drivers/firmware/efi/libstub/x86-stub.c | 17 +++++++++++++++++
>>>    3 files changed, 29 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
>>> index 014b89c890887b9a..19c40873fdd209b5 100644
>>> --- a/arch/x86/boot/compressed/sev.c
>>> +++ b/arch/x86/boot/compressed/sev.c
>>> @@ -315,20 +315,24 @@ static void enforce_vmpl0(void)
>>>     */
>>>    #define SNP_FEATURES_PRESENT (0)
>>>
>>> +u64 snp_get_unsupported_features(void)
>>> +{
>>> +     if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
>>> +             return 0;
>>> +     return sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
>>> +}
>>> +
>>>    void snp_check_features(void)
>>>    {
>>>        u64 unsupported;
>>>
>>> -     if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
>>> -             return;
>>> -
>>>        /*
>>>         * Terminate the boot if hypervisor has enabled any feature lacking
>>>         * guest side implementation. Pass on the unsupported features mask through
>>>         * EXIT_INFO_2 of the GHCB protocol so that those features can be reported
>>>         * as part of the guest boot failure.
>>>         */
>>> -     unsupported = sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
>>> +     unsupported = snp_get_unsupported_features();
>>>        if (unsupported) {
>>>                if (ghcb_version < 2 || (!boot_ghcb && !early_setup_ghcb()))
>>>                        sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
>>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>>> index 13dc2a9d23c1eb25..bf27b91644d0da5a 100644
>>> --- a/arch/x86/include/asm/sev.h
>>> +++ b/arch/x86/include/asm/sev.h
>>> @@ -157,6 +157,7 @@ static __always_inline void sev_es_nmi_complete(void)
>>>                __sev_es_nmi_complete();
>>>    }
>>>    extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
>>> +extern void sev_enable(struct boot_params *bp);
>>>
>>>    static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs)
>>>    {
>>> @@ -202,12 +203,14 @@ void snp_set_wakeup_secondary_cpu(void);
>>>    bool snp_init(struct boot_params *bp);
>>>    void __init __noreturn snp_abort(void);
>>>    int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio);
>>> +u64 snp_get_unsupported_features(void);
>>>    #else
>>>    static inline void sev_es_ist_enter(struct pt_regs *regs) { }
>>>    static inline void sev_es_ist_exit(void) { }
>>>    static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
>>>    static inline void sev_es_nmi_complete(void) { }
>>>    static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; }
>>> +static inline void sev_enable(struct boot_params *bp) { }
>>>    static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
>>>    static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
>>>    static inline void setup_ghcb(void) { }
>>> @@ -225,6 +228,7 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
>>>    {
>>>        return -ENOTTY;
>>>    }
>>> +static inline u64 snp_get_unsupported_features(void) { return 0; }
>>>    #endif
>>>
>>>    #endif
>>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
>>> index ce8434fce0c37982..33d11ba78f1d8c4f 100644
>>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>>> @@ -15,6 +15,7 @@
>>>    #include <asm/setup.h>
>>>    #include <asm/desc.h>
>>>    #include <asm/boot.h>
>>> +#include <asm/sev.h>
>>>
>>>    #include "efistub.h"
>>>
>>> @@ -714,6 +715,22 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
>>>                          &p->efi->efi_memmap, &p->efi->efi_memmap_hi);
>>>        p->efi->efi_memmap_size         = map->map_size;
>>>
>>> +     /*
>>> +      * Call the SEV init code while still running with the firmware's
>>> +      * GDT/IDT, so #VC exceptions will be handled by EFI.
>>> +      */
>>> +     if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
>>> +             u64 unsupported;
>>> +
>>> +             sev_enable(p->boot_params);
>>> +             unsupported = snp_get_unsupported_features();
>>> +             if (unsupported) {
>>> +                     efi_err("Unsupported SEV-SNP features detected: 0x%llx\n",
>>> +                             unsupported);
>>> +                     return EFI_UNSUPPORTED;
>>> +             }
>>> +     }
>>> +
>>>        return EFI_SUCCESS;
>>>    }
>>>
  

On Fri, May 19, 2023 at 09:04:46AM -0500, Tom Lendacky wrote:
> Deferring the checks is probably the safest thing to do, since that would
> match the way things are done today and known to work. I'm not sure what
> other things might pop up if we stay with this approach, for example, page
> state change calls using the GHCB MSR protocol that also don't save/restore
> the MSR value.
> 
> It is possible to audit these areas and stay with this approach, but I'm
> wondering if that wouldn't be better done as a separate patch series.
> 
> Adding @Joerg for any additional thoughts he might have around this area, too.

If I got it correctly the patch actually moves two things before
ExitBootServices:

	1) SEV features check

	2) SEV initialization

I think it makes a lot of sense to have 1) before ExitBootServices. It
allows to soft-fail in case the kernel does not support all required
SEV-SNP features and move on to a kernel which does. This check also only
needs the SEV_STATUS MSR and not any GHCB calls.

The problem is the GHCB protocol negotiation with the HV, but the GHCB
protocol is downward-compatible, so an older kernel can work with a
newer HV.

But 2) needs to stay after ExitBootServices, as it needs resources owned
by UEFI, e.g. the GHCB MSR and potentially the configured GHCB itself.
Fiddling around with the GHCB MSR while it is still owned by UEFI will
bite us in one or the other way (e.g. UEFI, before ExitBootServices, is
free to take IRQs with handlers that rely on the GHCB MSR content).

Regards,

	Joerg
  

On Mon, 22 May 2023 at 14:48, Joerg Roedel <jroedel@suse.de> wrote:
>
> On Fri, May 19, 2023 at 09:04:46AM -0500, Tom Lendacky wrote:
> > Deferring the checks is probably the safest thing to do, since that would
> > match the way things are done today and known to work. I'm not sure what
> > other things might pop up if we stay with this approach, for example, page
> > state change calls using the GHCB MSR protocol that also don't save/restore
> > the MSR value.
> >
> > It is possible to audit these areas and stay with this approach, but I'm
> > wondering if that wouldn't be better done as a separate patch series.
> >
> > Adding @Joerg for any additional thoughts he might have around this area, too.
>
> If I got it correctly the patch actually moves two things before
> ExitBootServices:
>
>         1) SEV features check
>
>         2) SEV initialization
>
> I think it makes a lot of sense to have 1) before ExitBootServices. It
> allows to soft-fail in case the kernel does not support all required
> SEV-SNP features and move on to a kernel which does. This check also only
> needs the SEV_STATUS MSR and not any GHCB calls.
>
> The problem is the GHCB protocol negotiation with the HV, but the GHCB
> protocol is downward-compatible, so an older kernel can work with a
> newer HV.
>
> But 2) needs to stay after ExitBootServices, as it needs resources owned
> by UEFI, e.g. the GHCB MSR and potentially the configured GHCB itself.
> Fiddling around with the GHCB MSR while it is still owned by UEFI will
> bite us in one or the other way (e.g. UEFI, before ExitBootServices, is
> free to take IRQs with handlers that rely on the GHCB MSR content).
>

Thanks for the insight. Note that I have sent a v3 in the mean time
that moves all of this *after* ExitBootServices() [0], but I failed to
cc you - apologies.

So IIUC, we could just read sev_status much earlier just to perform
the SNP feature check, and fail the boot gracefully on a mismatch. And
the sev_enable() call needs to move after ExitBootServices(), right?

That would result in only very minor duplication, afaict. I'll have a
stab at implementing this for v4.

Thanks,




[0] https://lore.kernel.org/all/20230522071415.501717-21-ardb@kernel.org/
  

On Mon, May 22, 2023 at 03:07:12PM +0200, Ard Biesheuvel wrote:
> So IIUC, we could just read sev_status much earlier just to perform
> the SNP feature check, and fail the boot gracefully on a mismatch. And
> the sev_enable() call needs to move after ExitBootServices(), right?

Right, sev_enable() negotiates the GHCB protocol version, which needs
the GHCB MSR, so that has to stay after ExitBootServices(). The
SEV feature check on the other side only needs to read the sev-status
MSR, which is no problem before ExitBootServices() (as long as it is
only read on SEV platforms).

> That would result in only very minor duplication, afaict. I'll have a
> stab at implementing this for v4.

Thanks,