[PATCH] efi/unaccepted: Use ACPI reclaim memory for unaccepted memory table

[PATCH] efi/unaccepted: Use ACPI reclaim memory for unaccepted memory table

[Ard Biesheuvel]

Kyril reports that crashkernels fail to work on confidential VMs that
rely on the unaccepted memory table, and this appears to be caused by
the fact that it is not considered part of the set of firmware tables
that the crashkernel needs to map.

This is an oversight, and a result of the use of the EFI_LOADER_DATA
memory type for this table. The correct memory type to use for any
firmware table is EFI_ACPI_RECLAIM_MEMORY (including ones created by the
EFI stub), even though the name suggests that is it specific to ACPI.
ACPI reclaim means that the memory is used by the firmware to expose
information to the operating system, but that the memory region has no
special significance to the firmware itself, and the OS is free to
reclaim the memory and use it as ordinary memory if it is not interested
in the contents, or if it has already consumed them. In Linux, this
memory is never reclaimed, but it is always covered by the kernel direct
map and generally made accessible as ordinary memory.

On x86, ACPI reclaim memory is translated into E820_ACPI, which the
kexec logic already recognizes as memory that the crashkernel may need
to to access, and so it will be mapped and accessible to the booting
crash kernel.

Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/unaccepted_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
index ca61f4733ea58693..9a655f30ba47db01 100644
--- a/drivers/firmware/efi/libstub/unaccepted_memory.c
+++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
@@ -62,7 +62,7 @@ efi_status_t allocate_unaccepted_bitmap(__u32 nr_desc,
 	bitmap_size = DIV_ROUND_UP(unaccepted_end - unaccepted_start,
 				   EFI_UNACCEPTED_UNIT_SIZE * BITS_PER_BYTE);
 
-	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
+	status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
 			     sizeof(*unaccepted_table) + bitmap_size,
 			     (void **)&unaccepted_table);
 	if (status != EFI_SUCCESS) {
-- 
2.39.2

[PATCH] x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges

[Kirill A. Shutemov]

e820__end_of_ram_pfn() is used to calculate max_pfn which, among other
things, guides where direct mapping ends. Any memory above max_pfn is
not going to be present in the direct mapping.

e820__end_of_ram_pfn() finds the end of the ram based on the highest
E820_TYPE_RAM range. But it doesn't includes E820_TYPE_ACPI ranges into
calculation.

Despite the name, E820_TYPE_ACPI covers not only ACPI data, but also EFI
tables and might be required by kernel to function properly.

Usually the problem is hidden because there is some E820_TYPE_RAM memory
above E820_TYPE_ACPI. But crashkernel only presents pre-allocated crash
memory as E820_TYPE_RAM on boot. If the preallocated range is small, it
can fit under the last E820_TYPE_ACPI range.

Modify e820__end_of_ram_pfn() and e820__end_of_low_ram_pfn() to cover
E820_TYPE_ACPI memory.

The problem was discovered during debugging kexec for TDX guest. TDX
guest uses E820_TYPE_ACPI to store the unaccepted memory bitmap and pass
it between the kernels on kexec.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/kernel/e820.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index fb8cf953380d..99c80680dc9e 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -827,7 +827,7 @@ u64 __init e820__memblock_alloc_reserved(u64 size, u64 align)
 /*
  * Find the highest page frame number we have available
  */
-static unsigned long __init e820_end_pfn(unsigned long limit_pfn, enum e820_type type)
+static unsigned long __init e820_end_ram_pfn(unsigned long limit_pfn)
 {
 	int i;
 	unsigned long last_pfn = 0;
@@ -838,7 +838,8 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, enum e820_type
 		unsigned long start_pfn;
 		unsigned long end_pfn;
 
-		if (entry->type != type)
+		if (entry->type != E820_TYPE_RAM &&
+		    entry->type != E820_TYPE_ACPI)
 			continue;
 
 		start_pfn = entry->addr >> PAGE_SHIFT;
@@ -864,12 +865,12 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, enum e820_type
 
 unsigned long __init e820__end_of_ram_pfn(void)
 {
-	return e820_end_pfn(MAX_ARCH_PFN, E820_TYPE_RAM);
+	return e820_end_ram_pfn(MAX_ARCH_PFN);
 }
 
 unsigned long __init e820__end_of_low_ram_pfn(void)
 {
-	return e820_end_pfn(1UL << (32 - PAGE_SHIFT), E820_TYPE_RAM);
+	return e820_end_ram_pfn(1UL << (32 - PAGE_SHIFT));
 }
 
 static void __init early_panic(char *msg)
-- 
2.41.0

Re: [PATCH] x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges

[Ard Biesheuvel]

On Wed, 16 Aug 2023 at 23:24, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> e820__end_of_ram_pfn() is used to calculate max_pfn which, among other
> things, guides where direct mapping ends. Any memory above max_pfn is
> not going to be present in the direct mapping.
>
> e820__end_of_ram_pfn() finds the end of the ram based on the highest
> E820_TYPE_RAM range. But it doesn't includes E820_TYPE_ACPI ranges into
> calculation.
>
> Despite the name, E820_TYPE_ACPI covers not only ACPI data, but also EFI
> tables and might be required by kernel to function properly.
>
> Usually the problem is hidden because there is some E820_TYPE_RAM memory
> above E820_TYPE_ACPI. But crashkernel only presents pre-allocated crash
> memory as E820_TYPE_RAM on boot. If the preallocated range is small, it
> can fit under the last E820_TYPE_ACPI range.
>
> Modify e820__end_of_ram_pfn() and e820__end_of_low_ram_pfn() to cover
> E820_TYPE_ACPI memory.
>
> The problem was discovered during debugging kexec for TDX guest. TDX
> guest uses E820_TYPE_ACPI to store the unaccepted memory bitmap and pass
> it between the kernels on kexec.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

No objections to this, but we might also simply drop E820_TYPE_ACPI
altogether: it is only used for EFI_ACPI_RECLAIM_MEMORY, which is
memory that can be used by the OS as ordinary RAM if it is not
interested in the contents (or has already consumed them). So this
could arguably be classified as E820_TYPE_RAM too.

> ---
>  arch/x86/kernel/e820.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index fb8cf953380d..99c80680dc9e 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -827,7 +827,7 @@ u64 __init e820__memblock_alloc_reserved(u64 size, u64 align)
>  /*
>   * Find the highest page frame number we have available
>   */
> -static unsigned long __init e820_end_pfn(unsigned long limit_pfn, enum e820_type type)
> +static unsigned long __init e820_end_ram_pfn(unsigned long limit_pfn)
>  {
>         int i;
>         unsigned long last_pfn = 0;
> @@ -838,7 +838,8 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, enum e820_type
>                 unsigned long start_pfn;
>                 unsigned long end_pfn;
>
> -               if (entry->type != type)
> +               if (entry->type != E820_TYPE_RAM &&
> +                   entry->type != E820_TYPE_ACPI)
>                         continue;
>
>                 start_pfn = entry->addr >> PAGE_SHIFT;
> @@ -864,12 +865,12 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, enum e820_type
>
>  unsigned long __init e820__end_of_ram_pfn(void)
>  {
> -       return e820_end_pfn(MAX_ARCH_PFN, E820_TYPE_RAM);
> +       return e820_end_ram_pfn(MAX_ARCH_PFN);
>  }
>
>  unsigned long __init e820__end_of_low_ram_pfn(void)
>  {
> -       return e820_end_pfn(1UL << (32 - PAGE_SHIFT), E820_TYPE_RAM);
> +       return e820_end_ram_pfn(1UL << (32 - PAGE_SHIFT));
>  }
>
>  static void __init early_panic(char *msg)
> --
> 2.41.0
>

Re: [PATCH] x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges

[Kirill A. Shutemov]

On Thu, Aug 17, 2023 at 10:25:56PM +0200, Ard Biesheuvel wrote:
> On Wed, 16 Aug 2023 at 23:24, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> >
> > e820__end_of_ram_pfn() is used to calculate max_pfn which, among other
> > things, guides where direct mapping ends. Any memory above max_pfn is
> > not going to be present in the direct mapping.
> >
> > e820__end_of_ram_pfn() finds the end of the ram based on the highest
> > E820_TYPE_RAM range. But it doesn't includes E820_TYPE_ACPI ranges into
> > calculation.
> >
> > Despite the name, E820_TYPE_ACPI covers not only ACPI data, but also EFI
> > tables and might be required by kernel to function properly.
> >
> > Usually the problem is hidden because there is some E820_TYPE_RAM memory
> > above E820_TYPE_ACPI. But crashkernel only presents pre-allocated crash
> > memory as E820_TYPE_RAM on boot. If the preallocated range is small, it
> > can fit under the last E820_TYPE_ACPI range.
> >
> > Modify e820__end_of_ram_pfn() and e820__end_of_low_ram_pfn() to cover
> > E820_TYPE_ACPI memory.
> >
> > The problem was discovered during debugging kexec for TDX guest. TDX
> > guest uses E820_TYPE_ACPI to store the unaccepted memory bitmap and pass
> > it between the kernels on kexec.
> >
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> 
> No objections to this, but we might also simply drop E820_TYPE_ACPI
> altogether: it is only used for EFI_ACPI_RECLAIM_MEMORY, which is
> memory that can be used by the OS as ordinary RAM if it is not
> interested in the contents (or has already consumed them). So this
> could arguably be classified as E820_TYPE_RAM too.

Hm. I'm not sure about this. E820_TYPE_ACPI also get tracked as
IORES_DESC_ACPI_TABLES resource and get passed to the next kernel on
kexec, regardless if it is crash kernel or not. I'm not sure we would not
break anything.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges

[Ard Biesheuvel]

On Fri, 18 Aug 2023 at 17:16, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> On Thu, Aug 17, 2023 at 10:25:56PM +0200, Ard Biesheuvel wrote:
> > On Wed, 16 Aug 2023 at 23:24, Kirill A. Shutemov
> > <kirill.shutemov@linux.intel.com> wrote:
> > >
> > > e820__end_of_ram_pfn() is used to calculate max_pfn which, among other
> > > things, guides where direct mapping ends. Any memory above max_pfn is
> > > not going to be present in the direct mapping.
> > >
> > > e820__end_of_ram_pfn() finds the end of the ram based on the highest
> > > E820_TYPE_RAM range. But it doesn't includes E820_TYPE_ACPI ranges into
> > > calculation.
> > >
> > > Despite the name, E820_TYPE_ACPI covers not only ACPI data, but also EFI
> > > tables and might be required by kernel to function properly.
> > >
> > > Usually the problem is hidden because there is some E820_TYPE_RAM memory
> > > above E820_TYPE_ACPI. But crashkernel only presents pre-allocated crash
> > > memory as E820_TYPE_RAM on boot. If the preallocated range is small, it
> > > can fit under the last E820_TYPE_ACPI range.
> > >
> > > Modify e820__end_of_ram_pfn() and e820__end_of_low_ram_pfn() to cover
> > > E820_TYPE_ACPI memory.
> > >
> > > The problem was discovered during debugging kexec for TDX guest. TDX
> > > guest uses E820_TYPE_ACPI to store the unaccepted memory bitmap and pass
> > > it between the kernels on kexec.
> > >
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >
> > No objections to this, but we might also simply drop E820_TYPE_ACPI
> > altogether: it is only used for EFI_ACPI_RECLAIM_MEMORY, which is
> > memory that can be used by the OS as ordinary RAM if it is not
> > interested in the contents (or has already consumed them). So this
> > could arguably be classified as E820_TYPE_RAM too.
>
> Hm. I'm not sure about this. E820_TYPE_ACPI also get tracked as
> IORES_DESC_ACPI_TABLES resource and get passed to the next kernel on
> kexec, regardless if it is crash kernel or not. I'm not sure we would not
> break anything.
>

Yeah, you're right. So this patch is necessary in any case.

Do we also need the EFI side patch then?

Re: [PATCH] x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges

[Kirill A. Shutemov]

On Wed, Sep 06, 2023 at 11:17:12AM +0200, Ard Biesheuvel wrote:
> On Fri, 18 Aug 2023 at 17:16, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> >
> > On Thu, Aug 17, 2023 at 10:25:56PM +0200, Ard Biesheuvel wrote:
> > > On Wed, 16 Aug 2023 at 23:24, Kirill A. Shutemov
> > > <kirill.shutemov@linux.intel.com> wrote:
> > > >
> > > > e820__end_of_ram_pfn() is used to calculate max_pfn which, among other
> > > > things, guides where direct mapping ends. Any memory above max_pfn is
> > > > not going to be present in the direct mapping.
> > > >
> > > > e820__end_of_ram_pfn() finds the end of the ram based on the highest
> > > > E820_TYPE_RAM range. But it doesn't includes E820_TYPE_ACPI ranges into
> > > > calculation.
> > > >
> > > > Despite the name, E820_TYPE_ACPI covers not only ACPI data, but also EFI
> > > > tables and might be required by kernel to function properly.
> > > >
> > > > Usually the problem is hidden because there is some E820_TYPE_RAM memory
> > > > above E820_TYPE_ACPI. But crashkernel only presents pre-allocated crash
> > > > memory as E820_TYPE_RAM on boot. If the preallocated range is small, it
> > > > can fit under the last E820_TYPE_ACPI range.
> > > >
> > > > Modify e820__end_of_ram_pfn() and e820__end_of_low_ram_pfn() to cover
> > > > E820_TYPE_ACPI memory.
> > > >
> > > > The problem was discovered during debugging kexec for TDX guest. TDX
> > > > guest uses E820_TYPE_ACPI to store the unaccepted memory bitmap and pass
> > > > it between the kernels on kexec.
> > > >
> > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > >
> > > No objections to this, but we might also simply drop E820_TYPE_ACPI
> > > altogether: it is only used for EFI_ACPI_RECLAIM_MEMORY, which is
> > > memory that can be used by the OS as ordinary RAM if it is not
> > > interested in the contents (or has already consumed them). So this
> > > could arguably be classified as E820_TYPE_RAM too.
> >
> > Hm. I'm not sure about this. E820_TYPE_ACPI also get tracked as
> > IORES_DESC_ACPI_TABLES resource and get passed to the next kernel on
> > kexec, regardless if it is crash kernel or not. I'm not sure we would not
> > break anything.
> >
> 
> Yeah, you're right. So this patch is necessary in any case.
> 
> Do we also need the EFI side patch then?

Yes, we need it to get it mapped into the crashkernel direct mapping.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges

[Kirill A. Shutemov]

On Thu, Sep 07, 2023 at 01:49:14PM +0300, Kirill A. Shutemov wrote:
> On Wed, Sep 06, 2023 at 11:17:12AM +0200, Ard Biesheuvel wrote:
> > On Fri, 18 Aug 2023 at 17:16, Kirill A. Shutemov
> > <kirill.shutemov@linux.intel.com> wrote:
> > >
> > > On Thu, Aug 17, 2023 at 10:25:56PM +0200, Ard Biesheuvel wrote:
> > > > On Wed, 16 Aug 2023 at 23:24, Kirill A. Shutemov
> > > > <kirill.shutemov@linux.intel.com> wrote:
> > > > >
> > > > > e820__end_of_ram_pfn() is used to calculate max_pfn which, among other
> > > > > things, guides where direct mapping ends. Any memory above max_pfn is
> > > > > not going to be present in the direct mapping.
> > > > >
> > > > > e820__end_of_ram_pfn() finds the end of the ram based on the highest
> > > > > E820_TYPE_RAM range. But it doesn't includes E820_TYPE_ACPI ranges into
> > > > > calculation.
> > > > >
> > > > > Despite the name, E820_TYPE_ACPI covers not only ACPI data, but also EFI
> > > > > tables and might be required by kernel to function properly.
> > > > >
> > > > > Usually the problem is hidden because there is some E820_TYPE_RAM memory
> > > > > above E820_TYPE_ACPI. But crashkernel only presents pre-allocated crash
> > > > > memory as E820_TYPE_RAM on boot. If the preallocated range is small, it
> > > > > can fit under the last E820_TYPE_ACPI range.
> > > > >
> > > > > Modify e820__end_of_ram_pfn() and e820__end_of_low_ram_pfn() to cover
> > > > > E820_TYPE_ACPI memory.
> > > > >
> > > > > The problem was discovered during debugging kexec for TDX guest. TDX
> > > > > guest uses E820_TYPE_ACPI to store the unaccepted memory bitmap and pass
> > > > > it between the kernels on kexec.
> > > > >
> > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > >
> > > > No objections to this, but we might also simply drop E820_TYPE_ACPI
> > > > altogether: it is only used for EFI_ACPI_RECLAIM_MEMORY, which is
> > > > memory that can be used by the OS as ordinary RAM if it is not
> > > > interested in the contents (or has already consumed them). So this
> > > > could arguably be classified as E820_TYPE_RAM too.
> > >
> > > Hm. I'm not sure about this. E820_TYPE_ACPI also get tracked as
> > > IORES_DESC_ACPI_TABLES resource and get passed to the next kernel on
> > > kexec, regardless if it is crash kernel or not. I'm not sure we would not
> > > break anything.
> > >
> > 
> > Yeah, you're right. So this patch is necessary in any case.
> > 
> > Do we also need the EFI side patch then?
> 
> Yes, we need it to get it mapped into the crashkernel direct mapping.

Ughh. The patch alone causes crash as EFI_ACPI_RELACLAIM_MEMORY is not
mapped into direct mapping during memory init.

The patch below fixes the issue.

I should have noticed it before, but I had essentially the same patch in
my tree for a different reason. Sorry for that :/

Please apply the patch below.

From b5d1faf9e515195c58cc5e34132284894fca17f2 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Mon, 14 Aug 2023 19:12:47 +0300
Subject: [PATCH] efi/unaccepted: Make sure unaccepted table is mapped in
 crashkernel case

Unaccepted table is now allocated from EFI_ACPI_RELACLAIM_MEMORY. It
translates into E820_TYPE_ACPI, which is not added to memblock and
therefore not mapped in the direct mapping.

It causes crash on the first touch of the table.

Use memblock_add() to make sure that the table is mapped in direct
mapping.

Align the range to the nearest page boarders. Ranges smaller than page
size are not going to be mapped.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Fixes: e7761d827e99 ("efi/unaccepted: Use ACPI reclaim memory for unaccepted memory table")
---
 drivers/firmware/efi/efi.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 1599f1176842..4f409652b3c6 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -623,6 +623,34 @@ static __init int match_config_table(const efi_guid_t *guid,
 	return 0;
 }
 
+/**
+ * reserve_unaccepted - Map and reserve unaccepted configuration table
+ * @unaccepted: Pointer to unaccepted memory table
+ *
+ * memblock_add() makes sure that the table is mapped in direct mapping. During
+ * normal boot it happens automatically because the table is allocated from
+ * usable memory. But during crashkernel boot only memory specifically
+ * reserved for crash scenario is mapped. memblock_add() forces the table to be
+ * mapped in crashkernel case.
+ *
+ * Align the range to the nearest page boarders. Ranges smaller than page size
+ * are not going to be mapped.
+ *
+ * memblock_reserve() makes sure that future allocations will not touch the
+ * table.
+ */
+
+static __init void reserve_unaccepted(struct efi_unaccepted_memory *unaccepted)
+{
+	phys_addr_t start, size;
+
+	start = PAGE_ALIGN_DOWN(efi.unaccepted);
+	size = PAGE_ALIGN(sizeof(*unaccepted) + unaccepted->size);
+
+	memblock_add(start, size);
+	memblock_reserve(start, size);
+}
+
 int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 				   int count,
 				   const efi_config_table_type_t *arch_tables)
@@ -751,11 +779,9 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 
 		unaccepted = early_memremap(efi.unaccepted, sizeof(*unaccepted));
 		if (unaccepted) {
-			unsigned long size;
 
 			if (unaccepted->version == 1) {
-				size = sizeof(*unaccepted) + unaccepted->size;
-				memblock_reserve(efi.unaccepted, size);
+				reserve_unaccepted(unaccepted);
 			} else {
 				efi.unaccepted = EFI_INVALID_TABLE_ADDR;
 			}
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges

[Ard Biesheuvel]

On Sun, 17 Sept 2023 at 19:06, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> On Thu, Sep 07, 2023 at 01:49:14PM +0300, Kirill A. Shutemov wrote:
> > On Wed, Sep 06, 2023 at 11:17:12AM +0200, Ard Biesheuvel wrote:
> > > On Fri, 18 Aug 2023 at 17:16, Kirill A. Shutemov
> > > <kirill.shutemov@linux.intel.com> wrote:
> > > >
> > > > On Thu, Aug 17, 2023 at 10:25:56PM +0200, Ard Biesheuvel wrote:
> > > > > On Wed, 16 Aug 2023 at 23:24, Kirill A. Shutemov
> > > > > <kirill.shutemov@linux.intel.com> wrote:
> > > > > >
> > > > > > e820__end_of_ram_pfn() is used to calculate max_pfn which, among other
> > > > > > things, guides where direct mapping ends. Any memory above max_pfn is
> > > > > > not going to be present in the direct mapping.
> > > > > >
> > > > > > e820__end_of_ram_pfn() finds the end of the ram based on the highest
> > > > > > E820_TYPE_RAM range. But it doesn't includes E820_TYPE_ACPI ranges into
> > > > > > calculation.
> > > > > >
> > > > > > Despite the name, E820_TYPE_ACPI covers not only ACPI data, but also EFI
> > > > > > tables and might be required by kernel to function properly.
> > > > > >
> > > > > > Usually the problem is hidden because there is some E820_TYPE_RAM memory
> > > > > > above E820_TYPE_ACPI. But crashkernel only presents pre-allocated crash
> > > > > > memory as E820_TYPE_RAM on boot. If the preallocated range is small, it
> > > > > > can fit under the last E820_TYPE_ACPI range.
> > > > > >
> > > > > > Modify e820__end_of_ram_pfn() and e820__end_of_low_ram_pfn() to cover
> > > > > > E820_TYPE_ACPI memory.
> > > > > >
> > > > > > The problem was discovered during debugging kexec for TDX guest. TDX
> > > > > > guest uses E820_TYPE_ACPI to store the unaccepted memory bitmap and pass
> > > > > > it between the kernels on kexec.
> > > > > >
> > > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > > >
> > > > > No objections to this, but we might also simply drop E820_TYPE_ACPI
> > > > > altogether: it is only used for EFI_ACPI_RECLAIM_MEMORY, which is
> > > > > memory that can be used by the OS as ordinary RAM if it is not
> > > > > interested in the contents (or has already consumed them). So this
> > > > > could arguably be classified as E820_TYPE_RAM too.
> > > >
> > > > Hm. I'm not sure about this. E820_TYPE_ACPI also get tracked as
> > > > IORES_DESC_ACPI_TABLES resource and get passed to the next kernel on
> > > > kexec, regardless if it is crash kernel or not. I'm not sure we would not
> > > > break anything.
> > > >
> > >
> > > Yeah, you're right. So this patch is necessary in any case.
> > >
> > > Do we also need the EFI side patch then?
> >
> > Yes, we need it to get it mapped into the crashkernel direct mapping.
>
> Ughh. The patch alone causes crash as EFI_ACPI_RELACLAIM_MEMORY is not
> mapped into direct mapping during memory init.
>

It would be good if this boot path could be covered by lkp@ as well -
currently, you are the only person testing this manually. The same
applies to SEV-SNP by the way - there is zero coverage except for the
manual testing that Boris or Tom Lendacky might do.

> The patch below fixes the issue.
>
> I should have noticed it before, but I had essentially the same patch in
> my tree for a different reason. Sorry for that :/
>

No worries - it shouldn't be only up to you to test this stuff. I
would have tested it myself if I had access to TDX hardware.

> Please apply the patch below.
>

OK, queued up now.



> From b5d1faf9e515195c58cc5e34132284894fca17f2 Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Date: Mon, 14 Aug 2023 19:12:47 +0300
> Subject: [PATCH] efi/unaccepted: Make sure unaccepted table is mapped in
>  crashkernel case
>
> Unaccepted table is now allocated from EFI_ACPI_RELACLAIM_MEMORY. It
> translates into E820_TYPE_ACPI, which is not added to memblock and
> therefore not mapped in the direct mapping.
>
> It causes crash on the first touch of the table.
>
> Use memblock_add() to make sure that the table is mapped in direct
> mapping.
>
> Align the range to the nearest page boarders. Ranges smaller than page
> size are not going to be mapped.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Fixes: e7761d827e99 ("efi/unaccepted: Use ACPI reclaim memory for unaccepted memory table")
> ---
>  drivers/firmware/efi/efi.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 1599f1176842..4f409652b3c6 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -623,6 +623,34 @@ static __init int match_config_table(const efi_guid_t *guid,
>         return 0;
>  }
>
> +/**
> + * reserve_unaccepted - Map and reserve unaccepted configuration table
> + * @unaccepted: Pointer to unaccepted memory table
> + *
> + * memblock_add() makes sure that the table is mapped in direct mapping. During
> + * normal boot it happens automatically because the table is allocated from
> + * usable memory. But during crashkernel boot only memory specifically
> + * reserved for crash scenario is mapped. memblock_add() forces the table to be
> + * mapped in crashkernel case.
> + *
> + * Align the range to the nearest page boarders. Ranges smaller than page size
> + * are not going to be mapped.
> + *
> + * memblock_reserve() makes sure that future allocations will not touch the
> + * table.
> + */
> +
> +static __init void reserve_unaccepted(struct efi_unaccepted_memory *unaccepted)
> +{
> +       phys_addr_t start, size;
> +
> +       start = PAGE_ALIGN_DOWN(efi.unaccepted);
> +       size = PAGE_ALIGN(sizeof(*unaccepted) + unaccepted->size);
> +
> +       memblock_add(start, size);
> +       memblock_reserve(start, size);
> +}
> +
>  int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
>                                    int count,
>                                    const efi_config_table_type_t *arch_tables)
> @@ -751,11 +779,9 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
>
>                 unaccepted = early_memremap(efi.unaccepted, sizeof(*unaccepted));
>                 if (unaccepted) {
> -                       unsigned long size;
>
>                         if (unaccepted->version == 1) {
> -                               size = sizeof(*unaccepted) + unaccepted->size;
> -                               memblock_reserve(efi.unaccepted, size);
> +                               reserve_unaccepted(unaccepted);
>                         } else {
>                                 efi.unaccepted = EFI_INVALID_TABLE_ADDR;
>                         }
> --
>   Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges

[Kirill A. Shutemov]

On Mon, Sep 18, 2023 at 09:01:27AM +0200, Ard Biesheuvel wrote:
> On Sun, 17 Sept 2023 at 19:06, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> >
> > On Thu, Sep 07, 2023 at 01:49:14PM +0300, Kirill A. Shutemov wrote:
> > > On Wed, Sep 06, 2023 at 11:17:12AM +0200, Ard Biesheuvel wrote:
> > > > On Fri, 18 Aug 2023 at 17:16, Kirill A. Shutemov
> > > > <kirill.shutemov@linux.intel.com> wrote:
> > > > >
> > > > > On Thu, Aug 17, 2023 at 10:25:56PM +0200, Ard Biesheuvel wrote:
> > > > > > On Wed, 16 Aug 2023 at 23:24, Kirill A. Shutemov
> > > > > > <kirill.shutemov@linux.intel.com> wrote:
> > > > > > >
> > > > > > > e820__end_of_ram_pfn() is used to calculate max_pfn which, among other
> > > > > > > things, guides where direct mapping ends. Any memory above max_pfn is
> > > > > > > not going to be present in the direct mapping.
> > > > > > >
> > > > > > > e820__end_of_ram_pfn() finds the end of the ram based on the highest
> > > > > > > E820_TYPE_RAM range. But it doesn't includes E820_TYPE_ACPI ranges into
> > > > > > > calculation.
> > > > > > >
> > > > > > > Despite the name, E820_TYPE_ACPI covers not only ACPI data, but also EFI
> > > > > > > tables and might be required by kernel to function properly.
> > > > > > >
> > > > > > > Usually the problem is hidden because there is some E820_TYPE_RAM memory
> > > > > > > above E820_TYPE_ACPI. But crashkernel only presents pre-allocated crash
> > > > > > > memory as E820_TYPE_RAM on boot. If the preallocated range is small, it
> > > > > > > can fit under the last E820_TYPE_ACPI range.
> > > > > > >
> > > > > > > Modify e820__end_of_ram_pfn() and e820__end_of_low_ram_pfn() to cover
> > > > > > > E820_TYPE_ACPI memory.
> > > > > > >
> > > > > > > The problem was discovered during debugging kexec for TDX guest. TDX
> > > > > > > guest uses E820_TYPE_ACPI to store the unaccepted memory bitmap and pass
> > > > > > > it between the kernels on kexec.
> > > > > > >
> > > > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > > > >
> > > > > > No objections to this, but we might also simply drop E820_TYPE_ACPI
> > > > > > altogether: it is only used for EFI_ACPI_RECLAIM_MEMORY, which is
> > > > > > memory that can be used by the OS as ordinary RAM if it is not
> > > > > > interested in the contents (or has already consumed them). So this
> > > > > > could arguably be classified as E820_TYPE_RAM too.
> > > > >
> > > > > Hm. I'm not sure about this. E820_TYPE_ACPI also get tracked as
> > > > > IORES_DESC_ACPI_TABLES resource and get passed to the next kernel on
> > > > > kexec, regardless if it is crash kernel or not. I'm not sure we would not
> > > > > break anything.
> > > > >
> > > >
> > > > Yeah, you're right. So this patch is necessary in any case.
> > > >
> > > > Do we also need the EFI side patch then?
> > >
> > > Yes, we need it to get it mapped into the crashkernel direct mapping.
> >
> > Ughh. The patch alone causes crash as EFI_ACPI_RELACLAIM_MEMORY is not
> > mapped into direct mapping during memory init.
> >
> 
> It would be good if this boot path could be covered by lkp@ as well -
> currently, you are the only person testing this manually. The same
> applies to SEV-SNP by the way - there is zero coverage except for the
> manual testing that Boris or Tom Lendacky might do.

It made me remember that I forgot attribute the finding:

Reported-by: "Hongyu Ning" <hongyu.ning@intel.com>

Hongyu tests linux-next periodically, but I agree we need to get LKP
working.

I will try to get LKP going for TDX guest.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges

[Ard Biesheuvel]

On Mon, 18 Sept 2023 at 14:13, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> On Mon, Sep 18, 2023 at 09:01:27AM +0200, Ard Biesheuvel wrote:
> > On Sun, 17 Sept 2023 at 19:06, Kirill A. Shutemov
> > <kirill.shutemov@linux.intel.com> wrote:
> > >
> > > On Thu, Sep 07, 2023 at 01:49:14PM +0300, Kirill A. Shutemov wrote:
> > > > On Wed, Sep 06, 2023 at 11:17:12AM +0200, Ard Biesheuvel wrote:
> > > > > On Fri, 18 Aug 2023 at 17:16, Kirill A. Shutemov
> > > > > <kirill.shutemov@linux.intel.com> wrote:
> > > > > >
> > > > > > On Thu, Aug 17, 2023 at 10:25:56PM +0200, Ard Biesheuvel wrote:
> > > > > > > On Wed, 16 Aug 2023 at 23:24, Kirill A. Shutemov
> > > > > > > <kirill.shutemov@linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > e820__end_of_ram_pfn() is used to calculate max_pfn which, among other
> > > > > > > > things, guides where direct mapping ends. Any memory above max_pfn is
> > > > > > > > not going to be present in the direct mapping.
> > > > > > > >
> > > > > > > > e820__end_of_ram_pfn() finds the end of the ram based on the highest
> > > > > > > > E820_TYPE_RAM range. But it doesn't includes E820_TYPE_ACPI ranges into
> > > > > > > > calculation.
> > > > > > > >
> > > > > > > > Despite the name, E820_TYPE_ACPI covers not only ACPI data, but also EFI
> > > > > > > > tables and might be required by kernel to function properly.
> > > > > > > >
> > > > > > > > Usually the problem is hidden because there is some E820_TYPE_RAM memory
> > > > > > > > above E820_TYPE_ACPI. But crashkernel only presents pre-allocated crash
> > > > > > > > memory as E820_TYPE_RAM on boot. If the preallocated range is small, it
> > > > > > > > can fit under the last E820_TYPE_ACPI range.
> > > > > > > >
> > > > > > > > Modify e820__end_of_ram_pfn() and e820__end_of_low_ram_pfn() to cover
> > > > > > > > E820_TYPE_ACPI memory.
> > > > > > > >
> > > > > > > > The problem was discovered during debugging kexec for TDX guest. TDX
> > > > > > > > guest uses E820_TYPE_ACPI to store the unaccepted memory bitmap and pass
> > > > > > > > it between the kernels on kexec.
> > > > > > > >
> > > > > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > > > > >
> > > > > > > No objections to this, but we might also simply drop E820_TYPE_ACPI
> > > > > > > altogether: it is only used for EFI_ACPI_RECLAIM_MEMORY, which is
> > > > > > > memory that can be used by the OS as ordinary RAM if it is not
> > > > > > > interested in the contents (or has already consumed them). So this
> > > > > > > could arguably be classified as E820_TYPE_RAM too.
> > > > > >
> > > > > > Hm. I'm not sure about this. E820_TYPE_ACPI also get tracked as
> > > > > > IORES_DESC_ACPI_TABLES resource and get passed to the next kernel on
> > > > > > kexec, regardless if it is crash kernel or not. I'm not sure we would not
> > > > > > break anything.
> > > > > >
> > > > >
> > > > > Yeah, you're right. So this patch is necessary in any case.
> > > > >
> > > > > Do we also need the EFI side patch then?
> > > >
> > > > Yes, we need it to get it mapped into the crashkernel direct mapping.
> > >
> > > Ughh. The patch alone causes crash as EFI_ACPI_RELACLAIM_MEMORY is not
> > > mapped into direct mapping during memory init.
> > >
> >
> > It would be good if this boot path could be covered by lkp@ as well -
> > currently, you are the only person testing this manually. The same
> > applies to SEV-SNP by the way - there is zero coverage except for the
> > manual testing that Boris or Tom Lendacky might do.
>
> It made me remember that I forgot attribute the finding:
>
> Reported-by: "Hongyu Ning" <hongyu.ning@intel.com>
>

OK, I will add that next time I update the branch.

> Hongyu tests linux-next periodically, but I agree we need to get LKP
> working.
>
> I will try to get LKP going for TDX guest.
>

Yes, please. We are likely to run into such issues again if we don't
have test coverage for this functionality.

Thanks,
Ard.

Re: [PATCH] efi/unaccepted: Use ACPI reclaim memory for unaccepted memory table

[Tom Lendacky]

On 8/16/23 14:05, Ard Biesheuvel wrote:
> Kyril reports that crashkernels fail to work on confidential VMs that
> rely on the unaccepted memory table, and this appears to be caused by
> the fact that it is not considered part of the set of firmware tables
> that the crashkernel needs to map.
> 
> This is an oversight, and a result of the use of the EFI_LOADER_DATA
> memory type for this table. The correct memory type to use for any
> firmware table is EFI_ACPI_RECLAIM_MEMORY (including ones created by the
> EFI stub), even though the name suggests that is it specific to ACPI.
> ACPI reclaim means that the memory is used by the firmware to expose
> information to the operating system, but that the memory region has no
> special significance to the firmware itself, and the OS is free to
> reclaim the memory and use it as ordinary memory if it is not interested
> in the contents, or if it has already consumed them. In Linux, this
> memory is never reclaimed, but it is always covered by the kernel direct
> map and generally made accessible as ordinary memory.
> 
> On x86, ACPI reclaim memory is translated into E820_ACPI, which the
> kexec logic already recognizes as memory that the crashkernel may need
> to to access, and so it will be mapped and accessible to the booting
> crash kernel.
> 
> Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   drivers/firmware/efi/libstub/unaccepted_memory.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
> index ca61f4733ea58693..9a655f30ba47db01 100644
> --- a/drivers/firmware/efi/libstub/unaccepted_memory.c
> +++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
> @@ -62,7 +62,7 @@ efi_status_t allocate_unaccepted_bitmap(__u32 nr_desc,
>   	bitmap_size = DIV_ROUND_UP(unaccepted_end - unaccepted_start,
>   				   EFI_UNACCEPTED_UNIT_SIZE * BITS_PER_BYTE);
>   
> -	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
> +	status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,

I bisected an SNP guest crash when using the tip tree to this commit. When
the kernel switches over to the swapper_pg_dir in init_mem_mapping(), the
unaccepted table is no longer mapped. Here's a copy of the stack trace:

[    0.074233] *** DEBUG: accept_memory:36 - unaccepted=0xffff88807f77ef18
[    0.075805] BUG: unable to handle page fault for address: ffff88807f77ef1c
[    0.076541] #PF: supervisor read access in kernel mode
[    0.077089] #PF: error_code(0x0000) - not-present page
[    0.077631] PGD 8000004c01067 P4D 8000004c01067 PUD 800017fffe067 PMD 8000004c04067 PTE 0
[    0.078498] Oops: 0000 [#1] PREEMPT SMP NOPTI
[    0.078967] CPU: 0 PID: 0 Comm: swapper Not tainted 6.6.0-rc2-sos-sev #12
[    0.079682] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS unknown 2/2/2022
[    0.080507] RIP: 0010:accept_memory+0x75/0x250
[    0.080984] Code: 74 4a 4c 89 e1 ba 24 00 00 00 48 c7 c6 68 9e 10 82 48 c7 c7 50 9e 53 82 e8 48 92 79 ff 4c 89 e7 e8 50 b5 6e ff ba 2c 00 00 00 <45> 8b 74 24 04 48 c7 c6 68 9e 10 82 48 c7 c7 3d 9e 53 82 e8 23 92
[    0.082969] RSP: 0000:ffffffff82803df8 EFLAGS: 00010046
[    0.083518] RAX: 0000000000000000 RBX: 000000017fffd000 RCX: 0000000000000000
[    0.084271] RDX: 000000000000002c RSI: ffffffff82803cc0 RDI: 00000000ffffffff
[    0.085029] RBP: 0000000000000000 R08: 00000000ffff7fff R09: 0000000000000001
[    0.085786] R10: 00000000ffff7fff R11: ffffffff8286f9e0 R12: ffff88807f77ef18
[    0.086537] R13: 000000017ffd2000 R14: 0000000000000000 R15: 000000017ffd2000
[    0.087293] FS:  0000000000000000(0000) GS:ffffffff83273000(0000) knlGS:0000000000000000
[    0.088154] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.088764] CR2: ffff88807f77ef1c CR3: 000800000382e000 CR4: 00000000000000f0
[    0.089520] Call Trace:
[    0.089783]  <TASK>
[    0.090005]  ? __die+0x1f/0x70
[    0.090334]  ? page_fault_oops+0x81/0x150
[    0.090780]  ? kernelmode_fixup_or_oops+0x84/0x110
[    0.091287]  ? exc_page_fault+0xa8/0x150
[    0.091705]  ? asm_exc_page_fault+0x22/0x30
[    0.092161]  ? accept_memory+0x75/0x250
[    0.092564]  ? accept_memory+0x70/0x250
[    0.092971]  ? memblock_alloc_range_nid+0xf4/0x160
[    0.093479]  ? numa_register_memblks.constprop.0+0x286/0x3a0
[    0.094079]  ? __pfx_dummy_numa_init+0x10/0x10
[    0.094548]  ? numa_init+0x102/0x2a0
[    0.094929]  ? setup_arch+0xc58/0x1010
[    0.095326]  ? start_kernel+0x5e/0x5e0
[    0.095728]  ? x86_64_start_reservations+0x14/0x30
[    0.096235]  ? x86_64_start_kernel+0x79/0x80
[    0.096688]  ? secondary_startup_64_no_verify+0x16b/0x16b
[    0.097269]  </TASK>
[    0.097499] Modules linked in:
[    0.097826] CR2: ffff88807f77ef1c
[    0.098178] ---[ end trace 0000000000000000 ]---

Thanks,
Tom

>   			     sizeof(*unaccepted_table) + bitmap_size,
>   			     (void **)&unaccepted_table);
>   	if (status != EFI_SUCCESS) {

Re: [PATCH] efi/unaccepted: Use ACPI reclaim memory for unaccepted memory table

[Kirill A . Shutemov]

On Mon, Sep 18, 2023 at 05:03:59PM -0500, Tom Lendacky wrote:
> On 8/16/23 14:05, Ard Biesheuvel wrote:
> > Kyril reports that crashkernels fail to work on confidential VMs that
> > rely on the unaccepted memory table, and this appears to be caused by
> > the fact that it is not considered part of the set of firmware tables
> > that the crashkernel needs to map.
> > 
> > This is an oversight, and a result of the use of the EFI_LOADER_DATA
> > memory type for this table. The correct memory type to use for any
> > firmware table is EFI_ACPI_RECLAIM_MEMORY (including ones created by the
> > EFI stub), even though the name suggests that is it specific to ACPI.
> > ACPI reclaim means that the memory is used by the firmware to expose
> > information to the operating system, but that the memory region has no
> > special significance to the firmware itself, and the OS is free to
> > reclaim the memory and use it as ordinary memory if it is not interested
> > in the contents, or if it has already consumed them. In Linux, this
> > memory is never reclaimed, but it is always covered by the kernel direct
> > map and generally made accessible as ordinary memory.
> > 
> > On x86, ACPI reclaim memory is translated into E820_ACPI, which the
> > kexec logic already recognizes as memory that the crashkernel may need
> > to to access, and so it will be mapped and accessible to the booting
> > crash kernel.
> > 
> > Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >   drivers/firmware/efi/libstub/unaccepted_memory.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
> > index ca61f4733ea58693..9a655f30ba47db01 100644
> > --- a/drivers/firmware/efi/libstub/unaccepted_memory.c
> > +++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
> > @@ -62,7 +62,7 @@ efi_status_t allocate_unaccepted_bitmap(__u32 nr_desc,
> >   	bitmap_size = DIV_ROUND_UP(unaccepted_end - unaccepted_start,
> >   				   EFI_UNACCEPTED_UNIT_SIZE * BITS_PER_BYTE);
> > -	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
> > +	status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
> 
> I bisected an SNP guest crash when using the tip tree to this commit. When
> the kernel switches over to the swapper_pg_dir in init_mem_mapping(), the
> unaccepted table is no longer mapped. Here's a copy of the stack trace:

Could you try this patch:
 https://lore.kernel.org/all/20230917170629.d35gnwb6o54bdrhl@box.shutemov.name

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] efi/unaccepted: Use ACPI reclaim memory for unaccepted memory table

[Tom Lendacky]

On 9/18/23 17:10, Kirill A . Shutemov wrote:
> On Mon, Sep 18, 2023 at 05:03:59PM -0500, Tom Lendacky wrote:
>> On 8/16/23 14:05, Ard Biesheuvel wrote:
>>> Kyril reports that crashkernels fail to work on confidential VMs that
>>> rely on the unaccepted memory table, and this appears to be caused by
>>> the fact that it is not considered part of the set of firmware tables
>>> that the crashkernel needs to map.
>>>
>>> This is an oversight, and a result of the use of the EFI_LOADER_DATA
>>> memory type for this table. The correct memory type to use for any
>>> firmware table is EFI_ACPI_RECLAIM_MEMORY (including ones created by the
>>> EFI stub), even though the name suggests that is it specific to ACPI.
>>> ACPI reclaim means that the memory is used by the firmware to expose
>>> information to the operating system, but that the memory region has no
>>> special significance to the firmware itself, and the OS is free to
>>> reclaim the memory and use it as ordinary memory if it is not interested
>>> in the contents, or if it has already consumed them. In Linux, this
>>> memory is never reclaimed, but it is always covered by the kernel direct
>>> map and generally made accessible as ordinary memory.
>>>
>>> On x86, ACPI reclaim memory is translated into E820_ACPI, which the
>>> kexec logic already recognizes as memory that the crashkernel may need
>>> to to access, and so it will be mapped and accessible to the booting
>>> crash kernel.
>>>
>>> Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>>    drivers/firmware/efi/libstub/unaccepted_memory.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
>>> index ca61f4733ea58693..9a655f30ba47db01 100644
>>> --- a/drivers/firmware/efi/libstub/unaccepted_memory.c
>>> +++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
>>> @@ -62,7 +62,7 @@ efi_status_t allocate_unaccepted_bitmap(__u32 nr_desc,
>>>    	bitmap_size = DIV_ROUND_UP(unaccepted_end - unaccepted_start,
>>>    				   EFI_UNACCEPTED_UNIT_SIZE * BITS_PER_BYTE);
>>> -	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
>>> +	status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
>>
>> I bisected an SNP guest crash when using the tip tree to this commit. When
>> the kernel switches over to the swapper_pg_dir in init_mem_mapping(), the
>> unaccepted table is no longer mapped. Here's a copy of the stack trace:
> 
> Could you try this patch:
>   https://lore.kernel.org/all/20230917170629.d35gnwb6o54bdrhl@box.shutemov.name

Yes, that fixes the issue for me.

Thanks,
Tom
> 

Re: [PATCH] efi/unaccepted: Use ACPI reclaim memory for unaccepted memory table

[Ard Biesheuvel]

On Tue, 19 Sept 2023 at 15:28, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 9/18/23 17:10, Kirill A . Shutemov wrote:
> > On Mon, Sep 18, 2023 at 05:03:59PM -0500, Tom Lendacky wrote:
> >> On 8/16/23 14:05, Ard Biesheuvel wrote:
> >>> Kyril reports that crashkernels fail to work on confidential VMs that
> >>> rely on the unaccepted memory table, and this appears to be caused by
> >>> the fact that it is not considered part of the set of firmware tables
> >>> that the crashkernel needs to map.
> >>>
> >>> This is an oversight, and a result of the use of the EFI_LOADER_DATA
> >>> memory type for this table. The correct memory type to use for any
> >>> firmware table is EFI_ACPI_RECLAIM_MEMORY (including ones created by the
> >>> EFI stub), even though the name suggests that is it specific to ACPI.
> >>> ACPI reclaim means that the memory is used by the firmware to expose
> >>> information to the operating system, but that the memory region has no
> >>> special significance to the firmware itself, and the OS is free to
> >>> reclaim the memory and use it as ordinary memory if it is not interested
> >>> in the contents, or if it has already consumed them. In Linux, this
> >>> memory is never reclaimed, but it is always covered by the kernel direct
> >>> map and generally made accessible as ordinary memory.
> >>>
> >>> On x86, ACPI reclaim memory is translated into E820_ACPI, which the
> >>> kexec logic already recognizes as memory that the crashkernel may need
> >>> to to access, and so it will be mapped and accessible to the booting
> >>> crash kernel.
> >>>
> >>> Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>> ---
> >>>    drivers/firmware/efi/libstub/unaccepted_memory.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
> >>> index ca61f4733ea58693..9a655f30ba47db01 100644
> >>> --- a/drivers/firmware/efi/libstub/unaccepted_memory.c
> >>> +++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
> >>> @@ -62,7 +62,7 @@ efi_status_t allocate_unaccepted_bitmap(__u32 nr_desc,
> >>>     bitmap_size = DIV_ROUND_UP(unaccepted_end - unaccepted_start,
> >>>                                EFI_UNACCEPTED_UNIT_SIZE * BITS_PER_BYTE);
> >>> -   status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
> >>> +   status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
> >>
> >> I bisected an SNP guest crash when using the tip tree to this commit. When
> >> the kernel switches over to the swapper_pg_dir in init_mem_mapping(), the
> >> unaccepted table is no longer mapped. Here's a copy of the stack trace:
> >
> > Could you try this patch:
> >   https://lore.kernel.org/all/20230917170629.d35gnwb6o54bdrhl@box.shutemov.name
>
> Yes, that fixes the issue for me.
>

Thanks for confirming. I've queued it up already, I will send it out
later this week.

Re: [PATCH] efi/unaccepted: Use ACPI reclaim memory for unaccepted memory table

[Ard Biesheuvel]

On Tue, 19 Sept 2023 at 13:37, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 19 Sept 2023 at 15:28, Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >
> > On 9/18/23 17:10, Kirill A . Shutemov wrote:
> > > On Mon, Sep 18, 2023 at 05:03:59PM -0500, Tom Lendacky wrote:
> > >> On 8/16/23 14:05, Ard Biesheuvel wrote:
> > >>> Kyril reports that crashkernels fail to work on confidential VMs that
> > >>> rely on the unaccepted memory table, and this appears to be caused by
> > >>> the fact that it is not considered part of the set of firmware tables
> > >>> that the crashkernel needs to map.
> > >>>
> > >>> This is an oversight, and a result of the use of the EFI_LOADER_DATA
> > >>> memory type for this table. The correct memory type to use for any
> > >>> firmware table is EFI_ACPI_RECLAIM_MEMORY (including ones created by the
> > >>> EFI stub), even though the name suggests that is it specific to ACPI.
> > >>> ACPI reclaim means that the memory is used by the firmware to expose
> > >>> information to the operating system, but that the memory region has no
> > >>> special significance to the firmware itself, and the OS is free to
> > >>> reclaim the memory and use it as ordinary memory if it is not interested
> > >>> in the contents, or if it has already consumed them. In Linux, this
> > >>> memory is never reclaimed, but it is always covered by the kernel direct
> > >>> map and generally made accessible as ordinary memory.
> > >>>
> > >>> On x86, ACPI reclaim memory is translated into E820_ACPI, which the
> > >>> kexec logic already recognizes as memory that the crashkernel may need
> > >>> to to access, and so it will be mapped and accessible to the booting
> > >>> crash kernel.
> > >>>
> > >>> Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > >>> ---
> > >>>    drivers/firmware/efi/libstub/unaccepted_memory.c | 2 +-
> > >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
> > >>> index ca61f4733ea58693..9a655f30ba47db01 100644
> > >>> --- a/drivers/firmware/efi/libstub/unaccepted_memory.c
> > >>> +++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
> > >>> @@ -62,7 +62,7 @@ efi_status_t allocate_unaccepted_bitmap(__u32 nr_desc,
> > >>>     bitmap_size = DIV_ROUND_UP(unaccepted_end - unaccepted_start,
> > >>>                                EFI_UNACCEPTED_UNIT_SIZE * BITS_PER_BYTE);
> > >>> -   status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
> > >>> +   status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
> > >>
> > >> I bisected an SNP guest crash when using the tip tree to this commit. When
> > >> the kernel switches over to the swapper_pg_dir in init_mem_mapping(), the
> > >> unaccepted table is no longer mapped. Here's a copy of the stack trace:
> > >
> > > Could you try this patch:
> > >   https://lore.kernel.org/all/20230917170629.d35gnwb6o54bdrhl@box.shutemov.name
> >
> > Yes, that fixes the issue for me.
> >
>
> Thanks for confirming. I've queued it up already, I will send it out
> later this week.

Tom,

Could you confirm whether or not this is related to loading a crashkernel?

Re: [PATCH] efi/unaccepted: Use ACPI reclaim memory for unaccepted memory table

[Tom Lendacky]

On 9/19/23 10:04, Ard Biesheuvel wrote:
> On Tue, 19 Sept 2023 at 13:37, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Tue, 19 Sept 2023 at 15:28, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>>
>>> On 9/18/23 17:10, Kirill A . Shutemov wrote:
>>>> On Mon, Sep 18, 2023 at 05:03:59PM -0500, Tom Lendacky wrote:
>>>>> On 8/16/23 14:05, Ard Biesheuvel wrote:
>>>>>> Kyril reports that crashkernels fail to work on confidential VMs that
>>>>>> rely on the unaccepted memory table, and this appears to be caused by
>>>>>> the fact that it is not considered part of the set of firmware tables
>>>>>> that the crashkernel needs to map.
>>>>>>
>>>>>> This is an oversight, and a result of the use of the EFI_LOADER_DATA
>>>>>> memory type for this table. The correct memory type to use for any
>>>>>> firmware table is EFI_ACPI_RECLAIM_MEMORY (including ones created by the
>>>>>> EFI stub), even though the name suggests that is it specific to ACPI.
>>>>>> ACPI reclaim means that the memory is used by the firmware to expose
>>>>>> information to the operating system, but that the memory region has no
>>>>>> special significance to the firmware itself, and the OS is free to
>>>>>> reclaim the memory and use it as ordinary memory if it is not interested
>>>>>> in the contents, or if it has already consumed them. In Linux, this
>>>>>> memory is never reclaimed, but it is always covered by the kernel direct
>>>>>> map and generally made accessible as ordinary memory.
>>>>>>
>>>>>> On x86, ACPI reclaim memory is translated into E820_ACPI, which the
>>>>>> kexec logic already recognizes as memory that the crashkernel may need
>>>>>> to to access, and so it will be mapped and accessible to the booting
>>>>>> crash kernel.
>>>>>>
>>>>>> Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>>>> ---
>>>>>>     drivers/firmware/efi/libstub/unaccepted_memory.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
>>>>>> index ca61f4733ea58693..9a655f30ba47db01 100644
>>>>>> --- a/drivers/firmware/efi/libstub/unaccepted_memory.c
>>>>>> +++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
>>>>>> @@ -62,7 +62,7 @@ efi_status_t allocate_unaccepted_bitmap(__u32 nr_desc,
>>>>>>      bitmap_size = DIV_ROUND_UP(unaccepted_end - unaccepted_start,
>>>>>>                                 EFI_UNACCEPTED_UNIT_SIZE * BITS_PER_BYTE);
>>>>>> -   status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
>>>>>> +   status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
>>>>>
>>>>> I bisected an SNP guest crash when using the tip tree to this commit. When
>>>>> the kernel switches over to the swapper_pg_dir in init_mem_mapping(), the
>>>>> unaccepted table is no longer mapped. Here's a copy of the stack trace:
>>>>
>>>> Could you try this patch:
>>>>    https://lore.kernel.org/all/20230917170629.d35gnwb6o54bdrhl@box.shutemov.name
>>>
>>> Yes, that fixes the issue for me.
>>>
>>
>> Thanks for confirming. I've queued it up already, I will send it out
>> later this week.
> 
> Tom,
> 
> Could you confirm whether or not this is related to loading a crashkernel?

This was experienced just booting an SNP guest, it was not related to 
loading a crashkernel.

Thanks,
Tom