[v2,06/11] sparc/sparc64: grab BQL before calling cpu_check_irqs

Message ID 20170302195337.31558-7-alex.bennee@linaro.org
State New
Headers show
Series
  • MTTCG fixups for 2.9
Related show

Commit Message

Alex Bennée March 2, 2017, 7:53 p.m.
IRQ modification is part of device emulation and should be done while
the BQL is held to prevent races when MTTCG is enabled. This adds
assertions in the hw emulation layer and wraps the calls from helpers
in the BQL.

Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
 hw/sparc/sun4m.c            |  3 +++
 hw/sparc64/sparc64.c        |  3 +++
 target/sparc/int64_helper.c |  3 +++
 target/sparc/win_helper.c   | 11 +++++++++++
 4 files changed, 20 insertions(+)

-- 
2.11.0

Comments

Paolo Bonzini March 3, 2017, 11:47 a.m. | #1
On 02/03/2017 20:53, Alex Bennée wrote:
> IRQ modification is part of device emulation and should be done while

> the BQL is held to prevent races when MTTCG is enabled. This adds

> assertions in the hw emulation layer and wraps the calls from helpers

> in the BQL.

> 

> Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  hw/sparc/sun4m.c            |  3 +++

>  hw/sparc64/sparc64.c        |  3 +++

>  target/sparc/int64_helper.c |  3 +++

>  target/sparc/win_helper.c   | 11 +++++++++++

>  4 files changed, 20 insertions(+)

> 

> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c

> index 61416a6426..873cd7df9a 100644

> --- a/hw/sparc/sun4m.c

> +++ b/hw/sparc/sun4m.c

> @@ -142,6 +142,9 @@ void cpu_check_irqs(CPUSPARCState *env)

>  {

>      CPUState *cs;

>  

> +    /* We should be holding the BQL before we mess with IRQs */

> +    g_assert(qemu_mutex_iothread_locked());

> +

>      if (env->pil_in && (env->interrupt_index == 0 ||

>                          (env->interrupt_index & ~15) == TT_EXTINT)) {

>          unsigned int i;

> diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c

> index b3d219c769..4e4fdab065 100644

> --- a/hw/sparc64/sparc64.c

> +++ b/hw/sparc64/sparc64.c

> @@ -55,6 +55,9 @@ void cpu_check_irqs(CPUSPARCState *env)

>      uint32_t pil = env->pil_in |

>                    (env->softint & ~(SOFTINT_TIMER | SOFTINT_STIMER));

>  

> +    /* We should be holding the BQL before we mess with IRQs */

> +    g_assert(qemu_mutex_iothread_locked());

> +

>      /* TT_IVEC has a higher priority (16) than TT_EXTINT (31..17) */

>      if (env->ivec_status & 0x20) {

>          return;

> diff --git a/target/sparc/int64_helper.c b/target/sparc/int64_helper.c

> index 605747c93c..f942973c22 100644

> --- a/target/sparc/int64_helper.c

> +++ b/target/sparc/int64_helper.c

> @@ -18,6 +18,7 @@

>   */

>  

>  #include "qemu/osdep.h"

> +#include "qemu/main-loop.h"

>  #include "cpu.h"

>  #include "exec/helper-proto.h"

>  #include "exec/log.h"

> @@ -208,7 +209,9 @@ static bool do_modify_softint(CPUSPARCState *env, uint32_t value)

>          env->softint = value;

>  #if !defined(CONFIG_USER_ONLY)

>          if (cpu_interrupts_enabled(env)) {

> +            qemu_mutex_lock_iothread();

>              cpu_check_irqs(env);

> +            qemu_mutex_unlock_iothread();

>          }

>  #endif

>          return true;

> diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c

> index 71b3dd37e8..b387b026d8 100644

> --- a/target/sparc/win_helper.c

> +++ b/target/sparc/win_helper.c

> @@ -18,6 +18,7 @@

>   */

>  

>  #include "qemu/osdep.h"

> +#include "qemu/main-loop.h"

>  #include "cpu.h"

>  #include "exec/exec-all.h"

>  #include "exec/helper-proto.h"

> @@ -86,7 +87,9 @@ void cpu_put_psr(CPUSPARCState *env, target_ulong val)

>  {

>      cpu_put_psr_raw(env, val);

>  #if ((!defined(TARGET_SPARC64)) && !defined(CONFIG_USER_ONLY))

> +    qemu_mutex_lock_iothread();

>      cpu_check_irqs(env);

> +    qemu_mutex_unlock_iothread();

>  #endif

>  }


This can be called from gdbstub, so you need to put the lock/unlock
around helper_wrpsr's call to cpu_put_psr instead.  Also please add a
comment /* Called with BQL held.  */ around cpu_put_psr.

Paolo

> @@ -368,7 +371,9 @@ void helper_wrpstate(CPUSPARCState *env, target_ulong new_state)

>  

>  #if !defined(CONFIG_USER_ONLY)

>      if (cpu_interrupts_enabled(env)) {

> +        qemu_mutex_lock_iothread();

>          cpu_check_irqs(env);

> +        qemu_mutex_unlock_iothread();

>      }

>  #endif

>  }

> @@ -381,7 +386,9 @@ void helper_wrpil(CPUSPARCState *env, target_ulong new_pil)

>      env->psrpil = new_pil;

>  

>      if (cpu_interrupts_enabled(env)) {

> +        qemu_mutex_lock_iothread();

>          cpu_check_irqs(env);

> +        qemu_mutex_unlock_iothread();

>      }

>  #endif

>  }

> @@ -408,7 +415,9 @@ void helper_done(CPUSPARCState *env)

>  

>  #if !defined(CONFIG_USER_ONLY)

>      if (cpu_interrupts_enabled(env)) {

> +        qemu_mutex_lock_iothread();

>          cpu_check_irqs(env);

> +        qemu_mutex_unlock_iothread();

>      }

>  #endif

>  }

> @@ -435,7 +444,9 @@ void helper_retry(CPUSPARCState *env)

>  

>  #if !defined(CONFIG_USER_ONLY)

>      if (cpu_interrupts_enabled(env)) {

> +        qemu_mutex_lock_iothread();

>          cpu_check_irqs(env);

> +        qemu_mutex_unlock_iothread();

>      }

>  #endif

>  }

>
Alex Bennée March 6, 2017, 10:28 a.m. | #2
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 02/03/2017 20:53, Alex Bennée wrote:

>> IRQ modification is part of device emulation and should be done while

>> the BQL is held to prevent races when MTTCG is enabled. This adds

>> assertions in the hw emulation layer and wraps the calls from helpers

>> in the BQL.

>>

>> Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> ---

>>  hw/sparc/sun4m.c            |  3 +++

>>  hw/sparc64/sparc64.c        |  3 +++

>>  target/sparc/int64_helper.c |  3 +++

>>  target/sparc/win_helper.c   | 11 +++++++++++

>>  4 files changed, 20 insertions(+)

>>

>> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c

>> index 61416a6426..873cd7df9a 100644

>> --- a/hw/sparc/sun4m.c

>> +++ b/hw/sparc/sun4m.c

>> @@ -142,6 +142,9 @@ void cpu_check_irqs(CPUSPARCState *env)

>>  {

>>      CPUState *cs;

>>

>> +    /* We should be holding the BQL before we mess with IRQs */

>> +    g_assert(qemu_mutex_iothread_locked());

>> +

>>      if (env->pil_in && (env->interrupt_index == 0 ||

>>                          (env->interrupt_index & ~15) == TT_EXTINT)) {

>>          unsigned int i;

>> diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c

>> index b3d219c769..4e4fdab065 100644

>> --- a/hw/sparc64/sparc64.c

>> +++ b/hw/sparc64/sparc64.c

>> @@ -55,6 +55,9 @@ void cpu_check_irqs(CPUSPARCState *env)

>>      uint32_t pil = env->pil_in |

>>                    (env->softint & ~(SOFTINT_TIMER | SOFTINT_STIMER));

>>

>> +    /* We should be holding the BQL before we mess with IRQs */

>> +    g_assert(qemu_mutex_iothread_locked());

>> +

>>      /* TT_IVEC has a higher priority (16) than TT_EXTINT (31..17) */

>>      if (env->ivec_status & 0x20) {

>>          return;

>> diff --git a/target/sparc/int64_helper.c b/target/sparc/int64_helper.c

>> index 605747c93c..f942973c22 100644

>> --- a/target/sparc/int64_helper.c

>> +++ b/target/sparc/int64_helper.c

>> @@ -18,6 +18,7 @@

>>   */

>>

>>  #include "qemu/osdep.h"

>> +#include "qemu/main-loop.h"

>>  #include "cpu.h"

>>  #include "exec/helper-proto.h"

>>  #include "exec/log.h"

>> @@ -208,7 +209,9 @@ static bool do_modify_softint(CPUSPARCState *env, uint32_t value)

>>          env->softint = value;

>>  #if !defined(CONFIG_USER_ONLY)

>>          if (cpu_interrupts_enabled(env)) {

>> +            qemu_mutex_lock_iothread();

>>              cpu_check_irqs(env);

>> +            qemu_mutex_unlock_iothread();

>>          }

>>  #endif

>>          return true;

>> diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c

>> index 71b3dd37e8..b387b026d8 100644

>> --- a/target/sparc/win_helper.c

>> +++ b/target/sparc/win_helper.c

>> @@ -18,6 +18,7 @@

>>   */

>>

>>  #include "qemu/osdep.h"

>> +#include "qemu/main-loop.h"

>>  #include "cpu.h"

>>  #include "exec/exec-all.h"

>>  #include "exec/helper-proto.h"

>> @@ -86,7 +87,9 @@ void cpu_put_psr(CPUSPARCState *env, target_ulong val)

>>  {

>>      cpu_put_psr_raw(env, val);

>>  #if ((!defined(TARGET_SPARC64)) && !defined(CONFIG_USER_ONLY))

>> +    qemu_mutex_lock_iothread();

>>      cpu_check_irqs(env);

>> +    qemu_mutex_unlock_iothread();

>>  #endif

>>  }

>

> This can be called from gdbstub, so you need to put the lock/unlock

> around helper_wrpsr's call to cpu_put_psr instead.  Also please add a

> comment /* Called with BQL held.  */ around cpu_put_psr.


OK will do. I have to say its hard to see the gdbstub being under the
BQL. Is this a feature of the packet handling being done in the main IO
thread?

>

> Paolo

>

>> @@ -368,7 +371,9 @@ void helper_wrpstate(CPUSPARCState *env, target_ulong new_state)

>>

>>  #if !defined(CONFIG_USER_ONLY)

>>      if (cpu_interrupts_enabled(env)) {

>> +        qemu_mutex_lock_iothread();

>>          cpu_check_irqs(env);

>> +        qemu_mutex_unlock_iothread();

>>      }

>>  #endif

>>  }

>> @@ -381,7 +386,9 @@ void helper_wrpil(CPUSPARCState *env, target_ulong new_pil)

>>      env->psrpil = new_pil;

>>

>>      if (cpu_interrupts_enabled(env)) {

>> +        qemu_mutex_lock_iothread();

>>          cpu_check_irqs(env);

>> +        qemu_mutex_unlock_iothread();

>>      }

>>  #endif

>>  }

>> @@ -408,7 +415,9 @@ void helper_done(CPUSPARCState *env)

>>

>>  #if !defined(CONFIG_USER_ONLY)

>>      if (cpu_interrupts_enabled(env)) {

>> +        qemu_mutex_lock_iothread();

>>          cpu_check_irqs(env);

>> +        qemu_mutex_unlock_iothread();

>>      }

>>  #endif

>>  }

>> @@ -435,7 +444,9 @@ void helper_retry(CPUSPARCState *env)

>>

>>  #if !defined(CONFIG_USER_ONLY)

>>      if (cpu_interrupts_enabled(env)) {

>> +        qemu_mutex_lock_iothread();

>>          cpu_check_irqs(env);

>> +        qemu_mutex_unlock_iothread();

>>      }

>>  #endif

>>  }

>>



--
Alex Bennée
Paolo Bonzini March 6, 2017, 1:22 p.m. | #3
On 06/03/2017 11:28, Alex Bennée wrote:
>> This can be called from gdbstub, so you need to put the lock/unlock

>> around helper_wrpsr's call to cpu_put_psr instead.  Also please add a

>> comment /* Called with BQL held.  */ around cpu_put_psr.

> 

> OK will do. I have to say its hard to see the gdbstub being under the

> BQL. Is this a feature of the packet handling being done in the main IO

> thread?


Yes.

It's probably nigh time to have stronger debug facilities for locks,
including changing our lock policy comments to optional assertions.

I'm even thinking of doing something like C++'s std::unique_lock, for
example

    // Takes a lock and unlocks it when the scope is left.
    // c++: std::lock_guard<std::mutex> sl(some_lock);
    QEMU_SCOPED_MUTEX(QemuMutex, some_lock) sl;


    // The lock is also unlocked correctly when the scope is left.
    // c++: std::unique_lock<std::mutex> sl(some_lock);
    QEMU_SCOPED_MUTEX(QemuMutex, some_lock) sl;
    ...
    for (;;) {
        qemu_scoped_mutex_unlock(&sl);
        if (foo) {
            break;
        }
        ...
        qemu_scoped_mutex_lock(&sl);
    }

    // The lock is taken later.
    // c++: std::unique_lock<std::mutex> sl(some_lock, std::defer_lock);
    QEMU_SCOPED_MUTEX_DEFER(QemuMutex, some_lock) sl;
    ...
    if (x) {
        qemu_scoped_mutex_lock(&sl);
    }

Paolo

Patch hide | download patch | download mbox

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 61416a6426..873cd7df9a 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -142,6 +142,9 @@  void cpu_check_irqs(CPUSPARCState *env)
 {
     CPUState *cs;
 
+    /* We should be holding the BQL before we mess with IRQs */
+    g_assert(qemu_mutex_iothread_locked());
+
     if (env->pil_in && (env->interrupt_index == 0 ||
                         (env->interrupt_index & ~15) == TT_EXTINT)) {
         unsigned int i;
diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
index b3d219c769..4e4fdab065 100644
--- a/hw/sparc64/sparc64.c
+++ b/hw/sparc64/sparc64.c
@@ -55,6 +55,9 @@  void cpu_check_irqs(CPUSPARCState *env)
     uint32_t pil = env->pil_in |
                   (env->softint & ~(SOFTINT_TIMER | SOFTINT_STIMER));
 
+    /* We should be holding the BQL before we mess with IRQs */
+    g_assert(qemu_mutex_iothread_locked());
+
     /* TT_IVEC has a higher priority (16) than TT_EXTINT (31..17) */
     if (env->ivec_status & 0x20) {
         return;
diff --git a/target/sparc/int64_helper.c b/target/sparc/int64_helper.c
index 605747c93c..f942973c22 100644
--- a/target/sparc/int64_helper.c
+++ b/target/sparc/int64_helper.c
@@ -18,6 +18,7 @@ 
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "exec/helper-proto.h"
 #include "exec/log.h"
@@ -208,7 +209,9 @@  static bool do_modify_softint(CPUSPARCState *env, uint32_t value)
         env->softint = value;
 #if !defined(CONFIG_USER_ONLY)
         if (cpu_interrupts_enabled(env)) {
+            qemu_mutex_lock_iothread();
             cpu_check_irqs(env);
+            qemu_mutex_unlock_iothread();
         }
 #endif
         return true;
diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c
index 71b3dd37e8..b387b026d8 100644
--- a/target/sparc/win_helper.c
+++ b/target/sparc/win_helper.c
@@ -18,6 +18,7 @@ 
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "exec/exec-all.h"
 #include "exec/helper-proto.h"
@@ -86,7 +87,9 @@  void cpu_put_psr(CPUSPARCState *env, target_ulong val)
 {
     cpu_put_psr_raw(env, val);
 #if ((!defined(TARGET_SPARC64)) && !defined(CONFIG_USER_ONLY))
+    qemu_mutex_lock_iothread();
     cpu_check_irqs(env);
+    qemu_mutex_unlock_iothread();
 #endif
 }
 
@@ -368,7 +371,9 @@  void helper_wrpstate(CPUSPARCState *env, target_ulong new_state)
 
 #if !defined(CONFIG_USER_ONLY)
     if (cpu_interrupts_enabled(env)) {
+        qemu_mutex_lock_iothread();
         cpu_check_irqs(env);
+        qemu_mutex_unlock_iothread();
     }
 #endif
 }
@@ -381,7 +386,9 @@  void helper_wrpil(CPUSPARCState *env, target_ulong new_pil)
     env->psrpil = new_pil;
 
     if (cpu_interrupts_enabled(env)) {
+        qemu_mutex_lock_iothread();
         cpu_check_irqs(env);
+        qemu_mutex_unlock_iothread();
     }
 #endif
 }
@@ -408,7 +415,9 @@  void helper_done(CPUSPARCState *env)
 
 #if !defined(CONFIG_USER_ONLY)
     if (cpu_interrupts_enabled(env)) {
+        qemu_mutex_lock_iothread();
         cpu_check_irqs(env);
+        qemu_mutex_unlock_iothread();
     }
 #endif
 }
@@ -435,7 +444,9 @@  void helper_retry(CPUSPARCState *env)
 
 #if !defined(CONFIG_USER_ONLY)
     if (cpu_interrupts_enabled(env)) {
+        qemu_mutex_lock_iothread();
         cpu_check_irqs(env);
+        qemu_mutex_unlock_iothread();
     }
 #endif
 }