[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 at 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 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.
>

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 at linaro.org>