[PATCH v2 0/2] small fixes when boot with acpi=force option

[PATCH v2 0/2] small fixes when boot with acpi=force option

[Yeoreum Yun]

When acpi=force option is used, the dt should be ignored whether it's
invalid, passed from command line or from configuration table but it
doesn't so it produces error message
while scanning dt early time thou it isn't used in booting process.

Change to ignore dt when acpi=force option is used.

v2:
  - ignore getting dtb from configuration table when acpi=force option is used

Yeoreum Yun (2):
  arm64/acpi: panic when failed to init acpi table with acpi=force
    option
  efi/fdt: ignore dtb when acpi option is used with force

 arch/arm64/kernel/acpi.c           |  2 ++
 drivers/firmware/efi/libstub/fdt.c | 10 ++++++----
 2 files changed, 8 insertions(+), 4 deletions(-)

--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

[PATCH v2 1/2] arm64/acpi: panic when failed to init acpi table with acpi=force option

[Yeoreum Yun]

when the acpi=force option is used,
the system does not fall back to the device tree (DT).
If it fails to initialize the ACPI table, it cannot proceed further.
In such cases, the system should invoke panic() to avoid contradicting
the user's explicit intent, as failing or
proceeding with unintended behavior would violate their wishes.

Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/acpi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index e6f66491fbe9..efdf24ed5c3e 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -225,6 +225,8 @@ void __init acpi_boot_table_init(void)
 		pr_err("Failed to init ACPI tables\n");
 		if (!param_acpi_force)
 			disable_acpi();
+		else
+			panic("Failed to boot with ACPI tables\n");
 	}

 done:
--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

Re: [PATCH v2 1/2] arm64/acpi: panic when failed to init acpi table with acpi=force option

[Ard Biesheuvel]

On Mon, 25 Nov 2024 at 18:08, Yeoreum Yun <yeoreum.yun@arm.com> wrote:
>
> when the acpi=force option is used,
> the system does not fall back to the device tree (DT).
> If it fails to initialize the ACPI table, it cannot proceed further.
> In such cases, the system should invoke panic() to avoid contradicting
> the user's explicit intent, as failing or
> proceeding with unintended behavior would violate their wishes.
>

Calling panic() at this point does not achieve anything useful,
though. Without ACPI tables or a DT, the only way to observe this
panic message is by using earlycon= with an explicit MMIO address, and
it might be better to limp on instead. Is there anything bad that
might happen because of this, other than the user's wishes getting
violated?


> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/acpi.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index e6f66491fbe9..efdf24ed5c3e 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -225,6 +225,8 @@ void __init acpi_boot_table_init(void)
>                 pr_err("Failed to init ACPI tables\n");
>                 if (!param_acpi_force)
>                         disable_acpi();
> +               else
> +                       panic("Failed to boot with ACPI tables\n");
>         }
>
>  done:
> --
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
>

Re: [PATCH v2 1/2] arm64/acpi: panic when failed to init acpi table with acpi=force option

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

On Mon, Nov 25, 2024 at 06:30:06PM +0100, Ard Biesheuvel wrote:
> On Mon, 25 Nov 2024 at 18:08, Yeoreum Yun <yeoreum.yun@arm.com> wrote:

> > when the acpi=force option is used,
> > the system does not fall back to the device tree (DT).
> > If it fails to initialize the ACPI table, it cannot proceed further.
> > In such cases, the system should invoke panic() to avoid contradicting
> > the user's explicit intent, as failing or
> > proceeding with unintended behavior would violate their wishes.

> Calling panic() at this point does not achieve anything useful,
> though. Without ACPI tables or a DT, the only way to observe this
> panic message is by using earlycon= with an explicit MMIO address, and
> it might be better to limp on instead. Is there anything bad that
> might happen because of this, other than the user's wishes getting
> violated?

It does rather depend why the user specified acpi=force, it's kind of an
unusual thing to specify on most systems...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] arm64/acpi: panic when failed to init acpi table with acpi=force option

[Levi Yun]

Hi Ard.

>
> Calling panic() at this point does not achieve anything useful,
> though. Without ACPI tables or a DT, the only way to observe this
> panic message is by using earlycon= with an explicit MMIO address, and
> it might be better to limp on instead. Is there anything bad that
> might happen because of this, other than the user's wishes getting
> violated?

IMHO, the most weird thing is progressing boot with acpi table although
it failed to initailise. in this situation continuing to boot maybe
dead in unexepceted places. I think it would be better to prevent
futher progress by calling the panic() in this situation.

[PATCH v2 2/2] efi/fdt: ignore dtb when acpi option is used with force

[Yeoreum Yun]

Since acpi=force doesn't use dt fallback, it's meaningless to load dt
from comaand line option or from configuration table.
Skip loading dt when acpi=force option is used.
otherwise it could produce unnecessary error message
while scanning dt if passed dt's format is invalid.

Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
 drivers/firmware/efi/libstub/fdt.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 6a337f1f8787..27291ef7c773 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -231,6 +231,8 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
 	struct exit_boot_struct priv;
 	unsigned long fdt_addr = 0;
 	unsigned long fdt_size = 0;
+	bool acpi_force = false;
+

 	if (!efi_novamap) {
 		status = efi_alloc_virtmap(&priv.runtime_map, &desc_size,
@@ -241,13 +243,17 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
 		}
 	}

+	if (strstr(cmdline_ptr, "acpi=force"))
+		acpi_force = true;
+
 	/*
 	 * Unauthenticated device tree data is a security hazard, so ignore
 	 * 'dtb=' unless UEFI Secure Boot is disabled.  We assume that secure
 	 * boot is enabled if we can't determine its state.
 	 */
 	if (!IS_ENABLED(CONFIG_EFI_ARMSTUB_DTB_LOADER) ||
-	    efi_get_secureboot() != efi_secureboot_mode_disabled) {
+	    efi_get_secureboot() != efi_secureboot_mode_disabled ||
+			acpi_force) {
 		if (strstr(cmdline_ptr, "dtb="))
 			efi_err("Ignoring DTB from command line.\n");
 	} else {
@@ -261,7 +267,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,

 	if (fdt_addr) {
 		efi_info("Using DTB from command line\n");
-	} else {
+	} else if (!acpi_force) {
 		/* Look for a device tree configuration table entry. */
 		fdt_addr = (uintptr_t)get_fdt(&fdt_size);
 		if (fdt_addr)
--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

Re: [PATCH v2 2/2] efi/fdt: ignore dtb when acpi option is used with force

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

On Mon, Nov 25, 2024 at 05:07:58PM +0000, Yeoreum Yun wrote:

>  	unsigned long fdt_addr = 0;
>  	unsigned long fdt_size = 0;
> +	bool acpi_force = false;
> +
> 

Extra blank line added here.

>  	if (!IS_ENABLED(CONFIG_EFI_ARMSTUB_DTB_LOADER) ||
> -	    efi_get_secureboot() != efi_secureboot_mode_disabled) {
> +	    efi_get_secureboot() != efi_secureboot_mode_disabled ||
> +			acpi_force) {

Should this line be aligned with the prior one?

The actual change looks sensible to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] efi/fdt: ignore dtb when acpi option is used with force

[Ard Biesheuvel]

On Mon, 25 Nov 2024 at 18:08, Yeoreum Yun <yeoreum.yun@arm.com> wrote:
>
> Since acpi=force doesn't use dt fallback, it's meaningless to load dt
> from comaand line option or from configuration table.
> Skip loading dt when acpi=force option is used.
> otherwise it could produce unnecessary error message
> while scanning dt if passed dt's format is invalid.
>
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>

I take it this is working around buggy firmware that passes both a DT
and ACPI tables, and the DT in question is broken?

If so, this should be fixed in the firmware. The EFI stub does not
reason at all about ACPI boot vs DT boot, and I would prefer to keep
it that way, especially because this code is shared with other
architectures. For instance, the meaning of acpi= could differ between
architectures, or they may not implement ACPI in the first place.

If not, I don't think there is anything to solve here. Those error
messages are not fatal, right? Note that older GRUB builds might use
the DT to pass initrd information even when booting via ACPI.



> ---
>  drivers/firmware/efi/libstub/fdt.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 6a337f1f8787..27291ef7c773 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -231,6 +231,8 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
>         struct exit_boot_struct priv;
>         unsigned long fdt_addr = 0;
>         unsigned long fdt_size = 0;
> +       bool acpi_force = false;
> +
>
>         if (!efi_novamap) {
>                 status = efi_alloc_virtmap(&priv.runtime_map, &desc_size,
> @@ -241,13 +243,17 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
>                 }
>         }
>
> +       if (strstr(cmdline_ptr, "acpi=force"))
> +               acpi_force = true;
> +
>         /*
>          * Unauthenticated device tree data is a security hazard, so ignore
>          * 'dtb=' unless UEFI Secure Boot is disabled.  We assume that secure
>          * boot is enabled if we can't determine its state.
>          */
>         if (!IS_ENABLED(CONFIG_EFI_ARMSTUB_DTB_LOADER) ||
> -           efi_get_secureboot() != efi_secureboot_mode_disabled) {
> +           efi_get_secureboot() != efi_secureboot_mode_disabled ||
> +                       acpi_force) {
>                 if (strstr(cmdline_ptr, "dtb="))
>                         efi_err("Ignoring DTB from command line.\n");
>         } else {
> @@ -261,7 +267,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
>
>         if (fdt_addr) {
>                 efi_info("Using DTB from command line\n");
> -       } else {
> +       } else if (!acpi_force) {
>                 /* Look for a device tree configuration table entry. */
>                 fdt_addr = (uintptr_t)get_fdt(&fdt_size);
>                 if (fdt_addr)
> --
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
>

Re: [PATCH v2 2/2] efi/fdt: ignore dtb when acpi option is used with force

[Levi Yun]

Hi Ard.

> I take it this is working around buggy firmware that passes both a DT
> and ACPI tables, and the DT in question is broken?
>
> If so, this should be fixed in the firmware. The EFI stub does not
> reason at all about ACPI boot vs DT boot, and I would prefer to keep
> it that way, especially because this code is shared with other
> architectures. For instance, the meaning of acpi= could differ between
> architectures, or they may not implement ACPI in the first place.

What I concern is that It doesntt necessary to check DT
otherwise if the FDT variable in variable storage's contents is
corrupted, it would complain while it check in early_init_dt_scan()
thou the dt isn't used in boot process.

also, although acpi= could differ from architecture, the force option's menaing
seems the same over architecture (ignore DT boot with ACPI tables).

So I think the check the "acpi=force" option to prevent loading DT seems
good.

Am I missing?

Thanks

Re: [PATCH v2 2/2] efi/fdt: ignore dtb when acpi option is used with force

[Ard Biesheuvel]

On Mon, 25 Nov 2024 at 18:46, Levi Yun <yeoreum.yun@arm.com> wrote:
>
> Hi Ard.
>
> > I take it this is working around buggy firmware that passes both a DT
> > and ACPI tables, and the DT in question is broken?
> >
> > If so, this should be fixed in the firmware. The EFI stub does not
> > reason at all about ACPI boot vs DT boot, and I would prefer to keep
> > it that way, especially because this code is shared with other
> > architectures. For instance, the meaning of acpi= could differ between
> > architectures, or they may not implement ACPI in the first place.
>
> What I concern is that It doesntt necessary to check DT
> otherwise if the FDT variable in variable storage's contents is
> corrupted, it would complain while it check in early_init_dt_scan()
> thou the dt isn't used in boot process.
>

The DT is not stored in a variable.

If CONFIG_EFI_ARMSTUB_DTB_LOADER is enabled, it may be provided via
dtb= on the command line, but I have little sympathy for a user that
passes both dtb= *and* acpi=force, so this is a scenario that we can
ignore.

Otherwise, it is taken from a EFI config table, which is just a
<guid,addr> tuple describing a location in physical memory where the
firmware has placed a DT. If the firmware puts a corrupted DT there,
the firmware should be fixed instead.

acpi=force is intended to force the use of ACPI tables on a system
that provides both.

> also, although acpi= could differ from architecture, the force option's menaing
> seems the same over architecture (ignore DT boot with ACPI tables).
>
> So I think the check the "acpi=force" option to prevent loading DT seems
> good.
>

The EFI stub does not care about ACPI vs DT boot, and I'd prefer to
keep it that way unless there is a good reason.

Which real-world problem does this patch aim to solve?

Re: [PATCH v2 2/2] efi/fdt: ignore dtb when acpi option is used with force

[Levi Yun]

Hi Ard.

>
> The DT is not stored in a variable.
>
> If CONFIG_EFI_ARMSTUB_DTB_LOADER is enabled, it may be provided via
> dtb= on the command line, but I have little sympathy for a user that
> passes both dtb= *and* acpi=force, so this is a scenario that we can
> ignore.
>
> Otherwise, it is taken from a EFI config table, which is just a
> <guid,addr> tuple describing a location in physical memory where the
> firmware has placed a DT. If the firmware puts a corrupted DT there,
> the firmware should be fixed instead.
>
> acpi=force is intended to force the use of ACPI tables on a system
> that provides both.
>
> > also, although acpi= could differ from architecture, the force option's menaing
> > seems the same over architecture (ignore DT boot with ACPI tables).
> >
> > So I think the check the "acpi=force" option to prevent loading DT seems
> > good.
> >
>
> The EFI stub does not care about ACPI vs DT boot, and I'd prefer to
> keep it that way unless there is a good reason.
>
> Which real-world problem does this patch aim to solve?

Well. I had lack of explaination. In case of Juno platform, it loads
FDT from "Fdt" variable from the storage and install it into
configuration table with corrupted Fdt because of FDT stored in variable
storage was corrupted.

In that siutation, If it loads corrupted fdt, it prints error message
while sanity check in early_init_dt_scan().
This kind of error message would be confused to user because
user already specifies to boot with acpi table only with acpi=force
option.

anyway, what kind of way to install fdt into configuraiton table is not
matter. but when the dt installed in configuration table isn't valid,
it could produce the error message which seems violate user specified
option.

unless check the acpi=force to ignore DT, I think it would require to
check the installed DT in configuration table or passed should have
simple sanity check doen in early_init_dt_scan() so that error messsage
which makes some confusion for this situation.

Am I missing?

Thanks.

Re: [PATCH v2 2/2] efi/fdt: ignore dtb when acpi option is used with force

[Ard Biesheuvel]

On Mon, 25 Nov 2024 at 19:16, Levi Yun <yeoreum.yun@arm.com> wrote:
>
> Hi Ard.
>
> >
> > The DT is not stored in a variable.
> >
> > If CONFIG_EFI_ARMSTUB_DTB_LOADER is enabled, it may be provided via
> > dtb= on the command line, but I have little sympathy for a user that
> > passes both dtb= *and* acpi=force, so this is a scenario that we can
> > ignore.
> >
> > Otherwise, it is taken from a EFI config table, which is just a
> > <guid,addr> tuple describing a location in physical memory where the
> > firmware has placed a DT. If the firmware puts a corrupted DT there,
> > the firmware should be fixed instead.
> >
> > acpi=force is intended to force the use of ACPI tables on a system
> > that provides both.
> >
> > > also, although acpi= could differ from architecture, the force option's menaing
> > > seems the same over architecture (ignore DT boot with ACPI tables).
> > >
> > > So I think the check the "acpi=force" option to prevent loading DT seems
> > > good.
> > >
> >
> > The EFI stub does not care about ACPI vs DT boot, and I'd prefer to
> > keep it that way unless there is a good reason.
> >
> > Which real-world problem does this patch aim to solve?
>
> Well. I had lack of explaination. In case of Juno platform, it loads
> FDT from "Fdt" variable from the storage and install it into
> configuration table with corrupted Fdt because of FDT stored in variable
> storage was corrupted.
>
> In that siutation, If it loads corrupted fdt, it prints error message
> while sanity check in early_init_dt_scan().
> This kind of error message would be confused to user because
> user already specifies to boot with acpi table only with acpi=force
> option.
>
> anyway, what kind of way to install fdt into configuraiton table is not
> matter. but when the dt installed in configuration table isn't valid,
> it could produce the error message which seems violate user specified
> option.
>
> unless check the acpi=force to ignore DT, I think it would require to
> check the installed DT in configuration table or passed should have
> simple sanity check doen in early_init_dt_scan() so that error messsage
> which makes some confusion for this situation.
>

Thanks for explaining the issue in more detail. Juno is a development
platform with a highly unusual boot stack so I don't think we need to
accommodate its quirks in the upstream kernel.

And I still don't think this is something worth fixing in general.
Even if the machine description should be taken from ACPI tables only,
the DT /chosen node is always used as a conduit by the EFI stub, and
there are cases, e.g., for initrd info or the kaslr seed, where this
information might come from the bootloader, such as older GRUB builds.

Re: [PATCH v2 2/2] efi/fdt: ignore dtb when acpi option is used with force

[Levi Yun]

Hi Ard,

> Thanks for explaining the issue in more detail. Juno is a development
> platform with a highly unusual boot stack so I don't think we need to
> accommodate its quirks in the upstream kernel.
>
> And I still don't think this is something worth fixing in general.
> Even if the machine description should be taken from ACPI tables only,
> the DT /chosen node is always used as a conduit by the EFI stub, and
> there are cases, e.g., for initrd info or the kaslr seed, where this
> information might come from the bootloader, such as older GRUB builds.

But suppose the DT loaded is corrupted (i.e.) no property for
"#size-cells" in root node.
In this case, uefi properties in chosen node will be installed in
"corrupted" root node.

my suggetion is not to check "acpi=force" option but verify dt which
loaded via dtb= command line option or
via configuration table in efi-stub code before updating
fdt with uefi properties in chosen node so that prevent generating error
message.

Am I missing?

Thanks