by Ard Biesheuvel

[permalink] [raw]

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

On Mon, 26 Feb 2024 at 22:38, Tom Lendacky <[email protected]> wrote:
>
> On 2/26/24 08:29, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <[email protected]>
> >
> > 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.

2024-02-27 15:45:36

by Tom Lendacky

[permalink] [raw]

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

On 2/27/24 08:55, Ard Biesheuvel wrote:
> On Mon, 26 Feb 2024 at 22:38, Tom Lendacky <[email protected]> wrote:
>>
>> On 2/26/24 08:29, Ard Biesheuvel wrote:
>>> From: Ard Biesheuvel <[email protected]>
>>>

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