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

[Thibaut Collet]

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 ?

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
>
>