On 10/2/23 16:41, Alex Williamson wrote: > On Mon, 2 Oct 2023 15:38:10 +0200 > Cédric Le Goater <c...@redhat.com> wrote: > >> On 10/2/23 13:11, marcandre.lur...@redhat.com wrote: >>> From: Marc-André Lureau <marcandre.lur...@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.lur...@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; >> >
