[PATCH v2 2/2] smbios: copy QEMU tables
Heinrich Schuchardt
xypron.glpk at gmx.de
Tue Jan 9 21:48:24 CET 2024
On 12/26/23 10:47, Simon Glass wrote:
> Hi Heinrich,
>
> On Sat, Dec 23, 2023 at 1:03 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>> From: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>>
>> QEMU provides SMBIOS tables with detailed information. We should not try to
>> replicate them in U-Boot.
>>
>> If we want to inform about U-Boot, we can add a Firmware Inventory
>> Information (type 45) table in future.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>> v2:
>> fix parsing of SMBIOS anchor
>> enable copying on x86 and 32bit ARM/RISC-V
>> ---
>> arch/x86/lib/tables.c | 2 +-
>> drivers/misc/Kconfig | 7 ++
>> drivers/misc/Makefile | 1 +
>> drivers/misc/qfw_smbios.c | 197 ++++++++++++++++++++++++++++++++++++
>> lib/efi_loader/efi_smbios.c | 4 +-
>> 5 files changed, 209 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/misc/qfw_smbios.c
>>
>> diff --git a/arch/x86/lib/tables.c b/arch/x86/lib/tables.c
>> index 5b5070f7ca..914d9de0cb 100644
>> --- a/arch/x86/lib/tables.c
>> +++ b/arch/x86/lib/tables.c
>> @@ -61,7 +61,7 @@ static struct table_info table_list[] = {
>> #ifdef CONFIG_GENERATE_ACPI_TABLE
>> { "acpi", write_acpi_tables, BLOBLISTT_ACPI_TABLES, 0x10000, 0x1000},
>> #endif
>> -#ifdef CONFIG_GENERATE_SMBIOS_TABLE
>> +#if defined(CONFIG_GENERATE_SMBIOS_TABLE) && !defined(CONFIG_QFW_SMBIOS)
>> { "smbios", write_smbios_table, BLOBLISTT_SMBIOS_TABLES, 0x1000, 0x100},
>> #endif
>> };
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index e8e4400516..d2536c54e0 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -561,6 +561,13 @@ config QFW_MMIO
>> Hidden option to enable MMIO QEMU fw_cfg interface. This will be
>> selected by the appropriate QEMU board.
>>
>> +config QFW_SMBIOS
>> + bool
>> + default y
>> + depends on QFW && SMBIOS && !SANDBOX
>> + help
>> + Hidden option to read SMBIOS tables from QEMU.
>> +
>> config I2C_EEPROM
>> bool "Enable driver for generic I2C-attached EEPROMs"
>> depends on MISC
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index cda701d38e..64bea92f51 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -66,6 +66,7 @@ obj-y += qfw.o
>> obj-$(CONFIG_QFW_ACPI) += qfw_acpi.o
>> obj-$(CONFIG_QFW_PIO) += qfw_pio.o
>> obj-$(CONFIG_QFW_MMIO) += qfw_mmio.o
>> +obj-$(CONFIG_QFW_SMBIOS) += qfw_smbios.o
>> obj-$(CONFIG_SANDBOX) += qfw_sandbox.o
>> endif
>> obj-$(CONFIG_ROCKCHIP_EFUSE) += rockchip-efuse.o
>> diff --git a/drivers/misc/qfw_smbios.c b/drivers/misc/qfw_smbios.c
>> new file mode 100644
>> index 0000000000..9019345783
>> --- /dev/null
>> +++ b/drivers/misc/qfw_smbios.c
>> @@ -0,0 +1,197 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * (C) Copyright 2023 Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> + */
>> +
>> +#define LOG_CATEGORY UCLASS_QFW
>> +
>> +#include <efi_loader.h>
>> +#include <errno.h>
>> +#include <log.h>
>> +#include <malloc.h>
>> +#include <mapmem.h>
>> +#include <qfw.h>
>> +#include <smbios.h>
>> +#include <tables_csum.h>
>> +#include <linux/sizes.h>
>> +#include <asm/global_data.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +/**
>> + * qfw_load_smbios_table() - load a QEMU firmware file
>> + *
>> + * @dev: QEMU firmware device
>> + * @size: parameter to return the size of the loaded table
>> + * @name: name of the table to load
>> + * Return: address of the loaded table, NULL on error
>> + */
>> +static void *qfw_load_smbios_table(struct udevice *dev, uint32_t *size,
>> + char *name)
>> +{
>> + struct fw_file *file;
>> + struct bios_linker_entry *table;
>> +
>> + file = qfw_find_file(dev, name);
>> + if (!file) {
>> + log_debug("Can't find %s\n", name);
>> + return NULL;
>> + }
>> +
>> + *size = be32_to_cpu(file->cfg.size);
>> +
>> + table = malloc(*size);
>> + if (!table) {
>> + log_err("Out of memory\n");
>> + return NULL;
>> + }
>> +
>> + qfw_read_entry(dev, be16_to_cpu(file->cfg.select), *size, table);
>> +
>> + return table;
>> +}
>> +
>> +/**
>> + * qfw_parse_smbios_anchor() - parse QEMU's SMBIOS anchor
>> + *
>> + * @dev: QEMU firmware device
>> + * @entry: SMBIOS 3 structure to be filled from QEMU's anchor
>> + * Return: 0 for success, -ve on error
>> + */
>> +static int qfw_parse_smbios_anchor(struct udevice *dev,
>> + struct smbios3_entry *entry)
>> +{
>> + void *table;
>> + uint32_t size;
>> + struct smbios_entry *entry2;
>> + struct smbios3_entry *entry3;
>> + const char smbios_sig[] = "_SM_";
>> + const char smbios3_sig[] = "_SM3_";
>> + int ret = 0;
>> +
>> + table = qfw_load_smbios_table(dev, &size, "etc/smbios/smbios-anchor");
>> + if (!table)
>> + return -ENOMEM;
>> + if (!memcmp(table, smbios3_sig, sizeof(smbios3_sig) - 1)) {
>> + entry3 = table;
>> + if (entry3->length != sizeof(struct smbios3_entry)) {
>> + ret = -ENOENT;
>> + goto out;
>> + }
>> + memcpy(entry, entry3, sizeof(struct smbios3_entry));
>> + } else if (!memcmp(table, smbios_sig, sizeof(smbios_sig) - 1)) {
>> + entry2 = table;
>> + if (entry2->length != sizeof(struct smbios_entry)) {
>> + ret = -ENOENT;
>> + goto out;
>> + }
>> + memset(entry, 0, sizeof(struct smbios3_entry));
>> + memcpy(entry, smbios3_sig, sizeof(smbios3_sig));
>> + entry->length = sizeof(struct smbios3_entry);
>> + entry->major_ver = entry2->major_ver;
>> + entry->minor_ver = entry2->minor_ver;
>> + entry->max_struct_size = entry2->max_struct_size;
>> + } else {
>> + ret = -ENOENT;
>> + goto out;
>> + }
>> + ret = 0;
>> +out:
>> + free(table);
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * qfw_write_smbios_tables() - copy SMBIOS tables from QEMU
>> + *
>> + * @addr: target buffer
>> + * @size: size of target buffer
>> + * Return: 0 for success, -ve on error
>> + */
>> +static int qfw_write_smbios_tables(u8 *addr, uint32_t size)
>> +{
>> + int ret;
>> + struct udevice *dev;
>> + struct smbios3_entry *entry = (void *)addr;
>
> map_sysmem() so you can use a sandbox test
This is a static function which never can be called directly by a test.
The caller already has a map_sysmem(). See below.
>
>> + void *table;
>> + uint32_t table_size;
>> +
>> + ret = qfw_get_dev(&dev);
>> + if (ret) {
>> + log_err("No QEMU firmware device\n");
>> + return ret;
>> + }
>> +
>> + ret = qfw_read_firmware_list(dev);
>> + if (ret) {
>> + log_err("Can't read firmware file list\n");
>> + return ret;
>> + }
>> +
>> + ret = qfw_parse_smbios_anchor(dev, entry);
>> + if (ret) {
>> + log_debug("Can't parse anchor\n");
>> + return ret;
>> + }
>> +
>> + addr += entry->length;
>> + entry->struct_table_address = (uintptr_t)addr;
>> + entry->checksum = 0;
>> + entry->checksum = table_compute_checksum(entry,
>> + sizeof(struct smbios3_entry));
>> +
>> + table = qfw_load_smbios_table(dev, &table_size,
>> + "etc/smbios/smbios-tables");
>> + if (table_size + sizeof(struct smbios3_entry) > size) {
>> + free(table);
>> + return -ENOMEM;
>> + }
>> + memcpy(addr, table, table_size);
>> + free(table);
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * qfw_evt_write_smbios_tables() - event handler for copying QEMU SMBIOS tables
>> + *
>> + * Return: 0 on success, -ve on error (only out of memory)
>> + */
>> +static int qfw_evt_write_smbios_tables(void)
>> +{
>> + phys_addr_t addr;
>> + void *ptr;
>> + int ret;
>> + /*
>> + * TODO:
>> + * This size is currently hard coded in lib/efi_loader/efi_smbios.c.
>> + * We need a field in global data for the size.
>
> Is that because it might be >4KB?
>
>> + */
>> + uint32_t size = SZ_4K;
>> +
>> + /* Reserve 64K for SMBIOS tables, aligned to a 4K boundary */
>> + ptr = memalign(SZ_4K, size);
>> + if (!ptr) {
>> + log_err("Out of memory\n");
>> + return -ENOMEM;
>> + }
>> + addr = map_to_sysmem(ptr);
>> +
>> + /* Generate SMBIOS tables */
>> + ret = qfw_write_smbios_tables(ptr, size);
>> + if (ret) {
>> + if (CONFIG_IS_ENABLED(GENERATE_SMBIOS_TABLE)) {
>> + log_info("Falling back to U-Boot generated SMBIOS tables\n");
>> + write_smbios_table(addr);
>
> Should we do this? Perhaps better to produce an error? If not, please
> add a comment as to why.
Error messages are written.
If QEMU provides SMBIOS tables depends on the QEMU version. E.g for
RISC-V patches enabling SMBIOS table generation are still pending.
Stopping the boot process because SMBIOS tables are not supplied by QEMU
is not very helpful for users stuck on an old QEMU version. We should
always do our best to complete the boot process.
>
>> + }
>> + } else {
>> + log_debug("SMBIOS tables copied from QEMU\n");
>> + }
>> +
>> + gd_set_smbios_start(addr);
>> +
>> + return 0;
>> +}
>> +
>> +EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, qfw_evt_write_smbios_tables);
>> diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
>> index bbb8421ce1..c4837da969 100644
>> --- a/lib/efi_loader/efi_smbios.c
>> +++ b/lib/efi_loader/efi_smbios.c
>> @@ -51,7 +51,9 @@ static int install_smbios_table(void)
>> u64 addr;
>> efi_status_t ret;
>>
>> - if (!IS_ENABLED(CONFIG_GENERATE_SMBIOS_TABLE) || IS_ENABLED(CONFIG_X86))
>> + if (!IS_ENABLED(CONFIG_GENERATE_SMBIOS_TABLE) ||
>> + IS_ENABLED(CONFIG_X86) ||
>> + IS_ENABLED(CONFIG_QFW_SMBIOS))
>
> Hmm it seems we need an new Kconfig for this condition?
I think we have too many symbols with unclear meaning.
Should we really use CONFIG_GENERATE_SMBIOS_TABLE=y in conjunction with
CONFIG_QFW_SMBIOS=y? Or should we have a Kconfig choice where we have
mutually exclusive sources for SMBIOS:
CONFIG SMBIOS_NONE - no SMBIOS table
CONFIG_QFW_SMBIOS - copy SMBIOS table from QEMU
CONFIG_SMBIOS_FROM DT - generate SMBIOS table from device-tree
CONFIG_SMBIOS_COREBOOT - copy SMBIOS table from coreboot
CONFIG SMBIOS_X86 - x86 specific code
There is nothing EFI specific in this function. Shouldn't we move it to
a different module?
For ACPI we have a folder lib/acpi/ Should we have a folder lib/smbios/?
Best regards
Heinrich
>
>> return 0;
>>
>> addr = SZ_4G;
>> --
>> 2.43.0
>>
>
> Otherwise:
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> Regards,
> Simon
More information about the U-Boot
mailing list