Re: [PATCH v1 17/21] target/s390x: add BQL to do_interrupt and cpu_exec_interrupt

[Cornelia Huck]

On Wed,  5 Aug 2020 14:12:59 -0400
Robert Foley <robert.foley@linaro.org> wrote:

> This is part of a series of changes to remove the implied BQL
> from the common code of cpu_handle_interrupt and
> cpu_handle_exception.  As part of removing the implied BQL
> from the common code, we are pushing the BQL holding
> down into the per-arch implementation functions of
> do_interrupt and cpu_exec_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 two key posts in the discussion, explaining
> the reasoning/benefits of this approach.
> 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 <robert.foley@linaro.org>
> ---
>  target/s390x/excp_helper.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
> index dde7afc2f0..b215b4a4a7 100644
> --- a/target/s390x/excp_helper.c
> +++ b/target/s390x/excp_helper.c
> @@ -470,7 +470,10 @@ void s390_cpu_do_interrupt(CPUState *cs)
>      S390CPU *cpu = S390_CPU(cs);
>      CPUS390XState *env = &cpu->env;
>      bool stopped = false;
> -
> +    bool bql = !qemu_mutex_iothread_locked();
> +    if (bql) {
> +        qemu_mutex_lock_iothread();
> +    }

I'm not sure I like that conditional locking. Can we instead create
__s390_cpu_do_interrupt() or so, move the meat of this function there,
take the bql unconditionally here, and...

>      qemu_log_mask(CPU_LOG_INT, "%s: %d at psw=%" PRIx64 ":%" PRIx64 "\n",
>                    __func__, cs->exception_index, env->psw.mask, 
> env->psw.addr);
>  
> @@ -541,10 +544,14 @@ try_deliver:
>          /* unhalt if we had a WAIT PSW somehwere in our injection chain */
>          s390_cpu_unhalt(cpu);
>      }
> +    if (bql) {
> +        qemu_mutex_unlock_iothread();
> +    }
>  }
>  
>  bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
> +    qemu_mutex_lock_iothread();
>      if (interrupt_request & CPU_INTERRUPT_HARD) {
>          S390CPU *cpu = S390_CPU(cs);
>          CPUS390XState *env = &cpu->env;
> @@ -552,10 +559,12 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int 
> interrupt_request)
>          if (env->ex_value) {
>              /* Execution of the target insn is indivisible from
>                 the parent EXECUTE insn.  */
> +            qemu_mutex_unlock_iothread();
>              return false;
>          }
>          if (s390_cpu_has_int(cpu)) {
>              s390_cpu_do_interrupt(cs);

...call __s390_cpu_do_interrupt() here?

> +            qemu_mutex_unlock_iothread();
>              return true;
>          }
>          if (env->psw.mask & PSW_MASK_WAIT) {
> @@ -564,6 +573,7 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int 
> interrupt_request)
>              cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HALT);
>          }
>      }
> +    qemu_mutex_unlock_iothread();
>      return false;
>  }
>