All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] arm/efi: fix ram base detection
@ 2021-03-12 16:54 Vincent Stehlé
  2021-03-22 16:00 ` Daniel Kiper
  0 siblings, 1 reply; 5+ messages in thread
From: Vincent Stehlé @ 2021-03-12 16:54 UTC (permalink / raw)
  To: grub-devel; +Cc: Vincent Stehlé, Grant Likely, Peter Jones, Leif Lindholm

On 32b Arm platforms, grub allocates memory for the initrd in the first
512MB of DRAM. To do so, the grub_efi_get_ram_base() function will be
called to compute the DRAM base. Currently this function returns the lowest
start address of all memory regions with attribute write-back.

However, if for example a small memory region with type reserved and
attribute write-back is present at the bottom of the memory map, it will be
chosen as DRAM base and initrd memory allocation will fail with:

  error: out of memory.

  Press any key to continue...

This is indeed the case with qemu arm machine virt when the secure world is
enabled and TF-A and OP-TEE are used. The secure world firmware will
reserve secure memory, resulting in the following EFI memory map:

  Type      Physical start  - end             #Pages        Size Attributes
  reserved  000000000e100000-000000000effffff 00000f00     15MiB WB
  conv-mem  0000000040000000-0000000047ef9fff 00007efa 130024KiB WB
  ACPI-rec  0000000047efa000-0000000047f05fff 0000000c     48KiB WB
  conv-mem  0000000047f06000-000000006d4f9fff 000255f4 612304KiB WB
  ldr-data  000000006d4fa000-000000006d4fafff 00000001      4KiB WB
 ...

In this case, the DRAM base is computed as 0xe100000, while it should be
0x40000000 instead.

Fix this issue by considering only conventional memory with attribute
write-back for DRAM base computation.

This is similar to what is done by Peter Jones in commit 3c1a5d940be5
("arm/arm64 loader: Better memory allocation and error messages.") in
Fedora's grub[1]. This patch reduces the modifications to a minimum.

[1]: https://github.com/rhboot/grub2.git

Fixes: bad144c60f66 ("efi: Add grub_efi_get_ram_base() function for arm64")
Suggested-by: Grant Likely <grant.likely@arm.com>
Signed-off-by: Vincent Stehlé <vincent.stehle@laposte.net>
Cc: Peter Jones <pjones@redhat.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/kern/efi/mm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 0cdb063bb..abf8772bc 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -677,7 +677,8 @@ grub_efi_get_ram_base(grub_addr_t *base_addr)
   for (desc = memory_map, *base_addr = GRUB_EFI_MAX_USABLE_ADDRESS;
        (grub_addr_t) desc < ((grub_addr_t) memory_map + memory_map_size);
        desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
-    if (desc->attribute & GRUB_EFI_MEMORY_WB)
+    if (desc->type == GRUB_EFI_CONVENTIONAL_MEMORY &&
+        desc->attribute & GRUB_EFI_MEMORY_WB)
       *base_addr = grub_min (*base_addr, desc->physical_start);
 
   grub_free(memory_map);
-- 
2.30.0



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] arm/efi: fix ram base detection
  2021-03-12 16:54 [PATCH] arm/efi: fix ram base detection Vincent Stehlé
@ 2021-03-22 16:00 ` Daniel Kiper
  2021-03-22 18:28   ` Leif Lindholm
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Kiper @ 2021-03-22 16:00 UTC (permalink / raw)
  To: Vincent Stehlé; +Cc: grub-devel, Grant Likely, Peter Jones, Leif Lindholm

On Fri, Mar 12, 2021 at 05:54:55PM +0100, Vincent Stehlé via Grub-devel wrote:
> On 32b Arm platforms, grub allocates memory for the initrd in the first
> 512MB of DRAM. To do so, the grub_efi_get_ram_base() function will be
> called to compute the DRAM base. Currently this function returns the lowest
> start address of all memory regions with attribute write-back.
>
> However, if for example a small memory region with type reserved and
> attribute write-back is present at the bottom of the memory map, it will be
> chosen as DRAM base and initrd memory allocation will fail with:
>
>   error: out of memory.
>
>   Press any key to continue...
>
> This is indeed the case with qemu arm machine virt when the secure world is
> enabled and TF-A and OP-TEE are used. The secure world firmware will
> reserve secure memory, resulting in the following EFI memory map:
>
>   Type      Physical start  - end             #Pages        Size Attributes
>   reserved  000000000e100000-000000000effffff 00000f00     15MiB WB
>   conv-mem  0000000040000000-0000000047ef9fff 00007efa 130024KiB WB
>   ACPI-rec  0000000047efa000-0000000047f05fff 0000000c     48KiB WB
>   conv-mem  0000000047f06000-000000006d4f9fff 000255f4 612304KiB WB
>   ldr-data  000000006d4fa000-000000006d4fafff 00000001      4KiB WB
>  ...
>
> In this case, the DRAM base is computed as 0xe100000, while it should be
> 0x40000000 instead.
>
> Fix this issue by considering only conventional memory with attribute
> write-back for DRAM base computation.
>
> This is similar to what is done by Peter Jones in commit 3c1a5d940be5
> ("arm/arm64 loader: Better memory allocation and error messages.") in
> Fedora's grub[1]. This patch reduces the modifications to a minimum.
>
> [1]: https://github.com/rhboot/grub2.git
>
> Fixes: bad144c60f66 ("efi: Add grub_efi_get_ram_base() function for arm64")
> Suggested-by: Grant Likely <grant.likely@arm.com>
> Signed-off-by: Vincent Stehlé <vincent.stehle@laposte.net>
> Cc: Peter Jones <pjones@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>

s/leif.lindholm@linaro.org/leif@nuviainc.com/

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

> ---
>  grub-core/kern/efi/mm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index 0cdb063bb..abf8772bc 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -677,7 +677,8 @@ grub_efi_get_ram_base(grub_addr_t *base_addr)
>    for (desc = memory_map, *base_addr = GRUB_EFI_MAX_USABLE_ADDRESS;
>         (grub_addr_t) desc < ((grub_addr_t) memory_map + memory_map_size);
>         desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
> -    if (desc->attribute & GRUB_EFI_MEMORY_WB)
> +    if (desc->type == GRUB_EFI_CONVENTIONAL_MEMORY &&
> +        desc->attribute & GRUB_EFI_MEMORY_WB)
>        *base_addr = grub_min (*base_addr, desc->physical_start);
>
>    grub_free(memory_map);
> --
> 2.30.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] arm/efi: fix ram base detection
  2021-03-22 16:00 ` Daniel Kiper
@ 2021-03-22 18:28   ` Leif Lindholm
  2021-03-30 20:01     ` Vincent Stehlé
  0 siblings, 1 reply; 5+ messages in thread
From: Leif Lindholm @ 2021-03-22 18:28 UTC (permalink / raw)
  To: Daniel Kiper; +Cc: Vincent Stehlé, grub-devel, Grant Likely, Peter Jones

On Mon, Mar 22, 2021 at 17:00:15 +0100, Daniel Kiper wrote:
> On Fri, Mar 12, 2021 at 05:54:55PM +0100, Vincent Stehlé via Grub-devel wrote:
> > On 32b Arm platforms, grub allocates memory for the initrd in the first
> > 512MB of DRAM. To do so, the grub_efi_get_ram_base() function will be
> > called to compute the DRAM base. Currently this function returns the lowest
> > start address of all memory regions with attribute write-back.
> >
> > However, if for example a small memory region with type reserved and
> > attribute write-back is present at the bottom of the memory map, it will be
> > chosen as DRAM base and initrd memory allocation will fail with:
> >
> >   error: out of memory.
> >
> >   Press any key to continue...
> >
> > This is indeed the case with qemu arm machine virt when the secure world is
> > enabled and TF-A and OP-TEE are used. The secure world firmware will
> > reserve secure memory, resulting in the following EFI memory map:
> >
> >   Type      Physical start  - end             #Pages        Size Attributes
> >   reserved  000000000e100000-000000000effffff 00000f00     15MiB WB
> >   conv-mem  0000000040000000-0000000047ef9fff 00007efa 130024KiB WB
> >   ACPI-rec  0000000047efa000-0000000047f05fff 0000000c     48KiB WB
> >   conv-mem  0000000047f06000-000000006d4f9fff 000255f4 612304KiB WB
> >   ldr-data  000000006d4fa000-000000006d4fafff 00000001      4KiB WB
> >  ...
> >
> > In this case, the DRAM base is computed as 0xe100000, while it should be
> > 0x40000000 instead.
> >
> > Fix this issue by considering only conventional memory with attribute
> > write-back for DRAM base computation.
> >
> > This is similar to what is done by Peter Jones in commit 3c1a5d940be5
> > ("arm/arm64 loader: Better memory allocation and error messages.") in
> > Fedora's grub[1]. This patch reduces the modifications to a minimum.
> >
> > [1]: https://github.com/rhboot/grub2.git
> >
> > Fixes: bad144c60f66 ("efi: Add grub_efi_get_ram_base() function for arm64")
> > Suggested-by: Grant Likely <grant.likely@arm.com>
> > Signed-off-by: Vincent Stehlé <vincent.stehle@laposte.net>
> > Cc: Peter Jones <pjones@redhat.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> 
> s/leif.lindholm@linaro.org/leif@nuviainc.com/

Thanks :)

> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> 
> > ---
> >  grub-core/kern/efi/mm.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index 0cdb063bb..abf8772bc 100644
> > --- a/grub-core/kern/efi/mm.c
> > +++ b/grub-core/kern/efi/mm.c
> > @@ -677,7 +677,8 @@ grub_efi_get_ram_base(grub_addr_t *base_addr)
> >    for (desc = memory_map, *base_addr = GRUB_EFI_MAX_USABLE_ADDRESS;
> >         (grub_addr_t) desc < ((grub_addr_t) memory_map + memory_map_size);
> >         desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
> > -    if (desc->attribute & GRUB_EFI_MEMORY_WB)
> > +    if (desc->type == GRUB_EFI_CONVENTIONAL_MEMORY &&
> > +        desc->attribute & GRUB_EFI_MEMORY_WB)

Can we safely assume we don't also need to check against
GRUB_EFI_PERSISTENT_MEMORY? If so, this is fine.

/
    Leif

> >        *base_addr = grub_min (*base_addr, desc->physical_start);
> >
> >    grub_free(memory_map);
> > --
> > 2.30.0
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] arm/efi: fix ram base detection
  2021-03-22 18:28   ` Leif Lindholm
@ 2021-03-30 20:01     ` Vincent Stehlé
  2021-04-12 13:50       ` Daniel Kiper
  0 siblings, 1 reply; 5+ messages in thread
From: Vincent Stehlé @ 2021-03-30 20:01 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: Daniel Kiper, grub-devel, Grant Likely, Peter Jones

On Mon, Mar 22, 2021 at 06:28:51PM +0000, Leif Lindholm wrote:
..
> > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > > index 0cdb063bb..abf8772bc 100644
> > > --- a/grub-core/kern/efi/mm.c
> > > +++ b/grub-core/kern/efi/mm.c
> > > @@ -677,7 +677,8 @@ grub_efi_get_ram_base(grub_addr_t *base_addr)
> > >    for (desc = memory_map, *base_addr = GRUB_EFI_MAX_USABLE_ADDRESS;
> > >         (grub_addr_t) desc < ((grub_addr_t) memory_map + memory_map_size);
> > >         desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
> > > -    if (desc->attribute & GRUB_EFI_MEMORY_WB)
> > > +    if (desc->type == GRUB_EFI_CONVENTIONAL_MEMORY &&
> > > +        desc->attribute & GRUB_EFI_MEMORY_WB)
> 
> Can we safely assume we don't also need to check against
> GRUB_EFI_PERSISTENT_MEMORY? If so, this is fine.

Hi Leif,

Thanks for the review.

This is a good question about persistent memory; I don't know if we should
check it or not.

I am "fighting" with qemu to add an nvdimm above or below the first normal
memory region to see how this behaves. I will let you know when I have
succeeded.

Best regards,
Vincent.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] arm/efi: fix ram base detection
  2021-03-30 20:01     ` Vincent Stehlé
@ 2021-04-12 13:50       ` Daniel Kiper
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Kiper @ 2021-04-12 13:50 UTC (permalink / raw)
  To: Vincent Stehlé; +Cc: grub-devel, Leif Lindholm, Grant Likely, Peter Jones

On Tue, Mar 30, 2021 at 10:01:23PM +0200, Vincent Stehlé via Grub-devel wrote:
> On Mon, Mar 22, 2021 at 06:28:51PM +0000, Leif Lindholm wrote:
> ..
> > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > > > index 0cdb063bb..abf8772bc 100644
> > > > --- a/grub-core/kern/efi/mm.c
> > > > +++ b/grub-core/kern/efi/mm.c
> > > > @@ -677,7 +677,8 @@ grub_efi_get_ram_base(grub_addr_t *base_addr)
> > > >    for (desc = memory_map, *base_addr = GRUB_EFI_MAX_USABLE_ADDRESS;
> > > >         (grub_addr_t) desc < ((grub_addr_t) memory_map + memory_map_size);
> > > >         desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
> > > > -    if (desc->attribute & GRUB_EFI_MEMORY_WB)
> > > > +    if (desc->type == GRUB_EFI_CONVENTIONAL_MEMORY &&
> > > > +        desc->attribute & GRUB_EFI_MEMORY_WB)
> >
> > Can we safely assume we don't also need to check against
> > GRUB_EFI_PERSISTENT_MEMORY? If so, this is fine.
>
> Hi Leif,
>
> Thanks for the review.
>
> This is a good question about persistent memory; I don't know if we should
> check it or not.
>
> I am "fighting" with qemu to add an nvdimm above or below the first normal
> memory region to see how this behaves. I will let you know when I have
> succeeded.

Any updates about this?

Daniel


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-04-12 13:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-12 16:54 [PATCH] arm/efi: fix ram base detection Vincent Stehlé
2021-03-22 16:00 ` Daniel Kiper
2021-03-22 18:28   ` Leif Lindholm
2021-03-30 20:01     ` Vincent Stehlé
2021-04-12 13:50       ` Daniel Kiper

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.