[PATCH v5 08/23] efi_loader: Use a separate struct for memory nodes

[Simon Glass]

Hi Ilias,

On Wed, 11 Dec 2024 at 08:08, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon,
>
> On Wed, 11 Dec 2024 at 15:54, Simon Glass <sjg at chromium.org> wrote:
> >
> > At present efi_memory uses struct efi_mem_desc for each node of its
> > memory list. This is convenient but is not ideal, since:
> >
> > - it means that the fields are under a 'desc' sub-structure
> > - it includes an unused 'reserved' field
>
> That's interesting. The EFI spec does not define that and I looked as
> back as 2.5.
> Heinrich, this predates my involvement with EFI, do you remember why
> it was added?
>
> Because it's defined in
> > - it includes virtual_start which is confusing as it is always the same
> >   as physical_start
> > - we must use u64 to store pointers, since that is how they are returned
> >   by calls to efi_get_memory_map()
>
> I don't understand the 'pointers' again here. It's a physical address.
>

Let's have a call so I can explain this...

> >
> > As a first step to tidying this up, create a new, private struct to hold
> > these fields.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > (no changes since v2)
> >
> > Changes in v2:
> > - Add new patch to use a separate stuct for memory nodes
> >
> >  lib/efi_loader/efi_memory.c | 49 +++++++++++++++++++++++++++++++------
> >  1 file changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > index 4bc6d12a1fe..db40aee8353 100644
> > --- a/lib/efi_loader/efi_memory.c
> > +++ b/lib/efi_loader/efi_memory.c
> > @@ -27,9 +27,39 @@ DECLARE_GLOBAL_DATA_PTR;
> >
> >  efi_uintn_t efi_memory_map_key;
> >
> > +/**
> > + * struct priv_mem_desc - defines an memory region
> > + *
> > + * Note that this struct is for use inside U-Boot and is not visible to the
> > + * EFI application, other than through calls to efi_get_memory_map(), where this
> > + * internal format is converted to the external struct efi_mem_desc format.
> > + *
> > + * @type (enum efi_memory_type): EFI memory-type
> > + * @reserved: unused
> > + * @physical_start: Start address of region in physical memory
> > + * @virtual_start: Start address of region in physical memory
> > + * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
> > + *     bytes)
> > + * @attribute: Memory attributes (see EFI_MEMORY...)
> > + */
> > +struct priv_mem_desc {
> > +       u32 type;
> > +       u32 reserved;
> > +       efi_physical_addr_t physical_start;
> > +       efi_virtual_addr_t virtual_start;
> > +       u64 num_pages;
> > +       u64 attribute;
> > +};
>
> This is not private, it's described by the EFI spec.

It is private to this module, and future patches change it to be used
only in this file, so I have commented and named it that way.

>
> > +
> > +/**
> > + * struct efi_mem_list - defines an EFI memory record
> > + *
> > + * @link: Link to prev/next node in list
> > + * @desc: Memory information about this node
> > + */
> >  struct efi_mem_list {
> >         struct list_head link;
> > -       struct efi_mem_desc desc;
> > +       struct priv_mem_desc desc;
> >  };
> >
> >  #define EFI_CARVE_NO_OVERLAP           -1
> > @@ -116,7 +146,7 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b)
> >   * @desc:      memory descriptor
> >   * Return:     end address + 1
> >   */
> > -static uint64_t desc_get_end(struct efi_mem_desc *desc)
> > +static uint64_t desc_get_end(struct priv_mem_desc *desc)
> >  {
> >         return desc->physical_start + (desc->num_pages << EFI_PAGE_SHIFT);
> >  }
> > @@ -138,8 +168,8 @@ static void efi_mem_sort(void)
> >         while (merge_again) {
> >                 merge_again = false;
> >                 list_for_each_entry(lmem, &efi_mem, link) {
> > -                       struct efi_mem_desc *prev;
> > -                       struct efi_mem_desc *cur;
> > +                       struct priv_mem_desc *prev;
> > +                       struct priv_mem_desc *cur;
> >                         uint64_t pages;
> >
> >                         if (!prevmem) {
> > @@ -194,11 +224,11 @@ static void efi_mem_sort(void)
> >   * to re-add the already carved out pages to the mapping.
> >   */
> >  static s64 efi_mem_carve_out(struct efi_mem_list *map,
> > -                            struct efi_mem_desc *carve_desc,
> > +                            struct priv_mem_desc *carve_desc,
> >                              bool overlap_conventional)
> >  {
> >         struct efi_mem_list *newmap;
> > -       struct efi_mem_desc *map_desc = &map->desc;
> > +       struct priv_mem_desc *map_desc = &map->desc;
> >         uint64_t map_start = map_desc->physical_start;
> >         uint64_t map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT);
> >         uint64_t carve_start = carve_desc->physical_start;
> > @@ -678,7 +708,12 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
> >         /* Return the list in ascending order */
> >         memory_map = &memory_map[map_entries - 1];
> >         list_for_each_entry(lmem, &efi_mem, link) {
> > -               *memory_map = lmem->desc;
>
> What's wrong with that ?

There are two different structs now

>
> > +               memory_map->type = lmem->desc.type;
> > +               memory_map->reserved = lmem->desc.reserved;
> > +               memory_map->physical_start = lmem->desc.physical_start;
> > +               memory_map->virtual_start = lmem->desc.virtual_start;
> > +               memory_map->num_pages = lmem->desc.num_pages;
> > +               memory_map->attribute = lmem->desc.attribute;
> >                 memory_map--;
> >         }
> >
> > --
> > 2.34.1
> >
>
> The only problem I see here is the reserved field. I think this patch
> should just remove that

OK

Regards,
Simon