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

[Jason Wang <jasowang@redhat.com>]

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