| Message ID | 20230702095735.860-1-cuiyunhui@bytedance.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2,1/3] riscv: obtain ACPI RSDP from FFI. | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes, riscv/for-next or riscv/master |
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, 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.
(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
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 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, 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? > > 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
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