> On 06-Mar-2024, at 12:11, Ani Sinha <anisi...@redhat.com> wrote:
>
>
>
> On Tue, 5 Mar 2024, Igor Mammedov wrote:
>
>> currently smbios_entry_add() preserves internally '-smbios type='
>> options but tables provided with '-smbios file=' are stored directly
>> into blob that eventually will be exposed to VM. And then later
>> QEMU adds default/'-smbios type' entries on top into the same blob.
>>
>> It makes impossible to generate tables more than once, hence
>> 'immutable' guard was used.
>> Make it possible to regenerate final blob by storing user provided
>> blobs into a dedicated area (usr_blobs) and then copy it when
>> composing final blob. Which also makes handling of -smbios
>> options consistent.
>>
>> As side effect of this and previous commits there is no need to
>> generate legacy smbios_entries at the time options are parsed.
>> Instead compose smbios_entries on demand from usr_blobs like
>> it is done for non-legacy SMBIOS tables.
>>
>> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
>> Tested-by: Fiona Ebner <f.eb...@proxmox.com>
>
> Reviewed-by: Ani Sinha <anisi...@redhat.com>
>
>> ---
>> hw/smbios/smbios.c | 179 +++++++++++++++++++++++----------------------
>> 1 file changed, 92 insertions(+), 87 deletions(-)
>>
>> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
>> index c46fc93357..aa2cc5bdbd 100644
>> --- a/hw/smbios/smbios.c
>> +++ b/hw/smbios/smbios.c
>> @@ -57,6 +57,14 @@ 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;
>>
>> uint8_t *smbios_tables;
>> size_t smbios_tables_len;
>> @@ -67,7 +75,6 @@ static SmbiosEntryPointType smbios_ep_type =
>> SMBIOS_ENTRY_POINT_TYPE_32;
>> static SmbiosEntryPoint ep;
>>
>> static int smbios_type4_count = 0;
>> -static bool smbios_immutable;
>> static bool smbios_have_defaults;
>> static uint32_t smbios_cpuid_version, smbios_cpuid_features;
>>
>> @@ -569,9 +576,8 @@ static void smbios_build_type_1_fields(void)
>>
>> uint8_t *smbios_get_table_legacy(uint32_t expected_t4_count, size_t *length)
>> {
>> - /* drop unwanted version of command-line file blob(s) */
>> - g_free(smbios_tables);
>> - smbios_tables = NULL;
>> + int i;
>> + size_t usr_offset;
>>
>> /* also complain if fields were given for types > 1 */
>> if (find_next_bit(have_fields_bitmap,
>> @@ -581,12 +587,33 @@ uint8_t *smbios_get_table_legacy(uint32_t
>> expected_t4_count, size_t *length)
>> exit(1);
>> }
>>
>> - if (!smbios_immutable) {
>> - smbios_build_type_0_fields();
>> - smbios_build_type_1_fields();
>> - smbios_validate_table(expected_t4_count);
>> - smbios_immutable = true;
>> + 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);
>
> I know this comes from existing code but can you please explain why we add
> 1 to it? This is confusing and a comment here would be nice.
>
>> + usr_offset += size;
>
> It would be better if we could add a comment here describing a bit what
> this is all about.
>
> user blobs are an array of smbios_structure_header entries whereas legacy
> tables are an array of smbios_table structures where
> smbios_table->data represents the a single user provided table blob in
> smbios_structure_header.
Igor, are you going to send a v3 for this with the comments added?
>
>> }
>> +
>> + smbios_build_type_0_fields();
>> + smbios_build_type_1_fields();
>> + smbios_validate_table(expected_t4_count);
>> *length = smbios_entries_len;
>> return smbios_entries;
>> }
>> @@ -1094,67 +1121,67 @@ void smbios_get_tables(MachineState *ms,
>> {
>> unsigned i, dimm_cnt, offset;
>>
>> - /* drop unwanted (legacy) version of command-line file blob(s) */
>> - g_free(smbios_entries);
>> - smbios_entries = NULL;
>> + g_free(smbios_tables);
>> + smbios_tables = g_memdup2(usr_blobs, usr_blobs_len);
>
> Again a comment describing here would be nice as to why memdup is ok.
>
>> + smbios_tables_len = usr_blobs_len;
>> + smbios_table_max = usr_table_max;
>> + smbios_table_cnt = usr_table_cnt;
>>
>> - if (!smbios_immutable) {
>> - smbios_build_type_0_table();
>> - smbios_build_type_1_table();
>> - smbios_build_type_2_table();
>> - smbios_build_type_3_table();
>> + smbios_build_type_0_table();
>> + smbios_build_type_1_table();
>> + smbios_build_type_2_table();
>> + smbios_build_type_3_table();
>>
>> - assert(ms->smp.sockets >= 1);
>> + assert(ms->smp.sockets >= 1);
>>
>> - for (i = 0; i < ms->smp.sockets; i++) {
>> - smbios_build_type_4_table(ms, i);
>> - }
>> + for (i = 0; i < ms->smp.sockets; i++) {
>> + smbios_build_type_4_table(ms, i);
>> + }
>>
>> - smbios_build_type_8_table();
>> - smbios_build_type_11_table();
>> + smbios_build_type_8_table();
>> + smbios_build_type_11_table();
>>
>> #define MAX_DIMM_SZ (16 * GiB)
>> #define GET_DIMM_SZ ((i < dimm_cnt - 1) ? MAX_DIMM_SZ \
>> : ((current_machine->ram_size - 1) %
>> MAX_DIMM_SZ) + 1)
>>
>> - dimm_cnt = QEMU_ALIGN_UP(current_machine->ram_size, MAX_DIMM_SZ) /
>> MAX_DIMM_SZ;
>> + dimm_cnt = QEMU_ALIGN_UP(current_machine->ram_size, MAX_DIMM_SZ) /
>> + MAX_DIMM_SZ;
>>
>> - /*
>> - * The offset determines if we need to keep additional space between
>> - * table 17 and table 19 header handle numbers so that they do
>> - * not overlap. For example, for a VM with larger than 8 TB guest
>> - * memory and DIMM like chunks of 16 GiB, the default space between
>> - * the two tables (T19_BASE - T17_BASE = 512) is not enough.
>> - */
>> - offset = (dimm_cnt > (T19_BASE - T17_BASE)) ? \
>> - dimm_cnt - (T19_BASE - T17_BASE) : 0;
>> + /*
>> + * The offset determines if we need to keep additional space between
>> + * table 17 and table 19 header handle numbers so that they do
>> + * not overlap. For example, for a VM with larger than 8 TB guest
>> + * memory and DIMM like chunks of 16 GiB, the default space between
>> + * the two tables (T19_BASE - T17_BASE = 512) is not enough.
>> + */
>> + offset = (dimm_cnt > (T19_BASE - T17_BASE)) ? \
>> + dimm_cnt - (T19_BASE - T17_BASE) : 0;
>>
>> - smbios_build_type_16_table(dimm_cnt);
>> + smbios_build_type_16_table(dimm_cnt);
>>
>> - for (i = 0; i < dimm_cnt; i++) {
>> - smbios_build_type_17_table(i, GET_DIMM_SZ);
>> - }
>> + for (i = 0; i < dimm_cnt; i++) {
>> + smbios_build_type_17_table(i, GET_DIMM_SZ);
>> + }
>>
>> - for (i = 0; i < mem_array_size; i++) {
>> - smbios_build_type_19_table(i, offset, mem_array[i].address,
>> - mem_array[i].length);
>> - }
>> + for (i = 0; i < mem_array_size; i++) {
>> + smbios_build_type_19_table(i, offset, mem_array[i].address,
>> + mem_array[i].length);
>> + }
>>
>> - /*
>> - * make sure 16 bit handle numbers in the headers of tables 19
>> - * and 32 do not overlap.
>> - */
>> - assert((mem_array_size + offset) < (T32_BASE - T19_BASE));
>> + /*
>> + * make sure 16 bit handle numbers in the headers of tables 19
>> + * and 32 do not overlap.
>> + */
>> + assert((mem_array_size + offset) < (T32_BASE - T19_BASE));
>>
>> - smbios_build_type_32_table();
>> - smbios_build_type_38_table();
>> - smbios_build_type_41_table(errp);
>> - smbios_build_type_127_table();
>> + smbios_build_type_32_table();
>> + smbios_build_type_38_table();
>> + smbios_build_type_41_table(errp);
>> + smbios_build_type_127_table();
>>
>> - smbios_validate_table(ms->smp.sockets);
>> - smbios_entry_point_setup();
>> - smbios_immutable = true;
>> - }
>> + smbios_validate_table(ms->smp.sockets);
>> + smbios_entry_point_setup();
>>
>> /* return tables blob and entry point (anchor), and their sizes */
>> *tables = smbios_tables;
>> @@ -1254,13 +1281,10 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
>> {
>> const char *val;
>>
>> - assert(!smbios_immutable);
>> -
>> val = qemu_opt_get(opts, "file");
>> if (val) {
>> struct smbios_structure_header *header;
>> - int size;
>> - struct smbios_table *table; /* legacy mode only */
>> + size_t size;
>>
>> if (!qemu_opts_validate(opts, qemu_smbios_file_opts, errp)) {
>> return;
>> @@ -1277,9 +1301,9 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
>> * (except in legacy mode, where the second '\0' is implicit and
>> * will be inserted by the BIOS).
>> */
>> - smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size);
>> - header = (struct smbios_structure_header *)(smbios_tables +
>> - smbios_tables_len);
>> + usr_blobs = g_realloc(usr_blobs, usr_blobs_len + size);
>> + header = (struct smbios_structure_header *)(usr_blobs +
>> + usr_blobs_len);
>>
>> if (load_image_size(val, (uint8_t *)header, size) != size) {
>> error_setg(errp, "Failed to load SMBIOS file %s", val);
>> @@ -1300,34 +1324,15 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
>> smbios_type4_count++;
>> }
>>
>> - smbios_tables_len += size;
>> - if (size > smbios_table_max) {
>> - smbios_table_max = size;
>> + if (!usr_blobs_sizes) {
>> + usr_blobs_sizes = g_array_new(false, false, sizeof(size_t));
>> }
>> - smbios_table_cnt++;
>> -
>> - /* add a copy of the newly loaded blob to legacy smbios_entries */
>> - /* NOTE: This code runs before smbios_set_defaults(), so we don't
>> - * yet know which mode (legacy vs. aggregate-table) will be
>> - * required. We therefore add the binary blob to both legacy
>> - * (smbios_entries) and aggregate (smbios_tables) tables, and
>> - * delete the one we don't need from smbios_set_defaults(),
>> - * once we know which machine version has been requested.
>> - */
>> - if (!smbios_entries) {
>> - smbios_entries_len = sizeof(uint16_t);
>> - smbios_entries = g_malloc0(smbios_entries_len);
>> + g_array_append_val(usr_blobs_sizes, size);
>> + usr_blobs_len += size;
>> + if (size > usr_table_max) {
>> + usr_table_max = size;
>> }
>> - 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);
>> - /* end: add a copy of the newly loaded blob to legacy
>> smbios_entries */
>> + usr_table_cnt++;
>>
>> return;
>> }
>> --
>> 2.39.3
