Hey, On Mon, Jul 03, 2023 at 03:19:01PM +0800, 运辉崔 wrote: > On Sun, Jul 2, 2023 at 9:48 PM Conor Dooley <conor@xxxxxxxxxx> 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@xxxxxxxxxxxxx/ 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@xxxxxxxxxxxxx > > > +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.
Attachment:
signature.asc
Description: PGP signature
