[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 at canonical.com>
>> Reviewed-by: Simon Glass <sjg at 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