* [RFC 0/3] try to solve the DMA to MMIO issue @ 2020-09-02 16:22 Li Qiang 2020-09-02 16:22 ` [RFC 1/3] e1000e: make the IO handler reentrant Li Qiang ` (3 more replies) 0 siblings, 4 replies; 20+ messages in thread From: Li Qiang @ 2020-09-02 16:22 UTC (permalink / raw) To: mst, kraxel, dmitry.fleytman, jasowang, alxndr, peter.maydell, pbonzini Cc: Li Qiang, liq3ea, qemu-devel The qemu device fuzzer has found several DMA to MMIO issue. These issues is caused by the guest driver programs the DMA address, then in the device MMIO handler it trigger the DMA and as the DMA address is MMIO it will trigger another dispatch and reenter the MMIO handler again. However most of the device is not reentrant. DMA to MMIO will cause issues depend by the device emulator, mostly it will crash the qemu. Following is three classic DMA to MMIO issue. e1000e: https://bugs.launchpad.net/qemu/+bug/1886362 xhci: https://bugs.launchpad.net/qemu/+bug/1891354 virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606 The DMA to MMIO issue I think can be classified as following: 1. DMA to the device itself 2. device A DMA to device B and to device C 3. device A DMA to device B and to device A The first case of course should not be allowed. The second case I think it ok as the device IO handler has no assumption about the IO data came from no matter it come from device or other device. This is for P2P DMA. The third case I think it also should not be allowed. So our issue has been reduced by one case: not allowed the device's IO handler reenter. Paolo suggested that we can refactor the device emulation with BH. However it is a lot of work. I have thought several propose to address this, also discuss this with Jason Wang in private email. I have can solve this issue in core framework or in specific device. After try several methods I choose address it in per-device for following reason: 1. If we address it in core framwork we have to recored and check the device or MR info in MR dispatch write function. Unfortunally we have no these info in core framework. 2. The performance will also be decrease largely 3. Only the device itself know its IO The (most of the) device emulation is protected by BQL one time only a device emulation code can be run. We can add a flag to indicate the IO is running. The first two patches does this. For simplicity at the RFC stage I just set it while enter the IO callback and clear it exit the IO callback. It should be check/set/clean according the per-device's IO emulation. The second issue which itself suffers a race condition so I uses a atomic. Li Qiang (3): e1000e: make the IO handler reentrant xhci: make the IO handler reentrant virtio-gpu: make the IO handler reentrant hw/display/virtio-gpu.c | 10 ++++++ hw/net/e1000e.c | 35 +++++++++++++++++++- hw/usb/hcd-xhci.c | 60 ++++++++++++++++++++++++++++++++++ hw/usb/hcd-xhci.h | 1 + include/hw/virtio/virtio-gpu.h | 1 + 5 files changed, 106 insertions(+), 1 deletion(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC 1/3] e1000e: make the IO handler reentrant 2020-09-02 16:22 [RFC 0/3] try to solve the DMA to MMIO issue Li Qiang @ 2020-09-02 16:22 ` Li Qiang 2020-09-02 16:22 ` [RFC 2/3] xhci: " Li Qiang ` (2 subsequent siblings) 3 siblings, 0 replies; 20+ messages in thread From: Li Qiang @ 2020-09-02 16:22 UTC (permalink / raw) To: mst, kraxel, dmitry.fleytman, jasowang, alxndr, peter.maydell, pbonzini Cc: Li Qiang, liq3ea, qemu-devel The guest can program the e1000e DMA address to its MMIO. This will cause an UAF issue. Following is the reproducer: cat << EOF | ./i386-softmmu/qemu-system-i386 -M q35,accel=qtest \ -qtest stdio -nographic -monitor none -serial none outl 0xcf8 0x80001010 outl 0xcfc 0xe1020000 outl 0xcf8 0x80001014 outl 0xcf8 0x80001004 outw 0xcfc 0x7 outl 0xcf8 0x800010a2 write 0xe102003b 0x1 0xff write 0xe1020103 0x1e 0xffffff055c5e5c30be4511d084ffffffffffffffffffffffffffffffffff write 0xe1020420 0x4 0xffffffff write 0xe1020424 0x4 0xffffffff write 0xe102042b 0x1 0xff write 0xe1020430 0x4 0x055c5e5c write 0x5c041 0x1 0x04 write 0x5c042 0x1 0x02 write 0x5c043 0x1 0xe1 write 0x5c048 0x1 0x8a write 0x5c04a 0x1 0x31 write 0x5c04b 0x1 0xff write 0xe1020403 0x1 0xff EOF This patch avoid this by adding a 'in_io' in E1000EState to indicate it is in IO processing. Buglink: https://bugs.launchpad.net/qemu/+bug/1886362 Reported-by: Alexander Bulekov <alxndr@bu.edu> Signed-off-by: Li Qiang <liq3ea@163.com> --- hw/net/e1000e.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c index fda34518c9..eb6b34b7f3 100644 --- a/hw/net/e1000e.c +++ b/hw/net/e1000e.c @@ -77,6 +77,8 @@ typedef struct E1000EState { bool disable_vnet; + bool in_io; + E1000ECore core; } E1000EState; @@ -98,7 +100,15 @@ static uint64_t e1000e_mmio_read(void *opaque, hwaddr addr, unsigned size) { E1000EState *s = opaque; - return e1000e_core_read(&s->core, addr, size); + uint64_t ret; + + if (s->in_io) { + return 0; + } + s->in_io = true; + ret = e1000e_core_read(&s->core, addr, size); + s->in_io = false; + return ret; } static void @@ -106,7 +116,13 @@ e1000e_mmio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) { E1000EState *s = opaque; + + if (s->in_io) { + return; + } + s->in_io = true; e1000e_core_write(&s->core, addr, val, size); + s->in_io = false; } static bool @@ -138,19 +154,28 @@ e1000e_io_read(void *opaque, hwaddr addr, unsigned size) uint32_t idx = 0; uint64_t val; + if (s->in_io) { + return 0; + } + s->in_io = true; + switch (addr) { case E1000_IOADDR: trace_e1000e_io_read_addr(s->ioaddr); + s->in_io = false; return s->ioaddr; case E1000_IODATA: if (e1000e_io_get_reg_index(s, &idx)) { val = e1000e_core_read(&s->core, idx, sizeof(val)); trace_e1000e_io_read_data(idx, val); + s->in_io = false; return val; } + s->in_io = false; return 0; default: trace_e1000e_wrn_io_read_unknown(addr); + s->in_io = false; return 0; } } @@ -162,19 +187,27 @@ e1000e_io_write(void *opaque, hwaddr addr, E1000EState *s = opaque; uint32_t idx = 0; + if (s->in_io) { + return; + } + s->in_io = true; + switch (addr) { case E1000_IOADDR: trace_e1000e_io_write_addr(val); s->ioaddr = (uint32_t) val; + s->in_io = false; return; case E1000_IODATA: if (e1000e_io_get_reg_index(s, &idx)) { trace_e1000e_io_write_data(idx, val); e1000e_core_write(&s->core, idx, val, sizeof(val)); } + s->in_io = false; return; default: trace_e1000e_wrn_io_write_unknown(addr); + s->in_io = false; return; } } -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [RFC 2/3] xhci: make the IO handler reentrant 2020-09-02 16:22 [RFC 0/3] try to solve the DMA to MMIO issue Li Qiang 2020-09-02 16:22 ` [RFC 1/3] e1000e: make the IO handler reentrant Li Qiang @ 2020-09-02 16:22 ` Li Qiang 2020-09-02 16:22 ` [RFC 3/3] virtio-gpu: " Li Qiang 2020-09-03 3:54 ` [RFC 0/3] try to solve the DMA to MMIO issue Jason Wang 3 siblings, 0 replies; 20+ messages in thread From: Li Qiang @ 2020-09-02 16:22 UTC (permalink / raw) To: mst, kraxel, dmitry.fleytman, jasowang, alxndr, peter.maydell, pbonzini Cc: Li Qiang, liq3ea, qemu-devel The guest can program the xhci DMA address to its MMIO. This will cause an UAF issue. Following is the reproducer: cat << EOF | ./i386-softmmu/qemu-system-i386 -device nec-usb-xhci \ -trace usb\* -device usb-audio -device usb-storage,drive=mydrive \ -drive id=mydrive,file=null-co://,size=2M,format=raw,if=none \ -nodefaults -nographic -qtest stdio outl 0xcf8 0x80001010 outl 0xcfc 0xc0202 outl 0xcf8 0x80001004 outl 0xcfc 0x1c77695e writel 0xc0040 0xffffd855 writeq 0xc2000 0xff05140100000000 write 0x1d 0x1 0x27 write 0x2d 0x1 0x2e write 0x17232 0x1 0x03 write 0x17254 0x1 0x05 write 0x17276 0x1 0x72 write 0x17278 0x1 0x02 write 0x3d 0x1 0x27 write 0x40 0x1 0x2e write 0x41 0x1 0x72 write 0x42 0x1 0x01 write 0x4d 0x1 0x2e write 0x4f 0x1 0x01 write 0x2007c 0x1 0xc7 writeq 0xc2000 0x5c05140100000000 write 0x20070 0x1 0x80 write 0x20078 0x1 0x08 write 0x2007c 0x1 0xfe write 0x2007d 0x1 0x08 write 0x20081 0x1 0xff write 0x20082 0x1 0x0b write 0x20089 0x1 0x8c write 0x2008d 0x1 0x04 write 0x2009d 0x1 0x10 writeq 0xc2000 0x2505ef019e092f00 EOF This patch avoid this by adding a 'in_io' in XHCIState to indicate it is in IO processing. Buglink: https://bugs.launchpad.net/qemu/+bug/1891354 Reported-by: Alexander Bulekov <alxndr@bu.edu> Signed-off-by: Li Qiang <liq3ea@163.com> --- hw/usb/hcd-xhci.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++ hw/usb/hcd-xhci.h | 1 + 2 files changed, 61 insertions(+) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 46a2186d91..06cd235123 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -2738,6 +2738,11 @@ static uint64_t xhci_cap_read(void *ptr, hwaddr reg, unsigned size) XHCIState *xhci = ptr; uint32_t ret; + if (xhci->in_io) { + return 0; + } + xhci->in_io = true; + switch (reg) { case 0x00: /* HCIVERSION, CAPLENGTH */ ret = 0x01000000 | LEN_CAP; @@ -2805,6 +2810,9 @@ static uint64_t xhci_cap_read(void *ptr, hwaddr reg, unsigned size) } trace_usb_xhci_cap_read(reg, ret); + + xhci->in_io = false; + return ret; } @@ -2813,6 +2821,11 @@ static uint64_t xhci_port_read(void *ptr, hwaddr reg, unsigned size) XHCIPort *port = ptr; uint32_t ret; + if (port->xhci->in_io) { + return 0; + } + port->xhci->in_io = true; + switch (reg) { case 0x00: /* PORTSC */ ret = port->portsc; @@ -2828,6 +2841,9 @@ static uint64_t xhci_port_read(void *ptr, hwaddr reg, unsigned size) } trace_usb_xhci_port_read(port->portnr, reg, ret); + + port->xhci->in_io = false; + return ret; } @@ -2837,6 +2853,11 @@ static void xhci_port_write(void *ptr, hwaddr reg, XHCIPort *port = ptr; uint32_t portsc, notify; + if (port->xhci->in_io) { + return; + } + port->xhci->in_io = true; + trace_usb_xhci_port_write(port->portnr, reg, val); switch (reg) { @@ -2896,6 +2917,7 @@ static void xhci_port_write(void *ptr, hwaddr reg, default: trace_usb_xhci_unimplemented("port write", reg); } + port->xhci->in_io = false; } static uint64_t xhci_oper_read(void *ptr, hwaddr reg, unsigned size) @@ -2903,6 +2925,11 @@ static uint64_t xhci_oper_read(void *ptr, hwaddr reg, unsigned size) XHCIState *xhci = ptr; uint32_t ret; + if (xhci->in_io) { + return 0; + } + xhci->in_io = true; + switch (reg) { case 0x00: /* USBCMD */ ret = xhci->usbcmd; @@ -2937,6 +2964,9 @@ static uint64_t xhci_oper_read(void *ptr, hwaddr reg, unsigned size) } trace_usb_xhci_oper_read(reg, ret); + + xhci->in_io = false; + return ret; } @@ -2946,6 +2976,11 @@ static void xhci_oper_write(void *ptr, hwaddr reg, XHCIState *xhci = ptr; DeviceState *d = DEVICE(ptr); + if (xhci->in_io) { + return; + } + xhci->in_io = true; + trace_usb_xhci_oper_write(reg, val); switch (reg) { @@ -3008,6 +3043,7 @@ static void xhci_oper_write(void *ptr, hwaddr reg, default: trace_usb_xhci_unimplemented("oper write", reg); } + xhci->in_io = false; } static uint64_t xhci_runtime_read(void *ptr, hwaddr reg, @@ -3016,6 +3052,11 @@ static uint64_t xhci_runtime_read(void *ptr, hwaddr reg, XHCIState *xhci = ptr; uint32_t ret = 0; + if (xhci->in_io) { + return 0; + } + xhci->in_io = true; + if (reg < 0x20) { switch (reg) { case 0x00: /* MFINDEX */ @@ -3054,6 +3095,9 @@ static uint64_t xhci_runtime_read(void *ptr, hwaddr reg, } trace_usb_xhci_runtime_read(reg, ret); + + xhci->in_io = false; + return ret; } @@ -3063,10 +3107,17 @@ static void xhci_runtime_write(void *ptr, hwaddr reg, XHCIState *xhci = ptr; int v = (reg - 0x20) / 0x20; XHCIInterrupter *intr = &xhci->intr[v]; + + if (xhci->in_io) { + return; + } + xhci->in_io = true; + trace_usb_xhci_runtime_write(reg, val); if (reg < 0x20) { trace_usb_xhci_unimplemented("runtime write", reg); + xhci->in_io = false; return; } @@ -3121,6 +3172,7 @@ static void xhci_runtime_write(void *ptr, hwaddr reg, default: trace_usb_xhci_unimplemented("oper write", reg); } + xhci->in_io = false; } static uint64_t xhci_doorbell_read(void *ptr, hwaddr reg, @@ -3137,10 +3189,17 @@ static void xhci_doorbell_write(void *ptr, hwaddr reg, XHCIState *xhci = ptr; unsigned int epid, streamid; + if (xhci->in_io) { + return; + } + + xhci->in_io = true; + trace_usb_xhci_doorbell_write(reg, val); if (!xhci_running(xhci)) { DPRINTF("xhci: wrote doorbell while xHC stopped or paused\n"); + xhci->in_io = false; return; } @@ -3165,6 +3224,7 @@ static void xhci_doorbell_write(void *ptr, hwaddr reg, xhci_kick_ep(xhci, reg, epid, streamid); } } + xhci->in_io = false; } static void xhci_cap_write(void *opaque, hwaddr addr, uint64_t val, diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h index 946af51fc2..ed16232c96 100644 --- a/hw/usb/hcd-xhci.h +++ b/hw/usb/hcd-xhci.h @@ -227,6 +227,7 @@ struct XHCIState { XHCIRing cmd_ring; bool nec_quirks; + bool in_io; }; #endif -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [RFC 3/3] virtio-gpu: make the IO handler reentrant 2020-09-02 16:22 [RFC 0/3] try to solve the DMA to MMIO issue Li Qiang 2020-09-02 16:22 ` [RFC 1/3] e1000e: make the IO handler reentrant Li Qiang 2020-09-02 16:22 ` [RFC 2/3] xhci: " Li Qiang @ 2020-09-02 16:22 ` Li Qiang 2020-09-03 5:12 ` Michael Tokarev 2020-09-03 3:54 ` [RFC 0/3] try to solve the DMA to MMIO issue Jason Wang 3 siblings, 1 reply; 20+ messages in thread From: Li Qiang @ 2020-09-02 16:22 UTC (permalink / raw) To: mst, kraxel, dmitry.fleytman, jasowang, alxndr, peter.maydell, pbonzini Cc: Li Qiang, liq3ea, qemu-devel 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(+) 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; -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [RFC 3/3] virtio-gpu: make the IO handler reentrant 2020-09-02 16:22 ` [RFC 3/3] virtio-gpu: " Li Qiang @ 2020-09-03 5:12 ` Michael Tokarev 2020-09-03 10:32 ` Li Qiang 0 siblings, 1 reply; 20+ messages in thread From: Michael Tokarev @ 2020-09-03 5:12 UTC (permalink / raw) To: Li Qiang, mst, kraxel, dmitry.fleytman, jasowang, alxndr, peter.maydell, pbonzini Cc: liq3ea, qemu-devel 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 3/3] virtio-gpu: make the IO handler reentrant 2020-09-03 5:12 ` Michael Tokarev @ 2020-09-03 10:32 ` Li Qiang 0 siblings, 0 replies; 20+ messages in thread From: Li Qiang @ 2020-09-03 10:32 UTC (permalink / raw) To: Michael Tokarev Cc: Peter Maydell, Dmitry Fleytman, Michael S. Tsirkin, Jason Wang, Li Qiang, Qemu Developers, Alexander Bulekov, Gerd Hoffmann, Paolo Bonzini 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-02 16:22 [RFC 0/3] try to solve the DMA to MMIO issue Li Qiang ` (2 preceding siblings ...) 2020-09-02 16:22 ` [RFC 3/3] virtio-gpu: " Li Qiang @ 2020-09-03 3:54 ` Jason Wang 2020-09-03 4:06 ` Alexander Bulekov 2020-09-03 10:53 ` Peter Maydell 3 siblings, 2 replies; 20+ messages in thread From: Jason Wang @ 2020-09-03 3:54 UTC (permalink / raw) To: Li Qiang, mst, kraxel, dmitry.fleytman, alxndr, peter.maydell, pbonzini Cc: liq3ea, qemu-devel On 2020/9/3 上午12:22, Li Qiang wrote: > The qemu device fuzzer has found several DMA to MMIO issue. > These issues is caused by the guest driver programs the DMA > address, then in the device MMIO handler it trigger the DMA > and as the DMA address is MMIO it will trigger another dispatch > and reenter the MMIO handler again. However most of the device > is not reentrant. > > DMA to MMIO will cause issues depend by the device emulator, > mostly it will crash the qemu. Following is three classic > DMA to MMIO issue. > > e1000e: https://bugs.launchpad.net/qemu/+bug/1886362 > xhci: https://bugs.launchpad.net/qemu/+bug/1891354 > virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606 > > The DMA to MMIO issue I think can be classified as following: > 1. DMA to the device itself > 2. device A DMA to device B and to device C > 3. device A DMA to device B and to device A > > The first case of course should not be allowed. > The second case I think it ok as the device IO handler has no > assumption about the IO data came from no matter it come from > device or other device. This is for P2P DMA. > The third case I think it also should not be allowed. > > So our issue has been reduced by one case: not allowed the > device's IO handler reenter. > > Paolo suggested that we can refactor the device emulation with > BH. However it is a lot of work. > I have thought several propose to address this, also discuss > this with Jason Wang in private email. > > I have can solve this issue in core framework or in specific device. > After try several methods I choose address it in per-device for > following reason: > 1. If we address it in core framwork we have to recored and check the > device or MR info in MR dispatch write function. Unfortunally we have > no these info in core framework. > 2. The performance will also be decrease largely > 3. Only the device itself know its IO I think we still need to seek a way to address this issue completely. How about adding a flag in MemoryRegionOps and detect the reentrancy through that flag? Thanks > > The (most of the) device emulation is protected by BQL one time only > a device emulation code can be run. We can add a flag to indicate the > IO is running. The first two patches does this. For simplicity at the > RFC stage I just set it while enter the IO callback and clear it exit > the IO callback. It should be check/set/clean according the per-device's > IO emulation. > The second issue which itself suffers a race condition so I uses a > atomic. > > > > > Li Qiang (3): > e1000e: make the IO handler reentrant > xhci: make the IO handler reentrant > virtio-gpu: make the IO handler reentrant > > hw/display/virtio-gpu.c | 10 ++++++ > hw/net/e1000e.c | 35 +++++++++++++++++++- > hw/usb/hcd-xhci.c | 60 ++++++++++++++++++++++++++++++++++ > hw/usb/hcd-xhci.h | 1 + > include/hw/virtio/virtio-gpu.h | 1 + > 5 files changed, 106 insertions(+), 1 deletion(-) > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 3:54 ` [RFC 0/3] try to solve the DMA to MMIO issue Jason Wang @ 2020-09-03 4:06 ` Alexander Bulekov 2020-09-03 4:24 ` Jason Wang 2020-09-03 10:53 ` Peter Maydell 1 sibling, 1 reply; 20+ messages in thread From: Alexander Bulekov @ 2020-09-03 4:06 UTC (permalink / raw) To: Jason Wang Cc: peter.maydell, dmitry.fleytman, mst, liq3ea, Li Qiang, qemu-devel, kraxel, pbonzini On 200903 1154, Jason Wang wrote: > > On 2020/9/3 上午12:22, Li Qiang wrote: > > The qemu device fuzzer has found several DMA to MMIO issue. > > These issues is caused by the guest driver programs the DMA > > address, then in the device MMIO handler it trigger the DMA > > and as the DMA address is MMIO it will trigger another dispatch > > and reenter the MMIO handler again. However most of the device > > is not reentrant. > > > > DMA to MMIO will cause issues depend by the device emulator, > > mostly it will crash the qemu. Following is three classic > > DMA to MMIO issue. > > > > e1000e: https://bugs.launchpad.net/qemu/+bug/1886362 > > xhci: https://bugs.launchpad.net/qemu/+bug/1891354 > > virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606 > > > > The DMA to MMIO issue I think can be classified as following: > > 1. DMA to the device itself > > 2. device A DMA to device B and to device C > > 3. device A DMA to device B and to device A > > > > The first case of course should not be allowed. > > The second case I think it ok as the device IO handler has no > > assumption about the IO data came from no matter it come from > > device or other device. This is for P2P DMA. > > The third case I think it also should not be allowed. > > > > So our issue has been reduced by one case: not allowed the > > device's IO handler reenter. > > > > Paolo suggested that we can refactor the device emulation with > > BH. However it is a lot of work. > > I have thought several propose to address this, also discuss > > this with Jason Wang in private email. > > > > I have can solve this issue in core framework or in specific device. > > After try several methods I choose address it in per-device for > > following reason: > > 1. If we address it in core framwork we have to recored and check the > > device or MR info in MR dispatch write function. Unfortunally we have > > no these info in core framework. > > 2. The performance will also be decrease largely > > 3. Only the device itself know its IO > > > I think we still need to seek a way to address this issue completely. > > How about adding a flag in MemoryRegionOps and detect the reentrancy through > that flag? What happens for devices with multiple MemoryRegions? Make all the MemoryRegionOps share the same flag? What about the virtio-gpu bug, where the problem happens in a bh->mmio access rather than an mmio->mmio access? -Alex > Thanks > > > > > > The (most of the) device emulation is protected by BQL one time only > > a device emulation code can be run. We can add a flag to indicate the > > IO is running. The first two patches does this. For simplicity at the > > RFC stage I just set it while enter the IO callback and clear it exit > > the IO callback. It should be check/set/clean according the per-device's > > IO emulation. > > The second issue which itself suffers a race condition so I uses a > > atomic. > > > > > > > > > > Li Qiang (3): > > e1000e: make the IO handler reentrant > > xhci: make the IO handler reentrant > > virtio-gpu: make the IO handler reentrant > > > > hw/display/virtio-gpu.c | 10 ++++++ > > hw/net/e1000e.c | 35 +++++++++++++++++++- > > hw/usb/hcd-xhci.c | 60 ++++++++++++++++++++++++++++++++++ > > hw/usb/hcd-xhci.h | 1 + > > include/hw/virtio/virtio-gpu.h | 1 + > > 5 files changed, 106 insertions(+), 1 deletion(-) > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 4:06 ` Alexander Bulekov @ 2020-09-03 4:24 ` Jason Wang 2020-09-03 4:50 ` Li Qiang 0 siblings, 1 reply; 20+ messages in thread From: Jason Wang @ 2020-09-03 4:24 UTC (permalink / raw) To: Alexander Bulekov Cc: peter.maydell, dmitry.fleytman, mst, liq3ea, Li Qiang, qemu-devel, kraxel, pbonzini On 2020/9/3 下午12:06, Alexander Bulekov wrote: > On 200903 1154, Jason Wang wrote: >> On 2020/9/3 上午12:22, Li Qiang wrote: >>> The qemu device fuzzer has found several DMA to MMIO issue. >>> These issues is caused by the guest driver programs the DMA >>> address, then in the device MMIO handler it trigger the DMA >>> and as the DMA address is MMIO it will trigger another dispatch >>> and reenter the MMIO handler again. However most of the device >>> is not reentrant. >>> >>> DMA to MMIO will cause issues depend by the device emulator, >>> mostly it will crash the qemu. Following is three classic >>> DMA to MMIO issue. >>> >>> e1000e: https://bugs.launchpad.net/qemu/+bug/1886362 >>> xhci: https://bugs.launchpad.net/qemu/+bug/1891354 >>> virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606 >>> >>> The DMA to MMIO issue I think can be classified as following: >>> 1. DMA to the device itself >>> 2. device A DMA to device B and to device C >>> 3. device A DMA to device B and to device A >>> >>> The first case of course should not be allowed. >>> The second case I think it ok as the device IO handler has no >>> assumption about the IO data came from no matter it come from >>> device or other device. This is for P2P DMA. >>> The third case I think it also should not be allowed. >>> >>> So our issue has been reduced by one case: not allowed the >>> device's IO handler reenter. >>> >>> Paolo suggested that we can refactor the device emulation with >>> BH. However it is a lot of work. >>> I have thought several propose to address this, also discuss >>> this with Jason Wang in private email. >>> >>> I have can solve this issue in core framework or in specific device. >>> After try several methods I choose address it in per-device for >>> following reason: >>> 1. If we address it in core framwork we have to recored and check the >>> device or MR info in MR dispatch write function. Unfortunally we have >>> no these info in core framework. >>> 2. The performance will also be decrease largely >>> 3. Only the device itself know its IO >> >> I think we still need to seek a way to address this issue completely. >> >> How about adding a flag in MemoryRegionOps and detect the reentrancy through >> that flag? > What happens for devices with multiple MemoryRegions? Make all the > MemoryRegionOps share the same flag? I think there could be two approaches: 1) record the device in MR as Qiang mentioned 2) Only forbid the reentrancy in MMIO handler and depends on the device to solve the multiple Memory Region issue, if the regions want to access the same data, it needs to be synchronized internally But the point is still to try to solve it in the layer of memory regions. Otherwise we may still hit similar issues. > > What about the virtio-gpu bug, where the problem happens in a bh->mmio > access rather than an mmio->mmio access? Yes, it needs more thought, but as a first step, we can try to fix the MMIO handler issue and do bh fix on top. Thanks > > -Alex > >> Thanks >> >> >>> The (most of the) device emulation is protected by BQL one time only >>> a device emulation code can be run. We can add a flag to indicate the >>> IO is running. The first two patches does this. For simplicity at the >>> RFC stage I just set it while enter the IO callback and clear it exit >>> the IO callback. It should be check/set/clean according the per-device's >>> IO emulation. >>> The second issue which itself suffers a race condition so I uses a >>> atomic. >>> >>> >>> >>> >>> Li Qiang (3): >>> e1000e: make the IO handler reentrant >>> xhci: make the IO handler reentrant >>> virtio-gpu: make the IO handler reentrant >>> >>> hw/display/virtio-gpu.c | 10 ++++++ >>> hw/net/e1000e.c | 35 +++++++++++++++++++- >>> hw/usb/hcd-xhci.c | 60 ++++++++++++++++++++++++++++++++++ >>> hw/usb/hcd-xhci.h | 1 + >>> include/hw/virtio/virtio-gpu.h | 1 + >>> 5 files changed, 106 insertions(+), 1 deletion(-) >>> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 4:24 ` Jason Wang @ 2020-09-03 4:50 ` Li Qiang 2020-09-03 6:16 ` Jason Wang 0 siblings, 1 reply; 20+ messages in thread From: Li Qiang @ 2020-09-03 4:50 UTC (permalink / raw) To: Jason Wang Cc: Peter Maydell, Dmitry Fleytman, Michael S. Tsirkin, Li Qiang, Qemu Developers, Alexander Bulekov, Gerd Hoffmann, Paolo Bonzini Jason Wang <jasowang@redhat.com> 于2020年9月3日周四 下午12:24写道: > > > On 2020/9/3 下午12:06, Alexander Bulekov wrote: > > On 200903 1154, Jason Wang wrote: > >> On 2020/9/3 上午12:22, Li Qiang wrote: > >>> The qemu device fuzzer has found several DMA to MMIO issue. > >>> These issues is caused by the guest driver programs the DMA > >>> address, then in the device MMIO handler it trigger the DMA > >>> and as the DMA address is MMIO it will trigger another dispatch > >>> and reenter the MMIO handler again. However most of the device > >>> is not reentrant. > >>> > >>> DMA to MMIO will cause issues depend by the device emulator, > >>> mostly it will crash the qemu. Following is three classic > >>> DMA to MMIO issue. > >>> > >>> e1000e: https://bugs.launchpad.net/qemu/+bug/1886362 > >>> xhci: https://bugs.launchpad.net/qemu/+bug/1891354 > >>> virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606 > >>> > >>> The DMA to MMIO issue I think can be classified as following: > >>> 1. DMA to the device itself > >>> 2. device A DMA to device B and to device C > >>> 3. device A DMA to device B and to device A > >>> > >>> The first case of course should not be allowed. > >>> The second case I think it ok as the device IO handler has no > >>> assumption about the IO data came from no matter it come from > >>> device or other device. This is for P2P DMA. > >>> The third case I think it also should not be allowed. > >>> > >>> So our issue has been reduced by one case: not allowed the > >>> device's IO handler reenter. > >>> > >>> Paolo suggested that we can refactor the device emulation with > >>> BH. However it is a lot of work. > >>> I have thought several propose to address this, also discuss > >>> this with Jason Wang in private email. > >>> > >>> I have can solve this issue in core framework or in specific device. > >>> After try several methods I choose address it in per-device for > >>> following reason: > >>> 1. If we address it in core framwork we have to recored and check the > >>> device or MR info in MR dispatch write function. Unfortunally we have > >>> no these info in core framework. > >>> 2. The performance will also be decrease largely > >>> 3. Only the device itself know its IO > >> > >> I think we still need to seek a way to address this issue completely. > >> > >> How about adding a flag in MemoryRegionOps and detect the reentrancy through > >> that flag? > > What happens for devices with multiple MemoryRegions? Make all the > > MemoryRegionOps share the same flag? > > > I think there could be two approaches: > > 1) record the device in MR as Qiang mentioned I have tried this as we discussed. But has following consideration: 1. The performance, we need to check/record/clean the MR in an array/hashtable. 2. The multiple MR and alias MR process in the memory layer. It is complicated and performance effective. So If we let the MR issue to the device itself, it is just as this patch does-let the device address the reentrancy issue.f Another solution. We connects a MR with the corresponding device. Now the device often tight MR with an 'opaque' field. Just uses it in the calling of MR callback. Then we add a flag in the device and needs to modify the MR register interface. So in the memory layer we can check/record/clean the MR->device->flag. But this is can't address the DMA (in BH) to MMIO issue as the BH runs in main thread. Thanks, Li Qiang > 2) Only forbid the reentrancy in MMIO handler and depends on the device > to solve the multiple Memory Region issue, if the regions want to access > the same data, it needs to be synchronized internally > > But the point is still to try to solve it in the layer of memory > regions. Otherwise we may still hit similar issues. > > > > > > What about the virtio-gpu bug, where the problem happens in a bh->mmio > > access rather than an mmio->mmio access? > > > Yes, it needs more thought, but as a first step, we can try to fix the > MMIO handler issue and do bh fix on top. > > Thanks > > > > > > -Alex > > > >> Thanks > >> > >> > >>> The (most of the) device emulation is protected by BQL one time only > >>> a device emulation code can be run. We can add a flag to indicate the > >>> IO is running. The first two patches does this. For simplicity at the > >>> RFC stage I just set it while enter the IO callback and clear it exit > >>> the IO callback. It should be check/set/clean according the per-device's > >>> IO emulation. > >>> The second issue which itself suffers a race condition so I uses a > >>> atomic. > >>> > >>> > >>> > >>> > >>> Li Qiang (3): > >>> e1000e: make the IO handler reentrant > >>> xhci: make the IO handler reentrant > >>> virtio-gpu: make the IO handler reentrant > >>> > >>> hw/display/virtio-gpu.c | 10 ++++++ > >>> hw/net/e1000e.c | 35 +++++++++++++++++++- > >>> hw/usb/hcd-xhci.c | 60 ++++++++++++++++++++++++++++++++++ > >>> hw/usb/hcd-xhci.h | 1 + > >>> include/hw/virtio/virtio-gpu.h | 1 + > >>> 5 files changed, 106 insertions(+), 1 deletion(-) > >>> > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 4:50 ` Li Qiang @ 2020-09-03 6:16 ` Jason Wang 2020-09-03 6:28 ` Li Qiang 0 siblings, 1 reply; 20+ messages in thread From: Jason Wang @ 2020-09-03 6:16 UTC (permalink / raw) To: Li Qiang Cc: Peter Maydell, Dmitry Fleytman, Michael S. Tsirkin, Li Qiang, Qemu Developers, Alexander Bulekov, Gerd Hoffmann, Paolo Bonzini On 2020/9/3 下午12:50, Li Qiang wrote: > Jason Wang <jasowang@redhat.com> 于2020年9月3日周四 下午12:24写道: >> >> On 2020/9/3 下午12:06, Alexander Bulekov wrote: >>> On 200903 1154, Jason Wang wrote: >>>> On 2020/9/3 上午12:22, Li Qiang wrote: >>>>> The qemu device fuzzer has found several DMA to MMIO issue. >>>>> These issues is caused by the guest driver programs the DMA >>>>> address, then in the device MMIO handler it trigger the DMA >>>>> and as the DMA address is MMIO it will trigger another dispatch >>>>> and reenter the MMIO handler again. However most of the device >>>>> is not reentrant. >>>>> >>>>> DMA to MMIO will cause issues depend by the device emulator, >>>>> mostly it will crash the qemu. Following is three classic >>>>> DMA to MMIO issue. >>>>> >>>>> e1000e: https://bugs.launchpad.net/qemu/+bug/1886362 >>>>> xhci: https://bugs.launchpad.net/qemu/+bug/1891354 >>>>> virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606 >>>>> >>>>> The DMA to MMIO issue I think can be classified as following: >>>>> 1. DMA to the device itself >>>>> 2. device A DMA to device B and to device C >>>>> 3. device A DMA to device B and to device A >>>>> >>>>> The first case of course should not be allowed. >>>>> The second case I think it ok as the device IO handler has no >>>>> assumption about the IO data came from no matter it come from >>>>> device or other device. This is for P2P DMA. >>>>> The third case I think it also should not be allowed. >>>>> >>>>> So our issue has been reduced by one case: not allowed the >>>>> device's IO handler reenter. >>>>> >>>>> Paolo suggested that we can refactor the device emulation with >>>>> BH. However it is a lot of work. >>>>> I have thought several propose to address this, also discuss >>>>> this with Jason Wang in private email. >>>>> >>>>> I have can solve this issue in core framework or in specific device. >>>>> After try several methods I choose address it in per-device for >>>>> following reason: >>>>> 1. If we address it in core framwork we have to recored and check the >>>>> device or MR info in MR dispatch write function. Unfortunally we have >>>>> no these info in core framework. >>>>> 2. The performance will also be decrease largely >>>>> 3. Only the device itself know its IO >>>> I think we still need to seek a way to address this issue completely. >>>> >>>> How about adding a flag in MemoryRegionOps and detect the reentrancy through >>>> that flag? >>> What happens for devices with multiple MemoryRegions? Make all the >>> MemoryRegionOps share the same flag? >> >> I think there could be two approaches: >> >> 1) record the device in MR as Qiang mentioned > I have tried this as we discussed. But has following consideration: > 1. The performance, we need to check/record/clean the MR in an array/hashtable. > > 2. The multiple MR and alias MR process in the memory layer. It is > complicated and performance effective. > So If we let the MR issue to the device itself, it is just as this > patch does-let the device address the reentrancy issue.f > > Another solution. We connects a MR with the corresponding device. Now > the device often tight MR with an 'opaque' field. > Just uses it in the calling of MR callback. Then we add a flag in the > device and needs to modify the MR register interface. > > So in the memory layer we can check/record/clean the MR->device->flag. > But this is can't address the DMA (in BH) to MMIO issue as the BH runs > in main thread. This is probably good enough to start. To my point of view, there're two different issues: 1) re-entrant MMIO handler 2) MMIO hanlder sync with BH For 1), we'd better solve it at core, For 2) it can only be solved in the device. Thanks > > Thanks, > Li Qiang > > > >> 2) Only forbid the reentrancy in MMIO handler and depends on the device >> to solve the multiple Memory Region issue, if the regions want to access >> the same data, it needs to be synchronized internally >> >> But the point is still to try to solve it in the layer of memory >> regions. Otherwise we may still hit similar issues. >> >> >>> What about the virtio-gpu bug, where the problem happens in a bh->mmio >>> access rather than an mmio->mmio access? >> >> Yes, it needs more thought, but as a first step, we can try to fix the >> MMIO handler issue and do bh fix on top. > > >> Thanks >> >> >>> -Alex >>> >>>> Thanks >>>> >>>> >>>>> The (most of the) device emulation is protected by BQL one time only >>>>> a device emulation code can be run. We can add a flag to indicate the >>>>> IO is running. The first two patches does this. For simplicity at the >>>>> RFC stage I just set it while enter the IO callback and clear it exit >>>>> the IO callback. It should be check/set/clean according the per-device's >>>>> IO emulation. >>>>> The second issue which itself suffers a race condition so I uses a >>>>> atomic. >>>>> >>>>> >>>>> >>>>> >>>>> Li Qiang (3): >>>>> e1000e: make the IO handler reentrant >>>>> xhci: make the IO handler reentrant >>>>> virtio-gpu: make the IO handler reentrant >>>>> >>>>> hw/display/virtio-gpu.c | 10 ++++++ >>>>> hw/net/e1000e.c | 35 +++++++++++++++++++- >>>>> hw/usb/hcd-xhci.c | 60 ++++++++++++++++++++++++++++++++++ >>>>> hw/usb/hcd-xhci.h | 1 + >>>>> include/hw/virtio/virtio-gpu.h | 1 + >>>>> 5 files changed, 106 insertions(+), 1 deletion(-) >>>>> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 6:16 ` Jason Wang @ 2020-09-03 6:28 ` Li Qiang 0 siblings, 0 replies; 20+ messages in thread From: Li Qiang @ 2020-09-03 6:28 UTC (permalink / raw) To: Jason Wang Cc: Peter Maydell, Dmitry Fleytman, Michael S. Tsirkin, Li Qiang, Qemu Developers, Alexander Bulekov, Gerd Hoffmann, Paolo Bonzini Jason Wang <jasowang@redhat.com> 于2020年9月3日周四 下午2:16写道: > > > On 2020/9/3 下午12:50, Li Qiang wrote: > > Jason Wang <jasowang@redhat.com> 于2020年9月3日周四 下午12:24写道: > >> > >> On 2020/9/3 下午12:06, Alexander Bulekov wrote: > >>> On 200903 1154, Jason Wang wrote: > >>>> On 2020/9/3 上午12:22, Li Qiang wrote: > >>>>> The qemu device fuzzer has found several DMA to MMIO issue. > >>>>> These issues is caused by the guest driver programs the DMA > >>>>> address, then in the device MMIO handler it trigger the DMA > >>>>> and as the DMA address is MMIO it will trigger another dispatch > >>>>> and reenter the MMIO handler again. However most of the device > >>>>> is not reentrant. > >>>>> > >>>>> DMA to MMIO will cause issues depend by the device emulator, > >>>>> mostly it will crash the qemu. Following is three classic > >>>>> DMA to MMIO issue. > >>>>> > >>>>> e1000e: https://bugs.launchpad.net/qemu/+bug/1886362 > >>>>> xhci: https://bugs.launchpad.net/qemu/+bug/1891354 > >>>>> virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606 > >>>>> > >>>>> The DMA to MMIO issue I think can be classified as following: > >>>>> 1. DMA to the device itself > >>>>> 2. device A DMA to device B and to device C > >>>>> 3. device A DMA to device B and to device A > >>>>> > >>>>> The first case of course should not be allowed. > >>>>> The second case I think it ok as the device IO handler has no > >>>>> assumption about the IO data came from no matter it come from > >>>>> device or other device. This is for P2P DMA. > >>>>> The third case I think it also should not be allowed. > >>>>> > >>>>> So our issue has been reduced by one case: not allowed the > >>>>> device's IO handler reenter. > >>>>> > >>>>> Paolo suggested that we can refactor the device emulation with > >>>>> BH. However it is a lot of work. > >>>>> I have thought several propose to address this, also discuss > >>>>> this with Jason Wang in private email. > >>>>> > >>>>> I have can solve this issue in core framework or in specific device. > >>>>> After try several methods I choose address it in per-device for > >>>>> following reason: > >>>>> 1. If we address it in core framwork we have to recored and check the > >>>>> device or MR info in MR dispatch write function. Unfortunally we have > >>>>> no these info in core framework. > >>>>> 2. The performance will also be decrease largely > >>>>> 3. Only the device itself know its IO > >>>> I think we still need to seek a way to address this issue completely. > >>>> > >>>> How about adding a flag in MemoryRegionOps and detect the reentrancy through > >>>> that flag? > >>> What happens for devices with multiple MemoryRegions? Make all the > >>> MemoryRegionOps share the same flag? > >> > >> I think there could be two approaches: > >> > >> 1) record the device in MR as Qiang mentioned > > I have tried this as we discussed. But has following consideration: > > 1. The performance, we need to check/record/clean the MR in an array/hashtable. > > > > 2. The multiple MR and alias MR process in the memory layer. It is > > complicated and performance effective. > > So If we let the MR issue to the device itself, it is just as this > > patch does-let the device address the reentrancy issue.f > > > > Another solution. We connects a MR with the corresponding device. Now > > the device often tight MR with an 'opaque' field. > > Just uses it in the calling of MR callback. Then we add a flag in the > > device and needs to modify the MR register interface. > > > > So in the memory layer we can check/record/clean the MR->device->flag. > > But this is can't address the DMA (in BH) to MMIO issue as the BH runs > > in main thread. > > > This is probably good enough to start. To my point of view, there're two > different issues: > > 1) re-entrant MMIO handler > 2) MMIO hanlder sync with BH > Agree, here I want to address these two kind of issue in a manner so it just be left to the device itself. I will try to add a new memory register function memory_region_init_io_with_device to connect the MR and device. And solve it in the memory layer. Thanks, Li Qiang > For 1), we'd better solve it at core, For 2) it can only be solved in > the device. > > Thanks > > > > > > Thanks, > > Li Qiang > > > > > > > >> 2) Only forbid the reentrancy in MMIO handler and depends on the device > >> to solve the multiple Memory Region issue, if the regions want to access > >> the same data, it needs to be synchronized internally > >> > >> But the point is still to try to solve it in the layer of memory > >> regions. Otherwise we may still hit similar issues. > >> > >> > >>> What about the virtio-gpu bug, where the problem happens in a bh->mmio > >>> access rather than an mmio->mmio access? > >> > >> Yes, it needs more thought, but as a first step, we can try to fix the > >> MMIO handler issue and do bh fix on top. > > > > > >> Thanks > >> > >> > >>> -Alex > >>> > >>>> Thanks > >>>> > >>>> > >>>>> The (most of the) device emulation is protected by BQL one time only > >>>>> a device emulation code can be run. We can add a flag to indicate the > >>>>> IO is running. The first two patches does this. For simplicity at the > >>>>> RFC stage I just set it while enter the IO callback and clear it exit > >>>>> the IO callback. It should be check/set/clean according the per-device's > >>>>> IO emulation. > >>>>> The second issue which itself suffers a race condition so I uses a > >>>>> atomic. > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> Li Qiang (3): > >>>>> e1000e: make the IO handler reentrant > >>>>> xhci: make the IO handler reentrant > >>>>> virtio-gpu: make the IO handler reentrant > >>>>> > >>>>> hw/display/virtio-gpu.c | 10 ++++++ > >>>>> hw/net/e1000e.c | 35 +++++++++++++++++++- > >>>>> hw/usb/hcd-xhci.c | 60 ++++++++++++++++++++++++++++++++++ > >>>>> hw/usb/hcd-xhci.h | 1 + > >>>>> include/hw/virtio/virtio-gpu.h | 1 + > >>>>> 5 files changed, 106 insertions(+), 1 deletion(-) > >>>>> > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 3:54 ` [RFC 0/3] try to solve the DMA to MMIO issue Jason Wang 2020-09-03 4:06 ` Alexander Bulekov @ 2020-09-03 10:53 ` Peter Maydell 2020-09-03 11:11 ` Li Qiang 1 sibling, 1 reply; 20+ messages in thread From: Peter Maydell @ 2020-09-03 10:53 UTC (permalink / raw) To: Jason Wang Cc: Dmitry Fleytman, Michael S. Tsirkin, Li Qiang, Li Qiang, QEMU Developers, Alexander Bulekov, Gerd Hoffmann, Paolo Bonzini On Thu, 3 Sep 2020 at 04:55, Jason Wang <jasowang@redhat.com> wrote: > I think we still need to seek a way to address this issue completely. > > How about adding a flag in MemoryRegionOps and detect the reentrancy > through that flag? This won't catch everything. Consider this situation: Device A makes DMA access to device B Device B's write-handling causes it to raise an outbound qemu_irq signal The qemu_irq signal is connected to device A Now we have reentered into device A's code That is to say, the problem is general to "device A does something that affects device B" links of all kinds, which can form loops. Self-DMA is just an easy way to find one category of these with the fuzzer. thanks -- PMM ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 10:53 ` Peter Maydell @ 2020-09-03 11:11 ` Li Qiang 2020-09-03 11:19 ` Peter Maydell 0 siblings, 1 reply; 20+ messages in thread From: Li Qiang @ 2020-09-03 11:11 UTC (permalink / raw) To: Peter Maydell Cc: Dmitry Fleytman, Michael S. Tsirkin, Jason Wang, Li Qiang, QEMU Developers, Alexander Bulekov, Gerd Hoffmann, Paolo Bonzini Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午6:53写道: > > On Thu, 3 Sep 2020 at 04:55, Jason Wang <jasowang@redhat.com> wrote: > > I think we still need to seek a way to address this issue completely. > > > > How about adding a flag in MemoryRegionOps and detect the reentrancy > > through that flag? > > This won't catch everything. Consider this situation: > Device A makes DMA access to device B > Device B's write-handling causes it to raise an > outbound qemu_irq signal > The qemu_irq signal is connected to device A Here mean device A is an interrupt controller? This is special case I think. > Now we have reentered into device A's code > > That is to say, the problem is general to "device A does > something that affects device B" links of all kinds, which As the P2P is a normal behavior, we can't just prevent this. Thanks, Li Qiang > can form loops. Self-DMA is just an easy way to find one > category of these with the fuzzer. > > thanks > -- PMM ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 11:11 ` Li Qiang @ 2020-09-03 11:19 ` Peter Maydell 2020-09-03 11:23 ` Li Qiang 2020-09-04 2:45 ` Jason Wang 0 siblings, 2 replies; 20+ messages in thread From: Peter Maydell @ 2020-09-03 11:19 UTC (permalink / raw) To: Li Qiang Cc: Dmitry Fleytman, Michael S. Tsirkin, Jason Wang, Li Qiang, QEMU Developers, Alexander Bulekov, Gerd Hoffmann, Paolo Bonzini On Thu, 3 Sep 2020 at 12:11, Li Qiang <liq3ea@gmail.com> wrote: > > Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午6:53写道: > > > > On Thu, 3 Sep 2020 at 04:55, Jason Wang <jasowang@redhat.com> wrote: > > > I think we still need to seek a way to address this issue completely. > > > > > > How about adding a flag in MemoryRegionOps and detect the reentrancy > > > through that flag? > > > > This won't catch everything. Consider this situation: > > Device A makes DMA access to device B > > Device B's write-handling causes it to raise an > > outbound qemu_irq signal > > The qemu_irq signal is connected to device A > > Here mean device A is an interrupt controller? No. Any device can have an inbound or outbound qemu_irq line. We use them not just for actual IRQ lines but for any situation where we need to pass an on-or-off signal from one device to another. > This is special case I think. It's an example of why looking purely at MMIO is not sufficient. We should prefer to see if we can come up with a design principle that works for all between-device coordination before we implement something that is specific to MMIO. thanks -- PMM ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 11:19 ` Peter Maydell @ 2020-09-03 11:23 ` Li Qiang 2020-09-03 11:28 ` Peter Maydell 2020-09-04 2:45 ` Jason Wang 1 sibling, 1 reply; 20+ messages in thread From: Li Qiang @ 2020-09-03 11:23 UTC (permalink / raw) To: Peter Maydell Cc: Dmitry Fleytman, Michael S. Tsirkin, Jason Wang, Li Qiang, QEMU Developers, Alexander Bulekov, Gerd Hoffmann, Paolo Bonzini Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午7:19写道: > > On Thu, 3 Sep 2020 at 12:11, Li Qiang <liq3ea@gmail.com> wrote: > > > > Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午6:53写道: > > > > > > On Thu, 3 Sep 2020 at 04:55, Jason Wang <jasowang@redhat.com> wrote: > > > > I think we still need to seek a way to address this issue completely. > > > > > > > > How about adding a flag in MemoryRegionOps and detect the reentrancy > > > > through that flag? > > > > > > This won't catch everything. Consider this situation: > > > Device A makes DMA access to device B > > > Device B's write-handling causes it to raise an > > > outbound qemu_irq signal > > > The qemu_irq signal is connected to device A > > > > Here mean device A is an interrupt controller? > > No. Any device can have an inbound or outbound qemu_irq line. > We use them not just for actual IRQ lines but for any > situation where we need to pass an on-or-off signal from > one device to another. Could you please provide some example, I haven't noticed this before. Thanks. > > > This is special case I think. > > It's an example of why looking purely at MMIO is not > sufficient. We should prefer to see if we can come up with > a design principle that works for all between-device > coordination before we implement something that is specific > to MMIO. So first we may be define a clean boundary/interface between device coordination? Thanks, Li Qiang > > thanks > -- PMM ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 11:23 ` Li Qiang @ 2020-09-03 11:28 ` Peter Maydell 2020-09-03 13:35 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 20+ messages in thread From: Peter Maydell @ 2020-09-03 11:28 UTC (permalink / raw) To: Li Qiang Cc: Dmitry Fleytman, Michael S. Tsirkin, Jason Wang, Li Qiang, QEMU Developers, Alexander Bulekov, Gerd Hoffmann, Paolo Bonzini On Thu, 3 Sep 2020 at 12:24, Li Qiang <liq3ea@gmail.com> wrote: > Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午7:19写道: > > No. Any device can have an inbound or outbound qemu_irq line. > > We use them not just for actual IRQ lines but for any > > situation where we need to pass an on-or-off signal from > > one device to another. > > Could you please provide some example, I haven't noticed this before. Look at any device that calls qdev_init_gpio_in() or qdev_init_gpio_in_named() for an example of inbound signals. Outbound signals might be created via qdev_init_gpio_out(), qdev_init_gpio_out_named() or sysbus_init_irq(). thanks -- PMM ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 11:28 ` Peter Maydell @ 2020-09-03 13:35 ` Philippe Mathieu-Daudé 2020-09-03 13:41 ` Peter Maydell 0 siblings, 1 reply; 20+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-03 13:35 UTC (permalink / raw) To: Peter Maydell, Li Qiang Cc: Dmitry Fleytman, Michael S. Tsirkin, Jason Wang, Li Qiang, QEMU Developers, Alexander Bulekov, Gerd Hoffmann, Paolo Bonzini On 9/3/20 1:28 PM, Peter Maydell wrote: > On Thu, 3 Sep 2020 at 12:24, Li Qiang <liq3ea@gmail.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午7:19写道: >>> No. Any device can have an inbound or outbound qemu_irq line. >>> We use them not just for actual IRQ lines but for any >>> situation where we need to pass an on-or-off signal from >>> one device to another. >> >> Could you please provide some example, I haven't noticed this before. > > Look at any device that calls qdev_init_gpio_in() or > qdev_init_gpio_in_named() for an example of inbound signals. > Outbound signals might be created via qdev_init_gpio_out(), > qdev_init_gpio_out_named() or sysbus_init_irq(). Not sure if this is a valid example, but when adding: -- >8 -- diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c index bca71b5934b..b8b4ba362b1 100644 --- a/hw/intc/ioapic.c +++ b/hw/intc/ioapic.c @@ -96,6 +96,8 @@ static void ioapic_service(IOAPICCommonState *s) uint32_t mask; uint64_t entry; + assert(!resettable_is_in_reset(OBJECT(s))); + for (i = 0; i < IOAPIC_NUM_PINS; i++) { mask = 1 << i; if (s->irr & mask) { --- I get a MMIO write triggered from an IRQ: (gdb) bt #3 0x0000555558e44a12 in memory_region_write_accessor (mr=0x61600001ab10, addr=0, value=0x7fffffffaa10, size=4, shift=0, mask=4294967295, attrs=...) at softmmu/memory.c:482 #4 0x0000555558e4453b in access_with_adjusted_size (addr=0, value=0x7fffffffaa10, size=4, access_size_min=1, access_size_max=4, access_fn= 0x555558e44600 <memory_region_write_accessor>, mr=0x61600001ab10, attrs=...) at softmmu/memory.c:545 #5 0x0000555558e42c56 in memory_region_dispatch_write (mr=0x61600001ab10, addr=0, data=0, op=MO_32, attrs=...) at softmmu/memory.c:1466 #6 0x0000555558f322b3 in address_space_stl_internal (as=0x55555c0120e0 <address_space_memory>, addr=4276092928, val=0, attrs=..., result=0x0, endian=DEVICE_LITTLE_ENDIAN) at memory_ldst.c.inc:315 #7 0x0000555558f32802 in address_space_stl_le (as=0x55555c0120e0 <address_space_memory>, addr=4276092928, val=0, attrs=..., result=0x0) at memory_ldst.c.inc:353 #8 0x0000555558be2e22 in stl_le_phys (as=0x55555c0120e0 <address_space_memory>, addr=4276092928, val=0) at /home/phil/source/qemu/include/exec/memory_ldst_phys.h.inc:103 #9 0x0000555558be0e14 in ioapic_service (s=0x61b000002a80) at hw/intc/ioapic.c:138 #10 0x0000555558be4901 in ioapic_set_irq (opaque=0x61b000002a80, vector=2, level=1) at hw/intc/ioapic.c:186 #11 0x00005555598769f6 in qemu_set_irq (irq=0x606000040f40, level=1) at hw/core/irq.c:44 #12 0x00005555585fc097 in gsi_handler (opaque=0x61200000b8c0, n=0, level=1) at hw/i386/x86.c:336 #13 0x00005555598769f6 in qemu_set_irq (irq=0x60600003db80, level=1) at hw/core/irq.c:44 #14 0x0000555557653047 in hpet_handle_legacy_irq (opaque=0x61f000000080, n=0, level=1) at hw/timer/hpet.c:707 #15 0x00005555598769f6 in qemu_set_irq (irq=0x606000042500, level=1) at hw/core/irq.c:44 #16 0x00005555571c0686 in pit_irq_timer_update (s=0x616000032018, current_time=0) at hw/timer/i8254.c:262 #17 0x00005555571c01c9 in pit_irq_control (opaque=0x616000031e80, n=0, enable=1) at hw/timer/i8254.c:304 #18 0x00005555598769f6 in qemu_set_irq (irq=0x6060000435e0, level=1) at hw/core/irq.c:44 #19 0x00005555576518cb in hpet_reset (d=0x61f000000080) at hw/timer/hpet.c:690 #20 0x000055555986dfbe in device_transitional_reset (obj=0x61f000000080) at hw/core/qdev.c:1114 #21 0x0000555559870e8e in resettable_phase_hold (obj=0x61f000000080, opaque=0x0, type=RESET_TYPE_COLD) at hw/core/resettable.c:182 #22 0x0000555559846add in bus_reset_child_foreach (obj=0x60c00002e000, cb=0x5555598707e0 <resettable_phase_hold>, opaque=0x0, type=RESET_TYPE_COLD) at hw/core/bus.c:94 #23 0x0000555559873c29 in resettable_child_foreach (rc=0x60e00003e160, obj=0x60c00002e000, cb=0x5555598707e0 <resettable_phase_hold>, opaque=0x0, type=RESET_TYPE_COLD) at hw/core/resettable.c:96 #24 0x0000555559870b01 in resettable_phase_hold (obj=0x60c00002e000, opaque=0x0, type=RESET_TYPE_COLD) at hw/core/resettable.c:173 #25 0x000055555986fbc3 in resettable_assert_reset (obj=0x60c00002e000, type=RESET_TYPE_COLD) at hw/core/resettable.c:60 #26 0x000055555986fa6a in resettable_reset (obj=0x60c00002e000, type=RESET_TYPE_COLD) at hw/core/resettable.c:45 #27 0x00005555598725ba in resettable_cold_reset_fn (opaque=0x60c00002e000) at hw/core/resettable.c:269 #28 0x000055555986f9e9 in qemu_devices_reset () at hw/core/reset.c:69 #29 0x000055555865d711 in pc_machine_reset (machine=0x615000020100) at hw/i386/pc.c:1901 #30 0x00005555589ea197 in qemu_system_reset (reason=SHUTDOWN_CAUSE_NONE) at softmmu/vl.c:1403 #31 0x00005555589f7738 in qemu_init (argc=16, argv=0x7fffffffd278, envp=0x7fffffffd300) at softmmu/vl.c:4458 #32 0x00005555571615fa in main (argc=16, argv=0x7fffffffd278, envp=0x7fffffffd300) at softmmu/main.c:49 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 13:35 ` Philippe Mathieu-Daudé @ 2020-09-03 13:41 ` Peter Maydell 0 siblings, 0 replies; 20+ messages in thread From: Peter Maydell @ 2020-09-03 13:41 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Dmitry Fleytman, Michael S. Tsirkin, Li Qiang, Jason Wang, Li Qiang, QEMU Developers, Alexander Bulekov, Gerd Hoffmann, Paolo Bonzini On Thu, 3 Sep 2020 at 14:36, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Not sure if this is a valid example, but when adding: > > -- >8 -- > diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c > index bca71b5934b..b8b4ba362b1 100644 > --- a/hw/intc/ioapic.c > +++ b/hw/intc/ioapic.c > @@ -96,6 +96,8 @@ static void ioapic_service(IOAPICCommonState *s) > uint32_t mask; > uint64_t entry; > > + assert(!resettable_is_in_reset(OBJECT(s))); > + > for (i = 0; i < IOAPIC_NUM_PINS; i++) { > mask = 1 << i; > if (s->irr & mask) { > --- > > I get a MMIO write triggered from an IRQ: Yeah, IRQs can trigger MMIO writes. In this case one underlying problem is that the hpet_reset() code is asserting a qemu_irq in a reset phase that it should not, because it's an old-style reset function and not a new-style 3-phase one (which would do the assertion of the IRQ only in the 3rd phase). I don't think this is a case of ending up with a recursive re-entry into the code of the original device, though. thanks -- PMM ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/3] try to solve the DMA to MMIO issue 2020-09-03 11:19 ` Peter Maydell 2020-09-03 11:23 ` Li Qiang @ 2020-09-04 2:45 ` Jason Wang 1 sibling, 0 replies; 20+ messages in thread From: Jason Wang @ 2020-09-04 2:45 UTC (permalink / raw) To: Peter Maydell, Li Qiang Cc: Dmitry Fleytman, Michael S. Tsirkin, Li Qiang, QEMU Developers, Alexander Bulekov, Gerd Hoffmann, Paolo Bonzini On 2020/9/3 下午7:19, Peter Maydell wrote: > On Thu, 3 Sep 2020 at 12:11, Li Qiang <liq3ea@gmail.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午6:53写道: >>> On Thu, 3 Sep 2020 at 04:55, Jason Wang <jasowang@redhat.com> wrote: >>>> I think we still need to seek a way to address this issue completely. >>>> >>>> How about adding a flag in MemoryRegionOps and detect the reentrancy >>>> through that flag? >>> This won't catch everything. Consider this situation: >>> Device A makes DMA access to device B >>> Device B's write-handling causes it to raise an >>> outbound qemu_irq signal >>> The qemu_irq signal is connected to device A >> Here mean device A is an interrupt controller? > No. Any device can have an inbound or outbound qemu_irq line. > We use them not just for actual IRQ lines but for any > situation where we need to pass an on-or-off signal from > one device to another. > >> This is special case I think. > It's an example of why looking purely at MMIO is not > sufficient. We should prefer to see if we can come up with > a design principle that works for all between-device > coordination before we implement something that is specific > to MMIO. As discussed, maybe we can track the pending operations in device itself and check it in all the possible entry of device codes (irq, MMIO or what ever else). This may be easier for stable backport. Thanks > > thanks > -- PMM > ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-09-04 2:46 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-02 16:22 [RFC 0/3] try to solve the DMA to MMIO issue Li Qiang 2020-09-02 16:22 ` [RFC 1/3] e1000e: make the IO handler reentrant Li Qiang 2020-09-02 16:22 ` [RFC 2/3] xhci: " Li Qiang 2020-09-02 16:22 ` [RFC 3/3] virtio-gpu: " Li Qiang 2020-09-03 5:12 ` Michael Tokarev 2020-09-03 10:32 ` Li Qiang 2020-09-03 3:54 ` [RFC 0/3] try to solve the DMA to MMIO issue Jason Wang 2020-09-03 4:06 ` Alexander Bulekov 2020-09-03 4:24 ` Jason Wang 2020-09-03 4:50 ` Li Qiang 2020-09-03 6:16 ` Jason Wang 2020-09-03 6:28 ` Li Qiang 2020-09-03 10:53 ` Peter Maydell 2020-09-03 11:11 ` Li Qiang 2020-09-03 11:19 ` Peter Maydell 2020-09-03 11:23 ` Li Qiang 2020-09-03 11:28 ` Peter Maydell 2020-09-03 13:35 ` Philippe Mathieu-Daudé 2020-09-03 13:41 ` Peter Maydell 2020-09-04 2:45 ` Jason Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.