firmware: added a firmware information passing method FFI

  Some BootLoaders do not support UEFI and cannot pass ACPI/SBMIOS table
addresses through UEFI, such as coreboot.

On the x86 platform, we pass the ACPI/SMBIOS table through the reserved
address segment 0xF0000, but other arches usually do not reserve this
address segment.

We have added a new firmware information transmission method named FFI
(FDT FIRMWARE INTERFACE), through FDT to obtain firmware information,
such as the base address of the ACPI and SMBIOS table.

Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 MAINTAINERS                 |  6 +++
 drivers/acpi/osl.c          |  8 ++++
 drivers/firmware/Kconfig    | 12 +++++
 drivers/firmware/Makefile   |  1 +
 drivers/firmware/dmi_scan.c | 96 ++++++++++++++++++++++---------------
 drivers/firmware/ffi.c      | 56 ++++++++++++++++++++++
 include/linux/ffi.h         | 15 ++++++
 7 files changed, 155 insertions(+), 39 deletions(-)
 create mode 100644 drivers/firmware/ffi.c
 create mode 100644 include/linux/ffi.h
  

On Wed, 26 Apr 2023 at 04:40, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
>
> Some BootLoaders do not support UEFI and cannot pass ACPI/SBMIOS table
> addresses through UEFI, such as coreboot.
>
> On the x86 platform, we pass the ACPI/SMBIOS table through the reserved
> address segment 0xF0000, but other arches usually do not reserve this
> address segment.
>
> We have added a new firmware information transmission method named FFI
> (FDT FIRMWARE INTERFACE), through FDT to obtain firmware information,
> such as the base address of the ACPI and SMBIOS table.
>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>

Hello Yunhui,

I am not sure this is a good idea: this is clearly intended for arm64,
which cannot use ACPI without the EFI memory map, which it uses to
cross reference memory opregion accesses, to determine the correct
memory type attributes.

What is the use case you are trying to accommodate here?




> ---
>  MAINTAINERS                 |  6 +++
>  drivers/acpi/osl.c          |  8 ++++
>  drivers/firmware/Kconfig    | 12 +++++
>  drivers/firmware/Makefile   |  1 +
>  drivers/firmware/dmi_scan.c | 96 ++++++++++++++++++++++---------------
>  drivers/firmware/ffi.c      | 56 ++++++++++++++++++++++
>  include/linux/ffi.h         | 15 ++++++
>  7 files changed, 155 insertions(+), 39 deletions(-)
>  create mode 100644 drivers/firmware/ffi.c
>  create mode 100644 include/linux/ffi.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8d5bc223f305..94664f3b4c96 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7750,6 +7750,12 @@ F:       arch/x86/platform/efi/
>  F:     drivers/firmware/efi/
>  F:     include/linux/efi*.h
>
> +FDT FIRMWARE INTERFACE (FFI)
> +M:     Yunhui Cui cuiyunhui@bytedance.com
> +S:     Maintained
> +F:     drivers/firmware/ffi.c
> +F:     include/linux/ffi.h
> +
>  EXTERNAL CONNECTOR SUBSYSTEM (EXTCON)
>  M:     MyungJoo Ham <myungjoo.ham@samsung.com>
>  M:     Chanwoo Choi <cw00.choi@samsung.com>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 3269a888fb7a..d45000041d2b 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -25,6 +25,7 @@
>  #include <linux/nmi.h>
>  #include <linux/acpi.h>
>  #include <linux/efi.h>
> +#include <linux/ffi.h>
>  #include <linux/ioport.h>
>  #include <linux/list.h>
>  #include <linux/jiffies.h>
> @@ -206,6 +207,13 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
>         if (pa)
>                 return pa;
>
> +#ifdef CONFIG_FDT_FW_INTERFACE
> +       if (fdt_fwtbl.acpi20 != FDT_INVALID_FWTBL_ADDR)
> +               return fdt_fwtbl.acpi20;
> +       if (fdt_fwtbl.acpi != FDT_INVALID_FWTBL_ADDR)
> +               return fdt_fwtbl.acpi;
> +       pr_err("Fdt system description tables not found\n");
> +#endif
>         if (efi_enabled(EFI_CONFIG_TABLES)) {
>                 if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
>                         return efi.acpi20;
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index b59e3041fd62..13c67b50c17a 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -303,6 +303,18 @@ config TURRIS_MOX_RWTM
>           other manufacturing data and also utilize the Entropy Bit Generator
>           for hardware random number generation.
>
> +config FDT_FW_INTERFACE
> +       bool "An interface for passing firmware info through FDT"
> +       depends on OF && OF_FLATTREE
> +       default n
> +       help
> +         When some bootloaders do not support UEFI, and the arch does not
> +         support SMBIOS_ENTRY_POINT_SCAN_START, then you can enable this option
> +         to support the transfer of firmware information, such as acpi, smbios
> +         tables.
> +
> +         Say Y here if you want to pass firmware information by FDT.
> +
>  source "drivers/firmware/arm_ffa/Kconfig"
>  source "drivers/firmware/broadcom/Kconfig"
>  source "drivers/firmware/cirrus/Kconfig"
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 28fcddcd688f..3b8b5d0868a6 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -33,6 +33,7 @@ obj-y                         += cirrus/
>  obj-y                          += meson/
>  obj-$(CONFIG_GOOGLE_FIRMWARE)  += google/
>  obj-y                          += efi/
> +obj-$(CONFIG_FDT_FW_INTERFACE) += ffi.o
>  obj-y                          += imx/
>  obj-y                          += psci/
>  obj-y                          += smccc/
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 015c95a825d3..1e1a74ed7d3b 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -6,6 +6,7 @@
>  #include <linux/ctype.h>
>  #include <linux/dmi.h>
>  #include <linux/efi.h>
> +#include <linux/ffi.h>
>  #include <linux/memblock.h>
>  #include <linux/random.h>
>  #include <asm/dmi.h>
> @@ -655,54 +656,71 @@ static int __init dmi_smbios3_present(const u8 *buf)
>         return 1;
>  }
>
> -static void __init dmi_scan_machine(void)
> +/*
> + * According to the DMTF SMBIOS reference spec v3.0.0, it is
> + * allowed to define both the 64-bit entry point (smbios3) and
> + * the 32-bit entry point (smbios), in which case they should
> + * either both point to the same SMBIOS structure table, or the
> + * table pointed to by the 64-bit entry point should contain a
> + * superset of the table contents pointed to by the 32-bit entry
> + * point (section 5.2)
> + * This implies that the 64-bit entry point should have
> + * precedence if it is defined and supported by the OS. If we
> + * have the 64-bit entry point, but fail to decode it, fall
> + * back to the legacy one (if available)
> + */
> +static int __init dmi_sacn_smbios(unsigned long smbios3, unsigned long smbios)
>  {
> -       char __iomem *p, *q;
> +       char __iomem *p;
>         char buf[32];
> +       #define INVALID_TABLE_ADDR (~0UL)
>
> -       if (efi_enabled(EFI_CONFIG_TABLES)) {
> -               /*
> -                * According to the DMTF SMBIOS reference spec v3.0.0, it is
> -                * allowed to define both the 64-bit entry point (smbios3) and
> -                * the 32-bit entry point (smbios), in which case they should
> -                * either both point to the same SMBIOS structure table, or the
> -                * table pointed to by the 64-bit entry point should contain a
> -                * superset of the table contents pointed to by the 32-bit entry
> -                * point (section 5.2)
> -                * This implies that the 64-bit entry point should have
> -                * precedence if it is defined and supported by the OS. If we
> -                * have the 64-bit entry point, but fail to decode it, fall
> -                * back to the legacy one (if available)
> -                */
> -               if (efi.smbios3 != EFI_INVALID_TABLE_ADDR) {
> -                       p = dmi_early_remap(efi.smbios3, 32);
> -                       if (p == NULL)
> -                               goto error;
> -                       memcpy_fromio(buf, p, 32);
> -                       dmi_early_unmap(p, 32);
> -
> -                       if (!dmi_smbios3_present(buf)) {
> -                               dmi_available = 1;
> -                               return;
> -                       }
> -               }
> -               if (efi.smbios == EFI_INVALID_TABLE_ADDR)
> -                       goto error;
> -
> -               /* This is called as a core_initcall() because it isn't
> -                * needed during early boot.  This also means we can
> -                * iounmap the space when we're done with it.
> -                */
> -               p = dmi_early_remap(efi.smbios, 32);
> +       if (smbios3 != INVALID_TABLE_ADDR) {
> +               p = dmi_early_remap(smbios3, 32);
>                 if (p == NULL)
> -                       goto error;
> +                       return -1;
>                 memcpy_fromio(buf, p, 32);
>                 dmi_early_unmap(p, 32);
>
> -               if (!dmi_present(buf)) {
> +               if (!dmi_smbios3_present(buf)) {
>                         dmi_available = 1;
> -                       return;
> +                       return 0;
>                 }
> +       }
> +
> +       if (smbios == INVALID_TABLE_ADDR)
> +               return -1;
> +
> +       /*
> +        * This is called as a core_initcall() because it isn't
> +        * needed during early boot.  This also means we can
> +        * iounmap the space when we're done with it.
> +        */
> +       p = dmi_early_remap(smbios, 32);
> +       if (p == NULL)
> +               return -1;
> +       memcpy_fromio(buf, p, 32);
> +       dmi_early_unmap(p, 32);
> +
> +       if (!dmi_present(buf)) {
> +               dmi_available = 1;
> +               return 0;
> +       }
> +       return -1;
> +}
> +
> +static void __init dmi_scan_machine(void)
> +{
> +       char __iomem *p, *q;
> +       char buf[32];
> +
> +#ifdef CONFIG_FDT_FW_INTERFACE
> +       if (dmi_sacn_smbios(fdt_fwtbl.smbios3, fdt_fwtbl.smbios))
> +               goto error;
> +#endif
> +       if (efi_enabled(EFI_CONFIG_TABLES)) {
> +               if (dmi_sacn_smbios(efi.smbios3, efi.smbios))
> +                       goto error;
>         } else if (IS_ENABLED(CONFIG_DMI_SCAN_MACHINE_NON_EFI_FALLBACK)) {
>                 p = dmi_early_remap(SMBIOS_ENTRY_POINT_SCAN_START, 0x10000);
>                 if (p == NULL)
> diff --git a/drivers/firmware/ffi.c b/drivers/firmware/ffi.c
> new file mode 100644
> index 000000000000..83c7abf22220
> --- /dev/null
> +++ b/drivers/firmware/ffi.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kobject.h>
> +#include <linux/ffi.h>
> +#include <linux/of_fdt.h>
> +#include <linux/libfdt.h>
> +
> +struct fdt_fwtable __read_mostly fdt_fwtbl = {
> +       .acpi                   = FDT_INVALID_FWTBL_ADDR,
> +       .acpi20                 = FDT_INVALID_FWTBL_ADDR,
> +       .smbios                 = FDT_INVALID_FWTBL_ADDR,
> +       .smbios3                = FDT_INVALID_FWTBL_ADDR,
> +};
> +EXPORT_SYMBOL(fdt_fwtbl);
> +
> +void __init of_fdt_fwtbl(void)
> +{
> +       int cfgtbl, len;
> +       fdt64_t *prop;
> +
> +       cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
> +       if (cfgtbl < 0) {
> +               pr_info("cfgtables not found.\n");
> +               return;
> +       }
> +       prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios_phy_ptr", &len);
> +       if (!prop || len != sizeof(u64))
> +               pr_info("smbios_phy_ptr not found.\n");
> +       else
> +               fdt_fwtbl.smbios = fdt64_to_cpu(*prop);
> +
> +       prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios3_phy_ptr", &len);
> +       if (!prop || len != sizeof(u64))
> +               pr_info("smbios3_phy_ptr not found.\n");
> +       else
> +               fdt_fwtbl.smbios3 = fdt64_to_cpu(*prop);
> +
> +       prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi_phy_ptr", &len);
> +       if (!prop || len != sizeof(u64))
> +               pr_info("acpi_phy_ptr not found.\n");
> +       else
> +               fdt_fwtbl.acpi = fdt64_to_cpu(*prop);
> +
> +       prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi20_phy_ptr", &len);
> +       if (!prop || len != sizeof(u64))
> +               pr_info("acpi20_phy_ptr not found.\n");
> +       else
> +               fdt_fwtbl.acpi20 = fdt64_to_cpu(*prop);
> +}
> +
> +void __init fdt_fwtbl_init(void)
> +{
> +       of_fdt_fwtbl();
> +}
> diff --git a/include/linux/ffi.h b/include/linux/ffi.h
> new file mode 100644
> index 000000000000..ffb50810a01e
> --- /dev/null
> +++ b/include/linux/ffi.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _LINUX_FDT_FW_H
> +#define _LINUX_FDT_FW_H
> +
> +#define FDT_INVALID_FWTBL_ADDR         (~0UL)
> +extern struct fdt_fwtable {
> +       unsigned long                   acpi;
> +       unsigned long                   acpi20;
> +       unsigned long                   smbios;
> +       unsigned long                   smbios3;
> +       unsigned long                   flags;
> +} fdt_fwtbl;
> +
> +#endif
> --
> 2.20.1
>
  

Hi Yunhui,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on linus/master jdelvare-staging/dmi-for-next v6.3 next-20230425]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yunhui-Cui/firmware-added-a-firmware-information-passing-method-FFI/20230426-114131
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20230426034001.16-1-cuiyunhui%40bytedance.com
patch subject: [PATCH] firmware: added a firmware information passing method FFI
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230426/202304261756.84GsEW3V-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7d1fe633611738698520294d2a598575a765cfbf
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Yunhui-Cui/firmware-added-a-firmware-information-passing-method-FFI/20230426-114131
        git checkout 7d1fe633611738698520294d2a598575a765cfbf
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304261756.84GsEW3V-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/firmware/ffi.c:18:13: warning: no previous prototype for 'of_fdt_fwtbl' [-Wmissing-prototypes]
      18 | void __init of_fdt_fwtbl(void)
         |             ^~~~~~~~~~~~
>> drivers/firmware/ffi.c:53:13: warning: no previous prototype for 'fdt_fwtbl_init' [-Wmissing-prototypes]
      53 | void __init fdt_fwtbl_init(void)
         |             ^~~~~~~~~~~~~~


vim +/of_fdt_fwtbl +18 drivers/firmware/ffi.c

    17	
  > 18	void __init of_fdt_fwtbl(void)
    19	{
    20		int cfgtbl, len;
    21		fdt64_t *prop;
    22	
    23		cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
    24		if (cfgtbl < 0) {
    25			pr_info("cfgtables not found.\n");
    26			return;
    27		}
    28		prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios_phy_ptr", &len);
    29		if (!prop || len != sizeof(u64))
    30			pr_info("smbios_phy_ptr not found.\n");
    31		else
    32			fdt_fwtbl.smbios = fdt64_to_cpu(*prop);
    33	
    34		prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios3_phy_ptr", &len);
    35		if (!prop || len != sizeof(u64))
    36			pr_info("smbios3_phy_ptr not found.\n");
    37		else
    38			fdt_fwtbl.smbios3 = fdt64_to_cpu(*prop);
    39	
    40		prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi_phy_ptr", &len);
    41		if (!prop || len != sizeof(u64))
    42			pr_info("acpi_phy_ptr not found.\n");
    43		else
    44			fdt_fwtbl.acpi = fdt64_to_cpu(*prop);
    45	
    46		prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi20_phy_ptr", &len);
    47		if (!prop || len != sizeof(u64))
    48			pr_info("acpi20_phy_ptr not found.\n");
    49		else
    50			fdt_fwtbl.acpi20 = fdt64_to_cpu(*prop);
    51	}
    52	
  > 53	void __init fdt_fwtbl_init(void)
  

Hi Ard,

On Wed, Apr 26, 2023 at 3:09 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Hello Yunhui,
>
> I am not sure this is a good idea: this is clearly intended for arm64,
> which cannot use ACPI without the EFI memory map, which it uses to
> cross reference memory opregion accesses, to determine the correct
> memory type attributes.
>
Not only for arm64, but also other arches, such as riscv.
For memory-related nodes, it will be done in the early scan of the device tree.


> What is the use case you are trying to accommodate here?
>
Some bootloaders do not support uefi, such as coreboot,
but need to support acpi, smbios.


Thanks,
Yunhui
  

On Wed, Apr 26, 2023 at 05:34:55PM +0800, 运辉崔 wrote:
> Hi Ard,
> 
> On Wed, Apr 26, 2023 at 3:09 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > Hello Yunhui,
> >
> > I am not sure this is a good idea: this is clearly intended for arm64,
> > which cannot use ACPI without the EFI memory map, which it uses to
> > cross reference memory opregion accesses, to determine the correct
> > memory type attributes.
> >
> Not only for arm64, but also other arches, such as riscv.
> For memory-related nodes, it will be done in the early scan of the device tree.

Ard's point is that the device tree doesn't have all the same information (e.g.
nothing in DT describes the memory type attributes), and so this isn't
sufficient.

We'd have to create entirely new ways to pass that information, which is not
very desirable.

> > What is the use case you are trying to accommodate here?
> >
> Some bootloaders do not support uefi, such as coreboot,
> but need to support acpi, smbios.

For arm64 at least, if you need ACPI you must have EFI, and trying to change
that will require significant work and long term maintenance.

Can you extend coreboot to provide EFI services, or to chain-load an EFI
payload?

Thanks,
Mark.
  

Hi Mark,

On Wed, Apr 26, 2023 at 6:07 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> Ard's point is that the device tree doesn't have all the same information (e.g.
> nothing in DT describes the memory type attributes), and so this isn't
> sufficient.

The device tree only needs to complete the parse of the memory type attributes,
it should not be very complicated.

>
> We'd have to create entirely new ways to pass that information, which is not
> very desirable.
>

>
> Can you extend coreboot to provide EFI services, or to chain-load an EFI
> payload?

Currently, coreboot does not support UEFI, and it may not support it
in the future.
Hi rminnich, what do you think?

Thanks,
Yunhui
  

On Wed, Apr 26, 2023 at 09:04:56PM -0700, ron minnich wrote:
> The device tree is self describing data. Adding new information is easy. If you
> add new information to a node, and older software does not know about it, it is
> no big deal.

It's true that it's easy to add fields to an extensible format, but that wasn't
my point of contention. The *semantic* (e.g. all of the relevant DT bindings)
and *consumption* of that data is the important part, and that's what I was
referring to, though I appreciate my wording did not make that clear.

> So I can't agree with this comment: "We'd have to create entirely new ways to
> pass that information, which is not
> very desirable."
> 
> The whole point of the dt is that you can always add new ways to pass
> information, by design. 
> 
> Adding memory attributes would be quite easy.

I don't disagree that is physically possible, and in isolation adding
properties to a DT is trivial, but the approach proposed is not "easy" unless
you ignore the cost of specifying analogues for all the EFI portions that you
plan to omit, ensuring that those stay functionally equivalent to their EFI
analogues as EFI and ACPI evolve over time, developing and maintaining the code
which must consume that, avoiding the issues that will arise due to novel
interactions (as e.g. DT and ACPI are mutually exclusive today, by design),
etc.

For example, the UEFI memory map is semantically and structurally different
from DT memory nodes. It encodes *different* information, and in practice needs
to encode a larger number of physical extents with properties (e.g.
cacheability, permissions) associated with each extent. The existing DT memory
nodes format isn't really amenable to encoding this, inventing a parallel
structure for this opens up all the usual problems of the two becoming
out-of-sync, and inventing a new mechanism to describe all of this in a
consistent way duplicates all the work done for EFI.

I appreciate that at a high level of abstractions this seems conceptually
simple, but in practice this is a complex area where components have subtle and
often implicit dependencies, and so there is inherent fractal complexity.

Thanks,
Mark.

> On Wed, Apr 26, 2023 at 8:38 PM 运辉崔 <cuiyunhui@bytedance.com> wrote:
> 
>     Hi Mark,
> 
>     On Wed, Apr 26, 2023 at 6:07 PM Mark Rutland <mark.rutland@arm.com> wrote:
>     >
>     > Ard's point is that the device tree doesn't have all the same information
>     (e.g.
>     > nothing in DT describes the memory type attributes), and so this isn't
>     > sufficient.
> 
>     The device tree only needs to complete the parse of the memory type
>     attributes,
>     it should not be very complicated.
> 
>     >
>     > We'd have to create entirely new ways to pass that information, which is
>     not
>     > very desirable.
>     >
> 
>     >
>     > Can you extend coreboot to provide EFI services, or to chain-load an EFI
>     > payload?
> 
>     Currently, coreboot does not support UEFI, and it may not support it
>     in the future.
>     Hi rminnich, what do you think?
> 
>     Thanks,
>     Yunhui
>
  

On Thu, 27 Apr 2023 at 12:24, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Apr 26, 2023 at 09:04:56PM -0700, ron minnich wrote:
> > The device tree is self describing data. Adding new information is easy. If you
> > add new information to a node, and older software does not know about it, it is
> > no big deal.
>
> It's true that it's easy to add fields to an extensible format, but that wasn't
> my point of contention. The *semantic* (e.g. all of the relevant DT bindings)
> and *consumption* of that data is the important part, and that's what I was
> referring to, though I appreciate my wording did not make that clear.
>
> > So I can't agree with this comment: "We'd have to create entirely new ways to
> > pass that information, which is not
> > very desirable."
> >
> > The whole point of the dt is that you can always add new ways to pass
> > information, by design.
> >
> > Adding memory attributes would be quite easy.
>
> I don't disagree that is physically possible, and in isolation adding
> properties to a DT is trivial, but the approach proposed is not "easy" unless
> you ignore the cost of specifying analogues for all the EFI portions that you
> plan to omit, ensuring that those stay functionally equivalent to their EFI
> analogues as EFI and ACPI evolve over time, developing and maintaining the code
> which must consume that, avoiding the issues that will arise due to novel
> interactions (as e.g. DT and ACPI are mutually exclusive today, by design),
> etc.
>

Indeed. Currently, Linux/arm64 supports two boot methods

- direct kernel boot
- EFI boot

and two types of hardware descriptions

- device tree (DT)
- ACPI

and the only combination we do not support is ACPI without EFI, as
ACPI on arm64 depends on the EFI memory map.

What this patch seems to be proposing is a combination of all of
these, i.e., doing a pseudo-EFI direct kernel boot where the EFI
dependencies of ACPI are being fulfilled by ad-hoc descriptions passed
in via DT.

I am concerned that this will result in a maintenance burden for Linux
with very little gain, so I feel we should not go down this road.
  

Hi Ard, Mark,

On Thu, Apr 27, 2023 at 8:52 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> and the only combination we do not support is ACPI without EFI, as
> ACPI on arm64 depends on the EFI memory map.
>
> What this patch seems to be proposing is a combination of all of
> these, i.e., doing a pseudo-EFI direct kernel boot where the EFI
> dependencies of ACPI are being fulfilled by ad-hoc descriptions passed
> in via DT.
>
> I am concerned that this will result in a maintenance burden for Linux
> with very little gain, so I feel we should not go down this road.

Judging from the current kernel, getting acpi smbios, memmap tables is
not just a way to have EFI, right?
smbios:SMBIOS_ENTRY_POINT_SCAN_START
acpi:CONFIG_ACPI_LEGACY_TABLES_LOOKUP
memmap: e820

Our current situation is that coreboot does not support EFI, but supports fdt,
but we need to support ACPI,  and riscv does not have a reserved
address segment
like x86 that can be used, so our current solution is to pass acpi and
other tables through fdt.

Based on this, do you have a better suggestion ?

Thanks，
Yunhui
  

On Fri, 28 Apr 2023 at 17:09, ron minnich <rminnich@gmail.com> wrote:
>
> There is lots of text in the preceding notes :-), which is nice because we're clearly looking at something that matters!
>
> But, note, ARM Chromebooks run Linux, and I checked with the firmware team just now:
> "Right. We're not using UEFI or ACPI or SMBIOS or DMI or any of that on Arm. Just the Device Tree."
>
> So I do not agree that we need UEFI tables due to some presumed semantics that they implement, because: several tens of millions of ARM chromebooks running Linux show otherwise.
>
> We've got a chance here to move to self describing data, and I think we need to take it. It will be a long time before we get this chance again.
>

I'm not sure what you mean by self-describing: device tree is
definitely not self-describing, and we maintain a huge collection of
DT bindings (which are documented in separate YAML files) in the
kernel tree that specify in detail how a device tree must be
constructed in order to comply with the device tree based boot
protocol.

However, introducing such a binding for SMBIOS is perfectly
reasonable, although I would suggest that we don't copy the
SMBIOS/SMBIOS3 entry point address into the device tree (as this patch
does), but properly describe the memory region that contains the
actual SMBIOS structured data directly, along with its version. This
might be reused by other DT based platforms as well.

Doing the same for ACPI is where we'll get into trouble, given that
we'd end up with two conflicting hardware descriptions and unfulfilled
dependencies on EFI specific data structures, and it is not the
kernel's job to reason about which h/w description should take
precedence, or to make guesses about memory types. So I fully agree
with Ron that moving to device tree is a much better choice here -
that way, you can avoid ACPI and UEFI altogether




> On Thu, Apr 27, 2023 at 8:18 PM 运辉崔 <cuiyunhui@bytedance.com> wrote:
>>
>> Hi Ard, Mark,
>>
>> On Thu, Apr 27, 2023 at 8:52 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> > and the only combination we do not support is ACPI without EFI, as
>> > ACPI on arm64 depends on the EFI memory map.
>> >
>> > What this patch seems to be proposing is a combination of all of
>> > these, i.e., doing a pseudo-EFI direct kernel boot where the EFI
>> > dependencies of ACPI are being fulfilled by ad-hoc descriptions passed
>> > in via DT.
>> >
>> > I am concerned that this will result in a maintenance burden for Linux
>> > with very little gain, so I feel we should not go down this road.
>>
>> Judging from the current kernel, getting acpi smbios, memmap tables is
>> not just a way to have EFI, right?
>> smbios:SMBIOS_ENTRY_POINT_SCAN_START
>> acpi:CONFIG_ACPI_LEGACY_TABLES_LOOKUP
>> memmap: e820
>>
>> Our current situation is that coreboot does not support EFI, but supports fdt,
>> but we need to support ACPI,  and riscv does not have a reserved
>> address segment
>> like x86 that can be used, so our current solution is to pass acpi and
>> other tables through fdt.
>>
>> Based on this, do you have a better suggestion ?
>>
>> Thanks，
>> Yunhui
  

Hi Ron, Ard

On Sat, Apr 29, 2023 at 2:03 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 28 Apr 2023 at 17:09, ron minnich <rminnich@gmail.com> wrote:
> >
> > There is lots of text in the preceding notes :-), which is nice because we're clearly looking at something that matters!
> >
> > But, note, ARM Chromebooks run Linux, and I checked with the firmware team just now:
> > "Right. We're not using UEFI or ACPI or SMBIOS or DMI or any of that on Arm. Just the Device Tree."
> >
> > So I do not agree that we need UEFI tables due to some presumed semantics that they implement, because: several tens of millions of ARM chromebooks running Linux show otherwise.
> >
> > We've got a chance here to move to self describing data, and I think we need to take it. It will be a long time before we get this chance again.
> >

> However, introducing such a binding for SMBIOS is perfectly
> reasonable, although I would suggest that we don't copy the
> SMBIOS/SMBIOS3 entry point address into the device tree (as this patch
> does), but properly describe the memory region that contains the
> actual SMBIOS structured data directly, along with its version. This
> might be reused by other DT based platforms as well.

Could you help to give me an example to add smbios in the memmap
region description?

Even after adding to the memmap region, it needs to be connected to
the current dmi_scan_machine()?
Or add another dmi code under the fdt framework?


> Doing the same for ACPI is where we'll get into trouble, given that
> we'd end up with two conflicting hardware descriptions and unfulfilled
> dependencies on EFI specific data structures, and it is not the
> kernel's job to reason about which h/w description should take
> precedence, or to make guesses about memory types. So I fully agree
> with Ron that moving to device tree is a much better choice here -
> that way, you can avoid ACPI and UEFI altogether

Thanks for your suggestions, I don't quite get what needs to be moved
to dts? Could you explain in detail?
Is it to realize the content of acpi based on the dts framework?

Thanks,
Yunhui
  

On Thu, 4 May 2023 at 14:06, 运辉崔 <cuiyunhui@bytedance.com> wrote:
>
> Hi Ron, Ard
>
> On Sat, Apr 29, 2023 at 2:03 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Fri, 28 Apr 2023 at 17:09, ron minnich <rminnich@gmail.com> wrote:
> > >
> > > There is lots of text in the preceding notes :-), which is nice because we're clearly looking at something that matters!
> > >
> > > But, note, ARM Chromebooks run Linux, and I checked with the firmware team just now:
> > > "Right. We're not using UEFI or ACPI or SMBIOS or DMI or any of that on Arm. Just the Device Tree."
> > >
> > > So I do not agree that we need UEFI tables due to some presumed semantics that they implement, because: several tens of millions of ARM chromebooks running Linux show otherwise.
> > >
> > > We've got a chance here to move to self describing data, and I think we need to take it. It will be a long time before we get this chance again.
> > >
>
> > However, introducing such a binding for SMBIOS is perfectly
> > reasonable, although I would suggest that we don't copy the
> > SMBIOS/SMBIOS3 entry point address into the device tree (as this patch
> > does), but properly describe the memory region that contains the
> > actual SMBIOS structured data directly, along with its version. This
> > might be reused by other DT based platforms as well.
>
> Could you help to give me an example to add smbios in the memmap
> region description?
>

I'm not a DT expert, better to ask the maintainers for guidance.

> Even after adding to the memmap region, it needs to be connected to
> the current dmi_scan_machine()?
> Or add another dmi code under the fdt framework?
>

Yes. This should be generic code that permits any DT platform to
expose SMBIOS data without relying on EFI. So I would imagine that the
support code will live somewhere under drivers/of/ perhaps?

>
> > Doing the same for ACPI is where we'll get into trouble, given that
> > we'd end up with two conflicting hardware descriptions and unfulfilled
> > dependencies on EFI specific data structures, and it is not the
> > kernel's job to reason about which h/w description should take
> > precedence, or to make guesses about memory types. So I fully agree
> > with Ron that moving to device tree is a much better choice here -
> > that way, you can avoid ACPI and UEFI altogether
>
> Thanks for your suggestions, I don't quite get what needs to be moved
> to dts? Could you explain in detail?
> Is it to realize the content of acpi based on the dts framework?
>

Yes. You will have to duplicate the platform topology description,
including CPU's, interrupt controllers, PCI host bridges and all other
non-discoverable peripherals. You will lose some functionality
regarding hotplugging and RAS, because DT does not support that, but
otherwise, it should be quite feasible to replace ACPI entirely.

But Ron is the expert here on replacing vendor provided firmware with
open(er) source alternatives, so perhaps he has some ideas on how to
bridge this gap?
  

Thanks for Ron's suggestions.

Hi Ard,  Mark,
>
> Is there some feeling here that it would be ok to restrict this discussion to risc-v, and not bring in ARM considerations.  WDYT?
>

Hi Ard,  Mark,

Now the coreboot we are using does not support EFI and only supports
one interface DTB. It seems that we have to pass the firmware
information through DTB.

From another point of view, ACPI and SMBIOS are common modules of the
kernel, not only EFI, but also other interfaces can also be connected
to this module, such as 0xF0000 for SMBIOS,
CONFIG_ACPI_LEGACY_TABLES_LOOKUP for ACPI,  this patch is also.

We just use the DTB channel to add a few nodes to complete the
transfer of firmware information, which does not interfere with DTS
itself.

We think it is unnecessary to add an ACPI-supporting framework under
the fdt framework we discussed before. We only need one set of ACPI
framework, but one more set will cause unnecessary trouble.

So, let's move on to this patch, shall we?



Thanks,
Yunhui
  

On Wed, 21 Jun 2023 at 10:04, 运辉崔 <cuiyunhui@bytedance.com> wrote:
>
> Thanks for Ron's suggestions.
>
> Hi Ard,  Mark,
> >
> > Is there some feeling here that it would be ok to restrict this discussion to risc-v, and not bring in ARM considerations.  WDYT?
> >
>
> Hi Ard,  Mark,
>
> Now the coreboot we are using does not support EFI and only supports
> one interface DTB. It seems that we have to pass the firmware
> information through DTB.
>
> From another point of view, ACPI and SMBIOS are common modules of the
> kernel, not only EFI, but also other interfaces can also be connected
> to this module, such as 0xF0000 for SMBIOS,
> CONFIG_ACPI_LEGACY_TABLES_LOOKUP for ACPI,  this patch is also.
>
> We just use the DTB channel to add a few nodes to complete the
> transfer of firmware information, which does not interfere with DTS
> itself.
>
> We think it is unnecessary to add an ACPI-supporting framework under
> the fdt framework we discussed before. We only need one set of ACPI
> framework, but one more set will cause unnecessary trouble.
>
> So, let's move on to this patch, shall we?
>

How do you intend to provide the ACPI core with the memory attribute
information that it needs to access SystemMemory OpRegions?
  

Hi Ard,

On Sat, Jun 24, 2023 at 8:52 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> How do you intend to provide the ACPI core with the memory attribute
> information that it needs to access SystemMemory OpRegions?

Regarding memory segments and attributes, our solution does not need
to build a memmap table in coreboot like EFI to connect to linux ACPI
core.

Because the memory segment and attributes have been passed through the
"memory" node and "reserved-memory" node attributes of DTS.

For Linux, no matter what kind of memory attributes of the firmware,
it is ultimately connected to the memblock module.

So the memory attributes you consider can be done through the existing
DTS (like Ron said before, Chrombook does everything through DTS).

So can we come to a consensus? Then start reviewing the code?

Thanks,
Yunhui
  

On Sun, 25 Jun 2023 at 09:33, 运辉崔 <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Sat, Jun 24, 2023 at 8:52 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > How do you intend to provide the ACPI core with the memory attribute
> > information that it needs to access SystemMemory OpRegions?
>
> Regarding memory segments and attributes, our solution does not need
> to build a memmap table in coreboot like EFI to connect to linux ACPI
> core.
>

So how do you expect the code below will behave?

arch/arm64/kernel/acpi.c:
270:void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
271-{
272-   efi_memory_desc_t *md, *region = NULL;
273-   pgprot_t prot;
274-
275-   if (WARN_ON_ONCE(!efi_enabled(EFI_MEMMAP)))
276-           return NULL;

acpi_os_ioremap() is used by all ACPI core code that needs to map MMIO
regions or DRAM from AML code. AML does not pass memory type
attributes, so we have to consult the EFI memory map for these.

As I have explained to you multiple times, ACPI on arm64 is *broken*
without the EFI memory map.

> Because the memory segment and attributes have been passed through the
> "memory" node and "reserved-memory" node attributes of DTS.
>
> For Linux, no matter what kind of memory attributes of the firmware,
> it is ultimately connected to the memblock module.
>

Incorrect. We are talking about any physical region here, not just
DRAM. And some DRAM regions may not be covered by memblock.

> So the memory attributes you consider can be done through the existing
> DTS (like Ron said before, Chrombook does everything through DTS).
>
> So can we come to a consensus? Then start reviewing the code?
>

No, sorry. Please try to understand the objections that I am raising
first. I am not saying this to annoy you, I am saying this because
your approach is flawed.
  

Hi Ard,

On Sun, Jun 25, 2023 at 3:43 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> acpi_os_ioremap() is used by all ACPI core code that needs to map MMIO
> regions or DRAM from AML code. AML does not pass memory type
> attributes, so we have to consult the EFI memory map for these.
>
> As I have explained to you multiple times, ACPI on arm64 is *broken*
> without the EFI memory map.
>

As Ron's suggested:
"...
It would be nice to separate those pieces on RISC-V; certainly they
were separate for a very long time in the x86 world (we had ACPI+SMM
on coreboot laptops without UEFI for example)
...
"

If it cannot be solved temporarily on arm64, then we cannot let it
continue to be bound in RISC-V.
And on the linux-next branch, RISC-V arch is not bound to EFI.
void *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
{
        return memremap(phys, size, MEMREMAP_WB);
}



>
> Incorrect. We are talking about any physical region here, not just
> DRAM. And some DRAM regions may not be covered by memblock.
>
It is very strange that so many devices can complete the hardware
description through DTS without the problem you mentioned.
Even if there is, then it shouldn't be the problem that this patch
should solve, should it?

> No, sorry. Please try to understand the objections that I am raising
> first. I am not saying this to annoy you, I am saying this because
> your approach is flawed.

The implementation is right in front of us, we need to support ACPI on
RISC-V based on coreboot.

Thanks,
Yunhui
  

On Sun, 25 Jun 2023 at 13:54, 运辉崔 <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Sun, Jun 25, 2023 at 3:43 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > acpi_os_ioremap() is used by all ACPI core code that needs to map MMIO
> > regions or DRAM from AML code. AML does not pass memory type
> > attributes, so we have to consult the EFI memory map for these.
> >
> > As I have explained to you multiple times, ACPI on arm64 is *broken*
> > without the EFI memory map.
> >
>
> As Ron's suggested:
> "...
> It would be nice to separate those pieces on RISC-V; certainly they
> were separate for a very long time in the x86 world (we had ACPI+SMM
> on coreboot laptops without UEFI for example)
> ...
> "
>
> If it cannot be solved temporarily on arm64, then we cannot let it
> continue to be bound in RISC-V.
> And on the linux-next branch, RISC-V arch is not bound to EFI.
> void *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> {
>         return memremap(phys, size, MEMREMAP_WB);
> }
>
>
>
> >
> > Incorrect. We are talking about any physical region here, not just
> > DRAM. And some DRAM regions may not be covered by memblock.
> >
> It is very strange that so many devices can complete the hardware
> description through DTS without the problem you mentioned.
> Even if there is, then it shouldn't be the problem that this patch
> should solve, should it?
>
> > No, sorry. Please try to understand the objections that I am raising
> > first. I am not saying this to annoy you, I am saying this because
> > your approach is flawed.
>
> The implementation is right in front of us, we need to support ACPI on
> RISC-V based on coreboot.
>

If this is only used on RISC-V, and implemented under arch/riscv, I
have no objections.
  

Hi Ron, Ard,

On Sun, Jun 25, 2023 at 11:57 PM ron minnich <rminnich@gmail.com> wrote:
>
> Hey Ard, thanks for the discussion, sounds like we are able to move forward now!

>
> On Sun, Jun 25, 2023, 6:13 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> If this is only used on RISC-V, and implemented under arch/riscv, I
>> have no objections.

Thank you for your suggestions that made us reach an agreement, let's
continue to review this patch.

The current logic is to implement the common interface under
drivers/firmware/, if we need this function, we can call
fdt_fwtbl_init() to complete it in arch/xxx/kernel/setup.c.

For enabling on RISC-V, we can complete it in a subsequent patch to
setup_arch-->fdt_fwtbl_init() in arch/riscv/kernel/setup.c.

What do you think?

Thanks,
Yunhui
  

On Mon, 26 Jun 2023 at 04:35, 运辉崔 <cuiyunhui@bytedance.com> wrote:
>
> Hi Ron, Ard,
>
> On Sun, Jun 25, 2023 at 11:57 PM ron minnich <rminnich@gmail.com> wrote:
> >
> > Hey Ard, thanks for the discussion, sounds like we are able to move forward now!
>
> >
> > On Sun, Jun 25, 2023, 6:13 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> If this is only used on RISC-V, and implemented under arch/riscv, I
> >> have no objections.
>
> Thank you for your suggestions that made us reach an agreement, let's
> continue to review this patch.
>
> The current logic is to implement the common interface under
> drivers/firmware/, if we need this function, we can call
> fdt_fwtbl_init() to complete it in arch/xxx/kernel/setup.c.
>
> For enabling on RISC-V, we can complete it in a subsequent patch to
> setup_arch-->fdt_fwtbl_init() in arch/riscv/kernel/setup.c.
>
> What do you think?
>

I think all of this belongs under arch/riscv
  

Hi Ard,

On Mon, Jun 26, 2023 at 2:43 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> I think all of this belongs under arch/riscv

Could you look at the content of the patch again? As we discussed
before, we need to connect to the ACPI and the SMBIOS entry
At least this part of the code has to be placed in the corresponding place:
drivers/acpi/osl.c: acpi_os_get_root_pointer()
drivers/firmware/dmi_scan.c:dmi_scan_machine()

Because obtaining firmware information through DTS belongs to the
content of the driver firmware, it is appropriate to put this piece of
code in drivers/firmware/ffi.c.

So I insist on the current revision, what do you think?

Thanks,
Yunhui
  

On Mon, 26 Jun 2023 at 10:05, 运辉崔 <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Mon, Jun 26, 2023 at 2:43 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > I think all of this belongs under arch/riscv
>
> Could you look at the content of the patch again? As we discussed
> before, we need to connect to the ACPI and the SMBIOS entry
> At least this part of the code has to be placed in the corresponding place:
> drivers/acpi/osl.c: acpi_os_get_root_pointer()
> drivers/firmware/dmi_scan.c:dmi_scan_machine()
>
> Because obtaining firmware information through DTS belongs to the
> content of the driver firmware, it is appropriate to put this piece of
> code in drivers/firmware/ffi.c.
>
> So I insist on the current revision, what do you think?
>

DT support for SMBIOS can live in generic code, but the binding has to
be sane. As I suggested before, it probably makes sense to supplant
the entrypoint rather than just carry its address - this means a 'reg'
property with base and size to describe the physical region, and at
least major/minor/docrev fields to describe the version.

In any case, there is really no point in supporting both entrypoints
(this applies to the ACPI root pointer as well).

For the ACPI side, you should just implement
acpi_arch_get_root_pointer() under arch/riscv, and wire it up in
whichever way you want. 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.

So NAK on the current revision, in case this was unclear.
  

Hi Ard, Mark,

On Mon, Jun 26, 2023 at 4:23 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> DT support for SMBIOS can live in generic code, but the binding has to
> be sane. As I suggested before, it probably makes sense to supplant
> the entrypoint rather than just carry its address - this means a 'reg'
> property with base and size to describe the physical region, and at
> least major/minor/docrev fields to describe the version.

Regarding dts node binding, our current definition is as follows:
/dts
{
...
cfgtables {
acpi_phy_ptr = 0000000000000000; //u64
smbios_phy_ptr = 0000000000000000; //u64
...
}
...
}

x86 only gave a root_pointer entry address
u64 x86_default_get_root_pointer(void)
{
       return boot_params.acpi_rsdp_addr;
}

Regarding the naming of the binding above, Mark,  do you have any suggestions?


> For the ACPI side, you should just implement
> acpi_arch_get_root_pointer() under arch/riscv, and wire it up in
> whichever way you want. 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.

You suggest putting SMBIOS in general code instead of ACPI, why?
From the perspective of firmware information passing, they are a class.

SMBIOS and ACPI are not related to ARCH, nor is DTS to obtain firmware
information,

Why do you have to put part of the ACPI code under arch/risc-v/?
The scope of the previous discussion was limited to RISC-V because of
historical reasons such as the binding with EFI on ARM64. We will only
enable this function on RISC-V in subsequent patches.

The realization of the FFI scheme itself is irrelevant to the arch.

Thanks，
Yunhui
  

Hi Ard,

1. Regarding the definition of DTS FFI nodes, according to your
suggestion, we plan to make the following modifications：

/ {
...
    ffi_cfg {
        acpi_tbl {
            root_pinter = ; //u64
            ...
        };
        smbios_tbl {
            root_pinter = ; //u64
            ...
        };
    };
    ...
};

2. Let's move on to the discussion: should we put code under arch/risc-v/?

On Mon, Jun 26, 2023 at 6:19 PM 运辉崔 <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard, Mark,
>
> On Mon, Jun 26, 2023 at 4:23 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > DT support for SMBIOS can live in generic code, but the binding has to
> > be sane. As I suggested before, it probably makes sense to supplant
> > the entrypoint rather than just carry its address - this means a 'reg'
> > property with base and size to describe the physical region, and at
> > least major/minor/docrev fields to describe the version.
>
> Regarding dts node binding, our current definition is as follows:
> /dts
> {
> ...
> cfgtables {
> acpi_phy_ptr = 0000000000000000; //u64
> smbios_phy_ptr = 0000000000000000; //u64
> ...
> }
> ...
> }
>
> x86 only gave a root_pointer entry address
> u64 x86_default_get_root_pointer(void)
> {
>        return boot_params.acpi_rsdp_addr;
> }
>
> Regarding the naming of the binding above, Mark,  do you have any suggestions?
>
>
> > For the ACPI side, you should just implement
> > acpi_arch_get_root_pointer() under arch/riscv, and wire it up in
> > whichever way you want. 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.
>
> You suggest putting SMBIOS in general code instead of ACPI, why?
> From the perspective of firmware information passing, they are a class.
>
> SMBIOS and ACPI are not related to ARCH, nor is DTS to obtain firmware
> information,
>
> Why do you have to put part of the ACPI code under arch/risc-v/?
> The scope of the previous discussion was limited to RISC-V because of
> historical reasons such as the binding with EFI on ARM64. We will only
> enable this function on RISC-V in subsequent patches.
>
> The realization of the FFI scheme itself is irrelevant to the arch.
>
> Thanks，
> Yunhui
  

(cc RISC-V maintainers and mailing list)

On Mon, 26 Jun 2023 at 12:20, 运辉崔 <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard, Mark,
>
> On Mon, Jun 26, 2023 at 4:23 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > DT support for SMBIOS can live in generic code, but the binding has to
> > be sane. As I suggested before, it probably makes sense to supplant
> > the entrypoint rather than just carry its address - this means a 'reg'
> > property with base and size to describe the physical region, and at
> > least major/minor/docrev fields to describe the version.
>
> Regarding dts node binding, our current definition is as follows:
> /dts
> {
> ...
> cfgtables {
> acpi_phy_ptr = 0000000000000000; //u64
> smbios_phy_ptr = 0000000000000000; //u64
> ...
> }
> ...
> }
>
> x86 only gave a root_pointer entry address
> u64 x86_default_get_root_pointer(void)
> {
>        return boot_params.acpi_rsdp_addr;
> }
>
> Regarding the naming of the binding above, Mark,  do you have any suggestions?
>

I will defer to Mark or other DT experts to help decide on the naming
and general shape of these.

However, as I have indicated twice now, it would be better to describe
the SMBIOS structured data directly, instead of passing the physical
address of one of the existing entry points. This avoids the need for
mapping and reserving additional pages that don't carry any relevant
information.

So the node in question should have at least (base, size) and the
major, minor and docrev version fields.

>
> > For the ACPI side, you should just implement
> > acpi_arch_get_root_pointer() under arch/riscv, and wire it up in
> > whichever way you want. 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.
>
> You suggest putting SMBIOS in general code instead of ACPI, why?

SMBIOS is a separate set of firmware tables that have little
significance to the kernel itself, and describing it via DT makes
sense.

ACPI serves a similar purpose as DT, and so having both at the same
time results in a maintenance burden, where the arch code is forced to
reason about whether they are consistent with each other, and if not,
which description has precedence.

> From the perspective of firmware information passing, they are a class.
>
> SMBIOS and ACPI are not related to ARCH, nor is DTS to obtain firmware
> information,
>
> Why do you have to put part of the ACPI code under arch/risc-v/?

Yes. And I don't think it should be using this FFI scheme either.

If the firmware uses DT as a conduit to deliver the ACPI system
description to the OS, it is probably better to pass this via the
/chosen node as a special boot argument.

> The scope of the previous discussion was limited to RISC-V because of
> historical reasons such as the binding with EFI on ARM64. We will only
> enable this function on RISC-V in subsequent patches.
>
> The realization of the FFI scheme itself is irrelevant to the arch.
>

I don't think we need a FFI scheme or framework for this.
  

Hi Ard,

>
> I will defer to Mark or other DT experts to help decide on the naming
> and general shape of these.

Okay, thanks.


> However, as I have indicated twice now, it would be better to describe
> the SMBIOS structured data directly, instead of passing the physical
> address of one of the existing entry points. This avoids the need for
> mapping and reserving additional pages that don't carry any relevant
> information.
>
> So the node in question should have at least (base, size) and the
> major, minor and docrev version fields.

Other platforms also need related memory to store this table, don't they?
Coreboot also completes the construction of this table according to
its existing code, rather than creating a new description method.

Furthermore, As we discussed before, SMBIOS-related code should be
placed in the general code, and an entry address is required to
connect to dmi_scan_machine().
according to what you said (base, size, region) how can it be
connected to dmi_scan with an entry address?

So, For SMBIOS, only keep the smbios part in drivers/firmware/ffi.c in
this patch? If yes in terms of code structure, I will update it in v2.


> SMBIOS is a separate set of firmware tables that have little
> significance to the kernel itself, and describing it via DT makes
> sense.
>
> ACPI serves a similar purpose as DT, and so having both at the same
> time results in a maintenance burden, where the arch code is forced to
> reason about whether they are consistent with each other, and if not,
> which description has precedence.
>

Well... I don’t want to discuss too much, according to your
suggestion, To implement acpi_arch_get_root_pointer() under
arch/riscv.
I'll update it on v2.

> If the firmware uses DT as a conduit to deliver the ACPI system
> description to the OS, it is probably better to pass this via the
> /chosen node as a special boot argument.
>

This is not the focus of our discussion this time, and we will discuss
it when we discuss node naming with DTS experts.


Thanks,
Yunhui