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

[Fam Zheng <famz@redhat.com>]

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