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

[Shaoqin Huang <shahuang@redhat.com>]

Hi Daniel,

On 6/27/25 3:15 PM, Daniel P. Berrangé wrote:
> On Fri, Jun 27, 2025 at 01:37:55PM +0800, Shaoqin Huang wrote:
>> 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?
> 
> No, the default value of the property shoudl reflect the long
> term desired behaviour - in this case 'use-legacy-x86-rom = false'.

Ok I understand your thoughts.

> 
> We must then reverse this default to 'true' for ALL historical versioned
> machine types on ALL architectures, where this device is built, or any
> specific machines where we want to keep the historical behaviour going
> forward.

But how do we implement that?

Maybe set the 'use-legacy-x86-rom = true' in hw_compat_9_2 ?

I'm not sure about that, could you give some hints about how to reverse 
the default to 'true' for ALL historical versioned machine types?

Thanks a lot.

> 
> 
> With regards,
> Daniel

-- 
Shaoqin