From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42447) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z1wQb-0001ji-30 for qemu-devel@nongnu.org; Mon, 08 Jun 2015 08:45:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z1wQV-0005xm-On for qemu-devel@nongnu.org; Mon, 08 Jun 2015 08:45:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43829) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z1wQV-0005xF-ID for qemu-devel@nongnu.org; Mon, 08 Jun 2015 08:45:15 -0400 Date: Mon, 8 Jun 2015 14:45:11 +0200 From: "Michael S. Tsirkin" Message-ID: <20150608144326-mutt-send-email-mst@redhat.com> References: <55701BED.2090802@redhat.com> <1433510652-13866-1-git-send-email-thibaut.collet@6wind.com> <20150608121022-mutt-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] net: Add support of VIRTIO_NET_F_GUEST_ANNOUNCE for vhost-net/vhost-user List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thibaut Collet Cc: Jason Wang , qemu-devel@nongnu.org, Stefan Hajnoczi , quintela@redhat.com On Mon, Jun 08, 2015 at 01:29:39PM +0200, Thibaut Collet wrote: > Hi, >=20 > > 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? >=20 > If you merely=A0add 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 cr= ashes > with live migration when vhost-user is present: > =A0- When a live migration migration is complete a RARP is automaticall= y send to > any net backend (self announce mechanism) > =A0- vhost user does not provide any receive function and RARP request = is stored > in the vhost-user queue > =A0- When a migration back is done all the net backend queues are purge= d. The > stored RARP request for vhost-user is then sent to the register receive > callback of vhost-user that is NULL. >=20 > Support of live migration for vhost user needs the whole patch. (and ma= ore if > we want support legacy guest with no support of=A0VIRTIO_NET_F_GUEST_AN= NOUNCE) How about implementing a receive function that discards all packets then? >=20 > > I don't get it.=A0 Why do you need any extra messages for old drivers= ?=A0 To > > detect old drivers, simply have backend check whether > > VIRTIO_NET_F_GUEST_ANNOUNCE is set. >=20 > For old driver we can not use the mechanism implemented in virtio-net t= hat > manages=A0VIRTIO_NET_F_GUEST_ANNOUNCE. In this case we must send the RA= RP on the > network interface created by vhost-user. > My first idea to do that was to add a message between QEMU and=A0vhost = 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. >=20 > Regards. Can't vhost-user backend sends this automatically? Why do we need to do anything in QEMU? >=20 > On Mon, Jun 8, 2015 at 12:12 PM, Michael S. Tsirkin wr= ote: >=20 > On Fri, Jun 05, 2015 at 03:24:12PM +0200, Thibaut Collet wrote: > > Add VIRTIO_NET_F_GUEST_ANNOUNCE capability to vhost-net when netd= ev > backend is > > vhost-user. > > > > For netdev backend using virtio-net NIC the self announce is mana= ged > directly > > by the virtio-net NIC and not by the netdev backend itself. > > To avoid duplication of announces (once from the guest and once f= rom > QEMU) a > > bitfield is added in the NetClientState structure. > > If this bit is set self announce does not send message to the gue= st to > request > > gratuitous ARP but let virtio-net NIC set the VIRTIO_NET_S_ANNOUN= CE for > > gratuitous ARP. > > > > Signed-off-by: Thibaut Collet > > --- > > v2: do not discard anymore packets send to vhost-user: it is GARP= request > after > >=A0 =A0 =A0live migration. > >=A0 =A0 =A0As suggested by S. Hajnoczi qemu_announce_self skips vi= rtio-net NIC > that > >=A0 =A0 =A0already send GARP. > > > >=A0 hw/net/vhost_net.c |=A0 =A0 2 ++ > >=A0 include/net/net.h=A0 |=A0 =A0 1 + > >=A0 net/vhost-user.c=A0 =A0|=A0 =A0 2 ++ > >=A0 savevm.c=A0 =A0 =A0 =A0 =A0 =A0|=A0 =A011 ++++++++--- > >=A0 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[] =3D { > >=A0 =A0 =A0 VIRTIO_NET_F_CTRL_MAC_ADDR, > >=A0 =A0 =A0 VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, > > > > +=A0 =A0 VIRTIO_NET_F_GUEST_ANNOUNCE, > > + > >=A0 =A0 =A0 VIRTIO_NET_F_MQ, > > > >=A0 =A0 =A0 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 { > >=A0 =A0 =A0 char *name; > >=A0 =A0 =A0 char info_str[256]; > >=A0 =A0 =A0 unsigned receive_disabled : 1; > > +=A0 =A0 unsigned self_announce_disabled : 1; > >=A0 =A0 =A0 NetClientDestructor *destructor; > >=A0 =A0 =A0 unsigned int queue_index; > >=A0 =A0 =A0 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, > > > >=A0 =A0 =A0 =A0 =A0 s =3D DO_UPCAST(VhostUserState, nc, nc); > > > > +=A0 =A0 =A0 =A0 /* Self announce is managed directly by virtio-n= et NIC */ > > +=A0 =A0 =A0 =A0 s->nc.self_announce_disabled =3D 1; > >=A0 =A0 =A0 =A0 =A0 /* We don't provide a receive callback */ > >=A0 =A0 =A0 =A0 =A0 s->nc.receive_disabled =3D 1; > >=A0 =A0 =A0 =A0 =A0 s->chr =3D 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) > >=A0 { > >=A0 =A0 =A0 uint8_t buf[60]; > >=A0 =A0 =A0 int len; > > +=A0 =A0 NetClientState *nc; > > > > -=A0 =A0 trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf= ->macaddr)); > > -=A0 =A0 len =3D announce_self_create(buf, nic->conf->macaddr.a); > > +=A0 =A0 nc =3D qemu_get_queue(nic); > > > > -=A0 =A0 qemu_send_packet_raw(qemu_get_queue(nic), buf, len); > > +=A0 =A0 if (!nc->peer->self_announce_disabled) { > > +=A0 =A0 =A0 =A0 trace_qemu_announce_self_iter(qemu_ether_ntoa(&n= ic->conf-> > macaddr)); > > +=A0 =A0 =A0 =A0 len =3D announce_self_create(buf, nic->conf->mac= addr.a); > > + > > +=A0 =A0 =A0 =A0 qemu_send_packet_raw(nc, buf, len); > > +=A0 =A0 } > >=A0 } > > >=20 > 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? > =20 >=20 > > -- > > 1.7.10.4 >=20 >=20