On Mon, 2 Oct 2023 20:24:11 +0200 Laszlo Ersek <ler...@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 <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. 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; > >> > > >
