Re: [Qemu-devel] [PATCH v3 1/2] vhost user: add support of live migration

[Michael S. Tsirkin]

On Wed, Jun 10, 2015 at 04:22:10PM +0200, Thibaut Collet wrote:
> I am not sure to understand your comments.
> 
> Could you confirm that the useless warning is:
> +        fprintf(stderr,
> +                "Warning: Guest with no VIRTIO_NET_F_GUEST_ANNOUNCE support.
> RARP must be sent by vhost-user backend\n");

Yes.

> > Can you move this out to vhost-user? It's not a generic function, is it?
> 
> Which part of the code do you think it must be moved in a specific file ?

vhost_net_inject_rarp is vhost-user specific.

> 
> On Wed, Jun 10, 2015 at 3:52 PM, Michael S. Tsirkin <address@hidden> wrote:
> 
>     On Wed, Jun 10, 2015 at 03:43:02PM +0200, Thibaut Collet wrote:
>     > Some vhost client/backend are able to support live migration.
>     > To provide this service the following features must be added:
>     > 1. GARP from guest after live migration:
>     >    the VIRTIO_NET_F_GUEST_ANNOUNCE capability is added to vhost-net when
>     netdev
>     >    backend is vhost-user to let virtio-net NIC manages GARP after
>     migration.
>     > 2. RARP on vhost-user:
>     >    After a live migration RARP are automatically sent to any interfaces.
>     A
>     >    receive callback is added to vhost-user to catch this packet. If 
> guest
>     >    supports VIRTIO_NET_F_GUEST_ANNOUNCE this packet is discarded to 
> avoid
>     >    message duplication (already done by virtio-net NIC). For legacy 
> guest
>     >    without VIRTIO_NET_F_GUEST_ANNOUNCE a warning message is displayed to
>     >    alert the user. RARP must be sent to the vhost client/backend.
>     >
>     > Signed-off-by: Thibaut Collet <address@hidden>
>     > ---
>     >  hw/net/vhost_net.c      |   15 +++++++++++++++
>     >  include/net/vhost_net.h |    1 +
>     >  net/vhost-user.c        |   21 +++++++++++++++++++--
>     >  3 files changed, 35 insertions(+), 2 deletions(-)
>     >
>     > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>     > index 426b23e..4a42325 100644
>     > --- a/hw/net/vhost_net.c
>     > +++ b/hw/net/vhost_net.c
>     > @@ -82,6 +82,8 @@ static const int user_feature_bits[] = {
>     >      VIRTIO_NET_F_CTRL_MAC_ADDR,
>     >      VIRTIO_NET_F_CTRL_GUEST_OFFLOADS,
>     >
>     > +    VIRTIO_NET_F_GUEST_ANNOUNCE,
>     > +
>     >      VIRTIO_NET_F_MQ,
>     >
>     >      VHOST_INVALID_FEATURE_BIT
>     > @@ -365,6 +367,15 @@ void vhost_net_stop(VirtIODevice *dev,
>     NetClientState *ncs,
>     >      assert(r >= 0);
>     >  }
>     >
>     > +void vhost_net_inject_rarp(struct vhost_net *net, const uint8_t *buf,
>     size_t size)
>     > +{
>     > +    if ((net->dev.acked_features & (1 << VIRTIO_NET_F_GUEST_ANNOUNCE)) 
> =
>     = 0) {
>     > +        fprintf(stderr,
>     > +                "Warning: Guest with no VIRTIO_NET_F_GUEST_ANNOUNCE
>     support. RARP must be sent by vhost-user backend\n");
>     > +        fflush(stderr);
> 
>     And maybe it does, then you are just filling log up with useless
>     warnings. Pls add some ifdef so this isn't normally compiled in.
> 
>     > +    }
> 
>     Can you move this out to vhost-user? It's not a generic function, is it?
> 
>     > +}
>     > +
>     >  void vhost_net_cleanup(struct vhost_net *net)
>     >  {
>     >      vhost_dev_cleanup(&net->dev);
>     > @@ -427,6 +438,10 @@ void vhost_net_stop(VirtIODevice *dev,
>     >  {
>     >  }
>     >
>     > +void vhost_net_inject_rarp(struct vhost_net *net, const uint8_t *buf,
>     size_t size);
>     > +{
>     > +}
>     > +
>     >  void vhost_net_cleanup(struct vhost_net *net)
>     >  {
>     >  }
>     > diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
>     > index b1c18a3..e82a0f9 100644
>     > --- a/include/net/vhost_net.h
>     > +++ b/include/net/vhost_net.h
>     > @@ -20,6 +20,7 @@ bool vhost_net_query(VHostNetState *net, VirtIODevice
>     *dev);
>     >  int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, int
>     total_queues);
>     >  void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs, int
>     total_queues);
>     >
>     > +void vhost_net_inject_rarp(VHostNetState *net, const uint8_t *buf,
>     size_t size);
>     >  void vhost_net_cleanup(VHostNetState *net);
>     >
>     >  unsigned vhost_net_get_features(VHostNetState *net, unsigned features);
>     > diff --git a/net/vhost-user.c b/net/vhost-user.c
>     > index 8d26728..69c2313 100644
>     > --- a/net/vhost-user.c
>     > +++ b/net/vhost-user.c
>     > @@ -66,6 +66,24 @@ static void vhost_user_stop(VhostUserState *s)
>     >      s->vhost_net = 0;
>     >  }
>     >
>     > +static ssize_t vhost_user_receive(NetClientState *nc, const uint8_t
>     *buf,
>     > +                                  size_t size)
>     > +{
>     > +    if (size == 60) {
>     > +        /* Assume it is a RARP request sent automatically after a
>     > +         * live migration */
>     > +        /* The RARP must be sent if guest does not support
>     > +         * VIRTIO_NET_F_GUEST_ANNOUNCE */
>     > +        VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
>     > +
>     > +        vhost_net_inject_rarp(s->vhost_net, buf, size);
>     > +    } else {
>     > +        fprintf(stderr,"Vhost user receives unexpected packets\n");
>     > +        fflush(stderr);
>     > +    }
>     > +    return size;
>     > +}
>     > +
>     >  static void vhost_user_cleanup(NetClientState *nc)
>     >  {
>     >      VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
>     > @@ -91,6 +109,7 @@ static bool vhost_user_has_ufo(NetClientState *nc)
>     >  static NetClientInfo net_vhost_user_info = {
>     >          .type = NET_CLIENT_OPTIONS_KIND_VHOST_USER,
>     >          .size = sizeof(VhostUserState),
>     > +        .receive = vhost_user_receive,
>     >          .cleanup = vhost_user_cleanup,
>     >          .has_vnet_hdr = vhost_user_has_vnet_hdr,
>     >          .has_ufo = vhost_user_has_ufo,
>     > @@ -147,8 +166,6 @@ static int net_vhost_user_init(NetClientState *peer,
>     const char *device,
>     >
>     >          s = DO_UPCAST(VhostUserState, nc, nc);
>     >
>     > -        /* We don't provide a receive callback */
>     > -        s->nc.receive_disabled = 1;
>     >          s->chr = chr;
>     >          s->nc.queue_index = i;
>     >
>     > --
>     > 1.7.10.4
> 
>