OpenE2K/qemu-e2k
13
4
Fork 0
Code Issues 4 Pull Requests Projects 2 Releases Activity

libqos/virtio.c: init all elems in qvring_indirect_desc_setup()

Browse Source 
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 uninitialized 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, we'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 uninitialized 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 <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-ID: <20240217192607.32565-2-dbarboza@ventanamicro.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
This commit is contained in:
Daniel Henrique Barboza 2024-02-17 16:26:02 -03:00 committed by Thomas Huth
parent c0c6a0e352
commit 2791490de1
1 changed files with 19 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
tests/qtest/libqos/virtio.c
View File

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