[Qemu-devel] [PATCH 0/7] Virtio support for endian-curious guests.

[Qemu-devel] [PATCH 0/7] Virtio support for endian-curious guests.

[Rusty Russell]

Virtio is currently defined as "guest-endian", but that's a slippery
concept when the target can change endian.  In particular, virtio devices
fail on little-endian powerpc 64.

Feedback welcome!
Rusty.

Rusty Russell (7):
  virtio: allow byte swapping for vring and config access
  target-ppc: ppc64 targets can be either endian.
  hw/net/virtio-net: use virtio wrappers to access headers.
  hw/net/virtio-balloon: use virtio wrappers to access page frame
    numbers.
  hw/block/virtio-blk: use virtio wrappers to access headers.
  hw/scsi/virtio-scsi: use virtio wrappers to access headers.
  hw/char/virtio-serial-bus: use virtio wrappers to access headers.

 configure                         |   1 +
 hw/block/virtio-blk.c             |  35 +++++-----
 hw/char/virtio-serial-bus.c       |  34 +++++-----
 hw/net/virtio-net.c               |  15 +++--
 hw/scsi/virtio-scsi.c             |  33 ++++-----
 hw/virtio/virtio-balloon.c        |   3 +-
 hw/virtio/virtio.c                |  46 +++++++++----
 include/hw/virtio/virtio-access.h | 138 ++++++++++++++++++++++++++++++++++++++
 target-ppc/misc_helper.c          |   8 +++
 9 files changed, 242 insertions(+), 71 deletions(-)
 create mode 100644 include/hw/virtio/virtio-access.h

-- 
1.8.1.2

[Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Rusty Russell]

Virtio is currently defined to work as "guest endian", but this is a
problem if the guest can change endian.  As most targets can't change
endian, we make it a per-target option to avoid pessimising.

This is based on a simpler patch by Anthony Liguouri, which only handled
the vring accesses.  We also need some drivers to access these helpers,
eg. for data which contains headers.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 hw/virtio/virtio.c                |  46 +++++++++----
 include/hw/virtio/virtio-access.h | 138 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 170 insertions(+), 14 deletions(-)
 create mode 100644 include/hw/virtio/virtio-access.h

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 8176c14..2887f17 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -18,6 +18,7 @@
 #include "hw/virtio/virtio.h"
 #include "qemu/atomic.h"
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
 
 /* The alignment to use between consumer and producer parts of vring.
  * x86 pagesize again. */
@@ -84,6 +85,20 @@ struct VirtQueue
     EventNotifier host_notifier;
 };
 
+#ifdef TARGET_VIRTIO_SWAPENDIAN
+bool virtio_byteswap;
+
+/* Ask target code if we should swap endian for all vring and config access. */
+static void mark_endian(void)
+{
+    virtio_byteswap = virtio_swap_endian();
+}
+#else
+static void mark_endian(void)
+{
+}
+#endif
+
 /* virt queue functions */
 static void virtqueue_init(VirtQueue *vq)
 {
@@ -100,49 +115,49 @@ static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, addr);
-    return ldq_phys(pa);
+    return virtio_ldq_phys(pa);
 }
 
 static inline uint32_t vring_desc_len(hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, len);
-    return ldl_phys(pa);
+    return virtio_ldl_phys(pa);
 }
 
 static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, flags);
-    return lduw_phys(pa);
+    return virtio_lduw_phys(pa);
 }
 
 static inline uint16_t vring_desc_next(hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, next);
-    return lduw_phys(pa);
+    return virtio_lduw_phys(pa);
 }
 
 static inline uint16_t vring_avail_flags(VirtQueue *vq)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, flags);
-    return lduw_phys(pa);
+    return virtio_lduw_phys(pa);
 }
 
 static inline uint16_t vring_avail_idx(VirtQueue *vq)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, idx);
-    return lduw_phys(pa);
+    return virtio_lduw_phys(pa);
 }
 
 static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, ring[i]);
-    return lduw_phys(pa);
+    return virtio_lduw_phys(pa);
 }
 
 static inline uint16_t vring_used_event(VirtQueue *vq)
@@ -154,42 +169,42 @@ static inline void vring_used_ring_id(VirtQueue *vq, int i, uint32_t val)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, ring[i].id);
-    stl_phys(pa, val);
+    virtio_stl_phys(pa, val);
 }
 
 static inline void vring_used_ring_len(VirtQueue *vq, int i, uint32_t val)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, ring[i].len);
-    stl_phys(pa, val);
+    virtio_stl_phys(pa, val);
 }
 
 static uint16_t vring_used_idx(VirtQueue *vq)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, idx);
-    return lduw_phys(pa);
+    return virtio_lduw_phys(pa);
 }
 
 static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, idx);
-    stw_phys(pa, val);
+    virtio_stw_phys(pa, val);
 }
 
 static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, flags);
-    stw_phys(pa, lduw_phys(pa) | mask);
+    virtio_stw_phys(pa, virtio_lduw_phys(pa) | mask);
 }
 
 static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, flags);
-    stw_phys(pa, lduw_phys(pa) & ~mask);
+    virtio_stw_phys(pa, virtio_lduw_phys(pa) & ~mask);
 }
 
 static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
@@ -199,7 +214,7 @@ static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
         return;
     }
     pa = vq->vring.used + offsetof(VRingUsed, ring[vq->vring.num]);
-    stw_phys(pa, val);
+    virtio_stw_phys(pa, val);
 }
 
 void virtio_queue_set_notification(VirtQueue *vq, int enable)
@@ -525,6 +540,9 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
     trace_virtio_set_status(vdev, val);
 
+    /* If guest virtio endian is uncertain, set it now. */
+    mark_endian();
+
     if (k->set_status) {
         k->set_status(vdev, val);
     }
diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
new file mode 100644
index 0000000..b1d531e
--- /dev/null
+++ b/include/hw/virtio/virtio-access.h
@@ -0,0 +1,138 @@
+/*
+ * Virtio Accessor Support: In case your target can change endian.
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ *  Rusty Russell   <rusty@au.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+#ifndef _QEMU_VIRTIO_ACCESS_H
+#define _QEMU_VIRTIO_ACCESS_H
+
+#ifdef TARGET_VIRTIO_SWAPENDIAN
+/* Architectures which need biendian define this function. */
+extern bool virtio_swap_endian(void);
+
+extern bool virtio_byteswap;
+#else
+#define virtio_byteswap false
+#endif
+
+static inline uint16_t virtio_lduw_phys(hwaddr pa)
+{
+    if (virtio_byteswap) {
+        return bswap16(lduw_phys(pa));
+    }
+    return lduw_phys(pa);
+    
+}
+
+static inline uint32_t virtio_ldl_phys(hwaddr pa)
+{
+    if (virtio_byteswap) {
+        return bswap32(ldl_phys(pa));
+    }
+    return ldl_phys(pa);
+}
+
+static inline uint64_t virtio_ldq_phys(hwaddr pa)
+{
+    if (virtio_byteswap) {
+        return bswap64(ldq_phys(pa));
+    }
+    return ldq_phys(pa);
+}
+
+static inline void virtio_stw_phys(hwaddr pa, uint16_t value)
+{
+    if (virtio_byteswap) {
+        stw_phys(pa, bswap16(value));
+    } else {
+        stw_phys(pa, value);
+    }
+}
+
+static inline void virtio_stl_phys(hwaddr pa, uint32_t value)
+{
+    if (virtio_byteswap) {
+        stl_phys(pa, bswap32(value));
+    } else {
+        stl_phys(pa, value);
+    }
+}
+
+static inline void virtio_stw_p(void *ptr, uint16_t v)
+{
+    if (virtio_byteswap) {
+	stw_p(ptr, bswap16(v));
+    } else {
+	stw_p(ptr, v);
+    }
+}
+
+static inline void virtio_stl_p(void *ptr, uint32_t v)
+{
+    if (virtio_byteswap) {
+	stl_p(ptr, bswap32(v));
+    } else {
+	stl_p(ptr, v);
+    }
+}
+
+static inline void virtio_stq_p(void *ptr, uint64_t v)
+{
+    if (virtio_byteswap) {
+	stq_p(ptr, bswap64(v));
+    } else {
+	stq_p(ptr, v);
+    }
+}
+
+static inline int virtio_lduw_p(const void *ptr)
+{
+    if (virtio_byteswap) {
+	return bswap16(lduw_p(ptr));
+    } else {
+	return lduw_p(ptr);
+    }
+}
+
+static inline int virtio_ldl_p(const void *ptr)
+{
+    if (virtio_byteswap) {
+	return bswap32(ldl_p(ptr));
+    } else {
+	return ldl_p(ptr);
+    }
+}
+
+static inline uint64_t virtio_ldq_p(const void *ptr)
+{
+    if (virtio_byteswap) {
+	return bswap64(ldq_p(ptr));
+    } else {
+	return ldq_p(ptr);
+    }
+}
+
+static inline uint32_t virtio_tswap32(uint32_t s)
+{
+    if (virtio_byteswap) {
+	return bswap32(tswap32(s));
+    } else {
+	return tswap32(s);
+    }
+}
+
+static inline void virtio_tswap32s(uint32_t *s)
+{
+    tswap32s(s);
+    if (virtio_byteswap) {
+	*s = bswap32(*s);
+    }
+}
+#endif /* _QEMU_VIRTIO_ACCESS_H */
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Anthony Liguori]

Rusty Russell <rusty@rustcorp.com.au> writes:

> Virtio is currently defined to work as "guest endian", but this is a
> problem if the guest can change endian.  As most targets can't change
> endian, we make it a per-target option to avoid pessimising.
>
> This is based on a simpler patch by Anthony Liguouri, which only handled
> the vring accesses.  We also need some drivers to access these helpers,
> eg. for data which contains headers.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  hw/virtio/virtio.c                |  46 +++++++++----
>  include/hw/virtio/virtio-access.h | 138 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 170 insertions(+), 14 deletions(-)
>  create mode 100644 include/hw/virtio/virtio-access.h
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 8176c14..2887f17 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -18,6 +18,7 @@
>  #include "hw/virtio/virtio.h"
>  #include "qemu/atomic.h"
>  #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
>  
>  /* The alignment to use between consumer and producer parts of vring.
>   * x86 pagesize again. */
> @@ -84,6 +85,20 @@ struct VirtQueue
>      EventNotifier host_notifier;
>  };
>  
> +#ifdef TARGET_VIRTIO_SWAPENDIAN
> +bool virtio_byteswap;
> +
> +/* Ask target code if we should swap endian for all vring and config access. */
> +static void mark_endian(void)
> +{
> +    virtio_byteswap = virtio_swap_endian();
> +}
> +#else
> +static void mark_endian(void)
> +{
> +}
> +#endif
> +

It would be very good to avoid a target specific define here.  We would
like to move to only building a single copy of the virtio code.

We have a mechanism to do weak functions via stubs/.  I think it would
be better to do cpu_get_byteswap() as a stub function and then overload
it in the ppc64 code.

>  /* virt queue functions */
>  static void virtqueue_init(VirtQueue *vq)
>  {
> @@ -100,49 +115,49 @@ static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
>  {
>      hwaddr pa;
>      pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, addr);
> -    return ldq_phys(pa);
> +    return virtio_ldq_phys(pa);
>  }
>  
>  static inline uint32_t vring_desc_len(hwaddr desc_pa, int i)
>  {
>      hwaddr pa;
>      pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, len);
> -    return ldl_phys(pa);
> +    return virtio_ldl_phys(pa);
>  }
>  
>  static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i)
>  {
>      hwaddr pa;
>      pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, flags);
> -    return lduw_phys(pa);
> +    return virtio_lduw_phys(pa);
>  }
>  
>  static inline uint16_t vring_desc_next(hwaddr desc_pa, int i)
>  {
>      hwaddr pa;
>      pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, next);
> -    return lduw_phys(pa);
> +    return virtio_lduw_phys(pa);
>  }
>  
>  static inline uint16_t vring_avail_flags(VirtQueue *vq)
>  {
>      hwaddr pa;
>      pa = vq->vring.avail + offsetof(VRingAvail, flags);
> -    return lduw_phys(pa);
> +    return virtio_lduw_phys(pa);
>  }
>  
>  static inline uint16_t vring_avail_idx(VirtQueue *vq)
>  {
>      hwaddr pa;
>      pa = vq->vring.avail + offsetof(VRingAvail, idx);
> -    return lduw_phys(pa);
> +    return virtio_lduw_phys(pa);
>  }
>  
>  static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
>  {
>      hwaddr pa;
>      pa = vq->vring.avail + offsetof(VRingAvail, ring[i]);
> -    return lduw_phys(pa);
> +    return virtio_lduw_phys(pa);
>  }
>  
>  static inline uint16_t vring_used_event(VirtQueue *vq)
> @@ -154,42 +169,42 @@ static inline void vring_used_ring_id(VirtQueue *vq, int i, uint32_t val)
>  {
>      hwaddr pa;
>      pa = vq->vring.used + offsetof(VRingUsed, ring[i].id);
> -    stl_phys(pa, val);
> +    virtio_stl_phys(pa, val);
>  }
>  
>  static inline void vring_used_ring_len(VirtQueue *vq, int i, uint32_t val)
>  {
>      hwaddr pa;
>      pa = vq->vring.used + offsetof(VRingUsed, ring[i].len);
> -    stl_phys(pa, val);
> +    virtio_stl_phys(pa, val);
>  }
>  
>  static uint16_t vring_used_idx(VirtQueue *vq)
>  {
>      hwaddr pa;
>      pa = vq->vring.used + offsetof(VRingUsed, idx);
> -    return lduw_phys(pa);
> +    return virtio_lduw_phys(pa);
>  }
>  
>  static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
>  {
>      hwaddr pa;
>      pa = vq->vring.used + offsetof(VRingUsed, idx);
> -    stw_phys(pa, val);
> +    virtio_stw_phys(pa, val);
>  }
>  
>  static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
>  {
>      hwaddr pa;
>      pa = vq->vring.used + offsetof(VRingUsed, flags);
> -    stw_phys(pa, lduw_phys(pa) | mask);
> +    virtio_stw_phys(pa, virtio_lduw_phys(pa) | mask);
>  }
>  
>  static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
>  {
>      hwaddr pa;
>      pa = vq->vring.used + offsetof(VRingUsed, flags);
> -    stw_phys(pa, lduw_phys(pa) & ~mask);
> +    virtio_stw_phys(pa, virtio_lduw_phys(pa) & ~mask);
>  }
>  
>  static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
> @@ -199,7 +214,7 @@ static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
>          return;
>      }
>      pa = vq->vring.used + offsetof(VRingUsed, ring[vq->vring.num]);
> -    stw_phys(pa, val);
> +    virtio_stw_phys(pa, val);
>  }
>  
>  void virtio_queue_set_notification(VirtQueue *vq, int enable)
> @@ -525,6 +540,9 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>      trace_virtio_set_status(vdev, val);
>  
> +    /* If guest virtio endian is uncertain, set it now. */
> +    mark_endian();
> +
>      if (k->set_status) {
>          k->set_status(vdev, val);
>      }
> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> new file mode 100644
> index 0000000..b1d531e
> --- /dev/null
> +++ b/include/hw/virtio/virtio-access.h
> @@ -0,0 +1,138 @@
> +/*
> + * Virtio Accessor Support: In case your target can change endian.
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * Authors:
> + *  Rusty Russell   <rusty@au.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +#ifndef _QEMU_VIRTIO_ACCESS_H
> +#define _QEMU_VIRTIO_ACCESS_H
> +
> +#ifdef TARGET_VIRTIO_SWAPENDIAN
> +/* Architectures which need biendian define this function. */
> +extern bool virtio_swap_endian(void);
> +
> +extern bool virtio_byteswap;
> +#else
> +#define virtio_byteswap false
> +#endif

I suspect this is a premature optimization.  With a weak function called
directly in the accessors below, I suspect you would see no measurable
performance overhead compared to this approach.

It's all very predictable so the CPU should do a decent job optimizing
the if () away.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Andreas Färber]

Am 08.08.2013 15:31, schrieb Anthony Liguori:
> Rusty Russell <rusty@rustcorp.com.au> writes:
> 
>> Virtio is currently defined to work as "guest endian", but this is a
>> problem if the guest can change endian.  As most targets can't change
>> endian, we make it a per-target option to avoid pessimising.
>>
>> This is based on a simpler patch by Anthony Liguouri, which only handled
>> the vring accesses.  We also need some drivers to access these helpers,
>> eg. for data which contains headers.
>>
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>> ---
>>  hw/virtio/virtio.c                |  46 +++++++++----
>>  include/hw/virtio/virtio-access.h | 138 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 170 insertions(+), 14 deletions(-)
>>  create mode 100644 include/hw/virtio/virtio-access.h
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 8176c14..2887f17 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -18,6 +18,7 @@
>>  #include "hw/virtio/virtio.h"
>>  #include "qemu/atomic.h"
>>  #include "hw/virtio/virtio-bus.h"
>> +#include "hw/virtio/virtio-access.h"
>>  
>>  /* The alignment to use between consumer and producer parts of vring.
>>   * x86 pagesize again. */
>> @@ -84,6 +85,20 @@ struct VirtQueue
>>      EventNotifier host_notifier;
>>  };
>>  
>> +#ifdef TARGET_VIRTIO_SWAPENDIAN
>> +bool virtio_byteswap;
>> +
>> +/* Ask target code if we should swap endian for all vring and config access. */
>> +static void mark_endian(void)
>> +{
>> +    virtio_byteswap = virtio_swap_endian();
>> +}
>> +#else
>> +static void mark_endian(void)
>> +{
>> +}
>> +#endif
>> +
> 
> It would be very good to avoid a target specific define here.  We would
> like to move to only building a single copy of the virtio code.

+1

> We have a mechanism to do weak functions via stubs/.  I think it would
> be better to do cpu_get_byteswap() as a stub function and then overload
> it in the ppc64 code.

If this as your name indicates is a per-CPU function then it should go
into CPUClass. Interesting question is, what is virtio supposed to do if
we have two ppc CPUs, one is Big Endian, the other is Little Endian.
We'd need to check current_cpu then, which for Xen is always NULL.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 08.08.2013 15:31, schrieb Anthony Liguori:
>> Rusty Russell <rusty@rustcorp.com.au> writes:
>> 
>>> Virtio is currently defined to work as "guest endian", but this is a
>>> problem if the guest can change endian.  As most targets can't change
>>> endian, we make it a per-target option to avoid pessimising.
>>>
>>> This is based on a simpler patch by Anthony Liguouri, which only handled
>>> the vring accesses.  We also need some drivers to access these helpers,
>>> eg. for data which contains headers.
>>>
>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>>> ---
>>>  hw/virtio/virtio.c                |  46 +++++++++----
>>>  include/hw/virtio/virtio-access.h | 138 ++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 170 insertions(+), 14 deletions(-)
>>>  create mode 100644 include/hw/virtio/virtio-access.h
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 8176c14..2887f17 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -18,6 +18,7 @@
>>>  #include "hw/virtio/virtio.h"
>>>  #include "qemu/atomic.h"
>>>  #include "hw/virtio/virtio-bus.h"
>>> +#include "hw/virtio/virtio-access.h"
>>>  
>>>  /* The alignment to use between consumer and producer parts of vring.
>>>   * x86 pagesize again. */
>>> @@ -84,6 +85,20 @@ struct VirtQueue
>>>      EventNotifier host_notifier;
>>>  };
>>>  
>>> +#ifdef TARGET_VIRTIO_SWAPENDIAN
>>> +bool virtio_byteswap;
>>> +
>>> +/* Ask target code if we should swap endian for all vring and config access. */
>>> +static void mark_endian(void)
>>> +{
>>> +    virtio_byteswap = virtio_swap_endian();
>>> +}
>>> +#else
>>> +static void mark_endian(void)
>>> +{
>>> +}
>>> +#endif
>>> +
>> 
>> It would be very good to avoid a target specific define here.  We would
>> like to move to only building a single copy of the virtio code.
>
> +1
>
>> We have a mechanism to do weak functions via stubs/.  I think it would
>> be better to do cpu_get_byteswap() as a stub function and then overload
>> it in the ppc64 code.
>
> If this as your name indicates is a per-CPU function then it should go
> into CPUClass. Interesting question is, what is virtio supposed to do if
> we have two ppc CPUs, one is Big Endian, the other is Little Endian.

PPC64 is big endian.  AFAIK, there is no such thing as a little endian
PPC64 processor.

This is just a processor mode where loads/stores are byte lane swapped.
Hence the name 'cpu_get_byteswap'.  It's just asking whether the
load/stores are being swapped or not.

At least for PPC64, it's not possible to enable/disable byte lane
swapping for individual CPUs.  It's done through a system-wide hcall.

FWIW, I think most bi-endian architectures are this way too so I think
this is equally applicable to ARM.  Peter, is that right?

Regards,

Anthony Liguori

> We'd need to check current_cpu then, which for Xen is always NULL.
>
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Daniel P. Berrange]

On Thu, Aug 08, 2013 at 10:40:28AM -0500, Anthony Liguori wrote:
> Andreas Färber <afaerber@suse.de> writes:
> >> We have a mechanism to do weak functions via stubs/.  I think it would
> >> be better to do cpu_get_byteswap() as a stub function and then overload
> >> it in the ppc64 code.
> >
> > If this as your name indicates is a per-CPU function then it should go
> > into CPUClass. Interesting question is, what is virtio supposed to do if
> > we have two ppc CPUs, one is Big Endian, the other is Little Endian.
> 
> PPC64 is big endian.  AFAIK, there is no such thing as a little endian
> PPC64 processor.

Unless I'm misunderstanding, this thread seems to suggest otherwise:

  "[Qemu-devel] [PATCH 0/5] 64bit PowerPC little endian support"

  https://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg00813.html


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Anthony Liguori]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Thu, Aug 08, 2013 at 10:40:28AM -0500, Anthony Liguori wrote:
>> Andreas Färber <afaerber@suse.de> writes:
>> >> We have a mechanism to do weak functions via stubs/.  I think it would
>> >> be better to do cpu_get_byteswap() as a stub function and then overload
>> >> it in the ppc64 code.
>> >
>> > If this as your name indicates is a per-CPU function then it should go
>> > into CPUClass. Interesting question is, what is virtio supposed to do if
>> > we have two ppc CPUs, one is Big Endian, the other is Little Endian.
>> 
>> PPC64 is big endian.  AFAIK, there is no such thing as a little endian
>> PPC64 processor.
>
> Unless I'm misunderstanding, this thread seems to suggest otherwise:
>
>   "[Qemu-devel] [PATCH 0/5] 64bit PowerPC little endian support"
>
>   https://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg00813.html

Yeah, it's confusing.  It feels like little endian to most software but
the distinction in hardware (and therefore QEMU) is important.

It's the same processor.  It still starts executing big endian
instructions.  A magic register value is tweaked and loads/stores are
swapped.  CPU data structures are still read as big endian though.  It's
really just load/stores that are affected.

The distinction is important in QEMU.  ppc64 is still
TARGET_WORDS_BIGENDIAN.  We still want most stl_phys to treat integers
as big endian.  There's just this extra concept that CPU loads/stores
are sometimes byte swapped.  That affects virtio but not a lot else.

Regards,

Anthony Liguori

>
>
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Peter Maydell]

On 8 August 2013 17:07, Anthony Liguori <anthony@codemonkey.ws> wrote:
> It's the same processor.  It still starts executing big endian
> instructions.  A magic register value is tweaked and loads/stores are
> swapped.

I dunno about PPC, but for ARM generally the boot-up state is
controlled by config signals which a SoC or board can hardwire,
so you can have a SoC which is configured to start in big-endian
mode.

> CPU data structures are still read as big endian though.

Do you have an example of what you mean by "CPU data structure"?

> The distinction is important in QEMU.  ppc64 is still
> TARGET_WORDS_BIGENDIAN.

Ideally TARGET_WORDS_BIGENDIAN would go away -- it is forcing
at compile time a setting which is actually a runtime one,
and a lot of the weirdness here flows from that.

> We still want most stl_phys to treat integers
> as big endian.

Any stl_phys() should [in an ideal design] be tied to a
"bus master" which has its own idea of which endianness
it is. That is, an stl_phys() for a DMA controller model
ought to use the endianness programmed for the DMA controller,
not whatever the CPU happens to be using.

-- PMM

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 8 August 2013 17:07, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> It's the same processor.  It still starts executing big endian
>> instructions.  A magic register value is tweaked and loads/stores are
>> swapped.
>
> I dunno about PPC, but for ARM generally the boot-up state is
> controlled by config signals which a SoC or board can hardwire,
> so you can have a SoC which is configured to start in big-endian
> mode.
>
>> CPU data structures are still read as big endian though.
>
> Do you have an example of what you mean by "CPU data structure"?

MMU tlb hash table.  If you grep ldl target-ppc/* you'll see that there
are only a few cases where bswap occurs.

>> The distinction is important in QEMU.  ppc64 is still
>> TARGET_WORDS_BIGENDIAN.
>
> Ideally TARGET_WORDS_BIGENDIAN would go away -- it is forcing
> at compile time a setting which is actually a runtime one,
> and a lot of the weirdness here flows from that.
>
>> We still want most stl_phys to treat integers
>> as big endian.
>
> Any stl_phys() should [in an ideal design] be tied to a
> "bus master" which has its own idea of which endianness
> it is. That is, an stl_phys() for a DMA controller model
> ought to use the endianness programmed for the DMA controller,
> not whatever the CPU happens to be using.

We have the DMA API that attempts to do this but maybe we need to
generalize it a bit more...

I think it's pretty true that we need a context and that the context
for, say instruction fetch, is distinct from the context for load/store
instructions.

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Peter Maydell]

On 8 August 2013 17:25, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> On 8 August 2013 17:07, Anthony Liguori <anthony@codemonkey.ws> wrote:
>>> CPU data structures are still read as big endian though.
>>
>> Do you have an example of what you mean by "CPU data structure"?
>
> MMU tlb hash table.  If you grep ldl target-ppc/* you'll see that there
> are only a few cases where bswap occurs.

Oh, right, that sort of in-memory data structure. If I
understand correctly, the equivalent of that for ARM would
be the MMU translation tables; on ARM there's a system
control register bit for which endianness those are.

>> Any stl_phys() should [in an ideal design] be tied to a
>> "bus master" which has its own idea of which endianness
>> it is. That is, an stl_phys() for a DMA controller model
>> ought to use the endianness programmed for the DMA controller,
>> not whatever the CPU happens to be using.
>
> We have the DMA API that attempts to do this but maybe we need to
> generalize it a bit more...
>
> I think it's pretty true that we need a context and that the context
> for, say instruction fetch, is distinct from the context for load/store
> instructions.

A context might also give us a place to put other interesting
information which in hardware gets passed around as transaction
attributes on the bus, such as "is this a userspace or privileged
instruction".

-- PMM

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Rusty Russell]

Anthony Liguori <anthony@codemonkey.ws> writes:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
>
>> On Thu, Aug 08, 2013 at 10:40:28AM -0500, Anthony Liguori wrote:
>>> Andreas Färber <afaerber@suse.de> writes:
>>> >> We have a mechanism to do weak functions via stubs/.  I think it would
>>> >> be better to do cpu_get_byteswap() as a stub function and then overload
>>> >> it in the ppc64 code.
>>> >
>>> > If this as your name indicates is a per-CPU function then it should go
>>> > into CPUClass. Interesting question is, what is virtio supposed to do if
>>> > we have two ppc CPUs, one is Big Endian, the other is Little Endian.
>>> 
>>> PPC64 is big endian.  AFAIK, there is no such thing as a little endian
>>> PPC64 processor.
>>
>> Unless I'm misunderstanding, this thread seems to suggest otherwise:
>>
>>   "[Qemu-devel] [PATCH 0/5] 64bit PowerPC little endian support"
>>
>>   https://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg00813.html
>
> Yeah, it's confusing.  It feels like little endian to most software but
> the distinction in hardware (and therefore QEMU) is important.
>
> It's the same processor.  It still starts executing big endian
> instructions.  A magic register value is tweaked and loads/stores are
> swapped.  CPU data structures are still read as big endian though.  It's
> really just load/stores that are affected.
>
> The distinction is important in QEMU.  ppc64 is still
> TARGET_WORDS_BIGENDIAN.  We still want most stl_phys to treat integers
> as big endian.  There's just this extra concept that CPU loads/stores
> are sometimes byte swapped.  That affects virtio but not a lot else.

You've redefined endian here; please don't do that.  Endian is the order
in memory which a CPU does loads and stores.  From any reasonable
definition, PPC is bi-endian.

It's actually a weird thing for the qemu core to know at all: almost
everything which cares is in target-specific code.  The exceptions are
gdb stubs and virtio, both of which are "native endian" (and that weird
code in exec.c: what is notdirty_mem_write?).

Your argument that we shouldn't fix stl_* might be justifiable (ie. just
hack virtio and gdb as one-offs), but it's neither clear nor "least
surprise".

Chers,
Rusty.

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Anton Blanchard]

Hi,

> > The distinction is important in QEMU.  ppc64 is still
> > TARGET_WORDS_BIGENDIAN.  We still want most stl_phys to treat
> > integers as big endian.  There's just this extra concept that CPU
> > loads/stores are sometimes byte swapped.  That affects virtio but
> > not a lot else.
> 
> You've redefined endian here; please don't do that.  Endian is the
> order in memory which a CPU does loads and stores.  From any
> reasonable definition, PPC is bi-endian.
> 
> It's actually a weird thing for the qemu core to know at all: almost
> everything which cares is in target-specific code.  The exceptions are
> gdb stubs and virtio, both of which are "native endian" (and that
> weird code in exec.c: what is notdirty_mem_write?).
> 
> Your argument that we shouldn't fix stl_* might be justifiable (ie.
> just hack virtio and gdb as one-offs), but it's neither clear nor
> "least surprise".

Here is the hack I have to get gdbstub going with a little endian
PowerPC kernel. Basically:

LE guest -> BE QEMU -> BE gdb (pointing at the LE vmlinux)

In this setup, gdb expects registers to be sent in little endian mode.

It's a pretty big mistake for the gdb remote protocol to be using
native endian to transfer registers especially when there is no other
protocol negotation to work out what endian that is.

Anton
--

Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -317,6 +317,8 @@ static GDBState *gdbserver_state;
 
 bool gdb_has_xml;
 
+bool gdbstub_cross_endian;
+
 #ifdef CONFIG_USER_ONLY
 /* XXX: This is not thread safe.  Do we care?  */
 static int gdbserver_fd = -1;
Index: b/include/exec/gdbstub.h
===================================================================
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -42,8 +42,13 @@ static inline int cpu_index(CPUState *cp
 /* The GDB remote protocol transfers values in target byte order.
This means
  * we can use the raw memory access routines to access the value
buffer.
  * Conveniently, these also handle the case where the buffer is
mis-aligned.
+ *
+ * We do need to byte swap if the CPU isn't running in the QEMU
compiled
+ * target endian mode.
  */
 
+extern bool gdbstub_cross_endian;
+
 static inline int gdb_get_reg8(uint8_t *mem_buf, uint8_t val)
 {
     stb_p(mem_buf, val);
@@ -52,28 +57,49 @@ static inline int gdb_get_reg8(uint8_t *
 
 static inline int gdb_get_reg16(uint8_t *mem_buf, uint16_t val)
 {
-    stw_p(mem_buf, val);
+    if (gdbstub_cross_endian)
+        stw_p(mem_buf, bswap16(val));
+    else
+        stw_p(mem_buf, val);
     return 2;
 }
 
 static inline int gdb_get_reg32(uint8_t *mem_buf, uint32_t val)
 {
-    stl_p(mem_buf, val);
+    if (gdbstub_cross_endian)
+        stq_p(mem_buf, bswap32(val));
+    else
+        stl_p(mem_buf, val);
     return 4;
 }
 
 static inline int gdb_get_reg64(uint8_t *mem_buf, uint64_t val)
 {
-    stq_p(mem_buf, val);
+    if (gdbstub_cross_endian)
+        stq_p(mem_buf, bswap64(val));
+    else
+        stq_p(mem_buf, val);
     return 8;
 }
 
 #if TARGET_LONG_BITS == 64
 #define gdb_get_regl(buf, val) gdb_get_reg64(buf, val)
-#define ldtul_p(addr) ldq_p(addr)
+static inline uint64_t ldtul_p(const void *ptr)
+{
+	uint64_t tmp = ldq_p(ptr);
+	if (gdbstub_cross_endian)
+		tmp = bswap64(tmp);
+	return tmp;
+}
 #else
 #define gdb_get_regl(buf, val) gdb_get_reg32(buf, val)
-#define ldtul_p(addr) ldl_p(addr)
+static inline uint32_t ldtul_p(const void *ptr)
+{
+	uint32_t tmp = ldl_p(ptr);
+	if (gdbstub_cross_endian)
+		tmp = bswap32(tmp);
+	return tmp;
+}
 #endif
 
 #endif

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Peter Maydell]

On 9 August 2013 03:58, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Anthony Liguori <anthony@codemonkey.ws> writes:
>> The distinction is important in QEMU.  ppc64 is still
>> TARGET_WORDS_BIGENDIAN.  We still want most stl_phys to treat integers
>> as big endian.  There's just this extra concept that CPU loads/stores
>> are sometimes byte swapped.  That affects virtio but not a lot else.
>
> You've redefined endian here; please don't do that.  Endian is the order
> in memory which a CPU does loads and stores.

Agreed (subject to the complicating factor that it's possible for
a CPU to have the order to be different for data, instruction and
TLB walk loads, so it's not a single setting for a CPU).

> From any reasonable definition, PPC is bi-endian.
>
> It's actually a weird thing for the qemu core to know at all:

It's a TCG performance optimisation and/or simplification hack,
basically -- by hardcoding the endianness we think the core is at
compile time we can either always-byteswap or never-byteswap in the
fast paths.

> almost
> everything which cares is in target-specific code.  The exceptions are
> gdb stubs and virtio, both of which are "native endian" (and that weird
> code in exec.c: what is notdirty_mem_write?).

The code for actually doing TCG CPU loads and stores cares too.

notdirty_mem_write is (I think) part of how we handle self-modifying
code: if you write to a page in memory that we have translated some
code from, then we need to intercept that write so that we can throw
away the translated code. notdirty_mem_write() is the function that
does that interception, throws away the code, figures out if we still
need to intercept next time around, and actually does the access
the guest asked for. (It might also be used for spotting when the
guest touches memory during migration and thus that page needs to be
retransmitted -- I haven't checked.)

-- PMM

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Anthony Liguori]

Rusty Russell <rusty@rustcorp.com.au> writes:

> Anthony Liguori <anthony@codemonkey.ws> writes:
>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>>
>> The distinction is important in QEMU.  ppc64 is still
>> TARGET_WORDS_BIGENDIAN.  We still want most stl_phys to treat integers
>> as big endian.  There's just this extra concept that CPU loads/stores
>> are sometimes byte swapped.  That affects virtio but not a lot else.
>
> You've redefined endian here; please don't do that.  Endian is the order
> in memory which a CPU does loads and stores.  From any reasonable
> definition, PPC is bi-endian.
>
> It's actually a weird thing for the qemu core to know at all: almost
> everything which cares is in target-specific code.  The exceptions are
> gdb stubs and virtio, both of which are "native endian" (and that weird
> code in exec.c: what is notdirty_mem_write?).
>
> Your argument that we shouldn't fix stl_* might be justifiable (ie. just
> hack virtio and gdb as one-offs), but it's neither clear nor "least
> surprise".

That's not what I'm suggesting.

I'm suggesting that we should introduce multiple variants of {ld,st}*
for different types of memory access.

These are bad names, but I'm thinking along the lines of:

/* Read a word as the load/store instructions would */
cpu_ldst_ldw()

/* Read a word as the instruction fetch unit would */
cpu_fetch_ldw()

/* Read a word as the hardware MMU would */
cpu_mmu_ldw()

Peter was suggesting that instead of having separate functions, we
should use a context:

ldw(cpu->ldst, ..)
ldw(cpu->fetch, ..)
...

I think I prefer functions though over a context.  But this is really
about TCG, not virtio.  As Ben pointed out, virtio endianness needs to
be independent of CPUs.  We process the ring outside of a specific CPU
context and it's possible that if we pick an arbitrary one, it will be
in the wrong context (if running BE userspace).

The only real problem I have with your original patch is putting virtio
knowledge in the target code.  I think your adjusted version is fine.

Regards,

Anthony Liguori

>
> Chers,
> Rusty.

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Peter Maydell]

On 8 August 2013 16:40, Anthony Liguori <anthony@codemonkey.ws> wrote:
> PPC64 is big endian.  AFAIK, there is no such thing as a little endian
> PPC64 processor.

What's your definition of "little endian processor" here if
it isn't "one which is doing byte swaps of data"? I would
describe a PPC64 with the relevant mode bit set as "little
endian".

> This is just a processor mode where loads/stores are byte lane swapped.
> Hence the name 'cpu_get_byteswap'.  It's just asking whether the
> load/stores are being swapped or not.
>
> At least for PPC64, it's not possible to enable/disable byte lane
> swapping for individual CPUs.  It's done through a system-wide hcall.
>
> FWIW, I think most bi-endian architectures are this way too so I think
> this is equally applicable to ARM.  Peter, is that right?

ARM's bi-endian story is complicated and depends on the CPU.
Older CPUs didn't do byte-lane swapping of data; instead
when the CPU was configured in "big endian" mode they would
do address munging (XOR the address with 3 when doing a byte
access; XOR with 1 for halfword access). New CPUs do byte-lane
swapping (but only for data, not for instruction fetch).
CPUs in the transition period can do either.

In all cases, this is a per-cpu-core thing: you can have
one core configured to be bigendian and the other little
endian. You could in theory have the OS support both big
and little endian userspace processes. We ideally would
want to support "QEMU is a little endian process but the
VM's vcpu is currently bigendian".

Fuller writeup here:
http://translatedcode.wordpress.com/2012/04/02/this-end-up/

-- PMM

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 8 August 2013 16:40, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> PPC64 is big endian.  AFAIK, there is no such thing as a little endian
>> PPC64 processor.
>
> What's your definition of "little endian processor" here if
> it isn't "one which is doing byte swaps of data"? I would
> describe a PPC64 with the relevant mode bit set as "little
> endian".

Let's focus this to QEMU.  PPC64 is still TARGET_WORDS_BIGENDIAN.  It
would be totally wrong to make this change to either a function call or
to TARGET_WORDS_LITTLEENDIAN.

>> This is just a processor mode where loads/stores are byte lane swapped.
>> Hence the name 'cpu_get_byteswap'.  It's just asking whether the
>> load/stores are being swapped or not.
>>
>> At least for PPC64, it's not possible to enable/disable byte lane
>> swapping for individual CPUs.  It's done through a system-wide hcall.
>>
>> FWIW, I think most bi-endian architectures are this way too so I think
>> this is equally applicable to ARM.  Peter, is that right?
>
> ARM's bi-endian story is complicated and depends on the CPU.
> Older CPUs didn't do byte-lane swapping of data; instead
> when the CPU was configured in "big endian" mode they would
> do address munging (XOR the address with 3 when doing a byte
> access; XOR with 1 for halfword access). New CPUs do byte-lane
> swapping (but only for data, not for instruction fetch).
> CPUs in the transition period can do either.
>
> In all cases, this is a per-cpu-core thing: you can have
> one core configured to be bigendian and the other little
> endian. You could in theory have the OS support both big
> and little endian userspace processes. We ideally would
> want to support "QEMU is a little endian process but the
> VM's vcpu is currently bigendian".

Eek.  Yeah, I guess we need to tie this to a CPUState then.

>
> Fuller writeup here:
> http://translatedcode.wordpress.com/2012/04/02/this-end-up/

Excellent, thanks!

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Andreas Färber]

Am 08.08.2013 17:40, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
>> Am 08.08.2013 15:31, schrieb Anthony Liguori:
>>> We have a mechanism to do weak functions via stubs/.  I think it would
>>> be better to do cpu_get_byteswap() as a stub function and then overload
>>> it in the ppc64 code.
>>
>> If this as your name indicates is a per-CPU function then it should go
>> into CPUClass. Interesting question is, what is virtio supposed to do if
>> we have two ppc CPUs, one is Big Endian, the other is Little Endian.
> 
> PPC64 is big endian.  AFAIK, there is no such thing as a little endian
> PPC64 processor.
> 
> This is just a processor mode where loads/stores are byte lane swapped.
> Hence the name 'cpu_get_byteswap'.  It's just asking whether the
> load/stores are being swapped or not.

Exactly, just read it as "is in ... Endian mode". On the CPUs I am more
familiar with (e.g., 970), this used to be controlled via an MSR bit,
which as CPUPPCState::msr exists per CPUState. I did not check on real
hardware, but from the QEMU code this would allow for the mixed-endian
scenario described above.

> At least for PPC64, it's not possible to enable/disable byte lane
> swapping for individual CPUs.  It's done through a system-wide hcall.

What is offending me is only the following: If we name it
cpu_get_byteswap() as proposed by you, then its first argument should be
a CPUState *cpu. Its value would be read from the derived type's state,
such as the MSR bit in the code path that you wanted duplicated. The
function implementing that register-reading would be a hook in CPUClass,
with a default implementation in qom/cpu.c rather than a fallback in
stubs/. To access CPUClass, CPUState cannot be NULL, as brought up by
Stefano for cpu_do_unassigned_access(); not following that pattern
prevents mixing CPU architectures, which my large refactorings have
partially been about. Cf. my guest-memory-dump refactoring.

If it is just some random global value, then please don't call it
cpu_*(). Since sPAPR is not a target of its own, I don't see how/where
you want to implement that hcall query as per-target function either,
that might rather call for a QEMUMachine hook?

I don't care or argue about byte lanes here, I am just trying to keep
API design consistent and not regressing on the way to heterogeneous
emulation.

Regards,
Andreas

> FWIW, I think most bi-endian architectures are this way too so I think
> this is equally applicable to ARM.  Peter, is that right?
> 
> Regards,
> 
> Anthony Liguori
> 
>> We'd need to check current_cpu then, which for Xen is always NULL.
>>
>> Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Rusty Russell]

Andreas Färber <afaerber@suse.de> writes:
> Am 08.08.2013 17:40, schrieb Anthony Liguori:
>> Andreas Färber <afaerber@suse.de> writes:
>>> Am 08.08.2013 15:31, schrieb Anthony Liguori:
>>>> We have a mechanism to do weak functions via stubs/.  I think it would
>>>> be better to do cpu_get_byteswap() as a stub function and then overload
>>>> it in the ppc64 code.
>>>
>>> If this as your name indicates is a per-CPU function then it should go
>>> into CPUClass. Interesting question is, what is virtio supposed to do if
>>> we have two ppc CPUs, one is Big Endian, the other is Little Endian.
>> 
>> PPC64 is big endian.  AFAIK, there is no such thing as a little endian
>> PPC64 processor.
>> 
>> This is just a processor mode where loads/stores are byte lane swapped.
>> Hence the name 'cpu_get_byteswap'.  It's just asking whether the
>> load/stores are being swapped or not.
>
> Exactly, just read it as "is in ... Endian mode". On the CPUs I am more
> familiar with (e.g., 970), this used to be controlled via an MSR bit,
> which as CPUPPCState::msr exists per CPUState. I did not check on real
> hardware, but from the QEMU code this would allow for the mixed-endian
> scenario described above.
>
>> At least for PPC64, it's not possible to enable/disable byte lane
>> swapping for individual CPUs.  It's done through a system-wide hcall.
>
> What is offending me is only the following: If we name it
> cpu_get_byteswap() as proposed by you, then its first argument should be
> a CPUState *cpu. Its value would be read from the derived type's state,
> such as the MSR bit in the code path that you wanted duplicated. The
> function implementing that register-reading would be a hook in CPUClass,
> with a default implementation in qom/cpu.c rather than a fallback in
> stubs/. To access CPUClass, CPUState cannot be NULL, as brought up by
> Stefano for cpu_do_unassigned_access(); not following that pattern
> prevents mixing CPU architectures, which my large refactorings have
> partially been about. Cf. my guest-memory-dump refactoring.
>
> If it is just some random global value, then please don't call it
> cpu_*(). Since sPAPR is not a target of its own, I don't see how/where
> you want to implement that hcall query as per-target function either,
> that might rather call for a QEMUMachine hook?
>
> I don't care or argue about byte lanes here, I am just trying to keep
> API design consistent and not regressing on the way to heterogeneous
> emulation.

That's a lot of replumbing and indirect function calls for a fairly
obscure case.  We certainly don't have a nice CPUState lying around in
virtio at the moment, for example.

I can try to plumb this in if there's consensus, but I suspect it's
making the job 10x harder.

(The next logical step would be for st* and ld* to take the cpu to query
its endianness, Anthony's weird ideas notwithstanding).

Cheers,
Rusty.

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Peter Maydell]

On 9 August 2013 08:35, Rusty Russell <rusty@rustcorp.com.au> wrote:
> That's a lot of replumbing and indirect function calls for a fairly
> obscure case.  We certainly don't have a nice CPUState lying around in
> virtio at the moment, for example.

Actually if you're in an IO routine you do: it will be in the
global variable 'current_cpu'. Most IO functions don't need this,
but it's what we use for things like "this device behaviour varies
depending on which CPU accesses it".

-- PMM

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Rusty Russell]

Peter Maydell <peter.maydell@linaro.org> writes:
> On 9 August 2013 08:35, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> That's a lot of replumbing and indirect function calls for a fairly
>> obscure case.  We certainly don't have a nice CPUState lying around in
>> virtio at the moment, for example.
>
> Actually if you're in an IO routine you do: it will be in the
> global variable 'current_cpu'. Most IO functions don't need this,
> but it's what we use for things like "this device behaviour varies
> depending on which CPU accesses it".

Hmm, that was NULL for me when called from virtio.  I have stuck with
first_cpu in the ppc64 case.

Patches coming,
Rusty.

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Benjamin Herrenschmidt]

On Fri, 2013-08-09 at 17:05 +0930, Rusty Russell wrote:

> > Exactly, just read it as "is in ... Endian mode". On the CPUs I am more
> > familiar with (e.g., 970), this used to be controlled via an MSR bit,

970 didn't have an LE mode :-)

> > which as CPUPPCState::msr exists per CPUState. I did not check on real
> > hardware, but from the QEMU code this would allow for the mixed-endian
> > scenario described above.

This whole exercise should have nothing to do with the current endian
mode of the CPU. If for example you are running lx86 (the x86 emulator
IBM provides) which exploits MSR:LE on POWER7 to run x86 binaries in
userspace, you don't want virtio to suddenly change endian !

The information we care about is the endianness of the operating system.

The most logical way to infer it is a different bit, which used to be
MSR:ILE and is now in LPCR for guests and controlled via a hypercall on
pseries, which indicates what is the endianness of interrupt vectors.

IE. It indicates how the cpu should set MSR:LE when taking an interrupt,
regardless of what the current MSR:LE value is at any given point in
time.

So what should be done in fact is whenever *that* bit is changed
(currently via hcall, maybe via MSR:ILE if we emulate that on older
models or LPCR when we emulate that), then the qemu cpu model can "call
out" to change the "OS endianness" which we can propagate to virtio.

Anything trying to do stuff based on the "current" endianness in the MSR
sounds like a cesspit to me.

> (The next logical step would be for st* and ld* to take the cpu to query
> its endianness, Anthony's weird ideas notwithstanding).

Why would we even consider something like that ?

Ben.

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Rusty Russell]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> This whole exercise should have nothing to do with the current endian
> mode of the CPU. If for example you are running lx86 (the x86 emulator
> IBM provides) which exploits MSR:LE on POWER7 to run x86 binaries in
> userspace, you don't want virtio to suddenly change endian !
>
> The information we care about is the endianness of the operating system.

Which is why my original patches nabbed the endianness when the target
updated the virtio device status.

You're making an assumption about the nature of the guest, that they
don't pass the virtio device directly through to userspace.

I don't care, though.  The point is to make something which works, until
the Real Fix (LE virtio).

> The most logical way to infer it is a different bit, which used to be
> MSR:ILE and is now in LPCR for guests and controlled via a hypercall on
> pseries, which indicates what is the endianness of interrupt vectors.
>
> IE. It indicates how the cpu should set MSR:LE when taking an interrupt,
> regardless of what the current MSR:LE value is at any given point in
> time.
>
> So what should be done in fact is whenever *that* bit is changed
> (currently via hcall, maybe via MSR:ILE if we emulate that on older
> models or LPCR when we emulate that), then the qemu cpu model can "call
> out" to change the "OS endianness" which we can propagate to virtio.
>
> Anything trying to do stuff based on the "current" endianness in the MSR
> sounds like a cesspit to me.

OK.  What should Anton's gdb stub do then?

Cheers,
Rusty.

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Benjamin Herrenschmidt]

On Mon, 2013-08-12 at 09:58 +0930, Rusty Russell wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> > This whole exercise should have nothing to do with the current endian
> > mode of the CPU. If for example you are running lx86 (the x86 emulator
> > IBM provides) which exploits MSR:LE on POWER7 to run x86 binaries in
> > userspace, you don't want virtio to suddenly change endian !
> >
> > The information we care about is the endianness of the operating system.
> 
> Which is why my original patches nabbed the endianness when the target
> updated the virtio device status.
> 
> You're making an assumption about the nature of the guest, that they
> don't pass the virtio device directly through to userspace.

Two points here:

 - Userspace is VERY likely to have the same endianness as the operating
system.

 - The case where we might support "foreign endian" userspace *and* pass
virtio directly to it *and* give a shit about virtio v1.0 doesn't exist
anywhere but your imagination right now :-)

> I don't care, though.  The point is to make something which works, until
> the Real Fix (LE virtio).

Exactly.

> > The most logical way to infer it is a different bit, which used to be
> > MSR:ILE and is now in LPCR for guests and controlled via a hypercall on
> > pseries, which indicates what is the endianness of interrupt vectors.
> >
> > IE. It indicates how the cpu should set MSR:LE when taking an interrupt,
> > regardless of what the current MSR:LE value is at any given point in
> > time.
> >
> > So what should be done in fact is whenever *that* bit is changed
> > (currently via hcall, maybe via MSR:ILE if we emulate that on older
> > models or LPCR when we emulate that), then the qemu cpu model can "call
> > out" to change the "OS endianness" which we can propagate to virtio.
> >
> > Anything trying to do stuff based on the "current" endianness in the MSR
> > sounds like a cesspit to me.
> 
> OK.  What should Anton's gdb stub do then?

Something else. It's a different problem and needs a different solution.

For one, I think, we should first fix the root problem with gdb (tagging
endianness in the protocol etc...) and once that's done, look at what
band-aid can be applied for old stuff if we care at all (it's not like
LE ppc64 is going to not require a new gdb anyway).

Cheers,
Ben.

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Andreas Färber]

Am 09.08.2013 09:35, schrieb Rusty Russell:
> Andreas Färber <afaerber@suse.de> writes:
>> [...] If we name it
>> cpu_get_byteswap() as proposed by you, then its first argument should be
>> a CPUState *cpu. Its value would be read from the derived type's state,
>> such as the MSR bit in the code path that you wanted duplicated. The
>> function implementing that register-reading would be a hook in CPUClass,
>> with a default implementation in qom/cpu.c rather than a fallback in
>> stubs/. To access CPUClass, CPUState cannot be NULL, as brought up by
>> Stefano for cpu_do_unassigned_access(); not following that pattern
>> prevents mixing CPU architectures, which my large refactorings have
>> partially been about. Cf. my guest-memory-dump refactoring.
>>
>> If it is just some random global value, then please don't call it
>> cpu_*(). Since sPAPR is not a target of its own, I don't see how/where
>> you want to implement that hcall query as per-target function either,
>> that might rather call for a QEMUMachine hook?
>>
>> I don't care or argue about byte lanes here, I am just trying to keep
>> API design consistent and not regressing on the way to heterogeneous
>> emulation.
> 
> That's a lot of replumbing and indirect function calls for a fairly
> obscure case.

It's how QOM methods generally work. And yes, little endian ppc64 is in
fact a pretty obscure case. But IBM was just recently reported to adopt
the IP licensing model like ARM, so chances are we will see the same
mixed-core scenarios as with ARM + MicroBlaze/SuperH these days.

http://news.techeye.net/chips/ibms-launches-intel-server-challenge

Generally the problem is that we can't have multiple same-named global
functions when combining multiple targets, so we need a way to dispatch
- either from the individual CPU or from the machine. I would assume in
practice mixed cores will have the same endianness.

Or by making endianness a user-tweakable property of the virtio devices
rather than trying to auto-detect it.

>  We certainly don't have a nice CPUState lying around in
> virtio at the moment, for example.

Compare
http://git.qemu.org/?p=qemu.git;a=commit;h=c658b94f6e8c206c59d02aa6fbac285b86b53d2c

cpu_single_env has since been renamed to the mentioned current_cpu and
been changed to CPUState type.

> I can try to plumb this in if there's consensus, but I suspect it's
> making the job 10x harder.

I doubt it's that complicated, estimated less than ten minutes for me,
and not doing it is making the other job significantly harder.
cpu_get_dump_info() is already a hard nut to crack.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Rusty Russell]

Andreas Färber <afaerber@suse.de> writes:
> Am 08.08.2013 15:31, schrieb Anthony Liguori:
>> Rusty Russell <rusty@rustcorp.com.au> writes:
>> 
>>> Virtio is currently defined to work as "guest endian", but this is a
>>> problem if the guest can change endian.  As most targets can't change
>>> endian, we make it a per-target option to avoid pessimising.
>>>
>>> This is based on a simpler patch by Anthony Liguouri, which only handled
>>> the vring accesses.  We also need some drivers to access these helpers,
>>> eg. for data which contains headers.
>>>
>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>>> ---
>>>  hw/virtio/virtio.c                |  46 +++++++++----
>>>  include/hw/virtio/virtio-access.h | 138 ++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 170 insertions(+), 14 deletions(-)
>>>  create mode 100644 include/hw/virtio/virtio-access.h
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 8176c14..2887f17 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -18,6 +18,7 @@
>>>  #include "hw/virtio/virtio.h"
>>>  #include "qemu/atomic.h"
>>>  #include "hw/virtio/virtio-bus.h"
>>> +#include "hw/virtio/virtio-access.h"
>>>  
>>>  /* The alignment to use between consumer and producer parts of vring.
>>>   * x86 pagesize again. */
>>> @@ -84,6 +85,20 @@ struct VirtQueue
>>>      EventNotifier host_notifier;
>>>  };
>>>  
>>> +#ifdef TARGET_VIRTIO_SWAPENDIAN
>>> +bool virtio_byteswap;
>>> +
>>> +/* Ask target code if we should swap endian for all vring and config access. */
>>> +static void mark_endian(void)
>>> +{
>>> +    virtio_byteswap = virtio_swap_endian();
>>> +}
>>> +#else
>>> +static void mark_endian(void)
>>> +{
>>> +}
>>> +#endif
>>> +
>> 
>> It would be very good to avoid a target specific define here.  We would
>> like to move to only building a single copy of the virtio code.
>
> +1
>
>> We have a mechanism to do weak functions via stubs/.  I think it would
>> be better to do cpu_get_byteswap() as a stub function and then overload
>> it in the ppc64 code.
>
> If this as your name indicates is a per-CPU function then it should go
> into CPUClass. Interesting question is, what is virtio supposed to do if
> we have two ppc CPUs, one is Big Endian, the other is Little Endian.
> We'd need to check current_cpu then, which for Xen is always NULL.

This is why the check is performed on a random CPU when they first
acknowledge the device.  It's a hacky assumption, but that's why there's
a proposal to nail virtio to LE for the 1.0 OASIS standard.

You can't actually change endian of a virtio device in flight: it
doesn't make sense since there's readable state there already.

Cheers,
Rusty.

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Rusty Russell]

Andreas Färber <afaerber@suse.de> writes:
> Am 08.08.2013 15:31, schrieb Anthony Liguori:
>> Rusty Russell <rusty@rustcorp.com.au> writes:
>> We have a mechanism to do weak functions via stubs/.  I think it would
>> be better to do cpu_get_byteswap() as a stub function and then overload
>> it in the ppc64 code.
>
> If this as your name indicates is a per-CPU function then it should go
> into CPUClass. Interesting question is, what is virtio supposed to do if
> we have two ppc CPUs, one is Big Endian, the other is Little Endian.
> We'd need to check current_cpu then, which for Xen is always NULL.

Below is the minimal solution, which is sufficient for virtio.

If Anton wants per-cpu endianness for gdb, he'll need something more
sophisticated.

Feedback welcome!
Rusty.

Subject: cpu_get_byteswap: function for endian-ambivalent targets.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index a5bb515..ed84267 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -357,4 +357,13 @@ void cpu_reset_interrupt(CPUState *cpu, int mask);
  */
 void cpu_resume(CPUState *cpu);
 
+/**
+ * cpu_get_byteswap:
+ *
+ * Is (any) CPU running in byteswapped mode: normally false.  This
+ * doesn't take a cpu argument, because we don't support heterogeneous
+ * endianness.
+ */
+bool cpu_get_byteswap(void);
+
 #endif
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 9b701b4..d4af94a 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -25,3 +25,4 @@ stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += cpus.o
+stub-obj-y += cpu_byteswap.o
diff --git a/stubs/cpu_byteswap.c b/stubs/cpu_byteswap.c
new file mode 100644
index 0000000..b3b669f
--- /dev/null
+++ b/stubs/cpu_byteswap.c
@@ -0,0 +1,6 @@
+#include "qom/cpu.h"
+
+bool cpu_get_byteswap(void)
+{
+    return false;
+}

Subject: target-ppc: ppc64 targets can be either endian.

In this case, we just query the first cpu.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
index 616aab6..0a508eb 100644
--- a/target-ppc/misc_helper.c
+++ b/target-ppc/misc_helper.c
@@ -116,3 +116,8 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
 {
     hreg_store_msr(env, value, 0);
 }
+
+bool cpu_get_byteswap(void)
+{
+    return first_cpu->hflags & (1 << MSR_LE);
+}

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Andreas Färber]

Am 09.08.2013 09:00, schrieb Rusty Russell:
> Andreas Färber <afaerber@suse.de> writes:
>> Am 08.08.2013 15:31, schrieb Anthony Liguori:
>>> Rusty Russell <rusty@rustcorp.com.au> writes:
>>> We have a mechanism to do weak functions via stubs/.  I think it would
>>> be better to do cpu_get_byteswap() as a stub function and then overload
>>> it in the ppc64 code.
>>
>> If this as your name indicates is a per-CPU function then it should go
>> into CPUClass. Interesting question is, what is virtio supposed to do if
>> we have two ppc CPUs, one is Big Endian, the other is Little Endian.
>> We'd need to check current_cpu then, which for Xen is always NULL.
> 
> Below is the minimal solution, which is sufficient for virtio.
> 
> If Anton wants per-cpu endianness for gdb, he'll need something more
> sophisticated.
> 
> Feedback welcome!
> Rusty.
> 
> Subject: cpu_get_byteswap: function for endian-ambivalent targets.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index a5bb515..ed84267 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -357,4 +357,13 @@ void cpu_reset_interrupt(CPUState *cpu, int mask);
>   */
>  void cpu_resume(CPUState *cpu);
>  
> +/**
> + * cpu_get_byteswap:
> + *
> + * Is (any) CPU running in byteswapped mode: normally false.  This
> + * doesn't take a cpu argument, because we don't support heterogeneous
> + * endianness.
> + */
> +bool cpu_get_byteswap(void);
> +
>  #endif
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 9b701b4..d4af94a 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -25,3 +25,4 @@ stub-obj-y += vm-stop.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_WIN32) += fd-register.o
>  stub-obj-y += cpus.o
> +stub-obj-y += cpu_byteswap.o
> diff --git a/stubs/cpu_byteswap.c b/stubs/cpu_byteswap.c
> new file mode 100644
> index 0000000..b3b669f
> --- /dev/null
> +++ b/stubs/cpu_byteswap.c
> @@ -0,0 +1,6 @@
> +#include "qom/cpu.h"
> +
> +bool cpu_get_byteswap(void)
> +{
> +    return false;
> +}

That is exactly what I asked not to do. first_cpu_get_byteswap() or
virtio_get_byteswap() etc. would be names OK for me. But see below.

> 
> Subject: target-ppc: ppc64 targets can be either endian.
> 
> In this case, we just query the first cpu.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
> index 616aab6..0a508eb 100644
> --- a/target-ppc/misc_helper.c
> +++ b/target-ppc/misc_helper.c
> @@ -116,3 +116,8 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
>  {
>      hreg_store_msr(env, value, 0);
>  }
> +
> +bool cpu_get_byteswap(void)
> +{
> +    return first_cpu->hflags & (1 << MSR_LE);
> +}

This assumes that first_cpu != NULL, which I pointed out is not the case
for Xen. I'm not aware of a ppc Xen implementation, but it shows that
you should define which endianness (probably little) you want to adopt
in such a case.

You should also urgently update your QEMU since that code will not
build. first_cpu is CPUState since a number of weeks, you would need to
cast to PowerPCCPU *cpu = POWERPC_CPU(first_cpu); in the non-NULL case.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Rusty Russell]

Anthony Liguori <anthony@codemonkey.ws> writes:
> I suspect this is a premature optimization.  With a weak function called
> directly in the accessors below, I suspect you would see no measurable
> performance overhead compared to this approach.
>
> It's all very predictable so the CPU should do a decent job optimizing
> the if () away.

Perhaps.  I was leery of introducing performance regressions, but the
actual I/O tends to dominate anyway.

So I tested this, by adding the patch (below) and benchmarking
qemu-system-i386 on my laptop before and after.

Setup: Intel(R) Core(TM) i5 CPU       M 560  @ 2.67GHz
(Performance cpu governer enabled)
Guest: virtio user net, virtio block on raw file, 1 CPU, 512MB RAM.
(Qemu run under eatmydata to eliminate syncs)

First test: ping -f -c 10000 -q 10.0.2.0 (100 times)
(Ping chosen since packets stay in qemu's user net code)

BEFORE:
        MIN: 824ms
        MAX: 914ms
        AVG: 876.95ms
        STDDEV: 16ms

AFTER:
        MIN: 872ms
        MAX: 933ms
        AVG: 904.35ms
        STDDEV: 15ms

Second test: dd if=/dev/vda iflag=direct count=10000 of=/dev/null (100 times)

BEFORE:
        MIN: 0.927994sec
        MAX: 1.051640sec
        AVG: 0.99733sec
        STDDEV: 0.028sec

AFTER:
        MIN: 0.941706sec
        MAX: 1.034810sec
        AVG: 0.988692sec
        STDDEV: 0.021sec

So, we can notice performance on ping, but anything which does actual IO
is a wash.

Cheers,
Rusty.

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 2887f17..df8733b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -85,20 +85,6 @@ struct VirtQueue
     EventNotifier host_notifier;
 };
 
-#ifdef TARGET_VIRTIO_SWAPENDIAN
-bool virtio_byteswap;
-
-/* Ask target code if we should swap endian for all vring and config access. */
-static void mark_endian(void)
-{
-    virtio_byteswap = virtio_swap_endian();
-}
-#else
-static void mark_endian(void)
-{
-}
-#endif
-
 /* virt queue functions */
 static void virtqueue_init(VirtQueue *vq)
 {
@@ -540,9 +526,6 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
     trace_virtio_set_status(vdev, val);
 
-    /* If guest virtio endian is uncertain, set it now. */
-    mark_endian();
-
     if (k->set_status) {
         k->set_status(vdev, val);
     }
diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index b1d531e..ea4166a 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -13,18 +13,9 @@
 #ifndef _QEMU_VIRTIO_ACCESS_H
 #define _QEMU_VIRTIO_ACCESS_H
 
-#ifdef TARGET_VIRTIO_SWAPENDIAN
-/* Architectures which need biendian define this function. */
-extern bool virtio_swap_endian(void);
-
-extern bool virtio_byteswap;
-#else
-#define virtio_byteswap false
-#endif
-
 static inline uint16_t virtio_lduw_phys(hwaddr pa)
 {
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
         return bswap16(lduw_phys(pa));
     }
     return lduw_phys(pa);
@@ -33,7 +24,7 @@ static inline uint16_t virtio_lduw_phys(hwaddr pa)
 
 static inline uint32_t virtio_ldl_phys(hwaddr pa)
 {
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
         return bswap32(ldl_phys(pa));
     }
     return ldl_phys(pa);
@@ -41,7 +32,7 @@ static inline uint32_t virtio_ldl_phys(hwaddr pa)
 
 static inline uint64_t virtio_ldq_phys(hwaddr pa)
 {
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
         return bswap64(ldq_phys(pa));
     }
     return ldq_phys(pa);
@@ -49,7 +40,7 @@ static inline uint64_t virtio_ldq_phys(hwaddr pa)
 
 static inline void virtio_stw_phys(hwaddr pa, uint16_t value)
 {
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
         stw_phys(pa, bswap16(value));
     } else {
         stw_phys(pa, value);
@@ -58,7 +49,7 @@ static inline void virtio_stw_phys(hwaddr pa, uint16_t value)
 
 static inline void virtio_stl_phys(hwaddr pa, uint32_t value)
 {
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
         stl_phys(pa, bswap32(value));
     } else {
         stl_phys(pa, value);
@@ -67,7 +58,7 @@ static inline void virtio_stl_phys(hwaddr pa, uint32_t value)
 
 static inline void virtio_stw_p(void *ptr, uint16_t v)
 {
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
 	stw_p(ptr, bswap16(v));
     } else {
 	stw_p(ptr, v);
@@ -76,7 +67,7 @@ static inline void virtio_stw_p(void *ptr, uint16_t v)
 
 static inline void virtio_stl_p(void *ptr, uint32_t v)
 {
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
 	stl_p(ptr, bswap32(v));
     } else {
 	stl_p(ptr, v);
@@ -85,7 +76,7 @@ static inline void virtio_stl_p(void *ptr, uint32_t v)
 
 static inline void virtio_stq_p(void *ptr, uint64_t v)
 {
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
 	stq_p(ptr, bswap64(v));
     } else {
 	stq_p(ptr, v);
@@ -94,7 +85,7 @@ static inline void virtio_stq_p(void *ptr, uint64_t v)
 
 static inline int virtio_lduw_p(const void *ptr)
 {
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
 	return bswap16(lduw_p(ptr));
     } else {
 	return lduw_p(ptr);
@@ -103,7 +94,7 @@ static inline int virtio_lduw_p(const void *ptr)
 
 static inline int virtio_ldl_p(const void *ptr)
 {
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
 	return bswap32(ldl_p(ptr));
     } else {
 	return ldl_p(ptr);
@@ -112,7 +103,7 @@ static inline int virtio_ldl_p(const void *ptr)
 
 static inline uint64_t virtio_ldq_p(const void *ptr)
 {
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
 	return bswap64(ldq_p(ptr));
     } else {
 	return ldq_p(ptr);
@@ -121,7 +112,7 @@ static inline uint64_t virtio_ldq_p(const void *ptr)
 
 static inline uint32_t virtio_tswap32(uint32_t s)
 {
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
 	return bswap32(tswap32(s));
     } else {
 	return tswap32(s);
@@ -131,7 +122,7 @@ static inline uint32_t virtio_tswap32(uint32_t s)
 static inline void virtio_tswap32s(uint32_t *s)
 {
     tswap32s(s);
-    if (virtio_byteswap) {
+    if (cpu_get_byteswap()) {
 	*s = bswap32(*s);
     }
 }
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index a5bb515..cc5068f 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -357,4 +357,11 @@ void cpu_reset_interrupt(CPUState *cpu, int mask);
  */
 void cpu_resume(CPUState *cpu);
 
+/**
+ * cpu_get_byteswap:
+ *
+ * Is (any) CPU running in byteswapped mode.  Normally false.
+ */
+bool cpu_get_byteswap(void);
+
 #endif
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 9b701b4..d4af94a 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -25,3 +25,4 @@ stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += cpus.o
+stub-obj-y += cpu_byteswap.o
diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
index b031586..18d399d 100644
--- a/target-ppc/misc_helper.c
+++ b/target-ppc/misc_helper.c
@@ -118,9 +118,7 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
     hreg_store_msr(env, value, 0);
 }
 
-/* Our virtio accesses are LE if the first CPU is LE when they touch
- * it.  We assume endian doesn't change after that! */ 
-bool virtio_swap_endian(void)
+bool cpu_get_byteswap(void)
 {
     return first_cpu->hflags & (1 << MSR_LE);
 }

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Anthony Liguori]

Rusty Russell <rusty@rustcorp.com.au> writes:

> Anthony Liguori <anthony@codemonkey.ws> writes:
>> I suspect this is a premature optimization.  With a weak function called
>> directly in the accessors below, I suspect you would see no measurable
>> performance overhead compared to this approach.
>>
>> It's all very predictable so the CPU should do a decent job optimizing
>> the if () away.
>
> Perhaps.  I was leery of introducing performance regressions, but the
> actual I/O tends to dominate anyway.
>
> So I tested this, by adding the patch (below) and benchmarking
> qemu-system-i386 on my laptop before and after.
>
> Setup: Intel(R) Core(TM) i5 CPU       M 560  @ 2.67GHz
> (Performance cpu governer enabled)
> Guest: virtio user net, virtio block on raw file, 1 CPU, 512MB RAM.
> (Qemu run under eatmydata to eliminate syncs)

FYI, cache=unsafe is equivalent to using eatmydata.

> First test: ping -f -c 10000 -q 10.0.2.0 (100 times)
> (Ping chosen since packets stay in qemu's user net code)
>
> BEFORE:
>         MIN: 824ms
>         MAX: 914ms
>         AVG: 876.95ms
>         STDDEV: 16ms
>
> AFTER:
>         MIN: 872ms
>         MAX: 933ms
>         AVG: 904.35ms
>         STDDEV: 15ms

I can reproduce this although I also see a larger standard deviation.

BEFORE:
	MIN: 496
	MAX: 1055
        AVG: 873.22
        STDEV: 136.88

AFTER:
        MIN: 494
        MAX: 1456
        AVG: 947.77
        STDEV: 150.89

In my datasets, the stdev is higher in the after case implying that
there is more variation.  Indeed, the MIN is pretty much the same.

GCC is inlining the functions, I'm still surprised that it's measurable
at all.

At any rate, I think the advantage of not increasing the amount of
target specific code outweighs the performance difference here.  As you
said, if there is real I/O, the differences isn't noticable.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Rusty Russell]

Anthony Liguori <anthony@codemonkey.ws> writes:
> Rusty Russell <rusty@rustcorp.com.au> writes:
>> (Qemu run under eatmydata to eliminate syncs)
>
> FYI, cache=unsafe is equivalent to using eatmydata.

Ah, thanks!

> I can reproduce this although I also see a larger standard deviation.
>
> BEFORE:
> 	MIN: 496
> 	MAX: 1055
>         AVG: 873.22
>         STDEV: 136.88
>
> AFTER:
>         MIN: 494
>         MAX: 1456
>         AVG: 947.77
>         STDEV: 150.89

BTW, how did you generate these stats?  Consider this my plug for my
little stats filter:
        https://github.com/rustyrussell/stats

>
> In my datasets, the stdev is higher in the after case implying that
> there is more variation.  Indeed, the MIN is pretty much the same.
>
> GCC is inlining the functions, I'm still surprised that it's measurable
> at all.

GCC won't inline across compilation units without -flto though, so the
stub call won't be inlined, right?

Cheers,
Rusty.

[Qemu-devel] [PATCH 2/7] target-ppc: ppc64 targets can be either endian.

[Rusty Russell]

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 configure                | 1 +
 target-ppc/misc_helper.c | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/configure b/configure
index ad32f87..cee32af 100755
--- a/configure
+++ b/configure
@@ -4217,6 +4217,7 @@ case "$target_name" in
   ppc64)
     TARGET_BASE_ARCH=ppc
     TARGET_ABI_DIR=ppc
+    echo "TARGET_VIRTIO_SWAPENDIAN=y" >> $config_target_mak
     gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml power-spe.xml"
   ;;
   ppc64abi32)
diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
index 616aab6..b031586 100644
--- a/target-ppc/misc_helper.c
+++ b/target-ppc/misc_helper.c
@@ -20,6 +20,7 @@
 #include "helper.h"
 
 #include "helper_regs.h"
+#include "hw/virtio/virtio-access.h"
 
 /*****************************************************************************/
 /* SPR accesses */
@@ -116,3 +117,10 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
 {
     hreg_store_msr(env, value, 0);
 }
+
+/* Our virtio accesses are LE if the first CPU is LE when they touch
+ * it.  We assume endian doesn't change after that! */ 
+bool virtio_swap_endian(void)
+{
+    return first_cpu->hflags & (1 << MSR_LE);
+}
-- 
1.8.1.2

[Qemu-devel] [PATCH 3/7] hw/net/virtio-net: use virtio wrappers to access headers.

[Rusty Russell]

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 hw/net/virtio-net.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 1ea9556..e77e28d 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -21,6 +21,7 @@
 #include "hw/virtio/virtio-net.h"
 #include "net/vhost_net.h"
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
 
 #define VIRTIO_NET_VM_VERSION    11
 
@@ -70,8 +71,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
     VirtIONet *n = VIRTIO_NET(vdev);
     struct virtio_net_config netcfg;
 
-    stw_p(&netcfg.status, n->status);
-    stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
+    virtio_stw_p(&netcfg.status, n->status);
+    virtio_stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
     memcpy(netcfg.mac, n->mac, ETH_ALEN);
     memcpy(config, &netcfg, n->config_size);
 }
@@ -510,7 +511,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 
     s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
                    sizeof(mac_data.entries));
-    mac_data.entries = ldl_p(&mac_data.entries);
+    mac_data.entries = virtio_ldl_p(&mac_data.entries);
     if (s != sizeof(mac_data.entries)) {
         return VIRTIO_NET_ERR;
     }
@@ -537,7 +538,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 
     s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
                    sizeof(mac_data.entries));
-    mac_data.entries = ldl_p(&mac_data.entries);
+    mac_data.entries = virtio_ldl_p(&mac_data.entries);
     if (s != sizeof(mac_data.entries)) {
         return VIRTIO_NET_ERR;
     }
@@ -569,7 +570,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
     size_t s;
 
     s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
-    vid = lduw_p(&vid);
+    vid = virtio_lduw_p(&vid);
     if (s != sizeof(vid)) {
         return VIRTIO_NET_ERR;
     }
@@ -604,7 +605,7 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;
     }
 
-    queues = lduw_p(&mq.virtqueue_pairs);
+    queues = virtio_lduw_p(&mq.virtqueue_pairs);
 
     if (queues < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
         queues > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
@@ -903,7 +904,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
     }
 
     if (mhdr_cnt) {
-        stw_p(&mhdr.num_buffers, i);
+        virtio_stw_p(&mhdr.num_buffers, i);
         iov_from_buf(mhdr_sg, mhdr_cnt,
                      0,
                      &mhdr.num_buffers, sizeof mhdr.num_buffers);
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH 3/7] hw/net/virtio-net: use virtio wrappers to access headers.

[Anthony Liguori]

Rusty Russell <rusty@rustcorp.com.au> writes:

> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  hw/net/virtio-net.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 1ea9556..e77e28d 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -21,6 +21,7 @@
>  #include "hw/virtio/virtio-net.h"
>  #include "net/vhost_net.h"
>  #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
>  
>  #define VIRTIO_NET_VM_VERSION    11
>  
> @@ -70,8 +71,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>      VirtIONet *n = VIRTIO_NET(vdev);
>      struct virtio_net_config netcfg;
>  
> -    stw_p(&netcfg.status, n->status);
> -    stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
> +    virtio_stw_p(&netcfg.status, n->status);
> +    virtio_stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
>      memcpy(netcfg.mac, n->mac, ETH_ALEN);
>      memcpy(config, &netcfg, n->config_size);
>  }
> @@ -510,7 +511,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>  
>      s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
>                     sizeof(mac_data.entries));
> -    mac_data.entries = ldl_p(&mac_data.entries);
> +    mac_data.entries = virtio_ldl_p(&mac_data.entries);
>      if (s != sizeof(mac_data.entries)) {
>          return VIRTIO_NET_ERR;
>      }
> @@ -537,7 +538,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>  
>      s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
>                     sizeof(mac_data.entries));
> -    mac_data.entries = ldl_p(&mac_data.entries);
> +    mac_data.entries = virtio_ldl_p(&mac_data.entries);
>      if (s != sizeof(mac_data.entries)) {
>          return VIRTIO_NET_ERR;
>      }
> @@ -569,7 +570,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
>      size_t s;
>  
>      s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
> -    vid = lduw_p(&vid);
> +    vid = virtio_lduw_p(&vid);
>      if (s != sizeof(vid)) {
>          return VIRTIO_NET_ERR;
>      }
> @@ -604,7 +605,7 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
>          return VIRTIO_NET_ERR;
>      }
>  
> -    queues = lduw_p(&mq.virtqueue_pairs);
> +    queues = virtio_lduw_p(&mq.virtqueue_pairs);
>  
>      if (queues < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
>          queues > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
> @@ -903,7 +904,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
>      }
>  
>      if (mhdr_cnt) {
> -        stw_p(&mhdr.num_buffers, i);
> +        virtio_stw_p(&mhdr.num_buffers, i);
>          iov_from_buf(mhdr_sg, mhdr_cnt,
>                       0,
>                       &mhdr.num_buffers, sizeof mhdr.num_buffers);
> -- 
> 1.8.1.2

[Qemu-devel] [PATCH 4/7] hw/net/virtio-balloon: use virtio wrappers to access page frame numbers.

[Rusty Russell]

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 hw/virtio/virtio-balloon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index d669756..c0b4f6e 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -30,6 +30,7 @@
 #endif
 
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
 
 static void balloon_page(void *addr, int deflate)
 {
@@ -192,7 +193,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
             ram_addr_t pa;
             ram_addr_t addr;
 
-            pa = (ram_addr_t)ldl_p(&pfn) << VIRTIO_BALLOON_PFN_SHIFT;
+            pa = (ram_addr_t)virtio_ldl_p(&pfn) << VIRTIO_BALLOON_PFN_SHIFT;
             offset += 4;
 
             /* FIXME: remove get_system_memory(), but how? */
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH 4/7] hw/net/virtio-balloon: use virtio wrappers to access page frame numbers.

[Anthony Liguori]

Rusty Russell <rusty@rustcorp.com.au> writes:

> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  hw/virtio/virtio-balloon.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index d669756..c0b4f6e 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -30,6 +30,7 @@
>  #endif
>  
>  #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
>  
>  static void balloon_page(void *addr, int deflate)
>  {
> @@ -192,7 +193,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>              ram_addr_t pa;
>              ram_addr_t addr;
>  
> -            pa = (ram_addr_t)ldl_p(&pfn) << VIRTIO_BALLOON_PFN_SHIFT;
> +            pa = (ram_addr_t)virtio_ldl_p(&pfn) << VIRTIO_BALLOON_PFN_SHIFT;
>              offset += 4;
>  
>              /* FIXME: remove get_system_memory(), but how? */
> -- 
> 1.8.1.2

[Qemu-devel] [PATCH 5/7] hw/block/virtio-blk: use virtio wrappers to access headers.

[Rusty Russell]

Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 hw/block/virtio-blk.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index cf12469..9b897fa 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -25,6 +25,7 @@
 # include <scsi/sg.h>
 #endif
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
 
 typedef struct VirtIOBlockReq
 {
@@ -76,7 +77,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
     trace_virtio_blk_rw_complete(req, ret);
 
     if (ret) {
-        bool is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
+        bool is_read = !(virtio_ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
         if (virtio_blk_handle_rw_error(req, -ret, is_read))
             return;
     }
@@ -223,12 +224,12 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
         hdr.status = CHECK_CONDITION;
     }
 
-    stl_p(&req->scsi->errors,
-          hdr.status | (hdr.msg_status << 8) |
-          (hdr.host_status << 16) | (hdr.driver_status << 24));
-    stl_p(&req->scsi->residual, hdr.resid);
-    stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
-    stl_p(&req->scsi->data_len, hdr.dxfer_len);
+    virtio_stl_p(&req->scsi->errors,
+		 hdr.status | (hdr.msg_status << 8) |
+		 (hdr.host_status << 16) | (hdr.driver_status << 24));
+    virtio_stl_p(&req->scsi->residual, hdr.resid);
+    virtio_stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
+    virtio_stl_p(&req->scsi->data_len, hdr.dxfer_len);
 
     virtio_blk_req_complete(req, status);
     g_free(req);
@@ -239,7 +240,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 
 fail:
     /* Just put anything nonzero so that the ioctl fails in the guest.  */
-    stl_p(&req->scsi->errors, 255);
+    virtio_stl_p(&req->scsi->errors, 255);
     virtio_blk_req_complete(req, status);
     g_free(req);
 }
@@ -285,7 +286,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     BlockRequest *blkreq;
     uint64_t sector;
 
-    sector = ldq_p(&req->out->sector);
+    sector = virtio_ldq_p(&req->out->sector);
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
 
@@ -319,7 +320,7 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
 {
     uint64_t sector;
 
-    sector = ldq_p(&req->out->sector);
+    sector = virtio_ldq_p(&req->out->sector);
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
 
@@ -357,7 +358,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
     req->out = (void *)req->elem.out_sg[0].iov_base;
     req->in = (void *)req->elem.in_sg[req->elem.in_num - 1].iov_base;
 
-    type = ldl_p(&req->out->type);
+    type = virtio_ldl_p(&req->out->type);
 
     if (type & VIRTIO_BLK_T_FLUSH) {
         virtio_blk_handle_flush(req, mrb);
@@ -485,12 +486,12 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 
     bdrv_get_geometry(s->bs, &capacity);
     memset(&blkcfg, 0, sizeof(blkcfg));
-    stq_raw(&blkcfg.capacity, capacity);
-    stl_raw(&blkcfg.seg_max, 128 - 2);
-    stw_raw(&blkcfg.cylinders, s->conf->cyls);
-    stl_raw(&blkcfg.blk_size, blk_size);
-    stw_raw(&blkcfg.min_io_size, s->conf->min_io_size / blk_size);
-    stw_raw(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size);
+    virtio_stq_p(&blkcfg.capacity, capacity);
+    virtio_stl_p(&blkcfg.seg_max, 128 - 2);
+    virtio_stw_p(&blkcfg.cylinders, s->conf->cyls);
+    virtio_stl_p(&blkcfg.blk_size, blk_size);
+    virtio_stw_p(&blkcfg.min_io_size, s->conf->min_io_size / blk_size);
+    virtio_stw_p(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size);
     blkcfg.heads = s->conf->heads;
     /*
      * We must ensure that the block device capacity is a multiple of
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH 5/7] hw/block/virtio-blk: use virtio wrappers to access headers.

[Peter Maydell]

On 8 August 2013 06:15, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.

This is the right thing, incidentally. The _raw() functions are
supposed to take a guest physical address (as an integer), where
the _p() ones take a host void*. However the implementation means
you can get away with misusing the _raw() function where you meant
the _p() function provided your code is only built for system
emulation, because the macro's casts mean the compiler won't
call you it. (We should probably try to clean up some of the
misuses.)

-- PMM

Re: [Qemu-devel] [PATCH 5/7] hw/block/virtio-blk: use virtio wrappers to access headers.

[Anthony Liguori]

Rusty Russell <rusty@rustcorp.com.au> writes:

> Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  hw/block/virtio-blk.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index cf12469..9b897fa 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -25,6 +25,7 @@
>  # include <scsi/sg.h>
>  #endif
>  #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
>  
>  typedef struct VirtIOBlockReq
>  {
> @@ -76,7 +77,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
>      trace_virtio_blk_rw_complete(req, ret);
>  
>      if (ret) {
> -        bool is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
> +        bool is_read = !(virtio_ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
>          if (virtio_blk_handle_rw_error(req, -ret, is_read))
>              return;
>      }
> @@ -223,12 +224,12 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>          hdr.status = CHECK_CONDITION;
>      }
>  
> -    stl_p(&req->scsi->errors,
> -          hdr.status | (hdr.msg_status << 8) |
> -          (hdr.host_status << 16) | (hdr.driver_status << 24));
> -    stl_p(&req->scsi->residual, hdr.resid);
> -    stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
> -    stl_p(&req->scsi->data_len, hdr.dxfer_len);
> +    virtio_stl_p(&req->scsi->errors,
> +		 hdr.status | (hdr.msg_status << 8) |
> +		 (hdr.host_status << 16) | (hdr.driver_status << 24));
> +    virtio_stl_p(&req->scsi->residual, hdr.resid);
> +    virtio_stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
> +    virtio_stl_p(&req->scsi->data_len, hdr.dxfer_len);
>  
>      virtio_blk_req_complete(req, status);
>      g_free(req);
> @@ -239,7 +240,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>  
>  fail:
>      /* Just put anything nonzero so that the ioctl fails in the guest.  */
> -    stl_p(&req->scsi->errors, 255);
> +    virtio_stl_p(&req->scsi->errors, 255);
>      virtio_blk_req_complete(req, status);
>      g_free(req);
>  }
> @@ -285,7 +286,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>      BlockRequest *blkreq;
>      uint64_t sector;
>  
> -    sector = ldq_p(&req->out->sector);
> +    sector = virtio_ldq_p(&req->out->sector);
>  
>      bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
>  
> @@ -319,7 +320,7 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
>  {
>      uint64_t sector;
>  
> -    sector = ldq_p(&req->out->sector);
> +    sector = virtio_ldq_p(&req->out->sector);
>  
>      bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
>  
> @@ -357,7 +358,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
>      req->out = (void *)req->elem.out_sg[0].iov_base;
>      req->in = (void *)req->elem.in_sg[req->elem.in_num - 1].iov_base;
>  
> -    type = ldl_p(&req->out->type);
> +    type = virtio_ldl_p(&req->out->type);
>  
>      if (type & VIRTIO_BLK_T_FLUSH) {
>          virtio_blk_handle_flush(req, mrb);
> @@ -485,12 +486,12 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>  
>      bdrv_get_geometry(s->bs, &capacity);
>      memset(&blkcfg, 0, sizeof(blkcfg));
> -    stq_raw(&blkcfg.capacity, capacity);
> -    stl_raw(&blkcfg.seg_max, 128 - 2);
> -    stw_raw(&blkcfg.cylinders, s->conf->cyls);
> -    stl_raw(&blkcfg.blk_size, blk_size);
> -    stw_raw(&blkcfg.min_io_size, s->conf->min_io_size / blk_size);
> -    stw_raw(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size);
> +    virtio_stq_p(&blkcfg.capacity, capacity);
> +    virtio_stl_p(&blkcfg.seg_max, 128 - 2);
> +    virtio_stw_p(&blkcfg.cylinders, s->conf->cyls);
> +    virtio_stl_p(&blkcfg.blk_size, blk_size);
> +    virtio_stw_p(&blkcfg.min_io_size, s->conf->min_io_size / blk_size);
> +    virtio_stw_p(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size);
>      blkcfg.heads = s->conf->heads;
>      /*
>       * We must ensure that the block device capacity is a multiple of
> -- 
> 1.8.1.2

[Qemu-devel] [PATCH 6/7] hw/scsi/virtio-scsi: use virtio wrappers to access headers.

[Rusty Russell]

Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 hw/scsi/virtio-scsi.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 08dd3f3..c417087 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -18,6 +18,7 @@
 #include <hw/scsi/scsi.h>
 #include <block/scsi.h>
 #include <hw/virtio/virtio-bus.h>
+#include "hw/virtio/virtio-access.h"
 
 typedef struct VirtIOSCSIReq {
     VirtIOSCSI *dev;
@@ -307,12 +308,12 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
     req->resp.cmd->response = VIRTIO_SCSI_S_OK;
     req->resp.cmd->status = status;
     if (req->resp.cmd->status == GOOD) {
-        req->resp.cmd->resid = tswap32(resid);
+        req->resp.cmd->resid = virtio_tswap32(resid);
     } else {
         req->resp.cmd->resid = 0;
         sense_len = scsi_req_get_sense(r, req->resp.cmd->sense,
                                        VIRTIO_SCSI_SENSE_SIZE);
-        req->resp.cmd->sense_len = tswap32(sense_len);
+        req->resp.cmd->sense_len = virtio_tswap32(sense_len);
     }
     virtio_scsi_complete_req(req);
 }
@@ -408,16 +409,16 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
     VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
     VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
 
-    stl_raw(&scsiconf->num_queues, s->conf.num_queues);
-    stl_raw(&scsiconf->seg_max, 128 - 2);
-    stl_raw(&scsiconf->max_sectors, s->conf.max_sectors);
-    stl_raw(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
-    stl_raw(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
-    stl_raw(&scsiconf->sense_size, s->sense_size);
-    stl_raw(&scsiconf->cdb_size, s->cdb_size);
-    stw_raw(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
-    stw_raw(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
-    stl_raw(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
+    virtio_stl_p(&scsiconf->num_queues, s->conf.num_queues);
+    virtio_stl_p(&scsiconf->seg_max, 128 - 2);
+    virtio_stl_p(&scsiconf->max_sectors, s->conf.max_sectors);
+    virtio_stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
+    virtio_stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
+    virtio_stl_p(&scsiconf->sense_size, s->sense_size);
+    virtio_stl_p(&scsiconf->cdb_size, s->cdb_size);
+    virtio_stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
+    virtio_stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
+    virtio_stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
 }
 
 static void virtio_scsi_set_config(VirtIODevice *vdev,
@@ -426,14 +427,14 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
     VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
 
-    if ((uint32_t) ldl_raw(&scsiconf->sense_size) >= 65536 ||
-        (uint32_t) ldl_raw(&scsiconf->cdb_size) >= 256) {
+    if ((uint32_t) virtio_ldl_p(&scsiconf->sense_size) >= 65536 ||
+        (uint32_t) virtio_ldl_p(&scsiconf->cdb_size) >= 256) {
         error_report("bad data written to virtio-scsi configuration space");
         exit(1);
     }
 
-    vs->sense_size = ldl_raw(&scsiconf->sense_size);
-    vs->cdb_size = ldl_raw(&scsiconf->cdb_size);
+    vs->sense_size = virtio_ldl_p(&scsiconf->sense_size);
+    vs->cdb_size = virtio_ldl_p(&scsiconf->cdb_size);
 }
 
 static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH 6/7] hw/scsi/virtio-scsi: use virtio wrappers to access headers.

[Anthony Liguori]

Rusty Russell <rusty@rustcorp.com.au> writes:

> Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  hw/scsi/virtio-scsi.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 08dd3f3..c417087 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -18,6 +18,7 @@
>  #include <hw/scsi/scsi.h>
>  #include <block/scsi.h>
>  #include <hw/virtio/virtio-bus.h>
> +#include "hw/virtio/virtio-access.h"
>  
>  typedef struct VirtIOSCSIReq {
>      VirtIOSCSI *dev;
> @@ -307,12 +308,12 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
>      req->resp.cmd->response = VIRTIO_SCSI_S_OK;
>      req->resp.cmd->status = status;
>      if (req->resp.cmd->status == GOOD) {
> -        req->resp.cmd->resid = tswap32(resid);
> +        req->resp.cmd->resid = virtio_tswap32(resid);
>      } else {
>          req->resp.cmd->resid = 0;
>          sense_len = scsi_req_get_sense(r, req->resp.cmd->sense,
>                                         VIRTIO_SCSI_SENSE_SIZE);
> -        req->resp.cmd->sense_len = tswap32(sense_len);
> +        req->resp.cmd->sense_len = virtio_tswap32(sense_len);
>      }
>      virtio_scsi_complete_req(req);
>  }
> @@ -408,16 +409,16 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>      VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
>  
> -    stl_raw(&scsiconf->num_queues, s->conf.num_queues);
> -    stl_raw(&scsiconf->seg_max, 128 - 2);
> -    stl_raw(&scsiconf->max_sectors, s->conf.max_sectors);
> -    stl_raw(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
> -    stl_raw(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
> -    stl_raw(&scsiconf->sense_size, s->sense_size);
> -    stl_raw(&scsiconf->cdb_size, s->cdb_size);
> -    stw_raw(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
> -    stw_raw(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
> -    stl_raw(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
> +    virtio_stl_p(&scsiconf->num_queues, s->conf.num_queues);
> +    virtio_stl_p(&scsiconf->seg_max, 128 - 2);
> +    virtio_stl_p(&scsiconf->max_sectors, s->conf.max_sectors);
> +    virtio_stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
> +    virtio_stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
> +    virtio_stl_p(&scsiconf->sense_size, s->sense_size);
> +    virtio_stl_p(&scsiconf->cdb_size, s->cdb_size);
> +    virtio_stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
> +    virtio_stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
> +    virtio_stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
>  }
>  
>  static void virtio_scsi_set_config(VirtIODevice *vdev,
> @@ -426,14 +427,14 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
>      VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
>  
> -    if ((uint32_t) ldl_raw(&scsiconf->sense_size) >= 65536 ||
> -        (uint32_t) ldl_raw(&scsiconf->cdb_size) >= 256) {
> +    if ((uint32_t) virtio_ldl_p(&scsiconf->sense_size) >= 65536 ||
> +        (uint32_t) virtio_ldl_p(&scsiconf->cdb_size) >= 256) {
>          error_report("bad data written to virtio-scsi configuration space");
>          exit(1);
>      }
>  
> -    vs->sense_size = ldl_raw(&scsiconf->sense_size);
> -    vs->cdb_size = ldl_raw(&scsiconf->cdb_size);
> +    vs->sense_size = virtio_ldl_p(&scsiconf->sense_size);
> +    vs->cdb_size = virtio_ldl_p(&scsiconf->cdb_size);
>  }
>  
>  static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
> -- 
> 1.8.1.2

[Qemu-devel] [PATCH 7/7] hw/char/virtio-serial-bus: use virtio wrappers to access headers.

[Rusty Russell]

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 hw/char/virtio-serial-bus.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index cc3d1dd..0421725 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -24,6 +24,7 @@
 #include "hw/sysbus.h"
 #include "trace.h"
 #include "hw/virtio/virtio-serial.h"
+#include "hw/virtio/virtio-access.h"
 
 static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
 {
@@ -185,9 +186,9 @@ static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id,
 {
     struct virtio_console_control cpkt;
 
-    stl_p(&cpkt.id, port_id);
-    stw_p(&cpkt.event, event);
-    stw_p(&cpkt.value, value);
+    virtio_stl_p(&cpkt.id, port_id);
+    virtio_stw_p(&cpkt.event, event);
+    virtio_stw_p(&cpkt.value, value);
 
     trace_virtio_serial_send_control_event(port_id, event, value);
     return send_control_msg(vser, &cpkt, sizeof(cpkt));
@@ -291,8 +292,8 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         return;
     }
 
-    cpkt.event = lduw_p(&gcpkt->event);
-    cpkt.value = lduw_p(&gcpkt->value);
+    cpkt.event = virtio_lduw_p(&gcpkt->event);
+    cpkt.value = virtio_lduw_p(&gcpkt->value);
 
     trace_virtio_serial_handle_control_message(cpkt.event, cpkt.value);
 
@@ -312,10 +313,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         return;
     }
 
-    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
+    port = find_port_by_id(vser, virtio_ldl_p(&gcpkt->id));
     if (!port) {
         error_report("virtio-serial-bus: Unexpected port id %u for device %s",
-                     ldl_p(&gcpkt->id), vser->bus.qbus.name);
+                     virtio_ldl_p(&gcpkt->id), vser->bus.qbus.name);
         return;
     }
 
@@ -342,9 +343,9 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         }
 
         if (port->name) {
-            stl_p(&cpkt.id, port->id);
-            stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
-            stw_p(&cpkt.value, 1);
+            virtio_stl_p(&cpkt.id, port->id);
+            virtio_stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
+            virtio_stw_p(&cpkt.value, 1);
 
             buffer_len = sizeof(cpkt) + strlen(port->name) + 1;
             buffer = g_malloc(buffer_len);
@@ -536,7 +537,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
     qemu_put_be32s(f, &s->config.max_nr_ports);
 
     /* The ports map */
-    max_nr_ports = tswap32(s->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(s->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         qemu_put_be32s(f, &s->ports_map[i]);
     }
@@ -690,8 +691,8 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     qemu_get_be16s(f, &s->config.rows);
 
     qemu_get_be32s(f, &max_nr_ports);
-    tswap32s(&max_nr_ports);
-    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
+    virtio_tswap32s(&max_nr_ports);
+    if (max_nr_ports > virtio_tswap32(s->config.max_nr_ports)) {
         /* Source could have had more ports than us. Fail migration. */
         return -EINVAL;
     }
@@ -760,7 +761,7 @@ static uint32_t find_free_port_id(VirtIOSerial *vser)
 {
     unsigned int i, max_nr_ports;
 
-    max_nr_ports = tswap32(vser->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(vser->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         uint32_t map, bit;
 
@@ -846,7 +847,7 @@ static int virtser_port_qdev_init(DeviceState *qdev)
         }
     }
 
-    max_nr_ports = tswap32(port->vser->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(port->vser->config.max_nr_ports);
     if (port->id >= max_nr_ports) {
         error_report("virtio-serial-bus: Out-of-range port id specified, max. allowed: %u",
                      max_nr_ports - 1);
@@ -946,7 +947,8 @@ static int virtio_serial_device_init(VirtIODevice *vdev)
         vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
     }
 
-    vser->config.max_nr_ports = tswap32(vser->serial.max_virtserial_ports);
+    vser->config.max_nr_ports =
+	virtio_tswap32(vser->serial.max_virtserial_ports);
     vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32)
         * sizeof(vser->ports_map[0]));
     /*
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH 7/7] hw/char/virtio-serial-bus: use virtio wrappers to access headers.

[Anthony Liguori]

Rusty Russell <rusty@rustcorp.com.au> writes:

> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  hw/char/virtio-serial-bus.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index cc3d1dd..0421725 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -24,6 +24,7 @@
>  #include "hw/sysbus.h"
>  #include "trace.h"
>  #include "hw/virtio/virtio-serial.h"
> +#include "hw/virtio/virtio-access.h"
>  
>  static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
>  {
> @@ -185,9 +186,9 @@ static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id,
>  {
>      struct virtio_console_control cpkt;
>  
> -    stl_p(&cpkt.id, port_id);
> -    stw_p(&cpkt.event, event);
> -    stw_p(&cpkt.value, value);
> +    virtio_stl_p(&cpkt.id, port_id);
> +    virtio_stw_p(&cpkt.event, event);
> +    virtio_stw_p(&cpkt.value, value);
>  
>      trace_virtio_serial_send_control_event(port_id, event, value);
>      return send_control_msg(vser, &cpkt, sizeof(cpkt));
> @@ -291,8 +292,8 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
>          return;
>      }
>  
> -    cpkt.event = lduw_p(&gcpkt->event);
> -    cpkt.value = lduw_p(&gcpkt->value);
> +    cpkt.event = virtio_lduw_p(&gcpkt->event);
> +    cpkt.value = virtio_lduw_p(&gcpkt->value);
>  
>      trace_virtio_serial_handle_control_message(cpkt.event, cpkt.value);
>  
> @@ -312,10 +313,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
>          return;
>      }
>  
> -    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> +    port = find_port_by_id(vser, virtio_ldl_p(&gcpkt->id));
>      if (!port) {
>          error_report("virtio-serial-bus: Unexpected port id %u for device %s",
> -                     ldl_p(&gcpkt->id), vser->bus.qbus.name);
> +                     virtio_ldl_p(&gcpkt->id), vser->bus.qbus.name);
>          return;
>      }
>  
> @@ -342,9 +343,9 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
>          }
>  
>          if (port->name) {
> -            stl_p(&cpkt.id, port->id);
> -            stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
> -            stw_p(&cpkt.value, 1);
> +            virtio_stl_p(&cpkt.id, port->id);
> +            virtio_stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
> +            virtio_stw_p(&cpkt.value, 1);
>  
>              buffer_len = sizeof(cpkt) + strlen(port->name) + 1;
>              buffer = g_malloc(buffer_len);
> @@ -536,7 +537,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
>      qemu_put_be32s(f, &s->config.max_nr_ports);
>  
>      /* The ports map */
> -    max_nr_ports = tswap32(s->config.max_nr_ports);
> +    max_nr_ports = virtio_tswap32(s->config.max_nr_ports);
>      for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
>          qemu_put_be32s(f, &s->ports_map[i]);
>      }
> @@ -690,8 +691,8 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
>      qemu_get_be16s(f, &s->config.rows);
>  
>      qemu_get_be32s(f, &max_nr_ports);
> -    tswap32s(&max_nr_ports);
> -    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> +    virtio_tswap32s(&max_nr_ports);
> +    if (max_nr_ports > virtio_tswap32(s->config.max_nr_ports)) {
>          /* Source could have had more ports than us. Fail migration. */
>          return -EINVAL;
>      }
> @@ -760,7 +761,7 @@ static uint32_t find_free_port_id(VirtIOSerial *vser)
>  {
>      unsigned int i, max_nr_ports;
>  
> -    max_nr_ports = tswap32(vser->config.max_nr_ports);
> +    max_nr_ports = virtio_tswap32(vser->config.max_nr_ports);
>      for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
>          uint32_t map, bit;
>  
> @@ -846,7 +847,7 @@ static int virtser_port_qdev_init(DeviceState *qdev)
>          }
>      }
>  
> -    max_nr_ports = tswap32(port->vser->config.max_nr_ports);
> +    max_nr_ports = virtio_tswap32(port->vser->config.max_nr_ports);
>      if (port->id >= max_nr_ports) {
>          error_report("virtio-serial-bus: Out-of-range port id specified, max. allowed: %u",
>                       max_nr_ports - 1);
> @@ -946,7 +947,8 @@ static int virtio_serial_device_init(VirtIODevice *vdev)
>          vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
>      }
>  
> -    vser->config.max_nr_ports = tswap32(vser->serial.max_virtserial_ports);
> +    vser->config.max_nr_ports =
> +	virtio_tswap32(vser->serial.max_virtserial_ports);
>      vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32)
>          * sizeof(vser->ports_map[0]));
>      /*
> -- 
> 1.8.1.2

[Qemu-devel] [PATCH 7/7] patch virtio-serial-biendian.patch

[Rusty Russell]

---
 hw/char/virtio-serial-bus.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index cc3d1dd..0421725 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -24,6 +24,7 @@
 #include "hw/sysbus.h"
 #include "trace.h"
 #include "hw/virtio/virtio-serial.h"
+#include "hw/virtio/virtio-access.h"
 
 static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
 {
@@ -185,9 +186,9 @@ static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id,
 {
     struct virtio_console_control cpkt;
 
-    stl_p(&cpkt.id, port_id);
-    stw_p(&cpkt.event, event);
-    stw_p(&cpkt.value, value);
+    virtio_stl_p(&cpkt.id, port_id);
+    virtio_stw_p(&cpkt.event, event);
+    virtio_stw_p(&cpkt.value, value);
 
     trace_virtio_serial_send_control_event(port_id, event, value);
     return send_control_msg(vser, &cpkt, sizeof(cpkt));
@@ -291,8 +292,8 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         return;
     }
 
-    cpkt.event = lduw_p(&gcpkt->event);
-    cpkt.value = lduw_p(&gcpkt->value);
+    cpkt.event = virtio_lduw_p(&gcpkt->event);
+    cpkt.value = virtio_lduw_p(&gcpkt->value);
 
     trace_virtio_serial_handle_control_message(cpkt.event, cpkt.value);
 
@@ -312,10 +313,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         return;
     }
 
-    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
+    port = find_port_by_id(vser, virtio_ldl_p(&gcpkt->id));
     if (!port) {
         error_report("virtio-serial-bus: Unexpected port id %u for device %s",
-                     ldl_p(&gcpkt->id), vser->bus.qbus.name);
+                     virtio_ldl_p(&gcpkt->id), vser->bus.qbus.name);
         return;
     }
 
@@ -342,9 +343,9 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         }
 
         if (port->name) {
-            stl_p(&cpkt.id, port->id);
-            stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
-            stw_p(&cpkt.value, 1);
+            virtio_stl_p(&cpkt.id, port->id);
+            virtio_stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
+            virtio_stw_p(&cpkt.value, 1);
 
             buffer_len = sizeof(cpkt) + strlen(port->name) + 1;
             buffer = g_malloc(buffer_len);
@@ -536,7 +537,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
     qemu_put_be32s(f, &s->config.max_nr_ports);
 
     /* The ports map */
-    max_nr_ports = tswap32(s->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(s->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         qemu_put_be32s(f, &s->ports_map[i]);
     }
@@ -690,8 +691,8 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     qemu_get_be16s(f, &s->config.rows);
 
     qemu_get_be32s(f, &max_nr_ports);
-    tswap32s(&max_nr_ports);
-    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
+    virtio_tswap32s(&max_nr_ports);
+    if (max_nr_ports > virtio_tswap32(s->config.max_nr_ports)) {
         /* Source could have had more ports than us. Fail migration. */
         return -EINVAL;
     }
@@ -760,7 +761,7 @@ static uint32_t find_free_port_id(VirtIOSerial *vser)
 {
     unsigned int i, max_nr_ports;
 
-    max_nr_ports = tswap32(vser->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(vser->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         uint32_t map, bit;
 
@@ -846,7 +847,7 @@ static int virtser_port_qdev_init(DeviceState *qdev)
         }
     }
 
-    max_nr_ports = tswap32(port->vser->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(port->vser->config.max_nr_ports);
     if (port->id >= max_nr_ports) {
         error_report("virtio-serial-bus: Out-of-range port id specified, max. allowed: %u",
                      max_nr_ports - 1);
@@ -946,7 +947,8 @@ static int virtio_serial_device_init(VirtIODevice *vdev)
         vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
     }
 
-    vser->config.max_nr_ports = tswap32(vser->serial.max_virtserial_ports);
+    vser->config.max_nr_ports =
+	virtio_tswap32(vser->serial.max_virtserial_ports);
     vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32)
         * sizeof(vser->ports_map[0]));
     /*
-- 
1.8.1.2

[Qemu-devel] [PATCH 0/7] virtio endian-ambivalent target fixes.

[Rusty Russell]

This is a re-transmit of the core of the virtio endian code.  Since
there seems to be some interest in ARM BE virtio, I've separated this from
the direct problem I was solving: PowerPC LE.

Please apply!
Rusty.

Rusty Russell (7):
  virtio_get_byteswap: function for endian-ambivalent targets using
    virtio.
  virtio: allow byte swapping for vring and config access
  hw/net/virtio-net: use virtio wrappers to access headers.
  hw/net/virtio-balloon: use virtio wrappers to access page frame
    numbers.
  hw/block/virtio-blk: use virtio wrappers to access headers.
  hw/scsi/virtio-scsi: use virtio wrappers to access headers.
  hw/char/virtio-serial-bus: use virtio wrappers to access headers.

 hw/block/virtio-blk.c             |  35 +++++-----
 hw/char/virtio-serial-bus.c       |  34 +++++-----
 hw/net/virtio-net.c               |  15 +++--
 hw/scsi/virtio-scsi.c             |  33 +++++-----
 hw/virtio/virtio-balloon.c        |   3 +-
 hw/virtio/virtio.c                |  34 ++++++----
 include/hw/virtio/virtio-access.h | 133 ++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h        |   2 +
 stubs/Makefile.objs               |   1 +
 stubs/virtio_get_byteswap.c       |   6 ++
 10 files changed, 225 insertions(+), 71 deletions(-)
 create mode 100644 include/hw/virtio/virtio-access.h
 create mode 100644 stubs/virtio_get_byteswap.c

-- 
1.8.1.2

[Qemu-devel] [PATCH 6/7] hw/scsi/virtio-scsi: use virtio wrappers to access headers.

[Rusty Russell]

Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/scsi/virtio-scsi.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 26d95a1..e581dd0 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -18,6 +18,7 @@
 #include <hw/scsi/scsi.h>
 #include <block/scsi.h>
 #include <hw/virtio/virtio-bus.h>
+#include "hw/virtio/virtio-access.h"
 
 typedef struct VirtIOSCSIReq {
     VirtIOSCSI *dev;
@@ -309,12 +310,12 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
     req->resp.cmd->response = VIRTIO_SCSI_S_OK;
     req->resp.cmd->status = status;
     if (req->resp.cmd->status == GOOD) {
-        req->resp.cmd->resid = tswap32(resid);
+        req->resp.cmd->resid = virtio_tswap32(resid);
     } else {
         req->resp.cmd->resid = 0;
         sense_len = scsi_req_get_sense(r, req->resp.cmd->sense,
                                        VIRTIO_SCSI_SENSE_SIZE);
-        req->resp.cmd->sense_len = tswap32(sense_len);
+        req->resp.cmd->sense_len = virtio_tswap32(sense_len);
     }
     virtio_scsi_complete_req(req);
 }
@@ -410,16 +411,16 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
     VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
     VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
 
-    stl_raw(&scsiconf->num_queues, s->conf.num_queues);
-    stl_raw(&scsiconf->seg_max, 128 - 2);
-    stl_raw(&scsiconf->max_sectors, s->conf.max_sectors);
-    stl_raw(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
-    stl_raw(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
-    stl_raw(&scsiconf->sense_size, s->sense_size);
-    stl_raw(&scsiconf->cdb_size, s->cdb_size);
-    stw_raw(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
-    stw_raw(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
-    stl_raw(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
+    virtio_stl_p(&scsiconf->num_queues, s->conf.num_queues);
+    virtio_stl_p(&scsiconf->seg_max, 128 - 2);
+    virtio_stl_p(&scsiconf->max_sectors, s->conf.max_sectors);
+    virtio_stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
+    virtio_stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
+    virtio_stl_p(&scsiconf->sense_size, s->sense_size);
+    virtio_stl_p(&scsiconf->cdb_size, s->cdb_size);
+    virtio_stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
+    virtio_stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
+    virtio_stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
 }
 
 static void virtio_scsi_set_config(VirtIODevice *vdev,
@@ -428,14 +429,14 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
     VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
 
-    if ((uint32_t) ldl_raw(&scsiconf->sense_size) >= 65536 ||
-        (uint32_t) ldl_raw(&scsiconf->cdb_size) >= 256) {
+    if ((uint32_t) virtio_ldl_p(&scsiconf->sense_size) >= 65536 ||
+        (uint32_t) virtio_ldl_p(&scsiconf->cdb_size) >= 256) {
         error_report("bad data written to virtio-scsi configuration space");
         exit(1);
     }
 
-    vs->sense_size = ldl_raw(&scsiconf->sense_size);
-    vs->cdb_size = ldl_raw(&scsiconf->cdb_size);
+    vs->sense_size = virtio_ldl_p(&scsiconf->sense_size);
+    vs->cdb_size = virtio_ldl_p(&scsiconf->cdb_size);
 }
 
 static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
-- 
1.8.1.2