Re: [PULL 3/3] hw/i386: Add the ramfb romfile compatibility

["Cédric Le Goater" <clg@redhat.com>]

On 7/28/25 12:51, Thomas Huth wrote:
> On 18/07/2025 21.43, Gerd Hoffmann wrote:
>> From: Shaoqin Huang <shahuang@redhat.com>
>>
>> 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 to disable the rom.
>>
>> This patch sets ramfb romfile option to false by default, except for x86
>> machines types (pc, q35, microvm) which need the rom file when booting
>> with seabios and machine types <= 10.0 (handling the case of arm virt,
>> for compat reasons).
>>
>> At the same time, set the "use-legacy-x86-rom" property to true on those
>> historical versioned machine types in order to avoid the memory layout
>> being changed.
>>
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
>> Message-ID: <20250717100941.2230408-4-shahuang@redhat.com>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>   hw/core/machine.c             |  2 ++
>>   hw/display/ramfb-standalone.c |  2 +-
>>   hw/i386/microvm.c             |  3 +++
>>   hw/i386/pc_piix.c             | 10 ++++++++++
>>   hw/i386/pc_q35.c              |  3 +++
>>   hw/vfio/pci.c                 |  2 +-
>>   6 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index e869821b2246..a7043e2a3425 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -39,6 +39,8 @@
>>   GlobalProperty hw_compat_10_0[] = {
>>       { "scsi-hd", "dpofua", "off" },
>> +    { "ramfb", "use-legacy-x86-rom", "true"},
>> +    { "vfio-pci", "use-legacy-x86-rom", "true" },
>>   };
>>   const size_t hw_compat_10_0_len = G_N_ELEMENTS(hw_compat_10_0);
>> diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
>> index 82d8c69f8903..72b2071aed01 100644
>> --- a/hw/display/ramfb-standalone.c
>> +++ b/hw/display/ramfb-standalone.c
>> @@ -64,7 +64,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),
>> +                     use_legacy_x86_rom, false),
>>   };
>>   static void ramfb_class_initfn(ObjectClass *klass, const void *data)
>> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
>> index e0daf0d4fc30..d90b69a16200 100644
>> --- a/hw/i386/microvm.c
>> +++ b/hw/i386/microvm.c
>> @@ -49,6 +49,7 @@
>>   #include "hw/acpi/generic_event_device.h"
>>   #include "hw/pci-host/gpex.h"
>>   #include "hw/usb/xhci.h"
>> +#include "hw/vfio/types.h"
>>   #include "elf.h"
>>   #include "kvm/kvm_i386.h"
>> @@ -633,6 +634,8 @@ GlobalProperty microvm_properties[] = {
>>        * so reserving io space is not going to work.  Turn it off.
>>        */
>>       { "pcie-root-port", "io-reserve", "0" },
>> +    { TYPE_RAMFB_DEVICE, "use-legacy-x86-rom", "true" },
>> +    { TYPE_VFIO_PCI, "use-legacy-x86-rom", "true" },
>>   };
>>   static void microvm_class_init(ObjectClass *oc, const void *data)
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index a3285fbc6450..ad5caff3a5d2 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -49,6 +49,7 @@
>>   #include "hw/i2c/smbus_eeprom.h"
>>   #include "system/memory.h"
>>   #include "hw/acpi/acpi.h"
>> +#include "hw/vfio/types.h"
>>   #include "qapi/error.h"
>>   #include "qemu/error-report.h"
>>   #include "system/xen.h"
>> @@ -77,6 +78,13 @@ static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
>>   static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
>>   #endif
>> +static GlobalProperty pc_piix_compat_defaults[] = {
>> +    { TYPE_RAMFB_DEVICE, "use-legacy-x86-rom", "true" },
>> +    { TYPE_VFIO_PCI, "use-legacy-x86-rom", "true" },
>> +};
>> +static const size_t pc_piix_compat_defaults_len =
>> +    G_N_ELEMENTS(pc_piix_compat_defaults);
>> +
>>   /*
>>    * Return the global irq number corresponding to a given device irq
>>    * pin. We could also use the bus number to have a more precise mapping.
>> @@ -492,6 +500,8 @@ static void pc_i440fx_machine_options(MachineClass *m)
>>                                      pc_set_south_bridge);
>>       object_class_property_set_description(oc, "x-south-bridge",
>>                                        "Use a different south bridge than PIIX3");
>> +    compat_props_add(m->compat_props,
>> +                     pc_piix_compat_defaults, pc_piix_compat_defaults_len);
>>   }
>>   static void pc_i440fx_machine_10_1_options(MachineClass *m)
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index cf871cfdad86..9b9519fa02d5 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/types.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 0b969b3359db..174499ecec65 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3810,7 +3810,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),
>> +                     use_legacy_x86_rom, false),
>>       DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate,
>>                               ON_OFF_AUTO_AUTO),
>>   };
> 
>   Hi guys,
> 
> there is something fishy going on now:
> 
> $ ./qemu-system-x86_64 -M q35 -device vfio-pci
> Unexpected error in object_property_find_err() at ../../home/thuth/devel/qemu/qom/object.c:1345:
> qemu-system-x86_64: -device vfio-pci: can't apply global vfio-pci.use-legacy-x86-rom=true: Property 'vfio-pci.use-legacy-x86-rom' not found
> Aborted (core dumped)
> 
> Could you please have a look?

Fix here :

  https://lore.kernel.org/qemu-devel/20250723062714.1245826-1-clg@redhat.com/

I am planning to send it with the next vfio PR. People are on vacations
last week and I still need a few Acks.

Thanks,

C.