[PATCH v2 2/2] efi_loader: provide tool to dump SMBIOS table

[Simon Glass]

Hi Heinrich,

On Mon, Jan 1, 2024 at 4:23 PM Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 1/1/24 23:41, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, Jan 1, 2024 at 11:50 AM Heinrich Schuchardt
> > <heinrich.schuchardt at canonical.com> wrote:
> >>
> >> An EFI binary dmidump.efi is provided that can be used to check the SMBIOS
> >> table for consistency and to dump it as a file.
> >>
> >> The tool provides the following commands:
> >>
> >> check
> >>      Check the SMBIOS table for consistency.
> >>
> >> exit
> >>      Leave the tool.
> >>
> >> help
> >>      Show available commands.
> >>
> >> save
> >>      Save the SMBIOS table to a file on the EFI system partition. The file
> >>      can be further analyzed with the dmidecode command line tool::
> >>
> >>          dmidecode --from-dump <filename>
> >>
> >> Specifying 'nocolor' as load option data suppresses colored output and
> >> clearing of the screen.
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> >> ---
> >> v2:
> >>          fix an incorrect variable usage when checking the result of memcmp
> >> ---
> >>   lib/efi_loader/Makefile  |   7 +
> >>   lib/efi_loader/dmidump.c | 595 +++++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 602 insertions(+)
> >>   create mode 100644 lib/efi_loader/dmidump.c
> >
> > Does this tool need tests?
>
> Once we have all the SMBIOS stuff in, we could use the tool in a test to
> verify that the SMBIOS tables are correctly installed.

Yes that is useful for the EFI side of things, along with your new
test for smbios within U-Boot.

>
> >
> > Can you please split up do_save() a bit and also move the command loop
> > into its own function? Otherwise it looks good to me.
> >
> >>
> >> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> >> index 24d33d5409..71880d330e 100644
> >> --- a/lib/efi_loader/Makefile
> >> +++ b/lib/efi_loader/Makefile
> >> @@ -16,6 +16,8 @@ CFLAGS_boothart.o := $(CFLAGS_EFI) -Os -ffreestanding
> >>   CFLAGS_REMOVE_boothart.o := $(CFLAGS_NON_EFI)
> >>   CFLAGS_helloworld.o := $(CFLAGS_EFI) -Os -ffreestanding
> >>   CFLAGS_REMOVE_helloworld.o := $(CFLAGS_NON_EFI)
> >> +CFLAGS_dmidump.o := $(CFLAGS_EFI) -Os -ffreestanding
> >> +CFLAGS_REMOVE_dmidump.o := $(CFLAGS_NON_EFI)
> >>   CFLAGS_dtbdump.o := $(CFLAGS_EFI) -Os -ffreestanding
> >>   CFLAGS_REMOVE_dtbdump.o := $(CFLAGS_NON_EFI)
> >>   CFLAGS_initrddump.o := $(CFLAGS_EFI) -Os -ffreestanding
> >
> > Is there a way to have a list of these tools such that the flags stuff
> > is done automatically and doesn't need to be repeated each time?
>
> Looking at
>
> scripts/Makefile.lib:102:
> _c_flags = $(filter-out $(CFLAGS_REMOVE_$(basetarget).o), $(orig_c_flags))
>
> this is not possible.

Not with that rule, but perhaps another could be added. Anyway, I
suppose we can put up with it for now.

>
> >
> >> @@ -31,6 +33,11 @@ always += helloworld.efi
> >>   targets += helloworld.o
> >>   endif
> >>
> >> +ifneq ($(CONFIG_GENERATE_SMBIOS_TABLE),)
> >> +always += dmidump.efi
> >> +targets += dmidump.o
> >
> > How about smbios_tool ? I think 'dmi' is a bit of an obfuscation.
>
> We can call it smbiosdump.efi.

Yes that is better.


>
> Thanks for reviewing.
>
> Best regards
>
> Heinrich
>
> >
> >> +endif
> >> +
> >>   ifeq ($(CONFIG_GENERATE_ACPI_TABLE),)
> >>   always += dtbdump.efi
> >>   targets += dtbdump.o
> >
> > [..]

Regards,
Simon