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

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

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;
>   }
>