* [PATCH 00/35] exec: drop BQL from interrupt handling
@ 2018-09-17 16:30 Emilio G. Cota
2018-09-17 16:30 ` [PATCH 02/35] target/i386: use cpu_reset_interrupt Emilio G. Cota
` (6 more replies)
0 siblings, 7 replies; 18+ messages in thread
From: Emilio G. Cota @ 2018-09-17 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Cornelia Huck, kvm, David Hildenbrand,
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,
qemu-arm, Stafford Horne, David Gibson, Chris Wulff,
Peter Crosthwaite, Marcelo Tosatti, Laurent Vivier,
Michael Walle, q
This series comes originally from a series of patches that Paolo
sent to me a long time ago. I have kept most of his S-o-b tags,
but I have done the forward port of the patches to the current
QEMU code base, so please blame all possible bugs on me, not him.
The goal of this series is to push the BQL out of interrupt
handling wherever possible. Many targets do not need the BQL
to handle interrupts, so for those targets we get rid of the
BQL in interrupt handling. In TCG mode, this by itself does
not yield measurable performance gains. However, it is a
necessary first step before tackling BQL contention when
managing CPU states (i.e. exiting the exec loop always
involves taking the BQL, which does not scale).
The patches:
- Patch 0 accesses icount_decr.u16 consistently with atomics.
This is important because we use it across threads to signal
that an interrupt is pending.
- 2-28: convert direct callers of cpu->interrupt_request
to use atomic accesses or CPU helper functions that use atomics.
I have broken these up into individual patches to ease review.
Note that in some cases I have added many atomic_reads, most
of which are probably unnecessary. However, I'd leave this up
to the maintainers, since I do not want to break things by
doing a single atomic_read(&cpu->interrupt_request) at the
beginning of a function and then miss subsequent updates to
cpu->interrupt_request.
- 29-31: drop the BQL requirement when handling cpu->interrupt_request
- 32-33: drop now unnecessary BQL acquisitions in i386 and ppc
- 34-35: push down the BQL acquisition to cpu->do_interrupt
and cpu->cpu_exec_interrupt. Here I am Cc'ing everyone so that
we can make sure I am not missing any target that requires
the BQL on either do_interrupt or cpu_exec_interrupt.
This series is checkpatch-clean, and can be fetched from:
https://github.com/cota/qemu/tree/cpu-lock
Note that the series applies on top of patches that are
already on the qemu-devel list and will hopefully soon
be upstream. This series begins after commit
4ece0cbd74 ("cpus: access .qemu_icount_bias with atomic64")
in that tree.
Thanks,
Emilio
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 02/35] target/i386: use cpu_reset_interrupt
2018-09-17 16:30 [PATCH 00/35] exec: drop BQL from interrupt handling Emilio G. Cota
@ 2018-09-17 16:30 ` Emilio G. Cota
2018-09-18 20:47 ` Richard Henderson
2018-09-17 16:30 ` [PATCH 11/35] target/i386: access cpu->interrupt_request with atomics Emilio G. Cota
` (5 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Emilio G. Cota @ 2018-09-17 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, kvm, Richard Henderson
From: Paolo Bonzini <pbonzini@redhat.com>
It will be changed to an atomic operation soon.
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: kvm@vger.kernel.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
target/i386/hax-all.c | 4 ++--
target/i386/hvf/x86hvf.c | 8 ++++----
target/i386/kvm.c | 14 +++++++-------
target/i386/seg_helper.c | 13 ++++++-------
target/i386/svm_helper.c | 2 +-
target/i386/whpx-all.c | 10 +++++-----
6 files changed, 25 insertions(+), 26 deletions(-)
diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
index d2e512856b..ae8b678db0 100644
--- a/target/i386/hax-all.c
+++ b/target/i386/hax-all.c
@@ -433,7 +433,7 @@ static int hax_vcpu_interrupt(CPUArchState *env)
irq = cpu_get_pic_interrupt(env);
if (irq >= 0) {
hax_inject_interrupt(env, irq);
- cpu->interrupt_request &= ~CPU_INTERRUPT_HARD;
+ cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
}
}
@@ -483,7 +483,7 @@ static int hax_vcpu_hax_exec(CPUArchState *env)
cpu->halted = 0;
if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
- cpu->interrupt_request &= ~CPU_INTERRUPT_POLL;
+ cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
apic_poll_irq(x86_cpu->apic_state);
}
diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c
index 6c88939b96..3ac796b885 100644
--- a/target/i386/hvf/x86hvf.c
+++ b/target/i386/hvf/x86hvf.c
@@ -402,7 +402,7 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
if (cpu_state->interrupt_request & CPU_INTERRUPT_NMI) {
if (!(env->hflags2 & HF2_NMI_MASK) && !(info & VMCS_INTR_VALID)) {
- cpu_state->interrupt_request &= ~CPU_INTERRUPT_NMI;
+ cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_NMI);
info = VMCS_INTR_VALID | VMCS_INTR_T_NMI | NMI_VEC;
wvmcs(cpu_state->hvf_fd, VMCS_ENTRY_INTR_INFO, info);
} else {
@@ -414,7 +414,7 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
(cpu_state->interrupt_request & CPU_INTERRUPT_HARD) &&
(EFLAGS(env) & IF_MASK) && !(info & VMCS_INTR_VALID)) {
int line = cpu_get_pic_interrupt(&x86cpu->env);
- cpu_state->interrupt_request &= ~CPU_INTERRUPT_HARD;
+ cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_HARD);
if (line >= 0) {
wvmcs(cpu_state->hvf_fd, VMCS_ENTRY_INTR_INFO, line |
VMCS_INTR_VALID | VMCS_INTR_T_HWINTR);
@@ -440,7 +440,7 @@ int hvf_process_events(CPUState *cpu_state)
}
if (cpu_state->interrupt_request & CPU_INTERRUPT_POLL) {
- cpu_state->interrupt_request &= ~CPU_INTERRUPT_POLL;
+ cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_POLL);
apic_poll_irq(cpu->apic_state);
}
if (((cpu_state->interrupt_request & CPU_INTERRUPT_HARD) &&
@@ -453,7 +453,7 @@ int hvf_process_events(CPUState *cpu_state)
do_cpu_sipi(cpu);
}
if (cpu_state->interrupt_request & CPU_INTERRUPT_TPR) {
- cpu_state->interrupt_request &= ~CPU_INTERRUPT_TPR;
+ cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_TPR);
hvf_cpu_synchronize_state(cpu_state);
apic_handle_tpr_access_report(cpu->apic_state, env->eip,
env->tpr_access_type);
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 0b2a07d3a4..5dd66809b0 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -2709,7 +2709,7 @@ static int kvm_put_vcpu_events(X86CPU *cpu, int level)
*/
events.smi.pending = cs->interrupt_request & CPU_INTERRUPT_SMI;
events.smi.latched_init = cs->interrupt_request & CPU_INTERRUPT_INIT;
- cs->interrupt_request &= ~(CPU_INTERRUPT_INIT | CPU_INTERRUPT_SMI);
+ cpu_reset_interrupt(cs, CPU_INTERRUPT_INIT | CPU_INTERRUPT_SMI);
} else {
/* Keep these in cs->interrupt_request. */
events.smi.pending = 0;
@@ -3005,7 +3005,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
if (cpu->interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
if (cpu->interrupt_request & CPU_INTERRUPT_NMI) {
qemu_mutex_lock_iothread();
- cpu->interrupt_request &= ~CPU_INTERRUPT_NMI;
+ cpu_reset_interrupt(cpu, CPU_INTERRUPT_NMI);
qemu_mutex_unlock_iothread();
DPRINTF("injected NMI\n");
ret = kvm_vcpu_ioctl(cpu, KVM_NMI);
@@ -3016,7 +3016,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
}
if (cpu->interrupt_request & CPU_INTERRUPT_SMI) {
qemu_mutex_lock_iothread();
- cpu->interrupt_request &= ~CPU_INTERRUPT_SMI;
+ cpu_reset_interrupt(cpu, CPU_INTERRUPT_SMI);
qemu_mutex_unlock_iothread();
DPRINTF("injected SMI\n");
ret = kvm_vcpu_ioctl(cpu, KVM_SMI);
@@ -3052,7 +3052,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
(env->eflags & IF_MASK)) {
int irq;
- cpu->interrupt_request &= ~CPU_INTERRUPT_HARD;
+ cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
irq = cpu_get_pic_interrupt(env);
if (irq >= 0) {
struct kvm_interrupt intr;
@@ -3123,7 +3123,7 @@ int kvm_arch_process_async_events(CPUState *cs)
/* We must not raise CPU_INTERRUPT_MCE if it's not supported. */
assert(env->mcg_cap);
- cs->interrupt_request &= ~CPU_INTERRUPT_MCE;
+ cpu_reset_interrupt(cs, CPU_INTERRUPT_MCE);
kvm_cpu_synchronize_state(cs);
@@ -3153,7 +3153,7 @@ int kvm_arch_process_async_events(CPUState *cs)
}
if (cs->interrupt_request & CPU_INTERRUPT_POLL) {
- cs->interrupt_request &= ~CPU_INTERRUPT_POLL;
+ cpu_reset_interrupt(cs, CPU_INTERRUPT_POLL);
apic_poll_irq(cpu->apic_state);
}
if (((cs->interrupt_request & CPU_INTERRUPT_HARD) &&
@@ -3166,7 +3166,7 @@ int kvm_arch_process_async_events(CPUState *cs)
do_cpu_sipi(cpu);
}
if (cs->interrupt_request & CPU_INTERRUPT_TPR) {
- cs->interrupt_request &= ~CPU_INTERRUPT_TPR;
+ cpu_reset_interrupt(cs, CPU_INTERRUPT_TPR);
kvm_cpu_synchronize_state(cs);
apic_handle_tpr_access_report(cpu->apic_state, env->eip,
env->tpr_access_type);
diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
index d1cbc6ebf0..0dd85329db 100644
--- a/target/i386/seg_helper.c
+++ b/target/i386/seg_helper.c
@@ -1323,7 +1323,7 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
#if !defined(CONFIG_USER_ONLY)
if (interrupt_request & CPU_INTERRUPT_POLL) {
- cs->interrupt_request &= ~CPU_INTERRUPT_POLL;
+ cpu_reset_interrupt(cs, CPU_INTERRUPT_POLL);
apic_poll_irq(cpu->apic_state);
/* Don't process multiple interrupt requests in a single call.
This is required to make icount-driven execution deterministic. */
@@ -1337,18 +1337,18 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
if ((interrupt_request & CPU_INTERRUPT_SMI) &&
!(env->hflags & HF_SMM_MASK)) {
cpu_svm_check_intercept_param(env, SVM_EXIT_SMI, 0, 0);
- cs->interrupt_request &= ~CPU_INTERRUPT_SMI;
+ cpu_reset_interrupt(cs, CPU_INTERRUPT_SMI);
do_smm_enter(cpu);
ret = true;
} else if ((interrupt_request & CPU_INTERRUPT_NMI) &&
!(env->hflags2 & HF2_NMI_MASK)) {
cpu_svm_check_intercept_param(env, SVM_EXIT_NMI, 0, 0);
- cs->interrupt_request &= ~CPU_INTERRUPT_NMI;
+ cpu_reset_interrupt(cs, CPU_INTERRUPT_NMI);
env->hflags2 |= HF2_NMI_MASK;
do_interrupt_x86_hardirq(env, EXCP02_NMI, 1);
ret = true;
} else if (interrupt_request & CPU_INTERRUPT_MCE) {
- cs->interrupt_request &= ~CPU_INTERRUPT_MCE;
+ cpu_reset_interrupt(cs, CPU_INTERRUPT_MCE);
do_interrupt_x86_hardirq(env, EXCP12_MCHK, 0);
ret = true;
} else if ((interrupt_request & CPU_INTERRUPT_HARD) &&
@@ -1359,8 +1359,7 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
!(env->hflags & HF_INHIBIT_IRQ_MASK))))) {
int intno;
cpu_svm_check_intercept_param(env, SVM_EXIT_INTR, 0, 0);
- cs->interrupt_request &= ~(CPU_INTERRUPT_HARD |
- CPU_INTERRUPT_VIRQ);
+ cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD | CPU_INTERRUPT_VIRQ);
intno = cpu_get_pic_interrupt(env);
qemu_log_mask(CPU_LOG_TB_IN_ASM,
"Servicing hardware INT=0x%02x\n", intno);
@@ -1380,7 +1379,7 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
qemu_log_mask(CPU_LOG_TB_IN_ASM,
"Servicing virtual hardware INT=0x%02x\n", intno);
do_interrupt_x86_hardirq(env, intno, 1);
- cs->interrupt_request &= ~CPU_INTERRUPT_VIRQ;
+ cpu_reset_interrupt(cs, CPU_INTERRUPT_VIRQ);
ret = true;
#endif
}
diff --git a/target/i386/svm_helper.c b/target/i386/svm_helper.c
index 342ece082f..c532639574 100644
--- a/target/i386/svm_helper.c
+++ b/target/i386/svm_helper.c
@@ -700,7 +700,7 @@ void do_vmexit(CPUX86State *env, uint32_t exit_code, uint64_t exit_info_1)
env->hflags &= ~HF_SVMI_MASK;
env->intercept = 0;
env->intercept_exceptions = 0;
- cs->interrupt_request &= ~CPU_INTERRUPT_VIRQ;
+ cpu_reset_interrupt(cs, CPU_INTERRUPT_VIRQ);
env->tsc_offset = 0;
env->gdt.base = x86_ldq_phys(cs, env->vm_hsave + offsetof(struct vmcb,
diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
index 57e53e1f1f..d9428dc987 100644
--- a/target/i386/whpx-all.c
+++ b/target/i386/whpx-all.c
@@ -728,14 +728,14 @@ static void whpx_vcpu_pre_run(CPUState *cpu)
if (!vcpu->interruption_pending &&
cpu->interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
if (cpu->interrupt_request & CPU_INTERRUPT_NMI) {
- cpu->interrupt_request &= ~CPU_INTERRUPT_NMI;
+ cpu_reset_interrupt(cpu, CPU_INTERRUPT_NMI);
vcpu->interruptable = false;
new_int.InterruptionType = WHvX64PendingNmi;
new_int.InterruptionPending = 1;
new_int.InterruptionVector = 2;
}
if (cpu->interrupt_request & CPU_INTERRUPT_SMI) {
- cpu->interrupt_request &= ~CPU_INTERRUPT_SMI;
+ cpu_reset_interrupt(cpu, CPU_INTERRUPT_SMI);
}
}
@@ -758,7 +758,7 @@ static void whpx_vcpu_pre_run(CPUState *cpu)
vcpu->interruptable && (env->eflags & IF_MASK)) {
assert(!new_int.InterruptionPending);
if (cpu->interrupt_request & CPU_INTERRUPT_HARD) {
- cpu->interrupt_request &= ~CPU_INTERRUPT_HARD;
+ cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
irq = cpu_get_pic_interrupt(env);
if (irq >= 0) {
new_int.InterruptionType = WHvX64PendingInterrupt;
@@ -850,7 +850,7 @@ static void whpx_vcpu_process_async_events(CPUState *cpu)
}
if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
- cpu->interrupt_request &= ~CPU_INTERRUPT_POLL;
+ cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
apic_poll_irq(x86_cpu->apic_state);
}
@@ -868,7 +868,7 @@ static void whpx_vcpu_process_async_events(CPUState *cpu)
}
if (cpu->interrupt_request & CPU_INTERRUPT_TPR) {
- cpu->interrupt_request &= ~CPU_INTERRUPT_TPR;
+ cpu_reset_interrupt(cpu, CPU_INTERRUPT_TPR);
if (!cpu->vcpu_dirty) {
whpx_get_registers(cpu);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 11/35] target/i386: access cpu->interrupt_request with atomics
2018-09-17 16:30 [PATCH 00/35] exec: drop BQL from interrupt handling Emilio G. Cota
2018-09-17 16:30 ` [PATCH 02/35] target/i386: use cpu_reset_interrupt Emilio G. Cota
@ 2018-09-17 16:30 ` Emilio G. Cota
2018-09-18 21:04 ` Richard Henderson
2018-09-17 16:31 ` [PATCH 32/35] target/i386/kvm: do not acquire the BQL to call cpu_reset_interrupt Emilio G. Cota
` (4 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Emilio G. Cota @ 2018-09-17 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, kvm, Richard Henderson
From: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: kvm@vger.kernel.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
target/i386/cpu.c | 7 ++++---
target/i386/helper.c | 4 ++--
target/i386/kvm.c | 44 +++++++++++++++++++++++-----------------
target/i386/svm_helper.c | 4 ++--
4 files changed, 33 insertions(+), 26 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f24295e6e4..f98e6e4318 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5433,15 +5433,16 @@ static bool x86_cpu_has_work(CPUState *cs)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ int interrupt_request = atomic_read(&cs->interrupt_request);
- return ((cs->interrupt_request & (CPU_INTERRUPT_HARD |
+ return ((interrupt_request & (CPU_INTERRUPT_HARD |
CPU_INTERRUPT_POLL)) &&
(env->eflags & IF_MASK)) ||
- (cs->interrupt_request & (CPU_INTERRUPT_NMI |
+ (interrupt_request & (CPU_INTERRUPT_NMI |
CPU_INTERRUPT_INIT |
CPU_INTERRUPT_SIPI |
CPU_INTERRUPT_MCE)) ||
- ((cs->interrupt_request & CPU_INTERRUPT_SMI) &&
+ ((interrupt_request & CPU_INTERRUPT_SMI) &&
!(env->hflags & HF_SMM_MASK));
}
diff --git a/target/i386/helper.c b/target/i386/helper.c
index e695f8ba7a..ee9f24d853 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -1035,12 +1035,12 @@ void do_cpu_init(X86CPU *cpu)
CPUState *cs = CPU(cpu);
CPUX86State *env = &cpu->env;
CPUX86State *save = g_new(CPUX86State, 1);
- int sipi = cs->interrupt_request & CPU_INTERRUPT_SIPI;
+ int sipi = atomic_read(&cs->interrupt_request) & CPU_INTERRUPT_SIPI;
*save = *env;
cpu_reset(cs);
- cs->interrupt_request = sipi;
+ atomic_mb_set(&cs->interrupt_request, sipi);
memcpy(&env->start_init_save, &save->start_init_save,
offsetof(CPUX86State, end_init_save) -
offsetof(CPUX86State, start_init_save));
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 5dd66809b0..e40c8d5461 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -2707,8 +2707,10 @@ static int kvm_put_vcpu_events(X86CPU *cpu, int level)
/* As soon as these are moved to the kernel, remove them
* from cs->interrupt_request.
*/
- events.smi.pending = cs->interrupt_request & CPU_INTERRUPT_SMI;
- events.smi.latched_init = cs->interrupt_request & CPU_INTERRUPT_INIT;
+ uint32_t interrupt_request = atomic_read(&cs->interrupt_request);
+
+ events.smi.pending = interrupt_request & CPU_INTERRUPT_SMI;
+ events.smi.latched_init = interrupt_request & CPU_INTERRUPT_INIT;
cpu_reset_interrupt(cs, CPU_INTERRUPT_INIT | CPU_INTERRUPT_SMI);
} else {
/* Keep these in cs->interrupt_request. */
@@ -2999,11 +3001,12 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
{
X86CPU *x86_cpu = X86_CPU(cpu);
CPUX86State *env = &x86_cpu->env;
+ int interrupt_request = atomic_read(&cpu->interrupt_request);
int ret;
/* Inject NMI */
- if (cpu->interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
- if (cpu->interrupt_request & CPU_INTERRUPT_NMI) {
+ if (interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
+ if (interrupt_request & CPU_INTERRUPT_NMI) {
qemu_mutex_lock_iothread();
cpu_reset_interrupt(cpu, CPU_INTERRUPT_NMI);
qemu_mutex_unlock_iothread();
@@ -3014,7 +3017,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
strerror(-ret));
}
}
- if (cpu->interrupt_request & CPU_INTERRUPT_SMI) {
+ if (atomic_read(&cpu->interrupt_request) & CPU_INTERRUPT_SMI) {
qemu_mutex_lock_iothread();
cpu_reset_interrupt(cpu, CPU_INTERRUPT_SMI);
qemu_mutex_unlock_iothread();
@@ -3035,12 +3038,12 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
* or (for userspace APIC, but it is cheap to combine the checks here)
* pending TPR access reports.
*/
- if (cpu->interrupt_request & (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
- if ((cpu->interrupt_request & CPU_INTERRUPT_INIT) &&
+ if (interrupt_request & (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
+ if ((interrupt_request & CPU_INTERRUPT_INIT) &&
!(env->hflags & HF_SMM_MASK)) {
cpu->exit_request = 1;
}
- if (cpu->interrupt_request & CPU_INTERRUPT_TPR) {
+ if (interrupt_request & CPU_INTERRUPT_TPR) {
cpu->exit_request = 1;
}
}
@@ -3048,11 +3051,12 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
if (!kvm_pic_in_kernel()) {
/* Try to inject an interrupt if the guest can accept it */
if (run->ready_for_interrupt_injection &&
- (cpu->interrupt_request & CPU_INTERRUPT_HARD) &&
+ (interrupt_request & CPU_INTERRUPT_HARD) &&
(env->eflags & IF_MASK)) {
int irq;
cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
+ interrupt_request &= ~CPU_INTERRUPT_HARD;
irq = cpu_get_pic_interrupt(env);
if (irq >= 0) {
struct kvm_interrupt intr;
@@ -3072,7 +3076,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
* interrupt, request an interrupt window exit. This will
* cause a return to userspace as soon as the guest is ready to
* receive interrupts. */
- if ((cpu->interrupt_request & CPU_INTERRUPT_HARD)) {
+ if (interrupt_request & CPU_INTERRUPT_HARD) {
run->request_interrupt_window = 1;
} else {
run->request_interrupt_window = 0;
@@ -3118,8 +3122,9 @@ int kvm_arch_process_async_events(CPUState *cs)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
+ int interrupt_request = atomic_read(&cs->interrupt_request);
- if (cs->interrupt_request & CPU_INTERRUPT_MCE) {
+ if (interrupt_request & CPU_INTERRUPT_MCE) {
/* We must not raise CPU_INTERRUPT_MCE if it's not supported. */
assert(env->mcg_cap);
@@ -3142,7 +3147,7 @@ int kvm_arch_process_async_events(CPUState *cs)
}
}
- if ((cs->interrupt_request & CPU_INTERRUPT_INIT) &&
+ if ((interrupt_request & CPU_INTERRUPT_INIT) &&
!(env->hflags & HF_SMM_MASK)) {
kvm_cpu_synchronize_state(cs);
do_cpu_init(cpu);
@@ -3152,20 +3157,20 @@ int kvm_arch_process_async_events(CPUState *cs)
return 0;
}
- if (cs->interrupt_request & CPU_INTERRUPT_POLL) {
+ if (interrupt_request & CPU_INTERRUPT_POLL) {
cpu_reset_interrupt(cs, CPU_INTERRUPT_POLL);
apic_poll_irq(cpu->apic_state);
}
- if (((cs->interrupt_request & CPU_INTERRUPT_HARD) &&
+ if (((interrupt_request & CPU_INTERRUPT_HARD) &&
(env->eflags & IF_MASK)) ||
- (cs->interrupt_request & CPU_INTERRUPT_NMI)) {
+ (interrupt_request & CPU_INTERRUPT_NMI)) {
cs->halted = 0;
}
- if (cs->interrupt_request & CPU_INTERRUPT_SIPI) {
+ if (interrupt_request & CPU_INTERRUPT_SIPI) {
kvm_cpu_synchronize_state(cs);
do_cpu_sipi(cpu);
}
- if (cs->interrupt_request & CPU_INTERRUPT_TPR) {
+ if (interrupt_request & CPU_INTERRUPT_TPR) {
cpu_reset_interrupt(cs, CPU_INTERRUPT_TPR);
kvm_cpu_synchronize_state(cs);
apic_handle_tpr_access_report(cpu->apic_state, env->eip,
@@ -3179,10 +3184,11 @@ static int kvm_handle_halt(X86CPU *cpu)
{
CPUState *cs = CPU(cpu);
CPUX86State *env = &cpu->env;
+ int interrupt_request = atomic_read(&cs->interrupt_request);
- if (!((cs->interrupt_request & CPU_INTERRUPT_HARD) &&
+ if (!((interrupt_request & CPU_INTERRUPT_HARD) &&
(env->eflags & IF_MASK)) &&
- !(cs->interrupt_request & CPU_INTERRUPT_NMI)) {
+ !(interrupt_request & CPU_INTERRUPT_NMI)) {
cs->halted = 1;
return EXCP_HLT;
}
diff --git a/target/i386/svm_helper.c b/target/i386/svm_helper.c
index c532639574..e18e53c869 100644
--- a/target/i386/svm_helper.c
+++ b/target/i386/svm_helper.c
@@ -316,7 +316,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
if (int_ctl & V_IRQ_MASK) {
CPUState *cs = CPU(x86_env_get_cpu(env));
- cs->interrupt_request |= CPU_INTERRUPT_VIRQ;
+ atomic_or(&cs->interrupt_request, CPU_INTERRUPT_VIRQ);
}
/* maybe we need to inject an event */
@@ -674,7 +674,7 @@ void do_vmexit(CPUX86State *env, uint32_t exit_code, uint64_t exit_info_1)
env->vm_vmcb + offsetof(struct vmcb, control.int_ctl));
int_ctl &= ~(V_TPR_MASK | V_IRQ_MASK);
int_ctl |= env->v_tpr & V_TPR_MASK;
- if (cs->interrupt_request & CPU_INTERRUPT_VIRQ) {
+ if (atomic_read(&cs->interrupt_request) & CPU_INTERRUPT_VIRQ) {
int_ctl |= V_IRQ_MASK;
}
x86_stl_phys(cs,
--
2.17.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 32/35] target/i386/kvm: do not acquire the BQL to call cpu_reset_interrupt
2018-09-17 16:30 [PATCH 00/35] exec: drop BQL from interrupt handling Emilio G. Cota
2018-09-17 16:30 ` [PATCH 02/35] target/i386: use cpu_reset_interrupt Emilio G. Cota
2018-09-17 16:30 ` [PATCH 11/35] target/i386: access cpu->interrupt_request with atomics Emilio G. Cota
@ 2018-09-17 16:31 ` Emilio G. Cota
2018-09-18 21:12 ` Richard Henderson
2018-09-19 21:19 ` Philippe Mathieu-Daudé
2018-09-17 16:31 ` [PATCH 34/35] exec: push BQL down to cpu->do_interrupt Emilio G. Cota
` (3 subsequent siblings)
6 siblings, 2 replies; 18+ messages in thread
From: Emilio G. Cota @ 2018-09-17 16:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, kvm, Richard Henderson
From: Paolo Bonzini <pbonzini@redhat.com>
It's not needed anymore.
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: kvm@vger.kernel.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
target/i386/kvm.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index e40c8d5461..41c4830be8 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -3007,9 +3007,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
/* Inject NMI */
if (interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
if (interrupt_request & CPU_INTERRUPT_NMI) {
- qemu_mutex_lock_iothread();
cpu_reset_interrupt(cpu, CPU_INTERRUPT_NMI);
- qemu_mutex_unlock_iothread();
DPRINTF("injected NMI\n");
ret = kvm_vcpu_ioctl(cpu, KVM_NMI);
if (ret < 0) {
@@ -3018,9 +3016,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
}
}
if (atomic_read(&cpu->interrupt_request) & CPU_INTERRUPT_SMI) {
- qemu_mutex_lock_iothread();
cpu_reset_interrupt(cpu, CPU_INTERRUPT_SMI);
- qemu_mutex_unlock_iothread();
DPRINTF("injected SMI\n");
ret = kvm_vcpu_ioctl(cpu, KVM_SMI);
if (ret < 0) {
--
2.17.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 34/35] exec: push BQL down to cpu->do_interrupt
2018-09-17 16:30 [PATCH 00/35] exec: drop BQL from interrupt handling Emilio G. Cota
` (2 preceding siblings ...)
2018-09-17 16:31 ` [PATCH 32/35] target/i386/kvm: do not acquire the BQL to call cpu_reset_interrupt Emilio G. Cota
@ 2018-09-17 16:31 ` Emilio G. Cota
2018-09-18 4:11 ` David Gibson
` (2 more replies)
2018-09-17 16:31 ` [PATCH 35/35] exec: push BQL down to cpu->cpu_exec_interrupt Emilio G. Cota
` (2 subsequent siblings)
6 siblings, 3 replies; 18+ messages in thread
From: Emilio G. Cota @ 2018-09-17 16:31 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Cornelia Huck, kvm, David Hildenbrand,
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,
qemu-arm, Stafford Horne, David Gibson, Chris Wulff,
Peter Crosthwaite, Marcelo Tosatti, Laurent Vivier,
Michael Walle, q
From: Paolo Bonzini <pbonzini@redhat.com>
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 <amarkovic@wavecomp.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Anthony Green <green@moxielogic.com>
Cc: Artyom Tarasenko <atar4qemu@gmail.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Chris Wulff <crwulff@gmail.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: David Hildenbrand <david@redhat.com>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
Cc: James Hogan <jhogan@kernel.org>
Cc: kvm@vger.kernel.org
Cc: Laurent Vivier <laurent@vivier.eu>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Michael Walle <michael@walle.cc>
Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Cc: qemu-ppc@nongnu.org
Cc: qemu-s390x@nongnu.org
Cc: Richard Henderson <rth@twiddle.net>
Cc: Stafford Horne <shorne@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
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();
+ }
+}
+
bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
{
if (interrupt_request & CPU_INTERRUPT_HARD) {
diff --git a/target/sh4/helper.c b/target/sh4/helper.c
index c699b8c0a1..6c508cd006 100644
--- a/target/sh4/helper.c
+++ b/target/sh4/helper.c
@@ -79,7 +79,8 @@ int cpu_sh4_is_cached(CPUSH4State * env, target_ulong addr)
#define MMU_DADDR_ERROR_READ (-12)
#define MMU_DADDR_ERROR_WRITE (-13)
-void superh_cpu_do_interrupt(CPUState *cs)
+/* call with the BQL held */
+static void superh_cpu_do_interrupt_locked(CPUState *cs)
{
SuperHCPU *cpu = SUPERH_CPU(cs);
CPUSH4State *env = &cpu->env;
@@ -211,6 +212,17 @@ void superh_cpu_do_interrupt(CPUState *cs)
}
}
+void superh_cpu_do_interrupt(CPUState *cs)
+{
+ if (qemu_mutex_iothread_locked()) {
+ superh_cpu_do_interrupt_locked(cs);
+ } else {
+ qemu_mutex_lock_iothread();
+ superh_cpu_do_interrupt_locked(cs);
+ qemu_mutex_unlock_iothread();
+ }
+}
+
static void update_itlb_use(CPUSH4State * env, int itlbnb)
{
uint8_t or_mask = 0, and_mask = (uint8_t) - 1;
diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c
index c9a6132700..ecafecdd3f 100644
--- a/target/xtensa/helper.c
+++ b/target/xtensa/helper.c
@@ -26,6 +26,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
#include "qemu/units.h"
#include "cpu.h"
#include "exec/exec-all.h"
@@ -251,8 +252,8 @@ static void handle_interrupt(CPUXtensaState *env)
}
}
-/* Called from cpu_handle_interrupt with BQL held */
-void xtensa_cpu_do_interrupt(CPUState *cs)
+/* Call with the BQL held */
+static void xtensa_cpu_do_interrupt_locked(CPUState *cs)
{
XtensaCPU *cpu = XTENSA_CPU(cs);
CPUXtensaState *env = &cpu->env;
@@ -305,6 +306,17 @@ void xtensa_cpu_do_interrupt(CPUState *cs)
}
check_interrupts(env);
}
+
+void xtensa_cpu_do_interrupt(CPUState *cs)
+{
+ if (qemu_mutex_iothread_locked()) {
+ xtensa_cpu_do_interrupt_locked(cs);
+ } else {
+ qemu_mutex_lock_iothread();
+ xtensa_cpu_do_interrupt_locked(cs);
+ qemu_mutex_unlock_iothread();
+ }
+}
#else
void xtensa_cpu_do_interrupt(CPUState *cs)
{
--
2.17.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 35/35] exec: push BQL down to cpu->cpu_exec_interrupt
2018-09-17 16:30 [PATCH 00/35] exec: drop BQL from interrupt handling Emilio G. Cota
` (3 preceding siblings ...)
2018-09-17 16:31 ` [PATCH 34/35] exec: push BQL down to cpu->do_interrupt Emilio G. Cota
@ 2018-09-17 16:31 ` Emilio G. Cota
2018-09-18 4:12 ` David Gibson
2018-09-18 7:48 ` David Hildenbrand
2018-09-18 9:51 ` [PATCH 00/35] exec: drop BQL from interrupt handling David Hildenbrand
2018-09-20 20:05 ` Mark Cave-Ayland
6 siblings, 2 replies; 18+ messages in thread
From: Emilio G. Cota @ 2018-09-17 16:31 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Cornelia Huck, kvm, David Hildenbrand,
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,
qemu-arm, Stafford Horne, David Gibson, Chris Wulff,
Peter Crosthwaite, Marcelo Tosatti, Laurent Vivier,
Michael Walle, q
From: Paolo Bonzini <pbonzini@redhat.com>
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.
Cc: Aleksandar Markovic <amarkovic@wavecomp.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Anthony Green <green@moxielogic.com>
Cc: Artyom Tarasenko <atar4qemu@gmail.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Chris Wulff <crwulff@gmail.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: David Hildenbrand <david@redhat.com>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
Cc: James Hogan <jhogan@kernel.org>
Cc: kvm@vger.kernel.org
Cc: Laurent Vivier <laurent@vivier.eu>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Michael Walle <michael@walle.cc>
Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Cc: qemu-ppc@nongnu.org
Cc: qemu-s390x@nongnu.org
Cc: Richard Henderson <rth@twiddle.net>
Cc: Stafford Horne <shorne@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
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(-)
diff --git a/docs/devel/multi-thread-tcg.txt b/docs/devel/multi-thread-tcg.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 register accesses
and also defer the reset/startup of vCPUs to the vCPU context by way
of async_run_on_cpu().
-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.
Memory Consistency
==================
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,
if (unlikely(atomic_read(&cpu->interrupt_request))) {
int interrupt_request;
- qemu_mutex_lock_iothread();
+
interrupt_request = 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 = EXCP_DEBUG;
- qemu_mutex_unlock_iothread();
return true;
}
if (replay_mode == 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 = 1;
cpu->exception_index = EXCP_HLT;
- qemu_mutex_unlock_iothread();
return true;
}
#if defined(TARGET_I386)
@@ -554,14 +552,12 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
do_cpu_init(x86_cpu);
cpu->exception_index = 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 = NULL;
}
-
- /* If we exit via cpu_loop_exit/longjmp it is reset in cpu_exec */
- qemu_mutex_unlock_iothread();
}
/* 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);
}
-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_request)
{
CPUClass *cc = CPU_GET_CLASS(cs);
CPUARMState *env = cs->env_ptr;
@@ -401,6 +402,16 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
return ret;
}
+bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
+{
+ bool ret;
+
+ qemu_mutex_lock_iothread();
+ ret = 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_request)
{
@@ -409,6 +420,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
CPUARMState *env = &cpu->env;
bool ret = false;
+ 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 = 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 @@
*/
#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 interrupt_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 deterministic. */
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 interrupt_request)
CPUPPCState *env = &cpu->env;
if (interrupt_request & CPU_INTERRUPT_HARD) {
+ qemu_mutex_lock_iothread();
ppc_hw_interrupt(env);
if (env->pending_interrupts == 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 interrupt_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 already
* been delivered. Go back to sleep. */
--
2.17.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 34/35] exec: push BQL down to cpu->do_interrupt
2018-09-17 16:31 ` [PATCH 34/35] exec: push BQL down to cpu->do_interrupt Emilio G. Cota
@ 2018-09-18 4:11 ` David Gibson
2018-09-18 7:12 ` David Hildenbrand
2018-09-19 16:38 ` Cornelia Huck
2 siblings, 0 replies; 18+ messages in thread
From: David Gibson @ 2018-09-18 4:11 UTC (permalink / raw)
To: Emilio G. Cota
Cc: Peter Maydell, Cornelia Huck, kvm, David Hildenbrand,
James Hogan, Anthony Green, Mark Cave-Ayland, qemu-devel,
Edgar E. Iglesias, Guan Xuetao, Marek Vasut, Alexander Graf,
Christian Borntraeger, Artyom Tarasenko, Eduardo Habkost,
qemu-s390x, qemu-arm, Stafford Horne, Richard Henderson,
Chris Wulff, Peter Crosthwaite, Marcelo Tosatti, Laurent Vivier,
Michael Walle, qemu-ppc, Al
[-- Attachment #1: Type: text/plain, Size: 9121 bytes --]
On Mon, Sep 17, 2018 at 12:31:02PM -0400, Emilio G. Cota wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> 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 <amarkovic@wavecomp.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Anthony Green <green@moxielogic.com>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Chris Wulff <crwulff@gmail.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
> Cc: James Hogan <jhogan@kernel.org>
> Cc: kvm@vger.kernel.org
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Michael Walle <michael@walle.cc>
> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-ppc@nongnu.org
> Cc: qemu-s390x@nongnu.org
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Stafford Horne <shorne@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
ppc parts
Acked-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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();
> + }
> +}
> +
> bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> {
> if (interrupt_request & CPU_INTERRUPT_HARD) {
> diff --git a/target/sh4/helper.c b/target/sh4/helper.c
> index c699b8c0a1..6c508cd006 100644
> --- a/target/sh4/helper.c
> +++ b/target/sh4/helper.c
> @@ -79,7 +79,8 @@ int cpu_sh4_is_cached(CPUSH4State * env, target_ulong addr)
> #define MMU_DADDR_ERROR_READ (-12)
> #define MMU_DADDR_ERROR_WRITE (-13)
>
> -void superh_cpu_do_interrupt(CPUState *cs)
> +/* call with the BQL held */
> +static void superh_cpu_do_interrupt_locked(CPUState *cs)
> {
> SuperHCPU *cpu = SUPERH_CPU(cs);
> CPUSH4State *env = &cpu->env;
> @@ -211,6 +212,17 @@ void superh_cpu_do_interrupt(CPUState *cs)
> }
> }
>
> +void superh_cpu_do_interrupt(CPUState *cs)
> +{
> + if (qemu_mutex_iothread_locked()) {
> + superh_cpu_do_interrupt_locked(cs);
> + } else {
> + qemu_mutex_lock_iothread();
> + superh_cpu_do_interrupt_locked(cs);
> + qemu_mutex_unlock_iothread();
> + }
> +}
> +
> static void update_itlb_use(CPUSH4State * env, int itlbnb)
> {
> uint8_t or_mask = 0, and_mask = (uint8_t) - 1;
> diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c
> index c9a6132700..ecafecdd3f 100644
> --- a/target/xtensa/helper.c
> +++ b/target/xtensa/helper.c
> @@ -26,6 +26,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
> #include "qemu/units.h"
> #include "cpu.h"
> #include "exec/exec-all.h"
> @@ -251,8 +252,8 @@ static void handle_interrupt(CPUXtensaState *env)
> }
> }
>
> -/* Called from cpu_handle_interrupt with BQL held */
> -void xtensa_cpu_do_interrupt(CPUState *cs)
> +/* Call with the BQL held */
> +static void xtensa_cpu_do_interrupt_locked(CPUState *cs)
> {
> XtensaCPU *cpu = XTENSA_CPU(cs);
> CPUXtensaState *env = &cpu->env;
> @@ -305,6 +306,17 @@ void xtensa_cpu_do_interrupt(CPUState *cs)
> }
> check_interrupts(env);
> }
> +
> +void xtensa_cpu_do_interrupt(CPUState *cs)
> +{
> + if (qemu_mutex_iothread_locked()) {
> + xtensa_cpu_do_interrupt_locked(cs);
> + } else {
> + qemu_mutex_lock_iothread();
> + xtensa_cpu_do_interrupt_locked(cs);
> + qemu_mutex_unlock_iothread();
> + }
> +}
> #else
> void xtensa_cpu_do_interrupt(CPUState *cs)
> {
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 35/35] exec: push BQL down to cpu->cpu_exec_interrupt
2018-09-17 16:31 ` [PATCH 35/35] exec: push BQL down to cpu->cpu_exec_interrupt Emilio G. Cota
@ 2018-09-18 4:12 ` David Gibson
2018-09-18 7:48 ` David Hildenbrand
1 sibling, 0 replies; 18+ messages in thread
From: David Gibson @ 2018-09-18 4:12 UTC (permalink / raw)
To: Emilio G. Cota
Cc: Peter Maydell, Cornelia Huck, kvm, David Hildenbrand,
James Hogan, Anthony Green, Mark Cave-Ayland, qemu-devel,
Edgar E. Iglesias, Guan Xuetao, Marek Vasut, Alexander Graf,
Christian Borntraeger, Artyom Tarasenko, Eduardo Habkost,
qemu-s390x, qemu-arm, Stafford Horne, Richard Henderson,
Chris Wulff, Peter Crosthwaite, Marcelo Tosatti, Laurent Vivier,
Michael Walle, qemu-ppc, Al
[-- Attachment #1: Type: text/plain, Size: 9405 bytes --]
On Mon, Sep 17, 2018 at 12:31:03PM -0400, Emilio G. Cota wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> 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.
>
> Cc: Aleksandar Markovic <amarkovic@wavecomp.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Anthony Green <green@moxielogic.com>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Chris Wulff <crwulff@gmail.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
> Cc: James Hogan <jhogan@kernel.org>
> Cc: kvm@vger.kernel.org
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Michael Walle <michael@walle.cc>
> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-ppc@nongnu.org
> Cc: qemu-s390x@nongnu.org
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Stafford Horne <shorne@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
ppc parts
Acked-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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(-)
>
> diff --git a/docs/devel/multi-thread-tcg.txt b/docs/devel/multi-thread-tcg.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 register accesses
> and also defer the reset/startup of vCPUs to the vCPU context by way
> of async_run_on_cpu().
>
> -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.
>
> Memory Consistency
> ==================
> 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,
>
> if (unlikely(atomic_read(&cpu->interrupt_request))) {
> int interrupt_request;
> - qemu_mutex_lock_iothread();
> +
> interrupt_request = 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 = EXCP_DEBUG;
> - qemu_mutex_unlock_iothread();
> return true;
> }
> if (replay_mode == 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 = 1;
> cpu->exception_index = EXCP_HLT;
> - qemu_mutex_unlock_iothread();
> return true;
> }
> #if defined(TARGET_I386)
> @@ -554,14 +552,12 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
> cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
> do_cpu_init(x86_cpu);
> cpu->exception_index = 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 = NULL;
> }
> -
> - /* If we exit via cpu_loop_exit/longjmp it is reset in cpu_exec */
> - qemu_mutex_unlock_iothread();
> }
>
> /* 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);
> }
>
> -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_request)
> {
> CPUClass *cc = CPU_GET_CLASS(cs);
> CPUARMState *env = cs->env_ptr;
> @@ -401,6 +402,16 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> return ret;
> }
>
> +bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> +{
> + bool ret;
> +
> + qemu_mutex_lock_iothread();
> + ret = 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_request)
> {
> @@ -409,6 +420,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> CPUARMState *env = &cpu->env;
> bool ret = false;
>
> + 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 = 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 @@
> */
>
> #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 interrupt_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 deterministic. */
> 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 interrupt_request)
> CPUPPCState *env = &cpu->env;
>
> if (interrupt_request & CPU_INTERRUPT_HARD) {
> + qemu_mutex_lock_iothread();
> ppc_hw_interrupt(env);
> if (env->pending_interrupts == 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 interrupt_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 already
> * been delivered. Go back to sleep. */
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 34/35] exec: push BQL down to cpu->do_interrupt
2018-09-17 16:31 ` [PATCH 34/35] exec: push BQL down to cpu->do_interrupt Emilio G. Cota
2018-09-18 4:11 ` David Gibson
@ 2018-09-18 7:12 ` David Hildenbrand
2018-09-19 16:38 ` Cornelia Huck
2 siblings, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2018-09-18 7:12 UTC (permalink / raw)
To: Emilio G. Cota, qemu-devel
Cc: Peter Maydell, Cornelia Huck, kvm, 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, qemu-arm,
Stafford Horne, David Gibson, Chris Wulff, Peter Crosthwaite,
Marcelo Tosatti, Laurent Vivier, Michael Walle, qemu-ppc,
Aleksandar
Am 17.09.18 um 18:31 schrieb Emilio G. Cota:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> 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 <amarkovic@wavecomp.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Anthony Green <green@moxielogic.com>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Chris Wulff <crwulff@gmail.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
> Cc: James Hogan <jhogan@kernel.org>
> Cc: kvm@vger.kernel.org
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Michael Walle <michael@walle.cc>
> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-ppc@nongnu.org
> Cc: qemu-s390x@nongnu.org
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Stafford Horne <shorne@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
> 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 <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 35/35] exec: push BQL down to cpu->cpu_exec_interrupt
2018-09-17 16:31 ` [PATCH 35/35] exec: push BQL down to cpu->cpu_exec_interrupt Emilio G. Cota
2018-09-18 4:12 ` David Gibson
@ 2018-09-18 7:48 ` David Hildenbrand
1 sibling, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2018-09-18 7:48 UTC (permalink / raw)
To: Emilio G. Cota, qemu-devel
Cc: Peter Maydell, Cornelia Huck, kvm, 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, qemu-arm,
Stafford Horne, David Gibson, Chris Wulff, Peter Crosthwaite,
Marcelo Tosatti, Laurent Vivier, Michael Walle, qemu-ppc,
Aleksandar
> 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 interrupt_request)
> the parent EXECUTE insn. */
> return false;
> }
> + qemu_mutex_lock_iothread();
> if (s390_cpu_has_int(cpu)) {
> s390_cpu_do_interrupt(cs);
You can directly use s390_cpu_do_interrupt_locked() here.
> + 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 already
> * been delivered. Go back to sleep. */
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 00/35] exec: drop BQL from interrupt handling
2018-09-17 16:30 [PATCH 00/35] exec: drop BQL from interrupt handling Emilio G. Cota
` (4 preceding siblings ...)
2018-09-17 16:31 ` [PATCH 35/35] exec: push BQL down to cpu->cpu_exec_interrupt Emilio G. Cota
@ 2018-09-18 9:51 ` David Hildenbrand
2018-09-20 20:05 ` Mark Cave-Ayland
6 siblings, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2018-09-18 9:51 UTC (permalink / raw)
To: Emilio G. Cota, qemu-devel
Cc: Peter Maydell, Cornelia Huck, kvm, 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, qemu-arm,
Stafford Horne, David Gibson, Chris Wulff, Peter Crosthwaite,
Marcelo Tosatti, Laurent Vivier, Michael Walle, qemu-ppc,
Aleksandar
Am 17.09.18 um 18:30 schrieb Emilio G. Cota:
> This series comes originally from a series of patches that Paolo
> sent to me a long time ago. I have kept most of his S-o-b tags,
> but I have done the forward port of the patches to the current
> QEMU code base, so please blame all possible bugs on me, not him.
>
> The goal of this series is to push the BQL out of interrupt
> handling wherever possible. Many targets do not need the BQL
> to handle interrupts, so for those targets we get rid of the
> BQL in interrupt handling. In TCG mode, this by itself does
> not yield measurable performance gains. However, it is a
> necessary first step before tackling BQL contention when
> managing CPU states (i.e. exiting the exec loop always
> involves taking the BQL, which does not scale).
>
> The patches:
>
> - Patch 0 accesses icount_decr.u16 consistently with atomics.
> This is important because we use it across threads to signal
> that an interrupt is pending.
>
> - 2-28: convert direct callers of cpu->interrupt_request
> to use atomic accesses or CPU helper functions that use atomics.
> I have broken these up into individual patches to ease review.
> Note that in some cases I have added many atomic_reads, most
> of which are probably unnecessary. However, I'd leave this up
> to the maintainers, since I do not want to break things by
> doing a single atomic_read(&cpu->interrupt_request) at the
> beginning of a function and then miss subsequent updates to
> cpu->interrupt_request.
>
> - 29-31: drop the BQL requirement when handling cpu->interrupt_request
>
> - 32-33: drop now unnecessary BQL acquisitions in i386 and ppc
>
> - 34-35: push down the BQL acquisition to cpu->do_interrupt
> and cpu->cpu_exec_interrupt. Here I am Cc'ing everyone so that
> we can make sure I am not missing any target that requires
> the BQL on either do_interrupt or cpu_exec_interrupt.
>
> This series is checkpatch-clean, and can be fetched from:
> https://github.com/cota/qemu/tree/cpu-lock
>
> Note that the series applies on top of patches that are
> already on the qemu-devel list and will hopefully soon
> be upstream. This series begins after commit
> 4ece0cbd74 ("cpus: access .qemu_icount_bias with atomic64")
> in that tree.
>
> Thanks,
>
> Emilio
>
I just did a (very slow) make -j32 of QEMU on my 16 core notebook inside
a MTTCG s390x-softmmu guest with 32 VCPUs. I used the same test when
reworking floating interrupt support to make sure there are no races.
As I got no deadlock, I assume you series does not break anything for s390x.
Thanks!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 02/35] target/i386: use cpu_reset_interrupt
2018-09-17 16:30 ` [PATCH 02/35] target/i386: use cpu_reset_interrupt Emilio G. Cota
@ 2018-09-18 20:47 ` Richard Henderson
0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2018-09-18 20:47 UTC (permalink / raw)
To: Emilio G. Cota, qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, kvm
On 9/17/18 9:30 AM, Emilio G. Cota wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> It will be changed to an atomic operation soon.
>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: kvm@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
> target/i386/hax-all.c | 4 ++--
> target/i386/hvf/x86hvf.c | 8 ++++----
> target/i386/kvm.c | 14 +++++++-------
> target/i386/seg_helper.c | 13 ++++++-------
> target/i386/svm_helper.c | 2 +-
> target/i386/whpx-all.c | 10 +++++-----
> 6 files changed, 25 insertions(+), 26 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 11/35] target/i386: access cpu->interrupt_request with atomics
2018-09-17 16:30 ` [PATCH 11/35] target/i386: access cpu->interrupt_request with atomics Emilio G. Cota
@ 2018-09-18 21:04 ` Richard Henderson
2018-09-19 15:02 ` Emilio G. Cota
0 siblings, 1 reply; 18+ messages in thread
From: Richard Henderson @ 2018-09-18 21:04 UTC (permalink / raw)
To: Emilio G. Cota, qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, kvm
On 9/17/18 9:30 AM, Emilio G. Cota wrote:
> cpu_reset(cs);
> - cs->interrupt_request = sipi;
> + atomic_mb_set(&cs->interrupt_request, sipi);
> memcpy(&env->start_init_save, &save->start_init_save,
Why does this need a memory barrier?
Anyway, I think a bare mechanical conversion would be best
for the first patch and then extra barriers added separately
and with a description of why.
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 32/35] target/i386/kvm: do not acquire the BQL to call cpu_reset_interrupt
2018-09-17 16:31 ` [PATCH 32/35] target/i386/kvm: do not acquire the BQL to call cpu_reset_interrupt Emilio G. Cota
@ 2018-09-18 21:12 ` Richard Henderson
2018-09-19 21:19 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2018-09-18 21:12 UTC (permalink / raw)
To: Emilio G. Cota, qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, kvm
On 9/17/18 9:31 AM, Emilio G. Cota wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> It's not needed anymore.
>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: kvm@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
> target/i386/kvm.c | 4 ----
> 1 file changed, 4 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 11/35] target/i386: access cpu->interrupt_request with atomics
2018-09-18 21:04 ` Richard Henderson
@ 2018-09-19 15:02 ` Emilio G. Cota
0 siblings, 0 replies; 18+ messages in thread
From: Emilio G. Cota @ 2018-09-19 15:02 UTC (permalink / raw)
To: Richard Henderson
Cc: Paolo Bonzini, Marcelo Tosatti, qemu-devel, kvm, Eduardo Habkost
On Tue, Sep 18, 2018 at 14:04:35 -0700, Richard Henderson wrote:
> On 9/17/18 9:30 AM, Emilio G. Cota wrote:
> > cpu_reset(cs);
> > - cs->interrupt_request = sipi;
> > + atomic_mb_set(&cs->interrupt_request, sipi);
> > memcpy(&env->start_init_save, &save->start_init_save,
>
> Why does this need a memory barrier?
>
> Anyway, I think a bare mechanical conversion would be best
> for the first patch and then extra barriers added separately
> and with a description of why.
Almost no corresponding read has a barrier so it's hard to
justify this one. I'll drop it.
Thanks,
Emilio
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 34/35] exec: push BQL down to cpu->do_interrupt
2018-09-17 16:31 ` [PATCH 34/35] exec: push BQL down to cpu->do_interrupt Emilio G. Cota
2018-09-18 4:11 ` David Gibson
2018-09-18 7:12 ` David Hildenbrand
@ 2018-09-19 16:38 ` Cornelia Huck
2 siblings, 0 replies; 18+ messages in thread
From: Cornelia Huck @ 2018-09-19 16:38 UTC (permalink / raw)
To: Emilio G. Cota
Cc: Peter Maydell, Chris Wulff, kvm, David Hildenbrand, James Hogan,
Anthony Green, Mark Cave-Ayland, qemu-devel, Edgar E. Iglesias,
Guan Xuetao, Marek Vasut, Alexander Graf, Christian Borntraeger,
Richard Henderson, Artyom Tarasenko, Eduardo Habkost, qemu-s390x,
qemu-arm, Stafford Horne, David Gibson, Peter Crosthwaite,
Marcelo Tosatti, Laurent Vivier, Michael Walle, qemu-ppc
On Mon, 17 Sep 2018 12:31:02 -0400
"Emilio G. Cota" <cota@braap.org> wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> 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 <amarkovic@wavecomp.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Anthony Green <green@moxielogic.com>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Chris Wulff <crwulff@gmail.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
> Cc: James Hogan <jhogan@kernel.org>
> Cc: kvm@vger.kernel.org
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Michael Walle <michael@walle.cc>
> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-ppc@nongnu.org
> Cc: qemu-s390x@nongnu.org
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Stafford Horne <shorne@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
> 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(-)
s390x parts:
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 32/35] target/i386/kvm: do not acquire the BQL to call cpu_reset_interrupt
2018-09-17 16:31 ` [PATCH 32/35] target/i386/kvm: do not acquire the BQL to call cpu_reset_interrupt Emilio G. Cota
2018-09-18 21:12 ` Richard Henderson
@ 2018-09-19 21:19 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-09-19 21:19 UTC (permalink / raw)
To: Emilio G. Cota, qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, kvm, Richard Henderson
On 9/17/18 6:31 PM, Emilio G. Cota wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> It's not needed anymore.
>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: kvm@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> target/i386/kvm.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index e40c8d5461..41c4830be8 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -3007,9 +3007,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
> /* Inject NMI */
> if (interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
> if (interrupt_request & CPU_INTERRUPT_NMI) {
> - qemu_mutex_lock_iothread();
> cpu_reset_interrupt(cpu, CPU_INTERRUPT_NMI);
> - qemu_mutex_unlock_iothread();
> DPRINTF("injected NMI\n");
> ret = kvm_vcpu_ioctl(cpu, KVM_NMI);
> if (ret < 0) {
> @@ -3018,9 +3016,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
> }
> }
> if (atomic_read(&cpu->interrupt_request) & CPU_INTERRUPT_SMI) {
> - qemu_mutex_lock_iothread();
> cpu_reset_interrupt(cpu, CPU_INTERRUPT_SMI);
> - qemu_mutex_unlock_iothread();
> DPRINTF("injected SMI\n");
> ret = kvm_vcpu_ioctl(cpu, KVM_SMI);
> if (ret < 0) {
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 00/35] exec: drop BQL from interrupt handling
2018-09-17 16:30 [PATCH 00/35] exec: drop BQL from interrupt handling Emilio G. Cota
` (5 preceding siblings ...)
2018-09-18 9:51 ` [PATCH 00/35] exec: drop BQL from interrupt handling David Hildenbrand
@ 2018-09-20 20:05 ` Mark Cave-Ayland
6 siblings, 0 replies; 18+ messages in thread
From: Mark Cave-Ayland @ 2018-09-20 20:05 UTC (permalink / raw)
To: Emilio G. Cota, qemu-devel
Cc: Peter Maydell, Cornelia Huck, kvm, David Hildenbrand,
James Hogan, Anthony Green, Edgar E. Iglesias, Guan Xuetao,
Marek Vasut, Alexander Graf, Christian Borntraeger,
Richard Henderson, Artyom Tarasenko, Eduardo Habkost, qemu-s390x,
qemu-arm, Stafford Horne, David Gibson, Chris Wulff,
Peter Crosthwaite, Marcelo Tosatti, Laurent Vivier,
Michael Walle, qemu-ppc, Aleksandar Markovic
On 17/09/2018 17:30, Emilio G. Cota wrote:
> This series comes originally from a series of patches that Paolo
> sent to me a long time ago. I have kept most of his S-o-b tags,
> but I have done the forward port of the patches to the current
> QEMU code base, so please blame all possible bugs on me, not him.
>
> The goal of this series is to push the BQL out of interrupt
> handling wherever possible. Many targets do not need the BQL
> to handle interrupts, so for those targets we get rid of the
> BQL in interrupt handling. In TCG mode, this by itself does
> not yield measurable performance gains. However, it is a
> necessary first step before tackling BQL contention when
> managing CPU states (i.e. exiting the exec loop always
> involves taking the BQL, which does not scale).
>
> The patches:
>
> - Patch 0 accesses icount_decr.u16 consistently with atomics.
> This is important because we use it across threads to signal
> that an interrupt is pending.
>
> - 2-28: convert direct callers of cpu->interrupt_request
> to use atomic accesses or CPU helper functions that use atomics.
> I have broken these up into individual patches to ease review.
> Note that in some cases I have added many atomic_reads, most
> of which are probably unnecessary. However, I'd leave this up
> to the maintainers, since I do not want to break things by
> doing a single atomic_read(&cpu->interrupt_request) at the
> beginning of a function and then miss subsequent updates to
> cpu->interrupt_request.
>
> - 29-31: drop the BQL requirement when handling cpu->interrupt_request
>
> - 32-33: drop now unnecessary BQL acquisitions in i386 and ppc
>
> - 34-35: push down the BQL acquisition to cpu->do_interrupt
> and cpu->cpu_exec_interrupt. Here I am Cc'ing everyone so that
> we can make sure I am not missing any target that requires
> the BQL on either do_interrupt or cpu_exec_interrupt.
>
> This series is checkpatch-clean, and can be fetched from:
> https://github.com/cota/qemu/tree/cpu-lock
>
> Note that the series applies on top of patches that are
> already on the qemu-devel list and will hopefully soon
> be upstream. This series begins after commit
> 4ece0cbd74 ("cpus: access .qemu_icount_bias with atomic64")
> in that tree.
I ran the above cpu-lock branch through my SPARC32 and SPARC64 test suite just to be
sure, and everything appeared to work fine. So for the SPARC32/64 patches:
Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-09-20 20:05 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-17 16:30 [PATCH 00/35] exec: drop BQL from interrupt handling Emilio G. Cota
2018-09-17 16:30 ` [PATCH 02/35] target/i386: use cpu_reset_interrupt Emilio G. Cota
2018-09-18 20:47 ` Richard Henderson
2018-09-17 16:30 ` [PATCH 11/35] target/i386: access cpu->interrupt_request with atomics Emilio G. Cota
2018-09-18 21:04 ` Richard Henderson
2018-09-19 15:02 ` Emilio G. Cota
2018-09-17 16:31 ` [PATCH 32/35] target/i386/kvm: do not acquire the BQL to call cpu_reset_interrupt Emilio G. Cota
2018-09-18 21:12 ` Richard Henderson
2018-09-19 21:19 ` Philippe Mathieu-Daudé
2018-09-17 16:31 ` [PATCH 34/35] exec: push BQL down to cpu->do_interrupt Emilio G. Cota
2018-09-18 4:11 ` David Gibson
2018-09-18 7:12 ` David Hildenbrand
2018-09-19 16:38 ` Cornelia Huck
2018-09-17 16:31 ` [PATCH 35/35] exec: push BQL down to cpu->cpu_exec_interrupt Emilio G. Cota
2018-09-18 4:12 ` David Gibson
2018-09-18 7:48 ` David Hildenbrand
2018-09-18 9:51 ` [PATCH 00/35] exec: drop BQL from interrupt handling David Hildenbrand
2018-09-20 20:05 ` Mark Cave-Ayland
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.