Re: [Qemu-devel] [RFC v2 08/32] postcopy: Add vhost-user flag for postcopy and check it

[Dr. David Alan Gilbert]

* Peter Xu (address@hidden) wrote:
> On Thu, Aug 24, 2017 at 08:27:06PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > Add a vhost feature flag for postcopy support, and
> > use the postcopy notifier to check it before allowing postcopy.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  contrib/libvhost-user/libvhost-user.h |  1 +
> >  docs/interop/vhost-user.txt           | 10 +++++++++
> >  hw/virtio/vhost-user.c                | 40 
> > ++++++++++++++++++++++++++++++++++-
> >  3 files changed, 50 insertions(+), 1 deletion(-)
> > 
> > diff --git a/contrib/libvhost-user/libvhost-user.h 
> > b/contrib/libvhost-user/libvhost-user.h
> > index acd019876d..95d0d34a28 100644
> > --- a/contrib/libvhost-user/libvhost-user.h
> > +++ b/contrib/libvhost-user/libvhost-user.h
> > @@ -34,6 +34,7 @@ enum VhostUserProtocolFeature {
> >      VHOST_USER_PROTOCOL_F_MQ = 0,
> >      VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
> >      VHOST_USER_PROTOCOL_F_RARP = 2,
> > +    VHOST_USER_PROTOCOL_F_PAGEFAULT = 7,
> >  
> >      VHOST_USER_PROTOCOL_F_MAX
> >  };
> > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> > index 954771d0d8..a279560eb0 100644
> > --- a/docs/interop/vhost-user.txt
> > +++ b/docs/interop/vhost-user.txt
> > @@ -273,6 +273,15 @@ Once the source has finished migration, rings will be 
> > stopped by
> >  the source. No further update must be done before rings are
> >  restarted.
> >  
> > +In postcopy migration the slave is started before all the memory has been
> > +received from the source host, and care must be taken to avoid accessing 
> > pages
> > +that have yet to be received.  The slave opens a 'userfault'-fd and 
> > registers
> > +the memory with it; this fd is then passed back over to the master.
> > +The master services requests on the userfaultfd for pages that are accessed
> > +and when the page is available it performs WAKE ioctl's on the userfaultfd
> > +to wake the stalled slave.  The client indicates support for this via the
> > +VHOST_USER_PROTOCOL_F_PAGEFAULT feature.
> > +
> >  IOMMU support
> >  -------------
> >  
> > @@ -327,6 +336,7 @@ Protocol features
> >  #define VHOST_USER_PROTOCOL_F_MTU            4
> >  #define VHOST_USER_PROTOCOL_F_SLAVE_REQ      5
> >  #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN   6
> > +#define VHOST_USER_PROTOCOL_F_PAGEFAULT      7
> >  
> >  Master message types
> >  --------------------
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 093675ed98..c51bbd1296 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -17,6 +17,8 @@
> >  #include "sysemu/kvm.h"
> >  #include "qemu/error-report.h"
> >  #include "qemu/sockets.h"
> > +#include "migration/migration.h"
> > +#include "migration/postcopy-ram.h"
> >  
> >  #include <sys/ioctl.h>
> >  #include <sys/socket.h>
> > @@ -34,7 +36,7 @@ enum VhostUserProtocolFeature {
> >      VHOST_USER_PROTOCOL_F_NET_MTU = 4,
> >      VHOST_USER_PROTOCOL_F_SLAVE_REQ = 5,
> >      VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6,
> > -
> > +    VHOST_USER_PROTOCOL_F_PAGEFAULT = 7,
> >      VHOST_USER_PROTOCOL_F_MAX
> >  };
> >  
> > @@ -123,8 +125,10 @@ static VhostUserMsg m __attribute__ ((unused));
> >  #define VHOST_USER_VERSION    (0x1)
> >  
> >  struct vhost_user {
> > +    struct vhost_dev *dev;
> >      CharBackend *chr;
> >      int slave_fd;
> > +    NotifierWithReturn postcopy_notifier;
> >  };
> >  
> >  static bool ioeventfd_enabled(void)
> > @@ -720,6 +724,33 @@ out:
> >      return ret;
> >  }
> >  
> > +static int vhost_user_postcopy_notifier(NotifierWithReturn *notifier,
> > +                                        void *opaque)
> > +{
> > +    struct PostcopyNotifyData *pnd = opaque;
> > +    struct vhost_user *u = container_of(notifier, struct vhost_user,
> > +                                         postcopy_notifier);
> > +    struct vhost_dev *dev = u->dev;
> > +
> > +    switch (pnd->reason) {
> > +    case POSTCOPY_NOTIFY_PROBE:
> > +        if (!virtio_has_feature(dev->protocol_features,
> > +                                VHOST_USER_PROTOCOL_F_PAGEFAULT)) {
> > +            /* TODO: Get the device name into this error somehow */
> > +            error_setg(pnd->errp,
> > +                       "vhost-user backend not capable of postcopy");
> > +            return -ENOENT;
> > +        }
> > +        break;
> > +
> > +    default:
> > +        /* We ignore notifications we don't know */
> > +        break;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static int vhost_user_init(struct vhost_dev *dev, void *opaque)
> >  {
> >      uint64_t features, protocol_features;
> > @@ -731,6 +762,7 @@ static int vhost_user_init(struct vhost_dev *dev, void 
> > *opaque)
> >      u = g_new0(struct vhost_user, 1);
> >      u->chr = opaque;
> >      u->slave_fd = -1;
> > +    u->dev = dev;
> >      dev->opaque = u;
> >  
> >      err = vhost_user_get_features(dev, &features);
> > @@ -787,6 +819,9 @@ static int vhost_user_init(struct vhost_dev *dev, void 
> > *opaque)
> >          return err;
> >      }
> >  
> > +    u->postcopy_notifier.notify = vhost_user_postcopy_notifier;
> > +    postcopy_add_notifier(&u->postcopy_notifier);
> > +
> >      return 0;
> >  }
> >  
> > @@ -797,6 +832,9 @@ static int vhost_user_cleanup(struct vhost_dev *dev)
> >      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> >  
> >      u = dev->opaque;
> > +    if (u->postcopy_notifier.notify) {
> 
> Detecting init using the notify hook is slightly strange here for
> me... If so, not sure whether we also need:
> 
>            u->postcopy_notifier.notify = NULL;
> 
> Or I'm not sure whether a 2nd call to vhost_user_cleanup() can be
> dangerous since postcopy_remove_notifier() will be called twice.

I've added the NULL assignment; I think I'd assumed that the notifier
remove also did that.

Dave

> 
> Besides that, the patch looks good to me.  Thanks,
> 
> > +        postcopy_remove_notifier(&u->postcopy_notifier);
> > +    }
> >      if (u->slave_fd >= 0) {
> >          qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
> >          close(u->slave_fd);
> > -- 
> > 2.13.5
> > 
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK