Patchwork [1/2] virtio-net: fix cross-endianness support

login
register
mail settings
Submitter Aurelien Jarno
Date Jan. 15, 2011, 7:27 p.m.
Message ID <1295119664-25920-1-git-send-email-aurelien@aurel32.net>
Download mbox | patch
Permalink /patch/79067/
State New
Headers show

Comments

Aurelien Jarno - Jan. 15, 2011, 7:27 p.m.
virtio-net used to work on cross-endianness configurations, but doesn't
anymore with recent guest kernels, as the new features don't handle
endianness correctly.

This patch fixes wrong conversion, and add missing ones to make
virtio-net working. Tested on the following configurations:
- i386 guest on x86_64 host
- ppc guest on x86_64 host
- i386 guest on mips host
- ppc guest on mips host

Cc: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 hw/virtio-net.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)
Aurelien Jarno - Jan. 25, 2011, 8:07 a.m.
On Sat, Jan 15, 2011 at 08:27:43PM +0100, Aurelien Jarno wrote:
> virtio-net used to work on cross-endianness configurations, but doesn't
> anymore with recent guest kernels, as the new features don't handle
> endianness correctly.
> 
> This patch fixes wrong conversion, and add missing ones to make
> virtio-net working. Tested on the following configurations:
> - i386 guest on x86_64 host
> - ppc guest on x86_64 host
> - i386 guest on mips host
> - ppc guest on mips host
> 
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  hw/virtio-net.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index ec1bf8d..515fb19 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -81,7 +81,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>      VirtIONet *n = to_virtio_net(vdev);
>      struct virtio_net_config netcfg;
>  
> -    netcfg.status = n->status;
> +    netcfg.status = lduw_p(&n->status);
>      memcpy(netcfg.mac, n->mac, ETH_ALEN);
>      memcpy(config, &netcfg, sizeof(netcfg));
>  }
> @@ -340,7 +340,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>      n->mac_table.multi_overflow = 0;
>      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
>  
> -    mac_data.entries = ldl_le_p(elem->out_sg[1].iov_base);
> +    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
>  
>      if (sizeof(mac_data.entries) +
>          (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
> @@ -356,7 +356,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>  
>      n->mac_table.first_multi = n->mac_table.in_use;
>  
> -    mac_data.entries = ldl_le_p(elem->out_sg[2].iov_base);
> +    mac_data.entries = ldl_p(elem->out_sg[2].iov_base);
>  
>      if (sizeof(mac_data.entries) +
>          (mac_data.entries * ETH_ALEN) > elem->out_sg[2].iov_len)
> @@ -386,7 +386,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
>          return VIRTIO_NET_ERR;
>      }
>  
> -    vid = lduw_le_p(elem->out_sg[1].iov_base);
> +    vid = lduw_p(elem->out_sg[1].iov_base);
>  
>      if (vid >= MAX_VLAN)
>          return VIRTIO_NET_ERR;
> @@ -675,8 +675,9 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
>          virtqueue_fill(n->rx_vq, &elem, total, i++);
>      }
>  
> -    if (mhdr)
> -        mhdr->num_buffers = i;
> +    if (mhdr) {
> +        mhdr->num_buffers = lduw_p(&i);
> +    }
>  
>      virtqueue_flush(n->rx_vq, i);
>      virtio_notify(&n->vdev, n->rx_vq);
> -- 
> 1.7.2.3
> 

Ping?
Liu Yu-B13201 - Feb. 28, 2011, 8:43 a.m.
> -----Original Message-----
> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> On Behalf Of Aurelien Jarno
> Sent: Sunday, January 16, 2011 3:28 AM
> To: qemu-devel@nongnu.org
> Cc: Anthony Liguori; Aurelien Jarno
> Subject: [Qemu-devel] [PATCH 1/2] virtio-net: fix 
> cross-endianness support
> 
> virtio-net used to work on cross-endianness configurations, 
> but doesn't
> anymore with recent guest kernels, as the new features don't handle
> endianness correctly.
> 
> This patch fixes wrong conversion, and add missing ones to make
> virtio-net working. Tested on the following configurations:
> - i386 guest on x86_64 host
> - ppc guest on x86_64 host
> - i386 guest on mips host
> - ppc guest on mips host
> 
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  hw/virtio-net.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index ec1bf8d..515fb19 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -81,7 +81,7 @@ static void 
> virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>      VirtIONet *n = to_virtio_net(vdev);
>      struct virtio_net_config netcfg;
>  
> -    netcfg.status = n->status;
> +    netcfg.status = lduw_p(&n->status);
>      memcpy(netcfg.mac, n->mac, ETH_ALEN);
>      memcpy(config, &netcfg, sizeof(netcfg));
>  }
> @@ -340,7 +340,7 @@ static int 
> virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>      n->mac_table.multi_overflow = 0;
>      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
>  
> -    mac_data.entries = ldl_le_p(elem->out_sg[1].iov_base);
> +    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
>  
>      if (sizeof(mac_data.entries) +
>          (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
> @@ -356,7 +356,7 @@ static int 
> virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>  
>      n->mac_table.first_multi = n->mac_table.in_use;
>  
> -    mac_data.entries = ldl_le_p(elem->out_sg[2].iov_base);
> +    mac_data.entries = ldl_p(elem->out_sg[2].iov_base);
>  

Hello Aurelien,

Not clear what is happen, but this commit break e500(PowerPC big endian) kvm.
Looks like e500 is happy to use ldl_le_p(), but this patch change it to use ldl_be_p().
Any thoughts about that?

Thanks,
Yu
Aurelien Jarno - March 3, 2011, 7:16 p.m.
On Mon, Feb 28, 2011 at 08:43:41AM +0000, Liu Yu-B13201 wrote:
>  
> 
> > -----Original Message-----
> > From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> > [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> > On Behalf Of Aurelien Jarno
> > Sent: Sunday, January 16, 2011 3:28 AM
> > To: qemu-devel@nongnu.org
> > Cc: Anthony Liguori; Aurelien Jarno
> > Subject: [Qemu-devel] [PATCH 1/2] virtio-net: fix 
> > cross-endianness support
> > 
> > virtio-net used to work on cross-endianness configurations, 
> > but doesn't
> > anymore with recent guest kernels, as the new features don't handle
> > endianness correctly.
> > 
> > This patch fixes wrong conversion, and add missing ones to make
> > virtio-net working. Tested on the following configurations:
> > - i386 guest on x86_64 host
> > - ppc guest on x86_64 host
> > - i386 guest on mips host
> > - ppc guest on mips host
> > 
> > Cc: Anthony Liguori <aliguori@us.ibm.com>
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > ---
> >  hw/virtio-net.c |   13 +++++++------
> >  1 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index ec1bf8d..515fb19 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -81,7 +81,7 @@ static void 
> > virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> >      VirtIONet *n = to_virtio_net(vdev);
> >      struct virtio_net_config netcfg;
> >  
> > -    netcfg.status = n->status;
> > +    netcfg.status = lduw_p(&n->status);
> >      memcpy(netcfg.mac, n->mac, ETH_ALEN);
> >      memcpy(config, &netcfg, sizeof(netcfg));
> >  }
> > @@ -340,7 +340,7 @@ static int 
> > virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >      n->mac_table.multi_overflow = 0;
> >      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
> >  
> > -    mac_data.entries = ldl_le_p(elem->out_sg[1].iov_base);
> > +    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
> >  
> >      if (sizeof(mac_data.entries) +
> >          (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
> > @@ -356,7 +356,7 @@ static int 
> > virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >  
> >      n->mac_table.first_multi = n->mac_table.in_use;
> >  
> > -    mac_data.entries = ldl_le_p(elem->out_sg[2].iov_base);
> > +    mac_data.entries = ldl_p(elem->out_sg[2].iov_base);
> >  
> 
> Hello Aurelien,

Hi,

> Not clear what is happen, but this commit break e500(PowerPC big endian) kvm.
> Looks like e500 is happy to use ldl_le_p(), but this patch change it to use ldl_be_p().

I am sorry about that. I have just found I have done my tests on the big
endian host using and old kernel, so which doesn't use this part of the
code.

> Ainy thoughts about that?
> 

My patch basically fixed the big endian guest on little endian host and 
broke the big endian guest on big endian host support. 

The endianness handling in virtio-net is different than in virtio-blk.
In virtio-blk we basically have to byteswap the data when the endianness
of the guest is different than the one of the guest. If I am correct, in
virtio-net it seems we have to byteswap the data solely depending on the 
guest endianness and independently on the host endianness. This is
something quite weird that we don't have in cpu-all.h

I am currently trying to write a patch, will come back to you when it's
ready.

Aurelien
Stefan Hajnoczi - March 3, 2011, 7:50 p.m.
On Thu, Mar 3, 2011 at 7:16 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> The endianness handling in virtio-net is different than in virtio-blk.
> In virtio-blk we basically have to byteswap the data when the endianness
> of the guest is different than the one of the guest. If I am correct, in
> virtio-net it seems we have to byteswap the data solely depending on the
> guest endianness and independently on the host endianness. This is
> something quite weird that we don't have in cpu-all.h

That sounds odd.  Virtio is guest endian so the host must do
guest<->host endian conversion.  Which bits of code make you think
otherwise?

Stefan
Aurelien Jarno - March 3, 2011, 8:51 p.m.
On Thu, Mar 03, 2011 at 07:50:03PM +0000, Stefan Hajnoczi wrote:
> On Thu, Mar 3, 2011 at 7:16 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > The endianness handling in virtio-net is different than in virtio-blk.
> > In virtio-blk we basically have to byteswap the data when the endianness
> > of the guest is different than the one of the guest. If I am correct, in
> > virtio-net it seems we have to byteswap the data solely depending on the
> > guest endianness and independently on the host endianness. This is
> > something quite weird that we don't have in cpu-all.h
> 
> That sounds odd.  Virtio is guest endian so the host must do
> guest<->host endian conversion.  Which bits of code make you think
> otherwise?
> 

That's exactly what my patch was doing, changing the ldx_le_p() into 
ldx_p(). In pratice it doesn't work.

Basically, with ldlx_le_p(), it only works when the endianness of the
guess and of the host are the same. With ldx_p() it only works when the
host is little endian. That's how I conclude that byteswapping should
only be done on big endian guests.

I agree it's weird and should be validated by some real tests, I am 
planning to do that soon.

Patch

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index ec1bf8d..515fb19 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -81,7 +81,7 @@  static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
     VirtIONet *n = to_virtio_net(vdev);
     struct virtio_net_config netcfg;
 
-    netcfg.status = n->status;
+    netcfg.status = lduw_p(&n->status);
     memcpy(netcfg.mac, n->mac, ETH_ALEN);
     memcpy(config, &netcfg, sizeof(netcfg));
 }
@@ -340,7 +340,7 @@  static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
     n->mac_table.multi_overflow = 0;
     memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
 
-    mac_data.entries = ldl_le_p(elem->out_sg[1].iov_base);
+    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
 
     if (sizeof(mac_data.entries) +
         (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
@@ -356,7 +356,7 @@  static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 
     n->mac_table.first_multi = n->mac_table.in_use;
 
-    mac_data.entries = ldl_le_p(elem->out_sg[2].iov_base);
+    mac_data.entries = ldl_p(elem->out_sg[2].iov_base);
 
     if (sizeof(mac_data.entries) +
         (mac_data.entries * ETH_ALEN) > elem->out_sg[2].iov_len)
@@ -386,7 +386,7 @@  static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;
     }
 
-    vid = lduw_le_p(elem->out_sg[1].iov_base);
+    vid = lduw_p(elem->out_sg[1].iov_base);
 
     if (vid >= MAX_VLAN)
         return VIRTIO_NET_ERR;
@@ -675,8 +675,9 @@  static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
         virtqueue_fill(n->rx_vq, &elem, total, i++);
     }
 
-    if (mhdr)
-        mhdr->num_buffers = i;
+    if (mhdr) {
+        mhdr->num_buffers = lduw_p(&i);
+    }
 
     virtqueue_flush(n->rx_vq, i);
     virtio_notify(&n->vdev, n->rx_vq);