Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information
On Thu, 25 Jan 2024 at 03:02, 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 > >> Reviewed-by: Simon Glass > >> Reviewed-by: Ilias Apalodimas > > [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.
Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information
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 > > > Reviewed-by: Simon Glass > > > Reviewed-by: Ilias Apalodimas > > [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. -- Tom signature.asc Description: PGP signature
Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information
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 Reviewed-by: Simon Glass Reviewed-by: Ilias Apalodimas [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
Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information
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 > Reviewed-by: Simon Glass > Reviewed-by: Ilias Apalodimas [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? -- Tom signature.asc Description: PGP signature
Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information
On Thu, 18 Jan 2024 at 14:54, Heinrich Schuchardt
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
> >> Reviewed-by: Simon Glass
> >> ---
> >> 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
Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information
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
Reviewed-by: Simon Glass
---
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
Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information
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
> Reviewed-by: Simon Glass
> ---
> 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
[PATCH v2 2/4] cmd: provide command to display SMBIOS information
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
Reviewed-by: Simon Glass
---
v2:
email address updated
---
cmd/Kconfig | 7 ++
cmd/Makefile | 1 +
cmd/smbios.c | 191 +++
3 files changed, 199 insertions(+)
create mode 100644 cmd/smbios.c
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 26ad03b..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 000..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
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+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: ");
+ han
