[PATCH v4 00/25] efi: Tidy up confusion between pointers and addresses

[PATCH v4 00/25] efi: Tidy up confusion between pointers and addresses

[Simon Glass]

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

Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/

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
- Drop the changes to the boottime API
- 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 (25):
  efi: Define fields in struct efi_mem_desc
  efi_loader: Fix typos in enum efi_allocate_type
  efi_loader: Drop extra brackets in efi_mem_carve_out()
  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 the enum for memory type
  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                          | 102 ++++--
 lib/efi_loader/efi_bootbin.c                  |   3 +-
 lib/efi_loader/efi_bootmgr.c                  |  10 +-
 lib/efi_loader/efi_boottime.c                 |  53 ++-
 lib/efi_loader/efi_device_path.c              |  18 +-
 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                   | 301 +++++++-----------
 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, 313 insertions(+), 257 deletions(-)

-- 
2.43.0

[PATCH v4 01/25] efi: Define fields in struct efi_mem_desc

[Simon Glass]

There is quite a bit of confusion in the EFI code as to whether a field
contains an address or a pointer. As a first step towards resolving
this, document the memory-descriptor struct, indicating that it holds
pointers, not addresses.

Dro the same for efi_add_memory_map() as it is widely used, as well as
efi_add_memory_map_pg() which is only used by lmb

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v3)

Changes in v3:
- Adjust comments

Changes in v2:
- Fix missing @
- Note that this holds pointers, not addresses

 include/efi.h               | 21 +++++++++++++++++++++
 include/efi_loader.h        |  6 ++++--
 lib/efi_loader/efi_memory.c |  4 +++-
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/include/efi.h b/include/efi.h
index c559fda3004..348ff9efd16 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -273,6 +273,27 @@ enum efi_memory_type {
 #define EFI_PAGE_SIZE		(1ULL << EFI_PAGE_SHIFT)
 #define EFI_PAGE_MASK		(EFI_PAGE_SIZE - 1)
 
+/**
+ * struct efi_mem_desc - defines an EFI memory record
+ *
+ * This field implements the EFI_MEMORY_DESCRIPTOR type of the UEFI
+ * specification.
+ *
+ * Note that this struct is for use outside U-Boot, so the two 'start' fields
+ * are pointers, not addresses. Use map_to_sysmem() to convert to an address, so
+ * that sandbox operates correctly.
+ *
+ * @type (enum efi_memory_type): EFI memory-type
+ * @reserved: unused
+ * @physical_start: Start address of region in physical memory
+ * @virtual_start: Start address of region in virtual memory, which will be the
+ *	same as @physical_start before where both addresses will always be the
+ *	same before SetVirtualMemoryMap() is called as the UEFI specification
+ *	requires identity mapping.
+ * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
+ *	bytes)
+ * @attribute: Memory attributes (see EFI_MEMORY_...)
+ */
 struct efi_mem_desc {
 	u32 type;
 	u32 reserved;
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 39809eac1bc..ee0cdd36500 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -788,8 +788,10 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
 /**
  * efi_add_memory_map_pg() - add pages to the memory map
  *
- * @start:			start address, must be a multiple of
- *				EFI_PAGE_SIZE
+ * @start: start address, must be a multiple of EFI_PAGE_SIZE. Note that this
+ * is actually a pointer provided as an integer. To pass in an address, pass
+ * in (ulong)map_to_sysmem(addr)
+ *
  * @pages:			number of pages to add
  * @memory_type:		type of memory added
  * @overlap_conventional:	region may only overlap free(conventional)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index e493934c713..f1154f73e05 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -384,7 +384,9 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 /**
  * efi_add_memory_map() - add memory area to the memory map
  *
- * @start:		start address of the memory area
+ * @start:		start address of the memory area. Note that this is
+ *			actually a pointer provided as an integer. To pass in
+ *			an address, pass in (ulong)map_to_sysmem(addr)
  * @size:		length in bytes of the memory area
  * @memory_type:	type of memory added
  *
-- 
2.43.0

[PATCH v4 02/25] efi_loader: Fix typos in enum efi_allocate_type

[Simon Glass]

Fix 'indicatged' and 'adress' typos.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

(no changes since v1)

 include/efi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/efi.h b/include/efi.h
index 348ff9efd16..76feefe6c50 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -175,7 +175,7 @@ enum efi_allocate_type {
 	EFI_ALLOCATE_MAX_ADDRESS,
 	/**
 	 * @EFI_ALLOCATE_ADDRESS:
-	 * Allocate a memory block starting at the indicatged adress.
+	 * Allocate a memory block starting at the indicated address.
 	 */
 	EFI_ALLOCATE_ADDRESS,
 	/**
-- 
2.43.0

Re: [PATCH v4 02/25] efi_loader: Fix typos in enum efi_allocate_type

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]

On Sun, Dec 01, 2024 at 08:24:21AM -0700, Simon Glass wrote:

> Fix 'indicatged' and 'adress' typos.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v4 03/25] efi_loader: Drop extra brackets in efi_mem_carve_out()

[Simon Glass]

Simplify a few expressions in this function.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_memory.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index f1154f73e05..3b1c7528e92 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -206,11 +206,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
 			     (carve_desc->num_pages << EFI_PAGE_SHIFT);
 
 	/* check whether we're overlapping */
-	if ((carve_end <= map_start) || (carve_start >= map_end))
+	if (carve_end <= map_start || carve_start >= map_end)
 		return EFI_CARVE_NO_OVERLAP;
 
 	/* We're overlapping with non-RAM, warn the caller if desired */
-	if (overlap_conventional && (map_desc->type != EFI_CONVENTIONAL_MEMORY))
+	if (overlap_conventional && map_desc->type != EFI_CONVENTIONAL_MEMORY)
 		return EFI_CARVE_OVERLAPS_NONRAM;
 
 	/* Sanitize carve_start and carve_end to lie within our bounds */
-- 
2.43.0

Re: [PATCH v4 03/25] efi_loader: Drop extra brackets in efi_mem_carve_out()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]

On Sun, Dec 01, 2024 at 08:24:22AM -0700, Simon Glass wrote:
> Simplify a few expressions in this function.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  lib/efi_loader/efi_memory.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index f1154f73e05..3b1c7528e92 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -206,11 +206,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
>  			     (carve_desc->num_pages << EFI_PAGE_SHIFT);
>  
>  	/* check whether we're overlapping */
> -	if ((carve_end <= map_start) || (carve_start >= map_end))
> +	if (carve_end <= map_start || carve_start >= map_end)
>  		return EFI_CARVE_NO_OVERLAP;
>  
>  	/* We're overlapping with non-RAM, warn the caller if desired */
> -	if (overlap_conventional && (map_desc->type != EFI_CONVENTIONAL_MEMORY))
> +	if (overlap_conventional && map_desc->type != EFI_CONVENTIONAL_MEMORY)
>  		return EFI_CARVE_OVERLAPS_NONRAM;
>  
>  	/* Sanitize carve_start and carve_end to lie within our bounds */

As I believe was mentioned in a previous iteration, please drop this as
they aren't excessive generates a compiler warning, merely for
clarification and should be kept.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 03/25] efi_loader: Drop extra brackets in efi_mem_carve_out()

[Simon Glass]

Hi Tom,

On Mon, 2 Dec 2024 at 13:16, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Dec 01, 2024 at 08:24:22AM -0700, Simon Glass wrote:
> > Simplify a few expressions in this function.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  lib/efi_loader/efi_memory.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > index f1154f73e05..3b1c7528e92 100644
> > --- a/lib/efi_loader/efi_memory.c
> > +++ b/lib/efi_loader/efi_memory.c
> > @@ -206,11 +206,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
> >                            (carve_desc->num_pages << EFI_PAGE_SHIFT);
> >
> >       /* check whether we're overlapping */
> > -     if ((carve_end <= map_start) || (carve_start >= map_end))
> > +     if (carve_end <= map_start || carve_start >= map_end)
> >               return EFI_CARVE_NO_OVERLAP;
> >
> >       /* We're overlapping with non-RAM, warn the caller if desired */
> > -     if (overlap_conventional && (map_desc->type != EFI_CONVENTIONAL_MEMORY))
> > +     if (overlap_conventional && map_desc->type != EFI_CONVENTIONAL_MEMORY)
> >               return EFI_CARVE_OVERLAPS_NONRAM;
> >
> >       /* Sanitize carve_start and carve_end to lie within our bounds */
>
> As I believe was mentioned in a previous iteration, please drop this as
> they aren't excessive generates a compiler warning, merely for
> clarification and should be kept.

I did this patch because checkpatch complained and I am changing these lines.

Regards,
Simon

Re: [PATCH v4 03/25] efi_loader: Drop extra brackets in efi_mem_carve_out()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]

On Mon, Dec 02, 2024 at 05:24:35PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 2 Dec 2024 at 13:16, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Dec 01, 2024 at 08:24:22AM -0700, Simon Glass wrote:
> > > Simplify a few expressions in this function.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  lib/efi_loader/efi_memory.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > index f1154f73e05..3b1c7528e92 100644
> > > --- a/lib/efi_loader/efi_memory.c
> > > +++ b/lib/efi_loader/efi_memory.c
> > > @@ -206,11 +206,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
> > >                            (carve_desc->num_pages << EFI_PAGE_SHIFT);
> > >
> > >       /* check whether we're overlapping */
> > > -     if ((carve_end <= map_start) || (carve_start >= map_end))
> > > +     if (carve_end <= map_start || carve_start >= map_end)
> > >               return EFI_CARVE_NO_OVERLAP;
> > >
> > >       /* We're overlapping with non-RAM, warn the caller if desired */
> > > -     if (overlap_conventional && (map_desc->type != EFI_CONVENTIONAL_MEMORY))
> > > +     if (overlap_conventional && map_desc->type != EFI_CONVENTIONAL_MEMORY)
> > >               return EFI_CARVE_OVERLAPS_NONRAM;
> > >
> > >       /* Sanitize carve_start and carve_end to lie within our bounds */
> >
> > As I believe was mentioned in a previous iteration, please drop this as
> > they aren't excessive generates a compiler warning, merely for
> > clarification and should be kept.
> 
> I did this patch because checkpatch complained and I am changing these lines.

And checkpatch is not the authority, it's guidelines. I believe the
review comments are "no, these should stay". Please drop this patch.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 03/25] efi_loader: Drop extra brackets in efi_mem_carve_out()

[Simon Glass]

Hi Tom,

On Mon, 2 Dec 2024 at 17:37, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Dec 02, 2024 at 05:24:35PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 2 Dec 2024 at 13:16, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sun, Dec 01, 2024 at 08:24:22AM -0700, Simon Glass wrote:
> > > > Simplify a few expressions in this function.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  lib/efi_loader/efi_memory.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > > index f1154f73e05..3b1c7528e92 100644
> > > > --- a/lib/efi_loader/efi_memory.c
> > > > +++ b/lib/efi_loader/efi_memory.c
> > > > @@ -206,11 +206,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
> > > >                            (carve_desc->num_pages << EFI_PAGE_SHIFT);
> > > >
> > > >       /* check whether we're overlapping */
> > > > -     if ((carve_end <= map_start) || (carve_start >= map_end))
> > > > +     if (carve_end <= map_start || carve_start >= map_end)
> > > >               return EFI_CARVE_NO_OVERLAP;
> > > >
> > > >       /* We're overlapping with non-RAM, warn the caller if desired */
> > > > -     if (overlap_conventional && (map_desc->type != EFI_CONVENTIONAL_MEMORY))
> > > > +     if (overlap_conventional && map_desc->type != EFI_CONVENTIONAL_MEMORY)
> > > >               return EFI_CARVE_OVERLAPS_NONRAM;
> > > >
> > > >       /* Sanitize carve_start and carve_end to lie within our bounds */
> > >
> > > As I believe was mentioned in a previous iteration, please drop this as
> > > they aren't excessive generates a compiler warning, merely for
> > > clarification and should be kept.
> >
> > I did this patch because checkpatch complained and I am changing these lines.
>
> And checkpatch is not the authority, it's guidelines.

Agreed.

> I believe the
> review comments are "no, these should stay". Please drop this patch.

Just so I can figure out what to do here, are you saying:
- merge this patch in with the one that produces a checkpatch warning
(i.e. remove brackets so resolve warning), or
- drop this patch and ignore the checkpatch warning in the result

I don't really mind about this, obviously. But as I suspect this
series is not going to be applied to your tree anyway, I'll await
events.

Regards,
Simon

Re: [PATCH v4 03/25] efi_loader: Drop extra brackets in efi_mem_carve_out()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2971 bytes --]

On Tue, Dec 03, 2024 at 06:46:11AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 2 Dec 2024 at 17:37, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Dec 02, 2024 at 05:24:35PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 2 Dec 2024 at 13:16, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sun, Dec 01, 2024 at 08:24:22AM -0700, Simon Glass wrote:
> > > > > Simplify a few expressions in this function.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >
> > > > > (no changes since v1)
> > > > >
> > > > >  lib/efi_loader/efi_memory.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > > > index f1154f73e05..3b1c7528e92 100644
> > > > > --- a/lib/efi_loader/efi_memory.c
> > > > > +++ b/lib/efi_loader/efi_memory.c
> > > > > @@ -206,11 +206,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
> > > > >                            (carve_desc->num_pages << EFI_PAGE_SHIFT);
> > > > >
> > > > >       /* check whether we're overlapping */
> > > > > -     if ((carve_end <= map_start) || (carve_start >= map_end))
> > > > > +     if (carve_end <= map_start || carve_start >= map_end)
> > > > >               return EFI_CARVE_NO_OVERLAP;
> > > > >
> > > > >       /* We're overlapping with non-RAM, warn the caller if desired */
> > > > > -     if (overlap_conventional && (map_desc->type != EFI_CONVENTIONAL_MEMORY))
> > > > > +     if (overlap_conventional && map_desc->type != EFI_CONVENTIONAL_MEMORY)
> > > > >               return EFI_CARVE_OVERLAPS_NONRAM;
> > > > >
> > > > >       /* Sanitize carve_start and carve_end to lie within our bounds */
> > > >
> > > > As I believe was mentioned in a previous iteration, please drop this as
> > > > they aren't excessive generates a compiler warning, merely for
> > > > clarification and should be kept.
> > >
> > > I did this patch because checkpatch complained and I am changing these lines.
> >
> > And checkpatch is not the authority, it's guidelines.
> 
> Agreed.
> 
> > I believe the
> > review comments are "no, these should stay". Please drop this patch.
> 
> Just so I can figure out what to do here, are you saying:
> - merge this patch in with the one that produces a checkpatch warning
> (i.e. remove brackets so resolve warning), or
> - drop this patch and ignore the checkpatch warning in the result

Yes, please just drop this patch, I only look at ERROR from checkpatch
when merging things and then it's a matter of what the error is.

> I don't really mind about this, obviously. But as I suspect this
> series is not going to be applied to your tree anyway, I'll await
> events.

Yes, if you're not going to be addressing most of the comments you get
this is going to linger in, well, whatever it is you plan to be doing
exactly.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 03/25] efi_loader: Drop extra brackets in efi_mem_carve_out()

[Simon Glass]

Hi Tom,

On Tue, 3 Dec 2024 at 07:04, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Dec 03, 2024 at 06:46:11AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 2 Dec 2024 at 17:37, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Mon, Dec 02, 2024 at 05:24:35PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 2 Dec 2024 at 13:16, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sun, Dec 01, 2024 at 08:24:22AM -0700, Simon Glass wrote:
> > > > > > Simplify a few expressions in this function.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > (no changes since v1)
> > > > > >
> > > > > >  lib/efi_loader/efi_memory.c | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > > > > index f1154f73e05..3b1c7528e92 100644
> > > > > > --- a/lib/efi_loader/efi_memory.c
> > > > > > +++ b/lib/efi_loader/efi_memory.c
> > > > > > @@ -206,11 +206,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
> > > > > >                            (carve_desc->num_pages << EFI_PAGE_SHIFT);
> > > > > >
> > > > > >       /* check whether we're overlapping */
> > > > > > -     if ((carve_end <= map_start) || (carve_start >= map_end))
> > > > > > +     if (carve_end <= map_start || carve_start >= map_end)
> > > > > >               return EFI_CARVE_NO_OVERLAP;
> > > > > >
> > > > > >       /* We're overlapping with non-RAM, warn the caller if desired */
> > > > > > -     if (overlap_conventional && (map_desc->type != EFI_CONVENTIONAL_MEMORY))
> > > > > > +     if (overlap_conventional && map_desc->type != EFI_CONVENTIONAL_MEMORY)
> > > > > >               return EFI_CARVE_OVERLAPS_NONRAM;
> > > > > >
> > > > > >       /* Sanitize carve_start and carve_end to lie within our bounds */
> > > > >
> > > > > As I believe was mentioned in a previous iteration, please drop this as
> > > > > they aren't excessive generates a compiler warning, merely for
> > > > > clarification and should be kept.
> > > >
> > > > I did this patch because checkpatch complained and I am changing these lines.
> > >
> > > And checkpatch is not the authority, it's guidelines.
> >
> > Agreed.
> >
> > > I believe the
> > > review comments are "no, these should stay". Please drop this patch.
> >
> > Just so I can figure out what to do here, are you saying:
> > - merge this patch in with the one that produces a checkpatch warning
> > (i.e. remove brackets so resolve warning), or
> > - drop this patch and ignore the checkpatch warning in the result
>
> Yes, please just drop this patch, I only look at ERROR from checkpatch
> when merging things and then it's a matter of what the error is.
>
> > I don't really mind about this, obviously. But as I suspect this
> > series is not going to be applied to your tree anyway, I'll await
> > events.
>
> Yes, if you're not going to be addressing most of the comments you get
> this is going to linger in, well, whatever it is you plan to be doing
> exactly.

I am keen to address comments, so long as they are about the code,
rather than 'we don't want this patch'.

Regards,
SImon

Re: [PATCH v4 03/25] efi_loader: Drop extra brackets in efi_mem_carve_out()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3649 bytes --]

On Tue, Dec 03, 2024 at 08:53:31AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 3 Dec 2024 at 07:04, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Dec 03, 2024 at 06:46:11AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 2 Dec 2024 at 17:37, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Mon, Dec 02, 2024 at 05:24:35PM -0700, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Mon, 2 Dec 2024 at 13:16, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Sun, Dec 01, 2024 at 08:24:22AM -0700, Simon Glass wrote:
> > > > > > > Simplify a few expressions in this function.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > > (no changes since v1)
> > > > > > >
> > > > > > >  lib/efi_loader/efi_memory.c | 4 ++--
> > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > > > > > index f1154f73e05..3b1c7528e92 100644
> > > > > > > --- a/lib/efi_loader/efi_memory.c
> > > > > > > +++ b/lib/efi_loader/efi_memory.c
> > > > > > > @@ -206,11 +206,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
> > > > > > >                            (carve_desc->num_pages << EFI_PAGE_SHIFT);
> > > > > > >
> > > > > > >       /* check whether we're overlapping */
> > > > > > > -     if ((carve_end <= map_start) || (carve_start >= map_end))
> > > > > > > +     if (carve_end <= map_start || carve_start >= map_end)
> > > > > > >               return EFI_CARVE_NO_OVERLAP;
> > > > > > >
> > > > > > >       /* We're overlapping with non-RAM, warn the caller if desired */
> > > > > > > -     if (overlap_conventional && (map_desc->type != EFI_CONVENTIONAL_MEMORY))
> > > > > > > +     if (overlap_conventional && map_desc->type != EFI_CONVENTIONAL_MEMORY)
> > > > > > >               return EFI_CARVE_OVERLAPS_NONRAM;
> > > > > > >
> > > > > > >       /* Sanitize carve_start and carve_end to lie within our bounds */
> > > > > >
> > > > > > As I believe was mentioned in a previous iteration, please drop this as
> > > > > > they aren't excessive generates a compiler warning, merely for
> > > > > > clarification and should be kept.
> > > > >
> > > > > I did this patch because checkpatch complained and I am changing these lines.
> > > >
> > > > And checkpatch is not the authority, it's guidelines.
> > >
> > > Agreed.
> > >
> > > > I believe the
> > > > review comments are "no, these should stay". Please drop this patch.
> > >
> > > Just so I can figure out what to do here, are you saying:
> > > - merge this patch in with the one that produces a checkpatch warning
> > > (i.e. remove brackets so resolve warning), or
> > > - drop this patch and ignore the checkpatch warning in the result
> >
> > Yes, please just drop this patch, I only look at ERROR from checkpatch
> > when merging things and then it's a matter of what the error is.
> >
> > > I don't really mind about this, obviously. But as I suspect this
> > > series is not going to be applied to your tree anyway, I'll await
> > > events.
> >
> > Yes, if you're not going to be addressing most of the comments you get
> > this is going to linger in, well, whatever it is you plan to be doing
> > exactly.
> 
> I am keen to address comments, so long as they are about the code,
> rather than 'we don't want this patch'.

We don't want this patch because the parenthesis do make reviewing the
code and order of operations clearer. This was the previous feedback.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 03/25] efi_loader: Drop extra brackets in efi_mem_carve_out()

[Simon Glass]

Hi Tom,

On Tue, 3 Dec 2024 at 09:03, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Dec 03, 2024 at 08:53:31AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 3 Dec 2024 at 07:04, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Dec 03, 2024 at 06:46:11AM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 2 Dec 2024 at 17:37, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Mon, Dec 02, 2024 at 05:24:35PM -0700, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Mon, 2 Dec 2024 at 13:16, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Sun, Dec 01, 2024 at 08:24:22AM -0700, Simon Glass wrote:
> > > > > > > > Simplify a few expressions in this function.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > (no changes since v1)
> > > > > > > >
> > > > > > > >  lib/efi_loader/efi_memory.c | 4 ++--
> > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > > > > > > index f1154f73e05..3b1c7528e92 100644
> > > > > > > > --- a/lib/efi_loader/efi_memory.c
> > > > > > > > +++ b/lib/efi_loader/efi_memory.c
> > > > > > > > @@ -206,11 +206,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
> > > > > > > >                            (carve_desc->num_pages << EFI_PAGE_SHIFT);
> > > > > > > >
> > > > > > > >       /* check whether we're overlapping */
> > > > > > > > -     if ((carve_end <= map_start) || (carve_start >= map_end))
> > > > > > > > +     if (carve_end <= map_start || carve_start >= map_end)
> > > > > > > >               return EFI_CARVE_NO_OVERLAP;
> > > > > > > >
> > > > > > > >       /* We're overlapping with non-RAM, warn the caller if desired */
> > > > > > > > -     if (overlap_conventional && (map_desc->type != EFI_CONVENTIONAL_MEMORY))
> > > > > > > > +     if (overlap_conventional && map_desc->type != EFI_CONVENTIONAL_MEMORY)
> > > > > > > >               return EFI_CARVE_OVERLAPS_NONRAM;
> > > > > > > >
> > > > > > > >       /* Sanitize carve_start and carve_end to lie within our bounds */
> > > > > > >
> > > > > > > As I believe was mentioned in a previous iteration, please drop this as
> > > > > > > they aren't excessive generates a compiler warning, merely for
> > > > > > > clarification and should be kept.
> > > > > >
> > > > > > I did this patch because checkpatch complained and I am changing these lines.
> > > > >
> > > > > And checkpatch is not the authority, it's guidelines.
> > > >
> > > > Agreed.
> > > >
> > > > > I believe the
> > > > > review comments are "no, these should stay". Please drop this patch.
> > > >
> > > > Just so I can figure out what to do here, are you saying:
> > > > - merge this patch in with the one that produces a checkpatch warning
> > > > (i.e. remove brackets so resolve warning), or
> > > > - drop this patch and ignore the checkpatch warning in the result
> > >
> > > Yes, please just drop this patch, I only look at ERROR from checkpatch
> > > when merging things and then it's a matter of what the error is.
> > >
> > > > I don't really mind about this, obviously. But as I suspect this
> > > > series is not going to be applied to your tree anyway, I'll await
> > > > events.
> > >
> > > Yes, if you're not going to be addressing most of the comments you get
> > > this is going to linger in, well, whatever it is you plan to be doing
> > > exactly.
> >
> > I am keen to address comments, so long as they are about the code,
> > rather than 'we don't want this patch'.
>
> We don't want this patch because the parenthesis do make reviewing the
> code and order of operations clearer. This was the previous feedback.

Given that this whole series is being rejected, I don't think this
feedback is critical.

Regards,
SImon

[PATCH v4 04/25] efi_loader: Add comments where incorrect addresses are used

[Simon Glass]

Some functions are passing addresses instead of pointers to the
efi_add_memory_map() function. This confusion is understandable since
the function arguments indicate an address.

Make a note of the 8 places where there are problems, which would break
usage in sandbox tests.

Future work will resolve these problems.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v2)

Changes in v2:
- Add a new patch with comments where incorrect addresses are used

 lib/efi_loader/efi_acpi.c    | 2 ++
 lib/efi_loader/efi_bootmgr.c | 5 +++++
 lib/efi_loader/efi_helper.c  | 1 +
 lib/efi_loader/efi_smbios.c  | 6 +++++-
 lib/efi_loader/efi_var_mem.c | 2 ++
 5 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
index 67bd7f8ca24..ffb360d461b 100644
--- a/lib/efi_loader/efi_acpi.c
+++ b/lib/efi_loader/efi_acpi.c
@@ -28,12 +28,14 @@ efi_status_t efi_acpi_register(void)
 	/* Mark space used for tables */
 	start = ALIGN_DOWN(gd->arch.table_start, EFI_PAGE_MASK);
 	end = ALIGN(gd->arch.table_end, EFI_PAGE_MASK);
+	/* TODO(sjg): This should use (ulong)map_sysmem(start, ...) */
 	ret = efi_add_memory_map(start, end - start, EFI_ACPI_RECLAIM_MEMORY);
 	if (ret != EFI_SUCCESS)
 		return ret;
 	if (gd->arch.table_start_high) {
 		start = ALIGN_DOWN(gd->arch.table_start_high, EFI_PAGE_MASK);
 		end = ALIGN(gd->arch.table_end_high, EFI_PAGE_MASK);
+		/* TODO(sjg): This should use (ulong)map_sysmem(start, ...) */
 		ret = efi_add_memory_map(start, end - start,
 					 EFI_ACPI_RECLAIM_MEMORY);
 		if (ret != EFI_SUCCESS)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 8c51a6ef2ed..bb635d77b53 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -365,6 +365,8 @@ static efi_status_t prepare_loaded_image(u16 *label, ulong addr, ulong size,
 	 * TODO: expose the ramdisk to OS.
 	 * Need to pass the ramdisk information by the architecture-specific
 	 * methods such as 'pmem' device-tree node.
+	 *
+	 * TODO(sjg): This should use (ulong)map_sysmem(addr)
 	 */
 	ret = efi_add_memory_map(addr, size, EFI_RESERVED_MEMORY_TYPE);
 	if (ret != EFI_SUCCESS) {
@@ -399,6 +401,7 @@ static efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
 
 	/* cleanup for iso or img image */
 	if (ctx->ramdisk_blk_dev) {
+		/* TODO(sjg): This should use (ulong)map_sysmem(...) */
 		ret = efi_add_memory_map(ctx->image_addr, ctx->image_size,
 					 EFI_CONVENTIONAL_MEMORY);
 		if (ret != EFI_SUCCESS)
@@ -506,6 +509,8 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
 
 		source_buffer = NULL;
 		source_size = 0;
+
+	/* TODO(sjg): This does not work on sandbox */
 	} else if (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) {
 		/*
 		 * loaded_dp must exist until efi application returns,
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
index bf96f61d3d0..fd89ef6036f 100644
--- a/lib/efi_loader/efi_helper.c
+++ b/lib/efi_loader/efi_helper.c
@@ -486,6 +486,7 @@ static efi_status_t copy_fdt(void **fdtp)
 		log_err("Failed to reserve space for FDT\n");
 		goto done;
 	}
+	/* TODO(sjg): This does not work on sandbox */
 	new_fdt = (void *)(uintptr_t)new_fdt_addr;
 	memcpy(new_fdt, fdt, fdt_totalsize(fdt));
 	fdt_set_totalsize(new_fdt, fdt_size);
diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index 8d2ef6deb51..02d4a5dd045 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -40,7 +40,11 @@ efi_status_t efi_smbios_register(void)
 		return EFI_NOT_FOUND;
 	}
 
-	/* Mark space used for tables */
+	/*
+	 * Mark space used for tables/
+	 *
+	 * TODO(sjg): This should use (ulong)map_sysmem(addr, ...)
+	 */
 	ret = efi_add_memory_map(addr, TABLE_SIZE, EFI_RUNTIME_SERVICES_DATA);
 	if (ret)
 		return ret;
diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
index 139e16aad7c..5c69a1e0f3e 100644
--- a/lib/efi_loader/efi_var_mem.c
+++ b/lib/efi_loader/efi_var_mem.c
@@ -226,6 +226,8 @@ efi_status_t efi_var_mem_init(void)
 				 &memory);
 	if (ret != EFI_SUCCESS)
 		return ret;
+
+	/* TODO(sjg): This does not work on sandbox */
 	efi_var_buf = (struct efi_var_file *)(uintptr_t)memory;
 	memset(efi_var_buf, 0, EFI_VAR_BUF_SIZE);
 	efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
-- 
2.43.0

Re: [PATCH v4 04/25] efi_loader: Add comments where incorrect addresses are used

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

On Sun, Dec 01, 2024 at 08:24:23AM -0700, Simon Glass wrote:

> Some functions are passing addresses instead of pointers to the
> efi_add_memory_map() function. This confusion is understandable since
> the function arguments indicate an address.
> 
> Make a note of the 8 places where there are problems, which would break
> usage in sandbox tests.
> 
> Future work will resolve these problems.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Please just resolve these rather than introducing a patch to then fix
them later. This is something that should have been fixup'd before
posting. Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 04/25] efi_loader: Add comments where incorrect addresses are used

[Simon Glass]

Hi Tom,

On Mon, 2 Dec 2024 at 13:18, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Dec 01, 2024 at 08:24:23AM -0700, Simon Glass wrote:
>
> > Some functions are passing addresses instead of pointers to the
> > efi_add_memory_map() function. This confusion is understandable since
> > the function arguments indicate an address.
> >
> > Make a note of the 8 places where there are problems, which would break
> > usage in sandbox tests.
> >
> > Future work will resolve these problems.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Please just resolve these rather than introducing a patch to then fix
> them later. This is something that should have been fixup'd before
> posting. Thanks.

That was deliberate, as I wanted people to see the problems. It will
save discussion on later patches where the problems are fixed, if we
can agree that these are actual problems. If people are happy to add
review tags to the later patches then I'm happy to redo it.

Regards,
Simon

Re: [PATCH v4 04/25] efi_loader: Add comments where incorrect addresses are used

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

On Mon, Dec 02, 2024 at 05:22:44PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 2 Dec 2024 at 13:18, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Dec 01, 2024 at 08:24:23AM -0700, Simon Glass wrote:
> >
> > > Some functions are passing addresses instead of pointers to the
> > > efi_add_memory_map() function. This confusion is understandable since
> > > the function arguments indicate an address.
> > >
> > > Make a note of the 8 places where there are problems, which would break
> > > usage in sandbox tests.
> > >
> > > Future work will resolve these problems.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > Please just resolve these rather than introducing a patch to then fix
> > them later. This is something that should have been fixup'd before
> > posting. Thanks.
> 
> That was deliberate, as I wanted people to see the problems. It will
> save discussion on later patches where the problems are fixed, if we
> can agree that these are actual problems. If people are happy to add
> review tags to the later patches then I'm happy to redo it.

No, please drop this patch. A "TODO: This is wrong" in several places
and then changing it in another patch does not help.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 04/25] efi_loader: Add comments where incorrect addresses are used

[Ilias Apalodimas]

Hi Simon,

On Tue, 3 Dec 2024 at 02:22, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tom,
>
> On Mon, 2 Dec 2024 at 13:18, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Dec 01, 2024 at 08:24:23AM -0700, Simon Glass wrote:
> >
> > > Some functions are passing addresses instead of pointers to the
> > > efi_add_memory_map() function. This confusion is understandable since
> > > the function arguments indicate an address.
> > >
> > > Make a note of the 8 places where there are problems, which would break
> > > usage in sandbox tests.
> > >
> > > Future work will resolve these problems.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > Please just resolve these rather than introducing a patch to then fix
> > them later. This is something that should have been fixup'd before
> > posting. Thanks.
>
> That was deliberate, as I wanted people to see the problems. It will
> save discussion on later patches where the problems are fixed, if we
> can agree that these are actual problems. If people are happy to add
> review tags to the later patches then I'm happy to redo it.

I am pretty sure Heinrich has repeated this in the past. Why do we
have to sprinkle around map_sysmem/unmap sysmem for sandbox?
Polluting the entire u-boot to support a special platform is less than
ideal. Why can't sandbox limit this internally and do whatever
mappings it needs when it receives an address?

Thanks
/Ilias
>
> Regards,
> Simon

Re: [PATCH v4 04/25] efi_loader: Add comments where incorrect addresses are used

[Simon Glass]

Hi Ilias,

On Tue, 3 Dec 2024 at 01:49, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Tue, 3 Dec 2024 at 02:22, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Tom,
> >
> > On Mon, 2 Dec 2024 at 13:18, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sun, Dec 01, 2024 at 08:24:23AM -0700, Simon Glass wrote:
> > >
> > > > Some functions are passing addresses instead of pointers to the
> > > > efi_add_memory_map() function. This confusion is understandable since
> > > > the function arguments indicate an address.
> > > >
> > > > Make a note of the 8 places where there are problems, which would break
> > > > usage in sandbox tests.
> > > >
> > > > Future work will resolve these problems.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > >
> > > Please just resolve these rather than introducing a patch to then fix
> > > them later. This is something that should have been fixup'd before
> > > posting. Thanks.
> >
> > That was deliberate, as I wanted people to see the problems. It will
> > save discussion on later patches where the problems are fixed, if we
> > can agree that these are actual problems. If people are happy to add
> > review tags to the later patches then I'm happy to redo it.
>
> I am pretty sure Heinrich has repeated this in the past. Why do we
> have to sprinkle around map_sysmem/unmap sysmem for sandbox?
> Polluting the entire u-boot to support a special platform is less than
> ideal. Why can't sandbox limit this internally and do whatever
> mappings it needs when it receives an address?

Tom, I suppose I have made my point.

Ilias, this is documented at [1] and has been the same for 10 years.

See for example the 'md' command, do_mem_md(), which shows how 'md 0'
is implemented in sandbox.

As I have mentioned before, the nice thing is that you can easily make
code work with sandbox just by converting casts from
address-to-pointer into map_sysmem().

Back to this series, if you look at efi_allocate_pages() you'll
currently see three calls (two map_to_sysmem() one map_sysmem(), but
now, with this series, there is none. It also makes it a lot easier to
understand what is going on, IMO.

Regards,
Simon

[1] https://docs.u-boot.org/en/latest/arch/sandbox/sandbox.html#memory-emulation

Re: [PATCH v4 04/25] efi_loader: Add comments where incorrect addresses are used

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3174 bytes --]

On Tue, Dec 03, 2024 at 06:45:58AM -0700, Simon Glass wrote:
> Hi Ilias,
> 
> On Tue, 3 Dec 2024 at 01:49, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Tue, 3 Dec 2024 at 02:22, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Tom,
> > >
> > > On Mon, 2 Dec 2024 at 13:18, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sun, Dec 01, 2024 at 08:24:23AM -0700, Simon Glass wrote:
> > > >
> > > > > Some functions are passing addresses instead of pointers to the
> > > > > efi_add_memory_map() function. This confusion is understandable since
> > > > > the function arguments indicate an address.
> > > > >
> > > > > Make a note of the 8 places where there are problems, which would break
> > > > > usage in sandbox tests.
> > > > >
> > > > > Future work will resolve these problems.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > >
> > > > Please just resolve these rather than introducing a patch to then fix
> > > > them later. This is something that should have been fixup'd before
> > > > posting. Thanks.
> > >
> > > That was deliberate, as I wanted people to see the problems. It will
> > > save discussion on later patches where the problems are fixed, if we
> > > can agree that these are actual problems. If people are happy to add
> > > review tags to the later patches then I'm happy to redo it.
> >
> > I am pretty sure Heinrich has repeated this in the past. Why do we
> > have to sprinkle around map_sysmem/unmap sysmem for sandbox?
> > Polluting the entire u-boot to support a special platform is less than
> > ideal. Why can't sandbox limit this internally and do whatever
> > mappings it needs when it receives an address?
> 
> Tom, I suppose I have made my point.
> 
> Ilias, this is documented at [1] and has been the same for 10 years.
> 
> See for example the 'md' command, do_mem_md(), which shows how 'md 0'
> is implemented in sandbox.
> 
> As I have mentioned before, the nice thing is that you can easily make
> code work with sandbox just by converting casts from
> address-to-pointer into map_sysmem().
> 
> Back to this series, if you look at efi_allocate_pages() you'll
> currently see three calls (two map_to_sysmem() one map_sysmem(), but
> now, with this series, there is none. It also makes it a lot easier to
> understand what is going on, IMO.
> 
> Regards,
> Simon
> 
> [1] https://docs.u-boot.org/en/latest/arch/sandbox/sandbox.html#memory-emulation

This is something I was wondering about yesterday in fact. Especially
these days when "everyone" is developing on machines with 8GB or more of
memory, why does sandbox not just malloc 512MB/1GB/2GB/4GB (make it a
switch?) and pretend that area is memory starting at some address.
Especially if it's less than 4GB and starts at 0x80000000 more "memory
starts at 0" bugs would be found and fixed. I swear we did something
like that in my OS class 20+ years ago. I'm not sure what your link is
meant for, as it doesn't say why sandbox works the way it does, merely
that it works this way. Not that it can't work some other way.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 04/25] efi_loader: Add comments where incorrect addresses are used

[Simon Glass]

Hi Tom,

On Tue, 3 Dec 2024 at 07:09, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Dec 03, 2024 at 06:45:58AM -0700, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Tue, 3 Dec 2024 at 01:49, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, 3 Dec 2024 at 02:22, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Tom,
> > > >
> > > > On Mon, 2 Dec 2024 at 13:18, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sun, Dec 01, 2024 at 08:24:23AM -0700, Simon Glass wrote:
> > > > >
> > > > > > Some functions are passing addresses instead of pointers to the
> > > > > > efi_add_memory_map() function. This confusion is understandable since
> > > > > > the function arguments indicate an address.
> > > > > >
> > > > > > Make a note of the 8 places where there are problems, which would break
> > > > > > usage in sandbox tests.
> > > > > >
> > > > > > Future work will resolve these problems.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > >
> > > > > Please just resolve these rather than introducing a patch to then fix
> > > > > them later. This is something that should have been fixup'd before
> > > > > posting. Thanks.
> > > >
> > > > That was deliberate, as I wanted people to see the problems. It will
> > > > save discussion on later patches where the problems are fixed, if we
> > > > can agree that these are actual problems. If people are happy to add
> > > > review tags to the later patches then I'm happy to redo it.
> > >
> > > I am pretty sure Heinrich has repeated this in the past. Why do we
> > > have to sprinkle around map_sysmem/unmap sysmem for sandbox?
> > > Polluting the entire u-boot to support a special platform is less than
> > > ideal. Why can't sandbox limit this internally and do whatever
> > > mappings it needs when it receives an address?
> >
> > Tom, I suppose I have made my point.
> >
> > Ilias, this is documented at [1] and has been the same for 10 years.
> >
> > See for example the 'md' command, do_mem_md(), which shows how 'md 0'
> > is implemented in sandbox.
> >
> > As I have mentioned before, the nice thing is that you can easily make
> > code work with sandbox just by converting casts from
> > address-to-pointer into map_sysmem().
> >
> > Back to this series, if you look at efi_allocate_pages() you'll
> > currently see three calls (two map_to_sysmem() one map_sysmem(), but
> > now, with this series, there is none. It also makes it a lot easier to
> > understand what is going on, IMO.
> >
> > Regards,
> > Simon
> >
> > [1] https://docs.u-boot.org/en/latest/arch/sandbox/sandbox.html#memory-emulation
>
> This is something I was wondering about yesterday in fact. Especially
> these days when "everyone" is developing on machines with 8GB or more of
> memory, why does sandbox not just malloc 512MB/1GB/2GB/4GB (make it a
> switch?) and pretend that area is memory starting at some address.
> Especially if it's less than 4GB and starts at 0x80000000 more "memory
> starts at 0" bugs would be found and fixed. I swear we did something
> like that in my OS class 20+ years ago. I'm not sure what your link is
> meant for, as it doesn't say why sandbox works the way it does, merely
> that it works this way. Not that it can't work some other way.

It could work some other way, it's just that this seems to be the best way.

It is easier to have memory starting at address zero (many platforms
do), For example, tests can assume that and are easier to write,
particularly tests which check cmdline output.

But note that the actual pointer address is elsewhere, so we do get a
segfault if a null pointer is used. That has indeed found a lot of
bugs.

Using a mapping does make it easy to see when code has been set up for sandbox.

Some tests rely on memory being preserved across phases, so if memory
moves around in each phase, it gets pretty confusing. Also things like
kernel_addr_r become hard, and printing out addresses becomes very
confusing when every address is 0x555555599fbc ...

In my opinion, sandbox is an important and useful feature of U-Boot.
It is where I do almost all of my development and testing. We have >1k
tests. So I believe it is worth the cost of a little discipline around
address/pointer casting. What do you think?

Regards,
Simon

[PATCH v4 05/25] efi_loader: Show the resulting memory address from an alloc

[Simon Glass]

Update efi_allocate_pool_ext() to log the pointer returned from this
call, which can be helpful when debugging.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v3)

Changes in v3:
- Show the returned address rather than the pointer

Changes in v2:
- Use EFI_PRINT() instead of log_debug()

 lib/efi_loader/efi_boottime.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 080e7f78ae3..f27b3827ed2 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -13,6 +13,7 @@
 #include <irq_func.h>
 #include <log.h>
 #include <malloc.h>
+#include <mapmem.h>
 #include <pe.h>
 #include <time.h>
 #include <u-boot/crc.h>
@@ -511,6 +512,9 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
 
 	EFI_ENTRY("%d, %zu, %p", pool_type, size, buffer);
 	r = efi_allocate_pool(pool_type, size, buffer);
+	if (r == EFI_SUCCESS)
+		EFI_PRINT("*buffer = %llx\n", (u64)map_to_sysmem(*buffer));
+
 	return EFI_EXIT(r);
 }
 
-- 
2.43.0

[PATCH v4 06/25] efi_loader: Update startimage_exit self-test to check error

[Simon Glass]

Check for an error returned from the decompress() function, just in
case.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---

(no changes since v1)

 lib/efi_selftest/efi_selftest_startimage_exit.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/efi_selftest/efi_selftest_startimage_exit.c b/lib/efi_selftest/efi_selftest_startimage_exit.c
index b65a10b7a4b..8d119f054c5 100644
--- a/lib/efi_selftest/efi_selftest_startimage_exit.c
+++ b/lib/efi_selftest/efi_selftest_startimage_exit.c
@@ -84,13 +84,15 @@ static efi_status_t decompress(u8 **image)
 static int setup(const efi_handle_t handle,
 		 const struct efi_system_table *systable)
 {
+	efi_status_t ret;
+
 	image_handle = handle;
 	boottime = systable->boottime;
 
 	/* Load the application image into memory */
-	decompress(&image);
+	ret = decompress(&image);
 
-	return EFI_ST_SUCCESS;
+	return ret;
 }
 
 /*
-- 
2.43.0

[PATCH v4 07/25] efi_loader: Move some memory-function comments to header

[Simon Glass]

Exported functions should be documented in the header file, not the
implementation. We tend to make such updates on a piecemeal basis to
avoid a 'flag day'. Move some comments related to memory allocation to
follow the convention.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v2)

Changes in v2:
- Rebase on early patch

 include/efi_loader.h        | 77 +++++++++++++++++++++++++++++++----
 lib/efi_loader/efi_memory.c | 81 -------------------------------------
 2 files changed, 69 insertions(+), 89 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index ee0cdd36500..00a1259c006 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -758,21 +758,68 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
  * Return:	size in pages
  */
 #define efi_size_in_pages(size) (((size) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT)
-/* Allocate boot service data pool memory */
+
+/**
+ * efi_alloc() - allocate boot-services-data pool-memory
+ *
+ * Allocate memory from pool and zero it out.
+ *
+ * @size:	number of bytes to allocate
+ * Return:	pointer to allocated memory or NULL
+ */
 void *efi_alloc(size_t len);
-/* Allocate pages on the specified alignment */
+
+/**
+ * efi_alloc_aligned_pages() - allocate aligned memory pages
+ *
+ * @len:		len in bytes
+ * @memory_type:	usage type of the allocated memory
+ * @align:		alignment in bytes
+ * Return:		aligned memory or NULL
+ */
 void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align);
-/* More specific EFI memory allocator, called by EFI payloads */
+
+/**
+ * efi_allocate_pages - allocate memory pages
+ *
+ * @type:		type of allocation to be performed
+ * @memory_type:	usage type of the allocated memory
+ * @pages:		number of pages to be allocated
+ * @memory:		returns a pointer to the allocated memory
+ * Return:		status code
+ */
 efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 				enum efi_memory_type memory_type,
 				efi_uintn_t pages, uint64_t *memory);
-/* EFI memory free function. */
+
+/**
+ * efi_free_pages() - free memory pages
+ *
+ * @memory:	start of the memory area to be freed
+ * @pages:	number of pages to be freed
+ * Return:	status code
+ */
 efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages);
-/* EFI memory allocator for small allocations */
+
+/**
+ * efi_allocate_pool - allocate memory from pool
+ *
+ * @pool_type:	type of the pool from which memory is to be allocated
+ * @size:	number of bytes to be allocated
+ * @buffer:	allocated memory
+ * Return:	status code
+ */
 efi_status_t efi_allocate_pool(enum efi_memory_type pool_type,
 			       efi_uintn_t size, void **buffer);
-/* EFI pool memory free function. */
+
+/**
+ * efi_free_pool() - free memory from pool
+ *
+ * @buffer:	start of memory to be freed
+ * Return:	status code
+ */
 efi_status_t efi_free_pool(void *buffer);
+
 /* Allocate and retrieve EFI memory map */
 efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
 				      struct efi_mem_desc **memory_map);
@@ -782,7 +829,21 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 				efi_uintn_t *map_key,
 				efi_uintn_t *descriptor_size,
 				uint32_t *descriptor_version);
-/* Adds a range into the EFI memory map */
+
+/**
+ * efi_add_memory_map() - add memory area to the memory map
+ *
+ * @start:		start address of the memory area. Note that this is
+ *			actually a pointer provided as an integer. To pass in
+ *			an address, pass in (ulong)map_to_sysmem(addr)
+ * @size:		length in bytes of the memory area
+ * @memory_type:	type of memory added
+ *
+ * Return:		status code
+ *
+ * This function automatically aligns the start and size of the memory area
+ * to EFI_PAGE_SIZE.
+ */
 efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
 
 /**
@@ -793,7 +854,7 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
  * in (ulong)map_to_sysmem(addr)
  *
  * @pages:			number of pages to add
- * @memory_type:		type of memory added
+ * @memory_type:		EFI type of memory added
  * @overlap_conventional:	region may only overlap free(conventional)
  *				memory
  * Return:			status code
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 3b1c7528e92..9d0f27dbb3e 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -257,17 +257,6 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
 	return EFI_CARVE_LOOP_AGAIN;
 }
 
-/**
- * efi_add_memory_map_pg() - add pages to the memory map
- *
- * @start:			start address, must be a multiple of
- *				EFI_PAGE_SIZE
- * @pages:			number of pages to add
- * @memory_type:		type of memory added
- * @overlap_conventional:	region may only overlap free(conventional)
- *				memory
- * Return:			status code
- */
 efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 				   int memory_type,
 				   bool overlap_conventional)
@@ -381,20 +370,6 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 	return EFI_SUCCESS;
 }
 
-/**
- * efi_add_memory_map() - add memory area to the memory map
- *
- * @start:		start address of the memory area. Note that this is
- *			actually a pointer provided as an integer. To pass in
- *			an address, pass in (ulong)map_to_sysmem(addr)
- * @size:		length in bytes of the memory area
- * @memory_type:	type of memory added
- *
- * Return:		status code
- *
- * This function automatically aligns the start and size of the memory area
- * to EFI_PAGE_SIZE.
- */
 efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type)
 {
 	u64 pages;
@@ -440,15 +415,6 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated)
 	return EFI_NOT_FOUND;
 }
 
-/**
- * efi_allocate_pages - allocate memory pages
- *
- * @type:		type of allocation to be performed
- * @memory_type:	usage type of the allocated memory
- * @pages:		number of pages to be allocated
- * @memory:		allocated memory
- * Return:		status code
- */
 efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 				enum efi_memory_type memory_type,
 				efi_uintn_t pages, uint64_t *memory)
@@ -516,13 +482,6 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 	return EFI_SUCCESS;
 }
 
-/**
- * efi_free_pages() - free memory pages
- *
- * @memory:	start of the memory area to be freed
- * @pages:	number of pages to be freed
- * Return:	status code
- */
 efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
 {
 	u64 len;
@@ -556,14 +515,6 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
 	return ret;
 }
 
-/**
- * efi_alloc_aligned_pages() - allocate aligned memory pages
- *
- * @len:		len in bytes
- * @memory_type:	usage type of the allocated memory
- * @align:		alignment in bytes
- * Return:		aligned memory or NULL
- */
 void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align)
 {
 	u64 req_pages = efi_size_in_pages(len);
@@ -608,14 +559,6 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align)
 	return (void *)(uintptr_t)aligned_mem;
 }
 
-/**
- * efi_allocate_pool - allocate memory from pool
- *
- * @pool_type:	type of the pool from which memory is to be allocated
- * @size:	number of bytes to be allocated
- * @buffer:	allocated memory
- * Return:	status code
- */
 efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer)
 {
 	efi_status_t r;
@@ -644,14 +587,6 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
 	return r;
 }
 
-/**
- * efi_alloc() - allocate boot services data pool memory
- *
- * Allocate memory from pool and zero it out.
- *
- * @size:	number of bytes to allocate
- * Return:	pointer to allocated memory or NULL
- */
 void *efi_alloc(size_t size)
 {
 	void *buf;
@@ -666,12 +601,6 @@ void *efi_alloc(size_t size)
 	return buf;
 }
 
-/**
- * efi_free_pool() - free memory from pool
- *
- * @buffer:	start of memory to be freed
- * Return:	status code
- */
 efi_status_t efi_free_pool(void *buffer)
 {
 	efi_status_t ret;
@@ -793,16 +722,6 @@ efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
 	return ret;
 }
 
-/**
- * efi_add_known_memory() - add memory types to the EFI memory map
- *
- * This function is to be used to add different memory types other
- * than EFI_CONVENTIONAL_MEMORY to the EFI memory map. The conventional
- * memory is handled by the LMB module and gets added to the memory
- * map through the LMB module.
- *
- * This function may be overridden for architectures specific purposes.
- */
 __weak void efi_add_known_memory(void)
 {
 }
-- 
2.43.0

[PATCH v4 08/25] doc: efi: Add the EFI-loader API documentation

[Simon Glass]

Bring in the documentation from the efi_loader.h header file, so we can
see the API defined there.

Fix efi_alloc() to avoid a warning.

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

(no changes since v3)

Changes in v3:
- Put the header-file in its own section

Changes in v2:
- Add new patch to add the EFI-loader API documentation

 doc/api/efi.rst      | 6 ++++++
 include/efi_loader.h | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/doc/api/efi.rst b/doc/api/efi.rst
index 43d6f936fb0..49814bcf5ea 100644
--- a/doc/api/efi.rst
+++ b/doc/api/efi.rst
@@ -54,6 +54,12 @@ drivers, handles, loaded images, and the memory map).
 .. kernel-doc:: cmd/efidebug.c
    :internal:
 
+Overall API
+-----------
+
+.. kernel-doc:: include/efi_loader.h
+   :internal:
+
 Initialization of the UEFI sub-system
 -------------------------------------
 
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 00a1259c006..3f75f2efcb6 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -764,7 +764,7 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
  *
  * Allocate memory from pool and zero it out.
  *
- * @size:	number of bytes to allocate
+ * @len:	number of bytes to allocate
  * Return:	pointer to allocated memory or NULL
  */
 void *efi_alloc(size_t len);
-- 
2.43.0

[PATCH v4 09/25] efi_loader: Use the enum for memory type

[Simon Glass]

Rather than an integer, it is better to use the enum provided, when
referring to an EFI memory-type. Update existing uses.

Call the value 'mem_type' consistently. Fix up one instance of
upper-case hex.

Fix up the calls in struct efi_boot_services so that they use the same
enum, adding the missing parameter names and enum efi_allocate_type.

While we are here, rename the 'memory' parameter to 'memoryp' so that it
is clear it is a return value.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v2)

Changes in v2:
- Drop the changes to the boottime API

 include/efi_loader.h             | 29 ++++++++++----------
 lib/efi_loader/efi_boottime.c    | 12 ++++-----
 lib/efi_loader/efi_device_path.c |  4 +--
 lib/efi_loader/efi_memory.c      | 46 +++++++++++++++++---------------
 4 files changed, 47 insertions(+), 44 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 3f75f2efcb6..a7228672f27 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -773,24 +773,25 @@ void *efi_alloc(size_t len);
  * efi_alloc_aligned_pages() - allocate aligned memory pages
  *
  * @len:		len in bytes
- * @memory_type:	usage type of the allocated memory
+ * @mem_type:		usage type of the allocated memory
  * @align:		alignment in bytes
  * Return:		aligned memory or NULL
  */
-void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align);
+void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type,
+			      size_t align);
 
 /**
  * efi_allocate_pages - allocate memory pages
  *
  * @type:		type of allocation to be performed
- * @memory_type:	usage type of the allocated memory
+ * @mem_type:		usage type of the allocated memory
  * @pages:		number of pages to be allocated
- * @memory:		returns a pointer to the allocated memory
+ * @memoryp:		returns a pointer to the allocated memory
  * Return:		status code
  */
 efi_status_t efi_allocate_pages(enum efi_allocate_type type,
-				enum efi_memory_type memory_type,
-				efi_uintn_t pages, uint64_t *memory);
+				enum efi_memory_type mem_type,
+				efi_uintn_t pages, uint64_t *memoryp);
 
 /**
  * efi_free_pages() - free memory pages
@@ -804,12 +805,12 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages);
 /**
  * efi_allocate_pool - allocate memory from pool
  *
- * @pool_type:	type of the pool from which memory is to be allocated
+ * @mem_type:	memory type of the pool from which memory is to be allocated
  * @size:	number of bytes to be allocated
  * @buffer:	allocated memory
  * Return:	status code
  */
-efi_status_t efi_allocate_pool(enum efi_memory_type pool_type,
+efi_status_t efi_allocate_pool(enum efi_memory_type mem_type,
 			       efi_uintn_t size, void **buffer);
 
 /**
@@ -837,14 +838,14 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
  *			actually a pointer provided as an integer. To pass in
  *			an address, pass in (ulong)map_to_sysmem(addr)
  * @size:		length in bytes of the memory area
- * @memory_type:	type of memory added
- *
+ * @mem_type:		EFI type of memory added
  * Return:		status code
  *
  * This function automatically aligns the start and size of the memory area
  * to EFI_PAGE_SIZE.
  */
-efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
+efi_status_t efi_add_memory_map(u64 start, u64 size,
+				enum efi_memory_type mem_type);
 
 /**
  * efi_add_memory_map_pg() - add pages to the memory map
@@ -854,13 +855,13 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
  * in (ulong)map_to_sysmem(addr)
  *
  * @pages:			number of pages to add
- * @memory_type:		EFI type of memory added
+ * @mem_type:		EFI type of memory added
  * @overlap_conventional:	region may only overlap free(conventional)
  *				memory
  * Return:			status code
  */
 efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
-				   int memory_type,
+				   enum efi_memory_type mem_type,
 				   bool overlap_conventional);
 
 /* Called by board init to initialize the EFI drivers */
@@ -919,7 +920,7 @@ struct efi_device_path *efi_dp_part_node(struct blk_desc *desc, int part);
 struct efi_device_path *efi_dp_from_file(const struct efi_device_path *dp,
 					 const char *path);
 struct efi_device_path *efi_dp_from_eth(void);
-struct efi_device_path *efi_dp_from_mem(uint32_t mem_type,
+struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type,
 					uint64_t start_address,
 					size_t size);
 /* Determine the last device path node that is not the end node. */
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index f27b3827ed2..334a4d64b3f 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -415,9 +415,9 @@ static void EFIAPI efi_restore_tpl(efi_uintn_t old_tpl)
 /**
  * efi_allocate_pages_ext() - allocate memory pages
  * @type:        type of allocation to be performed
- * @memory_type: usage type of the allocated memory
+ * @mem_type:    usage type of the allocated memory
  * @pages:       number of pages to be allocated
- * @memory:      allocated memory
+ * @memoryp:     allocated memory
  *
  * This function implements the AllocatePages service.
  *
@@ -426,14 +426,14 @@ static void EFIAPI efi_restore_tpl(efi_uintn_t old_tpl)
  *
  * Return: status code
  */
-static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int memory_type,
+static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int mem_type,
 						  efi_uintn_t pages,
-						  uint64_t *memory)
+						  uint64_t *memoryp)
 {
 	efi_status_t r;
 
-	EFI_ENTRY("%d, %d, 0x%zx, %p", type, memory_type, pages, memory);
-	r = efi_allocate_pages(type, memory_type, pages, memory);
+	EFI_ENTRY("%d, %d, 0x%zx, %p", type, mem_type, pages, memoryp);
+	r = efi_allocate_pages(type, mem_type, pages, memoryp);
 	return EFI_EXIT(r);
 }
 
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index ee387e1dfd4..9c8cd35b97b 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -975,7 +975,7 @@ struct efi_device_path __maybe_unused *efi_dp_from_eth(void)
 }
 
 /* Construct a device-path for memory-mapped image */
-struct efi_device_path *efi_dp_from_mem(uint32_t memory_type,
+struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type,
 					uint64_t start_address,
 					size_t size)
 {
@@ -990,7 +990,7 @@ struct efi_device_path *efi_dp_from_mem(uint32_t memory_type,
 	mdp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
 	mdp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MEMORY;
 	mdp->dp.length = sizeof(*mdp);
-	mdp->memory_type = memory_type;
+	mdp->memory_type = mem_type;
 	mdp->start_address = start_address;
 	mdp->end_address = start_address + size;
 	buf = &mdp[1];
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 9d0f27dbb3e..3cece51f030 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -258,7 +258,7 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
 }
 
 efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
-				   int memory_type,
+				   enum efi_memory_type mem_type,
 				   bool overlap_conventional)
 {
 	struct efi_mem_list *lmem;
@@ -268,10 +268,10 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 	struct efi_event *evt;
 
 	EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
-		  start, pages, memory_type, overlap_conventional ?
+		  start, pages, mem_type, overlap_conventional ?
 		  "yes" : "no");
 
-	if (memory_type >= EFI_MAX_MEMORY_TYPE)
+	if (mem_type >= EFI_MAX_MEMORY_TYPE)
 		return EFI_INVALID_PARAMETER;
 
 	if (!pages)
@@ -281,12 +281,12 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 	newlist = calloc(1, sizeof(*newlist));
 	if (!newlist)
 		return EFI_OUT_OF_RESOURCES;
-	newlist->desc.type = memory_type;
+	newlist->desc.type = mem_type;
 	newlist->desc.physical_start = start;
 	newlist->desc.virtual_start = start;
 	newlist->desc.num_pages = pages;
 
-	switch (memory_type) {
+	switch (mem_type) {
 	case EFI_RUNTIME_SERVICES_CODE:
 	case EFI_RUNTIME_SERVICES_DATA:
 		newlist->desc.attribute = EFI_MEMORY_WB | EFI_MEMORY_RUNTIME;
@@ -370,14 +370,15 @@ 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, int memory_type)
+efi_status_t efi_add_memory_map(u64 start, u64 size,
+				enum efi_memory_type mem_type)
 {
 	u64 pages;
 
 	pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK));
 	start &= ~EFI_PAGE_MASK;
 
-	return efi_add_memory_map_pg(start, pages, memory_type, false);
+	return efi_add_memory_map_pg(start, pages, mem_type, false);
 }
 
 /**
@@ -416,8 +417,8 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated)
 }
 
 efi_status_t efi_allocate_pages(enum efi_allocate_type type,
-				enum efi_memory_type memory_type,
-				efi_uintn_t pages, uint64_t *memory)
+				enum efi_memory_type mem_type,
+				efi_uintn_t pages, uint64_t *memoryp)
 {
 	u64 efi_addr, len;
 	uint flags;
@@ -425,10 +426,9 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 	phys_addr_t addr;
 
 	/* Check import parameters */
-	if (memory_type >= EFI_PERSISTENT_MEMORY_TYPE &&
-	    memory_type <= 0x6FFFFFFF)
+	if (mem_type >= EFI_PERSISTENT_MEMORY_TYPE && mem_type <= 0x6fffffff)
 		return EFI_INVALID_PARAMETER;
-	if (!memory)
+	if (!memoryp)
 		return EFI_INVALID_PARAMETER;
 	len = (u64)pages << EFI_PAGE_SHIFT;
 	/* Catch possible overflow on 64bit systems */
@@ -447,17 +447,17 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 		break;
 	case EFI_ALLOCATE_MAX_ADDRESS:
 		/* Max address */
-		addr = map_to_sysmem((void *)(uintptr_t)*memory);
+		addr = map_to_sysmem((void *)(uintptr_t)*memoryp);
 		addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, addr,
 						 flags);
 		if (!addr)
 			return EFI_OUT_OF_RESOURCES;
 		break;
 	case EFI_ALLOCATE_ADDRESS:
-		if (*memory & EFI_PAGE_MASK)
+		if (*memoryp & EFI_PAGE_MASK)
 			return EFI_NOT_FOUND;
 
-		addr = map_to_sysmem((void *)(uintptr_t)*memory);
+		addr = map_to_sysmem((void *)(uintptr_t)*memoryp);
 		addr = (u64)lmb_alloc_addr_flags(addr, len, flags);
 		if (!addr)
 			return EFI_NOT_FOUND;
@@ -469,7 +469,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 
 	efi_addr = (u64)(uintptr_t)map_sysmem(addr, 0);
 	/* Reserve that map in our memory maps */
-	ret = efi_add_memory_map_pg(efi_addr, pages, memory_type, true);
+	ret = efi_add_memory_map_pg(efi_addr, pages, mem_type, true);
 	if (ret != EFI_SUCCESS) {
 		/* Map would overlap, bail out */
 		lmb_free_flags(addr, (u64)pages << EFI_PAGE_SHIFT, flags);
@@ -477,7 +477,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 		return  EFI_OUT_OF_RESOURCES;
 	}
 
-	*memory = efi_addr;
+	*memoryp = efi_addr;
 
 	return EFI_SUCCESS;
 }
@@ -515,7 +515,8 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
 	return ret;
 }
 
-void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align)
+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;
@@ -533,12 +534,12 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align)
 		return NULL;
 
 	if (align < EFI_PAGE_SIZE) {
-		r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type,
+		r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type,
 				       req_pages, &mem);
 		return (r == EFI_SUCCESS) ? (void *)(uintptr_t)mem : NULL;
 	}
 
-	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type,
+	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type,
 			       true_pages, &mem);
 	if (r != EFI_SUCCESS)
 		return NULL;
@@ -559,7 +560,8 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align)
 	return (void *)(uintptr_t)aligned_mem;
 }
 
-efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer)
+efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size,
+			       void **buffer)
 {
 	efi_status_t r;
 	u64 addr;
@@ -575,7 +577,7 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
 		return EFI_SUCCESS;
 	}
 
-	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
+	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, num_pages,
 			       &addr);
 	if (r == EFI_SUCCESS) {
 		alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
-- 
2.43.0

Re: [PATCH v4 09/25] efi_loader: Use the enum for memory type

[Heinrich Schuchardt]

On 01.12.24 16:24, Simon Glass wrote:
> Rather than an integer, it is better to use the enum provided, when
> referring to an EFI memory-type. Update existing uses.
>
> Call the value 'mem_type' consistently. Fix up one instance of
> upper-case hex.
>
> Fix up the calls in struct efi_boot_services so that they use the same
> enum, adding the missing parameter names and enum efi_allocate_type.
>
> While we are here, rename the 'memory' parameter to 'memoryp' so that it
> is clear it is a return value.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Simon, I have no clue why you keep ignoring reviews.
This is just annoying and won't lead to merging patches.

NAK

Best regards

Heinrich


> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Drop the changes to the boottime API
>
>   include/efi_loader.h             | 29 ++++++++++----------
>   lib/efi_loader/efi_boottime.c    | 12 ++++-----
>   lib/efi_loader/efi_device_path.c |  4 +--
>   lib/efi_loader/efi_memory.c      | 46 +++++++++++++++++---------------
>   4 files changed, 47 insertions(+), 44 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 3f75f2efcb6..a7228672f27 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -773,24 +773,25 @@ void *efi_alloc(size_t len);
>    * efi_alloc_aligned_pages() - allocate aligned memory pages
>    *
>    * @len:		len in bytes
> - * @memory_type:	usage type of the allocated memory
> + * @mem_type:		usage type of the allocated memory
>    * @align:		alignment in bytes
>    * Return:		aligned memory or NULL
>    */
> -void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align);
> +void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type,
> +			      size_t align);
>
>   /**
>    * efi_allocate_pages - allocate memory pages
>    *
>    * @type:		type of allocation to be performed
> - * @memory_type:	usage type of the allocated memory
> + * @mem_type:		usage type of the allocated memory
>    * @pages:		number of pages to be allocated
> - * @memory:		returns a pointer to the allocated memory
> + * @memoryp:		returns a pointer to the allocated memory
>    * Return:		status code
>    */
>   efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> -				enum efi_memory_type memory_type,
> -				efi_uintn_t pages, uint64_t *memory);
> +				enum efi_memory_type mem_type,
> +				efi_uintn_t pages, uint64_t *memoryp);
>
>   /**
>    * efi_free_pages() - free memory pages
> @@ -804,12 +805,12 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages);
>   /**
>    * efi_allocate_pool - allocate memory from pool
>    *
> - * @pool_type:	type of the pool from which memory is to be allocated
> + * @mem_type:	memory type of the pool from which memory is to be allocated
>    * @size:	number of bytes to be allocated
>    * @buffer:	allocated memory
>    * Return:	status code
>    */
> -efi_status_t efi_allocate_pool(enum efi_memory_type pool_type,
> +efi_status_t efi_allocate_pool(enum efi_memory_type mem_type,
>   			       efi_uintn_t size, void **buffer);
>
>   /**
> @@ -837,14 +838,14 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>    *			actually a pointer provided as an integer. To pass in
>    *			an address, pass in (ulong)map_to_sysmem(addr)
>    * @size:		length in bytes of the memory area
> - * @memory_type:	type of memory added
> - *
> + * @mem_type:		EFI type of memory added
>    * Return:		status code
>    *
>    * This function automatically aligns the start and size of the memory area
>    * to EFI_PAGE_SIZE.
>    */
> -efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
> +efi_status_t efi_add_memory_map(u64 start, u64 size,
> +				enum efi_memory_type mem_type);
>
>   /**
>    * efi_add_memory_map_pg() - add pages to the memory map
> @@ -854,13 +855,13 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
>    * in (ulong)map_to_sysmem(addr)
>    *
>    * @pages:			number of pages to add
> - * @memory_type:		EFI type of memory added
> + * @mem_type:		EFI type of memory added
>    * @overlap_conventional:	region may only overlap free(conventional)
>    *				memory
>    * Return:			status code
>    */
>   efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> -				   int memory_type,
> +				   enum efi_memory_type mem_type,
>   				   bool overlap_conventional);
>
>   /* Called by board init to initialize the EFI drivers */
> @@ -919,7 +920,7 @@ struct efi_device_path *efi_dp_part_node(struct blk_desc *desc, int part);
>   struct efi_device_path *efi_dp_from_file(const struct efi_device_path *dp,
>   					 const char *path);
>   struct efi_device_path *efi_dp_from_eth(void);
> -struct efi_device_path *efi_dp_from_mem(uint32_t mem_type,
> +struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type,
>   					uint64_t start_address,
>   					size_t size);
>   /* Determine the last device path node that is not the end node. */
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index f27b3827ed2..334a4d64b3f 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -415,9 +415,9 @@ static void EFIAPI efi_restore_tpl(efi_uintn_t old_tpl)
>   /**
>    * efi_allocate_pages_ext() - allocate memory pages
>    * @type:        type of allocation to be performed
> - * @memory_type: usage type of the allocated memory
> + * @mem_type:    usage type of the allocated memory
>    * @pages:       number of pages to be allocated
> - * @memory:      allocated memory
> + * @memoryp:     allocated memory
>    *
>    * This function implements the AllocatePages service.
>    *
> @@ -426,14 +426,14 @@ static void EFIAPI efi_restore_tpl(efi_uintn_t old_tpl)
>    *
>    * Return: status code
>    */
> -static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int memory_type,
> +static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int mem_type,
>   						  efi_uintn_t pages,
> -						  uint64_t *memory)
> +						  uint64_t *memoryp)
>   {
>   	efi_status_t r;
>
> -	EFI_ENTRY("%d, %d, 0x%zx, %p", type, memory_type, pages, memory);
> -	r = efi_allocate_pages(type, memory_type, pages, memory);
> +	EFI_ENTRY("%d, %d, 0x%zx, %p", type, mem_type, pages, memoryp);
> +	r = efi_allocate_pages(type, mem_type, pages, memoryp);
>   	return EFI_EXIT(r);
>   }
>
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index ee387e1dfd4..9c8cd35b97b 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -975,7 +975,7 @@ struct efi_device_path __maybe_unused *efi_dp_from_eth(void)
>   }
>
>   /* Construct a device-path for memory-mapped image */
> -struct efi_device_path *efi_dp_from_mem(uint32_t memory_type,
> +struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type,
>   					uint64_t start_address,
>   					size_t size)
>   {
> @@ -990,7 +990,7 @@ struct efi_device_path *efi_dp_from_mem(uint32_t memory_type,
>   	mdp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
>   	mdp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MEMORY;
>   	mdp->dp.length = sizeof(*mdp);
> -	mdp->memory_type = memory_type;
> +	mdp->memory_type = mem_type;
>   	mdp->start_address = start_address;
>   	mdp->end_address = start_address + size;
>   	buf = &mdp[1];
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 9d0f27dbb3e..3cece51f030 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -258,7 +258,7 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
>   }
>
>   efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> -				   int memory_type,
> +				   enum efi_memory_type mem_type,
>   				   bool overlap_conventional)
>   {
>   	struct efi_mem_list *lmem;
> @@ -268,10 +268,10 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
>   	struct efi_event *evt;
>
>   	EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
> -		  start, pages, memory_type, overlap_conventional ?
> +		  start, pages, mem_type, overlap_conventional ?
>   		  "yes" : "no");
>
> -	if (memory_type >= EFI_MAX_MEMORY_TYPE)
> +	if (mem_type >= EFI_MAX_MEMORY_TYPE)
>   		return EFI_INVALID_PARAMETER;
>
>   	if (!pages)
> @@ -281,12 +281,12 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
>   	newlist = calloc(1, sizeof(*newlist));
>   	if (!newlist)
>   		return EFI_OUT_OF_RESOURCES;
> -	newlist->desc.type = memory_type;
> +	newlist->desc.type = mem_type;
>   	newlist->desc.physical_start = start;
>   	newlist->desc.virtual_start = start;
>   	newlist->desc.num_pages = pages;
>
> -	switch (memory_type) {
> +	switch (mem_type) {
>   	case EFI_RUNTIME_SERVICES_CODE:
>   	case EFI_RUNTIME_SERVICES_DATA:
>   		newlist->desc.attribute = EFI_MEMORY_WB | EFI_MEMORY_RUNTIME;
> @@ -370,14 +370,15 @@ 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, int memory_type)
> +efi_status_t efi_add_memory_map(u64 start, u64 size,
> +				enum efi_memory_type mem_type)
>   {
>   	u64 pages;
>
>   	pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK));
>   	start &= ~EFI_PAGE_MASK;
>
> -	return efi_add_memory_map_pg(start, pages, memory_type, false);
> +	return efi_add_memory_map_pg(start, pages, mem_type, false);
>   }
>
>   /**
> @@ -416,8 +417,8 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated)
>   }
>
>   efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> -				enum efi_memory_type memory_type,
> -				efi_uintn_t pages, uint64_t *memory)
> +				enum efi_memory_type mem_type,
> +				efi_uintn_t pages, uint64_t *memoryp)
>   {
>   	u64 efi_addr, len;
>   	uint flags;
> @@ -425,10 +426,9 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>   	phys_addr_t addr;
>
>   	/* Check import parameters */
> -	if (memory_type >= EFI_PERSISTENT_MEMORY_TYPE &&
> -	    memory_type <= 0x6FFFFFFF)
> +	if (mem_type >= EFI_PERSISTENT_MEMORY_TYPE && mem_type <= 0x6fffffff)
>   		return EFI_INVALID_PARAMETER;
> -	if (!memory)
> +	if (!memoryp)
>   		return EFI_INVALID_PARAMETER;
>   	len = (u64)pages << EFI_PAGE_SHIFT;
>   	/* Catch possible overflow on 64bit systems */
> @@ -447,17 +447,17 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>   		break;
>   	case EFI_ALLOCATE_MAX_ADDRESS:
>   		/* Max address */
> -		addr = map_to_sysmem((void *)(uintptr_t)*memory);
> +		addr = map_to_sysmem((void *)(uintptr_t)*memoryp);
>   		addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, addr,
>   						 flags);
>   		if (!addr)
>   			return EFI_OUT_OF_RESOURCES;
>   		break;
>   	case EFI_ALLOCATE_ADDRESS:
> -		if (*memory & EFI_PAGE_MASK)
> +		if (*memoryp & EFI_PAGE_MASK)
>   			return EFI_NOT_FOUND;
>
> -		addr = map_to_sysmem((void *)(uintptr_t)*memory);
> +		addr = map_to_sysmem((void *)(uintptr_t)*memoryp);
>   		addr = (u64)lmb_alloc_addr_flags(addr, len, flags);
>   		if (!addr)
>   			return EFI_NOT_FOUND;
> @@ -469,7 +469,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>
>   	efi_addr = (u64)(uintptr_t)map_sysmem(addr, 0);
>   	/* Reserve that map in our memory maps */
> -	ret = efi_add_memory_map_pg(efi_addr, pages, memory_type, true);
> +	ret = efi_add_memory_map_pg(efi_addr, pages, mem_type, true);
>   	if (ret != EFI_SUCCESS) {
>   		/* Map would overlap, bail out */
>   		lmb_free_flags(addr, (u64)pages << EFI_PAGE_SHIFT, flags);
> @@ -477,7 +477,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>   		return  EFI_OUT_OF_RESOURCES;
>   	}
>
> -	*memory = efi_addr;
> +	*memoryp = efi_addr;
>
>   	return EFI_SUCCESS;
>   }
> @@ -515,7 +515,8 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>   	return ret;
>   }
>
> -void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align)
> +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;
> @@ -533,12 +534,12 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align)
>   		return NULL;
>
>   	if (align < EFI_PAGE_SIZE) {
> -		r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type,
> +		r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type,
>   				       req_pages, &mem);
>   		return (r == EFI_SUCCESS) ? (void *)(uintptr_t)mem : NULL;
>   	}
>
> -	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type,
> +	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type,
>   			       true_pages, &mem);
>   	if (r != EFI_SUCCESS)
>   		return NULL;
> @@ -559,7 +560,8 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align)
>   	return (void *)(uintptr_t)aligned_mem;
>   }
>
> -efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer)
> +efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size,
> +			       void **buffer)
>   {
>   	efi_status_t r;
>   	u64 addr;
> @@ -575,7 +577,7 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
>   		return EFI_SUCCESS;
>   	}
>
> -	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
> +	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, num_pages,
>   			       &addr);
>   	if (r == EFI_SUCCESS) {
>   		alloc = (struct efi_pool_allocation *)(uintptr_t)addr;

Re: [PATCH v4 09/25] efi_loader: Use the enum for memory type

[Simon Glass]

Hi Heinrich,

On Tue, 3 Dec 2024 at 07:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 01.12.24 16:24, Simon Glass wrote:
> > Rather than an integer, it is better to use the enum provided, when
> > referring to an EFI memory-type. Update existing uses.
> >
> > Call the value 'mem_type' consistently. Fix up one instance of
> > upper-case hex.
> >
> > Fix up the calls in struct efi_boot_services so that they use the same
> > enum, adding the missing parameter names and enum efi_allocate_type.
> >
> > While we are here, rename the 'memory' parameter to 'memoryp' so that it
> > is clear it is a return value.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Simon, I have no clue why you keep ignoring reviews.
> This is just annoying and won't lead to merging patches.

I got your feedback on [1] and incorporated it in v2 onwards. Did you notice?

>
> NAK

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/patch/20241125204456.1525752-7-sjg@chromium.org/

Re: [PATCH v4 09/25] efi_loader: Use the enum for memory type

[Simon Glass]

Hi Heinrich,

On Tue, 3 Dec 2024 at 08:53, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Heinrich,
>
> On Tue, 3 Dec 2024 at 07:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 01.12.24 16:24, Simon Glass wrote:
> > > Rather than an integer, it is better to use the enum provided, when
> > > referring to an EFI memory-type. Update existing uses.
> > >
> > > Call the value 'mem_type' consistently. Fix up one instance of
> > > upper-case hex.
> > >
> > > Fix up the calls in struct efi_boot_services so that they use the same
> > > enum, adding the missing parameter names and enum efi_allocate_type.
> > >
> > > While we are here, rename the 'memory' parameter to 'memoryp' so that it
> > > is clear it is a return value.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > Simon, I have no clue why you keep ignoring reviews.
> > This is just annoying and won't lead to merging patches.
>
> I got your feedback on [1] and incorporated it in v2 onwards. Did you notice?
>
> >
> > NAK

It has been a week and I have not had any response on this. Does that
mean you are OK with the patch now?

Regards,
Simon

Re: [PATCH v4 09/25] efi_loader: Use the enum for memory type

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]

On Tue, Dec 10, 2024 at 09:17:43AM -0700, Simon Glass wrote:
> Hi Heinrich,
> 
> On Tue, 3 Dec 2024 at 08:53, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Heinrich,
> >
> > On Tue, 3 Dec 2024 at 07:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 01.12.24 16:24, Simon Glass wrote:
> > > > Rather than an integer, it is better to use the enum provided, when
> > > > referring to an EFI memory-type. Update existing uses.
> > > >
> > > > Call the value 'mem_type' consistently. Fix up one instance of
> > > > upper-case hex.
> > > >
> > > > Fix up the calls in struct efi_boot_services so that they use the same
> > > > enum, adding the missing parameter names and enum efi_allocate_type.
> > > >
> > > > While we are here, rename the 'memory' parameter to 'memoryp' so that it
> > > > is clear it is a return value.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > >
> > > Simon, I have no clue why you keep ignoring reviews.
> > > This is just annoying and won't lead to merging patches.
> >
> > I got your feedback on [1] and incorporated it in v2 onwards. Did you notice?
> >
> > >
> > > NAK
> 
> It has been a week and I have not had any response on this. Does that
> mean you are OK with the patch now?

I'm unclear what part of NAK implies that Heinrich is OK with this
patch? I believe not since part of it was to not rename "memory" to
"memoryp" and you're calling out doing that still. But I suspect you'll
just keep this in your fork all the same.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v4 10/25] efi_loader: Use a separate struct for memory nodes

[Simon Glass]

At present efi_memory uses struct efi_mem_desc for each node of its
memory list. This is convenient but is not ideal, since:

- it means that the fields are under a 'desc' sub-structure
- it includes an unused 'reserved' field
- it includes virtual_start which is confusing as it is always the same
  as physical_start
- we must use u64 to store pointers, since that is how they are returned
  by calls to efi_get_memory_map()

As a first step to tidying this up, create a new, private struct to hold
these fields.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v2)

Changes in v2:
- Add new patch to use a separate stuct for memory nodes

 lib/efi_loader/efi_memory.c | 49 +++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 3cece51f030..9e4158edfaf 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -27,9 +27,39 @@ DECLARE_GLOBAL_DATA_PTR;
 
 efi_uintn_t efi_memory_map_key;
 
+/**
+ * struct priv_mem_desc - defines an memory region
+ *
+ * Note that this struct is for use inside U-Boot and is not visible to the
+ * EFI application, other than through calls to efi_get_memory_map(), where this
+ * internal format is converted to the external struct efi_mem_desc format.
+ *
+ * @type (enum efi_memory_type): EFI memory-type
+ * @reserved: unused
+ * @physical_start: Start address of region in physical memory
+ * @virtual_start: Start address of region in physical memory
+ * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
+ *	bytes)
+ * @attribute: Memory attributes (see EFI_MEMORY...)
+ */
+struct priv_mem_desc {
+	u32 type;
+	u32 reserved;
+	efi_physical_addr_t physical_start;
+	efi_virtual_addr_t virtual_start;
+	u64 num_pages;
+	u64 attribute;
+};
+
+/**
+ * struct efi_mem_list - defines an EFI memory record
+ *
+ * @link: Link to prev/next node in list
+ * @desc: Memory information about this node
+ */
 struct efi_mem_list {
 	struct list_head link;
-	struct efi_mem_desc desc;
+	struct priv_mem_desc desc;
 };
 
 #define EFI_CARVE_NO_OVERLAP		-1
@@ -116,7 +146,7 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b)
  * @desc:	memory descriptor
  * Return:	end address + 1
  */
-static uint64_t desc_get_end(struct efi_mem_desc *desc)
+static uint64_t desc_get_end(struct priv_mem_desc *desc)
 {
 	return desc->physical_start + (desc->num_pages << EFI_PAGE_SHIFT);
 }
@@ -138,8 +168,8 @@ static void efi_mem_sort(void)
 	while (merge_again) {
 		merge_again = false;
 		list_for_each_entry(lmem, &efi_mem, link) {
-			struct efi_mem_desc *prev;
-			struct efi_mem_desc *cur;
+			struct priv_mem_desc *prev;
+			struct priv_mem_desc *cur;
 			uint64_t pages;
 
 			if (!prevmem) {
@@ -194,11 +224,11 @@ static void efi_mem_sort(void)
  * to re-add the already carved out pages to the mapping.
  */
 static s64 efi_mem_carve_out(struct efi_mem_list *map,
-			     struct efi_mem_desc *carve_desc,
+			     struct priv_mem_desc *carve_desc,
 			     bool overlap_conventional)
 {
 	struct efi_mem_list *newmap;
-	struct efi_mem_desc *map_desc = &map->desc;
+	struct priv_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;
@@ -680,7 +710,12 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 	/* Return the list in ascending order */
 	memory_map = &memory_map[map_entries - 1];
 	list_for_each_entry(lmem, &efi_mem, link) {
-		*memory_map = lmem->desc;
+		memory_map->type = lmem->desc.type;
+		memory_map->reserved = lmem->desc.reserved;
+		memory_map->physical_start = lmem->desc.physical_start;
+		memory_map->virtual_start = lmem->desc.virtual_start;
+		memory_map->num_pages = lmem->desc.num_pages;
+		memory_map->attribute = lmem->desc.attribute;
 		memory_map--;
 	}
 
-- 
2.43.0

[PATCH v4 11/25] efi_loader: Drop virtual_start from priv_mem_desc

[Simon Glass]

This field is always the same as physical_start, so keeping track of it
separately is unnecessary and confusing. Drop it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_memory.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 9e4158edfaf..518b8cff61d 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -37,7 +37,6 @@ efi_uintn_t efi_memory_map_key;
  * @type (enum efi_memory_type): EFI memory-type
  * @reserved: unused
  * @physical_start: Start address of region in physical memory
- * @virtual_start: Start address of region in physical memory
  * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
  *	bytes)
  * @attribute: Memory attributes (see EFI_MEMORY...)
@@ -46,7 +45,6 @@ struct priv_mem_desc {
 	u32 type;
 	u32 reserved;
 	efi_physical_addr_t physical_start;
-	efi_virtual_addr_t virtual_start;
 	u64 num_pages;
 	u64 attribute;
 };
@@ -187,7 +185,6 @@ static void efi_mem_sort(void)
 				pages = cur->num_pages;
 				prev->num_pages += pages;
 				prev->physical_start -= pages << EFI_PAGE_SHIFT;
-				prev->virtual_start -= pages << EFI_PAGE_SHIFT;
 				list_del(&lmem->link);
 				free(lmem);
 
@@ -255,7 +252,6 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
 			free(map);
 		} else {
 			map->desc.physical_start = carve_end;
-			map->desc.virtual_start = carve_end;
 			map->desc.num_pages = (map_end - carve_end)
 					      >> EFI_PAGE_SHIFT;
 		}
@@ -276,7 +272,6 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
 		return EFI_CARVE_OUT_OF_RESOURCES;
 	newmap->desc = map->desc;
 	newmap->desc.physical_start = carve_start;
-	newmap->desc.virtual_start = carve_start;
 	newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT;
 	/* Insert before current entry (descending address order) */
 	list_add_tail(&newmap->link, &map->link);
@@ -313,7 +308,6 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 		return EFI_OUT_OF_RESOURCES;
 	newlist->desc.type = mem_type;
 	newlist->desc.physical_start = start;
-	newlist->desc.virtual_start = start;
 	newlist->desc.num_pages = pages;
 
 	switch (mem_type) {
@@ -713,7 +707,9 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 		memory_map->type = lmem->desc.type;
 		memory_map->reserved = lmem->desc.reserved;
 		memory_map->physical_start = lmem->desc.physical_start;
-		memory_map->virtual_start = lmem->desc.virtual_start;
+
+		/* virtual and physical are always the same */
+		memory_map->virtual_start = lmem->desc.physical_start;
 		memory_map->num_pages = lmem->desc.num_pages;
 		memory_map->attribute = lmem->desc.attribute;
 		memory_map--;
-- 
2.43.0

[PATCH v4 12/25] efi_loader: Drop reserved from priv_mem_desc

[Simon Glass]

This field is not used. Drop it and set the value to 0 when the
memory-map is requested.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_memory.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 518b8cff61d..81e40fc923d 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -35,7 +35,6 @@ efi_uintn_t efi_memory_map_key;
  * internal format is converted to the external struct efi_mem_desc format.
  *
  * @type (enum efi_memory_type): EFI memory-type
- * @reserved: unused
  * @physical_start: Start address of region in physical memory
  * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
  *	bytes)
@@ -43,7 +42,6 @@ efi_uintn_t efi_memory_map_key;
  */
 struct priv_mem_desc {
 	u32 type;
-	u32 reserved;
 	efi_physical_addr_t physical_start;
 	u64 num_pages;
 	u64 attribute;
@@ -705,7 +703,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 	memory_map = &memory_map[map_entries - 1];
 	list_for_each_entry(lmem, &efi_mem, link) {
 		memory_map->type = lmem->desc.type;
-		memory_map->reserved = lmem->desc.reserved;
+		memory_map->reserved = 0;
 		memory_map->physical_start = lmem->desc.physical_start;
 
 		/* virtual and physical are always the same */
-- 
2.43.0

[PATCH v4 13/25] efi_loader: Use the enum for the memory type in priv_mem_desc

[Simon Glass]

We can make use of the enum now, since this struct is not exported to
the EFI app.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_memory.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 81e40fc923d..83f2e32b9a4 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -34,14 +34,14 @@ efi_uintn_t efi_memory_map_key;
  * EFI application, other than through calls to efi_get_memory_map(), where this
  * internal format is converted to the external struct efi_mem_desc format.
  *
- * @type (enum efi_memory_type): EFI memory-type
+ * @type: EFI memory-type
  * @physical_start: Start address of region in physical memory
  * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
  *	bytes)
  * @attribute: Memory attributes (see EFI_MEMORY...)
  */
 struct priv_mem_desc {
-	u32 type;
+	enum efi_memory_type type;
 	efi_physical_addr_t physical_start;
 	u64 num_pages;
 	u64 attribute;
-- 
2.43.0

[PATCH v4 14/25] efi_loader: Avoid assigning desc in efi_mem_carve_out()

[Simon Glass]

Rather than assigning desc and then changing two fields, assign all four
fields, for clarity.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_memory.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 83f2e32b9a4..b51bb367899 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -268,9 +268,10 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
 	newmap = calloc(1, sizeof(*newmap));
 	if (!newmap)
 		return EFI_CARVE_OUT_OF_RESOURCES;
-	newmap->desc = map->desc;
+	newmap->desc.type = map->desc.type;
 	newmap->desc.physical_start = carve_start;
 	newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT;
+	newmap->desc.attribute = map->desc.attribute;
 	/* Insert before current entry (descending address order) */
 	list_add_tail(&newmap->link, &map->link);
 
-- 
2.43.0

[PATCH v4 15/25] efi_loader: Move struct efi_mem_list fields together

[Simon Glass]

We don't need a separate struct for the values in this node. Move
everything in together, so that there is just one struct to consider.

Add a comment about physical_start, so it is clear that it is really
a pointer.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_memory.c | 85 +++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 46 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index b51bb367899..6e474c88784 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -28,36 +28,29 @@ DECLARE_GLOBAL_DATA_PTR;
 efi_uintn_t efi_memory_map_key;
 
 /**
- * struct priv_mem_desc - defines an memory region
+ * struct efi_mem_list - defines an EFI memory record
  *
  * Note that this struct is for use inside U-Boot and is not visible to the
  * EFI application, other than through calls to efi_get_memory_map(), where this
  * internal format is converted to the external struct efi_mem_desc format.
  *
+ * @link: Link to prev/next node in list
  * @type: EFI memory-type
- * @physical_start: Start address of region in physical memory
+ * @physical_start: Start address of region in physical memory. Note that this
+ *	is really a pointer stored as an address, so use map_to_sysmem() to
+ *	convert it to an address if needed
  * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
  *	bytes)
  * @attribute: Memory attributes (see EFI_MEMORY...)
  */
-struct priv_mem_desc {
+struct efi_mem_list {
+	struct list_head link;
 	enum efi_memory_type type;
 	efi_physical_addr_t physical_start;
 	u64 num_pages;
 	u64 attribute;
 };
 
-/**
- * struct efi_mem_list - defines an EFI memory record
- *
- * @link: Link to prev/next node in list
- * @desc: Memory information about this node
- */
-struct efi_mem_list {
-	struct list_head link;
-	struct priv_mem_desc desc;
-};
-
 #define EFI_CARVE_NO_OVERLAP		-1
 #define EFI_CARVE_LOOP_AGAIN		-2
 #define EFI_CARVE_OVERLAPS_NONRAM	-3
@@ -128,9 +121,9 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b)
 	struct efi_mem_list *mema = list_entry(a, struct efi_mem_list, link);
 	struct efi_mem_list *memb = list_entry(b, struct efi_mem_list, link);
 
-	if (mema->desc.physical_start == memb->desc.physical_start)
+	if (mema->physical_start == memb->physical_start)
 		return 0;
-	else if (mema->desc.physical_start < memb->desc.physical_start)
+	else if (mema->physical_start < memb->physical_start)
 		return 1;
 	else
 		return -1;
@@ -139,12 +132,12 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b)
 /**
  * desc_get_end() - get end address of memory area
  *
- * @desc:	memory descriptor
+ * @node:	memory node
  * Return:	end address + 1
  */
-static uint64_t desc_get_end(struct priv_mem_desc *desc)
+static uint64_t desc_get_end(struct efi_mem_list *node)
 {
-	return desc->physical_start + (desc->num_pages << EFI_PAGE_SHIFT);
+	return node->physical_start + (node->num_pages << EFI_PAGE_SHIFT);
 }
 
 /**
@@ -164,8 +157,8 @@ static void efi_mem_sort(void)
 	while (merge_again) {
 		merge_again = false;
 		list_for_each_entry(lmem, &efi_mem, link) {
-			struct priv_mem_desc *prev;
-			struct priv_mem_desc *cur;
+			struct efi_mem_list *prev;
+			struct efi_mem_list *cur;
 			uint64_t pages;
 
 			if (!prevmem) {
@@ -173,8 +166,8 @@ static void efi_mem_sort(void)
 				continue;
 			}
 
-			cur = &lmem->desc;
-			prev = &prevmem->desc;
+			cur = lmem;
+			prev = prevmem;
 
 			if ((desc_get_end(cur) == prev->physical_start) &&
 			    (prev->type == cur->type) &&
@@ -219,11 +212,11 @@ static void efi_mem_sort(void)
  * to re-add the already carved out pages to the mapping.
  */
 static s64 efi_mem_carve_out(struct efi_mem_list *map,
-			     struct priv_mem_desc *carve_desc,
+			     struct efi_mem_list *carve_desc,
 			     bool overlap_conventional)
 {
 	struct efi_mem_list *newmap;
-	struct priv_mem_desc *map_desc = &map->desc;
+	struct efi_mem_list *map_desc = map;
 	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;
@@ -249,8 +242,8 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
 			list_del(&map->link);
 			free(map);
 		} else {
-			map->desc.physical_start = carve_end;
-			map->desc.num_pages = (map_end - carve_end)
+			map->physical_start = carve_end;
+			map->num_pages = (map_end - carve_end)
 					      >> EFI_PAGE_SHIFT;
 		}
 
@@ -268,10 +261,10 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
 	newmap = calloc(1, sizeof(*newmap));
 	if (!newmap)
 		return EFI_CARVE_OUT_OF_RESOURCES;
-	newmap->desc.type = map->desc.type;
-	newmap->desc.physical_start = carve_start;
-	newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT;
-	newmap->desc.attribute = map->desc.attribute;
+	newmap->type = map->type;
+	newmap->physical_start = carve_start;
+	newmap->num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT;
+	newmap->attribute = map->attribute;
 	/* Insert before current entry (descending address order) */
 	list_add_tail(&newmap->link, &map->link);
 
@@ -305,20 +298,20 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 	newlist = calloc(1, sizeof(*newlist));
 	if (!newlist)
 		return EFI_OUT_OF_RESOURCES;
-	newlist->desc.type = mem_type;
-	newlist->desc.physical_start = start;
-	newlist->desc.num_pages = pages;
+	newlist->type = mem_type;
+	newlist->physical_start = start;
+	newlist->num_pages = pages;
 
 	switch (mem_type) {
 	case EFI_RUNTIME_SERVICES_CODE:
 	case EFI_RUNTIME_SERVICES_DATA:
-		newlist->desc.attribute = EFI_MEMORY_WB | EFI_MEMORY_RUNTIME;
+		newlist->attribute = EFI_MEMORY_WB | EFI_MEMORY_RUNTIME;
 		break;
 	case EFI_MMAP_IO:
-		newlist->desc.attribute = EFI_MEMORY_RUNTIME;
+		newlist->attribute = EFI_MEMORY_RUNTIME;
 		break;
 	default:
-		newlist->desc.attribute = EFI_MEMORY_WB;
+		newlist->attribute = EFI_MEMORY_WB;
 		break;
 	}
 
@@ -328,7 +321,7 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 		list_for_each_entry(lmem, &efi_mem, link) {
 			s64 r;
 
-			r = efi_mem_carve_out(lmem, &newlist->desc,
+			r = efi_mem_carve_out(lmem, newlist,
 					      overlap_conventional);
 			switch (r) {
 			case EFI_CARVE_OUT_OF_RESOURCES:
@@ -424,12 +417,12 @@ static efi_status_t efi_check_allocated(u64 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);
+		u64 start = item->physical_start;
+		u64 end = start + (item->num_pages << EFI_PAGE_SHIFT);
 
 		if (addr >= start && addr < end) {
 			if (must_be_allocated ^
-			    (item->desc.type == EFI_CONVENTIONAL_MEMORY))
+			    (item->type == EFI_CONVENTIONAL_MEMORY))
 				return EFI_SUCCESS;
 			else
 				return EFI_NOT_FOUND;
@@ -703,14 +696,14 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 	/* Return the list in ascending order */
 	memory_map = &memory_map[map_entries - 1];
 	list_for_each_entry(lmem, &efi_mem, link) {
-		memory_map->type = lmem->desc.type;
+		memory_map->type = lmem->type;
 		memory_map->reserved = 0;
-		memory_map->physical_start = lmem->desc.physical_start;
+		memory_map->physical_start = lmem->physical_start;
 
 		/* virtual and physical are always the same */
-		memory_map->virtual_start = lmem->desc.physical_start;
-		memory_map->num_pages = lmem->desc.num_pages;
-		memory_map->attribute = lmem->desc.attribute;
+		memory_map->virtual_start = lmem->physical_start;
+		memory_map->num_pages = lmem->num_pages;
+		memory_map->attribute = lmem->attribute;
 		memory_map--;
 	}
 
-- 
2.43.0

[PATCH v4 16/25] efi_loader: Rename struct efi_mem_list to mem_node

[Simon Glass]

This struct is really a node in the list, not a list itself. Also it
doesn't need an efi_ prefix. Rename it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_memory.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 6e474c88784..dbcba99ae39 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -28,7 +28,7 @@ DECLARE_GLOBAL_DATA_PTR;
 efi_uintn_t efi_memory_map_key;
 
 /**
- * struct efi_mem_list - defines an EFI memory record
+ * struct mem_node - defines an EFI memory record
  *
  * Note that this struct is for use inside U-Boot and is not visible to the
  * EFI application, other than through calls to efi_get_memory_map(), where this
@@ -43,7 +43,7 @@ efi_uintn_t efi_memory_map_key;
  *	bytes)
  * @attribute: Memory attributes (see EFI_MEMORY...)
  */
-struct efi_mem_list {
+struct mem_node {
 	struct list_head link;
 	enum efi_memory_type type;
 	efi_physical_addr_t physical_start;
@@ -118,8 +118,8 @@ static u64 checksum(struct efi_pool_allocation *alloc)
  */
 static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b)
 {
-	struct efi_mem_list *mema = list_entry(a, struct efi_mem_list, link);
-	struct efi_mem_list *memb = list_entry(b, struct efi_mem_list, link);
+	struct mem_node *mema = list_entry(a, struct mem_node, link);
+	struct mem_node *memb = list_entry(b, struct mem_node, link);
 
 	if (mema->physical_start == memb->physical_start)
 		return 0;
@@ -135,7 +135,7 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b)
  * @node:	memory node
  * Return:	end address + 1
  */
-static uint64_t desc_get_end(struct efi_mem_list *node)
+static uint64_t desc_get_end(struct mem_node *node)
 {
 	return node->physical_start + (node->num_pages << EFI_PAGE_SHIFT);
 }
@@ -147,8 +147,8 @@ static uint64_t desc_get_end(struct efi_mem_list *node)
  */
 static void efi_mem_sort(void)
 {
-	struct efi_mem_list *lmem;
-	struct efi_mem_list *prevmem = NULL;
+	struct mem_node *lmem;
+	struct mem_node *prevmem = NULL;
 	bool merge_again = true;
 
 	list_sort(NULL, &efi_mem, efi_mem_cmp);
@@ -157,8 +157,8 @@ static void efi_mem_sort(void)
 	while (merge_again) {
 		merge_again = false;
 		list_for_each_entry(lmem, &efi_mem, link) {
-			struct efi_mem_list *prev;
-			struct efi_mem_list *cur;
+			struct mem_node *prev;
+			struct mem_node *cur;
 			uint64_t pages;
 
 			if (!prevmem) {
@@ -211,12 +211,11 @@ static void efi_mem_sort(void)
  * In case of EFI_CARVE_OVERLAPS_NONRAM it is the callers responsibility
  * to re-add the already carved out pages to the mapping.
  */
-static s64 efi_mem_carve_out(struct efi_mem_list *map,
-			     struct efi_mem_list *carve_desc,
+static s64 efi_mem_carve_out(struct mem_node *map, struct mem_node *carve_desc,
 			     bool overlap_conventional)
 {
-	struct efi_mem_list *newmap;
-	struct efi_mem_list *map_desc = map;
+	struct mem_node *newmap;
+	struct mem_node *map_desc = map;
 	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;
@@ -278,8 +277,8 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 				   enum efi_memory_type mem_type,
 				   bool overlap_conventional)
 {
-	struct efi_mem_list *lmem;
-	struct efi_mem_list *newlist;
+	struct mem_node *lmem;
+	struct mem_node *newlist;
 	bool carve_again;
 	uint64_t carved_pages = 0;
 	struct efi_event *evt;
@@ -414,7 +413,7 @@ efi_status_t efi_add_memory_map(u64 start, u64 size,
  */
 static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated)
 {
-	struct efi_mem_list *item;
+	struct mem_node *item;
 
 	list_for_each_entry(item, &efi_mem, link) {
 		u64 start = item->physical_start;
@@ -666,7 +665,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 {
 	size_t map_entries;
 	efi_uintn_t map_size = 0;
-	struct efi_mem_list *lmem;
+	struct mem_node *lmem;
 	efi_uintn_t provided_map_size;
 
 	if (!memory_map_size)
-- 
2.43.0

[PATCH v4 17/25] efi_loader: Rename physical_start to base

[Simon Glass]

The word 'physical' suggests that there is an associated virtual address
but there is not. Use 'base' since this is the base address of a region.

Change some uint64_t and remove some brackets to keep checkpatch happy.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_memory.c | 38 ++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index dbcba99ae39..89f22388fc4 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -36,7 +36,7 @@ efi_uintn_t efi_memory_map_key;
  *
  * @link: Link to prev/next node in list
  * @type: EFI memory-type
- * @physical_start: Start address of region in physical memory. Note that this
+ * @base: Start address of region in physical memory. Note that this
  *	is really a pointer stored as an address, so use map_to_sysmem() to
  *	convert it to an address if needed
  * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
@@ -46,7 +46,7 @@ efi_uintn_t efi_memory_map_key;
 struct mem_node {
 	struct list_head link;
 	enum efi_memory_type type;
-	efi_physical_addr_t physical_start;
+	efi_physical_addr_t base;
 	u64 num_pages;
 	u64 attribute;
 };
@@ -121,9 +121,9 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b)
 	struct mem_node *mema = list_entry(a, struct mem_node, link);
 	struct mem_node *memb = list_entry(b, struct mem_node, link);
 
-	if (mema->physical_start == memb->physical_start)
+	if (mema->base == memb->base)
 		return 0;
-	else if (mema->physical_start < memb->physical_start)
+	else if (mema->base < memb->base)
 		return 1;
 	else
 		return -1;
@@ -137,7 +137,7 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b)
  */
 static uint64_t desc_get_end(struct mem_node *node)
 {
-	return node->physical_start + (node->num_pages << EFI_PAGE_SHIFT);
+	return node->base + (node->num_pages << EFI_PAGE_SHIFT);
 }
 
 /**
@@ -169,13 +169,13 @@ static void efi_mem_sort(void)
 			cur = lmem;
 			prev = prevmem;
 
-			if ((desc_get_end(cur) == prev->physical_start) &&
-			    (prev->type == cur->type) &&
-			    (prev->attribute == cur->attribute)) {
+			if (desc_get_end(cur) == prev->base &&
+			    prev->type == cur->type &&
+			    prev->attribute == cur->attribute) {
 				/* There is an existing map before, reuse it */
 				pages = cur->num_pages;
 				prev->num_pages += pages;
-				prev->physical_start -= pages << EFI_PAGE_SHIFT;
+				prev->base -= pages << EFI_PAGE_SHIFT;
 				list_del(&lmem->link);
 				free(lmem);
 
@@ -216,10 +216,10 @@ static s64 efi_mem_carve_out(struct mem_node *map, struct mem_node *carve_desc,
 {
 	struct mem_node *newmap;
 	struct mem_node *map_desc = map;
-	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 +
+	u64 map_start = map_desc->base;
+	u64 map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT);
+	u64 carve_start = carve_desc->base;
+	u64 carve_end = carve_start +
 			     (carve_desc->num_pages << EFI_PAGE_SHIFT);
 
 	/* check whether we're overlapping */
@@ -241,7 +241,7 @@ static s64 efi_mem_carve_out(struct mem_node *map, struct mem_node *carve_desc,
 			list_del(&map->link);
 			free(map);
 		} else {
-			map->physical_start = carve_end;
+			map->base = carve_end;
 			map->num_pages = (map_end - carve_end)
 					      >> EFI_PAGE_SHIFT;
 		}
@@ -261,7 +261,7 @@ static s64 efi_mem_carve_out(struct mem_node *map, struct mem_node *carve_desc,
 	if (!newmap)
 		return EFI_CARVE_OUT_OF_RESOURCES;
 	newmap->type = map->type;
-	newmap->physical_start = carve_start;
+	newmap->base = carve_start;
 	newmap->num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT;
 	newmap->attribute = map->attribute;
 	/* Insert before current entry (descending address order) */
@@ -298,7 +298,7 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 	if (!newlist)
 		return EFI_OUT_OF_RESOURCES;
 	newlist->type = mem_type;
-	newlist->physical_start = start;
+	newlist->base = start;
 	newlist->num_pages = pages;
 
 	switch (mem_type) {
@@ -416,7 +416,7 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated)
 	struct mem_node *item;
 
 	list_for_each_entry(item, &efi_mem, link) {
-		u64 start = item->physical_start;
+		u64 start = item->base;
 		u64 end = start + (item->num_pages << EFI_PAGE_SHIFT);
 
 		if (addr >= start && addr < end) {
@@ -697,10 +697,10 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 	list_for_each_entry(lmem, &efi_mem, link) {
 		memory_map->type = lmem->type;
 		memory_map->reserved = 0;
-		memory_map->physical_start = lmem->physical_start;
+		memory_map->physical_start = lmem->base;
 
 		/* virtual and physical are always the same */
-		memory_map->virtual_start = lmem->physical_start;
+		memory_map->virtual_start = lmem->base;
 		memory_map->num_pages = lmem->num_pages;
 		memory_map->attribute = lmem->attribute;
 		memory_map--;
-- 
2.43.0

[PATCH v4 18/25] efi_loader: Use correct type in efi_add_runtime_mmio()

[Simon Glass]

This function is passed the address of a void * so update the argument
to match. It is better to have casts in the caller than introduce
confusion as to what is passed in.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 2 +-
 arch/arm/mach-bcm283x/reset.c           | 2 +-
 include/efi_loader.h                    | 2 +-
 lib/efi_loader/efi_runtime.c            | 4 ++--
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/cpu/armv8/fsl-layerscape/cpu.c b/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
index d2d3e346a36..f9c379e9b06 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
+++ b/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
@@ -1272,7 +1272,7 @@ void __efi_runtime EFIAPI efi_reset_system(
 
 efi_status_t efi_reset_system_init(void)
 {
-	return efi_add_runtime_mmio(&rstcr, sizeof(*rstcr));
+	return efi_add_runtime_mmio((void **)&rstcr, sizeof(*rstcr));
 }
 
 #endif
diff --git a/arch/arm/mach-bcm283x/reset.c b/arch/arm/mach-bcm283x/reset.c
index 1dc7ce50d1d..fd10f01d4f7 100644
--- a/arch/arm/mach-bcm283x/reset.c
+++ b/arch/arm/mach-bcm283x/reset.c
@@ -85,7 +85,7 @@ void __efi_runtime EFIAPI efi_reset_system(
 efi_status_t efi_reset_system_init(void)
 {
 	wdog_regs = (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
-	return efi_add_runtime_mmio(&wdog_regs, sizeof(*wdog_regs));
+	return efi_add_runtime_mmio((void **)&wdog_regs, sizeof(*wdog_regs));
 }
 
 #endif
diff --git a/include/efi_loader.h b/include/efi_loader.h
index a7228672f27..4c346addae3 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -73,7 +73,7 @@ struct jmp_buf_data;
  * Call this with mmio_ptr as the _pointer_ to a pointer to an MMIO region
  * to make it available at runtime
  */
-efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len);
+efi_status_t efi_add_runtime_mmio(void **mmio_ptr, u64 len);
 
 /*
  * Special case handler for error/abort that just tries to dtrt to get
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 05369c47b01..9d3b940afbd 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -927,7 +927,7 @@ out:
  * @len:		size of the memory-mapped IO region
  * Returns:		status code
  */
-efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
+efi_status_t efi_add_runtime_mmio(void **mmio_ptr, u64 len)
 {
 	struct efi_runtime_mmio_list *newmmio;
 	uint64_t addr = *(uintptr_t *)mmio_ptr;
@@ -941,7 +941,7 @@ efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
 	if (!newmmio)
 		return EFI_OUT_OF_RESOURCES;
 	newmmio->ptr = mmio_ptr;
-	newmmio->paddr = *(uintptr_t *)mmio_ptr;
+	newmmio->paddr = (uintptr_t)*(void **)mmio_ptr;
 	newmmio->len = len;
 	list_add_tail(&newmmio->link, &efi_runtime_mmio);
 
-- 
2.43.0

[PATCH v4 19/25] efi_loader: Show the address for pool allocations

[Simon Glass]

When logging pool allocations, show the address of the pointer, not the
pointer itself. This makes it easier to debug with sandbox

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v4:
- Add new patch to show the address for pool allocations

 lib/efi_loader/efi_boottime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 334a4d64b3f..3ea239fda41 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -533,7 +533,7 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer)
 {
 	efi_status_t r;
 
-	EFI_ENTRY("%p", buffer);
+	EFI_ENTRY("%llx", (u64)map_to_sysmem(buffer));
 	r = efi_free_pool(buffer);
 	return EFI_EXIT(r);
 }
-- 
2.43.0

[PATCH v4 20/25] efi_loader: Don't try to add sandbox runtime code

[Simon Glass]

This cannot work since the code is not present in the emulated memory.
In any case, sandbox cannot make use of the runtime code.

For now, just drop it from sandbox. We can always adjust things to copy
it into memory, if needed.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_memory.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 89f22388fc4..b3ba67413ee 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -769,16 +769,22 @@ static void add_u_boot_and_runtime(void)
 	runtime_mask = SZ_64K - 1;
 #endif
 
-	/*
-	 * Add Runtime Services. We mark surrounding boottime code as runtime as
-	 * well to fulfill the runtime alignment constraints but avoid padding.
-	 */
-	runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask;
-	runtime_end = (uintptr_t)__efi_runtime_stop;
-	runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;
-	runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
-	efi_add_memory_map_pg(runtime_start, runtime_pages,
-			      EFI_RUNTIME_SERVICES_CODE, false);
+	if (!IS_ENABLED(CONFIG_SANDBOX)) {
+		/*
+		 * Add Runtime Services. We mark surrounding boottime code as
+		 * runtime as well to fulfill the runtime alignment constraints
+		 * but avoid padding.
+		 *
+		 * This is not enabled for sandbox, since we cannot map the
+		 * sandbox code into emulated SDRAM
+		 */
+		runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask;
+		runtime_end = (uintptr_t)__efi_runtime_stop;
+		runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;
+		runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
+		efi_add_memory_map_pg(runtime_start, runtime_pages,
+				      EFI_RUNTIME_SERVICES_CODE, false);
+	}
 }
 
 int efi_memory_init(void)
-- 
2.43.0

[PATCH v4 21/25] efi_loader: Update to use addresses internally

[Simon Glass]

This collects together several v3 patches into one, to avoid any
temporary test-breakage which would make future bisecting difficult.

For efi_memory, the efi_allocate_pool() call uses a pointer to refer to
memory, but efi_allocate_pages() uses an int. This causes quite a bit of
confusion, since sandbox has a distinction between pointers and
addresses. Adjust efi_allocate/free_pages_ext() to handle the
conversions.

Update efi_add_memory_map() function and its friend to use an address
rather than a pointer cast to an integer.

For lmb, now that efi_add_memory_map_pg() uses a address rather than a
pointer cast to an int, we can simplify the code here.

For efi_reserve_memory(), use addresses rather than pointers, so that it
doesn't have to use mapmem.

In general this simplifies the code, but the main benefit is that casts
are no-longer needed in most places, so the compiler can check that we
are doing the right thing.

For efi_add_runtime_mmio(), now that efi_add_memory_map() takes an
address rather than a pointer, do the mapping in this function.

Update the memset() parameter to be a char while we are here.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v4:
- Combine the various pointer-to-address patches into one

 include/efi_loader.h              | 15 ++++++------
 lib/efi_loader/efi_bootmgr.c      |  3 ++-
 lib/efi_loader/efi_boottime.c     | 39 ++++++++++++++++++++++---------
 lib/efi_loader/efi_dt_fixup.c     |  4 ----
 lib/efi_loader/efi_image_loader.c |  3 ++-
 lib/efi_loader/efi_memory.c       | 38 ++++++++++--------------------
 lib/efi_loader/efi_runtime.c      |  3 ++-
 lib/efi_loader/efi_var_mem.c      |  6 ++---
 lib/lmb.c                         | 10 +++-----
 9 files changed, 59 insertions(+), 62 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 4c346addae3..4e34e1caede 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -786,7 +786,7 @@ void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type,
  * @type:		type of allocation to be performed
  * @mem_type:		usage type of the allocated memory
  * @pages:		number of pages to be allocated
- * @memoryp:		returns a pointer to the allocated memory
+ * @memoryp:		returns the allocated address
  * Return:		status code
  */
 efi_status_t efi_allocate_pages(enum efi_allocate_type type,
@@ -796,7 +796,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 /**
  * efi_free_pages() - free memory pages
  *
- * @memory:	start of the memory area to be freed
+ * @memory:	start-address of the memory area to be freed
  * @pages:	number of pages to be freed
  * Return:	status code
  */
@@ -834,9 +834,9 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 /**
  * efi_add_memory_map() - add memory area to the memory map
  *
- * @start:		start address of the memory area. Note that this is
- *			actually a pointer provided as an integer. To pass in
- *			an address, pass in (ulong)map_to_sysmem(addr)
+ * @start:		start address, must be a multiple of EFI_PAGE_SIZE. Note
+ *			that this is an address, not a pointer. Use
+ *			map_to_sysmem(ptr) if you need to pass in a pointer
  * @size:		length in bytes of the memory area
  * @mem_type:		EFI type of memory added
  * Return:		status code
@@ -851,9 +851,8 @@ efi_status_t efi_add_memory_map(u64 start, u64 size,
  * efi_add_memory_map_pg() - add pages to the memory map
  *
  * @start: start address, must be a multiple of EFI_PAGE_SIZE. Note that this
- * is actually a pointer provided as an integer. To pass in an address, pass
- * in (ulong)map_to_sysmem(addr)
- *
+ *	is an address, not a pointer. Use map_to_sysmem(ptr) if you need to pass
+ *	in a pointer
  * @pages:			number of pages to add
  * @mem_type:		EFI type of memory added
  * @overlap_conventional:	region may only overlap free(conventional)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index bb635d77b53..30d4ce09e7e 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -14,6 +14,7 @@
 #include <efi.h>
 #include <log.h>
 #include <malloc.h>
+#include <mapmem.h>
 #include <net.h>
 #include <efi_loader.h>
 #include <efi_variable.h>
@@ -1302,7 +1303,7 @@ efi_status_t efi_bootmgr_run(void *fdt)
 	if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE)) {
 		free(fdt_lo);
 		if (fdt_distro)
-			efi_free_pages((uintptr_t)fdt_distro,
+			efi_free_pages(map_to_sysmem(fdt_distro),
 				       efi_size_in_pages(fdt_size));
 	}
 
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 3ea239fda41..e81a6d38db8 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -417,7 +417,7 @@ static void EFIAPI efi_restore_tpl(efi_uintn_t old_tpl)
  * @type:        type of allocation to be performed
  * @mem_type:    usage type of the allocated memory
  * @pages:       number of pages to be allocated
- * @memoryp:     allocated memory
+ * @memoryp:     allocated memory (a pointer, cast to u64)
  *
  * This function implements the AllocatePages service.
  *
@@ -431,15 +431,25 @@ static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int mem_type,
 						  uint64_t *memoryp)
 {
 	efi_status_t r;
+	u64 addr;
 
 	EFI_ENTRY("%d, %d, 0x%zx, %p", type, mem_type, pages, memoryp);
-	r = efi_allocate_pages(type, mem_type, pages, memoryp);
+	if (!memoryp)
+		return EFI_INVALID_PARAMETER;
+
+	/* we should not read this unless type indicates it is being used */
+	if (type == EFI_ALLOCATE_MAX_ADDRESS || type == EFI_ALLOCATE_ADDRESS)
+		addr = map_to_sysmem((void *)(uintptr_t)*memoryp);
+	r = efi_allocate_pages(type, mem_type, pages, &addr);
+	if (r == EFI_SUCCESS)
+		*memoryp = (uintptr_t)map_sysmem(addr, pages * EFI_PAGE_SIZE);
+
 	return EFI_EXIT(r);
 }
 
 /**
  * efi_free_pages_ext() - Free memory pages.
- * @memory: start of the memory area to be freed
+ * @memory: start of the memory area to be freed (a pointer, cast to u64)
  * @pages:  number of pages to be freed
  *
  * This function implements the FreePages service.
@@ -453,9 +463,12 @@ static efi_status_t EFIAPI efi_free_pages_ext(uint64_t memory,
 					      efi_uintn_t pages)
 {
 	efi_status_t r;
+	u64 addr;
 
 	EFI_ENTRY("%llx, 0x%zx", memory, pages);
-	r = efi_free_pages(memory, pages);
+	addr = map_to_sysmem((void *)(uintptr_t)memory);
+	r = efi_free_pages(addr, pages);
+
 	return EFI_EXIT(r);
 }
 
@@ -1951,8 +1964,9 @@ efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
 {
 	struct efi_file_handle *f;
 	efi_status_t ret;
-	u64 addr;
 	efi_uintn_t bs;
+	void *buf;
+	u64 addr;
 
 	/* Open file */
 	f = efi_file_from_path(file_path);
@@ -1978,10 +1992,11 @@ efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
 	}
 
 	/* Read file */
-	EFI_CALL(ret = f->read(f, &bs, (void *)(uintptr_t)addr));
+	buf = map_sysmem(addr, bs);
+	EFI_CALL(ret = f->read(f, &bs, buf));
 	if (ret != EFI_SUCCESS)
 		efi_free_pages(addr, efi_size_in_pages(bs));
-	*buffer = (void *)(uintptr_t)addr;
+	*buffer = buf;
 	*size = bs;
 error:
 	EFI_CALL(f->close(f));
@@ -2012,6 +2027,7 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
 	uint64_t addr, pages;
 	const efi_guid_t *guid;
 	struct efi_handler *handler;
+	void *buf;
 
 	/* In case of failure nothing is returned */
 	*buffer = NULL;
@@ -2050,15 +2066,16 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
 		ret = EFI_OUT_OF_RESOURCES;
 		goto out;
 	}
+	buf = map_sysmem(addr, buffer_size);
 	ret = EFI_CALL(load_file_protocol->load_file(
 					load_file_protocol, rem, boot_policy,
-					&buffer_size, (void *)(uintptr_t)addr));
+					&buffer_size, buf));
 	if (ret != EFI_SUCCESS)
 		efi_free_pages(addr, pages);
 out:
 	efi_close_protocol(device, guid, efi_root, NULL);
 	if (ret == EFI_SUCCESS) {
-		*buffer = (void *)(uintptr_t)addr;
+		*buffer = buf;
 		*size = buffer_size;
 	}
 
@@ -2121,7 +2138,7 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy,
 		ret = efi_load_pe(*image_obj, dest_buffer, source_size, info);
 	if (!source_buffer)
 		/* Release buffer to which file was loaded */
-		efi_free_pages((uintptr_t)dest_buffer,
+		efi_free_pages(map_to_sysmem(dest_buffer),
 			       efi_size_in_pages(source_size));
 	if (ret == EFI_SUCCESS || ret == EFI_SECURITY_VIOLATION) {
 		info->system_table = &systab;
@@ -3322,7 +3339,7 @@ close_next:
 		}
 	}
 
-	efi_free_pages((uintptr_t)loaded_image_protocol->image_base,
+	efi_free_pages(map_to_sysmem(loaded_image_protocol->image_base),
 		       efi_size_in_pages(loaded_image_protocol->image_size));
 	efi_delete_handle(&image_obj->header);
 
diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c
index 0dac94b0c6c..72d5d432a42 100644
--- a/lib/efi_loader/efi_dt_fixup.c
+++ b/lib/efi_loader/efi_dt_fixup.c
@@ -9,7 +9,6 @@
 #include <efi_loader.h>
 #include <efi_rng.h>
 #include <fdtdec.h>
-#include <mapmem.h>
 
 const efi_guid_t efi_guid_dt_fixup_protocol = EFI_DT_FIXUP_PROTOCOL_GUID;
 
@@ -26,9 +25,6 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap)
 	int type;
 	efi_uintn_t ret;
 
-	/* Convert from sandbox address space. */
-	addr = (uintptr_t)map_sysmem(addr, 0);
-
 	if (nomap)
 		type = EFI_RESERVED_MEMORY_TYPE;
 	else
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index 0ddf69a0918..2f2587b32a6 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -13,6 +13,7 @@
 #include <efi_loader.h>
 #include <log.h>
 #include <malloc.h>
+#include <mapmem.h>
 #include <pe.h>
 #include <sort.h>
 #include <crypto/mscode.h>
@@ -970,7 +971,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 	/* Run through relocations */
 	if (efi_loader_relocate(rel, rel_size, efi_reloc,
 				(unsigned long)image_base) != EFI_SUCCESS) {
-		efi_free_pages((uintptr_t) efi_reloc,
+		efi_free_pages(map_to_sysmem(efi_reloc),
 			       (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
 		ret = EFI_LOAD_ERROR;
 		goto err;
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index b3ba67413ee..5a6794652d1 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -36,9 +36,7 @@ efi_uintn_t efi_memory_map_key;
  *
  * @link: Link to prev/next node in list
  * @type: EFI memory-type
- * @base: Start address of region in physical memory. Note that this
- *	is really a pointer stored as an address, so use map_to_sysmem() to
- *	convert it to an address if needed
+ * @base: Start address of region in physical memory
  * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
  *	bytes)
  * @attribute: Memory attributes (see EFI_MEMORY...)
@@ -435,7 +433,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 				enum efi_memory_type mem_type,
 				efi_uintn_t pages, uint64_t *memoryp)
 {
-	u64 efi_addr, len;
+	u64 len;
 	uint flags;
 	efi_status_t ret;
 	phys_addr_t addr;
@@ -462,8 +460,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 		break;
 	case EFI_ALLOCATE_MAX_ADDRESS:
 		/* Max address */
-		addr = map_to_sysmem((void *)(uintptr_t)*memoryp);
-		addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, addr,
+		addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, *memoryp,
 						 flags);
 		if (!addr)
 			return EFI_OUT_OF_RESOURCES;
@@ -472,8 +469,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 		if (*memoryp & EFI_PAGE_MASK)
 			return EFI_NOT_FOUND;
 
-		addr = map_to_sysmem((void *)(uintptr_t)*memoryp);
-		addr = (u64)lmb_alloc_addr_flags(addr, len, flags);
+		addr = (u64)lmb_alloc_addr_flags(*memoryp, len, flags);
 		if (!addr)
 			return EFI_NOT_FOUND;
 		break;
@@ -482,17 +478,15 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 		return EFI_INVALID_PARAMETER;
 	}
 
-	efi_addr = (u64)(uintptr_t)map_sysmem(addr, 0);
 	/* Reserve that map in our memory maps */
-	ret = efi_add_memory_map_pg(efi_addr, pages, mem_type, true);
+	ret = efi_add_memory_map_pg(addr, pages, mem_type, true);
 	if (ret != EFI_SUCCESS) {
 		/* Map would overlap, bail out */
 		lmb_free_flags(addr, (u64)pages << EFI_PAGE_SHIFT, flags);
-		unmap_sysmem((void *)(uintptr_t)efi_addr);
 		return  EFI_OUT_OF_RESOURCES;
 	}
 
-	*memoryp = efi_addr;
+	*memoryp = addr;
 
 	return EFI_SUCCESS;
 }
@@ -515,18 +509,10 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
 	}
 
 	len = (u64)pages << EFI_PAGE_SHIFT;
-	/*
-	 * The 'memory' variable for sandbox holds a pointer which has already
-	 * been mapped with map_sysmem() from efi_allocate_pages(). Convert
-	 * it back to an address LMB understands
-	 */
-	status = lmb_free_flags(map_to_sysmem((void *)(uintptr_t)memory), len,
-				LMB_NOOVERWRITE);
+	status = lmb_free_flags(memory, len, LMB_NOOVERWRITE);
 	if (status)
 		return EFI_NOT_FOUND;
 
-	unmap_sysmem((void *)(uintptr_t)memory);
-
 	return ret;
 }
 
@@ -551,7 +537,7 @@ void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type,
 	if (align < EFI_PAGE_SIZE) {
 		r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type,
 				       req_pages, &mem);
-		return (r == EFI_SUCCESS) ? (void *)(uintptr_t)mem : NULL;
+		return (r == EFI_SUCCESS) ? map_sysmem(mem, len) : NULL;
 	}
 
 	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type,
@@ -572,7 +558,7 @@ void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type,
 		efi_free_pages(mem, free_pages);
 	}
 
-	return (void *)(uintptr_t)aligned_mem;
+	return map_sysmem(aligned_mem, len);
 }
 
 efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size,
@@ -595,7 +581,7 @@ efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size,
 	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, num_pages,
 			       &addr);
 	if (r == EFI_SUCCESS) {
-		alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
+		alloc = map_sysmem(addr, size);
 		alloc->num_pages = num_pages;
 		alloc->checksum = checksum(alloc);
 		*buffer = alloc->data;
@@ -626,7 +612,7 @@ efi_status_t efi_free_pool(void *buffer)
 	if (!buffer)
 		return EFI_INVALID_PARAMETER;
 
-	ret = efi_check_allocated((uintptr_t)buffer, true);
+	ret = efi_check_allocated(map_to_sysmem(buffer), true);
 	if (ret != EFI_SUCCESS)
 		return ret;
 
@@ -641,7 +627,7 @@ efi_status_t efi_free_pool(void *buffer)
 	/* Avoid double free */
 	alloc->checksum = 0;
 
-	ret = efi_free_pages((uintptr_t)alloc, alloc->num_pages);
+	ret = efi_free_pages(map_to_sysmem(alloc), alloc->num_pages);
 
 	return ret;
 }
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 9d3b940afbd..37ed5963e2b 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -13,6 +13,7 @@
 #include <efi_variable.h>
 #include <log.h>
 #include <malloc.h>
+#include <mapmem.h>
 #include <rtc.h>
 #include <asm/global_data.h>
 #include <u-boot/crc.h>
@@ -930,7 +931,7 @@ out:
 efi_status_t efi_add_runtime_mmio(void **mmio_ptr, u64 len)
 {
 	struct efi_runtime_mmio_list *newmmio;
-	uint64_t addr = *(uintptr_t *)mmio_ptr;
+	u64 addr = map_to_sysmem(*mmio_ptr);
 	efi_status_t ret;
 
 	ret = efi_add_memory_map(addr, len, EFI_MMAP_IO);
diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
index 5c69a1e0f3e..a96a1805865 100644
--- a/lib/efi_loader/efi_var_mem.c
+++ b/lib/efi_loader/efi_var_mem.c
@@ -7,6 +7,7 @@
 
 #include <efi_loader.h>
 #include <efi_variable.h>
+#include <mapmem.h>
 #include <u-boot/crc.h>
 
 /*
@@ -227,9 +228,8 @@ efi_status_t efi_var_mem_init(void)
 	if (ret != EFI_SUCCESS)
 		return ret;
 
-	/* TODO(sjg): This does not work on sandbox */
-	efi_var_buf = (struct efi_var_file *)(uintptr_t)memory;
-	memset(efi_var_buf, 0, EFI_VAR_BUF_SIZE);
+	efi_var_buf = map_sysmem(memory, EFI_VAR_BUF_SIZE);
+	memset(efi_var_buf, '\0', EFI_VAR_BUF_SIZE);
 	efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
 	efi_var_buf->length = (uintptr_t)efi_var_buf->var -
 			      (uintptr_t)efi_var_buf;
diff --git a/lib/lmb.c b/lib/lmb.c
index 14b9b8466ff..588787d2a90 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -10,7 +10,6 @@
 #include <efi_loader.h>
 #include <event.h>
 #include <image.h>
-#include <mapmem.h>
 #include <lmb.h>
 #include <log.h>
 #include <malloc.h>
@@ -448,7 +447,6 @@ static bool lmb_should_notify(enum lmb_flags flags)
 static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op,
 				 enum lmb_flags flags)
 {
-	u64 efi_addr;
 	u64 pages;
 	efi_status_t status;
 
@@ -460,11 +458,10 @@ static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op,
 	if (!lmb_should_notify(flags))
 		return 0;
 
-	efi_addr = (uintptr_t)map_sysmem(addr, 0);
-	pages = efi_size_in_pages(size + (efi_addr & EFI_PAGE_MASK));
-	efi_addr &= ~EFI_PAGE_MASK;
+	pages = efi_size_in_pages(size + (addr & EFI_PAGE_MASK));
+	addr &= ~EFI_PAGE_MASK;
 
-	status = efi_add_memory_map_pg(efi_addr, pages,
+	status = efi_add_memory_map_pg(addr, pages,
 				       op == MAP_OP_RESERVE ?
 				       EFI_BOOT_SERVICES_DATA :
 				       EFI_CONVENTIONAL_MEMORY,
@@ -474,7 +471,6 @@ static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op,
 			status & ~EFI_ERROR_MASK);
 		return -1;
 	}
-	unmap_sysmem((void *)(uintptr_t)efi_addr);
 
 	return 0;
 }
-- 
2.43.0

[PATCH v4 22/25] efi_loader: Correct address-usage in copy_fdt()

[Simon Glass]

Update this function to work correctly with sandbox

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
index fd89ef6036f..72902a5ade3 100644
--- a/lib/efi_loader/efi_helper.c
+++ b/lib/efi_loader/efi_helper.c
@@ -486,12 +486,11 @@ static efi_status_t copy_fdt(void **fdtp)
 		log_err("Failed to reserve space for FDT\n");
 		goto done;
 	}
-	/* TODO(sjg): This does not work on sandbox */
-	new_fdt = (void *)(uintptr_t)new_fdt_addr;
+	new_fdt = map_sysmem(new_fdt_addr, fdt_size);
 	memcpy(new_fdt, fdt, fdt_totalsize(fdt));
 	fdt_set_totalsize(new_fdt, fdt_size);
 
-	*fdtp = (void *)(uintptr_t)new_fdt_addr;
+	*fdtp = new_fdt;
 done:
 	return ret;
 }
-- 
2.43.0

[PATCH v4 23/25] efi_loader: Drop comments about incorrect addresses

[Simon Glass]

Now that these problems have been resolved, drop the comments.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_acpi.c    | 2 --
 lib/efi_loader/efi_bootmgr.c | 3 ---
 lib/efi_loader/efi_smbios.c  | 6 +-----
 3 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
index ffb360d461b..67bd7f8ca24 100644
--- a/lib/efi_loader/efi_acpi.c
+++ b/lib/efi_loader/efi_acpi.c
@@ -28,14 +28,12 @@ efi_status_t efi_acpi_register(void)
 	/* Mark space used for tables */
 	start = ALIGN_DOWN(gd->arch.table_start, EFI_PAGE_MASK);
 	end = ALIGN(gd->arch.table_end, EFI_PAGE_MASK);
-	/* TODO(sjg): This should use (ulong)map_sysmem(start, ...) */
 	ret = efi_add_memory_map(start, end - start, EFI_ACPI_RECLAIM_MEMORY);
 	if (ret != EFI_SUCCESS)
 		return ret;
 	if (gd->arch.table_start_high) {
 		start = ALIGN_DOWN(gd->arch.table_start_high, EFI_PAGE_MASK);
 		end = ALIGN(gd->arch.table_end_high, EFI_PAGE_MASK);
-		/* TODO(sjg): This should use (ulong)map_sysmem(start, ...) */
 		ret = efi_add_memory_map(start, end - start,
 					 EFI_ACPI_RECLAIM_MEMORY);
 		if (ret != EFI_SUCCESS)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 30d4ce09e7e..9383adbd958 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -366,8 +366,6 @@ static efi_status_t prepare_loaded_image(u16 *label, ulong addr, ulong size,
 	 * TODO: expose the ramdisk to OS.
 	 * Need to pass the ramdisk information by the architecture-specific
 	 * methods such as 'pmem' device-tree node.
-	 *
-	 * TODO(sjg): This should use (ulong)map_sysmem(addr)
 	 */
 	ret = efi_add_memory_map(addr, size, EFI_RESERVED_MEMORY_TYPE);
 	if (ret != EFI_SUCCESS) {
@@ -402,7 +400,6 @@ static efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
 
 	/* cleanup for iso or img image */
 	if (ctx->ramdisk_blk_dev) {
-		/* TODO(sjg): This should use (ulong)map_sysmem(...) */
 		ret = efi_add_memory_map(ctx->image_addr, ctx->image_size,
 					 EFI_CONVENTIONAL_MEMORY);
 		if (ret != EFI_SUCCESS)
diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index 02d4a5dd045..8d2ef6deb51 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -40,11 +40,7 @@ efi_status_t efi_smbios_register(void)
 		return EFI_NOT_FOUND;
 	}
 
-	/*
-	 * Mark space used for tables/
-	 *
-	 * TODO(sjg): This should use (ulong)map_sysmem(addr, ...)
-	 */
+	/* Mark space used for tables */
 	ret = efi_add_memory_map(addr, TABLE_SIZE, EFI_RUNTIME_SERVICES_DATA);
 	if (ret)
 		return ret;
-- 
2.43.0

[PATCH v4 24/25] efi_bootmgr: Avoid casts in try_load_from_uri_path()

[Simon Glass]

Update this function to use map_sysmem() so that it can work correctly
on sandbox

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_bootmgr.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 9383adbd958..98799aead84 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -498,6 +498,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
 	 * If the file is PE-COFF image, load the downloaded file.
 	 */
 	uri_len = strlen(uridp->uri);
+	source_buffer = map_sysmem(image_addr, image_size);
 	if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) ||
 	    !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) {
 		ret = prepare_loaded_image(lo_label, image_addr, image_size,
@@ -507,21 +508,19 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
 
 		source_buffer = NULL;
 		source_size = 0;
-
-	/* TODO(sjg): This does not work on sandbox */
-	} else if (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) {
+	} else if (efi_check_pe(source_buffer, image_size, NULL) ==
+			EFI_SUCCESS) {
 		/*
 		 * loaded_dp must exist until efi application returns,
 		 * will be freed in return_to_efibootmgr event callback.
 		 */
 		loaded_dp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
-					    (uintptr_t)image_addr, image_size);
+					    image_addr, image_size);
 		ret = efi_install_multiple_protocol_interfaces(
 			&mem_handle, &efi_guid_device_path, loaded_dp, NULL);
 		if (ret != EFI_SUCCESS)
 			goto err;
 
-		source_buffer = (void *)image_addr;
 		source_size = image_size;
 	} else {
 		log_err("Error: file type is not supported\n");
-- 
2.43.0

[PATCH v4 25/25] efi_loader: Simplify efi_dp_from_mem()

[Simon Glass]

This function should take a pointer, not an address. Update it along
with all users.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v3)

Changes in v3:
- Add comment to struct efi_device_path_memory
- Use a pointer for the values in struct efi_device_path_memory

Changes in v2:
- 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

 include/efi_api.h                | 10 ++++++++++
 include/efi_loader.h             |  5 ++---
 lib/efi_loader/efi_bootbin.c     |  3 +--
 lib/efi_loader/efi_bootmgr.c     |  2 +-
 lib/efi_loader/efi_device_path.c | 20 ++++++++++----------
 5 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index f07d074f93b..f7da915b0e4 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -573,6 +573,16 @@ struct efi_mac_addr {
 #  define DEVICE_PATH_SUB_TYPE_VENDOR		0x04
 #  define DEVICE_PATH_SUB_TYPE_CONTROLLER	0x05
 
+/**
+ * struct efi_device_path_memory - 'Memory Mapped Device Path' object
+ *
+ * @dp: header for device-path protocol
+ * @memory_type: see enum efi_memory_type
+ * @start_address: start address of the memory; note that this is provided to
+ *	the EFI application so must be a pointer cast to u64
+ * @end_address: end address of the memory; note that this is provided to
+ *	the EFI application so must be a pointer cast to u64
+ */
 struct efi_device_path_memory {
 	struct efi_device_path dp;
 	u32 memory_type;
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 4e34e1caede..1269907fa3c 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -919,9 +919,8 @@ struct efi_device_path *efi_dp_part_node(struct blk_desc *desc, int part);
 struct efi_device_path *efi_dp_from_file(const struct efi_device_path *dp,
 					 const char *path);
 struct efi_device_path *efi_dp_from_eth(void);
-struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type,
-					uint64_t start_address,
-					size_t size);
+struct efi_device_path *efi_dp_from_mem(enum efi_memory_type  mem_type,
+					void *start_ptr, size_t size);
 /* Determine the last device path node that is not the end node. */
 const struct efi_device_path *efi_dp_last_node(
 			const struct efi_device_path *dp);
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c
index a87006b3c0e..7e7a6bf31aa 100644
--- a/lib/efi_loader/efi_bootbin.c
+++ b/lib/efi_loader/efi_bootbin.c
@@ -136,8 +136,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
 		 * loaded directly into memory via JTAG, etc:
 		 */
 		file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
-					    (uintptr_t)source_buffer,
-					    source_size);
+					    source_buffer, source_size);
 		/*
 		 * Make sure that device for device_path exist
 		 * in load_image(). Otherwise, shell and grub will fail.
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 98799aead84..e3b8dfb6013 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -515,7 +515,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
 		 * will be freed in return_to_efibootmgr event callback.
 		 */
 		loaded_dp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
-					    image_addr, image_size);
+					    source_buffer, image_size);
 		ret = efi_install_multiple_protocol_interfaces(
 			&mem_handle, &efi_guid_device_path, loaded_dp, NULL);
 		if (ret != EFI_SUCCESS)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 9c8cd35b97b..5a93e10fb8e 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -11,6 +11,7 @@
 #include <dm.h>
 #include <dm/root.h>
 #include <log.h>
+#include <mapmem.h>
 #include <net.h>
 #include <usb.h>
 #include <mmc.h>
@@ -975,9 +976,8 @@ struct efi_device_path __maybe_unused *efi_dp_from_eth(void)
 }
 
 /* Construct a device-path for memory-mapped image */
-struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type,
-					uint64_t start_address,
-					size_t size)
+struct efi_device_path *efi_dp_from_mem(enum efi_memory_type  memory_type,
+					void *start_ptr, size_t size)
 {
 	struct efi_device_path_memory *mdp;
 	void *buf, *start;
@@ -990,9 +990,9 @@ struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type,
 	mdp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
 	mdp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MEMORY;
 	mdp->dp.length = sizeof(*mdp);
-	mdp->memory_type = mem_type;
-	mdp->start_address = start_address;
-	mdp->end_address = start_address + size;
+	mdp->memory_type = memory_type;
+	mdp->start_address = (uintptr_t)start_ptr;
+	mdp->end_address = mdp->start_address + size;
 	buf = &mdp[1];
 
 	*((struct efi_device_path *)buf) = END;
@@ -1061,7 +1061,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
 	struct efi_device_path *dp;
 	struct disk_partition fs_partition;
 	size_t image_size;
-	void *image_addr;
+	void *image_ptr;
 	int part = 0;
 
 	if (path && !file)
@@ -1070,10 +1070,10 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
 	if (IS_ENABLED(CONFIG_EFI_BINARY_EXEC) &&
 	    (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs")))  {
 		/* loadm command and semihosting */
-		efi_get_image_parameters(&image_addr, &image_size);
+		efi_get_image_parameters(&image_ptr, &image_size);
 
-		dp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
-				     (uintptr_t)image_addr, image_size);
+		dp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, image_ptr,
+				     image_size);
 	} else if (IS_ENABLED(CONFIG_NETDEVICES) && !strcmp(dev, "Net")) {
 		dp = efi_dp_from_eth();
 	} else if (!strcmp(dev, "Uart")) {
-- 
2.43.0

Re: [PATCH v4 00/25] efi: Tidy up confusion between pointers and addresses

[Ilias Apalodimas]

Hi Simon,

On Sun, Dec 01, 2024 at 08:24:19AM -0700, Simon Glass 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
>
> Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/

In v3 I did ask you to run SCT and spent some time on IRC to explain how to
run it.
The existing code passes all tests related to memory allocations.
This patch introduced 99 failures in MemoryAllocationServicesTest.
Apart from all the other feedback, SCT must pass.

Thanks
/Ilias

>
> 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
> - Drop the changes to the boottime API
> - 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 (25):
>   efi: Define fields in struct efi_mem_desc
>   efi_loader: Fix typos in enum efi_allocate_type
>   efi_loader: Drop extra brackets in efi_mem_carve_out()
>   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 the enum for memory type
>   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                          | 102 ++++--
>  lib/efi_loader/efi_bootbin.c                  |   3 +-
>  lib/efi_loader/efi_bootmgr.c                  |  10 +-
>  lib/efi_loader/efi_boottime.c                 |  53 ++-
>  lib/efi_loader/efi_device_path.c              |  18 +-
>  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                   | 301 +++++++-----------
>  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, 313 insertions(+), 257 deletions(-)
>
> --
> 2.43.0
>

Re: [PATCH v4 00/25] efi: Tidy up confusion between pointers and addresses

[Simon Glass]

Hi Ilias,

On Wed, 4 Dec 2024 at 07:46, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Sun, Dec 01, 2024 at 08:24:19AM -0700, Simon Glass 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
> >
> > Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
>
> In v3 I did ask you to run SCT and spent some time on IRC to explain how to
> run it.
> The existing code passes all tests related to memory allocations.
> This patch introduced 99 failures in MemoryAllocationServicesTest.
> Apart from all the other feedback, SCT must pass.

Hmm I must be doing something wrong, as it passed for me. I'll try again.

Regards,
Simon