[Qemu-devel] [RFC v2 0/8] packed ring virtio-net userspace backend support

[Qemu-devel] [RFC v2 0/8] packed ring virtio-net userspace backend support

[wexu]

From: Wei Xu <wexu@redhat.com>

Todo:
- address Rx slow performance
- event index interrupt suppression test

v1->v2
- sync to tiwei's v5
- reuse memory cache function with 1.0
- dropped detach patch and notification helper(04 & 05 in v1)
- guest virtio-net driver unload/reload support
- event suppression support(not tested)
- addressed feedback from v1

About guest virtio-net load/unload:
Since last_avail, avail_wrap_count, used_idx and used_wrap_count are
all 16 or bool type variables, so I turned to merge them to
'vhost_vring_state.num' in stead of introducing a new case
in handling ioctl, test has been done with a tweak in kernel side like:

--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1439,10 +1439,16 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
            r = -EFAULT;
            break;
        }
-               if (s.num > 0xffff) {
-                       r = -EINVAL;
-                       break;
-               }
+               if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) {
+                       vq->avail_wrap_counter = (bool)(uint16_t)(s.num >> 16);
+                       vq->last_used_idx = (uint16_t)(s.num >> 32);
+                       vq->used_wrap_counter = (bool)(uint16_t)(s.num >> 48);
+                } else {
+                    if (s.num > 0xffff) {
+                            r = -EINVAL;
+                            break;
+                    }
+                }
        vq->last_avail_idx = s.num;
        /* Forget the cached index value. */
        vq->avail_idx = vq->last_avail_idx;
@@ -1450,8 +1456,15 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
    case VHOST_GET_VRING_BASE:
        s.index = idx;
        s.num = vq->last_avail_idx;
+               if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) {
+                       s.num |= vq->avail_wrap_counter << 16;
+                       s.num |= vq->last_used_idx << 32;
+                       s.num |= vq->used_wrap_counter << 48;
+                }
        if (copy_to_user(argp, &s, sizeof s))
            r = -EFAULT;
+               if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED))
+                       s.num |= vq->avail_wrap_counter << 31;
        break;
    case VHOST_SET_VRING_ADDR:
        if (copy_from_user(&a, argp, sizeof a)) {

Wei Xu (8):
  virtio: feature bit, data structure, init for packed ring
  virtio: memory cache for packed ring
  virtio: empty check and desc read for packed ring
  virtio: get avail bytes check for packed ring
  virtio: queue pop support for packed ring
  virtio: flush/push support for packed ring
  virtio: event suppression support for packed ring
  virtio: support guest driver reload for vhost-net

 hw/net/vhost_net.c                             |   2 +
 hw/virtio/virtio.c                             | 677 +++++++++++++++++++++++--
 include/hw/virtio/virtio.h                     |   4 +-
 include/standard-headers/linux/virtio_config.h |  15 +
 4 files changed, 649 insertions(+), 49 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [RFC v2 1/8] virtio: feature bit, data structure, init for 1.1

[wexu]

From: Wei Xu <wexu@redhat.com>

New feature bit and members for packed ring.

Signed-off-by: Wei Xu <wexu@redhat.com>
---
 hw/net/vhost_net.c                             |  2 ++
 hw/virtio/virtio.c                             | 27 ++++++++++++++++++++++++--
 include/hw/virtio/virtio.h                     |  4 +++-
 include/standard-headers/linux/virtio_config.h |  2 ++
 4 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index e037db6..f593086 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -53,6 +53,7 @@ static const int kernel_feature_bits[] = {
     VIRTIO_F_VERSION_1,
     VIRTIO_NET_F_MTU,
     VIRTIO_F_IOMMU_PLATFORM,
+    VIRTIO_F_RING_PACKED,
     VHOST_INVALID_FEATURE_BIT
 };
 
@@ -78,6 +79,7 @@ static const int user_feature_bits[] = {
     VIRTIO_NET_F_MRG_RXBUF,
     VIRTIO_NET_F_MTU,
     VIRTIO_F_IOMMU_PLATFORM,
+    VIRTIO_F_RING_PACKED,
 
     /* This bit implies RARP isn't sent by QEMU out of band */
     VIRTIO_NET_F_GUEST_ANNOUNCE,
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 006d3d1..e192a9a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -39,6 +39,13 @@ typedef struct VRingDesc
     uint16_t next;
 } VRingDesc;
 
+typedef struct VRingDescPacked {
+    uint64_t addr;
+    uint32_t len;
+    uint16_t id;
+    uint16_t flags;
+} VRingDescPacked;
+
 typedef struct VRingAvail
 {
     uint16_t flags;
@@ -62,8 +69,14 @@ typedef struct VRingUsed
 typedef struct VRingMemoryRegionCaches {
     struct rcu_head rcu;
     MemoryRegionCache desc;
-    MemoryRegionCache avail;
-    MemoryRegionCache used;
+    union {
+        MemoryRegionCache avail;
+        MemoryRegionCache driver;
+    };
+    union {
+        MemoryRegionCache used;
+        MemoryRegionCache device;
+    };
 } VRingMemoryRegionCaches;
 
 typedef struct VRing
@@ -77,6 +90,11 @@ typedef struct VRing
     VRingMemoryRegionCaches *caches;
 } VRing;
 
+typedef struct VRingPackedDescEvent {
+    uint16_t off_wrap;
+    uint16_t flags;
+} VRingPackedDescEvent ;
+
 struct VirtQueue
 {
     VRing vring;
@@ -89,6 +107,9 @@ struct VirtQueue
 
     uint16_t used_idx;
 
+    bool avail_wrap_counter;
+    bool used_wrap_counter;
+
     /* Last used index value we have signalled on */
     uint16_t signalled_used;
 
@@ -1213,6 +1234,8 @@ void virtio_reset(void *opaque)
         vdev->vq[i].last_avail_idx = 0;
         vdev->vq[i].shadow_avail_idx = 0;
         vdev->vq[i].used_idx = 0;
+        vdev->vq[i].avail_wrap_counter = true;
+        vdev->vq[i].used_wrap_counter = true;
         virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
         vdev->vq[i].signalled_used = 0;
         vdev->vq[i].signalled_used_valid = false;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 098bdaa..4a7fb21 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -262,7 +262,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
     DEFINE_PROP_BIT64("any_layout", _state, _field, \
                       VIRTIO_F_ANY_LAYOUT, true), \
     DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
-                      VIRTIO_F_IOMMU_PLATFORM, false)
+                      VIRTIO_F_IOMMU_PLATFORM, false), \
+    DEFINE_PROP_BIT64("ring_packed", _state, _field, \
+                      VIRTIO_F_RING_PACKED, false)
 
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
 hwaddr virtio_queue_get_avail_addr(VirtIODevice *vdev, int n);
diff --git a/include/standard-headers/linux/virtio_config.h b/include/standard-headers/linux/virtio_config.h
index b777069..6ee5529 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -71,4 +71,6 @@
  * this is for compatibility with legacy systems.
  */
 #define VIRTIO_F_IOMMU_PLATFORM		33
+
+#define VIRTIO_F_RING_PACKED		34
 #endif /* _LINUX_VIRTIO_CONFIG_H */
-- 
1.8.3.1

Re: [Qemu-devel] [RFC v2 1/8] virtio: feature bit, data structure, init for 1.1

[Jason Wang]

On 2018年06月06日 03:07, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> New feature bit and members for packed ring.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> ---
>   hw/net/vhost_net.c                             |  2 ++
>   hw/virtio/virtio.c                             | 27 ++++++++++++++++++++++++--
>   include/hw/virtio/virtio.h                     |  4 +++-
>   include/standard-headers/linux/virtio_config.h |  2 ++
>   4 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index e037db6..f593086 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -53,6 +53,7 @@ static const int kernel_feature_bits[] = {
>       VIRTIO_F_VERSION_1,
>       VIRTIO_NET_F_MTU,
>       VIRTIO_F_IOMMU_PLATFORM,
> +    VIRTIO_F_RING_PACKED,
>       VHOST_INVALID_FEATURE_BIT
>   };
>   
> @@ -78,6 +79,7 @@ static const int user_feature_bits[] = {
>       VIRTIO_NET_F_MRG_RXBUF,
>       VIRTIO_NET_F_MTU,
>       VIRTIO_F_IOMMU_PLATFORM,
> +    VIRTIO_F_RING_PACKED,

This could be another patch for enabling packed ring for vhost. So it 
should be done in/after the patch of vhost support but not here.

>   
>       /* This bit implies RARP isn't sent by QEMU out of band */
>       VIRTIO_NET_F_GUEST_ANNOUNCE,
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 006d3d1..e192a9a 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -39,6 +39,13 @@ typedef struct VRingDesc
>       uint16_t next;
>   } VRingDesc;
>   
> +typedef struct VRingDescPacked {
> +    uint64_t addr;
> +    uint32_t len;
> +    uint16_t id;
> +    uint16_t flags;
> +} VRingDescPacked;
> +
>   typedef struct VRingAvail
>   {
>       uint16_t flags;
> @@ -62,8 +69,14 @@ typedef struct VRingUsed
>   typedef struct VRingMemoryRegionCaches {
>       struct rcu_head rcu;
>       MemoryRegionCache desc;
> -    MemoryRegionCache avail;
> -    MemoryRegionCache used;
> +    union {
> +        MemoryRegionCache avail;
> +        MemoryRegionCache driver;
> +    };
> +    union {
> +        MemoryRegionCache used;
> +        MemoryRegionCache device;
> +    };
>   } VRingMemoryRegionCaches;
>   
>   typedef struct VRing
> @@ -77,6 +90,11 @@ typedef struct VRing
>       VRingMemoryRegionCaches *caches;
>   } VRing;
>   
> +typedef struct VRingPackedDescEvent {
> +    uint16_t off_wrap;
> +    uint16_t flags;
> +} VRingPackedDescEvent ;
> +
>   struct VirtQueue
>   {
>       VRing vring;
> @@ -89,6 +107,9 @@ struct VirtQueue
>   
>       uint16_t used_idx;
>   
> +    bool avail_wrap_counter;
> +    bool used_wrap_counter;
> +
>       /* Last used index value we have signalled on */
>       uint16_t signalled_used;
>   
> @@ -1213,6 +1234,8 @@ void virtio_reset(void *opaque)
>           vdev->vq[i].last_avail_idx = 0;
>           vdev->vq[i].shadow_avail_idx = 0;
>           vdev->vq[i].used_idx = 0;
> +        vdev->vq[i].avail_wrap_counter = true;
> +        vdev->vq[i].used_wrap_counter = true;
>           virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
>           vdev->vq[i].signalled_used = 0;
>           vdev->vq[i].signalled_used_valid = false;
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 098bdaa..4a7fb21 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -262,7 +262,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
>       DEFINE_PROP_BIT64("any_layout", _state, _field, \
>                         VIRTIO_F_ANY_LAYOUT, true), \
>       DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
> -                      VIRTIO_F_IOMMU_PLATFORM, false)
> +                      VIRTIO_F_IOMMU_PLATFORM, false), \
> +    DEFINE_PROP_BIT64("ring_packed", _state, _field, \
> +                      VIRTIO_F_RING_PACKED, false)

This is wrong. Even if default is false, setting it to true will break 
everything. Please move this to the end of the series.

>   
>   hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
>   hwaddr virtio_queue_get_avail_addr(VirtIODevice *vdev, int n);
> diff --git a/include/standard-headers/linux/virtio_config.h b/include/standard-headers/linux/virtio_config.h
> index b777069..6ee5529 100644
> --- a/include/standard-headers/linux/virtio_config.h
> +++ b/include/standard-headers/linux/virtio_config.h
> @@ -71,4 +71,6 @@
>    * this is for compatibility with legacy systems.
>    */
>   #define VIRTIO_F_IOMMU_PLATFORM		33
> +
> +#define VIRTIO_F_RING_PACKED		34
>   #endif /* _LINUX_VIRTIO_CONFIG_H */

For formal version, it's better to have to just do a header sync in an 
independent patch.

Thanks

[Qemu-devel] [RFC v2 2/8] virtio: memory cache for packed ring

[wexu]

From: Wei Xu <wexu@redhat.com>

Mostly reuse memory cache with 1.0 except for the offset calculation.

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

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e192a9a..f6c0689 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -150,11 +150,8 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
     VRingMemoryRegionCaches *old = vq->vring.caches;
     VRingMemoryRegionCaches *new;
     hwaddr addr, size;
-    int event_size;
     int64_t len;
 
-    event_size = virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
-
     addr = vq->vring.desc;
     if (!addr) {
         return;
@@ -168,7 +165,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
         goto err_desc;
     }
 
-    size = virtio_queue_get_used_size(vdev, n) + event_size;
+    size = virtio_queue_get_used_size(vdev, n);
     len = address_space_cache_init(&new->used, vdev->dma_as,
                                    vq->vring.used, size, true);
     if (len < size) {
@@ -176,7 +173,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
         goto err_used;
     }
 
-    size = virtio_queue_get_avail_size(vdev, n) + event_size;
+    size = virtio_queue_get_avail_size(vdev, n);
     len = address_space_cache_init(&new->avail, vdev->dma_as,
                                    vq->vring.avail, size, false);
     if (len < size) {
@@ -2320,14 +2317,28 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
 
 hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
 {
-    return offsetof(VRingAvail, ring) +
-        sizeof(uint16_t) * vdev->vq[n].vring.num;
+    int s;
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
+        return sizeof(struct VRingPackedDescEvent);
+    } else {
+        s = virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
+        return offsetof(VRingAvail, ring) +
+            sizeof(uint16_t) * vdev->vq[n].vring.num + s;
+    }
 }
 
 hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
 {
-    return offsetof(VRingUsed, ring) +
-        sizeof(VRingUsedElem) * vdev->vq[n].vring.num;
+    int s;
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
+        return sizeof(struct VRingPackedDescEvent);
+    } else {
+        s = virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
+        return offsetof(VRingUsed, ring) +
+            sizeof(VRingUsedElem) * vdev->vq[n].vring.num + s;
+    }
 }
 
 uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)
-- 
1.8.3.1

Re: [Qemu-devel] [RFC v2 2/8] virtio: memory cache for packed ring

[Jason Wang]

On 2018年06月06日 03:07, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> Mostly reuse memory cache with 1.0 except for the offset calculation.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> ---
>   hw/virtio/virtio.c | 29 ++++++++++++++++++++---------
>   1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index e192a9a..f6c0689 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -150,11 +150,8 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
>       VRingMemoryRegionCaches *old = vq->vring.caches;
>       VRingMemoryRegionCaches *new;
>       hwaddr addr, size;
> -    int event_size;
>       int64_t len;
>   
> -    event_size = virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
> -
>       addr = vq->vring.desc;
>       if (!addr) {
>           return;
> @@ -168,7 +165,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
>           goto err_desc;
>       }
>   
> -    size = virtio_queue_get_used_size(vdev, n) + event_size;
> +    size = virtio_queue_get_used_size(vdev, n);
>       len = address_space_cache_init(&new->used, vdev->dma_as,
>                                      vq->vring.used, size, true);
>       if (len < size) {
> @@ -176,7 +173,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
>           goto err_used;
>       }
>   
> -    size = virtio_queue_get_avail_size(vdev, n) + event_size;
> +    size = virtio_queue_get_avail_size(vdev, n);
>       len = address_space_cache_init(&new->avail, vdev->dma_as,
>                                      vq->vring.avail, size, false);
>       if (len < size) {
> @@ -2320,14 +2317,28 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
>   
>   hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n)

I would rather rename this to virtio_queue_get_driver_size().

>   {
> -    return offsetof(VRingAvail, ring) +
> -        sizeof(uint16_t) * vdev->vq[n].vring.num;
> +    int s;
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +        return sizeof(struct VRingPackedDescEvent);
> +    } else {
> +        s = virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
> +        return offsetof(VRingAvail, ring) +
> +            sizeof(uint16_t) * vdev->vq[n].vring.num + s;
> +    }
>   }
>   
>   hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)

virtio_queue_get_device_size().

Thanks

>   {
> -    return offsetof(VRingUsed, ring) +
> -        sizeof(VRingUsedElem) * vdev->vq[n].vring.num;
> +    int s;
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +        return sizeof(struct VRingPackedDescEvent);
> +    } else {
> +        s = virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
> +        return offsetof(VRingUsed, ring) +
> +            sizeof(VRingUsedElem) * vdev->vq[n].vring.num + s;
> +    }
>   }
>   
>   uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)

Re: [Qemu-devel] [RFC v2 2/8] virtio: memory cache for packed ring

[Wei Xu]

On Wed, Jun 06, 2018 at 10:53:07AM +0800, Jason Wang wrote:
> 
> 
> On 2018年06月06日 03:07, wexu@redhat.com wrote:
> >From: Wei Xu <wexu@redhat.com>
> >
> >Mostly reuse memory cache with 1.0 except for the offset calculation.
> >
> >Signed-off-by: Wei Xu <wexu@redhat.com>
> >---
> >  hw/virtio/virtio.c | 29 ++++++++++++++++++++---------
> >  1 file changed, 20 insertions(+), 9 deletions(-)
> >
> >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >index e192a9a..f6c0689 100644
> >--- a/hw/virtio/virtio.c
> >+++ b/hw/virtio/virtio.c
> >@@ -150,11 +150,8 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
> >      VRingMemoryRegionCaches *old = vq->vring.caches;
> >      VRingMemoryRegionCaches *new;
> >      hwaddr addr, size;
> >-    int event_size;
> >      int64_t len;
> >-    event_size = virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
> >-
> >      addr = vq->vring.desc;
> >      if (!addr) {
> >          return;
> >@@ -168,7 +165,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
> >          goto err_desc;
> >      }
> >-    size = virtio_queue_get_used_size(vdev, n) + event_size;
> >+    size = virtio_queue_get_used_size(vdev, n);
> >      len = address_space_cache_init(&new->used, vdev->dma_as,
> >                                     vq->vring.used, size, true);
> >      if (len < size) {
> >@@ -176,7 +173,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
> >          goto err_used;
> >      }
> >-    size = virtio_queue_get_avail_size(vdev, n) + event_size;
> >+    size = virtio_queue_get_avail_size(vdev, n);
> >      len = address_space_cache_init(&new->avail, vdev->dma_as,
> >                                     vq->vring.avail, size, false);
> >      if (len < size) {
> >@@ -2320,14 +2317,28 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
> >  hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
> 
> I would rather rename this to virtio_queue_get_driver_size().

Would this confuse 1.0 if it is shared by both? Otherwise I will take it to next version, thanks.

Wei

> 
> >  {
> >-    return offsetof(VRingAvail, ring) +
> >-        sizeof(uint16_t) * vdev->vq[n].vring.num;
> >+    int s;
> >+
> >+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> >+        return sizeof(struct VRingPackedDescEvent);
> >+    } else {
> >+        s = virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
> >+        return offsetof(VRingAvail, ring) +
> >+            sizeof(uint16_t) * vdev->vq[n].vring.num + s;
> >+    }
> >  }
> >  hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
> 
> virtio_queue_get_device_size().
> 
> Thanks
> 
> >  {
> >-    return offsetof(VRingUsed, ring) +
> >-        sizeof(VRingUsedElem) * vdev->vq[n].vring.num;
> >+    int s;
> >+
> >+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> >+        return sizeof(struct VRingPackedDescEvent);
> >+    } else {
> >+        s = virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
> >+        return offsetof(VRingUsed, ring) +
> >+            sizeof(VRingUsedElem) * vdev->vq[n].vring.num + s;
> >+    }
> >  }
> >  uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)
> 

Re: [Qemu-devel] [RFC v2 2/8] virtio: memory cache for packed ring

[Paolo Bonzini]

On 05/06/2018 21:07, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
> 
> Mostly reuse memory cache with 1.0 except for the offset calculation.
> 
> Signed-off-by: Wei Xu <wexu@redhat.com>
> ---
>  hw/virtio/virtio.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index e192a9a..f6c0689 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -150,11 +150,8 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
>      VRingMemoryRegionCaches *old = vq->vring.caches;
>      VRingMemoryRegionCaches *new;
>      hwaddr addr, size;
> -    int event_size;
>      int64_t len;
>  
> -    event_size = virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
> -
>      addr = vq->vring.desc;
>      if (!addr) {
>          return;
> @@ -168,7 +165,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
>          goto err_desc;
>      }
>  
> -    size = virtio_queue_get_used_size(vdev, n) + event_size;
> +    size = virtio_queue_get_used_size(vdev, n);
>      len = address_space_cache_init(&new->used, vdev->dma_as,
>                                     vq->vring.used, size, true);
>      if (len < size) {
> @@ -176,7 +173,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
>          goto err_used;
>      }
>  
> -    size = virtio_queue_get_avail_size(vdev, n) + event_size;
> +    size = virtio_queue_get_avail_size(vdev, n);
>      len = address_space_cache_init(&new->avail, vdev->dma_as,
>                                     vq->vring.avail, size, false);
>      if (len < size) {
> @@ -2320,14 +2317,28 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
>  
>  hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
>  {
> -    return offsetof(VRingAvail, ring) +
> -        sizeof(uint16_t) * vdev->vq[n].vring.num;
> +    int s;
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +        return sizeof(struct VRingPackedDescEvent);
> +    } else {
> +        s = virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
> +        return offsetof(VRingAvail, ring) +
> +            sizeof(uint16_t) * vdev->vq[n].vring.num + s;
> +    }
>  }
>  
>  hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
>  {
> -    return offsetof(VRingUsed, ring) +
> -        sizeof(VRingUsedElem) * vdev->vq[n].vring.num;
> +    int s;
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +        return sizeof(struct VRingPackedDescEvent);
> +    } else {
> +        s = virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
> +        return offsetof(VRingUsed, ring) +
> +            sizeof(VRingUsedElem) * vdev->vq[n].vring.num + s;
> +    }
>  }
>  
>  uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

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

[wexu]

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 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));
+}
+
 /* 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);
+
+    /* Make sure we see the updated flag */
+    smp_mb();
+    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)) {
         goto done;
     }
     /* Needed after virtio_queue_empty(), see comment in
-- 
1.8.3.1

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

[Jason Wang]

On 2018年06月06日 03:07, 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 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

[Qemu-devel] [RFC v2 4/8] virtio: get avail bytes check for packed ring

[wexu]

From: Wei Xu <wexu@redhat.com>

mostly as same as 1.0 except traversing all desc to feed
headcount, need a refactor.

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

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index bd669a2..cdbb5af 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -650,9 +650,9 @@ static int virtqueue_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
     return VIRTQUEUE_READ_DESC_MORE;
 }
 
-void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
-                               unsigned int *out_bytes,
-                               unsigned max_in_bytes, unsigned max_out_bytes)
+static void virtqueue_split_get_avail_bytes(VirtQueue *vq,
+                            unsigned int *in_bytes, unsigned int *out_bytes,
+                            unsigned max_in_bytes, unsigned max_out_bytes)
 {
     VirtIODevice *vdev = vq->vdev;
     unsigned int max, idx;
@@ -775,6 +775,148 @@ err:
     goto done;
 }
 
+static void virtqueue_packed_get_avail_bytes(VirtQueue *vq,
+                            unsigned int *in_bytes, unsigned int *out_bytes,
+                            unsigned max_in_bytes, unsigned max_out_bytes)
+{
+    VirtIODevice *vdev = vq->vdev;
+    unsigned int max, idx;
+    unsigned int total_bufs, in_total, out_total;
+    MemoryRegionCache *desc_cache;
+    VRingMemoryRegionCaches *caches;
+    MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
+    int64_t len = 0;
+    VRingDescPacked desc;
+
+    if (unlikely(!vq->vring.desc)) {
+        if (in_bytes) {
+            *in_bytes = 0;
+        }
+        if (out_bytes) {
+            *out_bytes = 0;
+        }
+        return;
+    }
+
+    rcu_read_lock();
+    idx = vq->last_avail_idx;
+    total_bufs = in_total = out_total = 0;
+
+    max = vq->vring.num;
+    caches = vring_get_region_caches(vq);
+    if (caches->desc.len < max * sizeof(VRingDescPacked)) {
+        virtio_error(vdev, "Cannot map descriptor ring");
+        goto err;
+    }
+
+    desc_cache = &caches->desc;
+    vring_packed_desc_read(vdev, &desc, desc_cache, idx);
+    while (is_desc_avail(&desc)) {
+        unsigned int num_bufs;
+        unsigned int i;
+
+        num_bufs = total_bufs;
+
+        if (desc.flags & VRING_DESC_F_INDIRECT) {
+            if (desc.len % sizeof(VRingDescPacked)) {
+                virtio_error(vdev, "Invalid size for indirect buffer table");
+                goto err;
+            }
+
+            /* If we've got too many, that implies a descriptor loop. */
+            if (num_bufs >= max) {
+                virtio_error(vdev, "Looped descriptor");
+                goto err;
+            }
+
+            /* loop over the indirect descriptor table */
+            len = address_space_cache_init(&indirect_desc_cache,
+                                           vdev->dma_as,
+                                           desc.addr, desc.len, false);
+            desc_cache = &indirect_desc_cache;
+            if (len < desc.len) {
+                virtio_error(vdev, "Cannot map indirect buffer");
+                goto err;
+            }
+
+            max = desc.len / sizeof(VRingDescPacked);
+            num_bufs = i = 0;
+            vring_packed_desc_read(vdev, &desc, desc_cache, i);
+        }
+
+        do {
+            /* If we've got too many, that implies a descriptor loop. */
+            if (++num_bufs > max) {
+                virtio_error(vdev, "Looped descriptor");
+                goto err;
+            }
+
+            if (desc.flags & VRING_DESC_F_WRITE) {
+                in_total += desc.len;
+            } else {
+                out_total += desc.len;
+            }
+            if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
+                goto done;
+            }
+
+            if (desc_cache == &indirect_desc_cache) {
+                if (++i >= vq->vring.num) {
+                    i -= vq->vring.num;
+                }
+                vring_packed_desc_read(vdev, &desc, desc_cache, i);
+            } else {
+                if (++idx >= vq->vring.num) {
+                    idx -= vq->vring.num;
+                }
+                vring_packed_desc_read(vdev, &desc, desc_cache, idx);
+            }
+            /* Make sure we see the flags */
+            smp_mb();
+        } while (desc.flags & VRING_DESC_F_NEXT);
+
+        if (desc_cache == &indirect_desc_cache) {
+            address_space_cache_destroy(&indirect_desc_cache);
+            total_bufs++;
+            /* We missed one step on for indirect desc */
+            idx++;
+        } else {
+            total_bufs = num_bufs;
+        }
+
+        desc_cache = &caches->desc;
+        vring_packed_desc_read(vdev, &desc, desc_cache, idx % vq->vring.num);
+    }
+
+done:
+    address_space_cache_destroy(&indirect_desc_cache);
+    if (in_bytes) {
+        *in_bytes = in_total;
+    }
+    if (out_bytes) {
+        *out_bytes = out_total;
+    }
+    rcu_read_unlock();
+    return;
+
+err:
+    in_total = out_total = 0;
+    goto done;
+}
+
+void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
+                               unsigned int *out_bytes,
+                               unsigned max_in_bytes, unsigned max_out_bytes)
+{
+    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
+        virtqueue_packed_get_avail_bytes(vq, in_bytes, out_bytes,
+                                         max_in_bytes, max_out_bytes);
+    } else {
+        virtqueue_split_get_avail_bytes(vq, in_bytes, out_bytes,
+                                        max_in_bytes, max_out_bytes);
+    }
+}
+
 int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
                           unsigned int out_bytes)
 {
-- 
1.8.3.1

Re: [Qemu-devel] [RFC v2 4/8] virtio: get avail bytes check for packed ring

[Jason Wang]

On 2018年06月06日 03:07, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> mostly as same as 1.0 except traversing all desc to feed
> headcount, need a refactor.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> ---
>   hw/virtio/virtio.c | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 145 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index bd669a2..cdbb5af 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -650,9 +650,9 @@ static int virtqueue_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
>       return VIRTQUEUE_READ_DESC_MORE;
>   }
>   
> -void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> -                               unsigned int *out_bytes,
> -                               unsigned max_in_bytes, unsigned max_out_bytes)
> +static void virtqueue_split_get_avail_bytes(VirtQueue *vq,
> +                            unsigned int *in_bytes, unsigned int *out_bytes,
> +                            unsigned max_in_bytes, unsigned max_out_bytes)
>   {
>       VirtIODevice *vdev = vq->vdev;
>       unsigned int max, idx;
> @@ -775,6 +775,148 @@ err:
>       goto done;
>   }
>   
> +static void virtqueue_packed_get_avail_bytes(VirtQueue *vq,
> +                            unsigned int *in_bytes, unsigned int *out_bytes,
> +                            unsigned max_in_bytes, unsigned max_out_bytes)
> +{
> +    VirtIODevice *vdev = vq->vdev;
> +    unsigned int max, idx;
> +    unsigned int total_bufs, in_total, out_total;
> +    MemoryRegionCache *desc_cache;
> +    VRingMemoryRegionCaches *caches;
> +    MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
> +    int64_t len = 0;
> +    VRingDescPacked desc;
> +
> +    if (unlikely(!vq->vring.desc)) {
> +        if (in_bytes) {
> +            *in_bytes = 0;
> +        }
> +        if (out_bytes) {
> +            *out_bytes = 0;
> +        }
> +        return;
> +    }
> +
> +    rcu_read_lock();
> +    idx = vq->last_avail_idx;
> +    total_bufs = in_total = out_total = 0;
> +
> +    max = vq->vring.num;
> +    caches = vring_get_region_caches(vq);
> +    if (caches->desc.len < max * sizeof(VRingDescPacked)) {
> +        virtio_error(vdev, "Cannot map descriptor ring");
> +        goto err;
> +    }
> +
> +    desc_cache = &caches->desc;
> +    vring_packed_desc_read(vdev, &desc, desc_cache, idx);
> +    while (is_desc_avail(&desc)) {

So you did vring_packed_desc_read() like:

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);
}

How do you guarantee desc->flags was read before all other fields?

> +        unsigned int num_bufs;
> +        unsigned int i;
> +
> +        num_bufs = total_bufs;
> +
> +        if (desc.flags & VRING_DESC_F_INDIRECT) {
> +            if (desc.len % sizeof(VRingDescPacked)) {
> +                virtio_error(vdev, "Invalid size for indirect buffer table");
> +                goto err;
> +            }
> +
> +            /* If we've got too many, that implies a descriptor loop. */
> +            if (num_bufs >= max) {
> +                virtio_error(vdev, "Looped descriptor");
> +                goto err;
> +            }
> +
> +            /* loop over the indirect descriptor table */
> +            len = address_space_cache_init(&indirect_desc_cache,
> +                                           vdev->dma_as,
> +                                           desc.addr, desc.len, false);
> +            desc_cache = &indirect_desc_cache;
> +            if (len < desc.len) {
> +                virtio_error(vdev, "Cannot map indirect buffer");
> +                goto err;
> +            }
> +
> +            max = desc.len / sizeof(VRingDescPacked);
> +            num_bufs = i = 0;
> +            vring_packed_desc_read(vdev, &desc, desc_cache, i);
> +        }
> +
> +        do {
> +            /* If we've got too many, that implies a descriptor loop. */
> +            if (++num_bufs > max) {
> +                virtio_error(vdev, "Looped descriptor");
> +                goto err;
> +            }
> +
> +            if (desc.flags & VRING_DESC_F_WRITE) {
> +                in_total += desc.len;
> +            } else {
> +                out_total += desc.len;
> +            }
> +            if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
> +                goto done;
> +            }
> +
> +            if (desc_cache == &indirect_desc_cache) {
> +                if (++i >= vq->vring.num) {
> +                    i -= vq->vring.num;
> +                }
> +                vring_packed_desc_read(vdev, &desc, desc_cache, i);
> +            } else {
> +                if (++idx >= vq->vring.num) {
> +                    idx -= vq->vring.num;
> +                }
> +                vring_packed_desc_read(vdev, &desc, desc_cache, idx);
> +            }

Ok, so the difference is the index, can we use a single index instead of 
two (i, idx) here?

> +            /* Make sure we see the flags */
> +            smp_mb();

Why need this?

Thanks

> +        } while (desc.flags & VRING_DESC_F_NEXT);
> +
> +        if (desc_cache == &indirect_desc_cache) {
> +            address_space_cache_destroy(&indirect_desc_cache);
> +            total_bufs++;
> +            /* We missed one step on for indirect desc */
> +            idx++;
> +        } else {
> +            total_bufs = num_bufs;
> +        }
> +
> +        desc_cache = &caches->desc;
> +        vring_packed_desc_read(vdev, &desc, desc_cache, idx % vq->vring.num);
> +    }
> +
> +done:
> +    address_space_cache_destroy(&indirect_desc_cache);
> +    if (in_bytes) {
> +        *in_bytes = in_total;
> +    }
> +    if (out_bytes) {
> +        *out_bytes = out_total;
> +    }
> +    rcu_read_unlock();
> +    return;
> +
> +err:
> +    in_total = out_total = 0;
> +    goto done;
> +}
> +
> +void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> +                               unsigned int *out_bytes,
> +                               unsigned max_in_bytes, unsigned max_out_bytes)
> +{
> +    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> +        virtqueue_packed_get_avail_bytes(vq, in_bytes, out_bytes,
> +                                         max_in_bytes, max_out_bytes);
> +    } else {
> +        virtqueue_split_get_avail_bytes(vq, in_bytes, out_bytes,
> +                                        max_in_bytes, max_out_bytes);
> +    }
> +}
> +
>   int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
>                             unsigned int out_bytes)
>   {

[Qemu-devel] [RFC v2 5/8] virtio: queue pop for packed ring

[wexu]

From: Wei Xu <wexu@redhat.com>

Signed-off-by: Wei Xu <wexu@redhat.com>
---
 hw/virtio/virtio.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 144 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index cdbb5af..0160d03 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1041,7 +1041,7 @@ static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_nu
     return elem;
 }
 
-void *virtqueue_pop(VirtQueue *vq, size_t sz)
+static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
 {
     unsigned int i, head, max;
     VRingMemoryRegionCaches *caches;
@@ -1176,6 +1176,149 @@ err_undo_map:
     goto done;
 }
 
+static void *virtqueue_packed_pop(VirtQueue *vq, size_t sz)
+{
+    unsigned int i, head, max;
+    VRingMemoryRegionCaches *caches;
+    MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
+    MemoryRegionCache *cache;
+    int64_t len;
+    VirtIODevice *vdev = vq->vdev;
+    VirtQueueElement *elem = NULL;
+    unsigned out_num, in_num, elem_entries;
+    hwaddr addr[VIRTQUEUE_MAX_SIZE];
+    struct iovec iov[VIRTQUEUE_MAX_SIZE];
+    VRingDescPacked desc;
+
+    if (unlikely(vdev->broken)) {
+        return NULL;
+    }
+
+    rcu_read_lock();
+    if (virtio_queue_packed_empty_rcu(vq)) {
+        goto done;
+    }
+
+    /* When we start there are none of either input nor output. */
+    out_num = in_num = elem_entries = 0;
+
+    max = vq->vring.num;
+
+    if (vq->inuse >= vq->vring.num) {
+        virtio_error(vdev, "Virtqueue size exceeded");
+        goto done;
+    }
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
+        /* FIXME: TBD */
+    }
+
+    head = vq->last_avail_idx;
+    i = head;
+
+    caches = vring_get_region_caches(vq);
+    cache = &caches->desc;
+    vring_packed_desc_read(vdev, &desc, cache, i);
+    if (desc.flags & VRING_DESC_F_INDIRECT) {
+        if (desc.len % sizeof(VRingDescPacked)) {
+            virtio_error(vdev, "Invalid size for indirect buffer table");
+            goto done;
+        }
+
+        /* loop over the indirect descriptor table */
+        len = address_space_cache_init(&indirect_desc_cache, vdev->dma_as,
+                                       desc.addr, desc.len, false);
+        cache = &indirect_desc_cache;
+        if (len < desc.len) {
+            virtio_error(vdev, "Cannot map indirect buffer");
+            goto done;
+        }
+
+        max = desc.len / sizeof(VRingDescPacked);
+        i = 0;
+        vring_packed_desc_read(vdev, &desc, cache, i);
+    }
+
+    /* Collect all the descriptors */
+    while (1) {
+        bool map_ok;
+
+        if (desc.flags & VRING_DESC_F_WRITE) {
+            map_ok = virtqueue_map_desc(vdev, &in_num, addr + out_num,
+                                        iov + out_num,
+                                        VIRTQUEUE_MAX_SIZE - out_num, true,
+                                        desc.addr, desc.len);
+        } else {
+            if (in_num) {
+                virtio_error(vdev, "Incorrect order for descriptors");
+                goto err_undo_map;
+            }
+            map_ok = virtqueue_map_desc(vdev, &out_num, addr, iov,
+                                        VIRTQUEUE_MAX_SIZE, false,
+                                        desc.addr, desc.len);
+        }
+        if (!map_ok) {
+            goto err_undo_map;
+        }
+
+        /* If we've got too many, that implies a descriptor loop. */
+        if (++elem_entries > max) {
+            virtio_error(vdev, "Looped descriptor");
+            goto err_undo_map;
+        }
+
+        if (++i >= vq->vring.num) {
+            i -= vq->vring.num;
+        }
+
+        if (desc.flags & VRING_DESC_F_NEXT) {
+            vring_packed_desc_read(vq->vdev, &desc, cache, i);
+        } else {
+            break;
+        }
+    }
+
+    /* Now copy what we have collected and mapped */
+    elem = virtqueue_alloc_element(sz, out_num, in_num);
+    for (i = 0; i < out_num; i++) {
+        elem->out_addr[i] = addr[i];
+        elem->out_sg[i] = iov[i];
+    }
+    for (i = 0; i < in_num; i++) {
+        elem->in_addr[i] = addr[head + out_num + i];
+        elem->in_sg[i] = iov[out_num + i];
+    }
+
+    vq->last_avail_idx += (cache == &indirect_desc_cache) ?
+                          1 : out_num + in_num;
+    if (vq->last_avail_idx >= vq->vring.num) {
+        vq->last_avail_idx -= vq->vring.num;
+        vq->avail_wrap_counter = !vq->avail_wrap_counter;
+    }
+    vq->inuse++;
+
+    trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
+done:
+    address_space_cache_destroy(&indirect_desc_cache);
+    rcu_read_unlock();
+
+    return elem;
+
+err_undo_map:
+    virtqueue_undo_map_desc(out_num, in_num, iov);
+    g_free(elem);
+    goto done;
+}
+
+void *virtqueue_pop(VirtQueue *vq, size_t sz)
+{
+    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
+        return virtqueue_packed_pop(vq, sz);
+    } else {
+        return virtqueue_split_pop(vq, sz);
+    }
+}
+
 /* virtqueue_drop_all:
  * @vq: The #VirtQueue
  * Drops all queued buffers and indicates them to the guest
-- 
1.8.3.1

Re: [Qemu-devel] [RFC v2 5/8] virtio: queue pop for packed ring

[Jason Wang]

On 2018年06月06日 03:08, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> ---
>   hw/virtio/virtio.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 144 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index cdbb5af..0160d03 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1041,7 +1041,7 @@ static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_nu
>       return elem;
>   }
>   
> -void *virtqueue_pop(VirtQueue *vq, size_t sz)
> +static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
>   {
>       unsigned int i, head, max;
>       VRingMemoryRegionCaches *caches;
> @@ -1176,6 +1176,149 @@ err_undo_map:
>       goto done;
>   }
>   
> +static void *virtqueue_packed_pop(VirtQueue *vq, size_t sz)
> +{
> +    unsigned int i, head, max;
> +    VRingMemoryRegionCaches *caches;
> +    MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
> +    MemoryRegionCache *cache;
> +    int64_t len;
> +    VirtIODevice *vdev = vq->vdev;
> +    VirtQueueElement *elem = NULL;
> +    unsigned out_num, in_num, elem_entries;
> +    hwaddr addr[VIRTQUEUE_MAX_SIZE];
> +    struct iovec iov[VIRTQUEUE_MAX_SIZE];
> +    VRingDescPacked desc;
> +
> +    if (unlikely(vdev->broken)) {
> +        return NULL;
> +    }
> +
> +    rcu_read_lock();
> +    if (virtio_queue_packed_empty_rcu(vq)) {
> +        goto done;
> +    }

Instead of depending on the barriers inside 
virtio_queue_packed_empty_rcu(). I think it's better to keep a smp_rmb() 
here with comments.

> +
> +    /* When we start there are none of either input nor output. */
> +    out_num = in_num = elem_entries = 0;
> +
> +    max = vq->vring.num;
> +
> +    if (vq->inuse >= vq->vring.num) {
> +        virtio_error(vdev, "Virtqueue size exceeded");
> +        goto done;
> +    }
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> +        /* FIXME: TBD */
> +    }

This part could be removed.

> +
> +    head = vq->last_avail_idx;
> +    i = head;
> +
> +    caches = vring_get_region_caches(vq);
> +    cache = &caches->desc;
> +    vring_packed_desc_read(vdev, &desc, cache, i);

I think we'd better find a way to avoid reading descriptor twice.

Thanks

> +    if (desc.flags & VRING_DESC_F_INDIRECT) {
> +        if (desc.len % sizeof(VRingDescPacked)) {
> +            virtio_error(vdev, "Invalid size for indirect buffer table");
> +            goto done;
> +        }
> +
> +        /* loop over the indirect descriptor table */
> +        len = address_space_cache_init(&indirect_desc_cache, vdev->dma_as,
> +                                       desc.addr, desc.len, false);
> +        cache = &indirect_desc_cache;
> +        if (len < desc.len) {
> +            virtio_error(vdev, "Cannot map indirect buffer");
> +            goto done;
> +        }
> +
> +        max = desc.len / sizeof(VRingDescPacked);
> +        i = 0;
> +        vring_packed_desc_read(vdev, &desc, cache, i);
> +    }
> +
> +    /* Collect all the descriptors */
> +    while (1) {
> +        bool map_ok;
> +
> +        if (desc.flags & VRING_DESC_F_WRITE) {
> +            map_ok = virtqueue_map_desc(vdev, &in_num, addr + out_num,
> +                                        iov + out_num,
> +                                        VIRTQUEUE_MAX_SIZE - out_num, true,
> +                                        desc.addr, desc.len);
> +        } else {
> +            if (in_num) {
> +                virtio_error(vdev, "Incorrect order for descriptors");
> +                goto err_undo_map;
> +            }
> +            map_ok = virtqueue_map_desc(vdev, &out_num, addr, iov,
> +                                        VIRTQUEUE_MAX_SIZE, false,
> +                                        desc.addr, desc.len);
> +        }
> +        if (!map_ok) {
> +            goto err_undo_map;
> +        }
> +
> +        /* If we've got too many, that implies a descriptor loop. */
> +        if (++elem_entries > max) {
> +            virtio_error(vdev, "Looped descriptor");
> +            goto err_undo_map;
> +        }
> +
> +        if (++i >= vq->vring.num) {
> +            i -= vq->vring.num;
> +        }
> +
> +        if (desc.flags & VRING_DESC_F_NEXT) {
> +            vring_packed_desc_read(vq->vdev, &desc, cache, i);
> +        } else {
> +            break;
> +        }
> +    }
> +
> +    /* Now copy what we have collected and mapped */
> +    elem = virtqueue_alloc_element(sz, out_num, in_num);
> +    for (i = 0; i < out_num; i++) {
> +        elem->out_addr[i] = addr[i];
> +        elem->out_sg[i] = iov[i];
> +    }
> +    for (i = 0; i < in_num; i++) {
> +        elem->in_addr[i] = addr[head + out_num + i];
> +        elem->in_sg[i] = iov[out_num + i];
> +    }
> +
> +    vq->last_avail_idx += (cache == &indirect_desc_cache) ?
> +                          1 : out_num + in_num;
> +    if (vq->last_avail_idx >= vq->vring.num) {
> +        vq->last_avail_idx -= vq->vring.num;
> +        vq->avail_wrap_counter = !vq->avail_wrap_counter;
> +    }
> +    vq->inuse++;
> +
> +    trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
> +done:
> +    address_space_cache_destroy(&indirect_desc_cache);
> +    rcu_read_unlock();
> +
> +    return elem;
> +
> +err_undo_map:
> +    virtqueue_undo_map_desc(out_num, in_num, iov);
> +    g_free(elem);
> +    goto done;
> +}
> +
> +void *virtqueue_pop(VirtQueue *vq, size_t sz)
> +{
> +    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> +        return virtqueue_packed_pop(vq, sz);
> +    } else {
> +        return virtqueue_split_pop(vq, sz);
> +    }
> +}
> +
>   /* virtqueue_drop_all:
>    * @vq: The #VirtQueue
>    * Drops all queued buffers and indicates them to the guest

Re: [Qemu-devel] [RFC v2 5/8] virtio: queue pop for packed ring

[Wei Xu]

On Wed, Jun 06, 2018 at 11:29:54AM +0800, Jason Wang wrote:
> 
> 
> On 2018年06月06日 03:08, wexu@redhat.com wrote:
> >From: Wei Xu <wexu@redhat.com>
> >
> >Signed-off-by: Wei Xu <wexu@redhat.com>
> >---
> >  hw/virtio/virtio.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 144 insertions(+), 1 deletion(-)
> >
> >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >index cdbb5af..0160d03 100644
> >--- a/hw/virtio/virtio.c
> >+++ b/hw/virtio/virtio.c
> >@@ -1041,7 +1041,7 @@ static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_nu
> >      return elem;
> >  }
> >-void *virtqueue_pop(VirtQueue *vq, size_t sz)
> >+static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
> >  {
> >      unsigned int i, head, max;
> >      VRingMemoryRegionCaches *caches;
> >@@ -1176,6 +1176,149 @@ err_undo_map:
> >      goto done;
> >  }
> >+static void *virtqueue_packed_pop(VirtQueue *vq, size_t sz)
> >+{
> >+    unsigned int i, head, max;
> >+    VRingMemoryRegionCaches *caches;
> >+    MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
> >+    MemoryRegionCache *cache;
> >+    int64_t len;
> >+    VirtIODevice *vdev = vq->vdev;
> >+    VirtQueueElement *elem = NULL;
> >+    unsigned out_num, in_num, elem_entries;
> >+    hwaddr addr[VIRTQUEUE_MAX_SIZE];
> >+    struct iovec iov[VIRTQUEUE_MAX_SIZE];
> >+    VRingDescPacked desc;
> >+
> >+    if (unlikely(vdev->broken)) {
> >+        return NULL;
> >+    }
> >+
> >+    rcu_read_lock();
> >+    if (virtio_queue_packed_empty_rcu(vq)) {
> >+        goto done;
> >+    }
> 
> Instead of depending on the barriers inside virtio_queue_packed_empty_rcu().
> I think it's better to keep a smp_rmb() here with comments.

OK.

> 
> >+
> >+    /* When we start there are none of either input nor output. */
> >+    out_num = in_num = elem_entries = 0;
> >+
> >+    max = vq->vring.num;
> >+
> >+    if (vq->inuse >= vq->vring.num) {
> >+        virtio_error(vdev, "Virtqueue size exceeded");
> >+        goto done;
> >+    }
> >+
> >+    if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> >+        /* FIXME: TBD */
> >+    }
> 
> This part could be removed.

My bad, thanks.

> 
> >+
> >+    head = vq->last_avail_idx;
> >+    i = head;
> >+
> >+    caches = vring_get_region_caches(vq);
> >+    cache = &caches->desc;
> >+    vring_packed_desc_read(vdev, &desc, cache, i);
> 
> I think we'd better find a way to avoid reading descriptor twice.

Do you mean here and the read for empty check?

Wei

> 
> Thanks
> 
> >+    if (desc.flags & VRING_DESC_F_INDIRECT) {
> >+        if (desc.len % sizeof(VRingDescPacked)) {
> >+            virtio_error(vdev, "Invalid size for indirect buffer table");
> >+            goto done;
> >+        }
> >+
> >+        /* loop over the indirect descriptor table */
> >+        len = address_space_cache_init(&indirect_desc_cache, vdev->dma_as,
> >+                                       desc.addr, desc.len, false);
> >+        cache = &indirect_desc_cache;
> >+        if (len < desc.len) {
> >+            virtio_error(vdev, "Cannot map indirect buffer");
> >+            goto done;
> >+        }
> >+
> >+        max = desc.len / sizeof(VRingDescPacked);
> >+        i = 0;
> >+        vring_packed_desc_read(vdev, &desc, cache, i);
> >+    }
> >+
> >+    /* Collect all the descriptors */
> >+    while (1) {
> >+        bool map_ok;
> >+
> >+        if (desc.flags & VRING_DESC_F_WRITE) {
> >+            map_ok = virtqueue_map_desc(vdev, &in_num, addr + out_num,
> >+                                        iov + out_num,
> >+                                        VIRTQUEUE_MAX_SIZE - out_num, true,
> >+                                        desc.addr, desc.len);
> >+        } else {
> >+            if (in_num) {
> >+                virtio_error(vdev, "Incorrect order for descriptors");
> >+                goto err_undo_map;
> >+            }
> >+            map_ok = virtqueue_map_desc(vdev, &out_num, addr, iov,
> >+                                        VIRTQUEUE_MAX_SIZE, false,
> >+                                        desc.addr, desc.len);
> >+        }
> >+        if (!map_ok) {
> >+            goto err_undo_map;
> >+        }
> >+
> >+        /* If we've got too many, that implies a descriptor loop. */
> >+        if (++elem_entries > max) {
> >+            virtio_error(vdev, "Looped descriptor");
> >+            goto err_undo_map;
> >+        }
> >+
> >+        if (++i >= vq->vring.num) {
> >+            i -= vq->vring.num;
> >+        }
> >+
> >+        if (desc.flags & VRING_DESC_F_NEXT) {
> >+            vring_packed_desc_read(vq->vdev, &desc, cache, i);
> >+        } else {
> >+            break;
> >+        }
> >+    }
> >+
> >+    /* Now copy what we have collected and mapped */
> >+    elem = virtqueue_alloc_element(sz, out_num, in_num);
> >+    for (i = 0; i < out_num; i++) {
> >+        elem->out_addr[i] = addr[i];
> >+        elem->out_sg[i] = iov[i];
> >+    }
> >+    for (i = 0; i < in_num; i++) {
> >+        elem->in_addr[i] = addr[head + out_num + i];
> >+        elem->in_sg[i] = iov[out_num + i];
> >+    }
> >+
> >+    vq->last_avail_idx += (cache == &indirect_desc_cache) ?
> >+                          1 : out_num + in_num;
> >+    if (vq->last_avail_idx >= vq->vring.num) {
> >+        vq->last_avail_idx -= vq->vring.num;
> >+        vq->avail_wrap_counter = !vq->avail_wrap_counter;
> >+    }
> >+    vq->inuse++;
> >+
> >+    trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
> >+done:
> >+    address_space_cache_destroy(&indirect_desc_cache);
> >+    rcu_read_unlock();
> >+
> >+    return elem;
> >+
> >+err_undo_map:
> >+    virtqueue_undo_map_desc(out_num, in_num, iov);
> >+    g_free(elem);
> >+    goto done;
> >+}
> >+
> >+void *virtqueue_pop(VirtQueue *vq, size_t sz)
> >+{
> >+    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> >+        return virtqueue_packed_pop(vq, sz);
> >+    } else {
> >+        return virtqueue_split_pop(vq, sz);
> >+    }
> >+}
> >+
> >  /* virtqueue_drop_all:
> >   * @vq: The #VirtQueue
> >   * Drops all queued buffers and indicates them to the guest
> 

Re: [Qemu-devel] [RFC v2 5/8] virtio: queue pop for packed ring

[Jason Wang]

On 2018年06月06日 11:38, Wei Xu wrote:
>>> +
>>> +    head = vq->last_avail_idx;
>>> +    i = head;
>>> +
>>> +    caches = vring_get_region_caches(vq);
>>> +    cache = &caches->desc;
>>> +    vring_packed_desc_read(vdev, &desc, cache, i);
>> I think we'd better find a way to avoid reading descriptor twice.
> Do you mean here and the read for empty check?
>
> Wei
>

Yes.

Thanks

Re: [Qemu-devel] [RFC v2 5/8] virtio: queue pop for packed ring

[Wei Xu]

On Wed, Jun 06, 2018 at 11:41:18AM +0800, Jason Wang wrote:
> 
> 
> On 2018年06月06日 11:38, Wei Xu wrote:
> >>>+
> >>>+    head = vq->last_avail_idx;
> >>>+    i = head;
> >>>+
> >>>+    caches = vring_get_region_caches(vq);
> >>>+    cache = &caches->desc;
> >>>+    vring_packed_desc_read(vdev, &desc, cache, i);
> >>I think we'd better find a way to avoid reading descriptor twice.
> >Do you mean here and the read for empty check?
> >
> >Wei
> >
> 
> Yes.

OK, will figure it out.

> 
> Thanks
> 
> 

[Qemu-devel] [RFC v2 6/8] virtio: flush/push for packed ring

[wexu]

From: Wei Xu <wexu@redhat.com>

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

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 0160d03..6f2da83 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -371,6 +371,21 @@ static void vring_packed_desc_read(VirtIODevice *vdev, VRingDescPacked *desc,
     virtio_tswap16s(vdev, &desc->flags);
 }
 
+static void vring_packed_desc_write(VirtIODevice *vdev, VRingDescPacked *desc,
+                            MemoryRegionCache *cache, int i)
+{
+    virtio_tswap64s(vdev, &desc->addr);
+    virtio_tswap32s(vdev, &desc->len);
+    virtio_tswap16s(vdev, &desc->id);
+    virtio_tswap16s(vdev, &desc->flags);
+    address_space_write_cached(cache,
+                               sizeof(VRingDescPacked) * i, desc,
+                               sizeof(VRingDescPacked));
+    address_space_cache_invalidate(cache,
+                                   sizeof(VRingDescPacked) * i,
+                                   sizeof(VRingDescPacked));
+}
+
 static inline bool is_desc_avail(struct VRingDescPacked *desc)
 {
     return !!(desc->flags & AVAIL_DESC_PACKED(1)) !=
@@ -526,19 +541,11 @@ bool virtqueue_rewind(VirtQueue *vq, unsigned int num)
 }
 
 /* Called within rcu_read_lock().  */
-void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
+static void virtqueue_split_fill(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len, unsigned int idx)
 {
     VRingUsedElem uelem;
 
-    trace_virtqueue_fill(vq, elem, len, idx);
-
-    virtqueue_unmap_sg(vq, elem, len);
-
-    if (unlikely(vq->vdev->broken)) {
-        return;
-    }
-
     if (unlikely(!vq->vring.used)) {
         return;
     }
@@ -550,16 +557,64 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
     vring_used_write(vq, &uelem, idx);
 }
 
-/* Called within rcu_read_lock().  */
-void virtqueue_flush(VirtQueue *vq, unsigned int count)
+static void virtqueue_packed_fill(VirtQueue *vq, const VirtQueueElement *elem,
+                        unsigned int len, unsigned int idx)
 {
-    uint16_t old, new;
+    uint16_t w, head;
+    VRingMemoryRegionCaches *caches;
+    VRingDescPacked desc = {
+        .addr = 0,
+        .flags = 0,
+    };
+
+    if (unlikely(!vq->vring.desc)) {
+        return;
+    }
+
+    caches = vring_get_region_caches(vq);
+    head = vq->used_idx + idx;
+    head = head >= vq->vring.num ? (head - vq->vring.num) : head;
+    vring_packed_desc_read(vq->vdev, &desc, &caches->desc, head);
+
+    w = (desc.flags & AVAIL_DESC_PACKED(1)) >> 7;
+    desc.flags &= ~(AVAIL_DESC_PACKED(1) | USED_DESC_PACKED(1));
+    desc.flags |= AVAIL_DESC_PACKED(w) | USED_DESC_PACKED(w);
+    if (!(desc.flags & VRING_DESC_F_INDIRECT)) {
+        if (!(desc.flags & VRING_DESC_F_WRITE)) {
+            desc.len = 0;
+        } else {
+            desc.len = len;
+        }
+    }
+    vring_packed_desc_write(vq->vdev, &desc, &caches->desc, head);
+
+    /* Make sure flags has been updated */
+    smp_mb();
+}
+
+void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
+                    unsigned int len, unsigned int idx)
+{
+    trace_virtqueue_fill(vq, elem, len, idx);
+
+    virtqueue_unmap_sg(vq, elem, len);
 
     if (unlikely(vq->vdev->broken)) {
-        vq->inuse -= count;
         return;
     }
 
+    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
+        virtqueue_packed_fill(vq, elem, len, idx);
+    } else {
+        virtqueue_split_fill(vq, elem, len, idx);
+    }
+}
+
+/* Called within rcu_read_lock().  */
+static void virtqueue_split_flush(VirtQueue *vq, unsigned int count)
+{
+    uint16_t old, new;
+
     if (unlikely(!vq->vring.used)) {
         return;
     }
@@ -575,6 +630,34 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
         vq->signalled_used_valid = false;
 }
 
+static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
+{
+    if (unlikely(!vq->vring.desc)) {
+        return;
+    }
+
+    vq->inuse -= count;
+    vq->used_idx += count;
+    if (vq->used_idx >= vq->vring.num) {
+        vq->used_idx -= vq->vring.num;
+        vq->used_wrap_counter = !vq->used_wrap_counter;
+    }
+}
+
+void virtqueue_flush(VirtQueue *vq, unsigned int count)
+{
+    if (unlikely(vq->vdev->broken)) {
+        vq->inuse -= count;
+        return;
+    }
+
+    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
+        virtqueue_packed_flush(vq, count);
+    } else {
+        virtqueue_split_flush(vq, count);
+    }
+}
+
 void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len)
 {
-- 
1.8.3.1

Re: [Qemu-devel] [RFC v2 6/8] virtio: flush/push for packed ring

[Jason Wang]

On 2018年06月06日 03:08, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> ---
>   hw/virtio/virtio.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 96 insertions(+), 13 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 0160d03..6f2da83 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -371,6 +371,21 @@ static void vring_packed_desc_read(VirtIODevice *vdev, VRingDescPacked *desc,
>       virtio_tswap16s(vdev, &desc->flags);
>   }
>   
> +static void vring_packed_desc_write(VirtIODevice *vdev, VRingDescPacked *desc,
> +                            MemoryRegionCache *cache, int i)
> +{
> +    virtio_tswap64s(vdev, &desc->addr);
> +    virtio_tswap32s(vdev, &desc->len);
> +    virtio_tswap16s(vdev, &desc->id);
> +    virtio_tswap16s(vdev, &desc->flags);
> +    address_space_write_cached(cache,
> +                               sizeof(VRingDescPacked) * i, desc,
> +                               sizeof(VRingDescPacked));
> +    address_space_cache_invalidate(cache,
> +                                   sizeof(VRingDescPacked) * i,
> +                                   sizeof(VRingDescPacked));

Is there a guarantee that flags was wrote before all other fields?

> +}
> +
>   static inline bool is_desc_avail(struct VRingDescPacked *desc)
>   {
>       return !!(desc->flags & AVAIL_DESC_PACKED(1)) !=
> @@ -526,19 +541,11 @@ bool virtqueue_rewind(VirtQueue *vq, unsigned int num)
>   }
>   
>   /* Called within rcu_read_lock().  */
> -void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
> +static void virtqueue_split_fill(VirtQueue *vq, const VirtQueueElement *elem,
>                       unsigned int len, unsigned int idx)
>   {
>       VRingUsedElem uelem;
>   
> -    trace_virtqueue_fill(vq, elem, len, idx);
> -
> -    virtqueue_unmap_sg(vq, elem, len);
> -
> -    if (unlikely(vq->vdev->broken)) {
> -        return;
> -    }
> -
>       if (unlikely(!vq->vring.used)) {
>           return;
>       }
> @@ -550,16 +557,64 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>       vring_used_write(vq, &uelem, idx);
>   }
>   
> -/* Called within rcu_read_lock().  */
> -void virtqueue_flush(VirtQueue *vq, unsigned int count)
> +static void virtqueue_packed_fill(VirtQueue *vq, const VirtQueueElement *elem,
> +                        unsigned int len, unsigned int idx)
>   {
> -    uint16_t old, new;
> +    uint16_t w, head;
> +    VRingMemoryRegionCaches *caches;
> +    VRingDescPacked desc = {
> +        .addr = 0,
> +        .flags = 0,
> +    };
> +
> +    if (unlikely(!vq->vring.desc)) {
> +        return;
> +    }
> +
> +    caches = vring_get_region_caches(vq);
> +    head = vq->used_idx + idx;
> +    head = head >= vq->vring.num ? (head - vq->vring.num) : head;
> +    vring_packed_desc_read(vq->vdev, &desc, &caches->desc, head);
> +
> +    w = (desc.flags & AVAIL_DESC_PACKED(1)) >> 7;
> +    desc.flags &= ~(AVAIL_DESC_PACKED(1) | USED_DESC_PACKED(1));
> +    desc.flags |= AVAIL_DESC_PACKED(w) | USED_DESC_PACKED(w);

Why need read descriptor here? We should set the used counter by our own 
(e.g we have used_wrap_counter).

> +    if (!(desc.flags & VRING_DESC_F_INDIRECT)) {
> +        if (!(desc.flags & VRING_DESC_F_WRITE)) {
> +            desc.len = 0;
> +        } else {
> +            desc.len = len;
> +        }
> +    }
> +    vring_packed_desc_write(vq->vdev, &desc, &caches->desc, head);
> +
> +    /* Make sure flags has been updated */
> +    smp_mb();

This looks wrong, and if you think it's correct, you need a better 
comment to explain, e.g how it was paired with other.

Thanks

> +}
> +
> +void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
> +                    unsigned int len, unsigned int idx)
> +{
> +    trace_virtqueue_fill(vq, elem, len, idx);
> +
> +    virtqueue_unmap_sg(vq, elem, len);
>   
>       if (unlikely(vq->vdev->broken)) {
> -        vq->inuse -= count;
>           return;
>       }
>   
> +    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> +        virtqueue_packed_fill(vq, elem, len, idx);
> +    } else {
> +        virtqueue_split_fill(vq, elem, len, idx);
> +    }
> +}
> +
> +/* Called within rcu_read_lock().  */
> +static void virtqueue_split_flush(VirtQueue *vq, unsigned int count)
> +{
> +    uint16_t old, new;
> +
>       if (unlikely(!vq->vring.used)) {
>           return;
>       }
> @@ -575,6 +630,34 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
>           vq->signalled_used_valid = false;
>   }
>   
> +static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
> +{
> +    if (unlikely(!vq->vring.desc)) {
> +        return;
> +    }
> +
> +    vq->inuse -= count;
> +    vq->used_idx += count;
> +    if (vq->used_idx >= vq->vring.num) {
> +        vq->used_idx -= vq->vring.num;
> +        vq->used_wrap_counter = !vq->used_wrap_counter;
> +    }
> +}
> +
> +void virtqueue_flush(VirtQueue *vq, unsigned int count)
> +{
> +    if (unlikely(vq->vdev->broken)) {
> +        vq->inuse -= count;
> +        return;
> +    }
> +
> +    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> +        virtqueue_packed_flush(vq, count);
> +    } else {
> +        virtqueue_split_flush(vq, count);
> +    }
> +}
> +
>   void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>                       unsigned int len)
>   {

Re: [Qemu-devel] [RFC v2 6/8] virtio: flush/push for packed ring

[Maxime Coquelin]

Hi Wei,

On 9/20/18 4:13 PM, Maxime Coquelin wrote:
>>       }
>> @@ -575,6 +630,34 @@ void virtqueue_flush(VirtQueue *vq, unsigned int 
>> count)
>>           vq->signalled_used_valid = false;
>>   }
>> +static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
>> +{
>> +    if (unlikely(!vq->vring.desc)) {
>> +        return;
>> +    }
>> +
>> +    vq->inuse -= count;
>> +    vq->used_idx += count;
> 
> I think it is wrong, because count seems to be representing the number
> of descriptor chains.
> 
> But in case of packed ring layout, used index must be incremented by the
> number of descriptors.
> 
> For example, I'm having trouble with the ctrl queue, where the commands
> sent by the guest use 3 descriptors, but count is 1.

It seems you didn't fixed this issue in your last revision.
I guess it works for virtio-net Kernel driver (with the fix you
proposed) in guest because it is using indirect descs.

But Virtio PMD does not use indirect descs for the control vq.

Thanks,
Maxime

[Qemu-devel] [RFC v2 7/8] virtio: event suppression for packed ring

[wexu]

From: Wei Xu <wexu@redhat.com>

Signed-off-by: Wei Xu <wexu@redhat.com>
Signed-off-by: Wei Xu <wexu@redhat.com>
---
 hw/virtio/virtio.c                             | 115 +++++++++++++++++++++++--
 include/standard-headers/linux/virtio_config.h |  13 +++
 2 files changed, 119 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 6f2da83..4543974 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -226,6 +226,24 @@ static void vring_desc_read(VirtIODevice *vdev, VRingDesc *desc,
     virtio_tswap16s(vdev, &desc->next);
 }
 
+static void vring_packed_event_read(VirtIODevice *vdev,
+                            MemoryRegionCache *cache, VRingPackedDescEvent *e)
+{
+    address_space_read_cached(cache, 0, e, sizeof(*e));
+    virtio_tswap16s(vdev, &e->off_wrap);
+    virtio_tswap16s(vdev, &e->flags);
+}
+
+static void vring_packed_event_write(VirtIODevice *vdev,
+                            MemoryRegionCache *cache, VRingPackedDescEvent *e)
+{
+    virtio_tswap16s(vdev, &e->off_wrap);
+    virtio_tswap16s(vdev, &e->flags);
+    address_space_write_cached(cache, 0, e, sizeof(*e));
+    address_space_cache_invalidate(cache, 0, sizeof(VRingUsedElem));
+}
+
+
 static VRingMemoryRegionCaches *vring_get_region_caches(struct VirtQueue *vq)
 {
     VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
@@ -332,14 +350,8 @@ static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val)
     address_space_cache_invalidate(&caches->used, pa, sizeof(val));
 }
 
-void virtio_queue_set_notification(VirtQueue *vq, int enable)
+static void virtio_queue_set_notification_split(VirtQueue *vq, int enable)
 {
-    vq->notification = enable;
-
-    if (!vq->vring.desc) {
-        return;
-    }
-
     rcu_read_lock();
     if (virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
         vring_set_avail_event(vq, vring_avail_idx(vq));
@@ -355,6 +367,38 @@ void virtio_queue_set_notification(VirtQueue *vq, int enable)
     rcu_read_unlock();
 }
 
+static void virtio_queue_set_notification_packed(VirtQueue *vq, int enable)
+{
+    VRingPackedDescEvent e;
+    VRingMemoryRegionCaches *caches;
+
+    rcu_read_lock();
+    caches  = vring_get_region_caches(vq);
+    vring_packed_event_read(vq->vdev, &caches->device, &e);
+    if (enable) {
+        e.flags = RING_EVENT_FLAGS_ENABLE;
+    } else {
+        e.flags = RING_EVENT_FLAGS_DISABLE;
+    }
+    vring_packed_event_write(vq->vdev, &caches->device, &e);
+    rcu_read_unlock();
+}
+
+void virtio_queue_set_notification(VirtQueue *vq, int enable)
+{
+    vq->notification = enable;
+
+    if (!vq->vring.desc) {
+        return;
+    }
+
+    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
+        virtio_queue_set_notification_packed(vq, enable);
+    } else {
+        virtio_queue_set_notification_split(vq, enable);
+    }
+}
+
 int virtio_queue_ready(VirtQueue *vq)
 {
     return vq->vring.avail != 0;
@@ -2059,8 +2103,7 @@ static void virtio_set_isr(VirtIODevice *vdev, int value)
     }
 }
 
-/* Called within rcu_read_lock().  */
-static bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
+static bool virtio_split_should_notify(VirtIODevice *vdev, VirtQueue *vq)
 {
     uint16_t old, new;
     bool v;
@@ -2083,6 +2126,60 @@ static bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
     return !v || vring_need_event(vring_get_used_event(vq), new, old);
 }
 
+static bool vring_packed_need_event(VirtQueue *vq, uint16_t off_wrap,
+                                    uint16_t new, uint16_t old)
+{
+    bool wrap = vq->used_wrap_counter;
+    int off = off_wrap & ~(1 << 15);
+
+    if (new < old) {
+        new += vq->vring.num;
+        wrap ^= 1;
+    }
+
+    if (wrap != off_wrap >> 15) {
+        off += vq->vring.num;
+    }
+
+    return vring_need_event(off, new, old);
+}
+
+static bool virtio_packed_should_notify(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VRingPackedDescEvent e;
+    uint16_t old, new;
+    bool v;
+    VRingMemoryRegionCaches *caches;
+
+    caches  = vring_get_region_caches(vq);
+    vring_packed_event_read(vdev, &caches->driver, &e);
+
+    /* Make sure we see the updated flags */
+    smp_mb();
+    if (e.flags == RING_EVENT_FLAGS_DISABLE) {
+        return false;
+    } else if (e.flags == RING_EVENT_FLAGS_ENABLE) {
+        return true;
+    }
+
+    v = vq->signalled_used_valid;
+    vq->signalled_used_valid = true;
+    old = vq->signalled_used;
+    new = vq->signalled_used = vq->used_idx;
+
+    return !v || vring_packed_need_event(vq, e.off_wrap, new, old);
+}
+
+/* Called within rcu_read_lock().  */
+static bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
+{
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
+        return virtio_packed_should_notify(vdev, vq);
+    } else {
+        return virtio_split_should_notify(vdev, vq);
+    }
+}
+
 void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq)
 {
     bool should_notify;
diff --git a/include/standard-headers/linux/virtio_config.h b/include/standard-headers/linux/virtio_config.h
index 6ee5529..53e5c83 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -73,4 +73,17 @@
 #define VIRTIO_F_IOMMU_PLATFORM		33
 
 #define VIRTIO_F_RING_PACKED		34
+
+/* Enable events */
+#define RING_EVENT_FLAGS_ENABLE 0x0
+/* Disable events */
+#define RING_EVENT_FLAGS_DISABLE 0x1
+/*
+ *  * Enable events for a specific descriptor
+ *   * (as specified by Descriptor Ring Change Event Offset/Wrap Counter).
+ *    * Only valid if VIRTIO_F_RING_EVENT_IDX has been negotiated.
+ *     */
+#define RING_EVENT_FLAGS_DESC 0x2
+/* The value 0x3 is reserved */
+
 #endif /* _LINUX_VIRTIO_CONFIG_H */
-- 
1.8.3.1

Re: [Qemu-devel] [RFC v2 7/8] virtio: event suppression for packed ring

[Jason Wang]

On 2018年06月06日 03:08, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> Signed-off-by: Wei Xu <wexu@redhat.com>

Duplicated.

> ---
>   hw/virtio/virtio.c                             | 115 +++++++++++++++++++++++--
>   include/standard-headers/linux/virtio_config.h |  13 +++
>   2 files changed, 119 insertions(+), 9 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 6f2da83..4543974 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -226,6 +226,24 @@ static void vring_desc_read(VirtIODevice *vdev, VRingDesc *desc,
>       virtio_tswap16s(vdev, &desc->next);
>   }
>   
> +static void vring_packed_event_read(VirtIODevice *vdev,
> +                            MemoryRegionCache *cache, VRingPackedDescEvent *e)
> +{
> +    address_space_read_cached(cache, 0, e, sizeof(*e));
> +    virtio_tswap16s(vdev, &e->off_wrap);
> +    virtio_tswap16s(vdev, &e->flags);

You need to make sure flags is read before off_wrap.

> +}
> +
> +static void vring_packed_event_write(VirtIODevice *vdev,
> +                            MemoryRegionCache *cache, VRingPackedDescEvent *e)
> +{
> +    virtio_tswap16s(vdev, &e->off_wrap);
> +    virtio_tswap16s(vdev, &e->flags);
> +    address_space_write_cached(cache, 0, e, sizeof(*e));

You need make sure flags were wrote before off_wrap.

> +    address_space_cache_invalidate(cache, 0, sizeof(VRingUsedElem));
> +}
> +
> +
>   static VRingMemoryRegionCaches *vring_get_region_caches(struct VirtQueue *vq)
>   {
>       VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> @@ -332,14 +350,8 @@ static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val)
>       address_space_cache_invalidate(&caches->used, pa, sizeof(val));
>   }
>   
> -void virtio_queue_set_notification(VirtQueue *vq, int enable)
> +static void virtio_queue_set_notification_split(VirtQueue *vq, int enable)
>   {
> -    vq->notification = enable;
> -
> -    if (!vq->vring.desc) {
> -        return;
> -    }
> -
>       rcu_read_lock();
>       if (virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
>           vring_set_avail_event(vq, vring_avail_idx(vq));
> @@ -355,6 +367,38 @@ void virtio_queue_set_notification(VirtQueue *vq, int enable)
>       rcu_read_unlock();
>   }
>   
> +static void virtio_queue_set_notification_packed(VirtQueue *vq, int enable)
> +{
> +    VRingPackedDescEvent e;
> +    VRingMemoryRegionCaches *caches;
> +
> +    rcu_read_lock();
> +    caches  = vring_get_region_caches(vq);
> +    vring_packed_event_read(vq->vdev, &caches->device, &e);

Why need read?

> +    if (enable) {
> +        e.flags = RING_EVENT_FLAGS_ENABLE;
> +    } else {
> +        e.flags = RING_EVENT_FLAGS_DISABLE;
> +    }
> +    vring_packed_event_write(vq->vdev, &caches->device, &e);
> +    rcu_read_unlock();
> +}
> +
> +void virtio_queue_set_notification(VirtQueue *vq, int enable)
> +{
> +    vq->notification = enable;
> +
> +    if (!vq->vring.desc) {
> +        return;
> +    }
> +
> +    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> +        virtio_queue_set_notification_packed(vq, enable);
> +    } else {
> +        virtio_queue_set_notification_split(vq, enable);
> +    }
> +}
> +
>   int virtio_queue_ready(VirtQueue *vq)
>   {
>       return vq->vring.avail != 0;
> @@ -2059,8 +2103,7 @@ static void virtio_set_isr(VirtIODevice *vdev, int value)
>       }
>   }
>   
> -/* Called within rcu_read_lock().  */
> -static bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
> +static bool virtio_split_should_notify(VirtIODevice *vdev, VirtQueue *vq)
>   {
>       uint16_t old, new;
>       bool v;
> @@ -2083,6 +2126,60 @@ static bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
>       return !v || vring_need_event(vring_get_used_event(vq), new, old);
>   }
>   
> +static bool vring_packed_need_event(VirtQueue *vq, uint16_t off_wrap,
> +                                    uint16_t new, uint16_t old)
> +{
> +    bool wrap = vq->used_wrap_counter;
> +    int off = off_wrap & ~(1 << 15);
> +
> +    if (new < old) {
> +        new += vq->vring.num;
> +        wrap ^= 1;
> +    }
> +
> +    if (wrap != off_wrap >> 15) {
> +        off += vq->vring.num;
> +    }
> +
> +    return vring_need_event(off, new, old);
> +}
> +
> +static bool virtio_packed_should_notify(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VRingPackedDescEvent e;
> +    uint16_t old, new;
> +    bool v;
> +    VRingMemoryRegionCaches *caches;
> +
> +    caches  = vring_get_region_caches(vq);
> +    vring_packed_event_read(vdev, &caches->driver, &e);
> +
> +    /* Make sure we see the updated flags */
> +    smp_mb();

Like I mention several times, why need a memory barrier here?

> +    if (e.flags == RING_EVENT_FLAGS_DISABLE) {
> +        return false;
> +    } else if (e.flags == RING_EVENT_FLAGS_ENABLE) {
> +        return true;
> +    }
> +
> +    v = vq->signalled_used_valid;
> +    vq->signalled_used_valid = true;
> +    old = vq->signalled_used;
> +    new = vq->signalled_used = vq->used_idx;
> +
> +    return !v || vring_packed_need_event(vq, e.off_wrap, new, old);
> +}
> +
> +/* Called within rcu_read_lock().  */
> +static bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +        return virtio_packed_should_notify(vdev, vq);
> +    } else {
> +        return virtio_split_should_notify(vdev, vq);
> +    }
> +}
> +
>   void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq)
>   {
>       bool should_notify;
> diff --git a/include/standard-headers/linux/virtio_config.h b/include/standard-headers/linux/virtio_config.h
> index 6ee5529..53e5c83 100644
> --- a/include/standard-headers/linux/virtio_config.h
> +++ b/include/standard-headers/linux/virtio_config.h
> @@ -73,4 +73,17 @@
>   #define VIRTIO_F_IOMMU_PLATFORM		33
>   
>   #define VIRTIO_F_RING_PACKED		34
> +
> +/* Enable events */
> +#define RING_EVENT_FLAGS_ENABLE 0x0
> +/* Disable events */
> +#define RING_EVENT_FLAGS_DISABLE 0x1
> +/*
> + *  * Enable events for a specific descriptor
> + *   * (as specified by Descriptor Ring Change Event Offset/Wrap Counter).
> + *    * Only valid if VIRTIO_F_RING_EVENT_IDX has been negotiated.
> + *     */
> +#define RING_EVENT_FLAGS_DESC 0x2
> +/* The value 0x3 is reserved */
> +
>   #endif /* _LINUX_VIRTIO_CONFIG_H */

This could be done in a patch of header sync.

Thanks

[Qemu-devel] [RFC v2 8/8] virtio: guest driver reload for vhost-net

[wexu]

From: Wei Xu <wexu@redhat.com>

last_avail, avail_wrap_count, used_idx and used_wrap_count are
needed to support vhost-net backend, all these are either 16 or
bool variables, since state.num is 64bit wide, so here it is
possible to put them to the 'num' without introducing a new case
while handling ioctl.

Unload/Reload test has been done successfully with a patch in vhost kernel.

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

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 4543974..153f6d7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2862,33 +2862,59 @@ hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
     }
 }
 
-uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)
+uint64_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)
 {
-    return vdev->vq[n].last_avail_idx;
+    uint64_t num;
+
+    num = vdev->vq[n].last_avail_idx;
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
+        num |= ((uint64_t)vdev->vq[n].avail_wrap_counter) << 16;
+        num |= ((uint64_t)vdev->vq[n].used_idx) << 32;
+        num |= ((uint64_t)vdev->vq[n].used_wrap_counter) << 48;
+    }
+
+    return num;
 }
 
-void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx)
+void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint64_t num)
 {
-    vdev->vq[n].last_avail_idx = idx;
-    vdev->vq[n].shadow_avail_idx = idx;
+    vdev->vq[n].shadow_avail_idx = vdev->vq[n].last_avail_idx = (uint16_t)(num);
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
+        vdev->vq[n].avail_wrap_counter = (uint16_t)(num >> 16);
+        vdev->vq[n].used_idx = (uint16_t)(num >> 32);
+        vdev->vq[n].used_wrap_counter = (uint16_t)(num >> 48);
+    }
 }
 
 void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n)
 {
     rcu_read_lock();
-    if (vdev->vq[n].vring.desc) {
+    if (!vdev->vq[n].vring.desc) {
+        goto out;
+    }
+
+    if (!virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
         vdev->vq[n].last_avail_idx = vring_used_idx(&vdev->vq[n]);
-        vdev->vq[n].shadow_avail_idx = vdev->vq[n].last_avail_idx;
     }
+    vdev->vq[n].shadow_avail_idx = vdev->vq[n].last_avail_idx;
+
+out:
     rcu_read_unlock();
 }
 
 void virtio_queue_update_used_idx(VirtIODevice *vdev, int n)
 {
     rcu_read_lock();
-    if (vdev->vq[n].vring.desc) {
+    if (!vdev->vq[n].vring.desc) {
+        goto out;
+    }
+
+    if (!virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
         vdev->vq[n].used_idx = vring_used_idx(&vdev->vq[n]);
     }
+
+out:
     rcu_read_unlock();
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [RFC v2 8/8] virtio: guest driver reload for vhost-net

[Jason Wang]

On 2018年06月06日 03:08, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> last_avail, avail_wrap_count, used_idx and used_wrap_count are
> needed to support vhost-net backend, all these are either 16 or
> bool variables, since state.num is 64bit wide, so here it is
> possible to put them to the 'num' without introducing a new case
> while handling ioctl.
>
> Unload/Reload test has been done successfully with a patch in vhost kernel.

You need a patch to enable vhost.

And I think you can only do it for vhost-kenrel now since vhost-user 
protocol needs some extension I believe.

>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> ---
>   hw/virtio/virtio.c | 42 ++++++++++++++++++++++++++++++++++--------
>   1 file changed, 34 insertions(+), 8 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 4543974..153f6d7 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2862,33 +2862,59 @@ hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
>       }
>   }
>   
> -uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)
> +uint64_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)
>   {
> -    return vdev->vq[n].last_avail_idx;
> +    uint64_t num;
> +
> +    num = vdev->vq[n].last_avail_idx;
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +        num |= ((uint64_t)vdev->vq[n].avail_wrap_counter) << 16;
> +        num |= ((uint64_t)vdev->vq[n].used_idx) << 32;
> +        num |= ((uint64_t)vdev->vq[n].used_wrap_counter) << 48;

So s.num is 32bit, I don't think this can even work.

Thanks

> +    }
> +
> +    return num;
>   }
>   
> -void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx)
> +void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint64_t num)
>   {
> -    vdev->vq[n].last_avail_idx = idx;
> -    vdev->vq[n].shadow_avail_idx = idx;
> +    vdev->vq[n].shadow_avail_idx = vdev->vq[n].last_avail_idx = (uint16_t)(num);
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +        vdev->vq[n].avail_wrap_counter = (uint16_t)(num >> 16);
> +        vdev->vq[n].used_idx = (uint16_t)(num >> 32);
> +        vdev->vq[n].used_wrap_counter = (uint16_t)(num >> 48);
> +    }
>   }
>   
>   void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n)
>   {
>       rcu_read_lock();
> -    if (vdev->vq[n].vring.desc) {
> +    if (!vdev->vq[n].vring.desc) {
> +        goto out;
> +    }
> +
> +    if (!virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
>           vdev->vq[n].last_avail_idx = vring_used_idx(&vdev->vq[n]);
> -        vdev->vq[n].shadow_avail_idx = vdev->vq[n].last_avail_idx;
>       }
> +    vdev->vq[n].shadow_avail_idx = vdev->vq[n].last_avail_idx;
> +
> +out:
>       rcu_read_unlock();
>   }
>   
>   void virtio_queue_update_used_idx(VirtIODevice *vdev, int n)
>   {
>       rcu_read_lock();
> -    if (vdev->vq[n].vring.desc) {
> +    if (!vdev->vq[n].vring.desc) {
> +        goto out;
> +    }
> +
> +    if (!virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
>           vdev->vq[n].used_idx = vring_used_idx(&vdev->vq[n]);
>       }
> +
> +out:
>       rcu_read_unlock();
>   }
>   

Re: [Qemu-devel] [RFC v2 8/8] virtio: guest driver reload for vhost-net

[Wei Xu]

On Wed, Jun 06, 2018 at 11:48:19AM +0800, Jason Wang wrote:
> 
> 
> On 2018年06月06日 03:08, wexu@redhat.com wrote:
> >From: Wei Xu <wexu@redhat.com>
> >
> >last_avail, avail_wrap_count, used_idx and used_wrap_count are
> >needed to support vhost-net backend, all these are either 16 or
> >bool variables, since state.num is 64bit wide, so here it is
> >possible to put them to the 'num' without introducing a new case
> >while handling ioctl.
> >
> >Unload/Reload test has been done successfully with a patch in vhost kernel.
> 
> You need a patch to enable vhost.
> 
> And I think you can only do it for vhost-kenrel now since vhost-user
> protocol needs some extension I believe.

OK.

> 
> >
> >Signed-off-by: Wei Xu <wexu@redhat.com>
> >---
> >  hw/virtio/virtio.c | 42 ++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 34 insertions(+), 8 deletions(-)
> >
> >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >index 4543974..153f6d7 100644
> >--- a/hw/virtio/virtio.c
> >+++ b/hw/virtio/virtio.c
> >@@ -2862,33 +2862,59 @@ hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
> >      }
> >  }
> >-uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)
> >+uint64_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n)
> >  {
> >-    return vdev->vq[n].last_avail_idx;
> >+    uint64_t num;
> >+
> >+    num = vdev->vq[n].last_avail_idx;
> >+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> >+        num |= ((uint64_t)vdev->vq[n].avail_wrap_counter) << 16;
> >+        num |= ((uint64_t)vdev->vq[n].used_idx) << 32;
> >+        num |= ((uint64_t)vdev->vq[n].used_wrap_counter) << 48;
> 
> So s.num is 32bit, I don't think this can even work.

I mistakenly checked out s.num is 64bit, will add a new case in next version.

Wei

> 
> Thanks
> 
> >+    }
> >+
> >+    return num;
> >  }
> >-void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx)
> >+void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint64_t num)
> >  {
> >-    vdev->vq[n].last_avail_idx = idx;
> >-    vdev->vq[n].shadow_avail_idx = idx;
> >+    vdev->vq[n].shadow_avail_idx = vdev->vq[n].last_avail_idx = (uint16_t)(num);
> >+
> >+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> >+        vdev->vq[n].avail_wrap_counter = (uint16_t)(num >> 16);
> >+        vdev->vq[n].used_idx = (uint16_t)(num >> 32);
> >+        vdev->vq[n].used_wrap_counter = (uint16_t)(num >> 48);
> >+    }
> >  }
> >  void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n)
> >  {
> >      rcu_read_lock();
> >-    if (vdev->vq[n].vring.desc) {
> >+    if (!vdev->vq[n].vring.desc) {
> >+        goto out;
> >+    }
> >+
> >+    if (!virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> >          vdev->vq[n].last_avail_idx = vring_used_idx(&vdev->vq[n]);
> >-        vdev->vq[n].shadow_avail_idx = vdev->vq[n].last_avail_idx;
> >      }
> >+    vdev->vq[n].shadow_avail_idx = vdev->vq[n].last_avail_idx;
> >+
> >+out:
> >      rcu_read_unlock();
> >  }
> >  void virtio_queue_update_used_idx(VirtIODevice *vdev, int n)
> >  {
> >      rcu_read_lock();
> >-    if (vdev->vq[n].vring.desc) {
> >+    if (!vdev->vq[n].vring.desc) {
> >+        goto out;
> >+    }
> >+
> >+    if (!virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> >          vdev->vq[n].used_idx = vring_used_idx(&vdev->vq[n]);
> >      }
> >+
> >+out:
> >      rcu_read_unlock();
> >  }
> 

Re: [Qemu-devel] [RFC v2 0/8] packed ring virtio-net userspace backend support

[Jason Wang]

On 2018年06月06日 03:07, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> Todo:
> - address Rx slow performance
> - event index interrupt suppression test

Tiwei's code support event index, you can try with that.

Actually, unless you disable event_idx explicitly, it work by default.

>
> v1->v2
> - sync to tiwei's v5
> - reuse memory cache function with 1.0
> - dropped detach patch and notification helper(04 & 05 in v1)
> - guest virtio-net driver unload/reload support
> - event suppression support(not tested)
> - addressed feedback from v1
>
> About guest virtio-net load/unload:
> Since last_avail, avail_wrap_count, used_idx and used_wrap_count are
> all 16 or bool type variables, so I turned to merge them to
> 'vhost_vring_state.num' in stead of introducing a new case
> in handling ioctl, test has been done with a tweak in kernel side like:

s.num is unsigned int which is 32bit, but you want to hold 2 16bit vars 
and a boolean.

So I'm afraid it won't work...

>
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1439,10 +1439,16 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
>              r = -EFAULT;
>              break;
>          }
> -               if (s.num > 0xffff) {
> -                       r = -EINVAL;
> -                       break;
> -               }
> +               if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) {
> +                       vq->avail_wrap_counter = (bool)(uint16_t)(s.num >> 16);
> +                       vq->last_used_idx = (uint16_t)(s.num >> 32);
> +                       vq->used_wrap_counter = (bool)(uint16_t)(s.num >> 48);
> +                } else {
> +                    if (s.num > 0xffff) {
> +                            r = -EINVAL;
> +                            break;
> +                    }
> +                }
>          vq->last_avail_idx = s.num;
>          /* Forget the cached index value. */
>          vq->avail_idx = vq->last_avail_idx;
> @@ -1450,8 +1456,15 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
>      case VHOST_GET_VRING_BASE:
>          s.index = idx;
>          s.num = vq->last_avail_idx;
> +               if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) {
> +                       s.num |= vq->avail_wrap_counter << 16;
> +                       s.num |= vq->last_used_idx << 32;
> +                       s.num |= vq->used_wrap_counter << 48;
> +                }
>          if (copy_to_user(argp, &s, sizeof s))
>              r = -EFAULT;
> +               if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED))
> +                       s.num |= vq->avail_wrap_counter << 31;
>          break;
>      case VHOST_SET_VRING_ADDR:
>          if (copy_from_user(&a, argp, sizeof a)) {
>
> Wei Xu (8):
>    virtio: feature bit, data structure, init for packed ring
>    virtio: memory cache for packed ring
>    virtio: empty check and desc read for packed ring
>    virtio: get avail bytes check for packed ring
>    virtio: queue pop support for packed ring
>    virtio: flush/push support for packed ring
>    virtio: event suppression support for packed ring
>    virtio: support guest driver reload for vhost-net
>
>   hw/net/vhost_net.c                             |   2 +
>   hw/virtio/virtio.c                             | 677 +++++++++++++++++++++++--
>   include/hw/virtio/virtio.h                     |   4 +-
>   include/standard-headers/linux/virtio_config.h |  15 +
>   4 files changed, 649 insertions(+), 49 deletions(-)
>

Re: [Qemu-devel] [RFC v2 0/8] packed ring virtio-net userspace backend support

[Jason Wang]

On 2018年06月06日 03:07, wexu@redhat.com wrote:
> From: Wei Xu<wexu@redhat.com>
>
> Todo:
> - address Rx slow performance
> - event index interrupt suppression test

And there's something more need to test:

- vIOMMU support
- migration

Thanks

Re: [Qemu-devel] [RFC v2 0/8] packed ring virtio-net userspace backend support

[Wei Xu]

On Wed, Jun 06, 2018 at 11:49:22AM +0800, Jason Wang wrote:
> 
> 
> On 2018年06月06日 03:07, wexu@redhat.com wrote:
> >From: Wei Xu<wexu@redhat.com>
> >
> >Todo:
> >- address Rx slow performance
> >- event index interrupt suppression test
> 
> And there's something more need to test:
> 
> - vIOMMU support
> - migration

OK, I will test it and put it to next version.

> 
> Thanks