[PATCH v3 0/3] pc: Support configuration of SMBIOS entry point type

[PATCH v3 0/3] pc: Support configuration of SMBIOS entry point type

[Eduardo Habkost]

This includes code previously submitted[1] by Daniel P. Berrangé
to add a "smbios-ep" machine property on PC.

SMBIOS 3.0 is necessary to support more than ~720 VCPUs, as a
large number of VCPUs can easily hit the table size limit of
SMBIOS 2.1 entry points.

Changes from v2:
* Renamed option to "smbios-entry-point-type" for clarity
* Renamed option values to "32" and "64", for two reasons:
  * The option is not about reporting an exact SMBIOS
    version, but just the entry point format.
    FWIW, the SMBIOS specification uses the phrases "32-bit entry
    point" and "64-bit entry point" more often than "2.1 entry
    point" and "3.0 entry point".
  * QAPI doesn't allow us to use enum member names with dots
    or underscores

[1] https://lore.kernel.org/qemu-devel/20200908165438.1008942-5-berrange@redhat.com
    https://lore.kernel.org/qemu-devel/20200908165438.1008942-6-berrange@redhat.com

Eduardo Habkost (3):
  smbios: Rename SMBIOS_ENTRY_POINT_* enums
  hw/smbios: Use qapi for SmbiosEntryPointType
  hw/i386: expose a "smbios-entry-point-type" PC machine property

 include/hw/firmware/smbios.h | 10 ++--------
 include/hw/i386/pc.h         |  4 ++++
 hw/arm/virt.c                |  2 +-
 hw/i386/pc.c                 | 26 ++++++++++++++++++++++++++
 hw/i386/pc_piix.c            |  2 +-
 hw/i386/pc_q35.c             |  2 +-
 hw/smbios/smbios.c           |  8 ++++----
 qapi/machine.json            | 12 ++++++++++++
 8 files changed, 51 insertions(+), 15 deletions(-)

-- 
2.32.0

[PATCH v3 1/3] smbios: Rename SMBIOS_ENTRY_POINT_* enums

[Eduardo Habkost]

Rename the enums to match the naming style used by QAPI, and to
use "32" and "64" instead of "20" and "31".  This will allow us
to more easily move the enum to the QAPI schema later.

About the naming choice: "SMBIOS 2.1 entry point"/"SMBIO 3.0
entry point" and "32-bit entry point"/"64-bit entry point" are
synonymous in the SMBIOS specification.  However, the phrases
"32-bit entry point" and "64-bit entry point" are used more often.

The new names also avoid confusion between the entry point format
and the actual SMBIOS version reported in the entry point
structure.  For example: currently the 32-bit entry point
actually report SMBIOS 2.8 support, not 2.1.

Based on portions of a patch submitted by Daniel P. Berrangé.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
First version of this code was submitted at:
https://lore.kernel.org/qemu-devel/20200908165438.1008942-5-berrange@redhat.com

Changes from v2:
* Use "32" and "64" instead of "2_0" and "3_1"

Changes from v1:
* Patch was split in two
* Hunks included this patch are not changed from v1
---
 include/hw/firmware/smbios.h | 4 ++--
 hw/arm/virt.c                | 2 +-
 hw/i386/pc_piix.c            | 2 +-
 hw/i386/pc_q35.c             | 2 +-
 hw/smbios/smbios.c           | 8 ++++----
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
index 5a0dd0c8cff..d916baed6a9 100644
--- a/include/hw/firmware/smbios.h
+++ b/include/hw/firmware/smbios.h
@@ -27,8 +27,8 @@ struct smbios_phys_mem_area {
  * SMBIOS spec defined tables
  */
 typedef enum SmbiosEntryPointType {
-    SMBIOS_ENTRY_POINT_21,
-    SMBIOS_ENTRY_POINT_30,
+    SMBIOS_ENTRY_POINT_TYPE_32,
+    SMBIOS_ENTRY_POINT_TYPE_64,
 } SmbiosEntryPointType;
 
 /* SMBIOS Entry Point
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ca433adb5b1..2bd73d501da 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1589,7 +1589,7 @@ static void virt_build_smbios(VirtMachineState *vms)
 
     smbios_set_defaults("QEMU", product,
                         vmc->smbios_old_sys_ver ? "1.0" : mc->name, false,
-                        true, SMBIOS_ENTRY_POINT_30);
+                        true, SMBIOS_ENTRY_POINT_TYPE_64);
 
     smbios_get_tables(MACHINE(vms), NULL, 0,
                       &smbios_tables, &smbios_tables_len,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6ad0d763c57..17c050694f5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -177,7 +177,7 @@ static void pc_init1(MachineState *machine,
         smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
                             mc->name, pcmc->smbios_legacy_mode,
                             pcmc->smbios_uuid_encoded,
-                            SMBIOS_ENTRY_POINT_21);
+                            SMBIOS_ENTRY_POINT_TYPE_32);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index fcc6e4eb2b8..48419ebfd5f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -199,7 +199,7 @@ static void pc_q35_init(MachineState *machine)
         smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
                             mc->name, pcmc->smbios_legacy_mode,
                             pcmc->smbios_uuid_encoded,
-                            SMBIOS_ENTRY_POINT_21);
+                            SMBIOS_ENTRY_POINT_TYPE_32);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 7397e567373..6013df1698e 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -62,7 +62,7 @@ uint8_t *smbios_tables;
 size_t smbios_tables_len;
 unsigned smbios_table_max;
 unsigned smbios_table_cnt;
-static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_21;
+static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_32;
 
 static SmbiosEntryPoint ep;
 
@@ -432,7 +432,7 @@ static void smbios_validate_table(MachineState *ms)
         exit(1);
     }
 
-    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21 &&
+    if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_32 &&
         smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
         error_report("SMBIOS 2.1 table length %zu exceeds %d",
                      smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN);
@@ -927,7 +927,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
 static void smbios_entry_point_setup(void)
 {
     switch (smbios_ep_type) {
-    case SMBIOS_ENTRY_POINT_21:
+    case SMBIOS_ENTRY_POINT_TYPE_32:
         memcpy(ep.ep21.anchor_string, "_SM_", 4);
         memcpy(ep.ep21.intermediate_anchor_string, "_DMI_", 5);
         ep.ep21.length = sizeof(struct smbios_21_entry_point);
@@ -950,7 +950,7 @@ static void smbios_entry_point_setup(void)
         ep.ep21.structure_table_address = cpu_to_le32(0);
 
         break;
-    case SMBIOS_ENTRY_POINT_30:
+    case SMBIOS_ENTRY_POINT_TYPE_64:
         memcpy(ep.ep30.anchor_string, "_SM3_", 5);
         ep.ep30.length = sizeof(struct smbios_30_entry_point);
         ep.ep30.entry_point_revision = 1;
-- 
2.32.0

[PATCH v3 2/3] hw/smbios: Use qapi for SmbiosEntryPointType

[Eduardo Habkost]

This prepares for exposing the SMBIOS entry point type as a
machine property on x86.

Based on a patch from Daniel P. Berrangé.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
First version of this code was submitted at:
https://lore.kernel.org/qemu-devel/20200908165438.1008942-5-berrange@redhat.com

Changes from v2:
* Rename "2_0"/"3_1" to "32"/"64", to
  make the names more QAPI-friendly (as underscores and dots are
  not allowed by QAPI)
* Move definition from smbios.json back to machine.json
  (no need for a separate file just for one enum)

Changes from v1:
* Patch was split in two
* Moved definition to smbios.json
---
 include/hw/firmware/smbios.h | 10 ++--------
 qapi/machine.json            | 12 ++++++++++++
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
index d916baed6a9..4b7ad77a44f 100644
--- a/include/hw/firmware/smbios.h
+++ b/include/hw/firmware/smbios.h
@@ -1,6 +1,8 @@
 #ifndef QEMU_SMBIOS_H
 #define QEMU_SMBIOS_H
 
+#include "qapi/qapi-types-machine.h"
+
 /*
  * SMBIOS Support
  *
@@ -23,14 +25,6 @@ struct smbios_phys_mem_area {
     uint64_t length;
 };
 
-/*
- * SMBIOS spec defined tables
- */
-typedef enum SmbiosEntryPointType {
-    SMBIOS_ENTRY_POINT_TYPE_32,
-    SMBIOS_ENTRY_POINT_TYPE_64,
-} SmbiosEntryPointType;
-
 /* SMBIOS Entry Point
  * There are two types of entry points defined in the SMBIOS specification
  * (see below). BIOS must place the entry point(s) at a 16-byte-aligned
diff --git a/qapi/machine.json b/qapi/machine.json
index 5db54df298f..0a13579275f 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1411,3 +1411,15 @@
      '*cores': 'int',
      '*threads': 'int',
      '*maxcpus': 'int' } }
+
+##
+# @SmbiosEntryPointType:
+#
+# @32: SMBIOS version 2.1 (32-bit) Entry Point
+#
+# @64: SMBIOS version 3.0 (64-bit) Entry Point
+#
+# Since: 6.1
+##
+{ 'enum': 'SmbiosEntryPointType',
+  'data': [ '32', '64' ] }
-- 
2.32.0

[PATCH v3 3/3] hw/i386: expose a "smbios-entry-point-type" PC machine property

[Eduardo Habkost]

The i440fx and Q35 machine types are both hardcoded to use the
legacy SMBIOS 2.1 (32-bit) entry point. This is a sensible
conservative choice because SeaBIOS only supports SMBIOS 2.1

EDK2, however, can also support SMBIOS 3.0 (64-bit) entry points,
and QEMU already uses this on the ARM virt machine type.

This adds a property to allow the choice of SMBIOS entry point
versions For example to opt in to 64-bit SMBIOS entry point:

   $QEMU -machine q35,smbios-entry-point-type=64

Based on a patch submitted by Daniel Berrangé.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
This is patch was previously submitted at:
https://lore.kernel.org/qemu-devel/20200908165438.1008942-6-berrange@redhat.com

Changes from v2:
* Rename "smbios-ep" to "smbios-entry-point-type"

Changes from v1:
* Include qapi-visit-smbios.h instead of qapi-visit-machine.h
* Commit message fix: s/smbios_ep/smbios-ep/
---
 include/hw/i386/pc.h |  4 ++++
 hw/i386/pc.c         | 26 ++++++++++++++++++++++++++
 hw/i386/pc_piix.c    |  2 +-
 hw/i386/pc_q35.c     |  2 +-
 4 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 11426e26dc3..95f7f55cdc6 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -13,6 +13,7 @@
 #include "hw/hotplug.h"
 #include "qom/object.h"
 #include "hw/i386/sgx-epc.h"
+#include "hw/firmware/smbios.h"
 
 #define HPET_INTCAP "hpet-intcap"
 
@@ -39,6 +40,7 @@ typedef struct PCMachineState {
     /* Configuration options: */
     uint64_t max_ram_below_4g;
     OnOffAuto vmport;
+    SmbiosEntryPointType smbios_entry_point_type;
 
     bool acpi_build_enabled;
     bool smbus_enabled;
@@ -62,6 +64,8 @@ typedef struct PCMachineState {
 #define PC_MACHINE_SATA             "sata"
 #define PC_MACHINE_PIT              "pit"
 #define PC_MACHINE_MAX_FW_SIZE      "max-fw-size"
+#define PC_MACHINE_SMBIOS_EP        "smbios-entry-point-type"
+
 /**
  * PCMachineClass:
  *
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 86223acfd34..bbeae19fa2f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -77,6 +77,7 @@
 #include "hw/mem/nvdimm.h"
 #include "qapi/error.h"
 #include "qapi/qapi-visit-common.h"
+#include "qapi/qapi-visit-machine.h"
 #include "qapi/visitor.h"
 #include "hw/core/cpu.h"
 #include "hw/usb.h"
@@ -1494,6 +1495,23 @@ static void pc_machine_set_default_bus_bypass_iommu(Object *obj, bool value,
     pcms->default_bus_bypass_iommu = value;
 }
 
+static void pc_machine_get_smbios_ep(Object *obj, Visitor *v, const char *name,
+                                     void *opaque, Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+    SmbiosEntryPointType smbios_entry_point_type = pcms->smbios_entry_point_type;
+
+    visit_type_SmbiosEntryPointType(v, name, &smbios_entry_point_type, errp);
+}
+
+static void pc_machine_set_smbios_ep(Object *obj, Visitor *v, const char *name,
+                                     void *opaque, Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+
+    visit_type_SmbiosEntryPointType(v, name, &pcms->smbios_entry_point_type, errp);
+}
+
 static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
                                             const char *name, void *opaque,
                                             Error **errp)
@@ -1584,6 +1602,8 @@ static void pc_machine_initfn(Object *obj)
     pcms->vmport = ON_OFF_AUTO_OFF;
 #endif /* CONFIG_VMPORT */
     pcms->max_ram_below_4g = 0; /* use default */
+    pcms->smbios_entry_point_type = SMBIOS_ENTRY_POINT_TYPE_32;
+
     /* acpi build is enabled by default if machine supports it */
     pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
     pcms->smbus_enabled = true;
@@ -1727,6 +1747,12 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
         NULL, NULL);
     object_class_property_set_description(oc, PC_MACHINE_MAX_FW_SIZE,
         "Maximum combined firmware size");
+
+    object_class_property_add(oc, PC_MACHINE_SMBIOS_EP, "str",
+        pc_machine_get_smbios_ep, pc_machine_set_smbios_ep,
+        NULL, NULL);
+    object_class_property_set_description(oc, PC_MACHINE_SMBIOS_EP,
+        "SMBIOS Entry Point type [32, 64]");
 }
 
 static const TypeInfo pc_machine_info = {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 17c050694f5..45e3c760915 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -177,7 +177,7 @@ static void pc_init1(MachineState *machine,
         smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
                             mc->name, pcmc->smbios_legacy_mode,
                             pcmc->smbios_uuid_encoded,
-                            SMBIOS_ENTRY_POINT_TYPE_32);
+                            pcms->smbios_entry_point_type);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 48419ebfd5f..a6477db880b 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -199,7 +199,7 @@ static void pc_q35_init(MachineState *machine)
         smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
                             mc->name, pcmc->smbios_legacy_mode,
                             pcmc->smbios_uuid_encoded,
-                            SMBIOS_ENTRY_POINT_TYPE_32);
+                            pcms->smbios_entry_point_type);
     }
 
     /* allocate ram and load rom/bios */
-- 
2.32.0

Re: [PATCH v3 1/3] smbios: Rename SMBIOS_ENTRY_POINT_* enums

[Philippe Mathieu-Daudé]

On 10/26/21 17:10, Eduardo Habkost wrote:
> Rename the enums to match the naming style used by QAPI, and to
> use "32" and "64" instead of "20" and "31".  This will allow us
> to more easily move the enum to the QAPI schema later.
> 
> About the naming choice: "SMBIOS 2.1 entry point"/"SMBIO 3.0
> entry point" and "32-bit entry point"/"64-bit entry point" are
> synonymous in the SMBIOS specification.  However, the phrases
> "32-bit entry point" and "64-bit entry point" are used more often.
> 
> The new names also avoid confusion between the entry point format
> and the actual SMBIOS version reported in the entry point
> structure.  For example: currently the 32-bit entry point
> actually report SMBIOS 2.8 support, not 2.1.
> 
> Based on portions of a patch submitted by Daniel P. Berrangé.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> First version of this code was submitted at:
> https://lore.kernel.org/qemu-devel/20200908165438.1008942-5-berrange@redhat.com
> 
> Changes from v2:
> * Use "32" and "64" instead of "2_0" and "3_1"
> 
> Changes from v1:
> * Patch was split in two
> * Hunks included this patch are not changed from v1
> ---
>  include/hw/firmware/smbios.h | 4 ++--
>  hw/arm/virt.c                | 2 +-
>  hw/i386/pc_piix.c            | 2 +-
>  hw/i386/pc_q35.c             | 2 +-
>  hw/smbios/smbios.c           | 8 ++++----
>  5 files changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 2/3] hw/smbios: Use qapi for SmbiosEntryPointType

[Philippe Mathieu-Daudé]

On 10/26/21 17:10, Eduardo Habkost wrote:
> This prepares for exposing the SMBIOS entry point type as a
> machine property on x86.
> 
> Based on a patch from Daniel P. Berrangé.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> First version of this code was submitted at:
> https://lore.kernel.org/qemu-devel/20200908165438.1008942-5-berrange@redhat.com
> 
> Changes from v2:
> * Rename "2_0"/"3_1" to "32"/"64", to
>   make the names more QAPI-friendly (as underscores and dots are
>   not allowed by QAPI)
> * Move definition from smbios.json back to machine.json
>   (no need for a separate file just for one enum)
> 
> Changes from v1:
> * Patch was split in two
> * Moved definition to smbios.json
> ---
>  include/hw/firmware/smbios.h | 10 ++--------
>  qapi/machine.json            | 12 ++++++++++++
>  2 files changed, 14 insertions(+), 8 deletions(-)

> +##
> +# @SmbiosEntryPointType:
> +#
> +# @32: SMBIOS version 2.1 (32-bit) Entry Point
> +#
> +# @64: SMBIOS version 3.0 (64-bit) Entry Point
> +#
> +# Since: 6.1

6.2, otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +##
> +{ 'enum': 'SmbiosEntryPointType',
> +  'data': [ '32', '64' ] }
> 

Re: [PATCH v3 1/3] smbios: Rename SMBIOS_ENTRY_POINT_* enums

[Michael S. Tsirkin]

On Tue, Oct 26, 2021 at 11:10:58AM -0400, Eduardo Habkost wrote:
> Rename the enums to match the naming style used by QAPI, and to
> use "32" and "64" instead of "20" and "31".  This will allow us
> to more easily move the enum to the QAPI schema later.
> 
> About the naming choice: "SMBIOS 2.1 entry point"/"SMBIO 3.0

typo in commit log

> entry point" and "32-bit entry point"/"64-bit entry point" are
> synonymous in the SMBIOS specification.  However, the phrases
> "32-bit entry point" and "64-bit entry point" are used more often.
> 
> The new names also avoid confusion between the entry point format
> and the actual SMBIOS version reported in the entry point
> structure.  For example: currently the 32-bit entry point
> actually report SMBIOS 2.8 support, not 2.1.
> 
> Based on portions of a patch submitted by Daniel P. Berrangé.

I think you need the original S.O.B here too then.

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> First version of this code was submitted at:
> https://lore.kernel.org/qemu-devel/20200908165438.1008942-5-berrange@redhat.com
> 
> Changes from v2:
> * Use "32" and "64" instead of "2_0" and "3_1"
> 
> Changes from v1:
> * Patch was split in two
> * Hunks included this patch are not changed from v1
> ---
>  include/hw/firmware/smbios.h | 4 ++--
>  hw/arm/virt.c                | 2 +-
>  hw/i386/pc_piix.c            | 2 +-
>  hw/i386/pc_q35.c             | 2 +-
>  hw/smbios/smbios.c           | 8 ++++----
>  5 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
> index 5a0dd0c8cff..d916baed6a9 100644
> --- a/include/hw/firmware/smbios.h
> +++ b/include/hw/firmware/smbios.h
> @@ -27,8 +27,8 @@ struct smbios_phys_mem_area {
>   * SMBIOS spec defined tables
>   */
>  typedef enum SmbiosEntryPointType {
> -    SMBIOS_ENTRY_POINT_21,
> -    SMBIOS_ENTRY_POINT_30,
> +    SMBIOS_ENTRY_POINT_TYPE_32,
> +    SMBIOS_ENTRY_POINT_TYPE_64,
>  } SmbiosEntryPointType;
>  
>  /* SMBIOS Entry Point
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ca433adb5b1..2bd73d501da 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1589,7 +1589,7 @@ static void virt_build_smbios(VirtMachineState *vms)
>  
>      smbios_set_defaults("QEMU", product,
>                          vmc->smbios_old_sys_ver ? "1.0" : mc->name, false,
> -                        true, SMBIOS_ENTRY_POINT_30);
> +                        true, SMBIOS_ENTRY_POINT_TYPE_64);
>  
>      smbios_get_tables(MACHINE(vms), NULL, 0,
>                        &smbios_tables, &smbios_tables_len,
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 6ad0d763c57..17c050694f5 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -177,7 +177,7 @@ static void pc_init1(MachineState *machine,
>          smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
>                              mc->name, pcmc->smbios_legacy_mode,
>                              pcmc->smbios_uuid_encoded,
> -                            SMBIOS_ENTRY_POINT_21);
> +                            SMBIOS_ENTRY_POINT_TYPE_32);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index fcc6e4eb2b8..48419ebfd5f 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -199,7 +199,7 @@ static void pc_q35_init(MachineState *machine)
>          smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
>                              mc->name, pcmc->smbios_legacy_mode,
>                              pcmc->smbios_uuid_encoded,
> -                            SMBIOS_ENTRY_POINT_21);
> +                            SMBIOS_ENTRY_POINT_TYPE_32);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 7397e567373..6013df1698e 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -62,7 +62,7 @@ uint8_t *smbios_tables;
>  size_t smbios_tables_len;
>  unsigned smbios_table_max;
>  unsigned smbios_table_cnt;
> -static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_21;
> +static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_32;
>  
>  static SmbiosEntryPoint ep;
>  
> @@ -432,7 +432,7 @@ static void smbios_validate_table(MachineState *ms)
>          exit(1);
>      }
>  
> -    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21 &&
> +    if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_32 &&
>          smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
>          error_report("SMBIOS 2.1 table length %zu exceeds %d",
>                       smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN);
> @@ -927,7 +927,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
>  static void smbios_entry_point_setup(void)
>  {
>      switch (smbios_ep_type) {
> -    case SMBIOS_ENTRY_POINT_21:
> +    case SMBIOS_ENTRY_POINT_TYPE_32:
>          memcpy(ep.ep21.anchor_string, "_SM_", 4);
>          memcpy(ep.ep21.intermediate_anchor_string, "_DMI_", 5);
>          ep.ep21.length = sizeof(struct smbios_21_entry_point);
> @@ -950,7 +950,7 @@ static void smbios_entry_point_setup(void)
>          ep.ep21.structure_table_address = cpu_to_le32(0);
>  
>          break;
> -    case SMBIOS_ENTRY_POINT_30:
> +    case SMBIOS_ENTRY_POINT_TYPE_64:
>          memcpy(ep.ep30.anchor_string, "_SM3_", 5);
>          ep.ep30.length = sizeof(struct smbios_30_entry_point);
>          ep.ep30.entry_point_revision = 1;
> -- 
> 2.32.0

Re: [PATCH v3 1/3] smbios: Rename SMBIOS_ENTRY_POINT_* enums

[Eduardo Habkost]

On Wed, Oct 27, 2021 at 03:43:43AM -0400, Michael S. Tsirkin wrote:
> On Tue, Oct 26, 2021 at 11:10:58AM -0400, Eduardo Habkost wrote:
> > Rename the enums to match the naming style used by QAPI, and to
> > use "32" and "64" instead of "20" and "31".  This will allow us
> > to more easily move the enum to the QAPI schema later.
> > 
> > About the naming choice: "SMBIOS 2.1 entry point"/"SMBIO 3.0
> 
> typo in commit log

I'll fix it, thanks!

> 
> > entry point" and "32-bit entry point"/"64-bit entry point" are
> > synonymous in the SMBIOS specification.  However, the phrases
> > "32-bit entry point" and "64-bit entry point" are used more often.
> > 
> > The new names also avoid confusion between the entry point format
> > and the actual SMBIOS version reported in the entry point
> > structure.  For example: currently the 32-bit entry point
> > actually report SMBIOS 2.8 support, not 2.1.
> > 
> > Based on portions of a patch submitted by Daniel P. Berrangé.
> 
> I think you need the original S.O.B here too then.

I'm not sure it is appropriate here, as zero lines of code from
the original patch remain here.

> 
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > First version of this code was submitted at:
> > https://lore.kernel.org/qemu-devel/20200908165438.1008942-5-berrange@redhat.com
> > 
> > Changes from v2:
> > * Use "32" and "64" instead of "2_0" and "3_1"
> > 
> > Changes from v1:
> > * Patch was split in two
> > * Hunks included this patch are not changed from v1

-- 
Eduardo

Re: [PATCH v3 0/3] pc: Support configuration of SMBIOS entry point type

[Michael S. Tsirkin]

On Tue, Oct 26, 2021 at 11:10:57AM -0400, Eduardo Habkost wrote:
> This includes code previously submitted[1] by Daniel P. Berrangé
> to add a "smbios-ep" machine property on PC.
> 
> SMBIOS 3.0 is necessary to support more than ~720 VCPUs, as a
> large number of VCPUs can easily hit the table size limit of
> SMBIOS 2.1 entry points.


We need acks from QAPI supporters on this.

> Changes from v2:
> * Renamed option to "smbios-entry-point-type" for clarity
> * Renamed option values to "32" and "64", for two reasons:
>   * The option is not about reporting an exact SMBIOS
>     version, but just the entry point format.
>     FWIW, the SMBIOS specification uses the phrases "32-bit entry
>     point" and "64-bit entry point" more often than "2.1 entry
>     point" and "3.0 entry point".
>   * QAPI doesn't allow us to use enum member names with dots
>     or underscores
> 
> [1] https://lore.kernel.org/qemu-devel/20200908165438.1008942-5-berrange@redhat.com
>     https://lore.kernel.org/qemu-devel/20200908165438.1008942-6-berrange@redhat.com
> 
> Eduardo Habkost (3):
>   smbios: Rename SMBIOS_ENTRY_POINT_* enums
>   hw/smbios: Use qapi for SmbiosEntryPointType
>   hw/i386: expose a "smbios-entry-point-type" PC machine property
> 
>  include/hw/firmware/smbios.h | 10 ++--------
>  include/hw/i386/pc.h         |  4 ++++
>  hw/arm/virt.c                |  2 +-
>  hw/i386/pc.c                 | 26 ++++++++++++++++++++++++++
>  hw/i386/pc_piix.c            |  2 +-
>  hw/i386/pc_q35.c             |  2 +-
>  hw/smbios/smbios.c           |  8 ++++----
>  qapi/machine.json            | 12 ++++++++++++
>  8 files changed, 51 insertions(+), 15 deletions(-)
> 
> -- 
> 2.32.0

Re: [PATCH v3 3/3] hw/i386: expose a "smbios-entry-point-type" PC machine property

[Philippe Mathieu-Daudé]

On 10/26/21 17:11, Eduardo Habkost wrote:
> The i440fx and Q35 machine types are both hardcoded to use the
> legacy SMBIOS 2.1 (32-bit) entry point. This is a sensible
> conservative choice because SeaBIOS only supports SMBIOS 2.1
> 
> EDK2, however, can also support SMBIOS 3.0 (64-bit) entry points,
> and QEMU already uses this on the ARM virt machine type.
> 
> This adds a property to allow the choice of SMBIOS entry point
> versions For example to opt in to 64-bit SMBIOS entry point:
> 
>    $QEMU -machine q35,smbios-entry-point-type=64

It would be nice to have a test for this...

Otherwise,
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> Based on a patch submitted by Daniel Berrangé.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> This is patch was previously submitted at:
> https://lore.kernel.org/qemu-devel/20200908165438.1008942-6-berrange@redhat.com
> 
> Changes from v2:
> * Rename "smbios-ep" to "smbios-entry-point-type"
> 
> Changes from v1:
> * Include qapi-visit-smbios.h instead of qapi-visit-machine.h
> * Commit message fix: s/smbios_ep/smbios-ep/
> ---
>  include/hw/i386/pc.h |  4 ++++
>  hw/i386/pc.c         | 26 ++++++++++++++++++++++++++
>  hw/i386/pc_piix.c    |  2 +-
>  hw/i386/pc_q35.c     |  2 +-
>  4 files changed, 32 insertions(+), 2 deletions(-)

Re: [PATCH v3 3/3] hw/i386: expose a "smbios-entry-point-type" PC machine property

[Michael S. Tsirkin]

On Tue, Nov 02, 2021 at 09:51:35AM +0100, Philippe Mathieu-Daudé wrote:
> On 10/26/21 17:11, Eduardo Habkost wrote:
> > The i440fx and Q35 machine types are both hardcoded to use the
> > legacy SMBIOS 2.1 (32-bit) entry point. This is a sensible
> > conservative choice because SeaBIOS only supports SMBIOS 2.1
> > 
> > EDK2, however, can also support SMBIOS 3.0 (64-bit) entry points,
> > and QEMU already uses this on the ARM virt machine type.
> > 
> > This adds a property to allow the choice of SMBIOS entry point
> > versions For example to opt in to 64-bit SMBIOS entry point:
> > 
> >    $QEMU -machine q35,smbios-entry-point-type=64
> 
> It would be nice to have a test for this...
> 
> Otherwise,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Can we update seabios and the switch the default?
Maybe just for q35?
Or are there more considerations?


> > Based on a patch submitted by Daniel Berrangé.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > This is patch was previously submitted at:
> > https://lore.kernel.org/qemu-devel/20200908165438.1008942-6-berrange@redhat.com
> > 
> > Changes from v2:
> > * Rename "smbios-ep" to "smbios-entry-point-type"
> > 
> > Changes from v1:
> > * Include qapi-visit-smbios.h instead of qapi-visit-machine.h
> > * Commit message fix: s/smbios_ep/smbios-ep/
> > ---
> >  include/hw/i386/pc.h |  4 ++++
> >  hw/i386/pc.c         | 26 ++++++++++++++++++++++++++
> >  hw/i386/pc_piix.c    |  2 +-
> >  hw/i386/pc_q35.c     |  2 +-
> >  4 files changed, 32 insertions(+), 2 deletions(-)

Re: [PATCH v3 2/3] hw/smbios: Use qapi for SmbiosEntryPointType

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> This prepares for exposing the SMBIOS entry point type as a
> machine property on x86.
>
> Based on a patch from Daniel P. Berrangé.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---

[...]

> diff --git a/qapi/machine.json b/qapi/machine.json
> index 5db54df298f..0a13579275f 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1411,3 +1411,15 @@
>       '*cores': 'int',
>       '*threads': 'int',
>       '*maxcpus': 'int' } }
> +
> +##
> +# @SmbiosEntryPointType:
> +#
> +# @32: SMBIOS version 2.1 (32-bit) Entry Point
> +#
> +# @64: SMBIOS version 3.0 (64-bit) Entry Point
> +#
> +# Since: 6.1
> +##
> +{ 'enum': 'SmbiosEntryPointType',
> +  'data': [ '32', '64' ] }

QAPI schema
Acked-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v3 3/3] hw/i386: expose a "smbios-entry-point-type" PC machine property

[Eduardo Habkost]

On Tue, Nov 02, 2021 at 07:25:25AM -0400, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2021 at 09:51:35AM +0100, Philippe Mathieu-Daudé wrote:
> > On 10/26/21 17:11, Eduardo Habkost wrote:
> > > The i440fx and Q35 machine types are both hardcoded to use the
> > > legacy SMBIOS 2.1 (32-bit) entry point. This is a sensible
> > > conservative choice because SeaBIOS only supports SMBIOS 2.1
> > > 
> > > EDK2, however, can also support SMBIOS 3.0 (64-bit) entry points,
> > > and QEMU already uses this on the ARM virt machine type.
> > > 
> > > This adds a property to allow the choice of SMBIOS entry point
> > > versions For example to opt in to 64-bit SMBIOS entry point:
> > > 
> > >    $QEMU -machine q35,smbios-entry-point-type=64
> > 
> > It would be nice to have a test for this...
> > 
> > Otherwise,
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Can we update seabios and the switch the default?
> Maybe just for q35?
> Or are there more considerations?

We can switch the default, but SeaBIOS maintainers won't include
the SMBIOS 3.0 code I had submitted[1] until this is supported by
QEMU.

After we patch SeaBIOS to support SMBIOS 3.0 and update the
SeaBIOS binaries in the QEMU tree, we can switch the default in
Q35 and/or i440fx to SMBIOS 3.0.

[1] https://www.mail-archive.com/seabios@seabios.org/msg12415.html
    https://www.mail-archive.com/seabios@seabios.org/msg12438.html

> 
> 
> > > Based on a patch submitted by Daniel Berrangé.
> > > 
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > This is patch was previously submitted at:
> > > https://lore.kernel.org/qemu-devel/20200908165438.1008942-6-berrange@redhat.com
> > > 
> > > Changes from v2:
> > > * Rename "smbios-ep" to "smbios-entry-point-type"
> > > 
> > > Changes from v1:
> > > * Include qapi-visit-smbios.h instead of qapi-visit-machine.h
> > > * Commit message fix: s/smbios_ep/smbios-ep/
> > > ---
> > >  include/hw/i386/pc.h |  4 ++++
> > >  hw/i386/pc.c         | 26 ++++++++++++++++++++++++++
> > >  hw/i386/pc_piix.c    |  2 +-
> > >  hw/i386/pc_q35.c     |  2 +-
> > >  4 files changed, 32 insertions(+), 2 deletions(-)
> 

-- 
Eduardo

Re: [PATCH v3 3/3] hw/i386: expose a "smbios-entry-point-type" PC machine property

[Gerd Hoffmann]

On Tue, Nov 02, 2021 at 07:25:25AM -0400, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2021 at 09:51:35AM +0100, Philippe Mathieu-Daudé wrote:
> > On 10/26/21 17:11, Eduardo Habkost wrote:
> > > The i440fx and Q35 machine types are both hardcoded to use the
> > > legacy SMBIOS 2.1 (32-bit) entry point. This is a sensible
> > > conservative choice because SeaBIOS only supports SMBIOS 2.1
> > > 
> > > EDK2, however, can also support SMBIOS 3.0 (64-bit) entry points,
> > > and QEMU already uses this on the ARM virt machine type.
> > > 
> > > This adds a property to allow the choice of SMBIOS entry point
> > > versions For example to opt in to 64-bit SMBIOS entry point:
> > > 
> > >    $QEMU -machine q35,smbios-entry-point-type=64
> > 
> > It would be nice to have a test for this...
> > 
> > Otherwise,
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Can we update seabios and the switch the default?

seabios support is unfortunately not yet merged upstream.

> Maybe just for q35?
> Or are there more considerations?

It's a guest-visible change, so IMHO we need the runtime switch anyway
so old machine types can continue to use the smbios 2.x entry point.

We'll switch the default at some point for sure, but I don't think
that'll happen for the 6.2 machine type.  We are simply too late.

take care,
  Gerd