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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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