[PATCH v2 0/7] main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD

[PATCH v2 0/7] main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD

[Richard Henderson]

Simplify the usage of qemu_mutex_lock_iothread.
Split out for ease of review.

Changes for v2:
  * Add WITH_QEMU_IOTHREAD_LOCK and use it a couple of places.
    This re-implements patch 1, so r-b's dropped.

r~


Richard Henderson (7):
  qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD,
    WITH_QEMU_IOTHREAD_LOCK
  hw/mips: Use WITH_QEMU_IOTHREAD_LOCK in cpu_mips_irq_request
  target/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in ppc_maybe_interrupt
  target/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in cpu_interrupt_exittb
  hw/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in ppc_set_irq
  target/riscv: Use QEMU_IOTHREAD_LOCK_GUARD in riscv_cpu_update_mip
  accel/tcg: Use WITH_QEMU_IOTHREAD_LOCK in io_readx/io_writex

 include/qemu/main-loop.h  | 39 +++++++++++++++++++++++++++++++++++++++
 accel/tcg/cputlb.c        | 23 ++++++-----------------
 hw/mips/mips_int.c        | 37 ++++++++++++++-----------------------
 hw/ppc/ppc.c              | 10 +---------
 target/ppc/excp_helper.c  | 11 +----------
 target/ppc/helper_regs.c  | 14 ++++----------
 target/riscv/cpu_helper.c | 22 +++++++---------------
 7 files changed, 72 insertions(+), 84 deletions(-)

-- 
2.34.1

[PATCH v2 1/7] qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD, WITH_QEMU_IOTHREAD_LOCK

[Richard Henderson]

Create a couple of wrappers for locking/unlocking the iothread lock.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/main-loop.h | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 3c9a9a982d..73c60a9af4 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -343,6 +343,45 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line);
  */
 void qemu_mutex_unlock_iothread(void);
 
+/**
+ * QEMU_IOTHREAD_LOCK_GUARD
+ * WITH_QEMU_IOTHREAD_LOCK
+ *
+ * Wrap a block of code in a conditional qemu_mutex_{lock,unlock}_iothread.
+ */
+typedef struct IOThreadLockAuto {
+    bool locked;
+    bool iterate;
+} IOThreadLockAuto;
+
+static inline IOThreadLockAuto qemu_iothread_auto_lock(const char *file,
+                                                       int line)
+{
+    bool need = !qemu_mutex_iothread_locked();
+    if (need) {
+        qemu_mutex_lock_iothread_impl(file, line);
+    }
+    return (IOThreadLockAuto){ .locked = need, .iterate = true };
+}
+
+static inline void qemu_iothread_auto_unlock(IOThreadLockAuto *l)
+{
+    if (l->locked) {
+        qemu_mutex_unlock_iothread();
+    }
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(IOThreadLockAuto, qemu_iothread_auto_unlock)
+
+#define QEMU_IOTHREAD_LOCK_GUARD()                              \
+    g_auto(IOThreadLockAuto) _iothread_lock_auto                \
+        = qemu_iothread_auto_lock(__FILE__, __LINE__)           \
+
+#define WITH_QEMU_IOTHREAD_LOCK()                               \
+    for (QEMU_IOTHREAD_LOCK_GUARD();                            \
+         _iothread_lock_auto.iterate;                           \
+         _iothread_lock_auto.iterate = false)
+
 /*
  * qemu_cond_wait_iothread: Wait on condition for the main loop mutex
  *
-- 
2.34.1

Re: [PATCH v2 1/7] qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD, WITH_QEMU_IOTHREAD_LOCK

[Philippe Mathieu-Daudé]

On 22/11/22 21:57, Richard Henderson wrote:
> Create a couple of wrappers for locking/unlocking the iothread lock.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/qemu/main-loop.h | 39 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)


> +#define QEMU_IOTHREAD_LOCK_GUARD()                              \
> +    g_auto(IOThreadLockAuto) _iothread_lock_auto                \
> +        = qemu_iothread_auto_lock(__FILE__, __LINE__)           \
> +
> +#define WITH_QEMU_IOTHREAD_LOCK()                               \
> +    for (QEMU_IOTHREAD_LOCK_GUARD();                            \
> +         _iothread_lock_auto.iterate;                           \
> +         _iothread_lock_auto.iterate = false)

Nice, thanks!

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 1/7] qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD, WITH_QEMU_IOTHREAD_LOCK

[Richard Henderson]

On 11/22/22 12:57, Richard Henderson wrote:
> +#define QEMU_IOTHREAD_LOCK_GUARD()                              \
> +    g_auto(IOThreadLockAuto) _iothread_lock_auto                \
> +        = qemu_iothread_auto_lock(__FILE__, __LINE__)           \

Need an extra G_GNUC_UNUSED for (some versions of?) clang.
Which is really a bug, because it's always used via the cleanup.


r~

[PATCH v2 2/7] hw/mips: Use WITH_QEMU_IOTHREAD_LOCK in cpu_mips_irq_request

[Richard Henderson]

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/mips/mips_int.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
index 2db5e10fe0..c22598d353 100644
--- a/hw/mips/mips_int.c
+++ b/hw/mips/mips_int.c
@@ -32,36 +32,27 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
     MIPSCPU *cpu = opaque;
     CPUMIPSState *env = &cpu->env;
     CPUState *cs = CPU(cpu);
-    bool locked = false;
 
     if (irq < 0 || irq > 7) {
         return;
     }
 
-    /* Make sure locking works even if BQL is already held by the caller */
-    if (!qemu_mutex_iothread_locked()) {
-        locked = true;
-        qemu_mutex_lock_iothread();
-    }
+    WITH_QEMU_IOTHREAD_LOCK() {
+        if (level) {
+            env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
+        } else {
+            env->CP0_Cause &= ~(1 << (irq + CP0Ca_IP));
+        }
 
-    if (level) {
-        env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
-    } else {
-        env->CP0_Cause &= ~(1 << (irq + CP0Ca_IP));
-    }
+        if (kvm_enabled() && (irq == 2 || irq == 3)) {
+            kvm_mips_set_interrupt(cpu, irq, level);
+        }
 
-    if (kvm_enabled() && (irq == 2 || irq == 3)) {
-        kvm_mips_set_interrupt(cpu, irq, level);
-    }
-
-    if (env->CP0_Cause & CP0Ca_IP_mask) {
-        cpu_interrupt(cs, CPU_INTERRUPT_HARD);
-    } else {
-        cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
-    }
-
-    if (locked) {
-        qemu_mutex_unlock_iothread();
+        if (env->CP0_Cause & CP0Ca_IP_mask) {
+            cpu_interrupt(cs, CPU_INTERRUPT_HARD);
+        } else {
+            cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
+        }
     }
 }
 
-- 
2.34.1

[PATCH v2 3/7] target/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in ppc_maybe_interrupt

[Richard Henderson]

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/excp_helper.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 94adcb766b..8591bb3f73 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2163,22 +2163,13 @@ static int ppc_next_unmasked_interrupt(CPUPPCState *env)
 void ppc_maybe_interrupt(CPUPPCState *env)
 {
     CPUState *cs = env_cpu(env);
-    bool locked = false;
-
-    if (!qemu_mutex_iothread_locked()) {
-        locked = true;
-        qemu_mutex_lock_iothread();
-    }
+    QEMU_IOTHREAD_LOCK_GUARD();
 
     if (ppc_next_unmasked_interrupt(env)) {
         cpu_interrupt(cs, CPU_INTERRUPT_HARD);
     } else {
         cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
     }
-
-    if (locked) {
-        qemu_mutex_unlock_iothread();
-    }
 }
 
 #if defined(TARGET_PPC64)
-- 
2.34.1

[PATCH v2 4/7] target/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in cpu_interrupt_exittb

[Richard Henderson]

In addition, use tcg_enabled instead of !kvm_enabled.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/helper_regs.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index c0aee5855b..779e7db513 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -22,6 +22,7 @@
 #include "qemu/main-loop.h"
 #include "exec/exec-all.h"
 #include "sysemu/kvm.h"
+#include "sysemu/tcg.h"
 #include "helper_regs.h"
 #include "power8-pmu.h"
 #include "cpu-models.h"
@@ -203,17 +204,10 @@ void cpu_interrupt_exittb(CPUState *cs)
 {
     /*
      * We don't need to worry about translation blocks
-     * when running with KVM.
+     * unless running with TCG.
      */
-    if (kvm_enabled()) {
-        return;
-    }
-
-    if (!qemu_mutex_iothread_locked()) {
-        qemu_mutex_lock_iothread();
-        cpu_interrupt(cs, CPU_INTERRUPT_EXITTB);
-        qemu_mutex_unlock_iothread();
-    } else {
+    if (tcg_enabled()) {
+        QEMU_IOTHREAD_LOCK_GUARD();
         cpu_interrupt(cs, CPU_INTERRUPT_EXITTB);
     }
 }
-- 
2.34.1

[PATCH v2 5/7] hw/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in ppc_set_irq

[Richard Henderson]

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/ppc/ppc.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index dc86c1c7db..4e816c68c7 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -44,13 +44,9 @@ void ppc_set_irq(PowerPCCPU *cpu, int irq, int level)
 {
     CPUPPCState *env = &cpu->env;
     unsigned int old_pending;
-    bool locked = false;
 
     /* We may already have the BQL if coming from the reset path */
-    if (!qemu_mutex_iothread_locked()) {
-        locked = true;
-        qemu_mutex_lock_iothread();
-    }
+    QEMU_IOTHREAD_LOCK_GUARD();
 
     old_pending = env->pending_interrupts;
 
@@ -67,10 +63,6 @@ void ppc_set_irq(PowerPCCPU *cpu, int irq, int level)
 
     trace_ppc_irq_set_exit(env, irq, level, env->pending_interrupts,
                            CPU(cpu)->interrupt_request);
-
-    if (locked) {
-        qemu_mutex_unlock_iothread();
-    }
 }
 
 /* PowerPC 6xx / 7xx internal IRQ controller */
-- 
2.34.1

[PATCH v2 6/7] target/riscv: Use QEMU_IOTHREAD_LOCK_GUARD in riscv_cpu_update_mip

[Richard Henderson]

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/riscv/cpu_helper.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 278d163803..4c4b404dad 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -610,7 +610,6 @@ uint64_t riscv_cpu_update_mip(RISCVCPU *cpu, uint64_t mask, uint64_t value)
     CPURISCVState *env = &cpu->env;
     CPUState *cs = CPU(cpu);
     uint64_t gein, vsgein = 0, vstip = 0, old = env->mip;
-    bool locked = false;
 
     if (riscv_cpu_virt_enabled(env)) {
         gein = get_field(env->hstatus, HSTATUS_VGEIN);
@@ -621,21 +620,14 @@ uint64_t riscv_cpu_update_mip(RISCVCPU *cpu, uint64_t mask, uint64_t value)
     mask = ((mask == MIP_VSTIP) && env->vstime_irq) ? 0 : mask;
     vstip = env->vstime_irq ? MIP_VSTIP : 0;
 
-    if (!qemu_mutex_iothread_locked()) {
-        locked = true;
-        qemu_mutex_lock_iothread();
-    }
+    WITH_QEMU_IOTHREAD_LOCK() {
+        env->mip = (env->mip & ~mask) | (value & mask);
 
-    env->mip = (env->mip & ~mask) | (value & mask);
-
-    if (env->mip | vsgein | vstip) {
-        cpu_interrupt(cs, CPU_INTERRUPT_HARD);
-    } else {
-        cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
-    }
-
-    if (locked) {
-        qemu_mutex_unlock_iothread();
+        if (env->mip | vsgein | vstip) {
+            cpu_interrupt(cs, CPU_INTERRUPT_HARD);
+        } else {
+            cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
+        }
     }
 
     return old;
-- 
2.34.1

[PATCH v2 7/7] accel/tcg: Use WITH_QEMU_IOTHREAD_LOCK in io_readx/io_writex

[Richard Henderson]

Narrow the scope of the lock to the actual read/write,
moving the cpu_transation_failed call outside the lock.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cputlb.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 6f1c00682b..8b86b70c60 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1356,7 +1356,6 @@ static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
     MemoryRegionSection *section;
     MemoryRegion *mr;
     uint64_t val;
-    bool locked = false;
     MemTxResult r;
 
     section = iotlb_to_section(cpu, full->xlat_section, full->attrs);
@@ -1367,11 +1366,10 @@ static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
         cpu_io_recompile(cpu, retaddr);
     }
 
-    if (!qemu_mutex_iothread_locked()) {
-        qemu_mutex_lock_iothread();
-        locked = true;
+    WITH_QEMU_IOTHREAD_LOCK() {
+        r = memory_region_dispatch_read(mr, mr_offset, &val, op, full->attrs);
     }
-    r = memory_region_dispatch_read(mr, mr_offset, &val, op, full->attrs);
+
     if (r != MEMTX_OK) {
         hwaddr physaddr = mr_offset +
             section->offset_within_address_space -
@@ -1380,10 +1378,6 @@ static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
         cpu_transaction_failed(cpu, physaddr, addr, memop_size(op), access_type,
                                mmu_idx, full->attrs, r, retaddr);
     }
-    if (locked) {
-        qemu_mutex_unlock_iothread();
-    }
-
     return val;
 }
 
@@ -1410,7 +1404,6 @@ static void io_writex(CPUArchState *env, CPUTLBEntryFull *full,
     hwaddr mr_offset;
     MemoryRegionSection *section;
     MemoryRegion *mr;
-    bool locked = false;
     MemTxResult r;
 
     section = iotlb_to_section(cpu, full->xlat_section, full->attrs);
@@ -1427,11 +1420,10 @@ static void io_writex(CPUArchState *env, CPUTLBEntryFull *full,
      */
     save_iotlb_data(cpu, section, mr_offset);
 
-    if (!qemu_mutex_iothread_locked()) {
-        qemu_mutex_lock_iothread();
-        locked = true;
+    WITH_QEMU_IOTHREAD_LOCK() {
+        r = memory_region_dispatch_write(mr, mr_offset, val, op, full->attrs);
     }
-    r = memory_region_dispatch_write(mr, mr_offset, val, op, full->attrs);
+
     if (r != MEMTX_OK) {
         hwaddr physaddr = mr_offset +
             section->offset_within_address_space -
@@ -1441,9 +1433,6 @@ static void io_writex(CPUArchState *env, CPUTLBEntryFull *full,
                                MMU_DATA_STORE, mmu_idx, full->attrs, r,
                                retaddr);
     }
-    if (locked) {
-        qemu_mutex_unlock_iothread();
-    }
 }
 
 static inline target_ulong tlb_read_ofs(CPUTLBEntry *entry, size_t ofs)
-- 
2.34.1