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

  From: Ard Biesheuvel <ardb@kernel.org>

Reorganize the early SME/SVE init code so that SME/SVE related calls are
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)

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(-)
  

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 */
  

On Mon, 26 Feb 2024 at 22:38, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> 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.
>

Yes.

> > 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).
>

The idea of this change is to avoid calls into SME/SEV related code,
in a way that can easily be backported to -stable kernels.

The reason is that the SME/SEV code has changed much more than the
rest of the code, and so backporting this entire series is going to be
messy, as well as unnecessary given that those kernels are not
expected to get all the latest SEV related changes anyway. We do tend
to try and keep those -stable kernels building with recent Clang
versions, and so keeping the SME/SEV code out of the boot path
entirely would help running into codegen issues such as the ones this
series is working around in code that is substantially different from
the latest revision.

However, I decided to drop this patch anyway. The mainline code should
be obviously correct, and introducing potential problems this way does
not justify the goal of easier -stable backports.

..
> > 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.
>

Yeah, good point. I didn't spot this in my SEV-SNP boot testing,
presumably because the firmware's VC handler is still active in my
case, but this isn't guaranteed, e.g., when booting via the
decompressor, or when using 5-level paging.

Thanks for the review.
  

On 2/27/24 08:55, Ard Biesheuvel wrote:
> On Mon, 26 Feb 2024 at 22:38, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 2/26/24 08:29, Ard Biesheuvel wrote:
>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>

>>> -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.
>>
> 
> Yeah, good point. I didn't spot this in my SEV-SNP boot testing,
> presumably because the firmware's VC handler is still active in my
> case, but this isn't guaranteed, e.g., when booting via the
> decompressor, or when using 5-level paging.

Actually, the kernel's do_vc_no_ghcb() is the handler that gets invoked. 
Since the CPUID table is not initialized, -EOPNOTSUPP is returned from 
snp_cpuid(), which results in the handler using the MSR protocol to get 
the CPUID information from the hypervisor - which we don't want for SNP in 
this situation.

Thanks,
Tom


> 
> Thanks for the review.