| Message ID | 20241125170758.518943-3-yeoreum.yun@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | small fixes when boot with acpi=force option | expand |
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.
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} >
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
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?
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.
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.
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
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)
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(-) -- LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}