Message ID | 1434646046-27150-7-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
Am 18.06.2015 um 18:47 schrieb Paolo Bonzini: > @@ -1887,11 +1893,15 @@ int kvm_cpu_exec(CPUState *cpu) > break; > default: > DPRINTF("kvm_arch_handle_exit\n"); > + qemu_mutex_lock_iothread(); > ret = kvm_arch_handle_exit(cpu, run); > + qemu_mutex_unlock_iothread(); > break; > } > } while (ret == 0); > The next logical step would be to do a push down. Get rid of these two new lines and do it in every kvm_arch_handle_exit function. This would allow arch maintainers to do their part of lock breaking. Can be an addon patch, though. Christian
On Thu, 06/18 18:47, Paolo Bonzini wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > This opens the path to get rid of the iothread lock on vmexits in KVM > mode. On x86, the in-kernel irqchips has to be used because we otherwise > need to synchronize APIC and other per-cpu state accesses that could be > changed concurrently. > > s390x and ARM should be fine without specific locking as their > pre/post-run callbacks are empty. MIPS and POWER require locking for > the pre-run callback. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > kvm-all.c | 14 ++++++++++++-- > target-i386/kvm.c | 18 ++++++++++++++++++ > target-mips/kvm.c | 4 ++++ > target-ppc/kvm.c | 4 ++++ > 4 files changed, 38 insertions(+), 2 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index b2b1bc3..2bd8e9b 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -1795,6 +1795,8 @@ int kvm_cpu_exec(CPUState *cpu) > return EXCP_HLT; > } > > + qemu_mutex_unlock_iothread(); > + > do { > MemTxAttrs attrs; > > @@ -1813,11 +1815,9 @@ int kvm_cpu_exec(CPUState *cpu) > */ > qemu_cpu_kick_self(); > } > - qemu_mutex_unlock_iothread(); > > run_ret = kvm_vcpu_ioctl(cpu, KVM_RUN, 0); > > - qemu_mutex_lock_iothread(); > attrs = kvm_arch_post_run(cpu, run); > > if (run_ret < 0) { > @@ -1836,20 +1836,24 @@ int kvm_cpu_exec(CPUState *cpu) > switch (run->exit_reason) { > case KVM_EXIT_IO: > DPRINTF("handle_io\n"); > + qemu_mutex_lock_iothread(); > kvm_handle_io(run->io.port, attrs, > (uint8_t *)run + run->io.data_offset, > run->io.direction, > run->io.size, > run->io.count); > + qemu_mutex_unlock_iothread(); > ret = 0; > break; > case KVM_EXIT_MMIO: > DPRINTF("handle_mmio\n"); > + qemu_mutex_lock_iothread(); > address_space_rw(&address_space_memory, > run->mmio.phys_addr, attrs, > run->mmio.data, > run->mmio.len, > run->mmio.is_write); > + qemu_mutex_unlock_iothread(); > ret = 0; > break; > case KVM_EXIT_IRQ_WINDOW_OPEN: > @@ -1858,7 +1862,9 @@ int kvm_cpu_exec(CPUState *cpu) > break; > case KVM_EXIT_SHUTDOWN: > DPRINTF("shutdown\n"); > + qemu_mutex_lock_iothread(); > qemu_system_reset_request(); > + qemu_mutex_unlock_iothread(); > ret = EXCP_INTERRUPT; > break; > case KVM_EXIT_UNKNOWN: More context: fprintf(stderr, "KVM: unknown exit, hardware reason %" PRIx64 "\n", (uint64_t)run->hw.hardware_exit_reason); ret = -1; break; case KVM_EXIT_INTERNAL_ERROR: * ret = kvm_handle_internal_error(cpu, run); break; case KVM_EXIT_SYSTEM_EVENT: switch (run->system_event.type) { case KVM_SYSTEM_EVENT_SHUTDOWN: * qemu_system_shutdown_request(); ret = EXCP_INTERRUPT; break; case KVM_SYSTEM_EVENT_RESET: * qemu_system_reset_request(); ret = EXCP_INTERRUPT; > break; > default: > DPRINTF("kvm_arch_handle_exit\n"); > + qemu_mutex_lock_iothread(); > ret = kvm_arch_handle_exit(cpu, run); > + qemu_mutex_unlock_iothread(); > break; > } > } while (ret == 0); > > + qemu_mutex_lock_iothread(); > + > if (ret < 0) { > cpu_dump_state(cpu, stderr, fprintf, CPU_DUMP_CODE); > vm_stop(RUN_STATE_INTERNAL_ERROR); Could you explain why above three "*" calls are safe? Fam
On 23/06/2015 11:26, Fam Zheng wrote: > On Thu, 06/18 18:47, Paolo Bonzini wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> This opens the path to get rid of the iothread lock on vmexits in KVM >> mode. On x86, the in-kernel irqchips has to be used because we otherwise >> need to synchronize APIC and other per-cpu state accesses that could be >> changed concurrently. >> >> s390x and ARM should be fine without specific locking as their >> pre/post-run callbacks are empty. MIPS and POWER require locking for >> the pre-run callback. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> kvm-all.c | 14 ++++++++++++-- >> target-i386/kvm.c | 18 ++++++++++++++++++ >> target-mips/kvm.c | 4 ++++ >> target-ppc/kvm.c | 4 ++++ >> 4 files changed, 38 insertions(+), 2 deletions(-) >> >> diff --git a/kvm-all.c b/kvm-all.c >> index b2b1bc3..2bd8e9b 100644 >> --- a/kvm-all.c >> +++ b/kvm-all.c >> @@ -1795,6 +1795,8 @@ int kvm_cpu_exec(CPUState *cpu) >> return EXCP_HLT; >> } >> >> + qemu_mutex_unlock_iothread(); >> + >> do { >> MemTxAttrs attrs; >> >> @@ -1813,11 +1815,9 @@ int kvm_cpu_exec(CPUState *cpu) >> */ >> qemu_cpu_kick_self(); >> } >> - qemu_mutex_unlock_iothread(); >> >> run_ret = kvm_vcpu_ioctl(cpu, KVM_RUN, 0); >> >> - qemu_mutex_lock_iothread(); >> attrs = kvm_arch_post_run(cpu, run); >> >> if (run_ret < 0) { >> @@ -1836,20 +1836,24 @@ int kvm_cpu_exec(CPUState *cpu) >> switch (run->exit_reason) { >> case KVM_EXIT_IO: >> DPRINTF("handle_io\n"); >> + qemu_mutex_lock_iothread(); >> kvm_handle_io(run->io.port, attrs, >> (uint8_t *)run + run->io.data_offset, >> run->io.direction, >> run->io.size, >> run->io.count); >> + qemu_mutex_unlock_iothread(); >> ret = 0; >> break; >> case KVM_EXIT_MMIO: >> DPRINTF("handle_mmio\n"); >> + qemu_mutex_lock_iothread(); >> address_space_rw(&address_space_memory, >> run->mmio.phys_addr, attrs, >> run->mmio.data, >> run->mmio.len, >> run->mmio.is_write); >> + qemu_mutex_unlock_iothread(); >> ret = 0; >> break; >> case KVM_EXIT_IRQ_WINDOW_OPEN: >> @@ -1858,7 +1862,9 @@ int kvm_cpu_exec(CPUState *cpu) >> break; >> case KVM_EXIT_SHUTDOWN: >> DPRINTF("shutdown\n"); >> + qemu_mutex_lock_iothread(); >> qemu_system_reset_request(); >> + qemu_mutex_unlock_iothread(); >> ret = EXCP_INTERRUPT; >> break; >> case KVM_EXIT_UNKNOWN: > > More context: > > fprintf(stderr, "KVM: unknown exit, hardware reason %" PRIx64 "\n", > (uint64_t)run->hw.hardware_exit_reason); > ret = -1; > break; > case KVM_EXIT_INTERNAL_ERROR: > * ret = kvm_handle_internal_error(cpu, run); This one only accesses data internal to the VCPU thread. > break; > case KVM_EXIT_SYSTEM_EVENT: > switch (run->system_event.type) { > case KVM_SYSTEM_EVENT_SHUTDOWN: > * qemu_system_shutdown_request(); > ret = EXCP_INTERRUPT; > break; > case KVM_SYSTEM_EVENT_RESET: > * qemu_system_reset_request(); These two are thread-safe. Paolo > ret = EXCP_INTERRUPT; >> break; >> default: >> DPRINTF("kvm_arch_handle_exit\n"); >> + qemu_mutex_lock_iothread(); >> ret = kvm_arch_handle_exit(cpu, run); >> + qemu_mutex_unlock_iothread(); >> break; >> } >> } while (ret == 0); >> >> + qemu_mutex_lock_iothread(); >> + >> if (ret < 0) { >> cpu_dump_state(cpu, stderr, fprintf, CPU_DUMP_CODE); >> vm_stop(RUN_STATE_INTERNAL_ERROR); > > Could you explain why above three "*" calls are safe? > > Fam > >
On Tue, 06/23 11:29, Paolo Bonzini wrote: > > > On 23/06/2015 11:26, Fam Zheng wrote: > > On Thu, 06/18 18:47, Paolo Bonzini wrote: > >> From: Jan Kiszka <jan.kiszka@siemens.com> > >> > >> This opens the path to get rid of the iothread lock on vmexits in KVM > >> mode. On x86, the in-kernel irqchips has to be used because we otherwise > >> need to synchronize APIC and other per-cpu state accesses that could be > >> changed concurrently. > >> > >> s390x and ARM should be fine without specific locking as their > >> pre/post-run callbacks are empty. MIPS and POWER require locking for > >> the pre-run callback. > >> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >> --- > >> kvm-all.c | 14 ++++++++++++-- > >> target-i386/kvm.c | 18 ++++++++++++++++++ > >> target-mips/kvm.c | 4 ++++ > >> target-ppc/kvm.c | 4 ++++ > >> 4 files changed, 38 insertions(+), 2 deletions(-) > >> > >> diff --git a/kvm-all.c b/kvm-all.c > >> index b2b1bc3..2bd8e9b 100644 > >> --- a/kvm-all.c > >> +++ b/kvm-all.c > >> @@ -1795,6 +1795,8 @@ int kvm_cpu_exec(CPUState *cpu) > >> return EXCP_HLT; > >> } > >> > >> + qemu_mutex_unlock_iothread(); > >> + > >> do { > >> MemTxAttrs attrs; > >> > >> @@ -1813,11 +1815,9 @@ int kvm_cpu_exec(CPUState *cpu) > >> */ > >> qemu_cpu_kick_self(); > >> } > >> - qemu_mutex_unlock_iothread(); > >> > >> run_ret = kvm_vcpu_ioctl(cpu, KVM_RUN, 0); > >> > >> - qemu_mutex_lock_iothread(); > >> attrs = kvm_arch_post_run(cpu, run); > >> > >> if (run_ret < 0) { > >> @@ -1836,20 +1836,24 @@ int kvm_cpu_exec(CPUState *cpu) > >> switch (run->exit_reason) { > >> case KVM_EXIT_IO: > >> DPRINTF("handle_io\n"); > >> + qemu_mutex_lock_iothread(); > >> kvm_handle_io(run->io.port, attrs, > >> (uint8_t *)run + run->io.data_offset, > >> run->io.direction, > >> run->io.size, > >> run->io.count); > >> + qemu_mutex_unlock_iothread(); > >> ret = 0; > >> break; > >> case KVM_EXIT_MMIO: > >> DPRINTF("handle_mmio\n"); > >> + qemu_mutex_lock_iothread(); > >> address_space_rw(&address_space_memory, > >> run->mmio.phys_addr, attrs, > >> run->mmio.data, > >> run->mmio.len, > >> run->mmio.is_write); > >> + qemu_mutex_unlock_iothread(); > >> ret = 0; > >> break; > >> case KVM_EXIT_IRQ_WINDOW_OPEN: > >> @@ -1858,7 +1862,9 @@ int kvm_cpu_exec(CPUState *cpu) > >> break; > >> case KVM_EXIT_SHUTDOWN: > >> DPRINTF("shutdown\n"); > >> + qemu_mutex_lock_iothread(); > >> qemu_system_reset_request(); > >> + qemu_mutex_unlock_iothread(); > >> ret = EXCP_INTERRUPT; > >> break; > >> case KVM_EXIT_UNKNOWN: > > > > More context: > > > > fprintf(stderr, "KVM: unknown exit, hardware reason %" PRIx64 "\n", > > (uint64_t)run->hw.hardware_exit_reason); > > ret = -1; > > break; > > case KVM_EXIT_INTERNAL_ERROR: > > * ret = kvm_handle_internal_error(cpu, run); > > This one only accesses data internal to the VCPU thread. > > > break; > > case KVM_EXIT_SYSTEM_EVENT: > > switch (run->system_event.type) { > > case KVM_SYSTEM_EVENT_SHUTDOWN: > > * qemu_system_shutdown_request(); > > ret = EXCP_INTERRUPT; > > break; > > case KVM_SYSTEM_EVENT_RESET: > > * qemu_system_reset_request(); > > These two are thread-safe. But above you add lock/unlock around the qemu_system_reset_request() under KVM_EXIT_SHUTDOWN. What's different? Fam > > Paolo > > > ret = EXCP_INTERRUPT; > >> break; > >> default: > >> DPRINTF("kvm_arch_handle_exit\n"); > >> + qemu_mutex_lock_iothread(); > >> ret = kvm_arch_handle_exit(cpu, run); > >> + qemu_mutex_unlock_iothread(); > >> break; > >> } > >> } while (ret == 0); > >> > >> + qemu_mutex_lock_iothread(); > >> + > >> if (ret < 0) { > >> cpu_dump_state(cpu, stderr, fprintf, CPU_DUMP_CODE); > >> vm_stop(RUN_STATE_INTERNAL_ERROR); > > > > Could you explain why above three "*" calls are safe? > > > > Fam > > > >
On 23/06/2015 11:45, Fam Zheng wrote: >>> > > break; >>> > > case KVM_EXIT_SYSTEM_EVENT: >>> > > switch (run->system_event.type) { >>> > > case KVM_SYSTEM_EVENT_SHUTDOWN: >>> > > * qemu_system_shutdown_request(); >>> > > ret = EXCP_INTERRUPT; >>> > > break; >>> > > case KVM_SYSTEM_EVENT_RESET: >>> > > * qemu_system_reset_request(); >> > >> > These two are thread-safe. > But above you add lock/unlock around the qemu_system_reset_request() under > KVM_EXIT_SHUTDOWN. What's different? That's unnecessary. Also, just below this switch there's another call to kvm_arch_handle_exit that should be wrapped by lock/unlock (it works anyway because no one handles KVM_EXIT_SYSTEM_EVENT in kvm_arch_handle_exit, but it's not clean). Paolo
diff --git a/kvm-all.c b/kvm-all.c index b2b1bc3..2bd8e9b 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -1795,6 +1795,8 @@ int kvm_cpu_exec(CPUState *cpu) return EXCP_HLT; } + qemu_mutex_unlock_iothread(); + do { MemTxAttrs attrs; @@ -1813,11 +1815,9 @@ int kvm_cpu_exec(CPUState *cpu) */ qemu_cpu_kick_self(); } - qemu_mutex_unlock_iothread(); run_ret = kvm_vcpu_ioctl(cpu, KVM_RUN, 0); - qemu_mutex_lock_iothread(); attrs = kvm_arch_post_run(cpu, run); if (run_ret < 0) { @@ -1836,20 +1836,24 @@ int kvm_cpu_exec(CPUState *cpu) switch (run->exit_reason) { case KVM_EXIT_IO: DPRINTF("handle_io\n"); + qemu_mutex_lock_iothread(); kvm_handle_io(run->io.port, attrs, (uint8_t *)run + run->io.data_offset, run->io.direction, run->io.size, run->io.count); + qemu_mutex_unlock_iothread(); ret = 0; break; case KVM_EXIT_MMIO: DPRINTF("handle_mmio\n"); + qemu_mutex_lock_iothread(); address_space_rw(&address_space_memory, run->mmio.phys_addr, attrs, run->mmio.data, run->mmio.len, run->mmio.is_write); + qemu_mutex_unlock_iothread(); ret = 0; break; case KVM_EXIT_IRQ_WINDOW_OPEN: @@ -1858,7 +1862,9 @@ int kvm_cpu_exec(CPUState *cpu) break; case KVM_EXIT_SHUTDOWN: DPRINTF("shutdown\n"); + qemu_mutex_lock_iothread(); qemu_system_reset_request(); + qemu_mutex_unlock_iothread(); ret = EXCP_INTERRUPT; break; case KVM_EXIT_UNKNOWN: @@ -1887,11 +1893,15 @@ int kvm_cpu_exec(CPUState *cpu) break; default: DPRINTF("kvm_arch_handle_exit\n"); + qemu_mutex_lock_iothread(); ret = kvm_arch_handle_exit(cpu, run); + qemu_mutex_unlock_iothread(); break; } } while (ret == 0); + qemu_mutex_lock_iothread(); + if (ret < 0) { cpu_dump_state(cpu, stderr, fprintf, CPU_DUMP_CODE); vm_stop(RUN_STATE_INTERNAL_ERROR); diff --git a/target-i386/kvm.c b/target-i386/kvm.c index ca2da84..8c2a891 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -2192,7 +2192,10 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run) /* Inject NMI */ if (cpu->interrupt_request & CPU_INTERRUPT_NMI) { + qemu_mutex_lock_iothread(); cpu->interrupt_request &= ~CPU_INTERRUPT_NMI; + qemu_mutex_unlock_iothread(); + DPRINTF("injected NMI\n"); ret = kvm_vcpu_ioctl(cpu, KVM_NMI); if (ret < 0) { @@ -2201,6 +2204,10 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run) } } + if (!kvm_irqchip_in_kernel()) { + qemu_mutex_lock_iothread(); + } + /* Force the VCPU out of its inner loop to process any INIT requests * or (for userspace APIC, but it is cheap to combine the checks here) * pending TPR access reports. @@ -2244,6 +2251,8 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run) DPRINTF("setting tpr\n"); run->cr8 = cpu_get_apic_tpr(x86_cpu->apic_state); + + qemu_mutex_unlock_iothread(); } } @@ -2257,8 +2266,17 @@ MemTxAttrs kvm_arch_post_run(CPUState *cpu, struct kvm_run *run) } else { env->eflags &= ~IF_MASK; } + + /* We need to protect the apic state against concurrent accesses from + * different threads in case the userspace irqchip is used. */ + if (!kvm_irqchip_in_kernel()) { + qemu_mutex_lock_iothread(); + } cpu_set_apic_tpr(x86_cpu->apic_state, run->cr8); cpu_set_apic_base(x86_cpu->apic_state, run->apic_base); + if (!kvm_irqchip_in_kernel()) { + qemu_mutex_unlock_iothread(); + } return MEMTXATTRS_UNSPECIFIED; } diff --git a/target-mips/kvm.c b/target-mips/kvm.c index 948619f..7d2293d 100644 --- a/target-mips/kvm.c +++ b/target-mips/kvm.c @@ -99,6 +99,8 @@ void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run) int r; struct kvm_mips_interrupt intr; + qemu_mutex_lock_iothread(); + if ((cs->interrupt_request & CPU_INTERRUPT_HARD) && cpu_mips_io_interrupts_pending(cpu)) { intr.cpu = -1; @@ -109,6 +111,8 @@ void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run) __func__, cs->cpu_index, intr.irq); } } + + qemu_mutex_unlock_iothread(); } MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run) diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index afb4696..1fa1529 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -1242,6 +1242,8 @@ void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run) int r; unsigned irq; + qemu_mutex_lock_iothread(); + /* PowerPC QEMU tracks the various core input pins (interrupt, critical * interrupt, reset, etc) in PPC-specific env->irq_input_state. */ if (!cap_interrupt_level && @@ -1269,6 +1271,8 @@ void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run) /* We don't know if there are more interrupts pending after this. However, * the guest will return to userspace in the course of handling this one * anyways, so we will get a chance to deliver the rest. */ + + qemu_mutex_unlock_iothread(); } MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)