[PATCH v3 0/5] efi: Don't initalize SEV-SNP from the EFI stub

[PATCH v3 0/5] efi: Don't initalize SEV-SNP from the EFI stub

[Ard Biesheuvel]

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
-- 
2.49.0.805.g082f7c87e0-goog

[PATCH v3 1/5] x86/boot: Drop unused sev_enable() fallback

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

The misc.h header is not included by the EFI stub, which is the only C
caller of sev_enable(). This means the fallback for cases where
CONFIG_AMD_MEM_ENCRYPT is not set is never used, so it can be dropped.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/misc.h | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index dd8d1a85f671..1e950bc5b085 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -144,17 +144,6 @@ void snp_set_page_private(unsigned long paddr);
 void snp_set_page_shared(unsigned long paddr);
 void sev_prep_identity_maps(unsigned long top_level_pgt);
 #else
-static inline void sev_enable(struct boot_params *bp)
-{
-	/*
-	 * bp->cc_blob_address should only be set by boot/compressed kernel.
-	 * Initialize it to 0 unconditionally (thus here in this stub too) to
-	 * ensure that uninitialized values from buggy bootloaders aren't
-	 * propagated.
-	 */
-	if (bp)
-		bp->cc_blob_address = 0;
-}
 static inline void snp_check_features(void) { }
 static inline void sev_es_shutdown_ghcb(void) { }
 static inline bool sev_es_check_ghcb_fault(unsigned long address)
-- 
2.49.0.805.g082f7c87e0-goog

[PATCH v3 2/5] x86/efistub: Obtain SEV CC blob address from the stub

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

The x86 EFI stub no longer boots the core kernel via the traditional
decompressor but jumps straight to it, avoiding all the page fault
handling and other complexity that is entirely unnecessary when booting
via EFI, which guarantees that all system memory is mapped 1:1.

The SEV startup code in the core kernel expects the address of the CC
blob configuration table in boot_params, so store it there when booting
from EFI with SEV-SNP enabled. This removes the need to call
sev_enable() from the EFI stub.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/x86-stub.c | 21 +++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index cafc90d4caaf..d9ae1a230d39 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -681,17 +681,28 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
 	return EFI_SUCCESS;
 }
 
-static bool have_unsupported_snp_features(void)
+static bool check_snp_features(struct boot_params *bp)
 {
+	u64 status = sev_get_status();
 	u64 unsupported;
 
-	unsupported = snp_get_unsupported_features(sev_get_status());
+	unsupported = snp_get_unsupported_features(status);
 	if (unsupported) {
 		efi_err("Unsupported SEV-SNP features detected: 0x%llx\n",
 			unsupported);
-		return true;
+		return false;
 	}
-	return false;
+
+	if (status & MSR_AMD64_SEV_SNP_ENABLED) {
+		void *tbl = get_efi_config_table(EFI_CC_BLOB_GUID);
+
+		if (!tbl) {
+			efi_err("SEV-SNP is enabled but CC blob not found\n");
+			return false;
+		}
+		bp->cc_blob_address = (u32)(unsigned long)tbl;
+	}
+	return true;
 }
 
 static void efi_get_seed(void *seed, int size)
@@ -829,7 +840,7 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 
 	hdr = &boot_params->hdr;
 
-	if (have_unsupported_snp_features())
+	if (!check_snp_features(boot_params))
 		efi_exit(handle, EFI_UNSUPPORTED);
 
 	if (IS_ENABLED(CONFIG_EFI_DXE_MEM_ATTRIBUTES)) {
-- 
2.49.0.805.g082f7c87e0-goog

[PATCH v3 3/5] x86/boot: Drop redundant RMPADJUST in SEV SVSM presence check

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

snp_vmpl will be assigned a non-zero value when executing at a VMPL
other than 0, and this is inferred from a call to RMPADJUST, which only
works when running at VMPL0.

This means that testing snp_vmpl is sufficient, and there is no need to
perform the same check again.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/sev.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 478c65149cf0..91a2836b20a1 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -582,30 +582,16 @@ void sev_enable(struct boot_params *bp)
 	 */
 	if (sev_status & MSR_AMD64_SEV_SNP_ENABLED) {
 		u64 hv_features;
-		int ret;
 
 		hv_features = get_hv_features();
 		if (!(hv_features & GHCB_HV_FT_SNP))
 			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
 
 		/*
-		 * Enforce running at VMPL0 or with an SVSM.
-		 *
-		 * Use RMPADJUST (see the rmpadjust() function for a description of
-		 * what the instruction does) to update the VMPL1 permissions of a
-		 * page. If the guest is running at VMPL0, this will succeed. If the
-		 * guest is running at any other VMPL, this will fail. Linux SNP guests
-		 * only ever run at a single VMPL level so permission mask changes of a
-		 * lesser-privileged VMPL are a don't-care.
+		 * Running at VMPL0 is required unless an SVSM is present and
+		 * the hypervisor supports the required SVSM GHCB events.
 		 */
-		ret = rmpadjust((unsigned long)&boot_ghcb_page, RMP_PG_SIZE_4K, 1);
-
-		/*
-		 * Running at VMPL0 is not required if an SVSM is present and the hypervisor
-		 * supports the required SVSM GHCB events.
-		 */
-		if (ret &&
-		    !(snp_vmpl && (hv_features & GHCB_HV_FT_SNP_MULTI_VMPL)))
+		if (snp_vmpl > 0 && !(hv_features & GHCB_HV_FT_SNP_MULTI_VMPL))
 			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
 	}
 
-- 
2.49.0.805.g082f7c87e0-goog

[PATCH v3 4/5] x86/sev: Unify SEV-SNP hypervisor feature check

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

The decompressor and the core kernel both check the hypervisor feature
mask exposed by the hypervisor, but test it in slightly different ways.

This disparity seems unintentional, and simply a result of the fact that
the decompressor and the core kernel evolve differently over time when
it comes to setting up the SEV-SNP execution context.

So move the HV feature check into a helper function and call that
instead. For the core kernel, move the check to an earlier boot stage,
right after the point where it is established that the guest is
executing in SEV-SNP mode.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/sev.c      | 19 +----------
 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          |  2 ++
 6 files changed, 32 insertions(+), 38 deletions(-)

diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 91a2836b20a1..5477f8bf9c96 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -576,24 +576,7 @@ void sev_enable(struct boot_params *bp)
 			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_PROT_UNSUPPORTED);
 	}
 
-	/*
-	 * SNP is supported in v2 of the GHCB spec which mandates support for HV
-	 * features.
-	 */
-	if (sev_status & MSR_AMD64_SEV_SNP_ENABLED) {
-		u64 hv_features;
-
-		hv_features = get_hv_features();
-		if (!(hv_features & GHCB_HV_FT_SNP))
-			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
-
-		/*
-		 * Running at VMPL0 is required unless an SVSM is present and
-		 * the hypervisor supports the required SVSM GHCB events.
-		 */
-		if (snp_vmpl > 0 && !(hv_features & GHCB_HV_FT_SNP_MULTI_VMPL))
-			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
-	}
+	snp_check_hv_features();
 
 	if (snp && !(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
 		error("SEV-SNP supported indicated by CC blob, but not SEV status MSR.");
diff --git a/arch/x86/boot/startup/sev-shared.c b/arch/x86/boot/startup/sev-shared.c
index 173f3d1f777a..7151cdd37557 100644
--- a/arch/x86/boot/startup/sev-shared.c
+++ b/arch/x86/boot/startup/sev-shared.c
@@ -94,16 +94,10 @@ sev_es_terminate(unsigned int set, unsigned int reason)
 		asm volatile("hlt\n" : : : "memory");
 }
 
-/*
- * The hypervisor features are available from GHCB version 2 onward.
- */
-u64 get_hv_features(void)
+static u64 __head get_hv_features(void)
 {
 	u64 val;
 
-	if (ghcb_version < 2)
-		return 0;
-
 	sev_es_wr_ghcb_msr(GHCB_MSR_HV_FT_REQ);
 	VMGEXIT();
 
@@ -114,6 +108,31 @@ u64 get_hv_features(void)
 	return GHCB_MSR_HV_FT_RESP_VAL(val);
 }
 
+u64 __head snp_check_hv_features(void)
+{
+	/*
+	 * SNP is supported in v2 of the GHCB spec which mandates support for HV
+	 * features.
+	 */
+	if (sev_status & MSR_AMD64_SEV_SNP_ENABLED) {
+		u64 hv_features;
+
+		hv_features = get_hv_features();
+		if (!(hv_features & GHCB_HV_FT_SNP))
+			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
+
+		/*
+		 * Running at VMPL0 is required unless an SVSM is present and
+		 * the hypervisor supports the required SVSM GHCB events.
+		 */
+		if (snp_vmpl > 0 && !(hv_features & GHCB_HV_FT_SNP_MULTI_VMPL))
+			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
+
+		return hv_features;
+	}
+	return 0;
+}
+
 void snp_register_ghcb_early(unsigned long paddr)
 {
 	unsigned long pfn = paddr >> PAGE_SHIFT;
diff --git a/arch/x86/boot/startup/sme.c b/arch/x86/boot/startup/sme.c
index 5738b31c8e60..11caa343790d 100644
--- a/arch/x86/boot/startup/sme.c
+++ b/arch/x86/boot/startup/sme.c
@@ -533,6 +533,8 @@ void __head sme_enable(struct boot_params *bp)
 	if (snp_en ^ !!(msr & MSR_AMD64_SEV_SNP_ENABLED))
 		snp_abort();
 
+	sev_hv_features = snp_check_hv_features();
+
 	/* Check if memory encryption is enabled */
 	if (feature_mask == AMD_SME_BIT) {
 		if (!(bp->hdr.xloadflags & XLF_MEM_ENCRYPTION))
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 617988a5f3d7..20c37bff6259 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1170,17 +1170,6 @@ void __init sev_es_init_vc_handling(void)
 	if (!sev_es_check_cpu_features())
 		panic("SEV-ES CPU Features missing");
 
-	/*
-	 * SNP is supported in v2 of the GHCB spec which mandates support for HV
-	 * features.
-	 */
-	if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) {
-		sev_hv_features = get_hv_features();
-
-		if (!(sev_hv_features & GHCB_HV_FT_SNP))
-			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
-	}
-
 	/* Initialize per-cpu GHCB pages */
 	for_each_possible_cpu(cpu) {
 		alloc_runtime_data(cpu);
diff --git a/arch/x86/include/asm/sev-internal.h b/arch/x86/include/asm/sev-internal.h
index e54847a69107..3d3fbcae7ba7 100644
--- a/arch/x86/include/asm/sev-internal.h
+++ b/arch/x86/include/asm/sev-internal.h
@@ -4,7 +4,6 @@
 
 extern struct ghcb boot_ghcb_page;
 extern struct ghcb *boot_ghcb;
-extern u64 sev_hv_features;
 
 /* #VC handler runtime per-CPU data */
 struct sev_es_runtime_data {
@@ -107,6 +106,6 @@ enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
 void snp_register_ghcb_early(unsigned long paddr);
 bool sev_es_negotiate_protocol(void);
 bool sev_es_check_cpu_features(void);
-u64 get_hv_features(void);
+void check_hv_features(void);
 
 const struct snp_cpuid_table *snp_cpuid_get_table(void);
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index a8661dfc9a9a..8637a65973ef 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -418,6 +418,7 @@ struct svsm_call {
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 
 extern u8 snp_vmpl;
+extern u64 sev_hv_features;
 
 extern void __sev_es_ist_enter(struct pt_regs *regs);
 extern void __sev_es_ist_exit(void);
@@ -494,6 +495,7 @@ void snp_set_memory_private(unsigned long vaddr, unsigned long npages);
 void snp_set_wakeup_secondary_cpu(void);
 bool snp_init(struct boot_params *bp);
 void __noreturn snp_abort(void);
+u64 snp_check_hv_features(void);
 void snp_dmi_setup(void);
 int snp_issue_svsm_attest_req(u64 call_id, struct svsm_call *call, struct svsm_attest_call *input);
 void snp_accept_memory(phys_addr_t start, phys_addr_t end);
-- 
2.49.0.805.g082f7c87e0-goog

[PATCH v3 5/5] x86/efistub: Don't bother enabling SEV in the EFI stub

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

One of the last things the EFI stub does before handing over to the core
kernel when booting as a SEV guest is enabling SEV, even though this is
mostly redundant: one of the first things the core kernel does is
calling sme_enable(), after setting up the early GDT and IDT but before
even setting up the kernel page tables. sme_enable() performs the same
SEV-SNP initialization that the decompressor performs in sev_enable().

So let's just drop this call to sev_enable(), and rely on the core
kernel to initiaize SEV correctly.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/include/asm/sev.h              | 2 --
 drivers/firmware/efi/libstub/x86-stub.c | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 8637a65973ef..d762cc0fd47e 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -443,7 +443,6 @@ static __always_inline void sev_es_nmi_complete(void)
 		__sev_es_nmi_complete();
 }
 extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
-extern void sev_enable(struct boot_params *bp);
 
 /*
  * RMPADJUST modifies the RMP permissions of a page of a lesser-
@@ -531,7 +530,6 @@ static inline void sev_es_ist_exit(void) { }
 static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
 static inline void sev_es_nmi_complete(void) { }
 static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; }
-static inline void sev_enable(struct boot_params *bp) { }
 static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
 static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
 static inline void setup_ghcb(void) { }
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index d9ae1a230d39..6b4f5ac91e7f 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -936,12 +936,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 		goto fail;
 	}
 
-	/*
-	 * Call the SEV init code while still running with the firmware's
-	 * GDT/IDT, so #VC exceptions will be handled by EFI.
-	 */
-	sev_enable(boot_params);
-
 	efi_5level_switch();
 
 	enter_kernel(kernel_entry, boot_params);
-- 
2.49.0.805.g082f7c87e0-goog

Re: [PATCH v3 0/5] efi: Don't initalize SEV-SNP from the EFI stub

[Tom Lendacky]

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

Thanks,
Tom

> 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

Re: [PATCH v3 0/5] efi: Don't initalize SEV-SNP from the EFI stub

[Ard Biesheuvel]

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

OK, so memory acceptance from the EFI stub is still broken even when
using the MSR protocol, right?

But SVSM is relatively recent, and so we probably have more leeway to
fix it properly. And I assume the firmware itself can be expected to
have its own working configuration to communicate with the VMM and/or
the hypervisor?

This hybrid mode of executing in the firmware execution context but
taking control of everything under the hood is really a maintenance
nightmare, and so I'd rather do less of that than more of that. If
that implies that we need to re-engineer the early memory acceptance
and the population of the unaccepted memory table, I still think we
need to consider it. Ultimately, the memory acceptance in the stub is
not fundamentally needed, it is only there because there is a mismatch
between the table's granularity and the EFI memory map granularity. In
fact, given that this appears to be a rare occurrence anyway, and
ultimately under the control of the firmware (which we can also fix),
we might simply decide to use an unaccepted memory table with 4k
granularity unless all existing regions unaccepted memory regions are
aligned to 2M.

Re: [PATCH v3 0/5] efi: Don't initalize SEV-SNP from the EFI stub

[Ard Biesheuvel]

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.

Re: [PATCH v3 0/5] efi: Don't initalize SEV-SNP from the EFI stub

[Tom Lendacky]

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

Re: [PATCH v3 0/5] efi: Don't initalize SEV-SNP from the EFI stub

[Tom Lendacky]

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

Re: [PATCH v3 0/5] efi: Don't initalize SEV-SNP from the EFI stub

[Tom Lendacky]

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