From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53221) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmMdZ-00044T-4D for qemu-devel@nongnu.org; Wed, 14 Oct 2015 10:02:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZmMdU-00053N-Ry for qemu-devel@nongnu.org; Wed, 14 Oct 2015 10:02:36 -0400 Date: Wed, 14 Oct 2015 17:02:28 +0300 From: "Michael S. Tsirkin" Message-ID: <20151014165637-mutt-send-email-mst@redhat.com> References: <1444814805-15071-1-git-send-email-mst@redhat.com> <20151014155432.4146c045@bahia.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151014155432.4146c045@bahia.local> Subject: Re: [Qemu-devel] [PATCH] net: don't set native endianness List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: Marcel Apfelbaum , Jason Wang , qemu-devel@nongnu.org, qemu-stable@nongnu.org On Wed, Oct 14, 2015 at 03:54:32PM +0200, Greg Kurz wrote: > On Wed, 14 Oct 2015 12:26:58 +0300 > "Michael S. Tsirkin" wrote: > > > commit 5be7d9f1b1452613b95c6ba70b8d7ad3d0797991 > > vhost-net: tell tap backend about the vnet endianness > > makes vhost net always try to set LE - even if that matches the > > native endian-ness. > > > > This makes it fail on older kernels on x86 without TUNSETVNETLE support. > > > > Since qemu_set_vnet_le() is only called from vhost_net_set_vnet_endian(): > > > if (virtio_vdev_has_feature(dev, VIRTIO_F_VERSION_1) || > (virtio_legacy_is_cross_endian(dev) && !virtio_is_big_endian(dev))) { > r = qemu_set_vnet_le(peer, set); > if (r) { > error_report("backend does not support LE vnet headers"); > } > > and virtio_legacy_is_cross_endian() is { return false; } on x86, I guess this > is about virtio 1.0, right ? > > > To fix, make qemu_set_vnet_le/qemu_set_vnet_be skip the > > ioctl if it matches the host endian-ness. > > > > The qemu_set_vnet_le() change indeed makes sense since it fixes a bug. > > I am not so sure about qemu_set_vnet_be() since it is only called in the > case we have a ppc64le host and a legacy ppc64 guest, in which case > HOST_WORDS_BIGENDIAN is not defined... You are right, but it's there for symmetry. > IMHO it is better to rework the logic so that we only call qemu_set_vnet_* > when it is needed. There are only 3 cases actually: > - BE host with virtio 1 needs vnet_le > - BE host with legacy LE needs vnet_le > - LE host with legacy BE needs vnet_be > > Something like: > > static inline bool vhost_net_needs_vnet_le(VirtIODevice *dev) > { > #if defined(HOST_WORDS_BIGENDIAN) > return > virtio_vdev_has_feature(dev, VIRTIO_F_VERSION_1) || > !virtio_is_big_endian(dev); > #else > return false; > #endif > } > > static inline bool vhost_net_needs_vnet_be(VirtIODevice *dev) > { > #if defined(HOST_WORDS_BIGENDIAN) > return false; > #else > return virtio_is_big_endian(dev); > #endif > } > > static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer, > bool set) > { > int r = 0; > > if (vhost_net_needs_vnet_le(dev)) { > r = qemu_set_vnet_le(peer, set); > if (r) { > error_report("backend does not support LE vnet headers"); > } > } else if (vhost_net_needs_vnet_be(dev)) { > r = qemu_set_vnet_be(peer, set); > if (r) { > error_report("backend does not support BE vnet headers"); > } > } > > return r; > } > > Cheers. To me this looks wrong: why not call it unconditionally? The reason is that qemu_set_vnet_be/qemu_set_vnet_le fails on some backends and that is a low-level detail that should be hidden. > -- > Greg > > Reported-by: Marcel Apfelbaum > > Cc: Greg Kurz > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Michael S. Tsirkin > > --- > > net/net.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/net/net.c b/net/net.c > > index 28a5597..8e96011 100644 > > --- a/net/net.c > > +++ b/net/net.c > > @@ -517,20 +517,28 @@ void qemu_set_vnet_hdr_len(NetClientState *nc, int len) > > > > int qemu_set_vnet_le(NetClientState *nc, bool is_le) > > { > > +#ifdef HOST_WORDS_BIGENDIAN > > if (!nc || !nc->info->set_vnet_le) { > > return -ENOSYS; > > } > > > > return nc->info->set_vnet_le(nc, is_le); > > +#else > > + return 0; > > +#endif > > } > > > > int qemu_set_vnet_be(NetClientState *nc, bool is_be) > > { > > +#ifdef HOST_WORDS_BIGENDIAN > > + return 0; > > +#else > > if (!nc || !nc->info->set_vnet_be) { > > return -ENOSYS; > > } > > > > return nc->info->set_vnet_be(nc, is_be); > > +#endif > > } > > > > int qemu_can_send_packet(NetClientState *sender)