On Wed, Feb 14, 2024 at 5:18 AM Daniel Henrique Barboza
<dbarb...@ventanamicro.com> wrote:
>
> The loop isn't setting the values for the last element. Every other
> element is being initialized with addr = 0, flags = VRING_DESC_F_NEXT
> and next = i + 1. The last elem is never touched.
>
> This became a problem when enabling a RISC-V 'virt' libqos machine in
> the 'indirect' test of virti-blk-test.c. The 'flags' for the last
> element will end up being an odd number (since we didn't touch it).
> Being an odd number it will be mistaken by VRING_DESC_F_NEXT, which
> happens to be 1.
>
> Deep into hw/virt/virtio.c, in virtqueue_split_pop(), into
> virtqueue_split_read_next_desc(), a check for VRING_DESC_F_NEXT will be
> made to see if we're supposed to chain. The code will keep up chaining
> in the last element because the unintialized value happens to be odd.
> We'll error out right after that because desc->next (which is also
> uninitialized) will be >= max. A VIRTQUEUE_READ_DESC_ERROR will be
> returned, with an error message like this in the stderr:
>
> qemu-system-riscv64: Desc next is 49391
>
> Since we never returned, w'll end up timing out at qvirtio_wait_used_elem():
>
> ERROR:../tests/qtest/libqos/virtio.c:236:qvirtio_wait_used_elem:
> assertion failed: (g_get_monotonic_time() - start_time <= timeout_us)
>
> The root cause is using unintialized values from guest_alloc() in
> qvring_indirect_desc_setup(). There's no guarantee that the memory pages
> retrieved will be zeroed, so we can't make assumptions. In fact, commit
> 5b4f72f5e8 ("tests/qtest: properly initialise the vring used idx") fixed a
> similar problem stating "It is probably not wise to assume guest memory
> is zeroed anyway". I concur.
>
> Initialize all elems in qvring_indirect_desc_setup().
>
> Fixes: f294b029aa ("libqos: Added indirect descriptor support to virtio
> implementation")
> Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.fran...@wdc.com>
Alistair
> ---
> tests/qtest/libqos/virtio.c | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
> index 410513225f..4f39124eba 100644
> --- a/tests/qtest/libqos/virtio.c
> +++ b/tests/qtest/libqos/virtio.c
> @@ -280,14 +280,27 @@ QVRingIndirectDesc
> *qvring_indirect_desc_setup(QTestState *qs, QVirtioDevice *d,
> indirect->elem = elem;
> indirect->desc = guest_alloc(alloc, sizeof(struct vring_desc) * elem);
>
> - for (i = 0; i < elem - 1; ++i) {
> + for (i = 0; i < elem; ++i) {
> /* indirect->desc[i].addr */
> qvirtio_writeq(d, qs, indirect->desc + (16 * i), 0);
> - /* indirect->desc[i].flags */
> - qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12,
> - VRING_DESC_F_NEXT);
> - /* indirect->desc[i].next */
> - qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, i + 1);
> +
> + /*
> + * If it's not the last element of the ring, set
> + * the chain (VRING_DESC_F_NEXT) flag and
> + * desc->next. Clear the last element - there's
> + * no guarantee that guest_alloc() will do it.
> + */
> + if (i != elem - 1) {
> + /* indirect->desc[i].flags */
> + qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12,
> + VRING_DESC_F_NEXT);
> +
> + /* indirect->desc[i].next */
> + qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, i + 1);
> + } else {
> + qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12, 0);
> + qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, 0);
> + }
> }
>
> return indirect;
> --
> 2.43.0
>
>
