在 2021/3/22 上午11:22, Jason Wang 写道:
在 2021/3/18 下午9:52, Connor Kuehl 写道:
From section 2.6.5.3.1 (Driver Requirements: Indirect Descriptors)
of the virtio spec:
"A driver MUST NOT create a descriptor chain longer than the Queue
Size of the device."
This text suggests that the warning should trigger even if
indirect descriptors are in use.
So I think at least the commit log needs some tweak.
For split virtqueue. We had:
2.6.5.2 Driver Requirements: The Virtqueue Descriptor Table
Drivers MUST NOT add a descriptor chain longer than 2^32 bytes in
total; this implies that loops in the descriptor chain are forbidden!
2.6.5.3.1 Driver Requirements: Indirect Descriptors
A driver MUST NOT create a descriptor chain longer than the Queue Size
of the device.
If I understand the spec correctly, the check is only needed for a
single indirect descriptor table?
For packed virtqueue. We had:
2.7.17 Driver Requirements: Scatter-Gather Support
A driver MUST NOT create a descriptor list longer than allowed by the
device.
A driver MUST NOT create a descriptor list longer than the Queue Size.
2.7.19 Driver Requirements: Indirect Descriptors
A driver MUST NOT create a descriptor chain longer than allowed by the
device.
So it looks to me the packed part is fine.
Note that if I understand the spec correctly 2.7.17 implies 2.7.19.
Actually not. So in 2.7.7, spec said:
"
Some devices benefit by concurrently dispatching a large number of large
requests. The VIRTIO_F_INDIRECT_DESC feature allows this. To increase
ring capacity the driver can store a (read-only by the device) table of
indirect descriptors anywhere in memory, and insert a descriptor in the
main virtqueue (with Flags bit VIRTQ_DESC_F_INDIRECT on) that refers to
a buffer element containing this indirect descriptor table; addr and len
refer to the indirect table address and length in bytes, respectively.
"
And in 2.7.5, spec said
"
While unusual (most implementations either create all lists solely using
non-indirect descriptors, or always use a single indirect element), if
both features have been negotiated, mixing indirect and non-indirect
descriptors in a ring is valid, as long as each list only contains
descriptors of a given type.
"
So my understanding is that the indirect descriptor is used to sumbit
the request whose #buffers is greater than the virtqueue size. And the
spec allows the driver to create a list of indirect descriptors just
need to make sure the number of indirect descriptors in this list must
not exceed the size of the virtqueue (2.7.17). And for each indirector
descriptor, the number of chained descriptor must not exceed the
virtqueue size. So actually this aligns with split virtqueue.
So if I understand the spec correctly, what we need to do is to make
sure the descriptor chained in the indirect descriptor table does not
exceed the virtqueue size. That means we probably need to chain indirect
descriptors instead of a warn here.
Thanks
Thanks
Reported-by: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
Signed-off-by: Connor Kuehl <ckuehl@xxxxxxxxxx>
---
drivers/virtio/virtio_ring.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 71e16b53e9c1..1bc290f9ba13 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -444,11 +444,12 @@ static inline int virtqueue_add_split(struct
virtqueue *_vq,
head = vq->free_head;
+ WARN_ON_ONCE(total_sg > vq->split.vring.num);
+
if (virtqueue_use_indirect(_vq, total_sg))
desc = alloc_indirect_split(_vq, total_sg, gfp);
else {
desc = NULL;
- WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
}
if (desc) {
@@ -1118,6 +1119,8 @@ static inline int virtqueue_add_packed(struct
virtqueue *_vq,
BUG_ON(total_sg == 0);
+ WARN_ON_ONCE(total_sg > vq->packed.vring.num);
+
if (virtqueue_use_indirect(_vq, total_sg))
return virtqueue_add_indirect_packed(vq, sgs, total_sg,
out_sgs, in_sgs, data, gfp);
@@ -1125,8 +1128,6 @@ static inline int virtqueue_add_packed(struct
virtqueue *_vq,
head = vq->packed.next_avail_idx;
avail_used_flags = vq->packed.avail_used_flags;
- WARN_ON_ONCE(total_sg > vq->packed.vring.num && !vq->indirect);
-
desc = vq->packed.vring.desc;
i = head;
descs_used = total_sg;
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
