Re: [Qemu-devel] [PATCH v2 06/11] sparc/sparc64: grab BQL before calling cpu_check_irqs

[Alex Bennée]

Paolo Bonzini <address@hidden> 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 <address@hidden>
>> Signed-off-by: Alex Bennée <address@hidden>
>> ---
>>  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