Re: [Qemu-devel] [PATCH v2] net: Add support of VIRTIO_NET_F_GUEST_ANNOUNCE for vhost-net/vhost-user

[Michael S. Tsirkin]

On Mon, Jun 08, 2015 at 01:29:39PM +0200, Thibaut Collet wrote:
> Hi,
> 
> > I don't think qemu_send_packet_raw can ever work for vhost user.
> > What happens if you merely add VIRTIO_NET_F_GUEST_ANNOUNCE
> > to the feature list, and drop the rest of the patch?
> 
> If you merely add VIRTIO_NET_F_GUEST_ANNOUNCE you have the GARP with recent
> guest after a live migration.
> The rest of the patch (can be set in a separate commit) avoid a qemu crashes
> with live migration when vhost-user is present:
>  - When a live migration migration is complete a RARP is automatically send to
> any net backend (self announce mechanism)
>  - vhost user does not provide any receive function and RARP request is stored
> in the vhost-user queue
>  - When a migration back is done all the net backend queues are purged. The
> stored RARP request for vhost-user is then sent to the register receive
> callback of vhost-user that is NULL.
> 
> Support of live migration for vhost user needs the whole patch. (and maore if
> we want support legacy guest with no support of VIRTIO_NET_F_GUEST_ANNOUNCE)

How about implementing a receive function that discards all packets
then?

> 
> > I don't get it.  Why do you need any extra messages for old drivers?  To
> > detect old drivers, simply have backend check whether
> > VIRTIO_NET_F_GUEST_ANNOUNCE is set.
> 
> For old driver we can not use the mechanism implemented in virtio-net that
> manages VIRTIO_NET_F_GUEST_ANNOUNCE. In this case we must send the RARP on the
> network interface created by vhost-user.
> My first idea to do that was to add a message between QEMU and vhost client/
> backend (vapp or other) to let the vhost client/backend send the RARP.
> But maybe it will be easier to let QEMU send directly the RARP message on the
> network interface created by vhost user.
> This point can be done later if it is needed.
> 
> Regards.

Can't vhost-user backend sends this automatically?
Why do we need to do anything in QEMU?

> 
> On Mon, Jun 8, 2015 at 12:12 PM, Michael S. Tsirkin <address@hidden> wrote:
> 
>     On Fri, Jun 05, 2015 at 03:24:12PM +0200, Thibaut Collet wrote:
>     > Add VIRTIO_NET_F_GUEST_ANNOUNCE capability to vhost-net when netdev
>     backend is
>     > vhost-user.
>     >
>     > For netdev backend using virtio-net NIC the self announce is managed
>     directly
>     > by the virtio-net NIC and not by the netdev backend itself.
>     > To avoid duplication of announces (once from the guest and once from
>     QEMU) a
>     > bitfield is added in the NetClientState structure.
>     > If this bit is set self announce does not send message to the guest to
>     request
>     > gratuitous ARP but let virtio-net NIC set the VIRTIO_NET_S_ANNOUNCE for
>     > gratuitous ARP.
>     >
>     > Signed-off-by: Thibaut Collet <address@hidden>
>     > ---
>     > v2: do not discard anymore packets send to vhost-user: it is GARP 
> request
>     after
>     >     live migration.
>     >     As suggested by S. Hajnoczi qemu_announce_self skips virtio-net NIC
>     that
>     >     already send GARP.
>     >
>     >  hw/net/vhost_net.c |    2 ++
>     >  include/net/net.h  |    1 +
>     >  net/vhost-user.c   |    2 ++
>     >  savevm.c           |   11 ++++++++---
>     >  4 files changed, 13 insertions(+), 3 deletions(-)
>     >
>     > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>     > index 426b23e..a745f97 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
>     > diff --git a/include/net/net.h b/include/net/net.h
>     > index e66ca03..a78e9df 100644
>     > --- a/include/net/net.h
>     > +++ b/include/net/net.h
>     > @@ -85,6 +85,7 @@ struct NetClientState {
>     >      char *name;
>     >      char info_str[256];
>     >      unsigned receive_disabled : 1;
>     > +    unsigned self_announce_disabled : 1;
>     >      NetClientDestructor *destructor;
>     >      unsigned int queue_index;
>     >      unsigned rxfilter_notify_enabled:1;
>     > diff --git a/net/vhost-user.c b/net/vhost-user.c
>     > index 8d26728..b345446 100644
>     > --- a/net/vhost-user.c
>     > +++ b/net/vhost-user.c
>     > @@ -147,6 +147,8 @@ static int net_vhost_user_init(NetClientState *peer,
>     const char *device,
>     >
>     >          s = DO_UPCAST(VhostUserState, nc, nc);
>     >
>     > +        /* Self announce is managed directly by virtio-net NIC */
>     > +        s->nc.self_announce_disabled = 1;
>     >          /* We don't provide a receive callback */
>     >          s->nc.receive_disabled = 1;
>     >          s->chr = chr;
>     > diff --git a/savevm.c b/savevm.c
>     > index 3b0e222..7a134b1 100644
>     > --- a/savevm.c
>     > +++ b/savevm.c
>     > @@ -80,11 +80,16 @@ static void qemu_announce_self_iter(NICState *nic,
>     void *opaque)
>     >  {
>     >      uint8_t buf[60];
>     >      int len;
>     > +    NetClientState *nc;
>     >
>     > -    
> trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
>     > -    len = announce_self_create(buf, nic->conf->macaddr.a);
>     > +    nc = qemu_get_queue(nic);
>     >
>     > -    qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
>     > +    if (!nc->peer->self_announce_disabled) {
>     > +        trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->
>     macaddr));
>     > +        len = announce_self_create(buf, nic->conf->macaddr.a);
>     > +
>     > +        qemu_send_packet_raw(nc, buf, len);
>     > +    }
>     >  }
>     >
> 
>     I don't think qemu_send_packet_raw can ever work for vhost user.
>     What happens if you merely add VIRTIO_NET_F_GUEST_ANNOUNCE
>     to the feature list, and drop the rest of the patch?
>    
> 
>     > --
>     > 1.7.10.4
> 
>