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