[PATCH v2 1/4] smbios: type2: contained object handles

[PATCH v2 1/4] smbios: type2: contained object handles

[Heinrich Schuchardt]

The type 2 structure must include information about the contained objects.
It is fine to set the number of contained object handles to 0.

Add the missing field.

Fixes: 721e992a8af5 ("x86: Add SMBIOS table support")
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
v2:
	email address updated
---
 include/smbios.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/smbios.h b/include/smbios.h
index b507b9d9d72..98bb9e52cdf 100644
--- a/include/smbios.h
+++ b/include/smbios.h
@@ -139,6 +139,7 @@ struct __packed smbios_type2 {
 	u8 chassis_location;
 	u16 chassis_handle;
 	u8 board_type;
+	u8 number_contained_objects;
 	char eos[SMBIOS_STRUCT_EOS_BYTES];
 };
 
-- 
2.43.0

[PATCH v2 2/4] cmd: provide command to display SMBIOS information

[Heinrich Schuchardt]

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
---
 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 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);
-- 
2.43.0

[PATCH v2 3/4] doc: man-page for smbios command

[Heinrich Schuchardt]

Provide a man-page for the smbios command.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
v2:
	no change
---
 doc/usage/cmd/smbios.rst | 93 ++++++++++++++++++++++++++++++++++++++++
 doc/usage/index.rst      |  1 +
 2 files changed, 94 insertions(+)
 create mode 100644 doc/usage/cmd/smbios.rst

diff --git a/doc/usage/cmd/smbios.rst b/doc/usage/cmd/smbios.rst
new file mode 100644
index 00000000000..1ffd706d7de
--- /dev/null
+++ b/doc/usage/cmd/smbios.rst
@@ -0,0 +1,93 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later:
+
+smbios command
+==============
+
+Synopsis
+--------
+
+::
+
+        smbios
+
+Description
+-----------
+
+The smbios command displays information from the SMBIOS tables.
+
+Examples
+--------
+
+The example below shows an example output of the smbios command.
+
+::
+
+    => smbios
+    SMBIOS 2.8.0 present.
+    8 structures occupying 81 bytes
+    Table at 0x6d35018
+
+    Handle 0x0100, DMI type 1, 27 bytes at 0x6d35018
+    System Information
+        Manufacturer: QEMU
+        Product Name: Standard PC (i440FX + PIIX, 1996)
+        Version: pc-i440fx-2.5
+        Serial Number:
+        UUID 00000000-0000-0000-0000-000000000000
+        Wake Up Type:
+        Serial Number:
+        SKU Number:
+
+    Handle 0x0300, DMI type 3, 22 bytes at 0x6d35069
+    Header and Data:
+        00000000: 03 16 00 03 01 01 02 00 00 03 03 03 02 00 00 00
+        00000010: 00 00 00 00 00 00
+    Strings:
+        String 1: QEMU
+        String 2: pc-i440fx-2.5
+
+    Handle 0x0400, DMI type 4, 42 bytes at 0x6d35093
+    Header and Data:
+        00000000: 04 2a 00 04 01 03 01 02 63 06 00 00 fd ab 81 07
+        00000010: 03 00 00 00 d0 07 d0 07 41 01 ff ff ff ff ff ff
+        00000020: 00 00 00 01 01 01 02 00 01 00
+    Strings:
+        String 1: CPU 0
+        String 2: QEMU
+        String 3: pc-i440fx-2.5
+
+    Handle 0x1000, DMI type 16, 23 bytes at 0x6d350d7
+    Header and Data:
+        00000000: 10 17 00 10 01 03 06 00 00 02 00 fe ff 01 00 00
+        00000010: 00 00 00 00 00 00 00
+
+    Handle 0x1100, DMI type 17, 40 bytes at 0x6d350f0
+    Header and Data:
+        00000000: 11 28 00 11 00 10 fe ff ff ff ff ff 80 00 09 00
+        00000010: 01 00 07 02 00 00 00 02 00 00 00 00 00 00 00 00
+        00000020: 00 00 00 00 00 00 00 00
+    Strings:
+        String 1: DIMM 0
+        String 2: QEMU
+
+    Handle 0x1300, DMI type 19, 31 bytes at 0x6d35125
+    Header and Data:
+        00000000: 13 1f 00 13 00 00 00 00 ff ff 01 00 00 10 01 00
+        00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+
+    Handle 0x2000, DMI type 32, 11 bytes at 0x6d35146
+    Header and Data:
+        00000000: 20 0b 00 20 00 00 00 00 00 00 00
+
+    Handle 0x7f00, DMI type 127, 4 bytes at 0x6d35153
+    End Of Table
+
+Configuration
+-------------
+
+The command is only available if CONFIG_CMD_SMBIOS=y.
+
+Return value
+------------
+
+The return value $? is 0 (true) on success, 1 (false) otherwise.
diff --git a/doc/usage/index.rst b/doc/usage/index.rst
index c171c029b80..0d174eefaa5 100644
--- a/doc/usage/index.rst
+++ b/doc/usage/index.rst
@@ -103,6 +103,7 @@ Shell commands
    cmd/size
    cmd/sleep
    cmd/sm
+   cmd/smbios
    cmd/sound
    cmd/source
    cmd/temperature
-- 
2.43.0

[PATCH v2 4/4] test: unit test for smbios command

[Heinrich Schuchardt]

Provide a unit test for the smbios command.

Provide different test functions for QEMU, sandbox, and other systems.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	for generic boards we only can assume that a type 127 table exists
	email address updated
---
 test/py/tests/test_smbios.py | 41 ++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 test/py/tests/test_smbios.py

diff --git a/test/py/tests/test_smbios.py b/test/py/tests/test_smbios.py
new file mode 100644
index 00000000000..82b0b689830
--- /dev/null
+++ b/test/py/tests/test_smbios.py
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+"""Test smbios command"""
+
+import pytest
+
+@pytest.mark.buildconfigspec('cmd_smbios')
+@pytest.mark.notbuildconfigspec('qfw_smbios')
+@pytest.mark.notbuildconfigspec('sandbox')
+def test_cmd_smbios(u_boot_console):
+    """Run the smbios command"""
+    output = u_boot_console.run_command('smbios')
+    assert 'DMI type 127,' in output
+
+@pytest.mark.buildconfigspec('cmd_smbios')
+@pytest.mark.buildconfigspec('qfw_smbios')
+@pytest.mark.notbuildconfigspec('sandbox')
+# TODO:
+# QEMU v8.2.0 lacks SMBIOS support for RISC-V
+# Once support is available in our Docker image we can remove the constraint.
+@pytest.mark.notbuildconfigspec('riscv')
+def test_cmd_smbios_qemu(u_boot_console):
+    """Run the smbios command on QEMU"""
+    output = u_boot_console.run_command('smbios')
+    assert 'DMI type 1,' in output
+    assert 'Manufacturer: QEMU' in output
+    assert 'DMI type 127,' in output
+
+@pytest.mark.buildconfigspec('cmd_smbios')
+@pytest.mark.buildconfigspec('sandbox')
+def test_cmd_smbios_sandbox(u_boot_console):
+    """Run the smbios command on the sandbox"""
+    output = u_boot_console.run_command('smbios')
+    assert 'DMI type 0,' in output
+    assert 'String 1: U-Boot' in output
+    assert 'DMI type 1,' in output
+    assert 'Manufacturer: sandbox' in output
+    assert 'DMI type 2,' in output
+    assert 'DMI type 3,' in output
+    assert 'DMI type 4,' in output
+    assert 'DMI type 127,' in output
-- 
2.43.0

[PATCH v2 0/4] cmd: provide command to display SMBIOS information

[Heinrich Schuchardt]

U-Boot may supply an SMBIOS table or they may be copied from 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.

The following prerequisites had to be fixed:

* The definition of SMBIOS type 2 lacked a field.

v2:
	merged patch remove
	for generic boards we only can assume that a type 127 table exists
	my email address updated in all patches

Heinrich Schuchardt (4):
  smbios: type2: contained object handles
  cmd: provide command to display SMBIOS information
  doc: man-page for smbios command
  test: unit test for smbios command

 cmd/Kconfig                  |   7 ++
 cmd/Makefile                 |   1 +
 cmd/smbios.c                 | 191 +++++++++++++++++++++++++++++++++++
 doc/usage/cmd/smbios.rst     |  93 +++++++++++++++++
 doc/usage/index.rst          |   1 +
 include/smbios.h             |   1 +
 test/py/tests/test_smbios.py |  41 ++++++++
 7 files changed, 335 insertions(+)
 create mode 100644 cmd/smbios.c
 create mode 100644 doc/usage/cmd/smbios.rst
 create mode 100644 test/py/tests/test_smbios.py

-- 
2.43.0

Re: [PATCH v2 4/4] test: unit test for smbios command

[Ilias Apalodimas]

Hi Heinrich,

On Wed, 17 Jan 2024 at 17:34, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Provide a unit test for the smbios command.
>
> Provide different test functions for QEMU, sandbox, and other systems.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
>         for generic boards we only can assume that a type 127 table exists
>         email address updated
> ---
>  test/py/tests/test_smbios.py | 41 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 test/py/tests/test_smbios.py
>
> diff --git a/test/py/tests/test_smbios.py b/test/py/tests/test_smbios.py
> new file mode 100644
> index 00000000000..82b0b689830
> --- /dev/null
> +++ b/test/py/tests/test_smbios.py
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +"""Test smbios command"""
> +
> +import pytest
> +
> +@pytest.mark.buildconfigspec('cmd_smbios')
> +@pytest.mark.notbuildconfigspec('qfw_smbios')
> +@pytest.mark.notbuildconfigspec('sandbox')
> +def test_cmd_smbios(u_boot_console):
> +    """Run the smbios command"""
> +    output = u_boot_console.run_command('smbios')
> +    assert 'DMI type 127,' in output
> +
> +@pytest.mark.buildconfigspec('cmd_smbios')
> +@pytest.mark.buildconfigspec('qfw_smbios')
> +@pytest.mark.notbuildconfigspec('sandbox')
> +# TODO:
> +# QEMU v8.2.0 lacks SMBIOS support for RISC-V
> +# Once support is available in our Docker image we can remove the constraint.
> +@pytest.mark.notbuildconfigspec('riscv')
> +def test_cmd_smbios_qemu(u_boot_console):
> +    """Run the smbios command on QEMU"""
> +    output = u_boot_console.run_command('smbios')
> +    assert 'DMI type 1,' in output
> +    assert 'Manufacturer: QEMU' in output
> +    assert 'DMI type 127,' in output

Is there an easy way to run this test with and without the SMBIOS
tables provided by QEMU? We could test the u-boot generated ones that
way

Thanks
/Ilias
> +
> +@pytest.mark.buildconfigspec('cmd_smbios')
> +@pytest.mark.buildconfigspec('sandbox')
> +def test_cmd_smbios_sandbox(u_boot_console):
> +    """Run the smbios command on the sandbox"""
> +    output = u_boot_console.run_command('smbios')
> +    assert 'DMI type 0,' in output
> +    assert 'String 1: U-Boot' in output
> +    assert 'DMI type 1,' in output
> +    assert 'Manufacturer: sandbox' in output
> +    assert 'DMI type 2,' in output
> +    assert 'DMI type 3,' in output
> +    assert 'DMI type 4,' in output
> +    assert 'DMI type 127,' in output
> --
> 2.43.0
>

Re: [PATCH v2 4/4] test: unit test for smbios command

[Heinrich Schuchardt]

On 1/18/24 12:51, Ilias Apalodimas wrote:
> Hi Heinrich,
> 
> On Wed, 17 Jan 2024 at 17:34, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> Provide a unit test for the smbios command.
>>
>> Provide different test functions for QEMU, sandbox, and other systems.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> v2:
>>          for generic boards we only can assume that a type 127 table exists
>>          email address updated
>> ---
>>   test/py/tests/test_smbios.py | 41 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>   create mode 100644 test/py/tests/test_smbios.py
>>
>> diff --git a/test/py/tests/test_smbios.py b/test/py/tests/test_smbios.py
>> new file mode 100644
>> index 00000000000..82b0b689830
>> --- /dev/null
>> +++ b/test/py/tests/test_smbios.py
>> @@ -0,0 +1,41 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +"""Test smbios command"""
>> +
>> +import pytest
>> +
>> +@pytest.mark.buildconfigspec('cmd_smbios')
>> +@pytest.mark.notbuildconfigspec('qfw_smbios')
>> +@pytest.mark.notbuildconfigspec('sandbox')
>> +def test_cmd_smbios(u_boot_console):
>> +    """Run the smbios command"""
>> +    output = u_boot_console.run_command('smbios')
>> +    assert 'DMI type 127,' in output
>> +
>> +@pytest.mark.buildconfigspec('cmd_smbios')
>> +@pytest.mark.buildconfigspec('qfw_smbios')
>> +@pytest.mark.notbuildconfigspec('sandbox')
>> +# TODO:
>> +# QEMU v8.2.0 lacks SMBIOS support for RISC-V
>> +# Once support is available in our Docker image we can remove the constraint.
>> +@pytest.mark.notbuildconfigspec('riscv')
>> +def test_cmd_smbios_qemu(u_boot_console):
>> +    """Run the smbios command on QEMU"""
>> +    output = u_boot_console.run_command('smbios')
>> +    assert 'DMI type 1,' in output
>> +    assert 'Manufacturer: QEMU' in output
>> +    assert 'DMI type 127,' in output
> 
> Is there an easy way to run this test with and without the SMBIOS
> tables provided by QEMU? We could test the u-boot generated ones that
> way

We already test the sandbox without QFW. I would not want to duplicated 
QEMU defconfigs.

Best regards

Heinrich

> 
> Thanks
> /Ilias
>> +
>> +@pytest.mark.buildconfigspec('cmd_smbios')
>> +@pytest.mark.buildconfigspec('sandbox')
>> +def test_cmd_smbios_sandbox(u_boot_console):
>> +    """Run the smbios command on the sandbox"""
>> +    output = u_boot_console.run_command('smbios')
>> +    assert 'DMI type 0,' in output
>> +    assert 'String 1: U-Boot' in output
>> +    assert 'DMI type 1,' in output
>> +    assert 'Manufacturer: sandbox' in output
>> +    assert 'DMI type 2,' in output
>> +    assert 'DMI type 3,' in output
>> +    assert 'DMI type 4,' in output
>> +    assert 'DMI type 127,' in output
>> --
>> 2.43.0
>>

Re: [PATCH v2 4/4] test: unit test for smbios command

[Ilias Apalodimas]

On Thu, Jan 18, 2024 at 01:33:15PM +0100, Heinrich Schuchardt wrote:
> On 1/18/24 12:51, Ilias Apalodimas wrote:
> > Hi Heinrich,
> >
> > On Wed, 17 Jan 2024 at 17:34, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > >
> > > Provide a unit test for the smbios command.
> > >
> > > Provide different test functions for QEMU, sandbox, and other systems.
> > >
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > > v2:
> > >          for generic boards we only can assume that a type 127 table exists
> > >          email address updated
> > > ---
> > >   test/py/tests/test_smbios.py | 41 ++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 41 insertions(+)
> > >   create mode 100644 test/py/tests/test_smbios.py
> > >
> > > diff --git a/test/py/tests/test_smbios.py b/test/py/tests/test_smbios.py
> > > new file mode 100644
> > > index 00000000000..82b0b689830
> > > --- /dev/null
> > > +++ b/test/py/tests/test_smbios.py
> > > @@ -0,0 +1,41 @@
> > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > +
> > > +"""Test smbios command"""
> > > +
> > > +import pytest
> > > +
> > > +@pytest.mark.buildconfigspec('cmd_smbios')
> > > +@pytest.mark.notbuildconfigspec('qfw_smbios')
> > > +@pytest.mark.notbuildconfigspec('sandbox')
> > > +def test_cmd_smbios(u_boot_console):
> > > +    """Run the smbios command"""
> > > +    output = u_boot_console.run_command('smbios')
> > > +    assert 'DMI type 127,' in output
> > > +
> > > +@pytest.mark.buildconfigspec('cmd_smbios')
> > > +@pytest.mark.buildconfigspec('qfw_smbios')
> > > +@pytest.mark.notbuildconfigspec('sandbox')
> > > +# TODO:
> > > +# QEMU v8.2.0 lacks SMBIOS support for RISC-V
> > > +# Once support is available in our Docker image we can remove the constraint.
> > > +@pytest.mark.notbuildconfigspec('riscv')
> > > +def test_cmd_smbios_qemu(u_boot_console):
> > > +    """Run the smbios command on QEMU"""
> > > +    output = u_boot_console.run_command('smbios')
> > > +    assert 'DMI type 1,' in output
> > > +    assert 'Manufacturer: QEMU' in output
> > > +    assert 'DMI type 127,' in output
> >
> > Is there an easy way to run this test with and without the SMBIOS
> > tables provided by QEMU? We could test the u-boot generated ones that
> > way
>
> We already test the sandbox without QFW. I would not want to duplicated QEMU
> defconfigs.
>
Ah fair enough, I can add the generic tests in that one

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> Best regards
>
> Heinrich
>
> >
> > Thanks
> > /Ilias
> > > +
> > > +@pytest.mark.buildconfigspec('cmd_smbios')
> > > +@pytest.mark.buildconfigspec('sandbox')
> > > +def test_cmd_smbios_sandbox(u_boot_console):
> > > +    """Run the smbios command on the sandbox"""
> > > +    output = u_boot_console.run_command('smbios')
> > > +    assert 'DMI type 0,' in output
> > > +    assert 'String 1: U-Boot' in output
> > > +    assert 'DMI type 1,' in output
> > > +    assert 'Manufacturer: sandbox' in output
> > > +    assert 'DMI type 2,' in output
> > > +    assert 'DMI type 3,' in output
> > > +    assert 'DMI type 4,' in output
> > > +    assert 'DMI type 127,' in output
> > > --
> > > 2.43.0
> > >
>

Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information

[Ilias Apalodimas]

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

Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information

[Heinrich Schuchardt]

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

Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information

[Ilias Apalodimas]

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>

Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information

[Heinrich Schuchardt]

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

Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information

[Peter Robinson]

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.