[PATCH v5 2/4] async: Add an optional reentrancy guard to the BH API
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.
Signed-off-by: Alexander Bulekov
---
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 0f65a3cc9e..94d661ff7e 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);
@@ -332,9 +334,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
@@ -343,7 +347,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 *n
[PATCH v5 4/4] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
This protects devices from bh->mmio reentrancy issues.
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Alexander Bulekov
---
hw/9pfs/xen-9p-backend.c| 4 +++-
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/block/virtio-blk.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 ++-
25 files changed, 66 insertions(+), 35 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 26f965cabc..191a8c90aa 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(s)->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/block/virtio-blk.c b/hw/block/virtio-blk.c
index f717550fdc..e9f516e633 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -866,8 +866,9 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool
running,
* requests will be processed while starting the data plane.
*/
if (!s->bh && !virtio_bus_ioeventfd_enabled(bus)) {
-s->bh = aio_bh_new(blk_get_aio_context(s->conf.conf.blk),
- virtio_blk_dma_restart_bh, s);
+s->bh = aio_bh_new_guarded(blk_get_aio_context(s->conf.conf.blk),
+ virtio_blk_dma_restart_bh, s,
+ &DEVICE(s)->mem_reentrancy_guard);
blk_inc_in_flight(s->conf.conf.blk);
qemu_bh_schedule(s->bh);
}
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 6772849dec..67efa3c3ef 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2223,11 +2223,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
**errp)
qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
-qxl->update_
Re: [PATCH v4 2/3] async: Add an optional reentrancy guard to the BH API
On 230125 1624, Stefan Hajnoczi wrote:
> On Thu, Jan 19, 2023 at 02:03:07AM -0500, Alexander Bulekov wrote:
> > 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.
> >
> > Signed-off-by: Alexander Bulekov
> > ---
> > docs/devel/multiple-iothreads.txt | 2 ++
> > include/block/aio.h | 18 --
> > include/qemu/main-loop.h | 7 +--
> > tests/unit/ptimer-test-stubs.c| 3 ++-
> > util/async.c | 12 +++-
> > util/main-loop.c | 5 +++--
> > 6 files changed, 39 insertions(+), 8 deletions(-)
> >
> > diff --git a/docs/devel/multiple-iothreads.txt
> > b/docs/devel/multiple-iothreads.txt
> > index 343120f2ef..e4fafed9d9 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,6 +73,7 @@ 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 AioContext can be obtained from the IOThread using
> > diff --git a/include/block/aio.h b/include/block/aio.h
> > index 0f65a3cc9e..94d661ff7e 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);
> > @@ -332,9 +334,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
> > @@ -343,7 +347,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, c
Re: [PATCH v8 09/13] vfio/migration: Implement VFIO migration protocol v2
On Sun, 22 Jan 2023 12:31:33 +0200
Avihai Horon wrote:
> On 21/01/2023 1:07, Alex Williamson wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Mon, 16 Jan 2023 16:11:31 +0200
> > Avihai Horon wrote:
> >
> >> Implement the basic mandatory part of VFIO migration protocol v2.
> >> This includes all functionality that is necessary to support
> >> VFIO_MIGRATION_STOP_COPY part of the v2 protocol.
> >>
> >> The two protocols, v1 and v2, will co-exist and in the following patches
> >> v1 protocol code will be removed.
> >>
> >> There are several main differences between v1 and v2 protocols:
> >> - VFIO device state is now represented as a finite state machine instead
> >>of a bitmap.
> >>
> >> - Migration interface with kernel is now done using VFIO_DEVICE_FEATURE
> >>ioctl and normal read() and write() instead of the migration region.
> >>
> >> - Pre-copy is made optional in v2 protocol. Support for pre-copy will be
> >>added later on.
> >>
> >> Detailed information about VFIO migration protocol v2 and its difference
> >> compared to v1 protocol can be found here [1].
> >>
> >> [1]
> >> https://lore.kernel.org/all/20220224142024.147653-10-yish...@nvidia.com/
> >>
> >> Signed-off-by: Avihai Horon
> >> Reviewed-by: Cédric Le Goater
> >> ---
> >> include/hw/vfio/vfio-common.h | 5 +
> >> hw/vfio/common.c | 19 +-
> >> hw/vfio/migration.c | 455 +++---
> >> hw/vfio/trace-events | 7 +
> >> 4 files changed, 447 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> >> index bbaf72ba00..6d7d850bfe 100644
> >> --- a/include/hw/vfio/vfio-common.h
> >> +++ b/include/hw/vfio/vfio-common.h
> >> @@ -66,6 +66,11 @@ typedef struct VFIOMigration {
> >> int vm_running;
> >> Notifier migration_state;
> >> uint64_t pending_bytes;
> >> +uint32_t device_state;
> >> +int data_fd;
> >> +void *data_buffer;
> >> +size_t data_buffer_size;
> >> +bool v2;
> >> } VFIOMigration;
> >>
> >> typedef struct VFIOAddressSpace {
> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >> index 550b2d7ded..dcaa77d2a8 100644
> >> --- a/hw/vfio/common.c
> >> +++ b/hw/vfio/common.c
> >> @@ -355,10 +355,18 @@ static bool
> >> vfio_devices_all_dirty_tracking(VFIOContainer *container)
> >> return false;
> >> }
> >>
> >> -if ((vbasedev->pre_copy_dirty_page_tracking ==
> >> ON_OFF_AUTO_OFF) &&
> >> +if (!migration->v2 &&
> >> +(vbasedev->pre_copy_dirty_page_tracking ==
> >> ON_OFF_AUTO_OFF) &&
> >> (migration->device_state_v1 &
> >> VFIO_DEVICE_STATE_V1_RUNNING)) {
> >> return false;
> >> }
> >> +
> >> +if (migration->v2 &&
> >> +(vbasedev->pre_copy_dirty_page_tracking ==
> >> ON_OFF_AUTO_OFF) &&
> >> +(migration->device_state == VFIO_DEVICE_STATE_RUNNING ||
> >> + migration->device_state ==
> >> VFIO_DEVICE_STATE_RUNNING_P2P)) {
> >> +return false;
> >> +}
> >> }
> >> }
> >> return true;
> >> @@ -385,7 +393,14 @@ static bool
> >> vfio_devices_all_running_and_mig_active(VFIOContainer *container)
> >> return false;
> >> }
> >>
> >> -if (migration->device_state_v1 &
> >> VFIO_DEVICE_STATE_V1_RUNNING) {
> >> +if (!migration->v2 &&
> >> +migration->device_state_v1 &
> >> VFIO_DEVICE_STATE_V1_RUNNING) {
> >> +continue;
> >> +}
> >> +
> >> +if (migration->v2 &&
> >> +(migration->device_state == VFIO_DEVICE_STATE_RUNNING ||
> >> + migration->device_state ==
> >> VFIO_DEVICE_STATE_RUNNING_P2P)) {
> >> continue;
> >> } else {
> >> return false;
> >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> >> index 9df859f4d3..f19ada0f4f 100644
> >> --- a/hw/vfio/migration.c
> >> +++ b/hw/vfio/migration.c
> >> @@ -10,6 +10,7 @@
> >> #include "qemu/osdep.h"
> >> #include "qemu/main-loop.h"
> >> #include "qemu/cutils.h"
> >> +#include "qemu/units.h"
> >> #include
> >> #include
> >>
> >> @@ -44,8 +45,103 @@
> >> #define VFIO_MIG_FLAG_DEV_SETUP_STATE (0xef13ULL)
> >> #define VFIO_MIG_FLAG_DEV_DATA_STATE(0xef14ULL)
> >>
> >> +/*
> >> + * This is an arbitrary size based on migration of mlx5 devices, where
> >> typically
> >> + * total device migration size is on the order of 100s of MB. Testing with
> >> + * larger values, e.g. 128MB and 1GB, did not show a performance
> >> improvement.
> >> + */
> >> +#define VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE (1 * MiB)
> >> +
> >> static int64_t bytes_transferred;
> >>
> >> +static const char *mig_state_to_str(enum vfio_device_mig_state s
Re: [PATCH v4 3/3] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
On Thu, Jan 19, 2023 at 02:03:08AM -0500, Alexander Bulekov wrote: > This protects devices from bh->mmio reentrancy issues. > > Signed-off-by: Alexander Bulekov > --- > hw/9pfs/xen-9p-backend.c| 4 +++- > hw/block/dataplane/virtio-blk.c | 3 ++- > hw/block/dataplane/xen-block.c | 5 +++-- > hw/block/virtio-blk.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 ++- > 25 files changed, 66 insertions(+), 35 deletions(-) Should scripts/checkpatch.pl complain when qemu_bh_new() or aio_bh_new() are called from hw/? Adding a check is important so new instances cannot be added accidentally in the future. Stefan signature.asc Description: PGP signature
Re: [PATCH v4 3/3] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
On Thu, Jan 19, 2023 at 02:03:08AM -0500, Alexander Bulekov wrote: > This protects devices from bh->mmio reentrancy issues. > > Signed-off-by: Alexander Bulekov > --- > hw/9pfs/xen-9p-backend.c| 4 +++- > hw/block/dataplane/virtio-blk.c | 3 ++- > hw/block/dataplane/xen-block.c | 5 +++-- > hw/block/virtio-blk.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 ++- > 25 files changed, 66 insertions(+), 35 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH v4 2/3] async: Add an optional reentrancy guard to the BH API
On Thu, Jan 19, 2023 at 02:03:07AM -0500, Alexander Bulekov wrote:
> 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.
>
> Signed-off-by: Alexander Bulekov
> ---
> docs/devel/multiple-iothreads.txt | 2 ++
> include/block/aio.h | 18 --
> include/qemu/main-loop.h | 7 +--
> tests/unit/ptimer-test-stubs.c| 3 ++-
> util/async.c | 12 +++-
> util/main-loop.c | 5 +++--
> 6 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/docs/devel/multiple-iothreads.txt
> b/docs/devel/multiple-iothreads.txt
> index 343120f2ef..e4fafed9d9 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,6 +73,7 @@ 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 AioContext can be obtained from the IOThread using
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 0f65a3cc9e..94d661ff7e 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);
> @@ -332,9 +334,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
> @@ -343,7 +347,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 14d63b3091..08924c3212 100
