| Message ID | 20231002111154.1002655-5-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ramfb: migration support | expand |
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, Acked-by: Cédric Le Goater <clg@redhat.com> Thanks, C. > --- > 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;
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> > --- > 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; I'm not a seasoned migration reviewer, so -- apologies for the churn -- I'd prefer if this patch were split in three: - First patch: ramfb_setup() update. All callers pass in constant "false". Stub updated. - Second patch: new property for vfio-pci-nohotplug, hooked up to ramfb_setup() for real. - Third patch: new property for ramfb-standalone, hooked up to ramfb_setup() for real. Without this separation, the hunks are just heaped together in the patch, and I'm having a hard time understanding the data flow. Again, apologies that I'm requesting this for a very small patch. Thanks, Laszlo
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; >
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; >> >
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; > >> > > >
On 10/2/23 21:26, Alex Williamson wrote: > 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? I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-" prefixed properties! For some reason though, machine type compat knobs are never named like that, AFAIR. > 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. I do think it matters! For one, if migration is not possible with vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch (i.e. that it makes a difference)? In that case, the ramfb_setup() call from vfio-pci-nohotplug should just open-code "false" for the "migratable" parameter. But, more importantly, I think either we're missing something about RHBZ 1859424, or that use case is just plain wrong. Gerd, any comments perhaps? Migration certainly makes sense for ramfb-standalone though. Laszlo
On Mon, 2 Oct 2023 21:41:55 +0200 Laszlo Ersek <lersek@redhat.com> wrote: > On 10/2/23 21:26, Alex Williamson wrote: > > 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? > > I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to > disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-" > prefixed properties! > > For some reason though, machine type compat knobs are never named like > that, AFAIR. Maybe I'm misunderstanding your comment, but it appears quite common to use "x-" prefix things in the compat tables... GlobalProperty hw_compat_8_0[] = { { "migration", "multifd-flush-after-each-section", "on"}, { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, { TYPE_VIRTIO_NET, "host_uso", "off"}, { TYPE_VIRTIO_NET, "guest_uso4", "off"}, { TYPE_VIRTIO_NET, "guest_uso6", "off"}, }; const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); GlobalProperty hw_compat_7_2[] = { { "e1000e", "migrate-timadj", "off" }, { "virtio-mem", "x-early-migration", "false" }, { "migration", "x-preempt-pre-7-2", "true" }, { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" }, }; const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2); [etc] > > 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. > > I do think it matters! For one, if migration is not possible with > vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch > (i.e. that it makes a difference)? In that case, the ramfb_setup() call > from vfio-pci-nohotplug should just open-code "false" for the > "migratable" parameter. Some vfio devices support migration, most don't. I was thinking ramfb_setup might be called with something like: (vdev->ramfb_migrate && vdev->enable_migration) so that at least the ramfb migration state matches the device, but I think ultimately it only saves a little bit of overhead in registering the vmstate, either one not supporting migration should block migration. Hmm, since enable_migration is auto/on/off, it seems like device realize should fail if set to 'on' and ramfb_migrate is false. I think that's the only way the device options don't become self contradictory. Thanks, Alex
On 10/2/23 22:38, Alex Williamson wrote: > On Mon, 2 Oct 2023 21:41:55 +0200 > Laszlo Ersek <lersek@redhat.com> wrote: > >> On 10/2/23 21:26, Alex Williamson wrote: >>> 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? >> >> I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to >> disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-" >> prefixed properties! >> >> For some reason though, machine type compat knobs are never named like >> that, AFAIR. > > Maybe I'm misunderstanding your comment, but it appears quite common to > use "x-" prefix things in the compat tables... You didn't misunderstand; I was wrong. I judged this off the compat prop backports to RHEL that I remembered. Your examples from the tree are good evidence. > > GlobalProperty hw_compat_8_0[] = { > { "migration", "multifd-flush-after-each-section", "on"}, > { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, > { TYPE_VIRTIO_NET, "host_uso", "off"}, > { TYPE_VIRTIO_NET, "guest_uso4", "off"}, > { TYPE_VIRTIO_NET, "guest_uso6", "off"}, > }; > const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); > > GlobalProperty hw_compat_7_2[] = { > { "e1000e", "migrate-timadj", "off" }, > { "virtio-mem", "x-early-migration", "false" }, > { "migration", "x-preempt-pre-7-2", "true" }, > { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" }, > }; > const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2); > [etc] > >>> 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. >> >> I do think it matters! For one, if migration is not possible with >> vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch >> (i.e. that it makes a difference)? In that case, the ramfb_setup() call >> from vfio-pci-nohotplug should just open-code "false" for the >> "migratable" parameter. > > Some vfio devices support migration, most don't. I was thinking > ramfb_setup might be called with something like: > > (vdev->ramfb_migrate && vdev->enable_migration) > > so that at least the ramfb migration state matches the device, but I > think ultimately it only saves a little bit of overhead in registering > the vmstate, either one not supporting migration should block migration. > > Hmm, since enable_migration is auto/on/off, it seems like device > realize should fail if set to 'on' and ramfb_migrate is false. I think > that's the only way the device options don't become self contradictory. > Thanks, ... easy-looking migration patchset becomes quite complex; isn't that the story with almost all QEMU work? :) Thanks! Laszlo
On 10/2/23 22:38, Alex Williamson wrote: > On Mon, 2 Oct 2023 21:41:55 +0200 > Laszlo Ersek <lersek@redhat.com> wrote: > >> On 10/2/23 21:26, Alex Williamson wrote: >>> 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? >> >> I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to >> disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-" >> prefixed properties! >> >> For some reason though, machine type compat knobs are never named like >> that, AFAIR. > > Maybe I'm misunderstanding your comment, but it appears quite common to > use "x-" prefix things in the compat tables... > > GlobalProperty hw_compat_8_0[] = { > { "migration", "multifd-flush-after-each-section", "on"}, > { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, > { TYPE_VIRTIO_NET, "host_uso", "off"}, > { TYPE_VIRTIO_NET, "guest_uso4", "off"}, > { TYPE_VIRTIO_NET, "guest_uso6", "off"}, > }; > const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); > > GlobalProperty hw_compat_7_2[] = { > { "e1000e", "migrate-timadj", "off" }, > { "virtio-mem", "x-early-migration", "false" }, > { "migration", "x-preempt-pre-7-2", "true" }, > { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" }, > }; > const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2); > [etc] > >>> 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. >> >> I do think it matters! For one, if migration is not possible with >> vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch >> (i.e. that it makes a difference)? In that case, the ramfb_setup() call >> from vfio-pci-nohotplug should just open-code "false" for the >> "migratable" parameter. > > Some vfio devices support migration, most don't. I was thinking > ramfb_setup might be called with something like: > > (vdev->ramfb_migrate && vdev->enable_migration) > > so that at least the ramfb migration state matches the device, but I > think ultimately it only saves a little bit of overhead in registering > the vmstate, either one not supporting migration should block migration. > > Hmm, since enable_migration is auto/on/off, it seems like device > realize should fail if set to 'on' and ramfb_migrate is false. I think > that's the only way the device options don't become self contradictory. Why isn't VFIODisplay a QOM object ? vfio_display_probe() is more or less a realize routine, and we have a reset and finalize handlers for it. (thinking aloud) the "ramfb-migrate" property could then be moved down VFIODisplay, along with the other specific display properties. Compatibility could be handled with property aliases. "enable_migration" could set "ramfb-migrate".This looks like it would be nice model cleanup. May be not the right time ? Thanks, C.
Hi On Tue, Oct 3, 2023 at 11:43 AM Cédric Le Goater <clg@redhat.com> wrote: > > On 10/2/23 22:38, Alex Williamson wrote: > > On Mon, 2 Oct 2023 21:41:55 +0200 > > Laszlo Ersek <lersek@redhat.com> wrote: > > > >> On 10/2/23 21:26, Alex Williamson wrote: > >>> 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? > >> > >> I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to > >> disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-" > >> prefixed properties! > >> > >> For some reason though, machine type compat knobs are never named like > >> that, AFAIR. > > > > Maybe I'm misunderstanding your comment, but it appears quite common to > > use "x-" prefix things in the compat tables... > > > > GlobalProperty hw_compat_8_0[] = { > > { "migration", "multifd-flush-after-each-section", "on"}, > > { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, > > { TYPE_VIRTIO_NET, "host_uso", "off"}, > > { TYPE_VIRTIO_NET, "guest_uso4", "off"}, > > { TYPE_VIRTIO_NET, "guest_uso6", "off"}, > > }; > > const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); > > > > GlobalProperty hw_compat_7_2[] = { > > { "e1000e", "migrate-timadj", "off" }, > > { "virtio-mem", "x-early-migration", "false" }, > > { "migration", "x-preempt-pre-7-2", "true" }, > > { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" }, > > }; > > const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2); > > [etc] > > > >>> 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. > >> > >> I do think it matters! For one, if migration is not possible with > >> vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch > >> (i.e. that it makes a difference)? In that case, the ramfb_setup() call > >> from vfio-pci-nohotplug should just open-code "false" for the > >> "migratable" parameter. > > > > Some vfio devices support migration, most don't. I was thinking > > ramfb_setup might be called with something like: > > > > (vdev->ramfb_migrate && vdev->enable_migration) > > > > so that at least the ramfb migration state matches the device, but I > > think ultimately it only saves a little bit of overhead in registering > > the vmstate, either one not supporting migration should block migration. > > > > Hmm, since enable_migration is auto/on/off, it seems like device > > realize should fail if set to 'on' and ramfb_migrate is false. I think > > that's the only way the device options don't become self contradictory. > > Why isn't VFIODisplay a QOM object ? vfio_display_probe() is more or > less a realize routine, and we have a reset and finalize handlers for it. > > (thinking aloud) the "ramfb-migrate" property could then be moved > down VFIODisplay, along with the other specific display properties. > Compatibility could be handled with property aliases. "enable_migration" > could set "ramfb-migrate".This looks like it would be nice model cleanup. > > May be not the right time ? Yes, I thought about some similar changes (though I am not sure QOM is necessary). Now I am trying to test my changes that add a VFIODisplay migration subsection, but I don't think I have a GVT-g GPU (TGL GT1). When I try with a random PCI device, I get "VFIO migration is not supported in kernel". I can try to comment out some code, but that seems hazardous. -- Marc-André Lureau
On 10/3/23 10:23, Marc-André Lureau wrote: > Hi > > On Tue, Oct 3, 2023 at 11:43 AM Cédric Le Goater <clg@redhat.com> wrote: >> >> On 10/2/23 22:38, Alex Williamson wrote: >>> On Mon, 2 Oct 2023 21:41:55 +0200 >>> Laszlo Ersek <lersek@redhat.com> wrote: >>> >>>> On 10/2/23 21:26, Alex Williamson wrote: >>>>> 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? >>>> >>>> I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to >>>> disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-" >>>> prefixed properties! >>>> >>>> For some reason though, machine type compat knobs are never named like >>>> that, AFAIR. >>> >>> Maybe I'm misunderstanding your comment, but it appears quite common to >>> use "x-" prefix things in the compat tables... >>> >>> GlobalProperty hw_compat_8_0[] = { >>> { "migration", "multifd-flush-after-each-section", "on"}, >>> { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, >>> { TYPE_VIRTIO_NET, "host_uso", "off"}, >>> { TYPE_VIRTIO_NET, "guest_uso4", "off"}, >>> { TYPE_VIRTIO_NET, "guest_uso6", "off"}, >>> }; >>> const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); >>> >>> GlobalProperty hw_compat_7_2[] = { >>> { "e1000e", "migrate-timadj", "off" }, >>> { "virtio-mem", "x-early-migration", "false" }, >>> { "migration", "x-preempt-pre-7-2", "true" }, >>> { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" }, >>> }; >>> const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2); >>> [etc] >>> >>>>> 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. >>>> >>>> I do think it matters! For one, if migration is not possible with >>>> vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch >>>> (i.e. that it makes a difference)? In that case, the ramfb_setup() call >>>> from vfio-pci-nohotplug should just open-code "false" for the >>>> "migratable" parameter. >>> >>> Some vfio devices support migration, most don't. I was thinking >>> ramfb_setup might be called with something like: >>> >>> (vdev->ramfb_migrate && vdev->enable_migration) >>> >>> so that at least the ramfb migration state matches the device, but I >>> think ultimately it only saves a little bit of overhead in registering >>> the vmstate, either one not supporting migration should block migration. >>> >>> Hmm, since enable_migration is auto/on/off, it seems like device >>> realize should fail if set to 'on' and ramfb_migrate is false. I think >>> that's the only way the device options don't become self contradictory. >> >> Why isn't VFIODisplay a QOM object ? vfio_display_probe() is more or >> less a realize routine, and we have a reset and finalize handlers for it. >> >> (thinking aloud) the "ramfb-migrate" property could then be moved >> down VFIODisplay, along with the other specific display properties. >> Compatibility could be handled with property aliases. "enable_migration" >> could set "ramfb-migrate".This looks like it would be nice model cleanup. >> >> May be not the right time ? > > Yes, I thought about some similar changes (though I am not sure QOM is > necessary). > > Now I am trying to test my changes that add a VFIODisplay migration > subsection, but I don't think I have a GVT-g GPU (TGL GT1). When I try > with a random PCI device, I get "VFIO migration is not supported in > kernel". I can try to comment out some code, but that seems hazardous. I have recently setup a T480 with GVT-g and a windows guest. I can give your changes a try. C.
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;