Re: [Qemu-devel] [PATCH 1/8] virtio: feature bit, data structure for packed ring

[Wei Xu]

On Tue, Apr 10, 2018 at 03:05:24PM +0800, Jason Wang wrote:
> 
> 
> On 2018年04月04日 20:53, address@hidden wrote:
> >From: Wei Xu <address@hidden>
> >
> >Only minimum definitions from the spec are included
> >for prototype.
> >
> >Signed-off-by: Wei Xu <address@hidden>
> >---
> >  hw/virtio/virtio.c                             | 47 
> > +++++++++++++++++++++++---
> >  include/hw/virtio/virtio.h                     | 12 ++++++-
> >  include/standard-headers/linux/virtio_config.h |  2 ++
> >  3 files changed, 56 insertions(+), 5 deletions(-)
> >
> >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >index 006d3d1..9a6bfe7 100644
> >--- a/hw/virtio/virtio.c
> >+++ b/hw/virtio/virtio.c
> >@@ -39,6 +39,14 @@ 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;
> >@@ -61,9 +69,18 @@ typedef struct VRingUsed
> >  typedef struct VRingMemoryRegionCaches {
> >      struct rcu_head rcu;
> >-    MemoryRegionCache desc;
> >-    MemoryRegionCache avail;
> >-    MemoryRegionCache used;
> >+    union {
> >+        struct {
> >+            MemoryRegionCache desc;
> >+            MemoryRegionCache avail;
> >+            MemoryRegionCache used;
> >+        };
> >+        struct {
> >+            MemoryRegionCache desc_packed;
> >+            MemoryRegionCache driver;
> >+            MemoryRegionCache device;
> >+        };
> >+    };
> 
> I think we can reuse exist memory region caches? Especially consider
> device/driver area could be treated as a renaming of avail/used area.
> 
> E.g desc for desc_packed, avail for driver area and used for device area.

Yes, I will take it.

> 
> >  } VRingMemoryRegionCaches;
> >  typedef struct VRing
> >@@ -77,10 +94,31 @@ typedef struct VRing
> >      VRingMemoryRegionCaches *caches;
> >  } VRing;
> >+typedef struct VRingPackedDescEvent {
> >+    uint16_t desc_event_off:15,
> >+             desc_event_wrap:1;
> >+    uint16_t desc_event_flags:2;
> >+} VRingPackedDescEvent ;
> >+
> >+typedef struct VRingPacked
> >+{
> >+    unsigned int num;
> >+    unsigned int num_default;
> >+    unsigned int align;
> >+    hwaddr desc;
> >+    hwaddr driver;
> >+    hwaddr device;
> >+    VRingMemoryRegionCaches *caches;
> >+} VRingPacked;
> 
> Same here, can we reuse VRing here?

Yes.

> 
> >+
> >  struct VirtQueue
> >  {
> >-    VRing vring;
> >+    union {
> >+        struct VRing vring;
> >+        struct VRingPacked packed;
> >+    };
> >+    uint8_t wrap_counter:1;
> >      /* Next head to pop */
> >      uint16_t last_avail_idx;
> >@@ -1220,6 +1258,7 @@ void virtio_reset(void *opaque)
> >          vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
> >          vdev->vq[i].inuse = 0;
> >          virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
> >+        vdev->vq[i].wrap_counter = 1;
> >      }
> >  }
> >diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >index 098bdaa..563e88e 100644
> >--- a/include/hw/virtio/virtio.h
> >+++ b/include/hw/virtio/virtio.h
> >@@ -46,6 +46,14 @@ typedef struct VirtQueueElement
> >      unsigned int index;
> >      unsigned int out_num;
> >      unsigned int in_num;
> >+
> >+    /* Number of descriptors used by packed ring */
> 
> Do you mean the number of chained descriptors?

These has been removed.

> 
> >+    uint16_t count;
> >+    uint8_t wrap_counter:1;
> 
> What's the use of this bit? If you refer to my v1 vhost code, I used to have
> this, but it won't work for OOO completion e.g when zerocopy is disabled.
> I've dropped it now.
> 
> This is tricky and can only work when device complete descriptors in order.

Same here.

> 
> >+    /* FIXME: Length of every used buffer for a descriptor,
> >+       move to dynamical allocating due to out/in sgs numbers */
> >+    uint32_t len[VIRTQUEUE_MAX_SIZE];
> 
> Can you explain more about this?

Also here.

> 
> >+
> >      hwaddr *in_addr;
> >      hwaddr *out_addr;
> >      struct iovec *in_sg;
> >@@ -262,7 +270,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, true)
> 
> Remember to disable this for old machine types in next version.

Sure, will do.

> 
> Thanks
> 
> >  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 */
>