| Message ID | 20240213191736.733334-2-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: > > 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 <dbarboza@ventanamicro.com> Reviewed-by: Alistair Francis <alistair.francis@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 > >
On 13/02/2024 20.17, Daniel Henrique Barboza 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. s/unintialized/uninitialized/ > 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(): s/w'll/we'll/ > 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 s/unintialized/uninitialized/ With the typos fixed: Reviewed-by: Thomas Huth <thuth@redhat.com>
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;
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 <dbarboza@ventanamicro.com> --- tests/qtest/libqos/virtio.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)