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

[Greg Kurz]

On Wed, 14 Oct 2015 12:26:58 +0300
"Michael S. Tsirkin" <address@hidden> 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 <address@hidden>
> Cc: Greg Kurz <address@hidden>
> Cc: address@hidden
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  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)