| Message ID | 20250422100728.208479-7-ardb+git@google.com |
|---|---|
| Headers | show |
| Series | efi: Don't initalize SEV-SNP from the EFI stub | expand |
On Tue, 22 Apr 2025 at 18:40, Ard Biesheuvel <ardb@google.com> wrote: > > On Tue, Apr 22, 2025 at 5:51 PM Tom Lendacky <thomas.lendacky@amd.com> wrote: > > > > On 4/22/25 05:07, Ard Biesheuvel wrote: > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > > Hi Ard, > > > > I'll try to get to reviewing and testing this series very soon. > > Thanks. > > > But one > > thing I can see is that we never set the snp_vmpl level anymore in the > > EFI stub and so PVALIDATE will fail when running under an SVSM. > > > > But I don't think this series is completely at fault. This goes back to > > fixing memory acceptance before sev_enable() was called and I missed the > > SVSM situation. So I don't think we can completely remove all SNP > > initialization and might have to do svsm_setup_ca() which has a pre-req > > on setup_cpuid_table()... sigh. > > Why is that, though? The EFI stub never replaces the #VC and #PF handlers, and so cpuid instructions will be handled as before, right? And the SVSM setup code will run again when the core kernel boots and this time, it will need to update the cpuid tables to record the SVSM presence.
On 4/24/25 02:22, Ard Biesheuvel wrote: > On Tue, 22 Apr 2025 at 18:40, Ard Biesheuvel <ardb@google.com> wrote: >> >> On Tue, Apr 22, 2025 at 5:51 PM Tom Lendacky <thomas.lendacky@amd.com> wrote: >>> >>> On 4/22/25 05:07, Ard Biesheuvel wrote: >>>> From: Ard Biesheuvel <ardb@kernel.org> >>>> >>> >>> Hi Ard, >>> >>> I'll try to get to reviewing and testing this series very soon. >> >> Thanks. >> >>> But one >>> thing I can see is that we never set the snp_vmpl level anymore in the >>> EFI stub and so PVALIDATE will fail when running under an SVSM. >>> >>> But I don't think this series is completely at fault. This goes back to >>> fixing memory acceptance before sev_enable() was called and I missed the >>> SVSM situation. So I don't think we can completely remove all SNP >>> initialization and might have to do svsm_setup_ca() which has a pre-req >>> on setup_cpuid_table()... sigh. >>> > > Why is that, though? The EFI stub never replaces the #VC and #PF > handlers, and so cpuid instructions will be handled as before, right? > And the SVSM setup code will run again when the core kernel boots and > this time, it will need to update the cpuid tables to record the SVSM > presence. It's more of a statement about the CPUID table modifications made by svsm_setup_ca() that need to be skipped if setup_cpuid_table() isn't called, not the use of CPUID itself. But taking a closer look, snp_cpuid_get_table() is actually returning the address of cpuid_table_copy, which is a static in the file. So maybe it isn't an issue because the loop at the end of svsm_setup_ca() will not crash, which was the main concern. I think we can use CPUID 0x8000001f_EAX[28] to detect an SVSM and read MSR 0xc001f000 to get the CAA. OVMF has that support, just would need to figure out where to check for it, then we can probably skip the svsm_setup_ca() and do everything in the snp_accept_memory() path. Let me take a look... Thanks, Tom
On 4/24/25 09:18, Tom Lendacky wrote: > On 4/24/25 02:22, Ard Biesheuvel wrote: >> On Tue, 22 Apr 2025 at 18:40, Ard Biesheuvel <ardb@google.com> wrote: >>> >>> On Tue, Apr 22, 2025 at 5:51 PM Tom Lendacky <thomas.lendacky@amd.com> wrote: >>>> >>>> On 4/22/25 05:07, Ard Biesheuvel wrote: >>>>> From: Ard Biesheuvel <ardb@kernel.org> >>>>> >>>> >>>> Hi Ard, >>>> >>>> I'll try to get to reviewing and testing this series very soon. >>> >>> Thanks. >>> >>>> But one >>>> thing I can see is that we never set the snp_vmpl level anymore in the >>>> EFI stub and so PVALIDATE will fail when running under an SVSM. >>>> >>>> But I don't think this series is completely at fault. This goes back to >>>> fixing memory acceptance before sev_enable() was called and I missed the >>>> SVSM situation. So I don't think we can completely remove all SNP >>>> initialization and might have to do svsm_setup_ca() which has a pre-req >>>> on setup_cpuid_table()... sigh. >>>> >> >> Why is that, though? The EFI stub never replaces the #VC and #PF >> handlers, and so cpuid instructions will be handled as before, right? >> And the SVSM setup code will run again when the core kernel boots and >> this time, it will need to update the cpuid tables to record the SVSM >> presence. > > It's more of a statement about the CPUID table modifications made by > svsm_setup_ca() that need to be skipped if setup_cpuid_table() isn't > called, not the use of CPUID itself. > > But taking a closer look, snp_cpuid_get_table() is actually returning > the address of cpuid_table_copy, which is a static in the file. So maybe > it isn't an issue because the loop at the end of svsm_setup_ca() will > not crash, which was the main concern. > > I think we can use CPUID 0x8000001f_EAX[28] to detect an SVSM and read > MSR 0xc001f000 to get the CAA. OVMF has that support, just would need to > figure out where to check for it, then we can probably skip the > svsm_setup_ca() and do everything in the snp_accept_memory() path. > > Let me take a look... Initial look at something like this works (along with the fix for the mistake I made in OVMF). I need to test the kexec path to be certain, though. diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c index 478c65149cf0..d2f9cbbe943b 100644 --- a/arch/x86/boot/compressed/sev.c +++ b/arch/x86/boot/compressed/sev.c @@ -142,6 +142,7 @@ u64 svsm_get_caa_pa(void) int svsm_perform_call_protocol(struct svsm_call *call); u8 snp_vmpl; +bool snp_vmpl_checked; /* Include code for early handlers */ #include "../../boot/startup/sev-shared.c" @@ -241,6 +242,29 @@ static bool early_setup_ghcb(void) void snp_accept_memory(phys_addr_t start, phys_addr_t end) { + if (!snp_vmpl_checked) { + unsigned int eax, ebx, ecx, edx; + + /* + * CPUID Fn8000_001F_EAX[28] - SVSM support + */ + eax = 0x8000001f; + ecx = 0; + native_cpuid(&eax, &ebx, &ecx, &edx); + if (eax & BIT(28)) { + struct msr m; + + /* Obtain the address of the calling area to use */ + boot_rdmsr(MSR_SVSM_CAA, &m); + boot_svsm_caa = (void *)m.q; + boot_svsm_caa_pa = m.q; + + snp_vmpl = 2; + } + + snp_vmpl_checked = true; + } + for (phys_addr_t pa = start; pa < end; pa += PAGE_SIZE) __page_state_change(pa, SNP_PAGE_STATE_PRIVATE); } diff --git a/arch/x86/boot/startup/sev-shared.c b/arch/x86/boot/startup/sev-shared.c index 173f3d1f777a..5cca01700280 100644 --- a/arch/x86/boot/startup/sev-shared.c +++ b/arch/x86/boot/startup/sev-shared.c @@ -1342,6 +1342,8 @@ static bool __head svsm_setup_ca(const struct cc_blob_sev_info *cc_info) BUILD_BUG_ON(sizeof(*secrets_page) != PAGE_SIZE); + snp_vmpl_checked = true; + /* * Check if running at VMPL0. * diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c index 5b145446e991..5011b3a93a21 100644 --- a/arch/x86/coco/sev/core.c +++ b/arch/x86/coco/sev/core.c @@ -99,6 +99,7 @@ DEFINE_PER_CPU(struct sev_es_save_area *, sev_vmsa); */ u8 snp_vmpl __ro_after_init; EXPORT_SYMBOL_GPL(snp_vmpl); +bool snp_vmpl_checked __ro_after_init; static u64 __init get_snp_jump_table_addr(void) { > > Thanks, > Tom
On 4/25/25 13:18, Tom Lendacky wrote: > On 4/24/25 09:18, Tom Lendacky wrote: >> On 4/24/25 02:22, Ard Biesheuvel wrote: >>> On Tue, 22 Apr 2025 at 18:40, Ard Biesheuvel <ardb@google.com> wrote: >> >> Let me take a look... > > Initial look at something like this works (along with the fix for the > mistake I made in OVMF). I need to test the kexec path to be certain, > though. Ah, this version doesn't have the part in arch/x86/include/asm/sev.h that declares snp_vmpl_checked as an extern. But other than that... Thanks, Tom > > diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c > index 478c65149cf0..d2f9cbbe943b 100644 > --- a/arch/x86/boot/compressed/sev.c > +++ b/arch/x86/boot/compressed/sev.c > @@ -142,6 +142,7 @@ u64 svsm_get_caa_pa(void) > int svsm_perform_call_protocol(struct svsm_call *call); > > u8 snp_vmpl; > +bool snp_vmpl_checked; > > /* Include code for early handlers */ > #include "../../boot/startup/sev-shared.c" > @@ -241,6 +242,29 @@ static bool early_setup_ghcb(void) > > void snp_accept_memory(phys_addr_t start, phys_addr_t end) > { > + if (!snp_vmpl_checked) { > + unsigned int eax, ebx, ecx, edx; > + > + /* > + * CPUID Fn8000_001F_EAX[28] - SVSM support > + */ > + eax = 0x8000001f; > + ecx = 0; > + native_cpuid(&eax, &ebx, &ecx, &edx); > + if (eax & BIT(28)) { > + struct msr m; > + > + /* Obtain the address of the calling area to use */ > + boot_rdmsr(MSR_SVSM_CAA, &m); > + boot_svsm_caa = (void *)m.q; > + boot_svsm_caa_pa = m.q; > + > + snp_vmpl = 2; > + } > + > + snp_vmpl_checked = true; > + } > + > for (phys_addr_t pa = start; pa < end; pa += PAGE_SIZE) > __page_state_change(pa, SNP_PAGE_STATE_PRIVATE); > } > diff --git a/arch/x86/boot/startup/sev-shared.c b/arch/x86/boot/startup/sev-shared.c > index 173f3d1f777a..5cca01700280 100644 > --- a/arch/x86/boot/startup/sev-shared.c > +++ b/arch/x86/boot/startup/sev-shared.c > @@ -1342,6 +1342,8 @@ static bool __head svsm_setup_ca(const struct cc_blob_sev_info *cc_info) > > BUILD_BUG_ON(sizeof(*secrets_page) != PAGE_SIZE); > > + snp_vmpl_checked = true; > + > /* > * Check if running at VMPL0. > * > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c > index 5b145446e991..5011b3a93a21 100644 > --- a/arch/x86/coco/sev/core.c > +++ b/arch/x86/coco/sev/core.c > @@ -99,6 +99,7 @@ DEFINE_PER_CPU(struct sev_es_save_area *, sev_vmsa); > */ > u8 snp_vmpl __ro_after_init; > EXPORT_SYMBOL_GPL(snp_vmpl); > +bool snp_vmpl_checked __ro_after_init; > > static u64 __init get_snp_jump_table_addr(void) > { > >> >> Thanks, >> Tom
From: Ard Biesheuvel <ardb@kernel.org> Changes since v2: [1] - rebase onto tip/x86/boot - add patch to remove unused static inline fallback implementation of sev_enable() Changes since v1: [0] - address shortcomings pointed out by Tom, related to missing checks and to discovery of the CC blob table from the EFI stub [0] https://lore.kernel.org/all/20250414130417.1486395-2-ardb+git@google.com/T/#u [1] https://lore.kernel.org/all/20250416165743.4080995-6-ardb+git@google.com/T/#u Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Dionna Amalie Glaze <dionnaglaze@google.com> Cc: Kevin Loughlin <kevinloughlin@google.com> Ard Biesheuvel (5): x86/boot: Drop unused sev_enable() fallback x86/efistub: Obtain SEV CC blob address from the stub x86/boot: Drop redundant RMPADJUST in SEV SVSM presence check x86/sev: Unify SEV-SNP hypervisor feature check x86/efistub: Don't bother enabling SEV in the EFI stub arch/x86/boot/compressed/misc.h | 11 ------- arch/x86/boot/compressed/sev.c | 33 +------------------- arch/x86/boot/startup/sev-shared.c | 33 +++++++++++++++----- arch/x86/boot/startup/sme.c | 2 ++ arch/x86/coco/sev/core.c | 11 ------- arch/x86/include/asm/sev-internal.h | 3 +- arch/x86/include/asm/sev.h | 4 +-- drivers/firmware/efi/libstub/x86-stub.c | 27 +++++++++------- 8 files changed, 48 insertions(+), 76 deletions(-) base-commit: ff4c0560ab020d34baf0aa6434f66333d25ae524