Re: [Qemu-devel] [PATCH] kvm: First step to push iothread lock out of inner run loop

[Anthony Liguori <anthony@codemonkey.ws>]

On 06/22/2012 05:45 PM, Jan Kiszka wrote:
> This sketches a possible path to get rid of the iothread lock on vmexits
> in KVM mode. On x86, the 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. Not yet fully analyzed is the NMI
> injection path in the absence of an APIC.
>
> s390x should be fine without specific locking as their pre/post-run
> callbacks are empty. Power requires locking for the pre-run callback.
>
> This patch is untested, but a similar version was successfully used in
> a x86 setup with a network I/O path that needed no central iothread
> locking anymore (required special MMIO exit handling).
> ---
>   kvm-all.c         |   18 ++++++++++++++++--
>   target-i386/kvm.c |    7 +++++++
>   target-ppc/kvm.c  |    4 ++++
>   3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index f8e4328..9c3e26f 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1460,6 +1460,8 @@ int kvm_cpu_exec(CPUArchState *env)
>           return EXCP_HLT;
>       }
>
> +    qemu_mutex_unlock_iothread();
> +
>       do {
>           if (env->kvm_vcpu_dirty) {
>               kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
> @@ -1476,14 +1478,16 @@ int kvm_cpu_exec(CPUArchState *env)
>                */
>               qemu_cpu_kick_self();
>           }
> -        qemu_mutex_unlock_iothread();
>
>           run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
>
> -        qemu_mutex_lock_iothread();
>           kvm_arch_post_run(env, run);
>
> +        /* TODO: push coalesced mmio flushing to the point where we access
> +         * devices that are using it (currently VGA and E1000). */
> +        qemu_mutex_lock_iothread();
>           kvm_flush_coalesced_mmio_buffer();
> +        qemu_mutex_unlock_iothread();
>
>           if (run_ret<  0) {
>               if (run_ret == -EINTR || run_ret == -EAGAIN) {
> @@ -1499,19 +1503,23 @@ int kvm_cpu_exec(CPUArchState *env)
>           switch (run->exit_reason) {
>           case KVM_EXIT_IO:
>               DPRINTF("handle_io\n");
> +            qemu_mutex_lock_iothread();
>               kvm_handle_io(run->io.port,
>                             (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();
>               cpu_physical_memory_rw(run->mmio.phys_addr,
>                                      run->mmio.data,
>                                      run->mmio.len,
>                                      run->mmio.is_write);
> +            qemu_mutex_unlock_iothread();
>               ret = 0;
>               break;
>           case KVM_EXIT_IRQ_WINDOW_OPEN:
> @@ -1520,7 +1528,9 @@ int kvm_cpu_exec(CPUArchState *env)
>               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:
> @@ -1533,11 +1543,15 @@ int kvm_cpu_exec(CPUArchState *env)
>               break;
>           default:
>               DPRINTF("kvm_arch_handle_exit\n");
> +            qemu_mutex_lock_iothread();
>               ret = kvm_arch_handle_exit(env, run);
> +            qemu_mutex_unlock_iothread();
>               break;
>           }
>       } while (ret == 0);
>
> +    qemu_mutex_lock_iothread();
> +
>       if (ret<  0) {
>           cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
>           vm_stop(RUN_STATE_INTERNAL_ERROR);
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 0d0d8f6..0ad64d1 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -1631,7 +1631,10 @@ void kvm_arch_pre_run(CPUX86State *env, struct kvm_run *run)
>
>       /* Inject NMI */
>       if (env->interrupt_request&  CPU_INTERRUPT_NMI) {

Strictly speaking, wouldn't we need to use testbit() and setbit()?  I would 
expect in the very least a barrier would be needed.

Looks pretty nice overall.  I'll need to apply and spend some time carefully 
walking through the code.

Thanks for sharing!

Regards,

Anthony Liguori

> +        qemu_mutex_lock_iothread();
>           env->interrupt_request&= ~CPU_INTERRUPT_NMI;
> +        qemu_mutex_unlock_iothread();
> +
>           DPRINTF("injected NMI\n");
>           ret = kvm_vcpu_ioctl(env, KVM_NMI);
>           if (ret<  0) {
> @@ -1641,6 +1644,8 @@ void kvm_arch_pre_run(CPUX86State *env, 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 pending TPR access reports. */
>           if (env->interrupt_request&
> @@ -1682,6 +1687,8 @@ void kvm_arch_pre_run(CPUX86State *env, struct kvm_run *run)
>
>           DPRINTF("setting tpr\n");
>           run->cr8 = cpu_get_apic_tpr(env->apic_state);
> +
> +        qemu_mutex_unlock_iothread();
>       }
>   }
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index c09cc39..60d91a5 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -471,6 +471,8 @@ void kvm_arch_pre_run(CPUPPCState *env, 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&&
> @@ -497,6 +499,8 @@ void kvm_arch_pre_run(CPUPPCState *env, 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();
>   }
>
>   void kvm_arch_post_run(CPUPPCState *env, struct kvm_run *run)