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

[Shaoqin Huang <shahuang@redhat.com>]

Hi Eric,

On 6/26/25 4:01 PM, Eric Auger wrote:
> 
> 
> 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.

If so. Why don't we still keep the "use-legacy-x86-rom" default to true, 
and only set it to false to those arch which doesn't need it just like 
my original implementation.

Because I don't really know how other arch's memoery layout was impacted 
by this property set to false. I think keep their original behavior and 
only change it on arm64 is a good idea.

How do you think about it?

Thanks,
Shaoqin

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

-- 
Shaoqin