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

Tom Lendacky
2024-02-26T21:38:01Z

On 2/26/24 08:29, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
>
> 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@kernel.org>
> ---
> 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 */