Re: [Qemu-devel] [PATCH 3/8] virtio: add empty check for packed ring

[Jason Wang]

On 2018年06月04日 01:44, Wei Xu wrote:

+static int virtio_queue_empty_packed_rcu(VirtQueue *vq)
+{
+struct VRingDescPacked desc;
+VRingMemoryRegionCaches *cache;
+
+if (unlikely(!vq->packed.desc)) {
+return 1;
+}
+
+cache = vring_get_region_caches(vq);
+vring_desc_read_packed(vq->vdev, , >desc_packed, 
vq->last_avail_idx);
+
+/* Make sure we see the updated flag */
+smp_mb();

What we need here is to make sure flag is read before all other fields,
looks like this barrier can't.

Isn't flag updated yet if it has been read?



Consider the case of event index, you need make sure flag is read before 
off_wrap.


Thanks

Re: [Qemu-devel] [PATCH 3/8] virtio: add empty check for packed ring

[Wei Xu]

On Tue, Apr 10, 2018 at 03:23:03PM +0800, Jason Wang wrote:
> 
> 
> On 2018年04月04日 20:53, w...@redhat.com wrote:
> >From: Wei Xu 
> >
> >helper for ring empty check.
> 
> And descriptor read.

OK.

> 
> >
> >Signed-off-by: Wei Xu 
> >---
> >  hw/virtio/virtio.c | 62 
> > +++---
> >  1 file changed, 59 insertions(+), 3 deletions(-)
> >
> >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >index 73a35a4..478df3d 100644
> >--- a/hw/virtio/virtio.c
> >+++ b/hw/virtio/virtio.c
> >@@ -24,6 +24,9 @@
> >  #include "hw/virtio/virtio-access.h"
> >  #include "sysemu/dma.h"
> >+#define AVAIL_DESC_PACKED(b) ((b) << 7)
> >+#define USED_DESC_PACKED(b)  ((b) << 15)
> 
> Can we pass value other than 1 to this macro?

Yes, '0' is also provided for some clear/reset cases.

> 
> >+
> >  /*
> >   * The alignment to use between consumer and producer parts of vring.
> >   * x86 pagesize again. This is the default, used by transports like PCI
> >@@ -446,10 +449,27 @@ int virtio_queue_ready(VirtQueue *vq)
> >  return vq->vring.avail != 0;
> >  }
> >+static void vring_desc_read_packed(VirtIODevice *vdev, VRingDescPacked 
> >*desc,
> >+MemoryRegionCache *cache, int i)
> >+{
> >+address_space_read_cached(cache, i * sizeof(VRingDescPacked),
> >+  desc, sizeof(VRingDescPacked));
> >+virtio_tswap64s(vdev, >addr);
> >+virtio_tswap32s(vdev, >len);
> >+virtio_tswap16s(vdev, >id);
> >+virtio_tswap16s(vdev, >flags);
> >+}
> >+
> >+static inline bool is_desc_avail(struct VRingDescPacked* desc)
> >+{
> >+return (!!(desc->flags & AVAIL_DESC_PACKED(1)) !=
> >+!!(desc->flags & USED_DESC_PACKED(1)));
> 
> Don't we need to care about endian here?

Usually we don't since endian has been converted during reading,
will double check it.

> 
> >+}
> >+
> >  /* Fetch avail_idx from VQ memory only when we really need to know if
> >   * guest has added some buffers.
> >   * Called within rcu_read_lock().  */
> >-static int virtio_queue_empty_rcu(VirtQueue *vq)
> >+static int virtio_queue_empty_split_rcu(VirtQueue *vq)
> >  {
> >  if (unlikely(!vq->vring.avail)) {
> >  return 1;
> >@@ -462,7 +482,7 @@ static int virtio_queue_empty_rcu(VirtQueue *vq)
> >  return vring_avail_idx(vq) == vq->last_avail_idx;
> >  }
> >-int virtio_queue_empty(VirtQueue *vq)
> >+static int virtio_queue_empty_split(VirtQueue *vq)
> >  {
> >  bool empty;
> >@@ -480,6 +500,42 @@ int virtio_queue_empty(VirtQueue *vq)
> >  return empty;
> >  }
> >+static int virtio_queue_empty_packed_rcu(VirtQueue *vq)
> >+{
> >+struct VRingDescPacked desc;
> >+VRingMemoryRegionCaches *cache;
> >+
> >+if (unlikely(!vq->packed.desc)) {
> >+return 1;
> >+}
> >+
> >+cache = vring_get_region_caches(vq);
> >+vring_desc_read_packed(vq->vdev, , >desc_packed, 
> >vq->last_avail_idx);
> >+
> >+/* Make sure we see the updated flag */
> >+smp_mb();
> 
> What we need here is to make sure flag is read before all other fields,
> looks like this barrier can't.

Isn't flag updated yet if it has been read?

> 
> >+return !is_desc_avail();
> >+}
> >+
> >+static int virtio_queue_empty_packed(VirtQueue *vq)
> >+{
> >+bool empty;
> >+
> >+rcu_read_lock();
> >+empty = virtio_queue_empty_packed_rcu(vq);
> >+rcu_read_unlock();
> >+return empty;
> >+}
> >+
> >+int virtio_queue_empty(VirtQueue *vq)
> >+{
> >+if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> >+return virtio_queue_empty_packed(vq);
> >+} else {
> >+return virtio_queue_empty_split(vq);
> >+}
> >+}
> >+
> >  static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
> > unsigned int len)
> >  {
> >@@ -951,7 +1007,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
> >  return NULL;
> >  }
> >  rcu_read_lock();
> >-if (virtio_queue_empty_rcu(vq)) {
> >+if (virtio_queue_empty_split_rcu(vq)) {
> 
> I think you'd better have a switch inside virtio_queue_empty_rcu() like
> virtio_queue_empty() here.

OK.

> 
> Thanks
> 
> >  goto done;
> >  }
> >  /* Needed after virtio_queue_empty(), see comment in
> 

Re: [Qemu-devel] [PATCH 3/8] virtio: add empty check for packed ring

[Jason Wang]

On 2018年04月04日 20:53, w...@redhat.com wrote:

From: Wei Xu 

helper for ring empty check.


And descriptor read.



Signed-off-by: Wei Xu 
---
  hw/virtio/virtio.c | 62 +++---
  1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 73a35a4..478df3d 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -24,6 +24,9 @@
  #include "hw/virtio/virtio-access.h"
  #include "sysemu/dma.h"
  
+#define AVAIL_DESC_PACKED(b) ((b) << 7)

+#define USED_DESC_PACKED(b)  ((b) << 15)


Can we pass value other than 1 to this macro?


+
  /*
   * The alignment to use between consumer and producer parts of vring.
   * x86 pagesize again. This is the default, used by transports like PCI
@@ -446,10 +449,27 @@ int virtio_queue_ready(VirtQueue *vq)
  return vq->vring.avail != 0;
  }
  
+static void vring_desc_read_packed(VirtIODevice *vdev, VRingDescPacked *desc,

+MemoryRegionCache *cache, int i)
+{
+address_space_read_cached(cache, i * sizeof(VRingDescPacked),
+  desc, sizeof(VRingDescPacked));
+virtio_tswap64s(vdev, >addr);
+virtio_tswap32s(vdev, >len);
+virtio_tswap16s(vdev, >id);
+virtio_tswap16s(vdev, >flags);
+}
+
+static inline bool is_desc_avail(struct VRingDescPacked* desc)
+{
+return (!!(desc->flags & AVAIL_DESC_PACKED(1)) !=
+!!(desc->flags & USED_DESC_PACKED(1)));


Don't we need to care about endian here?


+}
+
  /* Fetch avail_idx from VQ memory only when we really need to know if
   * guest has added some buffers.
   * Called within rcu_read_lock().  */
-static int virtio_queue_empty_rcu(VirtQueue *vq)
+static int virtio_queue_empty_split_rcu(VirtQueue *vq)
  {
  if (unlikely(!vq->vring.avail)) {
  return 1;
@@ -462,7 +482,7 @@ static int virtio_queue_empty_rcu(VirtQueue *vq)
  return vring_avail_idx(vq) == vq->last_avail_idx;
  }
  
-int virtio_queue_empty(VirtQueue *vq)

+static int virtio_queue_empty_split(VirtQueue *vq)
  {
  bool empty;
  
@@ -480,6 +500,42 @@ int virtio_queue_empty(VirtQueue *vq)

  return empty;
  }
  
+static int virtio_queue_empty_packed_rcu(VirtQueue *vq)

+{
+struct VRingDescPacked desc;
+VRingMemoryRegionCaches *cache;
+
+if (unlikely(!vq->packed.desc)) {
+return 1;
+}
+
+cache = vring_get_region_caches(vq);
+vring_desc_read_packed(vq->vdev, , >desc_packed, 
vq->last_avail_idx);
+
+/* Make sure we see the updated flag */
+smp_mb();


What we need here is to make sure flag is read before all other fields, 
looks like this barrier can't.



+return !is_desc_avail();
+}
+
+static int virtio_queue_empty_packed(VirtQueue *vq)
+{
+bool empty;
+
+rcu_read_lock();
+empty = virtio_queue_empty_packed_rcu(vq);
+rcu_read_unlock();
+return empty;
+}
+
+int virtio_queue_empty(VirtQueue *vq)
+{
+if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
+return virtio_queue_empty_packed(vq);
+} else {
+return virtio_queue_empty_split(vq);
+}
+}
+
  static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
 unsigned int len)
  {
@@ -951,7 +1007,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
  return NULL;
  }
  rcu_read_lock();
-if (virtio_queue_empty_rcu(vq)) {
+if (virtio_queue_empty_split_rcu(vq)) {


I think you'd better have a switch inside virtio_queue_empty_rcu() like 
virtio_queue_empty() here.


Thanks


  goto done;
  }
  /* Needed after virtio_queue_empty(), see comment in

[Qemu-devel] [PATCH 3/8] virtio: add empty check for packed ring

[wexu]

From: Wei Xu 

helper for ring empty check.

Signed-off-by: Wei Xu 
---
 hw/virtio/virtio.c | 62 +++---
 1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 73a35a4..478df3d 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -24,6 +24,9 @@
 #include "hw/virtio/virtio-access.h"
 #include "sysemu/dma.h"
 
+#define AVAIL_DESC_PACKED(b) ((b) << 7)
+#define USED_DESC_PACKED(b)  ((b) << 15)
+
 /*
  * The alignment to use between consumer and producer parts of vring.
  * x86 pagesize again. This is the default, used by transports like PCI
@@ -446,10 +449,27 @@ int virtio_queue_ready(VirtQueue *vq)
 return vq->vring.avail != 0;
 }
 
+static void vring_desc_read_packed(VirtIODevice *vdev, VRingDescPacked *desc,
+MemoryRegionCache *cache, int i)
+{
+address_space_read_cached(cache, i * sizeof(VRingDescPacked),
+  desc, sizeof(VRingDescPacked));
+virtio_tswap64s(vdev, >addr);
+virtio_tswap32s(vdev, >len);
+virtio_tswap16s(vdev, >id);
+virtio_tswap16s(vdev, >flags);
+}
+
+static inline bool is_desc_avail(struct VRingDescPacked* desc)
+{
+return (!!(desc->flags & AVAIL_DESC_PACKED(1)) !=
+!!(desc->flags & USED_DESC_PACKED(1)));
+}
+
 /* Fetch avail_idx from VQ memory only when we really need to know if
  * guest has added some buffers.
  * Called within rcu_read_lock().  */
-static int virtio_queue_empty_rcu(VirtQueue *vq)
+static int virtio_queue_empty_split_rcu(VirtQueue *vq)
 {
 if (unlikely(!vq->vring.avail)) {
 return 1;
@@ -462,7 +482,7 @@ static int virtio_queue_empty_rcu(VirtQueue *vq)
 return vring_avail_idx(vq) == vq->last_avail_idx;
 }
 
-int virtio_queue_empty(VirtQueue *vq)
+static int virtio_queue_empty_split(VirtQueue *vq)
 {
 bool empty;
 
@@ -480,6 +500,42 @@ int virtio_queue_empty(VirtQueue *vq)
 return empty;
 }
 
+static int virtio_queue_empty_packed_rcu(VirtQueue *vq)
+{
+struct VRingDescPacked desc;
+VRingMemoryRegionCaches *cache;
+
+if (unlikely(!vq->packed.desc)) {
+return 1;
+}
+
+cache = vring_get_region_caches(vq);
+vring_desc_read_packed(vq->vdev, , >desc_packed, 
vq->last_avail_idx);
+
+/* Make sure we see the updated flag */
+smp_mb();
+return !is_desc_avail();
+}
+
+static int virtio_queue_empty_packed(VirtQueue *vq)
+{
+bool empty;
+
+rcu_read_lock();
+empty = virtio_queue_empty_packed_rcu(vq);
+rcu_read_unlock();
+return empty;
+}
+
+int virtio_queue_empty(VirtQueue *vq)
+{
+if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
+return virtio_queue_empty_packed(vq);
+} else {
+return virtio_queue_empty_split(vq);
+}
+}
+
 static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
unsigned int len)
 {
@@ -951,7 +1007,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 return NULL;
 }
 rcu_read_lock();
-if (virtio_queue_empty_rcu(vq)) {
+if (virtio_queue_empty_split_rcu(vq)) {
 goto done;
 }
 /* Needed after virtio_queue_empty(), see comment in
-- 
2.7.4