[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Ilias Apalodimas]

Following Ard's suggestion:
Runtime data sections are intended for data that is used by the runtime
services implementations.
Let's change they type to EFI_ACPI_RECLAIM_MEMORY

Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 cmd/bootefi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3619a20e6433..b54181909aff 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -111,13 +111,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_RUNTIME_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_RUNTIME_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");
-- 
2.7.4

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Ard Biesheuvel]

On Thu, 11 Apr 2019 at 11:39, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Following Ard's suggestion:
> Runtime data sections are intended for data that is used by the runtime
> services implementations.
> Let's change they type to EFI_ACPI_RECLAIM_MEMORY
>
> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

It makes sense to mention that this actually fixes the boot issue you
were seeing with the ARM Linux kernel when using short descriptors.


> ---
>  cmd/bootefi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 3619a20e6433..b54181909aff 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -111,13 +111,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_RUNTIME_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_RUNTIME_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");
> --
> 2.7.4
>

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Ilias Apalodimas]

Hi Ard,
> On Thu, 11 Apr 2019 at 11:39, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Following Ard's suggestion:
> > Runtime data sections are intended for data that is used by the runtime
> > services implementations.
> > Let's change they type to EFI_ACPI_RECLAIM_MEMORY
> >
> > Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> 
> It makes sense to mention that this actually fixes the boot issue you
> were seeing with the ARM Linux kernel when using short descriptors.
> 
I mentioned that in the original mail thread as you were typing this mail.
Since i still think there *is* a kernel issue that should be fixed this just
'avoids' the problem. Not sure if it makes too much sense to include it here

Thanks
/Ilias

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Heinrich Schuchardt]

On 4/11/19 8:39 PM, Ilias Apalodimas wrote:
> Following Ard's suggestion:
> Runtime data sections are intended for data that is used by the runtime
> services implementations.
> Let's change they type to EFI_ACPI_RECLAIM_MEMORY
>
> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   cmd/bootefi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 3619a20e6433..b54181909aff 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -111,13 +111,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_RUNTIME_SERVICES_DATA, fdt_pages,
> +				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,

GRUB uses EfiLoaderCode when installing its modified version of the FDT.

The "Embedded Base Boot Requirements (EBBR) Specification, Release v1.0"
does not require ACPI support. Can we expect EfiACPIReclaimMemory to be
supported if we do not have any ACPI table?

How about functions efi_smbios_register() and efi_acpi_register()?

How about systab.tables assigned in efi_initialize_system_table()? Which
of the addresses in systab.tables should be updated upon relocation.

The EFI Spec is really hazy: "A pointer to the table associated with
VendorGuid. Whether this pointer is a physical address or a
virtual address during runtime is determined by the VendorGuid."

The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
in the UEFI spec. So we can marvel about expected behavior. Is there any
other document specifying it?

Best regards

Heinrich

>   				 &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_RUNTIME_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");
>

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Ilias Apalodimas]

Hi Heinrich,
> On 4/11/19 8:39 PM, Ilias Apalodimas wrote:
> >Following Ard's suggestion:
> >Runtime data sections are intended for data that is used by the runtime
> >services implementations.
> >Let's change they type to EFI_ACPI_RECLAIM_MEMORY
> >
> >Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >---
> >  cmd/bootefi.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index 3619a20e6433..b54181909aff 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -111,13 +111,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_RUNTIME_SERVICES_DATA, fdt_pages,
> >+				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
> 
> GRUB uses EfiLoaderCode when installing its modified version of the FDT.
> 
> The "Embedded Base Boot Requirements (EBBR) Specification, Release v1.0"
> does not require ACPI support. Can we expect EfiACPIReclaimMemory to be
> supported if we do not have any ACPI table?
> 
> How about functions efi_smbios_register() and efi_acpi_register()?
> 
> How about systab.tables assigned in efi_initialize_system_table()? Which
> of the addresses in systab.tables should be updated upon relocation.
> 
> The EFI Spec is really hazy: "A pointer to the table associated with
> VendorGuid. Whether this pointer is a physical address or a
> virtual address during runtime is determined by the VendorGuid."
> 
> The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
> in the UEFI spec. So we can marvel about expected behavior. Is there any
> other document specifying it?

What about using EFI_BOOT_SERVICES_DATA instead of EFI_ACPI_RECLAIM_MEMORY?
This still fixes the issue and doesn't cause any of the potential problems you
mentioned

Thanks
/Ilias

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Heinrich Schuchardt]

On 4/11/19 9:41 PM, Ilias Apalodimas wrote:
> Hi Heinrich,
>> On 4/11/19 8:39 PM, Ilias Apalodimas wrote:
>>> Following Ard's suggestion:
>>> Runtime data sections are intended for data that is used by the runtime
>>> services implementations.
>>> Let's change they type to EFI_ACPI_RECLAIM_MEMORY
>>>
>>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> ---
>>>   cmd/bootefi.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 3619a20e6433..b54181909aff 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -111,13 +111,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_RUNTIME_SERVICES_DATA, fdt_pages,
>>> +				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
>>
>> GRUB uses EfiLoaderCode when installing its modified version of the FDT.
>>
>> The "Embedded Base Boot Requirements (EBBR) Specification, Release v1.0"
>> does not require ACPI support. Can we expect EfiACPIReclaimMemory to be
>> supported if we do not have any ACPI table?
>>
>> How about functions efi_smbios_register() and efi_acpi_register()?
>>
>> How about systab.tables assigned in efi_initialize_system_table()? Which
>> of the addresses in systab.tables should be updated upon relocation.
>>
>> The EFI Spec is really hazy: "A pointer to the table associated with
>> VendorGuid. Whether this pointer is a physical address or a
>> virtual address during runtime is determined by the VendorGuid."
>>
>> The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
>> in the UEFI spec. So we can marvel about expected behavior. Is there any
>> other document specifying it?
>
> What about using EFI_BOOT_SERVICES_DATA instead of EFI_ACPI_RECLAIM_MEMORY?
> This still fixes the issue and doesn't cause any of the potential problems you
> mentioned

Why services data and not loader data?

As said above we should consider all three supported tables ACPI,
SMBIOS, and FDT when thinking about a fix. The UEFI spec describes that
the addresses inside ACPI and SMBIOS tables are not fixed up when
entering runtime.

This indicates that at least these tables have to be accessible at
runtime. Why do you think that the FDT table should be treated
differently to the ACPI table?

Best regards

Heinrich

>
> Thanks
> /Ilias
>

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Ard Biesheuvel]

On Thu, 11 Apr 2019 at 12:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/11/19 9:41 PM, Ilias Apalodimas wrote:
> > Hi Heinrich,
> >> On 4/11/19 8:39 PM, Ilias Apalodimas wrote:
> >>> Following Ard's suggestion:
> >>> Runtime data sections are intended for data that is used by the runtime
> >>> services implementations.
> >>> Let's change they type to EFI_ACPI_RECLAIM_MEMORY
> >>>
> >>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>> ---
> >>>   cmd/bootefi.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index 3619a20e6433..b54181909aff 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -111,13 +111,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_RUNTIME_SERVICES_DATA, fdt_pages,
> >>> +                            EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
> >>
> >> GRUB uses EfiLoaderCode when installing its modified version of the FDT.
> >>
> >> The "Embedded Base Boot Requirements (EBBR) Specification, Release v1.0"
> >> does not require ACPI support. Can we expect EfiACPIReclaimMemory to be
> >> supported if we do not have any ACPI table?
> >>
> >> How about functions efi_smbios_register() and efi_acpi_register()?
> >>
> >> How about systab.tables assigned in efi_initialize_system_table()? Which
> >> of the addresses in systab.tables should be updated upon relocation.
> >>
> >> The EFI Spec is really hazy: "A pointer to the table associated with
> >> VendorGuid. Whether this pointer is a physical address or a
> >> virtual address during runtime is determined by the VendorGuid."
> >>
> >> The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
> >> in the UEFI spec. So we can marvel about expected behavior. Is there any
> >> other document specifying it?
> >
> > What about using EFI_BOOT_SERVICES_DATA instead of EFI_ACPI_RECLAIM_MEMORY?
> > This still fixes the issue and doesn't cause any of the potential problems you
> > mentioned
>
> Why services data and not loader data?
>

Services data is used by the boot services and loader data by the
loader. GRUB is a loader so it uses loader data, u-boot is both boot
services and a loader so it could arguably use both, but boot services
data is more idiomatic.

From the pov of the OS, it makes no difference at all.

> As said above we should consider all three supported tables ACPI,
> SMBIOS, and FDT when thinking about a fix. The UEFI spec describes that
> the addresses inside ACPI and SMBIOS tables are not fixed up when
> entering runtime.
>
> This indicates that at least these tables have to be accessible at
> runtime.

Accessible at runtime but *not* by the runtime services themselves.

> Why do you think that the FDT table should be treated> differently to the ACPI table?
>

Same thing. Accessible at runtime but not by the firmware services themselves.

Only data structures that the runtime services need to access in order
to implement the functionality they expose to the OS should be in
runtime services data memory. This applies to DT, ACPI and SMBIOS
tables alike, but as I mentioned, for historical reasons, SMBIOS
tables are usually exposed via runtime services data. Interestingly,
the UEFI spec mentions that smbios tables should be located in runtime
services data but no virtual mapping should be requested for them, and
this is actually impossible in EDK2, so the current practice of using
rtservicesdata for SMBIOS with the EFI_MEMORY_RUNTIME attribute set
also violates the UEFI spec.

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Heinrich Schuchardt]

On 4/11/19 10:50 PM, Ard Biesheuvel wrote:
> On Thu, 11 Apr 2019 at 12:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 4/11/19 9:41 PM, Ilias Apalodimas wrote:
>>> Hi Heinrich,
>>>> On 4/11/19 8:39 PM, Ilias Apalodimas wrote:
>>>>> Following Ard's suggestion:
>>>>> Runtime data sections are intended for data that is used by the runtime
>>>>> services implementations.
>>>>> Let's change they type to EFI_ACPI_RECLAIM_MEMORY
>>>>>
>>>>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>>> ---
>>>>>    cmd/bootefi.c | 4 ++--
>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>>> index 3619a20e6433..b54181909aff 100644
>>>>> --- a/cmd/bootefi.c
>>>>> +++ b/cmd/bootefi.c
>>>>> @@ -111,13 +111,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_RUNTIME_SERVICES_DATA, fdt_pages,
>>>>> +                            EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
>>>>
>>>> GRUB uses EfiLoaderCode when installing its modified version of the FDT.
>>>>
>>>> The "Embedded Base Boot Requirements (EBBR) Specification, Release v1.0"
>>>> does not require ACPI support. Can we expect EfiACPIReclaimMemory to be
>>>> supported if we do not have any ACPI table?
>>>>
>>>> How about functions efi_smbios_register() and efi_acpi_register()?
>>>>
>>>> How about systab.tables assigned in efi_initialize_system_table()? Which
>>>> of the addresses in systab.tables should be updated upon relocation.
>>>>
>>>> The EFI Spec is really hazy: "A pointer to the table associated with
>>>> VendorGuid. Whether this pointer is a physical address or a
>>>> virtual address during runtime is determined by the VendorGuid."
>>>>
>>>> The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
>>>> in the UEFI spec. So we can marvel about expected behavior. Is there any
>>>> other document specifying it?
>>>
>>> What about using EFI_BOOT_SERVICES_DATA instead of EFI_ACPI_RECLAIM_MEMORY?
>>> This still fixes the issue and doesn't cause any of the potential problems you
>>> mentioned
>>
>> Why services data and not loader data?
>>
>
> Services data is used by the boot services and loader data by the
> loader. GRUB is a loader so it uses loader data, u-boot is both boot
> services and a loader so it could arguably use both, but boot services
> data is more idiomatic.
>
>>From the pov of the OS, it makes no difference at all.
>
>> As said above we should consider all three supported tables ACPI,
>> SMBIOS, and FDT when thinking about a fix. The UEFI spec describes that
>> the addresses inside ACPI and SMBIOS tables are not fixed up when
>> entering runtime.
>>
>> This indicates that at least these tables have to be accessible at
>> runtime.
>
> Accessible at runtime but *not* by the runtime services themselves.
>
>> Why do you think that the FDT table should be treated> differently to the ACPI table?
>>
>
> Same thing. Accessible at runtime but not by the firmware services themselves.
>
> Only data structures that the runtime services need to access in order
> to implement the functionality they expose to the OS should be in
> runtime services data memory. This applies to DT, ACPI and SMBIOS
> tables alike, but as I mentioned, for historical reasons, SMBIOS
> tables are usually exposed via runtime services data. Interestingly,
> the UEFI spec mentions that smbios tables should be located in runtime
> services data but no virtual mapping should be requested for them, and
> this is actually impossible in EDK2, so the current practice of using
> rtservicesdata for SMBIOS with the EFI_MEMORY_RUNTIME attribute set
> also violates the UEFI spec.
>

Hello Ilias, hello Ard,

please, have a look at this patch:

efi_loader: update virtual address in efi_mem_carve_out
https://lists.denx.de/pipermail/u-boot/2019-April/364937.html

Possibly the bug reported here could have contributed the Linux crash
you experienced.

Best regards

Heinrich

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Ard Biesheuvel]

On Fri, 12 Apr 2019 at 10:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/11/19 10:50 PM, Ard Biesheuvel wrote:
> > On Thu, 11 Apr 2019 at 12:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 4/11/19 9:41 PM, Ilias Apalodimas wrote:
> >>> Hi Heinrich,
> >>>> On 4/11/19 8:39 PM, Ilias Apalodimas wrote:
> >>>>> Following Ard's suggestion:
> >>>>> Runtime data sections are intended for data that is used by the runtime
> >>>>> services implementations.
> >>>>> Let's change they type to EFI_ACPI_RECLAIM_MEMORY
> >>>>>
> >>>>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>>>> ---
> >>>>>    cmd/bootefi.c | 4 ++--
> >>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>>>> index 3619a20e6433..b54181909aff 100644
> >>>>> --- a/cmd/bootefi.c
> >>>>> +++ b/cmd/bootefi.c
> >>>>> @@ -111,13 +111,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_RUNTIME_SERVICES_DATA, fdt_pages,
> >>>>> +                            EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
> >>>>
> >>>> GRUB uses EfiLoaderCode when installing its modified version of the FDT.
> >>>>
> >>>> The "Embedded Base Boot Requirements (EBBR) Specification, Release v1.0"
> >>>> does not require ACPI support. Can we expect EfiACPIReclaimMemory to be
> >>>> supported if we do not have any ACPI table?
> >>>>
> >>>> How about functions efi_smbios_register() and efi_acpi_register()?
> >>>>
> >>>> How about systab.tables assigned in efi_initialize_system_table()? Which
> >>>> of the addresses in systab.tables should be updated upon relocation.
> >>>>
> >>>> The EFI Spec is really hazy: "A pointer to the table associated with
> >>>> VendorGuid. Whether this pointer is a physical address or a
> >>>> virtual address during runtime is determined by the VendorGuid."
> >>>>
> >>>> The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
> >>>> in the UEFI spec. So we can marvel about expected behavior. Is there any
> >>>> other document specifying it?
> >>>
> >>> What about using EFI_BOOT_SERVICES_DATA instead of EFI_ACPI_RECLAIM_MEMORY?
> >>> This still fixes the issue and doesn't cause any of the potential problems you
> >>> mentioned
> >>
> >> Why services data and not loader data?
> >>
> >
> > Services data is used by the boot services and loader data by the
> > loader. GRUB is a loader so it uses loader data, u-boot is both boot
> > services and a loader so it could arguably use both, but boot services
> > data is more idiomatic.
> >
> >>From the pov of the OS, it makes no difference at all.
> >
> >> As said above we should consider all three supported tables ACPI,
> >> SMBIOS, and FDT when thinking about a fix. The UEFI spec describes that
> >> the addresses inside ACPI and SMBIOS tables are not fixed up when
> >> entering runtime.
> >>
> >> This indicates that at least these tables have to be accessible at
> >> runtime.
> >
> > Accessible at runtime but *not* by the runtime services themselves.
> >
> >> Why do you think that the FDT table should be treated> differently to the ACPI table?
> >>
> >
> > Same thing. Accessible at runtime but not by the firmware services themselves.
> >
> > Only data structures that the runtime services need to access in order
> > to implement the functionality they expose to the OS should be in
> > runtime services data memory. This applies to DT, ACPI and SMBIOS
> > tables alike, but as I mentioned, for historical reasons, SMBIOS
> > tables are usually exposed via runtime services data. Interestingly,
> > the UEFI spec mentions that smbios tables should be located in runtime
> > services data but no virtual mapping should be requested for them, and
> > this is actually impossible in EDK2, so the current practice of using
> > rtservicesdata for SMBIOS with the EFI_MEMORY_RUNTIME attribute set
> > also violates the UEFI spec.
> >
>
> Hello Ilias, hello Ard,
>
> please, have a look at this patch:
>
> efi_loader: update virtual address in efi_mem_carve_out
> https://lists.denx.de/pipermail/u-boot/2019-April/364937.html
>
> Possibly the bug reported here could have contributed the Linux crash
> you experienced.
>

Hello Heinrich,

That patch looks completely unrelated, to be honest.

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Heinrich Schuchardt]

On 4/12/19 7:24 PM, Ard Biesheuvel wrote:
> On Fri, 12 Apr 2019 at 10:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 4/11/19 10:50 PM, Ard Biesheuvel wrote:
>>> On Thu, 11 Apr 2019 at 12:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 4/11/19 9:41 PM, Ilias Apalodimas wrote:
>>>>> Hi Heinrich,
>>>>>> On 4/11/19 8:39 PM, Ilias Apalodimas wrote:
>>>>>>> Following Ard's suggestion:
>>>>>>> Runtime data sections are intended for data that is used by the runtime
>>>>>>> services implementations.
>>>>>>> Let's change they type to EFI_ACPI_RECLAIM_MEMORY
>>>>>>>
>>>>>>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>>>>> ---
>>>>>>>     cmd/bootefi.c | 4 ++--
>>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>>>>> index 3619a20e6433..b54181909aff 100644
>>>>>>> --- a/cmd/bootefi.c
>>>>>>> +++ b/cmd/bootefi.c
>>>>>>> @@ -111,13 +111,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_RUNTIME_SERVICES_DATA, fdt_pages,
>>>>>>> +                            EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
>>>>>>
>>>>>> GRUB uses EfiLoaderCode when installing its modified version of the FDT.
>>>>>>
>>>>>> The "Embedded Base Boot Requirements (EBBR) Specification, Release v1.0"
>>>>>> does not require ACPI support. Can we expect EfiACPIReclaimMemory to be
>>>>>> supported if we do not have any ACPI table?
>>>>>>
>>>>>> How about functions efi_smbios_register() and efi_acpi_register()?
>>>>>>
>>>>>> How about systab.tables assigned in efi_initialize_system_table()? Which
>>>>>> of the addresses in systab.tables should be updated upon relocation.
>>>>>>
>>>>>> The EFI Spec is really hazy: "A pointer to the table associated with
>>>>>> VendorGuid. Whether this pointer is a physical address or a
>>>>>> virtual address during runtime is determined by the VendorGuid."
>>>>>>
>>>>>> The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
>>>>>> in the UEFI spec. So we can marvel about expected behavior. Is there any
>>>>>> other document specifying it?
>>>>>
>>>>> What about using EFI_BOOT_SERVICES_DATA instead of EFI_ACPI_RECLAIM_MEMORY?
>>>>> This still fixes the issue and doesn't cause any of the potential problems you
>>>>> mentioned
>>>>
>>>> Why services data and not loader data?
>>>>
>>>
>>> Services data is used by the boot services and loader data by the
>>> loader. GRUB is a loader so it uses loader data, u-boot is both boot
>>> services and a loader so it could arguably use both, but boot services
>>> data is more idiomatic.
>>>
>>> >From the pov of the OS, it makes no difference at all.
>>>
>>>> As said above we should consider all three supported tables ACPI,
>>>> SMBIOS, and FDT when thinking about a fix. The UEFI spec describes that
>>>> the addresses inside ACPI and SMBIOS tables are not fixed up when
>>>> entering runtime.
>>>>
>>>> This indicates that at least these tables have to be accessible at
>>>> runtime.
>>>
>>> Accessible at runtime but *not* by the runtime services themselves.
>>>
>>>> Why do you think that the FDT table should be treated> differently to the ACPI table?
>>>>
>>>
>>> Same thing. Accessible at runtime but not by the firmware services themselves.
>>>
>>> Only data structures that the runtime services need to access in order
>>> to implement the functionality they expose to the OS should be in
>>> runtime services data memory. This applies to DT, ACPI and SMBIOS
>>> tables alike, but as I mentioned, for historical reasons, SMBIOS
>>> tables are usually exposed via runtime services data. Interestingly,
>>> the UEFI spec mentions that smbios tables should be located in runtime
>>> services data but no virtual mapping should be requested for them, and
>>> this is actually impossible in EDK2, so the current practice of using
>>> rtservicesdata for SMBIOS with the EFI_MEMORY_RUNTIME attribute set
>>> also violates the UEFI spec.
>>>
>>
>> Hello Ilias, hello Ard,
>>
>> please, have a look at this patch:
>>
>> efi_loader: update virtual address in efi_mem_carve_out
>> https://lists.denx.de/pipermail/u-boot/2019-April/364937.html
>>
>> Possibly the bug reported here could have contributed the Linux crash
>> you experienced.
>>
>
> Hello Heinrich,
>
> That patch looks completely unrelated, to be honest.
>

I wondered if virtual address != physical address could be the cause why
the Linux memory management is incorrectly initialized.

---

GRUB uses EfiLoaderCode when installing its modified version of the FDT.

In EDK II
EmbeddedPkg/Library/DxeDtPlatformDtbLoaderLibDefault/DxeDtPlatformDtbLoaderLibDefault.c
function DtPlatformLoadDtb() calls function AllocateCopyPool().

In MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c function
AllocateCopyPool() uses EfiBootServicesData for the device tree

So I think using EfiBootServicesData in U-Boot should be safe.

Please, update the patch accordingly.

Best regards

Heinrich

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Ard Biesheuvel]

On Fri, 12 Apr 2019 at 10:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/12/19 7:24 PM, Ard Biesheuvel wrote:
> > On Fri, 12 Apr 2019 at 10:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 4/11/19 10:50 PM, Ard Biesheuvel wrote:
> >>> On Thu, 11 Apr 2019 at 12:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>
> >>>> On 4/11/19 9:41 PM, Ilias Apalodimas wrote:
> >>>>> Hi Heinrich,
> >>>>>> On 4/11/19 8:39 PM, Ilias Apalodimas wrote:
> >>>>>>> Following Ard's suggestion:
> >>>>>>> Runtime data sections are intended for data that is used by the runtime
> >>>>>>> services implementations.
> >>>>>>> Let's change they type to EFI_ACPI_RECLAIM_MEMORY
> >>>>>>>
> >>>>>>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>>>>>> ---
> >>>>>>>     cmd/bootefi.c | 4 ++--
> >>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>>>>>> index 3619a20e6433..b54181909aff 100644
> >>>>>>> --- a/cmd/bootefi.c
> >>>>>>> +++ b/cmd/bootefi.c
> >>>>>>> @@ -111,13 +111,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_RUNTIME_SERVICES_DATA, fdt_pages,
> >>>>>>> +                            EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
> >>>>>>
> >>>>>> GRUB uses EfiLoaderCode when installing its modified version of the FDT.
> >>>>>>
> >>>>>> The "Embedded Base Boot Requirements (EBBR) Specification, Release v1.0"
> >>>>>> does not require ACPI support. Can we expect EfiACPIReclaimMemory to be
> >>>>>> supported if we do not have any ACPI table?
> >>>>>>
> >>>>>> How about functions efi_smbios_register() and efi_acpi_register()?
> >>>>>>
> >>>>>> How about systab.tables assigned in efi_initialize_system_table()? Which
> >>>>>> of the addresses in systab.tables should be updated upon relocation.
> >>>>>>
> >>>>>> The EFI Spec is really hazy: "A pointer to the table associated with
> >>>>>> VendorGuid. Whether this pointer is a physical address or a
> >>>>>> virtual address during runtime is determined by the VendorGuid."
> >>>>>>
> >>>>>> The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
> >>>>>> in the UEFI spec. So we can marvel about expected behavior. Is there any
> >>>>>> other document specifying it?
> >>>>>
> >>>>> What about using EFI_BOOT_SERVICES_DATA instead of EFI_ACPI_RECLAIM_MEMORY?
> >>>>> This still fixes the issue and doesn't cause any of the potential problems you
> >>>>> mentioned
> >>>>
> >>>> Why services data and not loader data?
> >>>>
> >>>
> >>> Services data is used by the boot services and loader data by the
> >>> loader. GRUB is a loader so it uses loader data, u-boot is both boot
> >>> services and a loader so it could arguably use both, but boot services
> >>> data is more idiomatic.
> >>>
> >>> >From the pov of the OS, it makes no difference at all.
> >>>
> >>>> As said above we should consider all three supported tables ACPI,
> >>>> SMBIOS, and FDT when thinking about a fix. The UEFI spec describes that
> >>>> the addresses inside ACPI and SMBIOS tables are not fixed up when
> >>>> entering runtime.
> >>>>
> >>>> This indicates that at least these tables have to be accessible at
> >>>> runtime.
> >>>
> >>> Accessible at runtime but *not* by the runtime services themselves.
> >>>
> >>>> Why do you think that the FDT table should be treated> differently to the ACPI table?
> >>>>
> >>>
> >>> Same thing. Accessible at runtime but not by the firmware services themselves.
> >>>
> >>> Only data structures that the runtime services need to access in order
> >>> to implement the functionality they expose to the OS should be in
> >>> runtime services data memory. This applies to DT, ACPI and SMBIOS
> >>> tables alike, but as I mentioned, for historical reasons, SMBIOS
> >>> tables are usually exposed via runtime services data. Interestingly,
> >>> the UEFI spec mentions that smbios tables should be located in runtime
> >>> services data but no virtual mapping should be requested for them, and
> >>> this is actually impossible in EDK2, so the current practice of using
> >>> rtservicesdata for SMBIOS with the EFI_MEMORY_RUNTIME attribute set
> >>> also violates the UEFI spec.
> >>>
> >>
> >> Hello Ilias, hello Ard,
> >>
> >> please, have a look at this patch:
> >>
> >> efi_loader: update virtual address in efi_mem_carve_out
> >> https://lists.denx.de/pipermail/u-boot/2019-April/364937.html
> >>
> >> Possibly the bug reported here could have contributed the Linux crash
> >> you experienced.
> >>
> >
> > Hello Heinrich,
> >
> > That patch looks completely unrelated, to be honest.
> >
>
> I wondered if virtual address != physical address could be the cause why
> the Linux memory management is incorrectly initialized.
>

The virtual address field is completely ignored by the early linux
code, except for the case when it sets it itself, when passing back
the memory map to SetVirtualAddressMap().

The only point where we inspect the virtual address field is when
creating the runtime mapping based on the VA layout created by the
stub (and passed into SetVirtualAddressMap()).

> ---
>
> GRUB uses EfiLoaderCode when installing its modified version of the FDT.
>
> In EDK II
> EmbeddedPkg/Library/DxeDtPlatformDtbLoaderLibDefault/DxeDtPlatformDtbLoaderLibDefault.c
> function DtPlatformLoadDtb() calls function AllocateCopyPool().
>
> In MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c function
> AllocateCopyPool() uses EfiBootServicesData for the device tree
>
> So I think using EfiBootServicesData in U-Boot should be safe.
>
> Please, update the patch accordingly.
>

Either ACPI reclaim or BS data works for me

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Ilias Apalodimas]

Hi Heinrich,

> 
> Hello Ilias, hello Ard,
> 
> please, have a look at this patch:
> 
> efi_loader: update virtual address in efi_mem_carve_out
> https://lists.denx.de/pipermail/u-boot/2019-April/364937.html
> 
> Possibly the bug reported here could have contributed the Linux crash
> you experienced.
> 
Thanks for the heads up.
Unfortunately i've already tried that. I was talking to Patrick before he posted
the patch upstream. This seems unrelated anyway (all my tests were with the
patch applied regardless)
https://lore.kernel.org/linux-arm-kernel/20190411151320.GA23031 at apalos/
This has an explanation on the problem. 

The tl;dr version (quoting Russell)

"It is also designed to allow hardware-section sized mappings (making
it possible to map sections on 1MB granularity) but as a single Linux
page table always occupies 2MB, it is not permitted for the unused
half of an aligned 2MB slot to be used for a page table mapping -
hence this BUG_ON().
The ARM early mapping routines are intentionally designed such that
areas of memory that they are asked to map are non-overlapping - it
is the caller's responsibility to ensure this."

In order to make sure this won't trigger we got to make sure that the fdt is
placed on the first 1mb boundary of the memory (of any 2mb aligned area)
thus forcing the kernel to init the pte's correctly, 
instead of trying to do section mappings for the memory in that area.

The problem happens when you have a small no-map section within a 2MB
region, and it does cross the even-odd 1MB boundary.
So fdt at 0x7f00000 (or any other are after that like 0xc7f01000) will crash 
the kernel with a BUG_ON().
Placing it in 0x7e01000-7eFF000 would be fine (on armv7's with LPAE off in the
kernel)

Thanks
/Ilias

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Heinrich Schuchardt]

On 4/12/19 7:36 PM, Ilias Apalodimas wrote:
> Hi Heinrich,
>
>>
>> Hello Ilias, hello Ard,
>>
>> please, have a look at this patch:
>>
>> efi_loader: update virtual address in efi_mem_carve_out
>> https://lists.denx.de/pipermail/u-boot/2019-April/364937.html
>>
>> Possibly the bug reported here could have contributed the Linux crash
>> you experienced.
>>
> Thanks for the heads up.
> Unfortunately i've already tried that. I was talking to Patrick before he posted
> the patch upstream. This seems unrelated anyway (all my tests were with the
> patch applied regardless)
> https://lore.kernel.org/linux-arm-kernel/20190411151320.GA23031 at apalos/
> This has an explanation on the problem.
>
> The tl;dr version (quoting Russell)
>
> "It is also designed to allow hardware-section sized mappings (making
> it possible to map sections on 1MB granularity) but as a single Linux
> page table always occupies 2MB, it is not permitted for the unused
> half of an aligned 2MB slot to be used for a page table mapping -
> hence this BUG_ON().
> The ARM early mapping routines are intentionally designed such that
> areas of memory that they are asked to map are non-overlapping - it
> is the caller's responsibility to ensure this."
>
> In order to make sure this won't trigger we got to make sure that the fdt is
> placed on the first 1mb boundary of the memory (of any 2mb aligned area)
> thus forcing the kernel to init the pte's correctly,
> instead of trying to do section mappings for the memory in that area.
>
> The problem happens when you have a small no-map section within a 2MB
> region, and it does cross the even-odd 1MB boundary.
> So fdt at 0x7f00000 (or any other are after that like 0xc7f01000) will crash
> the kernel with a BUG_ON().
> Placing it in 0x7e01000-7eFF000 would be fine (on armv7's with LPAE off in the
> kernel)
>

I think Linux cannot make any assumptions about UEFI memory layout if it
is not explicitly specified in the UEFI spec. Everything is simply a
Linux bug.

Concerning FDT I suggest to stick to what EDK II does: use
EfiBootServicesData.

Best regards

Heinrich

> Thanks
> /Ilias
>

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Ard Biesheuvel]

(+ Leif)

On Thu, 11 Apr 2019 at 12:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/11/19 8:39 PM, Ilias Apalodimas wrote:
> > Following Ard's suggestion:
> > Runtime data sections are intended for data that is used by the runtime
> > services implementations.
> > Let's change they type to EFI_ACPI_RECLAIM_MEMORY
> >
> > Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   cmd/bootefi.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 3619a20e6433..b54181909aff 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -111,13 +111,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_RUNTIME_SERVICES_DATA, fdt_pages,
> > +                              EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
>
> GRUB uses EfiLoaderCode when installing its modified version of the FDT.
>
> The "Embedded Base Boot Requirements (EBBR) Specification, Release v1.0"
> does not require ACPI support. Can we expect EfiACPIReclaimMemory to be
> supported if we do not have any ACPI table?
>

In spite of the name, ACPI reclaim memory is fully specified by the
UEFI spec, so whether you actually use ACPI or not is irrelevant. It
is memory that can be treated by the OS as conventional memory if it
doesn't care about preserving the contents.

> How about functions efi_smbios_register() and efi_acpi_register()?
>

SMBIOS uses RuntimeServicesData memory for historical reasons, but
that is basically a mistake.
ACPI tables should use ACPI reclaim memory, and even the allocations
they refer to (such as the bitmap referenced by the BGRT table) should
use ACPI reclaim memory, which is another thing we are trying to get
fixed in the spec.

> How about systab.tables assigned in efi_initialize_system_table()? Which
> of the addresses in systab.tables should be updated upon relocation.
>
> The EFI Spec is really hazy: "A pointer to the table associated with
> VendorGuid. Whether this pointer is a physical address or a
> virtual address during runtime is determined by the VendorGuid."
>

Indeed. So it is up to the publisher to update the reference, but I am
not aware of any firmware implementations that do this in practice. It
is typically assumed that a firmware component that is still active at
runtime holds its own reference to data exposed via a configuration
table, and updates the reference during SetVirtualAddressMap.

There is also a known bug in EDK2 where the ESRT table is passed in
boot services memory, but the capsule update code actually tries to
access it at runtime, so this isn't as clean as we'd like it to be.

> The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
> in the UEFI spec. So we can marvel about expected behavior. Is there any
> other document specifying it?
>

No, its de facto specification is in the EDK2 source tree.

Since there is no reason the firmware itself would need to consume the
device tree at runtime, there is no point in keeping it in
EfiRuntimeServicesData memory or in updating the configuration table
entry.

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Heinrich Schuchardt]

On 4/11/19 9:50 PM, Ard Biesheuvel wrote:
> (+ Leif)
>
> On Thu, 11 Apr 2019 at 12:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 4/11/19 8:39 PM, Ilias Apalodimas wrote:
>>> Following Ard's suggestion:
>>> Runtime data sections are intended for data that is used by the runtime
>>> services implementations.
>>> Let's change they type to EFI_ACPI_RECLAIM_MEMORY
>>>
>>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> ---
>>>    cmd/bootefi.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 3619a20e6433..b54181909aff 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -111,13 +111,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_RUNTIME_SERVICES_DATA, fdt_pages,
>>> +                              EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
>>
>> GRUB uses EfiLoaderCode when installing its modified version of the FDT.
>>
>> The "Embedded Base Boot Requirements (EBBR) Specification, Release v1.0"
>> does not require ACPI support. Can we expect EfiACPIReclaimMemory to be
>> supported if we do not have any ACPI table?
>>
>
> In spite of the name, ACPI reclaim memory is fully specified by the
> UEFI spec, so whether you actually use ACPI or not is irrelevant. It
> is memory that can be treated by the OS as conventional memory if it
> doesn't care about preserving the contents.
>
>> How about functions efi_smbios_register() and efi_acpi_register()?
>>
>
> SMBIOS uses RuntimeServicesData memory for historical reasons, but
> that is basically a mistake.
> ACPI tables should use ACPI reclaim memory, and even the allocations
> they refer to (such as the bitmap referenced by the BGRT table) should
> use ACPI reclaim memory, which is another thing we are trying to get
> fixed in the spec.
>
>> How about systab.tables assigned in efi_initialize_system_table()? Which
>> of the addresses in systab.tables should be updated upon relocation.
>>
>> The EFI Spec is really hazy: "A pointer to the table associated with
>> VendorGuid. Whether this pointer is a physical address or a
>> virtual address during runtime is determined by the VendorGuid."
>>
>
> Indeed. So it is up to the publisher to update the reference, but I am
> not aware of any firmware implementations that do this in practice. It
> is typically assumed that a firmware component that is still active at
> runtime holds its own reference to data exposed via a configuration
> table, and updates the reference during SetVirtualAddressMap.
>
> There is also a known bug in EDK2 where the ESRT table is passed in
> boot services memory, but the capsule update code actually tries to
> access it at runtime, so this isn't as clean as we'd like it to be.
>
>> The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
>> in the UEFI spec. So we can marvel about expected behavior. Is there any
>> other document specifying it?
>>
>
> No, its de facto specification is in the EDK2 source tree.

As all ARM systems use it I guess this GUID should move into the UEFI
spec. Maybe Linaro could raise this issue.

Best regards

Heinrich

>
> Since there is no reason the firmware itself would need to consume the
> device tree at runtime, there is no point in keeping it in
> EfiRuntimeServicesData memory or in updating the configuration table
> entry.
>

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Ard Biesheuvel]

On Thu, 11 Apr 2019 at 13:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/11/19 9:50 PM, Ard Biesheuvel wrote:
> > (+ Leif)
> >
> > On Thu, 11 Apr 2019 at 12:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 4/11/19 8:39 PM, Ilias Apalodimas wrote:
> >>> Following Ard's suggestion:
> >>> Runtime data sections are intended for data that is used by the runtime
> >>> services implementations.
> >>> Let's change they type to EFI_ACPI_RECLAIM_MEMORY
> >>>
> >>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>> ---
> >>>    cmd/bootefi.c | 4 ++--
> >>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index 3619a20e6433..b54181909aff 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -111,13 +111,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_RUNTIME_SERVICES_DATA, fdt_pages,
> >>> +                              EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
> >>
> >> GRUB uses EfiLoaderCode when installing its modified version of the FDT.
> >>
> >> The "Embedded Base Boot Requirements (EBBR) Specification, Release v1.0"
> >> does not require ACPI support. Can we expect EfiACPIReclaimMemory to be
> >> supported if we do not have any ACPI table?
> >>
> >
> > In spite of the name, ACPI reclaim memory is fully specified by the
> > UEFI spec, so whether you actually use ACPI or not is irrelevant. It
> > is memory that can be treated by the OS as conventional memory if it
> > doesn't care about preserving the contents.
> >
> >> How about functions efi_smbios_register() and efi_acpi_register()?
> >>
> >
> > SMBIOS uses RuntimeServicesData memory for historical reasons, but
> > that is basically a mistake.
> > ACPI tables should use ACPI reclaim memory, and even the allocations
> > they refer to (such as the bitmap referenced by the BGRT table) should
> > use ACPI reclaim memory, which is another thing we are trying to get
> > fixed in the spec.
> >
> >> How about systab.tables assigned in efi_initialize_system_table()? Which
> >> of the addresses in systab.tables should be updated upon relocation.
> >>
> >> The EFI Spec is really hazy: "A pointer to the table associated with
> >> VendorGuid. Whether this pointer is a physical address or a
> >> virtual address during runtime is determined by the VendorGuid."
> >>
> >
> > Indeed. So it is up to the publisher to update the reference, but I am
> > not aware of any firmware implementations that do this in practice. It
> > is typically assumed that a firmware component that is still active at
> > runtime holds its own reference to data exposed via a configuration
> > table, and updates the reference during SetVirtualAddressMap.
> >
> > There is also a known bug in EDK2 where the ESRT table is passed in
> > boot services memory, but the capsule update code actually tries to
> > access it at runtime, so this isn't as clean as we'd like it to be.
> >
> >> The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
> >> in the UEFI spec. So we can marvel about expected behavior. Is there any
> >> other document specifying it?
> >>
> >
> > No, its de facto specification is in the EDK2 source tree.
>
> As all ARM systems use it I guess this GUID should move into the UEFI
> spec. Maybe Linaro could raise this issue.
>

The whole purpose of using GUIDs is that they are guaranteed to be
unique, and so they don't all have to be recorded in the same spec in
order to be usable safely in combination. So we could add it to the
UEFI spec, or to some other spec, or not at all, but it is not really
relevant to the discussion.

[U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Leif Lindholm]

On Thu, Apr 11, 2019 at 10:08:10PM +0200, Heinrich Schuchardt wrote:
> > > How about systab.tables assigned in efi_initialize_system_table()? Which
> > > of the addresses in systab.tables should be updated upon relocation.
> > > 
> > > The EFI Spec is really hazy: "A pointer to the table associated with
> > > VendorGuid. Whether this pointer is a physical address or a
> > > virtual address during runtime is determined by the VendorGuid."
> > > 
> > 
> > Indeed. So it is up to the publisher to update the reference, but I am
> > not aware of any firmware implementations that do this in practice. It
> > is typically assumed that a firmware component that is still active at
> > runtime holds its own reference to data exposed via a configuration
> > table, and updates the reference during SetVirtualAddressMap.
> > 
> > There is also a known bug in EDK2 where the ESRT table is passed in
> > boot services memory, but the capsule update code actually tries to
> > access it at runtime, so this isn't as clean as we'd like it to be.
> > 
> > > The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
> > > in the UEFI spec. So we can marvel about expected behavior. Is there any
> > > other document specifying it?
> > > 
> > 
> > No, its de facto specification is in the EDK2 source tree.

Well, in reality, it appeared simultanously-ish in Linux and GRUB, and
I think is entry into the EDK2 codebase came later :)

> As all ARM systems use it I guess this GUID should move into the UEFI
> spec. Maybe Linaro could raise this issue.

While the UEFI specification does list the ACPI, ACPI_20, SMBIOS, and
some other common GUIDs, this is not where they are defined. If it
needs to go into some specification, that would probably be the
devicetree.org one.

/
    Leif