From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55718) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHZSk-00055C-Du for qemu-devel@nongnu.org; Fri, 08 Jan 2016 11:00:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHZSh-000189-1A for qemu-devel@nongnu.org; Fri, 08 Jan 2016 11:00:26 -0500 Received: from e06smtp15.uk.ibm.com ([195.75.94.111]:48065) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHZSg-00015w-HB for qemu-devel@nongnu.org; Fri, 08 Jan 2016 11:00:22 -0500 Received: from localhost by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 8 Jan 2016 16:00:21 -0000 Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 02E3917D8042 for ; Fri, 8 Jan 2016 16:00:18 +0000 (GMT) Received: from d06av06.portsmouth.uk.ibm.com (d06av06.portsmouth.uk.ibm.com [9.149.37.217]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u08G0I8053870808 for ; Fri, 8 Jan 2016 16:00:18 GMT Received: from d06av06.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av06.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u08G0IDp012809 for ; Fri, 8 Jan 2016 11:00:18 -0500 Date: Fri, 8 Jan 2016 17:00:12 +0100 From: Greg Kurz Message-ID: <20160108170012.402949ff@bahia.local> In-Reply-To: <568FD4DE.3000105@redhat.com> References: <20160107110747.10897.41118.stgit@bahia.huguette.org> <20160107113202.10897.33293.stgit@bahia.huguette.org> <568EAF45.50401@redhat.com> <20160108151949.0a9b6090@bahia.local> <568FD4DE.3000105@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/6] virtio-net: use the backend cross-endian capabilities List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" On Fri, 8 Jan 2016 16:25:18 +0100 Laurent Vivier wrote: >=20 >=20 > On 08/01/2016 15:19, Greg Kurz wrote: > > On Thu, 7 Jan 2016 19:32:37 +0100 > > Laurent Vivier wrote: > >> > >=20 > > Sorry for the late answer to this one, I got diverted :) > >=20 > >> > >> On 07/01/2016 12:32, Greg Kurz wrote: > >>> When running a fully emulated device in cross-endian conditions, incl= uding > >>> a virtio 1.0 device offered to a big endian guest, we need to fix the= vnet > >>> headers. This is currently handled by the virtio_net_hdr_swap() funct= ion > >>> in the core virtio-net code but it should actually be handled by the = net > >>> backend. > >>> > >>> With this patch, virtio-net now tries to configure the backend to do = the > >>> endian fixing when the device starts. If the backend cannot support t= he > >>> requested endiannes, we have to fall back on virtio_net_hdr_swap(): t= his > >>> is recorded in the needs_vnet_hdr_swap flag, to be used in the TX and= RX > >>> paths. > >>> > >>> The current vhost-net code also tries to configure net backends. This= will > >>> be no more needed and will be addressed in a subsequent patch. > >>> > >>> Signed-off-by: Greg Kurz > >>> --- > >>> hw/net/virtio-net.c | 33 ++++++++++++++++++++++++++++++= +-- > >>> include/hw/virtio/virtio-net.h | 1 + > >>> 2 files changed, 32 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >>> index a877614e3e7a..d4cc94ea5e55 100644 > >>> --- a/hw/net/virtio-net.c > >>> +++ b/hw/net/virtio-net.c > >>> @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n= , uint8_t status) > >>> } > >>> } > >>> =20 > >>> +static void virtio_net_vnet_status(VirtIONet *n, uint8_t status) > >>> +{ > >>> + VirtIODevice *vdev =3D VIRTIO_DEVICE(n); > >>> + NetClientState *peer =3D qemu_get_queue(n->nic)->peer; > >>> + > >>> + if (virtio_net_started(n, status)) { > >>> + int r; > >>> + > >>> + if (virtio_is_big_endian(vdev)) { > >>> + r =3D qemu_set_vnet_be(peer, true); > >>> + } else { > >>> + r =3D qemu_set_vnet_le(peer, true); > >>> + } > >>> + > >>> + n->needs_vnet_hdr_swap =3D !!r; > >>> + } else if (virtio_net_started(n, vdev->status) && > >>> + !virtio_net_started(n, status)) { > >>> + if (virtio_is_big_endian(vdev)) { > >>> + qemu_set_vnet_be(peer, false); > >>> + } else { > >>> + qemu_set_vnet_le(peer, false); > >>> + } > >>> + } > >>> +} > >> > >> Could you explain why check 'virtio_net_started(n, status)' and then > >> 'virtio_net_started(n, vdev->status)' ? > >> > >=20 > > Before using the device, we need to inform the network backend about > > the endianness to use when parsing vnet headers. We do this when the > > driver activates the device (DRIVER_OK). This is the first check. > >=20 > > After using the device, we need to reset the network backend to the > > default (guest native endianness), otherwise the guest may loose network > > connectivity if it is rebooted into a different endianness. We do this > > when the driver deactivates the device (no DRIVER_OK). The second check > > ensures the device was active before: if we don't check that, the 'else' > > branch would be executed each time the driver updates the status with > > something not containing DRIVER_OK... :-\ > >=20 > >> Why qemu_set_vnet_[bl]e() use "true" in the first case and "false" in > >> the second case ? > >> > >=20 > > "true" tells the backend to enforce the corresponding endianness. > > "false" tells the backed to reset to the default (guest native endianne= ss). > >=20 > >> Why don't you store the result (r) in the second case ? > >> > >=20 > > Because @needs_vnet_hdr_swap is only being used when the device is acti= ve. > >=20 > > Thank you for the time you spent on reviewing this series ! >=20 > Thank you for the details, it's clear now. > Perhaps this can be added in the commit log or in some comments ? >=20 I realized when writing the mail that this is non-trivial indeed. I'm currently adding comments and updating the changelog :) > > Bonne Annee ! >=20 > Bonne Ann=C3=A9e ;) > Laurent >=20 > > -- > > Greg > >=20 > >>> static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t= status) > >>> { > >>> VirtIONet *n =3D VIRTIO_NET(vdev); > >>> @@ -159,6 +184,7 @@ static void virtio_net_set_status(struct VirtIODe= vice *vdev, uint8_t status) > >>> int i; > >>> uint8_t queue_status; > >>> =20 > >>> + virtio_net_vnet_status(n, status); > >>> virtio_net_vhost_status(n, status); > >>> =20 > >>> for (i =3D 0; i < n->max_queues; i++) { > >>> @@ -957,7 +983,10 @@ static void receive_header(VirtIONet *n, const s= truct iovec *iov, int iov_cnt, > >>> void *wbuf =3D (void *)buf; > >>> work_around_broken_dhclient(wbuf, wbuf + n->host_hdr_len, > >>> size - n->host_hdr_len); > >>> - virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf); > >>> + > >>> + if (n->needs_vnet_hdr_swap) { > >>> + virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf); > >>> + } > >>> iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_= hdr)); > >>> } else { > >>> struct virtio_net_hdr hdr =3D { > >>> @@ -1167,7 +1196,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQue= ue *q) > >>> error_report("virtio-net header incorrect"); > >>> exit(1); > >>> } > >>> - if (virtio_needs_swap(vdev)) { > >>> + if (n->needs_vnet_hdr_swap) { > >>> virtio_net_hdr_swap(vdev, (void *) &mhdr); > >>> sg2[0].iov_base =3D &mhdr; > >>> sg2[0].iov_len =3D n->guest_hdr_len; > >>> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virti= o-net.h > >>> index f3cc25feca2b..27bc868fbc7d 100644 > >>> --- a/include/hw/virtio/virtio-net.h > >>> +++ b/include/hw/virtio/virtio-net.h > >>> @@ -94,6 +94,7 @@ typedef struct VirtIONet { > >>> uint64_t curr_guest_offloads; > >>> QEMUTimer *announce_timer; > >>> int announce_counter; > >>> + bool needs_vnet_hdr_swap; > >>> } VirtIONet; > >>> =20 > >>> void virtio_net_set_netclient_name(VirtIONet *n, const char *name, > >>> > >>> > >> > >=20 >=20