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

[Igor Mammedov]

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>
> > ---
> > 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.

[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
> >   
>