[PATCH v6 0/4] memory: prevent dma-reentracy issues

[PATCH v6 0/4] memory: prevent dma-reentracy issues

[Alexander Bulekov]

These patches aim to solve two types of DMA-reentrancy issues:
  
1.) mmio -> dma -> mmio case
To solve this, we track whether the device is engaged in io by
checking/setting a reentrancy-guard within APIs used for MMIO access.
  
2.) bh -> dma write -> mmio case
This case is trickier, since we dont have a generic way to associate a
bh with the underlying Device/DeviceState. Thus, this version allows a
device to associate a reentrancy-guard with a bh, when creating it.
(Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)
  
I replaced most of the qemu_bh_new invocations with the guarded analog,
except for the ones where the DeviceState was not trivially accessible.

v5 -> v6:
    - Only apply checkpatch checks to code in paths containing "/hw/"
      (/hw/ and include/hw/)
    - Fix a bug in a _guarded call added to hw/block/virtio-blk.c
v4-> v5:
    - Add corresponding checkpatch checks
    - Save/restore reentrancy-flag when entering/exiting BHs
    - Improve documentation
    - Check object_dynamic_cast return value
  
v3 -> v4: Instead of changing all of the DMA APIs, instead add an
    optional reentrancy guard to the BH API.

v2 -> v3: Bite the bullet and modify the DMA APIs, rather than
    attempting to guess DeviceStates in BHs.

Alexander Bulekov (4):
  memory: prevent dma-reentracy issues
  async: Add an optional reentrancy guard to the BH API
  checkpatch: add qemu_bh_new/aio_bh_new checks
  hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

 docs/devel/multiple-iothreads.txt |  7 +++++++
 hw/9pfs/xen-9p-backend.c          |  4 +++-
 hw/block/dataplane/virtio-blk.c   |  3 ++-
 hw/block/dataplane/xen-block.c    |  5 +++--
 hw/char/virtio-serial-bus.c       |  3 ++-
 hw/display/qxl.c                  |  9 ++++++---
 hw/display/virtio-gpu.c           |  6 ++++--
 hw/ide/ahci.c                     |  3 ++-
 hw/ide/core.c                     |  3 ++-
 hw/misc/imx_rngc.c                |  6 ++++--
 hw/misc/macio/mac_dbdma.c         |  2 +-
 hw/net/virtio-net.c               |  3 ++-
 hw/nvme/ctrl.c                    |  6 ++++--
 hw/scsi/mptsas.c                  |  3 ++-
 hw/scsi/scsi-bus.c                |  3 ++-
 hw/scsi/vmw_pvscsi.c              |  3 ++-
 hw/usb/dev-uas.c                  |  3 ++-
 hw/usb/hcd-dwc2.c                 |  3 ++-
 hw/usb/hcd-ehci.c                 |  3 ++-
 hw/usb/hcd-uhci.c                 |  2 +-
 hw/usb/host-libusb.c              |  6 ++++--
 hw/usb/redirect.c                 |  6 ++++--
 hw/usb/xen-usb.c                  |  3 ++-
 hw/virtio/virtio-balloon.c        |  5 +++--
 hw/virtio/virtio-crypto.c         |  3 ++-
 include/block/aio.h               | 18 ++++++++++++++++--
 include/hw/qdev-core.h            |  7 +++++++
 include/qemu/main-loop.h          |  7 +++++--
 scripts/checkpatch.pl             |  8 ++++++++
 softmmu/memory.c                  | 17 +++++++++++++++++
 softmmu/trace-events              |  1 +
 tests/unit/ptimer-test-stubs.c    |  3 ++-
 util/async.c                      | 18 +++++++++++++++++-
 util/main-loop.c                  |  5 +++--
 util/trace-events                 |  1 +
 35 files changed, 147 insertions(+), 41 deletions(-)

-- 
2.39.0

[PATCH v6 1/4] memory: prevent dma-reentracy issues

[Alexander Bulekov]

Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
This flag is set/checked prior to calling a device's MemoryRegion
handlers, and set when device code initiates DMA.  The purpose of this
flag is to prevent two types of DMA-based reentrancy issues:

1.) mmio -> dma -> mmio case
2.) bh -> dma write -> mmio case

These issues have led to problems such as stack-exhaustion and
use-after-frees.

Summary of the problem from Peter Maydell:
https://lore.kernel.org/qemu-devel/CAFEAcA_23vc7hE3iaM-JVA6W38LK4hJoWae5KcknhPRD5fPBZA@mail.gmail.com

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
Acked-by: Peter Xu <peterx@redhat.com>
---
 include/hw/qdev-core.h |  7 +++++++
 softmmu/memory.c       | 17 +++++++++++++++++
 softmmu/trace-events   |  1 +
 3 files changed, 25 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 35fddb19a6..8858195262 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -162,6 +162,10 @@ struct NamedClockList {
     QLIST_ENTRY(NamedClockList) node;
 };
 
+typedef struct {
+    bool engaged_in_io;
+} MemReentrancyGuard;
+
 /**
  * DeviceState:
  * @realized: Indicates whether the device has been fully constructed.
@@ -194,6 +198,9 @@ struct DeviceState {
     int alias_required_for_version;
     ResettableState reset;
     GSList *unplug_blockers;
+
+    /* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
+    MemReentrancyGuard mem_reentrancy_guard;
 };
 
 struct DeviceListener {
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 9d64efca26..eefeeae317 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -533,6 +533,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
     uint64_t access_mask;
     unsigned access_size;
     unsigned i;
+    DeviceState *dev = NULL;
     MemTxResult r = MEMTX_OK;
 
     if (!access_size_min) {
@@ -542,6 +543,19 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
         access_size_max = 4;
     }
 
+    /* Do not allow more than one simultanous access to a device's IO Regions */
+    if (mr->owner &&
+        !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
+        dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
+        if (dev) {
+            if (dev->mem_reentrancy_guard.engaged_in_io) {
+                trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
+                return MEMTX_ERROR;
+            }
+            dev->mem_reentrancy_guard.engaged_in_io = true;
+        }
+    }
+
     /* FIXME: support unaligned access? */
     access_size = MAX(MIN(size, access_size_max), access_size_min);
     access_mask = MAKE_64BIT_MASK(0, access_size * 8);
@@ -556,6 +570,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
                         access_mask, attrs);
         }
     }
+    if (dev) {
+        dev->mem_reentrancy_guard.engaged_in_io = false;
+    }
     return r;
 }
 
diff --git a/softmmu/trace-events b/softmmu/trace-events
index 22606dc27b..62d04ea9a7 100644
--- a/softmmu/trace-events
+++ b/softmmu/trace-events
@@ -13,6 +13,7 @@ memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, u
 memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'"
 memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
+memory_region_reentrant_io(int cpu_index, void *mr, uint64_t offset, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" size %u"
 memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_sync_dirty(const char *mr, const char *listener, int global) "mr '%s' listener '%s' synced (global=%d)"
-- 
2.39.0

[PATCH v6 2/4] async: Add an optional reentrancy guard to the BH API

[Alexander Bulekov]

Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 docs/devel/multiple-iothreads.txt |  7 +++++++
 include/block/aio.h               | 18 ++++++++++++++++--
 include/qemu/main-loop.h          |  7 +++++--
 tests/unit/ptimer-test-stubs.c    |  3 ++-
 util/async.c                      | 18 +++++++++++++++++-
 util/main-loop.c                  |  5 +++--
 util/trace-events                 |  1 +
 7 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/docs/devel/multiple-iothreads.txt b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
  * LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
  * LEGACY timer_new_ms() - create a timer
  * LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
  * LEGACY qemu_aio_wait() - run an event loop iteration
 
 Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see include/block/aio.h):
  * aio_set_event_notifier() - monitor an event notifier
  * aio_timer_new() - create a timer
  * aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
  * aio_poll() - run an event loop iteration
 
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
 The AioContext can be obtained from the IOThread using
 iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
 Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index 8fba6a3584..3e3bdb9352 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
 #include "qemu/thread.h"
 #include "qemu/timer.h"
 #include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
 
 typedef struct BlockAIOCB BlockAIOCB;
 typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -331,9 +333,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
  * is opaque and must be allocated prior to its use.
  *
  * @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
  */
 QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-                        const char *name);
+                        const char *name, MemReentrancyGuard *reentrancy_guard);
 
 /**
  * aio_bh_new: Allocate a new bottom half structure
@@ -342,7 +346,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
  * string.
  */
 #define aio_bh_new(ctx, cb, opaque) \
-    aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+    aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+    aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
 
 /**
  * aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index c25f390696..84d1ce57f0 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -389,9 +389,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
 
 void qemu_fd_register(int fd);
 
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+    qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
 #define qemu_bh_new(cb, opaque) \
-    qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+    qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+                         MemReentrancyGuard *reentrancy_guard);
 void qemu_bh_schedule_idle(QEMUBH *bh);
 
 enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f5e75a96b6..24d5413f9d 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int attr_mask)
     return deadline;
 }
 
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+                         MemReentrancyGuard *reentrancy_guard)
 {
     QEMUBH *bh = g_new(QEMUBH, 1);
 
diff --git a/util/async.c b/util/async.c
index 0657b75397..fbebfd7897 100644
--- a/util/async.c
+++ b/util/async.c
@@ -65,6 +65,7 @@ struct QEMUBH {
     void *opaque;
     QSLIST_ENTRY(QEMUBH) next;
     unsigned flags;
+    MemReentrancyGuard *reentrancy_guard;
 };
 
 /* Called concurrently from any thread */
@@ -133,7 +134,7 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx, QEMUBHFunc *cb,
 }
 
 QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-                        const char *name)
+                        const char *name, MemReentrancyGuard *reentrancy_guard)
 {
     QEMUBH *bh;
     bh = g_new(QEMUBH, 1);
@@ -142,13 +143,28 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
         .cb = cb,
         .opaque = opaque,
         .name = name,
+        .reentrancy_guard = reentrancy_guard,
     };
     return bh;
 }
 
 void aio_bh_call(QEMUBH *bh)
 {
+    bool last_engaged_in_io = false;
+
+    if (bh->reentrancy_guard) {
+        last_engaged_in_io = bh->reentrancy_guard->engaged_in_io;
+        if (bh->reentrancy_guard->engaged_in_io) {
+            trace_reentrant_aio(bh->ctx, bh->name);
+        }
+        bh->reentrancy_guard->engaged_in_io = true;
+    }
+
     bh->cb(bh->opaque);
+
+    if (bh->reentrancy_guard) {
+        bh->reentrancy_guard->engaged_in_io = last_engaged_in_io;
+    }
 }
 
 /* Multiple occurrences of aio_bh_poll cannot be called concurrently. */
diff --git a/util/main-loop.c b/util/main-loop.c
index 58f776a8c9..07d2e2040a 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -617,9 +617,10 @@ void main_loop_wait(int nonblocking)
 
 /* Functions to operate on the main QEMU AioContext.  */
 
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name, MemReentrancyGuard *reentrancy_guard)
 {
-    return aio_bh_new_full(qemu_aio_context, cb, opaque, name);
+    return aio_bh_new_full(qemu_aio_context, cb, opaque, name,
+                           reentrancy_guard);
 }
 
 /*
diff --git a/util/trace-events b/util/trace-events
index c8f53d7d9f..dc3b1eb3bf 100644
--- a/util/trace-events
+++ b/util/trace-events
@@ -11,6 +11,7 @@ poll_remove(void *ctx, void *node, int fd) "ctx %p node %p fd %d"
 # async.c
 aio_co_schedule(void *ctx, void *co) "ctx %p co %p"
 aio_co_schedule_bh_cb(void *ctx, void *co) "ctx %p co %p"
+reentrant_aio(void *ctx, const char *name) "ctx %p name %s"
 
 # thread-pool.c
 thread_pool_submit(void *pool, void *req, void *opaque) "pool %p req %p opaque %p"
-- 
2.39.0

[PATCH v6 3/4] checkpatch: add qemu_bh_new/aio_bh_new checks

[Alexander Bulekov]

Advise authors to use the _guarded versions of the APIs, instead.

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 scripts/checkpatch.pl | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6ecabfb2b5..fbb71c70f8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2865,6 +2865,14 @@ sub process {
 		if ($line =~ /\bsignal\s*\(/ && !($line =~ /SIG_(?:IGN|DFL)/)) {
 			ERROR("use sigaction to establish signal handlers; signal is not portable\n" . $herecurr);
 		}
+# recommend qemu_bh_new_guarded instead of qemu_bh_new
+        if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\bqemu_bh_new\s*\(/) {
+			ERROR("use qemu_bh_new_guarded() instead of qemu_bh_new() to avoid reentrancy problems\n" . $herecurr);
+		}
+# recommend aio_bh_new_guarded instead of aio_bh_new
+        if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\baio_bh_new\s*\(/) {
+			ERROR("use aio_bh_new_guarded() instead of aio_bh_new() to avoid reentrancy problems\n" . $herecurr);
+		}
 # check for module_init(), use category-specific init macros explicitly please
 		if ($line =~ /^module_init\s*\(/) {
 			ERROR("please use block_init(), type_init() etc. instead of module_init()\n" . $herecurr);
-- 
2.39.0

[PATCH v6 4/4] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

[Alexander Bulekov]

This protects devices from bh->mmio reentrancy issues.

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 hw/9pfs/xen-9p-backend.c        | 4 +++-
 hw/block/dataplane/virtio-blk.c | 3 ++-
 hw/block/dataplane/xen-block.c  | 5 +++--
 hw/char/virtio-serial-bus.c     | 3 ++-
 hw/display/qxl.c                | 9 ++++++---
 hw/display/virtio-gpu.c         | 6 ++++--
 hw/ide/ahci.c                   | 3 ++-
 hw/ide/core.c                   | 3 ++-
 hw/misc/imx_rngc.c              | 6 ++++--
 hw/misc/macio/mac_dbdma.c       | 2 +-
 hw/net/virtio-net.c             | 3 ++-
 hw/nvme/ctrl.c                  | 6 ++++--
 hw/scsi/mptsas.c                | 3 ++-
 hw/scsi/scsi-bus.c              | 3 ++-
 hw/scsi/vmw_pvscsi.c            | 3 ++-
 hw/usb/dev-uas.c                | 3 ++-
 hw/usb/hcd-dwc2.c               | 3 ++-
 hw/usb/hcd-ehci.c               | 3 ++-
 hw/usb/hcd-uhci.c               | 2 +-
 hw/usb/host-libusb.c            | 6 ++++--
 hw/usb/redirect.c               | 6 ++++--
 hw/usb/xen-usb.c                | 3 ++-
 hw/virtio/virtio-balloon.c      | 5 +++--
 hw/virtio/virtio-crypto.c       | 3 ++-
 24 files changed, 63 insertions(+), 33 deletions(-)

diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 65c4979c3c..f077c1b255 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -441,7 +441,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
         xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
                                        XEN_FLEX_RING_SIZE(ring_order);
 
-        xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh, &xen_9pdev->rings[i]);
+        xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+                                                     &xen_9pdev->rings[i],
+                                                     &DEVICE(xen_9pdev)->mem_reentrancy_guard);
         xen_9pdev->rings[i].out_cons = 0;
         xen_9pdev->rings[i].out_size = 0;
         xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
     } else {
         s->ctx = qemu_get_aio_context();
     }
-    s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+    s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+                               &DEVICE(vdev)->mem_reentrancy_guard);
     s->batch_notify_vqs = bitmap_new(conf->num_queues);
 
     *dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 2785b9e849..e31806b317 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -632,8 +632,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice *xendev,
     } else {
         dataplane->ctx = qemu_get_aio_context();
     }
-    dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
-                               dataplane);
+    dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+                                       dataplane,
+                                       &DEVICE(xendev)->mem_reentrancy_guard);
 
     return dataplane;
 }
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    port->bh = qemu_bh_new(flush_queued_data_bh, port);
+    port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+                                   &dev->mem_reentrancy_guard);
     port->elem = NULL;
 }
 
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index ec712d3ca2..c0460c4ef1 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
 
     qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
 
-    qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+    qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+                                          &DEVICE(qxl)->mem_reentrancy_guard);
     qxl_reset_state(qxl);
 
-    qxl->update_area_bh = qemu_bh_new(qxl_render_update_area_bh, qxl);
-    qxl->ssd.cursor_bh = qemu_bh_new(qemu_spice_cursor_refresh_bh, &qxl->ssd);
+    qxl->update_area_bh = qemu_bh_new_guarded(qxl_render_update_area_bh, qxl,
+                                              &DEVICE(qxl)->mem_reentrancy_guard);
+    qxl->ssd.cursor_bh = qemu_bh_new_guarded(qemu_spice_cursor_refresh_bh, &qxl->ssd,
+                                             &DEVICE(qxl)->mem_reentrancy_guard);
 }
 
 static void qxl_realize_primary(PCIDevice *dev, Error **errp)
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5e15c79b94..66ac9b6cc5 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1339,8 +1339,10 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
 
     g->ctrl_vq = virtio_get_queue(vdev, 0);
     g->cursor_vq = virtio_get_queue(vdev, 1);
-    g->ctrl_bh = qemu_bh_new(virtio_gpu_ctrl_bh, g);
-    g->cursor_bh = qemu_bh_new(virtio_gpu_cursor_bh, g);
+    g->ctrl_bh = qemu_bh_new_guarded(virtio_gpu_ctrl_bh, g,
+                                     &qdev->mem_reentrancy_guard);
+    g->cursor_bh = qemu_bh_new_guarded(virtio_gpu_cursor_bh, g,
+                                       &qdev->mem_reentrancy_guard);
     QTAILQ_INIT(&g->reslist);
     QTAILQ_INIT(&g->cmdq);
     QTAILQ_INIT(&g->fenceq);
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 7ce001cacd..37091150cb 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1508,7 +1508,8 @@ static void ahci_cmd_done(const IDEDMA *dma)
     ahci_write_fis_d2h(ad);
 
     if (ad->port_regs.cmd_issue && !ad->check_bh) {
-        ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
+        ad->check_bh = qemu_bh_new_guarded(ahci_check_cmd_bh, ad,
+                                           &DEVICE(ad)->mem_reentrancy_guard);
         qemu_bh_schedule(ad->check_bh);
     }
 }
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 5d1039378f..8c8d1a8ec2 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -519,7 +519,8 @@ BlockAIOCB *ide_issue_trim(
 
     iocb = blk_aio_get(&trim_aiocb_info, s->blk, cb, cb_opaque);
     iocb->s = s;
-    iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
+    iocb->bh = qemu_bh_new_guarded(ide_trim_bh_cb, iocb,
+                                   &DEVICE(s)->mem_reentrancy_guard);
     iocb->ret = 0;
     iocb->qiov = qiov;
     iocb->i = -1;
diff --git a/hw/misc/imx_rngc.c b/hw/misc/imx_rngc.c
index 632c03779c..082c6980ad 100644
--- a/hw/misc/imx_rngc.c
+++ b/hw/misc/imx_rngc.c
@@ -228,8 +228,10 @@ static void imx_rngc_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->iomem);
 
     sysbus_init_irq(sbd, &s->irq);
-    s->self_test_bh = qemu_bh_new(imx_rngc_self_test, s);
-    s->seed_bh = qemu_bh_new(imx_rngc_seed, s);
+    s->self_test_bh = qemu_bh_new_guarded(imx_rngc_self_test, s,
+                                          &dev->mem_reentrancy_guard);
+    s->seed_bh = qemu_bh_new_guarded(imx_rngc_seed, s,
+                                     &dev->mem_reentrancy_guard);
 }
 
 static void imx_rngc_reset(DeviceState *dev)
diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 43bb1f56ba..80a789f32b 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -914,7 +914,7 @@ static void mac_dbdma_realize(DeviceState *dev, Error **errp)
 {
     DBDMAState *s = MAC_DBDMA(dev);
 
-    s->bh = qemu_bh_new(DBDMA_run_bh, s);
+    s->bh = qemu_bh_new_guarded(DBDMA_run_bh, s, &dev->mem_reentrancy_guard);
 }
 
 static void mac_dbdma_class_init(ObjectClass *oc, void *data)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3ae909041a..a170c724de 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2885,7 +2885,8 @@ static void virtio_net_add_queue(VirtIONet *n, int index)
         n->vqs[index].tx_vq =
             virtio_add_queue(vdev, n->net_conf.tx_queue_size,
                              virtio_net_handle_tx_bh);
-        n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]);
+        n->vqs[index].tx_bh = qemu_bh_new_guarded(virtio_net_tx_bh, &n->vqs[index],
+                                                  &DEVICE(vdev)->mem_reentrancy_guard);
     }
 
     n->vqs[index].tx_waiting = 0;
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index f25cc2c235..dcb250e772 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4318,7 +4318,8 @@ static void nvme_init_sq(NvmeSQueue *sq, NvmeCtrl *n, uint64_t dma_addr,
         QTAILQ_INSERT_TAIL(&(sq->req_list), &sq->io_req[i], entry);
     }
 
-    sq->bh = qemu_bh_new(nvme_process_sq, sq);
+    sq->bh = qemu_bh_new_guarded(nvme_process_sq, sq,
+                                 &DEVICE(sq->ctrl)->mem_reentrancy_guard);
 
     if (n->dbbuf_enabled) {
         sq->db_addr = n->dbbuf_dbs + (sqid << 3);
@@ -4708,7 +4709,8 @@ static void nvme_init_cq(NvmeCQueue *cq, NvmeCtrl *n, uint64_t dma_addr,
         }
     }
     n->cq[cqid] = cq;
-    cq->bh = qemu_bh_new(nvme_post_cqes, cq);
+    cq->bh = qemu_bh_new_guarded(nvme_post_cqes, cq,
+                                 &DEVICE(cq->ctrl)->mem_reentrancy_guard);
 }
 
 static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest *req)
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index c485da792c..3de288b454 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1322,7 +1322,8 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
     }
     s->max_devices = MPTSAS_NUM_PORTS;
 
-    s->request_bh = qemu_bh_new(mptsas_fetch_requests, s);
+    s->request_bh = qemu_bh_new_guarded(mptsas_fetch_requests, s,
+                                        &DEVICE(dev)->mem_reentrancy_guard);
 
     scsi_bus_init(&s->bus, sizeof(s->bus), &dev->qdev, &mptsas_scsi_info);
 }
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index ceceafb2cd..e5c9f7a53d 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -193,7 +193,8 @@ static void scsi_dma_restart_cb(void *opaque, bool running, RunState state)
         AioContext *ctx = blk_get_aio_context(s->conf.blk);
         /* The reference is dropped in scsi_dma_restart_bh.*/
         object_ref(OBJECT(s));
-        s->bh = aio_bh_new(ctx, scsi_dma_restart_bh, s);
+        s->bh = aio_bh_new_guarded(ctx, scsi_dma_restart_bh, s,
+                                   &DEVICE(s)->mem_reentrancy_guard);
         qemu_bh_schedule(s->bh);
     }
 }
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index fa76696855..4de34536e9 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1184,7 +1184,8 @@ pvscsi_realizefn(PCIDevice *pci_dev, Error **errp)
         pcie_endpoint_cap_init(pci_dev, PVSCSI_EXP_EP_OFFSET);
     }
 
-    s->completion_worker = qemu_bh_new(pvscsi_process_completion_queue, s);
+    s->completion_worker = qemu_bh_new_guarded(pvscsi_process_completion_queue, s,
+                                               &DEVICE(pci_dev)->mem_reentrancy_guard);
 
     scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(pci_dev), &pvscsi_scsi_info);
     /* override default SCSI bus hotplug-handler, with pvscsi's one */
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 88f99c05d5..f013ded91e 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -937,7 +937,8 @@ static void usb_uas_realize(USBDevice *dev, Error **errp)
 
     QTAILQ_INIT(&uas->results);
     QTAILQ_INIT(&uas->requests);
-    uas->status_bh = qemu_bh_new(usb_uas_send_status_bh, uas);
+    uas->status_bh = qemu_bh_new_guarded(usb_uas_send_status_bh, uas,
+                                         &d->mem_reentrancy_guard);
 
     dev->flags |= (1 << USB_DEV_FLAG_IS_SCSI_STORAGE);
     scsi_bus_init(&uas->bus, sizeof(uas->bus), DEVICE(dev), &usb_uas_scsi_info);
diff --git a/hw/usb/hcd-dwc2.c b/hw/usb/hcd-dwc2.c
index 8755e9cbb0..a0c4e782b2 100644
--- a/hw/usb/hcd-dwc2.c
+++ b/hw/usb/hcd-dwc2.c
@@ -1364,7 +1364,8 @@ static void dwc2_realize(DeviceState *dev, Error **errp)
     s->fi = USB_FRMINTVL - 1;
     s->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, dwc2_frame_boundary, s);
     s->frame_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, dwc2_work_timer, s);
-    s->async_bh = qemu_bh_new(dwc2_work_bh, s);
+    s->async_bh = qemu_bh_new_guarded(dwc2_work_bh, s,
+                                      &dev->mem_reentrancy_guard);
 
     sysbus_init_irq(sbd, &s->irq);
 }
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index d4da8dcb8d..c930c60921 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2533,7 +2533,8 @@ void usb_ehci_realize(EHCIState *s, DeviceState *dev, Error **errp)
     }
 
     s->frame_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ehci_work_timer, s);
-    s->async_bh = qemu_bh_new(ehci_work_bh, s);
+    s->async_bh = qemu_bh_new_guarded(ehci_work_bh, s,
+                                      &dev->mem_reentrancy_guard);
     s->device = dev;
 
     s->vmstate = qemu_add_vm_change_state_handler(usb_ehci_vm_state_change, s);
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 30ae0104bb..bdc891f57a 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1193,7 +1193,7 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
                               USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
         }
     }
-    s->bh = qemu_bh_new(uhci_bh, s);
+    s->bh = qemu_bh_new_guarded(uhci_bh, s, &DEVICE(dev)->mem_reentrancy_guard);
     s->frame_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, uhci_frame_timer, s);
     s->num_ports_vmstate = NB_PORTS;
     QTAILQ_INIT(&s->queues);
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 176868d345..f500db85ab 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1141,7 +1141,8 @@ static void usb_host_nodev_bh(void *opaque)
 static void usb_host_nodev(USBHostDevice *s)
 {
     if (!s->bh_nodev) {
-        s->bh_nodev = qemu_bh_new(usb_host_nodev_bh, s);
+        s->bh_nodev = qemu_bh_new_guarded(usb_host_nodev_bh, s,
+                                          &DEVICE(s)->mem_reentrancy_guard);
     }
     qemu_bh_schedule(s->bh_nodev);
 }
@@ -1739,7 +1740,8 @@ static int usb_host_post_load(void *opaque, int version_id)
     USBHostDevice *dev = opaque;
 
     if (!dev->bh_postld) {
-        dev->bh_postld = qemu_bh_new(usb_host_post_load_bh, dev);
+        dev->bh_postld = qemu_bh_new_guarded(usb_host_post_load_bh, dev,
+                                             &DEVICE(dev)->mem_reentrancy_guard);
     }
     qemu_bh_schedule(dev->bh_postld);
     dev->bh_postld_pending = true;
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index fd7df599bc..39fbaaab16 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1441,8 +1441,10 @@ static void usbredir_realize(USBDevice *udev, Error **errp)
         }
     }
 
-    dev->chardev_close_bh = qemu_bh_new(usbredir_chardev_close_bh, dev);
-    dev->device_reject_bh = qemu_bh_new(usbredir_device_reject_bh, dev);
+    dev->chardev_close_bh = qemu_bh_new_guarded(usbredir_chardev_close_bh, dev,
+                                                &DEVICE(dev)->mem_reentrancy_guard);
+    dev->device_reject_bh = qemu_bh_new_guarded(usbredir_device_reject_bh, dev,
+                                                &DEVICE(dev)->mem_reentrancy_guard);
     dev->attach_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, usbredir_do_attach, dev);
 
     packet_id_queue_init(&dev->cancelled, dev, "cancelled");
diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
index 0f7369e7ed..dec91294ad 100644
--- a/hw/usb/xen-usb.c
+++ b/hw/usb/xen-usb.c
@@ -1021,7 +1021,8 @@ static void usbback_alloc(struct XenLegacyDevice *xendev)
 
     QTAILQ_INIT(&usbif->req_free_q);
     QSIMPLEQ_INIT(&usbif->hotplug_q);
-    usbif->bh = qemu_bh_new(usbback_bh, usbif);
+    usbif->bh = qemu_bh_new_guarded(usbback_bh, usbif,
+                                    &DEVICE(xendev)->mem_reentrancy_guard);
 }
 
 static int usbback_free(struct XenLegacyDevice *xendev)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 746f07c4d2..309cebacc6 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -908,8 +908,9 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
         precopy_add_notifier(&s->free_page_hint_notify);
 
         object_ref(OBJECT(s->iothread));
-        s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
-                                     virtio_ballloon_get_free_page_hints, s);
+        s->free_page_bh = aio_bh_new_guarded(iothread_get_aio_context(s->iothread),
+                                             virtio_ballloon_get_free_page_hints, s,
+                                             &DEVICE(s)->mem_reentrancy_guard);
     }
 
     if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 516425e26a..4c95f1096e 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -1050,7 +1050,8 @@ static void virtio_crypto_device_realize(DeviceState *dev, Error **errp)
         vcrypto->vqs[i].dataq =
                  virtio_add_queue(vdev, 1024, virtio_crypto_handle_dataq_bh);
         vcrypto->vqs[i].dataq_bh =
-                 qemu_bh_new(virtio_crypto_dataq_bh, &vcrypto->vqs[i]);
+                 qemu_bh_new_guarded(virtio_crypto_dataq_bh, &vcrypto->vqs[i],
+                                     &dev->mem_reentrancy_guard);
         vcrypto->vqs[i].vcrypto = vcrypto;
     }
 
-- 
2.39.0

Re: [PATCH v6 0/4] memory: prevent dma-reentracy issues

[Alexander Bulekov]

ping

On 230204 2307, Alexander Bulekov wrote:
> These patches aim to solve two types of DMA-reentrancy issues:
>   
> 1.) mmio -> dma -> mmio case
> To solve this, we track whether the device is engaged in io by
> checking/setting a reentrancy-guard within APIs used for MMIO access.
>   
> 2.) bh -> dma write -> mmio case
> This case is trickier, since we dont have a generic way to associate a
> bh with the underlying Device/DeviceState. Thus, this version allows a
> device to associate a reentrancy-guard with a bh, when creating it.
> (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)
>   
> I replaced most of the qemu_bh_new invocations with the guarded analog,
> except for the ones where the DeviceState was not trivially accessible.
> 
> v5 -> v6:
>     - Only apply checkpatch checks to code in paths containing "/hw/"
>       (/hw/ and include/hw/)
>     - Fix a bug in a _guarded call added to hw/block/virtio-blk.c
> v4-> v5:
>     - Add corresponding checkpatch checks
>     - Save/restore reentrancy-flag when entering/exiting BHs
>     - Improve documentation
>     - Check object_dynamic_cast return value
>   
> v3 -> v4: Instead of changing all of the DMA APIs, instead add an
>     optional reentrancy guard to the BH API.
> 
> v2 -> v3: Bite the bullet and modify the DMA APIs, rather than
>     attempting to guess DeviceStates in BHs.
> 
> Alexander Bulekov (4):
>   memory: prevent dma-reentracy issues
>   async: Add an optional reentrancy guard to the BH API
>   checkpatch: add qemu_bh_new/aio_bh_new checks
>   hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
> 
>  docs/devel/multiple-iothreads.txt |  7 +++++++
>  hw/9pfs/xen-9p-backend.c          |  4 +++-
>  hw/block/dataplane/virtio-blk.c   |  3 ++-
>  hw/block/dataplane/xen-block.c    |  5 +++--
>  hw/char/virtio-serial-bus.c       |  3 ++-
>  hw/display/qxl.c                  |  9 ++++++---
>  hw/display/virtio-gpu.c           |  6 ++++--
>  hw/ide/ahci.c                     |  3 ++-
>  hw/ide/core.c                     |  3 ++-
>  hw/misc/imx_rngc.c                |  6 ++++--
>  hw/misc/macio/mac_dbdma.c         |  2 +-
>  hw/net/virtio-net.c               |  3 ++-
>  hw/nvme/ctrl.c                    |  6 ++++--
>  hw/scsi/mptsas.c                  |  3 ++-
>  hw/scsi/scsi-bus.c                |  3 ++-
>  hw/scsi/vmw_pvscsi.c              |  3 ++-
>  hw/usb/dev-uas.c                  |  3 ++-
>  hw/usb/hcd-dwc2.c                 |  3 ++-
>  hw/usb/hcd-ehci.c                 |  3 ++-
>  hw/usb/hcd-uhci.c                 |  2 +-
>  hw/usb/host-libusb.c              |  6 ++++--
>  hw/usb/redirect.c                 |  6 ++++--
>  hw/usb/xen-usb.c                  |  3 ++-
>  hw/virtio/virtio-balloon.c        |  5 +++--
>  hw/virtio/virtio-crypto.c         |  3 ++-
>  include/block/aio.h               | 18 ++++++++++++++++--
>  include/hw/qdev-core.h            |  7 +++++++
>  include/qemu/main-loop.h          |  7 +++++--
>  scripts/checkpatch.pl             |  8 ++++++++
>  softmmu/memory.c                  | 17 +++++++++++++++++
>  softmmu/trace-events              |  1 +
>  tests/unit/ptimer-test-stubs.c    |  3 ++-
>  util/async.c                      | 18 +++++++++++++++++-
>  util/main-loop.c                  |  5 +++--
>  util/trace-events                 |  1 +
>  35 files changed, 147 insertions(+), 41 deletions(-)
> 
> -- 
> 2.39.0
> 

Re: [PATCH v6 0/4] memory: prevent dma-reentracy issues

[Darren Kenny]

Hi Alex,

Everything looks good to me, I don't have anything else to add:

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.

On Saturday, 2023-02-04 at 23:07:33 -05, Alexander Bulekov wrote:
> These patches aim to solve two types of DMA-reentrancy issues:
>   
> 1.) mmio -> dma -> mmio case
> To solve this, we track whether the device is engaged in io by
> checking/setting a reentrancy-guard within APIs used for MMIO access.
>   
> 2.) bh -> dma write -> mmio case
> This case is trickier, since we dont have a generic way to associate a
> bh with the underlying Device/DeviceState. Thus, this version allows a
> device to associate a reentrancy-guard with a bh, when creating it.
> (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)
>   
> I replaced most of the qemu_bh_new invocations with the guarded analog,
> except for the ones where the DeviceState was not trivially accessible.
>
> v5 -> v6:
>     - Only apply checkpatch checks to code in paths containing "/hw/"
>       (/hw/ and include/hw/)
>     - Fix a bug in a _guarded call added to hw/block/virtio-blk.c
> v4-> v5:
>     - Add corresponding checkpatch checks
>     - Save/restore reentrancy-flag when entering/exiting BHs
>     - Improve documentation
>     - Check object_dynamic_cast return value
>   
> v3 -> v4: Instead of changing all of the DMA APIs, instead add an
>     optional reentrancy guard to the BH API.
>
> v2 -> v3: Bite the bullet and modify the DMA APIs, rather than
>     attempting to guess DeviceStates in BHs.
>
> Alexander Bulekov (4):
>   memory: prevent dma-reentracy issues
>   async: Add an optional reentrancy guard to the BH API
>   checkpatch: add qemu_bh_new/aio_bh_new checks
>   hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
>
>  docs/devel/multiple-iothreads.txt |  7 +++++++
>  hw/9pfs/xen-9p-backend.c          |  4 +++-
>  hw/block/dataplane/virtio-blk.c   |  3 ++-
>  hw/block/dataplane/xen-block.c    |  5 +++--
>  hw/char/virtio-serial-bus.c       |  3 ++-
>  hw/display/qxl.c                  |  9 ++++++---
>  hw/display/virtio-gpu.c           |  6 ++++--
>  hw/ide/ahci.c                     |  3 ++-
>  hw/ide/core.c                     |  3 ++-
>  hw/misc/imx_rngc.c                |  6 ++++--
>  hw/misc/macio/mac_dbdma.c         |  2 +-
>  hw/net/virtio-net.c               |  3 ++-
>  hw/nvme/ctrl.c                    |  6 ++++--
>  hw/scsi/mptsas.c                  |  3 ++-
>  hw/scsi/scsi-bus.c                |  3 ++-
>  hw/scsi/vmw_pvscsi.c              |  3 ++-
>  hw/usb/dev-uas.c                  |  3 ++-
>  hw/usb/hcd-dwc2.c                 |  3 ++-
>  hw/usb/hcd-ehci.c                 |  3 ++-
>  hw/usb/hcd-uhci.c                 |  2 +-
>  hw/usb/host-libusb.c              |  6 ++++--
>  hw/usb/redirect.c                 |  6 ++++--
>  hw/usb/xen-usb.c                  |  3 ++-
>  hw/virtio/virtio-balloon.c        |  5 +++--
>  hw/virtio/virtio-crypto.c         |  3 ++-
>  include/block/aio.h               | 18 ++++++++++++++++--
>  include/hw/qdev-core.h            |  7 +++++++
>  include/qemu/main-loop.h          |  7 +++++--
>  scripts/checkpatch.pl             |  8 ++++++++
>  softmmu/memory.c                  | 17 +++++++++++++++++
>  softmmu/trace-events              |  1 +
>  tests/unit/ptimer-test-stubs.c    |  3 ++-
>  util/async.c                      | 18 +++++++++++++++++-
>  util/main-loop.c                  |  5 +++--
>  util/trace-events                 |  1 +
>  35 files changed, 147 insertions(+), 41 deletions(-)
>
> -- 
> 2.39.0

Re: [PATCH v6 0/4] memory: prevent dma-reentracy issues

[Michael S. Tsirkin]

On Sun, Feb 12, 2023 at 09:11:41PM -0500, Alexander Bulekov wrote:
> ping
> 
> On 230204 2307, Alexander Bulekov wrote:
> > These patches aim to solve two types of DMA-reentrancy issues:
> >   
> > 1.) mmio -> dma -> mmio case
> > To solve this, we track whether the device is engaged in io by
> > checking/setting a reentrancy-guard within APIs used for MMIO access.
> >   
> > 2.) bh -> dma write -> mmio case
> > This case is trickier, since we dont have a generic way to associate a
> > bh with the underlying Device/DeviceState. Thus, this version allows a
> > device to associate a reentrancy-guard with a bh, when creating it.
> > (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)
> >   
> > I replaced most of the qemu_bh_new invocations with the guarded analog,
> > except for the ones where the DeviceState was not trivially accessible.
> > 
> > v5 -> v6:
> >     - Only apply checkpatch checks to code in paths containing "/hw/"
> >       (/hw/ and include/hw/)
> >     - Fix a bug in a _guarded call added to hw/block/virtio-blk.c
> > v4-> v5:
> >     - Add corresponding checkpatch checks
> >     - Save/restore reentrancy-flag when entering/exiting BHs
> >     - Improve documentation
> >     - Check object_dynamic_cast return value
> >   
> > v3 -> v4: Instead of changing all of the DMA APIs, instead add an
> >     optional reentrancy guard to the BH API.
> > 
> > v2 -> v3: Bite the bullet and modify the DMA APIs, rather than
> >     attempting to guess DeviceStates in BHs.

As long as we are adding the new APIs virtio things look ok to me.
Pls merge with rest of patches.

Acked-by: Michael S. Tsirkin <mst@redhat.com>


> > Alexander Bulekov (4):
> >   memory: prevent dma-reentracy issues
> >   async: Add an optional reentrancy guard to the BH API
> >   checkpatch: add qemu_bh_new/aio_bh_new checks
> >   hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
> > 
> >  docs/devel/multiple-iothreads.txt |  7 +++++++
> >  hw/9pfs/xen-9p-backend.c          |  4 +++-
> >  hw/block/dataplane/virtio-blk.c   |  3 ++-
> >  hw/block/dataplane/xen-block.c    |  5 +++--
> >  hw/char/virtio-serial-bus.c       |  3 ++-
> >  hw/display/qxl.c                  |  9 ++++++---
> >  hw/display/virtio-gpu.c           |  6 ++++--
> >  hw/ide/ahci.c                     |  3 ++-
> >  hw/ide/core.c                     |  3 ++-
> >  hw/misc/imx_rngc.c                |  6 ++++--
> >  hw/misc/macio/mac_dbdma.c         |  2 +-
> >  hw/net/virtio-net.c               |  3 ++-
> >  hw/nvme/ctrl.c                    |  6 ++++--
> >  hw/scsi/mptsas.c                  |  3 ++-
> >  hw/scsi/scsi-bus.c                |  3 ++-
> >  hw/scsi/vmw_pvscsi.c              |  3 ++-
> >  hw/usb/dev-uas.c                  |  3 ++-
> >  hw/usb/hcd-dwc2.c                 |  3 ++-
> >  hw/usb/hcd-ehci.c                 |  3 ++-
> >  hw/usb/hcd-uhci.c                 |  2 +-
> >  hw/usb/host-libusb.c              |  6 ++++--
> >  hw/usb/redirect.c                 |  6 ++++--
> >  hw/usb/xen-usb.c                  |  3 ++-
> >  hw/virtio/virtio-balloon.c        |  5 +++--
> >  hw/virtio/virtio-crypto.c         |  3 ++-
> >  include/block/aio.h               | 18 ++++++++++++++++--
> >  include/hw/qdev-core.h            |  7 +++++++
> >  include/qemu/main-loop.h          |  7 +++++--
> >  scripts/checkpatch.pl             |  8 ++++++++
> >  softmmu/memory.c                  | 17 +++++++++++++++++
> >  softmmu/trace-events              |  1 +
> >  tests/unit/ptimer-test-stubs.c    |  3 ++-
> >  util/async.c                      | 18 +++++++++++++++++-
> >  util/main-loop.c                  |  5 +++--
> >  util/trace-events                 |  1 +
> >  35 files changed, 147 insertions(+), 41 deletions(-)
> > 
> > -- 
> > 2.39.0
> > 

Re: [PATCH v6 0/4] memory: prevent dma-reentracy issues

[Thomas Huth]

On 13/02/2023 03.11, Alexander Bulekov wrote:
> ping

I think it would be really good to finally get these dma-reentrancy issues 
fixed! Who's supposed to pick up these patches? Paolo? David? Peter?

  Thomas


> On 230204 2307, Alexander Bulekov wrote:
>> These patches aim to solve two types of DMA-reentrancy issues:
>>    
>> 1.) mmio -> dma -> mmio case
>> To solve this, we track whether the device is engaged in io by
>> checking/setting a reentrancy-guard within APIs used for MMIO access.
>>    
>> 2.) bh -> dma write -> mmio case
>> This case is trickier, since we dont have a generic way to associate a
>> bh with the underlying Device/DeviceState. Thus, this version allows a
>> device to associate a reentrancy-guard with a bh, when creating it.
>> (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)
>>    
>> I replaced most of the qemu_bh_new invocations with the guarded analog,
>> except for the ones where the DeviceState was not trivially accessible.
>>
>> v5 -> v6:
>>      - Only apply checkpatch checks to code in paths containing "/hw/"
>>        (/hw/ and include/hw/)
>>      - Fix a bug in a _guarded call added to hw/block/virtio-blk.c
>> v4-> v5:
>>      - Add corresponding checkpatch checks
>>      - Save/restore reentrancy-flag when entering/exiting BHs
>>      - Improve documentation
>>      - Check object_dynamic_cast return value
>>    
>> v3 -> v4: Instead of changing all of the DMA APIs, instead add an
>>      optional reentrancy guard to the BH API.
>>
>> v2 -> v3: Bite the bullet and modify the DMA APIs, rather than
>>      attempting to guess DeviceStates in BHs.
>>
>> Alexander Bulekov (4):
>>    memory: prevent dma-reentracy issues
>>    async: Add an optional reentrancy guard to the BH API
>>    checkpatch: add qemu_bh_new/aio_bh_new checks
>>    hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
>>
>>   docs/devel/multiple-iothreads.txt |  7 +++++++
>>   hw/9pfs/xen-9p-backend.c          |  4 +++-
>>   hw/block/dataplane/virtio-blk.c   |  3 ++-
>>   hw/block/dataplane/xen-block.c    |  5 +++--
>>   hw/char/virtio-serial-bus.c       |  3 ++-
>>   hw/display/qxl.c                  |  9 ++++++---
>>   hw/display/virtio-gpu.c           |  6 ++++--
>>   hw/ide/ahci.c                     |  3 ++-
>>   hw/ide/core.c                     |  3 ++-
>>   hw/misc/imx_rngc.c                |  6 ++++--
>>   hw/misc/macio/mac_dbdma.c         |  2 +-
>>   hw/net/virtio-net.c               |  3 ++-
>>   hw/nvme/ctrl.c                    |  6 ++++--
>>   hw/scsi/mptsas.c                  |  3 ++-
>>   hw/scsi/scsi-bus.c                |  3 ++-
>>   hw/scsi/vmw_pvscsi.c              |  3 ++-
>>   hw/usb/dev-uas.c                  |  3 ++-
>>   hw/usb/hcd-dwc2.c                 |  3 ++-
>>   hw/usb/hcd-ehci.c                 |  3 ++-
>>   hw/usb/hcd-uhci.c                 |  2 +-
>>   hw/usb/host-libusb.c              |  6 ++++--
>>   hw/usb/redirect.c                 |  6 ++++--
>>   hw/usb/xen-usb.c                  |  3 ++-
>>   hw/virtio/virtio-balloon.c        |  5 +++--
>>   hw/virtio/virtio-crypto.c         |  3 ++-
>>   include/block/aio.h               | 18 ++++++++++++++++--
>>   include/hw/qdev-core.h            |  7 +++++++
>>   include/qemu/main-loop.h          |  7 +++++--
>>   scripts/checkpatch.pl             |  8 ++++++++
>>   softmmu/memory.c                  | 17 +++++++++++++++++
>>   softmmu/trace-events              |  1 +
>>   tests/unit/ptimer-test-stubs.c    |  3 ++-
>>   util/async.c                      | 18 +++++++++++++++++-
>>   util/main-loop.c                  |  5 +++--
>>   util/trace-events                 |  1 +
>>   35 files changed, 147 insertions(+), 41 deletions(-)
>>
>> -- 
>> 2.39.0
>>
> 

Re: [PATCH v6 0/4] memory: prevent dma-reentracy issues

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3541 bytes --]

On Sat, Feb 04, 2023 at 11:07:33PM -0500, Alexander Bulekov wrote:
> These patches aim to solve two types of DMA-reentrancy issues:
>   
> 1.) mmio -> dma -> mmio case
> To solve this, we track whether the device is engaged in io by
> checking/setting a reentrancy-guard within APIs used for MMIO access.
>   
> 2.) bh -> dma write -> mmio case
> This case is trickier, since we dont have a generic way to associate a
> bh with the underlying Device/DeviceState. Thus, this version allows a
> device to associate a reentrancy-guard with a bh, when creating it.
> (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)
>   
> I replaced most of the qemu_bh_new invocations with the guarded analog,
> except for the ones where the DeviceState was not trivially accessible.
> 
> v5 -> v6:
>     - Only apply checkpatch checks to code in paths containing "/hw/"
>       (/hw/ and include/hw/)
>     - Fix a bug in a _guarded call added to hw/block/virtio-blk.c
> v4-> v5:
>     - Add corresponding checkpatch checks
>     - Save/restore reentrancy-flag when entering/exiting BHs
>     - Improve documentation
>     - Check object_dynamic_cast return value
>   
> v3 -> v4: Instead of changing all of the DMA APIs, instead add an
>     optional reentrancy guard to the BH API.
> 
> v2 -> v3: Bite the bullet and modify the DMA APIs, rather than
>     attempting to guess DeviceStates in BHs.
> 
> Alexander Bulekov (4):
>   memory: prevent dma-reentracy issues
>   async: Add an optional reentrancy guard to the BH API
>   checkpatch: add qemu_bh_new/aio_bh_new checks
>   hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
> 
>  docs/devel/multiple-iothreads.txt |  7 +++++++
>  hw/9pfs/xen-9p-backend.c          |  4 +++-
>  hw/block/dataplane/virtio-blk.c   |  3 ++-
>  hw/block/dataplane/xen-block.c    |  5 +++--
>  hw/char/virtio-serial-bus.c       |  3 ++-
>  hw/display/qxl.c                  |  9 ++++++---
>  hw/display/virtio-gpu.c           |  6 ++++--
>  hw/ide/ahci.c                     |  3 ++-
>  hw/ide/core.c                     |  3 ++-
>  hw/misc/imx_rngc.c                |  6 ++++--
>  hw/misc/macio/mac_dbdma.c         |  2 +-
>  hw/net/virtio-net.c               |  3 ++-
>  hw/nvme/ctrl.c                    |  6 ++++--
>  hw/scsi/mptsas.c                  |  3 ++-
>  hw/scsi/scsi-bus.c                |  3 ++-
>  hw/scsi/vmw_pvscsi.c              |  3 ++-
>  hw/usb/dev-uas.c                  |  3 ++-
>  hw/usb/hcd-dwc2.c                 |  3 ++-
>  hw/usb/hcd-ehci.c                 |  3 ++-
>  hw/usb/hcd-uhci.c                 |  2 +-
>  hw/usb/host-libusb.c              |  6 ++++--
>  hw/usb/redirect.c                 |  6 ++++--
>  hw/usb/xen-usb.c                  |  3 ++-
>  hw/virtio/virtio-balloon.c        |  5 +++--
>  hw/virtio/virtio-crypto.c         |  3 ++-
>  include/block/aio.h               | 18 ++++++++++++++++--
>  include/hw/qdev-core.h            |  7 +++++++
>  include/qemu/main-loop.h          |  7 +++++--
>  scripts/checkpatch.pl             |  8 ++++++++
>  softmmu/memory.c                  | 17 +++++++++++++++++
>  softmmu/trace-events              |  1 +
>  tests/unit/ptimer-test-stubs.c    |  3 ++-
>  util/async.c                      | 18 +++++++++++++++++-
>  util/main-loop.c                  |  5 +++--
>  util/trace-events                 |  1 +
>  35 files changed, 147 insertions(+), 41 deletions(-)
> 
> -- 
> 2.39.0
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 0/4] memory: prevent dma-reentracy issues

[Alexander Bulekov]

On 230216 1214, Thomas Huth wrote:
> On 13/02/2023 03.11, Alexander Bulekov wrote:
> > ping
> 
> I think it would be really good to finally get these dma-reentrancy issues
> fixed! Who's supposed to pick up these patches? Paolo? David? Peter?

Ping

> 
>  Thomas

Re: [PATCH v6 0/4] memory: prevent dma-reentracy issues

[Peter Xu]

On Tue, Feb 28, 2023 at 11:07:14AM -0500, Alexander Bulekov wrote:
> On 230216 1214, Thomas Huth wrote:
> > On 13/02/2023 03.11, Alexander Bulekov wrote:
> > > ping
> > 
> > I think it would be really good to finally get these dma-reentrancy issues
> > fixed! Who's supposed to pick up these patches? Paolo? David? Peter?
> 
> Ping

Sorry to not have replied here - I talked to Paolo and I think Paolo has a
plan to review it.

Let's wait for another 2-3 days, perhaps?  Otherwise from what I can tell
this series already received enough R-b/A-bs from major maintainers, this
should be mergeable material going via any tree (for memory, it was always
Paolo who sends the PR before).

Thanks,

-- 
Peter Xu

Re: [PATCH v6 4/4] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

[Michael S. Tsirkin]

On Sat, Feb 04, 2023 at 11:07:37PM -0500, Alexander Bulekov wrote:
> This protects devices from bh->mmio reentrancy issues.
> 
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/9pfs/xen-9p-backend.c        | 4 +++-
>  hw/block/dataplane/virtio-blk.c | 3 ++-
>  hw/block/dataplane/xen-block.c  | 5 +++--
>  hw/char/virtio-serial-bus.c     | 3 ++-
>  hw/display/qxl.c                | 9 ++++++---
>  hw/display/virtio-gpu.c         | 6 ++++--
>  hw/ide/ahci.c                   | 3 ++-
>  hw/ide/core.c                   | 3 ++-
>  hw/misc/imx_rngc.c              | 6 ++++--
>  hw/misc/macio/mac_dbdma.c       | 2 +-
>  hw/net/virtio-net.c             | 3 ++-
>  hw/nvme/ctrl.c                  | 6 ++++--
>  hw/scsi/mptsas.c                | 3 ++-
>  hw/scsi/scsi-bus.c              | 3 ++-
>  hw/scsi/vmw_pvscsi.c            | 3 ++-
>  hw/usb/dev-uas.c                | 3 ++-
>  hw/usb/hcd-dwc2.c               | 3 ++-
>  hw/usb/hcd-ehci.c               | 3 ++-
>  hw/usb/hcd-uhci.c               | 2 +-
>  hw/usb/host-libusb.c            | 6 ++++--
>  hw/usb/redirect.c               | 6 ++++--
>  hw/usb/xen-usb.c                | 3 ++-
>  hw/virtio/virtio-balloon.c      | 5 +++--
>  hw/virtio/virtio-crypto.c       | 3 ++-
>  24 files changed, 63 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> index 65c4979c3c..f077c1b255 100644
> --- a/hw/9pfs/xen-9p-backend.c
> +++ b/hw/9pfs/xen-9p-backend.c
> @@ -441,7 +441,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
>          xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
>                                         XEN_FLEX_RING_SIZE(ring_order);
>  
> -        xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh, &xen_9pdev->rings[i]);
> +        xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
> +                                                     &xen_9pdev->rings[i],
> +                                                     &DEVICE(xen_9pdev)->mem_reentrancy_guard);
>          xen_9pdev->rings[i].out_cons = 0;
>          xen_9pdev->rings[i].out_size = 0;
>          xen_9pdev->rings[i].inprogress = false;
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index b28d81737e..a6202997ee 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
>      } else {
>          s->ctx = qemu_get_aio_context();
>      }
> -    s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
> +    s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
> +                               &DEVICE(vdev)->mem_reentrancy_guard);
>      s->batch_notify_vqs = bitmap_new(conf->num_queues);
>  
>      *dataplane = s;
> diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
> index 2785b9e849..e31806b317 100644
> --- a/hw/block/dataplane/xen-block.c
> +++ b/hw/block/dataplane/xen-block.c
> @@ -632,8 +632,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice *xendev,
>      } else {
>          dataplane->ctx = qemu_get_aio_context();
>      }
> -    dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
> -                               dataplane);
> +    dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
> +                                       dataplane,
> +                                       &DEVICE(xendev)->mem_reentrancy_guard);
>  
>      return dataplane;
>  }
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 7d4601cb5d..dd619f0731 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    port->bh = qemu_bh_new(flush_queued_data_bh, port);
> +    port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
> +                                   &dev->mem_reentrancy_guard);
>      port->elem = NULL;
>  }
>  
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index ec712d3ca2..c0460c4ef1 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
>  
>      qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
>  
> -    qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
> +    qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
> +                                          &DEVICE(qxl)->mem_reentrancy_guard);
>      qxl_reset_state(qxl);
>  
> -    qxl->update_area_bh = qemu_bh_new(qxl_render_update_area_bh, qxl);
> -    qxl->ssd.cursor_bh = qemu_bh_new(qemu_spice_cursor_refresh_bh, &qxl->ssd);
> +    qxl->update_area_bh = qemu_bh_new_guarded(qxl_render_update_area_bh, qxl,
> +                                              &DEVICE(qxl)->mem_reentrancy_guard);
> +    qxl->ssd.cursor_bh = qemu_bh_new_guarded(qemu_spice_cursor_refresh_bh, &qxl->ssd,
> +                                             &DEVICE(qxl)->mem_reentrancy_guard);
>  }
>  
>  static void qxl_realize_primary(PCIDevice *dev, Error **errp)
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 5e15c79b94..66ac9b6cc5 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1339,8 +1339,10 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
>  
>      g->ctrl_vq = virtio_get_queue(vdev, 0);
>      g->cursor_vq = virtio_get_queue(vdev, 1);
> -    g->ctrl_bh = qemu_bh_new(virtio_gpu_ctrl_bh, g);
> -    g->cursor_bh = qemu_bh_new(virtio_gpu_cursor_bh, g);
> +    g->ctrl_bh = qemu_bh_new_guarded(virtio_gpu_ctrl_bh, g,
> +                                     &qdev->mem_reentrancy_guard);
> +    g->cursor_bh = qemu_bh_new_guarded(virtio_gpu_cursor_bh, g,
> +                                       &qdev->mem_reentrancy_guard);
>      QTAILQ_INIT(&g->reslist);
>      QTAILQ_INIT(&g->cmdq);
>      QTAILQ_INIT(&g->fenceq);
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 7ce001cacd..37091150cb 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1508,7 +1508,8 @@ static void ahci_cmd_done(const IDEDMA *dma)
>      ahci_write_fis_d2h(ad);
>  
>      if (ad->port_regs.cmd_issue && !ad->check_bh) {
> -        ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
> +        ad->check_bh = qemu_bh_new_guarded(ahci_check_cmd_bh, ad,
> +                                           &DEVICE(ad)->mem_reentrancy_guard);
>          qemu_bh_schedule(ad->check_bh);
>      }
>  }
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 5d1039378f..8c8d1a8ec2 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -519,7 +519,8 @@ BlockAIOCB *ide_issue_trim(
>  
>      iocb = blk_aio_get(&trim_aiocb_info, s->blk, cb, cb_opaque);
>      iocb->s = s;
> -    iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
> +    iocb->bh = qemu_bh_new_guarded(ide_trim_bh_cb, iocb,
> +                                   &DEVICE(s)->mem_reentrancy_guard);
>      iocb->ret = 0;
>      iocb->qiov = qiov;
>      iocb->i = -1;
> diff --git a/hw/misc/imx_rngc.c b/hw/misc/imx_rngc.c
> index 632c03779c..082c6980ad 100644
> --- a/hw/misc/imx_rngc.c
> +++ b/hw/misc/imx_rngc.c
> @@ -228,8 +228,10 @@ static void imx_rngc_realize(DeviceState *dev, Error **errp)
>      sysbus_init_mmio(sbd, &s->iomem);
>  
>      sysbus_init_irq(sbd, &s->irq);
> -    s->self_test_bh = qemu_bh_new(imx_rngc_self_test, s);
> -    s->seed_bh = qemu_bh_new(imx_rngc_seed, s);
> +    s->self_test_bh = qemu_bh_new_guarded(imx_rngc_self_test, s,
> +                                          &dev->mem_reentrancy_guard);
> +    s->seed_bh = qemu_bh_new_guarded(imx_rngc_seed, s,
> +                                     &dev->mem_reentrancy_guard);
>  }
>  
>  static void imx_rngc_reset(DeviceState *dev)
> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> index 43bb1f56ba..80a789f32b 100644
> --- a/hw/misc/macio/mac_dbdma.c
> +++ b/hw/misc/macio/mac_dbdma.c
> @@ -914,7 +914,7 @@ static void mac_dbdma_realize(DeviceState *dev, Error **errp)
>  {
>      DBDMAState *s = MAC_DBDMA(dev);
>  
> -    s->bh = qemu_bh_new(DBDMA_run_bh, s);
> +    s->bh = qemu_bh_new_guarded(DBDMA_run_bh, s, &dev->mem_reentrancy_guard);
>  }
>  
>  static void mac_dbdma_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 3ae909041a..a170c724de 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2885,7 +2885,8 @@ static void virtio_net_add_queue(VirtIONet *n, int index)
>          n->vqs[index].tx_vq =
>              virtio_add_queue(vdev, n->net_conf.tx_queue_size,
>                               virtio_net_handle_tx_bh);
> -        n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]);
> +        n->vqs[index].tx_bh = qemu_bh_new_guarded(virtio_net_tx_bh, &n->vqs[index],
> +                                                  &DEVICE(vdev)->mem_reentrancy_guard);
>      }
>  
>      n->vqs[index].tx_waiting = 0;
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index f25cc2c235..dcb250e772 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -4318,7 +4318,8 @@ static void nvme_init_sq(NvmeSQueue *sq, NvmeCtrl *n, uint64_t dma_addr,
>          QTAILQ_INSERT_TAIL(&(sq->req_list), &sq->io_req[i], entry);
>      }
>  
> -    sq->bh = qemu_bh_new(nvme_process_sq, sq);
> +    sq->bh = qemu_bh_new_guarded(nvme_process_sq, sq,
> +                                 &DEVICE(sq->ctrl)->mem_reentrancy_guard);
>  
>      if (n->dbbuf_enabled) {
>          sq->db_addr = n->dbbuf_dbs + (sqid << 3);
> @@ -4708,7 +4709,8 @@ static void nvme_init_cq(NvmeCQueue *cq, NvmeCtrl *n, uint64_t dma_addr,
>          }
>      }
>      n->cq[cqid] = cq;
> -    cq->bh = qemu_bh_new(nvme_post_cqes, cq);
> +    cq->bh = qemu_bh_new_guarded(nvme_post_cqes, cq,
> +                                 &DEVICE(cq->ctrl)->mem_reentrancy_guard);
>  }
>  
>  static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest *req)
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index c485da792c..3de288b454 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -1322,7 +1322,8 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
>      }
>      s->max_devices = MPTSAS_NUM_PORTS;
>  
> -    s->request_bh = qemu_bh_new(mptsas_fetch_requests, s);
> +    s->request_bh = qemu_bh_new_guarded(mptsas_fetch_requests, s,
> +                                        &DEVICE(dev)->mem_reentrancy_guard);
>  
>      scsi_bus_init(&s->bus, sizeof(s->bus), &dev->qdev, &mptsas_scsi_info);
>  }
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index ceceafb2cd..e5c9f7a53d 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -193,7 +193,8 @@ static void scsi_dma_restart_cb(void *opaque, bool running, RunState state)
>          AioContext *ctx = blk_get_aio_context(s->conf.blk);
>          /* The reference is dropped in scsi_dma_restart_bh.*/
>          object_ref(OBJECT(s));
> -        s->bh = aio_bh_new(ctx, scsi_dma_restart_bh, s);
> +        s->bh = aio_bh_new_guarded(ctx, scsi_dma_restart_bh, s,
> +                                   &DEVICE(s)->mem_reentrancy_guard);
>          qemu_bh_schedule(s->bh);
>      }
>  }
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index fa76696855..4de34536e9 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1184,7 +1184,8 @@ pvscsi_realizefn(PCIDevice *pci_dev, Error **errp)
>          pcie_endpoint_cap_init(pci_dev, PVSCSI_EXP_EP_OFFSET);
>      }
>  
> -    s->completion_worker = qemu_bh_new(pvscsi_process_completion_queue, s);
> +    s->completion_worker = qemu_bh_new_guarded(pvscsi_process_completion_queue, s,
> +                                               &DEVICE(pci_dev)->mem_reentrancy_guard);
>  
>      scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(pci_dev), &pvscsi_scsi_info);
>      /* override default SCSI bus hotplug-handler, with pvscsi's one */
> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
> index 88f99c05d5..f013ded91e 100644
> --- a/hw/usb/dev-uas.c
> +++ b/hw/usb/dev-uas.c
> @@ -937,7 +937,8 @@ static void usb_uas_realize(USBDevice *dev, Error **errp)
>  
>      QTAILQ_INIT(&uas->results);
>      QTAILQ_INIT(&uas->requests);
> -    uas->status_bh = qemu_bh_new(usb_uas_send_status_bh, uas);
> +    uas->status_bh = qemu_bh_new_guarded(usb_uas_send_status_bh, uas,
> +                                         &d->mem_reentrancy_guard);
>  
>      dev->flags |= (1 << USB_DEV_FLAG_IS_SCSI_STORAGE);
>      scsi_bus_init(&uas->bus, sizeof(uas->bus), DEVICE(dev), &usb_uas_scsi_info);
> diff --git a/hw/usb/hcd-dwc2.c b/hw/usb/hcd-dwc2.c
> index 8755e9cbb0..a0c4e782b2 100644
> --- a/hw/usb/hcd-dwc2.c
> +++ b/hw/usb/hcd-dwc2.c
> @@ -1364,7 +1364,8 @@ static void dwc2_realize(DeviceState *dev, Error **errp)
>      s->fi = USB_FRMINTVL - 1;
>      s->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, dwc2_frame_boundary, s);
>      s->frame_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, dwc2_work_timer, s);
> -    s->async_bh = qemu_bh_new(dwc2_work_bh, s);
> +    s->async_bh = qemu_bh_new_guarded(dwc2_work_bh, s,
> +                                      &dev->mem_reentrancy_guard);
>  
>      sysbus_init_irq(sbd, &s->irq);
>  }
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index d4da8dcb8d..c930c60921 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -2533,7 +2533,8 @@ void usb_ehci_realize(EHCIState *s, DeviceState *dev, Error **errp)
>      }
>  
>      s->frame_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ehci_work_timer, s);
> -    s->async_bh = qemu_bh_new(ehci_work_bh, s);
> +    s->async_bh = qemu_bh_new_guarded(ehci_work_bh, s,
> +                                      &dev->mem_reentrancy_guard);
>      s->device = dev;
>  
>      s->vmstate = qemu_add_vm_change_state_handler(usb_ehci_vm_state_change, s);
> diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
> index 30ae0104bb..bdc891f57a 100644
> --- a/hw/usb/hcd-uhci.c
> +++ b/hw/usb/hcd-uhci.c
> @@ -1193,7 +1193,7 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
>                                USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
>          }
>      }
> -    s->bh = qemu_bh_new(uhci_bh, s);
> +    s->bh = qemu_bh_new_guarded(uhci_bh, s, &DEVICE(dev)->mem_reentrancy_guard);
>      s->frame_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, uhci_frame_timer, s);
>      s->num_ports_vmstate = NB_PORTS;
>      QTAILQ_INIT(&s->queues);
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index 176868d345..f500db85ab 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -1141,7 +1141,8 @@ static void usb_host_nodev_bh(void *opaque)
>  static void usb_host_nodev(USBHostDevice *s)
>  {
>      if (!s->bh_nodev) {
> -        s->bh_nodev = qemu_bh_new(usb_host_nodev_bh, s);
> +        s->bh_nodev = qemu_bh_new_guarded(usb_host_nodev_bh, s,
> +                                          &DEVICE(s)->mem_reentrancy_guard);
>      }
>      qemu_bh_schedule(s->bh_nodev);
>  }
> @@ -1739,7 +1740,8 @@ static int usb_host_post_load(void *opaque, int version_id)
>      USBHostDevice *dev = opaque;
>  
>      if (!dev->bh_postld) {
> -        dev->bh_postld = qemu_bh_new(usb_host_post_load_bh, dev);
> +        dev->bh_postld = qemu_bh_new_guarded(usb_host_post_load_bh, dev,
> +                                             &DEVICE(dev)->mem_reentrancy_guard);
>      }
>      qemu_bh_schedule(dev->bh_postld);
>      dev->bh_postld_pending = true;
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index fd7df599bc..39fbaaab16 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -1441,8 +1441,10 @@ static void usbredir_realize(USBDevice *udev, Error **errp)
>          }
>      }
>  
> -    dev->chardev_close_bh = qemu_bh_new(usbredir_chardev_close_bh, dev);
> -    dev->device_reject_bh = qemu_bh_new(usbredir_device_reject_bh, dev);
> +    dev->chardev_close_bh = qemu_bh_new_guarded(usbredir_chardev_close_bh, dev,
> +                                                &DEVICE(dev)->mem_reentrancy_guard);
> +    dev->device_reject_bh = qemu_bh_new_guarded(usbredir_device_reject_bh, dev,
> +                                                &DEVICE(dev)->mem_reentrancy_guard);
>      dev->attach_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, usbredir_do_attach, dev);
>  
>      packet_id_queue_init(&dev->cancelled, dev, "cancelled");
> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
> index 0f7369e7ed..dec91294ad 100644
> --- a/hw/usb/xen-usb.c
> +++ b/hw/usb/xen-usb.c
> @@ -1021,7 +1021,8 @@ static void usbback_alloc(struct XenLegacyDevice *xendev)
>  
>      QTAILQ_INIT(&usbif->req_free_q);
>      QSIMPLEQ_INIT(&usbif->hotplug_q);
> -    usbif->bh = qemu_bh_new(usbback_bh, usbif);
> +    usbif->bh = qemu_bh_new_guarded(usbback_bh, usbif,
> +                                    &DEVICE(xendev)->mem_reentrancy_guard);
>  }
>  
>  static int usbback_free(struct XenLegacyDevice *xendev)
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 746f07c4d2..309cebacc6 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -908,8 +908,9 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>          precopy_add_notifier(&s->free_page_hint_notify);
>  
>          object_ref(OBJECT(s->iothread));
> -        s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
> -                                     virtio_ballloon_get_free_page_hints, s);
> +        s->free_page_bh = aio_bh_new_guarded(iothread_get_aio_context(s->iothread),
> +                                             virtio_ballloon_get_free_page_hints, s,
> +                                             &DEVICE(s)->mem_reentrancy_guard);
>      }
>  
>      if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> index 516425e26a..4c95f1096e 100644
> --- a/hw/virtio/virtio-crypto.c
> +++ b/hw/virtio/virtio-crypto.c
> @@ -1050,7 +1050,8 @@ static void virtio_crypto_device_realize(DeviceState *dev, Error **errp)
>          vcrypto->vqs[i].dataq =
>                   virtio_add_queue(vdev, 1024, virtio_crypto_handle_dataq_bh);
>          vcrypto->vqs[i].dataq_bh =
> -                 qemu_bh_new(virtio_crypto_dataq_bh, &vcrypto->vqs[i]);
> +                 qemu_bh_new_guarded(virtio_crypto_dataq_bh, &vcrypto->vqs[i],
> +                                     &dev->mem_reentrancy_guard);
>          vcrypto->vqs[i].vcrypto = vcrypto;
>      }
>  
> -- 
> 2.39.0

Re: [PATCH v6 0/4] memory: prevent dma-reentracy issues

[Michael S. Tsirkin]

On Sat, Feb 04, 2023 at 11:07:33PM -0500, Alexander Bulekov wrote:
> These patches aim to solve two types of DMA-reentrancy issues:
>   
> 1.) mmio -> dma -> mmio case
> To solve this, we track whether the device is engaged in io by
> checking/setting a reentrancy-guard within APIs used for MMIO access.
>   
> 2.) bh -> dma write -> mmio case
> This case is trickier, since we dont have a generic way to associate a
> bh with the underlying Device/DeviceState. Thus, this version allows a
> device to associate a reentrancy-guard with a bh, when creating it.
> (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)
>   
> I replaced most of the qemu_bh_new invocations with the guarded analog,
> except for the ones where the DeviceState was not trivially accessible.


Acked-by: Michael S. Tsirkin <mst@redhat.com>

> v5 -> v6:
>     - Only apply checkpatch checks to code in paths containing "/hw/"
>       (/hw/ and include/hw/)
>     - Fix a bug in a _guarded call added to hw/block/virtio-blk.c
> v4-> v5:
>     - Add corresponding checkpatch checks
>     - Save/restore reentrancy-flag when entering/exiting BHs
>     - Improve documentation
>     - Check object_dynamic_cast return value
>   
> v3 -> v4: Instead of changing all of the DMA APIs, instead add an
>     optional reentrancy guard to the BH API.
> 
> v2 -> v3: Bite the bullet and modify the DMA APIs, rather than
>     attempting to guess DeviceStates in BHs.
> 
> Alexander Bulekov (4):
>   memory: prevent dma-reentracy issues
>   async: Add an optional reentrancy guard to the BH API
>   checkpatch: add qemu_bh_new/aio_bh_new checks
>   hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
> 
>  docs/devel/multiple-iothreads.txt |  7 +++++++
>  hw/9pfs/xen-9p-backend.c          |  4 +++-
>  hw/block/dataplane/virtio-blk.c   |  3 ++-
>  hw/block/dataplane/xen-block.c    |  5 +++--
>  hw/char/virtio-serial-bus.c       |  3 ++-
>  hw/display/qxl.c                  |  9 ++++++---
>  hw/display/virtio-gpu.c           |  6 ++++--
>  hw/ide/ahci.c                     |  3 ++-
>  hw/ide/core.c                     |  3 ++-
>  hw/misc/imx_rngc.c                |  6 ++++--
>  hw/misc/macio/mac_dbdma.c         |  2 +-
>  hw/net/virtio-net.c               |  3 ++-
>  hw/nvme/ctrl.c                    |  6 ++++--
>  hw/scsi/mptsas.c                  |  3 ++-
>  hw/scsi/scsi-bus.c                |  3 ++-
>  hw/scsi/vmw_pvscsi.c              |  3 ++-
>  hw/usb/dev-uas.c                  |  3 ++-
>  hw/usb/hcd-dwc2.c                 |  3 ++-
>  hw/usb/hcd-ehci.c                 |  3 ++-
>  hw/usb/hcd-uhci.c                 |  2 +-
>  hw/usb/host-libusb.c              |  6 ++++--
>  hw/usb/redirect.c                 |  6 ++++--
>  hw/usb/xen-usb.c                  |  3 ++-
>  hw/virtio/virtio-balloon.c        |  5 +++--
>  hw/virtio/virtio-crypto.c         |  3 ++-
>  include/block/aio.h               | 18 ++++++++++++++++--
>  include/hw/qdev-core.h            |  7 +++++++
>  include/qemu/main-loop.h          |  7 +++++--
>  scripts/checkpatch.pl             |  8 ++++++++
>  softmmu/memory.c                  | 17 +++++++++++++++++
>  softmmu/trace-events              |  1 +
>  tests/unit/ptimer-test-stubs.c    |  3 ++-
>  util/async.c                      | 18 +++++++++++++++++-
>  util/main-loop.c                  |  5 +++--
>  util/trace-events                 |  1 +
>  35 files changed, 147 insertions(+), 41 deletions(-)
> 
> -- 
> 2.39.0

Re: [PATCH v6 4/4] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

[Paul Durrant]

On 01/03/2023 20:54, Michael S. Tsirkin wrote:
> On Sat, Feb 04, 2023 at 11:07:37PM -0500, Alexander Bulekov wrote:
>> This protects devices from bh->mmio reentrancy issues.
>>
>> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 

Xen parts...

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH v6 4/4] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

[Alexander Bulekov]

[[ CCing qemu-devel in case someone can spot something wrong faster than me]]

On 230308 1042, Thomas Huth wrote:

[snip]

> > > I'd really love to see this series included in QEMU 8.0, so to help with
> > > testing a little bit, I've put it in my gitlab-CI for testing. However, it
> > > hit a segfault in the macOS runner:
> > > 
> > > https://gitlab.com/thuth/qemu/-/jobs/3889796931#L4757
> > > 
> > > Do you have an idea what might be going wrong here?
> > > 
> > 
> > Unfortunately I wasn't able to reproduce this on x86_64 linux and I
> > don't have a mac to test on. I will try to comb through the code in
> > patch 4/4 as that seams like the most likely culprit.
> 
> Yes, you're right, it's the final patch that is causing the problem. I've
> pushed the branch without the final patch, and here it was still working
> fine:
> 
>  https://gitlab.com/thuth/qemu/-/jobs/3893883020
> 
> I also don't have a mac for testing, but cirrus-ci recently started to offer
> terminal access to the jobs ... so if you've got a github account, you could
> fork the qemu repo there and enable the cirrus-ci for it in the marketplace.
> See .gitlab-ci.d/cirrus/README.rst for how to trigger it via the gitlab-CI.
> Or if that's too complicated right now, let me know if I can test something
> for you in my setup.

The detailed cirrus logs indicate that the last test in bios-tables-test
was /x86_64/acpi/q35/multif-bridge and it started qemu with these devices:

-display none -machine q35 -accel kvm -accel tcg -net none -S -device
virtio-balloon,id=balloon0,addr=0x4.0x2 -device
pcie-root-port,id=rp0,multifunction=on,port=0x0,chassis =1,addr=0x2
-device pcie-root-port,id=rp1,port=0x1,chassis=2,addr=0x3.0x1 -device
pcie-root-port,id=rp2,port=0x0,chassis=3,bus=rp1,addr=0.0 -device
pci-bridge,bus=rp2,chassis_nr=4,id=br1 -device
pcie-root-port,id=rphptgt1, port=0x0,chassis=5,addr=2.1 -device
pcie-root-port,id=rphptgt2,port=0x0,chassis=6,addr=2.2 -device
pcie-root-port,id=rphptgt3,port=0x0,chassis=7,addr=2.3 -device
pci-testdev,bus=pcie.0,addr=2.4 -device pci-testdev,bus=pcie .0,addr=5.0
-device pci-testdev,bus=rp0,addr=0.0 -device pci-testdev,bus=br1 -drive
id=hd0,if=none,file=tests/acpi-test-disk-0q5LlN,format=raw -device
ide-hd,drive=hd0

using these identical arguments I still could not reproduce the issue
(replacing the test disk). The test attaches a few devices but it
doesn't seem like they would be related. 

Out of the listed devices, the ones that were touched in patch 4 seem to
be virtio-balloon and ide. 

I took at quick look at the corresponding changes and didn't immediately
notice something wrong, but I'll keep digging. Still have no clue why it
would only show up on mac (I tried different compilers and asan).

-Alex

Re: [PATCH v6 4/4] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

[Thomas Huth]

On 05/02/2023 05.07, Alexander Bulekov wrote:
> This protects devices from bh->mmio reentrancy issues.
> 
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
...
> diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> index 65c4979c3c..f077c1b255 100644
> --- a/hw/9pfs/xen-9p-backend.c
> +++ b/hw/9pfs/xen-9p-backend.c
> @@ -441,7 +441,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
>           xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
>                                          XEN_FLEX_RING_SIZE(ring_order);
>   
> -        xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh, &xen_9pdev->rings[i]);
> +        xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
> +                                                     &xen_9pdev->rings[i],
> +                                                     &DEVICE(xen_9pdev)->mem_reentrancy_guard);

xen_9pdev is not derived from DeviceState, so you must not cast it with 
DEVICE().

> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 7ce001cacd..37091150cb 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1508,7 +1508,8 @@ static void ahci_cmd_done(const IDEDMA *dma)
>       ahci_write_fis_d2h(ad);
>   
>       if (ad->port_regs.cmd_issue && !ad->check_bh) {
> -        ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
> +        ad->check_bh = qemu_bh_new_guarded(ahci_check_cmd_bh, ad,
> +                                           &DEVICE(ad)->mem_reentrancy_guard);
>           qemu_bh_schedule(ad->check_bh);
>       }
>   }

Dito - ad is not derived from DeviceState, so you cannot use DEVICE() here.

(This was causing the crash in the macOS CI job)

> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 5d1039378f..8c8d1a8ec2 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -519,7 +519,8 @@ BlockAIOCB *ide_issue_trim(
>   
>       iocb = blk_aio_get(&trim_aiocb_info, s->blk, cb, cb_opaque);
>       iocb->s = s;
> -    iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
> +    iocb->bh = qemu_bh_new_guarded(ide_trim_bh_cb, iocb,
> +                                   &DEVICE(s)->mem_reentrancy_guard);

IDEState s is also not directly derived from DeviceState. Not sure, but 
maybe you can get to the device here in a similar way that is done in 
ide_identify() :

      IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;

?

> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 746f07c4d2..309cebacc6 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -908,8 +908,9 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>           precopy_add_notifier(&s->free_page_hint_notify);
>   
>           object_ref(OBJECT(s->iothread));
> -        s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
> -                                     virtio_ballloon_get_free_page_hints, s);
> +        s->free_page_bh = aio_bh_new_guarded(iothread_get_aio_context(s->iothread),
> +                                             virtio_ballloon_get_free_page_hints, s,
> +                                             &DEVICE(s)->mem_reentrancy_guard);

You could use "dev" instead of "s" here to get rid of the DEVICE() cast.

The remaining changes look fine to me.

  Thomas

Re: [PATCH v6 1/4] memory: prevent dma-reentracy issues

[Fiona Ebner]

Am 05.02.23 um 05:07 schrieb Alexander Bulekov:
> Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> This flag is set/checked prior to calling a device's MemoryRegion
> handlers, and set when device code initiates DMA.  The purpose of this
> flag is to prevent two types of DMA-based reentrancy issues:
> 
> 1.) mmio -> dma -> mmio case
> 2.) bh -> dma write -> mmio case
> 
> These issues have led to problems such as stack-exhaustion and
> use-after-frees.
> 
> Summary of the problem from Peter Maydell:
> https://lore.kernel.org/qemu-devel/CAFEAcA_23vc7hE3iaM-JVA6W38LK4hJoWae5KcknhPRD5fPBZA@mail.gmail.com
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
> 
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> Acked-by: Peter Xu <peterx@redhat.com>
> ---
>  include/hw/qdev-core.h |  7 +++++++
>  softmmu/memory.c       | 17 +++++++++++++++++
>  softmmu/trace-events   |  1 +
>  3 files changed, 25 insertions(+)
> 
Hi,
there seems to be an issue with this patch or existing issue exposed by
this patch in combination with the LSI SCSI controller.

After applying this patch on current master (i.e.
ee59483267de29056b5b2ee2421ef3844e5c9932), a Debian 11 with the LSI
controller would not boot properly anymore:
> [    7.540907] sym0: <895a> rev 0x0 at pci 0000:00:05.0 irq 10
> [    7.546028] sym0: No NVRAM, ID 7, Fast-40, LVD, parity checking
> [    7.559724] sym0: SCSI BUS has been reset.
> [    7.560820] sym0: interrupted SCRIPT address not found.
> [    7.563802] scsi host2: sym-2.2.3
> [    7.881111] e1000 0000:00:03.0 eth0: (PCI:33MHz:32-bit) 52:54:00:12:34:56
> [    7.881998] e1000 0000:00:03.0 eth0: Intel(R) PRO/1000 Network Connection
> [    7.925902] e1000 0000:00:03.0 ens3: renamed from eth0
> [   32.654811] scsi 2:0:0:0: tag#192 ABORT operation started
> [   37.764283] scsi 2:0:0:0: ABORT operation timed-out.
> [   37.774974] scsi 2:0:0:0: tag#192 DEVICE RESET operation started
> [   42.882488] scsi 2:0:0:0: DEVICE RESET operation timed-out.
> [   42.883606] scsi 2:0:0:0: tag#192 BUS RESET operation started
> [   48.002437] scsi 2:0:0:0: BUS RESET operation timed-out.
> [   48.003030] scsi 2:0:0:0: tag#192 HOST RESET operation started
> [   48.010226] sym0: SCSI BUS has been reset.
> [   53.122472] scsi 2:0:0:0: HOST RESET operation timed-out.
> [   53.123030] scsi 2:0:0:0: Device offlined - not ready after error recovery

The commandline I used is:
./qemu-system-x86_64 \
   -cpu 'kvm64' \
   -m 4096 \
   -serial 'stdio' \
   -device 'lsi,id=scsihw0,bus=pci.0,addr=0x5' \
   -drive
'file=/dev/zvol/myzpool/vm-9006-disk-0,if=none,id=drive-scsi0,format=raw' \
   -device
'scsi-hd,bus=scsihw0.0,scsi-id=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
   -machine 'pc'

Happy to provide any more information if necessary!

CC-ing Fam Zheng (reviewer:SCSI)

Originally reported by one of our community members:
https://forum.proxmox.com/threads/123843/

Best Regards,
Fiona

Re: [PATCH v6 1/4] memory: prevent dma-reentracy issues

[Alexander Bulekov]

On 230310 1214, Fiona Ebner wrote:
> Am 05.02.23 um 05:07 schrieb Alexander Bulekov:
> > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> > This flag is set/checked prior to calling a device's MemoryRegion
> > handlers, and set when device code initiates DMA.  The purpose of this
> > flag is to prevent two types of DMA-based reentrancy issues:
> > 
> > 1.) mmio -> dma -> mmio case
> > 2.) bh -> dma write -> mmio case
> > 
> > These issues have led to problems such as stack-exhaustion and
> > use-after-frees.
> > 
> > Summary of the problem from Peter Maydell:
> > https://lore.kernel.org/qemu-devel/CAFEAcA_23vc7hE3iaM-JVA6W38LK4hJoWae5KcknhPRD5fPBZA@mail.gmail.com
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
> > 
> > Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > Acked-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/hw/qdev-core.h |  7 +++++++
> >  softmmu/memory.c       | 17 +++++++++++++++++
> >  softmmu/trace-events   |  1 +
> >  3 files changed, 25 insertions(+)
> > 
> Hi,
> there seems to be an issue with this patch or existing issue exposed by
> this patch in combination with the LSI SCSI controller.
> 
> After applying this patch on current master (i.e.
> ee59483267de29056b5b2ee2421ef3844e5c9932), a Debian 11 with the LSI
> controller would not boot properly anymore:
> > [    7.540907] sym0: <895a> rev 0x0 at pci 0000:00:05.0 irq 10
> > [    7.546028] sym0: No NVRAM, ID 7, Fast-40, LVD, parity checking
> > [    7.559724] sym0: SCSI BUS has been reset.
> > [    7.560820] sym0: interrupted SCRIPT address not found.
> > [    7.563802] scsi host2: sym-2.2.3
> > [    7.881111] e1000 0000:00:03.0 eth0: (PCI:33MHz:32-bit) 52:54:00:12:34:56
> > [    7.881998] e1000 0000:00:03.0 eth0: Intel(R) PRO/1000 Network Connection
> > [    7.925902] e1000 0000:00:03.0 ens3: renamed from eth0
> > [   32.654811] scsi 2:0:0:0: tag#192 ABORT operation started
> > [   37.764283] scsi 2:0:0:0: ABORT operation timed-out.
> > [   37.774974] scsi 2:0:0:0: tag#192 DEVICE RESET operation started
> > [   42.882488] scsi 2:0:0:0: DEVICE RESET operation timed-out.
> > [   42.883606] scsi 2:0:0:0: tag#192 BUS RESET operation started
> > [   48.002437] scsi 2:0:0:0: BUS RESET operation timed-out.
> > [   48.003030] scsi 2:0:0:0: tag#192 HOST RESET operation started
> > [   48.010226] sym0: SCSI BUS has been reset.
> > [   53.122472] scsi 2:0:0:0: HOST RESET operation timed-out.
> > [   53.123030] scsi 2:0:0:0: Device offlined - not ready after error recovery
> 
> The commandline I used is:
> ./qemu-system-x86_64 \
>    -cpu 'kvm64' \
>    -m 4096 \
>    -serial 'stdio' \
>    -device 'lsi,id=scsihw0,bus=pci.0,addr=0x5' \
>    -drive
> 'file=/dev/zvol/myzpool/vm-9006-disk-0,if=none,id=drive-scsi0,format=raw' \
>    -device
> 'scsi-hd,bus=scsihw0.0,scsi-id=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
>    -machine 'pc'
> 
> Happy to provide any more information if necessary!
> 
> CC-ing Fam Zheng (reviewer:SCSI)
> 
> Originally reported by one of our community members:
> https://forum.proxmox.com/threads/123843/
> 
> Best Regards,
> Fiona
> 

Thanks, I confirmed this by booting up a livecd iso with an lsi device
attached.  I will do some digging

Stack-trace:

#0  trace_memory_region_reentrant_io (cpu_index=<optimized out>, mr=<optimized out>, offset=<optimized out>, size=<optimized out>) at trace/trace-softmmu.h:337
#1  0x000055555815ce67 in access_with_adjusted_size (addr=addr@entry=0x1000, value=0x7ffef01fb980, size=size@entry=0x4, access_size_min=0x1, access_size_min@entry=0x0, access_size_max=0x4, access_fn=0x555558181370 <memory_region_read_accessor>, mr=0x627000000c50, attrs=...
) at ../softmmu/memory.c:552
#2  0x000055555815aec7 in memory_region_dispatch_read1 (mr=0x627000000c50, addr=0x1000, pval=<optimized out>, size=0x4, attrs=...) at ../softmmu/memory.c:1448
#3  memory_region_dispatch_read (mr=0x627000000c50, addr=0x1000, pval=<optimized out>, op=<optimized out>, attrs=...) at ../softmmu/memory.c:1475
#4  0x000055555819eb77 in flatview_read_continue (fv=<optimized out>, addr=0xfebf1000, attrs=..., ptr=<optimized out>, len=0x4, addr1=0x627000000c50, l=<optimized out>, mr=<optimized out>) at ../softmmu/physmem.c:2893
#5  0x000055555819f844 in flatview_read (fv=<optimized out>, addr=<optimized out>, attrs=..., buf=<optimized out>, len=<optimized out>) at ../softmmu/physmem.c:2935
#6  0x000055555819f554 in address_space_read_full (as=<optimized out>, addr=0xfebf1000, attrs=..., buf=0x7ffef01fd800, len=0x4) at ../softmmu/physmem.c:2948
#7  0x00005555575475ab in dma_memory_rw_relaxed (as=0x0, len=0x4, dir=DMA_DIRECTION_TO_DEVICE, attrs=..., addr=<optimized out>, buf=<optimized out>) at include/sysemu/dma.h:87
#8  dma_memory_rw (as=0x0, len=0x4, dir=DMA_DIRECTION_TO_DEVICE, attrs=..., addr=<optimized out>, buf=<optimized out>) at include/sysemu/dma.h:130
#9  pci_dma_rw (dev=<optimized out>, addr=0xfebf1000, len=0x4, dir=DMA_DIRECTION_TO_DEVICE, attrs=..., buf=<optimized out>) at include/hw/pci/pci_device.h:233
#10 pci_dma_read (dev=<optimized out>, addr=0xfebf1000, len=0x4, buf=<optimized out>) at include/hw/pci/pci_device.h:252
#11 read_dword (s=0x627000000100, addr=<optimized out>) at ../hw/scsi/lsi53c895a.c:472
#12 lsi_execute_script (s=s@entry=0x627000000100) at ../hw/scsi/lsi53c895a.c:1155
#13 0x0000555557540c67 in lsi_reg_writeb (s=<optimized out>, offset=<optimized out>, val=<optimized out>) at ../hw/scsi/lsi53c895a.c:2005
#14 0x000055555815d232 in memory_region_write_accessor (mr=<optimized out>, addr=<optimized out>, value=<optimized out>, size=<optimized out>, shift=<optimized out>, mask=<optimized out>, attrs=...) at ../softmmu/memory.c:493
#15 0x000055555815cbfb in access_with_adjusted_size (addr=0x2c, value=value@entry=0x7ffef01fdba0, size=size@entry=0x4, access_size_min=<optimized out>, access_size_min@entry=0x1, access_size_max=<optimized out>, access_fn=0x55555815cef0 <memory_region_write_accessor>, mr=0
x627000000b40, attrs=...) at ../softmmu/memory.c:569
#16 0x000055555815c134 in memory_region_dispatch_write (mr=0x627000000b40, addr=<optimized out>, data=<optimized out>, op=<optimized out>, attrs=...) at ../softmmu/memory.c:1539
#17 0x00005555581a87b1 in flatview_write_continue (fv=<optimized out>, addr=0xfebf302c, attrs=..., ptr=<optimized out>, len=0x4, addr1=0x627000000c50, l=<optimized out>, mr=<optimized out>) at ../softmmu/physmem.c:2826
#18 0x000055555819fdc4 in flatview_write (fv=<optimized out>, addr=<optimized out>, attrs=..., buf=<optimized out>, len=<optimized out>) at ../softmmu/physmem.c:2868
#19 0x000055555819fad4 in address_space_write (as=<optimized out>, addr=0xfebf302c, attrs=..., buf=0x7ffff3e16028, len=0x4) at ../softmmu/physmem.c:2964
#20 0x00005555584078dc in kvm_cpu_exec (cpu=cpu@entry=0x629000019200) at ../accel/kvm/kvm-all.c:2980
#21 0x0000555558421d64 in kvm_vcpu_thread_fn (arg=0x629000019200) at ../accel/kvm/kvm-accel-ops.c:51
#22 0x0000555558a5a279 in qemu_thread_start (args=0x60300008a6d0) at ../util/qemu-thread-posix.c:512
#23 0x00007ffff66a7fd4 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
#24 0x00007ffff672866c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

Re: [PATCH v6 1/4] memory: prevent dma-reentracy issues

[Alexander Bulekov]

On 230310 0723, Alexander Bulekov wrote:
> On 230310 1214, Fiona Ebner wrote:
> > Am 05.02.23 um 05:07 schrieb Alexander Bulekov:
> > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> > > This flag is set/checked prior to calling a device's MemoryRegion
> > > handlers, and set when device code initiates DMA.  The purpose of this
> > > flag is to prevent two types of DMA-based reentrancy issues:
> > > 
> > > 1.) mmio -> dma -> mmio case
> > > 2.) bh -> dma write -> mmio case
> > > 
> > > These issues have led to problems such as stack-exhaustion and
> > > use-after-frees.
> > > 
> > > Summary of the problem from Peter Maydell:
> > > https://lore.kernel.org/qemu-devel/CAFEAcA_23vc7hE3iaM-JVA6W38LK4hJoWae5KcknhPRD5fPBZA@mail.gmail.com
> > > 
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
> > > 
> > > Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > > Acked-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  include/hw/qdev-core.h |  7 +++++++
> > >  softmmu/memory.c       | 17 +++++++++++++++++
> > >  softmmu/trace-events   |  1 +
> > >  3 files changed, 25 insertions(+)
> > > 
> > Hi,
> > there seems to be an issue with this patch or existing issue exposed by
> > this patch in combination with the LSI SCSI controller.
> > 
> > After applying this patch on current master (i.e.
> > ee59483267de29056b5b2ee2421ef3844e5c9932), a Debian 11 with the LSI
> > controller would not boot properly anymore:
> > > [    7.540907] sym0: <895a> rev 0x0 at pci 0000:00:05.0 irq 10
> > > [    7.546028] sym0: No NVRAM, ID 7, Fast-40, LVD, parity checking
> > > [    7.559724] sym0: SCSI BUS has been reset.
> > > [    7.560820] sym0: interrupted SCRIPT address not found.
> > > [    7.563802] scsi host2: sym-2.2.3
> > > [    7.881111] e1000 0000:00:03.0 eth0: (PCI:33MHz:32-bit) 52:54:00:12:34:56
> > > [    7.881998] e1000 0000:00:03.0 eth0: Intel(R) PRO/1000 Network Connection
> > > [    7.925902] e1000 0000:00:03.0 ens3: renamed from eth0
> > > [   32.654811] scsi 2:0:0:0: tag#192 ABORT operation started
> > > [   37.764283] scsi 2:0:0:0: ABORT operation timed-out.
> > > [   37.774974] scsi 2:0:0:0: tag#192 DEVICE RESET operation started
> > > [   42.882488] scsi 2:0:0:0: DEVICE RESET operation timed-out.
> > > [   42.883606] scsi 2:0:0:0: tag#192 BUS RESET operation started
> > > [   48.002437] scsi 2:0:0:0: BUS RESET operation timed-out.
> > > [   48.003030] scsi 2:0:0:0: tag#192 HOST RESET operation started
> > > [   48.010226] sym0: SCSI BUS has been reset.
> > > [   53.122472] scsi 2:0:0:0: HOST RESET operation timed-out.
> > > [   53.123030] scsi 2:0:0:0: Device offlined - not ready after error recovery
> > 
> > The commandline I used is:
> > ./qemu-system-x86_64 \
> >    -cpu 'kvm64' \
> >    -m 4096 \
> >    -serial 'stdio' \
> >    -device 'lsi,id=scsihw0,bus=pci.0,addr=0x5' \
> >    -drive
> > 'file=/dev/zvol/myzpool/vm-9006-disk-0,if=none,id=drive-scsi0,format=raw' \
> >    -device
> > 'scsi-hd,bus=scsihw0.0,scsi-id=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
> >    -machine 'pc'
> > 
> > Happy to provide any more information if necessary!
> > 
> > CC-ing Fam Zheng (reviewer:SCSI)
> > 
> > Originally reported by one of our community members:
> > https://forum.proxmox.com/threads/123843/
> > 
> > Best Regards,
> > Fiona
> > 
> 
> Thanks, I confirmed this by booting up a livecd iso with an lsi device
> attached.  I will do some digging
> 
> Stack-trace:
> 
> #0  trace_memory_region_reentrant_io (cpu_index=<optimized out>, mr=<optimized out>, offset=<optimized out>, size=<optimized out>) at trace/trace-softmmu.h:337
> #1  0x000055555815ce67 in access_with_adjusted_size (addr=addr@entry=0x1000, value=0x7ffef01fb980, size=size@entry=0x4, access_size_min=0x1, access_size_min@entry=0x0, access_size_max=0x4, access_fn=0x555558181370 <memory_region_read_accessor>, mr=0x627000000c50, attrs=...
> ) at ../softmmu/memory.c:552
> #2  0x000055555815aec7 in memory_region_dispatch_read1 (mr=0x627000000c50, addr=0x1000, pval=<optimized out>, size=0x4, attrs=...) at ../softmmu/memory.c:1448

This MR seems to be "lsi-ram".

From hw/scsi/lsi53c895a.c:

memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s,
        "lsi-ram", 0x2000);                    

So the LSI device is reading an LSI "Script" from its own IO region.. In
this particular case, I think there was no reason to use
memory_region_init_io rather than memory_region_init_ram, but this makes
me worried that there are other devices that use something like this.

-Alex

Re: [PATCH v6 1/4] memory: prevent dma-reentracy issues

[Peter Maydell]

On Fri, 10 Mar 2023 at 12:32, Alexander Bulekov <alxndr@bu.edu> wrote:
> This MR seems to be "lsi-ram".
>
> From hw/scsi/lsi53c895a.c:
>
> memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s,
>         "lsi-ram", 0x2000);
>
> So the LSI device is reading an LSI "Script" from its own IO region.. In
> this particular case, I think there was no reason to use
> memory_region_init_io rather than memory_region_init_ram, but this makes
> me worried that there are other devices that use something like this.

This particular device predates the entire MemoryRegion set of
abstractions, so it might have seemed easier at the time.
The endianness handling of the current code is also a bit
confusing and might make it tricky to convert to a RAM MR.

thanks
-- PMM

Re: [PATCH v6 1/4] memory: prevent dma-reentracy issues

[Alexander Bulekov]

On 230310 1245, Peter Maydell wrote:
> On Fri, 10 Mar 2023 at 12:32, Alexander Bulekov <alxndr@bu.edu> wrote:
> > This MR seems to be "lsi-ram".
> >
> > From hw/scsi/lsi53c895a.c:
> >
> > memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s,
> >         "lsi-ram", 0x2000);
> >
> > So the LSI device is reading an LSI "Script" from its own IO region.. In
> > this particular case, I think there was no reason to use
> > memory_region_init_io rather than memory_region_init_ram, but this makes
> > me worried that there are other devices that use something like this.
> 
> This particular device predates the entire MemoryRegion set of
> abstractions, so it might have seemed easier at the time.
> The endianness handling of the current code is also a bit
> confusing and might make it tricky to convert to a RAM MR.

With my trivial mr_io - > mr_ram conversion, I no longer hit the
re-entrancy tracepoint, and my livecd boots, but it's probably not
exhaustively using the script functionality.. 

Does the endianness actually cause a problem? As long as all accesses
(CPU -> LSI_RAM and LSI -> LSI_RAM) occur through the address_space API,
are we safe?
-Alex

Re: [PATCH v6 1/4] memory: prevent dma-reentracy issues

[Mark Cave-Ayland]

On 10/03/2023 12:45, Peter Maydell wrote:

> On Fri, 10 Mar 2023 at 12:32, Alexander Bulekov <alxndr@bu.edu> wrote:
>> This MR seems to be "lsi-ram".
>>
>>  From hw/scsi/lsi53c895a.c:
>>
>> memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s,
>>          "lsi-ram", 0x2000);
>>
>> So the LSI device is reading an LSI "Script" from its own IO region.. In
>> this particular case, I think there was no reason to use
>> memory_region_init_io rather than memory_region_init_ram, but this makes
>> me worried that there are other devices that use something like this.
> 
> This particular device predates the entire MemoryRegion set of
> abstractions, so it might have seemed easier at the time.
> The endianness handling of the current code is also a bit
> confusing and might make it tricky to convert to a RAM MR.

Since the LSI controller is attached to a PCI BAR then the accesses from the host to 
the RAM should all be little endian (you can see the conversions in the driver I 
wrote for the 40p machine to allow it to boot Linux: 
https://github.com/openbios/openbios/blob/master/drivers/lsi.c).

I'm mildly curious that read_dword() which reads the SCRIPTS program internally 
returns the value via cpu_to_le32(), as at first glance I would expect that to be the 
other way around...



ATB,

Mark.

Re: [PATCH v6 1/4] memory: prevent dma-reentracy issues

[Alexander Bulekov]

On 230310 0802, Alexander Bulekov wrote:
> On 230310 1245, Peter Maydell wrote:
> > On Fri, 10 Mar 2023 at 12:32, Alexander Bulekov <alxndr@bu.edu> wrote:
> > > This MR seems to be "lsi-ram".
> > >
> > > From hw/scsi/lsi53c895a.c:
> > >
> > > memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s,
> > >         "lsi-ram", 0x2000);
> > >
> > > So the LSI device is reading an LSI "Script" from its own IO region.. In
> > > this particular case, I think there was no reason to use
> > > memory_region_init_io rather than memory_region_init_ram, but this makes
> > > me worried that there are other devices that use something like this.
> > 
> > This particular device predates the entire MemoryRegion set of
> > abstractions, so it might have seemed easier at the time.
> > The endianness handling of the current code is also a bit
> > confusing and might make it tricky to convert to a RAM MR.
> 
> With my trivial mr_io - > mr_ram conversion, I no longer hit the
> re-entrancy tracepoint, and my livecd boots, but it's probably not
> exhaustively using the script functionality.. 
> 
> Does the endianness actually cause a problem? As long as all accesses
> (CPU -> LSI_RAM and LSI -> LSI_RAM) occur through the address_space API,
> are we safe?
> -Alex

Or maybe a rom_device with memory_region_rom_device_set_romd(romd_mode =
false) is better here?

Re: [PATCH v6 1/4] memory: prevent dma-reentracy issues

[Peter Maydell]

On Fri, 10 Mar 2023 at 13:29, Alexander Bulekov <alxndr@bu.edu> wrote:
>
> On 230310 0802, Alexander Bulekov wrote:
> > On 230310 1245, Peter Maydell wrote:
> > > On Fri, 10 Mar 2023 at 12:32, Alexander Bulekov <alxndr@bu.edu> wrote:
> > > > This MR seems to be "lsi-ram".
> > > >
> > > > From hw/scsi/lsi53c895a.c:
> > > >
> > > > memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s,
> > > >         "lsi-ram", 0x2000);
> > > >
> > > > So the LSI device is reading an LSI "Script" from its own IO region.. In
> > > > this particular case, I think there was no reason to use
> > > > memory_region_init_io rather than memory_region_init_ram, but this makes
> > > > me worried that there are other devices that use something like this.
> > >
> > > This particular device predates the entire MemoryRegion set of
> > > abstractions, so it might have seemed easier at the time.
> > > The endianness handling of the current code is also a bit
> > > confusing and might make it tricky to convert to a RAM MR.
> >
> > With my trivial mr_io - > mr_ram conversion, I no longer hit the
> > re-entrancy tracepoint, and my livecd boots, but it's probably not
> > exhaustively using the script functionality..
> >
> > Does the endianness actually cause a problem? As long as all accesses
> > (CPU -> LSI_RAM and LSI -> LSI_RAM) occur through the address_space API,
> > are we safe?

I'm not sure -- I looked at the code and I wasn't confident
on exactly what the combination of the DEVICE_LITTLE_ENDIAN
MemoryRegion and the use of stn_le_p/ldn_le_p would be.
I think it ought to be possible to use a RAM MR, but we'd
need to check the handling on both BE and LE hosts. Migration
compatibility is the other thing we would need to check, to
avoid accidentally switching from "script_ram[] contents are
in order X" to "they are in order Y"...

> Or maybe a rom_device with memory_region_rom_device_set_romd(romd_mode =
> false) is better here?

I don't think that helps -- if we can let the guest do direct
reads from the region then it's equally OK to let it do
direct writes, so we could just use a RAM MR.

-- PMM