| Message ID | 20231003085644.1220326-6-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | WIP: ramfb: migration support | expand |
On 10/3/23 10:56, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@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.. Thanks, C. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@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(), > }; >
Hi On Tue, Oct 3, 2023 at 2:17 PM Cédric Le Goater <clg@redhat.com> wrote: > > On 10/3/23 10:56, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@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; } > Thanks, > > C. > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@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(), > > }; > > > >
On 10/3/23 10:56, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@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.lureau@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(+) Quoting the hunks somewhat out of order (so that I can better understand the dependencies): > > 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; So this is where the complications start. The structure that contains the "RAMFBState *ramfb" field is VFIODisplay (i.e., the "dpy" field here), not VFIOPCIDevice. In order to stay consistent with the previous patch in the series, we'd have to add the flag to VFIODisplay. But it seems that VFIODisplay cannot have its own properties. Because: - properties are associated in a TypeInfo.class_init function, with device_class_set_props() - there is no TypeInfo for VFIODisplay, only for VFIOPCIDevice. To make things even more complicated, both TYPE_VFIO_PCI and TYPE_VFIO_PCI_NOHOTPLUG use VFIOPCIDevice directly. The latter only differs in the exposure of the property "ramfb". Commit b290659fc3dd ("hw/vfio/display: add ramfb support", 2018-10-15) introduced the property with TYPE_VFIO_PCI_NOHOTPLUG, *but* it squeezed the boolean field backing the property to VFIOPCIDevice, rather than creating a separate structure type *containing* VFIOPCIDevice *plus* the new field "enable_ramfb". I'm not sure if that was optimal (it may have been "unavoidable" for all I know), but either way, now we have no choice but to follow suit, and add the property's "backing field" to VFIOPCIDevice *again*. OK. I kinda convinced myself this is proper. Let's move on. > 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 > @@ -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(), > }; > This is where we expose the new knob as a property, side to side with the previously mentioned "ramfb" one. OK. > 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); > Setting the property to "off" for old machine types, OK. > @@ -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); This looks good to me from two aspects, and not so good from two other aspects. - good: seems to ensure the knob consistency that Alex highlighted - good: we enforce the "ramfb=on requires display=on" predicate too in the same function - (1) not so good: with regard to error handling, I think the placement of this new logic could be improved, IMO. If we fail here, we're still past a successful vfio_migration_realize() call, and I'm not sure if jumping to the "out_deregister" label can undo *that*. Right now, nothing after the vfio_migration_realize() call can fail inside vfio_realize(), so the error path may not be ready for rolling back vfio_migration_realize(). Looking at the larger context: if (vdev->display_xres || vdev->display_yres) { if (vdev->dpy == NULL) { error_setg(errp, "xres and yres properties require display=on"); goto out_deregister; } if (vdev->dpy->edid_regs == NULL) { error_setg(errp, "xres and yres properties need edid support"); goto out_deregister; } } if (!pdev->failover_pair_id) { if (!vfio_migration_realize(vbasedev, errp)) { goto out_deregister; } } I suggest inserting the new consistency check *between* those two checks (and not after the second one). The reason is that both existent checks jump to "out_deregister" in case of failure, which means that you can insert further checks (and actions that don't allocate new resources) between them, and jump to "out_deregister" in case of failure. - (2) not so good: we should reject an explicit "x-ramfb-migrate=on" setting if "enable_ramfb" is false. The valid (accepted) combinations are: # display ramfb x-ramfb-migrate - ------- ----- --------------- 1 off off auto 2 off off off 3 on off auto 4 on off off 5 on on auto 6 on on off 7 on on on "ramfb ==> display" is alread enforced; we need to check "(x-ramfb-migrate==on) ==> ramfb" additionally. Now the tricky part follows: > 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; > +} This doesn't look right to me. Consider again the table of valid cases (as enforced by vfio_realize() per my suggestion above): # display ramfb x-ramfb-migrate result - ------- ----- --------------- ------------------------------------- 1 off off auto no VFIODisplay object --> CRASH 2 off off off VFIODisplay migration unnecessary, OK 3 on off auto no RAMFBState object --> CRASH 4 on off off VFIODisplay migration unnecessary, OK 5 on on auto VFIODisplay migration needed, OK 6 on on off VFIODisplay migration unnecessary, OK 7 on on on VFIODisplay migration needed, OK Suggestions: - (3) name the function vfio_display_migration_needed() - the condition should be: vdev->ramfb_migrate != ON_OFF_AUTO_OFF && vdev->enable_ramfb The condition (vdev->ramfb_migrate != ON_OFF_AUTO_OFF) restricts the table to the following rows: # display ramfb x-ramfb-migrate result - ------- ----- --------------- ------------------------------------- 1 off off auto no VFIODisplay object --> CRASH 3 on off auto no RAMFBState object --> CRASH 5 on on auto VFIODisplay migration needed, OK 7 on on on VFIODisplay migration needed, OK From these, cases #5 and #7 are fine, and the additional restriction (vdev->enable_ramfb) leaves them unhurt. We need to exclude cases #1 and #3, and that's what the additional restriction (vdev->enable_ramfb) will do. - (4) *Equivalently*, you could use the *easier-to-understand* condition vdev->ramfb_migrate == ON_OFF_AUTO_ON || (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO && vdev->enable_ramfb) - (5) And then I'd suggest adding the comment (which matches the latter formulation of the condition): /* * 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). */ > + > +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 > } > }; > Seems OK. > 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 > @@ -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 */ Seems right. > 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; > +} (6) Adding this test function with constant "true" returned seems helpful, yes; how about the following impl though: 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), > + } > +}; Thanks! Laszlo
On 10/3/23 12:47, Marc-André Lureau wrote: > Hi > > On Tue, Oct 3, 2023 at 2:17 PM Cédric Le Goater <clg@redhat.com> wrote: >> >> On 10/3/23 10:56, marcandre.lureau@redhat.com wrote: >>> From: Marc-André Lureau <marcandre.lureau@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.lureau@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(), >>> }; >>> >> >> > >
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(), };