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

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

[Bryan O'Donoghue]

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(-)

-- 
2.22.0

[U-Boot] [PATCH 1/2] efi_loader: Check the result of efi_add_memory_map against input addr

[Bryan O'Donoghue]

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);
 	}
 }
-- 
2.22.0

[U-Boot] [PATCH 2/2] efi_loader: Return non-zero for error in efi_add_memory_map()

[Bryan O'Donoghue]

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 */
-- 
2.22.0

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

[Heinrich Schuchardt]

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

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

[Bryan O'Donoghue]

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