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

[Alex Williamson]

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,

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