Hi Ilias, On Wed, 11 Dec 2024 at 23:40, Ilias Apalodimas <[email protected]> wrote: > > Hi Simon, > > On Wed, 11 Dec 2024 at 15:54, Simon Glass <[email protected]> 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 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/[email protected]/ > > > > 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
