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

[Ilias Apalodimas]

On Thu, 12 Dec 2024 at 15:45, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 11 Dec 2024 at 23:40, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> >  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
>
> All the other points are OK, just more work. However, this one defeats
> the purpose of the series. As you can see, I am lining up the EFI
> internal-table more with how U-Boot and lmb do things, with addresses.

I think Heinrich and I have repeated this more than once. EFI works
with physical addresses, just like sandbox needs to map_sysmem. It's
way easier for us to review and look at code exactly how the spec
describes it. Also the reason for duplicating that in the commit
message is
- the name is confusing, which reallys int
- The 'reserved' filed isn't needed, which is probably just a generic
bug in the EFI struct  definition


Thanks
/Ilias
> I cannot use both addresses and pointers in the same struct...I
> initially did that but it was just far too confusing.
>
> > - 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
> > >
>
> Regards,
> SImon