| Message ID | 1519324303-5674-4-git-send-email-aleksandar.markovic@rt-rk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Aleksandar Markovic <aleksandar.markovic@rt-rk.com> writes: > From: Aleksandar Markovic <aleksandar.markovic@mips.com> > > Make sure BQL is held for all interrupt requests. > > For MTTCG-enabled configurations, handling soft and hard interrupts > between vCPUs must be properly locked. By acquiring BQL, make sure > all paths triggering an IRQ are synchronized. > > Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com> > Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > --- > hw/mips/mips_int.c | 12 ++++++++++++ > target/mips/op_helper.c | 21 +++------------------ > 2 files changed, 15 insertions(+), 18 deletions(-) > > diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c > index 48192d2..5ddeb15 100644 > --- a/hw/mips/mips_int.c > +++ b/hw/mips/mips_int.c > @@ -21,6 +21,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/main-loop.h" > #include "hw/hw.h" > #include "hw/mips/cpudevs.h" > #include "cpu.h" > @@ -32,10 +33,17 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level) > MIPSCPU *cpu = opaque; > CPUMIPSState *env = &cpu->env; > CPUState *cs = CPU(cpu); > + bool locked = false; > > if (irq < 0 || irq > 7) > return; > > + /* Make sure locking works even if BQL is already held by the caller */ > + if (!qemu_mutex_iothread_locked()) { > + locked = true; > + qemu_mutex_lock_iothread(); > + } > + > if (level) { > env->CP0_Cause |= 1 << (irq + CP0Ca_IP); > > @@ -56,6 +64,10 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level) > } else { > cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD); > } > + > + if (locked) { > + qemu_mutex_unlock_iothread(); > + } > } > > void cpu_mips_irq_init_cpu(MIPSCPU *cpu) > diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c > index 44a9b06..6bd8e59 100644 > --- a/target/mips/op_helper.c > +++ b/target/mips/op_helper.c > @@ -17,7 +17,6 @@ > * License along with this library; if not, see <http://www.gnu.org/licenses/>. > */ > #include "qemu/osdep.h" > -#include "qemu/main-loop.h" > #include "cpu.h" > #include "internal.h" > #include "qemu/host-utils.h" > @@ -809,11 +808,7 @@ target_ulong helper_mftc0_tcschefback(CPUMIPSState *env) > > target_ulong helper_mfc0_count(CPUMIPSState *env) > { > - int32_t count; > - qemu_mutex_lock_iothread(); > - count = (int32_t) cpu_mips_get_count(env); > - qemu_mutex_unlock_iothread(); > - return count; > + return (int32_t)cpu_mips_get_count(env); > } > > target_ulong helper_mftc0_entryhi(CPUMIPSState *env) > @@ -1388,9 +1383,7 @@ void helper_mtc0_hwrena(CPUMIPSState *env, target_ulong arg1) > > void helper_mtc0_count(CPUMIPSState *env, target_ulong arg1) > { > - qemu_mutex_lock_iothread(); > cpu_mips_store_count(env, arg1); > - qemu_mutex_unlock_iothread(); > } > > void helper_mtc0_entryhi(CPUMIPSState *env, target_ulong arg1) > @@ -1439,9 +1432,7 @@ void helper_mttc0_entryhi(CPUMIPSState *env, target_ulong arg1) > > void helper_mtc0_compare(CPUMIPSState *env, target_ulong arg1) > { > - qemu_mutex_lock_iothread(); > cpu_mips_store_compare(env, arg1); > - qemu_mutex_unlock_iothread(); > } > > void helper_mtc0_status(CPUMIPSState *env, target_ulong arg1) > @@ -1495,9 +1486,7 @@ void helper_mtc0_srsctl(CPUMIPSState *env, target_ulong arg1) > > void helper_mtc0_cause(CPUMIPSState *env, target_ulong arg1) > { > - qemu_mutex_lock_iothread(); > cpu_mips_store_cause(env, arg1); > - qemu_mutex_unlock_iothread(); > } > > void helper_mttc0_cause(CPUMIPSState *env, target_ulong arg1) > @@ -2339,16 +2328,12 @@ target_ulong helper_rdhwr_synci_step(CPUMIPSState *env) > > target_ulong helper_rdhwr_cc(CPUMIPSState *env) > { > - int32_t count; > check_hwrena(env, 2, GETPC()); > #ifdef CONFIG_USER_ONLY > - count = env->CP0_Count; > + return env->CP0_Count; > #else > - qemu_mutex_lock_iothread(); > - count = (int32_t)cpu_mips_get_count(env); > - qemu_mutex_unlock_iothread(); > + return (int32_t)cpu_mips_get_count(env); > #endif > - return count; > } > > target_ulong helper_rdhwr_ccres(CPUMIPSState *env) -- Alex Bennée
On 04/04/2018 12:23, Alex Bennée wrote: > >> From: Aleksandar Markovic <aleksandar.markovic@mips.com> >> >> Make sure BQL is held for all interrupt requests. >> >> For MTTCG-enabled configurations, handling soft and hard interrupts >> between vCPUs must be properly locked. By acquiring BQL, make sure >> all paths triggering an IRQ are synchronized. >> >> Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com> >> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com> Is this actually necessary? What paths are not taking the lock? Thanks, Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 04/04/2018 12:23, Alex Bennée wrote: >> >>> From: Aleksandar Markovic <aleksandar.markovic@mips.com> >>> >>> Make sure BQL is held for all interrupt requests. >>> >>> For MTTCG-enabled configurations, handling soft and hard interrupts >>> between vCPUs must be properly locked. By acquiring BQL, make sure >>> all paths triggering an IRQ are synchronized. >>> >>> Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com> >>> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com> > > Is this actually necessary? What paths are not taking the lock? Helpers functions have to manually take the lock. AIUI from this patch the if (locked) dance allows a single function to be used which may trigger an IRQ from both helpers (no automatic locking) and hw emulation (locked by default). > > Thanks, > > Paolo -- Alex Bennée
On 04/04/2018 15:44, Alex Bennée wrote: >>>> Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com> >>>> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com> >> Is this actually necessary? What paths are not taking the lock? > Helpers functions have to manually take the lock. AIUI from this patch > the if (locked) dance allows a single function to be used which may > trigger an IRQ from both helpers (no automatic locking) and hw emulation > (locked by default). This makes it harder to understand which paths actually need to take the lock, and to split the lock in the future. We do it in some cases, but in general defining "*_locked" or "*_unlocked" functions is easier on the brain. Thanks, Paolo
diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c index 48192d2..5ddeb15 100644 --- a/hw/mips/mips_int.c +++ b/hw/mips/mips_int.c @@ -21,6 +21,7 @@ */ #include "qemu/osdep.h" +#include "qemu/main-loop.h" #include "hw/hw.h" #include "hw/mips/cpudevs.h" #include "cpu.h" @@ -32,10 +33,17 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level) MIPSCPU *cpu = opaque; CPUMIPSState *env = &cpu->env; CPUState *cs = CPU(cpu); + bool locked = false; if (irq < 0 || irq > 7) return; + /* Make sure locking works even if BQL is already held by the caller */ + if (!qemu_mutex_iothread_locked()) { + locked = true; + qemu_mutex_lock_iothread(); + } + if (level) { env->CP0_Cause |= 1 << (irq + CP0Ca_IP); @@ -56,6 +64,10 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level) } else { cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD); } + + if (locked) { + qemu_mutex_unlock_iothread(); + } } void cpu_mips_irq_init_cpu(MIPSCPU *cpu) diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c index 44a9b06..6bd8e59 100644 --- a/target/mips/op_helper.c +++ b/target/mips/op_helper.c @@ -17,7 +17,6 @@ * License along with this library; if not, see <http://www.gnu.org/licenses/>. */ #include "qemu/osdep.h" -#include "qemu/main-loop.h" #include "cpu.h" #include "internal.h" #include "qemu/host-utils.h" @@ -809,11 +808,7 @@ target_ulong helper_mftc0_tcschefback(CPUMIPSState *env) target_ulong helper_mfc0_count(CPUMIPSState *env) { - int32_t count; - qemu_mutex_lock_iothread(); - count = (int32_t) cpu_mips_get_count(env); - qemu_mutex_unlock_iothread(); - return count; + return (int32_t)cpu_mips_get_count(env); } target_ulong helper_mftc0_entryhi(CPUMIPSState *env) @@ -1388,9 +1383,7 @@ void helper_mtc0_hwrena(CPUMIPSState *env, target_ulong arg1) void helper_mtc0_count(CPUMIPSState *env, target_ulong arg1) { - qemu_mutex_lock_iothread(); cpu_mips_store_count(env, arg1); - qemu_mutex_unlock_iothread(); } void helper_mtc0_entryhi(CPUMIPSState *env, target_ulong arg1) @@ -1439,9 +1432,7 @@ void helper_mttc0_entryhi(CPUMIPSState *env, target_ulong arg1) void helper_mtc0_compare(CPUMIPSState *env, target_ulong arg1) { - qemu_mutex_lock_iothread(); cpu_mips_store_compare(env, arg1); - qemu_mutex_unlock_iothread(); } void helper_mtc0_status(CPUMIPSState *env, target_ulong arg1) @@ -1495,9 +1486,7 @@ void helper_mtc0_srsctl(CPUMIPSState *env, target_ulong arg1) void helper_mtc0_cause(CPUMIPSState *env, target_ulong arg1) { - qemu_mutex_lock_iothread(); cpu_mips_store_cause(env, arg1); - qemu_mutex_unlock_iothread(); } void helper_mttc0_cause(CPUMIPSState *env, target_ulong arg1) @@ -2339,16 +2328,12 @@ target_ulong helper_rdhwr_synci_step(CPUMIPSState *env) target_ulong helper_rdhwr_cc(CPUMIPSState *env) { - int32_t count; check_hwrena(env, 2, GETPC()); #ifdef CONFIG_USER_ONLY - count = env->CP0_Count; + return env->CP0_Count; #else - qemu_mutex_lock_iothread(); - count = (int32_t)cpu_mips_get_count(env); - qemu_mutex_unlock_iothread(); + return (int32_t)cpu_mips_get_count(env); #endif - return count; } target_ulong helper_rdhwr_ccres(CPUMIPSState *env)