Re: [Qemu-devel] [PATCH 3/9] vfio: unplug failover primary device before migration

[Alex Williamson <alex.williamson@redhat.com>]

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