Hi On Tue, Oct 10, 2023 at 11:03 AM Cédric Le Goater <c...@redhat.com> wrote: > > On 10/9/23 12:19, Laszlo Ersek wrote: > > On 10/9/23 08:32, marcandre.lur...@redhat.com wrote: > >> From: Marc-André Lureau <marcandre.lur...@redhat.com> > >> > >> Add a "VFIODisplay" subsection whenever "x-ramfb-migrate" is turned on. > >> > >> Turn it off by default on machines <= 8.1 for compatibility reasons. > >> > >> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > >> Reviewed-by: Laszlo Ersek <ler...@redhat.com> > >> --- > >> hw/vfio/pci.h | 3 +++ > >> hw/core/machine.c | 1 + > >> hw/vfio/display.c | 21 +++++++++++++++++++++ > >> hw/vfio/pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > >> stubs/ramfb.c | 2 ++ > >> 5 files changed, 71 insertions(+) > >> > >> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h > >> index 2d836093a8..fd06695542 100644 > >> --- a/hw/vfio/pci.h > >> +++ b/hw/vfio/pci.h > >> @@ -173,6 +173,7 @@ struct VFIOPCIDevice { > >> bool no_kvm_ioeventfd; > >> bool no_vfio_ioeventfd; > >> bool enable_ramfb; > >> + OnOffAuto ramfb_migrate; > >> bool defer_kvm_irq_routing; > >> bool clear_parent_atomics_on_exit; > >> VFIODisplay *dpy; > >> @@ -226,4 +227,6 @@ void vfio_display_reset(VFIOPCIDevice *vdev); > >> int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp); > >> void vfio_display_finalize(VFIOPCIDevice *vdev); > >> > >> +extern const VMStateDescription vfio_display_vmstate; > >> + > >> #endif /* HW_VFIO_VFIO_PCI_H */ > >> diff --git a/hw/core/machine.c b/hw/core/machine.c > >> index 6305f2d7a4..05aef2cf9f 100644 > >> --- a/hw/core/machine.c > >> +++ b/hw/core/machine.c > >> @@ -35,6 +35,7 @@ > >> GlobalProperty hw_compat_8_1[] = { > >> { TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" }, > >> { "ramfb", "x-migrate", "off" }, > >> + { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" } > >> }; > >> const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1); > >> > >> diff --git a/hw/vfio/display.c b/hw/vfio/display.c > >> index bec864f482..2739ba56ec 100644 > >> --- a/hw/vfio/display.c > >> +++ b/hw/vfio/display.c > >> @@ -542,3 +542,24 @@ void vfio_display_finalize(VFIOPCIDevice *vdev) > >> vfio_display_edid_exit(vdev->dpy); > >> g_free(vdev->dpy); > >> } > >> + > >> +static bool migrate_needed(void *opaque) > >> +{ > >> + VFIODisplay *dpy = opaque; > >> + bool ramfb_exists = dpy->ramfb != NULL; > >> + > >> + /* see vfio_display_migration_needed() */ > >> + assert(ramfb_exists); > >> + return ramfb_exists; > >> +} > >> + > >> +const VMStateDescription vfio_display_vmstate = { > >> + .name = "VFIODisplay", > >> + .version_id = 1, > >> + .minimum_version_id = 1, > >> + .needed = migrate_needed, > >> + .fields = (VMStateField[]) { > >> + VMSTATE_STRUCT_POINTER(ramfb, VFIODisplay, ramfb_vmstate, > >> RAMFBState), > >> + VMSTATE_END_OF_LIST(), > >> + } > >> +}; > >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > >> index 3b2ca3c24c..e44ed21180 100644 > >> --- a/hw/vfio/pci.c > >> +++ b/hw/vfio/pci.c > >> @@ -2608,6 +2608,32 @@ static bool vfio_msix_present(void *opaque, int > >> version_id) > >> return msix_present(pdev); > >> } > >> > >> +static bool vfio_display_migration_needed(void *opaque) > >> +{ > >> + VFIOPCIDevice *vdev = opaque; > >> + > >> + /* > >> + * We need to migrate the VFIODisplay object if ramfb *migration* was > >> + * explicitly requested (in which case we enforced both ramfb=on and > >> + * display=on), or ramfb migration was left at the default "auto" > >> + * setting, and *ramfb* was explicitly requested (in which case we > >> + * enforced display=on). > >> + */ > >> + return vdev->ramfb_migrate == ON_OFF_AUTO_ON || > >> + (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO && vdev->enable_ramfb); > >> +} > >> + > >> +const VMStateDescription vmstate_vfio_display = { > >> + .name = "VFIOPCIDevice/VFIODisplay", > >> + .version_id = 1, > >> + .minimum_version_id = 1, > >> + .needed = vfio_display_migration_needed, > >> + .fields = (VMStateField[]){ > >> + VMSTATE_STRUCT_POINTER(dpy, VFIOPCIDevice, vfio_display_vmstate, > >> VFIODisplay), > >> + VMSTATE_END_OF_LIST() > >> + } > >> +}; > >> + > >> const VMStateDescription vmstate_vfio_pci_config = { > >> .name = "VFIOPCIDevice", > >> .version_id = 1, > >> @@ -2616,6 +2642,10 @@ const VMStateDescription vmstate_vfio_pci_config = { > >> VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice), > >> VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present), > >> VMSTATE_END_OF_LIST() > >> + }, > >> + .subsections = (const VMStateDescription*[]) { > >> + &vmstate_vfio_display, > >> + NULL > >> } > >> }; > >> > >> @@ -3271,6 +3301,19 @@ static void vfio_realize(PCIDevice *pdev, Error > >> **errp) > >> } > >> } > >> > >> + if (vdev->ramfb_migrate == ON_OFF_AUTO_ON && !vdev->enable_ramfb) { > >> + warn_report("x-ramfb-migrate=on but ramfb=off"); > >> + vdev->ramfb_migrate = ON_OFF_AUTO_OFF; > > > > the warning could give a hint about the resultant action taken (i.e., > > forcing off x-ramfb-migrate), but don't repost just for that; it's a nit. > > yes. > > How about : > > warn_report("x-ramfb-migrate=on but ramfb=off. Forcing > x-ramfb-migrate to off.");
Sure, that's better. > > I can amend the patch if you agree. thanks!