| Message ID | 20240117153347.85074-2-heinrich.schuchardt@canonical.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2,1/4] smbios: type2: contained object handles | expand |
Hi Heinrich, A few nits below On Wed, Jan 17, 2024 at 04:33:45PM +0100, Heinrich Schuchardt wrote: > U-Boot can either generated an SMBIOS table or copy it from a prior boot > stage, e.g. QEMU. > > Provide a command to display the SMBIOS information. > > Currently only type 1 and 2 are translated to human readable text. > Other types may be added later. Currently only a hexdump and the list of > strings is provided for these. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > Reviewed-by: Simon Glass <sjg@chromium.org> > --- > v2: [...] > email address updated > +static struct smbios_header *next_table(struct smbios_header *table) > +{ > + const char *str; > + > + if (table->type == SMBIOS_END_OF_TABLE) > + return NULL; > + > + str = smbios_get_string(table, 0); > + return (struct smbios_header *)(++str); That can lead to unaligned access when we dereference that pointer, do we care? > +} > + > +static void smbios_print_generic(struct smbios_header *table) > +{ > + char *str = (char *)table + table->length; > + Do we want the header below printed if there are no strings? IOW can we exit early if (!*str) ? > + if (CONFIG_IS_ENABLED(HEXDUMP)) { > + printf("Header and Data:\n"); > + print_hex_dump("\t", DUMP_PREFIX_OFFSET, 16, 1, > + table, table->length, false); > + } > + if (*str) { > + printf("Strings:\n"); > + for (int index = 1; *str; ++index) { > + printf("\tString %u: %s\n", index, str); > + str += strlen(str) + 1; > + } > + } > +} > + > +void smbios_print_str(const char *label, void *table, u8 index) > +{ > + printf("\t%s: %s\n", label, smbios_get_string(table, index)); > +} > + Other than that, LGTM Thanks /Ilias
On 1/18/24 13:39, Ilias Apalodimas wrote: > Hi Heinrich, > > A few nits below > > On Wed, Jan 17, 2024 at 04:33:45PM +0100, Heinrich Schuchardt wrote: >> U-Boot can either generated an SMBIOS table or copy it from a prior boot >> stage, e.g. QEMU. >> >> Provide a command to display the SMBIOS information. >> >> Currently only type 1 and 2 are translated to human readable text. >> Other types may be added later. Currently only a hexdump and the list of >> strings is provided for these. >> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> Reviewed-by: Simon Glass <sjg@chromium.org> >> --- >> v2: > > [...] > >> email address updated >> +static struct smbios_header *next_table(struct smbios_header *table) >> +{ >> + const char *str; >> + >> + if (table->type == SMBIOS_END_OF_TABLE) >> + return NULL; >> + >> + str = smbios_get_string(table, 0); >> + return (struct smbios_header *)(++str); > > That can lead to unaligned access when we dereference that pointer, do we > care? SMBIOS tables are always byte aligned. This is why struct smbios_header is marked as packed. The GCCj documentation has this sentence: "The packed attribute specifies that a variable or structure field should have the smallest possible alignment - one byte for a variable, and one bit for a field, unless you specify a larger value with the aligned attribute." So shouldn't the compiler care about non-alignment? If there were a problematic usage, GCC would throw -Waddress-of-packed-member. Best regards Heinrich > >> +} >> + >> +static void smbios_print_generic(struct smbios_header *table) >> +{ >> + char *str = (char *)table + table->length; >> + > > Do we want the header below printed if there are no strings? > IOW can we exit early if (!*str) ? > >> + if (CONFIG_IS_ENABLED(HEXDUMP)) { >> + printf("Header and Data:\n"); >> + print_hex_dump("\t", DUMP_PREFIX_OFFSET, 16, 1, >> + table, table->length, false); >> + } >> + if (*str) { >> + printf("Strings:\n"); >> + for (int index = 1; *str; ++index) { >> + printf("\tString %u: %s\n", index, str); >> + str += strlen(str) + 1; >> + } >> + } >> +} >> + >> +void smbios_print_str(const char *label, void *table, u8 index) >> +{ >> + printf("\t%s: %s\n", label, smbios_get_string(table, index)); >> +} >> + > > > Other than that, LGTM > > Thanks > /Ilias
On Thu, 18 Jan 2024 at 14:54, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > On 1/18/24 13:39, Ilias Apalodimas wrote: > > Hi Heinrich, > > > > A few nits below > > > > On Wed, Jan 17, 2024 at 04:33:45PM +0100, Heinrich Schuchardt wrote: > >> U-Boot can either generated an SMBIOS table or copy it from a prior boot > >> stage, e.g. QEMU. > >> > >> Provide a command to display the SMBIOS information. > >> > >> Currently only type 1 and 2 are translated to human readable text. > >> Other types may be added later. Currently only a hexdump and the list of > >> strings is provided for these. > >> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >> Reviewed-by: Simon Glass <sjg@chromium.org> > >> --- > >> v2: > > > > [...] > > > >> email address updated > >> +static struct smbios_header *next_table(struct smbios_header *table) > >> +{ > >> + const char *str; > >> + > >> + if (table->type == SMBIOS_END_OF_TABLE) > >> + return NULL; > >> + > >> + str = smbios_get_string(table, 0); > >> + return (struct smbios_header *)(++str); > > > > That can lead to unaligned access when we dereference that pointer, do we > > care? > > SMBIOS tables are always byte aligned. This is why struct smbios_header > is marked as packed. The GCCj documentation has this sentence: > > "The packed attribute specifies that a variable or structure field > should have the smallest possible alignment - one byte for a variable, > and one bit for a field, unless you specify a larger value with the > aligned attribute." > > So shouldn't the compiler care about non-alignment? If there were a > problematic usage, GCC would throw -Waddress-of-packed-member. I don't think we have the strict alignment enabled in our makefiles. But, that won't be a problem, I missed the packed attribute in the smbios header. with or without the change in smbios_print_generic() Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
On Wed, Jan 17, 2024 at 04:33:45PM +0100, Heinrich Schuchardt wrote: > U-Boot can either generated an SMBIOS table or copy it from a prior boot > stage, e.g. QEMU. > > Provide a command to display the SMBIOS information. > > Currently only type 1 and 2 are translated to human readable text. > Other types may be added later. Currently only a hexdump and the list of > strings is provided for these. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > Reviewed-by: Simon Glass <sjg@chromium.org> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> [snip] > @@ -227,6 +227,13 @@ config CMD_SBI > help > Display information about the SBI implementation. > > +config CMD_SMBIOS > + bool "smbios" > + depends on SMBIOS > + default y > + help > + Display the SMBIOS information. > + So this would be enabled (today) on 888 boards and is a bit more than a kilobyte. I think we can just let this be enabled as needed in defconfigs?
On 1/24/24 22:16, Tom Rini wrote: > On Wed, Jan 17, 2024 at 04:33:45PM +0100, Heinrich Schuchardt wrote: > >> U-Boot can either generated an SMBIOS table or copy it from a prior boot >> stage, e.g. QEMU. >> >> Provide a command to display the SMBIOS information. >> >> Currently only type 1 and 2 are translated to human readable text. >> Other types may be added later. Currently only a hexdump and the list of >> strings is provided for these. >> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> Reviewed-by: Simon Glass <sjg@chromium.org> >> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > [snip] >> @@ -227,6 +227,13 @@ config CMD_SBI >> help >> Display information about the SBI implementation. >> >> +config CMD_SMBIOS >> + bool "smbios" >> + depends on SMBIOS >> + default y >> + help >> + Display the SMBIOS information. >> + > > So this would be enabled (today) on 888 boards and is a bit more than a > kilobyte. I think we can just let this be enabled as needed in > defconfigs? > As needed would be the boards where we want to run the related Python test. Sandbox and QEMU should be good enough? Best regards Heinrich
On Thu, Jan 25, 2024 at 12:24:33AM +0100, Heinrich Schuchardt wrote: > On 1/24/24 22:16, Tom Rini wrote: > > On Wed, Jan 17, 2024 at 04:33:45PM +0100, Heinrich Schuchardt wrote: > > > > > U-Boot can either generated an SMBIOS table or copy it from a prior boot > > > stage, e.g. QEMU. > > > > > > Provide a command to display the SMBIOS information. > > > > > > Currently only type 1 and 2 are translated to human readable text. > > > Other types may be added later. Currently only a hexdump and the list of > > > strings is provided for these. > > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > [snip] > > > @@ -227,6 +227,13 @@ config CMD_SBI > > > help > > > Display information about the SBI implementation. > > > +config CMD_SMBIOS > > > + bool "smbios" > > > + depends on SMBIOS > > > + default y > > > + help > > > + Display the SMBIOS information. > > > + > > > > So this would be enabled (today) on 888 boards and is a bit more than a > > kilobyte. I think we can just let this be enabled as needed in > > defconfigs? > > > > As needed would be the boards where we want to run the related Python test. > Sandbox and QEMU should be good enough? And the test needs the pytest mark for cmd_smbios, yes.
On Thu, 25 Jan 2024 at 03:02, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > On 1/24/24 22:16, Tom Rini wrote: > > On Wed, Jan 17, 2024 at 04:33:45PM +0100, Heinrich Schuchardt wrote: > > > >> U-Boot can either generated an SMBIOS table or copy it from a prior boot > >> stage, e.g. QEMU. > >> > >> Provide a command to display the SMBIOS information. > >> > >> Currently only type 1 and 2 are translated to human readable text. > >> Other types may be added later. Currently only a hexdump and the list of > >> strings is provided for these. > >> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >> Reviewed-by: Simon Glass <sjg@chromium.org> > >> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > [snip] > >> @@ -227,6 +227,13 @@ config CMD_SBI > >> help > >> Display information about the SBI implementation. > >> > >> +config CMD_SMBIOS > >> + bool "smbios" > >> + depends on SMBIOS > >> + default y > >> + help > >> + Display the SMBIOS information. > >> + > > > > So this would be enabled (today) on 888 boards and is a bit more than a > > kilobyte. I think we can just let this be enabled as needed in > > defconfigs? > > > > As needed would be the boards where we want to run the related Python > test. Sandbox and QEMU should be good enough? Yes, I think for most users seeing the smbios tables is probably not particularly useful.
diff --git a/cmd/Kconfig b/cmd/Kconfig index 26aeeeed03b..43c9c20c8fa 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -227,6 +227,13 @@ config CMD_SBI help Display information about the SBI implementation. +config CMD_SMBIOS + bool "smbios" + depends on SMBIOS + default y + help + Display the SMBIOS information. + endmenu menu "Boot commands" diff --git a/cmd/Makefile b/cmd/Makefile index e2a2b16ab25..87133cc27a8 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -168,6 +168,7 @@ obj-$(CONFIG_CMD_SETEXPR) += setexpr.o obj-$(CONFIG_CMD_SETEXPR_FMT) += printf.o obj-$(CONFIG_CMD_SPI) += spi.o obj-$(CONFIG_CMD_STRINGS) += strings.o +obj-$(CONFIG_CMD_SMBIOS) += smbios.o obj-$(CONFIG_CMD_SMC) += smccc.o obj-$(CONFIG_CMD_SYSBOOT) += sysboot.o obj-$(CONFIG_CMD_STACKPROTECTOR_TEST) += stackprot_test.o diff --git a/cmd/smbios.c b/cmd/smbios.c new file mode 100644 index 00000000000..63f908e92ce --- /dev/null +++ b/cmd/smbios.c @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * The 'smbios' command displays information from the SMBIOS table. + * + * Copyright (c) 2023, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> + */ + +#include <command.h> +#include <hexdump.h> +#include <mapmem.h> +#include <smbios.h> +#include <tables_csum.h> +#include <asm/global_data.h> + +DECLARE_GLOBAL_DATA_PTR; + +/** + * smbios_get_string() - get SMBIOS string from table + * + * @table: SMBIOS table + * @index: index of the string + * Return: address of string, may point to empty string + */ +static const char *smbios_get_string(void *table, int index) +{ + const char *str = (char *)table + + ((struct smbios_header *)table)->length; + + if (!*str) + ++str; + for (--index; *str && index; --index) + str += strlen(str) + 1; + + return str; +} + +static struct smbios_header *next_table(struct smbios_header *table) +{ + const char *str; + + if (table->type == SMBIOS_END_OF_TABLE) + return NULL; + + str = smbios_get_string(table, 0); + return (struct smbios_header *)(++str); +} + +static void smbios_print_generic(struct smbios_header *table) +{ + char *str = (char *)table + table->length; + + if (CONFIG_IS_ENABLED(HEXDUMP)) { + printf("Header and Data:\n"); + print_hex_dump("\t", DUMP_PREFIX_OFFSET, 16, 1, + table, table->length, false); + } + if (*str) { + printf("Strings:\n"); + for (int index = 1; *str; ++index) { + printf("\tString %u: %s\n", index, str); + str += strlen(str) + 1; + } + } +} + +void smbios_print_str(const char *label, void *table, u8 index) +{ + printf("\t%s: %s\n", label, smbios_get_string(table, index)); +} + +static void smbios_print_type1(struct smbios_type1 *table) +{ + printf("System Information\n"); + smbios_print_str("Manufacturer", table, table->manufacturer); + smbios_print_str("Product Name", table, table->product_name); + smbios_print_str("Version", table, table->version); + smbios_print_str("Serial Number", table, table->serial_number); + if (table->length >= 0x19) { + printf("\tUUID %pUl\n", table->uuid); + smbios_print_str("Wake Up Type", table, table->serial_number); + } + if (table->length >= 0x1b) { + smbios_print_str("Serial Number", table, table->serial_number); + smbios_print_str("SKU Number", table, table->sku_number); + } +} + +static void smbios_print_type2(struct smbios_type2 *table) +{ + u16 *handle; + + printf("Base Board Information\n"); + smbios_print_str("Manufacturer", table, table->manufacturer); + smbios_print_str("Product Name", table, table->product_name); + smbios_print_str("Version", table, table->version); + smbios_print_str("Serial Number", table, table->serial_number); + smbios_print_str("Asset Tag", table, table->asset_tag_number); + printf("\tFeature Flags: 0x%2x\n", table->feature_flags); + smbios_print_str("Chassis Location", table, table->chassis_location); + printf("\tChassis Handle: 0x%2x\n", table->chassis_handle); + smbios_print_str("Board Type", table, table->board_type); + printf("\tContained Object Handles: "); + handle = (void *)table->eos; + for (int i = 0; i < table->number_contained_objects; ++i) + printf("0x%04x ", handle[i]); + printf("\n"); +} + +static void smbios_print_type127(struct smbios_type127 *table) +{ + printf("End Of Table\n"); +} + +static int do_smbios(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + ulong addr; + void *entry; + u32 size; + char version[12]; + struct smbios_header *table; + static const char smbios_sig[] = "_SM_"; + static const char smbios3_sig[] = "_SM3_"; + size_t count = 0; + u32 max_struct_size; + + addr = gd_smbios_start(); + if (!addr) { + log_warning("SMBIOS not available\n"); + return CMD_RET_FAILURE; + } + entry = map_sysmem(addr, 0); + if (!memcmp(entry, smbios3_sig, sizeof(smbios3_sig) - 1)) { + struct smbios3_entry *entry3 = entry; + + table = (void *)(uintptr_t)entry3->struct_table_address; + snprintf(version, sizeof(version), "%d.%d.%d", + entry3->major_ver, entry3->minor_ver, entry3->doc_rev); + table = (void *)(uintptr_t)entry3->struct_table_address; + size = entry3->length; + max_struct_size = entry3->max_struct_size; + } else if (!memcmp(entry, smbios_sig, sizeof(smbios_sig) - 1)) { + struct smbios_entry *entry2 = entry; + + snprintf(version, sizeof(version), "%d.%d", + entry2->major_ver, entry2->minor_ver); + table = (void *)(uintptr_t)entry2->struct_table_address; + size = entry2->length; + max_struct_size = entry2->max_struct_size; + } else { + log_err("Unknown SMBIOS anchor format\n"); + return CMD_RET_FAILURE; + } + if (table_compute_checksum(entry, size)) { + log_err("Invalid anchor checksum\n"); + return CMD_RET_FAILURE; + } + printf("SMBIOS %s present.\n", version); + + for (struct smbios_header *pos = table; pos; pos = next_table(pos)) + ++count; + printf("%zd structures occupying %d bytes\n", count, max_struct_size); + printf("Table at 0x%llx\n", (unsigned long long)map_to_sysmem(table)); + + for (struct smbios_header *pos = table; pos; pos = next_table(pos)) { + printf("\nHandle 0x%04x, DMI type %d, %d bytes at 0x%llx\n", + pos->handle, pos->type, pos->length, + (unsigned long long)map_to_sysmem(pos)); + switch (pos->type) { + case 1: + smbios_print_type1((struct smbios_type1 *)pos); + break; + case 2: + smbios_print_type2((struct smbios_type2 *)pos); + break; + case 127: + smbios_print_type127((struct smbios_type127 *)pos); + break; + default: + smbios_print_generic(pos); + break; + } + } + + return CMD_RET_SUCCESS; +} + +U_BOOT_LONGHELP(smbios, "- display SMBIOS information"); + +U_BOOT_CMD(smbios, 1, 0, do_smbios, "display SMBIOS information", + smbios_help_text);