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
