On 26.11.24 09:00, Ilias Apalodimas wrote:
Hi Simon,
On Mon, 25 Nov 2024 at 22:45, Simon Glass <s...@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 <s...@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
