| Message ID | 20190802150605.5880-4-jfreimann@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | add failover feature for assigned network devices | expand |
On Fri, 2 Aug 2019 17:05:59 +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 | 24 +++++++++++++++++++++++- > hw/vfio/pci.h | 1 + > 2 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index d6ae9bd4ac..398d26669b 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -35,6 +35,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) > @@ -2693,6 +2696,12 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) > vdev->req_enabled = false; > } > > +static int has_standby_arg(void *opaque, const char *name, > + const char *value, Error **errp) > +{ > + return strcmp(name, "standby") == 0; > +} > + > static void vfio_realize(PCIDevice *pdev, Error **errp) > { > VFIOPCIDevice *vdev = PCI_VFIO(pdev); > @@ -2706,6 +2715,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > int i, ret; > bool is_mdev; > > + if (qemu_opt_foreach(pdev->qdev.opts, has_standby_arg, > + (void *) pdev->qdev.opts, &err) == 0) { > + 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); > + error_free(vdev->migration_blocker); > + } > + } else { > + pdev->qdev.allow_unplug_during_migration = true; I think you add this only in the next patch? > + } > + > if (!vdev->vbasedev.sysfsdev) { > if (!(~vdev->host.domain || ~vdev->host.bus || > ~vdev->host.slot || ~vdev->host.function)) { > @@ -3148,7 +3170,7 @@ static Property vfio_pci_dev_properties[] = { > > static const VMStateDescription vfio_pci_vmstate = { > .name = "vfio-pci", > - .unmigratable = 1, > + .unmigratable = 0, > }; > > static void vfio_pci_dev_class_init(ObjectClass *klass, void *data) > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h > index 27d58fc55b..0f6f8cb395 100644 > --- a/hw/vfio/pci.h > +++ b/hw/vfio/pci.h > @@ -169,6 +169,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); This patch interacts with support for vfio migration (last posted in <1562665760-26158-1-git-send-email-kwankhede@nvidia.com>, I've not seen a later version yet.) With that, we'd have three cases to consider: 1) device is a failover primary 2) device has a migration region 3) none of the above Can 1) and 2) happen simultaneously? If yes, what should take precedence?
On Mon, 12 Aug 2019 17:18:54 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Fri, 2 Aug 2019 17:05:59 +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 | 24 +++++++++++++++++++++++- > > hw/vfio/pci.h | 1 + > > 2 files changed, 24 insertions(+), 1 deletion(-) > > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > > index d6ae9bd4ac..398d26669b 100644 > > --- a/hw/vfio/pci.c > > +++ b/hw/vfio/pci.c > > @@ -35,6 +35,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) > > @@ -2693,6 +2696,12 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) > > vdev->req_enabled = false; > > } > > > > +static int has_standby_arg(void *opaque, const char *name, > > + const char *value, Error **errp) > > +{ > > + return strcmp(name, "standby") == 0; > > +} > > + > > static void vfio_realize(PCIDevice *pdev, Error **errp) > > { > > VFIOPCIDevice *vdev = PCI_VFIO(pdev); > > @@ -2706,6 +2715,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > > int i, ret; > > bool is_mdev; > > > > + if (qemu_opt_foreach(pdev->qdev.opts, has_standby_arg, > > + (void *) pdev->qdev.opts, &err) == 0) { > > + 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); > > + error_free(vdev->migration_blocker); > > + } > > + } else { > > + pdev->qdev.allow_unplug_during_migration = true; > > I think you add this only in the next patch? > > > + } > > + > > if (!vdev->vbasedev.sysfsdev) { > > if (!(~vdev->host.domain || ~vdev->host.bus || > > ~vdev->host.slot || ~vdev->host.function)) { > > @@ -3148,7 +3170,7 @@ static Property vfio_pci_dev_properties[] = { > > > > static const VMStateDescription vfio_pci_vmstate = { > > .name = "vfio-pci", > > - .unmigratable = 1, > > + .unmigratable = 0, > > }; > > > > static void vfio_pci_dev_class_init(ObjectClass *klass, void *data) > > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h > > index 27d58fc55b..0f6f8cb395 100644 > > --- a/hw/vfio/pci.h > > +++ b/hw/vfio/pci.h > > @@ -169,6 +169,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); > > This patch interacts with support for vfio migration (last posted in > <1562665760-26158-1-git-send-email-kwankhede@nvidia.com>, I've not seen > a later version yet.) > > With that, we'd have three cases to consider: > 1) device is a failover primary > 2) device has a migration region > 3) none of the above > > Can 1) and 2) happen simultaneously? If yes, what should take > precedence? Great questions. I would assume that a user specifying this option intends the behavior here regardless of the device's support for migration, which could be made more clear and easier to test by adding this option to other, otherwise migratable, QEMU NICs. Also, I thought we agreed that "standby" was not a sufficiently descriptive name for this device option and that this option would be rejected with an error on non-Ethernet class devices[1]. Thanks, Alex [1] https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg04727.html
On Mon, Aug 12, 2019 at 03:22:52PM -0600, Alex Williamson wrote: >On Mon, 12 Aug 2019 17:18:54 +0200 >Cornelia Huck <cohuck@redhat.com> wrote: > >> On Fri, 2 Aug 2019 17:05:59 +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 | 24 +++++++++++++++++++++++- >> > hw/vfio/pci.h | 1 + >> > 2 files changed, 24 insertions(+), 1 deletion(-) >> > >> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> > index d6ae9bd4ac..398d26669b 100644 >> > --- a/hw/vfio/pci.c >> > +++ b/hw/vfio/pci.c >> > @@ -35,6 +35,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) >> > @@ -2693,6 +2696,12 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) >> > vdev->req_enabled = false; >> > } >> > >> > +static int has_standby_arg(void *opaque, const char *name, >> > + const char *value, Error **errp) >> > +{ >> > + return strcmp(name, "standby") == 0; >> > +} >> > + >> > static void vfio_realize(PCIDevice *pdev, Error **errp) >> > { >> > VFIOPCIDevice *vdev = PCI_VFIO(pdev); >> > @@ -2706,6 +2715,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> > int i, ret; >> > bool is_mdev; >> > >> > + if (qemu_opt_foreach(pdev->qdev.opts, has_standby_arg, >> > + (void *) pdev->qdev.opts, &err) == 0) { >> > + 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); >> > + error_free(vdev->migration_blocker); >> > + } >> > + } else { >> > + pdev->qdev.allow_unplug_during_migration = true; >> >> I think you add this only in the next patch? >> >> > + } >> > + >> > if (!vdev->vbasedev.sysfsdev) { >> > if (!(~vdev->host.domain || ~vdev->host.bus || >> > ~vdev->host.slot || ~vdev->host.function)) { >> > @@ -3148,7 +3170,7 @@ static Property vfio_pci_dev_properties[] = { >> > >> > static const VMStateDescription vfio_pci_vmstate = { >> > .name = "vfio-pci", >> > - .unmigratable = 1, >> > + .unmigratable = 0, >> > }; >> > >> > static void vfio_pci_dev_class_init(ObjectClass *klass, void *data) >> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h >> > index 27d58fc55b..0f6f8cb395 100644 >> > --- a/hw/vfio/pci.h >> > +++ b/hw/vfio/pci.h >> > @@ -169,6 +169,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); >> >> This patch interacts with support for vfio migration (last posted in >> <1562665760-26158-1-git-send-email-kwankhede@nvidia.com>, I've not seen >> a later version yet.) >> >> With that, we'd have three cases to consider: >> 1) device is a failover primary >> 2) device has a migration region >> 3) none of the above >> >> Can 1) and 2) happen simultaneously? If yes, what should take >> precedence? > >Great questions. I would assume that a user specifying this option >intends the behavior here regardless of the device's support for >migration, which could be made more clear and easier to test by adding >this option to other, otherwise migratable, QEMU NICs. I agree and think it makes sense that if a user intentionally marks a device as a primary device of a failover pair then it should override the use of an existing migration region of the device. > >Also, I thought we agreed that "standby" was not a sufficiently >descriptive name for this device option and that this option would be >rejected with an error on non-Ethernet class devices[1]. Thanks, We did agree on that, sorry. Will fix in next version. regards, Jens
On Mon, Aug 12, 2019 at 05:18:54PM +0200, Cornelia Huck wrote: >On Fri, 2 Aug 2019 17:05:59 +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 | 24 +++++++++++++++++++++++- >> hw/vfio/pci.h | 1 + >> 2 files changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index d6ae9bd4ac..398d26669b 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -35,6 +35,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) >> @@ -2693,6 +2696,12 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) >> vdev->req_enabled = false; >> } >> >> +static int has_standby_arg(void *opaque, const char *name, >> + const char *value, Error **errp) >> +{ >> + return strcmp(name, "standby") == 0; >> +} >> + >> static void vfio_realize(PCIDevice *pdev, Error **errp) >> { >> VFIOPCIDevice *vdev = PCI_VFIO(pdev); >> @@ -2706,6 +2715,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> int i, ret; >> bool is_mdev; >> >> + if (qemu_opt_foreach(pdev->qdev.opts, has_standby_arg, >> + (void *) pdev->qdev.opts, &err) == 0) { >> + 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); >> + error_free(vdev->migration_blocker); >> + } >> + } else { >> + pdev->qdev.allow_unplug_during_migration = true; > >I think you add this only in the next patch? you're right, will fix >> + } >> + >> if (!vdev->vbasedev.sysfsdev) { >> if (!(~vdev->host.domain || ~vdev->host.bus || >> ~vdev->host.slot || ~vdev->host.function)) { >> @@ -3148,7 +3170,7 @@ static Property vfio_pci_dev_properties[] = { >> >> static const VMStateDescription vfio_pci_vmstate = { >> .name = "vfio-pci", >> - .unmigratable = 1, >> + .unmigratable = 0, >> }; >> >> static void vfio_pci_dev_class_init(ObjectClass *klass, void *data) >> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h >> index 27d58fc55b..0f6f8cb395 100644 >> --- a/hw/vfio/pci.h >> +++ b/hw/vfio/pci.h >> @@ -169,6 +169,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); > >This patch interacts with support for vfio migration (last posted in ><1562665760-26158-1-git-send-email-kwankhede@nvidia.com>, I've not seen >a later version yet.) > >With that, we'd have three cases to consider: >1) device is a failover primary >2) device has a migration region >3) none of the above > >Can 1) and 2) happen simultaneously? If yes, what should take >precedence? See my reply to Alex. If I understand it right nothing needs to be explicitly enabled for the migration region by the user. So if a user explicitly sets the failover parameters for the devie I think it makes sense that it should take precedence. regards, Jens
On Tue, 13 Aug 2019 08:45:49 +0200 Jens Freimann <jfreimann@redhat.com> wrote: > On Mon, Aug 12, 2019 at 03:22:52PM -0600, Alex Williamson wrote: > >On Mon, 12 Aug 2019 17:18:54 +0200 > >Cornelia Huck <cohuck@redhat.com> wrote: > > > >> On Fri, 2 Aug 2019 17:05:59 +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 | 24 +++++++++++++++++++++++- > >> > hw/vfio/pci.h | 1 + > >> > 2 files changed, 24 insertions(+), 1 deletion(-) > >> This patch interacts with support for vfio migration (last posted in > >> <1562665760-26158-1-git-send-email-kwankhede@nvidia.com>, I've not seen > >> a later version yet.) > >> > >> With that, we'd have three cases to consider: > >> 1) device is a failover primary > >> 2) device has a migration region > >> 3) none of the above > >> > >> Can 1) and 2) happen simultaneously? If yes, what should take > >> precedence? > > > >Great questions. I would assume that a user specifying this option > >intends the behavior here regardless of the device's support for > >migration, which could be made more clear and easier to test by adding > >this option to other, otherwise migratable, QEMU NICs. > > I agree and think it makes sense that if a user intentionally marks a > device as a primary device of a failover pair then it should override > the use of an existing migration region of the device. Yes, that makes sense to me as well.
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index d6ae9bd4ac..398d26669b 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -35,6 +35,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) @@ -2693,6 +2696,12 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) vdev->req_enabled = false; } +static int has_standby_arg(void *opaque, const char *name, + const char *value, Error **errp) +{ + return strcmp(name, "standby") == 0; +} + static void vfio_realize(PCIDevice *pdev, Error **errp) { VFIOPCIDevice *vdev = PCI_VFIO(pdev); @@ -2706,6 +2715,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) int i, ret; bool is_mdev; + if (qemu_opt_foreach(pdev->qdev.opts, has_standby_arg, + (void *) pdev->qdev.opts, &err) == 0) { + 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); + error_free(vdev->migration_blocker); + } + } 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)) { @@ -3148,7 +3170,7 @@ static Property vfio_pci_dev_properties[] = { static const VMStateDescription vfio_pci_vmstate = { .name = "vfio-pci", - .unmigratable = 1, + .unmigratable = 0, }; static void vfio_pci_dev_class_init(ObjectClass *klass, void *data) diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index 27d58fc55b..0f6f8cb395 100644 --- a/hw/vfio/pci.h +++ b/hw/vfio/pci.h @@ -169,6 +169,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 | 24 +++++++++++++++++++++++- hw/vfio/pci.h | 1 + 2 files changed, 24 insertions(+), 1 deletion(-)