[v2,1/3] riscv: obtain ACPI RSDP from FFI.

  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
  

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