[PATCH] efi_loader: consider no-map property of reserved memory

[Atish Patra]

On Wed, Sep 2, 2020 at 12:10 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 31.08.20 20:08, Atish Patra wrote:
> > On Thu, Aug 27, 2020 at 9:16 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>
> >> If a reserved memory node in the device tree has the property no-map,
> >> remove it from the UEFI memory map provided by GetMemoryMap().
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>
> In the mail-thread starting a
>
> [PATCH 1/1] EBBR: GetMemoryMap(), handling of no-map DT property
> https://lists.linaro.org/pipermail/boot-architecture/2020-September/001389.html
>
> the issue has been discussed. The conclusion was that we should not
> change the current behavior.
>

Digging some old threads :)

The merged version of the patch marks any reserved memory as
EFI_BOOT_SERVICES_DATA.
AFAIK, these memory regions will be available after ExitBootservice.
If the operating system chooses to
map these region and access them, it violates the purpose of the
reserved memory region specified by the firmware.

Did I miss something ?

> Best regards
>
> Heinrich
>
> >> ---
> >>  cmd/bootefi.c               | 34 ++++++++++++++++++++++++++++------
> >>  include/efi.h               |  3 +++
> >>  lib/efi_loader/efi_memory.c |  7 +++++--
> >>  3 files changed, 36 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >> index 40d5ef2b3a..f173105251 100644
> >> --- a/cmd/bootefi.c
> >> +++ b/cmd/bootefi.c
> >> @@ -135,12 +135,29 @@ done:
> >>         return ret;
> >>  }
> >>
> >> -static void efi_reserve_memory(u64 addr, u64 size)
> >> +/**
> >> + * efi_reserve_memory() - add reserved memory to memory map
> >> + *
> >> + * @addr:      start address of the reserved memory range
> >> + * @size:      size of the reserved memory range
> >> + * @nomap:     indicates that the memory range shall be hidden from the memory
> >> + *             map
> >> + */
> >> +static void efi_reserve_memory(u64 addr, u64 size, bool nomap)
> >>  {
> >> +       int type;
> >> +       efi_uintn_t ret;
> >> +
> >>         /* Convert from sandbox address space. */
> >>         addr = (uintptr_t)map_sysmem(addr, 0);
> >> -       if (efi_add_memory_map(addr, size,
> >> -                              EFI_RESERVED_MEMORY_TYPE) != EFI_SUCCESS)
> >> +
> >> +       if (nomap)
> >> +               type = EFI_NO_MAP_MEMORY;
> >> +       else
> >> +               type = EFI_RESERVED_MEMORY_TYPE;
> >> +
> >> +       ret = efi_add_memory_map(addr, size, type);
> >> +       if (ret != EFI_SUCCESS)
> >>                 log_err("Reserved memory mapping failed addr %llx size %llx\n",
> >>                         addr, size);
> >>  }
> >> @@ -166,7 +183,7 @@ static void efi_carve_out_dt_rsv(void *fdt)
> >>         for (i = 0; i < nr_rsv; i++) {
> >>                 if (fdt_get_mem_rsv(fdt, i, &addr, &size) != 0)
> >>                         continue;
> >> -               efi_reserve_memory(addr, size);
> >> +               efi_reserve_memory(addr, size, false);
> >>         }
> >>
> >>         /* process reserved-memory */
> >> @@ -186,8 +203,13 @@ static void efi_carve_out_dt_rsv(void *fdt)
> >>                          * a size instead of a reg property.
> >>                          */
> >>                         if (fdt_addr != FDT_ADDR_T_NONE &&
> >> -                           fdtdec_get_is_enabled(fdt, subnode))
> >> -                               efi_reserve_memory(fdt_addr, fdt_size);
> >> +                           fdtdec_get_is_enabled(fdt, subnode)) {
> >> +                               bool nomap;
> >> +
> >> +                               nomap = !!fdt_getprop(fdt, subnode, "no-map",
> >> +                                                     NULL);
> >> +                               efi_reserve_memory(fdt_addr, fdt_size, nomap);
> >> +                       }
> >>                         subnode = fdt_next_subnode(fdt, subnode);
> >>                 }
> >>         }
> >> diff --git a/include/efi.h b/include/efi.h
> >> index f986aad877..50190021ef 100644
> >> --- a/include/efi.h
> >> +++ b/include/efi.h
> >> @@ -181,6 +181,9 @@ enum efi_mem_type {
> >>
> >>         EFI_MAX_MEMORY_TYPE,
> >>         EFI_TABLE_END,  /* For efi_build_mem_table() */
> >> +
> >> +       /* Memory that shall not be mapped */
> >> +       EFI_NO_MAP_MEMORY,
> >
> > Should it be named as EFI_NO_MAP_RSVD_MEMORY to avoid ambiguity ?
> >
> >>  };
> >>
> >>  /* Attribute values */
> >> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> >> index 7be756e370..d156b9533c 100644
> >> --- a/lib/efi_loader/efi_memory.c
> >> +++ b/lib/efi_loader/efi_memory.c
> >> @@ -251,7 +251,7 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> >>         EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
> >>                   start, pages, memory_type, overlap_only_ram ? "yes" : "no");
> >>
> >> -       if (memory_type >= EFI_MAX_MEMORY_TYPE)
> >> +       if (memory_type >= EFI_MAX_MEMORY_TYPE && memory_type != EFI_NO_MAP_MEMORY)
> >>                 return EFI_INVALID_PARAMETER;
> >>
> >>         if (!pages)
> >> @@ -327,7 +327,10 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> >>         }
> >>
> >>         /* Add our new map */
> >> -        list_add_tail(&newlist->link, &efi_mem);
> >> +       if (memory_type == EFI_NO_MAP_MEMORY)
> >> +               free(newlist);
> >> +       else
> >> +               list_add_tail(&newlist->link, &efi_mem);
> >>
> >>         /* And make sure memory is listed in descending order */
> >>         efi_mem_sort();
> >> --
> >> 2.28.0
> >>
> >
> >
>


-- 
Regards,
Atish