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

[PATCH for-8.0 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.

Doesn't actually depend on anything, but patchew only handles a single
dependency, so I need to thread the dependency through the patch sets.

Based-on: 20221111074101.2069454-1-richard.henderson@linaro.org
("tcg: Support for Int128 with helpers")


r~


Richard Henderson (7):
  qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD
  hw/mips: Use QEMU_IOTHREAD_LOCK_GUARD 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
  target/riscv: Use QEMU_IOTHREAD_LOCK_GUARD in riscv_cpu_update_mip
  hw/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in ppc_set_irq
  accel/tcg: Use QEMU_IOTHREAD_LOCK_GUARD in io_readx/io_writex

 include/qemu/main-loop.h  | 29 +++++++++++++++++++++++++++++
 accel/tcg/cputlb.c        | 25 ++++++++-----------------
 hw/mips/mips_int.c        | 11 +----------
 hw/ppc/ppc.c              | 10 +---------
 target/ppc/excp_helper.c  | 11 +----------
 target/ppc/helper_regs.c  | 14 ++++----------
 target/riscv/cpu_helper.c | 10 +---------
 7 files changed, 45 insertions(+), 65 deletions(-)

-- 
2.34.1

[PATCH for-8.0 1/7] qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD

[Richard Henderson]

Create a wrapper for locking/unlocking the iothread lock.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop)
---
 include/qemu/main-loop.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 3c9a9a982d..c25f390696 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -343,6 +343,35 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line);
  */
 void qemu_mutex_unlock_iothread(void);
 
+/**
+ * QEMU_IOTHREAD_LOCK_GUARD
+ *
+ * Wrap a block of code in a conditional qemu_mutex_{lock,unlock}_iothread.
+ */
+typedef struct IOThreadLockAuto IOThreadLockAuto;
+
+static inline IOThreadLockAuto *qemu_iothread_auto_lock(const char *file,
+                                                        int line)
+{
+    if (qemu_mutex_iothread_locked()) {
+        return NULL;
+    }
+    qemu_mutex_lock_iothread_impl(file, line);
+    /* Anything non-NULL causes the cleanup function to be called */
+    return (IOThreadLockAuto *)(uintptr_t)1;
+}
+
+static inline void qemu_iothread_auto_unlock(IOThreadLockAuto *l)
+{
+    qemu_mutex_unlock_iothread();
+}
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(IOThreadLockAuto, qemu_iothread_auto_unlock)
+
+#define QEMU_IOTHREAD_LOCK_GUARD() \
+    g_autoptr(IOThreadLockAuto) _iothread_lock_auto __attribute__((unused)) \
+        = qemu_iothread_auto_lock(__FILE__, __LINE__)
+
 /*
  * qemu_cond_wait_iothread: Wait on condition for the main loop mutex
  *
-- 
2.34.1

Re: [PATCH for-8.0 1/7] qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Create a wrapper for locking/unlocking the iothread lock.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop)

You might want to review Paolo's comments from:

  Subject: [RFC PATCH] main-loop: introduce WITH_QEMU_IOTHREAD_LOCK
  Date: Mon, 24 Oct 2022 18:19:09 +0100
  Message-Id: <20221024171909.434818-1-alex.bennee@linaro.org>

So it would be worth having the WITH_QEMU_IOTHREAD_LOCK() and
MAYBE_WITH_QEMU_IOTHREAD_LOCK() helpers for completeness.

And of course the name cleanup.

> ---
>  include/qemu/main-loop.h | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
> index 3c9a9a982d..c25f390696 100644
> --- a/include/qemu/main-loop.h
> +++ b/include/qemu/main-loop.h
> @@ -343,6 +343,35 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line);
>   */
>  void qemu_mutex_unlock_iothread(void);
>  
> +/**
> + * QEMU_IOTHREAD_LOCK_GUARD
> + *
> + * Wrap a block of code in a conditional qemu_mutex_{lock,unlock}_iothread.
> + */
> +typedef struct IOThreadLockAuto IOThreadLockAuto;
> +
> +static inline IOThreadLockAuto *qemu_iothread_auto_lock(const char *file,
> +                                                        int line)
> +{
> +    if (qemu_mutex_iothread_locked()) {
> +        return NULL;
> +    }
> +    qemu_mutex_lock_iothread_impl(file, line);
> +    /* Anything non-NULL causes the cleanup function to be called */
> +    return (IOThreadLockAuto *)(uintptr_t)1;
> +}
> +
> +static inline void qemu_iothread_auto_unlock(IOThreadLockAuto *l)
> +{
> +    qemu_mutex_unlock_iothread();
> +}
> +
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(IOThreadLockAuto, qemu_iothread_auto_unlock)
> +
> +#define QEMU_IOTHREAD_LOCK_GUARD() \
> +    g_autoptr(IOThreadLockAuto) _iothread_lock_auto __attribute__((unused)) \
> +        = qemu_iothread_auto_lock(__FILE__, __LINE__)
> +
>  /*
>   * qemu_cond_wait_iothread: Wait on condition for the main loop mutex
>   *


-- 
Alex Bennée

Re: [PATCH for-8.0 1/7] qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD

[Richard Henderson]

On 11/18/22 05:30, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> Create a wrapper for locking/unlocking the iothread lock.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop)
> 
> You might want to review Paolo's comments from:
> 
>    Subject: [RFC PATCH] main-loop: introduce WITH_QEMU_IOTHREAD_LOCK
>    Date: Mon, 24 Oct 2022 18:19:09 +0100
>    Message-Id: <20221024171909.434818-1-alex.bennee@linaro.org>
> 
> So it would be worth having the WITH_QEMU_IOTHREAD_LOCK() and
> MAYBE_WITH_QEMU_IOTHREAD_LOCK() helpers for completeness.

I don't see that (MAYBE_)WITH_QEMU_IOTHREAD_LOCK is particularly useful in any of the 
cases that I converted.

> And of course the name cleanup.

What name cleanup?


r~

Re: [PATCH for-8.0 1/7] qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 11/18/22 05:30, Alex Bennée wrote:
>> Richard Henderson <richard.henderson@linaro.org> writes:
>> 
>>> Create a wrapper for locking/unlocking the iothread lock.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>> Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop)
>> You might want to review Paolo's comments from:
>>    Subject: [RFC PATCH] main-loop: introduce WITH_QEMU_IOTHREAD_LOCK
>>    Date: Mon, 24 Oct 2022 18:19:09 +0100
>>    Message-Id: <20221024171909.434818-1-alex.bennee@linaro.org>
>> So it would be worth having the WITH_QEMU_IOTHREAD_LOCK() and
>> MAYBE_WITH_QEMU_IOTHREAD_LOCK() helpers for completeness.
>
> I don't see that (MAYBE_)WITH_QEMU_IOTHREAD_LOCK is particularly
> useful in any of the cases that I converted.

Fair enough - as long as they are easy enough to add later. The WITH_
forms do work nicely to wrap a particular area under lock and make
things visually clear vs the LOCK_GUARD which basically holds the lock
to the end of function or exit.

>
>> And of course the name cleanup.
>
> What name cleanup?

  Also lots of bonus points for finally renaming these functions to
  "*_main_thread" rather than "*_iothread" since, confusingly, iothreads
  (plural) are the only ones that do not and cannot take the "iothread
  lock".

>
>
> r~


-- 
Alex Bennée

Re: [PATCH for-8.0 1/7] qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD

[Philippe Mathieu-Daudé]

On 21/11/22 10:55, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> On 11/18/22 05:30, Alex Bennée wrote:
>>> Richard Henderson <richard.henderson@linaro.org> writes:
>>>
>>>> Create a wrapper for locking/unlocking the iothread lock.
>>>>
>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>> ---
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop)
>>> You might want to review Paolo's comments from:
>>>     Subject: [RFC PATCH] main-loop: introduce WITH_QEMU_IOTHREAD_LOCK
>>>     Date: Mon, 24 Oct 2022 18:19:09 +0100
>>>     Message-Id: <20221024171909.434818-1-alex.bennee@linaro.org>
>>> So it would be worth having the WITH_QEMU_IOTHREAD_LOCK() and
>>> MAYBE_WITH_QEMU_IOTHREAD_LOCK() helpers for completeness.
>>
>> I don't see that (MAYBE_)WITH_QEMU_IOTHREAD_LOCK is particularly
>> useful in any of the cases that I converted.
> 
> Fair enough - as long as they are easy enough to add later. The WITH_
> forms do work nicely to wrap a particular area under lock and make
> things visually clear vs the LOCK_GUARD which basically holds the lock
> to the end of function or exit.

I concur for WITH_QEMU_IOTHREAD_LOCK(), it is a no-brainer.

Re: [PATCH for-8.0 1/7] qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Create a wrapper for locking/unlocking the iothread lock.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop)
> ---
>  include/qemu/main-loop.h | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
> index 3c9a9a982d..c25f390696 100644
> --- a/include/qemu/main-loop.h
> +++ b/include/qemu/main-loop.h
> @@ -343,6 +343,35 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line);
>   */
>  void qemu_mutex_unlock_iothread(void);
>  
> +/**
> + * QEMU_IOTHREAD_LOCK_GUARD
> + *
> + * Wrap a block of code in a conditional qemu_mutex_{lock,unlock}_iothread.
> + */
> +typedef struct IOThreadLockAuto IOThreadLockAuto;
> +
> +static inline IOThreadLockAuto *qemu_iothread_auto_lock(const char *file,
> +                                                        int line)
> +{
> +    if (qemu_mutex_iothread_locked()) {
> +        return NULL;
> +    }
> +    qemu_mutex_lock_iothread_impl(file, line);
> +    /* Anything non-NULL causes the cleanup function to be called */
> +    return (IOThreadLockAuto *)(uintptr_t)1;

Oh hang on, what black magic is this. Does the compiler do a NULL check
before calling the cleanup?

> +}
> +
> +static inline void qemu_iothread_auto_unlock(IOThreadLockAuto *l)
> +{
> +    qemu_mutex_unlock_iothread();
> +}
> +
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(IOThreadLockAuto, qemu_iothread_auto_unlock)
> +
> +#define QEMU_IOTHREAD_LOCK_GUARD() \
> +    g_autoptr(IOThreadLockAuto) _iothread_lock_auto __attribute__((unused)) \
> +        = qemu_iothread_auto_lock(__FILE__, __LINE__)
> +
>  /*
>   * qemu_cond_wait_iothread: Wait on condition for the main loop mutex
>   *


-- 
Alex Bennée

Re: [PATCH for-8.0 1/7] qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD

[Richard Henderson]

On 11/18/22 05:38, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> Create a wrapper for locking/unlocking the iothread lock.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop)
>> ---
>>   include/qemu/main-loop.h | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
>> index 3c9a9a982d..c25f390696 100644
>> --- a/include/qemu/main-loop.h
>> +++ b/include/qemu/main-loop.h
>> @@ -343,6 +343,35 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line);
>>    */
>>   void qemu_mutex_unlock_iothread(void);
>>   
>> +/**
>> + * QEMU_IOTHREAD_LOCK_GUARD
>> + *
>> + * Wrap a block of code in a conditional qemu_mutex_{lock,unlock}_iothread.
>> + */
>> +typedef struct IOThreadLockAuto IOThreadLockAuto;
>> +
>> +static inline IOThreadLockAuto *qemu_iothread_auto_lock(const char *file,
>> +                                                        int line)
>> +{
>> +    if (qemu_mutex_iothread_locked()) {
>> +        return NULL;
>> +    }
>> +    qemu_mutex_lock_iothread_impl(file, line);
>> +    /* Anything non-NULL causes the cleanup function to be called */
>> +    return (IOThreadLockAuto *)(uintptr_t)1;
> 
> Oh hang on, what black magic is this. Does the compiler do a NULL check
> before calling the cleanup?

Not the compiler, but...

>> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(IOThreadLockAuto, qemu_iothread_auto_unlock)

... this does.  Follow the macros down and you get

>   static G_GNUC_UNUSED inline void _GLIB_AUTOPTR_CLEAR_FUNC_NAME(TypeName) (TypeName *_ptr)
>     { if (_ptr) (cleanup) ((ParentName *) _ptr); }                                                       

r~

[PATCH for-8.0 2/7] hw/mips: Use QEMU_IOTHREAD_LOCK_GUARD in cpu_mips_irq_request

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
Cc: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 hw/mips/mips_int.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
index 2db5e10fe0..73437cd90f 100644
--- a/hw/mips/mips_int.c
+++ b/hw/mips/mips_int.c
@@ -32,17 +32,12 @@ 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();
-    }
+    QEMU_IOTHREAD_LOCK_GUARD();
 
     if (level) {
         env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
@@ -59,10 +54,6 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
     } else {
         cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
     }
-
-    if (locked) {
-        qemu_mutex_unlock_iothread();
-    }
 }
 
 void cpu_mips_irq_init_cpu(MIPSCPU *cpu)
-- 
2.34.1

Re: [PATCH for-8.0 2/7] hw/mips: Use QEMU_IOTHREAD_LOCK_GUARD in cpu_mips_irq_request

[Philippe Mathieu-Daudé]

On 18/11/22 10:18, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> Cc: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   hw/mips/mips_int.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
> index 2db5e10fe0..73437cd90f 100644
> --- a/hw/mips/mips_int.c
> +++ b/hw/mips/mips_int.c
> @@ -32,17 +32,12 @@ 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();
> -    }
> +    QEMU_IOTHREAD_LOCK_GUARD();
>   
>       if (level) {
>           env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
> @@ -59,10 +54,6 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
>       } else {
>           cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
>       }
> -
> -    if (locked) {
> -        qemu_mutex_unlock_iothread();
> -    }
>   }

I'd rather have a macro similar to WITH_RCU_READ_LOCK_GUARD()
so the locked context is obvious:

   WITH_QEMU_IOTHREAD_LOCK_GUARD() {
       ...
   }

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

[PATCH for-8.0 3/7] target/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in ppc_maybe_interrupt

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Cc: qemu-ppc@nongnu.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

Re: [PATCH for-8.0 3/7] target/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in ppc_maybe_interrupt

[Daniel Henrique Barboza]

On 11/18/22 06:18, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> Cc: qemu-ppc@nongnu.org
> ---


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   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)

Re: [PATCH for-8.0 3/7] target/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in ppc_maybe_interrupt

[Philippe Mathieu-Daudé]

On 18/11/22 10:18, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> Cc: qemu-ppc@nongnu.org
> ---
>   target/ppc/excp_helper.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)

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

[PATCH for-8.0 4/7] target/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in cpu_interrupt_exittb

[Richard Henderson]

In addition, use tcg_enabled instead of !kvm_enabled.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Cc: qemu-ppc@nongnu.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

Re: [PATCH for-8.0 4/7] target/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in cpu_interrupt_exittb

[Daniel Henrique Barboza]

On 11/18/22 06:18, Richard Henderson wrote:
> In addition, use tcg_enabled instead of !kvm_enabled.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Should we strive for this change (tcg_enabled instead of !kvm_enabled)
everywhere when applicable? There's a lot of places in the ppc code where
this can be done.


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


> Cc: qemu-ppc@nongnu.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);
>       }
>   }

Re: [PATCH for-8.0 4/7] target/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in cpu_interrupt_exittb

[Richard Henderson]

On 11/18/22 02:13, Daniel Henrique Barboza wrote:
> 
> 
> On 11/18/22 06:18, Richard Henderson wrote:
>> In addition, use tcg_enabled instead of !kvm_enabled.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
> 
> Should we strive for this change (tcg_enabled instead of !kvm_enabled)
> everywhere when applicable? There's a lot of places in the ppc code where
> this can be done.

I think it's better style, since that's generally what is meant.

It's important when the target supports multiple accelerators.  A test for !kvm begs the 
question of why we aren't also testing for e.g. !hvf.  I've noticed a couple of these in 
the code base.


r~

> 
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> 
>> Cc: qemu-ppc@nongnu.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);
>>       }
>>   }

Re: [PATCH for-8.0 4/7] target/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in cpu_interrupt_exittb

[Philippe Mathieu-Daudé]

On 18/11/22 10:18, Richard Henderson wrote:
> In addition, use tcg_enabled instead of !kvm_enabled.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> Cc: qemu-ppc@nongnu.org
> ---
>   target/ppc/helper_regs.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)

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

[PATCH for-8.0 5/7] target/riscv: Use QEMU_IOTHREAD_LOCK_GUARD in riscv_cpu_update_mip

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Cc: Alistair Francis <alistair.francis@wdc.com>
Cc: Bin Meng <bin.meng@windriver.com>
Cc: qemu-riscv@nongnu.org
---
 target/riscv/cpu_helper.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 278d163803..241d06bab8 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,10 +620,7 @@ 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();
-    }
+    QEMU_IOTHREAD_LOCK_GUARD();
 
     env->mip = (env->mip & ~mask) | (value & mask);
 
@@ -634,10 +630,6 @@ uint64_t riscv_cpu_update_mip(RISCVCPU *cpu, uint64_t mask, uint64_t value)
         cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
     }
 
-    if (locked) {
-        qemu_mutex_unlock_iothread();
-    }
-
     return old;
 }
 
-- 
2.34.1

Re: [PATCH for-8.0 5/7] target/riscv: Use QEMU_IOTHREAD_LOCK_GUARD in riscv_cpu_update_mip

[Alistair Francis]

On Fri, Nov 18, 2022 at 7:22 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Cc: Alistair Francis <alistair.francis@wdc.com>
> Cc: Bin Meng <bin.meng@windriver.com>
> Cc: qemu-riscv@nongnu.org
> ---
>  target/riscv/cpu_helper.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 278d163803..241d06bab8 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,10 +620,7 @@ 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();
> -    }
> +    QEMU_IOTHREAD_LOCK_GUARD();
>
>      env->mip = (env->mip & ~mask) | (value & mask);
>
> @@ -634,10 +630,6 @@ uint64_t riscv_cpu_update_mip(RISCVCPU *cpu, uint64_t mask, uint64_t value)
>          cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
>      }
>
> -    if (locked) {
> -        qemu_mutex_unlock_iothread();
> -    }
> -
>      return old;
>  }
>
> --
> 2.34.1
>
>

Re: [PATCH for-8.0 5/7] target/riscv: Use QEMU_IOTHREAD_LOCK_GUARD in riscv_cpu_update_mip

[Philippe Mathieu-Daudé]

On 18/11/22 10:18, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> Cc: Alistair Francis <alistair.francis@wdc.com>
> Cc: Bin Meng <bin.meng@windriver.com>
> Cc: qemu-riscv@nongnu.org
> ---
>   target/riscv/cpu_helper.c | 10 +---------
>   1 file changed, 1 insertion(+), 9 deletions(-)

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

[PATCH for-8.0 6/7] hw/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in ppc_set_irq

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Cc: qemu-ppc@nongnu.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

Re: [PATCH for-8.0 6/7] hw/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in ppc_set_irq

[Daniel Henrique Barboza]

On 11/18/22 06:18, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> Cc: qemu-ppc@nongnu.org
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   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 */

Re: [PATCH for-8.0 6/7] hw/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in ppc_set_irq

[Philippe Mathieu-Daudé]

On 18/11/22 10:18, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> Cc: qemu-ppc@nongnu.org
> ---
>   hw/ppc/ppc.c | 10 +---------
>   1 file changed, 1 insertion(+), 9 deletions(-)

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

[PATCH for-8.0 7/7] accel/tcg: Use QEMU_IOTHREAD_LOCK_GUARD 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.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cputlb.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 517b599c5f..d177afcad6 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,11 @@ 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;
+    {
+        QEMU_IOTHREAD_LOCK_GUARD();
+        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 +1379,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 +1405,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 +1421,11 @@ 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;
+    {
+        QEMU_IOTHREAD_LOCK_GUARD();
+        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 +1435,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

Re: [PATCH for-8.0 7/7] accel/tcg: Use QEMU_IOTHREAD_LOCK_GUARD in io_readx/io_writex

[Philippe Mathieu-Daudé]

On 18/11/22 10:18, Richard Henderson wrote:
> Narrow the scope of the lock to the actual read/write,
> moving the cpu_transation_failed call outside the lock.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/cputlb.c | 25 ++++++++-----------------
>   1 file changed, 8 insertions(+), 17 deletions(-)

> @@ -1367,11 +1366,11 @@ 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;
> +    {
> +        QEMU_IOTHREAD_LOCK_GUARD();
> +        r = memory_region_dispatch_read(mr, mr_offset, &val, op, full->attrs);
>       }
> -    r = memory_region_dispatch_read(mr, mr_offset, &val, op, full->attrs);
> +

Example of clearer WITH_QEMU_IOTHREAD_LOCK_GUARD() macro
use suggested earlier:

         WITH_QEMU_IOTHREAD_LOCK_GUARD() {
             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 +1379,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;
>   }

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

Re: [PATCH for-8.0 0/7] main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD

[Paolo Bonzini]

On 11/18/22 10:18, Richard Henderson wrote:
> Simplify the usage of qemu_mutex_lock_iothread.
> Split out for ease of review.
> 
> Doesn't actually depend on anything, but patchew only handles a single
> dependency, so I need to thread the dependency through the patch sets.
> 
> Based-on: 20221111074101.2069454-1-richard.henderson@linaro.org
> ("tcg: Support for Int128 with helpers")

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> 
> r~
> 
> 
> Richard Henderson (7):
>    qemu/main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD
>    hw/mips: Use QEMU_IOTHREAD_LOCK_GUARD 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
>    target/riscv: Use QEMU_IOTHREAD_LOCK_GUARD in riscv_cpu_update_mip
>    hw/ppc: Use QEMU_IOTHREAD_LOCK_GUARD in ppc_set_irq
>    accel/tcg: Use QEMU_IOTHREAD_LOCK_GUARD in io_readx/io_writex
> 
>   include/qemu/main-loop.h  | 29 +++++++++++++++++++++++++++++
>   accel/tcg/cputlb.c        | 25 ++++++++-----------------
>   hw/mips/mips_int.c        | 11 +----------
>   hw/ppc/ppc.c              | 10 +---------
>   target/ppc/excp_helper.c  | 11 +----------
>   target/ppc/helper_regs.c  | 14 ++++----------
>   target/riscv/cpu_helper.c | 10 +---------
>   7 files changed, 45 insertions(+), 65 deletions(-)
>