Re: [External] Re: [PATCH v2 2/3] firmware: introduce FFI for SMBIOS entry.

From: Conor Dooley
Date: Mon Jul 03 2023 - 04:36:38 EST

Hey,

On Mon, Jul 03, 2023 at 04:23:53PM +0800, 运辉崔 wrote:
>
> > nit: please don't write your commit messages as bullet lists
> Okay, thanks for your suggestion.
>
> > > +FDT FIRMWARE INTERFACE (FFI)
> > > +M: Yunhui Cui cuiyunhui@xxxxxxxxxxxxx
> > > +S: Maintained
> > > +F: drivers/firmware/ffi.c
> > > +F: include/linux/ffi.h
> >
> > Are you going to apply patches for this, or is someone else?
> Yes, it will be used by patch 3/3.

That's not what I asked :(

> > > EXTERNAL CONNECTOR SUBSYSTEM (EXTCON)
> > > M: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
> > > M: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
> > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > > index b59e3041fd62..ea0149fb4683 100644
> > > --- a/drivers/firmware/Kconfig
> > > +++ b/drivers/firmware/Kconfig
> > > @@ -303,6 +303,17 @@ 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 EFI, 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 smbios tables.
> >
> > Could you express this dependency on !SMBIOS_ENTRY_POINT_SCAN_START in
> > Kconfig & then simply the text to:
> > "Enable this option to support the transfer of firmware information,
> > such as smbios tables, for bootloaders that do not support EFI."
> > since it would not even appear if the arch supports scanning for the
> > entry point?
> > If I was was a punter trying to configure my kernel in menuconfig or
> > whatever, I should be able to decide based on the help text if I need
> > this, not going grepping for #defines in headers.
> Okay， I'll update on v3.
>
>
> >
> > > static void __init dmi_scan_machine(void)
> > > @@ -660,58 +686,22 @@ static void __init dmi_scan_machine(void)
> > > char __iomem *p, *q;
> > > char buf[32];
> > >
> > > +#ifdef CONFIG_FDT_FW_INTERFACE
> > > + if (dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
> >
> > "dmi_sacn_smbios"
> >
> > > + goto error;
> > > +#endif
> >
> > Does this not mean that if FDT_FW_INTERFACE is enabled, but the platform
> > wants to use EFI, it won't be able to? The `goto error;` makes this look
> > mutually exclusive to my efi-unaware eyes.
>
> If you have enabled FFI, then if something goes wrong, you should goto error.
> Just like the origin code:
> 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)
> goto error;

Does this not make FFI and EFI mutually exclusive Kconfig options?
Suppose you are on a system that does not implement FFI, but does
implement EFI - what's going to happen then?
AFAICT, dmi_sacn_smbios(ffi.smbios3, ffi.smbios) will fail & you'll do a
`goto error` & skip the EFI code. What am I missing?

> > > 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)
> > > + if (dmi_sacn_smbios(efi.smbios3, efi.smbios))
> > > 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 (p == NULL)
> > > - goto error;
> > > - memcpy_fromio(buf, p, 32);
> > > - dmi_early_unmap(p, 32);
> > > -
> > > - if (!dmi_present(buf)) {
> > > - dmi_available = 1;
> > > - return;
> > > - }
> > > diff --git a/drivers/firmware/ffi.c b/drivers/firmware/ffi.c
> > > new file mode 100644
> > > index 000000000000..169802b4a7a8
> > > --- /dev/null
> > > +++ b/drivers/firmware/ffi.c
> > > @@ -0,0 +1,36 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > > +#include <linux/of.h>
> > > +#include <linux/of_fdt.h>
> > > +#include <linux/libfdt.h>
> > > +#include <linux/ffi.h>
> > > +
> > > +#define FFI_INVALID_TABLE_ADDR (~0UL)
> > > +
> > > +struct ffi __read_mostly ffi = {
> > > + .smbios = FFI_INVALID_TABLE_ADDR,
> > > + .smbios3 = FFI_INVALID_TABLE_ADDR,
> > > +};
> >
> > > +EXPORT_SYMBOL(ffi);
> >
> > > +// SPDX-License-Identifier: GPL-2.0-only
> >
> > Why not EXPORT_SYMBOL_GPL? But also, who is the user of this export?
> Just like efi.

I don't really understand how that is an answer to the questions.

> > > +
> > > +void __init ffi_smbios_root_pointer(void)
> > > +{
> > > + int cfgtbl, len;
> > > + fdt64_t *prop;
> > > +
> > > + cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables");
> >
> > These DT properties need to be documented in a binding.
> >
> > > + if (cfgtbl < 0) {
> > > + pr_info("firmware table not found.\n");
> >
> > Isn't it perfectly valid for a DT not to contain this table? This print
> > should be, at the very least, a pr_debug().
> >
> > > + return;
> > > + }
> > > + prop = fdt_getprop_w(initial_boot_params, cfgtbl, "smbios_phy_ptr", &len);
> >
> > Again, undocumented DT property. Please document them in a binding.
> Okay, I'll add them into binding.
>
>
> >
> > > + if (!prop || len != sizeof(u64))
> > > + pr_info("smbios entry point not found.\n");
> > > + else
> > > + ffi.smbios = fdt64_to_cpu(*prop);
> > > +
> > > + pr_info("smbios root pointer: %lx\n", ffi.smbios);
> >
> > ffi.smbios is not set if (!prop || len != sizeof(u64)), looks like your
> > "if" should return and the contents of the else become unconditional?
> > Otherwise, this print seems wrong.

> OK, I will optimize this logic and print.

It's not an optimisation. If the if branch of your code is taken, it
currently will do
pr_info("smbios entry point not found.\n");
pr_info("smbios root pointer: %lx\n", ffi.smbios);
which makes no sense...

Thanks,
Conor.

Attachment: signature.asc
Description: PGP signature