On 10/3/23 12:47, Marc-André Lureau wrote: > Hi > > On Tue, Oct 3, 2023 at 2:17 PM Cédric Le Goater <c...@redhat.com> wrote: >> >> On 10/3/23 10:56, 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. >> >> >> This change breaks linking on various platforms with : >> >> /usr/bin/ld: >> libqemu-xtensa-softmmu.fa.p/hw_vfio_display.c.o:(.data.rel+0x50): undefined >> reference to `ramfb_vmstate' >> >> Some stubs updates are missing it seems.. >> > > diff --git a/stubs/ramfb.c b/stubs/ramfb.c > index 48143f3354..cf64733b10 100644 > --- a/stubs/ramfb.c > +++ b/stubs/ramfb.c > @@ -2,6 +2,8 @@ > #include "qapi/error.h" > #include "hw/display/ramfb.h" > > +const VMStateDescription ramfb_vmstate = {}; > + > > > And I think we should also change the "needed" condition to: > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 4689f2e5c1..b327844764 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2613,7 +2613,7 @@ static bool vfio_display_needed(void *opaque) > VFIOPCIDevice *vdev = opaque; > > /* the only thing that justifies the VFIODisplay sub-section atm */ > - return vdev->ramfb_migrate != ON_OFF_AUTO_OFF; > + return vdev->enable_ramfb && vdev->ramfb_migrate != ON_OFF_AUTO_OFF; > } >
Exactly, this was one of my comments in review. (But, see there -- I think the other formulation is easier to read and understand.) Laszlo > > >> Thanks, >> >> C. >> >>> >>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> >>> --- >>> hw/vfio/pci.h | 3 +++ >>> hw/core/machine.c | 1 + >>> hw/vfio/display.c | 23 +++++++++++++++++++++++ >>> hw/vfio/pci.c | 32 ++++++++++++++++++++++++++++++++ >>> 4 files changed, 59 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 47a07d1d9b..f2f8940a85 100644 >>> --- a/hw/core/machine.c >>> +++ b/hw/core/machine.c >>> @@ -32,6 +32,7 @@ >>> >>> GlobalProperty hw_compat_8_1[] = { >>> { "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..de5bf71dd1 100644 >>> --- a/hw/vfio/display.c >>> +++ b/hw/vfio/display.c >>> @@ -542,3 +542,26 @@ void vfio_display_finalize(VFIOPCIDevice *vdev) >>> vfio_display_edid_exit(vdev->dpy); >>> g_free(vdev->dpy); >>> } >>> + >>> +static bool migrate_needed(void *opaque) >>> +{ >>> + /* >>> + * If we are here, it's because vfio_display_needed(), which is only >>> true >>> + * when dpy->ramfb_migrate atm. >>> + * >>> + * If the migration condition is changed, we should check here if >>> + * ramfb_migrate is true. (this will need a way to lookup the >>> associated >>> + * VFIOPCIDevice somehow, or fields to be moved, ..) >>> + */ >>> + return true; >>> +} >>> + >>> +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), >>> + } >>> +}; >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>> index 3b2ca3c24c..4689f2e5c1 100644 >>> --- a/hw/vfio/pci.c >>> +++ b/hw/vfio/pci.c >>> @@ -2608,6 +2608,25 @@ static bool vfio_msix_present(void *opaque, int >>> version_id) >>> return msix_present(pdev); >>> } >>> >>> +static bool vfio_display_needed(void *opaque) >>> +{ >>> + VFIOPCIDevice *vdev = opaque; >>> + >>> + /* the only thing that justifies the VFIODisplay sub-section atm */ >>> + return vdev->ramfb_migrate != ON_OFF_AUTO_OFF; >>> +} >>> + >>> +const VMStateDescription vmstate_vfio_display = { >>> + .name = "VFIOPCIDevice/VFIODisplay", >>> + .version_id = 1, >>> + .minimum_version_id = 1, >>> + .needed = vfio_display_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 +2635,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 >>> } >>> }; >>> >>> @@ -3275,6 +3298,14 @@ static void vfio_realize(PCIDevice *pdev, Error >>> **errp) >>> if (!vfio_migration_realize(vbasedev, errp)) { >>> goto out_deregister; >>> } >>> + if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) { >>> + if (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO) { >>> + vdev->ramfb_migrate = ON_OFF_AUTO_OFF; >>> + } else if (vdev->ramfb_migrate == ON_OFF_AUTO_ON) { >>> + error_setg(errp, "x-ramfb-migrate requires migration"); >>> + goto out_deregister; >>> + } >>> + } >>> } >>> >>> vfio_register_err_notifier(vdev); >>> @@ -3484,6 +3515,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_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, >>> ramfb_migrate, ON_OFF_AUTO_AUTO), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> >> >> > >