Re: [PATCH 09/19] smbios: build legacy mode code only for 'pc' machine
On Mon, 4 Mar 2024 15:23:00 +0100 Igor Mammedov wrote: > On Mon, 4 Mar 2024 16:25:03 +0530 > Ani Sinha wrote: > > > > On 27-Feb-2024, at 21:17, Igor Mammedov wrote: > > > > > > basically moving code around without functional change. > > > And exposing some symbols so that they could be shared > > > between smbbios.c and new smbios_legacy.c > > > > > > plus some meson magic to build smbios_legacy.c only > > > for 'pc' machine and otherwise replace it with stub > > > if not selected. > > > > > > Signed-off-by: Igor Mammedov > > > --- [...] > > > +/* > > > + * preserve blob size for legacy mode so it could build its > > > + * blobs flavor from 'usr_blobs' > > > + */ > > > +smbios_add_usr_blob_size(size); > > > + > > > > Could this have been made as a part of a separate patch? It is extremely > > hard to understand why you are doing this when it’s a part of a larger code > > refactoring/modularisation. > > sure, will do on respin I also has split out variables renaming into a separate patch. Hope it will make it easier to review. > [...]
Re: [PATCH 09/19] smbios: build legacy mode code only for 'pc' machine
On Mon, 4 Mar 2024 16:25:03 +0530 Ani Sinha wrote: > > On 27-Feb-2024, at 21:17, Igor Mammedov wrote: > > > > basically moving code around without functional change. > > And exposing some symbols so that they could be shared > > between smbbios.c and new smbios_legacy.c > > > > plus some meson magic to build smbios_legacy.c only > > for 'pc' machine and otherwise replace it with stub > > if not selected. > > > > Signed-off-by: Igor Mammedov > > --- > > include/hw/firmware/smbios.h | 22 +++ > > hw/i386/Kconfig| 1 + > > hw/smbios/Kconfig | 2 + > > hw/smbios/meson.build | 4 + > > hw/smbios/smbios.c | 251 ++--- > > hw/smbios/smbios_legacy.c | 179 +++ > > hw/smbios/smbios_legacy_stub.c | 16 +++ > > 7 files changed, 270 insertions(+), 205 deletions(-) > > create mode 100644 hw/smbios/smbios_legacy.c > > create mode 100644 hw/smbios/smbios_legacy_stub.c > > > > diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h > > index 1fbff3c55f..a6d8fd6591 100644 > > --- a/include/hw/firmware/smbios.h > > +++ b/include/hw/firmware/smbios.h > > @@ -2,6 +2,7 @@ > > #define QEMU_SMBIOS_H > > > > #include "qapi/qapi-types-machine.h" > > +#include "qemu/bitmap.h" > > > > /* > > * SMBIOS Support > > @@ -16,8 +17,27 @@ > > * > > */ > > > > +extern uint8_t *usr_blobs; > > +extern GArray *usr_blobs_sizes; > > + > > +typedef struct { > > +const char *vendor, *version, *date; > > +bool have_major_minor, uefi; > > +uint8_t major, minor; > > +} smbios_type0_t; > > +extern smbios_type0_t smbios_type0; > > + > > +typedef struct { > > +const char *manufacturer, *product, *version, *serial, *sku, *family; > > +/* uuid is in qemu_uuid */ > > +} smbios_type1_t; > > +extern smbios_type1_t smbios_type1; > > + > > > > #define SMBIOS_MAX_TYPE 127 > > +extern DECLARE_BITMAP(smbios_have_binfile_bitmap, SMBIOS_MAX_TYPE + 1); > > +extern DECLARE_BITMAP(smbios_have_fields_bitmap, SMBIOS_MAX_TYPE + 1); > > + > > #define offsetofend(TYPE, MEMBER) \ > >(offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER)) > > > > @@ -290,6 +310,8 @@ struct smbios_type_127 { > > struct smbios_structure_header header; > > } QEMU_PACKED; > > > > +void smbios_validate_table(void); > > +void smbios_add_usr_blob_size(size_t size); > > 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, > > diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig > > index a1846be6f7..a6ee052f9a 100644 > > --- a/hw/i386/Kconfig > > +++ b/hw/i386/Kconfig > > @@ -76,6 +76,7 @@ config I440FX > > select PIIX > > select DIMM > > select SMBIOS > > +select SMBIOS_LEGACY > > select FW_CFG_DMA > > > > config ISAPC > > diff --git a/hw/smbios/Kconfig b/hw/smbios/Kconfig > > index 553adf4bfc..8d989a2f1b 100644 > > --- a/hw/smbios/Kconfig > > +++ b/hw/smbios/Kconfig > > @@ -1,2 +1,4 @@ > > config SMBIOS > > bool > > +config SMBIOS_LEGACY > > +bool > > diff --git a/hw/smbios/meson.build b/hw/smbios/meson.build > > index 7046967462..a59039f669 100644 > > --- a/hw/smbios/meson.build > > +++ b/hw/smbios/meson.build > > @@ -4,5 +4,9 @@ smbios_ss.add(when: 'CONFIG_IPMI', > > if_true: files('smbios_type_38.c'), > > if_false: files('smbios_type_38-stub.c')) > > > > +smbios_ss.add(when: 'CONFIG_SMBIOS_LEGACY', > > + if_true: files('smbios_legacy.c'), > > + if_false: files('smbios_legacy_stub.c')) > > + > > system_ss.add_all(when: 'CONFIG_SMBIOS', if_true: smbios_ss) > > system_ss.add(when: 'CONFIG_SMBIOS', if_false: files('smbios-stub.c')) > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c > > index 97cf762228..fb1f05fcde 100644 > > --- a/hw/smbios/smbios.c > > +++ b/hw/smbios/smbios.c > > @@ -31,38 +31,12 @@ > > #include "hw/pci/pci_device.h" > > #include "smbios_build.h" > > > > -/* legacy structures and constants for <= 2.0 machines */ > > -struct smbios_header { > > -uint16_t length; > > -uint8_t type; > > -} QEMU_PACKED; > > - > > -struct smbios_field { > > -struct smbios_header header; > > -uint8_t type; > > -uint16_t offset; > > -uint8_t data[]; > > -} QEMU_PACKED; > > - > > -struct smbios_table { > > -struct smbios_header header; > > -uint8_t data[]; > > -} QEMU_PACKED; > > - > > -#define SMBIOS_FIELD_ENTRY 0 > > -#define SMBIOS_TABLE_ENTRY 1 > > - > > -static uint8_t *smbios_entries; > > -static size_t smbios_entries_len; > > static bool smbios_uuid_encoded = true; > > -/* end: legacy structures & constants for <= 2.0 machines */ > > - > > /* > > * SMBIOS tables provided by user with '-smbios file=' option > > */ > > uint8_t *usr_blobs; > > size_t usr_blobs_len; > > -static GArray *usr_blobs_sizes; > > static unsigned usr_table_max; > > static unsigned usr_table_cnt; > > > >
Re: [PATCH 09/19] smbios: build legacy mode code only for 'pc' machine
> On 27-Feb-2024, at 21:17, Igor Mammedov wrote: > > basically moving code around without functional change. > And exposing some symbols so that they could be shared > between smbbios.c and new smbios_legacy.c > > plus some meson magic to build smbios_legacy.c only > for 'pc' machine and otherwise replace it with stub > if not selected. > > Signed-off-by: Igor Mammedov > --- > include/hw/firmware/smbios.h | 22 +++ > hw/i386/Kconfig| 1 + > hw/smbios/Kconfig | 2 + > hw/smbios/meson.build | 4 + > hw/smbios/smbios.c | 251 ++--- > hw/smbios/smbios_legacy.c | 179 +++ > hw/smbios/smbios_legacy_stub.c | 16 +++ > 7 files changed, 270 insertions(+), 205 deletions(-) > create mode 100644 hw/smbios/smbios_legacy.c > create mode 100644 hw/smbios/smbios_legacy_stub.c > > diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h > index 1fbff3c55f..a6d8fd6591 100644 > --- a/include/hw/firmware/smbios.h > +++ b/include/hw/firmware/smbios.h > @@ -2,6 +2,7 @@ > #define QEMU_SMBIOS_H > > #include "qapi/qapi-types-machine.h" > +#include "qemu/bitmap.h" > > /* > * SMBIOS Support > @@ -16,8 +17,27 @@ > * > */ > > +extern uint8_t *usr_blobs; > +extern GArray *usr_blobs_sizes; > + > +typedef struct { > +const char *vendor, *version, *date; > +bool have_major_minor, uefi; > +uint8_t major, minor; > +} smbios_type0_t; > +extern smbios_type0_t smbios_type0; > + > +typedef struct { > +const char *manufacturer, *product, *version, *serial, *sku, *family; > +/* uuid is in qemu_uuid */ > +} smbios_type1_t; > +extern smbios_type1_t smbios_type1; > + > > #define SMBIOS_MAX_TYPE 127 > +extern DECLARE_BITMAP(smbios_have_binfile_bitmap, SMBIOS_MAX_TYPE + 1); > +extern DECLARE_BITMAP(smbios_have_fields_bitmap, SMBIOS_MAX_TYPE + 1); > + > #define offsetofend(TYPE, MEMBER) \ >(offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER)) > > @@ -290,6 +310,8 @@ struct smbios_type_127 { > struct smbios_structure_header header; > } QEMU_PACKED; > > +void smbios_validate_table(void); > +void smbios_add_usr_blob_size(size_t size); > 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, > diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig > index a1846be6f7..a6ee052f9a 100644 > --- a/hw/i386/Kconfig > +++ b/hw/i386/Kconfig > @@ -76,6 +76,7 @@ config I440FX > select PIIX > select DIMM > select SMBIOS > +select SMBIOS_LEGACY > select FW_CFG_DMA > > config ISAPC > diff --git a/hw/smbios/Kconfig b/hw/smbios/Kconfig > index 553adf4bfc..8d989a2f1b 100644 > --- a/hw/smbios/Kconfig > +++ b/hw/smbios/Kconfig > @@ -1,2 +1,4 @@ > config SMBIOS > bool > +config SMBIOS_LEGACY > +bool > diff --git a/hw/smbios/meson.build b/hw/smbios/meson.build > index 7046967462..a59039f669 100644 > --- a/hw/smbios/meson.build > +++ b/hw/smbios/meson.build > @@ -4,5 +4,9 @@ smbios_ss.add(when: 'CONFIG_IPMI', > if_true: files('smbios_type_38.c'), > if_false: files('smbios_type_38-stub.c')) > > +smbios_ss.add(when: 'CONFIG_SMBIOS_LEGACY', > + if_true: files('smbios_legacy.c'), > + if_false: files('smbios_legacy_stub.c')) > + > system_ss.add_all(when: 'CONFIG_SMBIOS', if_true: smbios_ss) > system_ss.add(when: 'CONFIG_SMBIOS', if_false: files('smbios-stub.c')) > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c > index 97cf762228..fb1f05fcde 100644 > --- a/hw/smbios/smbios.c > +++ b/hw/smbios/smbios.c > @@ -31,38 +31,12 @@ > #include "hw/pci/pci_device.h" > #include "smbios_build.h" > > -/* legacy structures and constants for <= 2.0 machines */ > -struct smbios_header { > -uint16_t length; > -uint8_t type; > -} QEMU_PACKED; > - > -struct smbios_field { > -struct smbios_header header; > -uint8_t type; > -uint16_t offset; > -uint8_t data[]; > -} QEMU_PACKED; > - > -struct smbios_table { > -struct smbios_header header; > -uint8_t data[]; > -} QEMU_PACKED; > - > -#define SMBIOS_FIELD_ENTRY 0 > -#define SMBIOS_TABLE_ENTRY 1 > - > -static uint8_t *smbios_entries; > -static size_t smbios_entries_len; > static bool smbios_uuid_encoded = true; > -/* end: legacy structures & constants for <= 2.0 machines */ > - > /* > * SMBIOS tables provided by user with '-smbios file=' option > */ > uint8_t *usr_blobs; > size_t usr_blobs_len; > -static GArray *usr_blobs_sizes; > static unsigned usr_table_max; > static unsigned usr_table_cnt; > > @@ -78,19 +52,11 @@ static int smbios_type4_count = 0; > static bool smbios_have_defaults; > static uint32_t smbios_cpuid_version, smbios_cpuid_features; > > -static DECLARE_BITMAP(have_binfile_bitmap, SMBIOS_MAX_TYPE+1); > -static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1); > - > -static struct { > -const char *vendor, *version,
[PATCH 09/19] smbios: build legacy mode code only for 'pc' machine
basically moving code around without functional change. And exposing some symbols so that they could be shared between smbbios.c and new smbios_legacy.c plus some meson magic to build smbios_legacy.c only for 'pc' machine and otherwise replace it with stub if not selected. Signed-off-by: Igor Mammedov --- include/hw/firmware/smbios.h | 22 +++ hw/i386/Kconfig| 1 + hw/smbios/Kconfig | 2 + hw/smbios/meson.build | 4 + hw/smbios/smbios.c | 251 ++--- hw/smbios/smbios_legacy.c | 179 +++ hw/smbios/smbios_legacy_stub.c | 16 +++ 7 files changed, 270 insertions(+), 205 deletions(-) create mode 100644 hw/smbios/smbios_legacy.c create mode 100644 hw/smbios/smbios_legacy_stub.c diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h index 1fbff3c55f..a6d8fd6591 100644 --- a/include/hw/firmware/smbios.h +++ b/include/hw/firmware/smbios.h @@ -2,6 +2,7 @@ #define QEMU_SMBIOS_H #include "qapi/qapi-types-machine.h" +#include "qemu/bitmap.h" /* * SMBIOS Support @@ -16,8 +17,27 @@ * */ +extern uint8_t *usr_blobs; +extern GArray *usr_blobs_sizes; + +typedef struct { +const char *vendor, *version, *date; +bool have_major_minor, uefi; +uint8_t major, minor; +} smbios_type0_t; +extern smbios_type0_t smbios_type0; + +typedef struct { +const char *manufacturer, *product, *version, *serial, *sku, *family; +/* uuid is in qemu_uuid */ +} smbios_type1_t; +extern smbios_type1_t smbios_type1; + #define SMBIOS_MAX_TYPE 127 +extern DECLARE_BITMAP(smbios_have_binfile_bitmap, SMBIOS_MAX_TYPE + 1); +extern DECLARE_BITMAP(smbios_have_fields_bitmap, SMBIOS_MAX_TYPE + 1); + #define offsetofend(TYPE, MEMBER) \ (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER)) @@ -290,6 +310,8 @@ struct smbios_type_127 { struct smbios_structure_header header; } QEMU_PACKED; +void smbios_validate_table(void); +void smbios_add_usr_blob_size(size_t size); 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, diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig index a1846be6f7..a6ee052f9a 100644 --- a/hw/i386/Kconfig +++ b/hw/i386/Kconfig @@ -76,6 +76,7 @@ config I440FX select PIIX select DIMM select SMBIOS +select SMBIOS_LEGACY select FW_CFG_DMA config ISAPC diff --git a/hw/smbios/Kconfig b/hw/smbios/Kconfig index 553adf4bfc..8d989a2f1b 100644 --- a/hw/smbios/Kconfig +++ b/hw/smbios/Kconfig @@ -1,2 +1,4 @@ config SMBIOS bool +config SMBIOS_LEGACY +bool diff --git a/hw/smbios/meson.build b/hw/smbios/meson.build index 7046967462..a59039f669 100644 --- a/hw/smbios/meson.build +++ b/hw/smbios/meson.build @@ -4,5 +4,9 @@ smbios_ss.add(when: 'CONFIG_IPMI', if_true: files('smbios_type_38.c'), if_false: files('smbios_type_38-stub.c')) +smbios_ss.add(when: 'CONFIG_SMBIOS_LEGACY', + if_true: files('smbios_legacy.c'), + if_false: files('smbios_legacy_stub.c')) + system_ss.add_all(when: 'CONFIG_SMBIOS', if_true: smbios_ss) system_ss.add(when: 'CONFIG_SMBIOS', if_false: files('smbios-stub.c')) diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c index 97cf762228..fb1f05fcde 100644 --- a/hw/smbios/smbios.c +++ b/hw/smbios/smbios.c @@ -31,38 +31,12 @@ #include "hw/pci/pci_device.h" #include "smbios_build.h" -/* legacy structures and constants for <= 2.0 machines */ -struct smbios_header { -uint16_t length; -uint8_t type; -} QEMU_PACKED; - -struct smbios_field { -struct smbios_header header; -uint8_t type; -uint16_t offset; -uint8_t data[]; -} QEMU_PACKED; - -struct smbios_table { -struct smbios_header header; -uint8_t data[]; -} QEMU_PACKED; - -#define SMBIOS_FIELD_ENTRY 0 -#define SMBIOS_TABLE_ENTRY 1 - -static uint8_t *smbios_entries; -static size_t smbios_entries_len; static bool smbios_uuid_encoded = true; -/* end: legacy structures & constants for <= 2.0 machines */ - /* * SMBIOS tables provided by user with '-smbios file=' option */ uint8_t *usr_blobs; size_t usr_blobs_len; -static GArray *usr_blobs_sizes; static unsigned usr_table_max; static unsigned usr_table_cnt; @@ -78,19 +52,11 @@ static int smbios_type4_count = 0; static bool smbios_have_defaults; static uint32_t smbios_cpuid_version, smbios_cpuid_features; -static DECLARE_BITMAP(have_binfile_bitmap, SMBIOS_MAX_TYPE+1); -static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1); - -static struct { -const char *vendor, *version, *date; -bool have_major_minor, uefi; -uint8_t major, minor; -} type0; +DECLARE_BITMAP(smbios_have_binfile_bitmap, SMBIOS_MAX_TYPE + 1); +DECLARE_BITMAP(smbios_have_fields_bitmap, SMBIOS_MAX_TYPE + 1); -static struct { -const char *manufacturer, *product, *version, *serial, *sku, *family; -/* uuid