diff mbox series

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

Message ID 20200805181303.7822-18-robert.foley@linaro.org (mailing list archive)
State New, archived
Headers show
Series accel/tcg: remove implied BQL from cpu_handle_interrupt/exception path | expand

Commit Message

Robert Foley Aug. 5, 2020, 6:12 p.m. UTC
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(-)

Comments

Cornelia Huck Aug. 6, 2020, 8:59 a.m. UTC | #1
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;
>  }
>
Paolo Bonzini Aug. 6, 2020, 9:12 a.m. UTC | #2
On 06/08/20 10:59, Cornelia Huck wrote:
>>      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...
> 

Agreed, except the usual convention would be s390_cpu_do_interrupt_locked.

Paolo
Alex Bennée Aug. 6, 2020, 10:03 a.m. UTC | #3
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 06/08/20 10:59, Cornelia Huck wrote:
>>>      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...
>> 
>
> Agreed, except the usual convention would be
> s390_cpu_do_interrupt_locked.

We should probably document this convention in CODING_STYLE.rst/Naming

>
> Paolo
diff mbox series

Patch

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();
+    }
     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);
+            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;
 }