Re: [PATCH v0] Export Runtime Configuration Interface table to sysfs

Narendra.K
2019-07-08T12:26:43Z

On Tue, Jul 02, 2019 at 09:57:24PM +0200, Ard Biesheuvel wrote:
> On Sat, 29 Jun 2019 at 13:23, <Narendra.K@dell.com> wrote:
> >
> > From: Narendra K <Narendra.K@dell.com>
> >
> > System firmware advertises the address of the 'Runtime
> > Configuration Interface table version 2 (RCI2)' via
> > an EFI Configuration Table entry. This code retrieves the RCI2
> > table from the address and exports it to sysfs as a binary
> > attribute 'rci2' under /sys/firmware/efi/tables directory.
> > The approach adopted is similar to the attribute 'DMI' under
> > /sys/firmware/dmi/tables.
> >
> > RCI2 table contains BIOS HII in XML format and is used to populate
> > BIOS setup page in Dell EMC OpenManage Server Administrator tool.
> > The BIOS setup page contains BIOS tokens which can be configured.
> >
> > Signed-off-by: Narendra K <Narendra.K@dell.com>
> > ---
> > RFC -> v0:
> >
> > - Removed rci2 table from struct efi and defined it in rci2_table.c similar to
> > the way uv_systab_phys is define in dmesg.
> > - Removed the oem_tables array and added rci2 to common_tables array
> > - Removed the string 'rci2' from the common_tables array so that it is
> > not printed in dmesg.
> > - Merged function 'efi_rci2_table_init' into 'efi_rci2_sysfs_init' function to
> > avoid calling early_memremap/unmap functions.
> >
> > Also, a note unrelated to this patch - compilation error is observed when
> > testing with make defconfig related to uv_systab_phys in
> > arch/x86/platform/efi/efi.c. It seems like it needs to be protected with
> > CONFIG_X86_UV in efi_tables array.
> >
>
> Yes, I noticed this as well. This has been fixed now
>
> >
> > Documentation/ABI/testing/sysfs-firmware-efi | 8 +
> > arch/x86/platform/efi/efi.c | 1 +
> > drivers/firmware/efi/Makefile | 2 +-
> > drivers/firmware/efi/efi.c | 4 +
> > drivers/firmware/efi/rci2_table.c | 147 +++++++++++++++++++
> > include/linux/efi.h | 6 +
> > 6 files changed, 167 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/firmware/efi/rci2_table.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
> > index e794eac32a90..f7822c522a46 100644
> > --- a/Documentation/ABI/testing/sysfs-firmware-efi
> > +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> > @@ -28,3 +28,11 @@ Description: Displays the physical addresses of all EFI Configuration
> > versions are always printed first, i.e. ACPI20 comes
> > before ACPI.
> > Users: dmidecode
> > +
> > +What: /sys/firmware/efi/tables/rci2
> > +Date: June 2019
> > +Contact: Narendra K <Narendra.K@dell.com>, linux-bugs@dell.com
> > +Description: Displays the content of the Runtime Configuration Interface
> > + Table version 2 on Dell EMC PowerEdge systems in binary format
> > +Users: It is used by Dell EMC OpenManage Server Administrator tool to
> > + populate BIOS setup page.
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > index 002078645969..6e1c1b0ce015 100644
> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -80,6 +80,7 @@ static const unsigned long * const efi_tables[] = {
> > &efi.esrt,
> > &efi.properties_table,
> > &efi.mem_attr_table,
> > + &rci2_table_phys,
>
> Put #ifdef CONFIG_EFI_RCI2 around this line ^^^
>
> > };
> >
> > u64 efi_setup; /* efi setup_data physical address */
> > diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> > index d2d0d2030620..db07828ca1ed 100644
> > --- a/drivers/firmware/efi/Makefile
> > +++ b/drivers/firmware/efi/Makefile
> > @@ -11,7 +11,7 @@
> > KASAN_SANITIZE_runtime-wrappers.o := n
> >
> > obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
> > -obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o tpm.o
> > +obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o tpm.o rci2_table.o
>
> Please introduce a kconfig symbol CONFIG_EFI_RCI2 for this, and only
> include this file when the symbol is set. You can make it default to y
> on X86, but for other architectures, it does not make a lot of sense
> to include this by default.

Apologies for the delay in responding. I will add the kconfig option and
related changes in the next version of the patch. I think the config
option could be N by default with a note added in the help section to
set it to Y for Dell EMC PowerEdge systems. Please share your thoughts
on setting it to N by default.

[...]

--
With regards,
Narendra K