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

[Ani Sinha]

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

>     if (!smbios_immutable) {
>         smbios_build_type_0_table();
>         smbios_build_type_1_table();
> -- 
> 2.39.3
>