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(-)
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, &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)));
+}
+
/* 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();
+ 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)) {
goto done;
}
/* Needed after virtio_queue_empty(), see comment in
--
2.7.4
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
© 2016 - 2026 Red Hat, Inc.