| Message ID | 20191018202040.30349-12-jfreimann@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | add failover feature for assigned network devices | expand |
On Fri, 18 Oct 2019 22:20:40 +0200 Jens Freimann <jfreimann@redhat.com> wrote: > As usual block all vfio-pci devices from being migrated, but make an > exception for failover primary devices. This is achieved by setting > unmigratable to 0 but also add a migration blocker for all vfio-pci > devices except failover primary devices. These will be unplugged before > migration happens by the migration handler of the corresponding > virtio-net standby device. > > Signed-off-by: Jens Freimann <jfreimann@redhat.com> > --- > hw/vfio/pci.c | 31 +++++++++++++++++++++++++------ > hw/vfio/pci.h | 1 + > 2 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 12fac39804..a15b83c6b6 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -40,6 +40,9 @@ > #include "pci.h" > #include "trace.h" > #include "qapi/error.h" > +#include "migration/blocker.h" > +#include "qemu/option.h" > +#include "qemu/option_int.h" > > #define TYPE_VFIO_PCI "vfio-pci" > #define PCI_VFIO(obj) OBJECT_CHECK(VFIOPCIDevice, obj, TYPE_VFIO_PCI) > @@ -2712,12 +2715,26 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > int i, ret; > bool is_mdev; > > + if (!pdev->net_failover_pair_id) { > + error_setg(&vdev->migration_blocker, > + "VFIO device doesn't support migration"); > + ret = migrate_add_blocker(vdev->migration_blocker, &err); > + if (err) { > + error_propagate(errp, err); > + goto error; This looks wrong, you haven't set up vbasedev.name yet. > + } > + } else { > + pdev->qdev.allow_unplug_during_migration = true; > + } > + > if (!vdev->vbasedev.sysfsdev) { > if (!(~vdev->host.domain || ~vdev->host.bus || > ~vdev->host.slot || ~vdev->host.function)) { > error_setg(errp, "No provided host device"); > error_append_hint(errp, "Use -device vfio-pci,host=DDDD:BB:DD.F " > "or -device vfio-pci,sysfsdev=PATH_TO_DEVICE\n"); > + migrate_del_blocker(vdev->migration_blocker); > + error_free(vdev->migration_blocker); > return; > } > vdev->vbasedev.sysfsdev = > @@ -2729,6 +2746,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > if (stat(vdev->vbasedev.sysfsdev, &st) < 0) { > error_setg_errno(errp, errno, "no such host device"); > error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.sysfsdev); > + migrate_del_blocker(vdev->migration_blocker); > + error_free(vdev->migration_blocker); > return; > } Might be a bit easier cleanup-wise if you set up the blocker resp. allow unplug during migration only here. The only difference is that you'll get a different error message when trying to set up a non-failover device with invalid specs on a migratable-only setup. > > @@ -3008,6 +3027,8 @@ out_teardown: > vfio_bars_exit(vdev); > error: > error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name); > + migrate_del_blocker(vdev->migration_blocker); > + error_free(vdev->migration_blocker); Shouldn't you check whether migration_block has been set up, like in the finalize routine? > } > > static void vfio_instance_finalize(Object *obj) > @@ -3019,6 +3040,10 @@ static void vfio_instance_finalize(Object *obj) > vfio_bars_finalize(vdev); > g_free(vdev->emulated_config_bits); > g_free(vdev->rom); > + if (vdev->migration_blocker) { > + migrate_del_blocker(vdev->migration_blocker); > + error_free(vdev->migration_blocker); > + } > /* > * XXX Leaking igd_opregion is not an oversight, we can't remove the > * fw_cfg entry therefore leaking this allocation seems like the safest
On Mon, Oct 21, 2019 at 03:45:04PM +0200, Cornelia Huck wrote: >On Fri, 18 Oct 2019 22:20:40 +0200 >Jens Freimann <jfreimann@redhat.com> wrote: > >> As usual block all vfio-pci devices from being migrated, but make an >> exception for failover primary devices. This is achieved by setting >> unmigratable to 0 but also add a migration blocker for all vfio-pci >> devices except failover primary devices. These will be unplugged before >> migration happens by the migration handler of the corresponding >> virtio-net standby device. >> >> Signed-off-by: Jens Freimann <jfreimann@redhat.com> >> --- >> hw/vfio/pci.c | 31 +++++++++++++++++++++++++------ >> hw/vfio/pci.h | 1 + >> 2 files changed, 26 insertions(+), 6 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 12fac39804..a15b83c6b6 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -40,6 +40,9 @@ >> #include "pci.h" >> #include "trace.h" >> #include "qapi/error.h" >> +#include "migration/blocker.h" >> +#include "qemu/option.h" >> +#include "qemu/option_int.h" >> >> #define TYPE_VFIO_PCI "vfio-pci" >> #define PCI_VFIO(obj) OBJECT_CHECK(VFIOPCIDevice, obj, TYPE_VFIO_PCI) >> @@ -2712,12 +2715,26 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> int i, ret; >> bool is_mdev; >> >> + if (!pdev->net_failover_pair_id) { >> + error_setg(&vdev->migration_blocker, >> + "VFIO device doesn't support migration"); >> + ret = migrate_add_blocker(vdev->migration_blocker, &err); >> + if (err) { >> + error_propagate(errp, err); >> + goto error; > >This looks wrong, you haven't set up vbasedev.name yet. You're right. >> + } >> + } else { >> + pdev->qdev.allow_unplug_during_migration = true; >> + } >> + >> if (!vdev->vbasedev.sysfsdev) { >> if (!(~vdev->host.domain || ~vdev->host.bus || >> ~vdev->host.slot || ~vdev->host.function)) { >> error_setg(errp, "No provided host device"); >> error_append_hint(errp, "Use -device vfio-pci,host=DDDD:BB:DD.F " >> "or -device vfio-pci,sysfsdev=PATH_TO_DEVICE\n"); >> + migrate_del_blocker(vdev->migration_blocker); >> + error_free(vdev->migration_blocker); >> return; >> } >> vdev->vbasedev.sysfsdev = >> @@ -2729,6 +2746,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> if (stat(vdev->vbasedev.sysfsdev, &st) < 0) { >> error_setg_errno(errp, errno, "no such host device"); >> error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.sysfsdev); >> + migrate_del_blocker(vdev->migration_blocker); >> + error_free(vdev->migration_blocker); >> return; >> } > >Might be a bit easier cleanup-wise if you set up the blocker resp. >allow unplug during migration only here. The only difference is that >you'll get a different error message when trying to set up a >non-failover device with invalid specs on a migratable-only setup. Yes, so I moved it to this place now. This saves me cleanup up the migration blocker in the above two cases. I don't jump to error if adding the migration blocker failed, I just have to free the blocker Error and can return. > >> >> @@ -3008,6 +3027,8 @@ out_teardown: >> vfio_bars_exit(vdev); >> error: >> error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name); >> + migrate_del_blocker(vdev->migration_blocker); >> + error_free(vdev->migration_blocker); > >Shouldn't you check whether migration_block has been set up, like in >the finalize routine? yes, added the same check here. Thanks Conny! regards, Jens
On Fri, 18 Oct 2019 22:20:40 +0200 Jens Freimann <jfreimann@redhat.com> wrote: > As usual block all vfio-pci devices from being migrated, but make an > exception for failover primary devices. This is achieved by setting > unmigratable to 0 but also add a migration blocker for all vfio-pci > devices except failover primary devices. These will be unplugged before > migration happens by the migration handler of the corresponding > virtio-net standby device. > > Signed-off-by: Jens Freimann <jfreimann@redhat.com> > --- > hw/vfio/pci.c | 31 +++++++++++++++++++++++++------ > hw/vfio/pci.h | 1 + > 2 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 12fac39804..a15b83c6b6 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -40,6 +40,9 @@ > #include "pci.h" > #include "trace.h" > #include "qapi/error.h" > +#include "migration/blocker.h" > +#include "qemu/option.h" > +#include "qemu/option_int.h" > > #define TYPE_VFIO_PCI "vfio-pci" > #define PCI_VFIO(obj) OBJECT_CHECK(VFIOPCIDevice, obj, TYPE_VFIO_PCI) > @@ -2712,12 +2715,26 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > int i, ret; > bool is_mdev; > > + if (!pdev->net_failover_pair_id) { > + error_setg(&vdev->migration_blocker, > + "VFIO device doesn't support migration"); > + ret = migrate_add_blocker(vdev->migration_blocker, &err); > + if (err) { > + error_propagate(errp, err); > + goto error; > + } > + } else { > + pdev->qdev.allow_unplug_during_migration = true; Why is this unique to vfio-pci, shouldn't any device set as the primary for a failover allow unplug during migration, and therefore should it be done in the core code rather than device driver? With the net_failover_pair_id set in PCIDevice, I should be able to test with an e1000e NIC as primary, but this suggests they wouldn't be handled identically elsewhere. Thanks, Alex > + } > + > if (!vdev->vbasedev.sysfsdev) { > if (!(~vdev->host.domain || ~vdev->host.bus || > ~vdev->host.slot || ~vdev->host.function)) { > error_setg(errp, "No provided host device"); > error_append_hint(errp, "Use -device vfio-pci,host=DDDD:BB:DD.F " > "or -device vfio-pci,sysfsdev=PATH_TO_DEVICE\n"); > + migrate_del_blocker(vdev->migration_blocker); > + error_free(vdev->migration_blocker); > return; > } > vdev->vbasedev.sysfsdev = > @@ -2729,6 +2746,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > if (stat(vdev->vbasedev.sysfsdev, &st) < 0) { > error_setg_errno(errp, errno, "no such host device"); > error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.sysfsdev); > + migrate_del_blocker(vdev->migration_blocker); > + error_free(vdev->migration_blocker); > return; > } > > @@ -3008,6 +3027,8 @@ out_teardown: > vfio_bars_exit(vdev); > error: > error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name); > + migrate_del_blocker(vdev->migration_blocker); > + error_free(vdev->migration_blocker); > } > > static void vfio_instance_finalize(Object *obj) > @@ -3019,6 +3040,10 @@ static void vfio_instance_finalize(Object *obj) > vfio_bars_finalize(vdev); > g_free(vdev->emulated_config_bits); > g_free(vdev->rom); > + if (vdev->migration_blocker) { > + migrate_del_blocker(vdev->migration_blocker); > + error_free(vdev->migration_blocker); > + } > /* > * XXX Leaking igd_opregion is not an oversight, we can't remove the > * fw_cfg entry therefore leaking this allocation seems like the safest > @@ -3151,11 +3176,6 @@ static Property vfio_pci_dev_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > -static const VMStateDescription vfio_pci_vmstate = { > - .name = "vfio-pci", > - .unmigratable = 1, > -}; > - > static void vfio_pci_dev_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -3163,7 +3183,6 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data) > > dc->reset = vfio_pci_reset; > dc->props = vfio_pci_dev_properties; > - dc->vmsd = &vfio_pci_vmstate; > dc->desc = "VFIO-based PCI device assignment"; > set_bit(DEVICE_CATEGORY_MISC, dc->categories); > pdc->realize = vfio_realize; > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h > index 834a90d646..b329d50338 100644 > --- a/hw/vfio/pci.h > +++ b/hw/vfio/pci.h > @@ -168,6 +168,7 @@ typedef struct VFIOPCIDevice { > bool no_vfio_ioeventfd; > bool enable_ramfb; > VFIODisplay *dpy; > + Error *migration_blocker; > } VFIOPCIDevice; > > uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
On Mon, Oct 21, 2019 at 09:19:20AM -0600, Alex Williamson wrote: >On Fri, 18 Oct 2019 22:20:40 +0200 >Jens Freimann <jfreimann@redhat.com> wrote: [...] >> + if (!pdev->net_failover_pair_id) { >> + error_setg(&vdev->migration_blocker, >> + "VFIO device doesn't support migration"); >> + ret = migrate_add_blocker(vdev->migration_blocker, &err); >> + if (err) { >> + error_propagate(errp, err); >> + goto error; >> + } >> + } else { >> + pdev->qdev.allow_unplug_during_migration = true; > >Why is this unique to vfio-pci, shouldn't any device set as the primary >for a failover allow unplug during migration, and therefore should it >be done in the core code rather than device driver? With the >net_failover_pair_id set in PCIDevice, I should be able to test with an >e1000e NIC as primary, but this suggests they wouldn't be handled >identically elsewhere. Thanks, I assume you're talking about pci core code not qdev core. Failover is pci specific at this time (only the part to hide devices is more generic and is in qdev code). I can set the flag to allow migration in pci_qdev_realize(). regards, Jens
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 12fac39804..a15b83c6b6 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -40,6 +40,9 @@ #include "pci.h" #include "trace.h" #include "qapi/error.h" +#include "migration/blocker.h" +#include "qemu/option.h" +#include "qemu/option_int.h" #define TYPE_VFIO_PCI "vfio-pci" #define PCI_VFIO(obj) OBJECT_CHECK(VFIOPCIDevice, obj, TYPE_VFIO_PCI) @@ -2712,12 +2715,26 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) int i, ret; bool is_mdev; + if (!pdev->net_failover_pair_id) { + error_setg(&vdev->migration_blocker, + "VFIO device doesn't support migration"); + ret = migrate_add_blocker(vdev->migration_blocker, &err); + if (err) { + error_propagate(errp, err); + goto error; + } + } else { + pdev->qdev.allow_unplug_during_migration = true; + } + if (!vdev->vbasedev.sysfsdev) { if (!(~vdev->host.domain || ~vdev->host.bus || ~vdev->host.slot || ~vdev->host.function)) { error_setg(errp, "No provided host device"); error_append_hint(errp, "Use -device vfio-pci,host=DDDD:BB:DD.F " "or -device vfio-pci,sysfsdev=PATH_TO_DEVICE\n"); + migrate_del_blocker(vdev->migration_blocker); + error_free(vdev->migration_blocker); return; } vdev->vbasedev.sysfsdev = @@ -2729,6 +2746,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) if (stat(vdev->vbasedev.sysfsdev, &st) < 0) { error_setg_errno(errp, errno, "no such host device"); error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.sysfsdev); + migrate_del_blocker(vdev->migration_blocker); + error_free(vdev->migration_blocker); return; } @@ -3008,6 +3027,8 @@ out_teardown: vfio_bars_exit(vdev); error: error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name); + migrate_del_blocker(vdev->migration_blocker); + error_free(vdev->migration_blocker); } static void vfio_instance_finalize(Object *obj) @@ -3019,6 +3040,10 @@ static void vfio_instance_finalize(Object *obj) vfio_bars_finalize(vdev); g_free(vdev->emulated_config_bits); g_free(vdev->rom); + if (vdev->migration_blocker) { + migrate_del_blocker(vdev->migration_blocker); + error_free(vdev->migration_blocker); + } /* * XXX Leaking igd_opregion is not an oversight, we can't remove the * fw_cfg entry therefore leaking this allocation seems like the safest @@ -3151,11 +3176,6 @@ static Property vfio_pci_dev_properties[] = { DEFINE_PROP_END_OF_LIST(), }; -static const VMStateDescription vfio_pci_vmstate = { - .name = "vfio-pci", - .unmigratable = 1, -}; - static void vfio_pci_dev_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -3163,7 +3183,6 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data) dc->reset = vfio_pci_reset; dc->props = vfio_pci_dev_properties; - dc->vmsd = &vfio_pci_vmstate; dc->desc = "VFIO-based PCI device assignment"; set_bit(DEVICE_CATEGORY_MISC, dc->categories); pdc->realize = vfio_realize; diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index 834a90d646..b329d50338 100644 --- a/hw/vfio/pci.h +++ b/hw/vfio/pci.h @@ -168,6 +168,7 @@ typedef struct VFIOPCIDevice { bool no_vfio_ioeventfd; bool enable_ramfb; VFIODisplay *dpy; + Error *migration_blocker; } VFIOPCIDevice; uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
As usual block all vfio-pci devices from being migrated, but make an exception for failover primary devices. This is achieved by setting unmigratable to 0 but also add a migration blocker for all vfio-pci devices except failover primary devices. These will be unplugged before migration happens by the migration handler of the corresponding virtio-net standby device. Signed-off-by: Jens Freimann <jfreimann@redhat.com> --- hw/vfio/pci.c | 31 +++++++++++++++++++++++++------ hw/vfio/pci.h | 1 + 2 files changed, 26 insertions(+), 6 deletions(-)