| Message ID | 20230702095735.860-1-cuiyunhui@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] riscv: obtain ACPI RSDP from FFI. | expand |
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 cuiyunhui@bytedance.com > +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 <myungjoo.ham@samsung.com> > M: Chanwoo Choi <cw00.choi@samsung.com> > 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 cuiyunhui@bytedance.com > +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 <sunilvl@ventanamicro.com> 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/20230426034001.16-1-cuiyunhui@bytedance.com/ Thanks, Yunhui
Hi, Conor On Sun, Jul 2, 2023 at 9:48 PM Conor Dooley <conor@kernel.org> 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/20230426034001.16-1-cuiyunhui@bytedance.com/ 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 cuiyunhui@bytedance.com > > +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
Hey, On Mon, Jul 03, 2023 at 03:19:01PM +0800, 运辉崔 wrote: > On Sun, Jul 2, 2023 at 9:48 PM Conor Dooley <conor@kernel.org> 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/20230426034001.16-1-cuiyunhui@bytedance.com/ 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 cuiyunhui@bytedance.com > > > +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 cuiyunhui@bytedance.com > > +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 <myungjoo.ham@samsung.com> > > M: Chanwoo Choi <cw00.choi@samsung.com> > > 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
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 cuiyunhui@bytedance.com > > > +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 <myungjoo.ham@samsung.com> > > > M: Chanwoo Choi <cw00.choi@samsung.com> > > > 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 <conor.dooley@microchip.com> wrote: > > Hey, > > On Mon, Jul 03, 2023 at 03:19:01PM +0800, 运辉崔 wrote: > > On Sun, Jul 2, 2023 at 9:48 PM Conor Dooley <conor@kernel.org> 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/20230426034001.16-1-cuiyunhui@bytedance.com/ > > 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 cuiyunhui@bytedance.com > > > > +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, Thanks for your comments. On Mon, Jul 3, 2023 at 4:36 PM Conor Dooley <conor.dooley@microchip.com> 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 cuiyunhui@bytedance.com > > > > +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 <myungjoo.ham@samsung.com> > > > > M: Chanwoo Choi <cw00.choi@samsung.com> > > > > 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
(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
On Mon, Jul 03, 2023 at 08:41:30PM +0800, 运辉崔 wrote: > On Mon, Jul 3, 2023 at 4:36 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > On Mon, Jul 03, 2023 at 04:23:53PM +0800, 运辉崔 wrote: > > > > > +FDT FIRMWARE INTERFACE (FFI) > > > > > +M: Yunhui Cui cuiyunhui@bytedance.com > > > > > +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.
Hi Conor, On Mon, Jul 3, 2023 at 8:20 PM Conor Dooley <conor.dooley@microchip.com> 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, On Mon, Jul 3, 2023 at 9:03 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > On Mon, Jul 03, 2023 at 08:41:30PM +0800, 运辉崔 wrote: > > On Mon, Jul 3, 2023 at 4:36 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > On Mon, Jul 03, 2023 at 04:23:53PM +0800, 运辉崔 wrote: > > > > > > > +FDT FIRMWARE INTERFACE (FFI) > > > > > > +M: Yunhui Cui cuiyunhui@bytedance.com > > > > > > +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*? I'll update to v3: F: drivers/firmware/ffi* F: include/linux/ffi* And I'll plan to maintain them. But now, I don't know how to apply the patches. Could you give some suggestions? > > > > > > > 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. Okay， this can refer to the code of EFI. :) > > > 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. Okay, I'll update it on v3. > > > if (!dmi_sacn_smbios(ffi.smbios3, ffi.smbios)) > > return; > > } > > #endif > > Thanks, > Conor. Thanks, Yunhui
Hi drew, On Mon, Jul 3, 2023 at 9:01 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > (This is a reply to a non-existent cover letter.) This has been discussed many times with Ard, Please refer to : https://patches.linaro.org/project/linux-acpi/patch/20230426034001.16-1-cuiyunhui@bytedance.com/ > 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 Thanks, Yunhui
On Mon, Jul 03, 2023 at 09:30:10PM +0800, 运辉崔 wrote: > Hi drew, > > On Mon, Jul 3, 2023 at 9:01 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > > > (This is a reply to a non-existent cover letter.) > > This has been discussed many times with Ard, Please refer to : > https://patches.linaro.org/project/linux-acpi/patch/20230426034001.16-1-cuiyunhui@bytedance.com/ There's nothing in that thread that convinces me this is a good idea. Indeed, afaict from that thread, Ard nacked this. It was only when it was suggested that arch/riscv would own the code, that he stopped complaining about it. I wouldn't call that an endorsement. Thanks, drew > > > > 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 > > Thanks, > Yunhui
On Mon, Jul 03, 2023 at 04:17:07PM +0200, Andrew Jones wrote: > On Mon, Jul 03, 2023 at 09:30:10PM +0800, 运辉崔 wrote: > > On Mon, Jul 3, 2023 at 9:01 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > > > > > > (This is a reply to a non-existent cover letter.) > > > > This has been discussed many times with Ard, Please refer to : > > https://patches.linaro.org/project/linux-acpi/patch/20230426034001.16-1-cuiyunhui@bytedance.com/ > > There's nothing in that thread that convinces me this is a good idea. > Indeed, afaict from that thread, Ard nacked this. It was only when it > was suggested that arch/riscv would own the code, that he stopped > complaining about it. I wouldn't call that an endorsement. In fact, he expressly said that it would then be up to the RISC-V maintainers as to what to do: But please check with the RISC-V maintainers if they are up for this, and whether they want to see this mechanism contributed to one of the pertinent specifications.
On Mon, 3 Jul 2023 at 15:33, 运辉崔 <cuiyunhui@bytedance.com> wrote: > > Hi drew, > > On Mon, Jul 3, 2023 at 9:01 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > > > (This is a reply to a non-existent cover letter.) > > This has been discussed many times with Ard, Please refer to : > https://patches.linaro.org/project/linux-acpi/patch/20230426034001.16-1-cuiyunhui@bytedance.com/ Hi Yunhui,
On 3 Jul 2023, at 19:58, Emil Renner Berthing <emil.renner.berthing@gmail.com> wrote: > > On Mon, 3 Jul 2023 at 15:33, 运辉崔 <cuiyunhui@bytedance.com> wrote: >> >> Hi drew, >> >> On Mon, Jul 3, 2023 at 9:01 PM Andrew Jones <ajones@ventanamicro.com> wrote: >>> >>> >>> (This is a reply to a non-existent cover letter.) >> >> This has been discussed many times with Ard, Please refer to : >> https://patches.linaro.org/project/linux-acpi/patch/20230426034001.16-1-cuiyunhui@bytedance.com/ > > Hi Yunhui, > > From that discussion it was mentioned that that arm supports 3 methods > of booting: > direct + devicetree > EFI + devicetree > EFI + ACPI > ..but not > direct + ACPI > > To me it isn't obvious from that or this thread, and since arm seems > to be doing fine without the 4th option I'm curious why that's > necessary on riscv? If anything we should be removing option 1, because that’s not a cross-OS standard (though RISC-V’s SBI direct booting is at least not tied to the OS). Any application-class platform spec is going to mandate EFI, because, whatever your thoughts of EFI are, that is *the* standard. And if you’re willing to pick up all the complexity of ACPI, what’s a bit of EFI (especially if you only go for a minimal one a la U-Boot)? Jess >>> 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 > > Also you didn't answer this question, which I'd also like to hear a reply to. > > /Emil > >>> Thanks, >>> drew >> >> Thanks, >> Yunhui >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Jessica Clarke <jrtc27@jrtc27.com> writes: > On 3 Jul 2023, at 19:58, Emil Renner Berthing <emil.renner.berthing@gmail.com> wrote: >> >> On Mon, 3 Jul 2023 at 15:33, 运辉崔 <cuiyunhui@bytedance.com> wrote: >>> >>> Hi drew, >>> >>> On Mon, Jul 3, 2023 at 9:01 PM Andrew Jones <ajones@ventanamicro.com> wrote: >>>> >>>> >>>> (This is a reply to a non-existent cover letter.) >>> >>> This has been discussed many times with Ard, Please refer to : >>> https://patches.linaro.org/project/linux-acpi/patch/20230426034001.16-1-cuiyunhui@bytedance.com/ >> >> Hi Yunhui, >> >> From that discussion it was mentioned that that arm supports 3 methods >> of booting: >> direct + devicetree >> EFI + devicetree >> EFI + ACPI >> ..but not >> direct + ACPI >> >> To me it isn't obvious from that or this thread, and since arm seems >> to be doing fine without the 4th option I'm curious why that's >> necessary on riscv? > > If anything we should be removing option 1, because that’s not a > cross-OS standard (though RISC-V’s SBI direct booting is at least not > tied to the OS). Any application-class platform spec is going to > mandate EFI, because, whatever your thoughts of EFI are, that is *the* > standard. And if you’re willing to pick up all the complexity of ACPI, > what’s a bit of EFI (especially if you only go for a minimal one a la > U-Boot)? Well said! Yunhui, why not simply add a minimal UEFI stub to Coreboot (like Jess points out above)? IMO what U-boot (or https://github.com/cloud-hypervisor/rust-hypervisor-firmware if you're into Rust ;-)) is doing, and just having a small UEFI shim is the way to go.
Hi Björn, On Wed, Jul 5, 2023 at 10:43 PM Björn Töpel <bjorn@kernel.org> wrote: > > Jessica Clarke <jrtc27@jrtc27.com> writes: > > > On 3 Jul 2023, at 19:58, Emil Renner Berthing <emil.renner.berthing@gmail.com> wrote: > >> > >> On Mon, 3 Jul 2023 at 15:33, 运辉崔 <cuiyunhui@bytedance.com> wrote: > >>> > >>> Hi drew, > >>> > >>> On Mon, Jul 3, 2023 at 9:01 PM Andrew Jones <ajones@ventanamicro.com> wrote: > >>>> > >>>> > >>>> (This is a reply to a non-existent cover letter.) > >>> > >>> This has been discussed many times with Ard, Please refer to : > >>> https://patches.linaro.org/project/linux-acpi/patch/20230426034001.16-1-cuiyunhui@bytedance.com/ > >> > >> Hi Yunhui, > >> > >> From that discussion it was mentioned that that arm supports 3 methods > >> of booting: > >> direct + devicetree > >> EFI + devicetree > >> EFI + ACPI > >> ..but not > >> direct + ACPI > >> > >> To me it isn't obvious from that or this thread, and since arm seems > >> to be doing fine without the 4th option I'm curious why that's > >> necessary on riscv? > > > > If anything we should be removing option 1, because that’s not a > > cross-OS standard (though RISC-V’s SBI direct booting is at least not > > tied to the OS). Any application-class platform spec is going to > > mandate EFI, because, whatever your thoughts of EFI are, that is *the* > > standard. And if you’re willing to pick up all the complexity of ACPI, > > what’s a bit of EFI (especially if you only go for a minimal one a la > > U-Boot)? > > Well said! > > Yunhui, why not simply add a minimal UEFI stub to Coreboot (like Jess > points out above)? In fact, in the v1 email, Coreboot's maintainer Ron has made it clear that Coreboot does not support EFI, and it is necessary to transmit firmware information through DTS on RISC-V. @Ron, can you help explain it to them? > IMO what U-boot (or > https://github.com/cloud-hypervisor/rust-hypervisor-firmware if you're > into Rust ;-)) is doing, and just having a small UEFI shim is the way to > go. Thanks, Yunhui
运辉崔 <cuiyunhui@bytedance.com> writes: > Hi Björn, > > On Wed, Jul 5, 2023 at 10:43 PM Björn Töpel <bjorn@kernel.org> wrote: >> >> Jessica Clarke <jrtc27@jrtc27.com> writes: >> >> > On 3 Jul 2023, at 19:58, Emil Renner Berthing <emil.renner.berthing@gmail.com> wrote: >> >> >> >> On Mon, 3 Jul 2023 at 15:33, 运辉崔 <cuiyunhui@bytedance.com> wrote: >> >>> >> >>> Hi drew, >> >>> >> >>> On Mon, Jul 3, 2023 at 9:01 PM Andrew Jones <ajones@ventanamicro.com> wrote: >> >>>> >> >>>> >> >>>> (This is a reply to a non-existent cover letter.) >> >>> >> >>> This has been discussed many times with Ard, Please refer to : >> >>> https://patches.linaro.org/project/linux-acpi/patch/20230426034001.16-1-cuiyunhui@bytedance.com/ >> >> >> >> Hi Yunhui, >> >> >> >> From that discussion it was mentioned that that arm supports 3 methods >> >> of booting: >> >> direct + devicetree >> >> EFI + devicetree >> >> EFI + ACPI >> >> ..but not >> >> direct + ACPI >> >> >> >> To me it isn't obvious from that or this thread, and since arm seems >> >> to be doing fine without the 4th option I'm curious why that's >> >> necessary on riscv? >> > >> > If anything we should be removing option 1, because that’s not a >> > cross-OS standard (though RISC-V’s SBI direct booting is at least not >> > tied to the OS). Any application-class platform spec is going to >> > mandate EFI, because, whatever your thoughts of EFI are, that is *the* >> > standard. And if you’re willing to pick up all the complexity of ACPI, >> > what’s a bit of EFI (especially if you only go for a minimal one a la >> > U-Boot)? >> >> Well said! >> >> Yunhui, why not simply add a minimal UEFI stub to Coreboot (like Jess >> points out above)? > > In fact, in the v1 email, Coreboot's maintainer Ron has made it clear > that Coreboot does not support EFI, and it is necessary to transmit > firmware information through DTS on RISC-V. It clear that Coreboot doesn't support UEFI today. We're "arguing" that it's less work/verification adding the neccesary minimal UEFI plumbing for Coreboot, than jumping through hoops in the kernel to work around it. I'm getting some UEFI FUD vibes here. I also think that parts of UEFI is kind of ugly, but it's, as Jess says, *the* spec and honestly, a bit what's expected (Hi CXL). UEFI is a specification, and implementing the minimal requirements for UEFI is not that of a big deal. Look at Alex Graf's (et al) work on u-boot UEFI. U-boot is small/lean/open *and* manage to support enough UEFI for ACPI. The whole "Oh, UEFI is so bad, bloated, and closed" hand-wavery is a bit tiring. :-( ...these last four sections is more of a beer discussion. I'll take my "my FW is better than yours" rants elsewhere. ;-) Björn
Ron, On Thu, 6 Jul 2023 at 18:37, ron minnich <rminnich@gmail.com> wrote: > I'm trying hard not to get sucked into this argument, but it is, at > least, impolite, to accuse us of engaging in hand-wavery. There's > probably more UEFI expertise in coreboot and oreboot than most > places, and that's precisely why we want a non-UEFI > option. Familiarity with UEFI did not breed respect. If my reply was interpreted as rude, please accept my apologies. That was not my intention. > So can we get back to the business at hand? bytedance may be the > company proposing this change, but they are certainly not the only > company interested in seeing it happen. A project I'm involved in > needs this work to go through. I noted that the discussion continued from Ard's reply (which, no surprise, resonates with my worldview :-)): https://lore.kernel.org/linux-riscv/CAMj1kXFZren0Q19DimwQaETCLz64D4bZQC5B2N=i3SAWHygkTQ@mail.gmail.com/ Let's continue there. Björn
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 cuiyunhui@bytedance.com +S: Maintained +F: arch/riscv/include/asm/ffi.h +F: arch/riscv/kernel/ffi.c + RISC-V PMU DRIVERS M: Atish Patra <atishp@atishpatra.org> R: Anup Patel <anup@brainfault.org> 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 */
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 <cuiyunhui@bytedance.com> --- 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