Re: [Qemu-devel] [PATCH v2] virtio dataplane: adapt dataplane for virtio Version 1

2015-09-01 Thread Stefan Hajnoczi 
On Wed, Aug 26, 2015 at 03:24:42PM +0200, Pierre Morel wrote:
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 07fd69c..ad86be5 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -65,24 +65,34 @@ static void vring_unmap(void *buffer, bool is_write)
>  }
>  
>  /* Map the guest's vring to host memory */
> +
> +#define MAP_VRING_DESCRIPTOR(d) {   \
> +hwaddr addr;\
> +hwaddr size;\
> +void *ptr;  \
> +\
> +addr = virtio_queue_get_##d##_addr(vdev, n);\
> +size = virtio_queue_get_##d## _size(vdev, n);   \
> +ptr = vring_map(>mr_##d, addr, size, true);  \
> +if (!ptr) { \
> +error_report("Failed to map vring "#d" "\
> + "addr %#" HWADDR_PRIx " size %" HWADDR_PRIu,   \
> + addr, size);   \
> +goto out_err_##d;   \
> +}   \
> +vr->d = ptr;\
> +}

Not a fan of the macro, IMO it just obfuscates the code.

Also, normally do ... while (0) is used for statement-like macros to
keep it a single statement instead of a compound statement (where the
trailing semicolon would break if, while, etc).

> +
>  bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
>  {
> -hwaddr vring_addr = virtio_queue_get_ring_addr(vdev, n);
> -hwaddr vring_size = virtio_queue_get_ring_size(vdev, n);
> -void *vring_ptr;
> +struct vring *vr = >vr;
>  
>  vring->broken = false;
> +vr->num = virtio_queue_get_num(vdev, n);
>  
> -vring_ptr = vring_map(>mr, vring_addr, vring_size, true);
> -if (!vring_ptr) {
> -error_report("Failed to map vring "
> - "addr %#" HWADDR_PRIx " size %" HWADDR_PRIu,
> - vring_addr, vring_size);
> -vring->broken = true;
> -return false;
> -}
> -
> -vring_init(>vr, virtio_queue_get_num(vdev, n), vring_ptr, 4096);
> +MAP_VRING_DESCRIPTOR(desc);
> +MAP_VRING_DESCRIPTOR(avail);
> +MAP_VRING_DESCRIPTOR(used);

This doesn't take VIRTIO_RING_F_EVENT_IDX into account, where we access
beyond the end of the rings.  Please check that code path and map the
memory.

Re: [Qemu-devel] [PATCH v2] virtio dataplane: adapt dataplane for virtio Version 1

2015-08-27 Thread Michael S. Tsirkin 
On Wed, Aug 26, 2015 at 03:24:42PM +0200, Pierre Morel wrote:
 Let dataplane allocate different region for the desc/avail/used
 ring regions.
 
 Signed-off-by: Pierre Morel pmo...@linux.vnet.ibm.com
 Acked-by: Greg Kurz gk...@linux.vnet.ibm.com
 Tested-by: Greg Kurz gk...@linux.vnet.ibm.com

Also need to Cc stable on this.

 ---
  hw/virtio/dataplane/vring.c | 48 
 ++---
  include/hw/virtio/dataplane/vring.h |  4 +++-
  2 files changed, 37 insertions(+), 15 deletions(-)
 
 diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
 index 07fd69c..ad86be5 100644
 --- a/hw/virtio/dataplane/vring.c
 +++ b/hw/virtio/dataplane/vring.c
 @@ -65,24 +65,34 @@ static void vring_unmap(void *buffer, bool is_write)
  }
  
  /* Map the guest's vring to host memory */
 +
 +#define MAP_VRING_DESCRIPTOR(d) {   \
 +hwaddr addr;\
 +hwaddr size;\
 +void *ptr;  \
 +\
 +addr = virtio_queue_get_##d##_addr(vdev, n);\
 +size = virtio_queue_get_##d## _size(vdev, n);   \
 +ptr = vring_map(vring-mr_##d, addr, size, true);  \
 +if (!ptr) { \
 +error_report(Failed to map vring #d \
 + addr %# HWADDR_PRIx  size % HWADDR_PRIu,   \
 + addr, size);   \
 +goto out_err_##d;   \
 +}   \
 +vr-d = ptr;\
 +}
 +
  bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
  {
 -hwaddr vring_addr = virtio_queue_get_ring_addr(vdev, n);
 -hwaddr vring_size = virtio_queue_get_ring_size(vdev, n);
 -void *vring_ptr;
 +struct vring *vr = vring-vr;
  
  vring-broken = false;
 +vr-num = virtio_queue_get_num(vdev, n);
  
 -vring_ptr = vring_map(vring-mr, vring_addr, vring_size, true);
 -if (!vring_ptr) {
 -error_report(Failed to map vring 
 - addr %# HWADDR_PRIx  size % HWADDR_PRIu,
 - vring_addr, vring_size);
 -vring-broken = true;
 -return false;
 -}
 -
 -vring_init(vring-vr, virtio_queue_get_num(vdev, n), vring_ptr, 4096);
 +MAP_VRING_DESCRIPTOR(desc);
 +MAP_VRING_DESCRIPTOR(avail);
 +MAP_VRING_DESCRIPTOR(used);
  
  vring-last_avail_idx = virtio_queue_get_last_avail_idx(vdev, n);
  vring-last_used_idx = vring_get_used_idx(vdev, vring);
 @@ -92,6 +102,14 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
  trace_vring_setup(virtio_queue_get_ring_addr(vdev, n),
vring-vr.desc, vring-vr.avail, vring-vr.used);
  return true;
 +
 +out_err_used:
 +memory_region_unref(vring-mr_avail);
 +out_err_avail:
 +memory_region_unref(vring-mr_desc);
 +out_err_desc:
 +vring-broken = true;
 +return false;
  }
  
  void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
 @@ -99,7 +117,9 @@ void vring_teardown(Vring *vring, VirtIODevice *vdev, int 
 n)
  virtio_queue_set_last_avail_idx(vdev, n, vring-last_avail_idx);
  virtio_queue_invalidate_signalled_used(vdev, n);
  
 -memory_region_unref(vring-mr);
 +memory_region_unref(vring-mr_desc);
 +memory_region_unref(vring-mr_avail);
 +memory_region_unref(vring-mr_used);
  }
  
  /* Disable guest-host notifies */
 diff --git a/include/hw/virtio/dataplane/vring.h 
 b/include/hw/virtio/dataplane/vring.h
 index 8d97db9..a596e4c 100644
 --- a/include/hw/virtio/dataplane/vring.h
 +++ b/include/hw/virtio/dataplane/vring.h
 @@ -22,7 +22,9 @@
  #include hw/virtio/virtio.h
  
  typedef struct {
 -MemoryRegion *mr;   /* memory region containing the vring */
 +MemoryRegion *mr_desc;  /* memory region for the vring desc */
 +MemoryRegion *mr_avail; /* memory region for the vring avail */
 +MemoryRegion *mr_used;  /* memory region for the vring used */
  struct vring vr;/* virtqueue vring mapped to host memory 
 */
  uint16_t last_avail_idx;/* last processed avail ring index */
  uint16_t last_used_idx; /* last processed used ring index */
 -- 
 2.3.0

Re: [Qemu-devel] [PATCH v2] virtio dataplane: adapt dataplane for virtio Version 1

2015-08-26 Thread Cornelia Huck 
On Wed, 26 Aug 2015 15:24:42 +0200
Pierre Morel pmo...@linux.vnet.ibm.com wrote:

 Let dataplane allocate different region for the desc/avail/used

s/region/regions/

 ring regions.
 
 Signed-off-by: Pierre Morel pmo...@linux.vnet.ibm.com
 Acked-by: Greg Kurz gk...@linux.vnet.ibm.com
 Tested-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  hw/virtio/dataplane/vring.c | 48 
 ++---
  include/hw/virtio/dataplane/vring.h |  4 +++-
  2 files changed, 37 insertions(+), 15 deletions(-)
 
 diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
 index 07fd69c..ad86be5 100644
 --- a/hw/virtio/dataplane/vring.c
 +++ b/hw/virtio/dataplane/vring.c
 @@ -65,24 +65,34 @@ static void vring_unmap(void *buffer, bool is_write)
  }
 
  /* Map the guest's vring to host memory */
 +
 +#define MAP_VRING_DESCRIPTOR(d) {   \
 +hwaddr addr;\
 +hwaddr size;\
 +void *ptr;  \
 +\
 +addr = virtio_queue_get_##d##_addr(vdev, n);\
 +size = virtio_queue_get_##d## _size(vdev, n);   \

Inconsistent spacing (but probably can be fixed when applying).

 +ptr = vring_map(vring-mr_##d, addr, size, true);  \
 +if (!ptr) { \
 +error_report(Failed to map vring #d \
 + addr %# HWADDR_PRIx  size % HWADDR_PRIu,   \
 + addr, size);   \
 +goto out_err_##d;   \
 +}   \
 +vr-d = ptr;\
 +}
 +
  bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
  {
 -hwaddr vring_addr = virtio_queue_get_ring_addr(vdev, n);
 -hwaddr vring_size = virtio_queue_get_ring_size(vdev, n);
 -void *vring_ptr;
 +struct vring *vr = vring-vr;
 
  vring-broken = false;
 +vr-num = virtio_queue_get_num(vdev, n);
 
 -vring_ptr = vring_map(vring-mr, vring_addr, vring_size, true);
 -if (!vring_ptr) {
 -error_report(Failed to map vring 
 - addr %# HWADDR_PRIx  size % HWADDR_PRIu,
 - vring_addr, vring_size);
 -vring-broken = true;
 -return false;
 -}
 -
 -vring_init(vring-vr, virtio_queue_get_num(vdev, n), vring_ptr, 4096);
 +MAP_VRING_DESCRIPTOR(desc);
 +MAP_VRING_DESCRIPTOR(avail);
 +MAP_VRING_DESCRIPTOR(used);
 
  vring-last_avail_idx = virtio_queue_get_last_avail_idx(vdev, n);
  vring-last_used_idx = vring_get_used_idx(vdev, vring);
 @@ -92,6 +102,14 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
  trace_vring_setup(virtio_queue_get_ring_addr(vdev, n),
vring-vr.desc, vring-vr.avail, vring-vr.used);
  return true;
 +
 +out_err_used:
 +memory_region_unref(vring-mr_avail);
 +out_err_avail:
 +memory_region_unref(vring-mr_desc);
 +out_err_desc:
 +vring-broken = true;
 +return false;

My only worry is that this might be a bit hard to parse as the gotos
are hidden in the macro. But in the end, both this and the original
version are basically fine.

  }
 
  void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

[Qemu-devel] [PATCH v2] virtio dataplane: adapt dataplane for virtio Version 1

2015-08-26 Thread Pierre Morel 
Let dataplane allocate different region for the desc/avail/used
ring regions.

Signed-off-by: Pierre Morel pmo...@linux.vnet.ibm.com
Acked-by: Greg Kurz gk...@linux.vnet.ibm.com
Tested-by: Greg Kurz gk...@linux.vnet.ibm.com
---
 hw/virtio/dataplane/vring.c | 48 ++---
 include/hw/virtio/dataplane/vring.h |  4 +++-
 2 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index 07fd69c..ad86be5 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -65,24 +65,34 @@ static void vring_unmap(void *buffer, bool is_write)
 }
 
 /* Map the guest's vring to host memory */
+
+#define MAP_VRING_DESCRIPTOR(d) {   \
+hwaddr addr;\
+hwaddr size;\
+void *ptr;  \
+\
+addr = virtio_queue_get_##d##_addr(vdev, n);\
+size = virtio_queue_get_##d## _size(vdev, n);   \
+ptr = vring_map(vring-mr_##d, addr, size, true);  \
+if (!ptr) { \
+error_report(Failed to map vring #d \
+ addr %# HWADDR_PRIx  size % HWADDR_PRIu,   \
+ addr, size);   \
+goto out_err_##d;   \
+}   \
+vr-d = ptr;\
+}
+
 bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
 {
-hwaddr vring_addr = virtio_queue_get_ring_addr(vdev, n);
-hwaddr vring_size = virtio_queue_get_ring_size(vdev, n);
-void *vring_ptr;
+struct vring *vr = vring-vr;
 
 vring-broken = false;
+vr-num = virtio_queue_get_num(vdev, n);
 
-vring_ptr = vring_map(vring-mr, vring_addr, vring_size, true);
-if (!vring_ptr) {
-error_report(Failed to map vring 
- addr %# HWADDR_PRIx  size % HWADDR_PRIu,
- vring_addr, vring_size);
-vring-broken = true;
-return false;
-}
-
-vring_init(vring-vr, virtio_queue_get_num(vdev, n), vring_ptr, 4096);
+MAP_VRING_DESCRIPTOR(desc);
+MAP_VRING_DESCRIPTOR(avail);
+MAP_VRING_DESCRIPTOR(used);
 
 vring-last_avail_idx = virtio_queue_get_last_avail_idx(vdev, n);
 vring-last_used_idx = vring_get_used_idx(vdev, vring);
@@ -92,6 +102,14 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
 trace_vring_setup(virtio_queue_get_ring_addr(vdev, n),
   vring-vr.desc, vring-vr.avail, vring-vr.used);
 return true;
+
+out_err_used:
+memory_region_unref(vring-mr_avail);
+out_err_avail:
+memory_region_unref(vring-mr_desc);
+out_err_desc:
+vring-broken = true;
+return false;
 }
 
 void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
@@ -99,7 +117,9 @@ void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
 virtio_queue_set_last_avail_idx(vdev, n, vring-last_avail_idx);
 virtio_queue_invalidate_signalled_used(vdev, n);
 
-memory_region_unref(vring-mr);
+memory_region_unref(vring-mr_desc);
+memory_region_unref(vring-mr_avail);
+memory_region_unref(vring-mr_used);
 }
 
 /* Disable guest-host notifies */
diff --git a/include/hw/virtio/dataplane/vring.h 
b/include/hw/virtio/dataplane/vring.h
index 8d97db9..a596e4c 100644
--- a/include/hw/virtio/dataplane/vring.h
+++ b/include/hw/virtio/dataplane/vring.h
@@ -22,7 +22,9 @@
 #include hw/virtio/virtio.h
 
 typedef struct {
-MemoryRegion *mr;   /* memory region containing the vring */
+MemoryRegion *mr_desc;  /* memory region for the vring desc */
+MemoryRegion *mr_avail; /* memory region for the vring avail */
+MemoryRegion *mr_used;  /* memory region for the vring used */
 struct vring vr;/* virtqueue vring mapped to host memory */
 uint16_t last_avail_idx;/* last processed avail ring index */
 uint16_t last_used_idx; /* last processed used ring index */
-- 
2.3.0

Re: [Qemu-devel] [PATCH v2] virtio dataplane: adapt dataplane for virtio Version 1

2015-08-26 Thread Greg Kurz 
On Wed, 26 Aug 2015 15:24:42 +0200
Pierre Morel pmo...@linux.vnet.ibm.com wrote:
 Let dataplane allocate different region for the desc/avail/used
 ring regions.
 
 Signed-off-by: Pierre Morel pmo...@linux.vnet.ibm.com
 Acked-by: Greg Kurz gk...@linux.vnet.ibm.com

Stefan,

In case SoB is more appropriate, here it is:

Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com

Cheers.

--
Greg

 Tested-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  hw/virtio/dataplane/vring.c | 48 
 ++---
  include/hw/virtio/dataplane/vring.h |  4 +++-
  2 files changed, 37 insertions(+), 15 deletions(-)
 
 diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
 index 07fd69c..ad86be5 100644
 --- a/hw/virtio/dataplane/vring.c
 +++ b/hw/virtio/dataplane/vring.c
 @@ -65,24 +65,34 @@ static void vring_unmap(void *buffer, bool is_write)
  }
 
  /* Map the guest's vring to host memory */
 +
 +#define MAP_VRING_DESCRIPTOR(d) {   \
 +hwaddr addr;\
 +hwaddr size;\
 +void *ptr;  \
 +\
 +addr = virtio_queue_get_##d##_addr(vdev, n);\
 +size = virtio_queue_get_##d## _size(vdev, n);   \
 +ptr = vring_map(vring-mr_##d, addr, size, true);  \
 +if (!ptr) { \
 +error_report(Failed to map vring #d \
 + addr %# HWADDR_PRIx  size % HWADDR_PRIu,   \
 + addr, size);   \
 +goto out_err_##d;   \
 +}   \
 +vr-d = ptr;\
 +}
 +
  bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
  {
 -hwaddr vring_addr = virtio_queue_get_ring_addr(vdev, n);
 -hwaddr vring_size = virtio_queue_get_ring_size(vdev, n);
 -void *vring_ptr;
 +struct vring *vr = vring-vr;
 
  vring-broken = false;
 +vr-num = virtio_queue_get_num(vdev, n);
 
 -vring_ptr = vring_map(vring-mr, vring_addr, vring_size, true);
 -if (!vring_ptr) {
 -error_report(Failed to map vring 
 - addr %# HWADDR_PRIx  size % HWADDR_PRIu,
 - vring_addr, vring_size);
 -vring-broken = true;
 -return false;
 -}
 -
 -vring_init(vring-vr, virtio_queue_get_num(vdev, n), vring_ptr, 4096);
 +MAP_VRING_DESCRIPTOR(desc);
 +MAP_VRING_DESCRIPTOR(avail);
 +MAP_VRING_DESCRIPTOR(used);
 
  vring-last_avail_idx = virtio_queue_get_last_avail_idx(vdev, n);
  vring-last_used_idx = vring_get_used_idx(vdev, vring);
 @@ -92,6 +102,14 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
  trace_vring_setup(virtio_queue_get_ring_addr(vdev, n),
vring-vr.desc, vring-vr.avail, vring-vr.used);
  return true;
 +
 +out_err_used:
 +memory_region_unref(vring-mr_avail);
 +out_err_avail:
 +memory_region_unref(vring-mr_desc);
 +out_err_desc:
 +vring-broken = true;
 +return false;
  }
 
  void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
 @@ -99,7 +117,9 @@ void vring_teardown(Vring *vring, VirtIODevice *vdev, int 
 n)
  virtio_queue_set_last_avail_idx(vdev, n, vring-last_avail_idx);
  virtio_queue_invalidate_signalled_used(vdev, n);
 
 -memory_region_unref(vring-mr);
 +memory_region_unref(vring-mr_desc);
 +memory_region_unref(vring-mr_avail);
 +memory_region_unref(vring-mr_used);
  }
 
  /* Disable guest-host notifies */
 diff --git a/include/hw/virtio/dataplane/vring.h 
 b/include/hw/virtio/dataplane/vring.h
 index 8d97db9..a596e4c 100644
 --- a/include/hw/virtio/dataplane/vring.h
 +++ b/include/hw/virtio/dataplane/vring.h
 @@ -22,7 +22,9 @@
  #include hw/virtio/virtio.h
 
  typedef struct {
 -MemoryRegion *mr;   /* memory region containing the vring */
 +MemoryRegion *mr_desc;  /* memory region for the vring desc */
 +MemoryRegion *mr_avail; /* memory region for the vring avail */
 +MemoryRegion *mr_used;  /* memory region for the vring used */
  struct vring vr;/* virtqueue vring mapped to host memory 
 */
  uint16_t last_avail_idx;/* last processed avail ring index */
  uint16_t last_used_idx; /* last processed used ring index */