Re: [PATCH v2 4/7] target: Push BQL on ->do_interrupt down into per-arch implementation
On 8/19/20 11:28 AM, Robert Foley wrote:
> avr is another exception. avr, arm and cris all had a similar
> case where their *_cpu_exec_interrupt was calling to
> the CPUClass ->do_interrupt. This causes an issue when we push
> the lock down since ->do_interrupt will try to acquire the BQL, but
> the calling context already has it.
Alpha is in this lest as well, correct?
> diff --git a/target/alpha/cpu.h b/target/alpha/cpu.h
> index 4c6753df34..be29bdd530 100644
> --- a/target/alpha/cpu.h
> +++ b/target/alpha/cpu.h
> @@ -276,7 +276,7 @@ struct AlphaCPU {
> extern const VMStateDescription vmstate_alpha_cpu;
> #endif
>
> -void alpha_cpu_do_interrupt_locked(CPUState *cpu);
> +void alpha_cpu_do_interrupt(CPUState *cpu);
> bool alpha_cpu_exec_interrupt(CPUState *cpu, int int_req);
> void alpha_cpu_dump_state(CPUState *cs, FILE *f, int flags);
> hwaddr alpha_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> diff --git a/target/alpha/helper.c b/target/alpha/helper.c
> index ff9a2a7765..e497dd269e 100644
> --- a/target/alpha/helper.c
> +++ b/target/alpha/helper.c
> @@ -295,7 +295,7 @@ bool alpha_cpu_tlb_fill(CPUState *cs, vaddr addr, int
> size,
> }
> #endif /* USER_ONLY */
>
> -void alpha_cpu_do_interrupt_locked(CPUState *cs)
> +static void alpha_cpu_do_interrupt_locked(CPUState *cs)
> {
> AlphaCPU *cpu = ALPHA_CPU(cs);
> CPUAlphaState *env = >env;
> @@ -407,6 +407,13 @@ void alpha_cpu_do_interrupt_locked(CPUState *cs)
> #endif /* !USER_ONLY */
> }
>
> +void alpha_cpu_do_interrupt(CPUState *cs)
> +{
> +qemu_mutex_lock_iothread();
> +alpha_cpu_do_interrupt_locked(cs);
> +qemu_mutex_unlock_iothread();
> +}
> +
> bool alpha_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> {
> AlphaCPU *cpu = ALPHA_CPU(cs);
This rename should have been done in patch 1, as with all others.
Moreover, this leaves a bug in alpha_cpu_exec_interrupt in that it should be
calling alpha_cpu_do_interrupt_locked.
That seems to be the only instance of this mistake.
r~
[PATCH v2 4/7] target: Push BQL on ->do_interrupt down into per-arch implementation
As part of pushing the BQL down into the per-arch implementation, the first change is to remove the holding of BQL from cpu_handle_exception. Next, we made changes per-arch to re-add a new *_do_interrupt function, which gets the BQL and then calls to *_do_interrupt_locked. We also pointed the per-arch ->do_interrupt at the new *_do_interrupt. This patch is part of a series of transitions to move the BQL down into the do_interrupt per arch functions. This set of transitions is needed to maintain bisectability. It is worth mentioning that arm and cris are slightly different. In a prior patch for these arches, we added a new CPUClass method ->do_interrupt_locked. The only difference for arm and cris is that their new *_do_interrupt functions will be able to utilize this new ->do_interrupt_locked method. avr is another exception. avr, arm and cris all had a similar case where their *_cpu_exec_interrupt was calling to the CPUClass ->do_interrupt. This causes an issue when we push the lock down since ->do_interrupt will try to acquire the BQL, but the calling context already has it. To solve this for arm and cris, we added a new CPUCLass method as explained above. Moreover, it was actually required for these arches since they have more than one possible value of ->do_interrupt. In the case of avr, there is only one possible value of ->do_interrupt, so for that reason we changed the avr_cpu_exec_interrupt to call directly to avr_cpu_do_interrupt_locked rather than call cc->do_interrupt. The purpose of this set of changes is to set the groundwork so that an arch could move towards removing the BQL from the cpu_handle_interrupt/exception paths. This approach was suggested by Paolo Bonzini. For reference, here are key posts in the discussion, explaining the reasoning/benefits of this approach. https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg01517.html https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00784.html https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html Signed-off-by: Robert Foley --- accel/tcg/cpu-exec.c| 2 -- target/alpha/cpu.c | 2 +- target/alpha/cpu.h | 2 +- target/alpha/helper.c | 9 - target/arm/cpu.c| 2 +- target/arm/cpu.h| 2 ++ target/arm/cpu_tcg.c| 2 +- target/arm/helper.c | 8 target/avr/cpu.c| 2 +- target/avr/cpu.h| 2 +- target/avr/helper.c | 16 target/cris/cpu.c | 7 +-- target/cris/cpu.h | 1 + target/cris/helper.c| 8 target/hppa/cpu.c | 2 +- target/hppa/cpu.h | 2 +- target/hppa/int_helper.c| 9 - target/i386/cpu.c | 2 +- target/i386/cpu.h | 2 +- target/i386/seg_helper.c| 9 - target/lm32/cpu.c | 2 +- target/lm32/cpu.h | 2 +- target/lm32/helper.c| 9 - target/m68k/cpu.c | 2 +- target/m68k/cpu.h | 2 +- target/m68k/op_helper.c | 11 +-- target/microblaze/cpu.c | 2 +- target/microblaze/cpu.h | 2 +- target/microblaze/helper.c | 11 +-- target/mips/cpu.c | 2 +- target/mips/helper.c| 9 - target/mips/internal.h | 2 +- target/nios2/cpu.c | 2 +- target/nios2/cpu.h | 1 + target/nios2/helper.c | 9 + target/openrisc/cpu.c | 2 +- target/openrisc/cpu.h | 2 +- target/openrisc/interrupt.c | 9 - target/ppc/cpu.h| 1 + target/ppc/excp_helper.c| 7 +++ target/ppc/translate_init.inc.c | 2 +- target/riscv/cpu.c | 2 +- target/riscv/cpu.h | 2 +- target/riscv/cpu_helper.c | 11 ++- target/rx/cpu.c | 2 +- target/rx/cpu.h | 1 + target/rx/helper.c | 7 +++ target/s390x/cpu.c | 2 +- target/s390x/excp_helper.c | 7 +++ target/s390x/internal.h | 1 + target/sh4/cpu.c| 2 +- target/sh4/cpu.h| 2 +- target/sh4/helper.c | 11 +-- target/sparc/cpu.c | 2 +- target/sparc/cpu.h | 1 + target/sparc/int32_helper.c | 7 +++ target/sparc/int64_helper.c | 7 +++ target/tilegx/cpu.c | 9 - target/unicore32/cpu.c | 2 +- target/unicore32/cpu.h | 1 + target/unicore32/softmmu.c | 7 +++ target/xtensa/cpu.c | 2 +- target/xtensa/cpu.h | 2 +- target/xtensa/exc_helper.c | 11 +-- 64 files changed, 223 insertions(+), 60 deletions(-) diff --git
