Re: [PATCH v6 05/10] x86/sme: Avoid SME/SVE related checks on non-SME/SVE platforms

[Tom Lendacky]

On 2/26/24 08:29, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@xxxxxxxxxx>
>
> Reorganize the early SME/SVE init code so that SME/SVE related calls are

I'm assuming you mean SEV here and in the subject line.

> deferred until it has been determined that the platform actually
> supports this, and so those calls could actually make sense.
>
> This removes logic from the early boot path that executes from the 1:1
> mapping when booting a CONFIG_AMD_MEM_ENCRYPT=y kernel on a system that
> does not implement that (i.e., 99% of distro kernels)

Maybe I'm missing something but I don't see how this has changed anything 
other than moving the call to sme_encrypt_kernel() within the if statement 
(which means the check at the beginning of sme_encrypt_kernel() could be 
changed do just check for SEV now).

> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> ---
>   arch/x86/include/asm/mem_encrypt.h | 4 ++--
>   arch/x86/kernel/head64.c           | 6 +++---
>   arch/x86/mm/mem_encrypt_identity.c | 8 +++-----
>   3 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
> index b31eb9fd5954..b1437ba0b3b8 100644
> --- a/arch/x86/include/asm/mem_encrypt.h
> +++ b/arch/x86/include/asm/mem_encrypt.h
> @@ -48,7 +48,7 @@ void __init sme_unmap_bootdata(char *real_mode_data);
>   void __init sme_early_init(void);
>   
>   void __init sme_encrypt_kernel(struct boot_params *bp);
> -void __init sme_enable(struct boot_params *bp);
> +void sme_enable(struct boot_params *bp);
>   
>   int __init early_set_memory_decrypted(unsigned long vaddr, unsigned long size);
>   int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size);
> @@ -82,7 +82,7 @@ static inline void __init sme_unmap_bootdata(char *real_mode_data) { }
>   static inline void __init sme_early_init(void) { }
>   
>   static inline void __init sme_encrypt_kernel(struct boot_params *bp) { }
> -static inline void __init sme_enable(struct boot_params *bp) { }
> +static inline void sme_enable(struct boot_params *bp) { }
>   
>   static inline void sev_es_init_vc_handling(void) { }
>   
> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> index deaaea3280d9..f37278d1cf85 100644
> --- a/arch/x86/kernel/head64.c
> +++ b/arch/x86/kernel/head64.c
> @@ -98,9 +98,6 @@ static unsigned long __head sme_postprocess_startup(struct boot_params *bp, pmdv
>   	unsigned long vaddr, vaddr_end;
>   	int i;
>   
> -	/* Encrypt the kernel and related (if SME is active) */
> -	sme_encrypt_kernel(bp);
> -
>   	/*
>   	 * Clear the memory encryption mask from the .bss..decrypted section.
>   	 * The bss section will be memset to zero later in the initialization so
> @@ -108,6 +105,9 @@ static unsigned long __head sme_postprocess_startup(struct boot_params *bp, pmdv
>   	 * attribute.
>   	 */

The comment above this if statement should now probably be moved into the 
if portion of the statement after the sme_encrypt_kernel() call, since now 
more than just bss_decrypted work is being done here.

>   	if (sme_get_me_mask()) {
> +		/* Encrypt the kernel and related */
> +		sme_encrypt_kernel(bp);
> +
>   		vaddr = (unsigned long)__start_bss_decrypted;
>   		vaddr_end = (unsigned long)__end_bss_decrypted;
>   
> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> index 0166ab1780cc..7ddcf960e92a 100644
> --- a/arch/x86/mm/mem_encrypt_identity.c
> +++ b/arch/x86/mm/mem_encrypt_identity.c
> @@ -45,6 +45,7 @@
>   #include <asm/sections.h>
>   #include <asm/cmdline.h>
>   #include <asm/coco.h>
> +#include <asm/init.h>
>   #include <asm/sev.h>
>   
>   #include "mm_internal.h"
> @@ -502,18 +503,15 @@ void __init sme_encrypt_kernel(struct boot_params *bp)
>   	native_write_cr3(__native_read_cr3());
>   }
>   
> -void __init sme_enable(struct boot_params *bp)
> +void __head sme_enable(struct boot_params *bp)
>   {
>   	const char *cmdline_ptr, *cmdline_arg, *cmdline_on;
>   	unsigned int eax, ebx, ecx, edx;
>   	unsigned long feature_mask;
>   	unsigned long me_mask;
>   	char buffer[16];
> -	bool snp;
>   	u64 msr;
>   
> -	snp = snp_init(bp);

The snp_init() call is here because the SNP CPUID table needs to be 
established before the below CPUID instruction is executed. This can't be 
moved.

Thanks,
Tom

> -
>   	/* Check for the SME/SEV support leaf */
>   	eax = 0x80000000;
>   	ecx = 0;
> @@ -546,7 +544,7 @@ void __init sme_enable(struct boot_params *bp)
>   	feature_mask = (msr & MSR_AMD64_SEV_ENABLED) ? AMD_SEV_BIT : AMD_SME_BIT;
>   
>   	/* The SEV-SNP CC blob should never be present unless SEV-SNP is enabled. */
> -	if (snp && !(msr & MSR_AMD64_SEV_SNP_ENABLED))
> +	if (snp_init(bp) && !(msr & MSR_AMD64_SEV_SNP_ENABLED))
>   		snp_abort();
>   
>   	/* Check if memory encryption is enabled */