Re: [PATCH RFC 1/2] virtio: introduce packed ring defines

[Michael S. Tsirkin]

On Tue, Feb 27, 2018 at 09:54:58AM +0100, Jens Freimann wrote:
> On Fri, Feb 23, 2018 at 07:18:00PM +0800, Tiwei Bie wrote:
> > Signed-off-by: Tiwei Bie 
> > ---
> > include/uapi/linux/virtio_config.h | 18 +-
> > include/uapi/linux/virtio_ring.h   | 68 
> > ++
> > 2 files changed, 85 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/uapi/linux/virtio_config.h 
> > b/include/uapi/linux/virtio_config.h
> > index 308e2096291f..e3d077ef5207 100644
> > --- a/include/uapi/linux/virtio_config.h
> > +++ b/include/uapi/linux/virtio_config.h
> > @@ -49,7 +49,7 @@
> >  * transport being used (eg. virtio_ring), the rest are per-device feature
> >  * bits. */
> > #define VIRTIO_TRANSPORT_F_START28
> > -#define VIRTIO_TRANSPORT_F_END 34
> > +#define VIRTIO_TRANSPORT_F_END 37
> > 
> > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > /* Do we get callbacks when the ring is completely used, even if we've
> > @@ -71,4 +71,20 @@
> >  * this is for compatibility with legacy systems.
> >  */
> > #define VIRTIO_F_IOMMU_PLATFORM 33
> > +
> > +/* This feature indicates support for the packed virtqueue layout. */
> > +#define VIRTIO_F_RING_PACKED   34
> 
> Spec says VIRTIO_F_PACKED_RING not RING_PACKED

I changed that now :)

> > +
> > +/*
> > + * This feature indicates that all buffers are used by the device
> > + * in the same order in which they have been made available.
> > + */
> > +#define VIRTIO_F_IN_ORDER  35
> > +
> > +/*
> > + * This feature indicates that drivers pass extra data (besides
> > + * identifying the Virtqueue) in their device notifications.
> > + */
> > +#define VIRTIO_F_NOTIFICATION_DATA 36
> > +
> > #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
> > diff --git a/include/uapi/linux/virtio_ring.h 
> > b/include/uapi/linux/virtio_ring.h
> > index 6d5d5faa989b..77b1d4aeef72 100644
> > --- a/include/uapi/linux/virtio_ring.h
> > +++ b/include/uapi/linux/virtio_ring.h
> > @@ -44,6 +44,9 @@
> > /* This means the buffer contains a list of buffer descriptors. */
> > #define VRING_DESC_F_INDIRECT   4
> > 
> > +#define VRING_DESC_F_AVAIL(b)  ((b) << 7)
> > +#define VRING_DESC_F_USED(b)   ((b) << 15)
> > +
> > /* The Host uses this in used->flags to advise the Guest: don't kick me when
> >  * you add a buffer.  It's unreliable, so it's simply an optimization.  
> > Guest
> >  * will still kick if it's out of buffers. */
> > @@ -104,6 +107,36 @@ struct vring {
> > struct vring_used *used;
> > };
> > 
> > +struct vring_packed_desc_event {
> > +   /* Descriptor Event Offset */
> > +   __virtio16 desc_event_off   : 15,
> > +   /* Descriptor Event Wrap Counter */
> > +  desc_event_wrap  : 1;
> > +   /* Descriptor Event Flags */
> > +   __virtio16 desc_event_flags : 2;
> > +};
> 
> Where would the virtqueue number go in driver notifications?
> 
> regards,
> Jens

Re: [PATCH RFC 1/2] virtio: introduce packed ring defines

[Tiwei Bie]

On Tue, Feb 27, 2018 at 09:54:58AM +0100, Jens Freimann wrote:
> On Fri, Feb 23, 2018 at 07:18:00PM +0800, Tiwei Bie wrote:
[...]
> > 
> > +struct vring_packed_desc_event {
> > +   /* Descriptor Event Offset */
> > +   __virtio16 desc_event_off   : 15,
> > +   /* Descriptor Event Wrap Counter */
> > +  desc_event_wrap  : 1;
> > +   /* Descriptor Event Flags */
> > +   __virtio16 desc_event_flags : 2;
> > +};
> 
> Where would the virtqueue number go in driver notifications?

This structure is for event suppression instead of notification.

You could refer to the "Event Suppression Structure Format"
section of the spec for more details:

https://github.com/oasis-tcs/virtio-docs/blob/master/virtio-v1.1-packed-wd08.pdf

Best regards,
Tiwei Bie

Re: [PATCH RFC 1/2] virtio: introduce packed ring defines

[Tiwei Bie]

On Tue, Feb 27, 2018 at 09:26:27AM +, David Laight wrote:
> From: Tiwei Bie
> > Sent: 23 February 2018 11:18
> ...
> > +struct vring_packed_desc_event {
> > +   /* Descriptor Event Offset */
> > +   __virtio16 desc_event_off   : 15,
> > +   /* Descriptor Event Wrap Counter */
> > +  desc_event_wrap  : 1;
> > +   /* Descriptor Event Flags */
> > +   __virtio16 desc_event_flags : 2;
> > +};
> 
> This looks like you are assuming that a bit-field has a defined
> layout and can be used to map a 'hardware' structure.
> The don't, don't use them like that.
> 
>   David
> 

Thanks for the comments! Above definition isn't used in
this RFC, and the corresponding parts (event suppression)
haven't been implemented yet. It's more like some pseudo
code (I should add some comments about this in the code).

I planned to change it to something like this in the next
version:

struct vring_packed_desc_event {
__virtio16 off_wrap;
__virtio16 flags;  // XXX maybe not a good name for future
}; // extension. Only 2bits are used now.

But it seems that I had a misunderstanding about the spec
on this previously:

https://lists.oasis-open.org/archives/virtio-dev/201802/msg00173.html

Anyway, it will be addressed. Thank you very much! ;-)

Best regards,
Tiwei Bie

RE: [PATCH RFC 1/2] virtio: introduce packed ring defines

[David Laight]

From: Tiwei Bie
> Sent: 23 February 2018 11:18
...
> +struct vring_packed_desc_event {
> + /* Descriptor Event Offset */
> + __virtio16 desc_event_off   : 15,
> + /* Descriptor Event Wrap Counter */
> +desc_event_wrap  : 1;
> + /* Descriptor Event Flags */
> + __virtio16 desc_event_flags : 2;
> +};

This looks like you are assuming that a bit-field has a defined
layout and can be used to map a 'hardware' structure.
The don't, don't use them like that.

David

Re: [PATCH RFC 1/2] virtio: introduce packed ring defines

[Jens Freimann]

On Tue, Feb 27, 2018 at 09:54:58AM +0100, Jens Freimann wrote:

On Fri, Feb 23, 2018 at 07:18:00PM +0800, Tiwei Bie wrote:

Signed-off-by: Tiwei Bie 
---
include/uapi/linux/virtio_config.h | 18 +-
include/uapi/linux/virtio_ring.h   | 68 ++
2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/virtio_config.h 
b/include/uapi/linux/virtio_config.h
index 308e2096291f..e3d077ef5207 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -49,7 +49,7 @@
* transport being used (eg. virtio_ring), the rest are per-device feature
* bits. */
#define VIRTIO_TRANSPORT_F_START28
-#define VIRTIO_TRANSPORT_F_END 34
+#define VIRTIO_TRANSPORT_F_END 37

#ifndef VIRTIO_CONFIG_NO_LEGACY
/* Do we get callbacks when the ring is completely used, even if we've
@@ -71,4 +71,20 @@
* this is for compatibility with legacy systems.
*/
#define VIRTIO_F_IOMMU_PLATFORM 33
+
+/* This feature indicates support for the packed virtqueue layout. */
+#define VIRTIO_F_RING_PACKED   34


Spec says VIRTIO_F_PACKED_RING not RING_PACKED


Ignore this. Seems to have changed.

regards,
Jens 

Re: [PATCH RFC 1/2] virtio: introduce packed ring defines

[Jens Freimann]

On Fri, Feb 23, 2018 at 07:18:00PM +0800, Tiwei Bie wrote:

Signed-off-by: Tiwei Bie 
---
include/uapi/linux/virtio_config.h | 18 +-
include/uapi/linux/virtio_ring.h   | 68 ++
2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/virtio_config.h 
b/include/uapi/linux/virtio_config.h
index 308e2096291f..e3d077ef5207 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -49,7 +49,7 @@
 * transport being used (eg. virtio_ring), the rest are per-device feature
 * bits. */
#define VIRTIO_TRANSPORT_F_START28
-#define VIRTIO_TRANSPORT_F_END 34
+#define VIRTIO_TRANSPORT_F_END 37

#ifndef VIRTIO_CONFIG_NO_LEGACY
/* Do we get callbacks when the ring is completely used, even if we've
@@ -71,4 +71,20 @@
 * this is for compatibility with legacy systems.
 */
#define VIRTIO_F_IOMMU_PLATFORM 33
+
+/* This feature indicates support for the packed virtqueue layout. */
+#define VIRTIO_F_RING_PACKED   34


Spec says VIRTIO_F_PACKED_RING not RING_PACKED

+
+/*
+ * This feature indicates that all buffers are used by the device
+ * in the same order in which they have been made available.
+ */
+#define VIRTIO_F_IN_ORDER  35
+
+/*
+ * This feature indicates that drivers pass extra data (besides
+ * identifying the Virtqueue) in their device notifications.
+ */
+#define VIRTIO_F_NOTIFICATION_DATA 36
+
#endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
index 6d5d5faa989b..77b1d4aeef72 100644
--- a/include/uapi/linux/virtio_ring.h
+++ b/include/uapi/linux/virtio_ring.h
@@ -44,6 +44,9 @@
/* This means the buffer contains a list of buffer descriptors. */
#define VRING_DESC_F_INDIRECT   4

+#define VRING_DESC_F_AVAIL(b)  ((b) << 7)
+#define VRING_DESC_F_USED(b)   ((b) << 15)
+
/* The Host uses this in used->flags to advise the Guest: don't kick me when
 * you add a buffer.  It's unreliable, so it's simply an optimization.  Guest
 * will still kick if it's out of buffers. */
@@ -104,6 +107,36 @@ struct vring {
struct vring_used *used;
};

+struct vring_packed_desc_event {
+   /* Descriptor Event Offset */
+   __virtio16 desc_event_off   : 15,
+   /* Descriptor Event Wrap Counter */
+  desc_event_wrap  : 1;
+   /* Descriptor Event Flags */
+   __virtio16 desc_event_flags : 2;
+};


Where would the virtqueue number go in driver notifications?

regards,
Jens 

[PATCH RFC 1/2] virtio: introduce packed ring defines

[Tiwei Bie]

Signed-off-by: Tiwei Bie 
---
 include/uapi/linux/virtio_config.h | 18 +-
 include/uapi/linux/virtio_ring.h   | 68 ++
 2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/virtio_config.h 
b/include/uapi/linux/virtio_config.h
index 308e2096291f..e3d077ef5207 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -49,7 +49,7 @@
  * transport being used (eg. virtio_ring), the rest are per-device feature
  * bits. */
 #define VIRTIO_TRANSPORT_F_START   28
-#define VIRTIO_TRANSPORT_F_END 34
+#define VIRTIO_TRANSPORT_F_END 37
 
 #ifndef VIRTIO_CONFIG_NO_LEGACY
 /* Do we get callbacks when the ring is completely used, even if we've
@@ -71,4 +71,20 @@
  * this is for compatibility with legacy systems.
  */
 #define VIRTIO_F_IOMMU_PLATFORM33
+
+/* This feature indicates support for the packed virtqueue layout. */
+#define VIRTIO_F_RING_PACKED   34
+
+/*
+ * This feature indicates that all buffers are used by the device
+ * in the same order in which they have been made available.
+ */
+#define VIRTIO_F_IN_ORDER  35
+
+/*
+ * This feature indicates that drivers pass extra data (besides
+ * identifying the Virtqueue) in their device notifications.
+ */
+#define VIRTIO_F_NOTIFICATION_DATA 36
+
 #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
index 6d5d5faa989b..77b1d4aeef72 100644
--- a/include/uapi/linux/virtio_ring.h
+++ b/include/uapi/linux/virtio_ring.h
@@ -44,6 +44,9 @@
 /* This means the buffer contains a list of buffer descriptors. */
 #define VRING_DESC_F_INDIRECT  4
 
+#define VRING_DESC_F_AVAIL(b)  ((b) << 7)
+#define VRING_DESC_F_USED(b)   ((b) << 15)
+
 /* The Host uses this in used->flags to advise the Guest: don't kick me when
  * you add a buffer.  It's unreliable, so it's simply an optimization.  Guest
  * will still kick if it's out of buffers. */
@@ -104,6 +107,36 @@ struct vring {
struct vring_used *used;
 };
 
+struct vring_packed_desc_event {
+   /* Descriptor Event Offset */
+   __virtio16 desc_event_off   : 15,
+   /* Descriptor Event Wrap Counter */
+  desc_event_wrap  : 1;
+   /* Descriptor Event Flags */
+   __virtio16 desc_event_flags : 2;
+};
+
+struct vring_packed_desc {
+   /* Buffer Address. */
+   __virtio64 addr;
+   /* Buffer Length. */
+   __virtio32 len;
+   /* Buffer ID. */
+   __virtio16 id;
+   /* The flags depending on descriptor type. */
+   __virtio16 flags;
+};
+
+struct vring_packed {
+   unsigned int num;
+
+   struct vring_packed_desc *desc;
+
+   struct vring_packed_desc_event *driver;
+
+   struct vring_packed_desc_event *device;
+};
+
 /* Alignment requirements for vring elements.
  * When using pre-virtio 1.0 layout, these fall out naturally.
  */
@@ -171,4 +204,39 @@ static inline int vring_need_event(__u16 event_idx, __u16 
new_idx, __u16 old)
return (__u16)(new_idx - event_idx - 1) < (__u16)(new_idx - old);
 }
 
+/* The standard layout for the packed ring is a continuous chunk of memory
+ * which looks like this.
+ *
+ * struct vring_packed
+ * {
+ * // The actual descriptors (16 bytes each)
+ * struct vring_packed_desc desc[num];
+ *
+ * // Padding to the next align boundary.
+ * char pad[];
+ *
+ * // Driver Event Suppression
+ * struct vring_packed_desc_event driver;
+ *
+ * // Device Event Suppression
+ * struct vring_packed_desc_event device;
+ * };
+ */
+
+static inline void vring_packed_init(struct vring_packed *vr, unsigned int num,
+void *p, unsigned long align)
+{
+   vr->num = num;
+   vr->desc = p;
+   vr->driver = (void *)(((uintptr_t)p + sizeof(struct vring_packed_desc)
+   * num + align - 1) & ~(align - 1));
+   vr->device = vr->driver + 1;
+}
+
+static inline unsigned vring_packed_size(unsigned int num, unsigned long align)
+{
+   return ((sizeof(struct vring_packed_desc) * num + align - 1)
+   & ~(align - 1)) + sizeof(struct vring_packed_desc_event) * 2;
+}
+
 #endif /* _UAPI_LINUX_VIRTIO_RING_H */
-- 
2.14.1

[PATCH RFC 1/2] virtio: introduce packed ring defines

[Jason Wang]

Signed-off-by: Jason Wang 
---
 include/uapi/linux/virtio_config.h |  9 +
 include/uapi/linux/virtio_ring.h   | 17 +
 2 files changed, 26 insertions(+)

diff --git a/include/uapi/linux/virtio_config.h 
b/include/uapi/linux/virtio_config.h
index 308e209..5903d51 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -71,4 +71,13 @@
  * this is for compatibility with legacy systems.
  */
 #define VIRTIO_F_IOMMU_PLATFORM33
+
+#define VIRTIO_F_RING_PACKED   34
+
+/*
+ * This feature indicates that all buffers are used by the device in
+ * the same order in which they have been made available.
+ */
+#define VIRTIO_F_IN_ORDER  35
+
 #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
index 6d5d5fa..a169b53 100644
--- a/include/uapi/linux/virtio_ring.h
+++ b/include/uapi/linux/virtio_ring.h
@@ -43,6 +43,8 @@
 #define VRING_DESC_F_WRITE 2
 /* This means the buffer contains a list of buffer descriptors. */
 #define VRING_DESC_F_INDIRECT  4
+#define VRING_DESC_F_AVAIL  7
+#define VRING_DESC_F_USED  15
 
 /* The Host uses this in used->flags to advise the Guest: don't kick me when
  * you add a buffer.  It's unreliable, so it's simply an optimization.  Guest
@@ -62,6 +64,17 @@
  * at the end of the used ring. Guest should ignore the used->flags field. */
 #define VIRTIO_RING_F_EVENT_IDX29
 
+struct vring_desc_packed {
+   /* Buffer Address. */
+   __virtio64 addr;
+   /* Buffer Length. */
+   __virtio32 len;
+   /* Buffer ID. */
+   __virtio16 id;
+   /* The flags depending on descriptor type. */
+   __virtio16 flags;
+};
+
 /* Virtio ring descriptors: 16 bytes.  These can chain together via "next". */
 struct vring_desc {
/* Address (guest-physical). */
@@ -86,6 +99,10 @@ struct vring_used_elem {
__virtio32 id;
/* Total length of the descriptor chain which was used (written to) */
__virtio32 len;
+   /* Index of the descriptor that needs to write, used by packed ring. */
+   u16 idx;
+   /* Wrap counter for this used desc, used by packed ring. */
+   bool wrap_counter;
 };
 
 struct vring_used {
-- 
2.7.4