[U-Boot] [PATCH v2 0/7] efi_loader: Fix inconsistencies in efi_add_memory_map usage
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 (7): efi_loader: Change return type of efi_add_memory_map() efi_loader: Change efi_add_memory_map() to return EFI_SUCCESS efi_loader: Return non-zero for error in efi_add_memory_map() efi_loader: Update efi_allocate_pages() to new efi_add_memory_map() efi_loader: Update efi_free_pages() to new efi_add_memory_map() efi_loader: Treat the result of efi_add_memory_map as efi_status_t efi_loader: Capture efi_add_memory_map() result efi_add_runtime_mmio()
cmd/bootefi.c | 4 ++-- include/efi_loader.h | 4 ++-- lib/efi_loader/efi_memory.c | 24 +++++++++++------------- lib/efi_loader/efi_runtime.c | 6 +++--- 4 files changed, 18 insertions(+), 20 deletions(-)
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 changes efi_add_memory_map() to return efi_status_t. A subsequent set of patches will change the internal values themselves and finally the users of efi_add_memory_map().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Suggested-by: Heinrich Schuchardt xypron.glpk@gmx.de --- include/efi_loader.h | 4 ++-- lib/efi_loader/efi_memory.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
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..c5a8f3ab29 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -223,8 +223,8 @@ 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_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;
On 7/15/19 12:29 AM, 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 changes efi_add_memory_map() to return efi_status_t. A subsequent set of patches will change the internal values themselves and finally the users of efi_add_memory_map().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie
Your Signed-off-by should be the last line in the header.
Suggested-by: Heinrich Schuchardt xypron.glpk@gmx.de
include/efi_loader.h | 4 ++-- lib/efi_loader/efi_memory.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
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);diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 27379381e8..c5a8f3ab29 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -223,8 +223,8 @@ 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_status_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
bool overlap_only_ram)
efi_add_memory_map() wants to return 0 to indicate success in two places. Instead of returning zero we should return the defined efi_status_t return value EFI_SUCCESS.
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie --- 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 c5a8f3ab29..b513553fa4 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -239,7 +239,7 @@ efi_status_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)); @@ -326,7 +326,7 @@ efi_status_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type, } }
- return start; + return EFI_SUCCESS; }
/**
The previous implementation of efi_add_memory_map() returned the passed address on success, instead of an efi_status_t.
With the new function signature instead of returning zero, return EFI_NO_MAPPING on carve-out errors.
Fixes: 5d00995c361c ("efi_loader: Implement memory allocation and map") Fixes: 74c16acce30b ("efi_loader: Don't allocate from memory holes") Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Alexander Graf agraf@csgraf.de Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie --- 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 b513553fa4..c1d7b88997 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -277,7 +277,7 @@ efi_status_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 +307,7 @@ efi_status_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 */
efi_add_memory_map() now returns efi_status_t not the passed uint64_t address on success. We need to capture that change in efi_allocate_pages().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie --- lib/efi_loader/efi_memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index c1d7b88997..4ce3dd8f8c 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -455,7 +455,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;
efi_add_memory_map() now returns efi_status_t not the passed uint64_t address on success. We need to capture that change in efi_free_pages().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie --- lib/efi_loader/efi_memory.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 4ce3dd8f8c..57016a393b 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -501,13 +501,11 @@ 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); - /* Merging of adjacent free regions is missing */ - - if (r == memory) - return EFI_SUCCESS; + ret = efi_add_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY, false); + if (ret != EFI_SUCCESS) + return EFI_NOT_FOUND;
- return EFI_NOT_FOUND; + return ret; }
/**
The return type of efi_add_memory_map() has been updated to return efi_status_t as a result we should check the return of efi_add_memory_map() against EFI_SUCCESS as opposed to the original input addr parameter.
This change will also remove this error message on raspberrypi 3 boot: "FDT memrsv map 0: Failed to add to map".
Fixes: 416e07e2cfcf ("efi: Drop error return in efi_carve_out_dt_rsv()") Cc: 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 ++-- 1 file changed, 2 insertions(+), 2 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); } }
efi_add_runtime_mmio() does an efi_add_memory_map() call. We have recently changed the return value of efi_add_memory_map() to return an efi_status_t code.
This patch captures the result code of efi_add_memory_map() and if that result code is not EFI_SUCCESS returns EFI_OUT_OF_RESOURCES;
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie --- lib/efi_loader/efi_runtime.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
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));
On 7/15/19 12:29 AM, Bryan O'Donoghue wrote:
V2:
Thanks for updating your patch series.
Each single patch applied should end up in something that succeeds in Travis CI. So a patch changing a return value should not be separated from a patch adjusting the callers of the function or changing the definition the function. I think all seven patches should be merged to one.
Best regards
Heinrich
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 (7): efi_loader: Change return type of efi_add_memory_map() efi_loader: Change efi_add_memory_map() to return EFI_SUCCESS efi_loader: Return non-zero for error in efi_add_memory_map() efi_loader: Update efi_allocate_pages() to new efi_add_memory_map() efi_loader: Update efi_free_pages() to new efi_add_memory_map() efi_loader: Treat the result of efi_add_memory_map as efi_status_t efi_loader: Capture efi_add_memory_map() result efi_add_runtime_mmio()
cmd/bootefi.c | 4 ++-- include/efi_loader.h | 4 ++-- lib/efi_loader/efi_memory.c | 24 +++++++++++------------- lib/efi_loader/efi_runtime.c | 6 +++--- 4 files changed, 18 insertions(+), 20 deletions(-)
participants (2)
-
Bryan O'Donoghue -
Heinrich Schuchardt