Re: Fwd: Re: [PATCH] efi_loader: Omit memory with "no-map" when returning memory map.

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 8/31/21 1:12 PM, Kristian Amlie wrote:
> On 31/08/2021 12:46, Heinrich Schuchardt wrote:
>>
>>
>> ------------------------------------------------------------------------
>> *Von:* Ard Biesheuvel <ardb@kernel.org>
>> *Gesendet:* 31. August 2021 12:33:56 MESZ
>> *An:* Heinrich Schuchardt <xypron.glpk@gmx.de>
>> *CC:* Kristian Amlie <kristian.amlie@northern.tech>
>> *Betreff:* Re: [PATCH] efi_loader: Omit memory with "no-map" when
>> returning memory map.
>>
>> On Tue, 31 Aug 2021 at 08:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>>
>>      On 8/27/21 9:55 AM, Kristian Amlie wrote:
>>
>>      You can use scripts/get_maintainer.pl to find the right addressees for
>>      your patches.
>>
>>          efi_reserve_memory() states that memory marked with "no-map"
>>          shall not
>>          be accessed by the UEFI payload. Make sure efi_get_memory_map()
>>          honors
>>          this.
>>
>>
>> Accessing memory and describing memory are two different things.
>> Describing reserved memory in the memory map is important, because it
>> helps us distinguish it from MMIO regions.
>
> Ok, my mistake, I thought the kernel would deduce this separately
> through the DTB.
>
>>
>>          This helps the case when booting vexpress_ca9x4 under QEMU. Because
>>          the kernel wants to use an address in the lowest 128MiB of the
>>          range,
>>          but this range is reserved with "no-map", the kernel complains
>>          that it
>>          can not allocate the low memory it needs. In reality the actual
>>          usable
>>          memory starts much higher, which is reflected correctly in the
>>          memory
>>          map after this fix.
>>
>>
>>
>> This is a u-boot patch right? (I cannot tell from the context, as
>> there are no mailing lists on cc)
>>
>> It is u-boot's job to describe all the memory, no matter how it is
>> used. Even if the kernel itself may not use it as system memory, there
>> are cases where kernel infrastructure is used to map these regions:
>> for instance, the ACPI core may need to map a SystemMemory OpRegion,
>> and we need the EFI memory map to tell us whether to use memory or I/O
>> semantics.
>>
>> As for the 128 MB issue: can you reproduce this with a recent kernel?
>> We made some changes recently to the EFI stub as well as the
>> decompressor code to prevent the decompressor code from relocating
>> itself to the base of DRAM, and to allow the decompressed kernel to
>> reside at any 2 MB aligned offset in memory.

This would allow putting the kernel at the very top of memory. But
consider this function that misbehaves:

arch/arm/include/asm/efi.h:

76 /* on ARM, the initrd should be loaded in a lowmem region */
77 static inline unsigned long efi_get_max_initrd_addr(unsigned long
image_addr)
78 {
79         return round_down(image_addr, SZ_4M) + SZ_512M;
80 }

0x1F000000 = efi_get_max_initrd_addr(0xff000000);

And below 0x1f000000 there may be no RAM at all.

Best regards

Heinrich

>
> I'll try this and get back to you!
>
> --
> Kristian
>
>>
>>
>>      The 'no-map' requirement needs to be fulfilled by the kernel.
>>
>>      The GetMemoryMap() boot time service must comply to the UEFI
>>      specification.
>>
>>      The Devicetree Specification has this clarification:
>>
>>      "Reserved regions with the no-map property must be listed in the memory
>>      map with type EfiReservedMemoryType. All other reserved regions must be
>>      listed with type EfiBootServicesData."
>>      https://devicetree-specification.readthedocs.io/en/latest/chapter3-devicenodes.html
>>      <https://devicetree-specification.readthedocs.io/en/latest/chapter3-devicenodes.html>
>>
>>      Should the kernel calculate its internal 128 MiB requirement incorrectly
>>      this needs be fixed in the kernel EFI stub. Does the problem exist with
>>      kernel 5.14?
>>
>>      I wonder if the 128 MiB requirement of the kernel can be lifted for
>>      32bit ARM as we already did for RISC-V. Cf.
>>
>>
>> As mentioned above, this should already be fixed, in v5.11 or later
>>
>>      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.14&id=c79e89ecaa246c880292ba68cbe08c9c30db77e3
>>      <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.14&id=c79e89ecaa246c880292ba68cbe08c9c30db77e3>
>>
>>      Cc Ard, maintainer of the kernel EFI stub.
>>
>>      Best regards
>>
>>      Heinrich
>>
>>
>>          Signed-off-by: Kristian Amlie <kristian.amlie@northern.tech>
>>          ------------------------------------------------------------------------
>>          lib/efi_loader/efi_memory.c | 14 +++++++++++++-
>>          1 file changed, 13 insertions(+), 1 deletion(-)
>>
>>          diff --git a/lib/efi_loader/efi_memory.c
>>          b/lib/efi_loader/efi_memory.c
>>          index f4acbee4f9..7f8543143a 100644
>>          --- a/lib/efi_loader/efi_memory.c
>>          +++ b/lib/efi_loader/efi_memory.c
>>          @@ -646,8 +646,16 @@ efi_status_t efi_get_memory_map(efi_uintn_t
>>          *memory_map_size,
>>
>>          provided_map_size = *memory_map_size;
>>
>>          - list_for_each(lhandle, &efi_mem)
>>          + list_for_each(lhandle, &efi_mem) {
>>          + struct efi_mem_list *lmem;
>>          +
>>          + lmem = list_entry(lhandle, struct efi_mem_list, link);
>>          +
>>          + if (lmem->desc.type == EFI_RESERVED_MEMORY_TYPE)
>>          + continue;
>>          +
>>          map_entries++;
>>          + }
>>
>>          map_size = map_entries * sizeof(struct efi_mem_desc);
>>
>>          @@ -672,6 +680,10 @@ efi_status_t efi_get_memory_map(efi_uintn_t
>>          *memory_map_size,
>>          struct efi_mem_list *lmem;
>>
>>          lmem = list_entry(lhandle, struct efi_mem_list, link);
>>          +
>>          + if (lmem->desc.type == EFI_RESERVED_MEMORY_TYPE)
>>          + continue;
>>          +
>>          *memory_map = lmem->desc;
>>          memory_map--;
>>          }
>>
>>
>
>