Re: [PATCH] arm/efi: fix ram base detection

[Leif Lindholm <leif@nuviainc.com>]

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