From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46424) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHYut-0007Fx-Jv for qemu-devel@nongnu.org; Fri, 08 Jan 2016 10:25:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHYuo-0006QD-FQ for qemu-devel@nongnu.org; Fri, 08 Jan 2016 10:25:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60993) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHYuo-0006Q1-7W for qemu-devel@nongnu.org; Fri, 08 Jan 2016 10:25:22 -0500 References: <20160107110747.10897.41118.stgit@bahia.huguette.org> <20160107113202.10897.33293.stgit@bahia.huguette.org> <568EAF45.50401@redhat.com> <20160108151949.0a9b6090@bahia.local> From: Laurent Vivier Message-ID: <568FD4DE.3000105@redhat.com> Date: Fri, 8 Jan 2016 16:25:18 +0100 MIME-Version: 1.0 In-Reply-To: <20160108151949.0a9b6090@bahia.local> Content-Type: text/plain; charset=windows-1252 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: Greg Kurz Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" 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 networ= k > 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 ! Thank you for the details, it's clear now. Perhaps this can be added in the commit log or in some comments ? > Bonne Annee ! Bonne Ann=E9e ;) Laurent > -- > 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