Re: [PATCH 5/7] x86/sev: Add a function to contain all SEV-specific setup operations

[Tom Lendacky]

On 7/29/26 20:48, Melody Wang wrote:
> To make the code clean in the boot phase, add a sev_prepare() wrapper
> which contains early SEV-specific checks in order to have all that code
> in a single place.
> 
> No functional changes.

I think this needs to be re-worked. Looking ahead to the next patch, if
anything is later added to the prepare routine that also returns, it will
all be attributed to the unsupported features. Right now you avoid that by
terminating in sev_prepare() in the next patch.

You might need some form of callback for error conditions or something
else, but as is, I think this is too fragile.

Thanks,
Tom

> 
> Signed-off-by: Melody Wang <huibo.wang@xxxxxxx>
> Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>
> ---
>  arch/x86/boot/compressed/sev.c          |  9 +++++++++
>  arch/x86/include/asm/sev.h              |  3 +++
>  drivers/firmware/efi/libstub/x86-stub.c | 20 ++++++--------------
>  3 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index fc2029746c50..655291a03dcc 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -511,3 +511,12 @@ bool early_is_sevsnp_guest(void)
>  	}
>  	return true;
>  }
> +
> +u64 sev_prepare(void)
> +{
> +	u64 unsupported = snp_get_unsupported_features(sev_get_status());
> +	if (unsupported)
> +		return unsupported;
> +
> +	return 0;
> +}
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 491a891a7694..b430c1aab403 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -613,6 +613,8 @@ static inline void sev_evict_cache(void *va, int npages)
>  	}
>  }
>  
> +u64 sev_prepare(void);
> +
>  #else	/* !CONFIG_AMD_MEM_ENCRYPT */
>  
>  #define snp_vmpl 0
> @@ -661,6 +663,7 @@ static inline enum es_result savic_register_gpa(u64 gpa) { return ES_UNSUPPORTED
>  static inline enum es_result savic_unregister_gpa(u64 *gpa) { return ES_UNSUPPORTED; }
>  static inline void hvs_ghcb_msr_write(u32 reg, u64 value) { }
>  static inline u64 hvs_ghcb_msr_read(u32 reg) { return 0; }
> +static inline u64 sev_prepare(void) { return 0; }
>  
>  #endif	/* CONFIG_AMD_MEM_ENCRYPT */
>  
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index cef32e2c82d8..95fa16fc9887 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -783,19 +783,6 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
>  	return EFI_SUCCESS;
>  }
>  
> -static bool have_unsupported_snp_features(void)
> -{
> -	u64 unsupported;
> -
> -	unsupported = snp_get_unsupported_features(sev_get_status());
> -	if (unsupported) {
> -		efi_err("Unsupported SEV-SNP features detected: 0x%llx\n",
> -			unsupported);
> -		return true;
> -	}
> -	return false;
> -}
> -
>  static void efi_get_seed(void *seed, int size)
>  {
>  	efi_get_random_bytes(size, seed);
> @@ -919,6 +906,7 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>  	unsigned long kernel_entry;
>  	struct setup_header *hdr;
>  	efi_status_t status;
> +	u64 unsup_feats;
>  
>  	efi_system_table = sys_table_arg;
>  	/* Check if we were booted by the EFI firmware */
> @@ -933,8 +921,12 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>  
>  	hdr = &boot_params->hdr;
>  
> -	if (have_unsupported_snp_features())
> +	unsup_feats = sev_prepare();
> +	if (unsup_feats) {
> +		efi_err("Unsupported SEV-SNP features detected: 0x%llx\n",
> +			unsup_feats);
>  		efi_exit(handle, EFI_UNSUPPORTED);
> +	}
>  
>  	if (IS_ENABLED(CONFIG_EFI_DXE_MEM_ATTRIBUTES)) {
>  		efi_dxe_table = get_efi_config_table(EFI_DXE_SERVICES_TABLE_GUID);