virtio_ring: define flags as shifts consistently

RFC In review Tiwei Bie 2 participants 20 messages Dec 7, 2018 – Dec 9, 2018

Subsystems (3)

Networking Network drivers virtio core

lore

Share & tools

lore

Tiwei Bie

author

2018-12-07 08:48 UTC #0 lore raw

cover virtio_ring: define flags as shifts consistently

This is a follow up of the discussion in this thread:
https://patchwork.ozlabs.org/patch/1001015/#2042353

Tiwei Bie (3):
virtio_ring: define flags as shifts consistently
virtio_ring: add VIRTIO_RING_NO_LEGACY
virtio_ring: use new vring flags

 drivers/virtio/virtio_ring.c     | 100 ++++++++++++++++++-------------
 include/uapi/linux/virtio_ring.h |  61 +++++++++++++------
 2 files changed, 103 insertions(+), 58 deletions(-)

Signature · 1 line

2.17.1

Tiwei Bie

author

2018-12-07 08:48 UTC #1 lore raw

RFC 1/3 virtio_ring: define flags as shifts consistently

Introduce _SPLIT_ and/or _PACKED_ variants for VRING_DESC_F_*, VRING_AVAIL_F_NO_INTERRUPT and VRING_USED_F_NO_NOTIFY. These variants are defined as shifts instead of shifted values for consistency with other _F_ flags.

Signed-off-by Tiwei Bie <[email protected]>

---
 include/uapi/linux/virtio_ring.h | 57 ++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 17 deletions(-)

include/uapi/linux/virtio_ring.h +40 ~~-17~~

index 2414f8af26b3..9b0c0d92ab62 100644
@@ -37,29 +37,52 @@
 #include <linux/types.h>
 #include <linux/virtio_types.h>
 
-/* This marks a buffer as continuing via the next field. */
+/*
+ * Notice: unlike other _F_ flags, below flags are defined as shifted
+ * values instead of shifts for compatibility.
+ */
+/* Same as VRING_SPLIT_DESC_F_NEXT. */
 #define VRING_DESC_F_NEXT	1
-/* This marks a buffer as write-only (otherwise read-only). */
+/* Same as VRING_SPLIT_DESC_F_WRITE. */
 #define VRING_DESC_F_WRITE	2
-/* This means the buffer contains a list of buffer descriptors. */
+/* Same as VRING_SPLIT_DESC_F_INDIRECT. */
 #define VRING_DESC_F_INDIRECT	4
-
-/*
- * Mark a descriptor as available or used in packed ring.
- * Notice: they are defined as shifts instead of shifted values.
- */
-#define VRING_PACKED_DESC_F_AVAIL	7
-#define VRING_PACKED_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
- * will still kick if it's out of buffers. */
+/* Same as VRING_SPLIT_USED_F_NO_NOTIFY. */
 #define VRING_USED_F_NO_NOTIFY	1
-/* The Guest uses this in avail->flags to advise the Host: don't interrupt me
- * when you consume a buffer.  It's unreliable, so it's simply an
- * optimization.  */
+/* Same as VRING_SPLIT_AVAIL_F_NO_INTERRUPT. */
 #define VRING_AVAIL_F_NO_INTERRUPT	1
 
+/* Mark a buffer as continuing via the next field in split ring. */
+#define VRING_SPLIT_DESC_F_NEXT		0
+/* Mark a buffer as write-only (otherwise read-only) in split ring. */
+#define VRING_SPLIT_DESC_F_WRITE	1
+/* Mean the buffer contains a list of buffer descriptors in split ring. */
+#define VRING_SPLIT_DESC_F_INDIRECT	2
+
+/*
+ * The Host uses this in used->flags in split ring 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.
+ */
+#define VRING_SPLIT_USED_F_NO_NOTIFY		0
+/*
+ * The Guest uses this in avail->flags in split ring to advise the Host:
+ * don't interrupt me when you consume a buffer.  It's unreliable, so it's
+ * simply an optimization.
+ */
+#define VRING_SPLIT_AVAIL_F_NO_INTERRUPT	0
+
+/* Mark a buffer as continuing via the next field in packed ring. */
+#define VRING_PACKED_DESC_F_NEXT	0
+/* Mark a buffer as write-only (otherwise read-only) in packed ring. */
+#define VRING_PACKED_DESC_F_WRITE	1
+/* Mean the buffer contains a list of buffer descriptors in packed ring. */
+#define VRING_PACKED_DESC_F_INDIRECT	2
+
+/* Mark a descriptor as available or used in packed ring. */
+#define VRING_PACKED_DESC_F_AVAIL	7
+#define VRING_PACKED_DESC_F_USED	15
+
 /* Enable events in packed ring. */
 #define VRING_PACKED_EVENT_FLAG_ENABLE	0x0
 /* Disable events in packed ring. */

Signature · 1 line

2.17.1

Tiwei Bie

author

2018-12-07 08:48 UTC #2 lore raw

RFC 1/3 virtio_ring: define flags as shifts consistently

In reply to Tiwei Bie

Signed-off-by Tiwei Bie <[email protected]>

---
 include/uapi/linux/virtio_ring.h | 57 ++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 17 deletions(-)

include/uapi/linux/virtio_ring.h +40 ~~-17~~

index 2414f8af26b3..9b0c0d92ab62 100644
@@ -37,29 +37,52 @@
 #include <linux/types.h>
 #include <linux/virtio_types.h>
 
-/* This marks a buffer as continuing via the next field. */
+/*
+ * Notice: unlike other _F_ flags, below flags are defined as shifted
+ * values instead of shifts for compatibility.
+ */
+/* Same as VRING_SPLIT_DESC_F_NEXT. */
 #define VRING_DESC_F_NEXT	1
-/* This marks a buffer as write-only (otherwise read-only). */
+/* Same as VRING_SPLIT_DESC_F_WRITE. */
 #define VRING_DESC_F_WRITE	2
-/* This means the buffer contains a list of buffer descriptors. */
+/* Same as VRING_SPLIT_DESC_F_INDIRECT. */
 #define VRING_DESC_F_INDIRECT	4
-
-/*
- * Mark a descriptor as available or used in packed ring.
- * Notice: they are defined as shifts instead of shifted values.
- */
-#define VRING_PACKED_DESC_F_AVAIL	7
-#define VRING_PACKED_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
- * will still kick if it's out of buffers. */
+/* Same as VRING_SPLIT_USED_F_NO_NOTIFY. */
 #define VRING_USED_F_NO_NOTIFY	1
-/* The Guest uses this in avail->flags to advise the Host: don't interrupt me
- * when you consume a buffer.  It's unreliable, so it's simply an
- * optimization.  */
+/* Same as VRING_SPLIT_AVAIL_F_NO_INTERRUPT. */
 #define VRING_AVAIL_F_NO_INTERRUPT	1
 
+/* Mark a buffer as continuing via the next field in split ring. */
+#define VRING_SPLIT_DESC_F_NEXT		0
+/* Mark a buffer as write-only (otherwise read-only) in split ring. */
+#define VRING_SPLIT_DESC_F_WRITE	1
+/* Mean the buffer contains a list of buffer descriptors in split ring. */
+#define VRING_SPLIT_DESC_F_INDIRECT	2
+
+/*
+ * The Host uses this in used->flags in split ring 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.
+ */
+#define VRING_SPLIT_USED_F_NO_NOTIFY		0
+/*
+ * The Guest uses this in avail->flags in split ring to advise the Host:
+ * don't interrupt me when you consume a buffer.  It's unreliable, so it's
+ * simply an optimization.
+ */
+#define VRING_SPLIT_AVAIL_F_NO_INTERRUPT	0
+
+/* Mark a buffer as continuing via the next field in packed ring. */
+#define VRING_PACKED_DESC_F_NEXT	0
+/* Mark a buffer as write-only (otherwise read-only) in packed ring. */
+#define VRING_PACKED_DESC_F_WRITE	1
+/* Mean the buffer contains a list of buffer descriptors in packed ring. */
+#define VRING_PACKED_DESC_F_INDIRECT	2
+
+/* Mark a descriptor as available or used in packed ring. */
+#define VRING_PACKED_DESC_F_AVAIL	7
+#define VRING_PACKED_DESC_F_USED	15
+
 /* Enable events in packed ring. */
 #define VRING_PACKED_EVENT_FLAG_ENABLE	0x0
 /* Disable events in packed ring. */

Signature · 1 line

2.17.1

Tiwei Bie

author

2018-12-07 08:48 UTC #3 lore raw

RFC 2/3 virtio_ring: add VIRTIO_RING_NO_LEGACY

In reply to Tiwei Bie

Introduce VIRTIO_RING_NO_LEGACY to support disabling legacy macros and layout definitions.

Signed-off-by Tiwei Bie <[email protected]>

---
VRING_AVAIL_ALIGN_SIZE, VRING_USED_ALIGN_SIZE and VRING_DESC_ALIGN_SIZE are not pre-virtio 1.0, but can also be disabled by VIRTIO_RING_NO_LEGACY in this patch, because their names are not consistent with other names. Not sure whether this is a good idea. If we want this, we may also want to define _SPLIT_ version for them.

 include/uapi/linux/virtio_ring.h | 4 ++++
 1 file changed, 4 insertions(+)

include/uapi/linux/virtio_ring.h +4 -0

index 9b0c0d92ab62..192573827850 100644
@@ -37,6 +37,7 @@
 #include <linux/types.h>
 #include <linux/virtio_types.h>
 
+#ifndef VIRTIO_RING_NO_LEGACY
 /*
  * Notice: unlike other _F_ flags, below flags are defined as shifted
  * values instead of shifts for compatibility.

@@ -51,6 +52,7 @@
 #define VRING_USED_F_NO_NOTIFY	1
 /* Same as VRING_SPLIT_AVAIL_F_NO_INTERRUPT. */
 #define VRING_AVAIL_F_NO_INTERRUPT	1
+#endif /* VIRTIO_RING_NO_LEGACY */
 
 /* Mark a buffer as continuing via the next field in split ring. */
 #define VRING_SPLIT_DESC_F_NEXT		0

@@ -151,6 +153,7 @@ struct vring {
 	struct vring_used *used;
 };
 
+#ifndef VIRTIO_RING_NO_LEGACY
 /* Alignment requirements for vring elements.
  * When using pre-virtio 1.0 layout, these fall out naturally.
  */

@@ -203,6 +206,7 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
 		 + align - 1) & ~(align - 1))
 		+ sizeof(__virtio16) * 3 + sizeof(struct vring_used_elem) * num;
 }
+#endif /* VIRTIO_RING_NO_LEGACY */
 
 /* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
 /* Assuming a given event_idx value from the other side, if

Signature · 1 line

2.17.1

Michael S. Tsirkin

maintainer

2018-12-07 18:05 UTC #4 lore raw

Re: [RFC 2/3] virtio_ring: add VIRTIO_RING_NO_LEGACY

Quoted 11 lines · Tiwei Bie

Introduce VIRTIO_RING_NO_LEGACY to support disabling legacy macros and layout definitions.
Suggested-by Michael S. Tsirkin <[email protected]>
Signed-off-by Tiwei Bie <[email protected]>
---
VRING_AVAIL_ALIGN_SIZE, VRING_USED_ALIGN_SIZE and VRING_DESC_ALIGN_SIZE are not pre-virtio 1.0, but can also be disabled by VIRTIO_RING_NO_LEGACY in this patch, because their names are not consistent with other names. Not sure whether this is a good idea. If we want this, we may also want to define _SPLIT_ version for them.

I don't think it's a good idea to have alignment in there - the point of NO_LEGACY is to help catch bugs not to sanitize coding style IMHO.

And spec calls "legacy" the 0.X interfaces, let's not muddy the waters.

Quoted 42 lines

 include/uapi/linux/virtio_ring.h | 4 ++++
 1 file changed, 4 insertions(+)

include/uapi/linux/virtio_ring.h +4 -0

index 9b0c0d92ab62..192573827850 100644
@@ -37,6 +37,7 @@
 #include <linux/types.h>
 #include <linux/virtio_types.h>
 
+#ifndef VIRTIO_RING_NO_LEGACY
 /*
  * Notice: unlike other _F_ flags, below flags are defined as shifted
  * values instead of shifts for compatibility.

@@ -51,6 +52,7 @@
 #define VRING_USED_F_NO_NOTIFY	1
 /* Same as VRING_SPLIT_AVAIL_F_NO_INTERRUPT. */
 #define VRING_AVAIL_F_NO_INTERRUPT	1
+#endif /* VIRTIO_RING_NO_LEGACY */
 
 /* Mark a buffer as continuing via the next field in split ring. */
 #define VRING_SPLIT_DESC_F_NEXT		0

@@ -151,6 +153,7 @@ struct vring {
 	struct vring_used *used;
 };
 
+#ifndef VIRTIO_RING_NO_LEGACY
 /* Alignment requirements for vring elements.
  * When using pre-virtio 1.0 layout, these fall out naturally.
  */

@@ -203,6 +206,7 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
 		 + align - 1) & ~(align - 1))
 		+ sizeof(__virtio16) * 3 + sizeof(struct vring_used_elem) * num;
 }
+#endif /* VIRTIO_RING_NO_LEGACY */
 
 /* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
 /* Assuming a given event_idx value from the other side, if

Signature · 1 line

2.17.1

Tiwei Bie

author

2018-12-08 13:33 UTC #5 lore raw

Re: [RFC 2/3] virtio_ring: add VIRTIO_RING_NO_LEGACY

Quoted 13 lines · Michael S. Tsirkin

Quoted 11 lines · Tiwei Bie
Introduce VIRTIO_RING_NO_LEGACY to support disabling legacy macros and layout definitions.
Suggested-by Michael S. Tsirkin <[email protected]>
Signed-off-by Tiwei Bie <[email protected]>
---
VRING_AVAIL_ALIGN_SIZE, VRING_USED_ALIGN_SIZE and VRING_DESC_ALIGN_SIZE are not pre-virtio 1.0, but can also be disabled by VIRTIO_RING_NO_LEGACY in this patch, because their names are not consistent with other names. Not sure whether this is a good idea. If we want this, we may also want to define _SPLIT_ version for them.

Quoted 4 lines

I don't think it's a good idea to have alignment in there - the point of NO_LEGACY is to help catch bugs not to sanitize coding style IMHO.
And spec calls "legacy" the 0.X interfaces, let's not muddy the waters.

Make sense. Thanks!

Quoted 43 lines


 include/uapi/linux/virtio_ring.h | 4 ++++
 1 file changed, 4 insertions(+)

include/uapi/linux/virtio_ring.h +4 -0

index 9b0c0d92ab62..192573827850 100644
@@ -37,6 +37,7 @@
 #include <linux/types.h>
 #include <linux/virtio_types.h>
 
+#ifndef VIRTIO_RING_NO_LEGACY
 /*
  * Notice: unlike other _F_ flags, below flags are defined as shifted
  * values instead of shifts for compatibility.

@@ -51,6 +52,7 @@
 #define VRING_USED_F_NO_NOTIFY	1
 /* Same as VRING_SPLIT_AVAIL_F_NO_INTERRUPT. */
 #define VRING_AVAIL_F_NO_INTERRUPT	1
+#endif /* VIRTIO_RING_NO_LEGACY */
 
 /* Mark a buffer as continuing via the next field in split ring. */
 #define VRING_SPLIT_DESC_F_NEXT		0

@@ -151,6 +153,7 @@ struct vring {
 	struct vring_used *used;
 };
 
+#ifndef VIRTIO_RING_NO_LEGACY
 /* Alignment requirements for vring elements.
  * When using pre-virtio 1.0 layout, these fall out naturally.
  */

@@ -203,6 +206,7 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
 		 + align - 1) & ~(align - 1))
 		+ sizeof(__virtio16) * 3 + sizeof(struct vring_used_elem) * num;
 }
+#endif /* VIRTIO_RING_NO_LEGACY */
 
 /* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
 /* Assuming a given event_idx value from the other side, if

Signature · 1 line

2.17.1

Tiwei Bie

author

2018-12-08 13:33 UTC #6 lore raw

Re: [RFC 2/3] virtio_ring: add VIRTIO_RING_NO_LEGACY

In reply to Michael S. Tsirkin

Quoted 13 lines · Michael S. Tsirkin

Quoted 11 lines · Tiwei Bie
Introduce VIRTIO_RING_NO_LEGACY to support disabling legacy macros and layout definitions.
Suggested-by Michael S. Tsirkin <[email protected]>
Signed-off-by Tiwei Bie <[email protected]>
---
VRING_AVAIL_ALIGN_SIZE, VRING_USED_ALIGN_SIZE and VRING_DESC_ALIGN_SIZE are not pre-virtio 1.0, but can also be disabled by VIRTIO_RING_NO_LEGACY in this patch, because their names are not consistent with other names. Not sure whether this is a good idea. If we want this, we may also want to define _SPLIT_ version for them.

Quoted 4 lines

I don't think it's a good idea to have alignment in there - the point of NO_LEGACY is to help catch bugs not to sanitize coding style IMHO.
And spec calls "legacy" the 0.X interfaces, let's not muddy the waters.

Make sense. Thanks!

Quoted 43 lines


 include/uapi/linux/virtio_ring.h | 4 ++++
 1 file changed, 4 insertions(+)

include/uapi/linux/virtio_ring.h +4 -0

index 9b0c0d92ab62..192573827850 100644
@@ -37,6 +37,7 @@
 #include <linux/types.h>
 #include <linux/virtio_types.h>
 
+#ifndef VIRTIO_RING_NO_LEGACY
 /*
  * Notice: unlike other _F_ flags, below flags are defined as shifted
  * values instead of shifts for compatibility.

@@ -51,6 +52,7 @@
 #define VRING_USED_F_NO_NOTIFY	1
 /* Same as VRING_SPLIT_AVAIL_F_NO_INTERRUPT. */
 #define VRING_AVAIL_F_NO_INTERRUPT	1
+#endif /* VIRTIO_RING_NO_LEGACY */
 
 /* Mark a buffer as continuing via the next field in split ring. */
 #define VRING_SPLIT_DESC_F_NEXT		0

@@ -151,6 +153,7 @@ struct vring {
 	struct vring_used *used;
 };
 
+#ifndef VIRTIO_RING_NO_LEGACY
 /* Alignment requirements for vring elements.
  * When using pre-virtio 1.0 layout, these fall out naturally.
  */

@@ -203,6 +206,7 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
 		 + align - 1) & ~(align - 1))
 		+ sizeof(__virtio16) * 3 + sizeof(struct vring_used_elem) * num;
 }
+#endif /* VIRTIO_RING_NO_LEGACY */
 
 /* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
 /* Assuming a given event_idx value from the other side, if

Signature · 1 line

2.17.1

Michael S. Tsirkin

maintainer

2018-12-07 18:05 UTC #7 lore raw

Re: [RFC 2/3] virtio_ring: add VIRTIO_RING_NO_LEGACY

In reply to Tiwei Bie

Quoted 11 lines · Tiwei Bie

Introduce VIRTIO_RING_NO_LEGACY to support disabling legacy macros and layout definitions.
Suggested-by Michael S. Tsirkin <[email protected]>
Signed-off-by Tiwei Bie <[email protected]>
---
VRING_AVAIL_ALIGN_SIZE, VRING_USED_ALIGN_SIZE and VRING_DESC_ALIGN_SIZE are not pre-virtio 1.0, but can also be disabled by VIRTIO_RING_NO_LEGACY in this patch, because their names are not consistent with other names. Not sure whether this is a good idea. If we want this, we may also want to define _SPLIT_ version for them.

I don't think it's a good idea to have alignment in there - the point of NO_LEGACY is to help catch bugs not to sanitize coding style IMHO.

And spec calls "legacy" the 0.X interfaces, let's not muddy the waters.

Quoted 42 lines

 include/uapi/linux/virtio_ring.h | 4 ++++
 1 file changed, 4 insertions(+)

include/uapi/linux/virtio_ring.h +4 -0

index 9b0c0d92ab62..192573827850 100644
@@ -37,6 +37,7 @@
 #include <linux/types.h>
 #include <linux/virtio_types.h>
 
+#ifndef VIRTIO_RING_NO_LEGACY
 /*
  * Notice: unlike other _F_ flags, below flags are defined as shifted
  * values instead of shifts for compatibility.

@@ -51,6 +52,7 @@
 #define VRING_USED_F_NO_NOTIFY	1
 /* Same as VRING_SPLIT_AVAIL_F_NO_INTERRUPT. */
 #define VRING_AVAIL_F_NO_INTERRUPT	1
+#endif /* VIRTIO_RING_NO_LEGACY */
 
 /* Mark a buffer as continuing via the next field in split ring. */
 #define VRING_SPLIT_DESC_F_NEXT		0

@@ -151,6 +153,7 @@ struct vring {
 	struct vring_used *used;
 };
 
+#ifndef VIRTIO_RING_NO_LEGACY
 /* Alignment requirements for vring elements.
  * When using pre-virtio 1.0 layout, these fall out naturally.
  */

@@ -203,6 +206,7 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
 		 + align - 1) & ~(align - 1))
 		+ sizeof(__virtio16) * 3 + sizeof(struct vring_used_elem) * num;
 }
+#endif /* VIRTIO_RING_NO_LEGACY */
 
 /* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
 /* Assuming a given event_idx value from the other side, if

Signature · 1 line

2.17.1

Tiwei Bie

author

2018-12-07 08:48 UTC #8 lore raw

RFC 2/3 virtio_ring: add VIRTIO_RING_NO_LEGACY

In reply to Tiwei Bie

Introduce VIRTIO_RING_NO_LEGACY to support disabling legacy macros and layout definitions.

Signed-off-by Tiwei Bie <[email protected]>

 include/uapi/linux/virtio_ring.h | 4 ++++
 1 file changed, 4 insertions(+)

include/uapi/linux/virtio_ring.h +4 -0

index 9b0c0d92ab62..192573827850 100644
@@ -37,6 +37,7 @@
 #include <linux/types.h>
 #include <linux/virtio_types.h>
 
+#ifndef VIRTIO_RING_NO_LEGACY
 /*
  * Notice: unlike other _F_ flags, below flags are defined as shifted
  * values instead of shifts for compatibility.

@@ -51,6 +52,7 @@
 #define VRING_USED_F_NO_NOTIFY	1
 /* Same as VRING_SPLIT_AVAIL_F_NO_INTERRUPT. */
 #define VRING_AVAIL_F_NO_INTERRUPT	1
+#endif /* VIRTIO_RING_NO_LEGACY */
 
 /* Mark a buffer as continuing via the next field in split ring. */
 #define VRING_SPLIT_DESC_F_NEXT		0

@@ -151,6 +153,7 @@ struct vring {
 	struct vring_used *used;
 };
 
+#ifndef VIRTIO_RING_NO_LEGACY
 /* Alignment requirements for vring elements.
  * When using pre-virtio 1.0 layout, these fall out naturally.
  */

@@ -203,6 +206,7 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
 		 + align - 1) & ~(align - 1))
 		+ sizeof(__virtio16) * 3 + sizeof(struct vring_used_elem) * num;
 }
+#endif /* VIRTIO_RING_NO_LEGACY */
 
 /* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
 /* Assuming a given event_idx value from the other side, if

Signature · 1 line

2.17.1

Tiwei Bie

author

2018-12-07 08:48 UTC #9 lore raw

RFC 3/3 virtio_ring: use new vring flags

In reply to Tiwei Bie

Switch to using the _SPLIT_ and _PACKED_ variants of vring flags in split ring and packed ring respectively.

Signed-off-by Tiwei Bie <[email protected]>

---
 drivers/virtio/virtio_ring.c | 100 +++++++++++++++++++++--------------
 1 file changed, 59 insertions(+), 41 deletions(-)

drivers/virtio/virtio_ring.c +59 ~~-41~~

index cd7e755484e3..2806f69c6c9f 100644
@@ -371,17 +371,17 @@ static void vring_unmap_one_split(const struct vring_virtqueue *vq,
 
 	flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
 
-	if (flags & VRING_DESC_F_INDIRECT) {
+	if (flags & BIT(VRING_SPLIT_DESC_F_INDIRECT)) {
 		dma_unmap_single(vring_dma_dev(vq),
 				 virtio64_to_cpu(vq->vq.vdev, desc->addr),
 				 virtio32_to_cpu(vq->vq.vdev, desc->len),
-				 (flags & VRING_DESC_F_WRITE) ?
+				 (flags & BIT(VRING_SPLIT_DESC_F_WRITE)) ?
 				 DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	} else {
 		dma_unmap_page(vring_dma_dev(vq),
 			       virtio64_to_cpu(vq->vq.vdev, desc->addr),
 			       virtio32_to_cpu(vq->vq.vdev, desc->len),
-			       (flags & VRING_DESC_F_WRITE) ?
+			       (flags & BIT(VRING_SPLIT_DESC_F_WRITE)) ?
 			       DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	}
 }

@@ -481,7 +481,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 			if (vring_mapping_error(vq, addr))
 				goto unmap_release;
 
-			desc[i].flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT);
+			desc[i].flags = cpu_to_virtio16(_vq->vdev,
+						BIT(VRING_SPLIT_DESC_F_NEXT));
 			desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
 			desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
 			prev = i;

@@ -494,7 +495,9 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 			if (vring_mapping_error(vq, addr))
 				goto unmap_release;
 
-			desc[i].flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT | VRING_DESC_F_WRITE);
+			desc[i].flags = cpu_to_virtio16(_vq->vdev,
+						BIT(VRING_SPLIT_DESC_F_NEXT) |
+						BIT(VRING_SPLIT_DESC_F_WRITE));
 			desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
 			desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
 			prev = i;

@@ -502,7 +505,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 		}
 	}
 	/* Last one doesn't continue. */
-	desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
+	desc[prev].flags &= cpu_to_virtio16(_vq->vdev,
+				(u16)~BIT(VRING_SPLIT_DESC_F_NEXT));
 
 	if (indirect) {
 		/* Now that the indirect table is filled in, map it. */

@@ -513,7 +517,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 			goto unmap_release;
 
 		vq->split.vring.desc[head].flags = cpu_to_virtio16(_vq->vdev,
-				VRING_DESC_F_INDIRECT);
+				BIT(VRING_SPLIT_DESC_F_INDIRECT));
 		vq->split.vring.desc[head].addr = cpu_to_virtio64(_vq->vdev,
 				addr);

@@ -603,8 +607,8 @@ static bool virtqueue_kick_prepare_split(struct virtqueue *_vq)
 					      new, old);
 	} else {
 		needs_kick = !(vq->split.vring.used->flags &
-					cpu_to_virtio16(_vq->vdev,
-						VRING_USED_F_NO_NOTIFY));
+				cpu_to_virtio16(_vq->vdev,
+					BIT(VRING_SPLIT_USED_F_NO_NOTIFY)));
 	}
 	END_USE(vq);
 	return needs_kick;

@@ -614,7 +618,8 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
 			     void **ctx)
 {
 	unsigned int i, j;
-	__virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
+	__virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev,
+				BIT(VRING_SPLIT_DESC_F_NEXT));
 
 	/* Clear data ptr. */
 	vq->split.desc_state[head].data = NULL;

@@ -649,7 +654,8 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
 				vq->split.vring.desc[head].len);
 
 		BUG_ON(!(vq->split.vring.desc[head].flags &
-			 cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT)));
+			 cpu_to_virtio16(vq->vq.vdev,
+				 BIT(VRING_SPLIT_DESC_F_INDIRECT))));
 		BUG_ON(len == 0 || len % sizeof(struct vring_desc));
 
 		for (j = 0; j < len / sizeof(struct vring_desc); j++)

@@ -715,7 +721,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
 	/* If we expect an interrupt for the next entry, tell host
 	 * by writing event index and flush out the write before
 	 * the read in the next get_buf call. */
-	if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
+	if (!(vq->split.avail_flags_shadow &
+			BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT)))
 		virtio_store_mb(vq->weak_barriers,
 				&vring_used_event(&vq->split.vring),
 				cpu_to_virtio16(_vq->vdev, vq->last_used_idx));

@@ -730,8 +737,10 @@ static void virtqueue_disable_cb_split(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
-	if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
-		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
+	if (!(vq->split.avail_flags_shadow &
+			BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT))) {
+		vq->split.avail_flags_shadow |=
+			BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT);
 		if (!vq->event)
 			vq->split.vring.avail->flags =
 				cpu_to_virtio16(_vq->vdev,

@@ -751,8 +760,10 @@ static unsigned virtqueue_enable_cb_prepare_split(struct virtqueue *_vq)
 	/* Depending on the VIRTIO_RING_F_EVENT_IDX feature, we need to
 	 * either clear the flags bit or point the event index at the next
 	 * entry. Always do both to keep code simple. */
-	if (vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
-		vq->split.avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
+	if (vq->split.avail_flags_shadow &
+			BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT)) {
+		vq->split.avail_flags_shadow &=
+			~BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT);
 		if (!vq->event)
 			vq->split.vring.avail->flags =
 				cpu_to_virtio16(_vq->vdev,

@@ -784,8 +795,10 @@ static bool virtqueue_enable_cb_delayed_split(struct virtqueue *_vq)
 	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
 	 * either clear the flags bit or point the event index at the next
 	 * entry. Always update the event index to keep code simple. */
-	if (vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
-		vq->split.avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
+	if (vq->split.avail_flags_shadow &
+			BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT)) {
+		vq->split.avail_flags_shadow &=
+			~BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT);
 		if (!vq->event)
 			vq->split.vring.avail->flags =
 				cpu_to_virtio16(_vq->vdev,

@@ -912,15 +925,15 @@ static void vring_unmap_state_packed(const struct vring_virtqueue *vq,
 
 	flags = state->flags;
 
-	if (flags & VRING_DESC_F_INDIRECT) {
+	if (flags & BIT(VRING_PACKED_DESC_F_INDIRECT)) {
 		dma_unmap_single(vring_dma_dev(vq),
 				 state->addr, state->len,
-				 (flags & VRING_DESC_F_WRITE) ?
+				 (flags & BIT(VRING_PACKED_DESC_F_WRITE)) ?
 				 DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	} else {
 		dma_unmap_page(vring_dma_dev(vq),
 			       state->addr, state->len,
-			       (flags & VRING_DESC_F_WRITE) ?
+			       (flags & BIT(VRING_PACKED_DESC_F_WRITE)) ?
 			       DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	}
 }

@@ -935,17 +948,17 @@ static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
 
 	flags = le16_to_cpu(desc->flags);
 
-	if (flags & VRING_DESC_F_INDIRECT) {
+	if (flags & BIT(VRING_PACKED_DESC_F_INDIRECT)) {
 		dma_unmap_single(vring_dma_dev(vq),
 				 le64_to_cpu(desc->addr),
 				 le32_to_cpu(desc->len),
-				 (flags & VRING_DESC_F_WRITE) ?
+				 (flags & BIT(VRING_PACKED_DESC_F_WRITE)) ?
 				 DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	} else {
 		dma_unmap_page(vring_dma_dev(vq),
 			       le64_to_cpu(desc->addr),
 			       le32_to_cpu(desc->len),
-			       (flags & VRING_DESC_F_WRITE) ?
+			       (flags & BIT(VRING_PACKED_DESC_F_WRITE)) ?
 			       DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	}
 }

@@ -1002,7 +1015,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 				goto unmap_release;
 
 			desc[i].flags = cpu_to_le16(n < out_sgs ?
-						0 : VRING_DESC_F_WRITE);
+					0 : BIT(VRING_PACKED_DESC_F_WRITE));
 			desc[i].addr = cpu_to_le64(addr);
 			desc[i].len = cpu_to_le32(sg->length);
 			i++;

@@ -1025,8 +1038,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 		vq->packed.desc_extra[id].addr = addr;
 		vq->packed.desc_extra[id].len = total_sg *
 				sizeof(struct vring_packed_desc);
-		vq->packed.desc_extra[id].flags = VRING_DESC_F_INDIRECT |
-						  vq->packed.avail_used_flags;
+		vq->packed.desc_extra[id].flags =
+				BIT(VRING_PACKED_DESC_F_INDIRECT) |
+				vq->packed.avail_used_flags;
 	}
 
 	/*

@@ -1035,8 +1049,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 	 * the list are made available.
 	 */
 	virtio_wmb(vq->weak_barriers);
-	vq->packed.vring.desc[head].flags = cpu_to_le16(VRING_DESC_F_INDIRECT |
-						vq->packed.avail_used_flags);
+	vq->packed.vring.desc[head].flags =
+		cpu_to_le16(BIT(VRING_PACKED_DESC_F_INDIRECT) |
+			    vq->packed.avail_used_flags);
 
 	/* We're using some buffers from the free list. */
 	vq->vq.num_free -= 1;

@@ -1047,8 +1062,8 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 		n = 0;
 		vq->packed.avail_wrap_counter ^= 1;
 		vq->packed.avail_used_flags ^=
-				1 << VRING_PACKED_DESC_F_AVAIL |
-				1 << VRING_PACKED_DESC_F_USED;
+				BIT(VRING_PACKED_DESC_F_AVAIL) |
+				BIT(VRING_PACKED_DESC_F_USED);
 	}
 	vq->packed.next_avail_idx = n;
 	vq->free_head = vq->packed.desc_state[id].next;

@@ -1141,8 +1156,10 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 				goto unmap_release;
 
 			flags = cpu_to_le16(vq->packed.avail_used_flags |
-				    (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
-				    (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
+				    (++c == total_sg ? 0 :
+					BIT(VRING_PACKED_DESC_F_NEXT)) |
+				    (n < out_sgs ? 0 :
+					BIT(VRING_PACKED_DESC_F_WRITE)));
 			if (i == head)
 				head_flags = flags;
 			else

@@ -1164,8 +1181,8 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 			if ((unlikely(++i >= vq->packed.vring.num))) {
 				i = 0;
 				vq->packed.avail_used_flags ^=
-					1 << VRING_PACKED_DESC_F_AVAIL |
-					1 << VRING_PACKED_DESC_F_USED;
+					BIT(VRING_PACKED_DESC_F_AVAIL) |
+					BIT(VRING_PACKED_DESC_F_USED);
 			}
 		}
 	}

@@ -1258,7 +1275,7 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
 	off_wrap = le16_to_cpu(snapshot.off_wrap);
 
 	wrap_counter = off_wrap >> VRING_PACKED_EVENT_F_WRAP_CTR;
-	event_idx = off_wrap & ~(1 << VRING_PACKED_EVENT_F_WRAP_CTR);
+	event_idx = off_wrap & ~BIT(VRING_PACKED_EVENT_F_WRAP_CTR);
 	if (wrap_counter != vq->packed.avail_wrap_counter)
 		event_idx -= vq->packed.vring.num;

@@ -1321,8 +1338,8 @@ static inline bool is_used_desc_packed(const struct vring_virtqueue *vq,
 	u16 flags;
 
 	flags = le16_to_cpu(vq->packed.vring.desc[idx].flags);
-	avail = !!(flags & (1 << VRING_PACKED_DESC_F_AVAIL));
-	used = !!(flags & (1 << VRING_PACKED_DESC_F_USED));
+	avail = !!(flags & BIT(VRING_PACKED_DESC_F_AVAIL));
+	used = !!(flags & BIT(VRING_PACKED_DESC_F_USED));
 
 	return avail == used && used == used_wrap_counter;
 }

@@ -1452,7 +1469,7 @@ static bool virtqueue_poll_packed(struct virtqueue *_vq, u16 off_wrap)
 	u16 used_idx;
 
 	wrap_counter = off_wrap >> VRING_PACKED_EVENT_F_WRAP_CTR;
-	used_idx = off_wrap & ~(1 << VRING_PACKED_EVENT_F_WRAP_CTR);
+	used_idx = off_wrap & ~BIT(VRING_PACKED_EVENT_F_WRAP_CTR);
 
 	return is_used_desc_packed(vq, used_idx, wrap_counter);
 }

@@ -1625,7 +1642,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	vq->packed.avail_wrap_counter = 1;
 	vq->packed.used_wrap_counter = 1;
 	vq->packed.event_flags_shadow = 0;
-	vq->packed.avail_used_flags = 1 << VRING_PACKED_DESC_F_AVAIL;
+	vq->packed.avail_used_flags = BIT(VRING_PACKED_DESC_F_AVAIL);
 
 	vq->packed.desc_state = kmalloc_array(num,
 			sizeof(struct vring_desc_state_packed),

@@ -2088,7 +2105,8 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback) {
-		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
+		vq->split.avail_flags_shadow |=
+			BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT);
 		if (!vq->event)
 			vq->split.vring.avail->flags = cpu_to_virtio16(vdev,
 					vq->split.avail_flags_shadow);

Signature · 1 line

2.17.1

Michael S. Tsirkin

maintainer

2018-12-07 18:10 UTC #10 lore raw

Re: [RFC 3/3] virtio_ring: use new vring flags

Quoted 6 lines · Tiwei Bie

Switch to using the _SPLIT_ and _PACKED_ variants of vring flags in split ring and packed ring respectively.
Signed-off-by Tiwei Bie <[email protected]>
---

drivers/virtio/virtio_ring.c static inline int virtqueue_add_split(struct virtqueue *_vq, · line 512

Quoted 9 lines

drivers/virtio/virtio_ring.c +2 -1

@@ -502,7 +505,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 		}
 	}
 	/* Last one doesn't continue. */
-	desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
+	desc[prev].flags &= cpu_to_virtio16(_vq->vdev,
+				(u16)~BIT(VRING_SPLIT_DESC_F_NEXT));
 
 	if (indirect) {
 		/* Now that the indirect table is filled in, map it. */

I kind of dislike it that this forces use of a cast here. Kind of makes it more fragile. Let's use a temporary instead?

Quoted 2 lines

Signature · 1 line
2.17.1

Tiwei Bie

author

2018-12-08 13:47 UTC #11 lore raw

Re: [RFC 3/3] virtio_ring: use new vring flags

Quoted 14 lines · Michael S. Tsirkin

Quoted 13 lines · Tiwei Bie
Switch to using the _SPLIT_ and _PACKED_ variants of vring flags in split ring and packed ring respectively.
Signed-off-by Tiwei Bie <[email protected]>
---
drivers/virtio/virtio_ring.c +2 -1
@@ -502,7 +505,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 		}
 	}
 	/* Last one doesn't continue. */
-	desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
+	desc[prev].flags &= cpu_to_virtio16(_vq->vdev,
+				(u16)~BIT(VRING_SPLIT_DESC_F_NEXT));
 

Quoted 5 lines

drivers/virtio/virtio_ring.c static inline int virtqueue_add_split(struct virtqueue *_vq, · line 512
Quoted 2 lines
drivers/virtio/virtio_ring.c +0 -0
 	if (indirect) {
 		/* Now that the indirect table is filled in, map it. */
I kind of dislike it that this forces use of a cast here. Kind of makes it more fragile. Let's use a temporary instead?

I tried something like this:

u16 mask = ~BIT(VRING_SPLIT_DESC_F_NEXT);

And it will still cause the warning:

warning: large integer implicitly truncated to unsigned type [-Woverflow]
u16 mask = ~BIT(VRING_SPLIT_DESC_F_NEXT);

If the cast isn't wanted, maybe use ~(1 << VRING_SPLIT_DESC_F_NEXT) directly?

Quoted 3 lines

Quoted 3 lines
Signature · 2 lines
-- 
2.17.1

Michael S. Tsirkin

maintainer

2018-12-09 14:33 UTC #12 lore raw

Re: [RFC 3/3] virtio_ring: use new vring flags

Quoted 25 lines · Tiwei Bie

Quoted 14 lines · Michael S. Tsirkin
Quoted 13 lines · Tiwei Bie
Switch to using the _SPLIT_ and _PACKED_ variants of vring flags in split ring and packed ring respectively.
Signed-off-by Tiwei Bie <[email protected]>
---
drivers/virtio/virtio_ring.c +2 -1
@@ -502,7 +505,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 		}
 	}
 	/* Last one doesn't continue. */
-	desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
+	desc[prev].flags &= cpu_to_virtio16(_vq->vdev,
+				(u16)~BIT(VRING_SPLIT_DESC_F_NEXT));
 
Quoted 5 lines
drivers/virtio/virtio_ring.c static inline int virtqueue_add_split(struct virtqueue *_vq, · line 512
Quoted 2 lines
drivers/virtio/virtio_ring.c +0 -0
 	if (indirect) {
 		/* Now that the indirect table is filled in, map it. */
I kind of dislike it that this forces use of a cast here. Kind of makes it more fragile. Let's use a temporary instead?
I tried something like this:
u16 mask = ~BIT(VRING_SPLIT_DESC_F_NEXT);

Quoted 7 lines

And it will still cause the warning:
warning: large integer implicitly truncated to unsigned type [-Woverflow]
u16 mask = ~BIT(VRING_SPLIT_DESC_F_NEXT);
If the cast isn't wanted, maybe use ~(1 << VRING_SPLIT_DESC_F_NEXT) directly?

What I'd like to see is a macro that warns/errors out if the shift is too large. I'll think it over on the weekend.

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Signature · 2 lines
-- 
2.17.1

Michael S. Tsirkin

maintainer

2018-12-09 14:33 UTC #13 lore raw

Re: [RFC 3/3] virtio_ring: use new vring flags

In reply to Tiwei Bie

Quoted 25 lines · Tiwei Bie

Quoted 14 lines · Michael S. Tsirkin
Quoted 13 lines · Tiwei Bie
Switch to using the _SPLIT_ and _PACKED_ variants of vring flags in split ring and packed ring respectively.
Signed-off-by Tiwei Bie <[email protected]>
---
drivers/virtio/virtio_ring.c +2 -1
@@ -502,7 +505,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 		}
 	}
 	/* Last one doesn't continue. */
-	desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
+	desc[prev].flags &= cpu_to_virtio16(_vq->vdev,
+				(u16)~BIT(VRING_SPLIT_DESC_F_NEXT));
 
Quoted 5 lines
drivers/virtio/virtio_ring.c static inline int virtqueue_add_split(struct virtqueue *_vq, · line 512
Quoted 2 lines
drivers/virtio/virtio_ring.c +0 -0
 	if (indirect) {
 		/* Now that the indirect table is filled in, map it. */
I kind of dislike it that this forces use of a cast here. Kind of makes it more fragile. Let's use a temporary instead?
I tried something like this:
u16 mask = ~BIT(VRING_SPLIT_DESC_F_NEXT);

Quoted 7 lines

And it will still cause the warning:
warning: large integer implicitly truncated to unsigned type [-Woverflow]
u16 mask = ~BIT(VRING_SPLIT_DESC_F_NEXT);
If the cast isn't wanted, maybe use ~(1 << VRING_SPLIT_DESC_F_NEXT) directly?

What I'd like to see is a macro that warns/errors out if the shift is too large. I'll think it over on the weekend.

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Signature · 2 lines
-- 
2.17.1

Michael S. Tsirkin

maintainer

2018-12-07 18:10 UTC #14 lore raw

Re: [RFC 3/3] virtio_ring: use new vring flags

In reply to Tiwei Bie

Quoted 6 lines · Tiwei Bie

Switch to using the _SPLIT_ and _PACKED_ variants of vring flags in split ring and packed ring respectively.
Signed-off-by Tiwei Bie <[email protected]>
---

drivers/virtio/virtio_ring.c static inline int virtqueue_add_split(struct virtqueue *_vq, · line 512

Quoted 9 lines

drivers/virtio/virtio_ring.c +2 -1

@@ -502,7 +505,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 		}
 	}
 	/* Last one doesn't continue. */
-	desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
+	desc[prev].flags &= cpu_to_virtio16(_vq->vdev,
+				(u16)~BIT(VRING_SPLIT_DESC_F_NEXT));
 
 	if (indirect) {
 		/* Now that the indirect table is filled in, map it. */

I kind of dislike it that this forces use of a cast here. Kind of makes it more fragile. Let's use a temporary instead?

Quoted 2 lines

Signature · 1 line
2.17.1

Tiwei Bie

author

2018-12-07 08:48 UTC #15 lore raw

RFC 3/3 virtio_ring: use new vring flags

In reply to Tiwei Bie

Switch to using the _SPLIT_ and _PACKED_ variants of vring flags in split ring and packed ring respectively.

Signed-off-by Tiwei Bie <[email protected]>

---
 drivers/virtio/virtio_ring.c | 100 +++++++++++++++++++++--------------
 1 file changed, 59 insertions(+), 41 deletions(-)

drivers/virtio/virtio_ring.c +59 ~~-41~~

index cd7e755484e3..2806f69c6c9f 100644
@@ -371,17 +371,17 @@ static void vring_unmap_one_split(const struct vring_virtqueue *vq,
 
 	flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
 
-	if (flags & VRING_DESC_F_INDIRECT) {
+	if (flags & BIT(VRING_SPLIT_DESC_F_INDIRECT)) {
 		dma_unmap_single(vring_dma_dev(vq),
 				 virtio64_to_cpu(vq->vq.vdev, desc->addr),
 				 virtio32_to_cpu(vq->vq.vdev, desc->len),
-				 (flags & VRING_DESC_F_WRITE) ?
+				 (flags & BIT(VRING_SPLIT_DESC_F_WRITE)) ?
 				 DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	} else {
 		dma_unmap_page(vring_dma_dev(vq),
 			       virtio64_to_cpu(vq->vq.vdev, desc->addr),
 			       virtio32_to_cpu(vq->vq.vdev, desc->len),
-			       (flags & VRING_DESC_F_WRITE) ?
+			       (flags & BIT(VRING_SPLIT_DESC_F_WRITE)) ?
 			       DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	}
 }

@@ -481,7 +481,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 			if (vring_mapping_error(vq, addr))
 				goto unmap_release;
 
-			desc[i].flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT);
+			desc[i].flags = cpu_to_virtio16(_vq->vdev,
+						BIT(VRING_SPLIT_DESC_F_NEXT));
 			desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
 			desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
 			prev = i;

@@ -494,7 +495,9 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 			if (vring_mapping_error(vq, addr))
 				goto unmap_release;
 
-			desc[i].flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT | VRING_DESC_F_WRITE);
+			desc[i].flags = cpu_to_virtio16(_vq->vdev,
+						BIT(VRING_SPLIT_DESC_F_NEXT) |
+						BIT(VRING_SPLIT_DESC_F_WRITE));
 			desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
 			desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
 			prev = i;

@@ -502,7 +505,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 		}
 	}
 	/* Last one doesn't continue. */
-	desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
+	desc[prev].flags &= cpu_to_virtio16(_vq->vdev,
+				(u16)~BIT(VRING_SPLIT_DESC_F_NEXT));
 
 	if (indirect) {
 		/* Now that the indirect table is filled in, map it. */

@@ -513,7 +517,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 			goto unmap_release;
 
 		vq->split.vring.desc[head].flags = cpu_to_virtio16(_vq->vdev,
-				VRING_DESC_F_INDIRECT);
+				BIT(VRING_SPLIT_DESC_F_INDIRECT));
 		vq->split.vring.desc[head].addr = cpu_to_virtio64(_vq->vdev,
 				addr);

@@ -603,8 +607,8 @@ static bool virtqueue_kick_prepare_split(struct virtqueue *_vq)
 					      new, old);
 	} else {
 		needs_kick = !(vq->split.vring.used->flags &
-					cpu_to_virtio16(_vq->vdev,
-						VRING_USED_F_NO_NOTIFY));
+				cpu_to_virtio16(_vq->vdev,
+					BIT(VRING_SPLIT_USED_F_NO_NOTIFY)));
 	}
 	END_USE(vq);
 	return needs_kick;

@@ -614,7 +618,8 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
 			     void **ctx)
 {
 	unsigned int i, j;
-	__virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
+	__virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev,
+				BIT(VRING_SPLIT_DESC_F_NEXT));
 
 	/* Clear data ptr. */
 	vq->split.desc_state[head].data = NULL;

@@ -649,7 +654,8 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
 				vq->split.vring.desc[head].len);
 
 		BUG_ON(!(vq->split.vring.desc[head].flags &
-			 cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT)));
+			 cpu_to_virtio16(vq->vq.vdev,
+				 BIT(VRING_SPLIT_DESC_F_INDIRECT))));
 		BUG_ON(len == 0 || len % sizeof(struct vring_desc));
 
 		for (j = 0; j < len / sizeof(struct vring_desc); j++)

@@ -715,7 +721,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
 	/* If we expect an interrupt for the next entry, tell host
 	 * by writing event index and flush out the write before
 	 * the read in the next get_buf call. */
-	if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
+	if (!(vq->split.avail_flags_shadow &
+			BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT)))
 		virtio_store_mb(vq->weak_barriers,
 				&vring_used_event(&vq->split.vring),
 				cpu_to_virtio16(_vq->vdev, vq->last_used_idx));

@@ -730,8 +737,10 @@ static void virtqueue_disable_cb_split(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
-	if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
-		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
+	if (!(vq->split.avail_flags_shadow &
+			BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT))) {
+		vq->split.avail_flags_shadow |=
+			BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT);
 		if (!vq->event)
 			vq->split.vring.avail->flags =
 				cpu_to_virtio16(_vq->vdev,

@@ -751,8 +760,10 @@ static unsigned virtqueue_enable_cb_prepare_split(struct virtqueue *_vq)
 	/* Depending on the VIRTIO_RING_F_EVENT_IDX feature, we need to
 	 * either clear the flags bit or point the event index at the next
 	 * entry. Always do both to keep code simple. */
-	if (vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
-		vq->split.avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
+	if (vq->split.avail_flags_shadow &
+			BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT)) {
+		vq->split.avail_flags_shadow &=
+			~BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT);
 		if (!vq->event)
 			vq->split.vring.avail->flags =
 				cpu_to_virtio16(_vq->vdev,

@@ -784,8 +795,10 @@ static bool virtqueue_enable_cb_delayed_split(struct virtqueue *_vq)
 	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
 	 * either clear the flags bit or point the event index at the next
 	 * entry. Always update the event index to keep code simple. */
-	if (vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
-		vq->split.avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
+	if (vq->split.avail_flags_shadow &
+			BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT)) {
+		vq->split.avail_flags_shadow &=
+			~BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT);
 		if (!vq->event)
 			vq->split.vring.avail->flags =
 				cpu_to_virtio16(_vq->vdev,

@@ -912,15 +925,15 @@ static void vring_unmap_state_packed(const struct vring_virtqueue *vq,
 
 	flags = state->flags;
 
-	if (flags & VRING_DESC_F_INDIRECT) {
+	if (flags & BIT(VRING_PACKED_DESC_F_INDIRECT)) {
 		dma_unmap_single(vring_dma_dev(vq),
 				 state->addr, state->len,
-				 (flags & VRING_DESC_F_WRITE) ?
+				 (flags & BIT(VRING_PACKED_DESC_F_WRITE)) ?
 				 DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	} else {
 		dma_unmap_page(vring_dma_dev(vq),
 			       state->addr, state->len,
-			       (flags & VRING_DESC_F_WRITE) ?
+			       (flags & BIT(VRING_PACKED_DESC_F_WRITE)) ?
 			       DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	}
 }

@@ -935,17 +948,17 @@ static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
 
 	flags = le16_to_cpu(desc->flags);
 
-	if (flags & VRING_DESC_F_INDIRECT) {
+	if (flags & BIT(VRING_PACKED_DESC_F_INDIRECT)) {
 		dma_unmap_single(vring_dma_dev(vq),
 				 le64_to_cpu(desc->addr),
 				 le32_to_cpu(desc->len),
-				 (flags & VRING_DESC_F_WRITE) ?
+				 (flags & BIT(VRING_PACKED_DESC_F_WRITE)) ?
 				 DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	} else {
 		dma_unmap_page(vring_dma_dev(vq),
 			       le64_to_cpu(desc->addr),
 			       le32_to_cpu(desc->len),
-			       (flags & VRING_DESC_F_WRITE) ?
+			       (flags & BIT(VRING_PACKED_DESC_F_WRITE)) ?
 			       DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	}
 }

@@ -1002,7 +1015,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 				goto unmap_release;
 
 			desc[i].flags = cpu_to_le16(n < out_sgs ?
-						0 : VRING_DESC_F_WRITE);
+					0 : BIT(VRING_PACKED_DESC_F_WRITE));
 			desc[i].addr = cpu_to_le64(addr);
 			desc[i].len = cpu_to_le32(sg->length);
 			i++;

@@ -1025,8 +1038,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 		vq->packed.desc_extra[id].addr = addr;
 		vq->packed.desc_extra[id].len = total_sg *
 				sizeof(struct vring_packed_desc);
-		vq->packed.desc_extra[id].flags = VRING_DESC_F_INDIRECT |
-						  vq->packed.avail_used_flags;
+		vq->packed.desc_extra[id].flags =
+				BIT(VRING_PACKED_DESC_F_INDIRECT) |
+				vq->packed.avail_used_flags;
 	}
 
 	/*

@@ -1035,8 +1049,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 	 * the list are made available.
 	 */
 	virtio_wmb(vq->weak_barriers);
-	vq->packed.vring.desc[head].flags = cpu_to_le16(VRING_DESC_F_INDIRECT |
-						vq->packed.avail_used_flags);
+	vq->packed.vring.desc[head].flags =
+		cpu_to_le16(BIT(VRING_PACKED_DESC_F_INDIRECT) |
+			    vq->packed.avail_used_flags);
 
 	/* We're using some buffers from the free list. */
 	vq->vq.num_free -= 1;

@@ -1047,8 +1062,8 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 		n = 0;
 		vq->packed.avail_wrap_counter ^= 1;
 		vq->packed.avail_used_flags ^=
-				1 << VRING_PACKED_DESC_F_AVAIL |
-				1 << VRING_PACKED_DESC_F_USED;
+				BIT(VRING_PACKED_DESC_F_AVAIL) |
+				BIT(VRING_PACKED_DESC_F_USED);
 	}
 	vq->packed.next_avail_idx = n;
 	vq->free_head = vq->packed.desc_state[id].next;

@@ -1141,8 +1156,10 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 				goto unmap_release;
 
 			flags = cpu_to_le16(vq->packed.avail_used_flags |
-				    (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
-				    (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
+				    (++c == total_sg ? 0 :
+					BIT(VRING_PACKED_DESC_F_NEXT)) |
+				    (n < out_sgs ? 0 :
+					BIT(VRING_PACKED_DESC_F_WRITE)));
 			if (i == head)
 				head_flags = flags;
 			else

@@ -1164,8 +1181,8 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 			if ((unlikely(++i >= vq->packed.vring.num))) {
 				i = 0;
 				vq->packed.avail_used_flags ^=
-					1 << VRING_PACKED_DESC_F_AVAIL |
-					1 << VRING_PACKED_DESC_F_USED;
+					BIT(VRING_PACKED_DESC_F_AVAIL) |
+					BIT(VRING_PACKED_DESC_F_USED);
 			}
 		}
 	}

@@ -1258,7 +1275,7 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
 	off_wrap = le16_to_cpu(snapshot.off_wrap);
 
 	wrap_counter = off_wrap >> VRING_PACKED_EVENT_F_WRAP_CTR;
-	event_idx = off_wrap & ~(1 << VRING_PACKED_EVENT_F_WRAP_CTR);
+	event_idx = off_wrap & ~BIT(VRING_PACKED_EVENT_F_WRAP_CTR);
 	if (wrap_counter != vq->packed.avail_wrap_counter)
 		event_idx -= vq->packed.vring.num;

@@ -1321,8 +1338,8 @@ static inline bool is_used_desc_packed(const struct vring_virtqueue *vq,
 	u16 flags;
 
 	flags = le16_to_cpu(vq->packed.vring.desc[idx].flags);
-	avail = !!(flags & (1 << VRING_PACKED_DESC_F_AVAIL));
-	used = !!(flags & (1 << VRING_PACKED_DESC_F_USED));
+	avail = !!(flags & BIT(VRING_PACKED_DESC_F_AVAIL));
+	used = !!(flags & BIT(VRING_PACKED_DESC_F_USED));
 
 	return avail == used && used == used_wrap_counter;
 }

@@ -1452,7 +1469,7 @@ static bool virtqueue_poll_packed(struct virtqueue *_vq, u16 off_wrap)
 	u16 used_idx;
 
 	wrap_counter = off_wrap >> VRING_PACKED_EVENT_F_WRAP_CTR;
-	used_idx = off_wrap & ~(1 << VRING_PACKED_EVENT_F_WRAP_CTR);
+	used_idx = off_wrap & ~BIT(VRING_PACKED_EVENT_F_WRAP_CTR);
 
 	return is_used_desc_packed(vq, used_idx, wrap_counter);
 }

@@ -1625,7 +1642,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	vq->packed.avail_wrap_counter = 1;
 	vq->packed.used_wrap_counter = 1;
 	vq->packed.event_flags_shadow = 0;
-	vq->packed.avail_used_flags = 1 << VRING_PACKED_DESC_F_AVAIL;
+	vq->packed.avail_used_flags = BIT(VRING_PACKED_DESC_F_AVAIL);
 
 	vq->packed.desc_state = kmalloc_array(num,
 			sizeof(struct vring_desc_state_packed),

@@ -2088,7 +2105,8 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback) {
-		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
+		vq->split.avail_flags_shadow |=
+			BIT(VRING_SPLIT_AVAIL_F_NO_INTERRUPT);
 		if (!vq->event)
 			vq->split.vring.avail->flags = cpu_to_virtio16(vdev,
 					vq->split.avail_flags_shadow);

Signature · 1 line

2.17.1

Michael S. Tsirkin

maintainer

2018-12-07 18:11 UTC #16 lore raw

In reply to Tiwei Bie

Quoted 2 lines · Tiwei Bie

This is a follow up of the discussion in this thread:
https://patchwork.ozlabs.org/patch/1001015/#2042353

How was this tested? I'd suggest building virtio
before and after the changes, stripped binary
should be exactly the same.

Quoted 11 lines

Tiwei Bie (3):
virtio_ring: define flags as shifts consistently
virtio_ring: add VIRTIO_RING_NO_LEGACY
virtio_ring: use new vring flags
 drivers/virtio/virtio_ring.c     | 100 ++++++++++++++++++-------------
 include/uapi/linux/virtio_ring.h |  61 +++++++++++++------
 2 files changed, 103 insertions(+), 58 deletions(-)
Signature · 1 line
2.17.1

Tiwei Bie

author

2018-12-08 13:26 UTC #17 lore raw

Quoted 7 lines · Michael S. Tsirkin

Quoted 2 lines · Tiwei Bie
This is a follow up of the discussion in this thread:
https://patchwork.ozlabs.org/patch/1001015/#2042353
How was this tested? I'd suggest building virtio
before and after the changes, stripped binary
should be exactly the same.

Sure, I will do the test with scripts/objdiff.

Quoted 13 lines

Quoted 13 lines
Tiwei Bie (3):
virtio_ring: define flags as shifts consistently
virtio_ring: add VIRTIO_RING_NO_LEGACY
virtio_ring: use new vring flags
 drivers/virtio/virtio_ring.c     | 100 ++++++++++++++++++-------------
 include/uapi/linux/virtio_ring.h |  61 +++++++++++++------
 2 files changed, 103 insertions(+), 58 deletions(-)
Signature · 1 line
2.17.1

Tiwei Bie

author

2018-12-08 13:26 UTC #18 lore raw

In reply to Michael S. Tsirkin

Quoted 7 lines · Michael S. Tsirkin

Quoted 2 lines · Tiwei Bie
This is a follow up of the discussion in this thread:
https://patchwork.ozlabs.org/patch/1001015/#2042353
How was this tested? I'd suggest building virtio
before and after the changes, stripped binary
should be exactly the same.

Sure, I will do the test with scripts/objdiff.

Quoted 13 lines

Quoted 13 lines
Tiwei Bie (3):
virtio_ring: define flags as shifts consistently
virtio_ring: add VIRTIO_RING_NO_LEGACY
virtio_ring: use new vring flags
 drivers/virtio/virtio_ring.c     | 100 ++++++++++++++++++-------------
 include/uapi/linux/virtio_ring.h |  61 +++++++++++++------
 2 files changed, 103 insertions(+), 58 deletions(-)
Signature · 1 line
2.17.1

Michael S. Tsirkin

maintainer

2018-12-07 18:11 UTC #19 lore raw

In reply to Tiwei Bie

Quoted 2 lines · Tiwei Bie

This is a follow up of the discussion in this thread:
https://patchwork.ozlabs.org/patch/1001015/#2042353

How was this tested? I'd suggest building virtio
before and after the changes, stripped binary
should be exactly the same.

Quoted 11 lines

Tiwei Bie (3):
virtio_ring: define flags as shifts consistently
virtio_ring: add VIRTIO_RING_NO_LEGACY
virtio_ring: use new vring flags
 drivers/virtio/virtio_ring.c     | 100 ++++++++++++++++++-------------
 include/uapi/linux/virtio_ring.h |  61 +++++++++++++------
 2 files changed, 103 insertions(+), 58 deletions(-)
Signature · 1 line
2.17.1