Re: [Qemu-devel] [PATCH V2] virtio-net: unbreak any layout

2015-07-16 Thread Michael S. Tsirkin 
On Wed, Jul 15, 2015 at 03:56:07PM +0800, Jason Wang wrote:
 Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
 (virtio-net: byteswap virtio-net header) breaks any layout by
 requiring out_sg[0].iov_len = n-guest_hdr_len. Fixing this by
 copying header to temporary buffer if swap is needed, and then use
 this buffer as part of out_sg.
 
 Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
 (virtio-net: byteswap virtio-net header)
 Cc: qemu-sta...@nongnu.org
 Cc: c...@fr.ibm.com
 Signed-off-by: Jason Wang jasow...@redhat.com
 ---
 Changes from V1:
 - avoid header copying if there's no need to do header swap
 - don't write the header back
 ---
  hw/net/virtio-net.c   | 17 ++---
  include/hw/virtio/virtio-access.h |  9 +
  2 files changed, 23 insertions(+), 3 deletions(-)
 
 diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
 index e3c2db3..12322bd 100644
 --- a/hw/net/virtio-net.c
 +++ b/hw/net/virtio-net.c
 @@ -1142,7 +1142,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
  ssize_t ret, len;
  unsigned int out_num = elem.out_num;
  struct iovec *out_sg = elem.out_sg[0];
 -struct iovec sg[VIRTQUEUE_MAX_SIZE];
 +struct iovec sg[VIRTQUEUE_MAX_SIZE], sg2[VIRTQUEUE_MAX_SIZE];
 +struct virtio_net_hdr hdr;
  
  if (out_num  1) {
  error_report(virtio-net header not in first element);
 @@ -1150,11 +1151,21 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
  }
  
  if (n-has_vnet_hdr) {
 -if (out_sg[0].iov_len  n-guest_hdr_len) {
 +if (iov_size(out_sg, out_num)  n-guest_hdr_len) {
  error_report(virtio-net header incorrect);
  exit(1);
  }

this scans the iov unnecessarily. How about checking return
code from iov_to_buf instead?

 -virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
 +if (virtio_needs_swap(vdev)) {
 +iov_to_buf(out_sg, out_num, 0, hdr, sizeof(hdr));
 +virtio_net_hdr_swap(vdev, (void *) hdr);
 +sg2[0].iov_base = hdr;
 +sg2[0].iov_len = sizeof(hdr);
 +out_num = iov_copy(sg2[1], ARRAY_SIZE(sg2) - 1,
 +   out_sg, out_num,
 +   sizeof(hdr), -1);


This might truncate packet if it does not fit in 1024 anymore.
It might be better to just drop it.

 +out_num += 1;
 +out_sg = sg2;
 +}
  }
  
  /*
 diff --git a/include/hw/virtio/virtio-access.h 
 b/include/hw/virtio/virtio-access.h
 index cee5dd7..1ec1dfd 100644
 --- a/include/hw/virtio/virtio-access.h
 +++ b/include/hw/virtio/virtio-access.h
 @@ -143,6 +143,15 @@ static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, 
 const void *ptr)
  }
  }
  
 +static inline bool virtio_needs_swap(VirtIODevice *vdev)
 +{
 +#ifdef HOST_WORDS_BIGENDIAN
 +return virtio_access_is_big_endian(vdev) ? false : true;
 +#else
 +return virtio_access_is_big_endian(vdev) ? true : false;
 +#endif
 +}
 +
  static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
  {
  #ifdef HOST_WORDS_BIGENDIAN
 -- 
 2.1.4

Re: [Qemu-devel] [PATCH V2] virtio-net: unbreak any layout

2015-07-16 Thread Michael S. Tsirkin 
On Thu, Jul 16, 2015 at 01:54:38PM +0800, Jason Wang wrote:
 
 
 On 07/15/2015 07:23 PM, Michael S. Tsirkin wrote:
  On Wed, Jul 15, 2015 at 03:56:07PM +0800, Jason Wang wrote:
  Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
  (virtio-net: byteswap virtio-net header) breaks any layout by
  requiring out_sg[0].iov_len = n-guest_hdr_len. Fixing this by
  copying header to temporary buffer if swap is needed, and then use
  this buffer as part of out_sg.
 
  Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
  (virtio-net: byteswap virtio-net header)
  Cc: qemu-sta...@nongnu.org
  Cc: c...@fr.ibm.com
  Signed-off-by: Jason Wang jasow...@redhat.com
  ---
  Changes from V1:
  - avoid header copying if there's no need to do header swap
  - don't write the header back
  ---
   hw/net/virtio-net.c   | 17 ++---
   include/hw/virtio/virtio-access.h |  9 +
   2 files changed, 23 insertions(+), 3 deletions(-)
 
  diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
  index e3c2db3..12322bd 100644
  --- a/hw/net/virtio-net.c
  +++ b/hw/net/virtio-net.c
  @@ -1142,7 +1142,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
   ssize_t ret, len;
   unsigned int out_num = elem.out_num;
   struct iovec *out_sg = elem.out_sg[0];
  -struct iovec sg[VIRTQUEUE_MAX_SIZE];
  +struct iovec sg[VIRTQUEUE_MAX_SIZE], sg2[VIRTQUEUE_MAX_SIZE];
  +struct virtio_net_hdr hdr;
   
   if (out_num  1) {
   error_report(virtio-net header not in first element);
  @@ -1150,11 +1151,21 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue 
  *q)
   }
   
   if (n-has_vnet_hdr) {
  -if (out_sg[0].iov_len  n-guest_hdr_len) {
  +if (iov_size(out_sg, out_num)  n-guest_hdr_len) {
   error_report(virtio-net header incorrect);
   exit(1);
   }
  -virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
  +if (virtio_needs_swap(vdev)) {
  +iov_to_buf(out_sg, out_num, 0, hdr, sizeof(hdr));
  +virtio_net_hdr_swap(vdev, (void *) hdr);
  +sg2[0].iov_base = hdr;
  +sg2[0].iov_len = sizeof(hdr);
  +out_num = iov_copy(sg2[1], ARRAY_SIZE(sg2) - 1,
  +   out_sg, out_num,
  +   sizeof(hdr), -1);
  +out_num += 1;
  +out_sg = sg2;
  +}
   }
   
   /*
  I guess this works but iov cpy is pretty expensive.
 
  - When using the tun backend, we can just set VNETBE/VNETLE
correctly and avoid swaps in userspace.
It's easy to probe for - why don't we do this?
 
 We need to fix stable branches first. And if we do this for 2.4, if
 running on a older kernel which does not support VNETBE/VNETLE, do we
 want to fail the initialization? If yes, it breaks backward
 compatibility. If no, we still need the swap here.
 
 
  - OTOH when not using the tun backend, offloads are
generally disabled, so the swap isn't needed
since header is going to be all 0s.
 
 I agree it's better to check backend (e.g host_hdr_len) before trying to
 do the swap.
 
 
  - what's left is old kernels when using tun.
how about probing for that explicitly?
In other words, virtio_net_needs_swap as opposed to
the generic virtio_needs_swap.
 
 So we still need swap anyway. How about apply this patch first for
 stable and 2.4 and probe the VNETBE/VNETLE explicitly on top for 2.5?

I'm fine with it being a patch on top, but I think it's preferable
to use VNETBE/VNETLE even in 2.4 if that's available.

 
 
  diff --git a/include/hw/virtio/virtio-access.h 
  b/include/hw/virtio/virtio-access.h
  index cee5dd7..1ec1dfd 100644
  --- a/include/hw/virtio/virtio-access.h
  +++ b/include/hw/virtio/virtio-access.h
  @@ -143,6 +143,15 @@ static inline uint64_t virtio_ldq_p(VirtIODevice 
  *vdev, const void *ptr)
   }
   }
   
  +static inline bool virtio_needs_swap(VirtIODevice *vdev)
  +{
  +#ifdef HOST_WORDS_BIGENDIAN
  +return virtio_access_is_big_endian(vdev) ? false : true;
  +#else
  +return virtio_access_is_big_endian(vdev) ? true : false;
  +#endif
  +}
  +
   static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
   {
   #ifdef HOST_WORDS_BIGENDIAN
  -- 
  2.1.4

Re: [Qemu-devel] [PATCH V2] virtio-net: unbreak any layout

2015-07-16 Thread Michael S. Tsirkin 
On Thu, Jul 16, 2015 at 02:49:48PM +0800, Jason Wang wrote:
 
 
 On 07/16/2015 02:42 PM, Michael S. Tsirkin wrote:
  On Wed, Jul 15, 2015 at 03:56:07PM +0800, Jason Wang wrote:
  Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
  (virtio-net: byteswap virtio-net header) breaks any layout by
  requiring out_sg[0].iov_len = n-guest_hdr_len. Fixing this by
  copying header to temporary buffer if swap is needed, and then use
  this buffer as part of out_sg.
 
  Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
  (virtio-net: byteswap virtio-net header)
  Cc: qemu-sta...@nongnu.org
  Cc: c...@fr.ibm.com
  Signed-off-by: Jason Wang jasow...@redhat.com
  ---
  Changes from V1:
  - avoid header copying if there's no need to do header swap
  - don't write the header back
  ---
   hw/net/virtio-net.c   | 17 ++---
   include/hw/virtio/virtio-access.h |  9 +
   2 files changed, 23 insertions(+), 3 deletions(-)
 
  diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
  index e3c2db3..12322bd 100644
  --- a/hw/net/virtio-net.c
  +++ b/hw/net/virtio-net.c
  @@ -1142,7 +1142,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
   ssize_t ret, len;
   unsigned int out_num = elem.out_num;
   struct iovec *out_sg = elem.out_sg[0];
  -struct iovec sg[VIRTQUEUE_MAX_SIZE];
  +struct iovec sg[VIRTQUEUE_MAX_SIZE], sg2[VIRTQUEUE_MAX_SIZE];
  +struct virtio_net_hdr hdr;
   
   if (out_num  1) {
   error_report(virtio-net header not in first element);
  @@ -1150,11 +1151,21 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue 
  *q)
   }
   
   if (n-has_vnet_hdr) {
  -if (out_sg[0].iov_len  n-guest_hdr_len) {
  +if (iov_size(out_sg, out_num)  n-guest_hdr_len) {
   error_report(virtio-net header incorrect);
   exit(1);
   }
  this scans the iov unnecessarily. How about checking return
  code from iov_to_buf instead?
 
 Looks ok since hdr is very short anyway.

but iov_size scans the whole iov, not just the header.

 
  -virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
  +if (virtio_needs_swap(vdev)) {
  +iov_to_buf(out_sg, out_num, 0, hdr, sizeof(hdr));
  +virtio_net_hdr_swap(vdev, (void *) hdr);
  +sg2[0].iov_base = hdr;
  +sg2[0].iov_len = sizeof(hdr);
  +out_num = iov_copy(sg2[1], ARRAY_SIZE(sg2) - 1,
  +   out_sg, out_num,
  +   sizeof(hdr), -1);
 
  This might truncate packet if it does not fit in 1024 anymore.
  It might be better to just drop it.
 
 Ok, will do this in V3.
 
 
  +out_num += 1;
  +out_sg = sg2;
  +}
   }
   
   /*
  diff --git a/include/hw/virtio/virtio-access.h 
  b/include/hw/virtio/virtio-access.h
  index cee5dd7..1ec1dfd 100644
  --- a/include/hw/virtio/virtio-access.h
  +++ b/include/hw/virtio/virtio-access.h
  @@ -143,6 +143,15 @@ static inline uint64_t virtio_ldq_p(VirtIODevice 
  *vdev, const void *ptr)
   }
   }
   
  +static inline bool virtio_needs_swap(VirtIODevice *vdev)
  +{
  +#ifdef HOST_WORDS_BIGENDIAN
  +return virtio_access_is_big_endian(vdev) ? false : true;
  +#else
  +return virtio_access_is_big_endian(vdev) ? true : false;
  +#endif
  +}
  +
   static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
   {
   #ifdef HOST_WORDS_BIGENDIAN
  -- 
  2.1.4

Re: [Qemu-devel] [PATCH V2] virtio-net: unbreak any layout

2015-07-16 Thread Jason Wang 


On 07/16/2015 03:29 PM, Michael S. Tsirkin wrote:
 On Thu, Jul 16, 2015 at 02:49:48PM +0800, Jason Wang wrote:
  
  
  On 07/16/2015 02:42 PM, Michael S. Tsirkin wrote:
   On Wed, Jul 15, 2015 at 03:56:07PM +0800, Jason Wang wrote:
   Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
   (virtio-net: byteswap virtio-net header) breaks any layout by
   requiring out_sg[0].iov_len = n-guest_hdr_len. Fixing this by
   copying header to temporary buffer if swap is needed, and then use
   this buffer as part of out_sg.
  
   Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
   (virtio-net: byteswap virtio-net header)
   Cc: qemu-sta...@nongnu.org
   Cc: c...@fr.ibm.com
   Signed-off-by: Jason Wang jasow...@redhat.com
   ---
   Changes from V1:
   - avoid header copying if there's no need to do header swap
   - don't write the header back
   ---
hw/net/virtio-net.c   | 17 ++---
include/hw/virtio/virtio-access.h |  9 +
2 files changed, 23 insertions(+), 3 deletions(-)
  
   diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
   index e3c2db3..12322bd 100644
   --- a/hw/net/virtio-net.c
   +++ b/hw/net/virtio-net.c
   @@ -1142,7 +1142,8 @@ static int32_t 
   virtio_net_flush_tx(VirtIONetQueue *q)
ssize_t ret, len;
unsigned int out_num = elem.out_num;
struct iovec *out_sg = elem.out_sg[0];
   -struct iovec sg[VIRTQUEUE_MAX_SIZE];
   +struct iovec sg[VIRTQUEUE_MAX_SIZE], sg2[VIRTQUEUE_MAX_SIZE];
   +struct virtio_net_hdr hdr;

if (out_num  1) {
error_report(virtio-net header not in first element);
   @@ -1150,11 +1151,21 @@ static int32_t 
   virtio_net_flush_tx(VirtIONetQueue *q)
}

if (n-has_vnet_hdr) {
   -if (out_sg[0].iov_len  n-guest_hdr_len) {
   +if (iov_size(out_sg, out_num)  n-guest_hdr_len) {
error_report(virtio-net header incorrect);
exit(1);
}
   this scans the iov unnecessarily. How about checking return
   code from iov_to_buf instead?
  
  Looks ok since hdr is very short anyway.
 but iov_size scans the whole iov, not just the header.


Right.

Re: [Qemu-devel] [PATCH V2] virtio-net: unbreak any layout

2015-07-16 Thread Jason Wang 


On 07/16/2015 02:42 PM, Michael S. Tsirkin wrote:
 On Wed, Jul 15, 2015 at 03:56:07PM +0800, Jason Wang wrote:
 Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
 (virtio-net: byteswap virtio-net header) breaks any layout by
 requiring out_sg[0].iov_len = n-guest_hdr_len. Fixing this by
 copying header to temporary buffer if swap is needed, and then use
 this buffer as part of out_sg.

 Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
 (virtio-net: byteswap virtio-net header)
 Cc: qemu-sta...@nongnu.org
 Cc: c...@fr.ibm.com
 Signed-off-by: Jason Wang jasow...@redhat.com
 ---
 Changes from V1:
 - avoid header copying if there's no need to do header swap
 - don't write the header back
 ---
  hw/net/virtio-net.c   | 17 ++---
  include/hw/virtio/virtio-access.h |  9 +
  2 files changed, 23 insertions(+), 3 deletions(-)

 diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
 index e3c2db3..12322bd 100644
 --- a/hw/net/virtio-net.c
 +++ b/hw/net/virtio-net.c
 @@ -1142,7 +1142,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
  ssize_t ret, len;
  unsigned int out_num = elem.out_num;
  struct iovec *out_sg = elem.out_sg[0];
 -struct iovec sg[VIRTQUEUE_MAX_SIZE];
 +struct iovec sg[VIRTQUEUE_MAX_SIZE], sg2[VIRTQUEUE_MAX_SIZE];
 +struct virtio_net_hdr hdr;
  
  if (out_num  1) {
  error_report(virtio-net header not in first element);
 @@ -1150,11 +1151,21 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
  }
  
  if (n-has_vnet_hdr) {
 -if (out_sg[0].iov_len  n-guest_hdr_len) {
 +if (iov_size(out_sg, out_num)  n-guest_hdr_len) {
  error_report(virtio-net header incorrect);
  exit(1);
  }
 this scans the iov unnecessarily. How about checking return
 code from iov_to_buf instead?

Looks ok since hdr is very short anyway.


 -virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
 +if (virtio_needs_swap(vdev)) {
 +iov_to_buf(out_sg, out_num, 0, hdr, sizeof(hdr));
 +virtio_net_hdr_swap(vdev, (void *) hdr);
 +sg2[0].iov_base = hdr;
 +sg2[0].iov_len = sizeof(hdr);
 +out_num = iov_copy(sg2[1], ARRAY_SIZE(sg2) - 1,
 +   out_sg, out_num,
 +   sizeof(hdr), -1);

 This might truncate packet if it does not fit in 1024 anymore.
 It might be better to just drop it.

Ok, will do this in V3.


 +out_num += 1;
 +out_sg = sg2;
 +}
  }
  
  /*
 diff --git a/include/hw/virtio/virtio-access.h 
 b/include/hw/virtio/virtio-access.h
 index cee5dd7..1ec1dfd 100644
 --- a/include/hw/virtio/virtio-access.h
 +++ b/include/hw/virtio/virtio-access.h
 @@ -143,6 +143,15 @@ static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, 
 const void *ptr)
  }
  }
  
 +static inline bool virtio_needs_swap(VirtIODevice *vdev)
 +{
 +#ifdef HOST_WORDS_BIGENDIAN
 +return virtio_access_is_big_endian(vdev) ? false : true;
 +#else
 +return virtio_access_is_big_endian(vdev) ? true : false;
 +#endif
 +}
 +
  static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
  {
  #ifdef HOST_WORDS_BIGENDIAN
 -- 
 2.1.4

Re: [Qemu-devel] [PATCH V2] virtio-net: unbreak any layout

2015-07-15 Thread Jason Wang 


On 07/15/2015 07:23 PM, Michael S. Tsirkin wrote:
 On Wed, Jul 15, 2015 at 03:56:07PM +0800, Jason Wang wrote:
 Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
 (virtio-net: byteswap virtio-net header) breaks any layout by
 requiring out_sg[0].iov_len = n-guest_hdr_len. Fixing this by
 copying header to temporary buffer if swap is needed, and then use
 this buffer as part of out_sg.

 Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
 (virtio-net: byteswap virtio-net header)
 Cc: qemu-sta...@nongnu.org
 Cc: c...@fr.ibm.com
 Signed-off-by: Jason Wang jasow...@redhat.com
 ---
 Changes from V1:
 - avoid header copying if there's no need to do header swap
 - don't write the header back
 ---
  hw/net/virtio-net.c   | 17 ++---
  include/hw/virtio/virtio-access.h |  9 +
  2 files changed, 23 insertions(+), 3 deletions(-)

 diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
 index e3c2db3..12322bd 100644
 --- a/hw/net/virtio-net.c
 +++ b/hw/net/virtio-net.c
 @@ -1142,7 +1142,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
  ssize_t ret, len;
  unsigned int out_num = elem.out_num;
  struct iovec *out_sg = elem.out_sg[0];
 -struct iovec sg[VIRTQUEUE_MAX_SIZE];
 +struct iovec sg[VIRTQUEUE_MAX_SIZE], sg2[VIRTQUEUE_MAX_SIZE];
 +struct virtio_net_hdr hdr;
  
  if (out_num  1) {
  error_report(virtio-net header not in first element);
 @@ -1150,11 +1151,21 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
  }
  
  if (n-has_vnet_hdr) {
 -if (out_sg[0].iov_len  n-guest_hdr_len) {
 +if (iov_size(out_sg, out_num)  n-guest_hdr_len) {
  error_report(virtio-net header incorrect);
  exit(1);
  }
 -virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
 +if (virtio_needs_swap(vdev)) {
 +iov_to_buf(out_sg, out_num, 0, hdr, sizeof(hdr));
 +virtio_net_hdr_swap(vdev, (void *) hdr);
 +sg2[0].iov_base = hdr;
 +sg2[0].iov_len = sizeof(hdr);
 +out_num = iov_copy(sg2[1], ARRAY_SIZE(sg2) - 1,
 +   out_sg, out_num,
 +   sizeof(hdr), -1);
 +out_num += 1;
 +out_sg = sg2;
 +}
  }
  
  /*
 I guess this works but iov cpy is pretty expensive.

 - When using the tun backend, we can just set VNETBE/VNETLE
   correctly and avoid swaps in userspace.
   It's easy to probe for - why don't we do this?

We need to fix stable branches first. And if we do this for 2.4, if
running on a older kernel which does not support VNETBE/VNETLE, do we
want to fail the initialization? If yes, it breaks backward
compatibility. If no, we still need the swap here.


 - OTOH when not using the tun backend, offloads are
   generally disabled, so the swap isn't needed
   since header is going to be all 0s.

I agree it's better to check backend (e.g host_hdr_len) before trying to
do the swap.


 - what's left is old kernels when using tun.
   how about probing for that explicitly?
   In other words, virtio_net_needs_swap as opposed to
   the generic virtio_needs_swap.

So we still need swap anyway. How about apply this patch first for
stable and 2.4 and probe the VNETBE/VNETLE explicitly on top for 2.5?



 diff --git a/include/hw/virtio/virtio-access.h 
 b/include/hw/virtio/virtio-access.h
 index cee5dd7..1ec1dfd 100644
 --- a/include/hw/virtio/virtio-access.h
 +++ b/include/hw/virtio/virtio-access.h
 @@ -143,6 +143,15 @@ static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, 
 const void *ptr)
  }
  }
  
 +static inline bool virtio_needs_swap(VirtIODevice *vdev)
 +{
 +#ifdef HOST_WORDS_BIGENDIAN
 +return virtio_access_is_big_endian(vdev) ? false : true;
 +#else
 +return virtio_access_is_big_endian(vdev) ? true : false;
 +#endif
 +}
 +
  static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
  {
  #ifdef HOST_WORDS_BIGENDIAN
 -- 
 2.1.4

Re: [Qemu-devel] [PATCH V2] virtio-net: unbreak any layout

2015-07-15 Thread Paolo Bonzini 


On 15/07/2015 09:56, Jason Wang wrote:
 Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
 (virtio-net: byteswap virtio-net header) breaks any layout by
 requiring out_sg[0].iov_len = n-guest_hdr_len. Fixing this by
 copying header to temporary buffer if swap is needed, and then use
 this buffer as part of out_sg.
 
 Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
 (virtio-net: byteswap virtio-net header)
 Cc: qemu-sta...@nongnu.org
 Cc: c...@fr.ibm.com
 Signed-off-by: Jason Wang jasow...@redhat.com
 ---
 Changes from V1:
 - avoid header copying if there's no need to do header swap
 - don't write the header back
 ---
  hw/net/virtio-net.c   | 17 ++---
  include/hw/virtio/virtio-access.h |  9 +
  2 files changed, 23 insertions(+), 3 deletions(-)
 
 diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
 index e3c2db3..12322bd 100644
 --- a/hw/net/virtio-net.c
 +++ b/hw/net/virtio-net.c
 @@ -1142,7 +1142,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
  ssize_t ret, len;
  unsigned int out_num = elem.out_num;
  struct iovec *out_sg = elem.out_sg[0];
 -struct iovec sg[VIRTQUEUE_MAX_SIZE];
 +struct iovec sg[VIRTQUEUE_MAX_SIZE], sg2[VIRTQUEUE_MAX_SIZE];
 +struct virtio_net_hdr hdr;
  
  if (out_num  1) {
  error_report(virtio-net header not in first element);
 @@ -1150,11 +1151,21 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
  }
  
  if (n-has_vnet_hdr) {
 -if (out_sg[0].iov_len  n-guest_hdr_len) {
 +if (iov_size(out_sg, out_num)  n-guest_hdr_len) {
  error_report(virtio-net header incorrect);
  exit(1);
  }
 -virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
 +if (virtio_needs_swap(vdev)) {
 +iov_to_buf(out_sg, out_num, 0, hdr, sizeof(hdr));
 +virtio_net_hdr_swap(vdev, (void *) hdr);
 +sg2[0].iov_base = hdr;
 +sg2[0].iov_len = sizeof(hdr);
 +out_num = iov_copy(sg2[1], ARRAY_SIZE(sg2) - 1,
 +   out_sg, out_num,
 +   sizeof(hdr), -1);
 +out_num += 1;
 +out_sg = sg2;
 +}
  }
  
  /*
 diff --git a/include/hw/virtio/virtio-access.h 
 b/include/hw/virtio/virtio-access.h
 index cee5dd7..1ec1dfd 100644
 --- a/include/hw/virtio/virtio-access.h
 +++ b/include/hw/virtio/virtio-access.h
 @@ -143,6 +143,15 @@ static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, 
 const void *ptr)
  }
  }
  
 +static inline bool virtio_needs_swap(VirtIODevice *vdev)
 +{
 +#ifdef HOST_WORDS_BIGENDIAN
 +return virtio_access_is_big_endian(vdev) ? false : true;
 +#else
 +return virtio_access_is_big_endian(vdev) ? true : false;
 +#endif
 +}
 +
  static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
  {
  #ifdef HOST_WORDS_BIGENDIAN
 

Reviewed-by: Paolo Bonzini pbonz...@redhat.com

[Qemu-devel] [PATCH V2] virtio-net: unbreak any layout

2015-07-15 Thread Jason Wang 
Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
(virtio-net: byteswap virtio-net header) breaks any layout by
requiring out_sg[0].iov_len = n-guest_hdr_len. Fixing this by
copying header to temporary buffer if swap is needed, and then use
this buffer as part of out_sg.

Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
(virtio-net: byteswap virtio-net header)
Cc: qemu-sta...@nongnu.org
Cc: c...@fr.ibm.com
Signed-off-by: Jason Wang jasow...@redhat.com
---
Changes from V1:
- avoid header copying if there's no need to do header swap
- don't write the header back
---
 hw/net/virtio-net.c   | 17 ++---
 include/hw/virtio/virtio-access.h |  9 +
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index e3c2db3..12322bd 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1142,7 +1142,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
 ssize_t ret, len;
 unsigned int out_num = elem.out_num;
 struct iovec *out_sg = elem.out_sg[0];
-struct iovec sg[VIRTQUEUE_MAX_SIZE];
+struct iovec sg[VIRTQUEUE_MAX_SIZE], sg2[VIRTQUEUE_MAX_SIZE];
+struct virtio_net_hdr hdr;
 
 if (out_num  1) {
 error_report(virtio-net header not in first element);
@@ -1150,11 +1151,21 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
 }
 
 if (n-has_vnet_hdr) {
-if (out_sg[0].iov_len  n-guest_hdr_len) {
+if (iov_size(out_sg, out_num)  n-guest_hdr_len) {
 error_report(virtio-net header incorrect);
 exit(1);
 }
-virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
+if (virtio_needs_swap(vdev)) {
+iov_to_buf(out_sg, out_num, 0, hdr, sizeof(hdr));
+virtio_net_hdr_swap(vdev, (void *) hdr);
+sg2[0].iov_base = hdr;
+sg2[0].iov_len = sizeof(hdr);
+out_num = iov_copy(sg2[1], ARRAY_SIZE(sg2) - 1,
+   out_sg, out_num,
+   sizeof(hdr), -1);
+out_num += 1;
+out_sg = sg2;
+}
 }
 
 /*
diff --git a/include/hw/virtio/virtio-access.h 
b/include/hw/virtio/virtio-access.h
index cee5dd7..1ec1dfd 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -143,6 +143,15 @@ static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, 
const void *ptr)
 }
 }
 
+static inline bool virtio_needs_swap(VirtIODevice *vdev)
+{
+#ifdef HOST_WORDS_BIGENDIAN
+return virtio_access_is_big_endian(vdev) ? false : true;
+#else
+return virtio_access_is_big_endian(vdev) ? true : false;
+#endif
+}
+
 static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
 {
 #ifdef HOST_WORDS_BIGENDIAN
-- 
2.1.4

Re: [Qemu-devel] [PATCH V2] virtio-net: unbreak any layout

2015-07-15 Thread Michael S. Tsirkin 
On Wed, Jul 15, 2015 at 03:56:07PM +0800, Jason Wang wrote:
 Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
 (virtio-net: byteswap virtio-net header) breaks any layout by
 requiring out_sg[0].iov_len = n-guest_hdr_len. Fixing this by
 copying header to temporary buffer if swap is needed, and then use
 this buffer as part of out_sg.
 
 Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
 (virtio-net: byteswap virtio-net header)
 Cc: qemu-sta...@nongnu.org
 Cc: c...@fr.ibm.com
 Signed-off-by: Jason Wang jasow...@redhat.com
 ---
 Changes from V1:
 - avoid header copying if there's no need to do header swap
 - don't write the header back
 ---
  hw/net/virtio-net.c   | 17 ++---
  include/hw/virtio/virtio-access.h |  9 +
  2 files changed, 23 insertions(+), 3 deletions(-)
 
 diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
 index e3c2db3..12322bd 100644
 --- a/hw/net/virtio-net.c
 +++ b/hw/net/virtio-net.c
 @@ -1142,7 +1142,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
  ssize_t ret, len;
  unsigned int out_num = elem.out_num;
  struct iovec *out_sg = elem.out_sg[0];
 -struct iovec sg[VIRTQUEUE_MAX_SIZE];
 +struct iovec sg[VIRTQUEUE_MAX_SIZE], sg2[VIRTQUEUE_MAX_SIZE];
 +struct virtio_net_hdr hdr;
  
  if (out_num  1) {
  error_report(virtio-net header not in first element);
 @@ -1150,11 +1151,21 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
  }
  
  if (n-has_vnet_hdr) {
 -if (out_sg[0].iov_len  n-guest_hdr_len) {
 +if (iov_size(out_sg, out_num)  n-guest_hdr_len) {
  error_report(virtio-net header incorrect);
  exit(1);
  }
 -virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
 +if (virtio_needs_swap(vdev)) {
 +iov_to_buf(out_sg, out_num, 0, hdr, sizeof(hdr));
 +virtio_net_hdr_swap(vdev, (void *) hdr);
 +sg2[0].iov_base = hdr;
 +sg2[0].iov_len = sizeof(hdr);
 +out_num = iov_copy(sg2[1], ARRAY_SIZE(sg2) - 1,
 +   out_sg, out_num,
 +   sizeof(hdr), -1);
 +out_num += 1;
 +out_sg = sg2;
 +}
  }
  
  /*

I guess this works but iov cpy is pretty expensive.

- When using the tun backend, we can just set VNETBE/VNETLE
  correctly and avoid swaps in userspace.
  It's easy to probe for - why don't we do this?

- OTOH when not using the tun backend, offloads are
  generally disabled, so the swap isn't needed
  since header is going to be all 0s.

- what's left is old kernels when using tun.
  how about probing for that explicitly?
  In other words, virtio_net_needs_swap as opposed to
  the generic virtio_needs_swap.


 diff --git a/include/hw/virtio/virtio-access.h 
 b/include/hw/virtio/virtio-access.h
 index cee5dd7..1ec1dfd 100644
 --- a/include/hw/virtio/virtio-access.h
 +++ b/include/hw/virtio/virtio-access.h
 @@ -143,6 +143,15 @@ static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, 
 const void *ptr)
  }
  }
  
 +static inline bool virtio_needs_swap(VirtIODevice *vdev)
 +{
 +#ifdef HOST_WORDS_BIGENDIAN
 +return virtio_access_is_big_endian(vdev) ? false : true;
 +#else
 +return virtio_access_is_big_endian(vdev) ? true : false;
 +#endif
 +}
 +
  static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
  {
  #ifdef HOST_WORDS_BIGENDIAN
 -- 
 2.1.4