[PATCH v5 00/23] efi: Tidy up confusion between pointers and addresses

[Ilias Apalodimas]

 Hi Simon,

On Wed, 11 Dec 2024 at 15:54, Simon Glass <sjg at chromium.org> wrote:
>
> The EFI-loader implementation converts things back and forth between
> addresses and pointers, with not much consistency in how this is done.
>
> Within most of U-Boot a pointer is a void * and an address is a ulong
>
> This convention is very helpful, since it is obvious in common code as
> to whether you need to call map_sysmem() and friends, or not.
>
> As part of cleaning up the EFI memory-management, I found it almost
> impossible to know in some cases whether something is an address or a
> pointer. I decided to give up on that and come back to it when this is
> resolved.
>
> This series starts applying the normal ulong/void * convention to the
> EFI_loader code, so making things easier to follow. For now, u64 is
> often used instead of ulong, but the effect is the same.
>
> The main changes are:
> - Rather than using the external struct efi_mem_desc data-structure for
>   internal bookkeeping, create a new one, so it can have different
>   semantics
> - Within efi_memory.c addresses are used, rather than addresses
>   masquerading as pointers. The conversions are done in efi_boottime
>
> Unforunately my foray into attempting to use enum for the memory type
> failed. The problem is not just that the external interface cannot use
> an enum. In fact the enum in the spec is really just confusing, since
> values not mentioned in the enum are valid. While we could handle this
> by declaring a few more values in enum efi_memory_type, it doesn't seem
> worth it. For example, if we declare 0x6fffffff and -1 as valid values,
> we get the correct range, but then we need to be careful about
> conversion in efi_boottime. If we declare 0xffffffff as valid, then the
> enum ends up being 64-bits wide! Yes, that would work, I suppose it
> wouldn't matter, but at that point I believe using an enum is actually
> more confusing than not. It also made passing SCT tricky, since invalid
> values are passed in...
>

So my general feedback on this is
- Split it into a number of reviewable patches
- Send the typos & doc changes in one patchset that we can easily pick
up since it's mostly reviewed
- Drop the two patches that add 'sjg' stuff that are removed later
- Investigate why there's a reserved member in efi_mem_desc -- which I
think is a bug and send the fix as a single patch
- Drop the renames and duplication of efi_mem_desc with priv_mem_desc.
I don't see any point in duplication that, keeping the memory
descriptors in a list as it currently works is fine
- Send the map_to_sysmem() places you need in a different patchset
with an explanation of *why* this is working currently (or not
working)

Hope this helps
/Ilias

> Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
>
> Changes in v5:
> - Drop the enum / mem_type patch as it is not needed
> - Drop the patch to remove extra brackets in efi_mem_carve_out()
>
> Changes in v4:
> - Add new patch to show the address for pool allocations
> - Combine the various pointer-to-address patches into one
>
> Changes in v3:
> - Adjust comments
> - Show the returned address rather than the pointer
> - Put the header-file in its own section
> - Add comment to struct efi_device_path_memory
> - Use a pointer for the values in struct efi_device_path_memory
>
> Changes in v2:
> - Fix missing @
> - Note that this holds pointers, not addresses
> - Add a new patch with comments where incorrect addresses are used
> - Use EFI_PRINT() instead of log_debug()
> - Rebase on early patch
> - Add new patch to add the EFI-loader API documentation
> - Add new patch to use a separate stuct for memory nodes
> - Drop patch 'Convert efi_get_memory_map() to return pointers'
> - Drop patch 'efi_loader: Make more use of ulong'
> - Significantly expand and redirect the series
>
> Simon Glass (23):
>   efi: Define fields in struct efi_mem_desc
>   efi_loader: Fix typos in enum efi_allocate_type
>   efi_loader: Add comments where incorrect addresses are used
>   efi_loader: Show the resulting memory address from an alloc
>   efi_loader: Update startimage_exit self-test to check error
>   efi_loader: Move some memory-function comments to header
>   doc: efi: Add the EFI-loader API documentation
>   efi_loader: Use a separate struct for memory nodes
>   efi_loader: Drop virtual_start from priv_mem_desc
>   efi_loader: Drop reserved from priv_mem_desc
>   efi_loader: Use the enum for the memory type in priv_mem_desc
>   efi_loader: Avoid assigning desc in efi_mem_carve_out()
>   efi_loader: Move struct efi_mem_list fields together
>   efi_loader: Rename struct efi_mem_list to mem_node
>   efi_loader: Rename physical_start to base
>   efi_loader: Use correct type in efi_add_runtime_mmio()
>   efi_loader: Show the address for pool allocations
>   efi_loader: Don't try to add sandbox runtime code
>   efi_loader: Update to use addresses internally
>   efi_loader: Correct address-usage in copy_fdt()
>   efi_loader: Drop comments about incorrect addresses
>   efi_bootmgr: Avoid casts in try_load_from_uri_path()
>   efi_loader: Simplify efi_dp_from_mem()
>
>  arch/arm/cpu/armv8/fsl-layerscape/cpu.c       |   2 +-
>  arch/arm/mach-bcm283x/reset.c                 |   2 +-
>  doc/api/efi.rst                               |   6 +
>  include/efi.h                                 |  23 +-
>  include/efi_api.h                             |  10 +
>  include/efi_loader.h                          |  87 +++++-
>  lib/efi_loader/efi_bootbin.c                  |   3 +-
>  lib/efi_loader/efi_bootmgr.c                  |  10 +-
>  lib/efi_loader/efi_boottime.c                 |  43 ++-
>  lib/efi_loader/efi_device_path.c              |  16 +-
>  lib/efi_loader/efi_dt_fixup.c                 |   4 -
>  lib/efi_loader/efi_helper.c                   |   4 +-
>  lib/efi_loader/efi_image_loader.c             |   3 +-
>  lib/efi_loader/efi_memory.c                   | 261 +++++++-----------
>  lib/efi_loader/efi_runtime.c                  |   7 +-
>  lib/efi_loader/efi_var_mem.c                  |   6 +-
>  .../efi_selftest_startimage_exit.c            |   6 +-
>  lib/lmb.c                                     |  10 +-
>  18 files changed, 278 insertions(+), 225 deletions(-)
>
> --
> 2.34.1
>