Hi Simon, On Wed, 11 Dec 2024 at 15:54, Simon Glass <[email protected]> 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 <[email protected]> > --- > > (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
