[PATCH 07/13] efi_loader: Make more use of ulong
Simon Glass
sjg at chromium.org
Tue Nov 26 16:40:13 CET 2024
Hi Heinrich,
On Tue, 26 Nov 2024 at 03:13, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> 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.
No, I am actually trying to resolve the confusion between ulong and
void * in the EFI code. Note that phys_addr_t is unrelated to sandbox.
>
> The EFI specification requires an identity mapping. This means the value
> of a EFI_PHYSICAL_ADDRESS must be usable as a pointer.
Yes, that's right.
>
> 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.
This is not correct and this attitude is making it very hard to
understand what the EFI loader is doing with memory.
>
> >>
> >> 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.
My understanding is that a 32-bit machine cannot access 64-bit
addresses in U-Boot, so accounting for them makes no sense?
>
> >>
> >> 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.
We normally use ulong for size as well as address, in U-Boot.
Regards,
SImon
More information about the U-Boot
mailing list