| Message ID | 20210825224256.1750286-1-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/virtio: Update vring after modifying its queue size | expand |
On 8/26/21 12:42 AM, Philippe Mathieu-Daudé wrote: > When a ring queue size is modified, we need to call > virtio_queue_update_rings() to re-init the memory region > caches. Otherwise the region might have outdated memory > size, and later load/store access might trigger an > assertion, such: > > qemu-system-i386: include/exec/memory_ldst_cached.h.inc:30: uint16_t address_space_lduw_le_cached(MemoryRegionCache *, hwaddr, MemTxAttrs, MemTxResult *): > Assertion `addr < cache->len && 2 <= cache->len - addr' failed. > Thread 1 "qemu-system-i38" received signal SIGABRT, Aborted. > 0x00007ffff4d312a2 in raise () from /lib64/libc.so.6 > (gdb) bt > #1 0x00007ffff4d1a8a4 in abort () at /lib64/libc.so.6 > #4 0x0000555558f2b8ec in address_space_lduw_le_cached (cache=0x61300010a7c0, addr=134, attrs=..., result=0x0) at include/exec/memory_ldst_cached.h.inc:30 > #5 0x0000555558f2ac6b in lduw_le_phys_cached (cache=0x61300010a7c0, addr=134) at include/exec/memory_ldst_phys.h.inc:67 > #6 0x0000555558f2a3cd in virtio_lduw_phys_cached (vdev=0x62d00003a680, cache=0x61300010a7c0, pa=134) at include/hw/virtio/virtio-access.h:166 > #7 0x0000555558f300ea in vring_avail_ring (vq=0x7fffdd55d8a0, i=65) at hw/virtio/virtio.c:326 > #8 0x0000555558f33b10 in vring_get_used_event (vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:332 > #9 0x0000555558f33677 in virtio_split_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2471 > #10 0x0000555558f1859f in virtio_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2523 > #11 0x0000555558f188cc in virtio_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2565 > #12 0x0000555557c2bd52 in virtio_input_handle_sts (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/input/virtio-input.c:100 > #13 0x0000555558f16df7 in virtio_queue_notify (vdev=0x62d00003a680, n=1) at hw/virtio/virtio.c:2363 > #14 0x00005555583f45c0 in virtio_pci_notify_write (opaque=0x62d000032400, addr=7, val=0, size=1) at hw/virtio/virtio-pci.c:1369 > #15 0x0000555558b80b04 in memory_region_write_accessor (mr=0x62d000033190, addr=7, value=0x7fffffff8eb0, size=1, shift=0, mask=255, attrs=...) at softmmu/memory.c:492 Earlier call which updated vring.num: memory_region_ops_write cpu -1 mr 0x62d000032e60 addr 0xe0004018 value 0x41 size 1 name 'virtio-pci-common-virtio-input' Thread 1 "qemu-system-i38" hit Breakpoint 1, virtio_queue_set_num (vdev=0x62d00003a680, n=1, num=65) at hw/virtio/virtio.c:2276 2276 if (!!num != !!vdev->vq[n].vring.num || (gdb) bt #0 virtio_queue_set_num (vdev=0x62d00003a680, n=1, num=65) at hw/virtio/virtio.c:2276 #1 0x00005555583f2a2c in virtio_pci_common_write (opaque=0x62d000032400, addr=24, val=65, size=1) at hw/virtio/virtio-pci.c:1297 #2 0x0000555558b81b04 in memory_region_write_accessor (mr=0x62d000032e60, addr=24, value=0x7fffffff8eb0, size=1, shift=0, mask=255, attrs=...) at softmmu/memory.c:492 #3 0x0000555558b81442 in access_with_adjusted_size (addr=24, value=0x7fffffff8eb0, size=1, access_size_min=1, access_size_max=4, access_fn= 0x555558b81500 <memory_region_write_accessor>, mr=0x62d000032e60, attrs=...) at softmmu/memory.c:554 #4 0x0000555558b7fd57 in memory_region_dispatch_write (mr=0x62d000032e60, addr=24, data=65, op=MO_8, attrs=...) at softmmu/memory.c:1504 #5 0x0000555558b513f4 in flatview_write_continue (fv=0x60600005cc00, addr=3758112792, attrs=..., ptr=0x6020000dbb90, len=1, addr1=24, l=1, mr=0x62d000032e60) at softmmu/physmem.c:2777 #6 0x0000555558b3fdd0 in flatview_write (fv=0x60600005cc00, addr=3758112792, attrs=..., buf=0x6020000dbb90, len=1) at softmmu/physmem.c:2817 #7 0x0000555558b3f959 in address_space_write (as=0x60800000d920, addr=3758112792, attrs=..., buf=0x6020000dbb90, len=1) at softmmu/physmem.c:2909 > Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com> > Fixes: ab223c9518e ("virtio: allow virtio-1 queue layout") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/302 BTW this also fixes #300 and #301 :) > BugLink: https://bugs.launchpad.net/qemu/+bug/1913510 > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/virtio/virtio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 874377f37a7..04ffe5f420e 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2255,6 +2255,7 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) > return; > } > vdev->vq[n].vring.num = num; > + virtio_queue_update_rings(vdev, n); > } > > VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector) >
On Thu, Aug 26, 2021 at 6:43 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > When a ring queue size is modified, we need to call > virtio_queue_update_rings() to re-init the memory region > caches. Otherwise the region might have outdated memory > size, and later load/store access might trigger an > assertion, such: > > qemu-system-i386: include/exec/memory_ldst_cached.h.inc:30: uint16_t address_space_lduw_le_cached(MemoryRegionCache *, hwaddr, MemTxAttrs, MemTxResult *): > Assertion `addr < cache->len && 2 <= cache->len - addr' failed. > Thread 1 "qemu-system-i38" received signal SIGABRT, Aborted. > 0x00007ffff4d312a2 in raise () from /lib64/libc.so.6 > (gdb) bt > #1 0x00007ffff4d1a8a4 in abort () at /lib64/libc.so.6 > #4 0x0000555558f2b8ec in address_space_lduw_le_cached (cache=0x61300010a7c0, addr=134, attrs=..., result=0x0) at include/exec/memory_ldst_cached.h.inc:30 > #5 0x0000555558f2ac6b in lduw_le_phys_cached (cache=0x61300010a7c0, addr=134) at include/exec/memory_ldst_phys.h.inc:67 > #6 0x0000555558f2a3cd in virtio_lduw_phys_cached (vdev=0x62d00003a680, cache=0x61300010a7c0, pa=134) at include/hw/virtio/virtio-access.h:166 > #7 0x0000555558f300ea in vring_avail_ring (vq=0x7fffdd55d8a0, i=65) at hw/virtio/virtio.c:326 > #8 0x0000555558f33b10 in vring_get_used_event (vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:332 > #9 0x0000555558f33677 in virtio_split_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2471 > #10 0x0000555558f1859f in virtio_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2523 > #11 0x0000555558f188cc in virtio_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2565 > #12 0x0000555557c2bd52 in virtio_input_handle_sts (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/input/virtio-input.c:100 > #13 0x0000555558f16df7 in virtio_queue_notify (vdev=0x62d00003a680, n=1) at hw/virtio/virtio.c:2363 > #14 0x00005555583f45c0 in virtio_pci_notify_write (opaque=0x62d000032400, addr=7, val=0, size=1) at hw/virtio/virtio-pci.c:1369 > #15 0x0000555558b80b04 in memory_region_write_accessor (mr=0x62d000033190, addr=7, value=0x7fffffff8eb0, size=1, shift=0, mask=255, attrs=...) at softmmu/memory.c:492 > > Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com> > Fixes: ab223c9518e ("virtio: allow virtio-1 queue layout") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/302 > BugLink: https://bugs.launchpad.net/qemu/+bug/1913510 > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/virtio/virtio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 874377f37a7..04ffe5f420e 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2255,6 +2255,7 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) > return; > } > vdev->vq[n].vring.num = num; > + virtio_queue_update_rings(vdev, n); > } > Spec said: " The driver MUST configure the other virtqueue fields before enabling the virtqueue with queue_enable. " So I think we should forbid the num to be changed if the virtqueue is ready? Thanks > VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector) > -- > 2.31.1 >
On 8/26/21 5:28 AM, Jason Wang wrote: > On Thu, Aug 26, 2021 at 6:43 AM Philippe Mathieu-Daudé > <philmd@redhat.com> wrote: >> >> When a ring queue size is modified, we need to call >> virtio_queue_update_rings() to re-init the memory region >> caches. Otherwise the region might have outdated memory >> size, and later load/store access might trigger an >> assertion, such: >> >> qemu-system-i386: include/exec/memory_ldst_cached.h.inc:30: uint16_t address_space_lduw_le_cached(MemoryRegionCache *, hwaddr, MemTxAttrs, MemTxResult *): >> Assertion `addr < cache->len && 2 <= cache->len - addr' failed. >> Thread 1 "qemu-system-i38" received signal SIGABRT, Aborted. >> 0x00007ffff4d312a2 in raise () from /lib64/libc.so.6 >> (gdb) bt >> #1 0x00007ffff4d1a8a4 in abort () at /lib64/libc.so.6 >> #4 0x0000555558f2b8ec in address_space_lduw_le_cached (cache=0x61300010a7c0, addr=134, attrs=..., result=0x0) at include/exec/memory_ldst_cached.h.inc:30 >> #5 0x0000555558f2ac6b in lduw_le_phys_cached (cache=0x61300010a7c0, addr=134) at include/exec/memory_ldst_phys.h.inc:67 >> #6 0x0000555558f2a3cd in virtio_lduw_phys_cached (vdev=0x62d00003a680, cache=0x61300010a7c0, pa=134) at include/hw/virtio/virtio-access.h:166 >> #7 0x0000555558f300ea in vring_avail_ring (vq=0x7fffdd55d8a0, i=65) at hw/virtio/virtio.c:326 >> #8 0x0000555558f33b10 in vring_get_used_event (vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:332 >> #9 0x0000555558f33677 in virtio_split_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2471 >> #10 0x0000555558f1859f in virtio_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2523 >> #11 0x0000555558f188cc in virtio_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2565 >> #12 0x0000555557c2bd52 in virtio_input_handle_sts (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/input/virtio-input.c:100 >> #13 0x0000555558f16df7 in virtio_queue_notify (vdev=0x62d00003a680, n=1) at hw/virtio/virtio.c:2363 >> #14 0x00005555583f45c0 in virtio_pci_notify_write (opaque=0x62d000032400, addr=7, val=0, size=1) at hw/virtio/virtio-pci.c:1369 >> #15 0x0000555558b80b04 in memory_region_write_accessor (mr=0x62d000033190, addr=7, value=0x7fffffff8eb0, size=1, shift=0, mask=255, attrs=...) at softmmu/memory.c:492 >> >> Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com> >> Fixes: ab223c9518e ("virtio: allow virtio-1 queue layout") >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/302 >> BugLink: https://bugs.launchpad.net/qemu/+bug/1913510 >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/virtio/virtio.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 874377f37a7..04ffe5f420e 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -2255,6 +2255,7 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) >> return; >> } >> vdev->vq[n].vring.num = num; >> + virtio_queue_update_rings(vdev, n); >> } >> > > Spec said: > > " > The driver MUST configure the other virtqueue fields before enabling > the virtqueue with queue_enable. > " > > So I think we should forbid the num to be changed if the virtqueue is ready? Alright. virtio_pci_common_write() doesn't report errors although.
On 8/26/21 10:40 AM, Philippe Mathieu-Daudé wrote: > On 8/26/21 5:28 AM, Jason Wang wrote: >> On Thu, Aug 26, 2021 at 6:43 AM Philippe Mathieu-Daudé >> <philmd@redhat.com> wrote: >>> >>> When a ring queue size is modified, we need to call >>> virtio_queue_update_rings() to re-init the memory region >>> caches. Otherwise the region might have outdated memory >>> size, and later load/store access might trigger an >>> assertion, such: >>> >>> qemu-system-i386: include/exec/memory_ldst_cached.h.inc:30: uint16_t address_space_lduw_le_cached(MemoryRegionCache *, hwaddr, MemTxAttrs, MemTxResult *): >>> Assertion `addr < cache->len && 2 <= cache->len - addr' failed. >>> Thread 1 "qemu-system-i38" received signal SIGABRT, Aborted. >>> 0x00007ffff4d312a2 in raise () from /lib64/libc.so.6 >>> (gdb) bt >>> #1 0x00007ffff4d1a8a4 in abort () at /lib64/libc.so.6 >>> #4 0x0000555558f2b8ec in address_space_lduw_le_cached (cache=0x61300010a7c0, addr=134, attrs=..., result=0x0) at include/exec/memory_ldst_cached.h.inc:30 >>> #5 0x0000555558f2ac6b in lduw_le_phys_cached (cache=0x61300010a7c0, addr=134) at include/exec/memory_ldst_phys.h.inc:67 >>> #6 0x0000555558f2a3cd in virtio_lduw_phys_cached (vdev=0x62d00003a680, cache=0x61300010a7c0, pa=134) at include/hw/virtio/virtio-access.h:166 >>> #7 0x0000555558f300ea in vring_avail_ring (vq=0x7fffdd55d8a0, i=65) at hw/virtio/virtio.c:326 >>> #8 0x0000555558f33b10 in vring_get_used_event (vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:332 >>> #9 0x0000555558f33677 in virtio_split_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2471 >>> #10 0x0000555558f1859f in virtio_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2523 >>> #11 0x0000555558f188cc in virtio_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2565 >>> #12 0x0000555557c2bd52 in virtio_input_handle_sts (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/input/virtio-input.c:100 >>> #13 0x0000555558f16df7 in virtio_queue_notify (vdev=0x62d00003a680, n=1) at hw/virtio/virtio.c:2363 >>> #14 0x00005555583f45c0 in virtio_pci_notify_write (opaque=0x62d000032400, addr=7, val=0, size=1) at hw/virtio/virtio-pci.c:1369 >>> #15 0x0000555558b80b04 in memory_region_write_accessor (mr=0x62d000033190, addr=7, value=0x7fffffff8eb0, size=1, shift=0, mask=255, attrs=...) at softmmu/memory.c:492 >>> >>> Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com> >>> Fixes: ab223c9518e ("virtio: allow virtio-1 queue layout") >>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/302 >>> BugLink: https://bugs.launchpad.net/qemu/+bug/1913510 >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> hw/virtio/virtio.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>> index 874377f37a7..04ffe5f420e 100644 >>> --- a/hw/virtio/virtio.c >>> +++ b/hw/virtio/virtio.c >>> @@ -2255,6 +2255,7 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) >>> return; >>> } >>> vdev->vq[n].vring.num = num; >>> + virtio_queue_update_rings(vdev, n); >>> } >>> >> >> Spec said: >> >> " >> The driver MUST configure the other virtqueue fields before enabling >> the virtqueue with queue_enable. >> " >> >> So I think we should forbid the num to be changed if the virtqueue is ready? What about virtio_queue_set_addr() and virtio_queue_set_align()?
On Thu, Aug 26, 2021 at 05:16:03PM +0200, Philippe Mathieu-Daudé wrote: > On 8/26/21 10:40 AM, Philippe Mathieu-Daudé wrote: > > On 8/26/21 5:28 AM, Jason Wang wrote: > >> On Thu, Aug 26, 2021 at 6:43 AM Philippe Mathieu-Daudé > >> <philmd@redhat.com> wrote: > >>> > >>> When a ring queue size is modified, we need to call > >>> virtio_queue_update_rings() to re-init the memory region > >>> caches. Otherwise the region might have outdated memory > >>> size, and later load/store access might trigger an > >>> assertion, such: > >>> > >>> qemu-system-i386: include/exec/memory_ldst_cached.h.inc:30: uint16_t address_space_lduw_le_cached(MemoryRegionCache *, hwaddr, MemTxAttrs, MemTxResult *): > >>> Assertion `addr < cache->len && 2 <= cache->len - addr' failed. > >>> Thread 1 "qemu-system-i38" received signal SIGABRT, Aborted. > >>> 0x00007ffff4d312a2 in raise () from /lib64/libc.so.6 > >>> (gdb) bt > >>> #1 0x00007ffff4d1a8a4 in abort () at /lib64/libc.so.6 > >>> #4 0x0000555558f2b8ec in address_space_lduw_le_cached (cache=0x61300010a7c0, addr=134, attrs=..., result=0x0) at include/exec/memory_ldst_cached.h.inc:30 > >>> #5 0x0000555558f2ac6b in lduw_le_phys_cached (cache=0x61300010a7c0, addr=134) at include/exec/memory_ldst_phys.h.inc:67 > >>> #6 0x0000555558f2a3cd in virtio_lduw_phys_cached (vdev=0x62d00003a680, cache=0x61300010a7c0, pa=134) at include/hw/virtio/virtio-access.h:166 > >>> #7 0x0000555558f300ea in vring_avail_ring (vq=0x7fffdd55d8a0, i=65) at hw/virtio/virtio.c:326 > >>> #8 0x0000555558f33b10 in vring_get_used_event (vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:332 > >>> #9 0x0000555558f33677 in virtio_split_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2471 > >>> #10 0x0000555558f1859f in virtio_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2523 > >>> #11 0x0000555558f188cc in virtio_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2565 > >>> #12 0x0000555557c2bd52 in virtio_input_handle_sts (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/input/virtio-input.c:100 > >>> #13 0x0000555558f16df7 in virtio_queue_notify (vdev=0x62d00003a680, n=1) at hw/virtio/virtio.c:2363 > >>> #14 0x00005555583f45c0 in virtio_pci_notify_write (opaque=0x62d000032400, addr=7, val=0, size=1) at hw/virtio/virtio-pci.c:1369 > >>> #15 0x0000555558b80b04 in memory_region_write_accessor (mr=0x62d000033190, addr=7, value=0x7fffffff8eb0, size=1, shift=0, mask=255, attrs=...) at softmmu/memory.c:492 > >>> > >>> Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com> > >>> Fixes: ab223c9518e ("virtio: allow virtio-1 queue layout") > >>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/302 > >>> BugLink: https://bugs.launchpad.net/qemu/+bug/1913510 > >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >>> --- > >>> hw/virtio/virtio.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >>> index 874377f37a7..04ffe5f420e 100644 > >>> --- a/hw/virtio/virtio.c > >>> +++ b/hw/virtio/virtio.c > >>> @@ -2255,6 +2255,7 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) > >>> return; > >>> } > >>> vdev->vq[n].vring.num = num; > >>> + virtio_queue_update_rings(vdev, n); > >>> } > >>> > >> > >> Spec said: > >> > >> " > >> The driver MUST configure the other virtqueue fields before enabling > >> the virtqueue with queue_enable. > >> " > >> > >> So I think we should forbid the num to be changed if the virtqueue is ready? > > What about virtio_queue_set_addr() and virtio_queue_set_align()? Same thing I guess.
On Thu, Aug 26, 2021 at 10:40:47AM +0200, Philippe Mathieu-Daudé wrote: > On 8/26/21 5:28 AM, Jason Wang wrote: > > On Thu, Aug 26, 2021 at 6:43 AM Philippe Mathieu-Daudé > > <philmd@redhat.com> wrote: > >> > >> When a ring queue size is modified, we need to call > >> virtio_queue_update_rings() to re-init the memory region > >> caches. Otherwise the region might have outdated memory > >> size, and later load/store access might trigger an > >> assertion, such: > >> > >> qemu-system-i386: include/exec/memory_ldst_cached.h.inc:30: uint16_t address_space_lduw_le_cached(MemoryRegionCache *, hwaddr, MemTxAttrs, MemTxResult *): > >> Assertion `addr < cache->len && 2 <= cache->len - addr' failed. > >> Thread 1 "qemu-system-i38" received signal SIGABRT, Aborted. > >> 0x00007ffff4d312a2 in raise () from /lib64/libc.so.6 > >> (gdb) bt > >> #1 0x00007ffff4d1a8a4 in abort () at /lib64/libc.so.6 > >> #4 0x0000555558f2b8ec in address_space_lduw_le_cached (cache=0x61300010a7c0, addr=134, attrs=..., result=0x0) at include/exec/memory_ldst_cached.h.inc:30 > >> #5 0x0000555558f2ac6b in lduw_le_phys_cached (cache=0x61300010a7c0, addr=134) at include/exec/memory_ldst_phys.h.inc:67 > >> #6 0x0000555558f2a3cd in virtio_lduw_phys_cached (vdev=0x62d00003a680, cache=0x61300010a7c0, pa=134) at include/hw/virtio/virtio-access.h:166 > >> #7 0x0000555558f300ea in vring_avail_ring (vq=0x7fffdd55d8a0, i=65) at hw/virtio/virtio.c:326 > >> #8 0x0000555558f33b10 in vring_get_used_event (vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:332 > >> #9 0x0000555558f33677 in virtio_split_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2471 > >> #10 0x0000555558f1859f in virtio_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2523 > >> #11 0x0000555558f188cc in virtio_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2565 > >> #12 0x0000555557c2bd52 in virtio_input_handle_sts (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/input/virtio-input.c:100 > >> #13 0x0000555558f16df7 in virtio_queue_notify (vdev=0x62d00003a680, n=1) at hw/virtio/virtio.c:2363 > >> #14 0x00005555583f45c0 in virtio_pci_notify_write (opaque=0x62d000032400, addr=7, val=0, size=1) at hw/virtio/virtio-pci.c:1369 > >> #15 0x0000555558b80b04 in memory_region_write_accessor (mr=0x62d000033190, addr=7, value=0x7fffffff8eb0, size=1, shift=0, mask=255, attrs=...) at softmmu/memory.c:492 > >> > >> Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com> > >> Fixes: ab223c9518e ("virtio: allow virtio-1 queue layout") > >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/302 > >> BugLink: https://bugs.launchpad.net/qemu/+bug/1913510 > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> hw/virtio/virtio.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >> index 874377f37a7..04ffe5f420e 100644 > >> --- a/hw/virtio/virtio.c > >> +++ b/hw/virtio/virtio.c > >> @@ -2255,6 +2255,7 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) > >> return; > >> } > >> vdev->vq[n].vring.num = num; > >> + virtio_queue_update_rings(vdev, n); > >> } > >> > > > > Spec said: > > > > " > > The driver MUST configure the other virtqueue fields before enabling > > the virtqueue with queue_enable. > > " > > > > So I think we should forbid the num to be changed if the virtqueue is ready? > > Alright. virtio_pci_common_write() doesn't report errors although. It can't - just log guest error and ignore. Maybe set needs_reset ...
On Thu, Aug 26, 2021 at 12:42:56AM +0200, Philippe Mathieu-Daudé wrote: > When a ring queue size is modified, we need to call > virtio_queue_update_rings() to re-init the memory region > caches. Otherwise the region might have outdated memory > size, and later load/store access might trigger an > assertion, such: > > qemu-system-i386: include/exec/memory_ldst_cached.h.inc:30: uint16_t address_space_lduw_le_cached(MemoryRegionCache *, hwaddr, MemTxAttrs, MemTxResult *): > Assertion `addr < cache->len && 2 <= cache->len - addr' failed. > Thread 1 "qemu-system-i38" received signal SIGABRT, Aborted. > 0x00007ffff4d312a2 in raise () from /lib64/libc.so.6 > (gdb) bt > #1 0x00007ffff4d1a8a4 in abort () at /lib64/libc.so.6 > #4 0x0000555558f2b8ec in address_space_lduw_le_cached (cache=0x61300010a7c0, addr=134, attrs=..., result=0x0) at include/exec/memory_ldst_cached.h.inc:30 > #5 0x0000555558f2ac6b in lduw_le_phys_cached (cache=0x61300010a7c0, addr=134) at include/exec/memory_ldst_phys.h.inc:67 > #6 0x0000555558f2a3cd in virtio_lduw_phys_cached (vdev=0x62d00003a680, cache=0x61300010a7c0, pa=134) at include/hw/virtio/virtio-access.h:166 > #7 0x0000555558f300ea in vring_avail_ring (vq=0x7fffdd55d8a0, i=65) at hw/virtio/virtio.c:326 > #8 0x0000555558f33b10 in vring_get_used_event (vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:332 > #9 0x0000555558f33677 in virtio_split_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2471 > #10 0x0000555558f1859f in virtio_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2523 > #11 0x0000555558f188cc in virtio_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2565 > #12 0x0000555557c2bd52 in virtio_input_handle_sts (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/input/virtio-input.c:100 > #13 0x0000555558f16df7 in virtio_queue_notify (vdev=0x62d00003a680, n=1) at hw/virtio/virtio.c:2363 > #14 0x00005555583f45c0 in virtio_pci_notify_write (opaque=0x62d000032400, addr=7, val=0, size=1) at hw/virtio/virtio-pci.c:1369 > #15 0x0000555558b80b04 in memory_region_write_accessor (mr=0x62d000033190, addr=7, value=0x7fffffff8eb0, size=1, shift=0, mask=255, attrs=...) at softmmu/memory.c:492 > > Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com> > Fixes: ab223c9518e ("virtio: allow virtio-1 queue layout") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/302 > BugLink: https://bugs.launchpad.net/qemu/+bug/1913510 > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/virtio/virtio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 874377f37a7..04ffe5f420e 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2255,6 +2255,7 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) > return; > } > vdev->vq[n].vring.num = num; > + virtio_queue_update_rings(vdev, n); > } Can this line in hw/virtio/virtio-mmio.c be removed now? case VIRTIO_MMIO_QUEUE_NUM: trace_virtio_mmio_queue_write(value, VIRTQUEUE_MAX_SIZE); virtio_queue_set_num(vdev, vdev->queue_sel, value); if (proxy->legacy) { virtio_queue_update_rings(vdev, vdev->queue_sel); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Stefan
On 210826 0042, Philippe Mathieu-Daudé wrote: > When a ring queue size is modified, we need to call > virtio_queue_update_rings() to re-init the memory region > caches. Otherwise the region might have outdated memory > size, and later load/store access might trigger an > assertion, such: > > qemu-system-i386: include/exec/memory_ldst_cached.h.inc:30: uint16_t address_space_lduw_le_cached(MemoryRegionCache *, hwaddr, MemTxAttrs, MemTxResult *): > Assertion `addr < cache->len && 2 <= cache->len - addr' failed. > Thread 1 "qemu-system-i38" received signal SIGABRT, Aborted. > 0x00007ffff4d312a2 in raise () from /lib64/libc.so.6 > (gdb) bt > #1 0x00007ffff4d1a8a4 in abort () at /lib64/libc.so.6 > #4 0x0000555558f2b8ec in address_space_lduw_le_cached (cache=0x61300010a7c0, addr=134, attrs=..., result=0x0) at include/exec/memory_ldst_cached.h.inc:30 > #5 0x0000555558f2ac6b in lduw_le_phys_cached (cache=0x61300010a7c0, addr=134) at include/exec/memory_ldst_phys.h.inc:67 > #6 0x0000555558f2a3cd in virtio_lduw_phys_cached (vdev=0x62d00003a680, cache=0x61300010a7c0, pa=134) at include/hw/virtio/virtio-access.h:166 > #7 0x0000555558f300ea in vring_avail_ring (vq=0x7fffdd55d8a0, i=65) at hw/virtio/virtio.c:326 > #8 0x0000555558f33b10 in vring_get_used_event (vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:332 > #9 0x0000555558f33677 in virtio_split_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2471 > #10 0x0000555558f1859f in virtio_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2523 > #11 0x0000555558f188cc in virtio_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2565 > #12 0x0000555557c2bd52 in virtio_input_handle_sts (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/input/virtio-input.c:100 > #13 0x0000555558f16df7 in virtio_queue_notify (vdev=0x62d00003a680, n=1) at hw/virtio/virtio.c:2363 > #14 0x00005555583f45c0 in virtio_pci_notify_write (opaque=0x62d000032400, addr=7, val=0, size=1) at hw/virtio/virtio-pci.c:1369 > #15 0x0000555558b80b04 in memory_region_write_accessor (mr=0x62d000033190, addr=7, value=0x7fffffff8eb0, size=1, shift=0, mask=255, attrs=...) at softmmu/memory.c:492 > > Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com> > Fixes: ab223c9518e ("virtio: allow virtio-1 queue layout") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/302 > BugLink: https://bugs.launchpad.net/qemu/+bug/1913510 OSS-Fuzz found ways to trigger assertion failure for virtio-{rng, serial, scsi, balloon, 9p}. Would it be useful to have qtest reproducers for these? -Alex > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/virtio/virtio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 874377f37a7..04ffe5f420e 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2255,6 +2255,7 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) > return; > } > vdev->vq[n].vring.num = num; > + virtio_queue_update_rings(vdev, n); > } > > VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector) > -- > 2.31.1 >
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 874377f37a7..04ffe5f420e 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -2255,6 +2255,7 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) return; } vdev->vq[n].vring.num = num; + virtio_queue_update_rings(vdev, n); } VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector)
When a ring queue size is modified, we need to call virtio_queue_update_rings() to re-init the memory region caches. Otherwise the region might have outdated memory size, and later load/store access might trigger an assertion, such: qemu-system-i386: include/exec/memory_ldst_cached.h.inc:30: uint16_t address_space_lduw_le_cached(MemoryRegionCache *, hwaddr, MemTxAttrs, MemTxResult *): Assertion `addr < cache->len && 2 <= cache->len - addr' failed. Thread 1 "qemu-system-i38" received signal SIGABRT, Aborted. 0x00007ffff4d312a2 in raise () from /lib64/libc.so.6 (gdb) bt #1 0x00007ffff4d1a8a4 in abort () at /lib64/libc.so.6 #4 0x0000555558f2b8ec in address_space_lduw_le_cached (cache=0x61300010a7c0, addr=134, attrs=..., result=0x0) at include/exec/memory_ldst_cached.h.inc:30 #5 0x0000555558f2ac6b in lduw_le_phys_cached (cache=0x61300010a7c0, addr=134) at include/exec/memory_ldst_phys.h.inc:67 #6 0x0000555558f2a3cd in virtio_lduw_phys_cached (vdev=0x62d00003a680, cache=0x61300010a7c0, pa=134) at include/hw/virtio/virtio-access.h:166 #7 0x0000555558f300ea in vring_avail_ring (vq=0x7fffdd55d8a0, i=65) at hw/virtio/virtio.c:326 #8 0x0000555558f33b10 in vring_get_used_event (vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:332 #9 0x0000555558f33677 in virtio_split_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2471 #10 0x0000555558f1859f in virtio_should_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2523 #11 0x0000555558f188cc in virtio_notify (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/virtio/virtio.c:2565 #12 0x0000555557c2bd52 in virtio_input_handle_sts (vdev=0x62d00003a680, vq=0x7fffdd55d8a0) at hw/input/virtio-input.c:100 #13 0x0000555558f16df7 in virtio_queue_notify (vdev=0x62d00003a680, n=1) at hw/virtio/virtio.c:2363 #14 0x00005555583f45c0 in virtio_pci_notify_write (opaque=0x62d000032400, addr=7, val=0, size=1) at hw/virtio/virtio-pci.c:1369 #15 0x0000555558b80b04 in memory_region_write_accessor (mr=0x62d000033190, addr=7, value=0x7fffffff8eb0, size=1, shift=0, mask=255, attrs=...) at softmmu/memory.c:492 Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com> Fixes: ab223c9518e ("virtio: allow virtio-1 queue layout") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/302 BugLink: https://bugs.launchpad.net/qemu/+bug/1913510 Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/virtio/virtio.c | 1 + 1 file changed, 1 insertion(+)