Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230602101313.3557775-1-ardb@kernel.org> <20230602101313.3557775-21-ardb@kernel.org>
 <bfe5bbac-37e2-6728-606a-c652bafad6b6@amd.com> <849a65c8-a320-a8a8-8784-0ee3737eee9e@amd.com>
In-Reply-To: <849a65c8-a320-a8a8-8784-0ee3737eee9e@amd.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Fri, 2 Jun 2023 23:29:57 +0200
Message-ID: <CAMj1kXGwKfbX+tvch0OiyN69jhOOi_zcUdkzXf2ipQ4Gd++3FQ@mail.gmail.com>
Subject: Re: [PATCH v4 20/21] x86/efistub: Perform SNP feature test while
 running in the firmware
To:     Tom Lendacky <thomas.lendacky@amd.com>
Cc:     linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org,
        Evgeniy Baskov <baskov@ispras.ru>,
        Borislav Petkov <bp@alien8.de>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Alexey Khoroshilov <khoroshilov@ispras.ru>,
        Peter Jones <pjones@redhat.com>,
        Gerd Hoffmann <kraxel@redhat.com>,
        Dave Young <dyoung@redhat.com>,
        Mario Limonciello <mario.limonciello@amd.com>,
        Kees Cook <keescook@chromium.org>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Joerg Roedel <jroedel@suse.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 2 Jun 2023 at 22:39, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> 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@kernel.org>
> >> ---
> >>   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.
>

Could we just move the CPUID call to the start of the function?