On Thu, 29 Feb 2024 16:23:21 +0530
Ani Sinha <anisi...@redhat.com> wrote:
> > On 27-Feb-2024, at 21:17, Igor Mammedov <imamm...@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 <imamm...@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
> >
>
