2023-04-26 03:45:22

by yunhui cui

[permalink] [raw]
Subject: [PATCH] firmware: added a firmware information passing method FFI

Some BootLoaders do not support UEFI and cannot pass ACPI/SBMIOS table
addresses through UEFI, such as coreboot.

On the x86 platform, we pass the ACPI/SMBIOS table through the reserved
address segment 0xF0000, but other arches usually do not reserve this
address segment.

We have added a new firmware information transmission method named FFI
(FDT FIRMWARE INTERFACE), through FDT to obtain firmware information,
such as the base address of the ACPI and SMBIOS table.

Signed-off-by: Yunhui Cui <[email protected]>
---
MAINTAINERS | 6 +++
drivers/acpi/osl.c | 8 ++++
drivers/firmware/Kconfig | 12 +++++
drivers/firmware/Makefile | 1 +
drivers/firmware/dmi_scan.c | 96 ++++++++++++++++++++++---------------
drivers/firmware/ffi.c | 56 ++++++++++++++++++++++
include/linux/ffi.h | 15 ++++++
7 files changed, 155 insertions(+), 39 deletions(-)
create mode 100644 drivers/firmware/ffi.c
create mode 100644 include/linux/ffi.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 8d5bc223f305..94664f3b4c96 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7750,6 +7750,12 @@ F: arch/x86/platform/efi/
F: drivers/firmware/efi/
F: include/linux/efi*.h

+FDT FIRMWARE INTERFACE (FFI)
+M: Yunhui Cui [email protected]
+S: Maintained
+F: drivers/firmware/ffi.c
+F: include/linux/ffi.h
+
EXTERNAL CONNECTOR SUBSYSTEM (EXTCON)
M: MyungJoo Ham <[email protected]>
M: Chanwoo Choi <[email protected]>
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 3269a888fb7a..d45000041d2b 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -25,6 +25,7 @@
#include <linux/nmi.h>
#include <linux/acpi.h>
#include <linux/efi.h>
+#include <linux/ffi.h>
#include <linux/ioport.h>
#include <linux/list.h>
#include <linux/jiffies.h>
@@ -206,6 +207,13 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
if (pa)
return pa;

+#ifdef CONFIG_FDT_FW_INTERFACE
+ if (fdt_fwtbl.acpi20 != FDT_INVALID_FWTBL_ADDR)
+ return fdt_fwtbl.acpi20;
+ if (fdt_fwtbl.acpi != FDT_INVALID_FWTBL_ADDR)
+ return fdt_fwtbl.acpi;
+ pr_err("Fdt system description tables not found\n");
+#endif
if (efi_enabled(EFI_CONFIG_TABLES)) {
if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
return efi.acpi20;
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b59e3041fd62..13c67b50c17a 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -303,6 +303,18 @@ config TURRIS_MOX_RWTM
other manufacturing data and also utilize the Entropy Bit Generator
for hardware random number generation.

+config FDT_FW_INTERFACE
+ bool "An interface for passing firmware info through FDT"
+ depends on OF && OF_FLATTREE
+ default n
+ help
+ When some bootloaders do not support UEFI, and the arch does not
+ support SMBIOS_ENTRY_POINT_SCAN_START, then you can enable this option
+ to support the transfer of firmware information, such as acpi, smbios
+ tables.
+
+ Say Y here if you want to pass firmware information by FDT.
+
source "drivers/firmware/arm_ffa/Kconfig"
source "drivers/firmware/broadcom/Kconfig"
source "drivers/firmware/cirrus/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 28fcddcd688f..3b8b5d0868a6 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -33,6 +33,7 @@ obj-y += cirrus/
obj-y += meson/
obj-$(CONFIG_GOOGLE_FIRMWARE) += google/
obj-y += efi/
+obj-$(CONFIG_FDT_FW_INTERFACE) += ffi.o
obj-y += imx/
obj-y += psci/
obj-y += smccc/
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 015c95a825d3..1e1a74ed7d3b 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -6,6 +6,7 @@
#include <linux/ctype.h>
#include <linux/dmi.h>
#include <linux/efi.h>
+#include <linux/ffi.h>
#include <linux/memblock.h>
#include <linux/random.h>
#include <asm/dmi.h>
@@ -655,54 +656,71 @@ static int __init dmi_smbios3_present(const u8 *buf)
return 1;
}

-static void __init dmi_scan_machine(void)
+/*
+ * According to the DMTF SMBIOS reference spec v3.0.0, it is
+ * allowed to define both the 64-bit entry point (smbios3) and
+ * the 32-bit entry point (smbios), in which case they should
+ * either both point to the same SMBIOS structure table, or the
+ * table pointed to by the 64-bit entry point should contain a
+ * superset of the table contents pointed to by the 32-bit entry
+ * point (section 5.2)
+ * This implies that the 64-bit entry point should have
+ * precedence if it is defined and supported by the OS. If we
+ * have the 64-bit entry point, but fail to decode it, fall
+ * back to the legacy one (if available)
+ */
+static int __init dmi_sacn_smbios(unsigned long smbios3, unsigned long smbios)
{
- char __iomem *p, *q;
+ char __iomem *p;
char buf[32];
+ #define INVALID_TABLE_ADDR (~0UL)

- if (efi_enabled(EFI_CONFIG_TABLES)) {
- /*
- * According to the DMTF SMBIOS reference spec v3.0.0, it is
- * allowed to define both the 64-bit entry point (smbios3) and
- * the 32-bit entry point (smbios), in which case they should
- * either both point to the same SMBIOS structure table, or the
- * table pointed to by the 64-bit entry point should contain a
- * superset of the table contents pointed to by the 32-bit entry
- * point (section 5.2)
- * This implies that the 64-bit entry point should have
- * precedence if it is defined and supported by the OS. If we
- * have the 64-bit entry point, but fail to decode it, fall
- * back to the legacy one (if available)
- */
- if (efi.smbios3 != EFI_INVALID_TABLE_ADDR) {
- p = dmi_early_remap(efi.smbios3, 32);
- if (p == NULL)
- goto error;
- memcpy_fromio(buf, p, 32);
- dmi_early_unmap(p, 32);
-
- if (!dmi_smbios3_present(buf)) {
- dmi_available = 1;
- return;
- }
- }
- if (efi.smbios == EFI_INVALID_TABLE_ADDR)
- goto error;
-
- /* This is called as a core_initcall() because it isn't
- * needed during early boot. This also means we can
- * iounmap the space when we're done with it.
- */
- p = dmi_early_remap(efi.smbios, 32);
+ if (smbios3 != INVALID_TABLE_ADDR) {
+ p = dmi_early_remap(smbios3, 32);
if (p == NULL)
- goto error;
+ return -1;
memcpy_fromio(buf, p, 32);
dmi_early_unmap(p, 32);

- if (!dmi_present(buf)) {
+ if (!dmi_smbios3_present(buf)) {
dmi_available = 1;
- return;
+ return 0;
}
+ }
+
+ if (smbios == INVALID_TABLE_ADDR)
+ return -1;
+
+ /*
+ * This is called as a core_initcall() because it isn't
+ * needed during early boot. This also means we can
+ * iounmap the space when we're done with it.
+ */
+ p = dmi_early_remap(smbios, 32);
+ if (p == NULL)
+ return -1;
+ memcpy_fromio(buf, p, 32);
+ dmi_early_unmap(p, 32);
+
+ if (!dmi_present(buf)) {
+ dmi_available = 1;
+ return 0;
+ }
+ return -1;
+}
+
+static void __init dmi_scan_machine(void)
+{
+ char __iomem *p, *q;
+ char buf[32];
+
+#ifdef CONFIG_FDT_FW_INTERFACE
+ if (dmi_sacn_smbios(fdt_fwtbl.smbios3, fdt_fwtbl.smbios))
+ goto error;
+#endif
+ if (efi_enabled(EFI_CONFIG_TABLES)) {
+ if (dmi_sacn_smbios(efi.smbios3, efi.smbios))
+ goto error;
} else if (IS_ENABLED(CONFIG_DMI_SCAN_MACHINE_NON_EFI_FALLBACK)) {
p = dmi_early_remap(SMBIOS_ENTRY_POINT_SCAN_START, 0x10000);
if (p == NULL)
diff --git a/drivers/firmware/ffi.c b/drivers/firmware/ffi.c
new file mode 100644
index 000000000000..83c7abf22220
--- /dev/null
+++ b/drivers/firmware/ffi.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kobject.h>
+#include <linux/ffi.h>
+#include <linux/of_fdt.h>
+#include <linux/libfdt.h>
+
+struct fdt_fwtable __read_mostly fdt_fwtbl = {
+ .acpi = FDT_INVALID_FWTBL_ADDR,
+ .acpi20 = FDT_INVALID_FWTBL_ADDR,
+ .smbios = FDT_INVALID_FWTBL_ADDR,
+ .smbios3 = FDT_INVALID_FWTBL_ADDR,
+};
+EXPORT_SYMBOL(fdt_fwtbl);
+
+void __init of_fdt_fwtbl(void)
+{
+ int cfgtbl, len;
+ fdt64_t *prop;
+
+ cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
+ if (cfgtbl < 0) {
+ pr_info("cfgtables not found.\n");
+ return;
+ }
+ prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios_phy_ptr", &len);
+ if (!prop || len != sizeof(u64))
+ pr_info("smbios_phy_ptr not found.\n");
+ else
+ fdt_fwtbl.smbios = fdt64_to_cpu(*prop);
+
+ prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios3_phy_ptr", &len);
+ if (!prop || len != sizeof(u64))
+ pr_info("smbios3_phy_ptr not found.\n");
+ else
+ fdt_fwtbl.smbios3 = fdt64_to_cpu(*prop);
+
+ prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi_phy_ptr", &len);
+ if (!prop || len != sizeof(u64))
+ pr_info("acpi_phy_ptr not found.\n");
+ else
+ fdt_fwtbl.acpi = fdt64_to_cpu(*prop);
+
+ prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi20_phy_ptr", &len);
+ if (!prop || len != sizeof(u64))
+ pr_info("acpi20_phy_ptr not found.\n");
+ else
+ fdt_fwtbl.acpi20 = fdt64_to_cpu(*prop);
+}
+
+void __init fdt_fwtbl_init(void)
+{
+ of_fdt_fwtbl();
+}
diff --git a/include/linux/ffi.h b/include/linux/ffi.h
new file mode 100644
index 000000000000..ffb50810a01e
--- /dev/null
+++ b/include/linux/ffi.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_FDT_FW_H
+#define _LINUX_FDT_FW_H
+
+#define FDT_INVALID_FWTBL_ADDR (~0UL)
+extern struct fdt_fwtable {
+ unsigned long acpi;
+ unsigned long acpi20;
+ unsigned long smbios;
+ unsigned long smbios3;
+ unsigned long flags;
+} fdt_fwtbl;
+
+#endif
--
2.20.1


2023-04-26 07:20:38

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH] firmware: added a firmware information passing method FFI

On Wed, 26 Apr 2023 at 04:40, Yunhui Cui <[email protected]> wrote:
>
> Some BootLoaders do not support UEFI and cannot pass ACPI/SBMIOS table
> addresses through UEFI, such as coreboot.
>
> On the x86 platform, we pass the ACPI/SMBIOS table through the reserved
> address segment 0xF0000, but other arches usually do not reserve this
> address segment.
>
> We have added a new firmware information transmission method named FFI
> (FDT FIRMWARE INTERFACE), through FDT to obtain firmware information,
> such as the base address of the ACPI and SMBIOS table.
>
> Signed-off-by: Yunhui Cui <[email protected]>

Hello Yunhui,

I am not sure this is a good idea: this is clearly intended for arm64,
which cannot use ACPI without the EFI memory map, which it uses to
cross reference memory opregion accesses, to determine the correct
memory type attributes.

What is the use case you are trying to accommodate here?




> ---
> MAINTAINERS | 6 +++
> drivers/acpi/osl.c | 8 ++++
> drivers/firmware/Kconfig | 12 +++++
> drivers/firmware/Makefile | 1 +
> drivers/firmware/dmi_scan.c | 96 ++++++++++++++++++++++---------------
> drivers/firmware/ffi.c | 56 ++++++++++++++++++++++
> include/linux/ffi.h | 15 ++++++
> 7 files changed, 155 insertions(+), 39 deletions(-)
> create mode 100644 drivers/firmware/ffi.c
> create mode 100644 include/linux/ffi.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8d5bc223f305..94664f3b4c96 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7750,6 +7750,12 @@ F: arch/x86/platform/efi/
> F: drivers/firmware/efi/
> F: include/linux/efi*.h
>
> +FDT FIRMWARE INTERFACE (FFI)
> +M: Yunhui Cui [email protected]
> +S: Maintained
> +F: drivers/firmware/ffi.c
> +F: include/linux/ffi.h
> +
> EXTERNAL CONNECTOR SUBSYSTEM (EXTCON)
> M: MyungJoo Ham <[email protected]>
> M: Chanwoo Choi <[email protected]>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 3269a888fb7a..d45000041d2b 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -25,6 +25,7 @@
> #include <linux/nmi.h>
> #include <linux/acpi.h>
> #include <linux/efi.h>
> +#include <linux/ffi.h>
> #include <linux/ioport.h>
> #include <linux/list.h>
> #include <linux/jiffies.h>
> @@ -206,6 +207,13 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
> if (pa)
> return pa;
>
> +#ifdef CONFIG_FDT_FW_INTERFACE
> + if (fdt_fwtbl.acpi20 != FDT_INVALID_FWTBL_ADDR)
> + return fdt_fwtbl.acpi20;
> + if (fdt_fwtbl.acpi != FDT_INVALID_FWTBL_ADDR)
> + return fdt_fwtbl.acpi;
> + pr_err("Fdt system description tables not found\n");
> +#endif
> if (efi_enabled(EFI_CONFIG_TABLES)) {
> if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> return efi.acpi20;
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index b59e3041fd62..13c67b50c17a 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -303,6 +303,18 @@ config TURRIS_MOX_RWTM
> other manufacturing data and also utilize the Entropy Bit Generator
> for hardware random number generation.
>
> +config FDT_FW_INTERFACE
> + bool "An interface for passing firmware info through FDT"
> + depends on OF && OF_FLATTREE
> + default n
> + help
> + When some bootloaders do not support UEFI, and the arch does not
> + support SMBIOS_ENTRY_POINT_SCAN_START, then you can enable this option
> + to support the transfer of firmware information, such as acpi, smbios
> + tables.
> +
> + Say Y here if you want to pass firmware information by FDT.
> +
> source "drivers/firmware/arm_ffa/Kconfig"
> source "drivers/firmware/broadcom/Kconfig"
> source "drivers/firmware/cirrus/Kconfig"
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 28fcddcd688f..3b8b5d0868a6 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -33,6 +33,7 @@ obj-y += cirrus/
> obj-y += meson/
> obj-$(CONFIG_GOOGLE_FIRMWARE) += google/
> obj-y += efi/
> +obj-$(CONFIG_FDT_FW_INTERFACE) += ffi.o
> obj-y += imx/
> obj-y += psci/
> obj-y += smccc/
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 015c95a825d3..1e1a74ed7d3b 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -6,6 +6,7 @@
> #include <linux/ctype.h>
> #include <linux/dmi.h>
> #include <linux/efi.h>
> +#include <linux/ffi.h>
> #include <linux/memblock.h>
> #include <linux/random.h>
> #include <asm/dmi.h>
> @@ -655,54 +656,71 @@ static int __init dmi_smbios3_present(const u8 *buf)
> return 1;
> }
>
> -static void __init dmi_scan_machine(void)
> +/*
> + * According to the DMTF SMBIOS reference spec v3.0.0, it is
> + * allowed to define both the 64-bit entry point (smbios3) and
> + * the 32-bit entry point (smbios), in which case they should
> + * either both point to the same SMBIOS structure table, or the
> + * table pointed to by the 64-bit entry point should contain a
> + * superset of the table contents pointed to by the 32-bit entry
> + * point (section 5.2)
> + * This implies that the 64-bit entry point should have
> + * precedence if it is defined and supported by the OS. If we
> + * have the 64-bit entry point, but fail to decode it, fall
> + * back to the legacy one (if available)
> + */
> +static int __init dmi_sacn_smbios(unsigned long smbios3, unsigned long smbios)
> {
> - char __iomem *p, *q;
> + char __iomem *p;
> char buf[32];
> + #define INVALID_TABLE_ADDR (~0UL)
>
> - if (efi_enabled(EFI_CONFIG_TABLES)) {
> - /*
> - * According to the DMTF SMBIOS reference spec v3.0.0, it is
> - * allowed to define both the 64-bit entry point (smbios3) and
> - * the 32-bit entry point (smbios), in which case they should
> - * either both point to the same SMBIOS structure table, or the
> - * table pointed to by the 64-bit entry point should contain a
> - * superset of the table contents pointed to by the 32-bit entry
> - * point (section 5.2)
> - * This implies that the 64-bit entry point should have
> - * precedence if it is defined and supported by the OS. If we
> - * have the 64-bit entry point, but fail to decode it, fall
> - * back to the legacy one (if available)
> - */
> - if (efi.smbios3 != EFI_INVALID_TABLE_ADDR) {
> - p = dmi_early_remap(efi.smbios3, 32);
> - if (p == NULL)
> - goto error;
> - memcpy_fromio(buf, p, 32);
> - dmi_early_unmap(p, 32);
> -
> - if (!dmi_smbios3_present(buf)) {
> - dmi_available = 1;
> - return;
> - }
> - }
> - if (efi.smbios == EFI_INVALID_TABLE_ADDR)
> - goto error;
> -
> - /* This is called as a core_initcall() because it isn't
> - * needed during early boot. This also means we can
> - * iounmap the space when we're done with it.
> - */
> - p = dmi_early_remap(efi.smbios, 32);
> + if (smbios3 != INVALID_TABLE_ADDR) {
> + p = dmi_early_remap(smbios3, 32);
> if (p == NULL)
> - goto error;
> + return -1;
> memcpy_fromio(buf, p, 32);
> dmi_early_unmap(p, 32);
>
> - if (!dmi_present(buf)) {
> + if (!dmi_smbios3_present(buf)) {
> dmi_available = 1;
> - return;
> + return 0;
> }
> + }
> +
> + if (smbios == INVALID_TABLE_ADDR)
> + return -1;
> +
> + /*
> + * This is called as a core_initcall() because it isn't
> + * needed during early boot. This also means we can
> + * iounmap the space when we're done with it.
> + */
> + p = dmi_early_remap(smbios, 32);
> + if (p == NULL)
> + return -1;
> + memcpy_fromio(buf, p, 32);
> + dmi_early_unmap(p, 32);
> +
> + if (!dmi_present(buf)) {
> + dmi_available = 1;
> + return 0;
> + }
> + return -1;
> +}
> +
> +static void __init dmi_scan_machine(void)
> +{
> + char __iomem *p, *q;
> + char buf[32];
> +
> +#ifdef CONFIG_FDT_FW_INTERFACE
> + if (dmi_sacn_smbios(fdt_fwtbl.smbios3, fdt_fwtbl.smbios))
> + goto error;
> +#endif
> + if (efi_enabled(EFI_CONFIG_TABLES)) {
> + if (dmi_sacn_smbios(efi.smbios3, efi.smbios))
> + goto error;
> } else if (IS_ENABLED(CONFIG_DMI_SCAN_MACHINE_NON_EFI_FALLBACK)) {
> p = dmi_early_remap(SMBIOS_ENTRY_POINT_SCAN_START, 0x10000);
> if (p == NULL)
> diff --git a/drivers/firmware/ffi.c b/drivers/firmware/ffi.c
> new file mode 100644
> index 000000000000..83c7abf22220
> --- /dev/null
> +++ b/drivers/firmware/ffi.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kobject.h>
> +#include <linux/ffi.h>
> +#include <linux/of_fdt.h>
> +#include <linux/libfdt.h>
> +
> +struct fdt_fwtable __read_mostly fdt_fwtbl = {
> + .acpi = FDT_INVALID_FWTBL_ADDR,
> + .acpi20 = FDT_INVALID_FWTBL_ADDR,
> + .smbios = FDT_INVALID_FWTBL_ADDR,
> + .smbios3 = FDT_INVALID_FWTBL_ADDR,
> +};
> +EXPORT_SYMBOL(fdt_fwtbl);
> +
> +void __init of_fdt_fwtbl(void)
> +{
> + int cfgtbl, len;
> + fdt64_t *prop;
> +
> + cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
> + if (cfgtbl < 0) {
> + pr_info("cfgtables not found.\n");
> + return;
> + }
> + prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios_phy_ptr", &len);
> + if (!prop || len != sizeof(u64))
> + pr_info("smbios_phy_ptr not found.\n");
> + else
> + fdt_fwtbl.smbios = fdt64_to_cpu(*prop);
> +
> + prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios3_phy_ptr", &len);
> + if (!prop || len != sizeof(u64))
> + pr_info("smbios3_phy_ptr not found.\n");
> + else
> + fdt_fwtbl.smbios3 = fdt64_to_cpu(*prop);
> +
> + prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi_phy_ptr", &len);
> + if (!prop || len != sizeof(u64))
> + pr_info("acpi_phy_ptr not found.\n");
> + else
> + fdt_fwtbl.acpi = fdt64_to_cpu(*prop);
> +
> + prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi20_phy_ptr", &len);
> + if (!prop || len != sizeof(u64))
> + pr_info("acpi20_phy_ptr not found.\n");
> + else
> + fdt_fwtbl.acpi20 = fdt64_to_cpu(*prop);
> +}
> +
> +void __init fdt_fwtbl_init(void)
> +{
> + of_fdt_fwtbl();
> +}
> diff --git a/include/linux/ffi.h b/include/linux/ffi.h
> new file mode 100644
> index 000000000000..ffb50810a01e
> --- /dev/null
> +++ b/include/linux/ffi.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _LINUX_FDT_FW_H
> +#define _LINUX_FDT_FW_H
> +
> +#define FDT_INVALID_FWTBL_ADDR (~0UL)
> +extern struct fdt_fwtable {
> + unsigned long acpi;
> + unsigned long acpi20;
> + unsigned long smbios;
> + unsigned long smbios3;
> + unsigned long flags;
> +} fdt_fwtbl;
> +
> +#endif
> --
> 2.20.1
>

2023-04-26 09:28:05

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH] firmware: added a firmware information passing method FFI

Hi Yunhui,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on linus/master jdelvare-staging/dmi-for-next v6.3 next-20230425]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Yunhui-Cui/firmware-added-a-firmware-information-passing-method-FFI/20230426-114131
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20230426034001.16-1-cuiyunhui%40bytedance.com
patch subject: [PATCH] firmware: added a firmware information passing method FFI
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230426/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/7d1fe633611738698520294d2a598575a765cfbf
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yunhui-Cui/firmware-added-a-firmware-information-passing-method-FFI/20230426-114131
git checkout 7d1fe633611738698520294d2a598575a765cfbf
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> drivers/firmware/ffi.c:18:13: warning: no previous prototype for 'of_fdt_fwtbl' [-Wmissing-prototypes]
18 | void __init of_fdt_fwtbl(void)
| ^~~~~~~~~~~~
>> drivers/firmware/ffi.c:53:13: warning: no previous prototype for 'fdt_fwtbl_init' [-Wmissing-prototypes]
53 | void __init fdt_fwtbl_init(void)
| ^~~~~~~~~~~~~~


vim +/of_fdt_fwtbl +18 drivers/firmware/ffi.c

17
> 18 void __init of_fdt_fwtbl(void)
19 {
20 int cfgtbl, len;
21 fdt64_t *prop;
22
23 cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
24 if (cfgtbl < 0) {
25 pr_info("cfgtables not found.\n");
26 return;
27 }
28 prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios_phy_ptr", &len);
29 if (!prop || len != sizeof(u64))
30 pr_info("smbios_phy_ptr not found.\n");
31 else
32 fdt_fwtbl.smbios = fdt64_to_cpu(*prop);
33
34 prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios3_phy_ptr", &len);
35 if (!prop || len != sizeof(u64))
36 pr_info("smbios3_phy_ptr not found.\n");
37 else
38 fdt_fwtbl.smbios3 = fdt64_to_cpu(*prop);
39
40 prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi_phy_ptr", &len);
41 if (!prop || len != sizeof(u64))
42 pr_info("acpi_phy_ptr not found.\n");
43 else
44 fdt_fwtbl.acpi = fdt64_to_cpu(*prop);
45
46 prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi20_phy_ptr", &len);
47 if (!prop || len != sizeof(u64))
48 pr_info("acpi20_phy_ptr not found.\n");
49 else
50 fdt_fwtbl.acpi20 = fdt64_to_cpu(*prop);
51 }
52
> 53 void __init fdt_fwtbl_init(void)

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

2023-04-26 09:41:43

by yunhui cui

[permalink] [raw]
Subject: Re: [External] Re: [PATCH] firmware: added a firmware information passing method FFI

Hi Ard,

On Wed, Apr 26, 2023 at 3:09 PM Ard Biesheuvel <[email protected]> wrote:
>
> Hello Yunhui,
>
> I am not sure this is a good idea: this is clearly intended for arm64,
> which cannot use ACPI without the EFI memory map, which it uses to
> cross reference memory opregion accesses, to determine the correct
> memory type attributes.
>
Not only for arm64, but also other arches, such as riscv.
For memory-related nodes, it will be done in the early scan of the device tree.


> What is the use case you are trying to accommodate here?
>
Some bootloaders do not support uefi, such as coreboot,
but need to support acpi, smbios.


Thanks,
Yunhui

2023-04-26 10:10:10

by Mark Rutland

[permalink] [raw]
Subject: Re: [External] Re: [PATCH] firmware: added a firmware information passing method FFI

On Wed, Apr 26, 2023 at 05:34:55PM +0800, 运辉崔 wrote:
> Hi Ard,
>
> On Wed, Apr 26, 2023 at 3:09 PM Ard Biesheuvel <[email protected]> wrote:
> >
> > Hello Yunhui,
> >
> > I am not sure this is a good idea: this is clearly intended for arm64,
> > which cannot use ACPI without the EFI memory map, which it uses to
> > cross reference memory opregion accesses, to determine the correct
> > memory type attributes.
> >
> Not only for arm64, but also other arches, such as riscv.
> For memory-related nodes, it will be done in the early scan of the device tree.

Ard's point is that the device tree doesn't have all the same information (e.g.
nothing in DT describes the memory type attributes), and so this isn't
sufficient.

We'd have to create entirely new ways to pass that information, which is not
very desirable.

> > What is the use case you are trying to accommodate here?
> >
> Some bootloaders do not support uefi, such as coreboot,
> but need to support acpi, smbios.

For arm64 at least, if you need ACPI you must have EFI, and trying to change
that will require significant work and long term maintenance.

Can you extend coreboot to provide EFI services, or to chain-load an EFI
payload?

Thanks,
Mark.

2023-04-27 03:39:48

by yunhui cui

[permalink] [raw]
Subject: Re: [External] Re: [PATCH] firmware: added a firmware information passing method FFI

Hi Mark,

On Wed, Apr 26, 2023 at 6:07 PM Mark Rutland <[email protected]> wrote:
>
> Ard's point is that the device tree doesn't have all the same information (e.g.
> nothing in DT describes the memory type attributes), and so this isn't
> sufficient.

The device tree only needs to complete the parse of the memory type attributes,
it should not be very complicated.

>
> We'd have to create entirely new ways to pass that information, which is not
> very desirable.
>

>
> Can you extend coreboot to provide EFI services, or to chain-load an EFI
> payload?

Currently, coreboot does not support UEFI, and it may not support it
in the future.
Hi rminnich, what do you think?

Thanks,
Yunhui