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-ayl...@ilande.co.uk>
> Signed-off-by: Alex Bennée <alex.ben...@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
> }
>
