| Message ID | 20200902162206.101872-4-liq3ea@163.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | try to solve the DMA to MMIO issue | expand |
02.09.2020 19:22, Li Qiang wrote: .. > @@ -809,6 +809,10 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g) > { > struct virtio_gpu_ctrl_command *cmd; > > + if (atomic_read(&g->in_io)) { > + return; > + } > + atomic_set(&g->in_io, 1); Can't we race in these two instructions? Both threads atomic_reads at the same time, both see zero, and both are trying to set it to 1, no? Just asking really, b/c despite of the atomic_ prefix, to me this look a bit unsafe.. Thanks, /mjt
Michael Tokarev <mjt@tls.msk.ru> 于2020年9月3日周四 下午1:12写道： > > 02.09.2020 19:22, Li Qiang wrote: > .. > > @@ -809,6 +809,10 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g) > > { > > struct virtio_gpu_ctrl_command *cmd; > > > > + if (atomic_read(&g->in_io)) { > > + return; > > + } > > + atomic_set(&g->in_io, 1); > > Can't we race in these two instructions? Both > threads atomic_reads at the same time, both see zero, > and both are trying to set it to 1, no? > > Just asking really, b/c despite of the atomic_ prefix, > to me this look a bit unsafe.. Yes you're right. My patch is wrong. Here I try to address race condition and DMA to MMIO issue at the same time. I will first focus the DMA to MMIO issue in the revision patch. Thanks, Li Qiang > > Thanks, > > /mjt
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 5f0dd7c150..404b7dc174 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -809,6 +809,10 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g) { struct virtio_gpu_ctrl_command *cmd; + if (atomic_read(&g->in_io)) { + return; + } + atomic_set(&g->in_io, 1); while (!QTAILQ_EMPTY(&g->cmdq)) { cmd = QTAILQ_FIRST(&g->cmdq); @@ -838,6 +842,7 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g) g_free(cmd); } } + atomic_set(&g->in_io, 0); } static void virtio_gpu_gl_unblock(VirtIOGPUBase *b) @@ -1144,6 +1149,10 @@ static void virtio_gpu_reset(VirtIODevice *vdev) struct virtio_gpu_simple_resource *res, *tmp; struct virtio_gpu_ctrl_command *cmd; + if (atomic_read(&g->in_io)) { + return; + } + atomic_set(&g->in_io, 1); #ifdef CONFIG_VIRGL if (g->parent_obj.use_virgl_renderer) { virtio_gpu_virgl_reset(g); @@ -1179,6 +1188,7 @@ static void virtio_gpu_reset(VirtIODevice *vdev) #endif virtio_gpu_base_reset(VIRTIO_GPU_BASE(vdev)); + atomic_set(&g->in_io, 0); } static void diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h index 7517438e10..aadcf0e332 100644 --- a/include/hw/virtio/virtio-gpu.h +++ b/include/hw/virtio/virtio-gpu.h @@ -150,6 +150,7 @@ typedef struct VirtIOGPU { bool renderer_inited; bool renderer_reset; + bool in_io; QEMUTimer *fence_poll; QEMUTimer *print_stats;
The guest can program the virtio desc table. When the used ring is programmed by virtio-gpu's MMIO it may cause an reentrant issue. Following is the reproducer: cat << EOF | ./i386-softmmu/qemu-system-i386 -nographic -M pc -nodefaults -m 512M -device virtio-vga -qtest stdio outl 0xcf8 0x80001018 outl 0xcfc 0xe0800000 outl 0xcf8 0x80001020 outl 0xcf8 0x80001004 outw 0xcfc 0x7 writeq 0xe0801024 0x10646c00776c6cff writeq 0xe080102d 0xe0801000320000 writeq 0xe0801015 0x12b2901ba000000 write 0x10646c02 0x1 0x2c write 0x999 0x1 0x25 write 0x8 0x1 0x78 write 0x2c7 0x1 0x32 write 0x2cb 0x1 0xff write 0x2cc 0x1 0x7e writeq 0xe0803000 0xf2b8f0540ff83 EOF This patch avoid this by adding a 'in_io' in VirtIOGPU to indicate it is in IO processing. Notice this also address the race condition between 'virtio_gpu_process_cmdq' and 'virtio_gpu_reset' as the 'virtio_gpu_process_cmdq' is run in a BH which in the main thread and 'virtio_gpu_reset' is run in the vcpu thread and both of them access the 'g->cmdq'. Buglink: https://bugs.launchpad.net/qemu/+bug/1888606 Reported-by: Alexander Bulekov <alxndr@bu.edu> Signed-off-by: Li Qiang <liq3ea@163.com> --- hw/display/virtio-gpu.c | 10 ++++++++++ include/hw/virtio/virtio-gpu.h | 1 + 2 files changed, 11 insertions(+)