[PATCH v2 0/3] efistub/x86: Fix early SEV-SNP memory acceptance

[PATCH v2 0/3] efistub/x86: Fix early SEV-SNP memory acceptance

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

This is a follow-up to [0]. Instead of avoiding memory acceptance from
the EFI stub altogether, switch to a separate API that can be
implemented by SEV-SNP without relying on the shared GHCB page, which
cannot be created yet when still executing in the firmware context with
the firmware's page tables.

[0] https://lore.kernel.org/all/20250325091614.1203411-2-ardb+git@google.com/T/#u

Cc: Tom Lendacky <thomas.lendacky@amd.com>,
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>,
Cc: Dionna Amalie Glaze <dionnaglaze@google.com>,
Cc: Kevin Loughlin <kevinloughlin@google.com>

Ard Biesheuvel (3):
  x86/boot: Move accept_memory() into decompressor
  x86/boot: Use separate API for memory acceptance in the EFI stub
  x86/boot: Implement early memory acceptance for SEV-SNP

 arch/x86/boot/compressed/mem.c                   | 47 +++++++++++++++-
 arch/x86/boot/compressed/sev.c                   | 34 ++++++++++--
 drivers/firmware/efi/libstub/efistub.h           |  3 +-
 drivers/firmware/efi/libstub/unaccepted_memory.c | 57 +++-----------------
 drivers/firmware/efi/libstub/x86-stub.c          | 41 ++++++++++++++
 5 files changed, 124 insertions(+), 58 deletions(-)

-- 
2.49.0.504.g3bcea36a83-goog

[PATCH v2 1/3] x86/boot: Move accept_memory() into decompressor

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

accept_memory() is only called from the decompressor, and uses an API
that will become specific to the decompressor as well, given that the
EFI stub will need to switch to a special memory acceptance API that can
be called while running in the firmware context with the firmware's page
tables.

So move the function into arch/x86/boot/compressed/mem.c

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/mem.c                   | 45 ++++++++++++++++++++
 drivers/firmware/efi/libstub/unaccepted_memory.c | 45 --------------------
 2 files changed, 45 insertions(+), 45 deletions(-)

diff --git a/arch/x86/boot/compressed/mem.c b/arch/x86/boot/compressed/mem.c
index dbba332e4a12..6a888b80669e 100644
--- a/arch/x86/boot/compressed/mem.c
+++ b/arch/x86/boot/compressed/mem.c
@@ -84,3 +84,48 @@ bool init_unaccepted_memory(void)
 
 	return true;
 }
+
+void accept_memory(phys_addr_t start, unsigned long size)
+{
+	unsigned long range_start, range_end;
+	phys_addr_t end = start + size;
+	unsigned long bitmap_size;
+	u64 unit_size;
+
+	if (!unaccepted_table)
+		return;
+
+	unit_size = unaccepted_table->unit_size;
+
+	/*
+	 * Only care for the part of the range that is represented
+	 * in the bitmap.
+	 */
+	if (start < unaccepted_table->phys_base)
+		start = unaccepted_table->phys_base;
+	if (end < unaccepted_table->phys_base)
+		return;
+
+	/* Translate to offsets from the beginning of the bitmap */
+	start -= unaccepted_table->phys_base;
+	end -= unaccepted_table->phys_base;
+
+	/* Make sure not to overrun the bitmap */
+	if (end > unaccepted_table->size * unit_size * BITS_PER_BYTE)
+		end = unaccepted_table->size * unit_size * BITS_PER_BYTE;
+
+	range_start = start / unit_size;
+	bitmap_size = DIV_ROUND_UP(end, unit_size);
+
+	for_each_set_bitrange_from(range_start, range_end,
+				   unaccepted_table->bitmap, bitmap_size) {
+		unsigned long phys_start, phys_end;
+
+		phys_start = range_start * unit_size + unaccepted_table->phys_base;
+		phys_end = range_end * unit_size + unaccepted_table->phys_base;
+
+		arch_accept_memory(phys_start, phys_end);
+		bitmap_clear(unaccepted_table->bitmap,
+			     range_start, range_end - range_start);
+	}
+}
diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
index 757dbe734a47..02040bd6a330 100644
--- a/drivers/firmware/efi/libstub/unaccepted_memory.c
+++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
@@ -176,48 +176,3 @@ void process_unaccepted_memory(u64 start, u64 end)
 	bitmap_set(unaccepted_table->bitmap,
 		   start / unit_size, (end - start) / unit_size);
 }
-
-void accept_memory(phys_addr_t start, unsigned long size)
-{
-	unsigned long range_start, range_end;
-	phys_addr_t end = start + size;
-	unsigned long bitmap_size;
-	u64 unit_size;
-
-	if (!unaccepted_table)
-		return;
-
-	unit_size = unaccepted_table->unit_size;
-
-	/*
-	 * Only care for the part of the range that is represented
-	 * in the bitmap.
-	 */
-	if (start < unaccepted_table->phys_base)
-		start = unaccepted_table->phys_base;
-	if (end < unaccepted_table->phys_base)
-		return;
-
-	/* Translate to offsets from the beginning of the bitmap */
-	start -= unaccepted_table->phys_base;
-	end -= unaccepted_table->phys_base;
-
-	/* Make sure not to overrun the bitmap */
-	if (end > unaccepted_table->size * unit_size * BITS_PER_BYTE)
-		end = unaccepted_table->size * unit_size * BITS_PER_BYTE;
-
-	range_start = start / unit_size;
-	bitmap_size = DIV_ROUND_UP(end, unit_size);
-
-	for_each_set_bitrange_from(range_start, range_end,
-				   unaccepted_table->bitmap, bitmap_size) {
-		unsigned long phys_start, phys_end;
-
-		phys_start = range_start * unit_size + unaccepted_table->phys_base;
-		phys_end = range_end * unit_size + unaccepted_table->phys_base;
-
-		arch_accept_memory(phys_start, phys_end);
-		bitmap_clear(unaccepted_table->bitmap,
-			     range_start, range_end - range_start);
-	}
-}
-- 
2.49.0.504.g3bcea36a83-goog

[PATCH v2 2/3] x86/boot: Use separate API for memory acceptance in the EFI stub

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

The EFI stub runs in the execution context of the firmware, and even if
the memory acceptance that occurs when constructing the E820 memory map
after ExitBootServices(), it is still too early for SEV-SNP based memory
acceptance to use the shared GHCB page, as this requires manipulating
the firmware's live page tables.

So implement the unaccepted memory processing locally in the EFI stub.
The SEV-SNP handling will be updated in a subsequent patch.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/mem.c                   |  2 +-
 drivers/firmware/efi/libstub/efistub.h           |  3 +-
 drivers/firmware/efi/libstub/unaccepted_memory.c | 12 +++---
 drivers/firmware/efi/libstub/x86-stub.c          | 39 ++++++++++++++++++++
 4 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/arch/x86/boot/compressed/mem.c b/arch/x86/boot/compressed/mem.c
index 6a888b80669e..caa3640fc2a0 100644
--- a/arch/x86/boot/compressed/mem.c
+++ b/arch/x86/boot/compressed/mem.c
@@ -32,7 +32,7 @@ static bool early_is_tdx_guest(void)
 	return is_tdx;
 }
 
-void arch_accept_memory(phys_addr_t start, phys_addr_t end)
+static void arch_accept_memory(phys_addr_t start, phys_addr_t end)
 {
 	/* Platform-specific memory-acceptance call goes here */
 	if (early_is_tdx_guest()) {
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index f5ba032863a9..88751d83f584 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -1231,8 +1231,7 @@ efi_zboot_entry(efi_handle_t handle, efi_system_table_t *systab);
 efi_status_t allocate_unaccepted_bitmap(__u32 nr_desc,
 					struct efi_boot_memmap *map);
 void process_unaccepted_memory(u64 start, u64 end);
-void accept_memory(phys_addr_t start, unsigned long size);
-void arch_accept_memory(phys_addr_t start, phys_addr_t end);
+void efistub_accept_memory(phys_addr_t start, phys_addr_t end);
 
 efi_status_t efi_zboot_decompress_init(unsigned long *alloc_size);
 efi_status_t efi_zboot_decompress(u8 *out, unsigned long outlen);
diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
index 02040bd6a330..4e90077ca797 100644
--- a/drivers/firmware/efi/libstub/unaccepted_memory.c
+++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
@@ -118,7 +118,7 @@ void process_unaccepted_memory(u64 start, u64 end)
 	 * immediately accepted in its entirety.
 	 */
 	if (end - start < 2 * unit_size) {
-		arch_accept_memory(start, end);
+		efistub_accept_memory(start, end);
 		return;
 	}
 
@@ -129,13 +129,13 @@ void process_unaccepted_memory(u64 start, u64 end)
 
 	/* Immediately accept a <unit_size piece at the start: */
 	if (start & unit_mask) {
-		arch_accept_memory(start, round_up(start, unit_size));
+		efistub_accept_memory(start, round_up(start, unit_size));
 		start = round_up(start, unit_size);
 	}
 
 	/* Immediately accept a <unit_size piece at the end: */
 	if (end & unit_mask) {
-		arch_accept_memory(round_down(end, unit_size), end);
+		efistub_accept_memory(round_down(end, unit_size), end);
 		end = round_down(end, unit_size);
 	}
 
@@ -144,8 +144,8 @@ void process_unaccepted_memory(u64 start, u64 end)
 	 * into the bitmap.
 	 */
 	if (start < unaccepted_table->phys_base) {
-		arch_accept_memory(start,
-				   min(unaccepted_table->phys_base, end));
+		efistub_accept_memory(start,
+				      min(unaccepted_table->phys_base, end));
 		start = unaccepted_table->phys_base;
 	}
 
@@ -165,7 +165,7 @@ void process_unaccepted_memory(u64 start, u64 end)
 			     unaccepted_table->phys_base;
 		phys_end = end + unaccepted_table->phys_base;
 
-		arch_accept_memory(phys_start, phys_end);
+		efistub_accept_memory(phys_start, phys_end);
 		end = bitmap_size * unit_size * BITS_PER_BYTE;
 	}
 
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index cafc90d4caaf..7d9cf473f4d0 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -17,6 +17,7 @@
 #include <asm/boot.h>
 #include <asm/kaslr.h>
 #include <asm/sev.h>
+#include <asm/shared/tdx.h>
 
 #include "efistub.h"
 #include "x86-stub.h"
@@ -364,6 +365,44 @@ static void setup_unaccepted_memory(void)
 		efi_err("Memory acceptance protocol failed\n");
 }
 
+#ifdef CONFIG_UNACCEPTED_MEMORY
+
+static bool efistub_is_tdx_guest(void)
+{
+	u32 eax = TDX_CPUID_LEAF_ID, sig[3] = {};
+
+	if (!IS_ENABLED(CONFIG_INTEL_TDX_GUEST))
+		return false;
+
+	native_cpuid(&eax, &sig[0], &sig[2], &sig[1]);
+	return !memcmp(TDX_IDENT, sig, sizeof(sig));
+}
+
+static bool efistub_is_sevsnp_guest(void)
+{
+	return sev_get_status() & MSR_AMD64_SEV_SNP_ENABLED;
+}
+
+void efistub_accept_memory(phys_addr_t start, phys_addr_t end)
+{
+	static bool once, is_tdx, is_sevsnp;
+
+	if (!once) {
+		if (efistub_is_tdx_guest())
+			is_tdx = true;
+		else if (efistub_is_sevsnp_guest())
+			is_sevsnp = true;
+		once = true;
+	}
+
+	if (is_tdx)
+		tdx_accept_memory(start, end);
+	else if (is_sevsnp)
+		snp_accept_memory(start, end);
+}
+
+#endif
+
 static efi_char16_t *efistub_fw_vendor(void)
 {
 	unsigned long vendor = efi_table_attr(efi_system_table, fw_vendor);
-- 
2.49.0.504.g3bcea36a83-goog

[PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

Switch to a different API for accepting memory in SEV-SNP guests, one
which is actually supported at the point during boot where the EFI stub
may need to accept memory, but the SEV-SNP init code has not executed
yet.

Co-developed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/sev.c          | 34 +++++++++++++++++---
 drivers/firmware/efi/libstub/x86-stub.c |  4 ++-
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index bb55934c1cee..88100bf83ded 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -164,10 +164,7 @@ bool sev_snp_enabled(void)
 
 static void __page_state_change(unsigned long paddr, enum psc_op op)
 {
-	u64 val;
-
-	if (!sev_snp_enabled())
-		return;
+	u64 val, msr;
 
 	/*
 	 * If private -> shared then invalidate the page before requesting the
@@ -176,6 +173,9 @@ static void __page_state_change(unsigned long paddr, enum psc_op op)
 	if (op == SNP_PAGE_STATE_SHARED)
 		pvalidate_4k_page(paddr, paddr, false);
 
+	/* Save the current GHCB MSR value */
+	msr = sev_es_rd_ghcb_msr();
+
 	/* Issue VMGEXIT to change the page state in RMP table. */
 	sev_es_wr_ghcb_msr(GHCB_MSR_PSC_REQ_GFN(paddr >> PAGE_SHIFT, op));
 	VMGEXIT();
@@ -185,6 +185,9 @@ static void __page_state_change(unsigned long paddr, enum psc_op op)
 	if ((GHCB_RESP_CODE(val) != GHCB_MSR_PSC_RESP) || GHCB_MSR_PSC_RESP_VAL(val))
 		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PSC);
 
+	/* Restore the GHCB MSR value */
+	sev_es_wr_ghcb_msr(msr);
+
 	/*
 	 * Now that page state is changed in the RMP table, validate it so that it is
 	 * consistent with the RMP entry.
@@ -195,11 +198,17 @@ static void __page_state_change(unsigned long paddr, enum psc_op op)
 
 void snp_set_page_private(unsigned long paddr)
 {
+	if (!sev_snp_enabled())
+		return;
+
 	__page_state_change(paddr, SNP_PAGE_STATE_PRIVATE);
 }
 
 void snp_set_page_shared(unsigned long paddr)
 {
+	if (!sev_snp_enabled())
+		return;
+
 	__page_state_change(paddr, SNP_PAGE_STATE_SHARED);
 }
 
@@ -261,6 +270,11 @@ static phys_addr_t __snp_accept_memory(struct snp_psc_desc *desc,
 	return pa;
 }
 
+/*
+ * The memory acceptance support uses the boot GHCB page to perform
+ * the required page state change operation before validating the
+ * pages.
+ */
 void snp_accept_memory(phys_addr_t start, phys_addr_t end)
 {
 	struct snp_psc_desc desc = {};
@@ -275,6 +289,18 @@ void snp_accept_memory(phys_addr_t start, phys_addr_t end)
 		pa = __snp_accept_memory(&desc, pa, end);
 }
 
+/*
+ * The early version of memory acceptance is needed when being called
+ * from the EFI stub driver. The pagetable manipulation to mark the
+ * boot GHCB page as shared can't be performed at this stage, so use
+ * the GHCB page state change MSR protocol instead.
+ */
+void snp_accept_memory_early(phys_addr_t start, phys_addr_t end)
+{
+	for (phys_addr_t pa = start; pa < end; pa += PAGE_SIZE)
+		__page_state_change(pa, SNP_PAGE_STATE_PRIVATE);
+}
+
 void sev_es_shutdown_ghcb(void)
 {
 	if (!boot_ghcb)
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 7d9cf473f4d0..dcf436dea99e 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -383,6 +383,8 @@ static bool efistub_is_sevsnp_guest(void)
 	return sev_get_status() & MSR_AMD64_SEV_SNP_ENABLED;
 }
 
+void snp_accept_memory_early(phys_addr_t start, phys_addr_t end);
+
 void efistub_accept_memory(phys_addr_t start, phys_addr_t end)
 {
 	static bool once, is_tdx, is_sevsnp;
@@ -398,7 +400,7 @@ void efistub_accept_memory(phys_addr_t start, phys_addr_t end)
 	if (is_tdx)
 		tdx_accept_memory(start, end);
 	else if (is_sevsnp)
-		snp_accept_memory(start, end);
+		snp_accept_memory_early(start, end);
 }
 
 #endif
-- 
2.49.0.504.g3bcea36a83-goog

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Kirill A. Shutemov]

On Fri, Apr 04, 2025 at 10:29:25AM +0200, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Switch to a different API for accepting memory in SEV-SNP guests, one
> which is actually supported at the point during boot where the EFI stub
> may need to accept memory, but the SEV-SNP init code has not executed
> yet.

I probably miss the point, but why cannot decompressor use the same _early
version of accept too and avoid code duplication?

Maybe spell it out in the commit message for someone like me :P

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Ard Biesheuvel]

On Fri, 4 Apr 2025 at 11:43, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> On Fri, Apr 04, 2025 at 10:29:25AM +0200, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > Switch to a different API for accepting memory in SEV-SNP guests, one
> > which is actually supported at the point during boot where the EFI stub
> > may need to accept memory, but the SEV-SNP init code has not executed
> > yet.
>
> I probably miss the point, but why cannot decompressor use the same _early
> version of accept too and avoid code duplication?
>
> Maybe spell it out in the commit message for someone like me :P
>

I assumed there was a reason that the shared GHCB page is used early
on. Maybe it is faster than accepting memory page by page?

It also depends on how important the memory acceptance is for the
legacy decompressor - AIUI the use case is primarily kexec, but
wouldn't the first kernel have accepted all memory already? I.e., if
it is slower, we might not care if it is a rare case.

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Dionna Amalie Glaze]

On Fri, Apr 4, 2025 at 1:46 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 4 Apr 2025 at 11:43, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> >
> > On Fri, Apr 04, 2025 at 10:29:25AM +0200, Ard Biesheuvel wrote:
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > Switch to a different API for accepting memory in SEV-SNP guests, one
> > > which is actually supported at the point during boot where the EFI stub
> > > may need to accept memory, but the SEV-SNP init code has not executed
> > > yet.
> >
> > I probably miss the point, but why cannot decompressor use the same _early
> > version of accept too and avoid code duplication?
> >
> > Maybe spell it out in the commit message for someone like me :P
> >
>
> I assumed there was a reason that the shared GHCB page is used early
> on. Maybe it is faster than accepting memory page by page?

This is correct. The MSR protocol does a round trip per page, whereas
the GHCB page can communicate hundreds of state changes per round
trip.
>
> It also depends on how important the memory acceptance is for the
> legacy decompressor - AIUI the use case is primarily kexec, but
> wouldn't the first kernel have accepted all memory already? I.e., if

The first kernel may not accept all memory due to the laziness of
unaccepted memory transitions.
I'm not sure if we have the planned background acceptance process in
place (probably not), but we
can't expect that to have finished before the first kexec.

> it is slower, we might not care if it is a rare case.

If the GHCB is available, we should always prefer it.

-- 
-Dionna Glaze, PhD, CISSP, CCSP (she/her)

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Kirill A. Shutemov]

On Fri, Apr 04, 2025 at 08:07:03AM -0700, Dionna Amalie Glaze wrote:
> If the GHCB is available, we should always prefer it.

I believe we should consider the cost of code duplication in this
situation.

If the non-early version is only used in the kexec path, it will not be
tested as frequently and could be more easily broken. I think it would be
acceptable for kexec to be slightly slower if it results in more
maintainable code.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Ingo Molnar]

* Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote:

> On Fri, Apr 04, 2025 at 08:07:03AM -0700, Dionna Amalie Glaze wrote:
> > If the GHCB is available, we should always prefer it.
> 
> I believe we should consider the cost of code duplication in this
> situation.
> 
> If the non-early version is only used in the kexec path, it will not be
> tested as frequently and could be more easily broken. I think it would be
> acceptable for kexec to be slightly slower if it results in more
> maintainable code.

Absolutely so.

Thanks,

	Ingo

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Ard Biesheuvel]

On Mon, 7 Apr 2025 at 18:44, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote:
>
> > On Fri, Apr 04, 2025 at 08:07:03AM -0700, Dionna Amalie Glaze wrote:
> > > If the GHCB is available, we should always prefer it.
> >
> > I believe we should consider the cost of code duplication in this
> > situation.
> >
> > If the non-early version is only used in the kexec path, it will not be
> > tested as frequently and could be more easily broken. I think it would be
> > acceptable for kexec to be slightly slower if it results in more
> > maintainable code.
>
> Absolutely so.
>

It would be nice if someone could quantify 'slightly slower' - I am
leaning to the same conclusion but I have no clue what the actual
performance impact is.

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Kirill A. Shutemov]

On Mon, Apr 07, 2025 at 07:21:17PM +0200, Ard Biesheuvel wrote:
> On Mon, 7 Apr 2025 at 18:44, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >
> > * Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote:
> >
> > > On Fri, Apr 04, 2025 at 08:07:03AM -0700, Dionna Amalie Glaze wrote:
> > > > If the GHCB is available, we should always prefer it.
> > >
> > > I believe we should consider the cost of code duplication in this
> > > situation.
> > >
> > > If the non-early version is only used in the kexec path, it will not be
> > > tested as frequently and could be more easily broken. I think it would be
> > > acceptable for kexec to be slightly slower if it results in more
> > > maintainable code.
> >
> > Absolutely so.
> >
> 
> It would be nice if someone could quantify 'slightly slower' - I am
> leaning to the same conclusion but I have no clue what the actual
> performance impact is.

If we can survive the performance of the initial boot, we can live with it
for kexec.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Ard Biesheuvel]

On Mon, 7 Apr 2025 at 19:33, Kirill A. Shutemov <kirill@shutemov.name> wrote:
>
> On Mon, Apr 07, 2025 at 07:21:17PM +0200, Ard Biesheuvel wrote:
> > On Mon, 7 Apr 2025 at 18:44, Ingo Molnar <mingo@kernel.org> wrote:
> > >
> > >
> > > * Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote:
> > >
> > > > On Fri, Apr 04, 2025 at 08:07:03AM -0700, Dionna Amalie Glaze wrote:
> > > > > If the GHCB is available, we should always prefer it.
> > > >
> > > > I believe we should consider the cost of code duplication in this
> > > > situation.
> > > >
> > > > If the non-early version is only used in the kexec path, it will not be
> > > > tested as frequently and could be more easily broken. I think it would be
> > > > acceptable for kexec to be slightly slower if it results in more
> > > > maintainable code.
> > >
> > > Absolutely so.
> > >
> >
> > It would be nice if someone could quantify 'slightly slower' - I am
> > leaning to the same conclusion but I have no clue what the actual
> > performance impact is.
>
> If we can survive the performance of the initial boot, we can live with it
> for kexec.
>

The initial boot does not occur via the decompressor, but via the EFI
stub, where memory acceptance is handled by the firmware (as it
should).

Given that the traditional decompressor carves out an allocation from
the raw E820 map without using any of the higher level APIs, it has to
accept the memory itself if it is marked as unaccepted in the table.

Perhaps the decompressor should try to avoid unaccepted memory?

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Kirill A. Shutemov]

On Mon, Apr 07, 2025 at 07:45:59PM +0200, Ard Biesheuvel wrote:
> On Mon, 7 Apr 2025 at 19:33, Kirill A. Shutemov <kirill@shutemov.name> wrote:
> >
> > On Mon, Apr 07, 2025 at 07:21:17PM +0200, Ard Biesheuvel wrote:
> > > On Mon, 7 Apr 2025 at 18:44, Ingo Molnar <mingo@kernel.org> wrote:
> > > >
> > > >
> > > > * Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote:
> > > >
> > > > > On Fri, Apr 04, 2025 at 08:07:03AM -0700, Dionna Amalie Glaze wrote:
> > > > > > If the GHCB is available, we should always prefer it.
> > > > >
> > > > > I believe we should consider the cost of code duplication in this
> > > > > situation.
> > > > >
> > > > > If the non-early version is only used in the kexec path, it will not be
> > > > > tested as frequently and could be more easily broken. I think it would be
> > > > > acceptable for kexec to be slightly slower if it results in more
> > > > > maintainable code.
> > > >
> > > > Absolutely so.
> > > >
> > >
> > > It would be nice if someone could quantify 'slightly slower' - I am
> > > leaning to the same conclusion but I have no clue what the actual
> > > performance impact is.
> >
> > If we can survive the performance of the initial boot, we can live with it
> > for kexec.
> >
> 
> The initial boot does not occur via the decompressor, but via the EFI
> stub, where memory acceptance is handled by the firmware (as it
> should).

I wounder what protocol BIOS uses.

> Given that the traditional decompressor carves out an allocation from
> the raw E820 map without using any of the higher level APIs, it has to
> accept the memory itself if it is marked as unaccepted in the table.
> 
> Perhaps the decompressor should try to avoid unaccepted memory?

It limits KASLR. I would rather wait more on kexec.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Tom Lendacky]

On 4/7/25 04:25, Kirill A. Shutemov wrote:
> On Fri, Apr 04, 2025 at 08:07:03AM -0700, Dionna Amalie Glaze wrote:
>> If the GHCB is available, we should always prefer it.
> 
> I believe we should consider the cost of code duplication in this
> situation.
> 
> If the non-early version is only used in the kexec path, it will not be
> tested as frequently and could be more easily broken. I think it would be
> acceptable for kexec to be slightly slower if it results in more
> maintainable code.
> 

Is accept_memory() in the decompressor or efistub only used in the kexec
path?

Thanks,
Tom

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Ard Biesheuvel]

On Mon, 7 Apr 2025 at 20:05, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 4/7/25 04:25, Kirill A. Shutemov wrote:
> > On Fri, Apr 04, 2025 at 08:07:03AM -0700, Dionna Amalie Glaze wrote:
> >> If the GHCB is available, we should always prefer it.
> >
> > I believe we should consider the cost of code duplication in this
> > situation.
> >
> > If the non-early version is only used in the kexec path, it will not be
> > tested as frequently and could be more easily broken. I think it would be
> > acceptable for kexec to be slightly slower if it results in more
> > maintainable code.
> >
>
> Is accept_memory() in the decompressor or efistub only used in the kexec
> path?
>

The EFI stub does not call accept_memory(), only the decompressor
does. The only use case for explicit memory acceptance in the EFI stub
is process_unaccepted_memory(), which accepts the misaligned chunk of
memory that cannot be described at 2M granularity by the accepted
memory table.

Remember that the EFI stub no longer calls into the legacy
decompressor at all - it decompresses the kernel while executing the
EFI boot services and branches straight to it.

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Tom Lendacky]

On 4/7/25 14:59, Ard Biesheuvel wrote:
> On Mon, 7 Apr 2025 at 20:05, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 4/7/25 04:25, Kirill A. Shutemov wrote:
>>> On Fri, Apr 04, 2025 at 08:07:03AM -0700, Dionna Amalie Glaze wrote:
>>>> If the GHCB is available, we should always prefer it.
>>>
>>> I believe we should consider the cost of code duplication in this
>>> situation.
>>>
>>> If the non-early version is only used in the kexec path, it will not be
>>> tested as frequently and could be more easily broken. I think it would be
>>> acceptable for kexec to be slightly slower if it results in more
>>> maintainable code.
>>>
>>
>> Is accept_memory() in the decompressor or efistub only used in the kexec
>> path?
>>
> 
> The EFI stub does not call accept_memory(), only the decompressor
> does. The only use case for explicit memory acceptance in the EFI stub

Since EFI stub never uses accept_memory() I looked at moving enablement
of SEV to be before the setup of the accepted memory bitmap, as SEV
enablement doesn't need any e820 info. But that didn't work because the
real issue is early_setup_ghcb() calls set_page_decrypted() which calls
set_clr_page_flags(). The latter function is not meant to work with EFI
page tables, so there is an incompatibility.

If we had a way to check for whether we are coming through the EFI stub
vs the decompressor, then snp_accept_memory() could decide to skip
early_setup_ghcb() when called from the EFI stub and call either
__snp_accept_memory() from the decompressor or __page_state_change()
from the EFI stub (the latter having to be updated to return a value).

I think there are other areas that might need investigating because I
noticed that efi_warn() is successful before efi_exit_boot_services()
but blows up immediately after (possibly in the EFI #VC handler having
to do with addressing the string?).

Thanks,
Tom


> is process_unaccepted_memory(), which accepts the misaligned chunk of
> memory that cannot be described at 2M granularity by the accepted
> memory table.
> 
> Remember that the EFI stub no longer calls into the legacy
> decompressor at all - it decompresses the kernel while executing the
> EFI boot services and branches straight to it.

Re: [PATCH v2 3/3] x86/boot: Implement early memory acceptance for SEV-SNP

[Ard Biesheuvel]

On Tue, 8 Apr 2025 at 17:53, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 4/7/25 14:59, Ard Biesheuvel wrote:
> > On Mon, 7 Apr 2025 at 20:05, Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >>
> >> On 4/7/25 04:25, Kirill A. Shutemov wrote:
> >>> On Fri, Apr 04, 2025 at 08:07:03AM -0700, Dionna Amalie Glaze wrote:
> >>>> If the GHCB is available, we should always prefer it.
> >>>
> >>> I believe we should consider the cost of code duplication in this
> >>> situation.
> >>>
> >>> If the non-early version is only used in the kexec path, it will not be
> >>> tested as frequently and could be more easily broken. I think it would be
> >>> acceptable for kexec to be slightly slower if it results in more
> >>> maintainable code.
> >>>
> >>
> >> Is accept_memory() in the decompressor or efistub only used in the kexec
> >> path?
> >>
> >
> > The EFI stub does not call accept_memory(), only the decompressor
> > does. The only use case for explicit memory acceptance in the EFI stub
>
> Since EFI stub never uses accept_memory() I looked at moving enablement
> of SEV to be before the setup of the accepted memory bitmap, as SEV
> enablement doesn't need any e820 info. But that didn't work because the
> real issue is early_setup_ghcb() calls set_page_decrypted() which calls
> set_clr_page_flags(). The latter function is not meant to work with EFI
> page tables, so there is an incompatibility.
>

I would prefer to go into the other direction, and not enable SEV at
all in the EFI stub. The core kernel will happily enable SEV anyway,
right?

> If we had a way to check for whether we are coming through the EFI stub
> vs the decompressor, then snp_accept_memory() could decide to skip
> early_setup_ghcb() when called from the EFI stub and call either
> __snp_accept_memory() from the decompressor or __page_state_change()
> from the EFI stub (the latter having to be updated to return a value).
>
> I think there are other areas that might need investigating because I
> noticed that efi_warn() is successful before efi_exit_boot_services()
> but blows up immediately after (possibly in the EFI #VC handler having
> to do with addressing the string?).
>

efi_warn() blows up because the console is gone after
ExitBootServices(). That is not SEV related.