Re: [Qemu-devel] [RFC v2 3/8] virtio: empty check and desc read for packed ring

[Jason Wang]

On 2018年06月06日 03:07, address@hidden wrote:
> From: Wei Xu <address@hidden>
>
> helper for ring empty check and descriptor read.
>
> 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 f6c0689..bd669a2 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
> @@ -357,10 +360,27 @@ int virtio_queue_ready(VirtQueue *vq)
>       return vq->vring.avail != 0;
>   }
>   
> +static void vring_packed_desc_read(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));
> +}
See spec and the discussion in the past for vhost patches:

"""
Thus VIRTQ_DESC_F_AVAIL and VIRTQ_DESC_F_USED bits are different for an 
available descriptor and
equal for a used descriptor.
Note that this observation is mostly useful for sanity-checking as these 
are necessary but not sufficient
conditions - for example, all descriptors are zero-initialized. To 
detect used and available descriptors it is
possible for drivers and devices to keep track of the last observed 
value of VIRTQ_DESC_F_USED/VIRTQ_-
DESC_F_AVAIL. Other techniques to detect 
VIRTQ_DESC_F_AVAIL/VIRTQ_DESC_F_USED bit changes
might also be possible.
"""

This may not work especially consider we may do migration or savevm/loadvm.

> +
>   /* 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_split_empty_rcu(VirtQueue *vq)
>   {
>       if (unlikely(!vq->vring.avail)) {
>           return 1;
> @@ -373,7 +393,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_split_empty(VirtQueue *vq)
>   {
>       bool empty;
>   
> @@ -391,6 +411,42 @@ int virtio_queue_empty(VirtQueue *vq)
>       return empty;
>   }
>   
> +static int virtio_queue_packed_empty_rcu(VirtQueue *vq)
> +{
> +    struct VRingDescPacked desc;
> +    VRingMemoryRegionCaches *cache;
> +
> +    if (unlikely(!vq->vring.desc)) {
> +        return 1;
> +    }
> +
> +    cache = vring_get_region_caches(vq);
> +    vring_packed_desc_read(vq->vdev, &desc, &cache->desc, vq->last_avail_idx);
> +
I believe only we only care flag here.

> +    /* Make sure we see the updated flag */
> +    smp_mb();
Why need this?

Note there's a smp_rmb() after virtio_queue_empty_rcu() in virtquue_pop().

> +    return !is_desc_avail(&desc);
> +}
> +
> +static int virtio_queue_packed_empty(VirtQueue *vq)
> +{
> +    bool empty;
> +
> +    rcu_read_lock();
> +    empty = virtio_queue_packed_empty_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_packed_empty(vq);
> +    } else {
> +        return virtio_queue_split_empty(vq);
> +    }
> +}
> +
>   static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
>                                  unsigned int len)
>   {
> @@ -862,7 +918,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>           return NULL;
>       }
>       rcu_read_lock();
> -    if (virtio_queue_empty_rcu(vq)) {
> +    if (virtio_queue_split_empty_rcu(vq)) {
This is meaningless unless you rename virtqueue_pop to 
virtqueue_pop_split and call it only for split ring.
Thanks

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