[3/8] virtio: add empty check for packed ring

From: Wei Xu <wexu@redhat.com>

helper for ring empty check.

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

On 2018年04月04日 20:53, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> helper for ring empty check.

And descriptor read.

>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> ---
>   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, &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?

> +}
> +
>   /* 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.

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

Thanks

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

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

OK.

> 
> >
> >Signed-off-by: Wei Xu <wexu@redhat.com>
> >---
> >  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
>

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, &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?
>

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

Thanks