[PATCH v4 0/2] ramfb: Add property to control if load the romfile

[PATCH v4 0/2] ramfb: Add property to control if load the romfile

[Shaoqin Huang]

Now the ramfb will load the vgabios-ramfb.bin unconditionally, but only
the x86 need the vgabios-ramfb.bin, this can cause that when use the
release package on arm64 it can't find the vgabios-ramfb.bin.

Because only seabios will use the vgabios-ramfb.bin, load the rom logic
is x86-specific. For other !x86 platforms, the edk2 ships an EFI driver
for ramfb, so they don't need to load the romfile.

So add a new property use_legacy_x86_rom in both ramfb and vfio_pci
device, because the vfio display also use the ramfb_setup() to load
the vgabios-ramfb.bin file.

After have this property, the machine type can set the compatibility to
not load the vgabios-ramfb.bin if the arch doesn't need it.

Then I 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.

Changelog:
---------
v3 -> v4:
  - Set the new property to false by default, only set it to true on x86.
v2 -> v3:
  - Fix the underscore error.
  - Add a new patch to set the property in arm compatibility.
v1 -> v2:
  - Change the property name.

v3: https://lore.kernel.org/all/20250609073408.2083831-1-shahuang@redhat.com/
v2: https://lore.kernel.org/all/20250606070234.2063451-1-shahuang@redhat.com/
v1: https://lore.kernel.org/all/20250605030351.2056571-1-shahuang@redhat.com/

Shaoqin Huang (2):
  ramfb: Add property to control if load the romfile
  hw/i386: Add the ramfb romfile compatatibility

 hw/display/ramfb-standalone.c | 4 +++-
 hw/display/ramfb-stubs.c      | 2 +-
 hw/display/ramfb.c            | 6 ++++--
 hw/i386/pc_q35.c              | 3 +++
 hw/vfio/display.c             | 4 ++--
 hw/vfio/pci.c                 | 1 +
 hw/vfio/pci.h                 | 1 +
 include/hw/display/ramfb.h    | 2 +-
 8 files changed, 16 insertions(+), 7 deletions(-)

-- 
2.40.1

[PATCH v4 1/2] ramfb: Add property to control if load the romfile

[Shaoqin Huang]

Now the ramfb will load the vgabios-ramfb.bin unconditionally, but only
the x86 need the vgabios-ramfb.bin, this can cause that when use the
release package on arm64 it can't find the vgabios-ramfb.bin.

Because only seabios will use the vgabios-ramfb.bin, load the rom logic
is x86-specific. For other !x86 platforms, the edk2 ships an EFI driver
for ramfb, so they don't need to load the romfile.

So add a new property use_legacy_x86_rom in both ramfb and vfio_pci
device, because the vfio display also use the ramfb_setup() to load
the vgabios-ramfb.bin file.

After have this property, the machine type can set the compatibility to
not load the vgabios-ramfb.bin if the arch doesn't need it.

Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
 hw/display/ramfb-standalone.c | 4 +++-
 hw/display/ramfb-stubs.c      | 2 +-
 hw/display/ramfb.c            | 6 ++++--
 hw/vfio/display.c             | 4 ++--
 hw/vfio/pci.c                 | 1 +
 hw/vfio/pci.h                 | 1 +
 include/hw/display/ramfb.h    | 2 +-
 7 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
index 1be106b57f..af1175bf96 100644
--- a/hw/display/ramfb-standalone.c
+++ b/hw/display/ramfb-standalone.c
@@ -17,6 +17,7 @@ struct RAMFBStandaloneState {
     QemuConsole *con;
     RAMFBState *state;
     bool migrate;
+    bool use_legacy_x86_rom;
 };
 
 static void display_update_wrapper(void *dev)
@@ -39,7 +40,7 @@ static void ramfb_realizefn(DeviceState *dev, Error **errp)
     RAMFBStandaloneState *ramfb = RAMFB(dev);
 
     ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
-    ramfb->state = ramfb_setup(errp);
+    ramfb->state = ramfb_setup(ramfb->use_legacy_x86_rom, errp);
 }
 
 static bool migrate_needed(void *opaque)
@@ -62,6 +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),
 };
 
 static void ramfb_class_initfn(ObjectClass *klass, void *data)
diff --git a/hw/display/ramfb-stubs.c b/hw/display/ramfb-stubs.c
index cf64733b10..b83551357b 100644
--- a/hw/display/ramfb-stubs.c
+++ b/hw/display/ramfb-stubs.c
@@ -8,7 +8,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
 {
 }
 
-RAMFBState *ramfb_setup(Error **errp)
+RAMFBState *ramfb_setup(bool romfile, Error **errp)
 {
     error_setg(errp, "ramfb support not available");
     return NULL;
diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 8c0f907673..9a17d97d07 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -135,7 +135,7 @@ const VMStateDescription ramfb_vmstate = {
     }
 };
 
-RAMFBState *ramfb_setup(Error **errp)
+RAMFBState *ramfb_setup(bool romfile, Error **errp)
 {
     FWCfgState *fw_cfg = fw_cfg_find();
     RAMFBState *s;
@@ -147,7 +147,9 @@ RAMFBState *ramfb_setup(Error **errp)
 
     s = g_new0(RAMFBState, 1);
 
-    rom_add_vga("vgabios-ramfb.bin");
+    if (romfile) {
+        rom_add_vga("vgabios-ramfb.bin");
+    }
     fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
                              NULL, ramfb_fw_cfg_write, s,
                              &s->cfg, sizeof(s->cfg), false);
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index ea87830fe0..8bfd8eb1e3 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -365,7 +365,7 @@ static bool vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
                                           &vfio_display_dmabuf_ops,
                                           vdev);
     if (vdev->enable_ramfb) {
-        vdev->dpy->ramfb = ramfb_setup(errp);
+        vdev->dpy->ramfb = ramfb_setup(vdev->use_legacy_x86_rom, errp);
         if (!vdev->dpy->ramfb) {
             return false;
         }
@@ -494,7 +494,7 @@ static bool vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
                                           &vfio_display_region_ops,
                                           vdev);
     if (vdev->enable_ramfb) {
-        vdev->dpy->ramfb = ramfb_setup(errp);
+        vdev->dpy->ramfb = ramfb_setup(vdev->use_legacy_x86_rom, errp);
         if (!vdev->dpy->ramfb) {
             return false;
         }
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 7f1532fbed..ff0d93fae0 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3564,6 +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_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate,
                             ON_OFF_AUTO_AUTO),
 };
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index d94ecaba68..713956157e 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -177,6 +177,7 @@ struct VFIOPCIDevice {
     bool no_kvm_ioeventfd;
     bool no_vfio_ioeventfd;
     bool enable_ramfb;
+    bool use_legacy_x86_rom;
     OnOffAuto ramfb_migrate;
     bool defer_kvm_irq_routing;
     bool clear_parent_atomics_on_exit;
diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
index a7e0019144..172aa6dc89 100644
--- a/include/hw/display/ramfb.h
+++ b/include/hw/display/ramfb.h
@@ -6,7 +6,7 @@
 /* ramfb.c */
 typedef struct RAMFBState RAMFBState;
 void ramfb_display_update(QemuConsole *con, RAMFBState *s);
-RAMFBState *ramfb_setup(Error **errp);
+RAMFBState *ramfb_setup(bool romfile, Error **errp);
 
 extern const VMStateDescription ramfb_vmstate;
 
-- 
2.40.1

[PATCH v4 2/2] hw/i386: Add the ramfb romfile compatatibility

[Shaoqin Huang]

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.

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

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_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" },
 };
 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),
 };
-- 
2.40.1

Re: [PATCH v4 0/2] ramfb: Add property to control if load the romfile

[Shaoqin Huang]

Hi guys,

Kindly ping for this series.

Thanks,
Shaoqin

On 6/17/25 11:05 AM, Shaoqin Huang wrote:
> Now the ramfb will load the vgabios-ramfb.bin unconditionally, but only
> the x86 need the vgabios-ramfb.bin, this can cause that when use the
> release package on arm64 it can't find the vgabios-ramfb.bin.
> 
> Because only seabios will use the vgabios-ramfb.bin, load the rom logic
> is x86-specific. For other !x86 platforms, the edk2 ships an EFI driver
> for ramfb, so they don't need to load the romfile.
> 
> So add a new property use_legacy_x86_rom in both ramfb and vfio_pci
> device, because the vfio display also use the ramfb_setup() to load
> the vgabios-ramfb.bin file.
> 
> After have this property, the machine type can set the compatibility to
> not load the vgabios-ramfb.bin if the arch doesn't need it.
> 
> Then I 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.
> 
> Changelog:
> ---------
> v3 -> v4:
>    - Set the new property to false by default, only set it to true on x86.
> v2 -> v3:
>    - Fix the underscore error.
>    - Add a new patch to set the property in arm compatibility.
> v1 -> v2:
>    - Change the property name.
> 
> v3: https://lore.kernel.org/all/20250609073408.2083831-1-shahuang@redhat.com/
> v2: https://lore.kernel.org/all/20250606070234.2063451-1-shahuang@redhat.com/
> v1: https://lore.kernel.org/all/20250605030351.2056571-1-shahuang@redhat.com/
> 
> Shaoqin Huang (2):
>    ramfb: Add property to control if load the romfile
>    hw/i386: Add the ramfb romfile compatatibility
> 
>   hw/display/ramfb-standalone.c | 4 +++-
>   hw/display/ramfb-stubs.c      | 2 +-
>   hw/display/ramfb.c            | 6 ++++--
>   hw/i386/pc_q35.c              | 3 +++
>   hw/vfio/display.c             | 4 ++--
>   hw/vfio/pci.c                 | 1 +
>   hw/vfio/pci.h                 | 1 +
>   include/hw/display/ramfb.h    | 2 +-
>   8 files changed, 16 insertions(+), 7 deletions(-)
> 

-- 
Shaoqin

Re: [PATCH v4 2/2] hw/i386: Add the ramfb romfile compatatibility

[Eric Auger]

On 6/17/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.
s/compatatibility/compatibility in the title
> 
> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
> ---
>  hw/display/ramfb-standalone.c | 2 +-
>  hw/i386/pc_q35.c              | 3 +++
>  hw/vfio/pci.c                 | 2 +-
>  3 files changed, 5 insertions(+), 2 deletions(-)
> 
> 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_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" },
this will only keep the legacy behavior along with q35 machine type but
not on other machines being used for x86. what about pc-i440fx? Doesn't
it apply to it as well? Are there other machine types also impacted.

Also what about Daniel's comment in v3:
https://lore.kernel.org/all/aEak8utPPkHepVfR@redhat.com/
"For non-x86, historical versioned machine types will need
likely it set to true, in order to avoid the memory layout
being changed IIUC."

Is it actually needed?

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),
>  };

Re: [PATCH v4 1/2] ramfb: Add property to control if load the romfile

[Eric Auger]

Hi Shaoqin,
On 6/17/25 5:05 AM, Shaoqin Huang wrote:
> Now the ramfb will load the vgabios-ramfb.bin unconditionally, but only
Currently the ramfb device loads ...
> the x86 need the vgabios-ramfb.bin, this can cause that when use the
> release package on arm64 it can't find the vgabios-ramfb.bin.
> 
> Because only seabios will use the vgabios-ramfb.bin, load the rom logic
> is x86-specific. For other !x86 platforms, the edk2 ships an EFI driver
> for ramfb, so they don't need to load the romfile.
> 
> So add a new property use_legacy_x86_rom in both ramfb and vfio_pci
was renamed with - instead of _
> device, because the vfio display also use the ramfb_setup() to load
> the vgabios-ramfb.bin file.
> 
> After have this property, the machine type can set the compatibility to
> not load the vgabios-ramfb.bin if the arch doesn't need it.

Please add in the commit message that for now the default value is true
but it will be turned off by default in subsequent patch when compats
get properly handled.
> 
> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
> ---
>  hw/display/ramfb-standalone.c | 4 +++-
>  hw/display/ramfb-stubs.c      | 2 +-
>  hw/display/ramfb.c            | 6 ++++--
>  hw/vfio/display.c             | 4 ++--
>  hw/vfio/pci.c                 | 1 +
>  hw/vfio/pci.h                 | 1 +
>  include/hw/display/ramfb.h    | 2 +-
>  7 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
> index 1be106b57f..af1175bf96 100644
> --- a/hw/display/ramfb-standalone.c
> +++ b/hw/display/ramfb-standalone.c
> @@ -17,6 +17,7 @@ struct RAMFBStandaloneState {
>      QemuConsole *con;
>      RAMFBState *state;
>      bool migrate;
> +    bool use_legacy_x86_rom;
>  };
>  
>  static void display_update_wrapper(void *dev)
> @@ -39,7 +40,7 @@ static void ramfb_realizefn(DeviceState *dev, Error **errp)
>      RAMFBStandaloneState *ramfb = RAMFB(dev);
>  
>      ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
> -    ramfb->state = ramfb_setup(errp);
> +    ramfb->state = ramfb_setup(ramfb->use_legacy_x86_rom, errp);
>  }
>  
>  static bool migrate_needed(void *opaque)
> @@ -62,6 +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),
>  };
>  
>  static void ramfb_class_initfn(ObjectClass *klass, void *data)
> diff --git a/hw/display/ramfb-stubs.c b/hw/display/ramfb-stubs.c
> index cf64733b10..b83551357b 100644
> --- a/hw/display/ramfb-stubs.c
> +++ b/hw/display/ramfb-stubs.c
> @@ -8,7 +8,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>  {
>  }
>  
> -RAMFBState *ramfb_setup(Error **errp)
> +RAMFBState *ramfb_setup(bool romfile, Error **errp)
>  {
>      error_setg(errp, "ramfb support not available");
>      return NULL;
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 8c0f907673..9a17d97d07 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -135,7 +135,7 @@ const VMStateDescription ramfb_vmstate = {
>      }
>  };
>  
> -RAMFBState *ramfb_setup(Error **errp)
> +RAMFBState *ramfb_setup(bool romfile, Error **errp)
>  {
>      FWCfgState *fw_cfg = fw_cfg_find();
>      RAMFBState *s;
> @@ -147,7 +147,9 @@ RAMFBState *ramfb_setup(Error **errp)
>  
>      s = g_new0(RAMFBState, 1);
>  
> -    rom_add_vga("vgabios-ramfb.bin");
> +    if (romfile) {
> +        rom_add_vga("vgabios-ramfb.bin");
> +    }
>      fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
>                               NULL, ramfb_fw_cfg_write, s,
>                               &s->cfg, sizeof(s->cfg), false);
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index ea87830fe0..8bfd8eb1e3 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -365,7 +365,7 @@ static bool vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
>                                            &vfio_display_dmabuf_ops,
>                                            vdev);
>      if (vdev->enable_ramfb) {
> -        vdev->dpy->ramfb = ramfb_setup(errp);
> +        vdev->dpy->ramfb = ramfb_setup(vdev->use_legacy_x86_rom, errp);
>          if (!vdev->dpy->ramfb) {
>              return false;
>          }
> @@ -494,7 +494,7 @@ static bool vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
>                                            &vfio_display_region_ops,
>                                            vdev);
>      if (vdev->enable_ramfb) {
> -        vdev->dpy->ramfb = ramfb_setup(errp);
> +        vdev->dpy->ramfb = ramfb_setup(vdev->use_legacy_x86_rom, errp);
>          if (!vdev->dpy->ramfb) {
>              return false;
>          }
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 7f1532fbed..ff0d93fae0 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3564,6 +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_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate,
>                              ON_OFF_AUTO_AUTO),
>  };
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index d94ecaba68..713956157e 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -177,6 +177,7 @@ struct VFIOPCIDevice {
>      bool no_kvm_ioeventfd;
>      bool no_vfio_ioeventfd;
>      bool enable_ramfb;
> +    bool use_legacy_x86_rom;
>      OnOffAuto ramfb_migrate;
>      bool defer_kvm_irq_routing;
>      bool clear_parent_atomics_on_exit;
> diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
> index a7e0019144..172aa6dc89 100644
> --- a/include/hw/display/ramfb.h
> +++ b/include/hw/display/ramfb.h
> @@ -6,7 +6,7 @@
>  /* ramfb.c */
>  typedef struct RAMFBState RAMFBState;
>  void ramfb_display_update(QemuConsole *con, RAMFBState *s);
> -RAMFBState *ramfb_setup(Error **errp);
> +RAMFBState *ramfb_setup(bool romfile, Error **errp);
>  
>  extern const VMStateDescription ramfb_vmstate;
>  
Thanks

Eric

Re: [PATCH v4 1/2] ramfb: Add property to control if load the romfile

[Shaoqin Huang]

Hi Eric,

On 6/23/25 5:20 PM, Eric Auger wrote:
> Hi Shaoqin,
> On 6/17/25 5:05 AM, Shaoqin Huang wrote:
>> Now the ramfb will load the vgabios-ramfb.bin unconditionally, but only
> Currently the ramfb device loads ...

Ok, will change it.

>> the x86 need the vgabios-ramfb.bin, this can cause that when use the
>> release package on arm64 it can't find the vgabios-ramfb.bin.
>>
>> Because only seabios will use the vgabios-ramfb.bin, load the rom logic
>> is x86-specific. For other !x86 platforms, the edk2 ships an EFI driver
>> for ramfb, so they don't need to load the romfile.
>>
>> So add a new property use_legacy_x86_rom in both ramfb and vfio_pci
> was renamed with - instead of _

Ok, will fix it.

>> device, because the vfio display also use the ramfb_setup() to load
>> the vgabios-ramfb.bin file.
>>
>> After have this property, the machine type can set the compatibility to
>> not load the vgabios-ramfb.bin if the arch doesn't need it.
> 
> Please add in the commit message that for now the default value is true
> but it will be turned off by default in subsequent patch when compats
> get properly handled.

Got it.

Thanks.

>>
>> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
>> ---
>>   hw/display/ramfb-standalone.c | 4 +++-
>>   hw/display/ramfb-stubs.c      | 2 +-
>>   hw/display/ramfb.c            | 6 ++++--
>>   hw/vfio/display.c             | 4 ++--
>>   hw/vfio/pci.c                 | 1 +
>>   hw/vfio/pci.h                 | 1 +
>>   include/hw/display/ramfb.h    | 2 +-
>>   7 files changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
>> index 1be106b57f..af1175bf96 100644
>> --- a/hw/display/ramfb-standalone.c
>> +++ b/hw/display/ramfb-standalone.c
>> @@ -17,6 +17,7 @@ struct RAMFBStandaloneState {
>>       QemuConsole *con;
>>       RAMFBState *state;
>>       bool migrate;
>> +    bool use_legacy_x86_rom;
>>   };
>>   
>>   static void display_update_wrapper(void *dev)
>> @@ -39,7 +40,7 @@ static void ramfb_realizefn(DeviceState *dev, Error **errp)
>>       RAMFBStandaloneState *ramfb = RAMFB(dev);
>>   
>>       ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
>> -    ramfb->state = ramfb_setup(errp);
>> +    ramfb->state = ramfb_setup(ramfb->use_legacy_x86_rom, errp);
>>   }
>>   
>>   static bool migrate_needed(void *opaque)
>> @@ -62,6 +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),
>>   };
>>   
>>   static void ramfb_class_initfn(ObjectClass *klass, void *data)
>> diff --git a/hw/display/ramfb-stubs.c b/hw/display/ramfb-stubs.c
>> index cf64733b10..b83551357b 100644
>> --- a/hw/display/ramfb-stubs.c
>> +++ b/hw/display/ramfb-stubs.c
>> @@ -8,7 +8,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>>   {
>>   }
>>   
>> -RAMFBState *ramfb_setup(Error **errp)
>> +RAMFBState *ramfb_setup(bool romfile, Error **errp)
>>   {
>>       error_setg(errp, "ramfb support not available");
>>       return NULL;
>> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
>> index 8c0f907673..9a17d97d07 100644
>> --- a/hw/display/ramfb.c
>> +++ b/hw/display/ramfb.c
>> @@ -135,7 +135,7 @@ const VMStateDescription ramfb_vmstate = {
>>       }
>>   };
>>   
>> -RAMFBState *ramfb_setup(Error **errp)
>> +RAMFBState *ramfb_setup(bool romfile, Error **errp)
>>   {
>>       FWCfgState *fw_cfg = fw_cfg_find();
>>       RAMFBState *s;
>> @@ -147,7 +147,9 @@ RAMFBState *ramfb_setup(Error **errp)
>>   
>>       s = g_new0(RAMFBState, 1);
>>   
>> -    rom_add_vga("vgabios-ramfb.bin");
>> +    if (romfile) {
>> +        rom_add_vga("vgabios-ramfb.bin");
>> +    }
>>       fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
>>                                NULL, ramfb_fw_cfg_write, s,
>>                                &s->cfg, sizeof(s->cfg), false);
>> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
>> index ea87830fe0..8bfd8eb1e3 100644
>> --- a/hw/vfio/display.c
>> +++ b/hw/vfio/display.c
>> @@ -365,7 +365,7 @@ static bool vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
>>                                             &vfio_display_dmabuf_ops,
>>                                             vdev);
>>       if (vdev->enable_ramfb) {
>> -        vdev->dpy->ramfb = ramfb_setup(errp);
>> +        vdev->dpy->ramfb = ramfb_setup(vdev->use_legacy_x86_rom, errp);
>>           if (!vdev->dpy->ramfb) {
>>               return false;
>>           }
>> @@ -494,7 +494,7 @@ static bool vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
>>                                             &vfio_display_region_ops,
>>                                             vdev);
>>       if (vdev->enable_ramfb) {
>> -        vdev->dpy->ramfb = ramfb_setup(errp);
>> +        vdev->dpy->ramfb = ramfb_setup(vdev->use_legacy_x86_rom, errp);
>>           if (!vdev->dpy->ramfb) {
>>               return false;
>>           }
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 7f1532fbed..ff0d93fae0 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3564,6 +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_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate,
>>                               ON_OFF_AUTO_AUTO),
>>   };
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index d94ecaba68..713956157e 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -177,6 +177,7 @@ struct VFIOPCIDevice {
>>       bool no_kvm_ioeventfd;
>>       bool no_vfio_ioeventfd;
>>       bool enable_ramfb;
>> +    bool use_legacy_x86_rom;
>>       OnOffAuto ramfb_migrate;
>>       bool defer_kvm_irq_routing;
>>       bool clear_parent_atomics_on_exit;
>> diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
>> index a7e0019144..172aa6dc89 100644
>> --- a/include/hw/display/ramfb.h
>> +++ b/include/hw/display/ramfb.h
>> @@ -6,7 +6,7 @@
>>   /* ramfb.c */
>>   typedef struct RAMFBState RAMFBState;
>>   void ramfb_display_update(QemuConsole *con, RAMFBState *s);
>> -RAMFBState *ramfb_setup(Error **errp);
>> +RAMFBState *ramfb_setup(bool romfile, Error **errp);
>>   
>>   extern const VMStateDescription ramfb_vmstate;
>>   
> Thanks
> 
> Eric
> 

-- 
Shaoqin

Re: [PATCH v4 2/2] hw/i386: Add the ramfb romfile compatatibility

[Shaoqin Huang]

Hi Eric,

On 6/23/25 5:20 PM, Eric Auger wrote:
> 
> 
> On 6/17/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.
> s/compatatibility/compatibility in the title

Ok. Will fix it.

>>
>> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
>> ---
>>   hw/display/ramfb-standalone.c | 2 +-
>>   hw/i386/pc_q35.c              | 3 +++
>>   hw/vfio/pci.c                 | 2 +-
>>   3 files changed, 5 insertions(+), 2 deletions(-)
>>
>> 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_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" },
> this will only keep the legacy behavior along with q35 machine type but
> not on other machines being used for x86. what about pc-i440fx? Doesn't
> it apply to it as well? Are there other machine types also impacted.

Ok I will also add it with pc-i440fx. I think only q35 and i440fx are 
impacted.

> 
> Also what about Daniel's comment in v3:
> https://lore.kernel.org/all/aEak8utPPkHepVfR@redhat.com/
> "For non-x86, historical versioned machine types will need
> likely it set to true, in order to avoid the memory layout
> being changed IIUC."
> 
> Is it actually needed?

If those machine types need to set it to true. I think they can set it 
after they have this property.

Thanks,
Shaoqin

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

Re: [PATCH v4 2/2] hw/i386: Add the ramfb romfile compatatibility

[Eric Auger]

On 6/26/25 4:05 AM, Shaoqin Huang wrote:
> Hi Eric,
> 
> On 6/23/25 5:20 PM, Eric Auger wrote:
>>
>>
>> On 6/17/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.
>> s/compatatibility/compatibility in the title
> 
> Ok. Will fix it.
> 
>>>
>>> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
>>> ---
>>>   hw/display/ramfb-standalone.c | 2 +-
>>>   hw/i386/pc_q35.c              | 3 +++
>>>   hw/vfio/pci.c                 | 2 +-
>>>   3 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> 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_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" },
>> this will only keep the legacy behavior along with q35 machine type but
>> not on other machines being used for x86. what about pc-i440fx? Doesn't
>> it apply to it as well? Are there other machine types also impacted.
> 
> Ok I will also add it with pc-i440fx. I think only q35 and i440fx are
> impacted.
> 
>>
>> Also what about Daniel's comment in v3:
>> https://lore.kernel.org/all/aEak8utPPkHepVfR@redhat.com/
>> "For non-x86, historical versioned machine types will need
>> likely it set to true, in order to avoid the memory layout
>> being changed IIUC."
>>
>> Is it actually needed?
> 
> If those machine types need to set it to true. I think they can set it
> after they have this property.
nope it does not work like that. In case we really need to take care of
this, this must be handled by compats.

Thanks

Eric
> 
> Thanks,
> Shaoqin
> 
>>
>> 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),
>>>   };
>>
> 

Re: [PATCH v4 2/2] hw/i386: Add the ramfb romfile compatatibility

[Shaoqin Huang]

Hi Eric,

On 6/26/25 4:01 PM, Eric Auger wrote:
> 
> 
> On 6/26/25 4:05 AM, Shaoqin Huang wrote:
>> Hi Eric,
>>
>> On 6/23/25 5:20 PM, Eric Auger wrote:
>>>
>>>
>>> On 6/17/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.
>>> s/compatatibility/compatibility in the title
>>
>> Ok. Will fix it.
>>
>>>>
>>>> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
>>>> ---
>>>>    hw/display/ramfb-standalone.c | 2 +-
>>>>    hw/i386/pc_q35.c              | 3 +++
>>>>    hw/vfio/pci.c                 | 2 +-
>>>>    3 files changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> 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_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" },
>>> this will only keep the legacy behavior along with q35 machine type but
>>> not on other machines being used for x86. what about pc-i440fx? Doesn't
>>> it apply to it as well? Are there other machine types also impacted.
>>
>> Ok I will also add it with pc-i440fx. I think only q35 and i440fx are
>> impacted.
>>
>>>
>>> Also what about Daniel's comment in v3:
>>> https://lore.kernel.org/all/aEak8utPPkHepVfR@redhat.com/
>>> "For non-x86, historical versioned machine types will need
>>> likely it set to true, in order to avoid the memory layout
>>> being changed IIUC."
>>>
>>> Is it actually needed?
>>
>> If those machine types need to set it to true. I think they can set it
>> after they have this property.
> nope it does not work like that. In case we really need to take care of
> this, this must be handled by compats.

If so. Why don't we still keep the "use-legacy-x86-rom" default to true, 
and only set it to false to those arch which doesn't need it just like 
my original implementation.

Because I don't really know how other arch's memoery layout was impacted 
by this property set to false. I think keep their original behavior and 
only change it on arm64 is a good idea.

How do you think about it?

Thanks,
Shaoqin

> 
> Thanks
> 
> Eric
>>
>> Thanks,
>> Shaoqin
>>
>>>
>>> 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

Re: [PATCH v4 2/2] hw/i386: Add the ramfb romfile compatatibility

[Daniel P. Berrangé]

On Fri, Jun 27, 2025 at 01:37:55PM +0800, Shaoqin Huang wrote:
> Hi Eric,
> 
> On 6/26/25 4:01 PM, Eric Auger wrote:
> > 
> > 
> > On 6/26/25 4:05 AM, Shaoqin Huang wrote:
> > > Hi Eric,
> > > 
> > > On 6/23/25 5:20 PM, Eric Auger wrote:
> > > > 
> > > > 
> > > > On 6/17/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.
> > > > s/compatatibility/compatibility in the title
> > > 
> > > Ok. Will fix it.
> > > 
> > > > > 
> > > > > Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
> > > > > ---
> > > > >    hw/display/ramfb-standalone.c | 2 +-
> > > > >    hw/i386/pc_q35.c              | 3 +++
> > > > >    hw/vfio/pci.c                 | 2 +-
> > > > >    3 files changed, 5 insertions(+), 2 deletions(-)
> > > > > 
> > > > > 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_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" },
> > > > this will only keep the legacy behavior along with q35 machine type but
> > > > not on other machines being used for x86. what about pc-i440fx? Doesn't
> > > > it apply to it as well? Are there other machine types also impacted.
> > > 
> > > Ok I will also add it with pc-i440fx. I think only q35 and i440fx are
> > > impacted.
> > > 
> > > > 
> > > > Also what about Daniel's comment in v3:
> > > > https://lore.kernel.org/all/aEak8utPPkHepVfR@redhat.com/
> > > > "For non-x86, historical versioned machine types will need
> > > > likely it set to true, in order to avoid the memory layout
> > > > being changed IIUC."
> > > > 
> > > > Is it actually needed?
> > > 
> > > If those machine types need to set it to true. I think they can set it
> > > after they have this property.
> > nope it does not work like that. In case we really need to take care of
> > this, this must be handled by compats.
> 
> If so. Why don't we still keep the "use-legacy-x86-rom" default to true, and
> only set it to false to those arch which doesn't need it just like my
> original implementation.
> 
> Because I don't really know how other arch's memoery layout was impacted by
> this property set to false. I think keep their original behavior and only
> change it on arm64 is a good idea.
> 
> How do you think about it?

No, the default value of the property shoudl reflect the long
term desired behaviour - in this case 'use-legacy-x86-rom = false'.

We must then reverse this default to 'true' for ALL historical versioned
machine types on ALL architectures, where this device is built, or any
specific machines where we want to keep the historical behaviour going
forward.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 2/2] hw/i386: Add the ramfb romfile compatatibility

[Shaoqin Huang]

Hi Daniel,

On 6/27/25 3:15 PM, Daniel P. Berrangé wrote:
> On Fri, Jun 27, 2025 at 01:37:55PM +0800, Shaoqin Huang wrote:
>> Hi Eric,
>>
>> On 6/26/25 4:01 PM, Eric Auger wrote:
>>>
>>>
>>> On 6/26/25 4:05 AM, Shaoqin Huang wrote:
>>>> Hi Eric,
>>>>
>>>> On 6/23/25 5:20 PM, Eric Auger wrote:
>>>>>
>>>>>
>>>>> On 6/17/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.
>>>>> s/compatatibility/compatibility in the title
>>>>
>>>> Ok. Will fix it.
>>>>
>>>>>>
>>>>>> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
>>>>>> ---
>>>>>>     hw/display/ramfb-standalone.c | 2 +-
>>>>>>     hw/i386/pc_q35.c              | 3 +++
>>>>>>     hw/vfio/pci.c                 | 2 +-
>>>>>>     3 files changed, 5 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> 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_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" },
>>>>> this will only keep the legacy behavior along with q35 machine type but
>>>>> not on other machines being used for x86. what about pc-i440fx? Doesn't
>>>>> it apply to it as well? Are there other machine types also impacted.
>>>>
>>>> Ok I will also add it with pc-i440fx. I think only q35 and i440fx are
>>>> impacted.
>>>>
>>>>>
>>>>> Also what about Daniel's comment in v3:
>>>>> https://lore.kernel.org/all/aEak8utPPkHepVfR@redhat.com/
>>>>> "For non-x86, historical versioned machine types will need
>>>>> likely it set to true, in order to avoid the memory layout
>>>>> being changed IIUC."
>>>>>
>>>>> Is it actually needed?
>>>>
>>>> If those machine types need to set it to true. I think they can set it
>>>> after they have this property.
>>> nope it does not work like that. In case we really need to take care of
>>> this, this must be handled by compats.
>>
>> If so. Why don't we still keep the "use-legacy-x86-rom" default to true, and
>> only set it to false to those arch which doesn't need it just like my
>> original implementation.
>>
>> Because I don't really know how other arch's memoery layout was impacted by
>> this property set to false. I think keep their original behavior and only
>> change it on arm64 is a good idea.
>>
>> How do you think about it?
> 
> No, the default value of the property shoudl reflect the long
> term desired behaviour - in this case 'use-legacy-x86-rom = false'.

Ok I understand your thoughts.

> 
> We must then reverse this default to 'true' for ALL historical versioned
> machine types on ALL architectures, where this device is built, or any
> specific machines where we want to keep the historical behaviour going
> forward.

But how do we implement that?

Maybe set the 'use-legacy-x86-rom = true' in hw_compat_9_2 ?

I'm not sure about that, could you give some hints about how to reverse 
the default to 'true' for ALL historical versioned machine types?

Thanks a lot.

> 
> 
> With regards,
> Daniel

-- 
Shaoqin

Re: [PATCH v4 2/2] hw/i386: Add the ramfb romfile compatatibility

[Gerd Hoffmann]

  Hi,

> > If those machine types need to set it to true. I think they can set it
> > after they have this property.
> nope it does not work like that. In case we really need to take care of
> this, this must be handled by compats.

ramfb is a sysbus device so it can only used for machine types where it
is explicitly enabled:

  # git grep machine_class_allow_dynamic_sysbus_dev.*TYPE_RAMFB_DEVICE
  hw/arm/virt.c:    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
  hw/i386/microvm.c:    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
  hw/i386/pc_piix.c:    machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
  hw/i386/pc_q35.c:    machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
  hw/loongarch/virt.c:    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
  hw/riscv/virt.c:    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);

So these six are the only machine types we have to worry about.

The three x86 machine types (pc, q35, microvm) will actually use the rom
(when booting with seabios).

For arm/riscv/loongarch virt we want disable the rom.

Everything else doesn't matter much because ramfb can not be used in the
first place, you'll get an error like this when trying:

  # qemu-default -M isapc -device ramfb
  qemu-system-x86_64: -device ramfb: Parameter 'driver' expects a dynamic sysbus device type for the machine

HTH & take care,
  Gerd

Re: [PATCH v4 2/2] hw/i386: Add the ramfb romfile compatatibility

[Eric Auger]

Hi Gerd,

On 7/1/25 2:04 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>>> If those machine types need to set it to true. I think they can set it
>>> after they have this property.
>> nope it does not work like that. In case we really need to take care of
>> this, this must be handled by compats.
> 
> ramfb is a sysbus device so it can only used for machine types where it
> is explicitly enabled:
> 
>   # git grep machine_class_allow_dynamic_sysbus_dev.*TYPE_RAMFB_DEVICE
>   hw/arm/virt.c:    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
>   hw/i386/microvm.c:    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
>   hw/i386/pc_piix.c:    machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
>   hw/i386/pc_q35.c:    machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
>   hw/loongarch/virt.c:    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
>   hw/riscv/virt.c:    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
> 
> So these six are the only machine types we have to worry about.
> 
> The three x86 machine types (pc, q35, microvm) will actually use the rom
> (when booting with seabios).
makes total sense. So microvm is needed as well.

Thank you for the confirmation!

Eric
> 
> For arm/riscv/loongarch virt we want disable the rom.
> 
> Everything else doesn't matter much because ramfb can not be used in the
> first place, you'll get an error like this when trying:
> 
>   # qemu-default -M isapc -device ramfb
>   qemu-system-x86_64: -device ramfb: Parameter 'driver' expects a dynamic sysbus device type for the machine
> 
> HTH & take care,
>   Gerd
>