[09/19] smbios: build legacy mode code only for 'pc' machine

basically moving code around without functional change.
And exposing some symbols so that they could be shared
between smbbios.c and new smbios_legacy.c

plus some meson magic to build smbios_legacy.c only
for 'pc' machine and otherwise replace it with stub
if not selected.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/firmware/smbios.h   |  22 +++
 hw/i386/Kconfig                |   1 +
 hw/smbios/Kconfig              |   2 +
 hw/smbios/meson.build          |   4 +
 hw/smbios/smbios.c             | 251 ++++++---------------------------
 hw/smbios/smbios_legacy.c      | 179 +++++++++++++++++++++++
 hw/smbios/smbios_legacy_stub.c |  16 +++
 7 files changed, 270 insertions(+), 205 deletions(-)
 create mode 100644 hw/smbios/smbios_legacy.c
 create mode 100644 hw/smbios/smbios_legacy_stub.c

> On 27-Feb-2024, at 21:17, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> basically moving code around without functional change.
> And exposing some symbols so that they could be shared
> between smbbios.c and new smbios_legacy.c
> 
> plus some meson magic to build smbios_legacy.c only
> for 'pc' machine and otherwise replace it with stub
> if not selected.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> include/hw/firmware/smbios.h   |  22 +++
> hw/i386/Kconfig                |   1 +
> hw/smbios/Kconfig              |   2 +
> hw/smbios/meson.build          |   4 +
> hw/smbios/smbios.c             | 251 ++++++---------------------------
> hw/smbios/smbios_legacy.c      | 179 +++++++++++++++++++++++
> hw/smbios/smbios_legacy_stub.c |  16 +++
> 7 files changed, 270 insertions(+), 205 deletions(-)
> create mode 100644 hw/smbios/smbios_legacy.c
> create mode 100644 hw/smbios/smbios_legacy_stub.c
> 
> diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
> index 1fbff3c55f..a6d8fd6591 100644
> --- a/include/hw/firmware/smbios.h
> +++ b/include/hw/firmware/smbios.h
> @@ -2,6 +2,7 @@
> #define QEMU_SMBIOS_H
> 
> #include "qapi/qapi-types-machine.h"
> +#include "qemu/bitmap.h"
> 
> /*
>  * SMBIOS Support
> @@ -16,8 +17,27 @@
>  *
>  */
> 
> +extern uint8_t *usr_blobs;
> +extern GArray *usr_blobs_sizes;
> +
> +typedef struct {
> +    const char *vendor, *version, *date;
> +    bool have_major_minor, uefi;
> +    uint8_t major, minor;
> +} smbios_type0_t;
> +extern smbios_type0_t smbios_type0;
> +
> +typedef struct {
> +    const char *manufacturer, *product, *version, *serial, *sku, *family;
> +    /* uuid is in qemu_uuid */
> +} smbios_type1_t;
> +extern smbios_type1_t smbios_type1;
> +
> 
> #define SMBIOS_MAX_TYPE 127
> +extern DECLARE_BITMAP(smbios_have_binfile_bitmap, SMBIOS_MAX_TYPE + 1);
> +extern DECLARE_BITMAP(smbios_have_fields_bitmap, SMBIOS_MAX_TYPE + 1);
> +
> #define offsetofend(TYPE, MEMBER) \
>        (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
> 
> @@ -290,6 +310,8 @@ struct smbios_type_127 {
>     struct smbios_structure_header header;
> } QEMU_PACKED;
> 
> +void smbios_validate_table(void);
> +void smbios_add_usr_blob_size(size_t size);
> void smbios_entry_add(QemuOpts *opts, Error **errp);
> void smbios_set_cpuid(uint32_t version, uint32_t features);
> void smbios_set_defaults(const char *manufacturer, const char *product,
> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index a1846be6f7..a6ee052f9a 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -76,6 +76,7 @@ config I440FX
>     select PIIX
>     select DIMM
>     select SMBIOS
> +    select SMBIOS_LEGACY
>     select FW_CFG_DMA
> 
> config ISAPC
> diff --git a/hw/smbios/Kconfig b/hw/smbios/Kconfig
> index 553adf4bfc..8d989a2f1b 100644
> --- a/hw/smbios/Kconfig
> +++ b/hw/smbios/Kconfig
> @@ -1,2 +1,4 @@
> config SMBIOS
>     bool
> +config SMBIOS_LEGACY
> +    bool
> diff --git a/hw/smbios/meson.build b/hw/smbios/meson.build
> index 7046967462..a59039f669 100644
> --- a/hw/smbios/meson.build
> +++ b/hw/smbios/meson.build
> @@ -4,5 +4,9 @@ smbios_ss.add(when: 'CONFIG_IPMI',
>               if_true: files('smbios_type_38.c'),
>               if_false: files('smbios_type_38-stub.c'))
> 
> +smbios_ss.add(when: 'CONFIG_SMBIOS_LEGACY',
> +              if_true: files('smbios_legacy.c'),
> +              if_false: files('smbios_legacy_stub.c'))
> +
> system_ss.add_all(when: 'CONFIG_SMBIOS', if_true: smbios_ss)
> system_ss.add(when: 'CONFIG_SMBIOS', if_false: files('smbios-stub.c'))
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 97cf762228..fb1f05fcde 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -31,38 +31,12 @@
> #include "hw/pci/pci_device.h"
> #include "smbios_build.h"
> 
> -/* legacy structures and constants for <= 2.0 machines */
> -struct smbios_header {
> -    uint16_t length;
> -    uint8_t type;
> -} QEMU_PACKED;
> -
> -struct smbios_field {
> -    struct smbios_header header;
> -    uint8_t type;
> -    uint16_t offset;
> -    uint8_t data[];
> -} QEMU_PACKED;
> -
> -struct smbios_table {
> -    struct smbios_header header;
> -    uint8_t data[];
> -} QEMU_PACKED;
> -
> -#define SMBIOS_FIELD_ENTRY 0
> -#define SMBIOS_TABLE_ENTRY 1
> -
> -static uint8_t *smbios_entries;
> -static size_t smbios_entries_len;
> static bool smbios_uuid_encoded = true;
> -/* end: legacy structures & constants for <= 2.0 machines */
> -
> /*
>  * SMBIOS tables provided by user with '-smbios file=<foo>' option
>  */
> uint8_t *usr_blobs;
> size_t usr_blobs_len;
> -static GArray *usr_blobs_sizes;
> static unsigned usr_table_max;
> static unsigned usr_table_cnt;
> 
> @@ -78,19 +52,11 @@ static int smbios_type4_count = 0;
> static bool smbios_have_defaults;
> static uint32_t smbios_cpuid_version, smbios_cpuid_features;
> 
> -static DECLARE_BITMAP(have_binfile_bitmap, SMBIOS_MAX_TYPE+1);
> -static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
> -
> -static struct {
> -    const char *vendor, *version, *date;
> -    bool have_major_minor, uefi;
> -    uint8_t major, minor;
> -} type0;
> +DECLARE_BITMAP(smbios_have_binfile_bitmap, SMBIOS_MAX_TYPE + 1);
> +DECLARE_BITMAP(smbios_have_fields_bitmap, SMBIOS_MAX_TYPE + 1);
> 
> -static struct {
> -    const char *manufacturer, *product, *version, *serial, *sku, *family;
> -    /* uuid is in qemu_uuid */
> -} type1;
> +smbios_type0_t smbios_type0;
> +smbios_type1_t smbios_type1;
> 
> static struct {
>     const char *manufacturer, *product, *version, *serial, *asset, *location;
> @@ -491,7 +457,7 @@ static void smbios_check_type4_count(uint32_t expected_t4_count)
>     }
> }
> 
> -static void smbios_validate_table(void)
> +void smbios_validate_table(void)
> {
>     if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_32 &&
>         smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
> @@ -501,140 +467,12 @@ static void smbios_validate_table(void)
>     }
> }
> 
> -
> -/* legacy setup functions for <= 2.0 machines */
> -static void smbios_add_field(int type, int offset, const void *data, size_t len)
> -{
> -    struct smbios_field *field;
> -
> -    if (!smbios_entries) {
> -        smbios_entries_len = sizeof(uint16_t);
> -        smbios_entries = g_malloc0(smbios_entries_len);
> -    }
> -    smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
> -                                                  sizeof(*field) + len);
> -    field = (struct smbios_field *)(smbios_entries + smbios_entries_len);
> -    field->header.type = SMBIOS_FIELD_ENTRY;
> -    field->header.length = cpu_to_le16(sizeof(*field) + len);
> -
> -    field->type = type;
> -    field->offset = cpu_to_le16(offset);
> -    memcpy(field->data, data, len);
> -
> -    smbios_entries_len += sizeof(*field) + len;
> -    (*(uint16_t *)smbios_entries) =
> -            cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
> -}
> -
> -static void smbios_maybe_add_str(int type, int offset, const char *data)
> -{
> -    if (data) {
> -        smbios_add_field(type, offset, data, strlen(data) + 1);
> -    }
> -}
> -
> -static void smbios_build_type_0_fields(void)
> -{
> -    smbios_maybe_add_str(0, offsetof(struct smbios_type_0, vendor_str),
> -                         type0.vendor);
> -    smbios_maybe_add_str(0, offsetof(struct smbios_type_0, bios_version_str),
> -                         type0.version);
> -    smbios_maybe_add_str(0, offsetof(struct smbios_type_0,
> -                                     bios_release_date_str),
> -                         type0.date);
> -    if (type0.have_major_minor) {
> -        smbios_add_field(0, offsetof(struct smbios_type_0,
> -                                     system_bios_major_release),
> -                         &type0.major, 1);
> -        smbios_add_field(0, offsetof(struct smbios_type_0,
> -                                     system_bios_minor_release),
> -                         &type0.minor, 1);
> -    }
> -}
> -
> -static void smbios_build_type_1_fields(void)
> -{
> -    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, manufacturer_str),
> -                         type1.manufacturer);
> -    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, product_name_str),
> -                         type1.product);
> -    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, version_str),
> -                         type1.version);
> -    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, serial_number_str),
> -                         type1.serial);
> -    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, sku_number_str),
> -                         type1.sku);
> -    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, family_str),
> -                         type1.family);
> -    if (qemu_uuid_set) {
> -        /* We don't encode the UUID in the "wire format" here because this
> -         * function is for legacy mode and needs to keep the guest ABI, and
> -         * because we don't know what's the SMBIOS version advertised by the
> -         * BIOS.
> -         */
> -        smbios_add_field(1, offsetof(struct smbios_type_1, uuid),
> -                         &qemu_uuid, 16);
> -    }
> -}
> -
> -uint8_t *smbios_get_table_legacy(size_t *length)
> -{
> -    int i;
> -    size_t usr_offset;
> -
> -    /* also complain if fields were given for types > 1 */
> -    if (find_next_bit(have_fields_bitmap,
> -                      SMBIOS_MAX_TYPE + 1, 2) < SMBIOS_MAX_TYPE + 1) {
> -        error_report("can't process fields for smbios "
> -                     "types > 1 on machine versions < 2.1!");
> -        exit(1);
> -    }
> -
> -    if (test_bit(4, have_binfile_bitmap)) {
> -        error_report("can't process table for smbios "
> -                     "type 4 on machine versions < 2.1!");
> -        exit(1);
> -    }
> -
> -    g_free(smbios_entries);
> -    smbios_entries_len = sizeof(uint16_t);
> -    smbios_entries = g_malloc0(smbios_entries_len);
> -
> -    for (i = 0, usr_offset = 0; usr_blobs_sizes && i < usr_blobs_sizes->len;
> -         i++)
> -    {
> -        struct smbios_table *table;
> -        struct smbios_structure_header *header;
> -        size_t size = g_array_index(usr_blobs_sizes, size_t, i);
> -
> -        header = (struct smbios_structure_header *)(usr_blobs + usr_offset);
> -        smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
> -                                                   size + sizeof(*table));
> -        table = (struct smbios_table *)(smbios_entries + smbios_entries_len);
> -        table->header.type = SMBIOS_TABLE_ENTRY;
> -        table->header.length = cpu_to_le16(sizeof(*table) + size);
> -        memcpy(table->data, header, size);
> -        smbios_entries_len += sizeof(*table) + size;
> -        (*(uint16_t *)smbios_entries) =
> -            cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
> -        usr_offset += size;
> -    }
> -
> -    smbios_build_type_0_fields();
> -    smbios_build_type_1_fields();
> -    smbios_validate_table();
> -    *length = smbios_entries_len;
> -    return smbios_entries;
> -}
> -/* end: legacy setup functions for <= 2.0 machines */
> -
> -
> bool smbios_skip_table(uint8_t type, bool required_table)
> {
> -    if (test_bit(type, have_binfile_bitmap)) {
> +    if (test_bit(type, smbios_have_binfile_bitmap)) {
>         return true; /* user provided their own binary blob(s) */
>     }
> -    if (test_bit(type, have_fields_bitmap)) {
> +    if (test_bit(type, smbios_have_fields_bitmap)) {
>         return false; /* user provided fields via command line */
>     }
>     if (smbios_have_defaults && required_table) {
> @@ -661,25 +499,25 @@ static void smbios_build_type_0_table(void)
> {
>     SMBIOS_BUILD_TABLE_PRE(0, T0_BASE, false); /* optional, leave up to BIOS */
> 
> -    SMBIOS_TABLE_SET_STR(0, vendor_str, type0.vendor);
> -    SMBIOS_TABLE_SET_STR(0, bios_version_str, type0.version);
> +    SMBIOS_TABLE_SET_STR(0, vendor_str, smbios_type0.vendor);
> +    SMBIOS_TABLE_SET_STR(0, bios_version_str, smbios_type0.version);
> 
>     t->bios_starting_address_segment = cpu_to_le16(0xE800); /* from SeaBIOS */
> 
> -    SMBIOS_TABLE_SET_STR(0, bios_release_date_str, type0.date);
> +    SMBIOS_TABLE_SET_STR(0, bios_release_date_str, smbios_type0.date);
> 
>     t->bios_rom_size = 0; /* hardcoded in SeaBIOS with FIXME comment */
> 
>     t->bios_characteristics = cpu_to_le64(0x08); /* Not supported */
>     t->bios_characteristics_extension_bytes[0] = 0;
>     t->bios_characteristics_extension_bytes[1] = 0x14; /* TCD/SVVP | VM */
> -    if (type0.uefi) {
> +    if (smbios_type0.uefi) {
>         t->bios_characteristics_extension_bytes[1] |= 0x08; /* |= UEFI */
>     }
> 
> -    if (type0.have_major_minor) {
> -        t->system_bios_major_release = type0.major;
> -        t->system_bios_minor_release = type0.minor;
> +    if (smbios_type0.have_major_minor) {
> +        t->system_bios_major_release = smbios_type0.major;
> +        t->system_bios_minor_release = smbios_type0.minor;
>     } else {
>         t->system_bios_major_release = 0;
>         t->system_bios_minor_release = 0;
> @@ -709,18 +547,18 @@ static void smbios_build_type_1_table(void)
> {
>     SMBIOS_BUILD_TABLE_PRE(1, T1_BASE, true); /* required */
> 
> -    SMBIOS_TABLE_SET_STR(1, manufacturer_str, type1.manufacturer);
> -    SMBIOS_TABLE_SET_STR(1, product_name_str, type1.product);
> -    SMBIOS_TABLE_SET_STR(1, version_str, type1.version);
> -    SMBIOS_TABLE_SET_STR(1, serial_number_str, type1.serial);
> +    SMBIOS_TABLE_SET_STR(1, manufacturer_str, smbios_type1.manufacturer);
> +    SMBIOS_TABLE_SET_STR(1, product_name_str, smbios_type1.product);
> +    SMBIOS_TABLE_SET_STR(1, version_str, smbios_type1.version);
> +    SMBIOS_TABLE_SET_STR(1, serial_number_str, smbios_type1.serial);
>     if (qemu_uuid_set) {
>         smbios_encode_uuid(&t->uuid, &qemu_uuid);
>     } else {
>         memset(&t->uuid, 0, 16);
>     }
>     t->wake_up_type = 0x06; /* power switch */
> -    SMBIOS_TABLE_SET_STR(1, sku_number_str, type1.sku);
> -    SMBIOS_TABLE_SET_STR(1, family_str, type1.family);
> +    SMBIOS_TABLE_SET_STR(1, sku_number_str, smbios_type1.sku);
> +    SMBIOS_TABLE_SET_STR(1, family_str, smbios_type1.family);
> 
>     SMBIOS_BUILD_TABLE_POST;
> }
> @@ -1055,9 +893,9 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
>     smbios_uuid_encoded = uuid_encoded;
>     smbios_ep_type = ep_type;
> 
> -    SMBIOS_SET_DEFAULT(type1.manufacturer, manufacturer);
> -    SMBIOS_SET_DEFAULT(type1.product, product);
> -    SMBIOS_SET_DEFAULT(type1.version, version);
> +    SMBIOS_SET_DEFAULT(smbios_type1.manufacturer, manufacturer);
> +    SMBIOS_SET_DEFAULT(smbios_type1.product, product);
> +    SMBIOS_SET_DEFAULT(smbios_type1.version, version);
>     SMBIOS_SET_DEFAULT(type2.manufacturer, manufacturer);
>     SMBIOS_SET_DEFAULT(type2.product, product);
>     SMBIOS_SET_DEFAULT(type2.version, version);
> @@ -1321,23 +1159,25 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
>         }
> 
>         if (header->type <= SMBIOS_MAX_TYPE) {
> -            if (test_bit(header->type, have_fields_bitmap)) {
> +            if (test_bit(header->type, smbios_have_fields_bitmap)) {
>                 error_setg(errp,
>                            "can't load type %d struct, fields already specified!",
>                            header->type);
>                 return;
>             }
> -            set_bit(header->type, have_binfile_bitmap);
> +            set_bit(header->type, smbios_have_binfile_bitmap);
>         }
> 
>         if (header->type == 4) {
>             smbios_type4_count++;
>         }
> 
> -        if (!usr_blobs_sizes) {
> -            usr_blobs_sizes = g_array_new(false, false, sizeof(size_t));
> -        }
> -        g_array_append_val(usr_blobs_sizes, size);
> +        /*
> +         * preserve blob size for legacy mode so it could build its
> +         * blobs flavor from 'usr_blobs'
> +         */
> +        smbios_add_usr_blob_size(size);
> +

Could this have been made as a part of a separate patch? It is extremely hard to understand why you are doing this when it’s a part of a larger code refactoring/modularisation.


>         usr_blobs_len += size;
>         if (size > usr_table_max) {
>             usr_table_max = size;
> @@ -1356,41 +1196,42 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
>             return;
>         }
> 
> -        if (test_bit(type, have_binfile_bitmap)) {
> +        if (test_bit(type, smbios_have_binfile_bitmap)) {
>             error_setg(errp, "can't add fields, binary file already loaded!");
>             return;
>         }
> -        set_bit(type, have_fields_bitmap);
> +        set_bit(type, smbios_have_fields_bitmap);
> 
>         switch (type) {
>         case 0:
>             if (!qemu_opts_validate(opts, qemu_smbios_type0_opts, errp)) {
>                 return;
>             }
> -            save_opt(&type0.vendor, opts, "vendor");
> -            save_opt(&type0.version, opts, "version");
> -            save_opt(&type0.date, opts, "date");
> -            type0.uefi = qemu_opt_get_bool(opts, "uefi", false);
> +            save_opt(&smbios_type0.vendor, opts, "vendor");
> +            save_opt(&smbios_type0.version, opts, "version");
> +            save_opt(&smbios_type0.date, opts, "date");
> +            smbios_type0.uefi = qemu_opt_get_bool(opts, "uefi", false);
> 
>             val = qemu_opt_get(opts, "release");
>             if (val) {
> -                if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) {
> +                if (sscanf(val, "%hhu.%hhu", &smbios_type0.major,
> +                           &smbios_type0.minor) != 2) {
>                     error_setg(errp, "Invalid release");
>                     return;
>                 }
> -                type0.have_major_minor = true;
> +                smbios_type0.have_major_minor = true;
>             }
>             return;
>         case 1:
>             if (!qemu_opts_validate(opts, qemu_smbios_type1_opts, errp)) {
>                 return;
>             }
> -            save_opt(&type1.manufacturer, opts, "manufacturer");
> -            save_opt(&type1.product, opts, "product");
> -            save_opt(&type1.version, opts, "version");
> -            save_opt(&type1.serial, opts, "serial");
> -            save_opt(&type1.sku, opts, "sku");
> -            save_opt(&type1.family, opts, "family");
> +            save_opt(&smbios_type1.manufacturer, opts, "manufacturer");
> +            save_opt(&smbios_type1.product, opts, "product");
> +            save_opt(&smbios_type1.version, opts, "version");
> +            save_opt(&smbios_type1.serial, opts, "serial");
> +            save_opt(&smbios_type1.sku, opts, "sku");
> +            save_opt(&smbios_type1.family, opts, "family");
> 
>             val = qemu_opt_get(opts, "uuid");
>             if (val) {
> diff --git a/hw/smbios/smbios_legacy.c b/hw/smbios/smbios_legacy.c
> new file mode 100644
> index 0000000000..21f143e738
> --- /dev/null
> +++ b/hw/smbios/smbios_legacy.c
> @@ -0,0 +1,179 @@
> +/*
> + * SMBIOS legacy support
> + *
> + * Copyright (C) 2009 Hewlett-Packard Development Company, L.P.
> + * Copyright (C) 2013 Red Hat, Inc.
> + *
> + * Authors:
> + *  Alex Williamson <alex.williamson@hp.com>
> + *  Markus Armbruster <armbru@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + * Contributions after 2012-01-13 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/bswap.h"
> +#include "hw/firmware/smbios.h"
> +#include "sysemu/sysemu.h"
> +#include "qemu/error-report.h"
> +
> +struct smbios_header {
> +    uint16_t length;
> +    uint8_t type;
> +} QEMU_PACKED;
> +
> +struct smbios_field {
> +    struct smbios_header header;
> +    uint8_t type;
> +    uint16_t offset;
> +    uint8_t data[];
> +} QEMU_PACKED;
> +
> +struct smbios_table {
> +    struct smbios_header header;
> +    uint8_t data[];
> +} QEMU_PACKED;
> +
> +#define SMBIOS_FIELD_ENTRY 0
> +#define SMBIOS_TABLE_ENTRY 1
> +
> +static uint8_t *smbios_entries;
> +static size_t smbios_entries_len;
> +GArray *usr_blobs_sizes;
> +
> +void smbios_add_usr_blob_size(size_t size)
> +{
> +    if (!usr_blobs_sizes) {
> +        usr_blobs_sizes = g_array_new(false, false, sizeof(size_t));
> +    }
> +    g_array_append_val(usr_blobs_sizes, size);
> +}
> +
> +static void smbios_add_field(int type, int offset, const void *data, size_t len)
> +{
> +    struct smbios_field *field;
> +
> +    if (!smbios_entries) {
> +        smbios_entries_len = sizeof(uint16_t);
> +        smbios_entries = g_malloc0(smbios_entries_len);
> +    }
> +    smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
> +                                                  sizeof(*field) + len);
> +    field = (struct smbios_field *)(smbios_entries + smbios_entries_len);
> +    field->header.type = SMBIOS_FIELD_ENTRY;
> +    field->header.length = cpu_to_le16(sizeof(*field) + len);
> +
> +    field->type = type;
> +    field->offset = cpu_to_le16(offset);
> +    memcpy(field->data, data, len);
> +
> +    smbios_entries_len += sizeof(*field) + len;
> +    (*(uint16_t *)smbios_entries) =
> +            cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
> +}
> +
> +static void smbios_maybe_add_str(int type, int offset, const char *data)
> +{
> +    if (data) {
> +        smbios_add_field(type, offset, data, strlen(data) + 1);
> +    }
> +}
> +
> +static void smbios_build_type_0_fields(void)
> +{
> +    smbios_maybe_add_str(0, offsetof(struct smbios_type_0, vendor_str),
> +                         smbios_type0.vendor);
> +    smbios_maybe_add_str(0, offsetof(struct smbios_type_0, bios_version_str),
> +                         smbios_type0.version);
> +    smbios_maybe_add_str(0, offsetof(struct smbios_type_0,
> +                                     bios_release_date_str),
> +                         smbios_type0.date);
> +    if (smbios_type0.have_major_minor) {
> +        smbios_add_field(0, offsetof(struct smbios_type_0,
> +                                     system_bios_major_release),
> +                         &smbios_type0.major, 1);
> +        smbios_add_field(0, offsetof(struct smbios_type_0,
> +                                     system_bios_minor_release),
> +                         &smbios_type0.minor, 1);
> +    }
> +}
> +
> +static void smbios_build_type_1_fields(void)
> +{
> +    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, manufacturer_str),
> +                         smbios_type1.manufacturer);
> +    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, product_name_str),
> +                         smbios_type1.product);
> +    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, version_str),
> +                         smbios_type1.version);
> +    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, serial_number_str),
> +                         smbios_type1.serial);
> +    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, sku_number_str),
> +                         smbios_type1.sku);
> +    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, family_str),
> +                         smbios_type1.family);
> +    if (qemu_uuid_set) {
> +        /*
> +         * We don't encode the UUID in the "wire format" here because this
> +         * function is for legacy mode and needs to keep the guest ABI, and
> +         * because we don't know what's the SMBIOS version advertised by the
> +         * BIOS.
> +         */
> +        smbios_add_field(1, offsetof(struct smbios_type_1, uuid),
> +                         &qemu_uuid, 16);
> +    }
> +}
> +
> +uint8_t *smbios_get_table_legacy(size_t *length)
> +{
> +    int i;
> +    size_t usr_offset;
> +
> +    /* complain if fields were given for types > 1 */
> +    if (find_next_bit(smbios_have_fields_bitmap,
> +                      SMBIOS_MAX_TYPE + 1, 2) < SMBIOS_MAX_TYPE + 1) {
> +        error_report("can't process fields for smbios "
> +                     "types > 1 on machine versions < 2.1!");
> +        exit(1);
> +    }
> +
> +    if (test_bit(4, smbios_have_binfile_bitmap)) {
> +        error_report("can't process table for smbios "
> +                     "type 4 on machine versions < 2.1!");
> +        exit(1);
> +    }
> +
> +    g_free(smbios_entries);
> +    smbios_entries_len = sizeof(uint16_t);
> +    smbios_entries = g_malloc0(smbios_entries_len);
> +
> +    for (i = 0, usr_offset = 0; usr_blobs_sizes && i < usr_blobs_sizes->len;
> +         i++)
> +    {
> +        struct smbios_table *table;
> +        struct smbios_structure_header *header;
> +        size_t size = g_array_index(usr_blobs_sizes, size_t, i);
> +
> +        header = (struct smbios_structure_header *)(usr_blobs + usr_offset);
> +        smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
> +                                                   size + sizeof(*table));
> +        table = (struct smbios_table *)(smbios_entries + smbios_entries_len);
> +        table->header.type = SMBIOS_TABLE_ENTRY;
> +        table->header.length = cpu_to_le16(sizeof(*table) + size);
> +        memcpy(table->data, header, size);
> +        smbios_entries_len += sizeof(*table) + size;
> +        (*(uint16_t *)smbios_entries) =
> +            cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
> +        usr_offset += size;
> +    }
> +
> +    smbios_build_type_0_fields();
> +    smbios_build_type_1_fields();
> +    smbios_validate_table();
> +    *length = smbios_entries_len;
> +    return smbios_entries;
> +}
> diff --git a/hw/smbios/smbios_legacy_stub.c b/hw/smbios/smbios_legacy_stub.c
> new file mode 100644
> index 0000000000..91e6c75029
> --- /dev/null
> +++ b/hw/smbios/smbios_legacy_stub.c
> @@ -0,0 +1,16 @@
> +/*
> + * IPMI SMBIOS firmware handling
> + *
> + * Copyright (c) 2024 Igor Mammedov, Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/firmware/smbios.h"
> +
> +void smbios_add_usr_blob_size(size_t size)
> +{
> +}
> +
> -- 
> 2.39.3
>

On Mon, 4 Mar 2024 16:25:03 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> > On 27-Feb-2024, at 21:17, Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> > basically moving code around without functional change.
> > And exposing some symbols so that they could be shared
> > between smbbios.c and new smbios_legacy.c
> > 
> > plus some meson magic to build smbios_legacy.c only
> > for 'pc' machine and otherwise replace it with stub
> > if not selected.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > include/hw/firmware/smbios.h   |  22 +++
> > hw/i386/Kconfig                |   1 +
> > hw/smbios/Kconfig              |   2 +
> > hw/smbios/meson.build          |   4 +
> > hw/smbios/smbios.c             | 251 ++++++---------------------------
> > hw/smbios/smbios_legacy.c      | 179 +++++++++++++++++++++++
> > hw/smbios/smbios_legacy_stub.c |  16 +++
> > 7 files changed, 270 insertions(+), 205 deletions(-)
> > create mode 100644 hw/smbios/smbios_legacy.c
> > create mode 100644 hw/smbios/smbios_legacy_stub.c
> > 
> > diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
> > index 1fbff3c55f..a6d8fd6591 100644
> > --- a/include/hw/firmware/smbios.h
> > +++ b/include/hw/firmware/smbios.h
> > @@ -2,6 +2,7 @@
> > #define QEMU_SMBIOS_H
> > 
> > #include "qapi/qapi-types-machine.h"
> > +#include "qemu/bitmap.h"
> > 
> > /*
> >  * SMBIOS Support
> > @@ -16,8 +17,27 @@
> >  *
> >  */
> > 
> > +extern uint8_t *usr_blobs;
> > +extern GArray *usr_blobs_sizes;
> > +
> > +typedef struct {
> > +    const char *vendor, *version, *date;
> > +    bool have_major_minor, uefi;
> > +    uint8_t major, minor;
> > +} smbios_type0_t;
> > +extern smbios_type0_t smbios_type0;
> > +
> > +typedef struct {
> > +    const char *manufacturer, *product, *version, *serial, *sku, *family;
> > +    /* uuid is in qemu_uuid */
> > +} smbios_type1_t;
> > +extern smbios_type1_t smbios_type1;
> > +
> > 
> > #define SMBIOS_MAX_TYPE 127
> > +extern DECLARE_BITMAP(smbios_have_binfile_bitmap, SMBIOS_MAX_TYPE + 1);
> > +extern DECLARE_BITMAP(smbios_have_fields_bitmap, SMBIOS_MAX_TYPE + 1);
> > +
> > #define offsetofend(TYPE, MEMBER) \
> >        (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
> > 
> > @@ -290,6 +310,8 @@ struct smbios_type_127 {
> >     struct smbios_structure_header header;
> > } QEMU_PACKED;
> > 
> > +void smbios_validate_table(void);
> > +void smbios_add_usr_blob_size(size_t size);
> > void smbios_entry_add(QemuOpts *opts, Error **errp);
> > void smbios_set_cpuid(uint32_t version, uint32_t features);
> > void smbios_set_defaults(const char *manufacturer, const char *product,
> > diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> > index a1846be6f7..a6ee052f9a 100644
> > --- a/hw/i386/Kconfig
> > +++ b/hw/i386/Kconfig
> > @@ -76,6 +76,7 @@ config I440FX
> >     select PIIX
> >     select DIMM
> >     select SMBIOS
> > +    select SMBIOS_LEGACY
> >     select FW_CFG_DMA
> > 
> > config ISAPC
> > diff --git a/hw/smbios/Kconfig b/hw/smbios/Kconfig
> > index 553adf4bfc..8d989a2f1b 100644
> > --- a/hw/smbios/Kconfig
> > +++ b/hw/smbios/Kconfig
> > @@ -1,2 +1,4 @@
> > config SMBIOS
> >     bool
> > +config SMBIOS_LEGACY
> > +    bool
> > diff --git a/hw/smbios/meson.build b/hw/smbios/meson.build
> > index 7046967462..a59039f669 100644
> > --- a/hw/smbios/meson.build
> > +++ b/hw/smbios/meson.build
> > @@ -4,5 +4,9 @@ smbios_ss.add(when: 'CONFIG_IPMI',
> >               if_true: files('smbios_type_38.c'),
> >               if_false: files('smbios_type_38-stub.c'))
> > 
> > +smbios_ss.add(when: 'CONFIG_SMBIOS_LEGACY',
> > +              if_true: files('smbios_legacy.c'),
> > +              if_false: files('smbios_legacy_stub.c'))
> > +
> > system_ss.add_all(when: 'CONFIG_SMBIOS', if_true: smbios_ss)
> > system_ss.add(when: 'CONFIG_SMBIOS', if_false: files('smbios-stub.c'))
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index 97cf762228..fb1f05fcde 100644
> > --- a/hw/smbios/smbios.c
> > +++ b/hw/smbios/smbios.c
> > @@ -31,38 +31,12 @@
> > #include "hw/pci/pci_device.h"
> > #include "smbios_build.h"
> > 
> > -/* legacy structures and constants for <= 2.0 machines */
> > -struct smbios_header {
> > -    uint16_t length;
> > -    uint8_t type;
> > -} QEMU_PACKED;
> > -
> > -struct smbios_field {
> > -    struct smbios_header header;
> > -    uint8_t type;
> > -    uint16_t offset;
> > -    uint8_t data[];
> > -} QEMU_PACKED;
> > -
> > -struct smbios_table {
> > -    struct smbios_header header;
> > -    uint8_t data[];
> > -} QEMU_PACKED;
> > -
> > -#define SMBIOS_FIELD_ENTRY 0
> > -#define SMBIOS_TABLE_ENTRY 1
> > -
> > -static uint8_t *smbios_entries;
> > -static size_t smbios_entries_len;
> > static bool smbios_uuid_encoded = true;
> > -/* end: legacy structures & constants for <= 2.0 machines */
> > -
> > /*
> >  * SMBIOS tables provided by user with '-smbios file=<foo>' option
> >  */
> > uint8_t *usr_blobs;
> > size_t usr_blobs_len;
> > -static GArray *usr_blobs_sizes;
> > static unsigned usr_table_max;
> > static unsigned usr_table_cnt;
> > 
> > @@ -78,19 +52,11 @@ static int smbios_type4_count = 0;
> > static bool smbios_have_defaults;
> > static uint32_t smbios_cpuid_version, smbios_cpuid_features;
> > 
> > -static DECLARE_BITMAP(have_binfile_bitmap, SMBIOS_MAX_TYPE+1);
> > -static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
> > -
> > -static struct {
> > -    const char *vendor, *version, *date;
> > -    bool have_major_minor, uefi;
> > -    uint8_t major, minor;
> > -} type0;
> > +DECLARE_BITMAP(smbios_have_binfile_bitmap, SMBIOS_MAX_TYPE + 1);
> > +DECLARE_BITMAP(smbios_have_fields_bitmap, SMBIOS_MAX_TYPE + 1);
> > 
> > -static struct {
> > -    const char *manufacturer, *product, *version, *serial, *sku, *family;
> > -    /* uuid is in qemu_uuid */
> > -} type1;
> > +smbios_type0_t smbios_type0;
> > +smbios_type1_t smbios_type1;
> > 
> > static struct {
> >     const char *manufacturer, *product, *version, *serial, *asset, *location;
> > @@ -491,7 +457,7 @@ static void smbios_check_type4_count(uint32_t expected_t4_count)
> >     }
> > }
> > 
> > -static void smbios_validate_table(void)
> > +void smbios_validate_table(void)
> > {
> >     if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_32 &&
> >         smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
> > @@ -501,140 +467,12 @@ static void smbios_validate_table(void)
> >     }
> > }
> > 
> > -
> > -/* legacy setup functions for <= 2.0 machines */
> > -static void smbios_add_field(int type, int offset, const void *data, size_t len)
> > -{
> > -    struct smbios_field *field;
> > -
> > -    if (!smbios_entries) {
> > -        smbios_entries_len = sizeof(uint16_t);
> > -        smbios_entries = g_malloc0(smbios_entries_len);
> > -    }
> > -    smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
> > -                                                  sizeof(*field) + len);
> > -    field = (struct smbios_field *)(smbios_entries + smbios_entries_len);
> > -    field->header.type = SMBIOS_FIELD_ENTRY;
> > -    field->header.length = cpu_to_le16(sizeof(*field) + len);
> > -
> > -    field->type = type;
> > -    field->offset = cpu_to_le16(offset);
> > -    memcpy(field->data, data, len);
> > -
> > -    smbios_entries_len += sizeof(*field) + len;
> > -    (*(uint16_t *)smbios_entries) =
> > -            cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
> > -}
> > -
> > -static void smbios_maybe_add_str(int type, int offset, const char *data)
> > -{
> > -    if (data) {
> > -        smbios_add_field(type, offset, data, strlen(data) + 1);
> > -    }
> > -}
> > -
> > -static void smbios_build_type_0_fields(void)
> > -{
> > -    smbios_maybe_add_str(0, offsetof(struct smbios_type_0, vendor_str),
> > -                         type0.vendor);
> > -    smbios_maybe_add_str(0, offsetof(struct smbios_type_0, bios_version_str),
> > -                         type0.version);
> > -    smbios_maybe_add_str(0, offsetof(struct smbios_type_0,
> > -                                     bios_release_date_str),
> > -                         type0.date);
> > -    if (type0.have_major_minor) {
> > -        smbios_add_field(0, offsetof(struct smbios_type_0,
> > -                                     system_bios_major_release),
> > -                         &type0.major, 1);
> > -        smbios_add_field(0, offsetof(struct smbios_type_0,
> > -                                     system_bios_minor_release),
> > -                         &type0.minor, 1);
> > -    }
> > -}
> > -
> > -static void smbios_build_type_1_fields(void)
> > -{
> > -    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, manufacturer_str),
> > -                         type1.manufacturer);
> > -    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, product_name_str),
> > -                         type1.product);
> > -    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, version_str),
> > -                         type1.version);
> > -    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, serial_number_str),
> > -                         type1.serial);
> > -    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, sku_number_str),
> > -                         type1.sku);
> > -    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, family_str),
> > -                         type1.family);
> > -    if (qemu_uuid_set) {
> > -        /* We don't encode the UUID in the "wire format" here because this
> > -         * function is for legacy mode and needs to keep the guest ABI, and
> > -         * because we don't know what's the SMBIOS version advertised by the
> > -         * BIOS.
> > -         */
> > -        smbios_add_field(1, offsetof(struct smbios_type_1, uuid),
> > -                         &qemu_uuid, 16);
> > -    }
> > -}
> > -
> > -uint8_t *smbios_get_table_legacy(size_t *length)
> > -{
> > -    int i;
> > -    size_t usr_offset;
> > -
> > -    /* also complain if fields were given for types > 1 */
> > -    if (find_next_bit(have_fields_bitmap,
> > -                      SMBIOS_MAX_TYPE + 1, 2) < SMBIOS_MAX_TYPE + 1) {
> > -        error_report("can't process fields for smbios "
> > -                     "types > 1 on machine versions < 2.1!");
> > -        exit(1);
> > -    }
> > -
> > -    if (test_bit(4, have_binfile_bitmap)) {
> > -        error_report("can't process table for smbios "
> > -                     "type 4 on machine versions < 2.1!");
> > -        exit(1);
> > -    }
> > -
> > -    g_free(smbios_entries);
> > -    smbios_entries_len = sizeof(uint16_t);
> > -    smbios_entries = g_malloc0(smbios_entries_len);
> > -
> > -    for (i = 0, usr_offset = 0; usr_blobs_sizes && i < usr_blobs_sizes->len;
> > -         i++)
> > -    {
> > -        struct smbios_table *table;
> > -        struct smbios_structure_header *header;
> > -        size_t size = g_array_index(usr_blobs_sizes, size_t, i);
> > -
> > -        header = (struct smbios_structure_header *)(usr_blobs + usr_offset);
> > -        smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
> > -                                                   size + sizeof(*table));
> > -        table = (struct smbios_table *)(smbios_entries + smbios_entries_len);
> > -        table->header.type = SMBIOS_TABLE_ENTRY;
> > -        table->header.length = cpu_to_le16(sizeof(*table) + size);
> > -        memcpy(table->data, header, size);
> > -        smbios_entries_len += sizeof(*table) + size;
> > -        (*(uint16_t *)smbios_entries) =
> > -            cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
> > -        usr_offset += size;
> > -    }
> > -
> > -    smbios_build_type_0_fields();
> > -    smbios_build_type_1_fields();
> > -    smbios_validate_table();
> > -    *length = smbios_entries_len;
> > -    return smbios_entries;
> > -}
> > -/* end: legacy setup functions for <= 2.0 machines */
> > -
> > -
> > bool smbios_skip_table(uint8_t type, bool required_table)
> > {
> > -    if (test_bit(type, have_binfile_bitmap)) {
> > +    if (test_bit(type, smbios_have_binfile_bitmap)) {
> >         return true; /* user provided their own binary blob(s) */
> >     }
> > -    if (test_bit(type, have_fields_bitmap)) {
> > +    if (test_bit(type, smbios_have_fields_bitmap)) {
> >         return false; /* user provided fields via command line */
> >     }
> >     if (smbios_have_defaults && required_table) {
> > @@ -661,25 +499,25 @@ static void smbios_build_type_0_table(void)
> > {
> >     SMBIOS_BUILD_TABLE_PRE(0, T0_BASE, false); /* optional, leave up to BIOS */
> > 
> > -    SMBIOS_TABLE_SET_STR(0, vendor_str, type0.vendor);
> > -    SMBIOS_TABLE_SET_STR(0, bios_version_str, type0.version);
> > +    SMBIOS_TABLE_SET_STR(0, vendor_str, smbios_type0.vendor);
> > +    SMBIOS_TABLE_SET_STR(0, bios_version_str, smbios_type0.version);
> > 
> >     t->bios_starting_address_segment = cpu_to_le16(0xE800); /* from SeaBIOS */
> > 
> > -    SMBIOS_TABLE_SET_STR(0, bios_release_date_str, type0.date);
> > +    SMBIOS_TABLE_SET_STR(0, bios_release_date_str, smbios_type0.date);
> > 
> >     t->bios_rom_size = 0; /* hardcoded in SeaBIOS with FIXME comment */
> > 
> >     t->bios_characteristics = cpu_to_le64(0x08); /* Not supported */
> >     t->bios_characteristics_extension_bytes[0] = 0;
> >     t->bios_characteristics_extension_bytes[1] = 0x14; /* TCD/SVVP | VM */
> > -    if (type0.uefi) {
> > +    if (smbios_type0.uefi) {
> >         t->bios_characteristics_extension_bytes[1] |= 0x08; /* |= UEFI */
> >     }
> > 
> > -    if (type0.have_major_minor) {
> > -        t->system_bios_major_release = type0.major;
> > -        t->system_bios_minor_release = type0.minor;
> > +    if (smbios_type0.have_major_minor) {
> > +        t->system_bios_major_release = smbios_type0.major;
> > +        t->system_bios_minor_release = smbios_type0.minor;
> >     } else {
> >         t->system_bios_major_release = 0;
> >         t->system_bios_minor_release = 0;
> > @@ -709,18 +547,18 @@ static void smbios_build_type_1_table(void)
> > {
> >     SMBIOS_BUILD_TABLE_PRE(1, T1_BASE, true); /* required */
> > 
> > -    SMBIOS_TABLE_SET_STR(1, manufacturer_str, type1.manufacturer);
> > -    SMBIOS_TABLE_SET_STR(1, product_name_str, type1.product);
> > -    SMBIOS_TABLE_SET_STR(1, version_str, type1.version);
> > -    SMBIOS_TABLE_SET_STR(1, serial_number_str, type1.serial);
> > +    SMBIOS_TABLE_SET_STR(1, manufacturer_str, smbios_type1.manufacturer);
> > +    SMBIOS_TABLE_SET_STR(1, product_name_str, smbios_type1.product);
> > +    SMBIOS_TABLE_SET_STR(1, version_str, smbios_type1.version);
> > +    SMBIOS_TABLE_SET_STR(1, serial_number_str, smbios_type1.serial);
> >     if (qemu_uuid_set) {
> >         smbios_encode_uuid(&t->uuid, &qemu_uuid);
> >     } else {
> >         memset(&t->uuid, 0, 16);
> >     }
> >     t->wake_up_type = 0x06; /* power switch */
> > -    SMBIOS_TABLE_SET_STR(1, sku_number_str, type1.sku);
> > -    SMBIOS_TABLE_SET_STR(1, family_str, type1.family);
> > +    SMBIOS_TABLE_SET_STR(1, sku_number_str, smbios_type1.sku);
> > +    SMBIOS_TABLE_SET_STR(1, family_str, smbios_type1.family);
> > 
> >     SMBIOS_BUILD_TABLE_POST;
> > }
> > @@ -1055,9 +893,9 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
> >     smbios_uuid_encoded = uuid_encoded;
> >     smbios_ep_type = ep_type;
> > 
> > -    SMBIOS_SET_DEFAULT(type1.manufacturer, manufacturer);
> > -    SMBIOS_SET_DEFAULT(type1.product, product);
> > -    SMBIOS_SET_DEFAULT(type1.version, version);
> > +    SMBIOS_SET_DEFAULT(smbios_type1.manufacturer, manufacturer);
> > +    SMBIOS_SET_DEFAULT(smbios_type1.product, product);
> > +    SMBIOS_SET_DEFAULT(smbios_type1.version, version);
> >     SMBIOS_SET_DEFAULT(type2.manufacturer, manufacturer);
> >     SMBIOS_SET_DEFAULT(type2.product, product);
> >     SMBIOS_SET_DEFAULT(type2.version, version);
> > @@ -1321,23 +1159,25 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> >         }
> > 
> >         if (header->type <= SMBIOS_MAX_TYPE) {
> > -            if (test_bit(header->type, have_fields_bitmap)) {
> > +            if (test_bit(header->type, smbios_have_fields_bitmap)) {
> >                 error_setg(errp,
> >                            "can't load type %d struct, fields already specified!",
> >                            header->type);
> >                 return;
> >             }
> > -            set_bit(header->type, have_binfile_bitmap);
> > +            set_bit(header->type, smbios_have_binfile_bitmap);
> >         }
> > 
> >         if (header->type == 4) {
> >             smbios_type4_count++;
> >         }
> > 
> > -        if (!usr_blobs_sizes) {
> > -            usr_blobs_sizes = g_array_new(false, false, sizeof(size_t));
> > -        }
> > -        g_array_append_val(usr_blobs_sizes, size);
> > +        /*
> > +         * preserve blob size for legacy mode so it could build its
> > +         * blobs flavor from 'usr_blobs'
> > +         */
> > +        smbios_add_usr_blob_size(size);
> > +  
> 
> Could this have been made as a part of a separate patch? It is extremely hard to understand why you are doing this when it’s a part of a larger code refactoring/modularisation.

sure, will do on respin


> 
> 
> >         usr_blobs_len += size;
> >         if (size > usr_table_max) {
> >             usr_table_max = size;
> > @@ -1356,41 +1196,42 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> >             return;
> >         }
> > 
> > -        if (test_bit(type, have_binfile_bitmap)) {
> > +        if (test_bit(type, smbios_have_binfile_bitmap)) {
> >             error_setg(errp, "can't add fields, binary file already loaded!");
> >             return;
> >         }
> > -        set_bit(type, have_fields_bitmap);
> > +        set_bit(type, smbios_have_fields_bitmap);
> > 
> >         switch (type) {
> >         case 0:
> >             if (!qemu_opts_validate(opts, qemu_smbios_type0_opts, errp)) {
> >                 return;
> >             }
> > -            save_opt(&type0.vendor, opts, "vendor");
> > -            save_opt(&type0.version, opts, "version");
> > -            save_opt(&type0.date, opts, "date");
> > -            type0.uefi = qemu_opt_get_bool(opts, "uefi", false);
> > +            save_opt(&smbios_type0.vendor, opts, "vendor");
> > +            save_opt(&smbios_type0.version, opts, "version");
> > +            save_opt(&smbios_type0.date, opts, "date");
> > +            smbios_type0.uefi = qemu_opt_get_bool(opts, "uefi", false);
> > 
> >             val = qemu_opt_get(opts, "release");
> >             if (val) {
> > -                if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) {
> > +                if (sscanf(val, "%hhu.%hhu", &smbios_type0.major,
> > +                           &smbios_type0.minor) != 2) {
> >                     error_setg(errp, "Invalid release");
> >                     return;
> >                 }
> > -                type0.have_major_minor = true;
> > +                smbios_type0.have_major_minor = true;
> >             }
> >             return;
> >         case 1:
> >             if (!qemu_opts_validate(opts, qemu_smbios_type1_opts, errp)) {
> >                 return;
> >             }
> > -            save_opt(&type1.manufacturer, opts, "manufacturer");
> > -            save_opt(&type1.product, opts, "product");
> > -            save_opt(&type1.version, opts, "version");
> > -            save_opt(&type1.serial, opts, "serial");
> > -            save_opt(&type1.sku, opts, "sku");
> > -            save_opt(&type1.family, opts, "family");
> > +            save_opt(&smbios_type1.manufacturer, opts, "manufacturer");
> > +            save_opt(&smbios_type1.product, opts, "product");
> > +            save_opt(&smbios_type1.version, opts, "version");
> > +            save_opt(&smbios_type1.serial, opts, "serial");
> > +            save_opt(&smbios_type1.sku, opts, "sku");
> > +            save_opt(&smbios_type1.family, opts, "family");
> > 
> >             val = qemu_opt_get(opts, "uuid");
> >             if (val) {
> > diff --git a/hw/smbios/smbios_legacy.c b/hw/smbios/smbios_legacy.c
> > new file mode 100644
> > index 0000000000..21f143e738
> > --- /dev/null
> > +++ b/hw/smbios/smbios_legacy.c
> > @@ -0,0 +1,179 @@
> > +/*
> > + * SMBIOS legacy support
> > + *
> > + * Copyright (C) 2009 Hewlett-Packard Development Company, L.P.
> > + * Copyright (C) 2013 Red Hat, Inc.
> > + *
> > + * Authors:
> > + *  Alex Williamson <alex.williamson@hp.com>
> > + *  Markus Armbruster <armbru@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > + * the COPYING file in the top-level directory.
> > + *
> > + * Contributions after 2012-01-13 are licensed under the terms of the
> > + * GNU GPL, version 2 or (at your option) any later version.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/bswap.h"
> > +#include "hw/firmware/smbios.h"
> > +#include "sysemu/sysemu.h"
> > +#include "qemu/error-report.h"
> > +
> > +struct smbios_header {
> > +    uint16_t length;
> > +    uint8_t type;
> > +} QEMU_PACKED;
> > +
> > +struct smbios_field {
> > +    struct smbios_header header;
> > +    uint8_t type;
> > +    uint16_t offset;
> > +    uint8_t data[];
> > +} QEMU_PACKED;
> > +
> > +struct smbios_table {
> > +    struct smbios_header header;
> > +    uint8_t data[];
> > +} QEMU_PACKED;
> > +
> > +#define SMBIOS_FIELD_ENTRY 0
> > +#define SMBIOS_TABLE_ENTRY 1
> > +
> > +static uint8_t *smbios_entries;
> > +static size_t smbios_entries_len;
> > +GArray *usr_blobs_sizes;
> > +
> > +void smbios_add_usr_blob_size(size_t size)
> > +{
> > +    if (!usr_blobs_sizes) {
> > +        usr_blobs_sizes = g_array_new(false, false, sizeof(size_t));
> > +    }
> > +    g_array_append_val(usr_blobs_sizes, size);
> > +}
> > +
> > +static void smbios_add_field(int type, int offset, const void *data, size_t len)
> > +{
> > +    struct smbios_field *field;
> > +
> > +    if (!smbios_entries) {
> > +        smbios_entries_len = sizeof(uint16_t);
> > +        smbios_entries = g_malloc0(smbios_entries_len);
> > +    }
> > +    smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
> > +                                                  sizeof(*field) + len);
> > +    field = (struct smbios_field *)(smbios_entries + smbios_entries_len);
> > +    field->header.type = SMBIOS_FIELD_ENTRY;
> > +    field->header.length = cpu_to_le16(sizeof(*field) + len);
> > +
> > +    field->type = type;
> > +    field->offset = cpu_to_le16(offset);
> > +    memcpy(field->data, data, len);
> > +
> > +    smbios_entries_len += sizeof(*field) + len;
> > +    (*(uint16_t *)smbios_entries) =
> > +            cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
> > +}
> > +
> > +static void smbios_maybe_add_str(int type, int offset, const char *data)
> > +{
> > +    if (data) {
> > +        smbios_add_field(type, offset, data, strlen(data) + 1);
> > +    }
> > +}
> > +
> > +static void smbios_build_type_0_fields(void)
> > +{
> > +    smbios_maybe_add_str(0, offsetof(struct smbios_type_0, vendor_str),
> > +                         smbios_type0.vendor);
> > +    smbios_maybe_add_str(0, offsetof(struct smbios_type_0, bios_version_str),
> > +                         smbios_type0.version);
> > +    smbios_maybe_add_str(0, offsetof(struct smbios_type_0,
> > +                                     bios_release_date_str),
> > +                         smbios_type0.date);
> > +    if (smbios_type0.have_major_minor) {
> > +        smbios_add_field(0, offsetof(struct smbios_type_0,
> > +                                     system_bios_major_release),
> > +                         &smbios_type0.major, 1);
> > +        smbios_add_field(0, offsetof(struct smbios_type_0,
> > +                                     system_bios_minor_release),
> > +                         &smbios_type0.minor, 1);
> > +    }
> > +}
> > +
> > +static void smbios_build_type_1_fields(void)
> > +{
> > +    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, manufacturer_str),
> > +                         smbios_type1.manufacturer);
> > +    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, product_name_str),
> > +                         smbios_type1.product);
> > +    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, version_str),
> > +                         smbios_type1.version);
> > +    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, serial_number_str),
> > +                         smbios_type1.serial);
> > +    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, sku_number_str),
> > +                         smbios_type1.sku);
> > +    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, family_str),
> > +                         smbios_type1.family);
> > +    if (qemu_uuid_set) {
> > +        /*
> > +         * We don't encode the UUID in the "wire format" here because this
> > +         * function is for legacy mode and needs to keep the guest ABI, and
> > +         * because we don't know what's the SMBIOS version advertised by the
> > +         * BIOS.
> > +         */
> > +        smbios_add_field(1, offsetof(struct smbios_type_1, uuid),
> > +                         &qemu_uuid, 16);
> > +    }
> > +}
> > +
> > +uint8_t *smbios_get_table_legacy(size_t *length)
> > +{
> > +    int i;
> > +    size_t usr_offset;
> > +
> > +    /* complain if fields were given for types > 1 */
> > +    if (find_next_bit(smbios_have_fields_bitmap,
> > +                      SMBIOS_MAX_TYPE + 1, 2) < SMBIOS_MAX_TYPE + 1) {
> > +        error_report("can't process fields for smbios "
> > +                     "types > 1 on machine versions < 2.1!");
> > +        exit(1);
> > +    }
> > +
> > +    if (test_bit(4, smbios_have_binfile_bitmap)) {
> > +        error_report("can't process table for smbios "
> > +                     "type 4 on machine versions < 2.1!");
> > +        exit(1);
> > +    }
> > +
> > +    g_free(smbios_entries);
> > +    smbios_entries_len = sizeof(uint16_t);
> > +    smbios_entries = g_malloc0(smbios_entries_len);
> > +
> > +    for (i = 0, usr_offset = 0; usr_blobs_sizes && i < usr_blobs_sizes->len;
> > +         i++)
> > +    {
> > +        struct smbios_table *table;
> > +        struct smbios_structure_header *header;
> > +        size_t size = g_array_index(usr_blobs_sizes, size_t, i);
> > +
> > +        header = (struct smbios_structure_header *)(usr_blobs + usr_offset);
> > +        smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
> > +                                                   size + sizeof(*table));
> > +        table = (struct smbios_table *)(smbios_entries + smbios_entries_len);
> > +        table->header.type = SMBIOS_TABLE_ENTRY;
> > +        table->header.length = cpu_to_le16(sizeof(*table) + size);
> > +        memcpy(table->data, header, size);
> > +        smbios_entries_len += sizeof(*table) + size;
> > +        (*(uint16_t *)smbios_entries) =
> > +            cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
> > +        usr_offset += size;
> > +    }
> > +
> > +    smbios_build_type_0_fields();
> > +    smbios_build_type_1_fields();
> > +    smbios_validate_table();
> > +    *length = smbios_entries_len;
> > +    return smbios_entries;
> > +}
> > diff --git a/hw/smbios/smbios_legacy_stub.c b/hw/smbios/smbios_legacy_stub.c
> > new file mode 100644
> > index 0000000000..91e6c75029
> > --- /dev/null
> > +++ b/hw/smbios/smbios_legacy_stub.c
> > @@ -0,0 +1,16 @@
> > +/*
> > + * IPMI SMBIOS firmware handling
> > + *
> > + * Copyright (c) 2024 Igor Mammedov, Red Hat, Inc.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/firmware/smbios.h"
> > +
> > +void smbios_add_usr_blob_size(size_t size)
> > +{
> > +}
> > +
> > -- 
> > 2.39.3
> >   
>

On Mon, 4 Mar 2024 15:23:00 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Mon, 4 Mar 2024 16:25:03 +0530
> Ani Sinha <anisinha@redhat.com> wrote:
> 
> > > On 27-Feb-2024, at 21:17, Igor Mammedov <imammedo@redhat.com> wrote:
> > > 
> > > basically moving code around without functional change.
> > > And exposing some symbols so that they could be shared
> > > between smbbios.c and new smbios_legacy.c
> > > 
> > > plus some meson magic to build smbios_legacy.c only
> > > for 'pc' machine and otherwise replace it with stub
> > > if not selected.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
[...]
> > > +        /*
> > > +         * preserve blob size for legacy mode so it could build its
> > > +         * blobs flavor from 'usr_blobs'
> > > +         */
> > > +        smbios_add_usr_blob_size(size);
> > > +    
> > 
> > Could this have been made as a part of a separate patch? It is extremely hard to understand why you are doing this when it’s a part of a larger code refactoring/modularisation.  
> 
> sure, will do on respin

I also has split out variables renaming into a separate patch.
Hope it will make it easier to review.
 
> 
[...]