[PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL

[PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL

[Peter Xu]

This is v3 of the series. And, I think this is 6.2 material.

v2 changelog:
- Init qemu_work_item directly in do_run_on_cpu() [David]
- Rename memory_region_transaction_{push|pop} to *_depth_{inc|dec} [David]
- Changing wording of s/helpless/useless/ [David]
- Squashed v2 patch 7 into patch 8 [David]
- Collected r-bs

It was actually got forgotten for months until it was used to identify another
potential issue of bql usage here (besides it could still be helpful when
debugging a previous kvm dirty ring issue in that series):

https://lore.kernel.org/qemu-devel/CH0PR02MB7898BBD73D0F3F7D5003BB178BE19@CH0PR02MB7898.namprd02.prod.outlook.com/

So I figured maybe it's still worth to have it, hence a repost.

There're some changes against v1:

  - patch "cpus: Introduce qemu_cond_timedwait_iothread()" is dropped because
    it's introduced in another commit already (b0c3cf9407e64).

  - two more patches to move do_run_on_cpu() into softmmu/ to fix a linux-user
    compliation issue.

Please review, thanks.

=== Original Cover letter ===

This is a continuous work of previous discussion on memory transactions [1].
It should be helpful to fail QEMU far earlier if there's misuse of BQL against
the QEMU memory model.

One example is run_on_cpu() during memory commit.  That'll work previously, but
it'll fail with very strange errors (like KVM ioctl failure due to memslot
already existed, and it's not guaranteed to trigger constantly).  Now it'll
directly fail when run_on_cpu() is called.

Please have a look, thanks.

[1] https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03205.html

Peter Xu (8):
  cpus: Export queue work related fields to cpu.h
  cpus: Move do_run_on_cpu into softmmu/cpus.c
  memory: Introduce memory_region_transaction_depth_{inc|dec}()
  memory: Don't do topology update in memory finalize()
  cpus: Use qemu_cond_wait_iothread() where proper
  cpus: Remove the mutex parameter from do_run_on_cpu()
  memory: Assert on no ongoing memory transaction before release BQL
  memory: Delay the transaction pop() until commit completed

 cpus-common.c                  | 36 ++---------------------
 include/exec/memory-internal.h |  1 +
 include/hw/core/cpu.h          | 22 ++++++--------
 softmmu/cpus.c                 | 39 ++++++++++++++++++++++---
 softmmu/memory.c               | 53 ++++++++++++++++++++++++++++++----
 5 files changed, 94 insertions(+), 57 deletions(-)

-- 
2.31.1

[PATCH v3 1/8] cpus: Export queue work related fields to cpu.h

[Peter Xu]

This patch has no functional change, but prepares for moving the function
do_run_on_cpu() into softmmu/cpus.c.  It does:

  1. Move qemu_work_item into hw/core/cpu.h.
  2. Export queue_work_on_cpu()/qemu_work_cond.

All of them will be used by softmmu/cpus.c later.

Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 cpus-common.c         | 11 ++---------
 include/hw/core/cpu.h | 10 +++++++++-
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/cpus-common.c b/cpus-common.c
index 6e73d3e58d..d814b2439a 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -27,7 +27,7 @@
 static QemuMutex qemu_cpu_list_lock;
 static QemuCond exclusive_cond;
 static QemuCond exclusive_resume;
-static QemuCond qemu_work_cond;
+QemuCond qemu_work_cond;
 
 /* >= 1 if a thread is inside start_exclusive/end_exclusive.  Written
  * under qemu_cpu_list_lock, read with atomic operations.
@@ -114,14 +114,7 @@ CPUState *qemu_get_cpu(int index)
 /* current CPU in the current thread. It is only valid inside cpu_exec() */
 __thread CPUState *current_cpu;
 
-struct qemu_work_item {
-    QSIMPLEQ_ENTRY(qemu_work_item) node;
-    run_on_cpu_func func;
-    run_on_cpu_data data;
-    bool free, exclusive, done;
-};
-
-static void queue_work_on_cpu(CPUState *cpu, struct qemu_work_item *wi)
+void queue_work_on_cpu(CPUState *cpu, struct qemu_work_item *wi)
 {
     qemu_mutex_lock(&cpu->work_mutex);
     QSIMPLEQ_INSERT_TAIL(&cpu->work_list, wi, node);
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index bc864564ce..f62ae88524 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -243,7 +243,15 @@ typedef union {
 
 typedef void (*run_on_cpu_func)(CPUState *cpu, run_on_cpu_data data);
 
-struct qemu_work_item;
+struct qemu_work_item {
+    QSIMPLEQ_ENTRY(qemu_work_item) node;
+    run_on_cpu_func func;
+    run_on_cpu_data data;
+    bool free, exclusive, done;
+};
+
+void queue_work_on_cpu(CPUState *cpu, struct qemu_work_item *wi);
+extern QemuCond qemu_work_cond;
 
 #define CPU_UNSET_NUMA_NODE_ID -1
 #define CPU_TRACE_DSTATE_MAX_EVENTS 32
-- 
2.31.1

Re: [PATCH v3 1/8] cpus: Export queue work related fields to cpu.h

[Philippe Mathieu-Daudé]

On 7/28/21 8:31 PM, Peter Xu wrote:
> This patch has no functional change, but prepares for moving the function
> do_run_on_cpu() into softmmu/cpus.c.  It does:
> 
>   1. Move qemu_work_item into hw/core/cpu.h.
>   2. Export queue_work_on_cpu()/qemu_work_cond.
> 
> All of them will be used by softmmu/cpus.c later.
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  cpus-common.c         | 11 ++---------
>  include/hw/core/cpu.h | 10 +++++++++-
>  2 files changed, 11 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH v3 2/8] cpus: Move do_run_on_cpu into softmmu/cpus.c

[Peter Xu]

It's only used by softmmu binaries not linux-user ones.  Make it static and
drop the definition in the header too.

Since at it, initialize variable "wi" with less loc.

Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 cpus-common.c         | 25 -------------------------
 include/hw/core/cpu.h | 12 ------------
 softmmu/cpus.c        | 23 +++++++++++++++++++++++
 3 files changed, 23 insertions(+), 37 deletions(-)

diff --git a/cpus-common.c b/cpus-common.c
index d814b2439a..670826363f 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -124,31 +124,6 @@ void queue_work_on_cpu(CPUState *cpu, struct qemu_work_item *wi)
     qemu_cpu_kick(cpu);
 }
 
-void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data,
-                   QemuMutex *mutex)
-{
-    struct qemu_work_item wi;
-
-    if (qemu_cpu_is_self(cpu)) {
-        func(cpu, data);
-        return;
-    }
-
-    wi.func = func;
-    wi.data = data;
-    wi.done = false;
-    wi.free = false;
-    wi.exclusive = false;
-
-    queue_work_on_cpu(cpu, &wi);
-    while (!qatomic_mb_read(&wi.done)) {
-        CPUState *self_cpu = current_cpu;
-
-        qemu_cond_wait(&qemu_work_cond, mutex);
-        current_cpu = self_cpu;
-    }
-}
-
 void async_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data)
 {
     struct qemu_work_item *wi;
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index f62ae88524..711ecad62f 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -689,18 +689,6 @@ void qemu_cpu_kick(CPUState *cpu);
  */
 bool cpu_is_stopped(CPUState *cpu);
 
-/**
- * do_run_on_cpu:
- * @cpu: The vCPU to run on.
- * @func: The function to be executed.
- * @data: Data to pass to the function.
- * @mutex: Mutex to release while waiting for @func to run.
- *
- * Used internally in the implementation of run_on_cpu.
- */
-void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data,
-                   QemuMutex *mutex);
-
 /**
  * run_on_cpu:
  * @cpu: The vCPU to run on.
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 071085f840..49e0368438 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -382,6 +382,29 @@ void qemu_init_cpu_loop(void)
     qemu_thread_get_self(&io_thread);
 }
 
+static void
+do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data,
+              QemuMutex *mutex)
+{
+    struct qemu_work_item wi = {
+        .func = func,
+        .data = data,
+    };
+
+    if (qemu_cpu_is_self(cpu)) {
+        func(cpu, data);
+        return;
+    }
+
+    queue_work_on_cpu(cpu, &wi);
+    while (!qatomic_mb_read(&wi.done)) {
+        CPUState *self_cpu = current_cpu;
+
+        qemu_cond_wait(&qemu_work_cond, mutex);
+        current_cpu = self_cpu;
+    }
+}
+
 void run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data)
 {
     do_run_on_cpu(cpu, func, data, &qemu_global_mutex);
-- 
2.31.1

Re: [PATCH v3 2/8] cpus: Move do_run_on_cpu into softmmu/cpus.c

[Philippe Mathieu-Daudé]

On 7/28/21 8:31 PM, Peter Xu wrote:
> It's only used by softmmu binaries not linux-user ones.  Make it static and
> drop the definition in the header too.
> 
> Since at it, initialize variable "wi" with less loc.
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  cpus-common.c         | 25 -------------------------
>  include/hw/core/cpu.h | 12 ------------
>  softmmu/cpus.c        | 23 +++++++++++++++++++++++
>  3 files changed, 23 insertions(+), 37 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH v3 3/8] memory: Introduce memory_region_transaction_depth_{inc|dec}()

[Peter Xu]

memory_region_transaction_{begin|commit}() could be too big when finalizing a
memory region.  E.g., we should never attempt to update address space topology
during the finalize() of a memory region.  Provide helpers for further use.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 softmmu/memory.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index bfedaf9c4d..725d57ec17 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1079,10 +1079,20 @@ static void address_space_update_topology(AddressSpace *as)
     address_space_set_flatview(as);
 }
 
+static void memory_region_transaction_depth_inc(void)
+{
+    memory_region_transaction_depth++;
+}
+
+static void memory_region_transaction_depth_dec(void)
+{
+    memory_region_transaction_depth--;
+}
+
 void memory_region_transaction_begin(void)
 {
     qemu_flush_coalesced_mmio_buffer();
-    ++memory_region_transaction_depth;
+    memory_region_transaction_depth_inc();
 }
 
 void memory_region_transaction_commit(void)
@@ -1092,7 +1102,7 @@ void memory_region_transaction_commit(void)
     assert(memory_region_transaction_depth);
     assert(qemu_mutex_iothread_locked());
 
-    --memory_region_transaction_depth;
+    memory_region_transaction_depth_dec();
     if (!memory_region_transaction_depth) {
         if (memory_region_update_pending) {
             flatviews_reset();
-- 
2.31.1

Re: [PATCH v3 3/8] memory: Introduce memory_region_transaction_depth_{inc|dec}()

[David Hildenbrand]

On 28.07.21 20:31, Peter Xu wrote:
> memory_region_transaction_{begin|commit}() could be too big when finalizing a
> memory region.  E.g., we should never attempt to update address space topology
> during the finalize() of a memory region.  Provide helpers for further use.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   softmmu/memory.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4d..725d57ec17 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1079,10 +1079,20 @@ static void address_space_update_topology(AddressSpace *as)
>       address_space_set_flatview(as);
>   }
>   
> +static void memory_region_transaction_depth_inc(void)
> +{
> +    memory_region_transaction_depth++;
> +}
> +
> +static void memory_region_transaction_depth_dec(void)
> +{
> +    memory_region_transaction_depth--;
> +}
> +
>   void memory_region_transaction_begin(void)
>   {
>       qemu_flush_coalesced_mmio_buffer();
> -    ++memory_region_transaction_depth;
> +    memory_region_transaction_depth_inc();
>   }
>   
>   void memory_region_transaction_commit(void)
> @@ -1092,7 +1102,7 @@ void memory_region_transaction_commit(void)
>       assert(memory_region_transaction_depth);
>       assert(qemu_mutex_iothread_locked());
>   
> -    --memory_region_transaction_depth;
> +    memory_region_transaction_depth_dec();
>       if (!memory_region_transaction_depth) {
>           if (memory_region_update_pending) {
>               flatviews_reset();
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v3 4/8] memory: Don't do topology update in memory finalize()

[Peter Xu]

Topology update could be wrongly triggered in memory region finalize() if
there's bug somewhere else.  It'll be a very confusing stack when it
happens (e.g., sending KVM ioctl within the RCU thread, and we'll observe it
only until it fails!).

Instead of that, we use the push()/pop() helper to avoid memory transaction
commit, at the same time we use assertions to make sure there's no pending
updates or it's a nested transaction, so it could fail even earlier and in a
more explicit way.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 softmmu/memory.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 725d57ec17..35b2568fc2 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -170,6 +170,12 @@ struct MemoryRegionIoeventfd {
     EventNotifier *e;
 };
 
+/* Returns whether there's any pending memory updates */
+static bool memory_region_has_pending_update(void)
+{
+    return memory_region_update_pending || ioeventfd_update_pending;
+}
+
 static bool memory_region_ioeventfd_before(MemoryRegionIoeventfd *a,
                                            MemoryRegionIoeventfd *b)
 {
@@ -1756,12 +1762,25 @@ static void memory_region_finalize(Object *obj)
      * and cause an infinite loop.
      */
     mr->enabled = false;
-    memory_region_transaction_begin();
+
+    /*
+     * Use depth_inc()/depth_dec() instead of begin()/commit() to make sure
+     * below block won't trigger any topology update (which should never
+     * happen, but it's still a safety belt).
+     */
+    memory_region_transaction_depth_inc();
     while (!QTAILQ_EMPTY(&mr->subregions)) {
         MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions);
         memory_region_del_subregion(mr, subregion);
     }
-    memory_region_transaction_commit();
+    memory_region_transaction_depth_dec();
+
+    /*
+     * Make sure we're either in a nested transaction or there must have no
+     * pending updates due to memory_region_del_subregion() above.
+     */
+    assert(memory_region_transaction_depth ||
+           !memory_region_has_pending_update());
 
     mr->destructor(mr);
     memory_region_clear_coalescing(mr);
-- 
2.31.1

[PATCH v3 5/8] cpus: Use qemu_cond_wait_iothread() where proper

[Peter Xu]

The helper is introduced but we've still got plenty of places that are directly
referencing the qemu_global_mutex itself.  Spread the usage.

Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 softmmu/cpus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 49e0368438..e714dfbf2b 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -439,7 +439,7 @@ void qemu_wait_io_event(CPUState *cpu)
             slept = true;
             qemu_plugin_vcpu_idle_cb(cpu);
         }
-        qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(cpu->halt_cond);
     }
     if (slept) {
         qemu_plugin_vcpu_resume_cb(cpu);
@@ -582,7 +582,7 @@ void pause_all_vcpus(void)
     replay_mutex_unlock();
 
     while (!all_vcpus_paused()) {
-        qemu_cond_wait(&qemu_pause_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_pause_cond);
         CPU_FOREACH(cpu) {
             qemu_cpu_kick(cpu);
         }
@@ -653,7 +653,7 @@ void qemu_init_vcpu(CPUState *cpu)
     cpus_accel->create_vcpu_thread(cpu);
 
     while (!cpu->created) {
-        qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_cpu_cond);
     }
 }
 
-- 
2.31.1

Re: [PATCH v3 5/8] cpus: Use qemu_cond_wait_iothread() where proper

[Philippe Mathieu-Daudé]

On 7/28/21 8:31 PM, Peter Xu wrote:
> The helper is introduced but we've still got plenty of places that are directly
> referencing the qemu_global_mutex itself.  Spread the usage.
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  softmmu/cpus.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH v3 6/8] cpus: Remove the mutex parameter from do_run_on_cpu()

[Peter Xu]

We must use the BQL for do_run_on_cpu() without much choice, it means the
parameter is useless.  Remove it.  Meanwhile use the newly introduced
qemu_cond_wait_iothread() in do_run_on_cpu().

Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 softmmu/cpus.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index e714dfbf2b..9154cd7e78 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -383,8 +383,7 @@ void qemu_init_cpu_loop(void)
 }
 
 static void
-do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data,
-              QemuMutex *mutex)
+do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data)
 {
     struct qemu_work_item wi = {
         .func = func,
@@ -400,14 +399,14 @@ do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data,
     while (!qatomic_mb_read(&wi.done)) {
         CPUState *self_cpu = current_cpu;
 
-        qemu_cond_wait(&qemu_work_cond, mutex);
+        qemu_cond_wait_iothread(&qemu_work_cond);
         current_cpu = self_cpu;
     }
 }
 
 void run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data)
 {
-    do_run_on_cpu(cpu, func, data, &qemu_global_mutex);
+    do_run_on_cpu(cpu, func, data);
 }
 
 static void qemu_cpu_stop(CPUState *cpu, bool exit)
-- 
2.31.1

Re: [PATCH v3 6/8] cpus: Remove the mutex parameter from do_run_on_cpu()

[Philippe Mathieu-Daudé]

On 7/28/21 8:31 PM, Peter Xu wrote:
> We must use the BQL for do_run_on_cpu() without much choice, it means the
> parameter is useless.  Remove it.  Meanwhile use the newly introduced
> qemu_cond_wait_iothread() in do_run_on_cpu().
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  softmmu/cpus.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH v3 7/8] memory: Assert on no ongoing memory transaction before release BQL

[Peter Xu]

Firstly, add a "prepare" function before unlocking BQL.  There're only three
places that can release the BQL: unlock(), cond_wait() or cond_timedwait().

Make sure we don't have any more ongoing memory transaction when releasing the
BQL.  This will trigger an abort if we misuse the QEMU memory model, e.g., when
calling run_on_cpu() during a memory commit.

Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/exec/memory-internal.h | 1 +
 softmmu/cpus.c                 | 9 +++++++++
 softmmu/memory.c               | 6 ++++++
 3 files changed, 16 insertions(+)

diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
index 9fcc2af25c..3124b91c4b 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -35,6 +35,7 @@ static inline AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as)
 
 FlatView *address_space_get_flatview(AddressSpace *as);
 void flatview_unref(FlatView *view);
+bool memory_region_has_pending_transaction(void);
 
 extern const MemoryRegionOps unassigned_mem_ops;
 
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 9154cd7e78..4d190f9076 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -31,6 +31,7 @@
 #include "qapi/qapi-events-run-state.h"
 #include "qapi/qmp/qerror.h"
 #include "exec/gdbstub.h"
+#include "exec/memory-internal.h"
 #include "sysemu/hw_accel.h"
 #include "exec/exec-all.h"
 #include "qemu/thread.h"
@@ -66,6 +67,11 @@
 
 static QemuMutex qemu_global_mutex;
 
+static void qemu_mutex_unlock_iothread_prepare(void)
+{
+    assert(!memory_region_has_pending_transaction());
+}
+
 bool cpu_is_stopped(CPUState *cpu)
 {
     return cpu->stopped || !runstate_is_running();
@@ -520,16 +526,19 @@ void qemu_mutex_unlock_iothread(void)
 {
     g_assert(qemu_mutex_iothread_locked());
     iothread_locked = false;
+    qemu_mutex_unlock_iothread_prepare();
     qemu_mutex_unlock(&qemu_global_mutex);
 }
 
 void qemu_cond_wait_iothread(QemuCond *cond)
 {
+    qemu_mutex_unlock_iothread_prepare();
     qemu_cond_wait(cond, &qemu_global_mutex);
 }
 
 void qemu_cond_timedwait_iothread(QemuCond *cond, int ms)
 {
+    qemu_mutex_unlock_iothread_prepare();
     qemu_cond_timedwait(cond, &qemu_global_mutex, ms);
 }
 
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 35b2568fc2..62ec00b52d 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -176,6 +176,12 @@ static bool memory_region_has_pending_update(void)
     return memory_region_update_pending || ioeventfd_update_pending;
 }
 
+bool memory_region_has_pending_transaction(void)
+{
+    return memory_region_transaction_depth ||
+        memory_region_has_pending_update();
+}
+
 static bool memory_region_ioeventfd_before(MemoryRegionIoeventfd *a,
                                            MemoryRegionIoeventfd *b)
 {
-- 
2.31.1

[PATCH v3 8/8] memory: Delay the transaction pop() until commit completed

[Peter Xu]

This should be functionally the same as before, but this allows the
memory_region_transaction_depth to be non-zero during commit, which can help us
to do sanity check on misuses.

Since at it, fix an indentation issue on the bracket.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 softmmu/memory.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 62ec00b52d..e830649011 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1114,8 +1114,7 @@ void memory_region_transaction_commit(void)
     assert(memory_region_transaction_depth);
     assert(qemu_mutex_iothread_locked());
 
-    memory_region_transaction_depth_dec();
-    if (!memory_region_transaction_depth) {
+    if (memory_region_transaction_depth == 1) {
         if (memory_region_update_pending) {
             flatviews_reset();
 
@@ -1134,7 +1133,14 @@ void memory_region_transaction_commit(void)
             }
             ioeventfd_update_pending = false;
         }
-   }
+    }
+
+    /*
+     * Decrease the depth at last, so that memory_region_transaction_depth will
+     * still be non-zero during committing.  This can help us to do some sanity
+     * check within the process of committing.
+     */
+    memory_region_transaction_depth_dec();
 }
 
 static void memory_region_destructor_none(MemoryRegion *mr)
-- 
2.31.1

Re: [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL

[Peter Xu]

On Wed, Jul 28, 2021 at 02:31:43PM -0400, Peter Xu wrote:
> This is v3 of the series. And, I think this is 6.2 material.

Ping - just remembered this one too..

-- 
Peter Xu