Re: [PATCH 2/6] libqos/virtio.c: fix 'avail_event' offset in qvring_init()

[Alistair Francis]

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
>
>