Messages in this thread | | | Date | Mon, 26 Feb 2024 15:37:54 -0600 | Subject | Re: [PATCH v6 05/10] x86/sme: Avoid SME/SVE related checks on non-SME/SVE platforms | From | Tom Lendacky <> |
| |
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 */
| |