[Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes

[Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes

[Greg Kurz]

Hi,

This serie addresses the comments made on v5. The main goal is to share
most of the code to support both the current legacy virtio and the yet
to come 1.0 implementations.

The changes since the last post are:
- introduce a per-device property to supersede the
  evil virtio_byteswap global
- pass VirtIODevice to virtio memory accessors so they can use
  the per-device property
- migration support
- fix SCSI event endianness issue in virtio-scsi

Thank you for your comments.

---
Greg

Greg Kurz (1):
      virtio-9p: use virtio wrappers to access headers

Rusty Russell (7):
      virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio
      virtio: allow byte swapping for vring and config access
      virtio-net: use virtio wrappers to access headers
      virtio-balloon: use virtio wrappers to access page frame numbers
      virtio-blk: use virtio wrappers to access headers
      virtio-scsi: use virtio wrappers to access headers
      virtio-serial-bus: use virtio wrappers to access headers


 hw/9pfs/virtio-9p-device.c        |    3 +
 hw/block/virtio-blk.c             |   40 ++++++-----
 hw/char/virtio-serial-bus.c       |   39 ++++++----
 hw/net/virtio-net.c               |   17 +++--
 hw/scsi/virtio-scsi.c             |   38 +++++-----
 hw/virtio/virtio-balloon.c        |    4 +
 hw/virtio/virtio.c                |   98 +++++++++++++++-----------
 include/hw/virtio/virtio-access.h |  139 +++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h        |    3 +
 stubs/Makefile.objs               |    1 
 stubs/virtio_get_byteswap.c       |    6 ++
 11 files changed, 288 insertions(+), 100 deletions(-)
 create mode 100644 include/hw/virtio/virtio-access.h
 create mode 100644 stubs/virtio_get_byteswap.c

[Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio

[Greg Kurz]

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

virtio data structures are defined as "target endian", which assumes
that's a fixed value.  In fact, that actually means it's platform-specific.
The OASIS virtio 1.0 spec will fix this, by making all little endian.

We need to support both implementations and we want to share as much code
as possible.

A good way to do it is to introduce a per-device boolean property to tell
memory accessors whether they should swap bytes or not. This flag should
be set at device reset time, because:
- endianness won't change while the device is in use, and if we reboot
  into a different endianness, a new device reset will occur
- as suggested by Alexander Graf, we can keep all the logic to set the
  property in a single place and share all the virtio memory accessors
  between the two implementations

For legacy devices, we rely on a per-platform hook to set the flag. The
virtio 1.0 implementation will just have to add some more logic in
virtio_reset() instead of calling the hook:

if (vdev->legacy) {
   vdev->needs_byteswap = virtio_legacy_get_byteswap();
} else {
#ifdef HOST_WORDS_BIGENDIAN
   vdev->needs_byteswap = true;
#else
   vdev->needs_byteswap = false;
#endif
}

The needs_byteswap flag is preserved accross migrations.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
[ fixed checkpatch.pl error with the virtio_byteswap initialisation,
  ldq_phys() API change,
  relicensed virtio-access.h to GPLv2+ on Rusty's request,
  introduce a per-device needs_byteswap flag,
  add VirtIODevice * arg to virtio helpers,
  rename virtio_get_byteswap to virtio_legacy_get_byteswap,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio.c                |    5 +
 include/hw/virtio/virtio-access.h |  139 +++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h        |    3 +
 stubs/Makefile.objs               |    1 
 stubs/virtio_get_byteswap.c       |    6 ++
 5 files changed, 154 insertions(+)
 create mode 100644 include/hw/virtio/virtio-access.h
 create mode 100644 stubs/virtio_get_byteswap.c

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index aeabf3a..24b565f 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -19,6 +19,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.
@@ -546,6 +547,8 @@ void virtio_reset(void *opaque)
 
     virtio_set_status(vdev, 0);
 
+    vdev->needs_byteswap = virtio_legacy_get_byteswap();
+
     if (k->reset) {
         k->reset(vdev);
     }
@@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
 
     qemu_put_8s(f, &vdev->status);
     qemu_put_8s(f, &vdev->isr);
+    qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap);
     qemu_put_be16s(f, &vdev->queue_sel);
     qemu_put_be32s(f, &vdev->guest_features);
     qemu_put_be32(f, vdev->config_len);
@@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
 
     qemu_get_8s(f, &vdev->status);
     qemu_get_8s(f, &vdev->isr);
+    qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap);
     qemu_get_be16s(f, &vdev->queue_sel);
     qemu_get_be32s(f, &features);
 
diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
new file mode 100644
index 0000000..70dd1e2
--- /dev/null
+++ b/include/hw/virtio/virtio-access.h
@@ -0,0 +1,139 @@
+/*
+ * Virtio Accessor Support: In case your target can change endian.
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ *  Rusty Russell   <rusty@au.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+#ifndef _QEMU_VIRTIO_ACCESS_H
+#define _QEMU_VIRTIO_ACCESS_H
+#include "hw/virtio/virtio.h"
+
+static inline uint16_t virtio_lduw_phys(AddressSpace *as, hwaddr pa,
+                                        struct VirtIODevice *vdev)
+{
+    if (vdev->needs_byteswap) {
+        return bswap16(lduw_phys(as, pa));
+    }
+    return lduw_phys(as, pa);
+}
+
+static inline uint32_t virtio_ldl_phys(AddressSpace *as, hwaddr pa,
+                                       struct VirtIODevice *vdev)
+{
+    if (vdev->needs_byteswap) {
+        return bswap32(ldl_phys(as, pa));
+    }
+    return ldl_phys(as, pa);
+}
+
+static inline uint64_t virtio_ldq_phys(AddressSpace *as, hwaddr pa,
+                                       struct VirtIODevice *vdev)
+{
+    if (vdev->needs_byteswap) {
+        return bswap64(ldq_phys(as, pa));
+    }
+    return ldq_phys(as, pa);
+}
+
+static inline void virtio_stw_phys(AddressSpace *as, hwaddr pa, uint16_t value,
+                                   struct VirtIODevice *vdev)
+{
+    if (vdev->needs_byteswap) {
+        stw_phys(as, pa, bswap16(value));
+    } else {
+        stw_phys(as, pa, value);
+    }
+}
+
+static inline void virtio_stl_phys(AddressSpace *as, hwaddr pa, uint32_t value,
+                                   struct VirtIODevice *vdev)
+{
+    if (vdev->needs_byteswap) {
+        stl_phys(as, pa, bswap32(value));
+    } else {
+        stl_phys(as, pa, value);
+    }
+}
+
+static inline void virtio_stw_p(void *ptr, uint16_t v,
+                                struct VirtIODevice *vdev)
+{
+    if (vdev->needs_byteswap) {
+        stw_p(ptr, bswap16(v));
+    } else {
+        stw_p(ptr, v);
+    }
+}
+
+static inline void virtio_stl_p(void *ptr, uint32_t v,
+                                struct VirtIODevice *vdev)
+{
+    if (vdev->needs_byteswap) {
+        stl_p(ptr, bswap32(v));
+    } else {
+        stl_p(ptr, v);
+    }
+}
+
+static inline void virtio_stq_p(void *ptr, uint64_t v,
+                                struct VirtIODevice *vdev)
+{
+    if (vdev->needs_byteswap) {
+        stq_p(ptr, bswap64(v));
+    } else {
+        stq_p(ptr, v);
+    }
+}
+
+static inline int virtio_lduw_p(const void *ptr, struct VirtIODevice *vdev)
+{
+    if (vdev->needs_byteswap) {
+        return bswap16(lduw_p(ptr));
+    } else {
+        return lduw_p(ptr);
+    }
+}
+
+static inline int virtio_ldl_p(const void *ptr, struct VirtIODevice *vdev)
+{
+    if (vdev->needs_byteswap) {
+        return bswap32(ldl_p(ptr));
+    } else {
+        return ldl_p(ptr);
+    }
+}
+
+static inline uint64_t virtio_ldq_p(const void *ptr, struct VirtIODevice *vdev)
+{
+    if (vdev->needs_byteswap) {
+        return bswap64(ldq_p(ptr));
+    } else {
+        return ldq_p(ptr);
+    }
+}
+
+static inline uint32_t virtio_tswap32(uint32_t s, struct VirtIODevice *vdev)
+{
+    if (vdev->needs_byteswap) {
+        return bswap32(tswap32(s));
+    } else {
+        return tswap32(s);
+    }
+}
+
+static inline void virtio_tswap32s(uint32_t *s, struct VirtIODevice *vdev)
+{
+    tswap32s(s);
+    if (vdev->needs_byteswap) {
+        *s = bswap32(*s);
+    }
+}
+#endif /* _QEMU_VIRTIO_ACCESS_H */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 3e54e90..3595da5 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -121,6 +121,7 @@ struct VirtIODevice
     bool vm_running;
     VMChangeStateEntry *vmstate;
     char *bus_name;
+    bool needs_byteswap;
 };
 
 typedef struct VirtioDeviceClass {
@@ -253,4 +254,6 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler);
 void virtio_queue_notify_vq(VirtQueue *vq);
 void virtio_irq(VirtQueue *vq);
+
+extern bool virtio_legacy_get_byteswap(void);
 #endif
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 5ed1d38..a13381a 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -30,3 +30,4 @@ stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += cpus.o
 stub-obj-y += kvm.o
+stub-obj-y += virtio_get_byteswap.o
diff --git a/stubs/virtio_get_byteswap.c b/stubs/virtio_get_byteswap.c
new file mode 100644
index 0000000..28af5e3
--- /dev/null
+++ b/stubs/virtio_get_byteswap.c
@@ -0,0 +1,6 @@
+#include "hw/virtio/virtio.h"
+
+bool virtio_legacy_get_byteswap(void)
+{
+    return false;
+}

Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio

[Thomas Huth]

On Fri, 28 Mar 2014 11:57:17 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> From: Rusty Russell <rusty@rustcorp.com.au>
> 
> virtio data structures are defined as "target endian", which assumes
> that's a fixed value.  In fact, that actually means it's platform-specific.
> The OASIS virtio 1.0 spec will fix this, by making all little endian.
> 
> We need to support both implementations and we want to share as much code
> as possible.
> 
> A good way to do it is to introduce a per-device boolean property to tell
> memory accessors whether they should swap bytes or not. This flag should
> be set at device reset time, because:
> - endianness won't change while the device is in use, and if we reboot
>   into a different endianness, a new device reset will occur
> - as suggested by Alexander Graf, we can keep all the logic to set the
>   property in a single place and share all the virtio memory accessors
>   between the two implementations
> 
> For legacy devices, we rely on a per-platform hook to set the flag. The
> virtio 1.0 implementation will just have to add some more logic in
> virtio_reset() instead of calling the hook:
> 
> if (vdev->legacy) {
>    vdev->needs_byteswap = virtio_legacy_get_byteswap();
> } else {
> #ifdef HOST_WORDS_BIGENDIAN
>    vdev->needs_byteswap = true;
> #else
>    vdev->needs_byteswap = false;
> #endif
> }
> 
> The needs_byteswap flag is preserved accross migrations.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
>   ldq_phys() API change,
>   relicensed virtio-access.h to GPLv2+ on Rusty's request,
>   introduce a per-device needs_byteswap flag,
>   add VirtIODevice * arg to virtio helpers,
>   rename virtio_get_byteswap to virtio_legacy_get_byteswap,
>   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/virtio/virtio.c                |    5 +
>  include/hw/virtio/virtio-access.h |  139 +++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio.h        |    3 +
>  stubs/Makefile.objs               |    1 
>  stubs/virtio_get_byteswap.c       |    6 ++
>  5 files changed, 154 insertions(+)
>  create mode 100644 include/hw/virtio/virtio-access.h
>  create mode 100644 stubs/virtio_get_byteswap.c
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index aeabf3a..24b565f 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -19,6 +19,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.
> @@ -546,6 +547,8 @@ void virtio_reset(void *opaque)
> 
>      virtio_set_status(vdev, 0);
> 
> +    vdev->needs_byteswap = virtio_legacy_get_byteswap();
> +
>      if (k->reset) {
>          k->reset(vdev);
>      }
> @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> 
>      qemu_put_8s(f, &vdev->status);
>      qemu_put_8s(f, &vdev->isr);
> +    qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap);
>      qemu_put_be16s(f, &vdev->queue_sel);
>      qemu_put_be32s(f, &vdev->guest_features);
>      qemu_put_be32(f, vdev->config_len);
> @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> 
>      qemu_get_8s(f, &vdev->status);
>      qemu_get_8s(f, &vdev->isr);
> +    qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap);
>      qemu_get_be16s(f, &vdev->queue_sel);
>      qemu_get_be32s(f, &features);

Two remarks here:

1) You've declared needs_byteswap as "bool", but in above code you
assume that the "bool" type is implemented as a single byte. That's
most likely true on most system, but AFAIK it's specific to the
compiler. So for portable code, I think you should either change the
type of needs_byteswap or you should use a temporary uint8_t variable
here instead.

2) You're changing the layout of the saved data ... don't you also have
to increase the version numbers in that case, too? (e.g. change the
version id for the register_savevm call in virtio-blk.c, virtio-net.c,
etc.)?

 Thomas

Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio

[Greg Kurz]

On Fri, 28 Mar 2014 15:15:46 +0100
Thomas Huth <thuth@linux.vnet.ibm.com> wrote:
> On Fri, 28 Mar 2014 11:57:17 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > From: Rusty Russell <rusty@rustcorp.com.au>
> > 
> > virtio data structures are defined as "target endian", which assumes
> > that's a fixed value.  In fact, that actually means it's platform-specific.
> > The OASIS virtio 1.0 spec will fix this, by making all little endian.
> > 
> > We need to support both implementations and we want to share as much code
> > as possible.
> > 
> > A good way to do it is to introduce a per-device boolean property to tell
> > memory accessors whether they should swap bytes or not. This flag should
> > be set at device reset time, because:
> > - endianness won't change while the device is in use, and if we reboot
> >   into a different endianness, a new device reset will occur
> > - as suggested by Alexander Graf, we can keep all the logic to set the
> >   property in a single place and share all the virtio memory accessors
> >   between the two implementations
> > 
> > For legacy devices, we rely on a per-platform hook to set the flag. The
> > virtio 1.0 implementation will just have to add some more logic in
> > virtio_reset() instead of calling the hook:
> > 
> > if (vdev->legacy) {
> >    vdev->needs_byteswap = virtio_legacy_get_byteswap();
> > } else {
> > #ifdef HOST_WORDS_BIGENDIAN
> >    vdev->needs_byteswap = true;
> > #else
> >    vdev->needs_byteswap = false;
> > #endif
> > }
> > 
> > The needs_byteswap flag is preserved accross migrations.
> > 
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> >   ldq_phys() API change,
> >   relicensed virtio-access.h to GPLv2+ on Rusty's request,
> >   introduce a per-device needs_byteswap flag,
> >   add VirtIODevice * arg to virtio helpers,
> >   rename virtio_get_byteswap to virtio_legacy_get_byteswap,
> >   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  hw/virtio/virtio.c                |    5 +
> >  include/hw/virtio/virtio-access.h |  139 +++++++++++++++++++++++++++++++++++++
> >  include/hw/virtio/virtio.h        |    3 +
> >  stubs/Makefile.objs               |    1 
> >  stubs/virtio_get_byteswap.c       |    6 ++
> >  5 files changed, 154 insertions(+)
> >  create mode 100644 include/hw/virtio/virtio-access.h
> >  create mode 100644 stubs/virtio_get_byteswap.c
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index aeabf3a..24b565f 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -19,6 +19,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.
> > @@ -546,6 +547,8 @@ void virtio_reset(void *opaque)
> > 
> >      virtio_set_status(vdev, 0);
> > 
> > +    vdev->needs_byteswap = virtio_legacy_get_byteswap();
> > +
> >      if (k->reset) {
> >          k->reset(vdev);
> >      }
> > @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> > 
> >      qemu_put_8s(f, &vdev->status);
> >      qemu_put_8s(f, &vdev->isr);
> > +    qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap);
> >      qemu_put_be16s(f, &vdev->queue_sel);
> >      qemu_put_be32s(f, &vdev->guest_features);
> >      qemu_put_be32(f, vdev->config_len);
> > @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> > 
> >      qemu_get_8s(f, &vdev->status);
> >      qemu_get_8s(f, &vdev->isr);
> > +    qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap);
> >      qemu_get_be16s(f, &vdev->queue_sel);
> >      qemu_get_be32s(f, &features);
> 
> Two remarks here:
> 
> 1) You've declared needs_byteswap as "bool", but in above code you
> assume that the "bool" type is implemented as a single byte. That's
> most likely true on most system, but AFAIK it's specific to the
> compiler. So for portable code, I think you should either change the
> type of needs_byteswap or you should use a temporary uint8_t variable
> here instead.
> 

Thomas,

I guess turning needs_byteswap into a uint8_t is ok and less intrusive
for the code.

> 2) You're changing the layout of the saved data ... don't you also have
> to increase the version numbers in that case, too? (e.g. change the
> version id for the register_savevm call in virtio-blk.c, virtio-net.c,
> etc.)?
> 

Oops, my bad ! :-\

>  Thomas

Thanks for your time.

Cheers.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio

[Andreas Färber]

Am 28.03.2014 11:57, schrieb Greg Kurz:
> From: Rusty Russell <rusty@rustcorp.com.au>
> 
> virtio data structures are defined as "target endian", which assumes
> that's a fixed value.  In fact, that actually means it's platform-specific.
> The OASIS virtio 1.0 spec will fix this, by making all little endian.
> 
> We need to support both implementations and we want to share as much code
> as possible.
> 
> A good way to do it is to introduce a per-device boolean property to tell
> memory accessors whether they should swap bytes or not. This flag should
> be set at device reset time, because:
> - endianness won't change while the device is in use, and if we reboot
>   into a different endianness, a new device reset will occur
> - as suggested by Alexander Graf, we can keep all the logic to set the
>   property in a single place and share all the virtio memory accessors
>   between the two implementations
> 
> For legacy devices, we rely on a per-platform hook to set the flag. The
> virtio 1.0 implementation will just have to add some more logic in
> virtio_reset() instead of calling the hook:
> 
> if (vdev->legacy) {
>    vdev->needs_byteswap = virtio_legacy_get_byteswap();
> } else {
> #ifdef HOST_WORDS_BIGENDIAN
>    vdev->needs_byteswap = true;
> #else
>    vdev->needs_byteswap = false;
> #endif
> }
> 
> The needs_byteswap flag is preserved accross migrations.

"across"

Why? :) For all targets except ppc this field does not change during
runtime. Do you intend to support ppc64 <-> ppc64le migration, i.e.
protection against changing HOST_WORDS_BIGENDIAN?

Since you're setting the field on reset rather than in instance_init or
realize, resetting the device on one host with changing ILE may lead to
weird endianness settings: Devices are initially reset before the VM
starts. ILE will always be Big Endian then IIUC, since all PowerPCCPU
models default to Big Endian and SLOF runs Big Endian. SLOF might use a
virtio-blk device to boot from. We then boot into SLES12 ppc64le - ILE
indicates Little Endian now. As soon as the guest triggers a reset of
the device, such as by resetting the whole PCI bus, endianness changes
to Little Endian. Now you indeed have an endianness on the source that
is different from that of the newly Big Endian reset device on the
destination. Is this desired, or did you accidentally initialize the
flag in the wrong place?

If we do need it, maybe you can place the field into a subsection to
avoid imposing it on x86?

Regards,
Andreas

> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
>   ldq_phys() API change,
>   relicensed virtio-access.h to GPLv2+ on Rusty's request,
>   introduce a per-device needs_byteswap flag,
>   add VirtIODevice * arg to virtio helpers,
>   rename virtio_get_byteswap to virtio_legacy_get_byteswap,
>   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

-- 
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 v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio

[Greg Kurz]

On Fri, 28 Mar 2014 18:59:22 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 28.03.2014 11:57, schrieb Greg Kurz:
> > From: Rusty Russell <rusty@rustcorp.com.au>
> > 
> > virtio data structures are defined as "target endian", which assumes
> > that's a fixed value.  In fact, that actually means it's platform-specific.
> > The OASIS virtio 1.0 spec will fix this, by making all little endian.
> > 
> > We need to support both implementations and we want to share as much code
> > as possible.
> > 
> > A good way to do it is to introduce a per-device boolean property to tell
> > memory accessors whether they should swap bytes or not. This flag should
> > be set at device reset time, because:
> > - endianness won't change while the device is in use, and if we reboot
> >   into a different endianness, a new device reset will occur
> > - as suggested by Alexander Graf, we can keep all the logic to set the
> >   property in a single place and share all the virtio memory accessors
> >   between the two implementations
> > 
> > For legacy devices, we rely on a per-platform hook to set the flag. The
> > virtio 1.0 implementation will just have to add some more logic in
> > virtio_reset() instead of calling the hook:
> > 
> > if (vdev->legacy) {
> >    vdev->needs_byteswap = virtio_legacy_get_byteswap();
> > } else {
> > #ifdef HOST_WORDS_BIGENDIAN
> >    vdev->needs_byteswap = true;
> > #else
> >    vdev->needs_byteswap = false;
> > #endif
> > }
> > 
> > The needs_byteswap flag is preserved accross migrations.
> 
> "across"
> 
> Why? :) For all targets except ppc this field does not change during
> runtime. Do you intend to support ppc64 <-> ppc64le migration, i.e.
> protection against changing HOST_WORDS_BIGENDIAN?
> 

Because we only set this property at virtio_reset() time... how can
we ensure it still has the correct value after a migration then ?

And no, I don't intend to support cross-endian migration... The
only endianness change that we can support is rebooting into a
different endianness.

> Since you're setting the field on reset rather than in instance_init or
> realize, resetting the device on one host with changing ILE may lead to
> weird endianness settings: Devices are initially reset before the VM
> starts. ILE will always be Big Endian then IIUC, since all PowerPCCPU
> models default to Big Endian and SLOF runs Big Endian. SLOF might use a
> virtio-blk device to boot from. We then boot into SLES12 ppc64le - ILE 
> indicates Little Endian now. As soon as the guest triggers a reset of
> the device, such as by resetting the whole PCI bus, endianness changes
> to Little Endian. Now you indeed have an endianness on the source that
> is different from that of the newly Big Endian reset device on the
> destination. Is this desired, or did you accidentally initialize the
> flag in the wrong place?
> 

Hmmm... the assumption is that ALL virtio devices get reset after the guest
kernel switches to LE. Are you saying this is not the case if SLOF uses
a virtio-blk device to boot from ? This device would be handed over to
the guest kernel to be used as is ? If yes, then I don't see how we can
cope with that... The way legacy virtio is implemented, we cannot
reasonably support an endianness change without fully reseting the device.

I guess this is the main motivation behind virtio 1.0 :)

> If we do need it, maybe you can place the field into a subsection to
> avoid imposing it on x86?
> 

I think it is needed anyway as long as we want to support a ppc64 guest
that can change endianness and uses legacy virtio devices, even with
a x86 host.

> Regards,
> Andreas
> 
> > 
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> >   ldq_phys() API change,
> >   relicensed virtio-access.h to GPLv2+ on Rusty's request,
> >   introduce a per-device needs_byteswap flag,
> >   add VirtIODevice * arg to virtio helpers,
> >   rename virtio_get_byteswap to virtio_legacy_get_byteswap,
> >   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> 

Cheers.


-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio

[Alexander Graf]

On 03/28/2014 11:57 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> virtio data structures are defined as "target endian", which assumes
> that's a fixed value.  In fact, that actually means it's platform-specific.
> The OASIS virtio 1.0 spec will fix this, by making all little endian.
>
> We need to support both implementations and we want to share as much code
> as possible.
>
> A good way to do it is to introduce a per-device boolean property to tell
> memory accessors whether they should swap bytes or not. This flag should
> be set at device reset time, because:
> - endianness won't change while the device is in use, and if we reboot
>    into a different endianness, a new device reset will occur
> - as suggested by Alexander Graf, we can keep all the logic to set the
>    property in a single place and share all the virtio memory accessors
>    between the two implementations
>
> For legacy devices, we rely on a per-platform hook to set the flag. The
> virtio 1.0 implementation will just have to add some more logic in
> virtio_reset() instead of calling the hook:
>
> if (vdev->legacy) {
>     vdev->needs_byteswap = virtio_legacy_get_byteswap();
> } else {
> #ifdef HOST_WORDS_BIGENDIAN
>     vdev->needs_byteswap = true;
> #else
>     vdev->needs_byteswap = false;
> #endif
> }
>
> The needs_byteswap flag is preserved accross migrations.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
>    ldq_phys() API change,
>    relicensed virtio-access.h to GPLv2+ on Rusty's request,
>    introduce a per-device needs_byteswap flag,
>    add VirtIODevice * arg to virtio helpers,
>    rename virtio_get_byteswap to virtio_legacy_get_byteswap,
>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>   hw/virtio/virtio.c                |    5 +
>   include/hw/virtio/virtio-access.h |  139 +++++++++++++++++++++++++++++++++++++
>   include/hw/virtio/virtio.h        |    3 +
>   stubs/Makefile.objs               |    1
>   stubs/virtio_get_byteswap.c       |    6 ++
>   5 files changed, 154 insertions(+)
>   create mode 100644 include/hw/virtio/virtio-access.h
>   create mode 100644 stubs/virtio_get_byteswap.c
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index aeabf3a..24b565f 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -19,6 +19,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.
> @@ -546,6 +547,8 @@ void virtio_reset(void *opaque)
>   
>       virtio_set_status(vdev, 0);
>   
> +    vdev->needs_byteswap = virtio_legacy_get_byteswap();
> +
>       if (k->reset) {
>           k->reset(vdev);
>       }
> @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>   
>       qemu_put_8s(f, &vdev->status);
>       qemu_put_8s(f, &vdev->isr);
> +    qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap);
>       qemu_put_be16s(f, &vdev->queue_sel);
>       qemu_put_be32s(f, &vdev->guest_features);
>       qemu_put_be32(f, vdev->config_len);
> @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>   
>       qemu_get_8s(f, &vdev->status);
>       qemu_get_8s(f, &vdev->isr);
> +    qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap);
>       qemu_get_be16s(f, &vdev->queue_sel);
>       qemu_get_be32s(f, &features);
>   
> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> new file mode 100644
> index 0000000..70dd1e2
> --- /dev/null
> +++ b/include/hw/virtio/virtio-access.h
> @@ -0,0 +1,139 @@
> +/*
> + * Virtio Accessor Support: In case your target can change endian.
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * Authors:
> + *  Rusty Russell   <rusty@au.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +#ifndef _QEMU_VIRTIO_ACCESS_H
> +#define _QEMU_VIRTIO_ACCESS_H
> +#include "hw/virtio/virtio.h"
> +
> +static inline uint16_t virtio_lduw_phys(AddressSpace *as, hwaddr pa,
> +                                        struct VirtIODevice *vdev)
> +{
> +    if (vdev->needs_byteswap) {
> +        return bswap16(lduw_phys(as, pa));
> +    }
> +    return lduw_phys(as, pa);
> +}
> +
> +static inline uint32_t virtio_ldl_phys(AddressSpace *as, hwaddr pa,
> +                                       struct VirtIODevice *vdev)
> +{
> +    if (vdev->needs_byteswap) {
> +        return bswap32(ldl_phys(as, pa));
> +    }
> +    return ldl_phys(as, pa);
> +}
> +
> +static inline uint64_t virtio_ldq_phys(AddressSpace *as, hwaddr pa,
> +                                       struct VirtIODevice *vdev)
> +{
> +    if (vdev->needs_byteswap) {
> +        return bswap64(ldq_phys(as, pa));
> +    }
> +    return ldq_phys(as, pa);
> +}
> +
> +static inline void virtio_stw_phys(AddressSpace *as, hwaddr pa, uint16_t value,
> +                                   struct VirtIODevice *vdev)
> +{
> +    if (vdev->needs_byteswap) {
> +        stw_phys(as, pa, bswap16(value));
> +    } else {
> +        stw_phys(as, pa, value);
> +    }
> +}
> +
> +static inline void virtio_stl_phys(AddressSpace *as, hwaddr pa, uint32_t value,
> +                                   struct VirtIODevice *vdev)
> +{
> +    if (vdev->needs_byteswap) {
> +        stl_phys(as, pa, bswap32(value));
> +    } else {
> +        stl_phys(as, pa, value);
> +    }
> +}
> +
> +static inline void virtio_stw_p(void *ptr, uint16_t v,
> +                                struct VirtIODevice *vdev)
> +{
> +    if (vdev->needs_byteswap) {
> +        stw_p(ptr, bswap16(v));
> +    } else {
> +        stw_p(ptr, v);
> +    }
> +}
> +
> +static inline void virtio_stl_p(void *ptr, uint32_t v,
> +                                struct VirtIODevice *vdev)
> +{
> +    if (vdev->needs_byteswap) {
> +        stl_p(ptr, bswap32(v));
> +    } else {
> +        stl_p(ptr, v);
> +    }
> +}
> +
> +static inline void virtio_stq_p(void *ptr, uint64_t v,
> +                                struct VirtIODevice *vdev)
> +{
> +    if (vdev->needs_byteswap) {
> +        stq_p(ptr, bswap64(v));
> +    } else {
> +        stq_p(ptr, v);
> +    }
> +}
> +
> +static inline int virtio_lduw_p(const void *ptr, struct VirtIODevice *vdev)
> +{
> +    if (vdev->needs_byteswap) {
> +        return bswap16(lduw_p(ptr));
> +    } else {
> +        return lduw_p(ptr);
> +    }
> +}
> +
> +static inline int virtio_ldl_p(const void *ptr, struct VirtIODevice *vdev)
> +{
> +    if (vdev->needs_byteswap) {
> +        return bswap32(ldl_p(ptr));
> +    } else {
> +        return ldl_p(ptr);
> +    }
> +}
> +
> +static inline uint64_t virtio_ldq_p(const void *ptr, struct VirtIODevice *vdev)
> +{
> +    if (vdev->needs_byteswap) {
> +        return bswap64(ldq_p(ptr));
> +    } else {
> +        return ldq_p(ptr);
> +    }
> +}
> +
> +static inline uint32_t virtio_tswap32(uint32_t s, struct VirtIODevice *vdev)
> +{
> +    if (vdev->needs_byteswap) {
> +        return bswap32(tswap32(s));
> +    } else {
> +        return tswap32(s);
> +    }
> +}
> +
> +static inline void virtio_tswap32s(uint32_t *s, struct VirtIODevice *vdev)
> +{
> +    tswap32s(s);
> +    if (vdev->needs_byteswap) {
> +        *s = bswap32(*s);
> +    }

Couldn't you just reuse virtio_tswap32() here?

Also, I think a "needs_byteswap" flag gets confusing. Devices shouldn't 
have any access to CPU endian specific RAM accessors.

How about we introduce a flag like "is_bigendian" that we set depending 
on the CPU default endianness for legacy virtio. We can then change that 
flag accordingly. For the wrappers here, we could just use accessors 
like ldl_be_phys() or ldl_le_phys().

That should make things a lot easier to understand - and for virtio 1.0 
we only need to set is_bigendian = false :).


Alex

Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio

[Greg Kurz]

On Mon, 31 Mar 2014 16:50:55 +0200
Alexander Graf <agraf@suse.de> wrote:

> On 03/28/2014 11:57 AM, Greg Kurz wrote:
> > From: Rusty Russell <rusty@rustcorp.com.au>
> >
> > virtio data structures are defined as "target endian", which assumes
> > that's a fixed value.  In fact, that actually means it's platform-specific.
> > The OASIS virtio 1.0 spec will fix this, by making all little endian.
> >
> > We need to support both implementations and we want to share as much code
> > as possible.
> >
> > A good way to do it is to introduce a per-device boolean property to tell
> > memory accessors whether they should swap bytes or not. This flag should
> > be set at device reset time, because:
> > - endianness won't change while the device is in use, and if we reboot
> >    into a different endianness, a new device reset will occur
> > - as suggested by Alexander Graf, we can keep all the logic to set the
> >    property in a single place and share all the virtio memory accessors
> >    between the two implementations
> >
> > For legacy devices, we rely on a per-platform hook to set the flag. The
> > virtio 1.0 implementation will just have to add some more logic in
> > virtio_reset() instead of calling the hook:
> >
> > if (vdev->legacy) {
> >     vdev->needs_byteswap = virtio_legacy_get_byteswap();
> > } else {
> > #ifdef HOST_WORDS_BIGENDIAN
> >     vdev->needs_byteswap = true;
> > #else
> >     vdev->needs_byteswap = false;
> > #endif
> > }
> >
> > The needs_byteswap flag is preserved accross migrations.
> >
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> >    ldq_phys() API change,
> >    relicensed virtio-access.h to GPLv2+ on Rusty's request,
> >    introduce a per-device needs_byteswap flag,
> >    add VirtIODevice * arg to virtio helpers,
> >    rename virtio_get_byteswap to virtio_legacy_get_byteswap,
> >    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >   hw/virtio/virtio.c                |    5 +
> >   include/hw/virtio/virtio-access.h |  139 +++++++++++++++++++++++++++++++++++++
> >   include/hw/virtio/virtio.h        |    3 +
> >   stubs/Makefile.objs               |    1
> >   stubs/virtio_get_byteswap.c       |    6 ++
> >   5 files changed, 154 insertions(+)
> >   create mode 100644 include/hw/virtio/virtio-access.h
> >   create mode 100644 stubs/virtio_get_byteswap.c
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index aeabf3a..24b565f 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -19,6 +19,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.
> > @@ -546,6 +547,8 @@ void virtio_reset(void *opaque)
> >   
> >       virtio_set_status(vdev, 0);
> >   
> > +    vdev->needs_byteswap = virtio_legacy_get_byteswap();
> > +
> >       if (k->reset) {
> >           k->reset(vdev);
> >       }
> > @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >   
> >       qemu_put_8s(f, &vdev->status);
> >       qemu_put_8s(f, &vdev->isr);
> > +    qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap);
> >       qemu_put_be16s(f, &vdev->queue_sel);
> >       qemu_put_be32s(f, &vdev->guest_features);
> >       qemu_put_be32(f, vdev->config_len);
> > @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> >   
> >       qemu_get_8s(f, &vdev->status);
> >       qemu_get_8s(f, &vdev->isr);
> > +    qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap);
> >       qemu_get_be16s(f, &vdev->queue_sel);
> >       qemu_get_be32s(f, &features);
> >   
> > diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> > new file mode 100644
> > index 0000000..70dd1e2
> > --- /dev/null
> > +++ b/include/hw/virtio/virtio-access.h
> > @@ -0,0 +1,139 @@
> > +/*
> > + * Virtio Accessor Support: In case your target can change endian.
> > + *
> > + * Copyright IBM, Corp. 2013
> > + *
> > + * Authors:
> > + *  Rusty Russell   <rusty@au.ibm.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation, either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + */
> > +#ifndef _QEMU_VIRTIO_ACCESS_H
> > +#define _QEMU_VIRTIO_ACCESS_H
> > +#include "hw/virtio/virtio.h"
> > +
> > +static inline uint16_t virtio_lduw_phys(AddressSpace *as, hwaddr pa,
> > +                                        struct VirtIODevice *vdev)
> > +{
> > +    if (vdev->needs_byteswap) {
> > +        return bswap16(lduw_phys(as, pa));
> > +    }
> > +    return lduw_phys(as, pa);
> > +}
> > +
> > +static inline uint32_t virtio_ldl_phys(AddressSpace *as, hwaddr pa,
> > +                                       struct VirtIODevice *vdev)
> > +{
> > +    if (vdev->needs_byteswap) {
> > +        return bswap32(ldl_phys(as, pa));
> > +    }
> > +    return ldl_phys(as, pa);
> > +}
> > +
> > +static inline uint64_t virtio_ldq_phys(AddressSpace *as, hwaddr pa,
> > +                                       struct VirtIODevice *vdev)
> > +{
> > +    if (vdev->needs_byteswap) {
> > +        return bswap64(ldq_phys(as, pa));
> > +    }
> > +    return ldq_phys(as, pa);
> > +}
> > +
> > +static inline void virtio_stw_phys(AddressSpace *as, hwaddr pa, uint16_t value,
> > +                                   struct VirtIODevice *vdev)
> > +{
> > +    if (vdev->needs_byteswap) {
> > +        stw_phys(as, pa, bswap16(value));
> > +    } else {
> > +        stw_phys(as, pa, value);
> > +    }
> > +}
> > +
> > +static inline void virtio_stl_phys(AddressSpace *as, hwaddr pa, uint32_t value,
> > +                                   struct VirtIODevice *vdev)
> > +{
> > +    if (vdev->needs_byteswap) {
> > +        stl_phys(as, pa, bswap32(value));
> > +    } else {
> > +        stl_phys(as, pa, value);
> > +    }
> > +}
> > +
> > +static inline void virtio_stw_p(void *ptr, uint16_t v,
> > +                                struct VirtIODevice *vdev)
> > +{
> > +    if (vdev->needs_byteswap) {
> > +        stw_p(ptr, bswap16(v));
> > +    } else {
> > +        stw_p(ptr, v);
> > +    }
> > +}
> > +
> > +static inline void virtio_stl_p(void *ptr, uint32_t v,
> > +                                struct VirtIODevice *vdev)
> > +{
> > +    if (vdev->needs_byteswap) {
> > +        stl_p(ptr, bswap32(v));
> > +    } else {
> > +        stl_p(ptr, v);
> > +    }
> > +}
> > +
> > +static inline void virtio_stq_p(void *ptr, uint64_t v,
> > +                                struct VirtIODevice *vdev)
> > +{
> > +    if (vdev->needs_byteswap) {
> > +        stq_p(ptr, bswap64(v));
> > +    } else {
> > +        stq_p(ptr, v);
> > +    }
> > +}
> > +
> > +static inline int virtio_lduw_p(const void *ptr, struct VirtIODevice *vdev)
> > +{
> > +    if (vdev->needs_byteswap) {
> > +        return bswap16(lduw_p(ptr));
> > +    } else {
> > +        return lduw_p(ptr);
> > +    }
> > +}
> > +
> > +static inline int virtio_ldl_p(const void *ptr, struct VirtIODevice *vdev)
> > +{
> > +    if (vdev->needs_byteswap) {
> > +        return bswap32(ldl_p(ptr));
> > +    } else {
> > +        return ldl_p(ptr);
> > +    }
> > +}
> > +
> > +static inline uint64_t virtio_ldq_p(const void *ptr, struct VirtIODevice *vdev)
> > +{
> > +    if (vdev->needs_byteswap) {
> > +        return bswap64(ldq_p(ptr));
> > +    } else {
> > +        return ldq_p(ptr);
> > +    }
> > +}
> > +
> > +static inline uint32_t virtio_tswap32(uint32_t s, struct VirtIODevice *vdev)
> > +{
> > +    if (vdev->needs_byteswap) {
> > +        return bswap32(tswap32(s));
> > +    } else {
> > +        return tswap32(s);
> > +    }
> > +}
> > +
> > +static inline void virtio_tswap32s(uint32_t *s, struct VirtIODevice *vdev)
> > +{
> > +    tswap32s(s);
> > +    if (vdev->needs_byteswap) {
> > +        *s = bswap32(*s);
> > +    }
> 
> Couldn't you just reuse virtio_tswap32() here?
> 

Yes. Good catch !

> Also, I think a "needs_byteswap" flag gets confusing. Devices shouldn't 
> have any access to CPU endian specific RAM accessors.
> 
> How about we introduce a flag like "is_bigendian" that we set depending 
> on the CPU default endianness for legacy virtio. We can then change that 
> flag accordingly. For the wrappers here, we could just use accessors 
> like ldl_be_phys() or ldl_le_phys().
> 
> That should make things a lot easier to understand - and for virtio 1.0 
> we only need to set is_bigendian = false :).
> 

Yeah definitely !

> 
> Alex
> 
> 

Thanks.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

[Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access

[Greg Kurz]

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

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>
[ ldq_phys() API change,
  use per-device needs_byteswap flag,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio.c |   93 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 53 insertions(+), 40 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 24b565f..1877b46 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -102,53 +102,57 @@ static void virtqueue_init(VirtQueue *vq)
                                  vq->vring.align);
 }
 
-static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
+static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i,
+                                       struct VirtIODevice *vdev)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, addr);
-    return ldq_phys(&address_space_memory, pa);
+    return virtio_ldq_phys(&address_space_memory, pa, vdev);
 }
 
-static inline uint32_t vring_desc_len(hwaddr desc_pa, int i)
+static inline uint32_t vring_desc_len(hwaddr desc_pa, int i,
+                                      struct VirtIODevice *vdev)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, len);
-    return ldl_phys(&address_space_memory, pa);
+    return virtio_ldl_phys(&address_space_memory, pa, vdev);
 }
 
-static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i)
+static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i,
+                                        struct VirtIODevice *vdev)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, flags);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(&address_space_memory, pa, vdev);
 }
 
-static inline uint16_t vring_desc_next(hwaddr desc_pa, int i)
+static inline uint16_t vring_desc_next(hwaddr desc_pa, int i,
+                                       struct VirtIODevice *vdev)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, next);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(&address_space_memory, pa, vdev);
 }
 
 static inline uint16_t vring_avail_flags(VirtQueue *vq)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, flags);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
 }
 
 static inline uint16_t vring_avail_idx(VirtQueue *vq)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, idx);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
 }
 
 static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, ring[i]);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
 }
 
 static inline uint16_t vring_used_event(VirtQueue *vq)
@@ -160,44 +164,46 @@ 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(&address_space_memory, pa, val);
+    virtio_stl_phys(&address_space_memory, pa, val, vq->vdev);
 }
 
 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(&address_space_memory, pa, val);
+    virtio_stl_phys(&address_space_memory, pa, val, vq->vdev);
 }
 
 static uint16_t vring_used_idx(VirtQueue *vq)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, idx);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
 }
 
 static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, idx);
-    stw_phys(&address_space_memory, pa, val);
+    virtio_stw_phys(&address_space_memory, pa, val, vq->vdev);
 }
 
 static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, flags);
-    stw_phys(&address_space_memory,
-             pa, lduw_phys(&address_space_memory, pa) | mask);
+    virtio_stw_phys(&address_space_memory, pa,
+                    virtio_lduw_phys(&address_space_memory, pa,
+                                     vq->vdev) | mask, vq->vdev);
 }
 
 static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, flags);
-    stw_phys(&address_space_memory,
-             pa, lduw_phys(&address_space_memory, pa) & ~mask);
+    virtio_stw_phys(&address_space_memory, pa,
+                    virtio_lduw_phys(&address_space_memory, pa,
+                                     vq->vdev) & ~mask, vq->vdev);
 }
 
 static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
@@ -207,7 +213,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(&address_space_memory, pa, val);
+    virtio_stw_phys(&address_space_memory, pa, val, vq->vdev);
 }
 
 void virtio_queue_set_notification(VirtQueue *vq, int enable)
@@ -325,16 +331,18 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
 }
 
 static unsigned virtqueue_next_desc(hwaddr desc_pa,
-                                    unsigned int i, unsigned int max)
+                                    unsigned int i, unsigned int max,
+                                    struct VirtIODevice *vdev)
 {
     unsigned int next;
 
     /* If this descriptor says it doesn't chain, we're done. */
-    if (!(vring_desc_flags(desc_pa, i) & VRING_DESC_F_NEXT))
+    if (!(vring_desc_flags(desc_pa, i, vdev) & VRING_DESC_F_NEXT)) {
         return max;
+    }
 
     /* Check they're not leading us off end of descriptors. */
-    next = vring_desc_next(desc_pa, i);
+    next = vring_desc_next(desc_pa, i, vdev);
     /* Make sure compiler knows to grab that: we don't want it changing! */
     smp_wmb();
 
@@ -366,8 +374,9 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
         i = virtqueue_get_head(vq, idx++);
         desc_pa = vq->vring.desc;
 
-        if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
-            if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
+        if (vring_desc_flags(desc_pa, i, vq->vdev)
+            & VRING_DESC_F_INDIRECT) {
+            if (vring_desc_len(desc_pa, i, vq->vdev) % sizeof(VRingDesc)) {
                 error_report("Invalid size for indirect buffer table");
                 exit(1);
             }
@@ -380,8 +389,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 
             /* loop over the indirect descriptor table */
             indirect = 1;
-            max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
-            desc_pa = vring_desc_addr(desc_pa, i);
+            max = vring_desc_len(desc_pa, i, vq->vdev) / sizeof(VRingDesc);
+            desc_pa = vring_desc_addr(desc_pa, i, vq->vdev);
             num_bufs = i = 0;
         }
 
@@ -392,15 +401,17 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
                 exit(1);
             }
 
-            if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
-                in_total += vring_desc_len(desc_pa, i);
+            if (vring_desc_flags(desc_pa, i, vq->vdev)
+                & VRING_DESC_F_WRITE) {
+                in_total += vring_desc_len(desc_pa, i, vq->vdev);
             } else {
-                out_total += vring_desc_len(desc_pa, i);
+                out_total += vring_desc_len(desc_pa, i, vq->vdev);
             }
             if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
                 goto done;
             }
-        } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
+        } while ((i = virtqueue_next_desc(desc_pa, i, max, vq->vdev))
+                 != max);
 
         if (!indirect)
             total_bufs = num_bufs;
@@ -459,15 +470,15 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
         vring_avail_event(vq, vring_avail_idx(vq));
     }
 
-    if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
-        if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
+    if (vring_desc_flags(desc_pa, i, vq->vdev) & VRING_DESC_F_INDIRECT) {
+        if (vring_desc_len(desc_pa, i, vq->vdev) % sizeof(VRingDesc)) {
             error_report("Invalid size for indirect buffer table");
             exit(1);
         }
 
         /* loop over the indirect descriptor table */
-        max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
-        desc_pa = vring_desc_addr(desc_pa, i);
+        max = vring_desc_len(desc_pa, i, vq->vdev) / sizeof(VRingDesc);
+        desc_pa = vring_desc_addr(desc_pa, i, vq->vdev);
         i = 0;
     }
 
@@ -475,30 +486,32 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
     do {
         struct iovec *sg;
 
-        if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
+        if (vring_desc_flags(desc_pa, i, vq->vdev) & VRING_DESC_F_WRITE) {
             if (elem->in_num >= ARRAY_SIZE(elem->in_sg)) {
                 error_report("Too many write descriptors in indirect table");
                 exit(1);
             }
-            elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i);
+            elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i,
+                                                          vq->vdev);
             sg = &elem->in_sg[elem->in_num++];
         } else {
             if (elem->out_num >= ARRAY_SIZE(elem->out_sg)) {
                 error_report("Too many read descriptors in indirect table");
                 exit(1);
             }
-            elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i);
+            elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i,
+                                                            vq->vdev);
             sg = &elem->out_sg[elem->out_num++];
         }
 
-        sg->iov_len = vring_desc_len(desc_pa, i);
+        sg->iov_len = vring_desc_len(desc_pa, i, vq->vdev);
 
         /* If we've got too many, that implies a descriptor loop. */
         if ((elem->in_num + elem->out_num) > max) {
             error_report("Looped descriptor");
             exit(1);
         }
-    } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
+    } while ((i = virtqueue_next_desc(desc_pa, i, max, vq->vdev)) != max);
 
     /* Now map what we have collected */
     virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);

Re: [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access

[Thomas Huth]

On Fri, 28 Mar 2014 11:57:25 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> From: Rusty Russell <rusty@rustcorp.com.au>
> 
> 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>
> [ ldq_phys() API change,
>   use per-device needs_byteswap flag,
>   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/virtio/virtio.c |   93 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 53 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 24b565f..1877b46 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -102,53 +102,57 @@ static void virtqueue_init(VirtQueue *vq)
>                                   vq->vring.align);
>  }
> 
> -static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
> +static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i,
> +                                       struct VirtIODevice *vdev)
>  {
>      hwaddr pa;
>      pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, addr);
> -    return ldq_phys(&address_space_memory, pa);
> +    return virtio_ldq_phys(&address_space_memory, pa, vdev);
>  }
> 
> -static inline uint32_t vring_desc_len(hwaddr desc_pa, int i)
> +static inline uint32_t vring_desc_len(hwaddr desc_pa, int i,
> +                                      struct VirtIODevice *vdev)
>  {
>      hwaddr pa;
>      pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, len);
> -    return ldl_phys(&address_space_memory, pa);
> +    return virtio_ldl_phys(&address_space_memory, pa, vdev);
>  }
> 
> -static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i)
> +static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i,
> +                                        struct VirtIODevice *vdev)
>  {
>      hwaddr pa;
>      pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, flags);
> -    return lduw_phys(&address_space_memory, pa);
> +    return virtio_lduw_phys(&address_space_memory, pa, vdev);
>  }
> 
> -static inline uint16_t vring_desc_next(hwaddr desc_pa, int i)
> +static inline uint16_t vring_desc_next(hwaddr desc_pa, int i,
> +                                       struct VirtIODevice *vdev)
>  {
>      hwaddr pa;
>      pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, next);
> -    return lduw_phys(&address_space_memory, pa);
> +    return virtio_lduw_phys(&address_space_memory, pa, vdev);
>  }
> 
>  static inline uint16_t vring_avail_flags(VirtQueue *vq)
>  {
>      hwaddr pa;
>      pa = vq->vring.avail + offsetof(VRingAvail, flags);
> -    return lduw_phys(&address_space_memory, pa);
> +    return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
>  }
> 
>  static inline uint16_t vring_avail_idx(VirtQueue *vq)
>  {
>      hwaddr pa;
>      pa = vq->vring.avail + offsetof(VRingAvail, idx);
> -    return lduw_phys(&address_space_memory, pa);
> +    return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
>  }
> 
>  static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
>  {
>      hwaddr pa;
>      pa = vq->vring.avail + offsetof(VRingAvail, ring[i]);
> -    return lduw_phys(&address_space_memory, pa);
> +    return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
>  }
> 
>  static inline uint16_t vring_used_event(VirtQueue *vq)
> @@ -160,44 +164,46 @@ 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(&address_space_memory, pa, val);
> +    virtio_stl_phys(&address_space_memory, pa, val, vq->vdev);
>  }
> 
>  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(&address_space_memory, pa, val);
> +    virtio_stl_phys(&address_space_memory, pa, val, vq->vdev);
>  }
> 
>  static uint16_t vring_used_idx(VirtQueue *vq)
>  {
>      hwaddr pa;
>      pa = vq->vring.used + offsetof(VRingUsed, idx);
> -    return lduw_phys(&address_space_memory, pa);
> +    return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
>  }
> 
>  static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
>  {
>      hwaddr pa;
>      pa = vq->vring.used + offsetof(VRingUsed, idx);
> -    stw_phys(&address_space_memory, pa, val);
> +    virtio_stw_phys(&address_space_memory, pa, val, vq->vdev);
>  }
> 
>  static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
>  {
>      hwaddr pa;
>      pa = vq->vring.used + offsetof(VRingUsed, flags);
> -    stw_phys(&address_space_memory,
> -             pa, lduw_phys(&address_space_memory, pa) | mask);
> +    virtio_stw_phys(&address_space_memory, pa,
> +                    virtio_lduw_phys(&address_space_memory, pa,
> +                                     vq->vdev) | mask, vq->vdev);
>  }
> 
>  static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
>  {
>      hwaddr pa;
>      pa = vq->vring.used + offsetof(VRingUsed, flags);
> -    stw_phys(&address_space_memory,
> -             pa, lduw_phys(&address_space_memory, pa) & ~mask);
> +    virtio_stw_phys(&address_space_memory, pa,
> +                    virtio_lduw_phys(&address_space_memory, pa,
> +                                     vq->vdev) & ~mask, vq->vdev);
>  }
> 
>  static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
> @@ -207,7 +213,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(&address_space_memory, pa, val);
> +    virtio_stw_phys(&address_space_memory, pa, val, vq->vdev);
>  }
> 
>  void virtio_queue_set_notification(VirtQueue *vq, int enable)
> @@ -325,16 +331,18 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
>  }
> 
>  static unsigned virtqueue_next_desc(hwaddr desc_pa,
> -                                    unsigned int i, unsigned int max)
> +                                    unsigned int i, unsigned int max,
> +                                    struct VirtIODevice *vdev)
>  {
>      unsigned int next;
> 
>      /* If this descriptor says it doesn't chain, we're done. */
> -    if (!(vring_desc_flags(desc_pa, i) & VRING_DESC_F_NEXT))
> +    if (!(vring_desc_flags(desc_pa, i, vdev) & VRING_DESC_F_NEXT)) {
>          return max;
> +    }
> 
>      /* Check they're not leading us off end of descriptors. */
> -    next = vring_desc_next(desc_pa, i);
> +    next = vring_desc_next(desc_pa, i, vdev);
>      /* Make sure compiler knows to grab that: we don't want it changing! */
>      smp_wmb();
> 
> @@ -366,8 +374,9 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>          i = virtqueue_get_head(vq, idx++);
>          desc_pa = vq->vring.desc;
> 
> -        if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
> -            if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
> +        if (vring_desc_flags(desc_pa, i, vq->vdev)
> +            & VRING_DESC_F_INDIRECT) {

I think the above if-statement would still fit into one line with the
80-columns limit.

> +            if (vring_desc_len(desc_pa, i, vq->vdev) % sizeof(VRingDesc)) {
>                  error_report("Invalid size for indirect buffer table");
>                  exit(1);
>              }
> @@ -380,8 +389,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> 
>              /* loop over the indirect descriptor table */
>              indirect = 1;
> -            max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
> -            desc_pa = vring_desc_addr(desc_pa, i);
> +            max = vring_desc_len(desc_pa, i, vq->vdev) / sizeof(VRingDesc);
> +            desc_pa = vring_desc_addr(desc_pa, i, vq->vdev);
>              num_bufs = i = 0;
>          }
> 
> @@ -392,15 +401,17 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>                  exit(1);
>              }
> 
> -            if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
> -                in_total += vring_desc_len(desc_pa, i);
> +            if (vring_desc_flags(desc_pa, i, vq->vdev)
> +                & VRING_DESC_F_WRITE) {

Again, no need for two lines?

> +                in_total += vring_desc_len(desc_pa, i, vq->vdev);
>              } else {
> -                out_total += vring_desc_len(desc_pa, i);
> +                out_total += vring_desc_len(desc_pa, i, vq->vdev);
>              }
>              if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
>                  goto done;
>              }
> -        } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
> +        } while ((i = virtqueue_next_desc(desc_pa, i, max, vq->vdev))
> +                 != max);

Doesn't that while statement also still fit into 80 columns?

>          if (!indirect)
>              total_bufs = num_bufs;
> @@ -459,15 +470,15 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>          vring_avail_event(vq, vring_avail_idx(vq));
>      }
> 
> -    if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
> -        if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
> +    if (vring_desc_flags(desc_pa, i, vq->vdev) & VRING_DESC_F_INDIRECT) {
> +        if (vring_desc_len(desc_pa, i, vq->vdev) % sizeof(VRingDesc)) {
>              error_report("Invalid size for indirect buffer table");
>              exit(1);
>          }
> 
>          /* loop over the indirect descriptor table */
> -        max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
> -        desc_pa = vring_desc_addr(desc_pa, i);
> +        max = vring_desc_len(desc_pa, i, vq->vdev) / sizeof(VRingDesc);
> +        desc_pa = vring_desc_addr(desc_pa, i, vq->vdev);
>          i = 0;
>      }
> 
> @@ -475,30 +486,32 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>      do {
>          struct iovec *sg;
> 
> -        if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
> +        if (vring_desc_flags(desc_pa, i, vq->vdev) & VRING_DESC_F_WRITE) {
>              if (elem->in_num >= ARRAY_SIZE(elem->in_sg)) {
>                  error_report("Too many write descriptors in indirect table");
>                  exit(1);
>              }
> -            elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i);
> +            elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i,
> +                                                          vq->vdev);

That one seems to be pretty close, but could also still fit into 80
columns?

>              sg = &elem->in_sg[elem->in_num++];
>          } else {
>              if (elem->out_num >= ARRAY_SIZE(elem->out_sg)) {
>                  error_report("Too many read descriptors in indirect table");
>                  exit(1);
>              }
> -            elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i);
> +            elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i,
> +                                                            vq->vdev);
>              sg = &elem->out_sg[elem->out_num++];
>          }
> 
> -        sg->iov_len = vring_desc_len(desc_pa, i);
> +        sg->iov_len = vring_desc_len(desc_pa, i, vq->vdev);
> 
>          /* If we've got too many, that implies a descriptor loop. */
>          if ((elem->in_num + elem->out_num) > max) {
>              error_report("Looped descriptor");
>              exit(1);
>          }
> -    } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
> +    } while ((i = virtqueue_next_desc(desc_pa, i, max, vq->vdev)) != max);
> 
>      /* Now map what we have collected */
>      virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);

Apart from the cosmetic nits, patch looks fine to me.

Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access

[Greg Kurz]

On Fri, 28 Mar 2014 17:07:31 +0100
Thomas Huth <thuth@linux.vnet.ibm.com> wrote:

> On Fri, 28 Mar 2014 11:57:25 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > From: Rusty Russell <rusty@rustcorp.com.au>
> > 
> > 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>
> > [ ldq_phys() API change,
> >   use per-device needs_byteswap flag,
> >   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  hw/virtio/virtio.c |   93 ++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 53 insertions(+), 40 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 24b565f..1877b46 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -102,53 +102,57 @@ static void virtqueue_init(VirtQueue *vq)
> >                                   vq->vring.align);
> >  }
> > 
> > -static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
> > +static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i,
> > +                                       struct VirtIODevice *vdev)
> >  {
> >      hwaddr pa;
> >      pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, addr);
> > -    return ldq_phys(&address_space_memory, pa);
> > +    return virtio_ldq_phys(&address_space_memory, pa, vdev);
> >  }
> > 
> > -static inline uint32_t vring_desc_len(hwaddr desc_pa, int i)
> > +static inline uint32_t vring_desc_len(hwaddr desc_pa, int i,
> > +                                      struct VirtIODevice *vdev)
> >  {
> >      hwaddr pa;
> >      pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, len);
> > -    return ldl_phys(&address_space_memory, pa);
> > +    return virtio_ldl_phys(&address_space_memory, pa, vdev);
> >  }
> > 
> > -static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i)
> > +static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i,
> > +                                        struct VirtIODevice *vdev)
> >  {
> >      hwaddr pa;
> >      pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, flags);
> > -    return lduw_phys(&address_space_memory, pa);
> > +    return virtio_lduw_phys(&address_space_memory, pa, vdev);
> >  }
> > 
> > -static inline uint16_t vring_desc_next(hwaddr desc_pa, int i)
> > +static inline uint16_t vring_desc_next(hwaddr desc_pa, int i,
> > +                                       struct VirtIODevice *vdev)
> >  {
> >      hwaddr pa;
> >      pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, next);
> > -    return lduw_phys(&address_space_memory, pa);
> > +    return virtio_lduw_phys(&address_space_memory, pa, vdev);
> >  }
> > 
> >  static inline uint16_t vring_avail_flags(VirtQueue *vq)
> >  {
> >      hwaddr pa;
> >      pa = vq->vring.avail + offsetof(VRingAvail, flags);
> > -    return lduw_phys(&address_space_memory, pa);
> > +    return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
> >  }
> > 
> >  static inline uint16_t vring_avail_idx(VirtQueue *vq)
> >  {
> >      hwaddr pa;
> >      pa = vq->vring.avail + offsetof(VRingAvail, idx);
> > -    return lduw_phys(&address_space_memory, pa);
> > +    return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
> >  }
> > 
> >  static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
> >  {
> >      hwaddr pa;
> >      pa = vq->vring.avail + offsetof(VRingAvail, ring[i]);
> > -    return lduw_phys(&address_space_memory, pa);
> > +    return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
> >  }
> > 
> >  static inline uint16_t vring_used_event(VirtQueue *vq)
> > @@ -160,44 +164,46 @@ 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(&address_space_memory, pa, val);
> > +    virtio_stl_phys(&address_space_memory, pa, val, vq->vdev);
> >  }
> > 
> >  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(&address_space_memory, pa, val);
> > +    virtio_stl_phys(&address_space_memory, pa, val, vq->vdev);
> >  }
> > 
> >  static uint16_t vring_used_idx(VirtQueue *vq)
> >  {
> >      hwaddr pa;
> >      pa = vq->vring.used + offsetof(VRingUsed, idx);
> > -    return lduw_phys(&address_space_memory, pa);
> > +    return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
> >  }
> > 
> >  static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
> >  {
> >      hwaddr pa;
> >      pa = vq->vring.used + offsetof(VRingUsed, idx);
> > -    stw_phys(&address_space_memory, pa, val);
> > +    virtio_stw_phys(&address_space_memory, pa, val, vq->vdev);
> >  }
> > 
> >  static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
> >  {
> >      hwaddr pa;
> >      pa = vq->vring.used + offsetof(VRingUsed, flags);
> > -    stw_phys(&address_space_memory,
> > -             pa, lduw_phys(&address_space_memory, pa) | mask);
> > +    virtio_stw_phys(&address_space_memory, pa,
> > +                    virtio_lduw_phys(&address_space_memory, pa,
> > +                                     vq->vdev) | mask, vq->vdev);
> >  }
> > 
> >  static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
> >  {
> >      hwaddr pa;
> >      pa = vq->vring.used + offsetof(VRingUsed, flags);
> > -    stw_phys(&address_space_memory,
> > -             pa, lduw_phys(&address_space_memory, pa) & ~mask);
> > +    virtio_stw_phys(&address_space_memory, pa,
> > +                    virtio_lduw_phys(&address_space_memory, pa,
> > +                                     vq->vdev) & ~mask, vq->vdev);
> >  }
> > 
> >  static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
> > @@ -207,7 +213,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(&address_space_memory, pa, val);
> > +    virtio_stw_phys(&address_space_memory, pa, val, vq->vdev);
> >  }
> > 
> >  void virtio_queue_set_notification(VirtQueue *vq, int enable)
> > @@ -325,16 +331,18 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
> >  }
> > 
> >  static unsigned virtqueue_next_desc(hwaddr desc_pa,
> > -                                    unsigned int i, unsigned int max)
> > +                                    unsigned int i, unsigned int max,
> > +                                    struct VirtIODevice *vdev)
> >  {
> >      unsigned int next;
> > 
> >      /* If this descriptor says it doesn't chain, we're done. */
> > -    if (!(vring_desc_flags(desc_pa, i) & VRING_DESC_F_NEXT))
> > +    if (!(vring_desc_flags(desc_pa, i, vdev) & VRING_DESC_F_NEXT)) {
> >          return max;
> > +    }
> > 
> >      /* Check they're not leading us off end of descriptors. */
> > -    next = vring_desc_next(desc_pa, i);
> > +    next = vring_desc_next(desc_pa, i, vdev);
> >      /* Make sure compiler knows to grab that: we don't want it changing! */
> >      smp_wmb();
> > 
> > @@ -366,8 +374,9 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> >          i = virtqueue_get_head(vq, idx++);
> >          desc_pa = vq->vring.desc;
> > 
> > -        if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
> > -            if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
> > +        if (vring_desc_flags(desc_pa, i, vq->vdev)
> > +            & VRING_DESC_F_INDIRECT) {
> 
> I think the above if-statement would still fit into one line with the
> 80-columns limit.
> 
> > +            if (vring_desc_len(desc_pa, i, vq->vdev) % sizeof(VRingDesc)) {
> >                  error_report("Invalid size for indirect buffer table");
> >                  exit(1);
> >              }
> > @@ -380,8 +389,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> > 
> >              /* loop over the indirect descriptor table */
> >              indirect = 1;
> > -            max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
> > -            desc_pa = vring_desc_addr(desc_pa, i);
> > +            max = vring_desc_len(desc_pa, i, vq->vdev) / sizeof(VRingDesc);
> > +            desc_pa = vring_desc_addr(desc_pa, i, vq->vdev);
> >              num_bufs = i = 0;
> >          }
> > 
> > @@ -392,15 +401,17 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> >                  exit(1);
> >              }
> > 
> > -            if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
> > -                in_total += vring_desc_len(desc_pa, i);
> > +            if (vring_desc_flags(desc_pa, i, vq->vdev)
> > +                & VRING_DESC_F_WRITE) {
> 
> Again, no need for two lines?
> 
> > +                in_total += vring_desc_len(desc_pa, i, vq->vdev);
> >              } else {
> > -                out_total += vring_desc_len(desc_pa, i);
> > +                out_total += vring_desc_len(desc_pa, i, vq->vdev);
> >              }
> >              if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
> >                  goto done;
> >              }
> > -        } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
> > +        } while ((i = virtqueue_next_desc(desc_pa, i, max, vq->vdev))
> > +                 != max);
> 
> Doesn't that while statement also still fit into 80 columns?
> 
> >          if (!indirect)
> >              total_bufs = num_bufs;
> > @@ -459,15 +470,15 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
> >          vring_avail_event(vq, vring_avail_idx(vq));
> >      }
> > 
> > -    if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
> > -        if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
> > +    if (vring_desc_flags(desc_pa, i, vq->vdev) & VRING_DESC_F_INDIRECT) {
> > +        if (vring_desc_len(desc_pa, i, vq->vdev) % sizeof(VRingDesc)) {
> >              error_report("Invalid size for indirect buffer table");
> >              exit(1);
> >          }
> > 
> >          /* loop over the indirect descriptor table */
> > -        max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
> > -        desc_pa = vring_desc_addr(desc_pa, i);
> > +        max = vring_desc_len(desc_pa, i, vq->vdev) / sizeof(VRingDesc);
> > +        desc_pa = vring_desc_addr(desc_pa, i, vq->vdev);
> >          i = 0;
> >      }
> > 
> > @@ -475,30 +486,32 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
> >      do {
> >          struct iovec *sg;
> > 
> > -        if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
> > +        if (vring_desc_flags(desc_pa, i, vq->vdev) & VRING_DESC_F_WRITE) {
> >              if (elem->in_num >= ARRAY_SIZE(elem->in_sg)) {
> >                  error_report("Too many write descriptors in indirect table");
> >                  exit(1);
> >              }
> > -            elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i);
> > +            elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i,
> > +                                                          vq->vdev);
> 
> That one seems to be pretty close, but could also still fit into 80
> columns?
> 
> >              sg = &elem->in_sg[elem->in_num++];
> >          } else {
> >              if (elem->out_num >= ARRAY_SIZE(elem->out_sg)) {
> >                  error_report("Too many read descriptors in indirect table");
> >                  exit(1);
> >              }
> > -            elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i);
> > +            elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i,
> > +                                                            vq->vdev);
> >              sg = &elem->out_sg[elem->out_num++];
> >          }
> > 
> > -        sg->iov_len = vring_desc_len(desc_pa, i);
> > +        sg->iov_len = vring_desc_len(desc_pa, i, vq->vdev);
> > 
> >          /* If we've got too many, that implies a descriptor loop. */
> >          if ((elem->in_num + elem->out_num) > max) {
> >              error_report("Looped descriptor");
> >              exit(1);
> >          }
> > -    } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
> > +    } while ((i = virtqueue_next_desc(desc_pa, i, max, vq->vdev)) != max);
> > 
> >      /* Now map what we have collected */
> >      virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
> 
> Apart from the cosmetic nits, patch looks fine to me.
> 

Heh... remains of a first shot where I was passing vdev->needs_byteswap
and overpassed the limit. :)

> Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> 
> 
> 

Thanks !

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access

[Alexander Graf]

On 03/28/2014 11:57 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> 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>
> [ ldq_phys() API change,
>    use per-device needs_byteswap flag,
>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>   hw/virtio/virtio.c |   93 ++++++++++++++++++++++++++++++----------------------
>   1 file changed, 53 insertions(+), 40 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 24b565f..1877b46 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -102,53 +102,57 @@ static void virtqueue_init(VirtQueue *vq)
>                                    vq->vring.align);
>   }
>   
> -static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
> +static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i,
> +                                       struct VirtIODevice *vdev)

The logical ordering for helper is usually to have the device as the 
first parameter (it's basically the self object). Could you please order 
it accordingly?


Alex

Re: [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access

[Andreas Färber]

Am 31.03.2014 18:24, schrieb Alexander Graf:
> On 03/28/2014 11:57 AM, Greg Kurz wrote:
>> From: Rusty Russell <rusty@rustcorp.com.au>
>>
>> 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>
>> [ ldq_phys() API change,
>>    use per-device needs_byteswap flag,
>>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>> ---
>>   hw/virtio/virtio.c |   93
>> ++++++++++++++++++++++++++++++----------------------
>>   1 file changed, 53 insertions(+), 40 deletions(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 24b565f..1877b46 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -102,53 +102,57 @@ static void virtqueue_init(VirtQueue *vq)
>>                                    vq->vring.align);
>>   }
>>   -static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
>> +static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i,
>> +                                       struct VirtIODevice *vdev)
> 
> The logical ordering for helper is usually to have the device as the
> first parameter (it's basically the self object). Could you please order
> it accordingly?

Also please note that QEMU Coding Style requests the use of typedefs,
i.e. drop the "struct".

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 v6 2/8] virtio: allow byte swapping for vring and config access

[Greg Kurz]

On Mon, 31 Mar 2014 18:26:54 +0200
Andreas Färber <afaerber@suse.de> wrote:
> Am 31.03.2014 18:24, schrieb Alexander Graf:
> > On 03/28/2014 11:57 AM, Greg Kurz wrote:
> >> From: Rusty Russell <rusty@rustcorp.com.au>
> >>
> >> 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>
> >> [ ldq_phys() API change,
> >>    use per-device needs_byteswap flag,
> >>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >> ---
> >>   hw/virtio/virtio.c |   93
> >> ++++++++++++++++++++++++++++++----------------------
> >>   1 file changed, 53 insertions(+), 40 deletions(-)
> >>
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 24b565f..1877b46 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -102,53 +102,57 @@ static void virtqueue_init(VirtQueue *vq)
> >>                                    vq->vring.align);
> >>   }
> >>   -static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
> >> +static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i,
> >> +                                       struct VirtIODevice *vdev)
> > 
> > The logical ordering for helper is usually to have the device as the
> > first parameter (it's basically the self object). Could you please order
> > it accordingly?
> 
> Also please note that QEMU Coding Style requests the use of typedefs,
> i.e. drop the "struct".
> 
> Regards,
> Andreas
> 
> 

Oops my bad... I did not know about the HACKING file until your mail. :)
I will fixe that.

Thanks.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

[Qemu-devel] [PATCH v6 3/8] virtio-net: use virtio wrappers to access headers

[Greg Kurz]

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

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
[ use per-device needs_byteswap flag,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/net/virtio-net.c |   17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 439477b..eb89f48 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -23,6 +23,7 @@
 #include "hw/virtio/virtio-bus.h"
 #include "qapi/qmp/qjson.h"
 #include "monitor/monitor.h"
+#include "hw/virtio/virtio-access.h"
 
 #define VIRTIO_NET_VM_VERSION    11
 
@@ -72,8 +73,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, vdev);
+    virtio_stw_p(&netcfg.max_virtqueue_pairs, n->max_queues, vdev);
     memcpy(netcfg.mac, n->mac, ETH_ALEN);
     memcpy(config, &netcfg, n->config_size);
 }
@@ -614,6 +615,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
     struct virtio_net_ctrl_mac mac_data;
     size_t s;
     NetClientState *nc = qemu_get_queue(n->nic);
+    VirtIODevice *vdev = VIRTIO_DEVICE(n);
 
     if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
         if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
@@ -639,7 +641,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, vdev);
     if (s != sizeof(mac_data.entries)) {
         goto error;
     }
@@ -666,7 +668,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, vdev);
     if (s != sizeof(mac_data.entries)) {
         goto error;
     }
@@ -709,9 +711,10 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
     uint16_t vid;
     size_t s;
     NetClientState *nc = qemu_get_queue(n->nic);
+    VirtIODevice *vdev = VIRTIO_DEVICE(n);
 
     s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
-    vid = lduw_p(&vid);
+    vid = virtio_lduw_p(&vid, vdev);
     if (s != sizeof(vid)) {
         return VIRTIO_NET_ERR;
     }
@@ -748,7 +751,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, vdev);
 
     if (queues < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
         queues > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
@@ -1047,7 +1050,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, vdev);
         iov_from_buf(mhdr_sg, mhdr_cnt,
                      0,
                      &mhdr.num_buffers, sizeof mhdr.num_buffers);

Re: [Qemu-devel] [PATCH v6 3/8] virtio-net: use virtio wrappers to access headers

[Alexander Graf]

On 03/28/2014 11:57 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> [ use per-device needs_byteswap flag,
>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>   hw/net/virtio-net.c |   17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 439477b..eb89f48 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -23,6 +23,7 @@
>   #include "hw/virtio/virtio-bus.h"
>   #include "qapi/qmp/qjson.h"
>   #include "monitor/monitor.h"
> +#include "hw/virtio/virtio-access.h"
>   
>   #define VIRTIO_NET_VM_VERSION    11
>   
> @@ -72,8 +73,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, vdev);
> +    virtio_stw_p(&netcfg.max_virtqueue_pairs, n->max_queues, vdev);

If you make virtio_stw_p() only depend on endian specific operations 
this file could be built as part of obj rather than common-obj again, 
right? So it doesn't depend on the target to operate. So as part of this 
change you should also have a Makefile.obj change.

The same obviously applies to all the other files as well :)


Alex

[Qemu-devel] [PATCH v6 4/8] virtio-balloon: use virtio wrappers to access page frame numbers

[Greg Kurz]

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

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
[ use per-device needs_byteswap flag,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio-balloon.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a470a0b..e0b31ba 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,8 @@ 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,
+                                          vdev) << VIRTIO_BALLOON_PFN_SHIFT;
             offset += 4;
 
             /* FIXME: remove get_system_memory(), but how? */

Re: [Qemu-devel] [PATCH v6 4/8] virtio-balloon: use virtio wrappers to access page frame numbers

[Alexander Graf]

On 03/28/2014 11:57 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> [ use per-device needs_byteswap flag,
>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>   hw/virtio/virtio-balloon.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a470a0b..e0b31ba 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,8 @@ 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,
> +                                          vdev) << VIRTIO_BALLOON_PFN_SHIFT;

This is quite unreadable. Just create a new small local variable for the 
load result :).


Alex

[Qemu-devel] [PATCH v6 5/8] virtio-blk: use virtio wrappers to access headers

[Greg Kurz]

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

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>
[ use per-device needs_byteswap flag,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/block/virtio-blk.c |   40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 8a568e5..c652ff2 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -26,6 +26,7 @@
 # include <scsi/sg.h>
 #endif
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
 
 typedef struct VirtIOBlockReq
 {
@@ -77,7 +78,9 @@ 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_DEVICE(req->dev)) & VIRTIO_BLK_T_OUT);
         if (virtio_blk_handle_rw_error(req, -ret, is_read))
             return;
     }
@@ -224,12 +227,13 @@ 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_DEVICE(req->dev));
+    virtio_stl_p(&req->scsi->residual, hdr.resid, VIRTIO_DEVICE(req->dev));
+    virtio_stl_p(&req->scsi->sense_len, hdr.sb_len_wr, VIRTIO_DEVICE(req->dev));
+    virtio_stl_p(&req->scsi->data_len, hdr.dxfer_len, VIRTIO_DEVICE(req->dev));
 
     virtio_blk_req_complete(req, status);
     g_free(req);
@@ -240,7 +244,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_DEVICE(req->dev));
     virtio_blk_req_complete(req, status);
     g_free(req);
 }
@@ -286,7 +290,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, VIRTIO_DEVICE(req->dev));
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
 
@@ -320,7 +324,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, VIRTIO_DEVICE(req->dev));
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
 
@@ -358,7 +362,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, VIRTIO_DEVICE(req->dev));
 
     if (type & VIRTIO_BLK_T_FLUSH) {
         virtio_blk_handle_flush(req, mrb);
@@ -487,12 +491,14 @@ 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, vdev);
+    virtio_stl_p(&blkcfg.seg_max, 128 - 2, vdev);
+    virtio_stw_p(&blkcfg.cylinders, s->conf->cyls, vdev);
+    virtio_stl_p(&blkcfg.blk_size, blk_size, vdev);
+    virtio_stw_p(&blkcfg.min_io_size, s->conf->min_io_size / blk_size,
+                 vdev);
+    virtio_stw_p(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size,
+                 vdev);
     blkcfg.heads = s->conf->heads;
     /*
      * We must ensure that the block device capacity is a multiple of

Re: [Qemu-devel] [PATCH v6 5/8] virtio-blk: use virtio wrappers to access headers

[Alexander Graf]

On 03/28/2014 11:57 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> 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>
> [ use per-device needs_byteswap flag,
>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>   hw/block/virtio-blk.c |   40 +++++++++++++++++++++++-----------------
>   1 file changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 8a568e5..c652ff2 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -26,6 +26,7 @@
>   # include <scsi/sg.h>
>   #endif
>   #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
>   
>   typedef struct VirtIOBlockReq
>   {
> @@ -77,7 +78,9 @@ 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_DEVICE(req->dev)) & VIRTIO_BLK_T_OUT);

Any way you could fit this into the line limit by extracting part of it 
into a separate variable?


Alex

[Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers

[Greg Kurz]

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

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>
[ use per-device needs_byteswap flag,
  fix missing tswap32 in virtio_scsi_push_event(),
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/scsi/virtio-scsi.c |   38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index b0d7517..20d326e 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;
@@ -315,12 +316,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, VIRTIO_DEVICE(s));
     } else {
         req->resp.cmd->resid = 0;
         sense_len = scsi_req_get_sense(r, req->resp.cmd->sense,
                                        vs->sense_size);
-        req->resp.cmd->sense_len = tswap32(sense_len);
+        req->resp.cmd->sense_len = virtio_tswap32(sense_len, VIRTIO_DEVICE(s));
     }
     virtio_scsi_complete_req(req);
 }
@@ -416,16 +417,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, vdev);
+    virtio_stl_p(&scsiconf->seg_max, 128 - 2, vdev);
+    virtio_stl_p(&scsiconf->max_sectors, s->conf.max_sectors, vdev);
+    virtio_stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun, vdev);
+    virtio_stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent), vdev);
+    virtio_stl_p(&scsiconf->sense_size, s->sense_size, vdev);
+    virtio_stl_p(&scsiconf->cdb_size, s->cdb_size, vdev);
+    virtio_stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL, vdev);
+    virtio_stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET, vdev);
+    virtio_stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN, vdev);
 }
 
 static void virtio_scsi_set_config(VirtIODevice *vdev,
@@ -434,14 +435,15 @@ 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,
+                                vdev) >= 65536 ||
+        (uint32_t) virtio_ldl_p(&scsiconf->cdb_size, vdev) >= 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, vdev);
+    vs->cdb_size = virtio_ldl_p(&scsiconf->cdb_size, vdev);
 }
 
 static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
@@ -519,8 +521,8 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
 
     evt = req->resp.event;
     memset(evt, 0, sizeof(VirtIOSCSIEvent));
-    evt->event = event;
-    evt->reason = reason;
+    evt->event = virtio_tswap32(event);
+    evt->reason = virtio_tswap32(reason);
     if (!dev) {
         assert(event == VIRTIO_SCSI_T_EVENTS_MISSED);
     } else {

Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers

[Greg Kurz]

On Fri, 28 Mar 2014 11:57:56 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> From: Rusty Russell <rusty@rustcorp.com.au>
> 
> 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>
> [ use per-device needs_byteswap flag,
>   fix missing tswap32 in virtio_scsi_push_event(),
>   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/scsi/virtio-scsi.c |   38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index b0d7517..20d326e 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;
> @@ -315,12 +316,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, VIRTIO_DEVICE(s));
>      } else {
>          req->resp.cmd->resid = 0;
>          sense_len = scsi_req_get_sense(r, req->resp.cmd->sense,
>                                         vs->sense_size);
> -        req->resp.cmd->sense_len = tswap32(sense_len);
> +        req->resp.cmd->sense_len = virtio_tswap32(sense_len, VIRTIO_DEVICE(s));
>      }
>      virtio_scsi_complete_req(req);
>  }
> @@ -416,16 +417,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, vdev);
> +    virtio_stl_p(&scsiconf->seg_max, 128 - 2, vdev);
> +    virtio_stl_p(&scsiconf->max_sectors, s->conf.max_sectors, vdev);
> +    virtio_stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun, vdev);
> +    virtio_stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent), vdev);
> +    virtio_stl_p(&scsiconf->sense_size, s->sense_size, vdev);
> +    virtio_stl_p(&scsiconf->cdb_size, s->cdb_size, vdev);
> +    virtio_stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL, vdev);
> +    virtio_stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET, vdev);
> +    virtio_stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN, vdev);
>  }
> 
>  static void virtio_scsi_set_config(VirtIODevice *vdev,
> @@ -434,14 +435,15 @@ 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,
> +                                vdev) >= 65536 ||
> +        (uint32_t) virtio_ldl_p(&scsiconf->cdb_size, vdev) >= 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, vdev);
> +    vs->cdb_size = virtio_ldl_p(&scsiconf->cdb_size, vdev);
>  }
> 
>  static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
> @@ -519,8 +521,8 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
> 
>      evt = req->resp.event;
>      memset(evt, 0, sizeof(VirtIOSCSIEvent));
> -    evt->event = event;
> -    evt->reason = reason;
> +    evt->event = virtio_tswap32(event);
> +    evt->reason = virtio_tswap32(reason);

My bad again, it should be:

 +    evt->event = virtio_tswap32(event, vdev);
 +    evt->reason = virtio_tswap32(reason, vdev);

>      if (!dev) {
>          assert(event == VIRTIO_SCSI_T_EVENTS_MISSED);
>      } else {
> 
> 

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers

[Andreas Färber]

Am 28.03.2014 18:13, schrieb Greg Kurz:
> On Fri, 28 Mar 2014 11:57:56 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>> @@ -519,8 +521,8 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
>>
>>      evt = req->resp.event;
>>      memset(evt, 0, sizeof(VirtIOSCSIEvent));
>> -    evt->event = event;
>> -    evt->reason = reason;
>> +    evt->event = virtio_tswap32(event);
>> +    evt->reason = virtio_tswap32(reason);
> 
> My bad again, it should be:
> 
>  +    evt->event = virtio_tswap32(event, vdev);
>  +    evt->reason = virtio_tswap32(reason, vdev);

Pure bikeshedding, but wouldn't it make sense to have the vdev as first
argument?

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 v6 6/8] virtio-scsi: use virtio wrappers to access headers

[Greg Kurz]

On Fri, 28 Mar 2014 18:21:43 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 28.03.2014 18:13, schrieb Greg Kurz:
> > On Fri, 28 Mar 2014 11:57:56 +0100
> > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> >> @@ -519,8 +521,8 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
> >>
> >>      evt = req->resp.event;
> >>      memset(evt, 0, sizeof(VirtIOSCSIEvent));
> >> -    evt->event = event;
> >> -    evt->reason = reason;
> >> +    evt->event = virtio_tswap32(event);
> >> +    evt->reason = virtio_tswap32(reason);
> > 
> > My bad again, it should be:
> > 
> >  +    evt->event = virtio_tswap32(event, vdev);
> >  +    evt->reason = virtio_tswap32(reason, vdev);
> 
> Pure bikeshedding, but wouldn't it make sense to have the vdev as first
> argument?
> 

I have thought about it also... it would make sense to do the same with
all the other helpers in virtio-access.h then.

And while we are at it, since we pass &address_space_memory to all occurences
of virtio_*_phys() and I don't see why we would change that, maybe we
can also move that into the helpers. Thoughts ?

> Regards,
> Andreas
> 

Thanks.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers

[Peter Maydell]

On 28 March 2014 17:37, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> And while we are at it, since we pass &address_space_memory to all
> occurences of virtio_*_phys() and I don't see why we would change
> that, maybe we can also move that into the helpers. Thoughts ?

In the longer term I'm hoping that references to
&address_space_memory go away -- we should be modelling
separate address spaces per CPU and per every other
thing that can act as a DMA master (ie issue memory
transactions). I'm not sure exactly how virtio ought to
work since these accesses directly to memory are a total
hack, but probably we will end up setting the virtio
device up and handing it an AddressSpace* that it should use.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers

[Greg Kurz]

On Fri, 28 Mar 2014 17:43:07 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 28 March 2014 17:37, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> > And while we are at it, since we pass &address_space_memory to all
> > occurences of virtio_*_phys() and I don't see why we would change
> > that, maybe we can also move that into the helpers. Thoughts ?
> 
> In the longer term I'm hoping that references to
> &address_space_memory go away -- we should be modelling
> separate address spaces per CPU and per every other
> thing that can act as a DMA master (ie issue memory
> transactions). I'm not sure exactly how virtio ought to
> work since these accesses directly to memory are a total
> hack, but probably we will end up setting the virtio
> device up and handing it an AddressSpace* that it should use.
> 

Ok, I am now convinced. Let's make struct VirtIODevice* be the
first argument for all helpers and kill the AddressSpace* one.
Unless you envision we could end up with different address spaces
accross multiple virtio devices, I would then do as proposed
above... Even if we add an AddressSpace* to devices, the API
will remain the same.

> thanks
> -- PMM
> 

Cheers.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers

[Peter Maydell]

On 28 March 2014 18:04, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> Ok, I am now convinced. Let's make struct VirtIODevice* be the
> first argument for all helpers and kill the AddressSpace* one.
> Unless you envision we could end up with different address spaces
> accross multiple virtio devices

Well, any one virtio device would always use the same AddressSpace,
but consider a system model with two separate models of different
PCs in it, each of which has  its own self-contained memory, PCI
bus and virtio devices...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers

[Greg Kurz]

On Fri, 28 Mar 2014 18:14:55 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 28 March 2014 18:04, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> > Ok, I am now convinced. Let's make struct VirtIODevice* be the
> > first argument for all helpers and kill the AddressSpace* one.
> > Unless you envision we could end up with different address spaces
> > accross multiple virtio devices
> 
> Well, any one virtio device would always use the same AddressSpace,
> but consider a system model with two separate models of different
> PCs in it, each of which has  its own self-contained memory, PCI
> bus and virtio devices...
> 

A per-device AddressSpace* property would be more appropriate then. Correct ?

> thanks
> -- PMM
> 

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers

[Alexander Graf]

On 03/28/2014 11:57 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> 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>
> [ use per-device needs_byteswap flag,
>    fix missing tswap32 in virtio_scsi_push_event(),
>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>   hw/scsi/virtio-scsi.c |   38 ++++++++++++++++++++------------------
>   1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index b0d7517..20d326e 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"

Just a small nit, but I spot some inconsitency in < and " usage here :)


Alex

[Qemu-devel] [PATCH v6 7/8] virtio-serial-bus: use virtio wrappers to access headers

[Greg Kurz]

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

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
[ use per-device needs_byteswap flag,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/char/virtio-serial-bus.c |   39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..7a84b2a 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_DEVICE(vser));
+    virtio_stw_p(&cpkt.event, event, VIRTIO_DEVICE(vser));
+    virtio_stw_p(&cpkt.value, value, VIRTIO_DEVICE(vser));
 
     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, VIRTIO_DEVICE(vser));
+    cpkt.value = virtio_lduw_p(&gcpkt->value, VIRTIO_DEVICE(vser));
 
     trace_virtio_serial_handle_control_message(cpkt.event, cpkt.value);
 
@@ -312,10 +313,11 @@ 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, VIRTIO_DEVICE(vser)));
     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, VIRTIO_DEVICE(vser)),
+                     vser->bus.qbus.name);
         return;
     }
 
@@ -342,9 +344,10 @@ 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_DEVICE(vser));
+            virtio_stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME,
+                         VIRTIO_DEVICE(vser));
+            virtio_stw_p(&cpkt.value, 1, VIRTIO_DEVICE(vser));
 
             buffer_len = sizeof(cpkt) + strlen(port->name) + 1;
             buffer = g_malloc(buffer_len);
@@ -536,7 +539,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, VIRTIO_DEVICE(s));
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         qemu_put_be32s(f, &s->ports_map[i]);
     }
@@ -690,8 +693,9 @@ 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, VIRTIO_DEVICE(s));
+    if (max_nr_ports > virtio_tswap32(s->config.max_nr_ports,
+                                      VIRTIO_DEVICE(s))) {
         /* Source could have had more ports than us. Fail migration. */
         return -EINVAL;
     }
@@ -760,7 +764,8 @@ 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,
+                                  VIRTIO_DEVICE(vser));
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         uint32_t map, bit;
 
@@ -848,7 +853,8 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    max_nr_ports = tswap32(port->vser->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(port->vser->config.max_nr_ports,
+                                  VIRTIO_DEVICE(port->vser));
     if (port->id >= max_nr_ports) {
         error_setg(errp, "virtio-serial-bus: Out-of-range port id specified, "
                          "max. allowed: %u", max_nr_ports - 1);
@@ -949,7 +955,8 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
         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, VIRTIO_DEVICE(vser));
     vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32)
         * sizeof(vser->ports_map[0]));
     /*

Re: [Qemu-devel] [PATCH v6 7/8] virtio-serial-bus: use virtio wrappers to access headers

[Alexander Graf]

On 03/28/2014 11:58 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> [ use per-device needs_byteswap flag,
>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>   hw/char/virtio-serial-bus.c |   39 +++++++++++++++++++++++----------------
>   1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 2b647b6..7a84b2a 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_DEVICE(vser));

These are dynamic casts. You at least want to do these only once, 
probably even use some faster form of casting.


Alex

> +    virtio_stw_p(&cpkt.event, event, VIRTIO_DEVICE(vser));
> +    virtio_stw_p(&cpkt.value, value, VIRTIO_DEVICE(vser));
>   
>       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, VIRTIO_DEVICE(vser));
> +    cpkt.value = virtio_lduw_p(&gcpkt->value, VIRTIO_DEVICE(vser));
>   
>       trace_virtio_serial_handle_control_message(cpkt.event, cpkt.value);
>   
> @@ -312,10 +313,11 @@ 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, VIRTIO_DEVICE(vser)));
>       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, VIRTIO_DEVICE(vser)),
> +                     vser->bus.qbus.name);
>           return;
>       }
>   
> @@ -342,9 +344,10 @@ 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_DEVICE(vser));
> +            virtio_stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME,
> +                         VIRTIO_DEVICE(vser));
> +            virtio_stw_p(&cpkt.value, 1, VIRTIO_DEVICE(vser));
>   
>               buffer_len = sizeof(cpkt) + strlen(port->name) + 1;
>               buffer = g_malloc(buffer_len);
> @@ -536,7 +539,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, VIRTIO_DEVICE(s));
>       for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
>           qemu_put_be32s(f, &s->ports_map[i]);
>       }
> @@ -690,8 +693,9 @@ 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, VIRTIO_DEVICE(s));
> +    if (max_nr_ports > virtio_tswap32(s->config.max_nr_ports,
> +                                      VIRTIO_DEVICE(s))) {
>           /* Source could have had more ports than us. Fail migration. */
>           return -EINVAL;
>       }
> @@ -760,7 +764,8 @@ 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,
> +                                  VIRTIO_DEVICE(vser));
>       for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
>           uint32_t map, bit;
>   
> @@ -848,7 +853,8 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
>           }
>       }
>   
> -    max_nr_ports = tswap32(port->vser->config.max_nr_ports);
> +    max_nr_ports = virtio_tswap32(port->vser->config.max_nr_ports,
> +                                  VIRTIO_DEVICE(port->vser));
>       if (port->id >= max_nr_ports) {
>           error_setg(errp, "virtio-serial-bus: Out-of-range port id specified, "
>                            "max. allowed: %u", max_nr_ports - 1);
> @@ -949,7 +955,8 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
>           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, VIRTIO_DEVICE(vser));
>       vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32)
>           * sizeof(vser->ports_map[0]));
>       /*
>

[Qemu-devel] [PATCH v6 8/8] virtio-9p: use virtio wrappers to access headers

[Greg Kurz]

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

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-device.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 15a4983..c758500 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -19,6 +19,7 @@
 #include "fsdev/qemu-fsdev.h"
 #include "virtio-9p-xattr.h"
 #include "virtio-9p-coth.h"
+#include "hw/virtio/virtio-access.h"
 
 static uint32_t virtio_9p_get_features(VirtIODevice *vdev, uint32_t features)
 {
@@ -34,7 +35,7 @@ static void virtio_9p_get_config(VirtIODevice *vdev, uint8_t *config)
 
     len = strlen(s->tag);
     cfg = g_malloc0(sizeof(struct virtio_9p_config) + len);
-    stw_raw(&cfg->tag_len, len);
+    virtio_stw_p(&cfg->tag_len, len, vdev);
     /* We don't copy the terminating null to config space */
     memcpy(cfg->tag, s->tag, len);
     memcpy(config, cfg, s->config_size);

[Qemu-devel] [PATCH v4] target-ppc: ppc64 target's virtio can be either endian

[Greg Kurz]

We base it on the OS endian, as reflected by the endianness of the
interrupt vectors (handled through the ILE bit in the LPCR register).

Using first_cpu to fetch the registers from KVM may look arbitrary
and awkward, but it is okay because KVM sets/unsets the ILE bit on
all CPUs.

Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
[ rename virtio_get_byteswap to virtio_legacy_get_byteswap,
  support both LE and BE host,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Reviewed-by: Alexander Graf <agraf@suse.de>
---

Alex,

Welcome back ! :)

Changes since v3:
- rename the helper according to v6 of the main LE virtio patchset
- prepare to support LE ppc64 host
- tentatively fixed the list of changes embeded in SoB lines

Cheers.

--
Greg

 target-ppc/misc_helper.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
index 2eb2fa6..4dbb274 100644
--- a/target-ppc/misc_helper.c
+++ b/target-ppc/misc_helper.c
@@ -20,6 +20,8 @@
 #include "helper.h"
 
 #include "helper_regs.h"
+#include "hw/virtio/virtio.h"
+#include "sysemu/kvm.h"
 
 /*****************************************************************************/
 /* SPR accesses */
@@ -120,3 +122,15 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
 {
     hreg_store_msr(env, value, 0);
 }
+
+bool virtio_legacy_get_byteswap(void)
+{
+    PowerPCCPU *cp = POWERPC_CPU(first_cpu);
+    CPUPPCState *env = &cp->env;
+    bool ile = env->spr[SPR_LPCR] & LPCR_ILE;
+#ifdef HOST_WORDS_BIGENDIAN
+    return ile;
+#else
+    return !ile;
+#endif
+}