Re: [PATCH v4 20/21] x86/efistub: Perform SNP feature test while running in the firmware

[Tom Lendacky]

On 6/2/23 15:38, Tom Lendacky wrote:
> On 6/2/23 05:13, Ard Biesheuvel wrote:
> > Before refactoring the EFI stub boot flow to avoid the legacy bare metal
> > decompressor, duplicate the SNP feature check in the EFI stub before
> > handing over to the kernel proper.
> >
> > The SNP feature check can be performed while running under the EFI boot
> > services, which means we can fail gracefully and return an error to the
> > bootloader if the loaded kernel does not implement support for all the
> > features that the hypervisor enabled.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > ---
> >   arch/x86/boot/compressed/sev.c          | 74 ++++++++++++--------
> >   arch/x86/include/asm/sev.h              |  4 ++
> >   drivers/firmware/efi/libstub/x86-stub.c | 17 +++++
> >   3 files changed, 67 insertions(+), 28 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/sev.c 
> > b/arch/x86/boot/compressed/sev.c
> > index 014b89c890887b9a..be021e24f1ece421 100644
> > --- a/arch/x86/boot/compressed/sev.c
> > +++ b/arch/x86/boot/compressed/sev.c
>
>
> > +void sev_enable(struct boot_params *bp)
> > +{
> > +    unsigned int eax, ebx, ecx, edx;
> >       bool snp;
> >       /*
> > @@ -358,37 +391,14 @@ void sev_enable(struct boot_params *bp)
> >        */
> >       snp = snp_init(bp);
> > -    /* Check for the SME/SEV support leaf */
> > -    eax = 0x80000000;
> > -    ecx = 0;
> > -    native_cpuid(&eax, &ebx, &ecx, &edx);
> > -    if (eax < 0x8000001f)
> > -        return;
> > -
> > -    /*
> > -     * Check for the SME/SEV feature:
> > -     *   CPUID Fn8000_001F[EAX]
> > -     *   - Bit 0 - Secure Memory Encryption support
> > -     *   - Bit 1 - Secure Encrypted Virtualization support
> > -     *   CPUID Fn8000_001F[EBX]
> > -     *   - Bits 5:0 - Pagetable bit position used to indicate encryption
> > -     */
> > -    eax = 0x8000001f;
> > -    ecx = 0;
> > -    native_cpuid(&eax, &ebx, &ecx, &edx);
> > -    /* Check whether SEV is supported */
> > -    if (!(eax & BIT(1))) {
> > +    /* Set the SME mask if this is an SEV guest. */
> > +    sev_status = sev_get_status();
> > +    if (!(sev_status & MSR_AMD64_SEV_ENABLED)) {
> >           if (snp)
> >               error("SEV-SNP support indicated by CC blob, but not 
> > CPUID.");
> >           return;
> >       }
> > -    /* Set the SME mask if this is an SEV guest. */
> > -    boot_rdmsr(MSR_AMD64_SEV, &m);
> > -    sev_status = m.q;
> > -    if (!(sev_status & MSR_AMD64_SEV_ENABLED))
> > -        return;
> > -
> >       /* Negotiate the GHCB protocol version. */
> >       if (sev_status & MSR_AMD64_SEV_ES_ENABLED) {
> >           if (!sev_es_negotiate_protocol())
> > @@ -409,6 +419,14 @@ void sev_enable(struct boot_params *bp)
> >       if (snp && !(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
> >           error("SEV-SNP supported indicated by CC blob, but not SEV 
> > status MSR.");
> > +    /*
> > +     * Check for the SME/SEV feature:
> > +     *   CPUID Fn8000_001F[EBX]
> > +     *   - Bits 5:0 - Pagetable bit position used to indicate encryption
> > +     */
> > +    eax = 0x8000001f;
> > +    ecx = 0;
> > +    native_cpuid(&eax, &ebx, &ecx, &edx);
>
> This causes SEV-ES / SEV-SNP to crash.
>
> This goes back to a previous comment where calling either 
> sev_es_negotiate_protocol() or get_hv_features() blows away the GHCB value 
> in the GHCB MSR and as soon as the CPUID instruction is executed the boot 
> blows up.
>
> Even if we move this up to be done earlier, we can complete this function 
> successfully but then blow up further on.
>
> So you probably have to modify the routines in question to save and 
> restore the GHCB MSR value.

I should clarify that it doesn't in fact cause a problem until the final 
patch is applied and this path is taken.

Thanks,
Tom

> Thanks,
> Tom
>
> >       sme_me_mask = BIT_ULL(ebx & 0x3f);
> >   }