Re: [PATCH 06/19] smbios: get rid of smbios_legacy global

[Ani Sinha]

> On 29-Feb-2024, at 19:59, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> On Thu, 29 Feb 2024 16:23:21 +0530
> Ani Sinha <anisinha@redhat.com> wrote:
> 
>>> On 27-Feb-2024, at 21:17, Igor Mammedov <imammedo@redhat.com> wrote:
>>> 
>>> clean up smbios_set_defaults() which is reused by legacy
>>> and non legacy machines from being aware of 'legacy' notion
>>> and need to turn it off. And push legacy handling up to
>>> PC machine code where it's relevant.
>>> 
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Ani Sinha <anisinha@redhat.com>

>>> ---
>>> PS: I've moved/kept legacy smbios_entries to smbios_get_tables()
>>> but it at least is not visible to API users. To get rid of it
>>> as well, it would be necessary to change how '-smbios' CLI
>>> option is processed. Which is done later in the series.
>>> ---
>>> include/hw/firmware/smbios.h |  2 +-
>>> hw/arm/virt.c                |  2 +-
>>> hw/i386/fw_cfg.c             |  7 ++++---
>>> hw/loongarch/virt.c          |  2 +-
>>> hw/riscv/virt.c              |  2 +-
>>> hw/smbios/smbios.c           | 35 +++++++++++++++--------------------
>>> 6 files changed, 23 insertions(+), 27 deletions(-)
>>> 
>>> diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
>>> index a187fbbd3d..0818184834 100644
>>> --- a/include/hw/firmware/smbios.h
>>> +++ b/include/hw/firmware/smbios.h
>>> @@ -293,7 +293,7 @@ struct smbios_type_127 {
>>> 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,
>>> -                         const char *version, bool legacy_mode,
>>> +                         const char *version,
>>>                         bool uuid_encoded, SmbiosEntryPointType ep_type);
>>> void smbios_set_default_processor_family(uint16_t processor_family);
>>> uint8_t *smbios_get_table_legacy(uint32_t expected_t4_count, size_t 
>>> *length);
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 0af1943697..8588681f27 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -1633,7 +1633,7 @@ static void virt_build_smbios(VirtMachineState *vms)
>>>    }
>>> 
>>>    smbios_set_defaults("QEMU", product,
>>> -                        vmc->smbios_old_sys_ver ? "1.0" : mc->name, false,
>>> +                        vmc->smbios_old_sys_ver ? "1.0" : mc->name,
>>>                        true, SMBIOS_ENTRY_POINT_TYPE_64);
>>> 
>>>    /* build the array of physical mem area from base_memmap */
>>> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
>>> index fcb4fb0769..c1e9c0fd9c 100644
>>> --- a/hw/i386/fw_cfg.c
>>> +++ b/hw/i386/fw_cfg.c
>>> @@ -63,15 +63,16 @@ void fw_cfg_build_smbios(PCMachineState *pcms, 
>>> FWCfgState *fw_cfg)
>>>    if (pcmc->smbios_defaults) {
>>>        /* These values are guest ABI, do not change */
>>>        smbios_set_defaults("QEMU", mc->desc, mc->name,
>>> -                            pcmc->smbios_legacy_mode, 
>>> pcmc->smbios_uuid_encoded,
>>> +                            pcmc->smbios_uuid_encoded,
>>>                            pcms->smbios_entry_point_type);
>>>    }
>>> 
>>>    /* tell smbios about cpuid version and features */
>>>    smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
>>> 
>>> -    smbios_tables = smbios_get_table_legacy(ms->smp.cpus, 
>>> &smbios_tables_len);
>>> -    if (smbios_tables) {
>>> +    if (pcmc->smbios_legacy_mode) {
>>> +        smbios_tables = smbios_get_table_legacy(ms->smp.cpus,
>>> +                                                &smbios_tables_len);
>>>        fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
>>>                         smbios_tables, smbios_tables_len);
>>>        return;
>>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>>> index 0ad7d8c887..73fb3522ba 100644
>>> --- a/hw/loongarch/virt.c
>>> +++ b/hw/loongarch/virt.c
>>> @@ -320,7 +320,7 @@ static void virt_build_smbios(LoongArchMachineState 
>>> *lams)
>>>        return;
>>>    }
>>> 
>>> -    smbios_set_defaults("QEMU", product, mc->name, false,
>>> +    smbios_set_defaults("QEMU", product, mc->name,
>>>                        true, SMBIOS_ENTRY_POINT_TYPE_64);
>>> 
>>>    smbios_get_tables(ms, NULL, 0, &smbios_tables, &smbios_tables_len,
>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>> index fd35c74781..e2c9529df2 100644
>>> --- a/hw/riscv/virt.c
>>> +++ b/hw/riscv/virt.c
>>> @@ -1235,7 +1235,7 @@ static void virt_build_smbios(RISCVVirtState *s)
>>>        product = "KVM Virtual Machine";
>>>    }
>>> 
>>> -    smbios_set_defaults("QEMU", product, mc->name, false,
>>> +    smbios_set_defaults("QEMU", product, mc->name,
>>>                        true, SMBIOS_ENTRY_POINT_TYPE_64);
>>> 
>>>    if (riscv_is_32bit(&s->soc[0])) {
>>> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
>>> index 15339d8dbe..c46fc93357 100644
>>> --- a/hw/smbios/smbios.c
>>> +++ b/hw/smbios/smbios.c
>>> @@ -54,7 +54,6 @@ struct smbios_table {
>>> 
>>> static uint8_t *smbios_entries;
>>> static size_t smbios_entries_len;
>>> -static bool smbios_legacy = true;
>>> static bool smbios_uuid_encoded = true;
>>> /* end: legacy structures & constants for <= 2.0 machines */
>>> 
>>> @@ -570,9 +569,16 @@ static void smbios_build_type_1_fields(void)
>>> 
>>> uint8_t *smbios_get_table_legacy(uint32_t expected_t4_count, size_t *length)
>>> {
>>> -    if (!smbios_legacy) {
>>> -        *length = 0;
>>> -        return NULL;
>>> +    /* drop unwanted version of command-line file blob(s) */
>>> +    g_free(smbios_tables);
>>> +    smbios_tables = NULL;
>>> +
>>> +    /* 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 (!smbios_immutable) {
>>> @@ -1006,28 +1012,13 @@ void smbios_set_default_processor_family(uint16_t 
>>> processor_family)
>>> }
>>> 
>>> void smbios_set_defaults(const char *manufacturer, const char *product,
>>> -                         const char *version, bool legacy_mode,
>>> +                         const char *version,
>>>                         bool uuid_encoded, SmbiosEntryPointType ep_type)
>>> {
>>>    smbios_have_defaults = true;
>>> -    smbios_legacy = legacy_mode;
>>>    smbios_uuid_encoded = uuid_encoded;
>>>    smbios_ep_type = ep_type;
>>> 
>>> -    /* drop unwanted version of command-line file blob(s) */
>>> -    if (smbios_legacy) {
>>> -        g_free(smbios_tables);
>>> -        /* in legacy mode, 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);
>>> -        }
>>> -    } else {
>>> -        g_free(smbios_entries);
>>> -    }
>>> -
>>>    SMBIOS_SET_DEFAULT(type1.manufacturer, manufacturer);
>>>    SMBIOS_SET_DEFAULT(type1.product, product);
>>>    SMBIOS_SET_DEFAULT(type1.version, version);
>>> @@ -1103,6 +1094,10 @@ 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;
>>> +  
>> 
>> Can you please explain why you do this unconditionally without checking for 
>> legacy mode? Seems wrong?
> 
> with this patch legacy tables build is moved to fw_cfg_build_smbios(),
> however at this point QEMU still has option processing that fills
> both new and legacy smbios_entries blobs. 
> 
> this hunk cleanups not needed blob smbios_entries when building
> modern tables, and smbios_get_table_legacy() has a corresponding
> smbios_tables cleanup since modern is not needed there.

Yes the naming is confusing! There is smbios_entries and then there is 
smbios_tables. The former is only for legacy as the comment above 
smbios_add_field() says. 

> 
> [7/19] removes this in favor of a single blob.
> 
>> 
>>>    if (!smbios_immutable) {
>>>        smbios_build_type_0_table();
>>>        smbios_build_type_1_table();
>>> -- 
>>> 2.39.3