Re: [PATCH v1 17/21] target/s390x: add BQL to do_interrupt and cpu_exec_interrupt
Paolo Bonzini 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
--
Alex Bennée
Re: [PATCH v1 17/21] target/s390x: add BQL to do_interrupt and cpu_exec_interrupt
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
Re: [PATCH v1 17/21] target/s390x: add BQL to do_interrupt and cpu_exec_interrupt
On Wed, 5 Aug 2020 14:12:59 -0400
Robert Foley 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
> ---
> 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 = >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 = >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;
> }
>
[PATCH v1 17/21] target/s390x: add BQL to do_interrupt and cpu_exec_interrupt
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
---
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 = >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 = >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;
}
--
2.17.1
