diff mbox series

[v3,1/1] cmd: bootefi: Parse reserved-memory node from DT

Message ID 20200315120314.15398-1-xypron.glpk@gmx.de
State Accepted
Commit 7be64b885a360d0b7e78a5f624698a67513be53f
Headers show
Series [v3,1/1] cmd: bootefi: Parse reserved-memory node from DT | expand

Commit Message

Heinrich Schuchardt March 15, 2020, 12:03 p.m. UTC 
From: Atish Patra <atish.patra at wdc.com>

Currently, bootefi only parses memory reservation block to setup
EFI reserved memory mappings. However, it doesn't parse the
reserved-memory[1] device tree node that also can contain the
reserved memory regions.

Add capability to parse reserved-memory node and update the EFI memory
mappings accordingly.

1. <U-Boot source>/doc/device-tree-bindings/reserved-memory/reserved-memory.txt]

Signed-off-by: Atish Patra <atish.patra at wdc.com>

Fix an endless loop.

The /reserved-memory node may have children without reg property. Remove
a superfluous debug statement.

Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
---
 cmd/bootefi.c | 43 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 9 deletions(-)

--
2.25.1

Comments

Patrick Delaunay March 17, 2020, 8:06 a.m. UTC | #1 
Hi,

> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Heinrich Schuchardt
> Sent: dimanche 15 mars 2020 13:03
> 
> From: Atish Patra <atish.patra at wdc.com>
> 
> Currently, bootefi only parses memory reservation block to setup EFI reserved
> memory mappings. However, it doesn't parse the reserved-memory[1] device tree
> node that also can contain the reserved memory regions.
> 
> Add capability to parse reserved-memory node and update the EFI memory
> mappings accordingly.
> 
> 1. <U-Boot source>/doc/device-tree-bindings/reserved-memory/reserved-
> memory.txt]
> 
> Signed-off-by: Atish Patra <atish.patra at wdc.com>
> 
> Fix an endless loop.
> 
> The /reserved-memory node may have children without reg property. Remove a
> superfluous debug statement.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
>  cmd/bootefi.c | 43 ++++++++++++++++++++++++++++++++++---------
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 24fc42ae89..b5b9d88273 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -149,6 +149,20 @@ done:
>  	return ret;
>  }
> 
> +static void efi_reserve_memory(u64 addr, u64 size) {
> +	u64 pages;
> +
> +	/* Convert from sandbox address space. */
> +	addr = (uintptr_t)map_sysmem(addr, 0);
> +	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) != EFI_SUCCESS)
> +		printf("Reserved memory mapping failed addr %llx size %llx\n",
> +		       addr, size);
> +}
> +
>  /**
>   * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
>   *
> @@ -161,7 +175,8 @@ done:
>  static void efi_carve_out_dt_rsv(void *fdt)  {
>  	int nr_rsv, i;
> -	uint64_t addr, size, pages;
> +	u64 addr, size;
> +	int nodeoffset, subnode;
> 
>  	nr_rsv = fdt_num_mem_rsv(fdt);
> 
> @@ -169,15 +184,25 @@ static void efi_carve_out_dt_rsv(void *fdt)
>  	for (i = 0; i < nr_rsv; i++) {
>  		if (fdt_get_mem_rsv(fdt, i, &addr, &size) != 0)
>  			continue;
> +		efi_reserve_memory(addr, size);
> +	}
> 
> -		/* Convert from sandbox address space. */
> -		addr = (uintptr_t)map_sysmem(addr, 0);
> -
> -		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) != EFI_SUCCESS)
> -			printf("FDT memrsv map %d: Failed to add to map\n", i);
> +	/* process reserved-memory */
> +	nodeoffset = fdt_subnode_offset(fdt, 0, "reserved-memory");
> +	if (nodeoffset >= 0) {
> +		subnode = fdt_first_subnode(fdt, nodeoffset);
> +		while (subnode >= 0) {
> +			/* check if this subnode has a reg property */
> +			addr = fdtdec_get_addr_size(fdt, subnode, "reg",
> +						    (fdt_size_t *)&size);
> +			/*
> +			 * The /reserved-memory node may have children with
> +			 * a size instead of a reg property.
> +			 */
> +			if (addr != FDT_ADDR_T_NONE)
> +				efi_reserve_memory(addr, size);
> +			subnode = fdt_next_subnode(fdt, subnode);
> +		}
>  	}
>  }

This part seens the same that the existing function
common/image-fdt.c::boot_fdt_add_mem_rsv_regions()

For information, a status check was added by commit 
"image: fdt: check "status" of "/reserved-memory" subnodes"

See SHA1 28b417ce859490d6b06e71dbf4e842841e64d34d

Perhaps need to be added also here.

Regards

Patrick
Heinrich Schuchardt March 17, 2020, 11:10 a.m. UTC | #2 
On 3/17/20 9:06 AM, Patrick DELAUNAY wrote:
> Hi,
>
>> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Heinrich Schuchardt
>> Sent: dimanche 15 mars 2020 13:03
>>
>> From: Atish Patra <atish.patra at wdc.com>
>>
>> Currently, bootefi only parses memory reservation block to setup EFI reserved
>> memory mappings. However, it doesn't parse the reserved-memory[1] device tree
>> node that also can contain the reserved memory regions.
>>
>> Add capability to parse reserved-memory node and update the EFI memory
>> mappings accordingly.
>>
>> 1. <U-Boot source>/doc/device-tree-bindings/reserved-memory/reserved-
>> memory.txt]
>>
>> Signed-off-by: Atish Patra <atish.patra at wdc.com>
>>
>> Fix an endless loop.
>>
>> The /reserved-memory node may have children without reg property. Remove a
>> superfluous debug statement.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> ---
>>   cmd/bootefi.c | 43 ++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 34 insertions(+), 9 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 24fc42ae89..b5b9d88273 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -149,6 +149,20 @@ done:
>>   	return ret;
>>   }
>>
>> +static void efi_reserve_memory(u64 addr, u64 size) {
>> +	u64 pages;
>> +
>> +	/* Convert from sandbox address space. */
>> +	addr = (uintptr_t)map_sysmem(addr, 0);
>> +	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) != EFI_SUCCESS)
>> +		printf("Reserved memory mapping failed addr %llx size %llx\n",
>> +		       addr, size);
>> +}
>> +
>>   /**
>>    * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
>>    *
>> @@ -161,7 +175,8 @@ done:
>>   static void efi_carve_out_dt_rsv(void *fdt)  {
>>   	int nr_rsv, i;
>> -	uint64_t addr, size, pages;
>> +	u64 addr, size;
>> +	int nodeoffset, subnode;
>>
>>   	nr_rsv = fdt_num_mem_rsv(fdt);
>>
>> @@ -169,15 +184,25 @@ static void efi_carve_out_dt_rsv(void *fdt)
>>   	for (i = 0; i < nr_rsv; i++) {
>>   		if (fdt_get_mem_rsv(fdt, i, &addr, &size) != 0)
>>   			continue;
>> +		efi_reserve_memory(addr, size);
>> +	}
>>
>> -		/* Convert from sandbox address space. */
>> -		addr = (uintptr_t)map_sysmem(addr, 0);
>> -
>> -		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) != EFI_SUCCESS)
>> -			printf("FDT memrsv map %d: Failed to add to map\n", i);
>> +	/* process reserved-memory */
>> +	nodeoffset = fdt_subnode_offset(fdt, 0, "reserved-memory");
>> +	if (nodeoffset >= 0) {
>> +		subnode = fdt_first_subnode(fdt, nodeoffset);
>> +		while (subnode >= 0) {
>> +			/* check if this subnode has a reg property */
>> +			addr = fdtdec_get_addr_size(fdt, subnode, "reg",
>> +						    (fdt_size_t *)&size);
>> +			/*
>> +			 * The /reserved-memory node may have children with
>> +			 * a size instead of a reg property.
>> +			 */
>> +			if (addr != FDT_ADDR_T_NONE)
>> +				efi_reserve_memory(addr, size);
>> +			subnode = fdt_next_subnode(fdt, subnode);
>> +		}
>>   	}
>>   }
>
> This part seens the same that the existing function
> common/image-fdt.c::boot_fdt_add_mem_rsv_regions()
>
> For information, a status check was added by commit
> "image: fdt: check "status" of "/reserved-memory" subnodes"
>
> See SHA1 28b417ce859490d6b06e71dbf4e842841e64d34d
>
> Perhaps need to be added also here.

It makes sense to keep these coding stretches in sync.

@Thirupathaiah
* I did not find any example in the Linux code where a status for
   a subnode of /reserved-memory was specified. Where did you
   observe reserved memory not having status="okay"?
* Does Linux check the status?
* Could the /reserved-memory node itself have a status other than
   "okay"?

How should we treat reserved memory marked as 'reusable'? Should UEFI
make these regions as reserved?

I tried to understand when boot_fdt_add_mem_rsv_regions() is invoked. I
was astonished that the dhcp command calls it when loading a file. At
that time the device-tree used for booting is not known:

=> dhcp helloworld_arm64.efi
e1000: 52:54:00:12:34:56

Warning: e1000#0 using MAC address from ROM
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
DHCP client bound to address 10.0.2.15 (1010 ms)
Using e1000#0 device
TFTP from server 10.0.2.2; our IP address is 10.0.2.15
Filename 'helloworld_arm64.efi'.
common/image-fdt.c(110) boot_fdt_add_mem_rsv_regions:
Load address: 0x40200000
Loading: #
          2.2 MiB/s
done
Bytes transferred = 4528 (11b0 hex)

Best regards

Heinrich
Thirupathaiah Annapureddy March 24, 2020, 5:42 a.m. UTC | #3 
On 3/17/2020 4:10 AM, Heinrich Schuchardt wrote:
> On 3/17/20 9:06 AM, Patrick DELAUNAY wrote:
>> Hi,
>>
>>> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Heinrich Schuchardt
>>> Sent: dimanche 15 mars 2020 13:03
>>>

> 
> It makes sense to keep these coding stretches in sync.
> 
> @Thirupathaiah
> * I did not find any example in the Linux code where a status for
> ? a subnode of /reserved-memory was specified. Where did you
> ? observe reserved memory not having status="okay"?
A child node of /reserved-memory may be declared in a SOC specific dtsi file.
But a given board's dts file may chose to disable this i.e. it does not need
to reserve this memory. 

> * Does Linux check the status?
yes, it checks. 
__fdt_scan_reserved_mem() -> of_fdt_device_is_available()

Best Regards,
Thiru
diff mbox series

Patch

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 24fc42ae89..b5b9d88273 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -149,6 +149,20 @@  done:
 	return ret;
 }

+static void efi_reserve_memory(u64 addr, u64 size)
+{
+	u64 pages;
+
+	/* Convert from sandbox address space. */
+	addr = (uintptr_t)map_sysmem(addr, 0);
+	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) != EFI_SUCCESS)
+		printf("Reserved memory mapping failed addr %llx size %llx\n",
+		       addr, size);
+}
+
 /**
  * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
  *
@@ -161,7 +175,8 @@  done:
 static void efi_carve_out_dt_rsv(void *fdt)
 {
 	int nr_rsv, i;
-	uint64_t addr, size, pages;
+	u64 addr, size;
+	int nodeoffset, subnode;

 	nr_rsv = fdt_num_mem_rsv(fdt);

@@ -169,15 +184,25 @@  static void efi_carve_out_dt_rsv(void *fdt)
 	for (i = 0; i < nr_rsv; i++) {
 		if (fdt_get_mem_rsv(fdt, i, &addr, &size) != 0)
 			continue;
+		efi_reserve_memory(addr, size);
+	}

-		/* Convert from sandbox address space. */
-		addr = (uintptr_t)map_sysmem(addr, 0);
-
-		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) != EFI_SUCCESS)
-			printf("FDT memrsv map %d: Failed to add to map\n", i);
+	/* process reserved-memory */
+	nodeoffset = fdt_subnode_offset(fdt, 0, "reserved-memory");
+	if (nodeoffset >= 0) {
+		subnode = fdt_first_subnode(fdt, nodeoffset);
+		while (subnode >= 0) {
+			/* check if this subnode has a reg property */
+			addr = fdtdec_get_addr_size(fdt, subnode, "reg",
+						    (fdt_size_t *)&size);
+			/*
+			 * The /reserved-memory node may have children with
+			 * a size instead of a reg property.
+			 */
+			if (addr != FDT_ADDR_T_NONE)
+				efi_reserve_memory(addr, size);
+			subnode = fdt_next_subnode(fdt, subnode);
+		}
 	}
 }