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

[PATCH v1] 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.

So add a new property ramfb-romfile 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..2b96a49baa 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 ramfb_romfile;
 };
 
 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->ramfb_romfile, 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("ramfb-romfile", RAMFBStandaloneState, ramfb_romfile, 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..56f10564f9 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->ramfb_romfile, 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->ramfb_romfile, errp);
         if (!vdev->dpy->ramfb) {
             return false;
         }
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 7f1532fbed..bfdf365978 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("ramfb-romfile", VFIOPCIDevice, ramfb_romfile, 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..d567de8f10 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 ramfb_romfile;
     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 v1] ramfb: Add property to control if load the romfile

[Philippe Mathieu-Daudé]

Hi Shaoqin,

On 5/6/25 05:03, 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.
> 
> So add a new property ramfb-romfile 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(-)

Simpler to directly pass the ROM path instead of using a boolean,
so board (or CLI) could pass path to non-x86 rom.

> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 7f1532fbed..bfdf365978 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("ramfb-romfile", VFIOPCIDevice, ramfb_romfile, true),

         DEFINE_PROP_STRING("rom-path", ...);

>       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..d567de8f10 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 ramfb_romfile;

         char *rompath;

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

   RAMFBState *ramfb_setup(char *rompath, Error **errp);

>   
>   extern const VMStateDescription ramfb_vmstate;
>   

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

[Gerd Hoffmann]

  Hi,

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

> Simpler to directly pass the ROM path instead of using a boolean,
> so board (or CLI) could pass path to non-x86 rom.

The rom is loaded into a fw_cfg file which only seabios will look at.
So this rom logic is x86-specific.

edk2 ships an EFI driver for ramfb, that is how ramfb is used on !x86
platforms today, and I don't expect that to change.

IMHO a bool is perfectly fine here, I don't think we will ever need the
flexibility to specify some other rom here.

take care,
  Gerd

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

[Cédric Le Goater]

On 6/5/25 14:21, Gerd Hoffmann wrote:
>    Hi,
> 
>>> 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.
> 
>> Simpler to directly pass the ROM path instead of using a boolean,
>> so board (or CLI) could pass path to non-x86 rom.
> 
> The rom is loaded into a fw_cfg file which only seabios will look at.
> So this rom logic is x86-specific.
> 
> edk2 ships an EFI driver for ramfb, that is how ramfb is used on !x86
> platforms today, and I don't expect that to change.

Should we also set the vfio-pci::ramfb-romfile property to false in
a compat property for ARM machines then ? I don't know for RISC-V and
PPC.


C.
  
> IMHO a bool is perfectly fine here, I don't think we will ever need the
> flexibility to specify some other rom here.
> > take care,
>    Gerd
> 

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

[Cédric Le Goater]

On 6/5/25 05:03, 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.
> 
> So add a new property ramfb-romfile 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..2b96a49baa 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 ramfb_romfile;
>   };
>   
>   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->ramfb_romfile, 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("ramfb-romfile", RAMFBStandaloneState, ramfb_romfile, 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..56f10564f9 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->ramfb_romfile, 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->ramfb_romfile, errp);
>           if (!vdev->dpy->ramfb) {
>               return false;
>           }
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 7f1532fbed..bfdf365978 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("ramfb-romfile", VFIOPCIDevice, ramfb_romfile, 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..d567de8f10 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 ramfb_romfile;

The attribute 'ramfb_romfile' sounds like a file name. Could you please
add an 'enable' prefix or suffix.


Thanks,

C.

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

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

[Daniel P. Berrangé]

On Thu, Jun 05, 2025 at 04:24:07PM +0200, Cédric Le Goater wrote:
> On 6/5/25 14:21, Gerd Hoffmann wrote:
> >    Hi,
> > 
> > > > 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.
> > 
> > > Simpler to directly pass the ROM path instead of using a boolean,
> > > so board (or CLI) could pass path to non-x86 rom.
> > 
> > The rom is loaded into a fw_cfg file which only seabios will look at.
> > So this rom logic is x86-specific.
> > 
> > edk2 ships an EFI driver for ramfb, that is how ramfb is used on !x86
> > platforms today, and I don't expect that to change.
> 
> Should we also set the vfio-pci::ramfb-romfile property to false in
> a compat property for ARM machines then ? I don't know for RISC-V and
> PPC.

Sounds like we'd be better setting the property to false by default,
and then special case x86 machine types to set it to true.

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

[Philippe Mathieu-Daudé]

On 5/6/25 14:21, Gerd Hoffmann wrote:
>    Hi,
> 
>>> 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.
> 
>> Simpler to directly pass the ROM path instead of using a boolean,
>> so board (or CLI) could pass path to non-x86 rom.
> 
> The rom is loaded into a fw_cfg file which only seabios will look at.
> So this rom logic is x86-specific.
> 
> edk2 ships an EFI driver for ramfb, that is how ramfb is used on !x86
> platforms today, and I don't expect that to change.
> 
> IMHO a bool is perfectly fine here, I don't think we will ever need the
> flexibility to specify some other rom here.

Understood, better then! Maybe name the boolean "use_legacy_x86_rom" and
add a comment explaining EFI driver is expected on !x86?

Regards,

Phil.

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

[Shaoqin Huang]

Hi, guys

Thanks for all of your suggestions.

On 6/5/25 11:11 PM, Philippe Mathieu-Daudé wrote:
> On 5/6/25 14:21, Gerd Hoffmann wrote:
>>    Hi,
>>
>>>> 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.
>>
>>> Simpler to directly pass the ROM path instead of using a boolean,
>>> so board (or CLI) could pass path to non-x86 rom.
>>
>> The rom is loaded into a fw_cfg file which only seabios will look at.
>> So this rom logic is x86-specific.
>>
>> edk2 ships an EFI driver for ramfb, that is how ramfb is used on !x86
>> platforms today, and I don't expect that to change.
>>
>> IMHO a bool is perfectly fine here, I don't think we will ever need the
>> flexibility to specify some other rom here.
> 
> Understood, better then! Maybe name the boolean "use_legacy_x86_rom" and
> add a comment explaining EFI driver is expected on !x86?

I think the "use_legacy_x86_rom" is good, I will change the name to it. 
And I will add a comment to explain it.

Thanks,
Shaoqin

> 
> Regards,
> 
> Phil.
> 

-- 
Shaoqin

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

[Philippe Mathieu-Daudé]

On 6/6/25 05:20, Shaoqin Huang wrote:
> Hi, guys
> 
> Thanks for all of your suggestions.
> 
> On 6/5/25 11:11 PM, Philippe Mathieu-Daudé wrote:
>> On 5/6/25 14:21, Gerd Hoffmann wrote:
>>>    Hi,
>>>
>>>>> 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.
>>>
>>>> Simpler to directly pass the ROM path instead of using a boolean,
>>>> so board (or CLI) could pass path to non-x86 rom.
>>>
>>> The rom is loaded into a fw_cfg file which only seabios will look at.
>>> So this rom logic is x86-specific.
>>>
>>> edk2 ships an EFI driver for ramfb, that is how ramfb is used on !x86
>>> platforms today, and I don't expect that to change.
>>>
>>> IMHO a bool is perfectly fine here, I don't think we will ever need the
>>> flexibility to specify some other rom here.
>>
>> Understood, better then! Maybe name the boolean "use_legacy_x86_rom" and
>> add a comment explaining EFI driver is expected on !x86?
> 
> I think the "use_legacy_x86_rom" is good, I will change the name to it. 
> And I will add a comment to explain it.

Maybe even better could be to only register (expose) this property for
x86 machines. We'll discuss that on your v2, no need to hold.

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

[Shaoqin Huang]

Hi Daniel,

On 6/5/25 10:40 PM, Daniel P. Berrangé wrote:
> On Thu, Jun 05, 2025 at 04:24:07PM +0200, Cédric Le Goater wrote:
>> On 6/5/25 14:21, Gerd Hoffmann wrote:
>>>     Hi,
>>>
>>>>> 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.
>>>
>>>> Simpler to directly pass the ROM path instead of using a boolean,
>>>> so board (or CLI) could pass path to non-x86 rom.
>>>
>>> The rom is loaded into a fw_cfg file which only seabios will look at.
>>> So this rom logic is x86-specific.
>>>
>>> edk2 ships an EFI driver for ramfb, that is how ramfb is used on !x86
>>> platforms today, and I don't expect that to change.
>>
>> Should we also set the vfio-pci::ramfb-romfile property to false in
>> a compat property for ARM machines then ? I don't know for RISC-V and
>> PPC.
> 
> Sounds like we'd be better setting the property to false by default,
> and then special case x86 machine types to set it to true.

I want to do that setting the property to false by default, and only the 
x86 machine types to set it to true.

But I didn't find the similar things on x86 with the arm_virt_compat[] 
in the hw/arm/virt.c which can set the compat global in one arch.

Thanks,
Shaoqin

> 
> With regards,
> Daniel

-- 
Shaoqin