| Message ID | 20220227121856.34995-1-agraf@csgraf.de |
|---|---|
| State | Accepted, archived |
| Commit | 0832dd2900f3ec7bf2ae12866138fc0fd9970168 |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: Ignore DT when ACPI is on | expand |
> From: Alexander Graf <agraf@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. > > 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... 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@openbsd.org> > Signed-off-by: Alexander Graf <agraf@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 > >
On 27.02.22 14:07, Mark Kettenis wrote: >> From: Alexander Graf <agraf@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. >> >> 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@openbsd.org> > >> Signed-off-by: Alexander Graf <agraf@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 >> >>
On 2/27/22 15:13, Alexander Graf wrote: > > On 27.02.22 14:07, Mark Kettenis wrote: >>> From: Alexander Graf <agraf@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. 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. With which defconfig does this lead to a boot failure? Which board supplies ${fdt_addr_r} and has CONFIG_GENERATE_ACPI_TABLE=y? 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@openbsd.org> >> >>> Signed-off-by: Alexander Graf <agraf@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 >>> >>>
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@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@openbsd.org> >>> >>>> Signed-off-by: Alexander Graf <agraf@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 >>>> >>>> >
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 };
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. 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. Signed-off-by: Alexander Graf <agraf@csgraf.de> --- cmd/bootefi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)