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

[Atish Patra]

On Mon, May 10, 2021 at 4:47 PM Atish Patra <atishp at atishpatra.org> wrote:
>
> 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 ?
>

ping ?

> > 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



-- 
Regards,
Atish