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(-)
--
2.17.1
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.
Suggested-by: Michael S. Tsirkin <[email protected]>
Signed-off-by: Tiwei Bie <[email protected]>
---
include/uapi/linux/virtio_ring.h | 57 ++++++++++++++++++++++----------
1 file changed, 40 insertions(+), 17 deletions(-)
diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
index 2414f8af26b3..9b0c0d92ab62 100644
--- a/include/uapi/linux/virtio_ring.h
+++ b/include/uapi/linux/virtio_ring.h
@@ -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. */
--
2.17.1
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(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index cd7e755484e3..2806f69c6c9f 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -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);
--
2.17.1
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.
include/uapi/linux/virtio_ring.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
index 9b0c0d92ab62..192573827850 100644
--- a/include/uapi/linux/virtio_ring.h
+++ b/include/uapi/linux/virtio_ring.h
@@ -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
--
2.17.1
On Fri, Dec 07, 2018 at 04:48:41PM +0800, Tiwei Bie wrote:
> 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.
>
> include/uapi/linux/virtio_ring.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> index 9b0c0d92ab62..192573827850 100644
> --- a/include/uapi/linux/virtio_ring.h
> +++ b/include/uapi/linux/virtio_ring.h
> @@ -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
> --
> 2.17.1
On Fri, Dec 07, 2018 at 04:48:42PM +0800, Tiwei Bie wrote:
> 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]>
> ---
> @@ -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?
> --
> 2.17.1
On Fri, Dec 07, 2018 at 04:48:39PM +0800, Tiwei Bie wrote:
> 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.
> 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(-)
>
> --
> 2.17.1
On Fri, Dec 07, 2018 at 01:11:42PM -0500, Michael S. Tsirkin wrote:
> On Fri, Dec 07, 2018 at 04:48:39PM +0800, Tiwei Bie wrote:
> > 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.
>
>
> > 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(-)
> >
> > --
> > 2.17.1
On Fri, Dec 07, 2018 at 01:05:35PM -0500, Michael S. Tsirkin wrote:
> On Fri, Dec 07, 2018 at 04:48:41PM +0800, Tiwei Bie wrote:
> > 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.
Make sense. Thanks!
>
> >
> > include/uapi/linux/virtio_ring.h | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> > index 9b0c0d92ab62..192573827850 100644
> > --- a/include/uapi/linux/virtio_ring.h
> > +++ b/include/uapi/linux/virtio_ring.h
> > @@ -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
> > --
> > 2.17.1
On Fri, Dec 07, 2018 at 01:10:48PM -0500, Michael S. Tsirkin wrote:
> On Fri, Dec 07, 2018 at 04:48:42PM +0800, Tiwei Bie wrote:
> > 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]>
> > ---
> > @@ -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?
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?
>
> > --
> > 2.17.1
On Sat, Dec 08, 2018 at 09:47:29PM +0800, Tiwei Bie wrote:
> On Fri, Dec 07, 2018 at 01:10:48PM -0500, Michael S. Tsirkin wrote:
> > On Fri, Dec 07, 2018 at 04:48:42PM +0800, Tiwei Bie wrote:
> > > 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]>
> > > ---
> > > @@ -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?
>
> 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?
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.
>
> >
> > > --
> > > 2.17.1