Message-ID: <CAEEQ3wnjYK+Jj9Ce_yEHPL_z3eYn4OKP85YLXnVeaPd+SA3DJw@mail.gmail.com>
Date:   Mon, 3 Jul 2023 18:16:07 +0800
From:   运辉崔 <cuiyunhui@...edance.com>
To:     Conor Dooley <conor.dooley@...rochip.com>
Cc:     Conor Dooley <conor@...nel.org>, ardb@...nel.org,
        palmer@...belt.com, paul.walmsley@...ive.com,
        aou@...s.berkeley.edu, linux-riscv@...ts.infradead.org,
        rminnich@...il.com, mark.rutland@....com, lpieralisi@...nel.org,
        rafael@...nel.org, lenb@...nel.org, jdelvare@...e.com,
        yc.hung@...iatek.com, angelogioacchino.delregno@...labora.com,
        allen-kh.cheng@...iatek.com, pierre-louis.bossart@...ux.intel.com,
        tinghan.shen@...iatek.com, linux-kernel@...r.kernel.org,
        linux-acpi@...r.kernel.org, geshijian@...edance.com,
        weidong.wd@...edance.com, alexghiti@...osinc.com,
        sunilvl@...tanamicro.com
Subject: Re: [External] Re: [PATCH v2 1/3] riscv: obtain ACPI RSDP from FFI.

Hi Conor,

On Mon, Jul 3, 2023 at 4:13 PM Conor Dooley <conor.dooley@...rochip.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@...nel.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@...edance.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

Powered by blists - more mailing lists