[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 at 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 at 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 at linaro.org>
>>>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at 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