[U-Boot] [PATCH v4 0/1] efi_loader: Fix inconsistencies in efi_add_memory_map usage

[U-Boot] [PATCH v4 0/1] efi_loader: Fix inconsistencies in efi_add_memory_map usage

[Bryan O'Donoghue]

V4:

- Squash series down into one patch
- Adds RB Heinrich
- Ensures comment in efi_free_pages is kept

V3:

- Takes addition of function description and makes it a separate patch
- Subtracts dangling "uint64_t r" in V2 patch

Heinrich if you want to take this set as is add a SOB to
"efi_loader: Add a method description to efi_add_memory_map"

and if not please take in my change for "efi_add_runtime_mmio()" to your
single patch version.

V2:

Following on from a discussion with Heinrich Schuchardt, please find a
reworked set of patches updating efi_add_memory_map() to

- Return efi_status_t
- Return EFI_SUCCESS where appropriate
- Return EFI_NO_MAPPING in two cases where zero was returned to indicate an
  error
- Updating of users of efi_add_memory_map() to parse for
  EFI_SUCCESS/efi_status_t

I've opted to maintain other returned status codes propogated by functions
that call efi_add_memory_map(). For example efi_add_runtime_mmio()
continues return EFI_OUT_OF_RESOURCES instead of directly returning the
result code of efi_add_memory_map(). The idea being that other users of the
EFI layer such as Linux or grub would not be affected by this internal
u-boot change.

V1:

https://patchwork.ozlabs.org/patch/1129402/
https://patchwork.ozlabs.org/patch/1129403/

These two patches fix some inconsistent usage around efi_add_memory_map().

The first patch fixes the case where there is a mapping for an address
starting at 0 as is the case on RPI3. We should not print an error for
this. efi_add_memory_map(start = 0, ...) succeeds but
efi_carve_out_dt_rsv() does not properly parse the result code.

The second patch fixes the result code returned by efi_add_memory_map() in
two instances. Returing zero is the same as returning EFI_SUCCESS, we
should return one of the error codes from include/efi.h only, not zero to
indicate failure.



Bryan O'Donoghue (1):
  efi_loader: Change return type of efi_add_memory_map()

 cmd/bootefi.c                |  4 ++--
 include/efi_loader.h         |  4 ++--
 lib/efi_loader/efi_memory.c  | 32 ++++++++++++++++++++------------
 lib/efi_loader/efi_runtime.c |  6 +++---
 4 files changed, 27 insertions(+), 19 deletions(-)

-- 
2.22.0

[U-Boot] [PATCH v4 1/1] efi_loader: Change return type of efi_add_memory_map()

[Bryan O'Donoghue]

We currently have some inconsistent use of efi_add_memory_map()
throughout the code. In particular the return value of efi_add_memory_map()
is not interpreted the same way by various users in the codebase.

This patch does the following:

- Changes efi_add_memory_map() to return efi_status_t.
- Adds a method description to efi_add_memory_map().
- Changes efi_add_memory_map() to return EFI_SUCCESS
- Returns non-zero for error in efi_add_memory_map()
- Updates efi_allocate_pages() to new efi_add_memory_map()
- Updates efi_free_pages() to new efi_add_memory_map()
- Updates efi_carve_out_dt_rsv() to new efi_add_memory_map()
- Updates efi_add_runtime_mmio()  to new efi_add_memory_map()

Fixes: 5d00995c361c ("efi_loader: Implement memory allocation and map")
Fixes: 74c16acce30b ("efi_loader: Don't allocate from memory holes")
Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Alexander Graf <agraf@csgraf.de>
Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 cmd/bootefi.c                |  4 ++--
 include/efi_loader.h         |  4 ++--
 lib/efi_loader/efi_memory.c  | 32 ++++++++++++++++++++------------
 lib/efi_loader/efi_runtime.c |  6 +++---
 4 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index c19256e00d..04d3e3e4a7 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -169,8 +169,8 @@ static void efi_carve_out_dt_rsv(void *fdt)
 
 		pages = efi_size_in_pages(size + (addr & EFI_PAGE_MASK));
 		addr &= ~EFI_PAGE_MASK;
-		if (!efi_add_memory_map(addr, pages, EFI_RESERVED_MEMORY_TYPE,
-					false))
+		if (efi_add_memory_map(addr, pages, EFI_RESERVED_MEMORY_TYPE,
+				       false) != EFI_SUCCESS)
 			printf("FDT memrsv map %d: Failed to add to map\n", i);
 	}
 }
diff --git a/include/efi_loader.h b/include/efi_loader.h
index b07155cecb..69255f40ea 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -470,8 +470,8 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 				efi_uintn_t *descriptor_size,
 				uint32_t *descriptor_version);
 /* Adds a range into the EFI memory map */
-uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
-			    bool overlap_only_ram);
+efi_status_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
+				bool overlap_only_ram);
 /* Called by board init to initialize the EFI drivers */
 efi_status_t efi_driver_init(void);
 /* Called by board init to initialize the EFI memory map */
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 27379381e8..4c0216b1d2 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -223,8 +223,17 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
 	return EFI_CARVE_LOOP_AGAIN;
 }
 
-uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
-			    bool overlap_only_ram)
+/**
+ * efi_add_memory_map() - add memory area 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_only_ram:	the memory area must overlap existing
+ * Return:		status code
+ */
+efi_status_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
+				bool overlap_only_ram)
 {
 	struct list_head *lhandle;
 	struct efi_mem_list *newlist;
@@ -239,7 +248,7 @@ uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
 		return EFI_INVALID_PARAMETER;
 
 	if (!pages)
-		return start;
+		return EFI_SUCCESS;
 
 	++efi_memory_map_key;
 	newlist = calloc(1, sizeof(*newlist));
@@ -277,7 +286,7 @@ uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
 				 * The user requested to only have RAM overlaps,
 				 * but we hit a non-RAM region. Error out.
 				 */
-				return 0;
+				return EFI_NO_MAPPING;
 			case EFI_CARVE_NO_OVERLAP:
 				/* Just ignore this list entry */
 				break;
@@ -307,7 +316,7 @@ uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
 		 * The payload wanted to have RAM overlaps, but we overlapped
 		 * with an unallocated region. Error out.
 		 */
-		return 0;
+		return EFI_NO_MAPPING;
 	}
 
 	/* Add our new map */
@@ -326,7 +335,7 @@ uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
 		}
 	}
 
-	return start;
+	return EFI_SUCCESS;
 }
 
 /**
@@ -455,7 +464,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type,
 	}
 
 	/* Reserve that map in our memory maps */
-	if (efi_add_memory_map(addr, pages, memory_type, true) != addr)
+	if (efi_add_memory_map(addr, pages, memory_type, true) != EFI_SUCCESS)
 		/* Map would overlap, bail out */
 		return  EFI_OUT_OF_RESOURCES;
 
@@ -487,7 +496,6 @@ void *efi_alloc(uint64_t len, int memory_type)
  */
 efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
 {
-	uint64_t r = 0;
 	efi_status_t ret;
 
 	ret = efi_check_allocated(memory, true);
@@ -501,13 +509,13 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
 		return EFI_INVALID_PARAMETER;
 	}
 
-	r = efi_add_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY, false);
+	ret = efi_add_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY, false);
 	/* Merging of adjacent free regions is missing */
 
-	if (r == memory)
-		return EFI_SUCCESS;
+	if (ret != EFI_SUCCESS)
+		return EFI_NOT_FOUND;
 
-	return EFI_NOT_FOUND;
+	return ret;
 }
 
 /**
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 40fdc0ea92..12ee6faadd 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -659,10 +659,10 @@ efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
 	struct efi_runtime_mmio_list *newmmio;
 	u64 pages = (len + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
 	uint64_t addr = *(uintptr_t *)mmio_ptr;
-	uint64_t retaddr;
+	efi_status_t ret;
 
-	retaddr = efi_add_memory_map(addr, pages, EFI_MMAP_IO, false);
-	if (retaddr != addr)
+	ret = efi_add_memory_map(addr, pages, EFI_MMAP_IO, false);
+	if (ret != EFI_SUCCESS)
 		return EFI_OUT_OF_RESOURCES;
 
 	newmmio = calloc(1, sizeof(*newmmio));
-- 
2.22.0

[U-Boot] [PATCH v4 1/1] efi_loader: Change return type of efi_add_memory_map()

[Heinrich Schuchardt]

On 7/15/19 1:00 PM, Bryan O'Donoghue wrote:
> We currently have some inconsistent use of efi_add_memory_map()
> throughout the code. In particular the return value of efi_add_memory_map()
> is not interpreted the same way by various users in the codebase.
>
> This patch does the following:
>
> - Changes efi_add_memory_map() to return efi_status_t.
> - Adds a method description to efi_add_memory_map().
> - Changes efi_add_memory_map() to return EFI_SUCCESS
> - Returns non-zero for error in efi_add_memory_map()
> - Updates efi_allocate_pages() to new efi_add_memory_map()
> - Updates efi_free_pages() to new efi_add_memory_map()
> - Updates efi_carve_out_dt_rsv() to new efi_add_memory_map()
> - Updates efi_add_runtime_mmio()  to new efi_add_memory_map()
>
> Fixes: 5d00995c361c ("efi_loader: Implement memory allocation and map")
> Fixes: 74c16acce30b ("efi_loader: Don't allocate from memory holes")
> Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Alexander Graf <agraf@csgraf.de>
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>


Applied to efi-2019-10.

https://gitlab.denx.de/u-boot/custodians/u-boot-efi/tree/efi-2019-10

Best regards

Heinrich