From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Hildenbrand Subject: Re: [PATCH 34/35] exec: push BQL down to cpu->do_interrupt Date: Tue, 18 Sep 2018 09:12:37 +0200 Message-ID: <52469f18-7acd-da63-002f-e9d4c1d516e6@redhat.com> References: <20180917163103.6113-1-cota@braap.org> <20180917163103.6113-35-cota@braap.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Peter Maydell , Cornelia Huck , kvm@vger.kernel.org, James Hogan , Anthony Green , Mark Cave-Ayland , "Edgar E. Iglesias" , Guan Xuetao , Marek Vasut , Alexander Graf , Christian Borntraeger , Richard Henderson , Artyom Tarasenko , Eduardo Habkost , qemu-s390x@nongnu.org, qemu-arm@nongnu.org, Stafford Horne , David Gibson , Chris Wulff , Peter Crosthwaite , Marcelo Tosatti , Laurent Vivier , Michael Walle , qemu-ppc@nongnu.org, Aleksandar Markov To: "Emilio G. Cota" , qemu-devel@nongnu.org Return-path: In-Reply-To: <20180917163103.6113-35-cota@braap.org> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel2=m.gmane.org@nongnu.org Sender: "Qemu-devel" List-Id: kvm.vger.kernel.org Am 17.09.18 um 18:31 schrieb Emilio G. Cota: > From: Paolo Bonzini > > cpu->do_interrupt can now be called with BQL held (from > cpu->cpu_exec_interrupt) or without (from cpu_handle_exception). > > Only a few targets rely on global device state in cc->do_interrupt; > add checks to those targets to acquire the BQL if not already held. > > Cc: Aleksandar Markovic > Cc: Alexander Graf > Cc: Anthony Green > Cc: Artyom Tarasenko > Cc: Aurelien Jarno > Cc: Christian Borntraeger > Cc: Chris Wulff > Cc: Cornelia Huck > Cc: David Gibson > Cc: David Hildenbrand > Cc: "Edgar E. Iglesias" > Cc: Eduardo Habkost > Cc: Guan Xuetao > Cc: James Hogan > Cc: kvm@vger.kernel.org > Cc: Laurent Vivier > Cc: Marcelo Tosatti > Cc: Marek Vasut > Cc: Mark Cave-Ayland > Cc: Michael Walle > Cc: Peter Crosthwaite > Cc: Peter Maydell > Cc: qemu-arm@nongnu.org > Cc: qemu-ppc@nongnu.org > Cc: qemu-s390x@nongnu.org > Cc: Richard Henderson > Cc: Stafford Horne > Signed-off-by: Paolo Bonzini > Signed-off-by: Emilio G. Cota > --- > accel/tcg/cpu-exec.c | 2 -- > target/arm/helper.c | 28 ++++++++++++++++++++++++++-- > target/ppc/excp_helper.c | 8 +++++++- > target/s390x/excp_helper.c | 14 +++++++++++++- > target/sh4/helper.c | 14 +++++++++++++- > target/xtensa/helper.c | 16 ++++++++++++++-- > 6 files changed, 73 insertions(+), 9 deletions(-) > > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > index 2383763f9b..b649e3d772 100644 > --- a/accel/tcg/cpu-exec.c > +++ b/accel/tcg/cpu-exec.c > @@ -497,9 +497,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret) > #else > if (replay_exception()) { > CPUClass *cc = CPU_GET_CLASS(cpu); > - qemu_mutex_lock_iothread(); > cc->do_interrupt(cpu); > - qemu_mutex_unlock_iothread(); > cpu->exception_index = -1; > } else if (!replay_has_interrupt()) { > /* give a chance to iothread in replay mode */ > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 22dbc42305..548278da14 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -7644,7 +7644,8 @@ gen_invep: > return false; > } > > -void arm_v7m_cpu_do_interrupt(CPUState *cs) > +/* call with the BQL held */ > +static void arm_v7m_cpu_do_interrupt_locked(CPUState *cs) > { > ARMCPU *cpu = ARM_CPU(cs); > CPUARMState *env = &cpu->env; > @@ -7828,6 +7829,17 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs) > v7m_exception_taken(cpu, lr, false, ignore_stackfaults); > } > > +void arm_v7m_cpu_do_interrupt(CPUState *cs) > +{ > + if (qemu_mutex_iothread_locked()) { > + arm_v7m_cpu_do_interrupt_locked(cs); > + } else { > + qemu_mutex_lock_iothread(); > + arm_v7m_cpu_do_interrupt_locked(cs); > + qemu_mutex_unlock_iothread(); > + } > +} > + > /* Function used to synchronize QEMU's AArch64 register set with AArch32 > * register set. This is necessary when switching between AArch32 and AArch64 > * execution state. > @@ -8482,8 +8494,9 @@ static inline bool check_for_semihosting(CPUState *cs) > * Do any appropriate logging, handle PSCI calls, and then hand off > * to the AArch64-entry or AArch32-entry function depending on the > * target exception level's register width. > + * Call with the BQL held. > */ > -void arm_cpu_do_interrupt(CPUState *cs) > +static void arm_cpu_do_interrupt_locked(CPUState *cs) > { > ARMCPU *cpu = ARM_CPU(cs); > CPUARMState *env = &cpu->env; > @@ -8534,6 +8547,17 @@ void arm_cpu_do_interrupt(CPUState *cs) > } > } > > +void arm_cpu_do_interrupt(CPUState *cs) > +{ > + if (qemu_mutex_iothread_locked()) { > + arm_cpu_do_interrupt_locked(cs); > + } else { > + qemu_mutex_lock_iothread(); > + arm_cpu_do_interrupt_locked(cs); > + qemu_mutex_unlock_iothread(); > + } > +} > + > /* Return the exception level which controls this address translation regime */ > static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx) > { > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 70ac10e23b..8b2cc48cad 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -742,7 +742,13 @@ void ppc_cpu_do_interrupt(CPUState *cs) > PowerPCCPU *cpu = POWERPC_CPU(cs); > CPUPPCState *env = &cpu->env; > > - powerpc_excp(cpu, env->excp_model, cs->exception_index); > + if (qemu_mutex_iothread_locked()) { > + powerpc_excp(cpu, env->excp_model, cs->exception_index); > + } else { > + qemu_mutex_lock_iothread(); > + powerpc_excp(cpu, env->excp_model, cs->exception_index); > + qemu_mutex_unlock_iothread(); > + } > } > > static void ppc_hw_interrupt(CPUPPCState *env) > diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c > index f2b92d7cbc..931c0103c8 100644 > --- a/target/s390x/excp_helper.c > +++ b/target/s390x/excp_helper.c > @@ -378,7 +378,8 @@ static void do_mchk_interrupt(CPUS390XState *env) > load_psw(env, mask, addr); > } > > -void s390_cpu_do_interrupt(CPUState *cs) > +/* call with the BQL held */ > +static void s390_cpu_do_interrupt_locked(CPUState *cs) > { > QEMUS390FLICState *flic = QEMU_S390_FLIC(s390_get_flic()); > S390CPU *cpu = S390_CPU(cs); > @@ -457,6 +458,17 @@ try_deliver: > } > } > > +void s390_cpu_do_interrupt(CPUState *cs) > +{ > + if (qemu_mutex_iothread_locked()) { > + s390_cpu_do_interrupt_locked(cs); > + } else { > + qemu_mutex_lock_iothread(); > + s390_cpu_do_interrupt_locked(cs); > + qemu_mutex_unlock_iothread(); > + } > +} > + Yes, due to floating interrupts we need the iothread lock. This change looks sane to me from an s390x perspective: Reviewed-by: David Hildenbrand -- Thanks, David / dhildenb