Re: [PATCH v6 2/2] hw/i386: Add the ramfb romfile compatibility

[Shaoqin Huang]

Hi Eric,

Thanks for your review.
On 7/1/25 1:17 PM, Eric Auger wrote:
Hi,

On 7/1/25 5:05 AM, Shaoqin Huang wrote:
Set the "use-legacy-x86-rom" property to false by default, and only set
it to true on x86 since only x86 will need it.

At the same time, set the "use-legacy-x86-rom" property to true on those
historical versioned machine types in order to avoid the memory layout
being changed.

Signed-off-by: Shaoqin Huang <shahu...@redhat.com>
---
  hw/core/machine.c             |  2 ++
  hw/display/ramfb-standalone.c |  2 +-
  hw/i386/pc_piix.c             | 10 ++++++++++
  hw/i386/pc_q35.c              |  3 +++
  hw/vfio/pci.c                 |  2 +-
  5 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 63c6ef93d2..349aec1e0d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -46,6 +46,8 @@ GlobalProperty hw_compat_9_2[] = {
      { "migration", "multifd-clean-tls-termination", "false" },
      { "migration", "send-switchover-start", "off"},
      { "vfio-pci", "x-migration-multifd-transfer", "off" },
+    { "ramfb", "use-legacy-x86-rom", "true"},
+    { "vfio-pci", "use-legacy-x86-rom", "true" },
why 9.2? The 10.0 machine type should also apply the previous "true"
default setting. To me the new default shall only affect the latest 10.1
machine type.

It seems what I understand is wrong, I thought the 9.2 is the historical 
machine type.

10.0 machine type is the current machine type, it also should be 
included into the historical machine type when we add the property.

ok I will update it again with add those property into 10.0 machine type.

Thanks

  };
  const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2);
  
diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
index af1175bf96..ddbf42f181 100644
--- a/hw/display/ramfb-standalone.c
+++ b/hw/display/ramfb-standalone.c
@@ -63,7 +63,7 @@ static const VMStateDescription ramfb_dev_vmstate = {
  
  static const Property ramfb_properties[] = {
      DEFINE_PROP_BOOL("x-migrate", RAMFBStandaloneState, migrate,  true),
-    DEFINE_PROP_BOOL("use-legacy-x86-rom", RAMFBStandaloneState, 
use_legacy_x86_rom, true),
+    DEFINE_PROP_BOOL("use-legacy-x86-rom", RAMFBStandaloneState, 
use_legacy_x86_rom, false),
  };
  
  static void ramfb_class_initfn(ObjectClass *klass, void *data)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6c91e2d292..4a8bbc0e28 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -49,6 +49,7 @@
  #include "hw/i2c/smbus_eeprom.h"
  #include "exec/memory.h"
  #include "hw/acpi/acpi.h"
+#include "hw/vfio/pci.h"
  #include "qapi/error.h"
  #include "qemu/error-report.h"
  #include "system/xen.h"
@@ -77,6 +78,13 @@ static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
  static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
  #endif
  
+static GlobalProperty pc_piix_compat_defaults[] = {
+    { TYPE_RAMFB_DEVICE, "use-legacy-x86-rom", "true" },
+    { TYPE_VFIO_PCI, "use-legacy-x86-rom", "true" },
+};
+static const size_t pc_piix_compat_defaults_len =
+    G_N_ELEMENTS(pc_piix_compat_defaults);
+
  /*
   * Return the global irq number corresponding to a given device irq
   * pin. We could also use the bus number to have a more precise mapping.
@@ -477,6 +485,8 @@ static void pc_i440fx_machine_options(MachineClass *m)
                                     pc_set_south_bridge);
      object_class_property_set_description(oc, "x-south-bridge",
                                       "Use a different south bridge than 
PIIX3");
+    compat_props_add(m->compat_props,
+                     pc_piix_compat_defaults, pc_piix_compat_defaults_len);
  }
  
  static void pc_i440fx_machine_10_0_options(MachineClass *m)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index fd96d0345c..f6d89578d0 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -45,6 +45,7 @@
  #include "hw/i386/pc.h"
  #include "hw/i386/amd_iommu.h"
  #include "hw/i386/intel_iommu.h"
+#include "hw/vfio/pci.h"
  #include "hw/virtio/virtio-iommu.h"
  #include "hw/display/ramfb.h"
  #include "hw/ide/pci.h"
@@ -67,6 +68,8 @@
  
  static GlobalProperty pc_q35_compat_defaults[] = {
      { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "39" },
+    { TYPE_RAMFB_DEVICE, "use-legacy-x86-rom", "true" },
+    { TYPE_VFIO_PCI, "use-legacy-x86-rom", "true" },
I hope we do not omit any other machine that would actually need ramfb
rom. At the moment we take care of q35 and  piix. Anyone aware of any
other user?

Thanks

Eric
  };
  static const size_t pc_q35_compat_defaults_len =
      G_N_ELEMENTS(pc_q35_compat_defaults);
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index ff0d93fae0..a529500b70 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3564,7 +3564,7 @@ static const TypeInfo vfio_pci_dev_info = {
  
  static const Property vfio_pci_dev_nohotplug_properties[] = {
      DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
-    DEFINE_PROP_BOOL("use-legacy-x86-rom", VFIOPCIDevice, use_legacy_x86_rom, 
true),
+    DEFINE_PROP_BOOL("use-legacy-x86-rom", VFIOPCIDevice, use_legacy_x86_rom, 
false),
      DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate,
                              ON_OFF_AUTO_AUTO),
  };


--
Shaoqin