Hi Ilias, On Wed, 11 Dec 2024 at 08:08, Ilias Apalodimas <[email protected]> wrote: > > 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. >
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 <[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. 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
