| Message ID | 20240217192607.32565-2-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libqos, riscv: libqos fixes, add riscv machine | expand |
17.02.2024 22:26, Daniel Henrique Barboza: > 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); > + } > } In my view it would be cleaner to add 2 extra function calls after this loop for the i==elem-1 case: for (i = 0; i < elem - 1; ++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); } /* clear last element */ qvirtio_writeq(d, qs, indirect->desc + (16 * i), 0); qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12, 0); qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, 0); FWIW, -- it's too late to change it now I think. /mjt
On 01/03/2024 16.57, Michael Tokarev wrote: > 17.02.2024 22:26, Daniel Henrique Barboza: > >> 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); >> + } >> } > > In my view it would be cleaner to add 2 extra function calls after this > loop for the i==elem-1 case: > > for (i = 0; i < elem - 1; ++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); > > } > > /* clear last element */ > qvirtio_writeq(d, qs, indirect->desc + (16 * i), 0); > qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12, 0); > qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, 0); > > > FWIW, -- it's too late to change it now I think. Sorry, too late, since the first two patches looked like some good (and riscv-independent) libqos fixes, I went ahead and stuck them into my pull request today. Feel free to send a follow up patch if it bugs you too much. Thomas
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 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> --- tests/qtest/libqos/virtio.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)