1. We need to enable the ACPI function on RISC-V. When booting with
Coreboot, we encounter two problems:
a. Coreboot does not support EFI
b. On RISC-V, only the DTS channel can be used.
2. Based on this, we have added an interface for obtaining firmware
information transfer through FDT, named FFI.
3. We not only use FFI to pass ACPI RSDP, but also pass other
firmware information as an extension.
Signed-off-by: Yunhui Cui <[email protected]>
---
MAINTAINERS | 6 ++++++
arch/riscv/Kconfig | 10 ++++++++++
arch/riscv/include/asm/acpi.h | 9 +++++++++
arch/riscv/include/asm/ffi.h | 9 +++++++++
arch/riscv/kernel/Makefile | 1 +
arch/riscv/kernel/ffi.c | 37 +++++++++++++++++++++++++++++++++++
arch/riscv/kernel/setup.c | 2 ++
7 files changed, 74 insertions(+)
create mode 100644 arch/riscv/include/asm/ffi.h
create mode 100644 arch/riscv/kernel/ffi.c
diff --git a/MAINTAINERS b/MAINTAINERS
index cd5388a33410..e592f489e757 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18363,6 +18363,12 @@ F: arch/riscv/boot/dts/
X: arch/riscv/boot/dts/allwinner/
X: arch/riscv/boot/dts/renesas/
+RISC-V FDT FIRMWARE INTERFACE (FFI) SUPPORT
+M: Yunhui Cui [email protected]
+S: Maintained
+F: arch/riscv/include/asm/ffi.h
+F: arch/riscv/kernel/ffi.c
+
RISC-V PMU DRIVERS
M: Atish Patra <[email protected]>
R: Anup Patel <[email protected]>
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index b49793cf34eb..2e1c40fb2300 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -785,6 +785,16 @@ config EFI
allow the kernel to be booted as an EFI application. This
is only useful on systems that have UEFI firmware.
+config FFI
+ bool "Fdt firmware interface"
+ depends on OF
+ default y
+ help
+ Added an interface to obtain firmware information transfer
+ through FDT, named FFI. Some bootloaders do not support EFI,
+ such as coreboot.
+ We can pass firmware information through FFI, such as ACPI.
+
config CC_HAVE_STACKPROTECTOR_TLS
def_bool $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=tp -mstack-protector-guard-offset=0)
diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index f71ce21ff684..f9d1625dd159 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -15,6 +15,8 @@
/* Basic configuration for ACPI */
#ifdef CONFIG_ACPI
+#include <asm/ffi.h>
+
typedef u64 phys_cpuid_t;
#define PHYS_CPUID_INVALID INVALID_HARTID
@@ -66,6 +68,13 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
unsigned int cpu, const char **isa);
static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
+
+#define ACPI_HAVE_ARCH_GET_ROOT_POINTER
+static inline u64 acpi_arch_get_root_pointer(void)
+{
+ return acpi_rsdp;
+}
+
#else
static inline void acpi_init_rintc_map(void) { }
static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
diff --git a/arch/riscv/include/asm/ffi.h b/arch/riscv/include/asm/ffi.h
new file mode 100644
index 000000000000..847af02abd87
--- /dev/null
+++ b/arch/riscv/include/asm/ffi.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _ASM_FFI_H
+#define _ASM_FFI_H
+
+extern u64 acpi_rsdp;
+extern void ffi_init(void);
+
+#endif /* _ASM_FFI_H */
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index 506cc4a9a45a..274e06f4da33 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -92,6 +92,7 @@ obj-$(CONFIG_CRASH_CORE) += crash_core.o
obj-$(CONFIG_JUMP_LABEL) += jump_label.o
obj-$(CONFIG_EFI) += efi.o
+obj-$(CONFIG_FFI) += ffi.o
obj-$(CONFIG_COMPAT) += compat_syscall_table.o
obj-$(CONFIG_COMPAT) += compat_signal.o
obj-$(CONFIG_COMPAT) += compat_vdso/
diff --git a/arch/riscv/kernel/ffi.c b/arch/riscv/kernel/ffi.c
new file mode 100644
index 000000000000..c5ac2b5d9148
--- /dev/null
+++ b/arch/riscv/kernel/ffi.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ffi.c - FDT FIRMWARE INTERFACE
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/libfdt.h>
+
+u64 acpi_rsdp;
+
+void __init ffi_acpi_root_pointer(void)
+{
+ int cfgtbl, len;
+ fdt64_t *prop;
+
+ cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
+ if (cfgtbl < 0) {
+ pr_info("firmware table not found.\n");
+ return;
+ }
+
+ prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi_phy_ptr", &len);
+ if (!prop || len != sizeof(u64))
+ pr_info("acpi_rsdp not found.\n");
+ else
+ acpi_rsdp = fdt64_to_cpu(*prop);
+
+ pr_debug("acpi rsdp: %llx\n", acpi_rsdp);
+}
+
+void __init ffi_init(void)
+{
+ ffi_acpi_root_pointer();
+}
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 971fe776e2f8..5a933d6b6acb 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -36,6 +36,7 @@
#include <asm/thread_info.h>
#include <asm/kasan.h>
#include <asm/efi.h>
+#include <asm/ffi.h>
#include "head.h"
@@ -279,6 +280,7 @@ void __init setup_arch(char **cmdline_p)
parse_early_param();
efi_init();
+ ffi_init();
paging_init();
/* Parse the ACPI tables for possible boot-time configuration */
--
2.20.1
1. Some bootloaders do not support EFI, and the transfer of
firmware information can only be done through DTS,
such as Coreboot.
2. Some arches do not have a reserved address segment that
can be used to pass firmware parameters like x86.
3. Based on this, we have added an interface to obtain firmware
information through FDT, named FFI.
4. We not only use FFI to pass SMBIOS entry,
but also pass other firmware information as an extension.
Signed-off-by: Yunhui Cui <[email protected]>
---
MAINTAINERS | 6 ++
drivers/firmware/Kconfig | 11 ++++
drivers/firmware/Makefile | 1 +
drivers/firmware/dmi_scan.c | 128 +++++++++++++++++-------------------
drivers/firmware/ffi.c | 36 ++++++++++
include/linux/ffi.h | 15 +++++
6 files changed, 128 insertions(+), 69 deletions(-)
create mode 100644 drivers/firmware/ffi.c
create mode 100644 include/linux/ffi.h
diff --git a/MAINTAINERS b/MAINTAINERS
index e592f489e757..9b886ef36587 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7871,6 +7871,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/firmware/Kconfig b/drivers/firmware/Kconfig
index b59e3041fd62..ea0149fb4683 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -303,6 +303,17 @@ 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 EFI, 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 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..64c1ceffe373 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>
@@ -628,31 +629,56 @@ static int __init dmi_present(const u8 *buf)
}
/*
- * Check for the SMBIOS 3.0 64-bit entry point signature. Unlike the legacy
- * 32-bit entry point, there is no embedded DMI header (_DMI_) in here.
+ * 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_smbios3_present(const u8 *buf)
+static int __init dmi_sacn_smbios(unsigned long smbios3, unsigned long smbios)
{
- if (memcmp(buf, "_SM3_", 5) == 0 &&
- buf[6] >= 24 && buf[6] <= 32 &&
- dmi_checksum(buf, buf[6])) {
- dmi_ver = get_unaligned_be24(buf + 7);
- dmi_num = 0; /* No longer specified */
- dmi_len = get_unaligned_le32(buf + 12);
- dmi_base = get_unaligned_le64(buf + 16);
- smbios_entry_point_size = buf[6];
- memcpy(smbios_entry_point, buf, smbios_entry_point_size);
+ char __iomem *p;
+ char buf[32];
+ #define INVALID_TABLE_ADDR (~0UL)
- if (dmi_walk_early(dmi_decode) == 0) {
- pr_info("SMBIOS %d.%d.%d present.\n",
- dmi_ver >> 16, (dmi_ver >> 8) & 0xFF,
- dmi_ver & 0xFF);
- dmi_format_ids(dmi_ids_string, sizeof(dmi_ids_string));
- pr_info("DMI: %s\n", dmi_ids_string);
+ if (smbios3 != INVALID_TABLE_ADDR) {
+ p = dmi_early_remap(smbios3, 32);
+ if (p == NULL)
+ return -1;
+ memcpy_fromio(buf, p, 32);
+ dmi_early_unmap(p, 32);
+
+ if (!dmi_smbios3_present(buf)) {
+ dmi_available = 1;
return 0;
}
}
- return 1;
+
+ 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)
@@ -660,58 +686,22 @@ static void __init dmi_scan_machine(void)
char __iomem *p, *q;
char buf[32];
+#ifdef CONFIG_FDT_FW_INTERFACE
+ if (dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
+ goto error;
+#endif
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)
+ if (dmi_sacn_smbios(efi.smbios3, efi.smbios))
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 (p == NULL)
- goto error;
- memcpy_fromio(buf, p, 32);
- dmi_early_unmap(p, 32);
-
- if (!dmi_present(buf)) {
- dmi_available = 1;
- return;
- }
} else if (IS_ENABLED(CONFIG_DMI_SCAN_MACHINE_NON_EFI_FALLBACK)) {
p = dmi_early_remap(SMBIOS_ENTRY_POINT_SCAN_START, 0x10000);
if (p == NULL)
goto error;
/*
- * Same logic as above, look for a 64-bit entry point
- * first, and if not found, fall back to 32-bit entry point.
- */
+ * Same logic as above, look for a 64-bit entry point
+ * first, and if not found, fall back to 32-bit entry point.
+ */
memcpy_fromio(buf, p, 16);
for (q = p + 16; q < p + 0x10000; q += 16) {
memcpy_fromio(buf + 16, q, 16);
@@ -724,12 +714,12 @@ static void __init dmi_scan_machine(void)
}
/*
- * Iterate over all possible DMI header addresses q.
- * Maintain the 32 bytes around q in buf. On the
- * first iteration, substitute zero for the
- * out-of-range bytes so there is no chance of falsely
- * detecting an SMBIOS header.
- */
+ * Iterate over all possible DMI header addresses q.
+ * Maintain the 32 bytes around q in buf. On the
+ * first iteration, substitute zero for the
+ * out-of-range bytes so there is no chance of falsely
+ * detecting an SMBIOS header.
+ */
memset(buf, 0, 16);
for (q = p; q < p + 0x10000; q += 16) {
memcpy_fromio(buf + 16, q, 16);
diff --git a/drivers/firmware/ffi.c b/drivers/firmware/ffi.c
new file mode 100644
index 000000000000..169802b4a7a8
--- /dev/null
+++ b/drivers/firmware/ffi.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/libfdt.h>
+#include <linux/ffi.h>
+
+#define FFI_INVALID_TABLE_ADDR (~0UL)
+
+struct ffi __read_mostly ffi = {
+ .smbios = FFI_INVALID_TABLE_ADDR,
+ .smbios3 = FFI_INVALID_TABLE_ADDR,
+};
+EXPORT_SYMBOL(ffi);
+
+void __init ffi_smbios_root_pointer(void)
+{
+ int cfgtbl, len;
+ fdt64_t *prop;
+
+ cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
+ if (cfgtbl < 0) {
+ pr_info("firmware table not found.\n");
+ return;
+ }
+ prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios_phy_ptr", &len);
+ if (!prop || len != sizeof(u64))
+ pr_info("smbios entry point not found.\n");
+ else
+ ffi.smbios = fdt64_to_cpu(*prop);
+
+ pr_info("smbios root pointer: %lx\n", ffi.smbios);
+}
+
diff --git a/include/linux/ffi.h b/include/linux/ffi.h
new file mode 100644
index 000000000000..95298a805222
--- /dev/null
+++ b/include/linux/ffi.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_FFI_H
+#define _LINUX_FFI_H
+
+extern struct ffi {
+ unsigned long smbios; /* SMBIOS table (32 bit entry point) */
+ unsigned long smbios3; /* SMBIOS table (64 bit entry point) */
+ unsigned long flags;
+
+} ffi;
+
+void ffi_smbios_root_pointer(void);
+
+#endif /* _LINUX_FFI_H */
--
2.20.1
When we bringup with coreboot on riscv, we need to obtain
the entry address of SMBIOS through the FFI scheme.
Signed-off-by: Yunhui Cui <[email protected]>
---
arch/riscv/kernel/ffi.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/riscv/kernel/ffi.c b/arch/riscv/kernel/ffi.c
index c5ac2b5d9148..c44f943a1cb5 100644
--- a/arch/riscv/kernel/ffi.c
+++ b/arch/riscv/kernel/ffi.c
@@ -8,6 +8,7 @@
#include <linux/of.h>
#include <linux/of_fdt.h>
#include <linux/libfdt.h>
+#include <linux/ffi.h>
u64 acpi_rsdp;
@@ -34,4 +35,7 @@ void __init ffi_acpi_root_pointer(void)
void __init ffi_init(void)
{
ffi_acpi_root_pointer();
+#if CONFIG_FDT_FW_INTERFACE
+ ffi_smbios_root_pointer();
+#endif
}
--
2.20.1
Hey,
On Sun, Jul 02, 2023 at 05:57:34PM +0800, Yunhui Cui wrote:
> When we bringup with coreboot on riscv, we need to obtain
> the entry address of SMBIOS through the FFI scheme.
What do you need it for?
>
> Signed-off-by: Yunhui Cui <[email protected]>
> ---
> arch/riscv/kernel/ffi.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/riscv/kernel/ffi.c b/arch/riscv/kernel/ffi.c
> index c5ac2b5d9148..c44f943a1cb5 100644
> --- a/arch/riscv/kernel/ffi.c
> +++ b/arch/riscv/kernel/ffi.c
> @@ -8,6 +8,7 @@
> #include <linux/of.h>
> #include <linux/of_fdt.h>
> #include <linux/libfdt.h>
> +#include <linux/ffi.h>
>
> u64 acpi_rsdp;
>
> @@ -34,4 +35,7 @@ void __init ffi_acpi_root_pointer(void)
> void __init ffi_init(void)
> {
> ffi_acpi_root_pointer();
> +#if CONFIG_FDT_FW_INTERFACE
> + ffi_smbios_root_pointer();
> +#endif
Please stub this function so that we don't need ifdeffery here.
Cheers,
Conor.
Hey,
On Sun, Jul 02, 2023 at 05:57:33PM +0800, Yunhui Cui wrote:
> 1. Some bootloaders do not support EFI, and the transfer of
> firmware information can only be done through DTS,
> such as Coreboot.
>
> 2. Some arches do not have a reserved address segment that
> can be used to pass firmware parameters like x86.
>
> 3. Based on this, we have added an interface to obtain firmware
> information through FDT, named FFI.
>
> 4. We not only use FFI to pass SMBIOS entry,
> but also pass other firmware information as an extension.
nit: please don't write your commit messages as bullet lists
> +FDT FIRMWARE INTERFACE (FFI)
> +M: Yunhui Cui [email protected]
> +S: Maintained
> +F: drivers/firmware/ffi.c
> +F: include/linux/ffi.h
Are you going to apply patches for this, or is someone else?
> EXTERNAL CONNECTOR SUBSYSTEM (EXTCON)
> M: MyungJoo Ham <[email protected]>
> M: Chanwoo Choi <[email protected]>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index b59e3041fd62..ea0149fb4683 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -303,6 +303,17 @@ 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 EFI, 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 smbios tables.
Could you express this dependency on !SMBIOS_ENTRY_POINT_SCAN_START in
Kconfig & then simply the text to:
"Enable this option to support the transfer of firmware information,
such as smbios tables, for bootloaders that do not support EFI."
since it would not even appear if the arch supports scanning for the
entry point?
If I was was a punter trying to configure my kernel in menuconfig or
whatever, I should be able to decide based on the help text if I need
this, not going grepping for #defines in headers.
> static void __init dmi_scan_machine(void)
> @@ -660,58 +686,22 @@ static void __init dmi_scan_machine(void)
> char __iomem *p, *q;
> char buf[32];
>
> +#ifdef CONFIG_FDT_FW_INTERFACE
> + if (dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
"dmi_sacn_smbios"
> + goto error;
> +#endif
Does this not mean that if FDT_FW_INTERFACE is enabled, but the platform
wants to use EFI, it won't be able to? The `goto error;` makes this look
mutually exclusive to my efi-unaware eyes.
> 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)
> + if (dmi_sacn_smbios(efi.smbios3, efi.smbios))
> 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 (p == NULL)
> - goto error;
> - memcpy_fromio(buf, p, 32);
> - dmi_early_unmap(p, 32);
> -
> - if (!dmi_present(buf)) {
> - dmi_available = 1;
> - return;
> - }
> diff --git a/drivers/firmware/ffi.c b/drivers/firmware/ffi.c
> new file mode 100644
> index 000000000000..169802b4a7a8
> --- /dev/null
> +++ b/drivers/firmware/ffi.c
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/libfdt.h>
> +#include <linux/ffi.h>
> +
> +#define FFI_INVALID_TABLE_ADDR (~0UL)
> +
> +struct ffi __read_mostly ffi = {
> + .smbios = FFI_INVALID_TABLE_ADDR,
> + .smbios3 = FFI_INVALID_TABLE_ADDR,
> +};
> +EXPORT_SYMBOL(ffi);
> +// SPDX-License-Identifier: GPL-2.0-only
Why not EXPORT_SYMBOL_GPL? But also, who is the user of this export?
> +
> +void __init ffi_smbios_root_pointer(void)
> +{
> + int cfgtbl, len;
> + fdt64_t *prop;
> +
> + cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
These DT properties need to be documented in a binding.
> + if (cfgtbl < 0) {
> + pr_info("firmware table not found.\n");
Isn't it perfectly valid for a DT not to contain this table? This print
should be, at the very least, a pr_debug().
> + return;
> + }
> + prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios_phy_ptr", &len);
Again, undocumented DT property. Please document them in a binding.
> + if (!prop || len != sizeof(u64))
> + pr_info("smbios entry point not found.\n");
> + else
> + ffi.smbios = fdt64_to_cpu(*prop);
> +
> + pr_info("smbios root pointer: %lx\n", ffi.smbios);
ffi.smbios is not set if (!prop || len != sizeof(u64)), looks like your
"if" should return and the contents of the else become unconditional?
Otherwise, this print seems wrong.
> +}
> +
> diff --git a/include/linux/ffi.h b/include/linux/ffi.h
> new file mode 100644
> index 000000000000..95298a805222
> --- /dev/null
> +++ b/include/linux/ffi.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _LINUX_FFI_H
> +#define _LINUX_FFI_H
> +
> +extern struct ffi {
> + unsigned long smbios; /* SMBIOS table (32 bit entry point) */
> + unsigned long smbios3; /* SMBIOS table (64 bit entry point) */
> + unsigned long flags;
> +
> +} ffi;
> +
> +void ffi_smbios_root_pointer(void);
Please provide a stub for !FDT_FW_INTERFACE so that we don't need
ifdeffery at callsites.
Cheers,
Conor.
Hey,
%subject: riscv: obtain ACPI RSDP from FFI.
This subject is a bit unhelpful because FFI would commonly mean "foreign
function interface" & you have not yet introduced it. It seems like it
would be better to do s/FFI/devicetree/ or similar.
Please also drop the full stop from the commit messages ;)
Please use a cover letter for multi-patch series & include changelogs.
+CC Sunil, Alex:
Can you guys please take a look at this & see if it is something that we
want to do (ACPI without EFI)?
On Sun, Jul 02, 2023 at 05:57:32PM +0800, Yunhui Cui wrote:
> 1. We need to enable the ACPI function on RISC-V.
RISC-V already supports ACPI, the "we" in this commit message is
confusing. Who is "we"? Bytedance?
> When booting with
> Coreboot, we encounter two problems:
> a. Coreboot does not support EFI
> b. On RISC-V, only the DTS channel can be used.
We support ACPI, so this seems inaccurate. Could you explain it better
please?
> 2. Based on this, we have added an interface for obtaining firmware
> information transfer through FDT, named FFI.
Please use the long form of "FFI" before using the short form, since you
are inventing this & noone knows what it means yet.
> 3. We not only use FFI to pass ACPI RSDP, but also pass other
> firmware information as an extension.
This patch doesn't do that though?
> +RISC-V FDT FIRMWARE INTERFACE (FFI) SUPPORT
> +M: Yunhui Cui [email protected]
> +S: Maintained
> +F: arch/riscv/include/asm/ffi.h
> +F: arch/riscv/kernel/ffi.c
Please add this in alphabetical order, these entries have recently been
resorted. That said, maintainers entry for a trivial file in arch code
seems a wee bit odd, seems like it would be better suited rolled up into
your other entry for the interface, like how Ard's one looks for EFI?
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index b49793cf34eb..2e1c40fb2300 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -785,6 +785,16 @@ config EFI
> allow the kernel to be booted as an EFI application. This
> is only useful on systems that have UEFI firmware.
>
> +config FFI
> + bool "Fdt firmware interface"
> + depends on OF
> + default y
> + help
> + Added an interface to obtain firmware information transfer
> + through FDT, named FFI. Some bootloaders do not support EFI,
> + such as coreboot.
> + We can pass firmware information through FFI, such as ACPI.
I don't understand your Kconfig setup. Why don't you just have one
option (the one from patch 2/3), instead of adding 2 different but
similarly named options?
> config CC_HAVE_STACKPROTECTOR_TLS
> def_bool $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=tp -mstack-protector-guard-offset=0)
>
> diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> index f71ce21ff684..f9d1625dd159 100644
> --- a/arch/riscv/include/asm/acpi.h
> +++ b/arch/riscv/include/asm/acpi.h
> @@ -15,6 +15,8 @@
> /* Basic configuration for ACPI */
> #ifdef CONFIG_ACPI
>
> +#include <asm/ffi.h>
> +
> typedef u64 phys_cpuid_t;
> #define PHYS_CPUID_INVALID INVALID_HARTID
>
> @@ -66,6 +68,13 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
> unsigned int cpu, const char **isa);
>
> static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> +
> +#define ACPI_HAVE_ARCH_GET_ROOT_POINTER
How come this is not set in Kconfig like HAVE_FOO options usually are?
> +static inline u64 acpi_arch_get_root_pointer(void)
> +{
> + return acpi_rsdp;
> +}
> +
> #else
> static inline void acpi_init_rintc_map(void) { }
> static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> diff --git a/arch/riscv/include/asm/ffi.h b/arch/riscv/include/asm/ffi.h
> new file mode 100644
> index 000000000000..847af02abd87
> --- /dev/null
> +++ b/arch/riscv/include/asm/ffi.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _ASM_FFI_H
> +#define _ASM_FFI_H
> +
> +extern u64 acpi_rsdp;
/stuff/linux/drivers/acpi/osl.c:178:22: error: redefinition of 'acpi_rsdp' with a different type: 'unsigned long' vs 'u64' (aka 'unsigned long long')
Fails to build when Kexec is enabled.
> +extern void ffi_init(void);
> +
> +#endif /* _ASM_FFI_H */
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> index 506cc4a9a45a..274e06f4da33 100644
> --- a/arch/riscv/kernel/Makefile
> +++ b/arch/riscv/kernel/Makefile
> @@ -92,6 +92,7 @@ obj-$(CONFIG_CRASH_CORE) += crash_core.o
> obj-$(CONFIG_JUMP_LABEL) += jump_label.o
>
> obj-$(CONFIG_EFI) += efi.o
> +obj-$(CONFIG_FFI) += ffi.o
This file uses tabs for alignment, not spaces.
> obj-$(CONFIG_COMPAT) += compat_syscall_table.o
> obj-$(CONFIG_COMPAT) += compat_signal.o
> obj-$(CONFIG_COMPAT) += compat_vdso/
> diff --git a/arch/riscv/kernel/ffi.c b/arch/riscv/kernel/ffi.c
> new file mode 100644
> index 000000000000..c5ac2b5d9148
> --- /dev/null
> +++ b/arch/riscv/kernel/ffi.c
> @@ -0,0 +1,37 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ffi.c - FDT FIRMWARE INTERFACE
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/libfdt.h>
> +
> +u64 acpi_rsdp;
> +
> +void __init ffi_acpi_root_pointer(void)
> +{
> + int cfgtbl, len;
> + fdt64_t *prop;
> +
> + cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
> + if (cfgtbl < 0) {
> + pr_info("firmware table not found.\n");
> + return;
> + }
> +
> + prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi_phy_ptr", &len);
> + if (!prop || len != sizeof(u64))
> + pr_info("acpi_rsdp not found.\n");
> + else
> + acpi_rsdp = fdt64_to_cpu(*prop);
> +
> + pr_debug("acpi rsdp: %llx\n", acpi_rsdp);
Same comments here about undocumented DT properties and pr_*()s that
likely are not wanted (or correct).
> +}
> +
> +void __init ffi_init(void)
> +{
> + ffi_acpi_root_pointer();
What happens if, on a system with "normal" ACPI support, ffi_init() is
called & ffi_acpi_root_pointer() calls things like fdt_path_offset()?
> +}
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 971fe776e2f8..5a933d6b6acb 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -36,6 +36,7 @@
> #include <asm/thread_info.h>
> #include <asm/kasan.h>
> #include <asm/efi.h>
> +#include <asm/ffi.h>
>
> #include "head.h"
>
> @@ -279,6 +280,7 @@ void __init setup_arch(char **cmdline_p)
> parse_early_param();
>
> efi_init();
> + ffi_init();
What provides ffi_init() if CONFIG_FFI is disabled?
> paging_init();
>
> /* Parse the ACPI tables for possible boot-time configuration */
Cheers,
Conor.
Hi Yunhui Cui,
On Sun, Jul 02, 2023 at 02:47:41PM +0100, Conor Dooley wrote:
> Hey,
> %subject: riscv: obtain ACPI RSDP from FFI.
>
> This subject is a bit unhelpful because FFI would commonly mean "foreign
> function interface" & you have not yet introduced it. It seems like it
> would be better to do s/FFI/devicetree/ or similar.
> Please also drop the full stop from the commit messages ;)
>
> Please use a cover letter for multi-patch series & include changelogs.
>
> +CC Sunil, Alex:
>
> Can you guys please take a look at this & see if it is something that we
> want to do (ACPI without EFI)?
>
We have supported ACPI only with UEFI. The current booting contract
between firmware and OS is to pass only one of DT or ACPI, not both.
This approach brings another booting contract for Linux mixing ACPI and
DT which affects RVI specs. As per policy and since it can affect
multiple OSs, a frozen RVI spec is required for taking this patch into
linux. So, could you please bring this topic for discussion in [1] and
get agreement?
Isn't it simpler to provide a minimum UEFI configuration table and
stubbed BS/RS?
Have you done a PoC? I am curious how do you handle EFI memory map
dependencies.
In case this is approved, I am wondering why do we need new FFI?
[1] - https://lists.riscv.org/g/tech-brs
Thanks!
Sunil
Hi Sunil,
On Mon, Jul 3, 2023 at 12:22 PM Sunil V L <[email protected]> wrote:
>
> Hi Yunhui Cui,
> On Sun, Jul 02, 2023 at 02:47:41PM +0100, Conor Dooley wrote:
> > Hey,
> > %subject: riscv: obtain ACPI RSDP from FFI.
> >
> > This subject is a bit unhelpful because FFI would commonly mean "foreign
> > function interface" & you have not yet introduced it. It seems like it
> > would be better to do s/FFI/devicetree/ or similar.
> > Please also drop the full stop from the commit messages ;)
> >
> > Please use a cover letter for multi-patch series & include changelogs.
> >
> > +CC Sunil, Alex:
> >
> > Can you guys please take a look at this & see if it is something that we
> > want to do (ACPI without EFI)?
> >
>
> We have supported ACPI only with UEFI. The current booting contract
> between firmware and OS is to pass only one of DT or ACPI, not both.
> This approach brings another booting contract for Linux mixing ACPI and
> DT which affects RVI specs. As per policy and since it can affect
> multiple OSs, a frozen RVI spec is required for taking this patch into
> linux. So, could you please bring this topic for discussion in [1] and
> get agreement?
>
> Isn't it simpler to provide a minimum UEFI configuration table and
> stubbed BS/RS?
>
> Have you done a PoC? I am curious how do you handle EFI memory map
> dependencies.
Yes, Poc has been completed.
a memory node in DTS can solve it.
>
> In case this is approved, I am wondering why do we need new FFI?
>
> [1] - https://lists.riscv.org/g/tech-brs
We have discussed with Ard and Ron many times about the series of
questions you mentioned above, and reached a consensus.
Please see the v1:
https://patches.linaro.org/project/linux-acpi/patch/[email protected]/
Thanks,
Yunhui
Hi, Conor
On Sun, Jul 2, 2023 at 9:48 PM Conor Dooley <[email protected]> wrote:
>
> Hey,
> %subject: riscv: obtain ACPI RSDP from FFI.
>
> This subject is a bit unhelpful because FFI would commonly mean "foreign
> function interface" & you have not yet introduced it. It seems like it
> would be better to do s/FFI/devicetree/ or similar.
FFI: FDT FIRMWARE INTERFACE.
You are right, s/FFI/devicetree/ is of course possible， but I actually
want to use FFI as a general solution, put all relevant codes under
driver/firmware/, and use RISC-V arch to call general codes.
In this case, only one Kconfig CONFIG_FDT_FW_INTERFACE is enough， and
The FFI code will be placed first in the patchset.
But Ard's suggestion is to put the part of SMBIOS in the generic code,
and put the FFI for ACPI in the RISCV arch.
Please see the v1:
https://patches.linaro.org/project/linux-acpi/patch/[email protected]/
Put the following to /driver/firmware/ffi.c , What do you think?
void __init ffi_acpi_root_pointer(void)
{
...
}
> Please also drop the full stop from the commit messages ;)
Okay, thanks.
>
> Please use a cover letter for multi-patch series & include changelogs.
OK, On v3 I would use.
>
> +CC Sunil, Alex:
>
> Can you guys please take a look at this & see if it is something that we
> want to do (ACPI without EFI)?
>
> On Sun, Jul 02, 2023 at 05:57:32PM +0800, Yunhui Cui wrote:
> > 1. We need to enable the ACPI function on RISC-V.
>
> RISC-V already supports ACPI, the "we" in this commit message is
> confusing. Who is "we"? Bytedance?
>
> > When booting with
> > Coreboot, we encounter two problems:
> > a. Coreboot does not support EFI
>
>
> > b. On RISC-V, only the DTS channel can be used.
>
> We support ACPI, so this seems inaccurate. Could you explain it better
> please?
Yes, Sunil already supports ACPI, But it is based on EDK2 boot which
supports EFI.
In fact, We use Coreboot which has the features of a and b above.
>
> > 2. Based on this, we have added an interface for obtaining firmware
> > information transfer through FDT, named FFI.
>
> Please use the long form of "FFI" before using the short form, since you
> are inventing this & noone knows what it means yet.
>
> > 3. We not only use FFI to pass ACPI RSDP, but also pass other
> > firmware information as an extension.
>
> This patch doesn't do that though?
Similar problems may be encountered on other arches, which is also the
purpose of this sentence.
> > +RISC-V FDT FIRMWARE INTERFACE (FFI) SUPPORT
> > +M: Yunhui Cui [email protected]
> > +S: Maintained
> > +F: arch/riscv/include/asm/ffi.h
> > +F: arch/riscv/kernel/ffi.c
>
> Please add this in alphabetical order, these entries have recently been
> resorted. That said, maintainers entry for a trivial file in arch code
> seems a wee bit odd, seems like it would be better suited rolled up into
> your other entry for the interface, like how Ard's one looks for EFI?
>
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index b49793cf34eb..2e1c40fb2300 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -785,6 +785,16 @@ config EFI
> > allow the kernel to be booted as an EFI application. This
> > is only useful on systems that have UEFI firmware.
> >
> > +config FFI
> > + bool "Fdt firmware interface"
> > + depends on OF
> > + default y
> > + help
> > + Added an interface to obtain firmware information transfer
> > + through FDT, named FFI. Some bootloaders do not support EFI,
> > + such as coreboot.
> > + We can pass firmware information through FFI, such as ACPI.
>
> I don't understand your Kconfig setup. Why don't you just have one
> option (the one from patch 2/3), instead of adding 2 different but
> similarly named options?
OK, let me try it， and use the Kconfig CONFIG_FDT_FW_INTERFACE. EFI
seems to use two...
> > config CC_HAVE_STACKPROTECTOR_TLS
> > def_bool $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=tp -mstack-protector-guard-offset=0)
> >
> > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > index f71ce21ff684..f9d1625dd159 100644
> > --- a/arch/riscv/include/asm/acpi.h
> > +++ b/arch/riscv/include/asm/acpi.h
> > @@ -15,6 +15,8 @@
> > /* Basic configuration for ACPI */
> > #ifdef CONFIG_ACPI
> >
> > +#include <asm/ffi.h>
> > +
> > typedef u64 phys_cpuid_t;
> > #define PHYS_CPUID_INVALID INVALID_HARTID
> >
> > @@ -66,6 +68,13 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
> > unsigned int cpu, const char **isa);
> >
> > static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> > +
> > +#define ACPI_HAVE_ARCH_GET_ROOT_POINTER
>
> How come this is not set in Kconfig like HAVE_FOO options usually are?
This is modeled after x86 historical code.
See arch/x86/include/asm/acpi.h
> > +static inline u64 acpi_arch_get_root_pointer(void)
> > +{
> > + return acpi_rsdp;
> > +}
> > +
> > #else
> > static inline void acpi_init_rintc_map(void) { }
> > static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> > diff --git a/arch/riscv/include/asm/ffi.h b/arch/riscv/include/asm/ffi.h
> > new file mode 100644
> > index 000000000000..847af02abd87
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/ffi.h
> > @@ -0,0 +1,9 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _ASM_FFI_H
> > +#define _ASM_FFI_H
> > +
> > +extern u64 acpi_rsdp;
>
> /stuff/linux/drivers/acpi/osl.c:178:22: error: redefinition of 'acpi_rsdp' with a different type: 'unsigned long' vs 'u64' (aka 'unsigned long long')
>
> Fails to build when Kexec is enabled.
Rename my acpi_rsdp to arch_acpi_rsdp? WDYT?
>
> > +extern void ffi_init(void);
> > +
> > +#endif /* _ASM_FFI_H */
> > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > index 506cc4a9a45a..274e06f4da33 100644
> > --- a/arch/riscv/kernel/Makefile
> > +++ b/arch/riscv/kernel/Makefile
> > @@ -92,6 +92,7 @@ obj-$(CONFIG_CRASH_CORE) += crash_core.o
> > obj-$(CONFIG_JUMP_LABEL) += jump_label.o
> >
> > obj-$(CONFIG_EFI) += efi.o
> > +obj-$(CONFIG_FFI) += ffi.o
>
> This file uses tabs for alignment, not spaces.
Okay, got it.
>
> > obj-$(CONFIG_COMPAT) += compat_syscall_table.o
> > obj-$(CONFIG_COMPAT) += compat_signal.o
> > obj-$(CONFIG_COMPAT) += compat_vdso/
> > diff --git a/arch/riscv/kernel/ffi.c b/arch/riscv/kernel/ffi.c
> > new file mode 100644
> > index 000000000000..c5ac2b5d9148
> > --- /dev/null
> > +++ b/arch/riscv/kernel/ffi.c
> > @@ -0,0 +1,37 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * ffi.c - FDT FIRMWARE INTERFACE
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_fdt.h>
> > +#include <linux/libfdt.h>
> > +
> > +u64 acpi_rsdp;
> > +
> > +void __init ffi_acpi_root_pointer(void)
> > +{
> > + int cfgtbl, len;
> > + fdt64_t *prop;
> > +
> > + cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
> > + if (cfgtbl < 0) {
> > + pr_info("firmware table not found.\n");
> > + return;
> > + }
> > +
> > + prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi_phy_ptr", &len);
> > + if (!prop || len != sizeof(u64))
> > + pr_info("acpi_rsdp not found.\n");
> > + else
> > + acpi_rsdp = fdt64_to_cpu(*prop);
> > +
> > + pr_debug("acpi rsdp: %llx\n", acpi_rsdp);
>
> Same comments here about undocumented DT properties and pr_*()s that
> likely are not wanted (or correct).
Okay，update it on v3.
>
> > +}
> > +
> > +void __init ffi_init(void)
> > +{
> > + ffi_acpi_root_pointer();
>
> What happens if, on a system with "normal" ACPI support, ffi_init() is
> called & ffi_acpi_root_pointer() calls things like fdt_path_offset()?
According to the current logic, get it from FFI is enabled, if can
not, continue to use “normal” ACPI.
> > +}
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index 971fe776e2f8..5a933d6b6acb 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -36,6 +36,7 @@
> > #include <asm/thread_info.h>
> > #include <asm/kasan.h>
> > #include <asm/efi.h>
> > +#include <asm/ffi.h>
> >
> > #include "head.h"
> >
> > @@ -279,6 +280,7 @@ void __init setup_arch(char **cmdline_p)
> > parse_early_param();
> >
> > efi_init();
> > + ffi_init();
>
> What provides ffi_init() if CONFIG_FFI is disabled?
Ok， Modified on v3, put it with the CONFIG_FFI
>
> > paging_init();
> >
> > /* Parse the ACPI tables for possible boot-time configuration */
>
> Cheers,
> Conor.
Thanks,
Yunhui
Hi Conor,
On Sun, Jul 2, 2023 at 8:42 PM Conor Dooley <[email protected]> wrote:
>
> Hey,
>
> On Sun, Jul 02, 2023 at 05:57:34PM +0800, Yunhui Cui wrote:
> > When we bringup with coreboot on riscv, we need to obtain
> > the entry address of SMBIOS through the FFI scheme.
>
> What do you need it for?
RISC-V will be server-oriented, and the system needs to be managed
based on SMBIOS.
> >
> > @@ -34,4 +35,7 @@ void __init ffi_acpi_root_pointer(void)
> > void __init ffi_init(void)
> > {
> > ffi_acpi_root_pointer();
> > +#if CONFIG_FDT_FW_INTERFACE
> > + ffi_smbios_root_pointer();
> > +#endif
>
> Please stub this function so that we don't need ifdeffery here.
OK, I will update it on V3.
Thanks,
Yunhui
Hey,
On Mon, Jul 03, 2023 at 03:19:01PM +0800, 运辉崔 wrote:
> On Sun, Jul 2, 2023 at 9:48 PM Conor Dooley <[email protected]> wrote:
> >
> > %subject: riscv: obtain ACPI RSDP from FFI.
> >
> > This subject is a bit unhelpful because FFI would commonly mean "foreign
> > function interface" & you have not yet introduced it. It seems like it
> > would be better to do s/FFI/devicetree/ or similar.
>
> FFI: FDT FIRMWARE INTERFACE.
>
> You are right, s/FFI/devicetree/ is of course possible， but I actually
> want to use FFI as a general solution, put all relevant codes under
> driver/firmware/, and use RISC-V arch to call general codes.
Yes, I read the patchset. It's still unhelpful to someone reading
$subject because nobody knows what your version of FFI is IMO.
> In this case, only one Kconfig CONFIG_FDT_FW_INTERFACE is enough， and
> The FFI code will be placed first in the patchset.
>
> But Ard's suggestion is to put the part of SMBIOS in the generic code,
> and put the FFI for ACPI in the RISCV arch.
>
> Please see the v1:
> https://patches.linaro.org/project/linux-acpi/patch/[email protected]/
I read this too, I was following along with the discussion on the v1.
> Put the following to /driver/firmware/ffi.c , What do you think?
> void __init ffi_acpi_root_pointer(void)
> {
> ...
> }
Usually the NOP versions just go in the headers.
> > Please also drop the full stop from the commit messages ;)
> Okay, thanks.
>
> >
> > Please use a cover letter for multi-patch series & include changelogs.
> OK, On v3 I would use.
>
> >
> > +CC Sunil, Alex:
> >
> > Can you guys please take a look at this & see if it is something that we
> > want to do (ACPI without EFI)?
> >
> > On Sun, Jul 02, 2023 at 05:57:32PM +0800, Yunhui Cui wrote:
> > > 1. We need to enable the ACPI function on RISC-V.
> >
> > RISC-V already supports ACPI, the "we" in this commit message is
> > confusing. Who is "we"? Bytedance?
Who is the "we"?
> > > When booting with
> > > Coreboot, we encounter two problems:
> > > a. Coreboot does not support EFI
> >
> >
> > > b. On RISC-V, only the DTS channel can be used.
> >
> > We support ACPI, so this seems inaccurate. Could you explain it better
> > please?
>
> Yes, Sunil already supports ACPI, But it is based on EDK2 boot which
> supports EFI.
> In fact, We use Coreboot which has the features of a and b above.
My point is that the commit message has gaps in it.
This point b & point 1 make it seem like this patch adds ACPI support to
an architecture that only supports devicetree. "DTS channel" needs to be
explained further, to be frank I have no idea what that means. Does it
mean that coreboot on RISC-V only supports DT, or that the RISC-V linux
kernel requires a mini-DT when booting with EFI?
> > > 2. Based on this, we have added an interface for obtaining firmware
> > > information transfer through FDT, named FFI.
> >
> > Please use the long form of "FFI" before using the short form, since you
> > are inventing this & noone knows what it means yet.
> >
> > > 3. We not only use FFI to pass ACPI RSDP, but also pass other
> > > firmware information as an extension.
> >
> > This patch doesn't do that though?
>
> Similar problems may be encountered on other arches, which is also the
> purpose of this sentence.
Right, but that has nothing to do with this patch? This patch only
implements the ACPI side of things for RISC-V, it doesn't do the SMBIOS
stuff. Leave that for the patch that actually does that please.
> > > +RISC-V FDT FIRMWARE INTERFACE (FFI) SUPPORT
> > > +M: Yunhui Cui [email protected]
> > > +S: Maintained
> > > +F: arch/riscv/include/asm/ffi.h
> > > +F: arch/riscv/kernel/ffi.c
> >
> > Please add this in alphabetical order, these entries have recently been
> > resorted. That said, maintainers entry for a trivial file in arch code
> > seems a wee bit odd, seems like it would be better suited rolled up into
> > your other entry for the interface, like how Ard's one looks for EFI?
> >
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index b49793cf34eb..2e1c40fb2300 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -785,6 +785,16 @@ config EFI
> > > allow the kernel to be booted as an EFI application. This
> > > is only useful on systems that have UEFI firmware.
> > >
> > > +config FFI
> > > + bool "Fdt firmware interface"
> > > + depends on OF
> > > + default y
> > > + help
> > > + Added an interface to obtain firmware information transfer
> > > + through FDT, named FFI. Some bootloaders do not support EFI,
> > > + such as coreboot.
> > > + We can pass firmware information through FFI, such as ACPI.
> >
> > I don't understand your Kconfig setup. Why don't you just have one
> > option (the one from patch 2/3), instead of adding 2 different but
> > similarly named options?
> OK, let me try it， and use the Kconfig CONFIG_FDT_FW_INTERFACE. EFI
> seems to use two...
It doesn't use two different options, AFAIR. There's an EFI option in
the arch Kconfigs and then a menu in drivers/firmware/efi/Kconfig that
allows enabling sub-components. You've got two entries that appear
unrelated, despite parsing the same DT bits.
>
> > > config CC_HAVE_STACKPROTECTOR_TLS
> > > def_bool $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=tp -mstack-protector-guard-offset=0)
> > >
> > > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > > index f71ce21ff684..f9d1625dd159 100644
> > > --- a/arch/riscv/include/asm/acpi.h
> > > +++ b/arch/riscv/include/asm/acpi.h
> > > @@ -15,6 +15,8 @@
> > > /* Basic configuration for ACPI */
> > > #ifdef CONFIG_ACPI
> > >
> > > +#include <asm/ffi.h>
> > > +
> > > typedef u64 phys_cpuid_t;
> > > #define PHYS_CPUID_INVALID INVALID_HARTID
> > >
> > > @@ -66,6 +68,13 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
> > > unsigned int cpu, const char **isa);
> > >
> > > static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> > > +
> > > +#define ACPI_HAVE_ARCH_GET_ROOT_POINTER
> >
> > How come this is not set in Kconfig like HAVE_FOO options usually are?
> This is modeled after x86 historical code.
> See arch/x86/include/asm/acpi.h
Is that a good reason for propagating the pattern? Is there a benefit to
this, other than "x86 did this"?
> > > +static inline u64 acpi_arch_get_root_pointer(void)
> > > +{
> > > + return acpi_rsdp;
> > > +}
> > > +
> > > #else
> > > static inline void acpi_init_rintc_map(void) { }
> > > static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> > > diff --git a/arch/riscv/include/asm/ffi.h b/arch/riscv/include/asm/ffi.h
> > > new file mode 100644
> > > index 000000000000..847af02abd87
> > > --- /dev/null
> > > +++ b/arch/riscv/include/asm/ffi.h
> > > @@ -0,0 +1,9 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +
> > > +#ifndef _ASM_FFI_H
> > > +#define _ASM_FFI_H
> > > +
> > > +extern u64 acpi_rsdp;
> >
> > /stuff/linux/drivers/acpi/osl.c:178:22: error: redefinition of 'acpi_rsdp' with a different type: 'unsigned long' vs 'u64' (aka 'unsigned long long')
> >
> > Fails to build when Kexec is enabled.
>
> Rename my acpi_rsdp to arch_acpi_rsdp? WDYT?
You could do s/arch/riscv/ either, that'd match what we prefix a lot of
stuff with.
> > > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > > index 506cc4a9a45a..274e06f4da33 100644
> > > --- a/arch/riscv/kernel/Makefile
> > > +++ b/arch/riscv/kernel/Makefile
> > > @@ -92,6 +92,7 @@ obj-$(CONFIG_CRASH_CORE) += crash_core.o
> > > obj-$(CONFIG_JUMP_LABEL) += jump_label.o
> > >
> > > obj-$(CONFIG_EFI) += efi.o
> > > +obj-$(CONFIG_FFI) += ffi.o
> >
> > This file uses tabs for alignment, not spaces.
> Okay, got it.
>
> >
> > > obj-$(CONFIG_COMPAT) += compat_syscall_table.o
> > > obj-$(CONFIG_COMPAT) += compat_signal.o
> > > obj-$(CONFIG_COMPAT) += compat_vdso/
> > > diff --git a/arch/riscv/kernel/ffi.c b/arch/riscv/kernel/ffi.c
> > > new file mode 100644
> > > index 000000000000..c5ac2b5d9148
> > > --- /dev/null
> > > +++ b/arch/riscv/kernel/ffi.c
> > > +void __init ffi_init(void)
> > > +{
> > > + ffi_acpi_root_pointer();
> >
> > What happens if, on a system with "normal" ACPI support, ffi_init() is
> > called & ffi_acpi_root_pointer() calls things like fdt_path_offset()?
>
> According to the current logic, get it from FFI is enabled, if can
> not, continue to use “normal” ACPI.
I find it hard to understand what you mean here. Do you mean something
like "The calls to fdt_path_offset() will use the mini EFI DT and are
harmless. If the config table is not present, it will continue to use
\"normal\" ACPI."?
> > > +}
> > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > > index 971fe776e2f8..5a933d6b6acb 100644
> > > --- a/arch/riscv/kernel/setup.c
> > > +++ b/arch/riscv/kernel/setup.c
> > > @@ -36,6 +36,7 @@
> > > #include <asm/thread_info.h>
> > > #include <asm/kasan.h>
> > > #include <asm/efi.h>
> > > +#include <asm/ffi.h>
> > >
> > > #include "head.h"
> > >
> > > @@ -279,6 +280,7 @@ void __init setup_arch(char **cmdline_p)
> > > parse_early_param();
> > >
> > > efi_init();
> > > + ffi_init();
> >
> > What provides ffi_init() if CONFIG_FFI is disabled?
> Ok， Modified on v3, put it with the CONFIG_FFI
Sorry, what does this mean?
>
> >
> > > paging_init();
> > >
> > > /* Parse the ACPI tables for possible boot-time configuration */
Cheers,
Conor.
Hi Conor,
> nit: please don't write your commit messages as bullet lists
Okay, thanks for your suggestion.
> > +FDT FIRMWARE INTERFACE (FFI)
> > +M: Yunhui Cui [email protected]
> > +S: Maintained
> > +F: drivers/firmware/ffi.c
> > +F: include/linux/ffi.h
>
> Are you going to apply patches for this, or is someone else?
Yes, it will be used by patch 3/3.
>
> > EXTERNAL CONNECTOR SUBSYSTEM (EXTCON)
> > M: MyungJoo Ham <[email protected]>
> > M: Chanwoo Choi <[email protected]>
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index b59e3041fd62..ea0149fb4683 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -303,6 +303,17 @@ 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 EFI, 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 smbios tables.
>
> Could you express this dependency on !SMBIOS_ENTRY_POINT_SCAN_START in
> Kconfig & then simply the text to:
> "Enable this option to support the transfer of firmware information,
> such as smbios tables, for bootloaders that do not support EFI."
> since it would not even appear if the arch supports scanning for the
> entry point?
> If I was was a punter trying to configure my kernel in menuconfig or
> whatever, I should be able to decide based on the help text if I need
> this, not going grepping for #defines in headers.
Okay， I'll update on v3.
>
> > static void __init dmi_scan_machine(void)
> > @@ -660,58 +686,22 @@ static void __init dmi_scan_machine(void)
> > char __iomem *p, *q;
> > char buf[32];
> >
> > +#ifdef CONFIG_FDT_FW_INTERFACE
> > + if (dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
>
> "dmi_sacn_smbios"
>
> > + goto error;
> > +#endif
>
> Does this not mean that if FDT_FW_INTERFACE is enabled, but the platform
> wants to use EFI, it won't be able to? The `goto error;` makes this look
> mutually exclusive to my efi-unaware eyes.
If you have enabled FFI, then if something goes wrong, you should goto error.
Just like the origin code:
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)
goto error;
....
}
>
> > 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)
> > + if (dmi_sacn_smbios(efi.smbios3, efi.smbios))
> > 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 (p == NULL)
> > - goto error;
> > - memcpy_fromio(buf, p, 32);
> > - dmi_early_unmap(p, 32);
> > -
> > - if (!dmi_present(buf)) {
> > - dmi_available = 1;
> > - return;
> > - }
> > diff --git a/drivers/firmware/ffi.c b/drivers/firmware/ffi.c
> > new file mode 100644
> > index 000000000000..169802b4a7a8
> > --- /dev/null
> > +++ b/drivers/firmware/ffi.c
> > @@ -0,0 +1,36 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_fdt.h>
> > +#include <linux/libfdt.h>
> > +#include <linux/ffi.h>
> > +
> > +#define FFI_INVALID_TABLE_ADDR (~0UL)
> > +
> > +struct ffi __read_mostly ffi = {
> > + .smbios = FFI_INVALID_TABLE_ADDR,
> > + .smbios3 = FFI_INVALID_TABLE_ADDR,
> > +};
>
> > +EXPORT_SYMBOL(ffi);
>
> > +// SPDX-License-Identifier: GPL-2.0-only
>
> Why not EXPORT_SYMBOL_GPL? But also, who is the user of this export?
Just like efi.
>
> > +
> > +void __init ffi_smbios_root_pointer(void)
> > +{
> > + int cfgtbl, len;
> > + fdt64_t *prop;
> > +
> > + cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
>
> These DT properties need to be documented in a binding.
>
> > + if (cfgtbl < 0) {
> > + pr_info("firmware table not found.\n");
>
> Isn't it perfectly valid for a DT not to contain this table? This print
> should be, at the very least, a pr_debug().
>
> > + return;
> > + }
> > + prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios_phy_ptr", &len);
>
> Again, undocumented DT property. Please document them in a binding.
Okay, I'll add them into binding.
>
> > + if (!prop || len != sizeof(u64))
> > + pr_info("smbios entry point not found.\n");
> > + else
> > + ffi.smbios = fdt64_to_cpu(*prop);
> > +
> > + pr_info("smbios root pointer: %lx\n", ffi.smbios);
>
> ffi.smbios is not set if (!prop || len != sizeof(u64)), looks like your
> "if" should return and the contents of the else become unconditional?
> Otherwise, this print seems wrong.
OK, I will optimize this logic and print.
>
> > +}
> > +
> > diff --git a/include/linux/ffi.h b/include/linux/ffi.h
> > new file mode 100644
> > index 000000000000..95298a805222
> > --- /dev/null
> > +++ b/include/linux/ffi.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _LINUX_FFI_H
> > +#define _LINUX_FFI_H
> > +
> > +extern struct ffi {
> > + unsigned long smbios; /* SMBIOS table (32 bit entry point) */
> > + unsigned long smbios3; /* SMBIOS table (64 bit entry point) */
> > + unsigned long flags;
> > +
> > +} ffi;
> > +
> > +void ffi_smbios_root_pointer(void);
>
> Please provide a stub for !FDT_FW_INTERFACE so that we don't need
> ifdeffery at callsites.
OK, update it on v3.
Thanks,
Yunhui
On Mon, Jul 03, 2023 at 03:50:57PM +0800, 运辉崔 wrote:
> On Sun, Jul 2, 2023 at 8:42 PM Conor Dooley <[email protected]> wrote:
> >
> > Hey,
> >
> > On Sun, Jul 02, 2023 at 05:57:34PM +0800, Yunhui Cui wrote:
> > > When we bringup with coreboot on riscv, we need to obtain
> > > the entry address of SMBIOS through the FFI scheme.
> >
> > What do you need it for?
> RISC-V will be server-oriented, and the system needs to be managed
> based on SMBIOS.
Really what this commit message is missing is something that explains
why you need the FFI scheme. Say along the lines of:
Coreboot on RISC-V does not support EFI, and needs to use the
FDT Firmware Interface (FFI) to pass the entry address of SMBIOS
to the kernel.
Cheers,
Conor.
Hey,
On Mon, Jul 03, 2023 at 04:23:53PM +0800, 运辉崔 wrote:
>
> > nit: please don't write your commit messages as bullet lists
> Okay, thanks for your suggestion.
>
> > > +FDT FIRMWARE INTERFACE (FFI)
> > > +M: Yunhui Cui [email protected]
> > > +S: Maintained
> > > +F: drivers/firmware/ffi.c
> > > +F: include/linux/ffi.h
> >
> > Are you going to apply patches for this, or is someone else?
> Yes, it will be used by patch 3/3.
That's not what I asked :(
> > > EXTERNAL CONNECTOR SUBSYSTEM (EXTCON)
> > > M: MyungJoo Ham <[email protected]>
> > > M: Chanwoo Choi <[email protected]>
> > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > > index b59e3041fd62..ea0149fb4683 100644
> > > --- a/drivers/firmware/Kconfig
> > > +++ b/drivers/firmware/Kconfig
> > > @@ -303,6 +303,17 @@ 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 EFI, 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 smbios tables.
> >
> > Could you express this dependency on !SMBIOS_ENTRY_POINT_SCAN_START in
> > Kconfig & then simply the text to:
> > "Enable this option to support the transfer of firmware information,
> > such as smbios tables, for bootloaders that do not support EFI."
> > since it would not even appear if the arch supports scanning for the
> > entry point?
> > If I was was a punter trying to configure my kernel in menuconfig or
> > whatever, I should be able to decide based on the help text if I need
> > this, not going grepping for #defines in headers.
> Okay， I'll update on v3.
>
>
> >
> > > static void __init dmi_scan_machine(void)
> > > @@ -660,58 +686,22 @@ static void __init dmi_scan_machine(void)
> > > char __iomem *p, *q;
> > > char buf[32];
> > >
> > > +#ifdef CONFIG_FDT_FW_INTERFACE
> > > + if (dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
> >
> > "dmi_sacn_smbios"
> >
> > > + goto error;
> > > +#endif
> >
> > Does this not mean that if FDT_FW_INTERFACE is enabled, but the platform
> > wants to use EFI, it won't be able to? The `goto error;` makes this look
> > mutually exclusive to my efi-unaware eyes.
>
> If you have enabled FFI, then if something goes wrong, you should goto error.
> Just like the origin code:
> 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)
> goto error;
Does this not make FFI and EFI mutually exclusive Kconfig options?
Suppose you are on a system that does not implement FFI, but does
implement EFI - what's going to happen then?
AFAICT, dmi_sacn_smbios(ffi.smbios3, ffi.smbios) will fail & you'll do a
`goto error` & skip the EFI code. What am I missing?
> > > 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)
> > > + if (dmi_sacn_smbios(efi.smbios3, efi.smbios))
> > > 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 (p == NULL)
> > > - goto error;
> > > - memcpy_fromio(buf, p, 32);
> > > - dmi_early_unmap(p, 32);
> > > -
> > > - if (!dmi_present(buf)) {
> > > - dmi_available = 1;
> > > - return;
> > > - }
> > > diff --git a/drivers/firmware/ffi.c b/drivers/firmware/ffi.c
> > > new file mode 100644
> > > index 000000000000..169802b4a7a8
> > > --- /dev/null
> > > +++ b/drivers/firmware/ffi.c
> > > @@ -0,0 +1,36 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > > +#include <linux/of.h>
> > > +#include <linux/of_fdt.h>
> > > +#include <linux/libfdt.h>
> > > +#include <linux/ffi.h>
> > > +
> > > +#define FFI_INVALID_TABLE_ADDR (~0UL)
> > > +
> > > +struct ffi __read_mostly ffi = {
> > > + .smbios = FFI_INVALID_TABLE_ADDR,
> > > + .smbios3 = FFI_INVALID_TABLE_ADDR,
> > > +};
> >
> > > +EXPORT_SYMBOL(ffi);
> >
> > > +// SPDX-License-Identifier: GPL-2.0-only
> >
> > Why not EXPORT_SYMBOL_GPL? But also, who is the user of this export?
> Just like efi.
I don't really understand how that is an answer to the questions.
> > > +
> > > +void __init ffi_smbios_root_pointer(void)
> > > +{
> > > + int cfgtbl, len;
> > > + fdt64_t *prop;
> > > +
> > > + cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
> >
> > These DT properties need to be documented in a binding.
> >
> > > + if (cfgtbl < 0) {
> > > + pr_info("firmware table not found.\n");
> >
> > Isn't it perfectly valid for a DT not to contain this table? This print
> > should be, at the very least, a pr_debug().
> >
> > > + return;
> > > + }
> > > + prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios_phy_ptr", &len);
> >
> > Again, undocumented DT property. Please document them in a binding.
> Okay, I'll add them into binding.
>
>
> >
> > > + if (!prop || len != sizeof(u64))
> > > + pr_info("smbios entry point not found.\n");
> > > + else
> > > + ffi.smbios = fdt64_to_cpu(*prop);
> > > +
> > > + pr_info("smbios root pointer: %lx\n", ffi.smbios);
> >
> > ffi.smbios is not set if (!prop || len != sizeof(u64)), looks like your
> > "if" should return and the contents of the else become unconditional?
> > Otherwise, this print seems wrong.
> OK, I will optimize this logic and print.
It's not an optimisation. If the if branch of your code is taken, it
currently will do
pr_info("smbios entry point not found.\n");
pr_info("smbios root pointer: %lx\n", ffi.smbios);
which makes no sense...
Thanks,
Conor.
Hi Conor,
On Mon, Jul 3, 2023 at 4:13 PM Conor Dooley <[email protected]> wrote:
>
> Hey,
>
> On Mon, Jul 03, 2023 at 03:19:01PM +0800, 运辉崔 wrote:
> > On Sun, Jul 2, 2023 at 9:48 PM Conor Dooley <[email protected]> wrote:
> > >
> > > %subject: riscv: obtain ACPI RSDP from FFI.
> > >
> > > This subject is a bit unhelpful because FFI would commonly mean "foreign
> > > function interface" & you have not yet introduced it. It seems like it
> > > would be better to do s/FFI/devicetree/ or similar.
> >
> > FFI: FDT FIRMWARE INTERFACE.
> >
> > You are right, s/FFI/devicetree/ is of course possible， but I actually
> > want to use FFI as a general solution, put all relevant codes under
> > driver/firmware/, and use RISC-V arch to call general codes.
>
> Yes, I read the patchset. It's still unhelpful to someone reading
> $subject because nobody knows what your version of FFI is IMO.
>
> > In this case, only one Kconfig CONFIG_FDT_FW_INTERFACE is enough， and
> > The FFI code will be placed first in the patchset.
> >
> > But Ard's suggestion is to put the part of SMBIOS in the generic code,
> > and put the FFI for ACPI in the RISCV arch.
> >
> > Please see the v1:
> > https://patches.linaro.org/project/linux-acpi/patch/[email protected]/
>
> I read this too, I was following along with the discussion on the v1.
Okay, I will take your suggestion， to do s/FFI/devicetree/.
This needs to be confirmed with you:
Continue to follow the current code structure, patch 1/3 is placed in
arch/riscv/, and 2/3 is placed under driver/firmware?
>
> > Put the following to /driver/firmware/ffi.c , What do you think?
> > void __init ffi_acpi_root_pointer(void)
> > {
> > ...
> > }
>
> Usually the NOP versions just go in the headers.
>
> > > Please also drop the full stop from the commit messages ;)
> > Okay, thanks.
> >
> > >
> > > Please use a cover letter for multi-patch series & include changelogs.
> > OK, On v3 I would use.
> >
> > >
> > > +CC Sunil, Alex:
> > >
> > > Can you guys please take a look at this & see if it is something that we
> > > want to do (ACPI without EFI)?
> > >
> > > On Sun, Jul 02, 2023 at 05:57:32PM +0800, Yunhui Cui wrote:
> > > > 1. We need to enable the ACPI function on RISC-V.
> > >
> > > RISC-V already supports ACPI, the "we" in this commit message is
> > > confusing. Who is "we"? Bytedance?
>
> Who is the "we"?
"We" are people who need to use ACPI on RISC-V systems, including
ByteDance of course.
>
> > > > When booting with
> > > > Coreboot, we encounter two problems:
> > > > a. Coreboot does not support EFI
> > >
> > >
> > > > b. On RISC-V, only the DTS channel can be used.
> > >
> > > We support ACPI, so this seems inaccurate. Could you explain it better
> > > please?
> >
> > Yes, Sunil already supports ACPI, But it is based on EDK2 boot which
> > supports EFI.
> > In fact, We use Coreboot which has the features of a and b above.
>
> My point is that the commit message has gaps in it.
> This point b & point 1 make it seem like this patch adds ACPI support to
> an architecture that only supports devicetree. "DTS channel" needs to be
> explained further, to be frank I have no idea what that means. Does it
> mean that coreboot on RISC-V only supports DT, or that the RISC-V linux
> kernel requires a mini-DT when booting with EFI?
Yeah， Coreboot only supports DT, do not support EFI.
The first half sentence has already said "When booting with Coreboot,
we encounter two problems:"
How about changing the commit log to the following?
riscv: obtain ACPI RSDP from devicetree.
On RISC-V, when using Coreboot to start, since Coreboot only supports
DTS but not EFI, and
RISC-V does not have a reserved address segment.
When the system enables ACPI, ACPI RSDP needs to be passed through DTS
>
> > > > 2. Based on this, we have added an interface for obtaining firmware
> > > > information transfer through FDT, named FFI.
> > >
> > > Please use the long form of "FFI" before using the short form, since you
> > > are inventing this & noone knows what it means yet.
> > >
> > > > 3. We not only use FFI to pass ACPI RSDP, but also pass other
> > > > firmware information as an extension.
> > >
> > > This patch doesn't do that though?
> >
> > Similar problems may be encountered on other arches, which is also the
> > purpose of this sentence.
>
> Right, but that has nothing to do with this patch? This patch only
> implements the ACPI side of things for RISC-V, it doesn't do the SMBIOS
> stuff. Leave that for the patch that actually does that please.
Okay, Modify it to the above commit log and there will be no such problem.
> > > > +RISC-V FDT FIRMWARE INTERFACE (FFI) SUPPORT
> > > > +M: Yunhui Cui [email protected]
> > > > +S: Maintained
> > > > +F: arch/riscv/include/asm/ffi.h
> > > > +F: arch/riscv/kernel/ffi.c
> > >
> > > Please add this in alphabetical order, these entries have recently been
> > > resorted. That said, maintainers entry for a trivial file in arch code
> > > seems a wee bit odd, seems like it would be better suited rolled up into
> > > your other entry for the interface, like how Ard's one looks for EFI?
> > >
> > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > index b49793cf34eb..2e1c40fb2300 100644
> > > > --- a/arch/riscv/Kconfig
> > > > +++ b/arch/riscv/Kconfig
> > > > @@ -785,6 +785,16 @@ config EFI
> > > > allow the kernel to be booted as an EFI application. This
> > > > is only useful on systems that have UEFI firmware.
> > > >
> > > > +config FFI
> > > > + bool "Fdt firmware interface"
> > > > + depends on OF
> > > > + default y
> > > > + help
> > > > + Added an interface to obtain firmware information transfer
> > > > + through FDT, named FFI. Some bootloaders do not support EFI,
> > > > + such as coreboot.
> > > > + We can pass firmware information through FFI, such as ACPI.
> > >
> > > I don't understand your Kconfig setup. Why don't you just have one
> > > option (the one from patch 2/3), instead of adding 2 different but
> > > similarly named options?
> > OK, let me try it， and use the Kconfig CONFIG_FDT_FW_INTERFACE. EFI
> > seems to use two...
>
> It doesn't use two different options, AFAIR. There's an EFI option in
> the arch Kconfigs and then a menu in drivers/firmware/efi/Kconfig that
> allows enabling sub-components. You've got two entries that appear
> unrelated, despite parsing the same DT bits.
OKay, I'll update it on v3.
>
> >
> > > > config CC_HAVE_STACKPROTECTOR_TLS
> > > > def_bool $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=tp -mstack-protector-guard-offset=0)
> > > >
> > > > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > > > index f71ce21ff684..f9d1625dd159 100644
> > > > --- a/arch/riscv/include/asm/acpi.h
> > > > +++ b/arch/riscv/include/asm/acpi.h
> > > > @@ -15,6 +15,8 @@
> > > > /* Basic configuration for ACPI */
> > > > #ifdef CONFIG_ACPI
> > > >
> > > > +#include <asm/ffi.h>
> > > > +
> > > > typedef u64 phys_cpuid_t;
> > > > #define PHYS_CPUID_INVALID INVALID_HARTID
> > > >
> > > > @@ -66,6 +68,13 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
> > > > unsigned int cpu, const char **isa);
> > > >
> > > > static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> > > > +
> > > > +#define ACPI_HAVE_ARCH_GET_ROOT_POINTER
> > >
> > > How come this is not set in Kconfig like HAVE_FOO options usually are?
>
> > This is modeled after x86 historical code.
> > See arch/x86/include/asm/acpi.h
>
> Is that a good reason for propagating the pattern? Is there a benefit to
> this, other than "x86 did this"?
I smiled when I read this sentence，I haven't thought of a better way yet ：-）
>
> > > > +static inline u64 acpi_arch_get_root_pointer(void)
> > > > +{
> > > > + return acpi_rsdp;
> > > > +}
> > > > +
> > > > #else
> > > > static inline void acpi_init_rintc_map(void) { }
> > > > static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> > > > diff --git a/arch/riscv/include/asm/ffi.h b/arch/riscv/include/asm/ffi.h
> > > > new file mode 100644
> > > > index 000000000000..847af02abd87
> > > > --- /dev/null
> > > > +++ b/arch/riscv/include/asm/ffi.h
> > > > @@ -0,0 +1,9 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +
> > > > +#ifndef _ASM_FFI_H
> > > > +#define _ASM_FFI_H
> > > > +
> > > > +extern u64 acpi_rsdp;
> > >
> > > /stuff/linux/drivers/acpi/osl.c:178:22: error: redefinition of 'acpi_rsdp' with a different type: 'unsigned long' vs 'u64' (aka 'unsigned long long')
> > >
> > > Fails to build when Kexec is enabled.
> >
> > Rename my acpi_rsdp to arch_acpi_rsdp? WDYT?
>
> You could do s/arch/riscv/ either, that'd match what we prefix a lot of
> stuff with.
Sorry, I don't quite understand what you mean. Could you tell me in detail?
>
> > > > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > > > index 506cc4a9a45a..274e06f4da33 100644
> > > > --- a/arch/riscv/kernel/Makefile
> > > > +++ b/arch/riscv/kernel/Makefile
> > > > @@ -92,6 +92,7 @@ obj-$(CONFIG_CRASH_CORE) += crash_core.o
> > > > obj-$(CONFIG_JUMP_LABEL) += jump_label.o
> > > >
> > > > obj-$(CONFIG_EFI) += efi.o
> > > > +obj-$(CONFIG_FFI) += ffi.o
> > >
> > > This file uses tabs for alignment, not spaces.
> > Okay, got it.
> >
> > >
> > > > obj-$(CONFIG_COMPAT) += compat_syscall_table.o
> > > > obj-$(CONFIG_COMPAT) += compat_signal.o
> > > > obj-$(CONFIG_COMPAT) += compat_vdso/
> > > > diff --git a/arch/riscv/kernel/ffi.c b/arch/riscv/kernel/ffi.c
> > > > new file mode 100644
> > > > index 000000000000..c5ac2b5d9148
> > > > --- /dev/null
> > > > +++ b/arch/riscv/kernel/ffi.c
>
> > > > +void __init ffi_init(void)
> > > > +{
> > > > + ffi_acpi_root_pointer();
> > >
> > > What happens if, on a system with "normal" ACPI support, ffi_init() is
> > > called & ffi_acpi_root_pointer() calls things like fdt_path_offset()?
> >
> > According to the current logic, get it from FFI is enabled, if can
> > not, continue to use “normal” ACPI.
>
> I find it hard to understand what you mean here. Do you mean something
> like "The calls to fdt_path_offset() will use the mini EFI DT and are
> harmless. If the config table is not present, it will continue to use
> \"normal\" ACPI."?
acpi_os_get_root_pointer()
{
pa = acpi_arch_get_root_pointer();
if (pa)
return pa;
...//efi logic
}
Even if acpi_arch_get_root_pointer returns 0, it does not affect the
next efi logic.
>
> > > > +}
> > > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > > > index 971fe776e2f8..5a933d6b6acb 100644
> > > > --- a/arch/riscv/kernel/setup.c
> > > > +++ b/arch/riscv/kernel/setup.c
> > > > @@ -36,6 +36,7 @@
> > > > #include <asm/thread_info.h>
> > > > #include <asm/kasan.h>
> > > > #include <asm/efi.h>
> > > > +#include <asm/ffi.h>
> > > >
> > > > #include "head.h"
> > > >
> > > > @@ -279,6 +280,7 @@ void __init setup_arch(char **cmdline_p)
> > > > parse_early_param();
> > > >
> > > > efi_init();
> > > > + ffi_init();
> > >
> > > What provides ffi_init() if CONFIG_FFI is disabled?
>
> > Ok， Modified on v3, put it with the CONFIG_FFI
>
> Sorry, what does this mean?
I mean thanks for the idea, I'll update it in v3.
#ifdef CONFIG_FDT_FW_INTERFACE
ffi_init();
#endif
>
> >
> > >
> > > > paging_init();
> > > >
> > > > /* Parse the ACPI tables for possible boot-time configuration */
>
> Cheers,
> Conor.
Thanks,
Yunhui
On Mon, Jul 03, 2023 at 06:16:07PM +0800, 运辉崔 wrote:
> Hi Conor,
> This needs to be confirmed with you:
> Continue to follow the current code structure, patch 1/3 is placed in
> arch/riscv/, and 2/3 is placed under driver/firmware?
You do want the SMBIOS stuff to be cross architecture, right?
If so, keeping the code as-is seems to make the most sense to me.
> How about changing the commit log to the following?
>
> riscv: obtain ACPI RSDP from devicetree.
>
> On RISC-V, when using Coreboot to start, since Coreboot only supports
> DTS but not EFI, and
> RISC-V does not have a reserved address segment.
> When the system enables ACPI, ACPI RSDP needs to be passed through DTS
I would probably write something like:
On RISC-V, Coreboot does not support booting using EFI, only devicetree
nor does RISC-V have a reserved address segment.
To allow using Coreboot on platforms that require ACPI, the ACPI RSDP
needs to be passed to supervisor mode software using devicetree.
Add support for parsing the "configtbls" devicetree node to find the
ACPI entry point and use wire up acpi_arch_get_root_pointer().
This feature is known as FDT Firmware Interface (FFI).
> > > > > +extern u64 acpi_rsdp;
> > > >
> > > > /stuff/linux/drivers/acpi/osl.c:178:22: error: redefinition of 'acpi_rsdp' with a different type: 'unsigned long' vs 'u64' (aka 'unsigned long long')
> > > >
> > > > Fails to build when Kexec is enabled.
> > >
> > > Rename my acpi_rsdp to arch_acpi_rsdp? WDYT?
> >
> > You could do s/arch/riscv/ either, that'd match what we prefix a lot of
> > stuff with.
>
> Sorry, I don't quite understand what you mean. Could you tell me in detail?
What I meant is that variables & functions in /arch/riscv are often
prefixed with riscv_. I was saying that you could change "arch_acpi_rsdp"
to "riscv_acpi_rsdp".
Thanks,
Conor.
Hi Conor,
On Mon, Jul 3, 2023 at 8:20 PM Conor Dooley <[email protected]> wrote:
>
> On Mon, Jul 03, 2023 at 06:16:07PM +0800, 运辉崔 wrote:
> > Hi Conor,
>
> > This needs to be confirmed with you:
>
> > Continue to follow the current code structure, patch 1/3 is placed in
> > arch/riscv/, and 2/3 is placed under driver/firmware?
>
> You do want the SMBIOS stuff to be cross architecture, right?
> If so, keeping the code as-is seems to make the most sense to me.
Okay, other arches may use FFI in the future. Keep the code as-is seems.
> > How about changing the commit log to the following?
> >
> > riscv: obtain ACPI RSDP from devicetree.
> >
> > On RISC-V, when using Coreboot to start, since Coreboot only supports
> > DTS but not EFI, and
> > RISC-V does not have a reserved address segment.
> > When the system enables ACPI, ACPI RSDP needs to be passed through DTS
>
> I would probably write something like:
> On RISC-V, Coreboot does not support booting using EFI, only devicetree
> nor does RISC-V have a reserved address segment.
> To allow using Coreboot on platforms that require ACPI, the ACPI RSDP
> needs to be passed to supervisor mode software using devicetree.
> Add support for parsing the "configtbls" devicetree node to find the
> ACPI entry point and use wire up acpi_arch_get_root_pointer().
> This feature is known as FDT Firmware Interface (FFI).
Great, I have to learn from it.
> > > > > > +extern u64 acpi_rsdp;
> > > > >
> > > > > /stuff/linux/drivers/acpi/osl.c:178:22: error: redefinition of 'acpi_rsdp' with a different type: 'unsigned long' vs 'u64' (aka 'unsigned long long')
> > > > >
> > > > > Fails to build when Kexec is enabled.
> > > >
> > > > Rename my acpi_rsdp to arch_acpi_rsdp? WDYT?
> > >
> > > You could do s/arch/riscv/ either, that'd match what we prefix a lot of
> > > stuff with.
> >
> > Sorry, I don't quite understand what you mean. Could you tell me in detail?
>
> What I meant is that variables & functions in /arch/riscv are often
> prefixed with riscv_. I was saying that you could change "arch_acpi_rsdp"
> to "riscv_acpi_rsdp".
Oh, that's what it means, okay, I'll update it on v3.
>
> Thanks,
> Conor.
Thanks,
Yunhui
Hi Conor,
Thanks for your comments.
On Mon, Jul 3, 2023 at 4:36 PM Conor Dooley <[email protected]> wrote:
>
> Hey,
>
> On Mon, Jul 03, 2023 at 04:23:53PM +0800, 运辉崔 wrote:
> >
> > > nit: please don't write your commit messages as bullet lists
> > Okay, thanks for your suggestion.
> >
> > > > +FDT FIRMWARE INTERFACE (FFI)
> > > > +M: Yunhui Cui [email protected]
> > > > +S: Maintained
> > > > +F: drivers/firmware/ffi.c
> > > > +F: include/linux/ffi.h
> > >
> > > Are you going to apply patches for this, or is someone else?
> > Yes, it will be used by patch 3/3.
>
> That's not what I asked :(
Sorry, ok, what do you want to ask?
> > > > EXTERNAL CONNECTOR SUBSYSTEM (EXTCON)
> > > > M: MyungJoo Ham <[email protected]>
> > > > M: Chanwoo Choi <[email protected]>
> > > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > > > index b59e3041fd62..ea0149fb4683 100644
> > > > --- a/drivers/firmware/Kconfig
> > > > +++ b/drivers/firmware/Kconfig
> > > > @@ -303,6 +303,17 @@ 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 EFI, 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 smbios tables.
> > >
> > > Could you express this dependency on !SMBIOS_ENTRY_POINT_SCAN_START in
> > > Kconfig & then simply the text to:
> > > "Enable this option to support the transfer of firmware information,
> > > such as smbios tables, for bootloaders that do not support EFI."
> > > since it would not even appear if the arch supports scanning for the
> > > entry point?
> > > If I was was a punter trying to configure my kernel in menuconfig or
> > > whatever, I should be able to decide based on the help text if I need
> > > this, not going grepping for #defines in headers.
> > Okay， I'll update on v3.
> >
> >
> > >
> > > > static void __init dmi_scan_machine(void)
> > > > @@ -660,58 +686,22 @@ static void __init dmi_scan_machine(void)
> > > > char __iomem *p, *q;
> > > > char buf[32];
> > > >
> > > > +#ifdef CONFIG_FDT_FW_INTERFACE
> > > > + if (dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
> > >
> > > "dmi_sacn_smbios"
> > >
> > > > + goto error;
> > > > +#endif
> > >
> > > Does this not mean that if FDT_FW_INTERFACE is enabled, but the platform
> > > wants to use EFI, it won't be able to? The `goto error;` makes this look
> > > mutually exclusive to my efi-unaware eyes.
> >
> > If you have enabled FFI, then if something goes wrong, you should goto error.
> > Just like the origin code:
> > 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)
> > goto error;
>
> Does this not make FFI and EFI mutually exclusive Kconfig options?
> Suppose you are on a system that does not implement FFI, but does
> implement EFI - what's going to happen then?
> AFAICT, dmi_sacn_smbios(ffi.smbios3, ffi.smbios) will fail & you'll do a
> `goto error` & skip the EFI code. What am I missing?
Code is not intended to be mutually exclusive, get the correct value and return,
The code is going to be changed to this:
#ifdef CONFIG_FDT_FW_INTERFACE
if (ffi_enabled(FFI_CONFIG_TABLES)) {
if (!dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
return;
}
#endif
>
> > > > 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)
> > > > + if (dmi_sacn_smbios(efi.smbios3, efi.smbios))
> > > > 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 (p == NULL)
> > > > - goto error;
> > > > - memcpy_fromio(buf, p, 32);
> > > > - dmi_early_unmap(p, 32);
> > > > -
> > > > - if (!dmi_present(buf)) {
> > > > - dmi_available = 1;
> > > > - return;
> > > > - }
> > > > diff --git a/drivers/firmware/ffi.c b/drivers/firmware/ffi.c
> > > > new file mode 100644
> > > > index 000000000000..169802b4a7a8
> > > > --- /dev/null
> > > > +++ b/drivers/firmware/ffi.c
> > > > @@ -0,0 +1,36 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +
> > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > +
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_fdt.h>
> > > > +#include <linux/libfdt.h>
> > > > +#include <linux/ffi.h>
> > > > +
> > > > +#define FFI_INVALID_TABLE_ADDR (~0UL)
> > > > +
> > > > +struct ffi __read_mostly ffi = {
> > > > + .smbios = FFI_INVALID_TABLE_ADDR,
> > > > + .smbios3 = FFI_INVALID_TABLE_ADDR,
> > > > +};
> > >
> > > > +EXPORT_SYMBOL(ffi);
> > >
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > >
> > > Why not EXPORT_SYMBOL_GPL? But also, who is the user of this export?
> > Just like efi.
>
> I don't really understand how that is an answer to the questions.
I checked, the code is executed as the system starts, either Y or N, M
will not appear, and the same is true for ffi's user DMI.
So no need for EXPORT.
>
> > > > +
> > > > +void __init ffi_smbios_root_pointer(void)
> > > > +{
> > > > + int cfgtbl, len;
> > > > + fdt64_t *prop;
> > > > +
> > > > + cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
> > >
> > > These DT properties need to be documented in a binding.
> > >
> > > > + if (cfgtbl < 0) {
> > > > + pr_info("firmware table not found.\n");
> > >
> > > Isn't it perfectly valid for a DT not to contain this table? This print
> > > should be, at the very least, a pr_debug().
> > >
> > > > + return;
> > > > + }
> > > > + prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios_phy_ptr", &len);
> > >
> > > Again, undocumented DT property. Please document them in a binding.
> > Okay, I'll add them into binding.
> >
> >
> > >
> > > > + if (!prop || len != sizeof(u64))
> > > > + pr_info("smbios entry point not found.\n");
> > > > + else
> > > > + ffi.smbios = fdt64_to_cpu(*prop);
> > > > +
> > > > + pr_info("smbios root pointer: %lx\n", ffi.smbios);
> > >
> > > ffi.smbios is not set if (!prop || len != sizeof(u64)), looks like your
> > > "if" should return and the contents of the else become unconditional?
> > > Otherwise, this print seems wrong.
>
> > OK, I will optimize this logic and print.
>
> It's not an optimisation. If the if branch of your code is taken, it
> currently will do
> pr_info("smbios entry point not found.\n");
> pr_info("smbios root pointer: %lx\n", ffi.smbios);
> which makes no sense...
Yeah, I got it, that's what I mean by "optimize".
>
> Thanks,
> Conor.
>
Thanks,
Yunhui
On Mon, Jul 03, 2023 at 08:41:30PM +0800, 运辉崔 wrote:
> On Mon, Jul 3, 2023 at 4:36 PM Conor Dooley <[email protected]> wrote:
> > On Mon, Jul 03, 2023 at 04:23:53PM +0800, 运辉崔 wrote:
> > > > > +FDT FIRMWARE INTERFACE (FFI)
> > > > > +M: Yunhui Cui [email protected]
> > > > > +S: Maintained
> > > > > +F: drivers/firmware/ffi.c
> > > > > +F: include/linux/ffi.h
> > > >
> > > > Are you going to apply patches for this, or is someone else?
> > > Yes, it will be used by patch 3/3.
> >
> > That's not what I asked :(
>
> Sorry, ok, what do you want to ask?
Who is going to apply patches for drivers/firmware/ffi*?
> > > > > static void __init dmi_scan_machine(void)
> > > > > @@ -660,58 +686,22 @@ static void __init dmi_scan_machine(void)
> > > > > char __iomem *p, *q;
> > > > > char buf[32];
> > > > >
> > > > > +#ifdef CONFIG_FDT_FW_INTERFACE
> > > > > + if (dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
> > > >
> > > > "dmi_sacn_smbios"
> > > >
> > > > > + goto error;
> > > > > +#endif
> > > >
> > > > Does this not mean that if FDT_FW_INTERFACE is enabled, but the platform
> > > > wants to use EFI, it won't be able to? The `goto error;` makes this look
> > > > mutually exclusive to my efi-unaware eyes.
> > >
> > > If you have enabled FFI, then if something goes wrong, you should goto error.
> > > Just like the origin code:
> > > 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)
> > > goto error;
> >
> > Does this not make FFI and EFI mutually exclusive Kconfig options?
> > Suppose you are on a system that does not implement FFI, but does
> > implement EFI - what's going to happen then?
> > AFAICT, dmi_sacn_smbios(ffi.smbios3, ffi.smbios) will fail & you'll do a
> > `goto error` & skip the EFI code. What am I missing?
>
> Code is not intended to be mutually exclusive, get the correct value and return,
> The code is going to be changed to this:
>
> #ifdef CONFIG_FDT_FW_INTERFACE
Ideally, these would be IS_ENABLED() instead of #ifdef - but if you copy
what EFI does, then you don't need either, as there will always be an
ffi_enabled() defined.
> if (ffi_enabled(FFI_CONFIG_TABLES)) {
I don't know what this function is, but this code seems like a step in
the right direction.
> if (!dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
> return;
> }
> #endif
Thanks,
Conor.
(This is a reply to a non-existent cover letter.)
I'm not a big fan of adding yet another interface. Have you considered
doing something like [1]?
[1] https://github.com/tianocore/tianocore.github.io/wiki/UefiPayloadPkg
Thanks,
drew