[PATCH v2] ramfb: Add property to control if load the romfile

[PATCH v2] 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..4e4759c954 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

Re: [PATCH v2] ramfb: Add property to control if load the romfile

[Daniel P. Berrangé]

On Fri, Jun 06, 2025 at 03:02:34AM -0400, 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.

Can you make this a series, with an additional patch that updates the
current in-dev machine types to use this new property, so we're clear
about the proposed usage.

> 
> 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..4e4759c954 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
> 

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 v2] ramfb: Add property to control if load the romfile

[Cédric Le Goater]

On 6/6/25 09:52, Daniel P. Berrangé wrote:
> On Fri, Jun 06, 2025 at 03:02:34AM -0400, 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.
> 
> Can you make this a series, with an additional patch that updates the
> current in-dev machine types to use this new property, so we're clear
> about the proposed usage.

yes. And please change the vfio-pci property name to use underscores.

C.

Re: [PATCH v2] ramfb: Add property to control if load the romfile

[Cédric Le Goater]

On 6/6/25 10:06, Cédric Le Goater wrote:
> On 6/6/25 09:52, Daniel P. Berrangé wrote:
>> On Fri, Jun 06, 2025 at 03:02:34AM -0400, 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.
>>
>> Can you make this a series, with an additional patch that updates the
>> current in-dev machine types to use this new property, so we're clear
>> about the proposed usage.
> 
> yes. And please change the vfio-pci property name to use underscores.

Sorry, to *not* use underscores : use-legacy-x86-rom


Thanks,

C.

Re: [PATCH v2] ramfb: Add property to control if load the romfile

[Philippe Mathieu-Daudé]

On 6/6/25 09:52, Daniel P. Berrangé wrote:
> On Fri, Jun 06, 2025 at 03:02:34AM -0400, 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.
> 
> Can you make this a series, with an additional patch that updates the
> current in-dev machine types to use this new property, so we're clear
> about the proposed usage.
> 
>>
>> 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/vfio/pci.c b/hw/vfio/pci.c
>> index 7f1532fbed..4e4759c954 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),
>>   };

Alternatively with target-info API:

-- >8 --
diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 8c0f907673d..689f10625f8 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -12,6 +12,7 @@
   */

  #include "qemu/osdep.h"
+#include "qemu/target-info.h"
  #include "qapi/error.h"
  #include "hw/loader.h"
  #include "hw/display/ramfb.h"
@@ -147,7 +148,15 @@ RAMFBState *ramfb_setup(Error **errp)

      s = g_new0(RAMFBState, 1);

-    rom_add_vga("vgabios-ramfb.bin");
+    switch (target_system_arch()) {
+    case SYS_EMU_TARGET_I386:
+    case SYS_EMU_TARGET_X86_64:
+        rom_add_vga("vgabios-ramfb.bin");
+        break;
+    default:
+        break;
+    }
+
      fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
                               NULL, ramfb_fw_cfg_write, s,
                               &s->cfg, sizeof(s->cfg), false);
---

Recent work event introduces target_base_FOO() so that'd be:

-- >8 --
diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 8c0f907673d..2aa3b309010 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -12,6 +12,7 @@
   */

  #include "qemu/osdep.h"
+#include "qemu/target-info.h"
  #include "qapi/error.h"
  #include "hw/loader.h"
  #include "hw/display/ramfb.h"
@@ -147,7 +148,10 @@ RAMFBState *ramfb_setup(Error **errp)

      s = g_new0(RAMFBState, 1);

-    rom_add_vga("vgabios-ramfb.bin");
+    if (target_base_x86()) {
+        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);
---

Unfortunately I had to focus on more urgent stuff so this isn't
merged yet (besides I hurt a finger yesterday and am now typing
slower). I hope I'd be able to respin that next week.

Re: [PATCH v2] ramfb: Add property to control if load the romfile

[Daniel P. Berrangé]

On Fri, Jun 06, 2025 at 11:04:34AM +0200, Philippe Mathieu-Daudé wrote:
> On 6/6/25 09:52, Daniel P. Berrangé wrote:
> > On Fri, Jun 06, 2025 at 03:02:34AM -0400, 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.
> > 
> > Can you make this a series, with an additional patch that updates the
> > current in-dev machine types to use this new property, so we're clear
> > about the proposed usage.
> > 
> > > 
> > > 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/vfio/pci.c b/hw/vfio/pci.c
> > > index 7f1532fbed..4e4759c954 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),
> > >   };
> 
> Alternatively with target-info API:

Don't we need to use the property in order to tie this to the
machine type ? Even if existing non-x86 machines are not using
this ROM, the fact that QEMU loaded it will impact the guest
memory layout which needs to be preserved across migration,
and thus machine type versions.


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 v2] ramfb: Add property to control if load the romfile

[Pierrick Bouvier]

On 6/6/25 2:04 AM, Philippe Mathieu-Daudé wrote:
> On 6/6/25 09:52, Daniel P. Berrangé wrote:
>> On Fri, Jun 06, 2025 at 03:02:34AM -0400, 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.
>>
>> Can you make this a series, with an additional patch that updates the
>> current in-dev machine types to use this new property, so we're clear
>> about the proposed usage.
>>
>>>
>>> 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/vfio/pci.c b/hw/vfio/pci.c
>>> index 7f1532fbed..4e4759c954 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),
>>>    };
> 
> Alternatively with target-info API:
> 
> -- >8 --
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 8c0f907673d..689f10625f8 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -12,6 +12,7 @@
>     */
> 
>    #include "qemu/osdep.h"
> +#include "qemu/target-info.h"
>    #include "qapi/error.h"
>    #include "hw/loader.h"
>    #include "hw/display/ramfb.h"
> @@ -147,7 +148,15 @@ RAMFBState *ramfb_setup(Error **errp)
> 
>        s = g_new0(RAMFBState, 1);
> 
> -    rom_add_vga("vgabios-ramfb.bin");
> +    switch (target_system_arch()) {
> +    case SYS_EMU_TARGET_I386:
> +    case SYS_EMU_TARGET_X86_64:
> +        rom_add_vga("vgabios-ramfb.bin");
> +        break;
> +    default:
> +        break;
> +    }
> +
>        fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
>                                 NULL, ramfb_fw_cfg_write, s,
>                                 &s->cfg, sizeof(s->cfg), false);
> ---
> 
> Recent work event introduces target_base_FOO() so that'd be:
> 
> -- >8 --
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 8c0f907673d..2aa3b309010 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -12,6 +12,7 @@
>     */
> 
>    #include "qemu/osdep.h"
> +#include "qemu/target-info.h"
>    #include "qapi/error.h"
>    #include "hw/loader.h"
>    #include "hw/display/ramfb.h"
> @@ -147,7 +148,10 @@ RAMFBState *ramfb_setup(Error **errp)
> 
>        s = g_new0(RAMFBState, 1);
> 
> -    rom_add_vga("vgabios-ramfb.bin");
> +    if (target_base_x86()) {
> +        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);
> ---
> 
> Unfortunately I had to focus on more urgent stuff so this isn't
> merged yet (besides I hurt a finger yesterday and am now typing
> slower). I hope I'd be able to respin that next week.

In case it's too much effort to respin all the series in a close future, 
maybe it could be a good idea to just send target_system_arch(), so that 
people can start using it.
 From there, people can add target_base_FOO() easily on a as needed 
basis, without any hard conflict issue.

Thanks,
Pierrick

Re: [PATCH v2] ramfb: Add property to control if load the romfile

[Shaoqin Huang]

Hi Daniel,

On 6/6/25 3:52 PM, Daniel P. Berrangé wrote:
> On Fri, Jun 06, 2025 at 03:02:34AM -0400, 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.
> 
> Can you make this a series, with an additional patch that updates the
> current in-dev machine types to use this new property, so we're clear
> about the proposed usage.
> 

Ok. Thanks for notification. I will update it in the version 3.

>>
>> 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..4e4759c954 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
>>
> 
> With regards,
> Daniel

-- 
Shaoqin

Re: [PATCH v2] ramfb: Add property to control if load the romfile

[Shaoqin Huang]

On 6/6/25 4:07 PM, Cédric Le Goater wrote:
> On 6/6/25 10:06, Cédric Le Goater wrote:
>> On 6/6/25 09:52, Daniel P. Berrangé wrote:
>>> On Fri, Jun 06, 2025 at 03:02:34AM -0400, 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.
>>>
>>> Can you make this a series, with an additional patch that updates the
>>> current in-dev machine types to use this new property, so we're clear
>>> about the proposed usage.
>>
>> yes. And please change the vfio-pci property name to use underscores.
> 
> Sorry, to *not* use underscores : use-legacy-x86-rom

Thanks for pointing out it. Will fix it.

Thanks.

> 
> 
> Thanks,
> 
> C.
> 
> 

-- 
Shaoqin

Re: [PATCH v2] ramfb: Add property to control if load the romfile

[Alex Williamson]

On Mon, 9 Jun 2025 13:16:14 +0800
Shaoqin Huang <shahuang@redhat.com> wrote:

> On 6/6/25 4:07 PM, Cédric Le Goater wrote:
> > On 6/6/25 10:06, Cédric Le Goater wrote:  
> >> On 6/6/25 09:52, Daniel P. Berrangé wrote:  
> >>> On Fri, Jun 06, 2025 at 03:02:34AM -0400, 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.  
> >>>
> >>> Can you make this a series, with an additional patch that updates the
> >>> current in-dev machine types to use this new property, so we're clear
> >>> about the proposed usage.  
> >>
> >> yes. And please change the vfio-pci property name to use underscores.  
> > 
> > Sorry, to *not* use underscores : use-legacy-x86-rom  
> 
> Thanks for pointing out it. Will fix it.

It's also not evident from the property that this is restricted to
ramfb behavior.  Should the option on the vfio-pci device be prefixed
with ramfb?  Thanks,

Alex