[U-Boot] [PATCH 0/2] efi_loader: Fix inconsistencies in efi_add_memory_map usage
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 (2): efi_loader: Check the result of efi_add_memory_map against input addr efi_loader: Return non-zero for error in efi_add_memory_map()
cmd/bootefi.c | 4 ++-- lib/efi_loader/efi_memory.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
We should check the result of efi_add_memory_map() against the first input parameter, not against zero.
In efi_carve_out_dt_rsv() add_efi_memory_map() gets passed addr = 0. The function succeeds but the parsing routine interprets zero as an error.
Fix that now by comparing the result code of add_efi_memory_map() to the first input parameter as other users of add_efi_memory_map() already do.
Removes this error 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..0b404ccbd1 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) != addr) printf("FDT memrsv map %d: Failed to add to map\n", i); } }
If we are trying to map address zero, as is the case on raspberrypi, then we have no way of telling the difference between a valid efi_add_memory_map() at zero and an overlapping error at zero.
Instead of returning zero, return EFI_NO_MAPPING on carve-out errors. In include/efi.h we can see that EFI_SUCCESS is defined as 0, so it seems we ought to return one of the available EFI error codes. EFI_NO_MAPPING seems to make sense.
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 27379381e8..7d6aab255c 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -277,7 +277,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 +307,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 */
On 7/9/19 1:10 AM, Bryan O'Donoghue wrote:
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 (2): efi_loader: Check the result of efi_add_memory_map against input addr efi_loader: Return non-zero for error in efi_add_memory_map()
cmd/bootefi.c | 4 ++-- lib/efi_loader/efi_memory.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
Hello Bryon,
thanks for pointing out this design error.
I found no instance in our code where the return value of efi_add_memory_map() is used for anything else but checking if the operation was successful. It seems appropriate to change the signature of the function to
efi_status_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type, bool overlap_only_ram);
and return EFI_SUCCESS or an error code. It would be very kind if you could send an updated patch.
Best regards
Heinrich
On 09/07/2019 07:02, Heinrich Schuchardt wrote:
On 7/9/19 1:10 AM, Bryan O'Donoghue wrote:
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 (2): efi_loader: Check the result of efi_add_memory_map against input addr efi_loader: Return non-zero for error in efi_add_memory_map()
cmd/bootefi.c | 4 ++-- lib/efi_loader/efi_memory.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
Hello Bryon,
thanks for pointing out this design error.
I found no instance in our code where the return value of efi_add_memory_map() is used for anything else but checking if the operation was successful. It seems appropriate to change the signature of the function to
efi_status_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type, bool overlap_only_ram);
and return EFI_SUCCESS or an error code. It would be very kind if you could send an updated patch.
Best regards
Heinrich
sure np
participants (2)
-
Bryan O'Donoghue -
Heinrich Schuchardt