* [PATCH v3 1/8] cpus: Export queue work related fields to cpu.h
2021-07-28 18:31 [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
@ 2021-07-28 18:31 ` Peter Xu
2021-07-28 18:40 ` Philippe Mathieu-Daudé
2021-07-28 18:31 ` [PATCH v3 2/8] cpus: Move do_run_on_cpu into softmmu/cpus.c Peter Xu
` (7 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: Peter Xu @ 2021-07-28 18:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Richard Henderson, Philippe Mathieu-Daudé,
peterx, David Hildenbrand
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/8] cpus: Export queue work related fields to cpu.h
2021-07-28 18:31 ` [PATCH v3 1/8] cpus: Export queue work related fields to cpu.h Peter Xu
@ 2021-07-28 18:40 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-28 18:40 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, David Hildenbrand
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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 2/8] cpus: Move do_run_on_cpu into softmmu/cpus.c
2021-07-28 18:31 [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
2021-07-28 18:31 ` [PATCH v3 1/8] cpus: Export queue work related fields to cpu.h Peter Xu
@ 2021-07-28 18:31 ` Peter Xu
2021-07-28 18:35 ` Philippe Mathieu-Daudé
2021-07-28 18:31 ` [PATCH v3 3/8] memory: Introduce memory_region_transaction_depth_{inc|dec}() Peter Xu
` (6 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: Peter Xu @ 2021-07-28 18:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Richard Henderson, Philippe Mathieu-Daudé,
peterx, David Hildenbrand
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/8] cpus: Move do_run_on_cpu into softmmu/cpus.c
2021-07-28 18:31 ` [PATCH v3 2/8] cpus: Move do_run_on_cpu into softmmu/cpus.c Peter Xu
@ 2021-07-28 18:35 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-28 18:35 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, David Hildenbrand
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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 3/8] memory: Introduce memory_region_transaction_depth_{inc|dec}()
2021-07-28 18:31 [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
2021-07-28 18:31 ` [PATCH v3 1/8] cpus: Export queue work related fields to cpu.h Peter Xu
2021-07-28 18:31 ` [PATCH v3 2/8] cpus: Move do_run_on_cpu into softmmu/cpus.c Peter Xu
@ 2021-07-28 18:31 ` Peter Xu
2021-07-28 20:06 ` David Hildenbrand
2021-07-28 18:31 ` [PATCH v3 4/8] memory: Don't do topology update in memory finalize() Peter Xu
` (5 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: Peter Xu @ 2021-07-28 18:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Richard Henderson, Philippe Mathieu-Daudé,
peterx, David Hildenbrand
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/8] memory: Introduce memory_region_transaction_depth_{inc|dec}()
2021-07-28 18:31 ` [PATCH v3 3/8] memory: Introduce memory_region_transaction_depth_{inc|dec}() Peter Xu
@ 2021-07-28 20:06 ` David Hildenbrand
0 siblings, 0 replies; 15+ messages in thread
From: David Hildenbrand @ 2021-07-28 20:06 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 4/8] memory: Don't do topology update in memory finalize()
2021-07-28 18:31 [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
` (2 preceding siblings ...)
2021-07-28 18:31 ` [PATCH v3 3/8] memory: Introduce memory_region_transaction_depth_{inc|dec}() Peter Xu
@ 2021-07-28 18:31 ` Peter Xu
2021-07-28 18:31 ` [PATCH v3 5/8] cpus: Use qemu_cond_wait_iothread() where proper Peter Xu
` (4 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Peter Xu @ 2021-07-28 18:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Richard Henderson, Philippe Mathieu-Daudé,
peterx, David Hildenbrand
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 5/8] cpus: Use qemu_cond_wait_iothread() where proper
2021-07-28 18:31 [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
` (3 preceding siblings ...)
2021-07-28 18:31 ` [PATCH v3 4/8] memory: Don't do topology update in memory finalize() Peter Xu
@ 2021-07-28 18:31 ` Peter Xu
2021-07-28 18:38 ` Philippe Mathieu-Daudé
2021-07-28 18:31 ` [PATCH v3 6/8] cpus: Remove the mutex parameter from do_run_on_cpu() Peter Xu
` (3 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: Peter Xu @ 2021-07-28 18:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Richard Henderson, Philippe Mathieu-Daudé,
peterx, David Hildenbrand
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 5/8] cpus: Use qemu_cond_wait_iothread() where proper
2021-07-28 18:31 ` [PATCH v3 5/8] cpus: Use qemu_cond_wait_iothread() where proper Peter Xu
@ 2021-07-28 18:38 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-28 18:38 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, David Hildenbrand
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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 6/8] cpus: Remove the mutex parameter from do_run_on_cpu()
2021-07-28 18:31 [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
` (4 preceding siblings ...)
2021-07-28 18:31 ` [PATCH v3 5/8] cpus: Use qemu_cond_wait_iothread() where proper Peter Xu
@ 2021-07-28 18:31 ` Peter Xu
2021-07-28 18:39 ` Philippe Mathieu-Daudé
2021-07-28 18:31 ` [PATCH v3 7/8] memory: Assert on no ongoing memory transaction before release BQL Peter Xu
` (2 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: Peter Xu @ 2021-07-28 18:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Richard Henderson, Philippe Mathieu-Daudé,
peterx, David Hildenbrand
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 6/8] cpus: Remove the mutex parameter from do_run_on_cpu()
2021-07-28 18:31 ` [PATCH v3 6/8] cpus: Remove the mutex parameter from do_run_on_cpu() Peter Xu
@ 2021-07-28 18:39 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-28 18:39 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, David Hildenbrand
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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 7/8] memory: Assert on no ongoing memory transaction before release BQL
2021-07-28 18:31 [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
` (5 preceding siblings ...)
2021-07-28 18:31 ` [PATCH v3 6/8] cpus: Remove the mutex parameter from do_run_on_cpu() Peter Xu
@ 2021-07-28 18:31 ` Peter Xu
2021-07-28 18:31 ` [PATCH v3 8/8] memory: Delay the transaction pop() until commit completed Peter Xu
2021-09-08 15:30 ` [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
8 siblings, 0 replies; 15+ messages in thread
From: Peter Xu @ 2021-07-28 18:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Richard Henderson, Philippe Mathieu-Daudé,
peterx, David Hildenbrand
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 8/8] memory: Delay the transaction pop() until commit completed
2021-07-28 18:31 [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
` (6 preceding siblings ...)
2021-07-28 18:31 ` [PATCH v3 7/8] memory: Assert on no ongoing memory transaction before release BQL Peter Xu
@ 2021-07-28 18:31 ` Peter Xu
2021-09-08 15:30 ` [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
8 siblings, 0 replies; 15+ messages in thread
From: Peter Xu @ 2021-07-28 18:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Richard Henderson, Philippe Mathieu-Daudé,
peterx, David Hildenbrand
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL
2021-07-28 18:31 [PATCH v3 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
` (7 preceding siblings ...)
2021-07-28 18:31 ` [PATCH v3 8/8] memory: Delay the transaction pop() until commit completed Peter Xu
@ 2021-09-08 15:30 ` Peter Xu
8 siblings, 0 replies; 15+ messages in thread
From: Peter Xu @ 2021-09-08 15:30 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Richard Henderson, Philippe Mathieu-Daudé,
David Hildenbrand
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
^ permalink raw reply [flat|nested] 15+ messages in thread