[PATCH 07/13] efi_loader: Make more use of ulong
Heinrich Schuchardt
xypron.glpk at gmx.de
Tue Nov 26 11:13:40 CET 2024
On 26.11.24 09:00, Ilias Apalodimas wrote:
> Hi Simon,
>
>
> On Mon, 25 Nov 2024 at 22:45, Simon Glass <sjg at chromium.org> wrote:
>>
>> One of the confusing parts of the EFI loader is that it uses u64 for
>> addresses, whereas the rest of U-Boot typically uses ulong.
You are confusing sandbox virtual addresses (phys_addr_t) with the
physical addresses (EFI_PHYSICAL_ADDRESS) in the UEFI spec.
The EFI specification requires an identity mapping. This means the value
of a EFI_PHYSICAL_ADDRESS must be usable as a pointer.
The sandbox virtual addresses are not usable as pointers on the sandbox
and hence cannot be used for EFI_PHYSICAL_ADDRESS.
EFI_PHYSICAL_ADDRESS is a 64bit type.
The only useful place for sandbox virtual addresses is in the CLI. We
should get rid of them anywhere else to avoid further confusion.
>>
>> There is a case on 32-bit machines where phys_addr_t can be larger than
>> 32 bits, but this is very much the exception. Also, such machines have
>> mostly faded away and generally don't make use of EFI.
>
> I am not sure how true this statement is with LPAE etc. In any case, I
> don't want us to convert to ulong, there's no reason to break things
> for platforms we can't test. So please drop this patch
>
> Thanks
> /Ilias
>>
>> So in the interests of consistency, update the memory functions to use
>> ulong in most cases.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>
>> include/efi_loader.h | 4 ++--
>> lib/efi_loader/efi_memory.c | 42 ++++++++++++++++++-------------------
>> 2 files changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 0ef4c6f7dea..cae94fc4661 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -838,7 +838,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>> * @mem_type: EFI type of memory added
>> * Return: status code
>> */
>> -efi_status_t efi_add_memory_map(u64 start, u64 size,
>> +efi_status_t efi_add_memory_map(ulong start, ulong size,
>> enum efi_memory_type mem_type);
>>
>> /**
>> @@ -852,7 +852,7 @@ efi_status_t efi_add_memory_map(u64 start, u64 size,
>> * memory
>> * Return: status code
>> */
>> -efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
>> +efi_status_t efi_add_memory_map_pg(ulong start, ulong pages,
>> enum efi_memory_type mem_type,
>> bool overlap_conventional);
>>
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index 2c46915e5b9..4e9c372b622 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -199,10 +199,10 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
>> {
>> struct efi_mem_list *newmap;
>> struct efi_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;
>> - uint64_t carve_end = carve_start +
>> + ulong map_start = map_desc->physical_start;
>> + ulong map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT);
>> + ulong carve_start = carve_desc->physical_start;
>> + ulong carve_end = carve_start +
>> (carve_desc->num_pages << EFI_PAGE_SHIFT);
>>
>> /* check whether we're overlapping */
>> @@ -257,17 +257,17 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
>> return EFI_CARVE_LOOP_AGAIN;
>> }
>>
>> -efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
>> +efi_status_t efi_add_memory_map_pg(ulong start, ulong pages,
>> enum efi_memory_type mem_type,
>> bool overlap_conventional)
>> {
>> struct efi_mem_list *lmem;
>> struct efi_mem_list *newlist;
>> bool carve_again;
>> - uint64_t carved_pages = 0;
>> + ulong carved_pages = 0;
>> struct efi_event *evt;
>>
>> - EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
>> + EFI_PRINT("%s: 0x%lx 0x%lx %d %s\n", __func__,
>> start, pages, mem_type, overlap_conventional ?
>> "yes" : "no");
>>
>> @@ -370,10 +370,10 @@ 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,
>> +efi_status_t efi_add_memory_map(ulong start, ulong size,
>> enum efi_memory_type mem_type)
>> {
>> - u64 pages;
>> + ulong pages;
The result of efi_size_in_pages() is u64.
>>
>> pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK));
Maybe we should check here that pages <= SIZE_T_MAX.
>> start &= ~EFI_PAGE_MASK;
>> @@ -396,13 +396,13 @@ efi_status_t efi_add_memory_map(u64 start, u64 size,
>> * @must_be_allocated: return success if the page is allocated
>> * Return: status code
>> */
>> -static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated)
>> +static efi_status_t efi_check_allocated(ulong addr, bool must_be_allocated)
>> {
>> struct efi_mem_list *item;
>>
>> list_for_each_entry(item, &efi_mem, link) {
>> - u64 start = item->desc.physical_start;
>> - u64 end = start + (item->desc.num_pages << EFI_PAGE_SHIFT);
>> + ulong start = item->desc.physical_start;
>> + ulong end = start + (item->desc.num_pages << EFI_PAGE_SHIFT);
Physical start is u64 as defined in the EFI standard. ulong makes no
sense here.
>>
>> if (addr >= start && addr < end) {
>> if (must_be_allocated ^
>> @@ -420,7 +420,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>> enum efi_memory_type mem_type,
>> efi_uintn_t pages, uint64_t *memory)
>> {
>> - u64 efi_addr, len;
>> + ulong efi_addr, len;
efi_allocate_pages must return u64. Using ulong for efi_addr does not match.
>> uint flags;
>> efi_status_t ret;
>> phys_addr_t addr;
>> @@ -430,7 +430,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>> return EFI_INVALID_PARAMETER;
>> if (!memory)
>> return EFI_INVALID_PARAMETER;
>> - len = (u64)pages << EFI_PAGE_SHIFT;
>> + len = (ulong)pages << EFI_PAGE_SHIFT;
>> /* Catch possible overflow on 64bit systems */
>> if (sizeof(efi_uintn_t) == sizeof(u64) &&
>> (len >> EFI_PAGE_SHIFT) != (u64)pages)
>> @@ -491,7 +491,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>> */
>> efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>> {
>> - u64 len;
>> + ulong len;
pages << EFI_PAGE_SHIFT may be more than ULONG_MAX on 32bit systems.
>> long status;
>> efi_status_t ret;
>>
>> @@ -506,7 +506,7 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>> return EFI_INVALID_PARAMETER;
>> }
>>
>> - len = (u64)pages << EFI_PAGE_SHIFT;
>> + len = (ulong)pages << EFI_PAGE_SHIFT;
This might overflow on 32bit systems.
>> /*
>> * The 'memory' variable for sandbox holds a pointer which has already
>> * been mapped with map_sysmem() from efi_allocate_pages(). Convert
>> @@ -525,10 +525,10 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>> 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;
>> - u64 free_pages;
>> - u64 aligned_mem;
>> + ulong req_pages = efi_size_in_pages(len);
Please, use efi_uintn_t as this is the parameter type of AllocatePages().
>> + ulong true_pages = req_pages + efi_size_in_pages(align) - 1;
>> + ulong free_pages;
>> + ulong aligned_mem;
This does not compile on 32bit. efi_allocate_pages expects *u64 as
parameter.
>> efi_status_t r;
>> u64 mem;
>>
>> @@ -572,7 +572,7 @@ efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size,
>> efi_status_t r;
>> u64 addr;
>> struct efi_pool_allocation *alloc;
>> - u64 num_pages = efi_size_in_pages(size +
>> + ulong num_pages = efi_size_in_pages(size +
>> sizeof(struct efi_pool_allocation));
The EFI specification defines the argument parameter Pages of
AllocatePages() as being UINTN. So we should use efi_uintn_t here.
Best regards
Heinrich
>>
>> if (!buffer)
>> --
>> 2.43.0
>>
More information about the U-Boot
mailing list