[PATCH] efi_loader: Ignore DT when ACPI is on

[Alexander Graf]

Hey Heinrich,

On 27.02.22 16:33, Heinrich Schuchardt wrote:
> On 2/27/22 15:13, Alexander Graf wrote:
>>
>> On 27.02.22 14:07, Mark Kettenis wrote:
>>>> From: Alexander Graf <agraf at csgraf.de>
>>>> Date: Sun, 27 Feb 2022 13:18:56 +0100
>>>>
>>>> For targets that enable ACPI, we should not pass Device Trees into
>>>> the payload. However, our distro boot logic always passes the builtin
>>>> DT as an argument.
>
> Hello Alex,
>
> nice to see you back at U-Boot.


Thanks :)


> If CONFIG_GENERATE_ACPI_TABLE=n, no ACPI table is generated. In this
> case U-Boot may fall back to the built in devicetree.
>
> If CONFIG_GENERATE_ACPI_TABLE=y, copy_fdt() does not exist and a
> device-tree cannot be supplied to the EFI binary. If you still try
> supply a device tree, the bootefi command writes an error an refuses to
> boot.


This is the case I fell into (and that this patch touches), yes :).


> With which defconfig does this lead to a boot failure?


This is with the qemu_arm64_defconfig and this on top:

echo -e 
'CONFIG_ACPIGEN=y\nCONFIG_GENERATE_ACPI_TABLE=y\nCONFIG_CMD_ACPI=y' >> 
.config

I haven't fully made up my mind whether this is something we'll want a 
separate defconfig for eventually. I just wanted to play a bit with 
loading Windows 10/11 via U-Boot :).


> Which board supplies ${fdt_addr_r} and has CONFIG_GENERATE_ACPI_TABLE=y?


I don't see fdt_addr_r supplied at all in the QEMU env - there's only 
$fdtcontroladdr which gets passed from QEMU. QEMU passes both to the 
VM's firmware - DT via memory and location in register as well as ACPI 
via fw_cfg.

I'm also not running the bootmgr code path. Since I have no env with 
explicit boot entries, I run into the fallback case which always passes 
a DT to the payload (line 147 in config_distro_bootcmd.h).


Alex


>
> See include/config_distro_bootcmd.h line 127ff.
>
> Best regards
>
> Heinrich
>
>>>>
>>>> To make it easy to use ACPI with distro boot, let's just ignore the DT
>>>> argument to bootefi when ACPI is enabled. That way, we can 
>>>> successfully
>>>> distro boot payloads on ACPI enabled targets.
>>> I've never understood why it is bad to provide both the DT and ACPI
>>> tables.  Several TianoCore EDK2 based firmwares do this or at least
>>> give you the option to do this.  And it works fine.  If both are
>>> provided, you just leave it up to the OS to decide what it wants to
>>> use.  Which makes me think the text was put there in the standard by
>>> people with an agenda...
>>
>>
>> It's much more profane :). Upstream Linux went with "DT first, ACPI
>> last" while RHEL had a downstream patch that went "ACPI first, DT last".
>> And then people were confused why RHEL and upstream kernels had such
>> vastly different behavior on their systems.
>>
>> Long story short, the best way to avoid this ambiguity is to only
>> provide one of the 2 to the OS. And that's what the BBRs dictate now.
>> They don't dictate what firmware does internally though, so we're free
>> to do whatever we like - and I think "if someone went through the effort
>> to enable ACPI on this system, they probably want to use it" is a fair
>> assumption with U-Boot. So we should prefer ACPI if available.
>>
>>
>> Alex
>>
>>
>>>
>>> I think it would be good for U-Boot to give users a choice here, with
>>> all possible combinations (only DT, only ACPI, both DT and ACPI).  But
>>> this is a step in the right direction.
>>>
>>> Reviewed-by: Mark Kettenis <kettenis at openbsd.org>
>>>
>>>> Signed-off-by: Alexander Graf <agraf at csgraf.de>
>>>> ---
>>>>   cmd/bootefi.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>> index 94d18ca73f..2c9bc0dcd2 100644
>>>> --- a/cmd/bootefi.c
>>>> +++ b/cmd/bootefi.c
>>>> @@ -265,8 +265,8 @@ efi_status_t efi_install_fdt(void *fdt)
>>>>        */
>>>>   #if CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
>>>>       if (fdt) {
>>>> -        log_err("ERROR: can't have ACPI table and device tree.\n");
>>>> -        return EFI_LOAD_ERROR;
>>>> +        log_warning("WARNING: Can't have ACPI table and device tree
>>>> - ignoring DT.\n");
>>>> +        return EFI_SUCCESS;
>>>>       }
>>>>   #else
>>>>       bootm_headers_t img = { 0 };
>>>> -- 
>>>> 2.32.0
>>>>
>>>>
>