diff mbox

[1/8] virtio: feature bit, data structure for packed ring

Message ID 1522846444-31725-2-git-send-email-wexu@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wei Xu April 4, 2018, 12:53 p.m. UTC 
From: Wei Xu <wexu@redhat.com>

Only minimum definitions from the spec are included
for prototype.

Signed-off-by: Wei Xu <wexu@redhat.com>
---
 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(-)

Comments

Jason Wang April 10, 2018, 7:05 a.m. UTC | #1 
On 2018年04月04日 20:53, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> Only minimum definitions from the spec are included
> for prototype.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> ---
>   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.

>   } 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?

> +
>   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?

> +    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.

> +    /* 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?

> +
>       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.

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 */
Wei Xu June 3, 2018, 4:21 p.m. UTC | #2 
On Tue, Apr 10, 2018 at 03:05:24PM +0800, Jason Wang wrote:
> 
> 
> On 2018年04月04日 20:53, wexu@redhat.com wrote:
> >From: Wei Xu <wexu@redhat.com>
> >
> >Only minimum definitions from the spec are included
> >for prototype.
> >
> >Signed-off-by: Wei Xu <wexu@redhat.com>
> >---
> >  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 */
>
diff mbox

Patch

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;
+        };
+    };
 } 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;
+
 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 */
+    uint16_t count;
+    uint8_t wrap_counter:1;
+    /* 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];
+
     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)
 
 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 */