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

Hi Simon,

On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@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.

As a first step to tidying this up, create a new, private struct to hold these fields.

Signed-off-by: Simon Glass sjg@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.

+/**

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

•           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

Thanks /Ilias