From: Tom Lendacky <thomas.lendacky@amd.com>
To: Ard Biesheuvel <ardb@kernel.org>
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>
Subject: Re: [PATCH v5 19/20] x86/efistub: Perform SNP feature test while running in the firmware
Date: Wed, 7 Jun 2023 12:29:37 -0500 [thread overview]
Message-ID: <4610db60-6fa0-a0ae-8c74-23f14236cff7@amd.com> (raw)
In-Reply-To: <CAMj1kXHpxqvbo_NcgzAAHsE71GohoCcttatMXZsjVvXqPBO33w@mail.gmail.com>
On 6/7/23 11:51, Ard Biesheuvel wrote:
> On Wed, 7 Jun 2023 at 18:08, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 6/7/23 02:23, 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 | 71 +++++++++++---------
>>> arch/x86/include/asm/sev.h | 4 ++
>>> drivers/firmware/efi/libstub/x86-stub.c | 17 +++++
>>> 3 files changed, 62 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
>>> index 09dc8c187b3cc752..9593bc80c9c6b89d 100644
>>> --- a/arch/x86/boot/compressed/sev.c
>>> +++ b/arch/x86/boot/compressed/sev.c
>>
>> ...
>>
>>> -void sev_enable(struct boot_params *bp)
>>> +u64 sev_get_status(void)
>>> {
>>> unsigned int eax, ebx, ecx, edx;
>>> struct msr m;
>>> +
>>> + /* Check for the SME/SEV support leaf */
>>> + eax = 0x80000000;
>>> + ecx = 0;
>>> + native_cpuid(&eax, &ebx, &ecx, &edx);
>>> + if (eax < 0x8000001f)
>>> + return 0;
>>> +
>>> + /*
>>> + * 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)))
>>> + return 0;
>>> +
>>> + /* Set the SME mask if this is an SEV guest. */
>>> + sme_me_mask = BIT_ULL(ebx & 0x3f);
>>> +
>>> + boot_rdmsr(MSR_AMD64_SEV, &m);
>>> + return m.q;
>>> +}
>>> +
>>> +void sev_enable(struct boot_params *bp)
>>> +{
>>> bool snp;
>>>
>>> /*
>>> @@ -410,37 +447,13 @@ 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))) {
>>> + 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.");
>>
>> This ends up checking the CPUID path because if SEV isn't advertised in
>> CPUID the returned status value is 0. But it also checks the SEV_STATUS
>> MSR as well. So I think you can remove the SNP / SEV_STATUS check at the
>> end of this function (since that check is identical to this now) and just
>> update the message to indicate not CPUID or SEV status MSR.
>>
>
> But that one checks for MSR_AMD64_SEV_SNP_ENABLED not
> MSR_AMD64_SEV_ENABLED. Does that matter at all?
Ugh, my bad, I misread that last check. Ignore my comment.
>
>> The sme_me_mask should probably be cleared at this point before returning,
>> too. Or, alternately, in sev_get_status(), you can update the setting of
>> sme_me_mask to based on MSR_AMD64_SEV_ENABLED being set in the SEV_STATUS MSR.
>>
>
> I'll go for the latter, seems cleaner not to touch it in that case.
Sounds good.
Thanks
Tom
>
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -422,10 +422,12 @@ u64 sev_get_status(void)
> if (!(eax & BIT(1)))
> return 0;
>
> - /* Set the SME mask if this is an SEV guest. */
> - sme_me_mask = BIT_ULL(ebx & 0x3f);
> -
> boot_rdmsr(MSR_AMD64_SEV, &m);
> +
> + /* Set the SME mask if this is an SEV guest. */
> + if (m.q & MSR_AMD64_SEV_ENABLED)
> + sme_me_mask = BIT_ULL(ebx & 0x3f);
> +
> return m.q;
> }
>
>
>>> /* Search for Confidential Computing blob in the EFI config table. */
>>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>>> index 86e1296e87f513b7..081c39b0e8d0d208 100644
>>> --- a/arch/x86/include/asm/sev.h
>>> +++ b/arch/x86/include/asm/sev.h
>>> @@ -207,6 +207,8 @@ bool snp_init(struct boot_params *bp);
>>> void __init __noreturn snp_abort(void);
>>> int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio);
>>> void snp_accept_memory(phys_addr_t start, phys_addr_t end);
>>> +u64 snp_get_unsupported_features(u64 status);
>>> +u64 sev_get_status(void);
>>> #else
>>> static inline void sev_es_ist_enter(struct pt_regs *regs) { }
>>> static inline void sev_es_ist_exit(void) { }
>>> @@ -232,6 +234,8 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
>>> }
>>>
>>> static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { }
>>> +static inline u64 snp_get_unsupported_features(u64 status) { return 0; }
>>> +static inline u64 sev_get_status(void) { return 0; }
>>> #endif
>>>
>>> #endif
>>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
>>> index abcd5703e9f3f980..1015ef883f5850a4 100644
>>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>>> @@ -15,6 +15,7 @@
>>> #include <asm/setup.h>
>>> #include <asm/desc.h>
>>> #include <asm/boot.h>
>>> +#include <asm/sev.h>
>>>
>>> #include "efistub.h"
>>> #include "x86-stub.h"
>>> @@ -790,6 +791,19 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
>>> return EFI_SUCCESS;
>>> }
>>>
>>> +static bool have_unsupported_snp_features(void)
>>> +{
>>> + u64 unsupported;
>>> +
>>> + unsupported = snp_get_unsupported_features(sev_get_status());
>>
>> This will also set sme_me_mask, but I think that is ok, since on error
>> things will terminate, otherwise sev_enable() should update appropriately
>> later.
>>
>
> OK
>
>>> + if (unsupported) {
>>> + efi_err("Unsupported SEV-SNP features detected: 0x%llx\n",
>>> + unsupported);
>>> + return true;
>>> + }
>>> + return false;
>>> +}
>>> +
>>> static void __noreturn enter_kernel(unsigned long kernel_addr,
>>> struct boot_params *boot_params)
>>> {
>>> @@ -820,6 +834,9 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>>> if (efi_system_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
>>> efi_exit(handle, EFI_INVALID_PARAMETER);
>>>
>>> + if (have_unsupported_snp_features())
>>> + efi_exit(handle, EFI_UNSUPPORTED);
>>> +
>>> if (IS_ENABLED(CONFIG_EFI_DXE_MEM_ATTRIBUTES)) {
>>> efi_dxe_table = get_efi_config_table(EFI_DXE_SERVICES_TABLE_GUID);
>>> if (efi_dxe_table &&
next prev parent reply other threads:[~2023-06-07 17:30 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-07 7:23 [PATCH v5 00/20] efi/x86: Avoid bare metal decompressor during EFI boot Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 01/20] x86/efistub: Branch straight to kernel entry point from C code Ard Biesheuvel
2023-06-07 18:53 ` Borislav Petkov
2023-06-07 19:39 ` Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 02/20] x86/efistub: Simplify and clean up handover entry code Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 03/20] x86/decompressor: Avoid magic offsets for EFI handover entrypoint Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 04/20] x86/efistub: Clear BSS in EFI handover protocol entrypoint Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 05/20] x86/decompressor: Use proper sequence to take the address of the GOT Ard Biesheuvel
2023-06-21 11:08 ` Borislav Petkov
2023-06-23 14:00 ` Ard Biesheuvel
2023-07-07 13:56 ` Borislav Petkov
2023-06-07 7:23 ` [PATCH v5 06/20] x86/decompressor: Store boot_params pointer in callee save register Ard Biesheuvel
2023-07-10 9:06 ` Borislav Petkov
2023-07-10 21:55 ` Ard Biesheuvel
2023-07-11 7:57 ` Borislav Petkov
2023-06-07 7:23 ` [PATCH v5 07/20] x86/decompressor: Call trampoline as a normal function Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 08/20] x86/decompressor: Use standard calling convention for trampoline Ard Biesheuvel
2023-06-07 19:38 ` Yunhong Jiang
2023-06-07 20:07 ` Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 09/20] x86/decompressor: Avoid the need for a stack in the 32-bit trampoline Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 10/20] x86/decompressor: Call trampoline directly from C code Ard Biesheuvel
2023-06-07 18:09 ` Yunhong Jiang
2023-06-08 8:04 ` Ard Biesheuvel
2023-06-08 18:15 ` Yunhong Jiang
2023-06-07 7:23 ` [PATCH v5 11/20] x86/decompressor: Only call the trampoline when changing paging levels Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 12/20] x86/decompressor: Merge trampoline cleanup with switching code Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 13/20] x86/efistub: Perform 4/5 level paging switch from the stub Ard Biesheuvel
2023-06-07 20:19 ` Yunhong Jiang
2023-06-07 20:31 ` Ard Biesheuvel
2023-06-08 0:43 ` Yunhong Jiang
2023-06-08 6:34 ` Ard Biesheuvel
2023-06-08 16:10 ` Yunhong Jiang
2023-06-07 7:23 ` [PATCH v5 14/20] x86/efistub: Prefer EFI memory attributes protocol over DXE services Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 15/20] decompress: Use 8 byte alignment Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 16/20] x86/decompressor: Move global symbol references to C code Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 17/20] x86/decompressor: Factor out kernel decompression and relocation Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 18/20] efi/libstub: Add limit argument to efi_random_alloc() Ard Biesheuvel
2023-06-07 7:23 ` [PATCH v5 19/20] x86/efistub: Perform SNP feature test while running in the firmware Ard Biesheuvel
2023-06-07 16:07 ` Tom Lendacky
2023-06-07 16:51 ` Ard Biesheuvel
2023-06-07 17:29 ` Tom Lendacky [this message]
2023-06-07 7:23 ` [PATCH v5 20/20] x86/efistub: Avoid legacy decompressor when doing EFI boot Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4610db60-6fa0-a0ae-8c74-23f14236cff7@amd.com \
--to=thomas.lendacky@amd.com \
--cc=ardb@kernel.org \
--cc=baskov@ispras.ru \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=dyoung@redhat.com \
--cc=jroedel@suse.de \
--cc=keescook@chromium.org \
--cc=khoroshilov@ispras.ru \
--cc=kirill.shutemov@linux.intel.com \
--cc=kraxel@redhat.com \
--cc=linux-efi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mario.limonciello@amd.com \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=pjones@redhat.com \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).