| Message ID | 20240227154749.1818189-7-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Workaround Windows failing to find 64bit SMBIOS entry point with SeaBIOS | expand |
> 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 >
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 > > >
> 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
On 2/27/24 12:47, Igor Mammedov 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> > --- For hw/riscv/virt.c changes: Acked-by: Daniel Henrique Barboza <dbarboza@ventanamicro.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; > + > if (!smbios_immutable) { > smbios_build_type_0_table(); > smbios_build_type_1_table();
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; + if (!smbios_immutable) { smbios_build_type_0_table(); smbios_build_type_1_table();
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(-)