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

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 2 Oct 2023 20:24:11 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> 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.

If it's not for user control it's unfortunate that we expose it to the
user at all, but should it at least use the "x-" prefix to indicate that
it's not intended to be an API?  It's still odd to think that we can
have scenarios of a non-migratable vfio device registering a migratable
ramfb, and vice versa, but I suppose in the end it doesn't matter.
Thanks,

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;    
> >>  
> >   
>