On Wed, 14 Oct 2015 17:02:28 +0300
"Michael S. Tsirkin" <m...@redhat.com> wrote:
> 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" <m...@redhat.com> 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.
>
This is the only reason indeed.
>
> > 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?
Because it is already and will always be called conditionally. It seemed better
to
consolidate all the endianness logic in one place (only 3 cases to cover).
> 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.
>
Honestly, I don't quite see the relation between "only calling a QEMU API when
I need it" and "hiding low-level details"... both make sense to me actually,
so I won't argue more, and FWIW I give my:
Acked-by: Greg Kurz <gk...@linux.vnet.ibm.com>
>
> > --
> > Greg
>
>
>
>
> > > Reported-by: Marcel Apfelbaum <mar...@redhat.com>
> > > Cc: Greg Kurz <gk...@linux.vnet.ibm.com>
> > > Cc: qemu-sta...@nongnu.org
> > > Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> > > ---
> > > 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)
>
