[PATCH v2 09/28] efi_loader: Use the enum for memory type
Simon Glass
sjg at chromium.org
Thu Nov 28 20:10:59 CET 2024
Hi Heinrich,
On Thu, 28 Nov 2024 at 09:16, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 28.11.24 16:47, Simon Glass wrote:
> > Rather than an integer, it is better to use the enum provided, when
> > referring to an EFI memory-type. Update existing uses.
> >
> > Call the value 'mem_type' consistently. Fix up one instance of
> > upper-case hex.
> >
> > Fix up the calls in struct efi_boot_services so that they use the same
> > enum, adding the missing parameter names and enum efi_allocate_type.
>
> This was already NAKed for struct efi_boot_services in reply to your
> previous submission. We must have control over the size of the parameter.
>
> Where the memory type is provided by internal callers we could use the enum.
Yes, this is an internal call.
>
> >
> > While we are here, rename the 'memory' parameter to 'memoryp' so that it
> > is clear it is a return value.
>
> Parameter names should match the UEFI specification.
> This renaming just creates confustion.
The enum is there now, which makes everything very clear.
>
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v2:
> > - Drop the changes to the boottime API
> >
> > include/efi_loader.h | 29 ++++++++++----------
> > lib/efi_loader/efi_boottime.c | 12 ++++-----
> > lib/efi_loader/efi_device_path.c | 4 +--
> > lib/efi_loader/efi_memory.c | 46 +++++++++++++++++---------------
> > 4 files changed, 47 insertions(+), 44 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 3f75f2efcb6..a7228672f27 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -773,24 +773,25 @@ void *efi_alloc(size_t len);
> > * efi_alloc_aligned_pages() - allocate aligned memory pages
> > *
> > * @len: len in bytes
> > - * @memory_type: usage type of the allocated memory
> > + * @mem_type: usage type of the allocated memory
> > * @align: alignment in bytes
> > * Return: aligned memory or NULL
> > */
> > -void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align);
> > +void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type,
> > + size_t align);
> >
> > /**
> > * efi_allocate_pages - allocate memory pages
> > *
> > * @type: type of allocation to be performed
> > - * @memory_type: usage type of the allocated memory
> > + * @mem_type: usage type of the allocated memory
> > * @pages: number of pages to be allocated
> > - * @memory: returns a pointer to the allocated memory
> > + * @memoryp: returns a pointer to the allocated memory
> > * Return: status code
> > */
> > efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> > - enum efi_memory_type memory_type,
> > - efi_uintn_t pages, uint64_t *memory);
> > + enum efi_memory_type mem_type,
> > + efi_uintn_t pages, uint64_t *memoryp);
> >
> > /**
> > * efi_free_pages() - free memory pages
> > @@ -804,12 +805,12 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages);
> > /**
> > * efi_allocate_pool - allocate memory from pool
> > *
> > - * @pool_type: type of the pool from which memory is to be allocated
> > + * @mem_type: memory type of the pool from which memory is to be allocated
> > * @size: number of bytes to be allocated
> > * @buffer: allocated memory
> > * Return: status code
> > */
> > -efi_status_t efi_allocate_pool(enum efi_memory_type pool_type,
> > +efi_status_t efi_allocate_pool(enum efi_memory_type mem_type,
> > efi_uintn_t size, void **buffer);
> >
> > /**
> > @@ -837,14 +838,14 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
> > * actually a pointer provided as an integer. To pass in
> > * an address, pass in (ulong)map_to_sysmem(addr)
> > * @size: length in bytes of the memory area
> > - * @memory_type: type of memory added
> > - *
> > + * @mem_type: EFI type of memory added
> > * Return: status code
> > *
> > * This function automatically aligns the start and size of the memory area
> > * to EFI_PAGE_SIZE.
> > */
> > -efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
> > +efi_status_t efi_add_memory_map(u64 start, u64 size,
> > + enum efi_memory_type mem_type);
> >
> > /**
> > * efi_add_memory_map_pg() - add pages to the memory map
> > @@ -854,13 +855,13 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
> > * in (ulong)map_to_sysmem(addr)
> > *
> > * @pages: number of pages to add
> > - * @memory_type: EFI type of memory added
> > + * @mem_type: EFI type of memory added
> > * @overlap_conventional: region may only overlap free(conventional)
> > * memory
> > * Return: status code
> > */
> > efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> > - int memory_type,
> > + enum efi_memory_type mem_type,
> > bool overlap_conventional);
> >
> > /* Called by board init to initialize the EFI drivers */
> > @@ -919,7 +920,7 @@ struct efi_device_path *efi_dp_part_node(struct blk_desc *desc, int part);
> > struct efi_device_path *efi_dp_from_file(const struct efi_device_path *dp,
> > const char *path);
> > struct efi_device_path *efi_dp_from_eth(void);
> > -struct efi_device_path *efi_dp_from_mem(uint32_t mem_type,
> > +struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type,
> > uint64_t start_address,
> > size_t size);
> > /* Determine the last device path node that is not the end node. */
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index be8e4f41ad2..8f311f95d1f 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -414,9 +414,9 @@ static void EFIAPI efi_restore_tpl(efi_uintn_t old_tpl)
> > /**
> > * efi_allocate_pages_ext() - allocate memory pages
> > * @type: type of allocation to be performed
> > - * @memory_type: usage type of the allocated memory
> > + * @mem_type: usage type of the allocated memory
> > * @pages: number of pages to be allocated
> > - * @memory: allocated memory
> > + * @memoryp: allocated memory
> > *
> > * This function implements the AllocatePages service.
> > *
> > @@ -425,14 +425,14 @@ static void EFIAPI efi_restore_tpl(efi_uintn_t old_tpl)
> > *
> > * Return: status code
> > */
> > -static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int memory_type,
> > +static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int mem_type,
> > efi_uintn_t pages,
> > - uint64_t *memory)
> > + uint64_t *memoryp)
>
> We want the names to match the UEFI specification.
> This change is not helpful.
>
> > {
> > efi_status_t r;
> >
> > - EFI_ENTRY("%d, %d, 0x%zx, %p", type, memory_type, pages, memory);
> > - r = efi_allocate_pages(type, memory_type, pages, memory);
> > + EFI_ENTRY("%d, %d, 0x%zx, %p", type, mem_type, pages, memoryp);
> > + r = efi_allocate_pages(type, mem_type, pages, memoryp);
> > return EFI_EXIT(r);
> > }
> >
> > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> > index ee387e1dfd4..9c8cd35b97b 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -975,7 +975,7 @@ struct efi_device_path __maybe_unused *efi_dp_from_eth(void)
> > }
> >
> > /* Construct a device-path for memory-mapped image */
> > -struct efi_device_path *efi_dp_from_mem(uint32_t memory_type,
> > +struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type,
>
> Why rename the parameter?
To make it shorter, since we already have efi_memory_type there right
before it, now.
>
> > uint64_t start_address,
> > size_t size)
> > {
> > @@ -990,7 +990,7 @@ struct efi_device_path *efi_dp_from_mem(uint32_t memory_type,
> > mdp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
> > mdp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MEMORY;
> > mdp->dp.length = sizeof(*mdp);
> > - mdp->memory_type = memory_type;
> > + mdp->memory_type = mem_type;
> > mdp->start_address = start_address;
> > mdp->end_address = start_address + size;
> > buf = &mdp[1];
> > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > index 9d0f27dbb3e..3cece51f030 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_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> > - int memory_type,
> > + enum efi_memory_type mem_type,
> > bool overlap_conventional)
> > {
> > struct efi_mem_list *lmem;
> > @@ -268,10 +268,10 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> > struct efi_event *evt;
> >
> > EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
> > - start, pages, memory_type, overlap_conventional ?
> > + start, pages, mem_type, overlap_conventional ?
> > "yes" : "no");
> >
> > - if (memory_type >= EFI_MAX_MEMORY_TYPE)
> > + if (mem_type >= EFI_MAX_MEMORY_TYPE)
> > return EFI_INVALID_PARAMETER;
> >
> > if (!pages)
> > @@ -281,12 +281,12 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> > newlist = calloc(1, sizeof(*newlist));
> > if (!newlist)
> > return EFI_OUT_OF_RESOURCES;
> > - newlist->desc.type = memory_type;
> > + newlist->desc.type = mem_type;
> > newlist->desc.physical_start = start;
> > newlist->desc.virtual_start = start;
> > newlist->desc.num_pages = pages;
> >
> > - switch (memory_type) {
> > + switch (mem_type) {
> > case EFI_RUNTIME_SERVICES_CODE:
> > case EFI_RUNTIME_SERVICES_DATA:
> > newlist->desc.attribute = EFI_MEMORY_WB | EFI_MEMORY_RUNTIME;
> > @@ -370,14 +370,15 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> > return EFI_SUCCESS;
> > }
> >
> > -efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type)
> > +efi_status_t efi_add_memory_map(u64 start, u64 size,
> > + enum efi_memory_type mem_type)
>
> Why rename the parameter?
>
> > {
> > u64 pages;
> >
> > 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_add_memory_map_pg(start, pages, mem_type, false);
> > }
> >
> > /**
> > @@ -416,8 +417,8 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated)
> > }
> >
> > efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> > - enum efi_memory_type memory_type,
> > - efi_uintn_t pages, uint64_t *memory)
> > + enum efi_memory_type mem_type,
>
> We should stick to the parameter name from the UEFI specification.
>
>
> > + efi_uintn_t pages, uint64_t *memoryp)
>
> Ditto
>
> Best regards
>
> Heinrich
>
> > {
> > u64 efi_addr, len;
> > uint flags;
> > @@ -425,10 +426,9 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> > phys_addr_t addr;
> >
> > /* Check import parameters */
> > - if (memory_type >= EFI_PERSISTENT_MEMORY_TYPE &&
> > - memory_type <= 0x6FFFFFFF)
> > + if (mem_type >= EFI_PERSISTENT_MEMORY_TYPE && mem_type <= 0x6fffffff)
> > return EFI_INVALID_PARAMETER;
> > - if (!memory)
> > + if (!memoryp)
> > return EFI_INVALID_PARAMETER;
> > len = (u64)pages << EFI_PAGE_SHIFT;
> > /* Catch possible overflow on 64bit systems */
> > @@ -447,17 +447,17 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> > break;
> > case EFI_ALLOCATE_MAX_ADDRESS:
> > /* Max address */
> > - addr = map_to_sysmem((void *)(uintptr_t)*memory);
> > + addr = map_to_sysmem((void *)(uintptr_t)*memoryp);
> > addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, addr,
> > flags);
> > if (!addr)
> > return EFI_OUT_OF_RESOURCES;
> > break;
> > case EFI_ALLOCATE_ADDRESS:
> > - if (*memory & EFI_PAGE_MASK)
> > + if (*memoryp & EFI_PAGE_MASK)
> > return EFI_NOT_FOUND;
> >
> > - addr = map_to_sysmem((void *)(uintptr_t)*memory);
> > + addr = map_to_sysmem((void *)(uintptr_t)*memoryp);
> > addr = (u64)lmb_alloc_addr_flags(addr, len, flags);
> > if (!addr)
> > return EFI_NOT_FOUND;
> > @@ -469,7 +469,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_add_memory_map_pg(efi_addr, pages, mem_type, true);
> > if (ret != EFI_SUCCESS) {
> > /* Map would overlap, bail out */
> > lmb_free_flags(addr, (u64)pages << EFI_PAGE_SHIFT, flags);
> > @@ -477,7 +477,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> > return EFI_OUT_OF_RESOURCES;
> > }
> >
> > - *memory = efi_addr;
> > + *memoryp = efi_addr;
> >
> > return EFI_SUCCESS;
> > }
> > @@ -515,7 +515,8 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
> > return ret;
> > }
> >
> > -void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align)
> > +void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type,
> > + size_t align)
> > {
> > u64 req_pages = efi_size_in_pages(len);
> > u64 true_pages = req_pages + efi_size_in_pages(align) - 1;
> > @@ -533,12 +534,12 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align)
> > return NULL;
> >
> > if (align < EFI_PAGE_SIZE) {
> > - r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type,
> > + r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type,
> > req_pages, &mem);
> > return (r == EFI_SUCCESS) ? (void *)(uintptr_t)mem : NULL;
> > }
> >
> > - r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type,
> > + r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type,
> > true_pages, &mem);
> > if (r != EFI_SUCCESS)
> > return NULL;
> > @@ -559,7 +560,8 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align)
> > return (void *)(uintptr_t)aligned_mem;
> > }
> >
> > -efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer)
> > +efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size,
> > + void **buffer)
> > {
> > efi_status_t r;
> > u64 addr;
> > @@ -575,7 +577,7 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
> > return EFI_SUCCESS;
> > }
> >
> > - r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
> > + r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, num_pages,
> > &addr);
> > if (r == EFI_SUCCESS) {
> > alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
>
Regards,
Simon
More information about the U-Boot
mailing list