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, address@hidden wrote:
> >From: Wei Xu <address@hidden>
> >
> >helper for ring empty check.
> 
> And descriptor read.

OK.

> 
> >
> >Signed-off-by: Wei Xu <address@hidden>
> >---
> >  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, &desc->addr);
> >+    virtio_tswap32s(vdev, &desc->len);
> >+    virtio_tswap16s(vdev, &desc->id);
> >+    virtio_tswap16s(vdev, &desc->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, &cache->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(&desc);
> >+}
> >+
> >+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
>