Hi Heinrich On Wed, 12 Mar 2025 at 12:13, Heinrich Schuchardt <[email protected]> wrote:
> On 12.03.25 09:54, Sughosh Ganu wrote: > > From: Ilias Apalodimas <[email protected]> > > > > With upcoming changes supporting pmem nodes, we need to remove the > > pmem area from the EFI memory map. Add a function to do that. > > > > Signed-off-by: Ilias Apalodimas <[email protected]> > > Signed-off-by: Sughosh Ganu <[email protected]> > > Reviewed-by: Heinrich Schuchardt <[email protected]> > > --- > > Changes since V7: None > > > > include/efi_loader.h | 18 ++++++++++++ > > lib/efi_loader/efi_memory.c | 56 ++++++++++++++++++++++++++----------- > > 2 files changed, 58 insertions(+), 16 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index e9c10819ba2..c20c98c6476 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -878,6 +878,24 @@ efi_status_t efi_get_memory_map(efi_uintn_t > *memory_map_size, > > /* Adds a range into the EFI memory map */ > > efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type); > > > > +/** > > + * efi_update_memory_map() - update the memory map by adding/removing > pages > > + * > > + * @start: start address, must be a multiple of > > + * EFI_PAGE_SIZE > > + * @pages: number of pages to add > > + * @memory_type: type of memory added > > + * @overlap_conventional: region may only overlap free(conventional) > > + * memory > > + * @remove: remove memory map > > + * Return: status code > > + */ > > +efi_status_t efi_update_memory_map(u64 start, u64 pages, int > memory_type, > > + bool overlap_conventional, bool remove); > > Why do we have so many functions that are all doing nearly the same thing: > > * efi_map_update_notify() > * efi_update_memory_map() > * efi_remove_memory_map() > * efi_add_memory_map_pg() > > I start failing to understand when to use which. This is getting as > gruesome as LMB. > We are not there yet! > Can't we have a single exported function with a properly defined > interface to cover all use cases > We actually do, efi_update_memory_map() is what all the functions call with the correct arguments. The reset were added to make the thing easier not harder. You want to remove the wrappers? Thanks /Ilias Best regards > > Heinrich > > > + > > +/* Remove memory from the EFI memory map */ > > +efi_status_t efi_remove_memory_map(u64 start, u64 size, int > memory_type); > > + > > /* Called by board init to initialize the EFI drivers */ > > efi_status_t efi_driver_init(void); > > /* Called when a block device is added */ > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c > > index 6d00b186250..3fec40b6de5 100644 > > --- a/lib/efi_loader/efi_memory.c > > +++ b/lib/efi_loader/efi_memory.c > > @@ -258,7 +258,7 @@ static s64 efi_mem_carve_out(struct efi_mem_list > *map, > > } > > > > /** > > - * efi_add_memory_map_pg() - add pages to the memory map > > + * efi_update_memory_map() - update the memory map by adding/removing > pages > > * > > * @start: start address, must be a multiple of > > * EFI_PAGE_SIZE > > @@ -266,12 +266,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list > *map, > > * @memory_type: type of memory added > > * @overlap_conventional: region may only overlap free(conventional) > > * memory > > + * @remove: remove memory map > > * Return: status code > > */ > > -static > > -efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > - int memory_type, > > - bool overlap_conventional) > > +efi_status_t efi_update_memory_map(u64 start, u64 pages, int > memory_type, > > + bool overlap_conventional, bool remove) > > { > > struct efi_mem_list *lmem; > > struct efi_mem_list *newlist; > > @@ -279,9 +278,9 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 > pages, > > uint64_t carved_pages = 0; > > struct efi_event *evt; > > > > - EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, > > + EFI_PRINT("%s: 0x%llx 0x%llx %d %s %s\n", __func__, > > start, pages, memory_type, overlap_conventional ? > > - "yes" : "no"); > > + "yes" : "no", remove ? "remove" : "add"); > > > > if (memory_type >= EFI_MAX_MEMORY_TYPE) > > return EFI_INVALID_PARAMETER; > > @@ -364,7 +363,10 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 > pages, > > } > > > > /* Add our new map */ > > - list_add_tail(&newlist->link, &efi_mem); > > + if (!remove) > > + list_add_tail(&newlist->link, &efi_mem); > > + else > > + free(newlist); > > > > /* And make sure memory is listed in descending order */ > > efi_mem_sort(); > > @@ -401,7 +403,29 @@ efi_status_t efi_add_memory_map(u64 start, u64 > size, int memory_type) > > pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK)); > > start &= ~EFI_PAGE_MASK; > > > > - return efi_add_memory_map_pg(start, pages, memory_type, false); > > + return efi_update_memory_map(start, pages, memory_type, false, > false); > > +} > > + > > +/** > > + * efi_remove_memory_map() - remove memory area to the memory map > > + * > > + * @start: start address of the memory area > > + * @size: length in bytes of the memory area > > + * @memory_type: type of memory removed > > + * > > + * Return: status code > > + * > > + * This function automatically aligns the start and size of the memory > area > > + * to EFI_PAGE_SIZE. > > + */ > > +efi_status_t efi_remove_memory_map(u64 start, u64 size, int memory_type) > > +{ > > + u64 pages; > > + > > + pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK)); > > + start &= ~EFI_PAGE_MASK; > > + > > + return efi_update_memory_map(start, pages, memory_type, false, > true); > > } > > > > /** > > @@ -502,7 +526,7 @@ efi_status_t efi_allocate_pages(enum > efi_allocate_type type, > > > > efi_addr = (u64)(uintptr_t)map_sysmem(addr, 0); > > /* Reserve that map in our memory maps */ > > - ret = efi_add_memory_map_pg(efi_addr, pages, memory_type, true); > > + ret = efi_update_memory_map(efi_addr, pages, memory_type, true, > false); > > if (ret != EFI_SUCCESS) { > > /* Map would overlap, bail out */ > > lmb_free_flags(addr, (u64)pages << EFI_PAGE_SHIFT, flags); > > @@ -823,8 +847,8 @@ static void add_u_boot_and_runtime(void) > > uboot_stack_size) & ~EFI_PAGE_MASK; > > uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) - > > uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT; > > - efi_add_memory_map_pg(uboot_start, uboot_pages, > EFI_BOOT_SERVICES_CODE, > > - false); > > + efi_update_memory_map(uboot_start, uboot_pages, > EFI_BOOT_SERVICES_CODE, > > + false, false); > > #if defined(__aarch64__) > > /* > > * Runtime Services must be 64KiB aligned according to the > > @@ -842,8 +866,8 @@ static void add_u_boot_and_runtime(void) > > runtime_end = (uintptr_t)__efi_runtime_stop; > > runtime_end = (runtime_end + runtime_mask) & ~runtime_mask; > > runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; > > - efi_add_memory_map_pg(runtime_start, runtime_pages, > > - EFI_RUNTIME_SERVICES_CODE, false); > > + efi_update_memory_map(runtime_start, runtime_pages, > > + EFI_RUNTIME_SERVICES_CODE, false, false); > > } > > > > int efi_memory_init(void) > > @@ -878,11 +902,11 @@ int efi_map_update_notify(phys_addr_t addr, > phys_size_t size, > > pages = efi_size_in_pages(size + (efi_addr & EFI_PAGE_MASK)); > > efi_addr &= ~EFI_PAGE_MASK; > > > > - status = efi_add_memory_map_pg(efi_addr, pages, > > + status = efi_update_memory_map(efi_addr, pages, > > op == LMB_MAP_OP_RESERVE ? > > EFI_BOOT_SERVICES_DATA : > > EFI_CONVENTIONAL_MEMORY, > > - false); > > + false, false); > > if (status != EFI_SUCCESS) { > > log_err("LMB Map notify failure %lu\n", > > status & ~EFI_ERROR_MASK); > >
