Re: [PATCH v3 10/10] vfio: unplug failover primary device before migration

[Jens Freimann]

On Tue, Oct 15, 2019 at 07:52:12PM -0600, Alex Williamson wrote:
> On Fri, 11 Oct 2019 13:20:15 +0200
> Jens Freimann <address@hidden> 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 <address@hidden>
> > ---
> >  hw/vfio/pci.c | 35 ++++++++++++++++++++++++++++++++++-
> >  hw/vfio/pci.h |  2 ++
> >  2 files changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index c5e6fe61cb..64cf8e07d9 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)
> > @@ -2698,6 +2701,12 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice 
> > *vdev)
> >      vdev->req_enabled = false;
> >  }
> >
> > +static int has_net_failover_arg(void *opaque, const char *name,
> > +                           const char *value, Error **errp)
> > +{
> > +    return (strcmp(name, "net_failover_pair_id") == 0);
> > +}
> > +
> >  static void vfio_realize(PCIDevice *pdev, Error **errp)
> >  {
> >      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> > @@ -2710,6 +2719,20 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> >      int groupid;
> >      int i, ret;
> >      bool is_mdev;
> > +    uint16_t class_id;
> > +
> > +    if (qemu_opt_foreach(pdev->qdev.opts, has_net_failover_arg,
> > +                         (void *) pdev->qdev.opts, &err) == 0) {
>
> Why do we need a qemu_opt_foreach here versus testing
> vdev->net_failover_pair_id as you do below or similar to how we test
> sysfsdev immediately below this chunk?

We don't need it, I will change it and move it to where we check for
the PCI class.
> > +        error_setg(&vdev->migration_blocker,
> > +                "VFIO device doesn't support migration");
> > +        ret = migrate_add_blocker(vdev->migration_blocker, &err);
>
> Where's the migrate_del_blocker()/error_free() for any other realize
> error or device removal?
>
> > +        if (err) {
> > +            error_propagate(errp, err);
> > +            error_free(vdev->migration_blocker);
> > +        }
>
> As Connie noted, unclear if this aborts or continues without a
> migration blocker, which would be bad.

It aborts in my test. PCI realize propagates it further and eventually
it leads to aborting qemu.

It looks like this now:

    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);
         } else {
             error_propagate(errp, vdev->migration_blocker);
         }
         goto error;
     } else {
         pdev->qdev.allow_unplug_during_migration = true;
     }

> > +    } else {
> > +        pdev->qdev.allow_unplug_during_migration = true;
> > +    }
> >
> >      if (!vdev->vbasedev.sysfsdev) {
> >          if (!(~vdev->host.domain || ~vdev->host.bus ||
> > @@ -2812,6 +2835,14 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> >          goto error;
> >      }
> >
> > +    if (vdev->net_failover_pair_id != NULL) {
> > +        class_id = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> > +        if (class_id != PCI_CLASS_NETWORK_ETHERNET) {
> > +            error_setg(errp, "failover device is not an Ethernet device");
> > +            goto error;
> > +        }
> > +    }
>
> Not clear to me why we do this separate from setting up the migration
> blocker or why we use a different mechanism to test for the property.

I'm moving this check to hw/pci/pci.c as you suggested.

> > +
> >      /* vfio emulates a lot for us, but some bits need extra love */
> >      vdev->emulated_config_bits = g_malloc0(vdev->config_size);
> >
> > @@ -3110,6 +3141,8 @@ static Property vfio_pci_dev_properties[] = {
> >                              display, ON_OFF_AUTO_OFF),
> >      DEFINE_PROP_UINT32("xres", VFIOPCIDevice, display_xres, 0),
> >      DEFINE_PROP_UINT32("yres", VFIOPCIDevice, display_yres, 0),
> > +    DEFINE_PROP_STRING("net_failover_pair_id", VFIOPCIDevice,
> > +            net_failover_pair_id),
>
> Should this and the Ethernet class test be done in PCIDevice?  The
> migration aspect is the only thing unique to vfio since we don't
> otherwise support it, right?  For instance, I should be able to
> setup an emulated NIC with this failover pair id too, right?  Thanks,

Yes, we can do it in PCIDevice. Using it with an emulated device.
It wouldn't make sense for production but could make sense for
testing purposes.

Thanks for the review!

regards,
Jens