Re: [PATCH v2 4/5] ramfb: make migration conditional

[Laszlo Ersek]

On 10/2/23 16:41, Alex Williamson wrote:
> On Mon, 2 Oct 2023 15:38:10 +0200
> Cédric Le Goater <clg@redhat.com> wrote:
> 
>> On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> RAMFB migration was unsupported until now, let's make it conditional.
>>> The following patch will prevent machines <= 8.1 to migrate it.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>  
>> Maybe localize the new 'ramfb_migrate' attribute close to 'enable_ramfb'
>> in VFIOPCIDevice. Anyhow,
> 
> Shouldn't this actually be tied to whether the device is migratable
> (which for GVT-g - the only ramfb user afaik - it's not)?  What does it
> mean to have a ramfb-migrate=true property on a device that doesn't
> support migration, or false on a device that does support migration.  I
> don't understand why this is a user controllable property.  Thanks,

The comments in <https://bugzilla.redhat.com/show_bug.cgi?id=1859424> (which 
are unfortunately not public :/ ) suggest that ramfb migration was simply 
forgotten when vGPU migration was implemented. So, "now that vGPU migration is 
done", this should be added.

Comment 8 suggests that the following domain XML snippet

    <hostdev mode='subsystem' type='mdev' managed='no' model='vfio-pci' 
display='on' ramfb='on'>
      <source>
        <address uuid='b155147a-663a-4009-ae7f-e9a96805b3ce'/>
      </source>
      <alias name='ua-b155147a-663a-4009-ae7f-e9a96805b3ce'/>
      <address type='pci' domain='0x0000' bus='0x07' slot='0x00' 
function='0x0'/>
    </hostdev>

is migratable, but the ramfb device malfunctions on the destination host.

There's also a huge QEMU cmdline in comment#0 of the bug; I've not tried to 
read that.

AIUI BTW the property is not for the user to control, it's just a compat knob 
for versioned machine types. AIUI those are usually implemented with such 
(user-visible / -tweakable) device properties.

Laszlo

> 
> Alex
> 
>>> ---
>>>   hw/vfio/pci.h                 | 1 +
>>>   include/hw/display/ramfb.h    | 2 +-
>>>   hw/display/ramfb-standalone.c | 8 +++++++-
>>>   hw/display/ramfb.c            | 6 ++++--
>>>   hw/vfio/display.c             | 4 ++--
>>>   hw/vfio/pci.c                 | 1 +
>>>   stubs/ramfb.c                 | 2 +-
>>>   7 files changed, 17 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>>> index 2d836093a8..671cc78912 100644
>>> --- a/hw/vfio/pci.h
>>> +++ b/hw/vfio/pci.h
>>> @@ -156,6 +156,7 @@ struct VFIOPCIDevice {
>>>       OnOffAuto display;
>>>       uint32_t display_xres;
>>>       uint32_t display_yres;
>>> +    bool ramfb_migrate;
>>>       int32_t bootindex;
>>>       uint32_t igd_gms;
>>>       OffAutoPCIBAR msix_relo;
>>> diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
>>> index b33a2c467b..40063b62bd 100644
>>> --- a/include/hw/display/ramfb.h
>>> +++ b/include/hw/display/ramfb.h
>>> @@ -4,7 +4,7 @@
>>>   /* ramfb.c */
>>>   typedef struct RAMFBState RAMFBState;
>>>   void ramfb_display_update(QemuConsole *con, RAMFBState *s);
>>> -RAMFBState *ramfb_setup(Error **errp);
>>> +RAMFBState *ramfb_setup(bool migrate, Error **errp);
>>>   
>>>   /* ramfb-standalone.c */
>>>   #define TYPE_RAMFB_DEVICE "ramfb"
>>> diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
>>> index 8c0094397f..6bbd69ccdf 100644
>>> --- a/hw/display/ramfb-standalone.c
>>> +++ b/hw/display/ramfb-standalone.c
>>> @@ -15,6 +15,7 @@ struct RAMFBStandaloneState {
>>>       SysBusDevice parent_obj;
>>>       QemuConsole *con;
>>>       RAMFBState *state;
>>> +    bool migrate;
>>>   };
>>>   
>>>   static void display_update_wrapper(void *dev)
>>> @@ -37,9 +38,13 @@ static void ramfb_realizefn(DeviceState *dev, Error 
>>> **errp)
>>>       RAMFBStandaloneState *ramfb = RAMFB(dev);
>>>   
>>>       ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
>>> -    ramfb->state = ramfb_setup(errp);
>>> +    ramfb->state = ramfb_setup(ramfb->migrate, errp);
>>>   }
>>>   
>>> +static Property ramfb_properties[] = {
>>> +    DEFINE_PROP_BOOL("migrate", RAMFBStandaloneState, migrate,  true),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>>   static void ramfb_class_initfn(ObjectClass *klass, void *data)
>>>   {
>>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>> @@ -48,6 +53,7 @@ static void ramfb_class_initfn(ObjectClass *klass, void 
>>> *data)
>>>       dc->realize = ramfb_realizefn;
>>>       dc->desc = "ram framebuffer standalone device";
>>>       dc->user_creatable = true;
>>> +    device_class_set_props(dc, ramfb_properties);
>>>   }
>>>   
>>>   static const TypeInfo ramfb_info = {
>>> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
>>> index 4aaaa7d653..73e08d605f 100644
>>> --- a/hw/display/ramfb.c
>>> +++ b/hw/display/ramfb.c
>>> @@ -135,7 +135,7 @@ static const VMStateDescription vmstate_ramfb = {
>>>       }
>>>   };
>>>   
>>> -RAMFBState *ramfb_setup(Error **errp)
>>> +RAMFBState *ramfb_setup(bool migrate, Error **errp)
>>>   {
>>>       FWCfgState *fw_cfg = fw_cfg_find();
>>>       RAMFBState *s;
>>> @@ -147,7 +147,9 @@ RAMFBState *ramfb_setup(Error **errp)
>>>   
>>>       s = g_new0(RAMFBState, 1);
>>>   
>>> -    vmstate_register(NULL, 0, &vmstate_ramfb, s);
>>> +    if (migrate) {
>>> +        vmstate_register(NULL, 0, &vmstate_ramfb, s);
>>> +    }
>>>       rom_add_vga("vgabios-ramfb.bin");
>>>       fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
>>>                                NULL, ramfb_fw_cfg_write, s,
>>> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
>>> index bec864f482..3f6b251ccd 100644
>>> --- a/hw/vfio/display.c
>>> +++ b/hw/vfio/display.c
>>> @@ -356,7 +356,7 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice 
>>> *vdev, Error **errp)
>>>                                             &vfio_display_dmabuf_ops,
>>>                                             vdev);
>>>       if (vdev->enable_ramfb) {
>>> -        vdev->dpy->ramfb = ramfb_setup(errp);
>>> +        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate, errp);
>>>       }
>>>       vfio_display_edid_init(vdev);
>>>       return 0;
>>> @@ -483,7 +483,7 @@ static int vfio_display_region_init(VFIOPCIDevice 
>>> *vdev, Error **errp)
>>>                                             &vfio_display_region_ops,
>>>                                             vdev);
>>>       if (vdev->enable_ramfb) {
>>> -        vdev->dpy->ramfb = ramfb_setup(errp);
>>> +        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate, errp);
>>>       }
>>>       return 0;
>>>   }
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index 3b2ca3c24c..6575b8f32d 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -3484,6 +3484,7 @@ static const TypeInfo vfio_pci_dev_info = {
>>>   
>>>   static Property vfio_pci_dev_nohotplug_properties[] = {
>>>       DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
>>> +    DEFINE_PROP_BOOL("ramfb-migrate", VFIOPCIDevice, ramfb_migrate,  true),
>>>       DEFINE_PROP_END_OF_LIST(),
>>>   };
>>>   
>>> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
>>> index 48143f3354..8869a5db09 100644
>>> --- a/stubs/ramfb.c
>>> +++ b/stubs/ramfb.c
>>> @@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>>>   {
>>>   }
>>>   
>>> -RAMFBState *ramfb_setup(Error **errp)
>>> +RAMFBState *ramfb_setup(bool migrate, Error **errp)
>>>   {
>>>       error_setg(errp, "ramfb support not available");
>>>       return NULL;  
>>
>