[Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Greg Kurz]

Hi,

This version merges the changes requested during the v7 review, remarks from
ppc64 dump support review (yes, we talked about virtio there) and the work on
virtio subsections migration. Also two new patches have been added:
- patch #1 is a preliminary fix for virtio-serial posted by Alexander Graf
- patch #9 prepares the work on the virtio_is_big_endian() helper

The most significant changes are:
- introduction of a new CPU method for virtio
- endianness is taken from CPU that resets the device
- fastpath virtio memory accessors for fixed endian targets
- VMState based virtio subsections (compatibility friendly)

You'll find more detailed changelog in each patch.

Please comment and hopefully apply.

Thanks !

---

Greg Kurz (14):
      virtio-serial: don't migrate the config space
      virtio: introduce device specific migration calls
      virtio-net: implement per-device migration calls
      virtio-blk: implement per-device migration calls
      virtio-serial: implement per-device migration calls
      virtio-balloon: implement per-device migration calls
      virtio-rng: implement per-device migration calls
      virtio: add subsections to the migration stream
      exec: introduce target_words_bigendian() helper
      cpu: introduce CPUClass::virtio_is_big_endian()
      virtio: add endian-ambivalent support to VirtIODevice
      virtio: memory accessors for endian-ambivalent targets
      virtio-9p: use virtio wrappers to access headers
      target-ppc: enable virtio endian ambivalent support

Rusty Russell (6):
      virtio: allow byte swapping for vring
      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


 exec.c                            |   11 --
 hw/9pfs/virtio-9p-device.c        |    3 -
 hw/block/virtio-blk.c             |   62 ++++++-----
 hw/char/virtio-serial-bus.c       |   94 ++++++++++------
 hw/net/virtio-net.c               |   56 +++++++---
 hw/scsi/virtio-scsi.c             |   40 ++++---
 hw/virtio/virtio-balloon.c        |   33 +++---
 hw/virtio/virtio-pci.c            |   11 +-
 hw/virtio/virtio-rng.c            |   12 +-
 hw/virtio/virtio.c                |  217 ++++++++++++++++++++++++++++---------
 include/exec/cpu-common.h         |    1 
 include/hw/virtio/virtio-access.h |  170 +++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h        |   17 +++
 include/qom/cpu.h                 |   10 ++
 qom/cpu.c                         |    6 +
 target-ppc/cpu.h                  |    2 
 target-ppc/translate_init.c       |   13 ++
 17 files changed, 572 insertions(+), 186 deletions(-)
 create mode 100644 include/hw/virtio/virtio-access.h

--
Greg

[Qemu-devel] [PATCH v8 01/20] virtio-serial: don't migrate the config space

[Greg Kurz]

The device configuration is set at realize time and never changes. It
should not be migrated as it is done today. For the sake of compatibility,
let's just skip them at load time.

Signed-off-by: Alexander Graf <agraf@suse.de>
[ added missing casts to uint16_t *,
  added SoB and commit message,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/char/virtio-serial-bus.c |   16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..ee1ba16 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     uint32_t max_nr_ports, nr_active_ports, ports_map;
     unsigned int i;
     int ret;
+    uint32_t tmp;
 
     if (version_id > 3) {
         return -EINVAL;
@@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
         return 0;
     }
 
-    /* The config space */
-    qemu_get_be16s(f, &s->config.cols);
-    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)) {
-        /* Source could have had more ports than us. Fail migration. */
-        return -EINVAL;
-    }
+    /* Unused */
+    qemu_get_be16s(f, (uint16_t *) &tmp);
+    qemu_get_be16s(f, (uint16_t *) &tmp);
+    qemu_get_be32s(f, &tmp);
 
+    max_nr_ports = tswap32(s->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         qemu_get_be32s(f, &ports_map);
 

Re: [Qemu-devel] [PATCH v8 01/20] virtio-serial: don't migrate the config space

[Alexander Graf]

On 13.06.14 13:18, Greg Kurz wrote:
> The device configuration is set at realize time and never changes. It
> should not be migrated as it is done today. For the sake of compatibility,
> let's just skip them at load time.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> [ added missing casts to uint16_t *,
>    added SoB and commit message,
>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

Legally you would have to ask me for my SoB line and are not allowed to 
add it yourself, but here it is:

  Signed-off-by: Alexander Graf <agraf@suse.de>

So we're all good now ;).


Thanks!

Alex

Re: [Qemu-devel] [PATCH v8 01/20] virtio-serial: don't migrate the config space

[Amit Shah]

On (Fri) 13 Jun 2014 [13:18:42], Greg Kurz wrote:
> The device configuration is set at realize time and never changes. It
> should not be migrated as it is done today. For the sake of compatibility,
> let's just skip them at load time.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

The author field should also mention Alex instead of you, I suppose,
since it was authored by him.

Patch looks good.

		Amit

Re: [Qemu-devel] [PATCH v8 01/20] virtio-serial: don't migrate the config space

[Greg Kurz]

On Thu, 19 Jun 2014 16:10:15 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Fri) 13 Jun 2014 [13:18:42], Greg Kurz wrote:
> > The device configuration is set at realize time and never changes. It
> > should not be migrated as it is done today. For the sake of compatibility,
> > let's just skip them at load time.
> > 
> > Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> The author field should also mention Alex instead of you, I suppose,
> since it was authored by him.
> 

I'll add a From: line.

> Patch looks good.
> 
> 		Amit
> 

Thanks for the review.

-- 
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 v8 01/20] virtio-serial: don't migrate the config space

[Michael S. Tsirkin]

On Fri, Jun 13, 2014 at 01:18:42PM +0200, Greg Kurz wrote:
> The device configuration is set at realize time and never changes. It
> should not be migrated as it is done today. For the sake of compatibility,
> let's just skip them at load time.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> [ added missing casts to uint16_t *,
>   added SoB and commit message,
>   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

This one is definitely a good thing to have, regardless.
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/char/virtio-serial-bus.c |   16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 2b647b6..ee1ba16 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
>      uint32_t max_nr_ports, nr_active_ports, ports_map;
>      unsigned int i;
>      int ret;
> +    uint32_t tmp;
>  
>      if (version_id > 3) {
>          return -EINVAL;
> @@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
>          return 0;
>      }
>  
> -    /* The config space */
> -    qemu_get_be16s(f, &s->config.cols);
> -    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)) {
> -        /* Source could have had more ports than us. Fail migration. */
> -        return -EINVAL;
> -    }
> +    /* Unused */
> +    qemu_get_be16s(f, (uint16_t *) &tmp);
> +    qemu_get_be16s(f, (uint16_t *) &tmp);
> +    qemu_get_be32s(f, &tmp);
>  
> +    max_nr_ports = tswap32(s->config.max_nr_ports);
>      for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
>          qemu_get_be32s(f, &ports_map);
>  

[Qemu-devel] [PATCH v8 02/20] virtio: introduce device specific migration calls

[Greg Kurz]

In order to migrate virtio subsections, they should be streamed after
the device itself. We need the device specific code to be called from
the common migration code to achieve this. This patch introduces load
and save methods for this purpose.

Suggested-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/block/virtio-blk.c       |    2 +-
 hw/char/virtio-serial-bus.c |    2 +-
 hw/net/virtio-net.c         |    2 +-
 hw/scsi/virtio-scsi.c       |    2 +-
 hw/virtio/virtio-balloon.c  |    2 +-
 hw/virtio/virtio-rng.c      |    2 +-
 hw/virtio/virtio.c          |   13 ++++++++++++-
 include/hw/virtio/virtio.h  |    4 +++-
 8 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 85aa871..49f58cd 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -624,7 +624,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 2)
         return -EINVAL;
 
-    ret = virtio_load(vdev, f);
+    ret = virtio_load(vdev, f, version_id);
     if (ret) {
         return ret;
     }
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index ee1ba16..9f1df84 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -677,7 +677,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     }
 
     /* The virtio device */
-    ret = virtio_load(VIRTIO_DEVICE(s), f);
+    ret = virtio_load(VIRTIO_DEVICE(s), f, version_id);
     if (ret) {
         return ret;
     }
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 940a7cf..fc24bcc 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1333,7 +1333,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
         return -EINVAL;
 
-    ret = virtio_load(vdev, f);
+    ret = virtio_load(vdev, f, version_id);
     if (ret) {
         return ret;
     }
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index b39880a..d5c37a9 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -487,7 +487,7 @@ static int virtio_scsi_load(QEMUFile *f, void *opaque, int version_id)
     VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
     int ret;
 
-    ret = virtio_load(vdev, f);
+    ret = virtio_load(vdev, f, version_id);
     if (ret) {
         return ret;
     }
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 22cd52e..f35d883 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -341,7 +341,7 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 1)
         return -EINVAL;
 
-    ret = virtio_load(vdev, f);
+    ret = virtio_load(vdev, f, version_id);
     if (ret) {
         return ret;
     }
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index b6ab361..025de81 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -113,7 +113,7 @@ static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 1) {
         return -EINVAL;
     }
-    virtio_load(vdev, f);
+    virtio_load(vdev, f, version_id);
 
     /* We may have an element ready but couldn't process it due to a quota
      * limit.  Make sure to try again after live migration when the quota may
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a07ae8a..44517fc 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -843,6 +843,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
 {
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
     int i;
 
     if (k->save_config) {
@@ -877,6 +878,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
             k->save_queue(qbus->parent, i, f);
         }
     }
+
+    if (vdc->save != NULL) {
+        vdc->save(vdev, f);
+    }
 }
 
 int virtio_set_features(VirtIODevice *vdev, uint32_t val)
@@ -895,7 +900,7 @@ int virtio_set_features(VirtIODevice *vdev, uint32_t val)
     return bad ? -1 : 0;
 }
 
-int virtio_load(VirtIODevice *vdev, QEMUFile *f)
+int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
 {
     int i, ret;
     int32_t config_len;
@@ -904,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
     uint32_t supported_features;
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
     if (k->load_config) {
         ret = k->load_config(qbus->parent, f);
@@ -977,6 +983,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
     }
 
     virtio_notify_vector(vdev, VIRTIO_NO_VECTOR);
+
+    if (vdc->load != NULL) {
+        return vdc->load(vdev, f, version_id);
+    }
+
     return 0;
 }
 
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 3e54e90..3505ce5 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -150,6 +150,8 @@ typedef struct VirtioDeviceClass {
      * must mask in frontend instead.
      */
     void (*guest_notifier_mask)(VirtIODevice *vdev, int n, bool mask);
+    void (*save)(VirtIODevice *vdev, QEMUFile *f);
+    int (*load)(VirtIODevice *vdev, QEMUFile *f, int version_id);
 } VirtioDeviceClass;
 
 void virtio_init(VirtIODevice *vdev, const char *name,
@@ -184,7 +186,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
 
 void virtio_save(VirtIODevice *vdev, QEMUFile *f);
 
-int virtio_load(VirtIODevice *vdev, QEMUFile *f);
+int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id);
 
 void virtio_notify_config(VirtIODevice *vdev);
 

[Qemu-devel] [PATCH v8 03/20] virtio-net: implement per-device migration calls

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/net/virtio-net.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index fc24bcc..58e7b73 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1285,7 +1285,6 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue)
 
 static void virtio_net_save(QEMUFile *f, void *opaque)
 {
-    int i;
     VirtIONet *n = opaque;
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
 
@@ -1293,6 +1292,12 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
      * it might keep writing to memory. */
     assert(!n->vhost_started);
     virtio_save(vdev, f);
+}
+
+static void virtio_net_save_device(VirtIODevice *vdev, QEMUFile *f)
+{
+    VirtIONet *n = VIRTIO_NET(vdev);
+    int i;
 
     qemu_put_buffer(f, n->mac, ETH_ALEN);
     qemu_put_be32(f, n->vqs[0].tx_waiting);
@@ -1328,15 +1333,18 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIONet *n = opaque;
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
-    int ret, i, link_down;
 
     if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
         return -EINVAL;
 
-    ret = virtio_load(vdev, f, version_id);
-    if (ret) {
-        return ret;
-    }
+    return virtio_load(vdev, f, version_id);
+}
+
+static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
+                                  int version_id)
+{
+    VirtIONet *n = VIRTIO_NET(vdev);
+    int i, link_down;
 
     qemu_get_buffer(f, n->mac, ETH_ALEN);
     n->vqs[0].tx_waiting = qemu_get_be32(f);
@@ -1685,6 +1693,8 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
     vdc->set_status = virtio_net_set_status;
     vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
     vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
+    vdc->load = virtio_net_load_device;
+    vdc->save = virtio_net_save_device;
 }
 
 static const TypeInfo virtio_net_info = {

[Qemu-devel] [PATCH v8 04/20] virtio-blk: implement per-device migration calls

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/block/virtio-blk.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 49f58cd..e432bcd 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -601,12 +601,16 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
 
 static void virtio_blk_save(QEMUFile *f, void *opaque)
 {
-    VirtIOBlock *s = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-    VirtIOBlockReq *req = s->rq;
+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
 
     virtio_save(vdev, f);
+}
     
+static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
+{
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
+    VirtIOBlockReq *req = s->rq;
+
     while (req) {
         qemu_put_sbyte(f, 1);
         qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
@@ -619,15 +623,17 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIOBlock *s = opaque;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
-    int ret;
 
     if (version_id != 2)
         return -EINVAL;
 
-    ret = virtio_load(vdev, f, version_id);
-    if (ret) {
-        return ret;
-    }
+    return virtio_load(vdev, f, version_id);
+}
+
+static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
+                                  int version_id)
+{
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
 
     while (qemu_get_sbyte(f)) {
         VirtIOBlockReq *req = virtio_blk_alloc_request(s);
@@ -786,6 +792,8 @@ static void virtio_blk_class_init(ObjectClass *klass, void *data)
     vdc->get_features = virtio_blk_get_features;
     vdc->set_status = virtio_blk_set_status;
     vdc->reset = virtio_blk_reset;
+    vdc->save = virtio_blk_save_device;
+    vdc->load = virtio_blk_load_device;
 }
 
 static const TypeInfo virtio_device_info = {

[Qemu-devel] [PATCH v8 05/20] virtio-serial: implement per-device migration calls

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/char/virtio-serial-bus.c |   34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 9f1df84..3989722 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -521,14 +521,17 @@ static void vser_reset(VirtIODevice *vdev)
 
 static void virtio_serial_save(QEMUFile *f, void *opaque)
 {
-    VirtIOSerial *s = VIRTIO_SERIAL(opaque);
+    /* The virtio device */
+    virtio_save(VIRTIO_DEVICE(opaque), f);
+}
+
+static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f)
+{
+    VirtIOSerial *s = VIRTIO_SERIAL(vdev);
     VirtIOSerialPort *port;
     uint32_t nr_active_ports;
     unsigned int i, max_nr_ports;
 
-    /* The virtio device */
-    virtio_save(VIRTIO_DEVICE(s), f);
-
     /* The config space */
     qemu_put_be16s(f, &s->config.cols);
     qemu_put_be16s(f, &s->config.rows);
@@ -666,21 +669,22 @@ static int fetch_active_ports_list(QEMUFile *f, int version_id,
 
 static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 {
-    VirtIOSerial *s = VIRTIO_SERIAL(opaque);
-    uint32_t max_nr_ports, nr_active_ports, ports_map;
-    unsigned int i;
-    int ret;
-    uint32_t tmp;
-
     if (version_id > 3) {
         return -EINVAL;
     }
 
     /* The virtio device */
-    ret = virtio_load(VIRTIO_DEVICE(s), f, version_id);
-    if (ret) {
-        return ret;
-    }
+    return virtio_load(VIRTIO_DEVICE(opaque), f, version_id);
+}
+
+static int virtio_serial_load_device(VirtIODevice *vdev, QEMUFile *f,
+                                     int version_id)
+{
+    VirtIOSerial *s = VIRTIO_SERIAL(vdev);
+    uint32_t max_nr_ports, nr_active_ports, ports_map;
+    unsigned int i;
+    int ret;
+    uint32_t tmp;
 
     if (version_id < 2) {
         return 0;
@@ -1023,6 +1027,8 @@ static void virtio_serial_class_init(ObjectClass *klass, void *data)
     vdc->set_config = set_config;
     vdc->set_status = set_status;
     vdc->reset = vser_reset;
+    vdc->save = virtio_serial_save_device;
+    vdc->load = virtio_serial_load_device;
 }
 
 static const TypeInfo virtio_device_info = {

[Qemu-devel] [PATCH v8 06/20] virtio-balloon: implement per-device migration calls

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio-balloon.c |   25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index f35d883..84d1733 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -323,10 +323,12 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
 
 static void virtio_balloon_save(QEMUFile *f, void *opaque)
 {
-    VirtIOBalloon *s = VIRTIO_BALLOON(opaque);
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+    virtio_save(VIRTIO_DEVICE(opaque), f);
+}
 
-    virtio_save(vdev, f);
+static void virtio_balloon_save_device(VirtIODevice *vdev, QEMUFile *f)
+{
+    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
 
     qemu_put_be32(f, s->num_pages);
     qemu_put_be32(f, s->actual);
@@ -334,17 +336,16 @@ static void virtio_balloon_save(QEMUFile *f, void *opaque)
 
 static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
 {
-    VirtIOBalloon *s = VIRTIO_BALLOON(opaque);
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-    int ret;
-
     if (version_id != 1)
         return -EINVAL;
 
-    ret = virtio_load(vdev, f, version_id);
-    if (ret) {
-        return ret;
-    }
+    return virtio_load(VIRTIO_DEVICE(opaque), f, version_id);
+}
+
+static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
+                                      int version_id)
+{
+    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
 
     s->num_pages = qemu_get_be32(f);
     s->actual = qemu_get_be32(f);
@@ -414,6 +415,8 @@ static void virtio_balloon_class_init(ObjectClass *klass, void *data)
     vdc->get_config = virtio_balloon_get_config;
     vdc->set_config = virtio_balloon_set_config;
     vdc->get_features = virtio_balloon_get_features;
+    vdc->save = virtio_balloon_save_device;
+    vdc->load = virtio_balloon_load_device;
 }
 
 static const TypeInfo virtio_balloon_info = {

[Qemu-devel] [PATCH v8 07/20] virtio-rng: implement per-device migration calls

[Greg Kurz]

While we are here, we also check virtio_load() return value.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio-rng.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 025de81..1356aca 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -107,19 +107,20 @@ static void virtio_rng_save(QEMUFile *f, void *opaque)
 
 static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
 {
-    VirtIORNG *vrng = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(vrng);
-
     if (version_id != 1) {
         return -EINVAL;
     }
-    virtio_load(vdev, f, version_id);
+    return virtio_load(VIRTIO_DEVICE(opaque), f, version_id);
+}
 
+static int virtio_rng_load_device(VirtIODevice *vdev, QEMUFile *f,
+                                  int version_id)
+{
     /* We may have an element ready but couldn't process it due to a quota
      * limit.  Make sure to try again after live migration when the quota may
      * have been reset.
      */
-    virtio_rng_process(vrng);
+    virtio_rng_process(VIRTIO_RNG(vdev));
 
     return 0;
 }
@@ -219,6 +220,7 @@ static void virtio_rng_class_init(ObjectClass *klass, void *data)
     vdc->realize = virtio_rng_device_realize;
     vdc->unrealize = virtio_rng_device_unrealize;
     vdc->get_features = get_features;
+    vdc->load = virtio_rng_load_device;
 }
 
 static void virtio_rng_initfn(Object *obj)

[Qemu-devel] [PATCH v8 08/20] virtio: add subsections to the migration stream

[Greg Kurz]

There is a need to add some more fields to VirtIODevice that should be
migrated (broken status, endianness). The problem is that we do not
want to break compatibility while adding a new feature... This issue has
been addressed in the generic VMState code with the use of optional
subsections. As a *temporary* alternative to port the whole virtio
migration code to VMState, this patch mimics a similar subsectionning
ability for virtio, using the VMState code.

Since each virtio device is streamed in its own section, the idea is to
stream subsections between the end of the device section and the start
of the next sections. This allows an older QEMU to complain and exit
when fed with subsections:

Unknown savevm section type 5
load of migration failed

Suggested-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---

 Changes since last version (RFC):
 - use the VMState framework as suggested by Andreas Färber

 hw/virtio/virtio.c |   21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 44517fc..16b73d9 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 "migration/migration.h"
 
 /*
  * The alignment to use between consumer and producer parts of vring.
@@ -839,6 +840,16 @@ void virtio_notify_config(VirtIODevice *vdev)
     virtio_notify_vector(vdev, vdev->config_vector);
 }
 
+static const VMStateDescription vmstate_virtio = {
+    .name = "virtio",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 void virtio_save(VirtIODevice *vdev, QEMUFile *f)
 {
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
@@ -882,6 +893,9 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
     if (vdc->save != NULL) {
         vdc->save(vdev, f);
     }
+
+    /* Subsections */
+    vmstate_save_state(f, &vmstate_virtio, vdev);
 }
 
 int virtio_set_features(VirtIODevice *vdev, uint32_t val)
@@ -985,10 +999,13 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
     virtio_notify_vector(vdev, VIRTIO_NO_VECTOR);
 
     if (vdc->load != NULL) {
-        return vdc->load(vdev, f, version_id);
+        ret = vdc->load(vdev, f, version_id);
+        if (ret) {
+            return ret;
+        }
     }
 
-    return 0;
+    return vmstate_load_state(f, &vmstate_virtio, vdev, 1);
 }
 
 void virtio_cleanup(VirtIODevice *vdev)

[Qemu-devel] [PATCH v8 09/20] exec: introduce target_words_bigendian() helper

[Greg Kurz]

We currently have a virtio_is_big_endian() helper that provides the target
endianness to the virtio code. As of today, the helper returns a fixed
compile-time value. Of course, this will have to change if we want to
support target endianness changes at run-time.

Let's move the TARGET_WORDS_BIGENDIAN bits out to a new helper and have
virtio_is_big_endian() implemented on top of it.

This patch doesn't change any functionality.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 exec.c                     |   11 +----------
 hw/virtio/virtio-pci.c     |    3 ---
 include/exec/cpu-common.h  |    1 +
 include/hw/virtio/virtio.h |    5 +++++
 4 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/exec.c b/exec.c
index 4e179a6..a7d4431 100644
--- a/exec.c
+++ b/exec.c
@@ -2738,14 +2738,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
 }
 #endif
 
-#if !defined(CONFIG_USER_ONLY)
-
-/*
- * A helper function for the _utterly broken_ virtio device model to find out if
- * it's running on a big endian machine. Don't do this at home kids!
- */
-bool virtio_is_big_endian(void);
-bool virtio_is_big_endian(void)
+bool target_words_bigendian(void)
 {
 #if defined(TARGET_WORDS_BIGENDIAN)
     return true;
@@ -2754,8 +2747,6 @@ bool virtio_is_big_endian(void)
 #endif
 }
 
-#endif
-
 #ifndef CONFIG_USER_ONLY
 bool cpu_physical_memory_is_io(hwaddr phys_addr)
 {
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..390c8d2 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
 /* Flags track per-device state like workarounds for quirks in older guests. */
 #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG  (1 << 0)
 
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
 static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
                                VirtIOPCIProxy *dev);
 
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index a21b65a..eb798c1 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -122,4 +122,5 @@ void qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque);
 
 #endif
 
+bool target_words_bigendian(void);
 #endif /* !CPU_COMMON_H */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 3505ce5..daf0bb2 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -255,4 +255,9 @@ 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);
+
+static inline bool virtio_is_big_endian(void)
+{
+    return target_words_bigendian();
+}
 #endif

Re: [Qemu-devel] [PATCH v8 09/20] exec: introduce target_words_bigendian() helper

[Alexander Graf]

On 13.06.14 13:21, Greg Kurz wrote:
> We currently have a virtio_is_big_endian() helper that provides the target
> endianness to the virtio code. As of today, the helper returns a fixed
> compile-time value. Of course, this will have to change if we want to
> support target endianness changes at run-time.
>
> Let's move the TARGET_WORDS_BIGENDIAN bits out to a new helper and have
> virtio_is_big_endian() implemented on top of it.
>
> This patch doesn't change any functionality.
>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>   exec.c                     |   11 +----------
>   hw/virtio/virtio-pci.c     |    3 ---
>   include/exec/cpu-common.h  |    1 +
>   include/hw/virtio/virtio.h |    5 +++++
>   4 files changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 4e179a6..a7d4431 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2738,14 +2738,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>   }
>   #endif
>   
> -#if !defined(CONFIG_USER_ONLY)
> -
> -/*
> - * A helper function for the _utterly broken_ virtio device model to find out if
> - * it's running on a big endian machine. Don't do this at home kids!
> - */
> -bool virtio_is_big_endian(void);
> -bool virtio_is_big_endian(void)
> +bool target_words_bigendian(void)
>   {
>   #if defined(TARGET_WORDS_BIGENDIAN)
>       return true;
> @@ -2754,8 +2747,6 @@ bool virtio_is_big_endian(void)
>   #endif
>   }
>   
> -#endif
> -
>   #ifndef CONFIG_USER_ONLY
>   bool cpu_physical_memory_is_io(hwaddr phys_addr)
>   {
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ce97514..390c8d2 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -89,9 +89,6 @@
>   /* Flags track per-device state like workarounds for quirks in older guests. */
>   #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG  (1 << 0)
>   
> -/* HACK for virtio to determine if it's running a big endian guest */
> -bool virtio_is_big_endian(void);
> -
>   static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
>                                  VirtIOPCIProxy *dev);
>   
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index a21b65a..eb798c1 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -122,4 +122,5 @@ void qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque);
>   
>   #endif
>   
> +bool target_words_bigendian(void);

Please don't make this function globally available - usually no code in 
hw/ should know about this except for virtio. Put it in a virtio header 
instead.


Alex

Re: [Qemu-devel] [PATCH v8 09/20] exec: introduce target_words_bigendian() helper

[Greg Kurz]

On Fri, 13 Jun 2014 13:41:42 +0200
Alexander Graf <agraf@suse.de> wrote:
> 
> On 13.06.14 13:21, Greg Kurz wrote:
> > We currently have a virtio_is_big_endian() helper that provides the target
> > endianness to the virtio code. As of today, the helper returns a fixed
> > compile-time value. Of course, this will have to change if we want to
> > support target endianness changes at run-time.
> >
> > Let's move the TARGET_WORDS_BIGENDIAN bits out to a new helper and have
> > virtio_is_big_endian() implemented on top of it.
> >
> > This patch doesn't change any functionality.
> >
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >   exec.c                     |   11 +----------
> >   hw/virtio/virtio-pci.c     |    3 ---
> >   include/exec/cpu-common.h  |    1 +
> >   include/hw/virtio/virtio.h |    5 +++++
> >   4 files changed, 7 insertions(+), 13 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index 4e179a6..a7d4431 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -2738,14 +2738,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> >   }
> >   #endif
> >   
> > -#if !defined(CONFIG_USER_ONLY)
> > -
> > -/*
> > - * A helper function for the _utterly broken_ virtio device model to find out if
> > - * it's running on a big endian machine. Don't do this at home kids!
> > - */
> > -bool virtio_is_big_endian(void);
> > -bool virtio_is_big_endian(void)
> > +bool target_words_bigendian(void)
> >   {
> >   #if defined(TARGET_WORDS_BIGENDIAN)
> >       return true;
> > @@ -2754,8 +2747,6 @@ bool virtio_is_big_endian(void)
> >   #endif
> >   }
> >   
> > -#endif
> > -
> >   #ifndef CONFIG_USER_ONLY
> >   bool cpu_physical_memory_is_io(hwaddr phys_addr)
> >   {
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ce97514..390c8d2 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -89,9 +89,6 @@
> >   /* Flags track per-device state like workarounds for quirks in older guests. */
> >   #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG  (1 << 0)
> >   
> > -/* HACK for virtio to determine if it's running a big endian guest */
> > -bool virtio_is_big_endian(void);
> > -
> >   static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
> >                                  VirtIOPCIProxy *dev);
> >   
> > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> > index a21b65a..eb798c1 100644
> > --- a/include/exec/cpu-common.h
> > +++ b/include/exec/cpu-common.h
> > @@ -122,4 +122,5 @@ void qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque);
> >   
> >   #endif
> >   
> > +bool target_words_bigendian(void);
> 
> Please don't make this function globally available - usually no code in 
> hw/ should know about this except for virtio. Put it in a virtio header 
> instead.
> 
> 
> Alex
> 

Oops... sure I'll fix that.

-- 
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 v8 10/20] cpu: introduce CPUClass::virtio_is_big_endian()

[Greg Kurz]

If we want to support targets that can change endianness (modern PPC and
ARM for the moment), we need to add a per-CPU class method to be called
from the virtio code. The virtio_ prefix in the name is a hint for people
to avoid misusage (aka. anywhere but from the virtio code).

The default behaviour is to return the compile-time default target
endianness.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 include/qom/cpu.h |   10 ++++++++++
 qom/cpu.c         |    6 ++++++
 2 files changed, 16 insertions(+)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 4b352a2..30e8fe3 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -116,6 +116,7 @@ typedef struct CPUClass {
     CPUUnassignedAccess do_unassigned_access;
     void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
                                 int is_write, int is_user, uintptr_t retaddr);
+    bool (*virtio_is_big_endian)(CPUState *cpu);
     int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
                            uint8_t *buf, int len, bool is_write);
     void (*dump_state)(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
@@ -548,6 +549,15 @@ void cpu_interrupt(CPUState *cpu, int mask);
 
 #endif /* USER_ONLY */
 
+#ifndef CONFIG_USER_ONLY
+static inline bool cpu_virtio_is_big_endian(CPUState *cpu)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    return cc->virtio_is_big_endian(cpu);
+}
+#endif
+
 #ifdef CONFIG_SOFTMMU
 static inline void cpu_unassigned_access(CPUState *cpu, hwaddr addr,
                                          bool is_write, bool is_exec,
diff --git a/qom/cpu.c b/qom/cpu.c
index fada2d4..d9bf151 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -25,6 +25,7 @@
 #include "qemu/log.h"
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
+#include "exec/cpu-common.h"
 
 bool cpu_exists(int64_t id)
 {
@@ -196,6 +197,10 @@ static int cpu_common_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg)
     return 0;
 }
 
+static bool cpu_common_virtio_is_big_endian(CPUState *cpu)
+{
+    return target_words_bigendian();
+}
 
 void cpu_dump_state(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
                     int flags)
@@ -334,6 +339,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->write_elf64_note = cpu_common_write_elf64_note;
     k->gdb_read_register = cpu_common_gdb_read_register;
     k->gdb_write_register = cpu_common_gdb_write_register;
+    k->virtio_is_big_endian = cpu_common_virtio_is_big_endian;
     dc->realize = cpu_common_realizefn;
     /*
      * Reason: CPUs still need special care by board code: wiring up

Re: [Qemu-devel] [PATCH v8 10/20] cpu: introduce CPUClass::virtio_is_big_endian()

[Alexander Graf]

On 13.06.14 13:22, Greg Kurz wrote:
> If we want to support targets that can change endianness (modern PPC and
> ARM for the moment), we need to add a per-CPU class method to be called
> from the virtio code. The virtio_ prefix in the name is a hint for people
> to avoid misusage (aka. anywhere but from the virtio code).
>
> The default behaviour is to return the compile-time default target
> endianness.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>   include/qom/cpu.h |   10 ++++++++++
>   qom/cpu.c         |    6 ++++++
>   2 files changed, 16 insertions(+)
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 4b352a2..30e8fe3 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -116,6 +116,7 @@ typedef struct CPUClass {
>       CPUUnassignedAccess do_unassigned_access;
>       void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
>                                   int is_write, int is_user, uintptr_t retaddr);
> +    bool (*virtio_is_big_endian)(CPUState *cpu);
>       int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>                              uint8_t *buf, int len, bool is_write);
>       void (*dump_state)(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
> @@ -548,6 +549,15 @@ void cpu_interrupt(CPUState *cpu, int mask);
>   
>   #endif /* USER_ONLY */
>   
> +#ifndef CONFIG_USER_ONLY
> +static inline bool cpu_virtio_is_big_endian(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    return cc->virtio_is_big_endian(cpu);
> +}
> +#endif
> +

Why? Just do this from virtio code directly.


Alex

Re: [Qemu-devel] [PATCH v8 10/20] cpu: introduce CPUClass::virtio_is_big_endian()

[Greg Kurz]

On Fri, 13 Jun 2014 13:42:04 +0200
Alexander Graf <agraf@suse.de> wrote:
> 
> On 13.06.14 13:22, Greg Kurz wrote:
> > If we want to support targets that can change endianness (modern PPC and
> > ARM for the moment), we need to add a per-CPU class method to be called
> > from the virtio code. The virtio_ prefix in the name is a hint for people
> > to avoid misusage (aka. anywhere but from the virtio code).
> >
> > The default behaviour is to return the compile-time default target
> > endianness.
> >
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >   include/qom/cpu.h |   10 ++++++++++
> >   qom/cpu.c         |    6 ++++++
> >   2 files changed, 16 insertions(+)
> >
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index 4b352a2..30e8fe3 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -116,6 +116,7 @@ typedef struct CPUClass {
> >       CPUUnassignedAccess do_unassigned_access;
> >       void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
> >                                   int is_write, int is_user, uintptr_t retaddr);
> > +    bool (*virtio_is_big_endian)(CPUState *cpu);
> >       int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
> >                              uint8_t *buf, int len, bool is_write);
> >       void (*dump_state)(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
> > @@ -548,6 +549,15 @@ void cpu_interrupt(CPUState *cpu, int mask);
> >   
> >   #endif /* USER_ONLY */
> >   
> > +#ifndef CONFIG_USER_ONLY
> > +static inline bool cpu_virtio_is_big_endian(CPUState *cpu)
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +
> > +    return cc->virtio_is_big_endian(cpu);
> > +}
> > +#endif
> > +
> 
> Why? Just do this from virtio code directly.
> 
> 
> Alex
> 

Well I guess it is the same mistake as in the previous patch... I put
it there because it is a cpu method, but indeed it should only be
exposed to virtio code.

-- 
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 v8 11/20] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

Some CPU families can dynamically change their endianness. This means we
can have little endian ppc or big endian arm guests for example. This has
an impact on legacy virtio data structures since they are target endian.
We hence introduce a new property to track the endianness of each virtio
device. It is reasonnably assumed that endianness won't change while the
device is in use : we hence capture the device endianness when it gets
reset.

We migrate this property in a subsection, after the device descriptor. This
means the load code must not rely on it until it is restored. As a consequence,
the vring sanity checks had to be moved after the call to vmstate_load_state().
We enforce paranoia by poisoning the property at the begining of virtio_load().

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---

 Changes since last version (virtio migration RFC):
 - endianness cached at device reset time
 - reworked migration support

 hw/virtio/virtio-pci.c     |    8 ++--
 hw/virtio/virtio.c         |  100 +++++++++++++++++++++++++++++++++++++++-----
 include/hw/virtio/virtio.h |   12 ++++-
 3 files changed, 102 insertions(+), 18 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 390c8d2..2ffceb8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -406,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
         break;
     case 2:
         val = virtio_config_readw(vdev, addr);
-        if (virtio_is_big_endian()) {
+        if (virtio_is_big_endian(vdev)) {
             val = bswap16(val);
         }
         break;
     case 4:
         val = virtio_config_readl(vdev, addr);
-        if (virtio_is_big_endian()) {
+        if (virtio_is_big_endian(vdev)) {
             val = bswap32(val);
         }
         break;
@@ -440,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
         virtio_config_writeb(vdev, addr, val);
         break;
     case 2:
-        if (virtio_is_big_endian()) {
+        if (virtio_is_big_endian(vdev)) {
             val = bswap16(val);
         }
         virtio_config_writew(vdev, addr, val);
         break;
     case 4:
-        if (virtio_is_big_endian()) {
+        if (virtio_is_big_endian(vdev)) {
             val = bswap32(val);
         }
         virtio_config_writel(vdev, addr, val);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 16b73d9..6235df8 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -545,6 +545,24 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
     vdev->status = val;
 }
 
+static void virtio_set_endian_target_default(VirtIODevice *vdev)
+{
+#ifdef TARGET_WORDS_BIGENDIAN
+    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
+#else
+    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
+#endif
+}
+
+static void virtio_set_endian_cpu(VirtIODevice *vdev, CPUState *cpu)
+{
+    if (cpu_virtio_is_big_endian(cpu)) {
+        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
+    } else {
+        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
+    }
+}
+
 void virtio_reset(void *opaque)
 {
     VirtIODevice *vdev = opaque;
@@ -552,6 +570,13 @@ void virtio_reset(void *opaque)
     int i;
 
     virtio_set_status(vdev, 0);
+    if (current_cpu) {
+        /* Guest initiated reset */
+        virtio_set_endian_cpu(vdev, current_cpu);
+    } else {
+        /* System reset */
+        virtio_set_endian_target_default(vdev);
+    }
 
     if (k->reset) {
         k->reset(vdev);
@@ -840,6 +865,28 @@ void virtio_notify_config(VirtIODevice *vdev)
     virtio_notify_vector(vdev, vdev->config_vector);
 }
 
+static bool virtio_device_endian_needed(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+
+    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+#ifdef TARGET_WORDS_BIGENDIAN
+    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
+#else
+    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+#endif
+}
+
+static const VMStateDescription vmstate_virtio_device_endian = {
+    .name = "virtio/device_endian",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(device_endian, VirtIODevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_virtio = {
     .name = "virtio",
     .version_id = 1,
@@ -847,6 +894,13 @@ static const VMStateDescription vmstate_virtio = {
     .minimum_version_id_old = 1,
     .fields = (VMStateField[]) {
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd = &vmstate_virtio_device_endian,
+            .needed = &virtio_device_endian_needed
+        },
+        { 0 }
     }
 };
 
@@ -925,6 +979,12 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
+    /*
+     * We poison the endianness to ensure it does not get used before
+     * subsections have been loaded.
+     */
+    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_UNKNOWN;
+
     if (k->load_config) {
         ret = k->load_config(qbus->parent, f);
         if (ret)
@@ -971,18 +1031,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
         vdev->vq[i].notification = true;
 
         if (vdev->vq[i].pa) {
-            uint16_t nheads;
             virtqueue_init(&vdev->vq[i]);
-            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
-            /* Check it isn't doing very strange things with descriptor numbers. */
-            if (nheads > vdev->vq[i].vring.num) {
-                error_report("VQ %d size 0x%x Guest index 0x%x "
-                             "inconsistent with Host index 0x%x: delta 0x%x",
-                             i, vdev->vq[i].vring.num,
-                             vring_avail_idx(&vdev->vq[i]),
-                             vdev->vq[i].last_avail_idx, nheads);
-                return -1;
-            }
         } else if (vdev->vq[i].last_avail_idx) {
             error_report("VQ %d address 0x0 "
                          "inconsistent with Host index 0x%x",
@@ -1005,7 +1054,33 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
         }
     }
 
-    return vmstate_load_state(f, &vmstate_virtio, vdev, 1);
+    /* Subsections */
+    ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1);
+    if (ret) {
+        return ret;
+    }
+
+    if (vdev->device_endian == VIRTIO_DEVICE_ENDIAN_UNKNOWN) {
+        virtio_set_endian_target_default(vdev);
+    }
+
+    for (i = 0; i < num; i++) {
+        if (vdev->vq[i].pa) {
+            uint16_t nheads;
+            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
+            /* Check it isn't doing strange things with descriptor numbers. */
+            if (nheads > vdev->vq[i].vring.num) {
+                error_report("VQ %d size 0x%x Guest index 0x%x "
+                             "inconsistent with Host index 0x%x: delta 0x%x",
+                             i, vdev->vq[i].vring.num,
+                             vring_avail_idx(&vdev->vq[i]),
+                             vdev->vq[i].last_avail_idx, nheads);
+                return -1;
+            }
+        }
+    }
+
+    return 0;
 }
 
 void virtio_cleanup(VirtIODevice *vdev)
@@ -1062,6 +1137,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
     }
     vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change,
                                                      vdev);
+    virtio_set_endian_target_default(vdev);
 }
 
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index daf0bb2..a60104c 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -104,6 +104,12 @@ typedef struct VirtQueueElement
 #define VIRTIO_DEVICE(obj) \
         OBJECT_CHECK(VirtIODevice, (obj), TYPE_VIRTIO_DEVICE)
 
+enum virtio_device_endian {
+    VIRTIO_DEVICE_ENDIAN_UNKNOWN,
+    VIRTIO_DEVICE_ENDIAN_LITTLE,
+    VIRTIO_DEVICE_ENDIAN_BIG,
+};
+
 struct VirtIODevice
 {
     DeviceState parent_obj;
@@ -121,6 +127,7 @@ struct VirtIODevice
     bool vm_running;
     VMChangeStateEntry *vmstate;
     char *bus_name;
+    uint8_t device_endian;
 };
 
 typedef struct VirtioDeviceClass {
@@ -256,8 +263,9 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
 void virtio_queue_notify_vq(VirtQueue *vq);
 void virtio_irq(VirtQueue *vq);
 
-static inline bool virtio_is_big_endian(void)
+static inline bool virtio_is_big_endian(VirtIODevice *vdev)
 {
-    return target_words_bigendian();
+    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
 }
 #endif

Re: [Qemu-devel] [PATCH v8 11/20] virtio: add endian-ambivalent support to VirtIODevice

[Alexander Graf]

On 13.06.14 13:23, Greg Kurz wrote:
> Some CPU families can dynamically change their endianness. This means we
> can have little endian ppc or big endian arm guests for example. This has
> an impact on legacy virtio data structures since they are target endian.
> We hence introduce a new property to track the endianness of each virtio
> device. It is reasonnably assumed that endianness won't change while the
> device is in use : we hence capture the device endianness when it gets
> reset.
>
> We migrate this property in a subsection, after the device descriptor. This
> means the load code must not rely on it until it is restored. As a consequence,
> the vring sanity checks had to be moved after the call to vmstate_load_state().
> We enforce paranoia by poisoning the property at the begining of virtio_load().
>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>
>   Changes since last version (virtio migration RFC):
>   - endianness cached at device reset time
>   - reworked migration support
>
>   hw/virtio/virtio-pci.c     |    8 ++--
>   hw/virtio/virtio.c         |  100 +++++++++++++++++++++++++++++++++++++++-----
>   include/hw/virtio/virtio.h |   12 ++++-
>   3 files changed, 102 insertions(+), 18 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 390c8d2..2ffceb8 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -406,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
>           break;
>       case 2:
>           val = virtio_config_readw(vdev, addr);
> -        if (virtio_is_big_endian()) {
> +        if (virtio_is_big_endian(vdev)) {
>               val = bswap16(val);
>           }
>           break;
>       case 4:
>           val = virtio_config_readl(vdev, addr);
> -        if (virtio_is_big_endian()) {
> +        if (virtio_is_big_endian(vdev)) {
>               val = bswap32(val);
>           }
>           break;
> @@ -440,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
>           virtio_config_writeb(vdev, addr, val);
>           break;
>       case 2:
> -        if (virtio_is_big_endian()) {
> +        if (virtio_is_big_endian(vdev)) {
>               val = bswap16(val);
>           }
>           virtio_config_writew(vdev, addr, val);
>           break;
>       case 4:
> -        if (virtio_is_big_endian()) {
> +        if (virtio_is_big_endian(vdev)) {
>               val = bswap32(val);
>           }
>           virtio_config_writel(vdev, addr, val);
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 16b73d9..6235df8 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -545,6 +545,24 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
>       vdev->status = val;
>   }
>   
> +static void virtio_set_endian_target_default(VirtIODevice *vdev)
> +{
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
> +#else
> +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> +#endif
> +}
> +
> +static void virtio_set_endian_cpu(VirtIODevice *vdev, CPUState *cpu)
> +{
> +    if (cpu_virtio_is_big_endian(cpu)) {
> +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
> +    } else {
> +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> +    }
> +}
> +
>   void virtio_reset(void *opaque)
>   {
>       VirtIODevice *vdev = opaque;
> @@ -552,6 +570,13 @@ void virtio_reset(void *opaque)
>       int i;
>   
>       virtio_set_status(vdev, 0);
> +    if (current_cpu) {
> +        /* Guest initiated reset */
> +        virtio_set_endian_cpu(vdev, current_cpu);
> +    } else {
> +        /* System reset */
> +        virtio_set_endian_target_default(vdev);
> +    }
>   
>       if (k->reset) {
>           k->reset(vdev);
> @@ -840,6 +865,28 @@ void virtio_notify_config(VirtIODevice *vdev)
>       virtio_notify_vector(vdev, vdev->config_vector);
>   }
>   
> +static bool virtio_device_endian_needed(void *opaque)
> +{
> +    VirtIODevice *vdev = opaque;
> +
> +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
> +#else
> +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> +#endif

if (target_words_bigendian()) {
     return virtio_is_big_endian();
} else {
     return !virtio_is_big_endian();
}

Then we have one less dependency on TARGET_WORDS_BIGENDIAN which means a 
step towards compiling virtio once ;).


Alex

Re: [Qemu-devel] [PATCH v8 11/20] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Fri, 13 Jun 2014 13:46:39 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> On 13.06.14 13:23, Greg Kurz wrote:
> > Some CPU families can dynamically change their endianness. This means we
> > can have little endian ppc or big endian arm guests for example. This has
> > an impact on legacy virtio data structures since they are target endian.
> > We hence introduce a new property to track the endianness of each virtio
> > device. It is reasonnably assumed that endianness won't change while the
> > device is in use : we hence capture the device endianness when it gets
> > reset.
> >
> > We migrate this property in a subsection, after the device descriptor. This
> > means the load code must not rely on it until it is restored. As a consequence,
> > the vring sanity checks had to be moved after the call to vmstate_load_state().
> > We enforce paranoia by poisoning the property at the begining of virtio_load().
> >
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >
> >   Changes since last version (virtio migration RFC):
> >   - endianness cached at device reset time
> >   - reworked migration support
> >
> >   hw/virtio/virtio-pci.c     |    8 ++--
> >   hw/virtio/virtio.c         |  100 +++++++++++++++++++++++++++++++++++++++-----
> >   include/hw/virtio/virtio.h |   12 ++++-
> >   3 files changed, 102 insertions(+), 18 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 390c8d2..2ffceb8 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -406,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
> >           break;
> >       case 2:
> >           val = virtio_config_readw(vdev, addr);
> > -        if (virtio_is_big_endian()) {
> > +        if (virtio_is_big_endian(vdev)) {
> >               val = bswap16(val);
> >           }
> >           break;
> >       case 4:
> >           val = virtio_config_readl(vdev, addr);
> > -        if (virtio_is_big_endian()) {
> > +        if (virtio_is_big_endian(vdev)) {
> >               val = bswap32(val);
> >           }
> >           break;
> > @@ -440,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
> >           virtio_config_writeb(vdev, addr, val);
> >           break;
> >       case 2:
> > -        if (virtio_is_big_endian()) {
> > +        if (virtio_is_big_endian(vdev)) {
> >               val = bswap16(val);
> >           }
> >           virtio_config_writew(vdev, addr, val);
> >           break;
> >       case 4:
> > -        if (virtio_is_big_endian()) {
> > +        if (virtio_is_big_endian(vdev)) {
> >               val = bswap32(val);
> >           }
> >           virtio_config_writel(vdev, addr, val);
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 16b73d9..6235df8 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -545,6 +545,24 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
> >       vdev->status = val;
> >   }
> >   
> > +static void virtio_set_endian_target_default(VirtIODevice *vdev)
> > +{
> > +#ifdef TARGET_WORDS_BIGENDIAN
> > +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
> > +#else
> > +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> > +#endif
> > +}
> > +
> > +static void virtio_set_endian_cpu(VirtIODevice *vdev, CPUState *cpu)
> > +{
> > +    if (cpu_virtio_is_big_endian(cpu)) {
> > +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
> > +    } else {
> > +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> > +    }
> > +}
> > +
> >   void virtio_reset(void *opaque)
> >   {
> >       VirtIODevice *vdev = opaque;
> > @@ -552,6 +570,13 @@ void virtio_reset(void *opaque)
> >       int i;
> >   
> >       virtio_set_status(vdev, 0);
> > +    if (current_cpu) {
> > +        /* Guest initiated reset */
> > +        virtio_set_endian_cpu(vdev, current_cpu);
> > +    } else {
> > +        /* System reset */
> > +        virtio_set_endian_target_default(vdev);
> > +    }
> >   
> >       if (k->reset) {
> >           k->reset(vdev);
> > @@ -840,6 +865,28 @@ void virtio_notify_config(VirtIODevice *vdev)
> >       virtio_notify_vector(vdev, vdev->config_vector);
> >   }
> >   
> > +static bool virtio_device_endian_needed(void *opaque)
> > +{
> > +    VirtIODevice *vdev = opaque;
> > +
> > +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> > +#ifdef TARGET_WORDS_BIGENDIAN
> > +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
> > +#else
> > +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> > +#endif
> 
> if (target_words_bigendian()) {
>      return virtio_is_big_endian();
> } else {
>      return !virtio_is_big_endian();
> }
> 
> Then we have one less dependency on TARGET_WORDS_BIGENDIAN which means a 
> step towards compiling virtio once ;).
> 
> 
> Alex
> 

Heh ! It used to be target_words_bigendian() in the first place and I turned
it into TARGET_WORDS_BIGENDIAN to avoid "hey virtio isn't soon to be common
why don't you sort this out at compile time?" remarks. :)

Since we are definitely not on a fastpath here, I'll gladly fix this.

-- 
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 v8 11/20] virtio: add endian-ambivalent support to VirtIODevice

[Alexander Graf]

On 13.06.14 14:14, Greg Kurz wrote:
> On Fri, 13 Jun 2014 13:46:39 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>> On 13.06.14 13:23, Greg Kurz wrote:
>>> Some CPU families can dynamically change their endianness. This means we
>>> can have little endian ppc or big endian arm guests for example. This has
>>> an impact on legacy virtio data structures since they are target endian.
>>> We hence introduce a new property to track the endianness of each virtio
>>> device. It is reasonnably assumed that endianness won't change while the
>>> device is in use : we hence capture the device endianness when it gets
>>> reset.
>>>
>>> We migrate this property in a subsection, after the device descriptor. This
>>> means the load code must not rely on it until it is restored. As a consequence,
>>> the vring sanity checks had to be moved after the call to vmstate_load_state().
>>> We enforce paranoia by poisoning the property at the begining of virtio_load().
>>>
>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>> ---
>>>
>>>    Changes since last version (virtio migration RFC):
>>>    - endianness cached at device reset time
>>>    - reworked migration support
>>>
>>>    hw/virtio/virtio-pci.c     |    8 ++--
>>>    hw/virtio/virtio.c         |  100 +++++++++++++++++++++++++++++++++++++++-----
>>>    include/hw/virtio/virtio.h |   12 ++++-
>>>    3 files changed, 102 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>> index 390c8d2..2ffceb8 100644
>>> --- a/hw/virtio/virtio-pci.c
>>> +++ b/hw/virtio/virtio-pci.c
>>> @@ -406,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
>>>            break;
>>>        case 2:
>>>            val = virtio_config_readw(vdev, addr);
>>> -        if (virtio_is_big_endian()) {
>>> +        if (virtio_is_big_endian(vdev)) {
>>>                val = bswap16(val);
>>>            }
>>>            break;
>>>        case 4:
>>>            val = virtio_config_readl(vdev, addr);
>>> -        if (virtio_is_big_endian()) {
>>> +        if (virtio_is_big_endian(vdev)) {
>>>                val = bswap32(val);
>>>            }
>>>            break;
>>> @@ -440,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
>>>            virtio_config_writeb(vdev, addr, val);
>>>            break;
>>>        case 2:
>>> -        if (virtio_is_big_endian()) {
>>> +        if (virtio_is_big_endian(vdev)) {
>>>                val = bswap16(val);
>>>            }
>>>            virtio_config_writew(vdev, addr, val);
>>>            break;
>>>        case 4:
>>> -        if (virtio_is_big_endian()) {
>>> +        if (virtio_is_big_endian(vdev)) {
>>>                val = bswap32(val);
>>>            }
>>>            virtio_config_writel(vdev, addr, val);
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 16b73d9..6235df8 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -545,6 +545,24 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
>>>        vdev->status = val;
>>>    }
>>>    
>>> +static void virtio_set_endian_target_default(VirtIODevice *vdev)
>>> +{
>>> +#ifdef TARGET_WORDS_BIGENDIAN
>>> +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
>>> +#else
>>> +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
>>> +#endif
>>> +}
>>> +
>>> +static void virtio_set_endian_cpu(VirtIODevice *vdev, CPUState *cpu)
>>> +{
>>> +    if (cpu_virtio_is_big_endian(cpu)) {
>>> +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
>>> +    } else {
>>> +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
>>> +    }
>>> +}
>>> +
>>>    void virtio_reset(void *opaque)
>>>    {
>>>        VirtIODevice *vdev = opaque;
>>> @@ -552,6 +570,13 @@ void virtio_reset(void *opaque)
>>>        int i;
>>>    
>>>        virtio_set_status(vdev, 0);
>>> +    if (current_cpu) {
>>> +        /* Guest initiated reset */
>>> +        virtio_set_endian_cpu(vdev, current_cpu);
>>> +    } else {
>>> +        /* System reset */
>>> +        virtio_set_endian_target_default(vdev);
>>> +    }
>>>    
>>>        if (k->reset) {
>>>            k->reset(vdev);
>>> @@ -840,6 +865,28 @@ void virtio_notify_config(VirtIODevice *vdev)
>>>        virtio_notify_vector(vdev, vdev->config_vector);
>>>    }
>>>    
>>> +static bool virtio_device_endian_needed(void *opaque)
>>> +{
>>> +    VirtIODevice *vdev = opaque;
>>> +
>>> +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
>>> +#ifdef TARGET_WORDS_BIGENDIAN
>>> +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
>>> +#else
>>> +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
>>> +#endif
>> if (target_words_bigendian()) {
>>       return virtio_is_big_endian();
>> } else {
>>       return !virtio_is_big_endian();
>> }
>>
>> Then we have one less dependency on TARGET_WORDS_BIGENDIAN which means a
>> step towards compiling virtio once ;).
>>
>>
>> Alex
>>
> Heh ! It used to be target_words_bigendian() in the first place and I turned
> it into TARGET_WORDS_BIGENDIAN to avoid "hey virtio isn't soon to be common
> why don't you sort this out at compile time?" remarks. :)
>
> Since we are definitely not on a fastpath here, I'll gladly fix this.

Yeah, just combine it with virtio_set_endian_target_default.

static enum foo virtio_default_endian(vdev)
{
     if (target_words_bigendian()) {
         return VIRTIO_DEVICE_ENDIAN_BIG;
     ....
}

Instead of calling virtio_set_endian_target_default, you do

     vdev->device_endian = virtio_default_endian(vdev);

and

static bool virtio_biendian_section_needed(vdev)
{
     assert(unknown);
     return vdev->device_endian != virtio_default_endian(vdev);
}


Alex

Re: [Qemu-devel] [PATCH v8 11/20] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Fri, 13 Jun 2014 14:41:30 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> On 13.06.14 14:14, Greg Kurz wrote:
> > On Fri, 13 Jun 2014 13:46:39 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> >
> >> On 13.06.14 13:23, Greg Kurz wrote:
> >>> Some CPU families can dynamically change their endianness. This means we
> >>> can have little endian ppc or big endian arm guests for example. This has
> >>> an impact on legacy virtio data structures since they are target endian.
> >>> We hence introduce a new property to track the endianness of each virtio
> >>> device. It is reasonnably assumed that endianness won't change while the
> >>> device is in use : we hence capture the device endianness when it gets
> >>> reset.
> >>>
> >>> We migrate this property in a subsection, after the device descriptor. This
> >>> means the load code must not rely on it until it is restored. As a consequence,
> >>> the vring sanity checks had to be moved after the call to vmstate_load_state().
> >>> We enforce paranoia by poisoning the property at the begining of virtio_load().
> >>>
> >>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >>> ---
> >>>
> >>>    Changes since last version (virtio migration RFC):
> >>>    - endianness cached at device reset time
> >>>    - reworked migration support
> >>>
> >>>    hw/virtio/virtio-pci.c     |    8 ++--
> >>>    hw/virtio/virtio.c         |  100 +++++++++++++++++++++++++++++++++++++++-----
> >>>    include/hw/virtio/virtio.h |   12 ++++-
> >>>    3 files changed, 102 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >>> index 390c8d2..2ffceb8 100644
> >>> --- a/hw/virtio/virtio-pci.c
> >>> +++ b/hw/virtio/virtio-pci.c
> >>> @@ -406,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
> >>>            break;
> >>>        case 2:
> >>>            val = virtio_config_readw(vdev, addr);
> >>> -        if (virtio_is_big_endian()) {
> >>> +        if (virtio_is_big_endian(vdev)) {
> >>>                val = bswap16(val);
> >>>            }
> >>>            break;
> >>>        case 4:
> >>>            val = virtio_config_readl(vdev, addr);
> >>> -        if (virtio_is_big_endian()) {
> >>> +        if (virtio_is_big_endian(vdev)) {
> >>>                val = bswap32(val);
> >>>            }
> >>>            break;
> >>> @@ -440,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
> >>>            virtio_config_writeb(vdev, addr, val);
> >>>            break;
> >>>        case 2:
> >>> -        if (virtio_is_big_endian()) {
> >>> +        if (virtio_is_big_endian(vdev)) {
> >>>                val = bswap16(val);
> >>>            }
> >>>            virtio_config_writew(vdev, addr, val);
> >>>            break;
> >>>        case 4:
> >>> -        if (virtio_is_big_endian()) {
> >>> +        if (virtio_is_big_endian(vdev)) {
> >>>                val = bswap32(val);
> >>>            }
> >>>            virtio_config_writel(vdev, addr, val);
> >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >>> index 16b73d9..6235df8 100644
> >>> --- a/hw/virtio/virtio.c
> >>> +++ b/hw/virtio/virtio.c
> >>> @@ -545,6 +545,24 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
> >>>        vdev->status = val;
> >>>    }
> >>>    
> >>> +static void virtio_set_endian_target_default(VirtIODevice *vdev)
> >>> +{
> >>> +#ifdef TARGET_WORDS_BIGENDIAN
> >>> +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
> >>> +#else
> >>> +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> >>> +#endif
> >>> +}
> >>> +
> >>> +static void virtio_set_endian_cpu(VirtIODevice *vdev, CPUState *cpu)
> >>> +{
> >>> +    if (cpu_virtio_is_big_endian(cpu)) {
> >>> +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
> >>> +    } else {
> >>> +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> >>> +    }
> >>> +}
> >>> +
> >>>    void virtio_reset(void *opaque)
> >>>    {
> >>>        VirtIODevice *vdev = opaque;
> >>> @@ -552,6 +570,13 @@ void virtio_reset(void *opaque)
> >>>        int i;
> >>>    
> >>>        virtio_set_status(vdev, 0);
> >>> +    if (current_cpu) {
> >>> +        /* Guest initiated reset */
> >>> +        virtio_set_endian_cpu(vdev, current_cpu);
> >>> +    } else {
> >>> +        /* System reset */
> >>> +        virtio_set_endian_target_default(vdev);
> >>> +    }
> >>>    
> >>>        if (k->reset) {
> >>>            k->reset(vdev);
> >>> @@ -840,6 +865,28 @@ void virtio_notify_config(VirtIODevice *vdev)
> >>>        virtio_notify_vector(vdev, vdev->config_vector);
> >>>    }
> >>>    
> >>> +static bool virtio_device_endian_needed(void *opaque)
> >>> +{
> >>> +    VirtIODevice *vdev = opaque;
> >>> +
> >>> +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> >>> +#ifdef TARGET_WORDS_BIGENDIAN
> >>> +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
> >>> +#else
> >>> +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> >>> +#endif
> >> if (target_words_bigendian()) {
> >>       return virtio_is_big_endian();
> >> } else {
> >>       return !virtio_is_big_endian();
> >> }
> >>
> >> Then we have one less dependency on TARGET_WORDS_BIGENDIAN which means a
> >> step towards compiling virtio once ;).
> >>
> >>
> >> Alex
> >>
> > Heh ! It used to be target_words_bigendian() in the first place and I turned
> > it into TARGET_WORDS_BIGENDIAN to avoid "hey virtio isn't soon to be common
> > why don't you sort this out at compile time?" remarks. :)
> >
> > Since we are definitely not on a fastpath here, I'll gladly fix this.
> 
> Yeah, just combine it with virtio_set_endian_target_default.
> 
> static enum foo virtio_default_endian(vdev)
> {
>      if (target_words_bigendian()) {
>          return VIRTIO_DEVICE_ENDIAN_BIG;
>      ....
> }
> 
> Instead of calling virtio_set_endian_target_default, you do
> 
>      vdev->device_endian = virtio_default_endian(vdev);
> 
> and
> 
> static bool virtio_biendian_section_needed(vdev)
> {
>      assert(unknown);
>      return vdev->device_endian != virtio_default_endian(vdev);
> }
> 
> 
> Alex
> 

It sure looks better. 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 v8 12/20] virtio: memory accessors for endian-ambivalent targets

[Greg Kurz]

This is the virtio-access.h header file taken from Rusty's "endian-ambivalent
targets using legacy virtio" patch. It introduces helpers that should be used
when accessing vring data or by drivers for data that contains headers.
The virtio config space is also target endian, but the current code already
handles that with the virtio_is_big_endian() helper. There is no obvious
benefit at using the virtio accessors in this case.

Now we have two distinct paths: a fast inline one for fixed endian targets,
and a slow out-of-line one for targets that define the new TARGET_IS_BIENDIAN
macro.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
[ relicensed virtio-access.h to GPLv2+ on Rusty's request,
  pass &address_space_memory to physical memory accessors,
  per-device endianness,
  virtio tswap16 and tswap64 helpers,
  faspath for fixed endian targets,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Cc: Cédric Le Goater <clg@fr.ibm.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---

 Changes since version 7:
 - moved out of the original patch
 - fastpath for fixed endian targets

 include/hw/virtio/virtio-access.h |  170 +++++++++++++++++++++++++++++++++++++
 1 file changed, 170 insertions(+)
 create mode 100644 include/hw/virtio/virtio-access.h

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
new file mode 100644
index 0000000..46456fd
--- /dev/null
+++ b/include/hw/virtio/virtio-access.h
@@ -0,0 +1,170 @@
+/*
+ * 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"
+#include "exec/address-spaces.h"
+
+static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
+{
+#if defined(TARGET_IS_BIENDIAN)
+    return virtio_is_big_endian(vdev);
+#elif defined(TARGET_WORDS_BIGENDIAN)
+    return true;
+#else
+    return false;
+#endif
+}
+
+static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa)
+{
+    if (virtio_access_is_big_endian(vdev)) {
+        return lduw_be_phys(&address_space_memory, pa);
+    }
+    return lduw_le_phys(&address_space_memory, pa);
+}
+
+static inline uint32_t virtio_ldl_phys(VirtIODevice *vdev, hwaddr pa)
+{
+    if (virtio_access_is_big_endian(vdev)) {
+        return ldl_be_phys(&address_space_memory, pa);
+    }
+    return ldl_le_phys(&address_space_memory, pa);
+}
+
+static inline uint64_t virtio_ldq_phys(VirtIODevice *vdev, hwaddr pa)
+{
+    if (virtio_access_is_big_endian(vdev)) {
+        return ldq_be_phys(&address_space_memory, pa);
+    }
+    return ldq_le_phys(&address_space_memory, pa);
+}
+
+static inline void virtio_stw_phys(VirtIODevice *vdev, hwaddr pa,
+                                   uint16_t value)
+{
+    if (virtio_access_is_big_endian(vdev)) {
+        stw_be_phys(&address_space_memory, pa, value);
+    } else {
+        stw_le_phys(&address_space_memory, pa, value);
+    }
+}
+
+static inline void virtio_stl_phys(VirtIODevice *vdev, hwaddr pa,
+                                   uint32_t value)
+{
+    if (virtio_access_is_big_endian(vdev)) {
+        stl_be_phys(&address_space_memory, pa, value);
+    } else {
+        stl_le_phys(&address_space_memory, pa, value);
+    }
+}
+
+static inline void virtio_stw_p(VirtIODevice *vdev, void *ptr, uint16_t v)
+{
+    if (virtio_access_is_big_endian(vdev)) {
+        stw_be_p(ptr, v);
+    } else {
+        stw_le_p(ptr, v);
+    }
+}
+
+static inline void virtio_stl_p(VirtIODevice *vdev, void *ptr, uint32_t v)
+{
+    if (virtio_access_is_big_endian(vdev)) {
+        stl_be_p(ptr, v);
+    } else {
+        stl_le_p(ptr, v);
+    }
+}
+
+static inline void virtio_stq_p(VirtIODevice *vdev, void *ptr, uint64_t v)
+{
+    if (virtio_access_is_big_endian(vdev)) {
+        stq_be_p(ptr, v);
+    } else {
+        stq_le_p(ptr, v);
+    }
+}
+
+static inline int virtio_lduw_p(VirtIODevice *vdev, const void *ptr)
+{
+    if (virtio_access_is_big_endian(vdev)) {
+        return lduw_be_p(ptr);
+    } else {
+        return lduw_le_p(ptr);
+    }
+}
+
+static inline int virtio_ldl_p(VirtIODevice *vdev, const void *ptr)
+{
+    if (virtio_access_is_big_endian(vdev)) {
+        return ldl_be_p(ptr);
+    } else {
+        return ldl_le_p(ptr);
+    }
+}
+
+static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, const void *ptr)
+{
+    if (virtio_access_is_big_endian(vdev)) {
+        return ldq_be_p(ptr);
+    } else {
+        return ldq_le_p(ptr);
+    }
+}
+
+static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
+{
+#ifdef HOST_WORDS_BIGENDIAN
+    return virtio_access_is_big_endian(vdev) ? s : bswap16(s);
+#else
+    return virtio_access_is_big_endian(vdev) ? bswap16(s) : s;
+#endif
+}
+
+static inline void virtio_tswap16s(VirtIODevice *vdev, uint16_t *s)
+{
+    *s = virtio_tswap16(vdev, *s);
+}
+
+static inline uint32_t virtio_tswap32(VirtIODevice *vdev, uint32_t s)
+{
+#ifdef HOST_WORDS_BIGENDIAN
+    return virtio_access_is_big_endian(vdev) ? s : bswap32(s);
+#else
+    return virtio_access_is_big_endian(vdev) ? bswap32(s) : s;
+#endif
+}
+
+static inline void virtio_tswap32s(VirtIODevice *vdev, uint32_t *s)
+{
+    *s = virtio_tswap32(vdev, *s);
+}
+
+static inline uint64_t virtio_tswap64(VirtIODevice *vdev, uint64_t s)
+{
+#ifdef HOST_WORDS_BIGENDIAN
+    return virtio_access_is_big_endian(vdev) ? s : bswap64(s);
+#else
+    return virtio_access_is_big_endian(vdev) ? bswap64(s) : s;
+#endif
+}
+
+static inline void virtio_tswap64s(VirtIODevice *vdev, uint64_t *s)
+{
+    *s = virtio_tswap64(vdev, *s);
+}
+#endif /* _QEMU_VIRTIO_ACCESS_H */

[Qemu-devel] [PATCH v8 13/20] virtio: allow byte swapping for vring

[Greg Kurz]

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

Quoting original text from Rusty: "This is based on a simpler patch by Anthony
Liguouri".

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
[ add VirtIODevice * argument to most helpers,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
---
 hw/virtio/virtio.c |   89 ++++++++++++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 6235df8..6c315c5 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -20,6 +20,7 @@
 #include "qemu/atomic.h"
 #include "hw/virtio/virtio-bus.h"
 #include "migration/migration.h"
+#include "hw/virtio/virtio-access.h"
 
 /*
  * The alignment to use between consumer and producer parts of vring.
@@ -102,53 +103,56 @@ 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(VirtIODevice *vdev, hwaddr desc_pa,
+                                       int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, addr);
-    return ldq_phys(&address_space_memory, pa);
+    return virtio_ldq_phys(vdev, pa);
 }
 
-static inline uint32_t vring_desc_len(hwaddr desc_pa, int i)
+static inline uint32_t vring_desc_len(VirtIODevice *vdev, hwaddr desc_pa, int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, len);
-    return ldl_phys(&address_space_memory, pa);
+    return virtio_ldl_phys(vdev, pa);
 }
 
-static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i)
+static inline uint16_t vring_desc_flags(VirtIODevice *vdev, hwaddr desc_pa,
+                                        int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, flags);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(vdev, pa);
 }
 
-static inline uint16_t vring_desc_next(hwaddr desc_pa, int i)
+static inline uint16_t vring_desc_next(VirtIODevice *vdev, hwaddr desc_pa,
+                                       int i)
 {
     hwaddr pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, next);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(vdev, pa);
 }
 
 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(vq->vdev, pa);
 }
 
 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(vq->vdev, pa);
 }
 
 static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
 {
     hwaddr pa;
     pa = vq->vring.avail + offsetof(VRingAvail, ring[i]);
-    return lduw_phys(&address_space_memory, pa);
+    return virtio_lduw_phys(vq->vdev, pa);
 }
 
 static inline uint16_t vring_used_event(VirtQueue *vq)
@@ -160,44 +164,44 @@ 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(vq->vdev, pa, val);
 }
 
 static inline void vring_used_ring_len(VirtQueue *vq, int i, uint32_t val)
 {
     hwaddr pa;
     pa = vq->vring.used + offsetof(VRingUsed, ring[i].len);
-    stl_phys(&address_space_memory, pa, val);
+    virtio_stl_phys(vq->vdev, pa, val);
 }
 
 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(vq->vdev, pa);
 }
 
 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(vq->vdev, pa, val);
 }
 
 static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
 {
+    VirtIODevice *vdev = vq->vdev;
     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(vdev, pa, virtio_lduw_phys(vdev, pa) | mask);
 }
 
 static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
 {
+    VirtIODevice *vdev = vq->vdev;
     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(vdev, pa, virtio_lduw_phys(vdev, pa) & ~mask);
 }
 
 static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
@@ -207,7 +211,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(vq->vdev, pa, val);
 }
 
 void virtio_queue_set_notification(VirtQueue *vq, int enable)
@@ -324,17 +328,18 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
     return head;
 }
 
-static unsigned virtqueue_next_desc(hwaddr desc_pa,
+static unsigned virtqueue_next_desc(VirtIODevice *vdev, hwaddr desc_pa,
                                     unsigned int i, unsigned int max)
 {
     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(vdev, desc_pa, i) & 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(vdev, desc_pa, i);
     /* Make sure compiler knows to grab that: we don't want it changing! */
     smp_wmb();
 
@@ -357,6 +362,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 
     total_bufs = in_total = out_total = 0;
     while (virtqueue_num_heads(vq, idx)) {
+        VirtIODevice *vdev = vq->vdev;
         unsigned int max, num_bufs, indirect = 0;
         hwaddr desc_pa;
         int i;
@@ -366,8 +372,8 @@ 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(vdev, desc_pa, i) & VRING_DESC_F_INDIRECT) {
+            if (vring_desc_len(vdev, desc_pa, i) % sizeof(VRingDesc)) {
                 error_report("Invalid size for indirect buffer table");
                 exit(1);
             }
@@ -380,8 +386,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(vdev, desc_pa, i) / sizeof(VRingDesc);
+            desc_pa = vring_desc_addr(vdev, desc_pa, i);
             num_bufs = i = 0;
         }
 
@@ -392,15 +398,15 @@ 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(vdev, desc_pa, i) & VRING_DESC_F_WRITE) {
+                in_total += vring_desc_len(vdev, desc_pa, i);
             } else {
-                out_total += vring_desc_len(desc_pa, i);
+                out_total += vring_desc_len(vdev, desc_pa, i);
             }
             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(vdev, desc_pa, i, max)) != max);
 
         if (!indirect)
             total_bufs = num_bufs;
@@ -451,6 +457,7 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
 {
     unsigned int i, head, max;
     hwaddr desc_pa = vq->vring.desc;
+    VirtIODevice *vdev = vq->vdev;
 
     if (!virtqueue_num_heads(vq, vq->last_avail_idx))
         return 0;
@@ -461,19 +468,19 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
     max = vq->vring.num;
 
     i = head = virtqueue_get_head(vq, vq->last_avail_idx++);
-    if (vq->vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
+    if (vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
         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(vdev, desc_pa, i) & VRING_DESC_F_INDIRECT) {
+        if (vring_desc_len(vdev, desc_pa, i) % 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(vdev, desc_pa, i) / sizeof(VRingDesc);
+        desc_pa = vring_desc_addr(vdev, desc_pa, i);
         i = 0;
     }
 
@@ -481,30 +488,30 @@ 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(vdev, desc_pa, i) & 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(vdev, desc_pa, i);
             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(vdev, desc_pa, i);
             sg = &elem->out_sg[elem->out_num++];
         }
 
-        sg->iov_len = vring_desc_len(desc_pa, i);
+        sg->iov_len = vring_desc_len(vdev, desc_pa, i);
 
         /* 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(vdev, desc_pa, i, max)) != max);
 
     /* Now map what we have collected */
     virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);

[Qemu-devel] [PATCH v8 14/20] 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>
[ pass VirtIODevice * to memory accessors,
  TCP checksums fix by Cédric Le Goater,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Cc: Cédric Le Goater <clg@fr.ibm.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/net/virtio-net.c |   34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 58e7b73..fb17919 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(vdev, &netcfg.status, n->status);
+    virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
     memcpy(netcfg.mac, n->mac, ETH_ALEN);
     memcpy(config, &netcfg, n->config_size);
 }
@@ -611,6 +612,7 @@ static int virtio_net_handle_offloads(VirtIONet *n, uint8_t cmd,
 static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
                                  struct iovec *iov, unsigned int iov_cnt)
 {
+    VirtIODevice *vdev = VIRTIO_DEVICE(n);
     struct virtio_net_ctrl_mac mac_data;
     size_t s;
     NetClientState *nc = qemu_get_queue(n->nic);
@@ -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(vdev, &mac_data.entries);
     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(vdev, &mac_data.entries);
     if (s != sizeof(mac_data.entries)) {
         goto error;
     }
@@ -706,12 +708,13 @@ error:
 static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
                                         struct iovec *iov, unsigned int iov_cnt)
 {
+    VirtIODevice *vdev = VIRTIO_DEVICE(n);
     uint16_t vid;
     size_t s;
     NetClientState *nc = qemu_get_queue(n->nic);
 
     s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
-    vid = lduw_p(&vid);
+    vid = virtio_lduw_p(vdev, &vid);
     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(vdev, &mq.virtqueue_pairs);
 
     if (queues < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
         queues > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
@@ -863,6 +866,14 @@ static int virtio_net_has_buffers(VirtIONetQueue *q, int bufsize)
     return 1;
 }
 
+static void virtio_net_hdr_swap(VirtIODevice *vdev, struct virtio_net_hdr *hdr)
+{
+    virtio_tswap16s(vdev, &hdr->hdr_len);
+    virtio_tswap16s(vdev, &hdr->gso_size);
+    virtio_tswap16s(vdev, &hdr->csum_start);
+    virtio_tswap16s(vdev, &hdr->csum_offset);
+}
+
 /* dhclient uses AF_PACKET but doesn't pass auxdata to the kernel so
  * it never finds out that the packets don't have valid checksums.  This
  * causes dhclient to get upset.  Fedora's carried a patch for ages to
@@ -898,6 +909,7 @@ static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
         void *wbuf = (void *)buf;
         work_around_broken_dhclient(wbuf, wbuf + n->host_hdr_len,
                                     size - n->host_hdr_len);
+        virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf);
         iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr));
     } else {
         struct virtio_net_hdr hdr = {
@@ -1047,7 +1059,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(vdev, &mhdr.num_buffers, i);
         iov_from_buf(mhdr_sg, mhdr_cnt,
                      0,
                      &mhdr.num_buffers, sizeof mhdr.num_buffers);
@@ -1106,6 +1118,14 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
             exit(1);
         }
 
+        if (n->has_vnet_hdr) {
+            if (out_sg[0].iov_len < n->guest_hdr_len) {
+                error_report("virtio-net header incorrect");
+                exit(1);
+            }
+            virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
+        }
+
         /*
          * If host wants to see the guest header as is, we can
          * pass it on unchanged. Otherwise, copy just the parts

Re: [Qemu-devel] [PATCH v8 14/20] virtio-net: use virtio wrappers to access headers

[Alexander Graf]

On 13.06.14 13:24, 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>
> [ pass VirtIODevice * to memory accessors,
>    TCP checksums fix by Cédric Le Goater,
>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Cc: Cédric Le Goater <clg@fr.ibm.com>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>   hw/net/virtio-net.c |   34 +++++++++++++++++++++++++++-------
>   1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 58e7b73..fb17919 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(vdev, &netcfg.status, n->status);
> +    virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
>       memcpy(netcfg.mac, n->mac, ETH_ALEN);
>       memcpy(config, &netcfg, n->config_size);
>   }
> @@ -611,6 +612,7 @@ static int virtio_net_handle_offloads(VirtIONet *n, uint8_t cmd,
>   static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>                                    struct iovec *iov, unsigned int iov_cnt)
>   {
> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>       struct virtio_net_ctrl_mac mac_data;
>       size_t s;
>       NetClientState *nc = qemu_get_queue(n->nic);
> @@ -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(vdev, &mac_data.entries);
>       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(vdev, &mac_data.entries);
>       if (s != sizeof(mac_data.entries)) {
>           goto error;
>       }
> @@ -706,12 +708,13 @@ error:
>   static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
>                                           struct iovec *iov, unsigned int iov_cnt)
>   {
> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>       uint16_t vid;
>       size_t s;
>       NetClientState *nc = qemu_get_queue(n->nic);
>   
>       s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
> -    vid = lduw_p(&vid);
> +    vid = virtio_lduw_p(vdev, &vid);
>       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(vdev, &mq.virtqueue_pairs);
>   
>       if (queues < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
>           queues > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
> @@ -863,6 +866,14 @@ static int virtio_net_has_buffers(VirtIONetQueue *q, int bufsize)
>       return 1;
>   }
>   
> +static void virtio_net_hdr_swap(VirtIODevice *vdev, struct virtio_net_hdr *hdr)
> +{
> +    virtio_tswap16s(vdev, &hdr->hdr_len);
> +    virtio_tswap16s(vdev, &hdr->gso_size);
> +    virtio_tswap16s(vdev, &hdr->csum_start);
> +    virtio_tswap16s(vdev, &hdr->csum_offset);
> +}
> +
>   /* dhclient uses AF_PACKET but doesn't pass auxdata to the kernel so
>    * it never finds out that the packets don't have valid checksums.  This
>    * causes dhclient to get upset.  Fedora's carried a patch for ages to
> @@ -898,6 +909,7 @@ static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
>           void *wbuf = (void *)buf;
>           work_around_broken_dhclient(wbuf, wbuf + n->host_hdr_len,
>                                       size - n->host_hdr_len);
> +        virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf);
>           iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr));
>       } else {
>           struct virtio_net_hdr hdr = {
> @@ -1047,7 +1059,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(vdev, &mhdr.num_buffers, i);
>           iov_from_buf(mhdr_sg, mhdr_cnt,
>                        0,
>                        &mhdr.num_buffers, sizeof mhdr.num_buffers);
> @@ -1106,6 +1118,14 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>               exit(1);
>           }
>   
> +        if (n->has_vnet_hdr) {
> +            if (out_sg[0].iov_len < n->guest_hdr_len) {
> +                error_report("virtio-net header incorrect");
> +                exit(1);
> +            }
> +            virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);

Where does this come from? If you're doing twap() wouldn't that have 
been broken before too?


Alex

Re: [Qemu-devel] [PATCH v8 14/20] virtio-net: use virtio wrappers to access headers

[Cedric Le Goater]

On 06/13/2014 01:52 PM, Alexander Graf wrote:
> 
> On 13.06.14 13:24, 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>
>> [ pass VirtIODevice * to memory accessors,
>>    TCP checksums fix by Cédric Le Goater,
>>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
>> Cc: Cédric Le Goater <clg@fr.ibm.com>
>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>> ---
>>   hw/net/virtio-net.c |   34 +++++++++++++++++++++++++++-------
>>   1 file changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 58e7b73..fb17919 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(vdev, &netcfg.status, n->status);
>> +    virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
>>       memcpy(netcfg.mac, n->mac, ETH_ALEN);
>>       memcpy(config, &netcfg, n->config_size);
>>   }
>> @@ -611,6 +612,7 @@ static int virtio_net_handle_offloads(VirtIONet *n, uint8_t cmd,
>>   static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>>                                    struct iovec *iov, unsigned int iov_cnt)
>>   {
>> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>       struct virtio_net_ctrl_mac mac_data;
>>       size_t s;
>>       NetClientState *nc = qemu_get_queue(n->nic);
>> @@ -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(vdev, &mac_data.entries);
>>       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(vdev, &mac_data.entries);
>>       if (s != sizeof(mac_data.entries)) {
>>           goto error;
>>       }
>> @@ -706,12 +708,13 @@ error:
>>   static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
>>                                           struct iovec *iov, unsigned int iov_cnt)
>>   {
>> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>       uint16_t vid;
>>       size_t s;
>>       NetClientState *nc = qemu_get_queue(n->nic);
>>         s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
>> -    vid = lduw_p(&vid);
>> +    vid = virtio_lduw_p(vdev, &vid);
>>       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(vdev, &mq.virtqueue_pairs);
>>         if (queues < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
>>           queues > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
>> @@ -863,6 +866,14 @@ static int virtio_net_has_buffers(VirtIONetQueue *q, int bufsize)
>>       return 1;
>>   }
>>   +static void virtio_net_hdr_swap(VirtIODevice *vdev, struct virtio_net_hdr *hdr)
>> +{
>> +    virtio_tswap16s(vdev, &hdr->hdr_len);
>> +    virtio_tswap16s(vdev, &hdr->gso_size);
>> +    virtio_tswap16s(vdev, &hdr->csum_start);
>> +    virtio_tswap16s(vdev, &hdr->csum_offset);
>> +}
>> +
>>   /* dhclient uses AF_PACKET but doesn't pass auxdata to the kernel so
>>    * it never finds out that the packets don't have valid checksums.  This
>>    * causes dhclient to get upset.  Fedora's carried a patch for ages to
>> @@ -898,6 +909,7 @@ static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
>>           void *wbuf = (void *)buf;
>>           work_around_broken_dhclient(wbuf, wbuf + n->host_hdr_len,
>>                                       size - n->host_hdr_len);
>> +        virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf);
>>           iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr));
>>       } else {
>>           struct virtio_net_hdr hdr = {
>> @@ -1047,7 +1059,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(vdev, &mhdr.num_buffers, i);
>>           iov_from_buf(mhdr_sg, mhdr_cnt,
>>                        0,
>>                        &mhdr.num_buffers, sizeof mhdr.num_buffers);
>> @@ -1106,6 +1118,14 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>>               exit(1);
>>           }
>>   +        if (n->has_vnet_hdr) {
>> +            if (out_sg[0].iov_len < n->guest_hdr_len) {
>> +                error_report("virtio-net header incorrect");
>> +                exit(1);
>> +            }
>> +            virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
> 
> Where does this come from? If you're doing twap() wouldn't that have been broken before too?

That's a (possible) fix for virtio-net when vhost=off. It should probably 
be in another patch.

C.

Re: [Qemu-devel] [PATCH v8 14/20] virtio-net: use virtio wrappers to access headers

[Greg Kurz]

On Fri, 13 Jun 2014 14:24:47 +0200
Cedric Le Goater <clg@fr.ibm.com> wrote:

> On 06/13/2014 01:52 PM, Alexander Graf wrote:
> > 
> > On 13.06.14 13:24, 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>
> >> [ pass VirtIODevice * to memory accessors,
> >>    TCP checksums fix by Cédric Le Goater,
> >>    Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> >> Cc: Cédric Le Goater <clg@fr.ibm.com>
> >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >> ---
> >>   hw/net/virtio-net.c |   34 +++++++++++++++++++++++++++-------
> >>   1 file changed, 27 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 58e7b73..fb17919 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(vdev, &netcfg.status, n->status);
> >> +    virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
> >>       memcpy(netcfg.mac, n->mac, ETH_ALEN);
> >>       memcpy(config, &netcfg, n->config_size);
> >>   }
> >> @@ -611,6 +612,7 @@ static int virtio_net_handle_offloads(VirtIONet *n, uint8_t cmd,
> >>   static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >>                                    struct iovec *iov, unsigned int iov_cnt)
> >>   {
> >> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
> >>       struct virtio_net_ctrl_mac mac_data;
> >>       size_t s;
> >>       NetClientState *nc = qemu_get_queue(n->nic);
> >> @@ -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(vdev, &mac_data.entries);
> >>       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(vdev, &mac_data.entries);
> >>       if (s != sizeof(mac_data.entries)) {
> >>           goto error;
> >>       }
> >> @@ -706,12 +708,13 @@ error:
> >>   static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
> >>                                           struct iovec *iov, unsigned int iov_cnt)
> >>   {
> >> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
> >>       uint16_t vid;
> >>       size_t s;
> >>       NetClientState *nc = qemu_get_queue(n->nic);
> >>         s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
> >> -    vid = lduw_p(&vid);
> >> +    vid = virtio_lduw_p(vdev, &vid);
> >>       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(vdev, &mq.virtqueue_pairs);
> >>         if (queues < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
> >>           queues > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
> >> @@ -863,6 +866,14 @@ static int virtio_net_has_buffers(VirtIONetQueue *q, int bufsize)
> >>       return 1;
> >>   }
> >>   +static void virtio_net_hdr_swap(VirtIODevice *vdev, struct virtio_net_hdr *hdr)
> >> +{
> >> +    virtio_tswap16s(vdev, &hdr->hdr_len);
> >> +    virtio_tswap16s(vdev, &hdr->gso_size);
> >> +    virtio_tswap16s(vdev, &hdr->csum_start);
> >> +    virtio_tswap16s(vdev, &hdr->csum_offset);
> >> +}
> >> +
> >>   /* dhclient uses AF_PACKET but doesn't pass auxdata to the kernel so
> >>    * it never finds out that the packets don't have valid checksums.  This
> >>    * causes dhclient to get upset.  Fedora's carried a patch for ages to
> >> @@ -898,6 +909,7 @@ static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
> >>           void *wbuf = (void *)buf;
> >>           work_around_broken_dhclient(wbuf, wbuf + n->host_hdr_len,
> >>                                       size - n->host_hdr_len);
> >> +        virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf);
> >>           iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr));
> >>       } else {
> >>           struct virtio_net_hdr hdr = {
> >> @@ -1047,7 +1059,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(vdev, &mhdr.num_buffers, i);
> >>           iov_from_buf(mhdr_sg, mhdr_cnt,
> >>                        0,
> >>                        &mhdr.num_buffers, sizeof mhdr.num_buffers);
> >> @@ -1106,6 +1118,14 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> >>               exit(1);
> >>           }
> >>   +        if (n->has_vnet_hdr) {
> >> +            if (out_sg[0].iov_len < n->guest_hdr_len) {
> >> +                error_report("virtio-net header incorrect");
> >> +                exit(1);
> >> +            }
> >> +            virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
> > 
> > Where does this come from? If you're doing twap() wouldn't that have been broken before too?
> 
> That's a (possible) fix for virtio-net when vhost=off. It should probably 
> be in another patch.
> 
> C.
> 

Yeah you are right, this is a fix for a different issue. I'll simply drop these bits for now, and
send a standalone patch later.

-- 
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 v8 15/20] 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>
[ pass VirtIODevice * to memory accessors,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio-balloon.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 84d1733..cbd53cf 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)
 {
@@ -205,8 +206,9 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
         while (iov_to_buf(elem.out_sg, elem.out_num, offset, &pfn, 4) == 4) {
             ram_addr_t pa;
             ram_addr_t addr;
+            int p = virtio_ldl_p(vdev, &pfn);
 
-            pa = (ram_addr_t)ldl_p(&pfn) << VIRTIO_BALLOON_PFN_SHIFT;
+            pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT;
             offset += 4;
 
             /* FIXME: remove get_system_memory(), but how? */
@@ -247,8 +249,8 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
 
     while (iov_to_buf(elem->out_sg, elem->out_num, offset, &stat, sizeof(stat))
            == sizeof(stat)) {
-        uint16_t tag = tswap16(stat.tag);
-        uint64_t val = tswap64(stat.val);
+        uint16_t tag = virtio_tswap16(vdev, stat.tag);
+        uint64_t val = virtio_tswap64(vdev, stat.val);
 
         offset += sizeof(stat);
         if (tag < VIRTIO_BALLOON_S_NR)

[Qemu-devel] [PATCH v8 16/20] 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>
[ pass VirtIODevice * to memory accessors,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/block/virtio-blk.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e432bcd..dc7e811 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
 {
@@ -76,7 +77,8 @@ 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);
+        int p = virtio_ldl_p(VIRTIO_DEVICE(req->dev), &req->out->type);
+        bool is_read = !(p & VIRTIO_BLK_T_OUT);
         if (virtio_blk_handle_rw_error(req, -ret, is_read))
             return;
     }
@@ -129,6 +131,8 @@ int virtio_blk_handle_scsi_req(VirtIOBlock *blk,
 {
     int status = VIRTIO_BLK_S_OK;
     struct virtio_scsi_inhdr *scsi = NULL;
+    VirtIODevice *vdev = VIRTIO_DEVICE(blk);
+
 #ifdef __linux__
     int i;
     struct sg_io_hdr hdr;
@@ -223,12 +227,12 @@ int virtio_blk_handle_scsi_req(VirtIOBlock *blk,
         hdr.status = CHECK_CONDITION;
     }
 
-    stl_p(&scsi->errors,
-          hdr.status | (hdr.msg_status << 8) |
-          (hdr.host_status << 16) | (hdr.driver_status << 24));
-    stl_p(&scsi->residual, hdr.resid);
-    stl_p(&scsi->sense_len, hdr.sb_len_wr);
-    stl_p(&scsi->data_len, hdr.dxfer_len);
+    virtio_stl_p(vdev, &scsi->errors,
+                 hdr.status | (hdr.msg_status << 8) |
+                 (hdr.host_status << 16) | (hdr.driver_status << 24));
+    virtio_stl_p(vdev, &scsi->residual, hdr.resid);
+    virtio_stl_p(vdev, &scsi->sense_len, hdr.sb_len_wr);
+    virtio_stl_p(vdev, &scsi->data_len, hdr.dxfer_len);
 
     return status;
 #else
@@ -238,7 +242,7 @@ int virtio_blk_handle_scsi_req(VirtIOBlock *blk,
 fail:
     /* Just put anything nonzero so that the ioctl fails in the guest.  */
     if (scsi) {
-        stl_p(&scsi->errors, 255);
+        virtio_stl_p(vdev, &scsi->errors, 255);
     }
     return status;
 }
@@ -293,7 +297,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(VIRTIO_DEVICE(req->dev), &req->out->sector);
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
 
@@ -327,7 +331,7 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
 {
     uint64_t sector;
 
-    sector = ldq_p(&req->out->sector);
+    sector = virtio_ldq_p(VIRTIO_DEVICE(req->dev), &req->out->sector);
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
 
@@ -365,7 +369,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(VIRTIO_DEVICE(req->dev), &req->out->type);
 
     if (type & VIRTIO_BLK_T_FLUSH) {
         virtio_blk_handle_flush(req, mrb);
@@ -494,12 +498,12 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 
     bdrv_get_geometry(s->bs, &capacity);
     memset(&blkcfg, 0, sizeof(blkcfg));
-    stq_p(&blkcfg.capacity, capacity);
-    stl_p(&blkcfg.seg_max, 128 - 2);
-    stw_p(&blkcfg.cylinders, s->conf->cyls);
-    stl_p(&blkcfg.blk_size, blk_size);
-    stw_p(&blkcfg.min_io_size, s->conf->min_io_size / blk_size);
-    stw_p(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size);
+    virtio_stq_p(vdev, &blkcfg.capacity, capacity);
+    virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
+    virtio_stw_p(vdev, &blkcfg.cylinders, s->conf->cyls);
+    virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
+    virtio_stw_p(vdev, &blkcfg.min_io_size, s->conf->min_io_size / blk_size);
+    virtio_stw_p(vdev, &blkcfg.opt_io_size, s->conf->opt_io_size / blk_size);
     blkcfg.heads = s->conf->heads;
     /*
      * We must ensure that the block device capacity is a multiple of

[Qemu-devel] [PATCH v8 17/20] 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>
[ pass VirtIODevice * to memory accessors,
  fix missing tswap32 in virtio_scsi_push_event() by Cédric Le Goater,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Cc: Cédric Le Goater <clg@fr.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 d5c37a9..e9afe00 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,6 +316,7 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
     VirtIOSCSIReq *req = r->hba_private;
     VirtIOSCSI *s = req->dev;
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
     uint32_t sense_len;
 
     if (r->io_canceled) {
@@ -324,12 +326,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(vdev, resid);
     } 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(vdev, sense_len);
     }
     virtio_scsi_complete_req(req);
 }
@@ -425,16 +427,16 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
     VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
     VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
 
-    stl_p(&scsiconf->num_queues, s->conf.num_queues);
-    stl_p(&scsiconf->seg_max, 128 - 2);
-    stl_p(&scsiconf->max_sectors, s->conf.max_sectors);
-    stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
-    stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
-    stl_p(&scsiconf->sense_size, s->sense_size);
-    stl_p(&scsiconf->cdb_size, s->cdb_size);
-    stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
-    stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
-    stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
+    virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
+    virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
+    virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors);
+    virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
+    virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
+    virtio_stl_p(vdev, &scsiconf->sense_size, s->sense_size);
+    virtio_stl_p(vdev, &scsiconf->cdb_size, s->cdb_size);
+    virtio_stw_p(vdev, &scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
+    virtio_stw_p(vdev, &scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
+    virtio_stl_p(vdev, &scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
 }
 
 static void virtio_scsi_set_config(VirtIODevice *vdev,
@@ -443,14 +445,14 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
     VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
 
-    if ((uint32_t) ldl_p(&scsiconf->sense_size) >= 65536 ||
-        (uint32_t) ldl_p(&scsiconf->cdb_size) >= 256) {
+    if ((uint32_t) virtio_ldl_p(vdev, &scsiconf->sense_size) >= 65536 ||
+        (uint32_t) virtio_ldl_p(vdev, &scsiconf->cdb_size) >= 256) {
         error_report("bad data written to virtio-scsi configuration space");
         exit(1);
     }
 
-    vs->sense_size = ldl_p(&scsiconf->sense_size);
-    vs->cdb_size = ldl_p(&scsiconf->cdb_size);
+    vs->sense_size = virtio_ldl_p(vdev, &scsiconf->sense_size);
+    vs->cdb_size = virtio_ldl_p(vdev, &scsiconf->cdb_size);
 }
 
 static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
@@ -529,8 +531,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(vdev, event);
+    evt->reason = virtio_tswap32(vdev, reason);
     if (!dev) {
         assert(event == VIRTIO_SCSI_T_EVENTS_MISSED);
     } else {

Re: [Qemu-devel] [PATCH v8 17/20] virtio-scsi: use virtio wrappers to access headers

[Greg Kurz]

On Fri, 13 Jun 2014 13:24:59 +0200
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>
> [ pass VirtIODevice * to memory accessors,
>   fix missing tswap32 in virtio_scsi_push_event() by Cédric Le Goater,
>   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Cc: Cédric Le Goater <clg@fr.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 d5c37a9..e9afe00 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,6 +316,7 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
>      VirtIOSCSIReq *req = r->hba_private;
>      VirtIOSCSI *s = req->dev;
>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
> +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>      uint32_t sense_len;
> 
>      if (r->io_canceled) {
> @@ -324,12 +326,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(vdev, resid);
>      } 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(vdev, sense_len);
>      }
>      virtio_scsi_complete_req(req);
>  }
> @@ -425,16 +427,16 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>      VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
> 
> -    stl_p(&scsiconf->num_queues, s->conf.num_queues);
> -    stl_p(&scsiconf->seg_max, 128 - 2);
> -    stl_p(&scsiconf->max_sectors, s->conf.max_sectors);
> -    stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
> -    stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
> -    stl_p(&scsiconf->sense_size, s->sense_size);
> -    stl_p(&scsiconf->cdb_size, s->cdb_size);
> -    stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
> -    stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
> -    stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
> +    virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
> +    virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
> +    virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors);
> +    virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
> +    virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
> +    virtio_stl_p(vdev, &scsiconf->sense_size, s->sense_size);
> +    virtio_stl_p(vdev, &scsiconf->cdb_size, s->cdb_size);
> +    virtio_stw_p(vdev, &scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
> +    virtio_stw_p(vdev, &scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
> +    virtio_stl_p(vdev, &scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
>  }
> 
>  static void virtio_scsi_set_config(VirtIODevice *vdev,
> @@ -443,14 +445,14 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
>      VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
> 
> -    if ((uint32_t) ldl_p(&scsiconf->sense_size) >= 65536 ||
> -        (uint32_t) ldl_p(&scsiconf->cdb_size) >= 256) {
> +    if ((uint32_t) virtio_ldl_p(vdev, &scsiconf->sense_size) >= 65536 ||
> +        (uint32_t) virtio_ldl_p(vdev, &scsiconf->cdb_size) >= 256) {
>          error_report("bad data written to virtio-scsi configuration space");
>          exit(1);
>      }
> 
> -    vs->sense_size = ldl_p(&scsiconf->sense_size);
> -    vs->cdb_size = ldl_p(&scsiconf->cdb_size);
> +    vs->sense_size = virtio_ldl_p(vdev, &scsiconf->sense_size);
> +    vs->cdb_size = virtio_ldl_p(vdev, &scsiconf->cdb_size);
>  }
> 
>  static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
> @@ -529,8 +531,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(vdev, event);
> +    evt->reason = virtio_tswap32(vdev, reason);

This chunk is a fix for a different issue actually... I'll send it in a separate patch.

>      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 v8 17/20] virtio-scsi: use virtio wrappers to access headers

[Paolo Bonzini]

Il 13/06/2014 13:24, Greg Kurz ha scritto:
> 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>
> [ pass VirtIODevice * to memory accessors,
>   fix missing tswap32 in virtio_scsi_push_event() by Cédric Le Goater,
>   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Cc: Cédric Le Goater <clg@fr.ibm.com>

Be careful that, in the meanwhile, two tswap32s's have been added by 
commit b0b4ea1 (virtio-scsi: add target swap for VirtIOSCSICtrlTMFReq 
fields, 2014-06-10): one in virtio_scsi_do_tmf, one in 
virtio_scsi_handle_ctrl.

Paolo

Re: [Qemu-devel] [PATCH v8 17/20] virtio-scsi: use virtio wrappers to access headers

[Greg Kurz]

On Fri, 20 Jun 2014 10:24:43 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 13/06/2014 13:24, Greg Kurz ha scritto:
> > 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>
> > [ pass VirtIODevice * to memory accessors,
> >   fix missing tswap32 in virtio_scsi_push_event() by Cédric Le Goater,
> >   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Cc: Cédric Le Goater <clg@fr.ibm.com>
> 
> Be careful that, in the meanwhile, two tswap32s's have been added by 
> commit b0b4ea1 (virtio-scsi: add target swap for VirtIOSCSICtrlTMFReq 
> fields, 2014-06-10): one in virtio_scsi_do_tmf, one in 
> virtio_scsi_handle_ctrl.
> 
> Paolo
> 

Yeah I saw these. 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 v8 18/20] virtio-serial-bus: use virtio wrappers to access headers

[Greg Kurz]

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

We also fix max_nr_ports at reset time as the device endianness may have
changed.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
[ pass VirtIODevice * to memory accessors,
  fix max_nr_ports at reset time,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---

Changes since version 7:
 - fixed migration (load part)

 hw/char/virtio-serial-bus.c |   46 +++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 3989722..1b02aaa 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)
 {
@@ -183,11 +184,12 @@ static size_t send_control_msg(VirtIOSerial *vser, void *buf, size_t len)
 static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id,
                                  uint16_t event, uint16_t value)
 {
+    VirtIODevice *vdev = VIRTIO_DEVICE(vser);
     struct virtio_console_control cpkt;
 
-    stl_p(&cpkt.id, port_id);
-    stw_p(&cpkt.event, event);
-    stw_p(&cpkt.value, value);
+    virtio_stl_p(vdev, &cpkt.id, port_id);
+    virtio_stw_p(vdev, &cpkt.event, event);
+    virtio_stw_p(vdev, &cpkt.value, value);
 
     trace_virtio_serial_send_control_event(port_id, event, value);
     return send_control_msg(vser, &cpkt, sizeof(cpkt));
@@ -278,6 +280,7 @@ void virtio_serial_throttle_port(VirtIOSerialPort *port, bool throttle)
 /* Guest wants to notify us of some event */
 static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
 {
+    VirtIODevice *vdev = VIRTIO_DEVICE(vser);
     struct VirtIOSerialPort *port;
     VirtIOSerialPortClass *vsc;
     struct virtio_console_control cpkt, *gcpkt;
@@ -291,8 +294,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(vdev, &gcpkt->event);
+    cpkt.value = virtio_lduw_p(vdev, &gcpkt->value);
 
     trace_virtio_serial_handle_control_message(cpkt.event, cpkt.value);
 
@@ -312,10 +315,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         return;
     }
 
-    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
+    port = find_port_by_id(vser, virtio_ldl_p(vdev, &gcpkt->id));
     if (!port) {
         error_report("virtio-serial-bus: Unexpected port id %u for device %s",
-                     ldl_p(&gcpkt->id), vser->bus.qbus.name);
+                     virtio_ldl_p(vdev, &gcpkt->id), vser->bus.qbus.name);
         return;
     }
 
@@ -342,9 +345,9 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         }
 
         if (port->name) {
-            stl_p(&cpkt.id, port->id);
-            stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
-            stw_p(&cpkt.value, 1);
+            virtio_stl_p(vdev, &cpkt.id, port->id);
+            virtio_stw_p(vdev, &cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
+            virtio_stw_p(vdev, &cpkt.value, 1);
 
             buffer_len = sizeof(cpkt) + strlen(port->name) + 1;
             buffer = g_malloc(buffer_len);
@@ -517,6 +520,10 @@ static void vser_reset(VirtIODevice *vdev)
 
     vser = VIRTIO_SERIAL(vdev);
     guest_reset(vser);
+
+    /* In case we have switched endianness */
+    vser->config.max_nr_ports =
+        virtio_tswap32(vdev, vser->serial.max_virtserial_ports);
 }
 
 static void virtio_serial_save(QEMUFile *f, void *opaque)
@@ -539,7 +546,7 @@ static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f)
     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(vdev, s->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         qemu_put_be32s(f, &s->ports_map[i]);
     }
@@ -695,6 +702,12 @@ static int virtio_serial_load_device(VirtIODevice *vdev, QEMUFile *f,
     qemu_get_be16s(f, (uint16_t *) &tmp);
     qemu_get_be32s(f, &tmp);
 
+    /* Note: this is the only location where we use tswap32() instead of
+     * virtio_tswap32() because:
+     * - virtio_tswap32() only makes sense when the device is fully restored
+     * - the target endianness that was used to populate s->config is
+     *   necessarly the default one
+     */
     max_nr_ports = tswap32(s->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         qemu_get_be32s(f, &ports_map);
@@ -758,9 +771,10 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
 /* This function is only used if a port id is not provided by the user */
 static uint32_t find_free_port_id(VirtIOSerial *vser)
 {
+    VirtIODevice *vdev = VIRTIO_DEVICE(vser);
     unsigned int i, max_nr_ports;
 
-    max_nr_ports = tswap32(vser->config.max_nr_ports);
+    max_nr_ports = virtio_tswap32(vdev, vser->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         uint32_t map, bit;
 
@@ -813,6 +827,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
     VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(dev);
     VirtIOSerialPortClass *vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
     VirtIOSerialBus *bus = VIRTIO_SERIAL_BUS(qdev_get_parent_bus(dev));
+    VirtIODevice *vdev = VIRTIO_DEVICE(bus->vser);
     int max_nr_ports;
     bool plugging_port0;
     Error *err = NULL;
@@ -848,7 +863,7 @@ 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(vdev, port->vser->config.max_nr_ports);
     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);
@@ -870,7 +885,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
     add_port(port->vser, port->id);
 
     /* Send an update to the guest about this new port added */
-    virtio_notify_config(VIRTIO_DEVICE(port->vser));
+    virtio_notify_config(vdev);
 }
 
 static void virtser_port_device_unrealize(DeviceState *dev, Error **errp)
@@ -949,7 +964,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(vdev, vser->serial.max_virtserial_ports);
     vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32)
         * sizeof(vser->ports_map[0]));
     /*

[Qemu-devel] [PATCH v8 19/20] 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 653762a..2572747 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_p(&cfg->tag_len, len);
+    virtio_stw_p(vdev, &cfg->tag_len, len);
     /* 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 v8 20/20] target-ppc: enable virtio endian ambivalent support

[Greg Kurz]

The device endianness is the cpu endianness at device reset time.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 target-ppc/cpu.h            |    2 ++
 target-ppc/translate_init.c |   13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 75ed5fa..b076a14 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -29,6 +29,8 @@
 #define TARGET_LONG_BITS 64
 #define TARGET_PAGE_BITS 12
 
+#define TARGET_IS_BIENDIAN 1
+
 /* Note that the official physical address space bits is 62-M where M
    is implementation dependent.  I've not looked up M for the set of
    cpus we emulate at the system level.  */
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 4d94015..2468c69 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8463,6 +8463,16 @@ static void ppc_cpu_reset(CPUState *s)
     tlb_flush(s, 1);
 }
 
+#ifndef CONFIG_USER_ONLY
+static bool ppc_cpu_is_big_endian(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    return !msr_le;
+}
+#endif
+
 static void ppc_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -8550,6 +8560,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 #else
     cc->gdb_core_xml_file = "power-core.xml";
 #endif
+#ifndef CONFIG_USER_ONLY
+    cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
+#endif
 
     dc->fw_name = "PowerPC,UNKNOWN";
 }

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Alexander Graf]

On 13.06.14 13:18, Greg Kurz wrote:
> Hi,
>
> This version merges the changes requested during the v7 review, remarks from
> ppc64 dump support review (yes, we talked about virtio there) and the work on
> virtio subsections migration. Also two new patches have been added:
> - patch #1 is a preliminary fix for virtio-serial posted by Alexander Graf
> - patch #9 prepares the work on the virtio_is_big_endian() helper
>
> The most significant changes are:
> - introduction of a new CPU method for virtio
> - endianness is taken from CPU that resets the device
> - fastpath virtio memory accessors for fixed endian targets
> - VMState based virtio subsections (compatibility friendly)
>
> You'll find more detailed changelog in each patch.
>
> Please comment and hopefully apply.
>
> Thanks !

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Greg Kurz]

On Fri, 13 Jun 2014 13:56:13 +0200
Alexander Graf <agraf@suse.de> wrote:
> 
> On 13.06.14 13:18, Greg Kurz wrote:
> > Hi,
> >
> > This version merges the changes requested during the v7 review, remarks from
> > ppc64 dump support review (yes, we talked about virtio there) and the work on
> > virtio subsections migration. Also two new patches have been added:
> > - patch #1 is a preliminary fix for virtio-serial posted by Alexander Graf
> > - patch #9 prepares the work on the virtio_is_big_endian() helper
> >
> > The most significant changes are:
> > - introduction of a new CPU method for virtio
> > - endianness is taken from CPU that resets the device
> > - fastpath virtio memory accessors for fixed endian targets
> > - VMState based virtio subsections (compatibility friendly)
> >
> > You'll find more detailed changelog in each patch.
> >
> > Please comment and hopefully apply.
> >
> > Thanks !
> 
> Reviewed-by: Alexander Graf <agraf@suse.de>
> 
> 
> Alex
> 

Hi,

I've reworked the patch set according to Alex's last remarks. Do any of the
other maintainers have pending remarks ? Should I wait or send a v9 now ?

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 v8 00/20] virtio endian-ambivalent target

[Amit Shah]

On (Mon) 16 Jun 2014 [17:07:01], Greg Kurz wrote:
> Hi,
> 
> I've reworked the patch set according to Alex's last remarks. Do any of the
> other maintainers have pending remarks ? Should I wait or send a v9 now ?

I'm yet to go through the series; I'll get to it in a day or two.

		Amit

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 989 bytes --]

On Fri, Jun 13, 2014 at 01:18:00PM +0200, Greg Kurz wrote:
> This version merges the changes requested during the v7 review, remarks from
> ppc64 dump support review (yes, we talked about virtio there) and the work on
> virtio subsections migration. Also two new patches have been added:
> - patch #1 is a preliminary fix for virtio-serial posted by Alexander Graf
> - patch #9 prepares the work on the virtio_is_big_endian() helper
> 
> The most significant changes are:
> - introduction of a new CPU method for virtio
> - endianness is taken from CPU that resets the device
> - fastpath virtio memory accessors for fixed endian targets
> - VMState based virtio subsections (compatibility friendly)

I'm surprised it's not enough for the virtio device to have an
endianness field (big/little).  It seems these patches make endianness
depend on the CPUState through which the device is being accessed.

Can you explain why it's necessary to check the CPUState?

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Alexander Graf]

On 17.06.14 09:36, Stefan Hajnoczi wrote:
> On Fri, Jun 13, 2014 at 01:18:00PM +0200, Greg Kurz wrote:
>> This version merges the changes requested during the v7 review, remarks from
>> ppc64 dump support review (yes, we talked about virtio there) and the work on
>> virtio subsections migration. Also two new patches have been added:
>> - patch #1 is a preliminary fix for virtio-serial posted by Alexander Graf
>> - patch #9 prepares the work on the virtio_is_big_endian() helper
>>
>> The most significant changes are:
>> - introduction of a new CPU method for virtio
>> - endianness is taken from CPU that resets the device
>> - fastpath virtio memory accessors for fixed endian targets
>> - VMState based virtio subsections (compatibility friendly)
> I'm surprised it's not enough for the virtio device to have an
> endianness field (big/little).  It seems these patches make endianness
> depend on the CPUState through which the device is being accessed.
>
> Can you explain why it's necessary to check the CPUState?

They only check CPUState at the point in time of reset, as that's the 
only case where we can derive the implicit endian configuration from :).

After reset, endianness is a simple field in the state struct.


Alex

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

On Tue, Jun 17, 2014 at 09:40:19AM +0200, Alexander Graf wrote:
> 
> On 17.06.14 09:36, Stefan Hajnoczi wrote:
> >On Fri, Jun 13, 2014 at 01:18:00PM +0200, Greg Kurz wrote:
> >>This version merges the changes requested during the v7 review, remarks from
> >>ppc64 dump support review (yes, we talked about virtio there) and the work on
> >>virtio subsections migration. Also two new patches have been added:
> >>- patch #1 is a preliminary fix for virtio-serial posted by Alexander Graf
> >>- patch #9 prepares the work on the virtio_is_big_endian() helper
> >>
> >>The most significant changes are:
> >>- introduction of a new CPU method for virtio
> >>- endianness is taken from CPU that resets the device
> >>- fastpath virtio memory accessors for fixed endian targets
> >>- VMState based virtio subsections (compatibility friendly)
> >I'm surprised it's not enough for the virtio device to have an
> >endianness field (big/little).  It seems these patches make endianness
> >depend on the CPUState through which the device is being accessed.
> >
> >Can you explain why it's necessary to check the CPUState?
> 
> They only check CPUState at the point in time of reset, as that's the only
> case where we can derive the implicit endian configuration from :).

What bothers me is that real hardware can't do this.  Given that VIRTIO
1.0 is always little-endian I guess this is just a temporary hack for
ppc little-endian.  Would be nice to add a comment so it's clear why
this approach is being taken instead of a cleaner solution.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 2854 bytes --]

On Wed, 18 Jun 2014 18:38:14 +0800
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Tue, Jun 17, 2014 at 09:40:19AM +0200, Alexander Graf wrote:
> > 
> > On 17.06.14 09:36, Stefan Hajnoczi wrote:
> > >On Fri, Jun 13, 2014 at 01:18:00PM +0200, Greg Kurz wrote:
> > >>This version merges the changes requested during the v7 review, remarks from
> > >>ppc64 dump support review (yes, we talked about virtio there) and the work on
> > >>virtio subsections migration. Also two new patches have been added:
> > >>- patch #1 is a preliminary fix for virtio-serial posted by Alexander Graf
> > >>- patch #9 prepares the work on the virtio_is_big_endian() helper
> > >>
> > >>The most significant changes are:
> > >>- introduction of a new CPU method for virtio
> > >>- endianness is taken from CPU that resets the device
> > >>- fastpath virtio memory accessors for fixed endian targets
> > >>- VMState based virtio subsections (compatibility friendly)
> > >I'm surprised it's not enough for the virtio device to have an
> > >endianness field (big/little).  It seems these patches make endianness
> > >depend on the CPUState through which the device is being accessed.
> > >
> > >Can you explain why it's necessary to check the CPUState?
> > 
> > They only check CPUState at the point in time of reset, as that's the only
> > case where we can derive the implicit endian configuration from :).
> 
> What bothers me is that real hardware can't do this.  Given that VIRTIO
> 1.0 is always little-endian I guess this is just a temporary hack for
> ppc little-endian.  Would be nice to add a comment so it's clear why
> this approach is being taken instead of a cleaner solution.
> 
> Stefan

Hi Stefan,

Previous versions of this patch set had such comments:

"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."

but these lines got lost between v6 and v7... my bad... :-\

I agree all of this is a hack but:
- it has been on the table for nearly a year
- ppc LE distros are already available
- the memory accessors part makes sense for 1.0

and, speaking of 1.0, I couldn't find any clue about when QEMU would support
this (Cc'ing Rusty for his input), but we (IBM and distro partners) need
ppc LE support now.

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Michael S. Tsirkin]

On Wed, Jun 18, 2014 at 02:35:21PM +0200, Greg Kurz wrote:
> On Wed, 18 Jun 2014 18:38:14 +0800
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> > On Tue, Jun 17, 2014 at 09:40:19AM +0200, Alexander Graf wrote:
> > > 
> > > On 17.06.14 09:36, Stefan Hajnoczi wrote:
> > > >On Fri, Jun 13, 2014 at 01:18:00PM +0200, Greg Kurz wrote:
> > > >>This version merges the changes requested during the v7 review, remarks from
> > > >>ppc64 dump support review (yes, we talked about virtio there) and the work on
> > > >>virtio subsections migration. Also two new patches have been added:
> > > >>- patch #1 is a preliminary fix for virtio-serial posted by Alexander Graf
> > > >>- patch #9 prepares the work on the virtio_is_big_endian() helper
> > > >>
> > > >>The most significant changes are:
> > > >>- introduction of a new CPU method for virtio
> > > >>- endianness is taken from CPU that resets the device
> > > >>- fastpath virtio memory accessors for fixed endian targets
> > > >>- VMState based virtio subsections (compatibility friendly)
> > > >I'm surprised it's not enough for the virtio device to have an
> > > >endianness field (big/little).  It seems these patches make endianness
> > > >depend on the CPUState through which the device is being accessed.
> > > >
> > > >Can you explain why it's necessary to check the CPUState?
> > > 
> > > They only check CPUState at the point in time of reset, as that's the only
> > > case where we can derive the implicit endian configuration from :).
> > 
> > What bothers me is that real hardware can't do this.  Given that VIRTIO
> > 1.0 is always little-endian I guess this is just a temporary hack for
> > ppc little-endian.  Would be nice to add a comment so it's clear why
> > this approach is being taken instead of a cleaner solution.
> > 
> > Stefan
> 
> Hi Stefan,
> 
> Previous versions of this patch set had such comments:
> 
> "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."
> 
> but these lines got lost between v6 and v7... my bad... :-\
> 
> I agree all of this is a hack but:
> - it has been on the table for nearly a year
> - ppc LE distros are already available
> - the memory accessors part makes sense for 1.0
> 
> and, speaking of 1.0, I couldn't find any clue about when QEMU would support
> this (Cc'ing Rusty for his input), but we (IBM and distro partners) need
> ppc LE support now.
> 
> Cheers.

QEMU 2.2 I think.

One disadvantage of this work is that it removes some of the stimulus
for people to work on 1.0, replacing it with hacks. Hmm.

> -- 
> 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 v8 00/20] virtio endian-ambivalent target

[Alexander Graf]

On 18.06.14 17:12, Michael S. Tsirkin wrote:
> On Wed, Jun 18, 2014 at 02:35:21PM +0200, Greg Kurz wrote:
>> On Wed, 18 Jun 2014 18:38:14 +0800
>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>
>>> On Tue, Jun 17, 2014 at 09:40:19AM +0200, Alexander Graf wrote:
>>>> On 17.06.14 09:36, Stefan Hajnoczi wrote:
>>>>> On Fri, Jun 13, 2014 at 01:18:00PM +0200, Greg Kurz wrote:
>>>>>> This version merges the changes requested during the v7 review, remarks from
>>>>>> ppc64 dump support review (yes, we talked about virtio there) and the work on
>>>>>> virtio subsections migration. Also two new patches have been added:
>>>>>> - patch #1 is a preliminary fix for virtio-serial posted by Alexander Graf
>>>>>> - patch #9 prepares the work on the virtio_is_big_endian() helper
>>>>>>
>>>>>> The most significant changes are:
>>>>>> - introduction of a new CPU method for virtio
>>>>>> - endianness is taken from CPU that resets the device
>>>>>> - fastpath virtio memory accessors for fixed endian targets
>>>>>> - VMState based virtio subsections (compatibility friendly)
>>>>> I'm surprised it's not enough for the virtio device to have an
>>>>> endianness field (big/little).  It seems these patches make endianness
>>>>> depend on the CPUState through which the device is being accessed.
>>>>>
>>>>> Can you explain why it's necessary to check the CPUState?
>>>> They only check CPUState at the point in time of reset, as that's the only
>>>> case where we can derive the implicit endian configuration from :).
>>> What bothers me is that real hardware can't do this.  Given that VIRTIO
>>> 1.0 is always little-endian I guess this is just a temporary hack for
>>> ppc little-endian.  Would be nice to add a comment so it's clear why
>>> this approach is being taken instead of a cleaner solution.
>>>
>>> Stefan
>> Hi Stefan,
>>
>> Previous versions of this patch set had such comments:
>>
>> "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."
>>
>> but these lines got lost between v6 and v7... my bad... :-\
>>
>> I agree all of this is a hack but:
>> - it has been on the table for nearly a year
>> - ppc LE distros are already available
>> - the memory accessors part makes sense for 1.0
>>
>> and, speaking of 1.0, I couldn't find any clue about when QEMU would support
>> this (Cc'ing Rusty for his input), but we (IBM and distro partners) need
>> ppc LE support now.
>>
>> Cheers.
> QEMU 2.2 I think.
>
> One disadvantage of this work is that it removes some of the stimulus
> for people to work on 1.0, replacing it with hacks. Hmm.

If you prefer I can also apply these patches and send a pull request for 
them.


Alex

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Michael S. Tsirkin]

On Wed, Jun 18, 2014 at 05:14:17PM +0200, Alexander Graf wrote:
> 
> On 18.06.14 17:12, Michael S. Tsirkin wrote:
> >On Wed, Jun 18, 2014 at 02:35:21PM +0200, Greg Kurz wrote:
> >>On Wed, 18 Jun 2014 18:38:14 +0800
> >>Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >>
> >>>On Tue, Jun 17, 2014 at 09:40:19AM +0200, Alexander Graf wrote:
> >>>>On 17.06.14 09:36, Stefan Hajnoczi wrote:
> >>>>>On Fri, Jun 13, 2014 at 01:18:00PM +0200, Greg Kurz wrote:
> >>>>>>This version merges the changes requested during the v7 review, remarks from
> >>>>>>ppc64 dump support review (yes, we talked about virtio there) and the work on
> >>>>>>virtio subsections migration. Also two new patches have been added:
> >>>>>>- patch #1 is a preliminary fix for virtio-serial posted by Alexander Graf
> >>>>>>- patch #9 prepares the work on the virtio_is_big_endian() helper
> >>>>>>
> >>>>>>The most significant changes are:
> >>>>>>- introduction of a new CPU method for virtio
> >>>>>>- endianness is taken from CPU that resets the device
> >>>>>>- fastpath virtio memory accessors for fixed endian targets
> >>>>>>- VMState based virtio subsections (compatibility friendly)
> >>>>>I'm surprised it's not enough for the virtio device to have an
> >>>>>endianness field (big/little).  It seems these patches make endianness
> >>>>>depend on the CPUState through which the device is being accessed.
> >>>>>
> >>>>>Can you explain why it's necessary to check the CPUState?
> >>>>They only check CPUState at the point in time of reset, as that's the only
> >>>>case where we can derive the implicit endian configuration from :).
> >>>What bothers me is that real hardware can't do this.  Given that VIRTIO
> >>>1.0 is always little-endian I guess this is just a temporary hack for
> >>>ppc little-endian.  Would be nice to add a comment so it's clear why
> >>>this approach is being taken instead of a cleaner solution.
> >>>
> >>>Stefan
> >>Hi Stefan,
> >>
> >>Previous versions of this patch set had such comments:
> >>
> >>"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."
> >>
> >>but these lines got lost between v6 and v7... my bad... :-\
> >>
> >>I agree all of this is a hack but:
> >>- it has been on the table for nearly a year
> >>- ppc LE distros are already available
> >>- the memory accessors part makes sense for 1.0
> >>
> >>and, speaking of 1.0, I couldn't find any clue about when QEMU would support
> >>this (Cc'ing Rusty for his input), but we (IBM and distro partners) need
> >>ppc LE support now.
> >>
> >>Cheers.
> >QEMU 2.2 I think.
> >
> >One disadvantage of this work is that it removes some of the stimulus
> >for people to work on 1.0, replacing it with hacks. Hmm.
> 
> If you prefer I can also apply these patches and send a pull request for
> them.
> 
> 
> Alex

Not yet, please.  v8 still got some comments, so we need to get v9.
Then give people a bit of time to review.  If everyone's happy it should
be mergeable in about a week from now.  Also there are Paolo's patches
already in queue which will conflict, not sure it's a good time for you
to step up and start merging virtio patches.

I'm not nacking, chill :)

I know you want these patches very much, but they aren't yet 100% ready anyway
and I don't see why, meanwhile, I shouldn't mention the downsides for
others to consider.

-- 
MST

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2442 bytes --]

On Wed, Jun 18, 2014 at 02:35:21PM +0200, Greg Kurz wrote:
> On Wed, 18 Jun 2014 18:38:14 +0800
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> > On Tue, Jun 17, 2014 at 09:40:19AM +0200, Alexander Graf wrote:
> > > 
> > > On 17.06.14 09:36, Stefan Hajnoczi wrote:
> > > >On Fri, Jun 13, 2014 at 01:18:00PM +0200, Greg Kurz wrote:
> > > >>This version merges the changes requested during the v7 review, remarks from
> > > >>ppc64 dump support review (yes, we talked about virtio there) and the work on
> > > >>virtio subsections migration. Also two new patches have been added:
> > > >>- patch #1 is a preliminary fix for virtio-serial posted by Alexander Graf
> > > >>- patch #9 prepares the work on the virtio_is_big_endian() helper
> > > >>
> > > >>The most significant changes are:
> > > >>- introduction of a new CPU method for virtio
> > > >>- endianness is taken from CPU that resets the device
> > > >>- fastpath virtio memory accessors for fixed endian targets
> > > >>- VMState based virtio subsections (compatibility friendly)
> > > >I'm surprised it's not enough for the virtio device to have an
> > > >endianness field (big/little).  It seems these patches make endianness
> > > >depend on the CPUState through which the device is being accessed.
> > > >
> > > >Can you explain why it's necessary to check the CPUState?
> > > 
> > > They only check CPUState at the point in time of reset, as that's the only
> > > case where we can derive the implicit endian configuration from :).
> > 
> > What bothers me is that real hardware can't do this.  Given that VIRTIO
> > 1.0 is always little-endian I guess this is just a temporary hack for
> > ppc little-endian.  Would be nice to add a comment so it's clear why
> > this approach is being taken instead of a cleaner solution.
> > 
> > Stefan
> 
> Hi Stefan,
> 
> Previous versions of this patch set had such comments:
> 
> "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."
> 
> but these lines got lost between v6 and v7... my bad... :-\

Thanks!  I just wasn't mentally prepared when looking through the patch
series, the comment puts it into context.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Peter Maydell]

On 18 June 2014 11:38, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> What bothers me is that real hardware can't do this.

Real hardware doesn't have "endianness matches guest CPU endianness"
semantics, which is what the virtio spec mandates...

> Given that VIRTIO
> 1.0 is always little-endian I guess this is just a temporary hack for
> ppc little-endian.  Would be nice to add a comment so it's clear why
> this approach is being taken instead of a cleaner solution.

Also for ARM big-endian, and indeed for any CPU with runtime
configurable endianness that wants to use the kernel virtio
drivers that exist in the real world rather than the theoretical
future ones that might some day be written for the 1.0 virtio
spec...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Michael S. Tsirkin]

On Wed, Jun 18, 2014 at 01:53:15PM +0100, Peter Maydell wrote:
> On 18 June 2014 11:38, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > What bothers me is that real hardware can't do this.
> 
> Real hardware doesn't have "endianness matches guest CPU endianness"
> semantics, which is what the virtio spec mandates...

So it was buggy. We never thought anyone would do a cross endian CPU :(.
We are fixing it in 1.0.

> > Given that VIRTIO
> > 1.0 is always little-endian I guess this is just a temporary hack for
> > ppc little-endian.  Would be nice to add a comment so it's clear why
> > this approach is being taken instead of a cleaner solution.
> 
> Also for ARM big-endian, and indeed for any CPU with runtime
> configurable endianness that wants to use the kernel virtio
> drivers that exist in the real world rather than the theoretical
> future ones that might some day be written for the 1.0 virtio
> spec...
> 
> thanks
> -- PMM

That's not a theoretical future.
Spec will almost certainly be frozen two weeks from now.
So it is almost certain that drivers will be there in 3.17. 

Existing distros can then simply backport the
drivers - same as they would with any other new hardware.

-- 
MST

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Greg Kurz]

On Wed, 18 Jun 2014 16:42:04 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Jun 18, 2014 at 01:53:15PM +0100, Peter Maydell wrote:
> > On 18 June 2014 11:38, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > > What bothers me is that real hardware can't do this.
> > 
> > Real hardware doesn't have "endianness matches guest CPU endianness"
> > semantics, which is what the virtio spec mandates...
> 
> So it was buggy. We never thought anyone would do a cross endian CPU :(.
> We are fixing it in 1.0.
> 

virtio isn't the only victim... we also have vga. The problem can pop up
anywhere you rely on TARGET_WORDS_BIGENDIAN.

> > > Given that VIRTIO
> > > 1.0 is always little-endian I guess this is just a temporary hack for
> > > ppc little-endian.  Would be nice to add a comment so it's clear why
> > > this approach is being taken instead of a cleaner solution.
> > 
> > Also for ARM big-endian, and indeed for any CPU with runtime
> > configurable endianness that wants to use the kernel virtio
> > drivers that exist in the real world rather than the theoretical
> > future ones that might some day be written for the 1.0 virtio
> > spec...
> > 
> > thanks
> > -- PMM
> 
> That's not a theoretical future.
> Spec will almost certainly be frozen two weeks from now.
> So it is almost certain that drivers will be there in 3.17. 
> 

I don't want argue on the dates but I doubt that all legacy users
will switch to 1.0 as soon as it shows up... a transition period
may be needed.

> Existing distros can then simply backport the
> drivers - same as they would with any other new hardware.
> 

Are you saying that upstream QEMU should not to support the
transition between legacy and 1.0 at all ?

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 v8 00/20] virtio endian-ambivalent target

[Michael S. Tsirkin]

On Wed, Jun 18, 2014 at 04:28:04PM +0200, Greg Kurz wrote:
> On Wed, 18 Jun 2014 16:42:04 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Jun 18, 2014 at 01:53:15PM +0100, Peter Maydell wrote:
> > > On 18 June 2014 11:38, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > > > What bothers me is that real hardware can't do this.
> > > 
> > > Real hardware doesn't have "endianness matches guest CPU endianness"
> > > semantics, which is what the virtio spec mandates...
> > 
> > So it was buggy. We never thought anyone would do a cross endian CPU :(.
> > We are fixing it in 1.0.
> > 
> 
> virtio isn't the only victim... we also have vga. The problem can pop up
> anywhere you rely on TARGET_WORDS_BIGENDIAN.
> 
> > > > Given that VIRTIO
> > > > 1.0 is always little-endian I guess this is just a temporary hack for
> > > > ppc little-endian.  Would be nice to add a comment so it's clear why
> > > > this approach is being taken instead of a cleaner solution.
> > > 
> > > Also for ARM big-endian, and indeed for any CPU with runtime
> > > configurable endianness that wants to use the kernel virtio
> > > drivers that exist in the real world rather than the theoretical
> > > future ones that might some day be written for the 1.0 virtio
> > > spec...
> > > 
> > > thanks
> > > -- PMM
> > 
> > That's not a theoretical future.
> > Spec will almost certainly be frozen two weeks from now.
> > So it is almost certain that drivers will be there in 3.17. 
> > 
> 
> I don't want argue on the dates but I doubt that all legacy users
> will switch to 1.0 as soon as it shows up... a transition period
> may be needed.
> 
> > Existing distros can then simply backport the
> > drivers - same as they would with any other new hardware.
> > 
> 
> Are you saying that upstream QEMU should not to support the
> transition between legacy and 1.0 at all ?
> 
> Cheers.

Not at all.
I wouldn't invest the time in implementing this feature myself:
I would just wait a bit and then work on backporting drivers
instead. But you did and the patches are surprisingly clean.
So I don't see a reason to keep them out of tree.


> -- 
> 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 v8 00/20] virtio endian-ambivalent target

[Peter Maydell]

On 18 June 2014 15:28, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Wed, 18 Jun 2014 16:42:04 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> On Wed, Jun 18, 2014 at 01:53:15PM +0100, Peter Maydell wrote:
>> > On 18 June 2014 11:38, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > > What bothers me is that real hardware can't do this.
>> >
>> > Real hardware doesn't have "endianness matches guest CPU endianness"
>> > semantics, which is what the virtio spec mandates...
>>
>> So it was buggy. We never thought anyone would do a cross endian CPU :(.
>> We are fixing it in 1.0.
>>
>
> virtio isn't the only victim... we also have vga. The problem can pop up
> anywhere you rely on TARGET_WORDS_BIGENDIAN.

No, relying on TARGET_WORDS_BIGENDIAN is fine. It's only
a problem if your guest somehow assumes that messing with
the CPU state changes the behaviour of devices as a random
side effect. That's true for virtio. I'm pushing that it should not
be true for VGA (ie that the guest should have to explicitly tell
the VGA device "be the other endian now"). It's also not true for
most average devices whose endianness is
TARGET_WORDS_BIGENDIAN -- it just means they don't
change behaviour, and if the guest wants to be BE on a
fundamentally LE hardware device it gets to do the byte
swapping...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Alexander Graf]

On 18.06.14 17:35, Peter Maydell wrote:
> On 18 June 2014 15:28, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>> On Wed, 18 Jun 2014 16:42:04 +0300
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>> On Wed, Jun 18, 2014 at 01:53:15PM +0100, Peter Maydell wrote:
>>>> On 18 June 2014 11:38, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>>> What bothers me is that real hardware can't do this.
>>>> Real hardware doesn't have "endianness matches guest CPU endianness"
>>>> semantics, which is what the virtio spec mandates...
>>> So it was buggy. We never thought anyone would do a cross endian CPU :(.
>>> We are fixing it in 1.0.
>>>
>> virtio isn't the only victim... we also have vga. The problem can pop up
>> anywhere you rely on TARGET_WORDS_BIGENDIAN.
> No, relying on TARGET_WORDS_BIGENDIAN is fine. It's only
> a problem if your guest somehow assumes that messing with
> the CPU state changes the behaviour of devices as a random
> side effect. That's true for virtio. I'm pushing that it should not
> be true for VGA (ie that the guest should have to explicitly tell
> the VGA device "be the other endian now"). It's also not true for
> most average devices whose endianness is
> TARGET_WORDS_BIGENDIAN -- it just means they don't
> change behaviour, and if the guest wants to be BE on a
> fundamentally LE hardware device it gets to do the byte
> swapping...

I actually agree with Greg here. We implicitly create different VGA 
adapters today depending on TARGET_WORDS_BIGENDIAN. The FB endianness 
should have been separate devices (BE / LE) or runtime configuration 
from the beginning.

Anything that checks TARGET_WORDS_BIGENDIAN in hw/ is very likely to do 
something pretty wrong ;).


Alex

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Peter Maydell]

On 18 June 2014 16:37, Alexander Graf <agraf@suse.de> wrote:
> I actually agree with Greg here. We implicitly create different VGA adapters
> today depending on TARGET_WORDS_BIGENDIAN. The FB endianness should have
> been separate devices (BE / LE) or runtime configuration from the beginning.
>
> Anything that checks TARGET_WORDS_BIGENDIAN in hw/ is very likely to do
> something pretty wrong ;).

Directly, yes, it's a bit of a red flag. But a huge % of devices do
indirectly, by marking their memory regions as DEVICE_NATIVE_ENDIAN.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Alexander Graf]

On 18.06.14 17:40, Peter Maydell wrote:
> On 18 June 2014 16:37, Alexander Graf <agraf@suse.de> wrote:
>> I actually agree with Greg here. We implicitly create different VGA adapters
>> today depending on TARGET_WORDS_BIGENDIAN. The FB endianness should have
>> been separate devices (BE / LE) or runtime configuration from the beginning.
>>
>> Anything that checks TARGET_WORDS_BIGENDIAN in hw/ is very likely to do
>> something pretty wrong ;).
> Directly, yes, it's a bit of a red flag. But a huge % of devices do
> indirectly, by marking their memory regions as DEVICE_NATIVE_ENDIAN.

That's ok and the expected ABI to devices. It's TARGET_WORDS_BIGENDIAN 
that's a good marker that something's wrong.


Alex