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

[Thibaut Collet]

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)

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

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