diff mbox series

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

Message ID 20211026151100.1691925-4-ehabkost@redhat.com
State New
Headers show
Series pc: Support configuration of SMBIOS entry point type | expand

Commit Message

Eduardo Habkost Oct. 26, 2021, 3:11 p.m. UTC 
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(-)

Comments

Philippe Mathieu-Daudé Nov. 2, 2021, 8:51 a.m. UTC | #1 
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(-)
Michael S. Tsirkin Nov. 2, 2021, 11:25 a.m. UTC | #2 
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(-)
Eduardo Habkost Nov. 10, 2021, 10:06 p.m. UTC | #3 
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(-)
>
Gerd Hoffmann Nov. 22, 2021, 9:08 a.m. UTC | #4 
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
diff mbox series

Patch

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 */