From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Gibson Subject: Re: [PATCH 35/35] exec: push BQL down to cpu->cpu_exec_interrupt Date: Tue, 18 Sep 2018 14:12:19 +1000 Message-ID: <20180918041219.GF30868@umbus.fritz.box> References: <20180917163103.6113-1-cota@braap.org> <20180917163103.6113-36-cota@braap.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="yH1ZJFh+qWm+VodA" Cc: Peter Maydell , Cornelia Huck , kvm@vger.kernel.org, David Hildenbrand , James Hogan , Anthony Green , Mark Cave-Ayland , qemu-devel@nongnu.org, "Edgar E. Iglesias" , Guan Xuetao , Marek Vasut , Alexander Graf , Christian Borntraeger , Artyom Tarasenko , Eduardo Habkost , qemu-s390x@nongnu.org, qemu-arm@nongnu.org, Stafford Horne , Richard Henderson , Chris Wulff , Peter Crosthwaite , Marcelo Tosatti , Laurent Vivier , Michael Walle , qemu-ppc@nongnu.org, Al To: "Emilio G. Cota" Return-path: Content-Disposition: inline In-Reply-To: <20180917163103.6113-36-cota@braap.org> 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 --yH1ZJFh+qWm+VodA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 17, 2018 at 12:31:03PM -0400, Emilio G. Cota wrote: > From: Paolo Bonzini >=20 > Most interrupt requests do not need to take the BQL, and in fact > most architectures do not need it at all. Push the BQL acquisition > down to target code. >=20 > 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 ppc parts Acked-by: David Gibson > --- > docs/devel/multi-thread-tcg.txt | 7 +++++-- > accel/tcg/cpu-exec.c | 9 +-------- > target/arm/cpu.c | 15 ++++++++++++++- > target/i386/seg_helper.c | 3 +++ > target/ppc/excp_helper.c | 2 ++ > target/s390x/excp_helper.c | 3 +++ > 6 files changed, 28 insertions(+), 11 deletions(-) >=20 > diff --git a/docs/devel/multi-thread-tcg.txt b/docs/devel/multi-thread-tc= g.txt > index 782bebc28b..422de4736b 100644 > --- a/docs/devel/multi-thread-tcg.txt > +++ b/docs/devel/multi-thread-tcg.txt > @@ -231,8 +231,11 @@ BQL. Currently ARM targets serialise all ARM_CP_IO r= egister accesses > and also defer the reset/startup of vCPUs to the vCPU context by way > of async_run_on_cpu(). > =20 > -Updates to interrupt state are also protected by the BQL as they can > -often be cross vCPU. > +The CPUClass callbacks cpu_exec_interrupt and do_interrupt are invoked > +without BQL protection. Accesses to the interrupt controller from > +the vCPU thread, for example while processing CPU_INTERRUPT_HARD, must > +either call qemu_mutex_lock_iothread/qemu_mutex_unlock_iothread or use > +a separate mutex. > =20 > Memory Consistency > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > index b649e3d772..f5e08e79d1 100644 > --- a/accel/tcg/cpu-exec.c > +++ b/accel/tcg/cpu-exec.c > @@ -524,7 +524,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > =20 > if (unlikely(atomic_read(&cpu->interrupt_request))) { > int interrupt_request; > - qemu_mutex_lock_iothread(); > + > interrupt_request =3D atomic_read(&cpu->interrupt_request); > if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) { > /* Mask out external interrupts for this step. */ > @@ -533,7 +533,6 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > if (interrupt_request & CPU_INTERRUPT_DEBUG) { > cpu_reset_interrupt(cpu, CPU_INTERRUPT_DEBUG); > cpu->exception_index =3D EXCP_DEBUG; > - qemu_mutex_unlock_iothread(); > return true; > } > if (replay_mode =3D=3D REPLAY_MODE_PLAY && !replay_has_interrupt= ()) { > @@ -543,7 +542,6 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT); > cpu->halted =3D 1; > cpu->exception_index =3D EXCP_HLT; > - qemu_mutex_unlock_iothread(); > return true; > } > #if defined(TARGET_I386) > @@ -554,14 +552,12 @@ static inline bool cpu_handle_interrupt(CPUState *c= pu, > cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0); > do_cpu_init(x86_cpu); > cpu->exception_index =3D EXCP_HALTED; > - qemu_mutex_unlock_iothread(); > return true; > } > #else > else if (interrupt_request & CPU_INTERRUPT_RESET) { > replay_interrupt(); > cpu_reset(cpu); > - qemu_mutex_unlock_iothread(); > return true; > } > #endif > @@ -585,9 +581,6 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > the program flow was changed */ > *last_tb =3D NULL; > } > - > - /* If we exit via cpu_loop_exit/longjmp it is reset in cpu_exec = */ > - qemu_mutex_unlock_iothread(); > } > =20 > /* Finally, check if we need to exit to the main loop. */ > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index e2c492efdf..246ea13d8f 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -347,7 +347,8 @@ static void arm_cpu_reset(CPUState *s) > hw_watchpoint_update_all(cpu); > } > =20 > -bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request) > +/* call with the BQL held */ > +static bool arm_cpu_exec_interrupt_locked(CPUState *cs, int interrupt_re= quest) > { > CPUClass *cc =3D CPU_GET_CLASS(cs); > CPUARMState *env =3D cs->env_ptr; > @@ -401,6 +402,16 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interr= upt_request) > return ret; > } > =20 > +bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request) > +{ > + bool ret; > + > + qemu_mutex_lock_iothread(); > + ret =3D arm_cpu_exec_interrupt_locked(cs, interrupt_request); > + qemu_mutex_unlock_iothread(); > + return ret; > +} > + > #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64) > static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_reque= st) > { > @@ -409,6 +420,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, = int interrupt_request) > CPUARMState *env =3D &cpu->env; > bool ret =3D false; > =20 > + qemu_mutex_lock_iothread(); > /* ARMv7-M interrupt masking works differently than -A or -R. > * There is no FIQ/IRQ distinction. Instead of I and F bits > * masking FIQ and IRQ interrupts, an exception is taken only > @@ -422,6 +434,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, = int interrupt_request) > cc->do_interrupt(cs); > ret =3D true; > } > + qemu_mutex_unlock_iothread(); > return ret; > } > #endif > diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c > index 0dd85329db..2fdfbd3c37 100644 > --- a/target/i386/seg_helper.c > +++ b/target/i386/seg_helper.c > @@ -19,6 +19,7 @@ > */ > =20 > #include "qemu/osdep.h" > +#include "qemu/main-loop.h" > #include "cpu.h" > #include "qemu/log.h" > #include "exec/helper-proto.h" > @@ -1324,7 +1325,9 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int inter= rupt_request) > #if !defined(CONFIG_USER_ONLY) > if (interrupt_request & CPU_INTERRUPT_POLL) { > cpu_reset_interrupt(cs, CPU_INTERRUPT_POLL); > + qemu_mutex_lock_iothread(); > apic_poll_irq(cpu->apic_state); > + qemu_mutex_unlock_iothread(); > /* Don't process multiple interrupt requests in a single call. > This is required to make icount-driven execution deterministi= c. */ > return true; > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 8b2cc48cad..57acba2a80 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -885,10 +885,12 @@ bool ppc_cpu_exec_interrupt(CPUState *cs, int inter= rupt_request) > CPUPPCState *env =3D &cpu->env; > =20 > if (interrupt_request & CPU_INTERRUPT_HARD) { > + qemu_mutex_lock_iothread(); > ppc_hw_interrupt(env); > if (env->pending_interrupts =3D=3D 0) { > cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD); > } > + qemu_mutex_unlock_iothread(); > return true; > } > return false; > diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c > index 931c0103c8..f2a93abf01 100644 > --- a/target/s390x/excp_helper.c > +++ b/target/s390x/excp_helper.c > @@ -480,10 +480,13 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int inte= rrupt_request) > the parent EXECUTE insn. */ > return false; > } > + qemu_mutex_lock_iothread(); > if (s390_cpu_has_int(cpu)) { > s390_cpu_do_interrupt(cs); > + qemu_mutex_unlock_iothread(); > return true; > } > + qemu_mutex_unlock_iothread(); > if (env->psw.mask & PSW_MASK_WAIT) { > /* Woken up because of a floating interrupt but it has alrea= dy > * been delivered. Go back to sleep. */ --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --yH1ZJFh+qWm+VodA Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEdfRlhq5hpmzETofcbDjKyiDZs5IFAlugeyMACgkQbDjKyiDZ s5K4aA/+JPWJUKxpJYrtTEhIrJr9YRun/hjLvUDw3GzwfhCc+2QJKkJIbjkU6yV4 /B3/gbsz3PppoM1OzF0r3KJLgqU2T7PZVSAl+8SA3uza4pCq/gW1oi7t3+HjCuKc z8pWQyIgTo7BpC5lijrEtGKLi902Ida4yzZGHTTJ/hK2rb/7k8R9sJEO1PlQOgJy DUBeQ8DdzTLjVzbSAc2t7KxtLHjgW9+yZE91VohR07D4e9ruOfzLupn+lGa0ijTd jOvqPkR4j8qU855Z9nBgfFBL7s4/GhpkSyLL5WCuxdK3rytBmy/nVQBMff+ji71d bCez9jtB1OdIokiT+ZPDNvfM5R6k9juAEEB3+Va3sN1EYlwsKrAbxx1ywlwp8uvK mQw5ZNk2cQEKE7/F4M83cQl9u9OiFoartDcz1QvGtHfZGF3p6DjD/Mq0BqHZQFC/ Unr0lsN49QSJ1k5dCWdBCMgQcw4NPMKolcC80KB0rR996Z+wrbYGuPTB2tEqRxzl sNYzL+iAJIPx4pIOulldNL7mIIHAsN/JOAGyQ/PIth/HH+xGsm6HtgJtRA2HHYCR J/p5H1wntKQUghwpsuB2QzIA/Ay7GwRmgvS1d0+G3bHpkr6hBpUaS72QKQ7O+njW TGEesLqjoJPcT2oky601s0zGM319i5DgzBtjn4qUNfvGXdFmxtA= =mssK -----END PGP SIGNATURE----- --yH1ZJFh+qWm+VodA--