[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/6] libqos/virtio.c: fix 'avail_event' offset in qvring_init
From: |
Alistair Francis |
Subject: |
Re: [PATCH 2/6] libqos/virtio.c: fix 'avail_event' offset in qvring_init() |
Date: |
Thu, 15 Feb 2024 15:06:01 +1000 |
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
>
>
- [PATCH 0/6] libqos, riscv: libqos fixes, add riscv machine, Daniel Henrique Barboza, 2024/02/13
- [PATCH 1/6] libqos/virtio.c: init all elems in qvring_indirect_desc_setup(), Daniel Henrique Barboza, 2024/02/13
- [PATCH 2/6] libqos/virtio.c: fix 'avail_event' offset in qvring_init(), Daniel Henrique Barboza, 2024/02/13
- [PATCH 3/6] hw/riscv/virt.c: create '/soc/pci@...' fdt node earlier, Daniel Henrique Barboza, 2024/02/13
- [PATCH 4/6] hw/riscv/virt.c: add virtio-iommu-pci hotplug support, Daniel Henrique Barboza, 2024/02/13
- [PATCH 5/6] hw/riscv/virt.c: make aclint compatible with 'qtest' accel, Daniel Henrique Barboza, 2024/02/13
- [PATCH 6/6] tests/libqos: add riscv/virt machine nodes, Daniel Henrique Barboza, 2024/02/13