[Qemu-devel] [PATCH] net: don't set native endianness

[Qemu-devel] [PATCH] net: don't set native endianness

[Michael S. Tsirkin]

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.

To fix, make qemu_set_vnet_le/qemu_set_vnet_be skip the
ioctl if it matches the host endian-ness.

Reported-by: Marcel Apfelbaum <marcel@redhat.com>
Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Michael S. Tsirkin <mst@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)
-- 
MST

Re: [Qemu-devel] [PATCH] net: don't set native endianness

[Marcel Apfelbaum]

On 10/14/2015 12:26 PM, 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.
>
> To fix, make qemu_set_vnet_le/qemu_set_vnet_be skip the
> ioctl if it matches the host endian-ness.
>
> Reported-by: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael S. Tsirkin <mst@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)
>


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH] net: don't set native endianness

[Greg Kurz]

On Wed, 14 Oct 2015 12:26:58 +0300
"Michael S. Tsirkin" <mst@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...

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.

--
Greg

> Reported-by: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael S. Tsirkin <mst@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)

Re: [Qemu-devel] [PATCH] net: don't set native endianness

[Michael S. Tsirkin]

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" <mst@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.


> 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 <marcel@redhat.com>
> > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Michael S. Tsirkin <mst@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)

Re: [Qemu-devel] [PATCH] net: don't set native endianness

[Greg Kurz]

On Wed, 14 Oct 2015 17:02:28 +0300
"Michael S. Tsirkin" <mst@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" <mst@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 <gkurz@linux.vnet.ibm.com>

> 
> > --
> > Greg
> 
> 
> 
> 
> > > Reported-by: Marcel Apfelbaum <marcel@redhat.com>
> > > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > Cc: qemu-stable@nongnu.org
> > > Signed-off-by: Michael S. Tsirkin <mst@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)
> 

Re: [Qemu-devel] [PATCH] net: don't set native endianness

[Jason Wang]

On 10/14/2015 05:26 PM, 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.
>
> To fix, make qemu_set_vnet_le/qemu_set_vnet_be skip the
> ioctl if it matches the host endian-ness.
>
> Reported-by: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Acked-by: Jason Wang <jasowang@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)