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

[Eric Auger <eauger@redhat.com>]

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