Re: [Qemu-devel] [PATCH v3 2/2] vhost user: Add RARP injection for legacy guest

[Michael S. Tsirkin]

On Wed, Jun 10, 2015 at 10:25:57PM +0200, Thibaut Collet wrote:
> Yes backend can save everything to be able to send the rarp alone after a live
> migration.
> Main purpose of this patch is to answer to the point raise by Jason on the
> previous version of my patch:
> > Yes, your patch works well for recent drivers. But the problem is legacy
> > guest/driver without VIRTIO_NET_F_GUEST_ANNOUNCE. In this case there
> > will be no GARP sent after migration.
> 
> If Jason is OK with this solution this patch can be forgotten.
> 
> Maybe, to warn user of this issue if the backend does not send the rarp, it 
> can
> be useful to keep the warn message of the previous patch:
> > +        fprintf(stderr,
> > +                "Warning: Guest with no VIRTIO_NET_F_GUEST_ANNOUNCE 
> > support.
> RARP must be sent by vhost-user backend\n");
> > +        fflush(stderr);
> with a static bool to display this message only one time to limit the number 
> of
> message ?

I don't see why it's necessary.

> On Wed, Jun 10, 2015 at 6:00 PM, Michael S. Tsirkin <address@hidden> wrote:
> 
>     On Wed, Jun 10, 2015 at 05:48:47PM +0200, Thibaut Collet wrote:
>     > I have involved QEMU because QEMU prepares the rarp. I agree that 
> backend
>     has
>     > probably all the information to do that.
>     > But backend does not know if the guest supports
>     the VIRTIO_NET_F_GUEST_ANNOUNCE
> 
>     Why not?  Backend has the acked feature mask.
> 
> 
>     > and will send a useless rarp.
>     > Maybe this duplication of requests is not very important and in this 
> case
>     this
>     > patch is not mandatory.
>     >
>     > On Wed, Jun 10, 2015 at 5:34 PM, Michael S. Tsirkin <address@hidden>
>     wrote:
>     >
>     >     On Wed, Jun 10, 2015 at 03:43:03PM +0200, Thibaut Collet wrote:
>     >     > In case of live migration with legacy guest (without
>     >     VIRTIO_NET_F_GUEST_ANNOUNCE)
>     >     > a message is added between QEMU and the vhost client/backend.
>     >     > This message provides the RARP content, prepared by QEMU, to the
>     vhost
>     >     > client/backend.
>     >     > The vhost client/backend is responsible to send the RARP.
>     >     >
>     >     > Signed-off-by: Thibaut Collet <address@hidden>
>     >     > ---
>     >     >  docs/specs/vhost-user.txt   |   16 ++++++++++++++++
>     >     >  hw/net/vhost_net.c          |    8 ++++++++
>     >     >  hw/virtio/vhost-user.c      |   11 ++++++++++-
>     >     >  linux-headers/linux/vhost.h |    9 +++++++++
>     >     >  4 files changed, 43 insertions(+), 1 deletion(-)
>     >     >
>     >     > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
>     >     > index 2c8e934..ef5d475 100644
>     >     > --- a/docs/specs/vhost-user.txt
>     >     > +++ b/docs/specs/vhost-user.txt
>     >     > @@ -97,6 +97,7 @@ typedef struct VhostUserMsg {
>     >     >          uint64_t u64;
>     >     >          struct vhost_vring_state state;
>     >     >          struct vhost_vring_addr addr;
>     >     > +        struct vhost_inject_rarp rarp;
>     >     >          VhostUserMemory memory;
>     >     >      };
>     >     >  } QEMU_PACKED VhostUserMsg;
>     >     > @@ -132,6 +133,12 @@ Multi queue support
>     >     >  The protocol supports multiple queues by setting all index fields
>     in the
>     >     sent
>     >     >  messages to a properly calculated value.
>     >     >
>     >     > +Live migration support
>     >     > +----------------------
>     >     > +The protocol supports live migration. GARP from the migrated 
> guest
>     is
>     >     done
>     >     > +through the VIRTIO_NET_F_GUEST_ANNOUNCE mechanism for guest that
>     >     supports it or
>     >     > +through RARP.
>     >     > +
>     >     >  Message types
>     >     >  -------------
>     >     >
>     >     > @@ -269,3 +276,12 @@ Message types
>     >     >        Bits (0-7) of the payload contain the vring index. Bit 8 is
>     the
>     >     >        invalid FD flag. This flag is set when there is no file
>     descriptor
>     >     >        in the ancillary data.
>     >     > +
>     >     > + * VHOST_USER_NET_INJECT_RARP
>     >     > +
>     >     > +      Id: 15
>     >     > +      Master payload: rarp content
>     >     > +
>     >     > +      Provide the RARP message to send to the guest after a live
>     >     migration. This
>     >     > +      message is sent only for guest that does not support
>     >     > +      VIRTIO_NET_F_GUEST_ANNOUNCE.
>     >
>     >     I don't see why this is needed.
>     >     Can't backend simply send rarp itself?
>     >     Why do we need to involve QEMU?
>     >
>     >
>     >
>     >     > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>     >     > index 4a42325..f66d48d 100644
>     >     > --- a/hw/net/vhost_net.c
>     >     > +++ b/hw/net/vhost_net.c
>     >     > @@ -369,10 +369,18 @@ void vhost_net_stop(VirtIODevice *dev,
>     >     NetClientState *ncs,
>     >     >
>     >     >  void vhost_net_inject_rarp(struct vhost_net *net, const uint8_t
>     *buf,
>     >     size_t size)
>     >     >  {
>     >     > +    struct vhost_inject_rarp inject_rarp;
>     >     > +    memcpy(&inject_rarp.rarp, buf, size);
>     >     > +
>     >     >      if ((net->dev.acked_features & (1 <<
>     VIRTIO_NET_F_GUEST_ANNOUNCE)) =
>     >     = 0) {
>     >     > +        const VhostOps *vhost_ops = net->dev.vhost_ops;
>     >     > +        int r;
>     >     > +
>     >     >          fprintf(stderr,
>     >     >                  "Warning: Guest with no
>     VIRTIO_NET_F_GUEST_ANNOUNCE
>     >     support. RARP must be sent by vhost-user backend\n");
>     >     >          fflush(stderr);
>     >     > +        r = vhost_ops->vhost_call(&net->dev,
>     VHOST_NET_INJECT_RARP, &
>     >     inject_rarp);
>     >     > +        assert(r >= 0);
>     >     >      }
>     >     >  }
>     >     >
>     >     > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>     >     > index d6f2163..2e752ab 100644
>     >     > --- a/hw/virtio/vhost-user.c
>     >     > +++ b/hw/virtio/vhost-user.c
>     >     > @@ -41,6 +41,7 @@ typedef enum VhostUserRequest {
>     >     >      VHOST_USER_SET_VRING_KICK = 12,
>     >     >      VHOST_USER_SET_VRING_CALL = 13,
>     >     >      VHOST_USER_SET_VRING_ERR = 14,
>     >     > +    VHOST_USER_NET_INJECT_RARP = 15,
>     >     >      VHOST_USER_MAX
>     >     >  } VhostUserRequest;
>     >     >
>     >     > @@ -70,6 +71,7 @@ typedef struct VhostUserMsg {
>     >     >          uint64_t u64;
>     >     >          struct vhost_vring_state state;
>     >     >          struct vhost_vring_addr addr;
>     >     > +        struct vhost_inject_rarp rarp;
>     >     >          VhostUserMemory memory;
>     >     >      };
>     >     >  } QEMU_PACKED VhostUserMsg;
>     >     > @@ -104,7 +106,8 @@ static unsigned long int
>     ioctl_to_vhost_user_request
>     >     [VHOST_USER_MAX] = {
>     >     >      VHOST_GET_VRING_BASE,   /* VHOST_USER_GET_VRING_BASE */
>     >     >      VHOST_SET_VRING_KICK,   /* VHOST_USER_SET_VRING_KICK */
>     >     >      VHOST_SET_VRING_CALL,   /* VHOST_USER_SET_VRING_CALL */
>     >     > -    VHOST_SET_VRING_ERR     /* VHOST_USER_SET_VRING_ERR */
>     >     > +    VHOST_SET_VRING_ERR,    /* VHOST_USER_SET_VRING_ERR */
>     >     > +    VHOST_NET_INJECT_RARP   /* VHOST_USER_NET_INJECT_RARP */
>     >     >  };
>     >     >
>     >     >  static VhostUserRequest vhost_user_request_translate(unsigned 
> long
>     int
>     >     request)
>     >     > @@ -287,6 +290,12 @@ static int vhost_user_call(struct vhost_dev
>     *dev,
>     >     unsigned long int request,
>     >     >              msg.u64 |= VHOST_USER_VRING_NOFD_MASK;
>     >     >          }
>     >     >          break;
>     >     > +
>     >     > +    case VHOST_NET_INJECT_RARP:
>     >     > +        memcpy(&msg.rarp, arg, sizeof(struct vhost_inject_rarp));
>     >     > +        msg.size = sizeof(struct vhost_inject_rarp);
>     >     > +        break;
>     >     > +
>     >     >      default:
>     >     >          error_report("vhost-user trying to send unhandled 
> ioctl");
>     >     >          return -1;
>     >     > diff --git a/linux-headers/linux/vhost.h b/linux-headers/linux/
>     vhost.h
>     >     > index c656f61..1920134 100644
>     >     > --- a/linux-headers/linux/vhost.h
>     >     > +++ b/linux-headers/linux/vhost.h
>     >     > @@ -63,6 +63,10 @@ struct vhost_memory {
>     >     >       struct vhost_memory_region regions[0];
>     >     >  };
>     >     >
>     >     > +struct vhost_inject_rarp {
>     >     > +     __u8 rarp[60];
>     >     > +};
>     >     > +
>     >     >  /* ioctls */
>     >     >
>     >     >  #define VHOST_VIRTIO 0xAF
>     >     > @@ -121,6 +125,11 @@ struct vhost_memory {
>     >     >   * device.  This can be used to stop the ring (e.g. for
>     migration). */
>     >     >  #define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct
>     >     vhost_vring_file)
>     >     >
>     >     > +/* Inject a RARP in case of live migration for guest that does 
> not
>     >     support
>     >     > + * VIRTIO_NET_F_GUEST_ANNOUNCE */
>     >     > +#define VHOST_NET_INJECT_RARP _IOW(VHOST_VIRTIO, 0x31, struct
>     >     vhost_inject_rarp)
>     >     > +
>     >     > +
>     >     >  /* Feature bits */
>     >     >  /* Log all write descriptors. Can be changed while device is
>     active. */
>     >     >  #define VHOST_F_LOG_ALL 26
>     >     > --
>     >     > 1.7.10.4
>     >
>     >
> 
>