Re: [PULL 0/1] ufs fix for 2023-12-05
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any user-visible changes. signature.asc Description: PGP signature
Re: [PATCH 2/2] arm: xlnx-versal-virt: Add machine property ospi-flash
On [2023 Dec 05] Tue 15:22:26, Sai Pavan Boddu wrote:
> This property allows users to change flash model on command line as
> below.
>
>ex: "-M xlnx-versal-virt,ospi-flash=mt35xu02gbba"
>
> Signed-off-by: Sai Pavan Boddu
Reviewed-by: Francisco Iglesias
> ---
> hw/arm/xlnx-versal-virt.c | 31 ++-
> 1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index 537118224f..c57cff74d8 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -49,6 +49,7 @@ struct VersalVirt {
> struct {
> bool secure;
> } cfg;
> +char *ospi_model;
> };
>
> static void fdt_create(VersalVirt *s)
> @@ -637,6 +638,22 @@ static void sd_plugin_card(SDHCIState *sd, DriveInfo *di)
> _fatal);
> }
>
> +static char *versal_get_ospi_model(Object *obj, Error **errp)
> +{
> +VersalVirt *s = XLNX_VERSAL_VIRT_MACHINE(obj);
> +
> +return g_strdup(s->ospi_model);
> +}
> +
> +static void versal_set_ospi_model(Object *obj, const char *value, Error
> **errp)
> +{
> +VersalVirt *s = XLNX_VERSAL_VIRT_MACHINE(obj);
> +
> +g_free(s->ospi_model);
> +s->ospi_model = g_strdup(value);
> +}
> +
> +
> static void versal_virt_init(MachineState *machine)
> {
> VersalVirt *s = XLNX_VERSAL_VIRT_MACHINE(machine);
> @@ -736,7 +753,7 @@ static void versal_virt_init(MachineState *machine)
>
> spi_bus = qdev_get_child_bus(DEVICE(>soc.pmc.iou.ospi), "spi0");
>
> -flash_dev = qdev_new("mt35xu01g");
> +flash_dev = qdev_new(s->ospi_model ? s->ospi_model : "mt35xu01g");
> if (dinfo) {
> qdev_prop_set_drive_err(flash_dev, "drive",
> blk_by_legacy_dinfo(dinfo),
> _fatal);
> @@ -769,6 +786,13 @@ static void versal_virt_machine_instance_init(Object
> *obj)
> 0);
> }
>
> +static void versal_virt_machine_finalize(Object *obj)
> +{
> +VersalVirt *s = XLNX_VERSAL_VIRT_MACHINE(obj);
> +
> +g_free(s->ospi_model);
> +}
> +
> static void versal_virt_machine_class_init(ObjectClass *oc, void *data)
> {
> MachineClass *mc = MACHINE_CLASS(oc);
> @@ -780,6 +804,10 @@ static void versal_virt_machine_class_init(ObjectClass
> *oc, void *data)
> mc->default_cpus = XLNX_VERSAL_NR_ACPUS + XLNX_VERSAL_NR_RCPUS;
> mc->no_cdrom = true;
> mc->default_ram_id = "ddr";
> +object_class_property_add_str(oc, "ospi-flash", versal_get_ospi_model,
> + versal_set_ospi_model);
> +object_class_property_set_description(oc, "ospi-flash",
> + "Change the OSPI Flash model");
> }
>
> static const TypeInfo versal_virt_machine_init_typeinfo = {
> @@ -788,6 +816,7 @@ static const TypeInfo versal_virt_machine_init_typeinfo =
> {
> .class_init = versal_virt_machine_class_init,
> .instance_init = versal_virt_machine_instance_init,
> .instance_size = sizeof(VersalVirt),
> +.instance_finalize = versal_virt_machine_finalize,
> };
>
> static void versal_virt_machine_init_register_types(void)
> --
> 2.25.1
>
Re: [PATCH 1/2] block: m25p80: Add support of mt35xu02gbba
On [2023 Dec 05] Tue 15:22:25, Sai Pavan Boddu wrote:
> Add Micro 2Gb OSPI flash part with sfdp data.
>
> Signed-off-by: Sai Pavan Boddu
Reviewed-by: Francisco Iglesias
> ---
> hw/block/m25p80_sfdp.h | 1 +
> hw/block/m25p80.c | 3 +++
> hw/block/m25p80_sfdp.c | 36
> 3 files changed, 40 insertions(+)
>
> diff --git a/hw/block/m25p80_sfdp.h b/hw/block/m25p80_sfdp.h
> index 011a880f66..1733b56950 100644
> --- a/hw/block/m25p80_sfdp.h
> +++ b/hw/block/m25p80_sfdp.h
> @@ -16,6 +16,7 @@
> #define M25P80_SFDP_MAX_SIZE (1 << 24)
>
> uint8_t m25p80_sfdp_n25q256a(uint32_t addr);
> +uint8_t m25p80_sfdp_mt35xu02g(uint32_t addr);
>
> uint8_t m25p80_sfdp_mx25l25635e(uint32_t addr);
> uint8_t m25p80_sfdp_mx25l25635f(uint32_t addr);
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index afc3fdf4d6..c8c7f6c1c3 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -267,6 +267,9 @@ static const FlashPartInfo known_devices[] = {
> { INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K)
> },
> { INFO_STACKED("mt35xu01g", 0x2c5b1b, 0x104100, 128 << 10, 1024,
> ER_4K | ER_32K, 2) },
> +{ INFO_STACKED("mt35xu02gbba", 0x2c5b1c, 0x104100, 128 << 10, 2048,
> + ER_4K | ER_32K, 4),
> + .sfdp_read = m25p80_sfdp_mt35xu02g },
> { INFO_STACKED("n25q00",0x20ba21, 0x1000, 64 << 10, 2048, ER_4K, 4)
> },
> { INFO_STACKED("n25q00a", 0x20bb21, 0x1000, 64 << 10, 2048, ER_4K, 4)
> },
> { INFO_STACKED("mt25ql01g", 0x20ba21, 0x1040, 64 << 10, 2048, ER_4K, 2)
> },
> diff --git a/hw/block/m25p80_sfdp.c b/hw/block/m25p80_sfdp.c
> index b33811a4f5..6ee2cfaf11 100644
> --- a/hw/block/m25p80_sfdp.c
> +++ b/hw/block/m25p80_sfdp.c
> @@ -57,6 +57,42 @@ static const uint8_t sfdp_n25q256a[] = {
> };
> define_sfdp_read(n25q256a);
>
> +static const uint8_t sfdp_mt35xu02g[] = {
> +0x53, 0x46, 0x44, 0x50, 0x06, 0x01, 0x01, 0xff,
> +0x00, 0x06, 0x01, 0x10, 0x30, 0x00, 0x00, 0xff,
> +0x84, 0x00, 0x01, 0x02, 0x80, 0x00, 0x00, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xe5, 0x20, 0x8a, 0xff, 0xff, 0xff, 0xff, 0x7f,
> +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +0xee, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00,
> +0xff, 0xff, 0x00, 0x00, 0x0c, 0x20, 0x11, 0xd8,
> +0x0f, 0x52, 0x00, 0x00, 0x24, 0x5a, 0x99, 0x00,
> +0x8b, 0x8e, 0x03, 0xe1, 0xac, 0x01, 0x27, 0x38,
> +0x7a, 0x75, 0x7a, 0x75, 0xfb, 0xbd, 0xd5, 0x5c,
> +0x00, 0x00, 0x70, 0xff, 0x81, 0xb0, 0x38, 0x36,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0x43, 0x0e, 0xff, 0xff, 0x21, 0xdc, 0x5c, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +};
> +
> +define_sfdp_read(mt35xu02g);
>
> /*
> * Matronix
> --
> 2.25.1
>
Re: [PATCH 1/1] xlnx-versal-ospi: disable reentrancy detection for iomem_dac
On [2023 Dec 05] Tue 15:18:02, Sai Pavan Boddu wrote:
> The OSPI DMA reads flash data through the OSPI linear address space (the
> iomem_dac region), because of this the reentrancy guard introduced in
> commit a2e1753b ("memory: prevent dma-reentracy issues") is disabled for
> the memory region.
>
> Signed-off-by: Sai Pavan Boddu
Reviewed-by: Francisco Iglesias
Tested-by: Francisco Iglesias
> ---
> hw/ssi/xlnx-versal-ospi.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ssi/xlnx-versal-ospi.c b/hw/ssi/xlnx-versal-ospi.c
> index 1a61679c2f..5123e7dde7 100644
> --- a/hw/ssi/xlnx-versal-ospi.c
> +++ b/hw/ssi/xlnx-versal-ospi.c
> @@ -1772,6 +1772,7 @@ static void xlnx_versal_ospi_init(Object *obj)
> memory_region_init_io(>iomem_dac, obj, _dac_ops, s,
>TYPE_XILINX_VERSAL_OSPI "-dac", 0x2000);
> sysbus_init_mmio(sbd, >iomem_dac);
> +s->iomem_dac.disable_reentrancy_guard = true;
>
> sysbus_init_irq(sbd, >irq);
>
> --
> 2.25.1
>
>
[PATCH v2 11/14] docs: remove AioContext lock from IOThread docs
Encourage the use of locking primitives and stop mentioning the AioContext lock since it is being removed. Signed-off-by: Stefan Hajnoczi Reviewed-by: Eric Blake --- docs/devel/multiple-iothreads.txt | 45 +++ 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/docs/devel/multiple-iothreads.txt b/docs/devel/multiple-iothreads.txt index a3e949f6b3..4865196bde 100644 --- a/docs/devel/multiple-iothreads.txt +++ b/docs/devel/multiple-iothreads.txt @@ -88,27 +88,18 @@ loop, depending on which AioContext instance the caller passes in. How to synchronize with an IOThread --- -AioContext is not thread-safe so some rules must be followed when using file -descriptors, event notifiers, timers, or BHs across threads: +Variables that can be accessed by multiple threads require some form of +synchronization such as qemu_mutex_lock(), rcu_read_lock(), etc. -1. AioContext functions can always be called safely. They handle their -own locking internally. - -2. Other threads wishing to access the AioContext must use -aio_context_acquire()/aio_context_release() for mutual exclusion. Once the -context is acquired no other thread can access it or run event loop iterations -in this AioContext. - -Legacy code sometimes nests aio_context_acquire()/aio_context_release() calls. -Do not use nesting anymore, it is incompatible with the BDRV_POLL_WHILE() macro -used in the block layer and can lead to hangs. - -There is currently no lock ordering rule if a thread needs to acquire multiple -AioContexts simultaneously. Therefore, it is only safe for code holding the -QEMU global mutex to acquire other AioContexts. +AioContext functions like aio_set_fd_handler(), aio_set_event_notifier(), +aio_bh_new(), and aio_timer_new() are thread-safe. They can be used to trigger +activity in an IOThread. Side note: the best way to schedule a function call across threads is to call -aio_bh_schedule_oneshot(). No acquire/release or locking is needed. +aio_bh_schedule_oneshot(). + +The main loop thread can wait synchronously for a condition using +AIO_WAIT_WHILE(). AioContext and the block layer -- @@ -124,22 +115,16 @@ Block layer code must therefore expect to run in an IOThread and avoid using old APIs that implicitly use the main loop. See the "How to program for IOThreads" above for information on how to do that. -If main loop code such as a QMP function wishes to access a BlockDriverState -it must first call aio_context_acquire(bdrv_get_aio_context(bs)) to ensure -that callbacks in the IOThread do not run in parallel. - Code running in the monitor typically needs to ensure that past requests from the guest are completed. When a block device is running in an IOThread, the IOThread can also process requests from the guest (via ioeventfd). To achieve both objects, wrap the code between bdrv_drained_begin() and bdrv_drained_end(), thus creating a "drained -section". The functions must be called between aio_context_acquire() -and aio_context_release(). You can freely release and re-acquire the -AioContext within a drained section. +section". -Long-running jobs (usually in the form of coroutines) are best scheduled in -the BlockDriverState's AioContext to avoid the need to acquire/release around -each bdrv_*() call. The functions bdrv_add/remove_aio_context_notifier, -or alternatively blk_add/remove_aio_context_notifier if you use BlockBackends, -can be used to get a notification whenever bdrv_try_change_aio_context() moves a +Long-running jobs (usually in the form of coroutines) are often scheduled in +the BlockDriverState's AioContext. The functions +bdrv_add/remove_aio_context_notifier, or alternatively +blk_add/remove_aio_context_notifier if you use BlockBackends, can be used to +get a notification whenever bdrv_try_change_aio_context() moves a BlockDriverState to a different AioContext. -- 2.43.0
[PATCH v2 10/14] aio: remove aio_context_acquire()/aio_context_release() API
Delete these functions because nothing calls these functions anymore.
I introduced these APIs in commit 98563fc3ec44 ("aio: add
aio_context_acquire() and aio_context_release()") in 2014. It's with a
sigh of relief that I delete these APIs almost 10 years later.
Thanks to Paolo Bonzini's vision for multi-queue QEMU, we got an
understanding of where the code needed to go in order to remove the
limitations that the original dataplane and the IOThread/AioContext
approach that followed it.
Emanuele Giuseppe Esposito had the splendid determination to convert
large parts of the codebase so that they no longer needed the AioContext
lock. This was a painstaking process, both in the actual code changes
required and the iterations of code review that Emanuele eked out of
Kevin and me over many months.
Kevin Wolf tackled multitudes of graph locking conversions to protect
in-flight I/O from run-time changes to the block graph as well as the
clang Thread Safety Analysis annotations that allow the compiler to
check whether the graph lock is being used correctly.
And me, well, I'm just here to add some pizzazz to the QEMU multi-queue
block layer :). Thank you to everyone who helped with this effort,
including Eric Blake, code reviewer extraordinaire, and others who I've
forgotten to mention.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
---
include/block/aio.h | 17 -
util/async.c| 10 --
2 files changed, 27 deletions(-)
diff --git a/include/block/aio.h b/include/block/aio.h
index f08b358077..af05512a7d 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -278,23 +278,6 @@ void aio_context_ref(AioContext *ctx);
*/
void aio_context_unref(AioContext *ctx);
-/* Take ownership of the AioContext. If the AioContext will be shared between
- * threads, and a thread does not want to be interrupted, it will have to
- * take ownership around calls to aio_poll(). Otherwise, aio_poll()
- * automatically takes care of calling aio_context_acquire and
- * aio_context_release.
- *
- * Note that this is separate from bdrv_drained_begin/bdrv_drained_end. A
- * thread still has to call those to avoid being interrupted by the guest.
- *
- * Bottom halves, timers and callbacks can be created or removed without
- * acquiring the AioContext.
- */
-void aio_context_acquire(AioContext *ctx);
-
-/* Relinquish ownership of the AioContext. */
-void aio_context_release(AioContext *ctx);
-
/**
* aio_bh_schedule_oneshot_full: Allocate a new bottom half structure that will
* run only once and as soon as possible.
diff --git a/util/async.c b/util/async.c
index dfd44ef612..460529057c 100644
--- a/util/async.c
+++ b/util/async.c
@@ -719,16 +719,6 @@ void aio_context_unref(AioContext *ctx)
g_source_unref(>source);
}
-void aio_context_acquire(AioContext *ctx)
-{
-/* TODO remove this function */
-}
-
-void aio_context_release(AioContext *ctx)
-{
-/* TODO remove this function */
-}
-
QEMU_DEFINE_STATIC_CO_TLS(AioContext *, my_aiocontext)
AioContext *qemu_get_current_aio_context(void)
--
2.43.0
[PATCH v2 01/14] virtio-scsi: replace AioContext lock with tmf_bh_lock
Protect the Task Management Function BH state with a lock. The TMF BH
runs in the main loop thread. An IOThread might process a TMF at the
same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh
must be protected by a lock.
Run TMF request completion in the IOThread using aio_wait_bh_oneshot().
This avoids more locking to protect the virtqueue and SCSI layer state.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
Reviewed-by: Kevin Wolf
---
include/hw/virtio/virtio-scsi.h | 3 +-
hw/scsi/virtio-scsi.c | 62 ++---
2 files changed, 43 insertions(+), 22 deletions(-)
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 779568ab5d..da8cb928d9 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -85,8 +85,9 @@ struct VirtIOSCSI {
/*
* TMFs deferred to main loop BH. These fields are protected by
- * virtio_scsi_acquire().
+ * tmf_bh_lock.
*/
+QemuMutex tmf_bh_lock;
QEMUBH *tmf_bh;
QTAILQ_HEAD(, VirtIOSCSIReq) tmf_bh_list;
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 9c751bf296..4f8d35facc 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -123,6 +123,30 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
virtio_scsi_free_req(req);
}
+static void virtio_scsi_complete_req_bh(void *opaque)
+{
+VirtIOSCSIReq *req = opaque;
+
+virtio_scsi_complete_req(req);
+}
+
+/*
+ * Called from virtio_scsi_do_one_tmf_bh() in main loop thread. The main loop
+ * thread cannot touch the virtqueue since that could race with an IOThread.
+ */
+static void virtio_scsi_complete_req_from_main_loop(VirtIOSCSIReq *req)
+{
+VirtIOSCSI *s = req->dev;
+
+if (!s->ctx || s->ctx == qemu_get_aio_context()) {
+/* No need to schedule a BH when there is no IOThread */
+virtio_scsi_complete_req(req);
+} else {
+/* Run request completion in the IOThread */
+aio_wait_bh_oneshot(s->ctx, virtio_scsi_complete_req_bh, req);
+}
+}
+
static void virtio_scsi_bad_req(VirtIOSCSIReq *req)
{
virtio_error(VIRTIO_DEVICE(req->dev), "wrong size for virtio-scsi
headers");
@@ -338,10 +362,7 @@ static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req)
out:
object_unref(OBJECT(d));
-
-virtio_scsi_acquire(s);
-virtio_scsi_complete_req(req);
-virtio_scsi_release(s);
+virtio_scsi_complete_req_from_main_loop(req);
}
/* Some TMFs must be processed from the main loop thread */
@@ -354,18 +375,16 @@ static void virtio_scsi_do_tmf_bh(void *opaque)
GLOBAL_STATE_CODE();
-virtio_scsi_acquire(s);
+WITH_QEMU_LOCK_GUARD(>tmf_bh_lock) {
+QTAILQ_FOREACH_SAFE(req, >tmf_bh_list, next, tmp) {
+QTAILQ_REMOVE(>tmf_bh_list, req, next);
+QTAILQ_INSERT_TAIL(, req, next);
+}
-QTAILQ_FOREACH_SAFE(req, >tmf_bh_list, next, tmp) {
-QTAILQ_REMOVE(>tmf_bh_list, req, next);
-QTAILQ_INSERT_TAIL(, req, next);
+qemu_bh_delete(s->tmf_bh);
+s->tmf_bh = NULL;
}
-qemu_bh_delete(s->tmf_bh);
-s->tmf_bh = NULL;
-
-virtio_scsi_release(s);
-
QTAILQ_FOREACH_SAFE(req, , next, tmp) {
QTAILQ_REMOVE(, req, next);
virtio_scsi_do_one_tmf_bh(req);
@@ -379,8 +398,7 @@ static void virtio_scsi_reset_tmf_bh(VirtIOSCSI *s)
GLOBAL_STATE_CODE();
-virtio_scsi_acquire(s);
-
+/* Called after ioeventfd has been stopped, so tmf_bh_lock is not needed */
if (s->tmf_bh) {
qemu_bh_delete(s->tmf_bh);
s->tmf_bh = NULL;
@@ -393,19 +411,19 @@ static void virtio_scsi_reset_tmf_bh(VirtIOSCSI *s)
req->resp.tmf.response = VIRTIO_SCSI_S_TARGET_FAILURE;
virtio_scsi_complete_req(req);
}
-
-virtio_scsi_release(s);
}
static void virtio_scsi_defer_tmf_to_bh(VirtIOSCSIReq *req)
{
VirtIOSCSI *s = req->dev;
-QTAILQ_INSERT_TAIL(>tmf_bh_list, req, next);
+WITH_QEMU_LOCK_GUARD(>tmf_bh_lock) {
+QTAILQ_INSERT_TAIL(>tmf_bh_list, req, next);
-if (!s->tmf_bh) {
-s->tmf_bh = qemu_bh_new(virtio_scsi_do_tmf_bh, s);
-qemu_bh_schedule(s->tmf_bh);
+if (!s->tmf_bh) {
+s->tmf_bh = qemu_bh_new(virtio_scsi_do_tmf_bh, s);
+qemu_bh_schedule(s->tmf_bh);
+}
}
}
@@ -1235,6 +1253,7 @@ static void virtio_scsi_device_realize(DeviceState *dev,
Error **errp)
Error *err = NULL;
QTAILQ_INIT(>tmf_bh_list);
+qemu_mutex_init(>tmf_bh_lock);
virtio_scsi_common_realize(dev,
virtio_scsi_handle_ctrl,
@@ -1277,6 +1296,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev)
qbus_set_hotplug_handler(BUS(>bus), NULL);
virtio_scsi_common_unrealize(dev);
+qemu_mutex_destroy(>tmf_bh_lock);
}
static Property virtio_scsi_properties[] = {
--
2.43.0
[PATCH v2 00/14] aio: remove AioContext lock
v2:
- Add Patch 2 "scsi: assert that callbacks run in the correct AioContext"
[Kevin]
- Add Patch 7 "block: remove bdrv_co_lock()" [Eric and Kevin]
- Remove stray goto label in Patch 8 [Kevin]
- Fix "eeked" -> "eked" typo in Patch 10 [Eric]
This series removes the AioContext locking APIs from QEMU.
aio_context_acquire() and aio_context_release() are currently only needed to
support the locking discipline required by AIO_POLL_WHILE() (except for a stray
user that I converted in Patch 1). AIO_POLL_WHILE() doesn't really need the
AioContext lock anymore, so it's possible to remove the API. This is a nice
simplification because the AioContext locking rules were sometimes tricky or
underspecified, leading to many bugs of the years.
This patch series removes these APIs across the codebase and cleans up the
documentation/comments that refers to them.
Patch 1 is a AioContext lock user I forgot to convert in my earlier SCSI
conversion series.
Patch 2 adds an assertion to the SCSI code to ensure that callbacks are invoked
in the correct AioContext.
Patch 3 removes tests for the AioContext lock because they will no longer be
needed when the lock is gone.
Patches 4-10 remove the AioContext lock. These can be reviewed by categorizing
the call sites into 1. places that take the lock because they call an API that
requires the lock (ultimately AIO_POLL_WHILE()) and 2. places that take the
lock to protect state. There should be no instances of case 2 left. If you see
one, you've found a bug in this patch series!
Patches 11-14 remove comments.
Based-on: 20231204164259.1515217-1-stefa...@redhat.com ("[PATCH v2 0/4] scsi:
eliminate AioContext lock")
Since SCSI needs to stop relying on the AioContext lock before we can remove
the lock.
Stefan Hajnoczi (14):
virtio-scsi: replace AioContext lock with tmf_bh_lock
scsi: assert that callbacks run in the correct AioContext
tests: remove aio_context_acquire() tests
aio: make aio_context_acquire()/aio_context_release() a no-op
graph-lock: remove AioContext locking
block: remove AioContext locking
block: remove bdrv_co_lock()
scsi: remove AioContext locking
aio-wait: draw equivalence between AIO_WAIT_WHILE() and
AIO_WAIT_WHILE_UNLOCKED()
aio: remove aio_context_acquire()/aio_context_release() API
docs: remove AioContext lock from IOThread docs
scsi: remove outdated AioContext lock comment
job: remove outdated AioContext locking comments
block: remove outdated AioContext locking comments
docs/devel/multiple-iothreads.txt| 45 ++--
include/block/aio-wait.h | 16 +-
include/block/aio.h | 17 --
include/block/block-common.h | 3 -
include/block/block-global-state.h | 23 +-
include/block/block-io.h | 12 +-
include/block/block_int-common.h | 2 -
include/block/graph-lock.h | 21 +-
include/block/snapshot.h | 2 -
include/hw/virtio/virtio-scsi.h | 17 +-
include/qemu/job.h | 20 --
block.c | 363 ---
block/backup.c | 4 +-
block/blklogwrites.c | 8 +-
block/blkverify.c| 4 +-
block/block-backend.c| 33 +--
block/commit.c | 16 +-
block/copy-before-write.c| 22 +-
block/export/export.c| 22 +-
block/export/vhost-user-blk-server.c | 4 -
block/graph-lock.c | 44 +---
block/io.c | 45 +---
block/mirror.c | 41 +--
block/monitor/bitmap-qmp-cmds.c | 20 +-
block/monitor/block-hmp-cmds.c | 29 ---
block/qapi-sysemu.c | 27 +-
block/qapi.c | 18 +-
block/qcow2.c| 4 +-
block/quorum.c | 8 +-
block/raw-format.c | 5 -
block/replication.c | 72 +-
block/snapshot.c | 26 +-
block/stream.c | 12 +-
block/vmdk.c | 20 +-
block/write-threshold.c | 6 -
blockdev.c | 319 +--
blockjob.c | 30 +--
hw/block/dataplane/virtio-blk.c | 10 -
hw/block/dataplane/xen-block.c | 17 +-
hw/block/virtio-blk.c| 45 +---
hw/core/qdev-properties-system.c | 9 -
hw/scsi/scsi-bus.c | 2 -
hw/scsi/scsi-disk.c | 46 ++--
hw/scsi/virtio-scsi.c| 80 +++---
job.c| 16 --
migration/block.c| 33 +--
migration/migration-hmp-cmds.c | 3 -
migration/savevm.c | 22 --
net/colo-compare.c | 2 -
qemu-img.c | 4 -
qemu-io.c| 10 +-
qemu-nbd.c | 2 -
[PATCH v2 09/14] aio-wait: draw equivalence between AIO_WAIT_WHILE() and AIO_WAIT_WHILE_UNLOCKED()
Now that the AioContext lock no longer exists, AIO_WAIT_WHILE() and
AIO_WAIT_WHILE_UNLOCKED() are equivalent.
A future patch will get rid of AIO_WAIT_WHILE_UNLOCKED().
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
---
include/block/aio-wait.h | 16
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
index 5449b6d742..157f105916 100644
--- a/include/block/aio-wait.h
+++ b/include/block/aio-wait.h
@@ -63,9 +63,6 @@ extern AioWait global_aio_wait;
* @ctx: the aio context, or NULL if multiple aio contexts (for which the
* caller does not hold a lock) are involved in the polling condition.
* @cond: wait while this conditional expression is true
- * @unlock: whether to unlock and then lock again @ctx. This applies
- * only when waiting for another AioContext from the main loop.
- * Otherwise it's ignored.
*
* Wait while a condition is true. Use this to implement synchronous
* operations that require event loop activity.
@@ -78,7 +75,7 @@ extern AioWait global_aio_wait;
* wait on conditions between two IOThreads since that could lead to deadlock,
* go via the main loop instead.
*/
-#define AIO_WAIT_WHILE_INTERNAL(ctx, cond, unlock) ({ \
+#define AIO_WAIT_WHILE_INTERNAL(ctx, cond) ({ \
bool waited_ = false; \
AioWait *wait_ = _aio_wait; \
AioContext *ctx_ = (ctx); \
@@ -95,13 +92,7 @@ extern AioWait global_aio_wait;
assert(qemu_get_current_aio_context() == \
qemu_get_aio_context());\
while ((cond)) { \
-if (unlock && ctx_) { \
-aio_context_release(ctx_); \
-} \
aio_poll(qemu_get_aio_context(), true);\
-if (unlock && ctx_) { \
-aio_context_acquire(ctx_); \
-} \
waited_ = true;\
} \
} \
@@ -109,10 +100,11 @@ extern AioWait global_aio_wait;
waited_; })
#define AIO_WAIT_WHILE(ctx, cond) \
-AIO_WAIT_WHILE_INTERNAL(ctx, cond, true)
+AIO_WAIT_WHILE_INTERNAL(ctx, cond)
+/* TODO replace this with AIO_WAIT_WHILE() in a future patch */
#define AIO_WAIT_WHILE_UNLOCKED(ctx, cond) \
-AIO_WAIT_WHILE_INTERNAL(ctx, cond, false)
+AIO_WAIT_WHILE_INTERNAL(ctx, cond)
/**
* aio_wait_kick:
--
2.43.0
[PATCH v2 12/14] scsi: remove outdated AioContext lock comment
The SCSI subsystem no longer uses the AioContext lock. Request
processing runs exclusively in the BlockBackend's AioContext since
"scsi: only access SCSIDevice->requests from one thread" and hence the
lock is unnecessary.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
---
hw/scsi/scsi-disk.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 61be3d395a..2e7e1e9a1c 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -355,7 +355,6 @@ done:
scsi_req_unref(>req);
}
-/* Called with AioContext lock held */
static void scsi_dma_complete(void *opaque, int ret)
{
SCSIDiskReq *r = (SCSIDiskReq *)opaque;
--
2.43.0
[PATCH v2 05/14] graph-lock: remove AioContext locking
Stop acquiring/releasing the AioContext lock in
bdrv_graph_wrlock()/bdrv_graph_unlock() since the lock no longer has any
effect.
The distinction between bdrv_graph_wrunlock() and
bdrv_graph_wrunlock_ctx() becomes meaningless and they can be collapsed
into one function.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
Reviewed-by: Kevin Wolf
---
include/block/graph-lock.h | 21 ++---
block.c| 50 +++---
block/backup.c | 4 +--
block/blklogwrites.c | 8 ++---
block/blkverify.c | 4 +--
block/block-backend.c | 11 +++
block/commit.c | 16 +-
block/graph-lock.c | 44 ++
block/mirror.c | 22 ++---
block/qcow2.c | 4 +--
block/quorum.c | 8 ++---
block/replication.c| 14 -
block/snapshot.c | 4 +--
block/stream.c | 12 +++
block/vmdk.c | 20 ++--
blockdev.c | 8 ++---
blockjob.c | 12 +++
tests/unit/test-bdrv-drain.c | 40
tests/unit/test-bdrv-graph-mod.c | 20 ++--
scripts/block-coroutine-wrapper.py | 4 +--
20 files changed, 133 insertions(+), 193 deletions(-)
diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
index 22b5db1ed9..d7545e82d0 100644
--- a/include/block/graph-lock.h
+++ b/include/block/graph-lock.h
@@ -110,34 +110,17 @@ void unregister_aiocontext(AioContext *ctx);
*
* The wrlock can only be taken from the main loop, with BQL held, as only the
* main loop is allowed to modify the graph.
- *
- * If @bs is non-NULL, its AioContext is temporarily released.
- *
- * This function polls. Callers must not hold the lock of any AioContext other
- * than the current one and the one of @bs.
*/
void no_coroutine_fn TSA_ACQUIRE(graph_lock) TSA_NO_TSA
-bdrv_graph_wrlock(BlockDriverState *bs);
+bdrv_graph_wrlock(void);
/*
* bdrv_graph_wrunlock:
* Write finished, reset global has_writer to 0 and restart
* all readers that are waiting.
- *
- * If @bs is non-NULL, its AioContext is temporarily released.
*/
void no_coroutine_fn TSA_RELEASE(graph_lock) TSA_NO_TSA
-bdrv_graph_wrunlock(BlockDriverState *bs);
-
-/*
- * bdrv_graph_wrunlock_ctx:
- * Write finished, reset global has_writer to 0 and restart
- * all readers that are waiting.
- *
- * If @ctx is non-NULL, its lock is temporarily released.
- */
-void no_coroutine_fn TSA_RELEASE(graph_lock) TSA_NO_TSA
-bdrv_graph_wrunlock_ctx(AioContext *ctx);
+bdrv_graph_wrunlock(void);
/*
* bdrv_graph_co_rdlock:
diff --git a/block.c b/block.c
index bfb0861ec6..25e1ebc606 100644
--- a/block.c
+++ b/block.c
@@ -1708,12 +1708,12 @@ bdrv_open_driver(BlockDriverState *bs, BlockDriver
*drv, const char *node_name,
open_failed:
bs->drv = NULL;
-bdrv_graph_wrlock(NULL);
+bdrv_graph_wrlock();
if (bs->file != NULL) {
bdrv_unref_child(bs, bs->file);
assert(!bs->file);
}
-bdrv_graph_wrunlock(NULL);
+bdrv_graph_wrunlock();
g_free(bs->opaque);
bs->opaque = NULL;
@@ -3575,9 +3575,9 @@ int bdrv_set_backing_hd(BlockDriverState *bs,
BlockDriverState *backing_hd,
bdrv_ref(drain_bs);
bdrv_drained_begin(drain_bs);
-bdrv_graph_wrlock(backing_hd);
+bdrv_graph_wrlock();
ret = bdrv_set_backing_hd_drained(bs, backing_hd, errp);
-bdrv_graph_wrunlock(backing_hd);
+bdrv_graph_wrunlock();
bdrv_drained_end(drain_bs);
bdrv_unref(drain_bs);
@@ -3790,13 +3790,13 @@ BdrvChild *bdrv_open_child(const char *filename,
return NULL;
}
-bdrv_graph_wrlock(NULL);
+bdrv_graph_wrlock();
ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
child = bdrv_attach_child(parent, bs, bdref_key, child_class, child_role,
errp);
aio_context_release(ctx);
-bdrv_graph_wrunlock(NULL);
+bdrv_graph_wrunlock();
return child;
}
@@ -4650,9 +4650,9 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue,
Error **errp)
aio_context_release(ctx);
}
-bdrv_graph_wrlock(NULL);
+bdrv_graph_wrlock();
tran_commit(tran);
-bdrv_graph_wrunlock(NULL);
+bdrv_graph_wrunlock();
QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
BlockDriverState *bs = bs_entry->state.bs;
@@ -4669,9 +4669,9 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue,
Error **errp)
goto cleanup;
abort:
-bdrv_graph_wrlock(NULL);
+bdrv_graph_wrlock();
tran_abort(tran);
-bdrv_graph_wrunlock(NULL);
+bdrv_graph_wrunlock();
QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
if (bs_entry->prepared) {
@@ -4852,12 +4852,12 @@
[PATCH v2 03/14] tests: remove aio_context_acquire() tests
The aio_context_acquire() API is being removed. Drop the test case that
calls the API.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
Reviewed-by: Kevin Wolf
---
tests/unit/test-aio.c | 67 +--
1 file changed, 1 insertion(+), 66 deletions(-)
diff --git a/tests/unit/test-aio.c b/tests/unit/test-aio.c
index 337b6e4ea7..e77d86be87 100644
--- a/tests/unit/test-aio.c
+++ b/tests/unit/test-aio.c
@@ -100,76 +100,12 @@ static void event_ready_cb(EventNotifier *e)
/* Tests using aio_*. */
-typedef struct {
-QemuMutex start_lock;
-EventNotifier notifier;
-bool thread_acquired;
-} AcquireTestData;
-
-static void *test_acquire_thread(void *opaque)
-{
-AcquireTestData *data = opaque;
-
-/* Wait for other thread to let us start */
-qemu_mutex_lock(>start_lock);
-qemu_mutex_unlock(>start_lock);
-
-/* event_notifier_set might be called either before or after
- * the main thread's call to poll(). The test case's outcome
- * should be the same in either case.
- */
-event_notifier_set(>notifier);
-aio_context_acquire(ctx);
-aio_context_release(ctx);
-
-data->thread_acquired = true; /* success, we got here */
-
-return NULL;
-}
-
static void set_event_notifier(AioContext *nctx, EventNotifier *notifier,
EventNotifierHandler *handler)
{
aio_set_event_notifier(nctx, notifier, handler, NULL, NULL);
}
-static void dummy_notifier_read(EventNotifier *n)
-{
-event_notifier_test_and_clear(n);
-}
-
-static void test_acquire(void)
-{
-QemuThread thread;
-AcquireTestData data;
-
-/* Dummy event notifier ensures aio_poll() will block */
-event_notifier_init(, false);
-set_event_notifier(ctx, , dummy_notifier_read);
-g_assert(!aio_poll(ctx, false)); /* consume aio_notify() */
-
-qemu_mutex_init(_lock);
-qemu_mutex_lock(_lock);
-data.thread_acquired = false;
-
-qemu_thread_create(, "test_acquire_thread",
- test_acquire_thread,
- , QEMU_THREAD_JOINABLE);
-
-/* Block in aio_poll(), let other thread kick us and acquire context */
-aio_context_acquire(ctx);
-qemu_mutex_unlock(_lock); /* let the thread run */
-g_assert(aio_poll(ctx, true));
-g_assert(!data.thread_acquired);
-aio_context_release(ctx);
-
-qemu_thread_join();
-set_event_notifier(ctx, , NULL);
-event_notifier_cleanup();
-
-g_assert(data.thread_acquired);
-}
-
static void test_bh_schedule(void)
{
BHTestData data = { .n = 0 };
@@ -879,7 +815,7 @@ static void test_worker_thread_co_enter(void)
qemu_thread_get_self(_thread);
co = qemu_coroutine_create(co_check_current_thread, _thread);
-qemu_thread_create(_thread, "test_acquire_thread",
+qemu_thread_create(_thread, "test_aio_co_enter",
test_aio_co_enter,
co, QEMU_THREAD_JOINABLE);
@@ -899,7 +835,6 @@ int main(int argc, char **argv)
while (g_main_context_iteration(NULL, false));
g_test_init(, , NULL);
-g_test_add_func("/aio/acquire", test_acquire);
g_test_add_func("/aio/bh/schedule", test_bh_schedule);
g_test_add_func("/aio/bh/schedule10", test_bh_schedule10);
g_test_add_func("/aio/bh/cancel", test_bh_cancel);
--
2.43.0
[PATCH v2 08/14] scsi: remove AioContext locking
The AioContext lock no longer has any effect. Remove it.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
---
include/hw/virtio/virtio-scsi.h | 14 --
hw/scsi/scsi-bus.c | 2 --
hw/scsi/scsi-disk.c | 31 +--
hw/scsi/virtio-scsi.c | 18 --
4 files changed, 5 insertions(+), 60 deletions(-)
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index da8cb928d9..7f0573b1bf 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -101,20 +101,6 @@ struct VirtIOSCSI {
uint32_t host_features;
};
-static inline void virtio_scsi_acquire(VirtIOSCSI *s)
-{
-if (s->ctx) {
-aio_context_acquire(s->ctx);
-}
-}
-
-static inline void virtio_scsi_release(VirtIOSCSI *s)
-{
-if (s->ctx) {
-aio_context_release(s->ctx);
-}
-}
-
void virtio_scsi_common_realize(DeviceState *dev,
VirtIOHandleOutput ctrl,
VirtIOHandleOutput evt,
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index f3ec11f892..df68a44b6a 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1731,9 +1731,7 @@ void scsi_device_purge_requests(SCSIDevice *sdev,
SCSISense sense)
{
scsi_device_for_each_req_async(sdev, scsi_device_purge_one_req, NULL);
-aio_context_acquire(blk_get_aio_context(sdev->conf.blk));
blk_drain(sdev->conf.blk);
-aio_context_release(blk_get_aio_context(sdev->conf.blk));
scsi_device_set_ua(sdev, sense);
}
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index a5048e0aaf..61be3d395a 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2339,14 +2339,10 @@ static void scsi_disk_reset(DeviceState *dev)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev.qdev, dev);
uint64_t nb_sectors;
-AioContext *ctx;
scsi_device_purge_requests(>qdev, SENSE_CODE(RESET));
-ctx = blk_get_aio_context(s->qdev.conf.blk);
-aio_context_acquire(ctx);
blk_get_geometry(s->qdev.conf.blk, _sectors);
-aio_context_release(ctx);
nb_sectors /= s->qdev.blocksize / BDRV_SECTOR_SIZE;
if (nb_sectors) {
@@ -2545,15 +2541,13 @@ static void scsi_unrealize(SCSIDevice *dev)
static void scsi_hd_realize(SCSIDevice *dev, Error **errp)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
-AioContext *ctx = NULL;
+
/* can happen for devices without drive. The error message for missing
* backend will be issued in scsi_realize
*/
if (s->qdev.conf.blk) {
-ctx = blk_get_aio_context(s->qdev.conf.blk);
-aio_context_acquire(ctx);
if (!blkconf_blocksizes(>qdev.conf, errp)) {
-goto out;
+return;
}
}
s->qdev.blocksize = s->qdev.conf.logical_block_size;
@@ -2562,16 +2556,11 @@ static void scsi_hd_realize(SCSIDevice *dev, Error
**errp)
s->product = g_strdup("QEMU HARDDISK");
}
scsi_realize(>qdev, errp);
-out:
-if (ctx) {
-aio_context_release(ctx);
-}
}
static void scsi_cd_realize(SCSIDevice *dev, Error **errp)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
-AioContext *ctx;
int ret;
uint32_t blocksize = 2048;
@@ -2587,8 +2576,6 @@ static void scsi_cd_realize(SCSIDevice *dev, Error **errp)
blocksize = dev->conf.physical_block_size;
}
-ctx = blk_get_aio_context(dev->conf.blk);
-aio_context_acquire(ctx);
s->qdev.blocksize = blocksize;
s->qdev.type = TYPE_ROM;
s->features |= 1 << SCSI_DISK_F_REMOVABLE;
@@ -2596,7 +2583,6 @@ static void scsi_cd_realize(SCSIDevice *dev, Error **errp)
s->product = g_strdup("QEMU CD-ROM");
}
scsi_realize(>qdev, errp);
-aio_context_release(ctx);
}
@@ -2727,7 +2713,6 @@ static int get_device_type(SCSIDiskState *s)
static void scsi_block_realize(SCSIDevice *dev, Error **errp)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
-AioContext *ctx;
int sg_version;
int rc;
@@ -2742,9 +2727,6 @@ static void scsi_block_realize(SCSIDevice *dev, Error
**errp)
"be removed in a future version");
}
-ctx = blk_get_aio_context(s->qdev.conf.blk);
-aio_context_acquire(ctx);
-
/* check we are using a driver managing SG_IO (version 3 and after) */
rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, _version);
if (rc < 0) {
@@ -2752,18 +2734,18 @@ static void scsi_block_realize(SCSIDevice *dev, Error
**errp)
if (rc != -EPERM) {
error_append_hint(errp, "Is this a SCSI device?\n");
}
-goto out;
+return;
}
if (sg_version < 3) {
error_setg(errp, "scsi generic interface too old");
-goto out;
+return;
}
/* get device type from INQUIRY data */
rc = get_device_type(s);
if (rc < 0) {
error_setg(errp,
[PATCH v2 04/14] aio: make aio_context_acquire()/aio_context_release() a no-op
aio_context_acquire()/aio_context_release() has been replaced by
fine-grained locking to protect state shared by multiple threads. The
AioContext lock still plays the role of balancing locking in
AIO_WAIT_WHILE() and many functions in QEMU either require that the
AioContext lock is held or not held for this reason. In other words, the
AioContext lock is purely there for consistency with itself and serves
no real purpose anymore.
Stop actually acquiring/releasing the lock in
aio_context_acquire()/aio_context_release() so that subsequent patches
can remove callers across the codebase incrementally.
I have performed "make check" and qemu-iotests stress tests across
x86-64, ppc64le, and aarch64 to confirm that there are no failures as a
result of eliminating the lock.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
Acked-by: Kevin Wolf
---
util/async.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/util/async.c b/util/async.c
index 8f90ddc304..04ee83d220 100644
--- a/util/async.c
+++ b/util/async.c
@@ -725,12 +725,12 @@ void aio_context_unref(AioContext *ctx)
void aio_context_acquire(AioContext *ctx)
{
-qemu_rec_mutex_lock(>lock);
+/* TODO remove this function */
}
void aio_context_release(AioContext *ctx)
{
-qemu_rec_mutex_unlock(>lock);
+/* TODO remove this function */
}
QEMU_DEFINE_STATIC_CO_TLS(AioContext *, my_aiocontext)
--
2.43.0
[PATCH v2 13/14] job: remove outdated AioContext locking comments
The AioContext lock no longer exists.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
---
include/qemu/job.h | 20
1 file changed, 20 deletions(-)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index e502787dd8..9ea98b5927 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -67,8 +67,6 @@ typedef struct Job {
/**
* The completion function that will be called when the job completes.
- * Called with AioContext lock held, since many callback implementations
- * use bdrv_* functions that require to hold the lock.
*/
BlockCompletionFunc *cb;
@@ -264,9 +262,6 @@ struct JobDriver {
*
* This callback will not be invoked if the job has already failed.
* If it fails, abort and then clean will be called.
- *
- * Called with AioContext lock held, since many callbacs implementations
- * use bdrv_* functions that require to hold the lock.
*/
int (*prepare)(Job *job);
@@ -277,9 +272,6 @@ struct JobDriver {
*
* All jobs will complete with a call to either .commit() or .abort() but
* never both.
- *
- * Called with AioContext lock held, since many callback implementations
- * use bdrv_* functions that require to hold the lock.
*/
void (*commit)(Job *job);
@@ -290,9 +282,6 @@ struct JobDriver {
*
* All jobs will complete with a call to either .commit() or .abort() but
* never both.
- *
- * Called with AioContext lock held, since many callback implementations
- * use bdrv_* functions that require to hold the lock.
*/
void (*abort)(Job *job);
@@ -301,9 +290,6 @@ struct JobDriver {
* .commit() or .abort(). Regardless of which callback is invoked after
* completion, .clean() will always be called, even if the job does not
* belong to a transaction group.
- *
- * Called with AioContext lock held, since many callbacs implementations
- * use bdrv_* functions that require to hold the lock.
*/
void (*clean)(Job *job);
@@ -318,17 +304,12 @@ struct JobDriver {
* READY).
* (If the callback is NULL, the job is assumed to terminate
* without I/O.)
- *
- * Called with AioContext lock held, since many callback implementations
- * use bdrv_* functions that require to hold the lock.
*/
bool (*cancel)(Job *job, bool force);
/**
* Called when the job is freed.
- * Called with AioContext lock held, since many callback implementations
- * use bdrv_* functions that require to hold the lock.
*/
void (*free)(Job *job);
};
@@ -424,7 +405,6 @@ void job_ref_locked(Job *job);
* Release a reference that was previously acquired with job_ref_locked() or
* job_create(). If it's the last reference to the object, it will be freed.
*
- * Takes AioContext lock internally to invoke a job->driver callback.
* Called with job lock held.
*/
void job_unref_locked(Job *job);
--
2.43.0
[PATCH v2 14/14] block: remove outdated AioContext locking comments
The AioContext lock no longer exists.
There is one noteworthy change:
- * More specifically, these functions use BDRV_POLL_WHILE(bs), which
- * requires the caller to be either in the main thread and hold
- * the BlockdriverState (bs) AioContext lock, or directly in the
- * home thread that runs the bs AioContext. Calling them from
- * another thread in another AioContext would cause deadlocks.
+ * More specifically, these functions use BDRV_POLL_WHILE(bs), which requires
+ * the caller to be either in the main thread or directly in the home thread
+ * that runs the bs AioContext. Calling them from another thread in another
+ * AioContext would cause deadlocks.
I am not sure whether deadlocks are still possible. Maybe they have just
moved to the fine-grained locks that have replaced the AioContext. Since
I am not sure if the deadlocks are gone, I have kept the substance
unchanged and just removed mention of the AioContext.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
---
include/block/block-common.h | 3 --
include/block/block-io.h | 9 ++--
include/block/block_int-common.h | 2 -
block.c | 73 ++--
block/block-backend.c| 8 ---
block/export/vhost-user-blk-server.c | 4 --
tests/qemu-iotests/202 | 2 +-
tests/qemu-iotests/203 | 3 +-
8 files changed, 22 insertions(+), 82 deletions(-)
diff --git a/include/block/block-common.h b/include/block/block-common.h
index d7599564db..a846023a09 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -70,9 +70,6 @@
* automatically takes the graph rdlock when calling the wrapped function. In
* the same way, no_co_wrapper_bdrv_wrlock functions automatically take the
* graph wrlock.
- *
- * If the first parameter of the function is a BlockDriverState, BdrvChild or
- * BlockBackend pointer, the AioContext lock for it is taken in the wrapper.
*/
#define no_co_wrapper
#define no_co_wrapper_bdrv_rdlock
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 8eb39a858b..b49e0537dd 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -332,11 +332,10 @@ bdrv_co_copy_range(BdrvChild *src, int64_t src_offset,
* "I/O or GS" API functions. These functions can run without
* the BQL, but only in one specific iothread/main loop.
*
- * More specifically, these functions use BDRV_POLL_WHILE(bs), which
- * requires the caller to be either in the main thread and hold
- * the BlockdriverState (bs) AioContext lock, or directly in the
- * home thread that runs the bs AioContext. Calling them from
- * another thread in another AioContext would cause deadlocks.
+ * More specifically, these functions use BDRV_POLL_WHILE(bs), which requires
+ * the caller to be either in the main thread or directly in the home thread
+ * that runs the bs AioContext. Calling them from another thread in another
+ * AioContext would cause deadlocks.
*
* Therefore, these functions are not proper I/O, because they
* can't run in *any* iothreads, but only in a specific one.
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 4e31d161c5..151279d481 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1192,8 +1192,6 @@ struct BlockDriverState {
/* The error object in use for blocking operations on backing_hd */
Error *backing_blocker;
-/* Protected by AioContext lock */
-
/*
* If we are reading a disk image, give its size in sectors.
* Generally read-only; it is written to by load_snapshot and
diff --git a/block.c b/block.c
index 434b7f4d72..a097772238 100644
--- a/block.c
+++ b/block.c
@@ -1616,11 +1616,6 @@ out:
g_free(gen_node_name);
}
-/*
- * The caller must always hold @bs AioContext lock, because this function calls
- * bdrv_refresh_total_sectors() which polls when called from non-coroutine
- * context.
- */
static int no_coroutine_fn GRAPH_UNLOCKED
bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, const char *node_name,
QDict *options, int open_flags, Error **errp)
@@ -2901,7 +2896,7 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission
qapi_perm)
* Replaces the node that a BdrvChild points to without updating permissions.
*
* If @new_bs is non-NULL, the parent of @child must already be drained through
- * @child and the caller must hold the AioContext lock for @new_bs.
+ * @child.
*/
static void GRAPH_WRLOCK
bdrv_replace_child_noperm(BdrvChild *child, BlockDriverState *new_bs)
@@ -3041,9 +3036,8 @@ static TransactionActionDrv bdrv_attach_child_common_drv
= {
*
* Returns new created child.
*
- * The caller must hold the AioContext lock for @child_bs. Both @parent_bs and
- * @child_bs can move to a different AioContext in this function. Callers must
- * make sure that their AioContext locking is still correct after
[PATCH v2 07/14] block: remove bdrv_co_lock()
The bdrv_co_lock() and bdrv_co_unlock() functions are already no-ops.
Remove them.
Signed-off-by: Stefan Hajnoczi
---
include/block/block-global-state.h | 14 --
block.c| 10 --
blockdev.c | 4
3 files changed, 28 deletions(-)
diff --git a/include/block/block-global-state.h
b/include/block/block-global-state.h
index 0327f1c605..4ec0b217f0 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -267,20 +267,6 @@ int bdrv_debug_remove_breakpoint(BlockDriverState *bs,
const char *tag);
int bdrv_debug_resume(BlockDriverState *bs, const char *tag);
bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag);
-/**
- * Locks the AioContext of @bs if it's not the current AioContext. This avoids
- * double locking which could lead to deadlocks: This is a coroutine_fn, so we
- * know we already own the lock of the current AioContext.
- *
- * May only be called in the main thread.
- */
-void coroutine_fn bdrv_co_lock(BlockDriverState *bs);
-
-/**
- * Unlocks the AioContext of @bs if it's not the current AioContext.
- */
-void coroutine_fn bdrv_co_unlock(BlockDriverState *bs);
-
bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
GHashTable *visited, Transaction *tran,
Error **errp);
diff --git a/block.c b/block.c
index 91ace5d2d5..434b7f4d72 100644
--- a/block.c
+++ b/block.c
@@ -7431,16 +7431,6 @@ void coroutine_fn bdrv_co_leave(BlockDriverState *bs,
AioContext *old_ctx)
bdrv_dec_in_flight(bs);
}
-void coroutine_fn bdrv_co_lock(BlockDriverState *bs)
-{
-/* TODO removed in next patch */
-}
-
-void coroutine_fn bdrv_co_unlock(BlockDriverState *bs)
-{
-/* TODO removed in next patch */
-}
-
static void bdrv_do_remove_aio_context_notifier(BdrvAioNotifier *ban)
{
GLOBAL_STATE_CODE();
diff --git a/blockdev.c b/blockdev.c
index 8a1b28f830..3a5e7222ec 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2264,18 +2264,14 @@ void coroutine_fn qmp_block_resize(const char *device,
const char *node_name,
return;
}
-bdrv_co_lock(bs);
bdrv_drained_begin(bs);
-bdrv_co_unlock(bs);
old_ctx = bdrv_co_enter(bs);
blk_co_truncate(blk, size, false, PREALLOC_MODE_OFF, 0, errp);
bdrv_co_leave(bs, old_ctx);
-bdrv_co_lock(bs);
bdrv_drained_end(bs);
blk_co_unref(blk);
-bdrv_co_unlock(bs);
}
void qmp_block_stream(const char *job_id, const char *device,
--
2.43.0
[PATCH v2 02/14] scsi: assert that callbacks run in the correct AioContext
Since the removal of AioContext locking, the correctness of the code
relies on running requests from a single AioContext at any given time.
Add assertions that verify that callbacks are invoked in the correct
AioContext.
Signed-off-by: Stefan Hajnoczi
---
hw/scsi/scsi-disk.c | 14 ++
system/dma-helpers.c | 3 +++
2 files changed, 17 insertions(+)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 2c1bbb3530..a5048e0aaf 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -273,6 +273,10 @@ static void scsi_aio_complete(void *opaque, int ret)
SCSIDiskReq *r = (SCSIDiskReq *)opaque;
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+/* The request must only run in the BlockBackend's AioContext */
+assert(blk_get_aio_context(s->qdev.conf.blk) ==
+ qemu_get_current_aio_context());
+
assert(r->req.aiocb != NULL);
r->req.aiocb = NULL;
@@ -370,8 +374,13 @@ static void scsi_dma_complete(void *opaque, int ret)
static void scsi_read_complete_noio(SCSIDiskReq *r, int ret)
{
+SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
uint32_t n;
+/* The request must only run in the BlockBackend's AioContext */
+assert(blk_get_aio_context(s->qdev.conf.blk) ==
+ qemu_get_current_aio_context());
+
assert(r->req.aiocb == NULL);
if (scsi_disk_req_check_error(r, ret, false)) {
goto done;
@@ -496,8 +505,13 @@ static void scsi_read_data(SCSIRequest *req)
static void scsi_write_complete_noio(SCSIDiskReq *r, int ret)
{
+SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
uint32_t n;
+/* The request must only run in the BlockBackend's AioContext */
+assert(blk_get_aio_context(s->qdev.conf.blk) ==
+ qemu_get_current_aio_context());
+
assert (r->req.aiocb == NULL);
if (scsi_disk_req_check_error(r, ret, false)) {
goto done;
diff --git a/system/dma-helpers.c b/system/dma-helpers.c
index 528117f256..9b221cf94e 100644
--- a/system/dma-helpers.c
+++ b/system/dma-helpers.c
@@ -119,6 +119,9 @@ static void dma_blk_cb(void *opaque, int ret)
trace_dma_blk_cb(dbs, ret);
+/* DMAAIOCB is not thread-safe and must be accessed only from dbs->ctx */
+assert(ctx == qemu_get_current_aio_context());
+
dbs->acb = NULL;
dbs->offset += dbs->iov.size;
--
2.43.0
Re: [PATCH v3 0/2] block: commit/stream: Allow users to request only format driver names in backing file format
On Tue, Dec 05, 2023 at 18:14:40 +0100, Peter Krempa wrote: > Please see patches for rationale. > > Libvirt patches using this new flag will be posted soon-ish (after > cleanup). https://lists.libvirt.org/archives/list/de...@lists.libvirt.org/message/GCCZP5ANYTPVXCIEYGSM5NWYCGDL23V6/
[PATCH v3 1/2] block: commit: Allow users to request only format driver names in backing file format
Introduce a new flag 'backing-mask-protocol' for the block-commit QMP
command which instructs the internals to use 'raw' instead of the
protocol driver in case when a image is used without a dummy 'raw'
wrapper.
The flag is designed such that it can be always asserted by management
tools even when there isn't any update to backing files.
The flag will be used by libvirt so that the backing images still
reference the proper format even when libvirt will stop using the dummy
raw driver (raw driver with no other config). Libvirt needs this so that
the images stay compatible with older libvirt versions which didn't
expect that a protocol driver name can appear in the backing file format
field.
Signed-off-by: Peter Krempa
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block.c| 37 +-
block/commit.c | 6 -
blockdev.c | 6 +
include/block/block-global-state.h | 3 ++-
include/block/block_int-common.h | 4 ++-
include/block/block_int-global-state.h | 3 +++
qapi/block-core.json | 8 +-
tests/unit/test-bdrv-drain.c | 3 ++-
8 files changed, 58 insertions(+), 12 deletions(-)
diff --git a/block.c b/block.c
index bfb0861ec6..771a00a14d 100644
--- a/block.c
+++ b/block.c
@@ -1309,11 +1309,14 @@ static void bdrv_backing_detach(BdrvChild *c)
}
static int bdrv_backing_update_filename(BdrvChild *c, BlockDriverState *base,
-const char *filename, Error **errp)
+const char *filename,
+bool backing_mask_protocol,
+Error **errp)
{
BlockDriverState *parent = c->opaque;
bool read_only = bdrv_is_read_only(parent);
int ret;
+const char *format_name;
GLOBAL_STATE_CODE();
if (read_only) {
@@ -1323,9 +1326,23 @@ static int bdrv_backing_update_filename(BdrvChild *c,
BlockDriverState *base,
}
}
-ret = bdrv_change_backing_file(parent, filename,
- base->drv ? base->drv->format_name : "",
- false);
+if (base->drv) {
+/*
+ * If the new base image doesn't have a format driver layer, which we
+ * detect by the fact that @base is a protocol driver, we record
+ * 'raw' as the format instead of putting the protocol name as the
+ * backing format
+ */
+if (backing_mask_protocol && base->drv->protocol_name) {
+format_name = "raw";
+} else {
+format_name = base->drv->format_name;
+}
+} else {
+format_name = "";
+}
+
+ret = bdrv_change_backing_file(parent, filename, format_name, false);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not update backing file link");
}
@@ -1479,10 +1496,14 @@ static void GRAPH_WRLOCK bdrv_child_cb_detach(BdrvChild
*child)
}
static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
- const char *filename, Error **errp)
+ const char *filename,
+ bool backing_mask_protocol,
+ Error **errp)
{
if (c->role & BDRV_CHILD_COW) {
-return bdrv_backing_update_filename(c, base, filename, errp);
+return bdrv_backing_update_filename(c, base, filename,
+backing_mask_protocol,
+errp);
}
return 0;
}
@@ -5961,7 +5982,8 @@ void bdrv_unfreeze_backing_chain(BlockDriverState *bs,
BlockDriverState *base)
*
*/
int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
- const char *backing_file_str)
+ const char *backing_file_str,
+ bool backing_mask_protocol)
{
BlockDriverState *explicit_top = top;
bool update_inherits_from;
@@ -6027,6 +6049,7 @@ int bdrv_drop_intermediate(BlockDriverState *top,
BlockDriverState *base,
if (c->klass->update_filename) {
ret = c->klass->update_filename(c, base, backing_file_str,
+backing_mask_protocol,
_err);
if (ret < 0) {
/*
diff --git a/block/commit.c b/block/commit.c
index 69cc75be0c..4c351644c1 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -42,6 +42,7 @@ typedef struct CommitBlockJob {
bool base_read_only;
bool chain_frozen;
char *backing_file_str;
+bool backing_mask_protocol;
} CommitBlockJob;
static int commit_prepare(Job *job)
@@ -61,7 +62,8 @@ static int commit_prepare(Job *job)
/* FIXME: bdrv_drop_intermediate treats total failures and
[PATCH v3 2/2] block: stream: Allow users to request only format driver names in backing file format
Introduce a new flag 'backing-mask-protocol' for the block-stream QMP
command which instructs the internals to use 'raw' instead of the
protocol driver in case when a image is used without a dummy 'raw'
wrapper.
The flag is designed such that it can be always asserted by management
tools even when there isn't any update to backing files.
The flag will be used by libvirt so that the backing images still
reference the proper format even when libvirt will stop using the dummy
raw driver (raw driver with no other config). Libvirt needs this so that
the images stay compatible with older libvirt versions which didn't
expect that a protocol driver name can appear in the backing file format
field.
Signed-off-by: Peter Krempa
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/monitor/block-hmp-cmds.c | 2 +-
block/stream.c | 10 +-
blockdev.c | 7 +++
include/block/block_int-global-state.h | 3 +++
qapi/block-core.json | 9 -
5 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index c729cbf1eb..9080e29d4d 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -509,7 +509,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
const char *base = qdict_get_try_str(qdict, "base");
int64_t speed = qdict_get_try_int(qdict, "speed", 0);
-qmp_block_stream(device, device, base, NULL, NULL, NULL,
+qmp_block_stream(device, device, base, NULL, NULL, false, false, NULL,
qdict_haskey(qdict, "speed"), speed,
true, BLOCKDEV_ON_ERROR_REPORT, NULL,
false, false, false, false, );
diff --git a/block/stream.c b/block/stream.c
index 01fe7c0f16..da62410dde 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -39,6 +39,7 @@ typedef struct StreamBlockJob {
BlockDriverState *target_bs;
BlockdevOnError on_error;
char *backing_file_str;
+bool backing_mask_protocol;
bool bs_read_only;
} StreamBlockJob;
@@ -95,7 +96,12 @@ static int stream_prepare(Job *job)
if (unfiltered_base) {
base_id = s->backing_file_str ?: unfiltered_base->filename;
if (unfiltered_base->drv) {
-base_fmt = unfiltered_base->drv->format_name;
+if (s->backing_mask_protocol &&
+unfiltered_base->drv->protocol_name) {
+base_fmt = "raw";
+} else {
+base_fmt = unfiltered_base->drv->format_name;
+}
}
}
@@ -247,6 +253,7 @@ static const BlockJobDriver stream_job_driver = {
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
+ bool backing_mask_protocol,
BlockDriverState *bottom,
int creation_flags, int64_t speed,
BlockdevOnError on_error,
@@ -398,6 +405,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
s->base_overlay = base_overlay;
s->above_base = above_base;
s->backing_file_str = g_strdup(backing_file_str);
+s->backing_mask_protocol = backing_mask_protocol;
s->cor_filter_bs = cor_filter_bs;
s->target_bs = bs;
s->bs_read_only = bs_read_only;
diff --git a/blockdev.c b/blockdev.c
index 9ced07a8c4..a5ca1cf3ad 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2408,6 +2408,8 @@ void qmp_block_stream(const char *job_id, const char
*device,
const char *base,
const char *base_node,
const char *backing_file,
+ bool has_backing_mask_protocol,
+ bool backing_mask_protocol,
const char *bottom,
bool has_speed, int64_t speed,
bool has_on_error, BlockdevOnError on_error,
@@ -2443,6 +2445,10 @@ void qmp_block_stream(const char *job_id, const char
*device,
return;
}
+if (!has_backing_mask_protocol) {
+backing_mask_protocol = false;
+}
+
if (!has_on_error) {
on_error = BLOCKDEV_ON_ERROR_REPORT;
}
@@ -2531,6 +2537,7 @@ void qmp_block_stream(const char *job_id, const char
*device,
}
stream_start(job_id, bs, base_bs, backing_file,
+ backing_mask_protocol,
bottom_bs, job_flags, has_speed ? speed : 0, on_error,
filter_node_name, _err);
if (local_err) {
diff --git a/include/block/block_int-global-state.h
b/include/block/block_int-global-state.h
index 2162269df6..d2201e27f4 100644
--- a/include/block/block_int-global-state.h
+++ b/include/block/block_int-global-state.h
@@ -46,6 +46,8 @@
* flatten the whole backing file chain onto @bs.
* @backing_file_str: The file name that will be written to @bs as the
[PATCH v3 0/2] block: commit/stream: Allow users to request only format driver names in backing file format
Please see patches for rationale. Libvirt patches using this new flag will be posted soon-ish (after cleanup). v3: - changed name of flag to 'backing-mask-protocol' (Eric) - decided to keep Vladimir's R-b as he requested shorter name too v2: - fixed mistaken argument order in 'hmp_block_stream' - changed version in docs to 9.0 as getting this into RC 3 probably isn't realistic Peter Krempa (2): block: commit: Allow users to request only format driver names in backing file format block: stream: Allow users to request only format driver names in backing file format block.c| 37 +- block/commit.c | 6 - block/monitor/block-hmp-cmds.c | 2 +- block/stream.c | 10 ++- blockdev.c | 13 + include/block/block-global-state.h | 3 ++- include/block/block_int-common.h | 4 ++- include/block/block_int-global-state.h | 6 + qapi/block-core.json | 17 ++-- tests/unit/test-bdrv-drain.c | 3 ++- 10 files changed, 86 insertions(+), 15 deletions(-) -- 2.43.0
[PATCH 1/1] xlnx-versal-ospi: disable reentrancy detection for iomem_dac
The OSPI DMA reads flash data through the OSPI linear address space (the
iomem_dac region), because of this the reentrancy guard introduced in
commit a2e1753b ("memory: prevent dma-reentracy issues") is disabled for
the memory region.
Signed-off-by: Sai Pavan Boddu
---
hw/ssi/xlnx-versal-ospi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/ssi/xlnx-versal-ospi.c b/hw/ssi/xlnx-versal-ospi.c
index 1a61679c2f..5123e7dde7 100644
--- a/hw/ssi/xlnx-versal-ospi.c
+++ b/hw/ssi/xlnx-versal-ospi.c
@@ -1772,6 +1772,7 @@ static void xlnx_versal_ospi_init(Object *obj)
memory_region_init_io(>iomem_dac, obj, _dac_ops, s,
TYPE_XILINX_VERSAL_OSPI "-dac", 0x2000);
sysbus_init_mmio(sbd, >iomem_dac);
+s->iomem_dac.disable_reentrancy_guard = true;
sysbus_init_irq(sbd, >irq);
--
2.25.1
[PATCH 0/1] versal-ospi fix
Disable reentrancy on iomem_dac memory-region. Sai Pavan Boddu (1): xlnx-versal-ospi: disable reentrancy detection for iomem_dac hw/ssi/xlnx-versal-ospi.c | 1 + 1 file changed, 1 insertion(+) -- 2.25.1
[PATCH 0/2] OSPI updates
Add a new 2Gib octal flash mt35xu02gbba. Add an interface for versal virt board to swap the default flash. Sai Pavan Boddu (2): block: m25p80: Add support of mt35xu02gbba arm: xlnx-versal-virt: Add machine property ospi-flash hw/block/m25p80_sfdp.h| 1 + hw/arm/xlnx-versal-virt.c | 31 ++- hw/block/m25p80.c | 3 +++ hw/block/m25p80_sfdp.c| 36 4 files changed, 70 insertions(+), 1 deletion(-) -- 2.25.1
[PATCH 1/2] block: m25p80: Add support of mt35xu02gbba
Add Micro 2Gb OSPI flash part with sfdp data.
Signed-off-by: Sai Pavan Boddu
---
hw/block/m25p80_sfdp.h | 1 +
hw/block/m25p80.c | 3 +++
hw/block/m25p80_sfdp.c | 36
3 files changed, 40 insertions(+)
diff --git a/hw/block/m25p80_sfdp.h b/hw/block/m25p80_sfdp.h
index 011a880f66..1733b56950 100644
--- a/hw/block/m25p80_sfdp.h
+++ b/hw/block/m25p80_sfdp.h
@@ -16,6 +16,7 @@
#define M25P80_SFDP_MAX_SIZE (1 << 24)
uint8_t m25p80_sfdp_n25q256a(uint32_t addr);
+uint8_t m25p80_sfdp_mt35xu02g(uint32_t addr);
uint8_t m25p80_sfdp_mx25l25635e(uint32_t addr);
uint8_t m25p80_sfdp_mx25l25635f(uint32_t addr);
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index afc3fdf4d6..c8c7f6c1c3 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -267,6 +267,9 @@ static const FlashPartInfo known_devices[] = {
{ INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
{ INFO_STACKED("mt35xu01g", 0x2c5b1b, 0x104100, 128 << 10, 1024,
ER_4K | ER_32K, 2) },
+{ INFO_STACKED("mt35xu02gbba", 0x2c5b1c, 0x104100, 128 << 10, 2048,
+ ER_4K | ER_32K, 4),
+ .sfdp_read = m25p80_sfdp_mt35xu02g },
{ INFO_STACKED("n25q00",0x20ba21, 0x1000, 64 << 10, 2048, ER_4K, 4) },
{ INFO_STACKED("n25q00a", 0x20bb21, 0x1000, 64 << 10, 2048, ER_4K, 4) },
{ INFO_STACKED("mt25ql01g", 0x20ba21, 0x1040, 64 << 10, 2048, ER_4K, 2) },
diff --git a/hw/block/m25p80_sfdp.c b/hw/block/m25p80_sfdp.c
index b33811a4f5..6ee2cfaf11 100644
--- a/hw/block/m25p80_sfdp.c
+++ b/hw/block/m25p80_sfdp.c
@@ -57,6 +57,42 @@ static const uint8_t sfdp_n25q256a[] = {
};
define_sfdp_read(n25q256a);
+static const uint8_t sfdp_mt35xu02g[] = {
+0x53, 0x46, 0x44, 0x50, 0x06, 0x01, 0x01, 0xff,
+0x00, 0x06, 0x01, 0x10, 0x30, 0x00, 0x00, 0xff,
+0x84, 0x00, 0x01, 0x02, 0x80, 0x00, 0x00, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xe5, 0x20, 0x8a, 0xff, 0xff, 0xff, 0xff, 0x7f,
+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+0xee, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00,
+0xff, 0xff, 0x00, 0x00, 0x0c, 0x20, 0x11, 0xd8,
+0x0f, 0x52, 0x00, 0x00, 0x24, 0x5a, 0x99, 0x00,
+0x8b, 0x8e, 0x03, 0xe1, 0xac, 0x01, 0x27, 0x38,
+0x7a, 0x75, 0x7a, 0x75, 0xfb, 0xbd, 0xd5, 0x5c,
+0x00, 0x00, 0x70, 0xff, 0x81, 0xb0, 0x38, 0x36,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0x43, 0x0e, 0xff, 0xff, 0x21, 0xdc, 0x5c, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+};
+
+define_sfdp_read(mt35xu02g);
/*
* Matronix
--
2.25.1
[PATCH 1/1] xlnx-versal-ospi: disable reentrancy detection for iomem_dac
The OSPI DMA reads flash data through the OSPI linear address space (the
iomem_dac region), because of this the reentrancy guard introduced in
commit a2e1753b ("memory: prevent dma-reentracy issues") is disabled for
the memory region.
Signed-off-by: Sai Pavan Boddu
---
hw/ssi/xlnx-versal-ospi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/ssi/xlnx-versal-ospi.c b/hw/ssi/xlnx-versal-ospi.c
index 1a61679c2f..5123e7dde7 100644
--- a/hw/ssi/xlnx-versal-ospi.c
+++ b/hw/ssi/xlnx-versal-ospi.c
@@ -1772,6 +1772,7 @@ static void xlnx_versal_ospi_init(Object *obj)
memory_region_init_io(>iomem_dac, obj, _dac_ops, s,
TYPE_XILINX_VERSAL_OSPI "-dac", 0x2000);
sysbus_init_mmio(sbd, >iomem_dac);
+s->iomem_dac.disable_reentrancy_guard = true;
sysbus_init_irq(sbd, >irq);
--
2.25.1
[PATCH 0/1] versal-ospi fix
Disable reentrancy on iomem_dac memory-region. Sai Pavan Boddu (1): xlnx-versal-ospi: disable reentrancy detection for iomem_dac hw/ssi/xlnx-versal-ospi.c | 1 + 1 file changed, 1 insertion(+) -- 2.25.1
[PATCH 2/2] arm: xlnx-versal-virt: Add machine property ospi-flash
This property allows users to change flash model on command line as
below.
ex: "-M xlnx-versal-virt,ospi-flash=mt35xu02gbba"
Signed-off-by: Sai Pavan Boddu
---
hw/arm/xlnx-versal-virt.c | 31 ++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index 537118224f..c57cff74d8 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -49,6 +49,7 @@ struct VersalVirt {
struct {
bool secure;
} cfg;
+char *ospi_model;
};
static void fdt_create(VersalVirt *s)
@@ -637,6 +638,22 @@ static void sd_plugin_card(SDHCIState *sd, DriveInfo *di)
_fatal);
}
+static char *versal_get_ospi_model(Object *obj, Error **errp)
+{
+VersalVirt *s = XLNX_VERSAL_VIRT_MACHINE(obj);
+
+return g_strdup(s->ospi_model);
+}
+
+static void versal_set_ospi_model(Object *obj, const char *value, Error **errp)
+{
+VersalVirt *s = XLNX_VERSAL_VIRT_MACHINE(obj);
+
+g_free(s->ospi_model);
+s->ospi_model = g_strdup(value);
+}
+
+
static void versal_virt_init(MachineState *machine)
{
VersalVirt *s = XLNX_VERSAL_VIRT_MACHINE(machine);
@@ -736,7 +753,7 @@ static void versal_virt_init(MachineState *machine)
spi_bus = qdev_get_child_bus(DEVICE(>soc.pmc.iou.ospi), "spi0");
-flash_dev = qdev_new("mt35xu01g");
+flash_dev = qdev_new(s->ospi_model ? s->ospi_model : "mt35xu01g");
if (dinfo) {
qdev_prop_set_drive_err(flash_dev, "drive",
blk_by_legacy_dinfo(dinfo), _fatal);
@@ -769,6 +786,13 @@ static void versal_virt_machine_instance_init(Object *obj)
0);
}
+static void versal_virt_machine_finalize(Object *obj)
+{
+VersalVirt *s = XLNX_VERSAL_VIRT_MACHINE(obj);
+
+g_free(s->ospi_model);
+}
+
static void versal_virt_machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -780,6 +804,10 @@ static void versal_virt_machine_class_init(ObjectClass
*oc, void *data)
mc->default_cpus = XLNX_VERSAL_NR_ACPUS + XLNX_VERSAL_NR_RCPUS;
mc->no_cdrom = true;
mc->default_ram_id = "ddr";
+object_class_property_add_str(oc, "ospi-flash", versal_get_ospi_model,
+ versal_set_ospi_model);
+object_class_property_set_description(oc, "ospi-flash",
+ "Change the OSPI Flash model");
}
static const TypeInfo versal_virt_machine_init_typeinfo = {
@@ -788,6 +816,7 @@ static const TypeInfo versal_virt_machine_init_typeinfo = {
.class_init = versal_virt_machine_class_init,
.instance_init = versal_virt_machine_instance_init,
.instance_size = sizeof(VersalVirt),
+.instance_finalize = versal_virt_machine_finalize,
};
static void versal_virt_machine_init_register_types(void)
--
2.25.1
