| Message ID | 20240213191736.733334-3-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | libqos, riscv: libqos fixes, add riscv machine | expand |
On Wed, Feb 14, 2024 at 5:18 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > In qvring_init() we're writing vq->used->avail_event at "vq->used + 2 + > array_size". The struct pointed by vq->used is, from virtio_ring.h > Linux header): > > * // A ring of used descriptor heads with free-running index. > * __virtio16 used_flags; > * __virtio16 used_idx; > * struct vring_used_elem used[num]; > * __virtio16 avail_event_idx; > > So 'flags' is the word right at vq->used. 'idx' is vq->used + 2. We need > to skip 'used_idx' by adding + 2 bytes, and then sum the vector size, to > reach avail_event_idx. An example on how to properly access this field > can be found in qvirtqueue_kick(): > > avail_event = qvirtio_readw(d, qts, vq->used + 4 + > sizeof(struct vring_used_elem) * vq->size); > > This error was detected when enabling the RISC-V 'virt' libqos machine. > The 'idx' test from vhost-user-blk-test.c errors out with a timeout in > qvirtio_wait_used_elem(). The timeout happens because when processing > the first element, 'avail_event' is read in qvirtqueue_kick() as non-zero > because we didn't initialize it properly (and the memory at that point > happened to be non-zero). 'idx' is 0. > > All of this makes this condition fail because "idx - avail_event" will > overflow and be non-zero: > > /* < 1 because we add elements to avail queue one by one */ > if ((flags & VRING_USED_F_NO_NOTIFY) == 0 && > (!vq->event || (uint16_t)(idx-avail_event) < 1)) { > d->bus->virtqueue_kick(d, vq); > } > > As a result the virtqueue is never kicked and we'll timeout waiting for it. > > Fixes: 1053587c3f ("libqos: Added EVENT_IDX support") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > tests/qtest/libqos/virtio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c > index 4f39124eba..82a6e122bf 100644 > --- a/tests/qtest/libqos/virtio.c > +++ b/tests/qtest/libqos/virtio.c > @@ -265,7 +265,7 @@ void qvring_init(QTestState *qts, const QGuestAllocator *alloc, QVirtQueue *vq, > /* vq->used->idx */ > qvirtio_writew(vq->vdev, qts, vq->used + 2, 0); > /* vq->used->avail_event */ > - qvirtio_writew(vq->vdev, qts, vq->used + 2 + > + qvirtio_writew(vq->vdev, qts, vq->used + 4 + > sizeof(struct vring_used_elem) * vq->size, 0); > } > > -- > 2.43.0 > >
On 13/02/2024 20.17, Daniel Henrique Barboza wrote: > In qvring_init() we're writing vq->used->avail_event at "vq->used + 2 + > array_size". The struct pointed by vq->used is, from virtio_ring.h > Linux header): > > * // A ring of used descriptor heads with free-running index. > * __virtio16 used_flags; > * __virtio16 used_idx; > * struct vring_used_elem used[num]; > * __virtio16 avail_event_idx; > > So 'flags' is the word right at vq->used. 'idx' is vq->used + 2. We need > to skip 'used_idx' by adding + 2 bytes, and then sum the vector size, to > reach avail_event_idx. An example on how to properly access this field > can be found in qvirtqueue_kick(): > > avail_event = qvirtio_readw(d, qts, vq->used + 4 + > sizeof(struct vring_used_elem) * vq->size); > > This error was detected when enabling the RISC-V 'virt' libqos machine. > The 'idx' test from vhost-user-blk-test.c errors out with a timeout in > qvirtio_wait_used_elem(). The timeout happens because when processing > the first element, 'avail_event' is read in qvirtqueue_kick() as non-zero > because we didn't initialize it properly (and the memory at that point > happened to be non-zero). 'idx' is 0. > > All of this makes this condition fail because "idx - avail_event" will > overflow and be non-zero: > > /* < 1 because we add elements to avail queue one by one */ > if ((flags & VRING_USED_F_NO_NOTIFY) == 0 && > (!vq->event || (uint16_t)(idx-avail_event) < 1)) { > d->bus->virtqueue_kick(d, vq); > } > > As a result the virtqueue is never kicked and we'll timeout waiting for it. > > Fixes: 1053587c3f ("libqos: Added EVENT_IDX support") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > tests/qtest/libqos/virtio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c > index 4f39124eba..82a6e122bf 100644 > --- a/tests/qtest/libqos/virtio.c > +++ b/tests/qtest/libqos/virtio.c > @@ -265,7 +265,7 @@ void qvring_init(QTestState *qts, const QGuestAllocator *alloc, QVirtQueue *vq, > /* vq->used->idx */ > qvirtio_writew(vq->vdev, qts, vq->used + 2, 0); > /* vq->used->avail_event */ > - qvirtio_writew(vq->vdev, qts, vq->used + 2 + > + qvirtio_writew(vq->vdev, qts, vq->used + 4 + > sizeof(struct vring_used_elem) * vq->size, 0); > } > Reviewed-by: Thomas Huth <thuth@redhat.com>
diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c index 4f39124eba..82a6e122bf 100644 --- a/tests/qtest/libqos/virtio.c +++ b/tests/qtest/libqos/virtio.c @@ -265,7 +265,7 @@ void qvring_init(QTestState *qts, const QGuestAllocator *alloc, QVirtQueue *vq, /* vq->used->idx */ qvirtio_writew(vq->vdev, qts, vq->used + 2, 0); /* vq->used->avail_event */ - qvirtio_writew(vq->vdev, qts, vq->used + 2 + + qvirtio_writew(vq->vdev, qts, vq->used + 4 + sizeof(struct vring_used_elem) * vq->size, 0); }
In qvring_init() we're writing vq->used->avail_event at "vq->used + 2 + array_size". The struct pointed by vq->used is, from virtio_ring.h Linux header): * // A ring of used descriptor heads with free-running index. * __virtio16 used_flags; * __virtio16 used_idx; * struct vring_used_elem used[num]; * __virtio16 avail_event_idx; So 'flags' is the word right at vq->used. 'idx' is vq->used + 2. We need to skip 'used_idx' by adding + 2 bytes, and then sum the vector size, to reach avail_event_idx. An example on how to properly access this field can be found in qvirtqueue_kick(): avail_event = qvirtio_readw(d, qts, vq->used + 4 + sizeof(struct vring_used_elem) * vq->size); This error was detected when enabling the RISC-V 'virt' libqos machine. The 'idx' test from vhost-user-blk-test.c errors out with a timeout in qvirtio_wait_used_elem(). The timeout happens because when processing the first element, 'avail_event' is read in qvirtqueue_kick() as non-zero because we didn't initialize it properly (and the memory at that point happened to be non-zero). 'idx' is 0. All of this makes this condition fail because "idx - avail_event" will overflow and be non-zero: /* < 1 because we add elements to avail queue one by one */ if ((flags & VRING_USED_F_NO_NOTIFY) == 0 && (!vq->event || (uint16_t)(idx-avail_event) < 1)) { d->bus->virtqueue_kick(d, vq); } As a result the virtqueue is never kicked and we'll timeout waiting for it. Fixes: 1053587c3f ("libqos: Added EVENT_IDX support") Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- tests/qtest/libqos/virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)