diff mbox series

[v2,1/1] efi_loader: put device tree into EfiACPIReclaimMemory

Message ID 20200507181937.8923-1-xypron.glpk@gmx.de
State Accepted
Commit 42a426e027df472714e8d6209cafac291935c331
Headers show
Series [v2,1/1] efi_loader: put device tree into EfiACPIReclaimMemory | expand

Commit Message

Heinrich Schuchardt May 7, 2020, 6:19 p.m. UTC
According to the UEFI spec ACPI tables should be placed in
EfiACPIReclaimMemory. Let's do the same with the device tree.

Suggested-by: Ard Biesheuvel <ardb at kernel.org>
Cc: Grant Likely <grant.likely at arm.com>
Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
---
v2:
	adjust the unit test
---
 cmd/bootefi.c                          | 4 ++--
 lib/efi_selftest/efi_selftest_memory.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

--
2.26.2

Comments

Grant Likely May 11, 2020, 8:48 a.m. UTC | #1
On 07/05/2020 19:19, Heinrich Schuchardt wrote:
> According to the UEFI spec ACPI tables should be placed in
> EfiACPIReclaimMemory. Let's do the same with the device tree.
> 
> Suggested-by: Ard Biesheuvel <ardb at kernel.org>
> Cc: Grant Likely <grant.likely at arm.com>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
> v2:
> 	adjust the unit test

Is there any impact to changing the memory type for current users? Does 
the kernel currently expect the EFI_BOOT_SERVICES_DATA memory type? What 
happens if Grub or another EFI application replaces the DTB table? Will 
it try to use a different memory type, and will that matter?

g.

> ---
>   cmd/bootefi.c                          | 4 ++--
>   lib/efi_selftest/efi_selftest_memory.c | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 54b4b8f984..06573b14e9 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -127,13 +127,13 @@ static efi_status_t copy_fdt(void **fdtp)
>   	new_fdt_addr = (uintptr_t)map_sysmem(fdt_ram_start + 0x7f00000 +
>   					     fdt_size, 0);
>   	ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
> -				 EFI_BOOT_SERVICES_DATA, fdt_pages,
> +				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
>   				 &new_fdt_addr);
>   	if (ret != EFI_SUCCESS) {
>   		/* If we can't put it there, put it somewhere */
>   		new_fdt_addr = (ulong)memalign(EFI_PAGE_SIZE, fdt_size);
>   		ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
> -					 EFI_BOOT_SERVICES_DATA, fdt_pages,
> +					 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
>   					 &new_fdt_addr);
>   		if (ret != EFI_SUCCESS) {
>   			printf("ERROR: Failed to reserve space for FDT\n");
> diff --git a/lib/efi_selftest/efi_selftest_memory.c b/lib/efi_selftest/efi_selftest_memory.c
> index e71732dc6d..4d32a28006 100644
> --- a/lib/efi_selftest/efi_selftest_memory.c
> +++ b/lib/efi_selftest/efi_selftest_memory.c
> @@ -176,9 +176,9 @@ static int execute(void)
>   	/* Check memory reservation for the device tree */
>   	if (fdt_addr &&
>   	    find_in_memory_map(map_size, memory_map, desc_size, fdt_addr,
> -			       EFI_BOOT_SERVICES_DATA) != EFI_ST_SUCCESS) {
> +			       EFI_ACPI_RECLAIM_MEMORY) != EFI_ST_SUCCESS) {
>   		efi_st_error
> -			("Device tree not marked as boot services data\n");
> +			("Device tree not marked as ACPI reclaim memory\n");
>   		return EFI_ST_FAILURE;
>   	}
>   	return EFI_ST_SUCCESS;
> --
> 2.26.2
>
Heinrich Schuchardt May 11, 2020, 11:55 a.m. UTC | #2
On 11.05.20 10:48, Grant Likely wrote:
> On 07/05/2020 19:19, Heinrich Schuchardt wrote:
>> According to the UEFI spec ACPI tables should be placed in
>> EfiACPIReclaimMemory. Let's do the same with the device tree.
>>
>> Suggested-by: Ard Biesheuvel <ardb at kernel.org>
>> Cc: Grant Likely <grant.likely at arm.com>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> ---
>> v2:
>> ????adjust the unit test
>
> Is there any impact to changing the memory type for current users? Does
> the kernel currently expect the EFI_BOOT_SERVICES_DATA memory type? What
> happens if Grub or another EFI application replaces the DTB table? Will
> it try to use a different memory type, and will that matter?

If we are using GRUB, linux_boot() allocates EFI_LOADER_DATA memory and
copies the FDT to it. When the devicetree command is used GRUB also
allocates EFI_LOADER_DATA.

So changes in U-Boot have no effect on what Linux sees if booted via GRUB.

Both EFI_BOOT_SERVICES_DATA and EFI_LOADER_DATA are meant to be gone
past ExitBootServices(). EfiACPIReclaimMemory is were the UEFI payload
is adviced to consider if keeping the memory at runtime is needed.

The current patch therefore makes no difference to Linux before
ExitBootServices(). A side effect could be that Linux unnecessarily
keeps the EfiACPIReclaimMemory instead of reusing it. But Linux's
is_usable_memory() in drivers/firmware/efi/arm-init.c treats
EFI_ACPI_RECLAIM_MEMORY, EFI_ACPI_RECLAIM_MEMORY,
EFI_ACPI_RECLAIM_MEMORY just the same.

@Daniel, Leif
A patch for the EBBR specification suggests to use
EFI_ACPI_RECLAIM_MEMORY for the devicetree passed to Linux. Please,
consider if you would apply this in GRUB too.

Best regards

Heinrich

>
> g.
>
>> ---
>> ? cmd/bootefi.c????????????????????????? | 4 ++--
>> ? lib/efi_selftest/efi_selftest_memory.c | 4 ++--
>> ? 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 54b4b8f984..06573b14e9 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -127,13 +127,13 @@ static efi_status_t copy_fdt(void **fdtp)
>> ????? new_fdt_addr = (uintptr_t)map_sysmem(fdt_ram_start + 0x7f00000 +
>> ?????????????????????????? fdt_size, 0);
>> ????? ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
>> -???????????????? EFI_BOOT_SERVICES_DATA, fdt_pages,
>> +???????????????? EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
>> ?????????????????? &new_fdt_addr);
>> ????? if (ret != EFI_SUCCESS) {
>> ????????? /* If we can't put it there, put it somewhere */
>> ????????? new_fdt_addr = (ulong)memalign(EFI_PAGE_SIZE, fdt_size);
>> ????????? ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
>> -???????????????????? EFI_BOOT_SERVICES_DATA, fdt_pages,
>> +???????????????????? EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
>> ?????????????????????? &new_fdt_addr);
>> ????????? if (ret != EFI_SUCCESS) {
>> ????????????? printf("ERROR: Failed to reserve space for FDT\n");
>> diff --git a/lib/efi_selftest/efi_selftest_memory.c
>> b/lib/efi_selftest/efi_selftest_memory.c
>> index e71732dc6d..4d32a28006 100644
>> --- a/lib/efi_selftest/efi_selftest_memory.c
>> +++ b/lib/efi_selftest/efi_selftest_memory.c
>> @@ -176,9 +176,9 @@ static int execute(void)
>> ????? /* Check memory reservation for the device tree */
>> ????? if (fdt_addr &&
>> ????????? find_in_memory_map(map_size, memory_map, desc_size, fdt_addr,
>> -?????????????????? EFI_BOOT_SERVICES_DATA) != EFI_ST_SUCCESS) {
>> +?????????????????? EFI_ACPI_RECLAIM_MEMORY) != EFI_ST_SUCCESS) {
>> ????????? efi_st_error
>> -??????????? ("Device tree not marked as boot services data\n");
>> +??????????? ("Device tree not marked as ACPI reclaim memory\n");
>> ????????? return EFI_ST_FAILURE;
>> ????? }
>> ????? return EFI_ST_SUCCESS;
>> --
>> 2.26.2
>>
Grant Likely May 12, 2020, 2:18 p.m. UTC | #3
On 11/05/2020 12:55, Heinrich Schuchardt wrote:
> On 11.05.20 10:48, Grant Likely wrote:
>> On 07/05/2020 19:19, Heinrich Schuchardt wrote:
>>> According to the UEFI spec ACPI tables should be placed in
>>> EfiACPIReclaimMemory. Let's do the same with the device tree.
>>>
>>> Suggested-by: Ard Biesheuvel <ardb at kernel.org>
>>> Cc: Grant Likely <grant.likely at arm.com>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> ---
>>> v2:
>>>  ????adjust the unit test
>>
>> Is there any impact to changing the memory type for current users? Does
>> the kernel currently expect the EFI_BOOT_SERVICES_DATA memory type? What
>> happens if Grub or another EFI application replaces the DTB table? Will
>> it try to use a different memory type, and will that matter?
> 
> If we are using GRUB, linux_boot() allocates EFI_LOADER_DATA memory and
> copies the FDT to it. When the devicetree command is used GRUB also
> allocates EFI_LOADER_DATA.
> 
> So changes in U-Boot have no effect on what Linux sees if booted via GRUB.
> 
> Both EFI_BOOT_SERVICES_DATA and EFI_LOADER_DATA are meant to be gone
> past ExitBootServices(). EfiACPIReclaimMemory is were the UEFI payload
> is adviced to consider if keeping the memory at runtime is needed.
> 
> The current patch therefore makes no difference to Linux before
> ExitBootServices(). A side effect could be that Linux unnecessarily
> keeps the EfiACPIReclaimMemory instead of reusing it. But Linux's
> is_usable_memory() in drivers/firmware/efi/arm-init.c treats
> EFI_ACPI_RECLAIM_MEMORY, EFI_ACPI_RECLAIM_MEMORY,
> EFI_ACPI_RECLAIM_MEMORY just the same.

Okay. Works for me.

g.

> @Daniel, Leif
> A patch for the EBBR specification suggests to use
> EFI_ACPI_RECLAIM_MEMORY for the devicetree passed to Linux. Please,
> consider if you would apply this in GRUB too.
> 
> Best regards
> 
> Heinrich
> 
>>
>> g.
>>
>>> ---
>>>  ? cmd/bootefi.c????????????????????????? | 4 ++--
>>>  ? lib/efi_selftest/efi_selftest_memory.c | 4 ++--
>>>  ? 2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 54b4b8f984..06573b14e9 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -127,13 +127,13 @@ static efi_status_t copy_fdt(void **fdtp)
>>>  ????? new_fdt_addr = (uintptr_t)map_sysmem(fdt_ram_start + 0x7f00000 +
>>>  ?????????????????????????? fdt_size, 0);
>>>  ????? ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
>>> -???????????????? EFI_BOOT_SERVICES_DATA, fdt_pages,
>>> +???????????????? EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
>>>  ?????????????????? &new_fdt_addr);
>>>  ????? if (ret != EFI_SUCCESS) {
>>>  ????????? /* If we can't put it there, put it somewhere */
>>>  ????????? new_fdt_addr = (ulong)memalign(EFI_PAGE_SIZE, fdt_size);
>>>  ????????? ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
>>> -???????????????????? EFI_BOOT_SERVICES_DATA, fdt_pages,
>>> +???????????????????? EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
>>>  ?????????????????????? &new_fdt_addr);
>>>  ????????? if (ret != EFI_SUCCESS) {
>>>  ????????????? printf("ERROR: Failed to reserve space for FDT\n");
>>> diff --git a/lib/efi_selftest/efi_selftest_memory.c
>>> b/lib/efi_selftest/efi_selftest_memory.c
>>> index e71732dc6d..4d32a28006 100644
>>> --- a/lib/efi_selftest/efi_selftest_memory.c
>>> +++ b/lib/efi_selftest/efi_selftest_memory.c
>>> @@ -176,9 +176,9 @@ static int execute(void)
>>>  ????? /* Check memory reservation for the device tree */
>>>  ????? if (fdt_addr &&
>>>  ????????? find_in_memory_map(map_size, memory_map, desc_size, fdt_addr,
>>> -?????????????????? EFI_BOOT_SERVICES_DATA) != EFI_ST_SUCCESS) {
>>> +?????????????????? EFI_ACPI_RECLAIM_MEMORY) != EFI_ST_SUCCESS) {
>>>  ????????? efi_st_error
>>> -??????????? ("Device tree not marked as boot services data\n");
>>> +??????????? ("Device tree not marked as ACPI reclaim memory\n");
>>>  ????????? return EFI_ST_FAILURE;
>>>  ????? }
>>>  ????? return EFI_ST_SUCCESS;
>>> --
>>> 2.26.2
>>>
>
diff mbox series

Patch

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 54b4b8f984..06573b14e9 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -127,13 +127,13 @@  static efi_status_t copy_fdt(void **fdtp)
 	new_fdt_addr = (uintptr_t)map_sysmem(fdt_ram_start + 0x7f00000 +
 					     fdt_size, 0);
 	ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
-				 EFI_BOOT_SERVICES_DATA, fdt_pages,
+				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
 				 &new_fdt_addr);
 	if (ret != EFI_SUCCESS) {
 		/* If we can't put it there, put it somewhere */
 		new_fdt_addr = (ulong)memalign(EFI_PAGE_SIZE, fdt_size);
 		ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
-					 EFI_BOOT_SERVICES_DATA, fdt_pages,
+					 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
 					 &new_fdt_addr);
 		if (ret != EFI_SUCCESS) {
 			printf("ERROR: Failed to reserve space for FDT\n");
diff --git a/lib/efi_selftest/efi_selftest_memory.c b/lib/efi_selftest/efi_selftest_memory.c
index e71732dc6d..4d32a28006 100644
--- a/lib/efi_selftest/efi_selftest_memory.c
+++ b/lib/efi_selftest/efi_selftest_memory.c
@@ -176,9 +176,9 @@  static int execute(void)
 	/* Check memory reservation for the device tree */
 	if (fdt_addr &&
 	    find_in_memory_map(map_size, memory_map, desc_size, fdt_addr,
-			       EFI_BOOT_SERVICES_DATA) != EFI_ST_SUCCESS) {
+			       EFI_ACPI_RECLAIM_MEMORY) != EFI_ST_SUCCESS) {
 		efi_st_error
-			("Device tree not marked as boot services data\n");
+			("Device tree not marked as ACPI reclaim memory\n");
 		return EFI_ST_FAILURE;
 	}
 	return EFI_ST_SUCCESS;