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

[Paolo Bonzini <pbonzini@redhat.com>]

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
> 
>