lkml.org 
[lkml]   [2023]   [Jul]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 1/3] riscv: obtain ACPI RSDP from FFI.
    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.
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2023-07-02 15:48    [W:7.533 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site