kvm: First step to push iothread lock out of inner run loop

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

kvm-all.c +16 -2

index f8e4328..9c3e26f 100644
@@ -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);

target-i386/kvm.c +7 -0

index 0d0d8f6..0ad64d1 100644
@@ -1631,7 +1631,10 @@ void kvm_arch_pre_run(CPUX86State *env, struct kvm_run *run)
 
     /* Inject NMI */
     if (env->interrupt_request & CPU_INTERRUPT_NMI) {
+        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();
     }
 }
 

target-ppc/kvm.c +4 -0

index c09cc39..60d91a5 100644
@@ -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)

Should have declared this [RFC] in the subject and CC'ed kvm...

Quoted 37 lines · Jan Kiszka

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

kvm-all.c +2 -1

index f8e4328..9c3e26f 100644
@@ -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();
 

kvm-all.c int kvm_cpu_exec(CPUArchState *env) · line 1484

Quoted 4 lines

kvm-all.c +0 -1

         run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
 
-        qemu_mutex_lock_iothread();
         kvm_arch_post_run(env, run);

kvm-all.c int kvm_cpu_exec(CPUArchState *env) · line 1490

Quoted 5 lines

kvm-all.c +4 -0

+        /* 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();

Quoted 40 lines · Jan Kiszka

Should have declared this [RFC] in the subject and CC'ed kvm...

Quoted 37 lines · Jan Kiszka

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

kvm-all.c +2 -1

index f8e4328..9c3e26f 100644
@@ -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();
 

Quoted 4 lines

kvm-all.c int kvm_cpu_exec(CPUArchState *env) · line 1484

Quoted 4 lines

kvm-all.c +0 -1

         run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
 
-        qemu_mutex_lock_iothread();
         kvm_arch_post_run(env, run);

target-i386/kvm.c

void kvm_arch_post_run(CPUX86State *env, struct kvm_run *run)

{       
    if (run->if_flag) {
        env->eflags |= IF_MASK;
    } else {
        env->eflags &= ~IF_MASK;
    }
    cpu_set_apic_tpr(env->apic_state, run->cr8);
    cpu_set_apic_base(env->apic_state, run->apic_base);
}

Clearly there is no structure to any of the writes around the writes in x86's kvm_arch_post_run, so it is unsafe.

Quoted 59 lines · Marcelo Tosatti

Quoted 40 lines · Jan Kiszka

Should have declared this [RFC] in the subject and CC'ed kvm...

Quoted 37 lines · Jan Kiszka

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

kvm-all.c +2 -1

index f8e4328..9c3e26f 100644
@@ -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();
 

Quoted 4 lines

kvm-all.c int kvm_cpu_exec(CPUArchState *env) · line 1484

Quoted 4 lines

kvm-all.c +0 -1

         run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
 
-        qemu_mutex_lock_iothread();
         kvm_arch_post_run(env, run);

target-i386/kvm.c

void kvm_arch_post_run(CPUX86State *env, struct kvm_run *run)

{       
    if (run->if_flag) {
        env->eflags |= IF_MASK;
    } else {
        env->eflags &= ~IF_MASK;
    }
    cpu_set_apic_tpr(env->apic_state, run->cr8);
    cpu_set_apic_base(env->apic_state, run->apic_base);
}

Quoted 2 lines

Clearly there is no structure to any of the writes around the writes in x86's kvm_arch_post_run, so it is unsafe.

No access protocol to the CPUState and apic devices (who can write when, who can read when).

Hmm, we may need the iothread lock around cpu_set_apic_tpr for !kvm_irqchip_in_kernel(). And as we are at it, apic_base manipulation can be but there as well.

With in-kernel irqchip, there is no such need. Also, no one accesses eflags outside of the vcpu thread, independent of the irqchip mode.

Quoted 5 lines · Jan Kiszka

Hmm, we may need the iothread lock around cpu_set_apic_tpr for !kvm_irqchip_in_kernel(). And as we are at it, apic_base manipulation can be but there as well.

Quoted 2 lines

With in-kernel irqchip, there is no such need. Also, no one accesses eflags outside of the vcpu thread, independent of the irqchip mode.

In fact !kvm_irqchip_in_kernel() is broken wrt the tpr. Interrupt injection needs to be done atomically, but currently we check the tpr from the injecting thread, which means the cpu thread can race with it.
We need to move the check to the vcpu thread so that the guest vcpu is
halted.

Quoted 9 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka

Hmm, we may need the iothread lock around cpu_set_apic_tpr for !kvm_irqchip_in_kernel(). And as we are at it, apic_base manipulation can be but there as well.

With in-kernel irqchip, there is no such need. Also, no one accesses eflags outside of the vcpu thread, independent of the irqchip mode.

Quoted 5 lines

In fact !kvm_irqchip_in_kernel() is broken wrt the tpr. Interrupt injection needs to be done atomically, but currently we check the tpr from the injecting thread, which means the cpu thread can race with it.
We need to move the check to the vcpu thread so that the guest vcpu is
halted.

So apic_set_irq basically needs to be deferred to vcpu context, right? Will have a look.

Quoted 9 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka

Hmm, we may need the iothread lock around cpu_set_apic_tpr for !kvm_irqchip_in_kernel(). And as we are at it, apic_base manipulation can be but there as well.

With in-kernel irqchip, there is no such need. Also, no one accesses eflags outside of the vcpu thread, independent of the irqchip mode.

Quoted 5 lines

In fact !kvm_irqchip_in_kernel() is broken wrt the tpr. Interrupt injection needs to be done atomically, but currently we check the tpr from the injecting thread, which means the cpu thread can race with it.
We need to move the check to the vcpu thread so that the guest vcpu is
halted.

So apic_set_irq basically needs to be deferred to vcpu context, right? Will have a look.

Quoted 10 lines · Jan Kiszka

Quoted 9 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka

Hmm, we may need the iothread lock around cpu_set_apic_tpr for !kvm_irqchip_in_kernel(). And as we are at it, apic_base manipulation can be but there as well.

With in-kernel irqchip, there is no such need. Also, no one accesses eflags outside of the vcpu thread, independent of the irqchip mode.

Quoted 8 lines

Quoted 5 lines

In fact !kvm_irqchip_in_kernel() is broken wrt the tpr. Interrupt injection needs to be done atomically, but currently we check the tpr from the injecting thread, which means the cpu thread can race with it.
We need to move the check to the vcpu thread so that the guest vcpu is
halted.

So apic_set_irq basically needs to be deferred to vcpu context, right? Will have a look.

Tried to wrap my head around this, but only found different issues (patches under construction).

First of all, a simple run_on_cpu doesn't work as it may drops the BQL at unexpected points inside device models.

Then I thought about what could actually race here: The testing of the userspace TPR value under BQL vs. some modification by the CPU while in KVM mode. So we may either inject while the CPU is trying to prevent this - harmless as it happens on real hw as well - or not inject while

the CPU is enabling this. But the latter is quickly resolved because all such TPR changes in userspace APIC mode are trapped and then processed under BQL. At that point we will also reevaluate the pending interrupts and inject what was deferred before (kvm_arch_post_run -> cpu_set_apic_tpr -> apic_set_tpr -> apic_update_irq, or via apic_mem_writel).

So where is a race?

Jan

Quoted 23 lines · Jan Kiszka

Quoted 10 lines · Jan Kiszka

Quoted 9 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka

Hmm, we may need the iothread lock around cpu_set_apic_tpr for !kvm_irqchip_in_kernel(). And as we are at it, apic_base manipulation can be but there as well.

With in-kernel irqchip, there is no such need. Also, no one accesses eflags outside of the vcpu thread, independent of the irqchip mode.

Quoted 8 lines

Quoted 5 lines

In fact !kvm_irqchip_in_kernel() is broken wrt the tpr. Interrupt injection needs to be done atomically, but currently we check the tpr from the injecting thread, which means the cpu thread can race with it.
We need to move the check to the vcpu thread so that the guest vcpu is
halted.

So apic_set_irq basically needs to be deferred to vcpu context, right? Will have a look.

Tried to wrap my head around this, but only found different issues (patches under construction).

Quoted 7 lines

First of all, a simple run_on_cpu doesn't work as it may drops the BQL at unexpected points inside device models.

Then I thought about what could actually race here: The testing of the userspace TPR value under BQL vs. some modification by the CPU while in KVM mode. So we may either inject while the CPU is trying to prevent this - harmless as it happens on real hw as well - or not inject while

Hmm, this could actually be a problem as the race window is extended beyond the instruction that raises TPR. So we may generate unexpected spurious interrupts for the guest. And that's because the userspace APIC fails to update the CPU interrupt pin properly when the TPR is modified. Here is the bug...

Quoted 63 lines · Marcelo Tosatti

Quoted 61 lines · Marcelo Tosatti

Quoted 40 lines · Jan Kiszka

Should have declared this [RFC] in the subject and CC'ed kvm...

Quoted 37 lines · Jan Kiszka

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

kvm-all.c +2 -1

index f8e4328..9c3e26f 100644
@@ -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();
 

Quoted 4 lines

kvm-all.c int kvm_cpu_exec(CPUArchState *env) · line 1484

Quoted 4 lines

kvm-all.c +0 -1

          run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
 
-        qemu_mutex_lock_iothread();
          kvm_arch_post_run(env, run);

target-i386/kvm.c

void kvm_arch_post_run(CPUX86State *env, struct kvm_run *run) {
    if (run->if_flag) {
        env->eflags |= IF_MASK;
    } else {
        env->eflags &= ~IF_MASK;
    }
    cpu_set_apic_tpr(env->apic_state, run->cr8); cpu_set_apic_base(env->apic_state, run->apic_base); }

Clearly there is no structure to any of the writes around the writes in x86's kvm_arch_post_run, so it is unsafe.

Quoted 3 lines

No access protocol to the CPUState and apic devices (who can write when, who can read when).

Hmm, we may need the iothread lock around cpu_set_apic_tpr for !kvm_irqchip_in_kernel(). And as we are at it, apic_base manipulation can be but there as well.

With in-kernel irqchip, there is no such need. Also, no one accesses eflags outside of the vcpu thread, independent of the irqchip mode.

Jan

Quoted 64 lines · Jan Kiszka

Quoted 63 lines · Marcelo Tosatti

Quoted 61 lines · Marcelo Tosatti

Quoted 40 lines · Jan Kiszka

Should have declared this [RFC] in the subject and CC'ed kvm...

Quoted 37 lines · Jan Kiszka

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

kvm-all.c +2 -1

index f8e4328..9c3e26f 100644
@@ -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();
 

Quoted 4 lines

kvm-all.c int kvm_cpu_exec(CPUArchState *env) · line 1484

Quoted 4 lines

kvm-all.c +0 -1

         run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
 
-        qemu_mutex_lock_iothread();
         kvm_arch_post_run(env, run);

target-i386/kvm.c

void kvm_arch_post_run(CPUX86State *env, struct kvm_run *run)

{
    if (run->if_flag) {
        env->eflags |= IF_MASK;
    } else {
        env->eflags &= ~IF_MASK;
    }
    cpu_set_apic_tpr(env->apic_state, run->cr8);
    cpu_set_apic_base(env->apic_state, run->apic_base);
}

Clearly there is no structure to any of the writes around the writes in x86's kvm_arch_post_run, so it is unsafe.

Quoted 8 lines

Quoted 3 lines

No access protocol to the CPUState and apic devices (who can write when, who can read when).

Hmm, we may need the iothread lock around cpu_set_apic_tpr for !kvm_irqchip_in_kernel(). And as we are at it, apic_base manipulation can be but there as well.

As to !kvm_irqchip_in_kernel(),I think there is quite a few fields in CPUState (interrupt_request,eflags,halted,exit_request,exception_injected), which we must care about. They are heavily depended on env->apic_state, so we consider them as a atomic context, that is say during the updating of env's these context, we do not allow apic_state changed.
And that is why I introduce cpu_lock in the patch "[PATCH 2/2] kvm:
use per-cpu lock to free vcpu thread out of the big lock"

As a first step, I will post a series later that gets rid of kvm_flush_coalesced_mmio_buffer in the common vmexit path.

Quoted 2 lines · Jan Kiszka

As a first step, I will post a series later that gets rid of kvm_flush_coalesced_mmio_buffer in the common vmexit path.

If you defer this, I can think of two places that need to flush:
- anything that accesses those memory areas (such as DMA to the framebuffer, or updating the display)

- anything that modifies the memory map and possibly changes flushed addresses

Quoted 7 lines · Avi Kivity

Quoted 2 lines · Jan Kiszka

As a first step, I will post a series later that gets rid of kvm_flush_coalesced_mmio_buffer in the common vmexit path.

If you defer this, I can think of two places that need to flush:
- anything that accesses those memory areas (such as DMA to the framebuffer, or updating the display)

- anything that accesses related areas (in case of VGA: PIO accesses to the control ports). I'm providing memory_region_set_flush_coalesced that allows to flush on non-coalesced region accesses as well. Some PIO accesses unfortunately still need open-coded qemu_flush_coalesced_mmio_buffer as they do not use memory regions yet.

Quoted 9 lines · Jan Kiszka

Quoted 7 lines · Avi Kivity

Quoted 2 lines · Jan Kiszka

As a first step, I will post a series later that gets rid of kvm_flush_coalesced_mmio_buffer in the common vmexit path.

If you defer this, I can think of two places that need to flush:
- anything that accesses those memory areas (such as DMA to the framebuffer, or updating the display)

Quoted 5 lines

- anything that accesses related areas (in case of VGA: PIO accesses to the control ports). I'm providing memory_region_set_flush_coalesced that allows to flush on non-coalesced region accesses as well. Some PIO accesses unfortunately still need open-coded qemu_flush_coalesced_mmio_buffer as they do not use memory regions yet.

Framebuffer access will bypass the MemoryRegionOps callbacks, did you intend to hook those?

I'm not sure the problem is general enough to merit a check in our generic mmio dispatch code (granted, now it has a check in the vcpu exit path which is much worse).

Quoted 16 lines · Avi Kivity

Quoted 14 lines · Jan Kiszka

Quoted 7 lines · Avi Kivity

Quoted 2 lines · Jan Kiszka

As a first step, I will post a series later that gets rid of kvm_flush_coalesced_mmio_buffer in the common vmexit path.

If you defer this, I can think of two places that need to flush:
- anything that accesses those memory areas (such as DMA to the framebuffer, or updating the display)

- anything that accesses related areas (in case of VGA: PIO accesses to the control ports). I'm providing memory_region_set_flush_coalesced that allows to flush on non-coalesced region accesses as well. Some PIO accesses unfortunately still need open-coded qemu_flush_coalesced_mmio_buffer as they do not use memory regions yet.

Quoted 2 lines

Framebuffer access will bypass the MemoryRegionOps callbacks, did you intend to hook those?

Are there really cases where the framebuffer is accessible both via MMIO and RAM-like mappings at the same time? If so, the current flushing on vmexit would help either as the direct mappings would not trigger exits. Or what do you mean?

Quoted 17 lines · Jan Kiszka

Quoted 16 lines · Avi Kivity

Quoted 14 lines · Jan Kiszka

Quoted 7 lines · Avi Kivity

Quoted 2 lines · Jan Kiszka

As a first step, I will post a series later that gets rid of kvm_flush_coalesced_mmio_buffer in the common vmexit path.

If you defer this, I can think of two places that need to flush:
- anything that accesses those memory areas (such as DMA to the framebuffer, or updating the display)

- anything that accesses related areas (in case of VGA: PIO accesses to the control ports). I'm providing memory_region_set_flush_coalesced that allows to flush on non-coalesced region accesses as well. Some PIO accesses unfortunately still need open-coded qemu_flush_coalesced_mmio_buffer as they do not use memory regions yet.

Quoted 7 lines

Quoted 2 lines

Framebuffer access will bypass the MemoryRegionOps callbacks, did you intend to hook those?

Are there really cases where the framebuffer is accessible both via MMIO and RAM-like mappings at the same time? If so, the current flushing on vmexit would help either as the direct mappings would not trigger exits. Or what do you mean?

I meant accesses by the display code to put the framebuffer in front of the user's eyes. Those just access the memory directly. We could wrap them with memory_region_get/put_ram_ptr() (as Xen requires anyway) and have those issue the flush.

Quoted 5 lines

Quoted 4 lines

Are there really cases where the framebuffer is accessible both via MMIO and RAM-like mappings at the same time? If so, the current flushing on vmexit would help either as the direct mappings would not trigger exits. Or what do you mean?

Quoted 4 lines

I meant accesses by the display code to put the framebuffer in front of the user's eyes. Those just access the memory directly. We could wrap them with memory_region_get/put_ram_ptr() (as Xen requires anyway) and have those issue the flush.

That's a non issue, we already have to flush on display updates. See e.g. vga_update_display.

Quoted 40 lines · Jan Kiszka

Should have declared this [RFC] in the subject and CC'ed kvm...

Quoted 37 lines · Jan Kiszka

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

kvm-all.c +2 -1

index f8e4328..9c3e26f 100644
@@ -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();
 

Quoted 4 lines

kvm-all.c int kvm_cpu_exec(CPUArchState *env) · line 1484

Quoted 4 lines

kvm-all.c +0 -1

         run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
 
-        qemu_mutex_lock_iothread();
         kvm_arch_post_run(env, run);

target-i386/kvm.c

void kvm_arch_post_run(CPUX86State *env, struct kvm_run *run)

{       
    if (run->if_flag) {
        env->eflags |= IF_MASK;
    } else {
        env->eflags &= ~IF_MASK;
    }
    cpu_set_apic_tpr(env->apic_state, run->cr8);
    cpu_set_apic_base(env->apic_state, run->apic_base);
}

Clearly there is no structure to any of the writes around the writes in x86's kvm_arch_post_run, so it is unsafe.

In kvm_flush_coalesced_mmio_buffer, however, the first and last pointers can be read safely without the global lock (so you could move the lock after reading run->exit_reason, in that case).

Quoted 5 lines

kvm-all.c int kvm_cpu_exec(CPUArchState *env) · line 1490

Quoted 5 lines

kvm-all.c +4 -0

+        /* 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();

But you have to flush first to then figure out which device the coalesced mmio belongs to (don't get that comment).

Quoted 59 lines · Marcelo Tosatti

Quoted 40 lines · Jan Kiszka

Should have declared this [RFC] in the subject and CC'ed kvm...

Quoted 37 lines · Jan Kiszka

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

kvm-all.c +2 -1

index f8e4328..9c3e26f 100644
@@ -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();
 

Quoted 4 lines

kvm-all.c int kvm_cpu_exec(CPUArchState *env) · line 1484

Quoted 4 lines

kvm-all.c +0 -1

         run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
 
-        qemu_mutex_lock_iothread();
         kvm_arch_post_run(env, run);

target-i386/kvm.c

void kvm_arch_post_run(CPUX86State *env, struct kvm_run *run)

{       
    if (run->if_flag) {
        env->eflags |= IF_MASK;
    } else {
        env->eflags &= ~IF_MASK;
    }
    cpu_set_apic_tpr(env->apic_state, run->cr8);
    cpu_set_apic_base(env->apic_state, run->apic_base);
}

Quoted 2 lines

Clearly there is no structure to any of the writes around the writes in x86's kvm_arch_post_run, so it is unsafe.

Can't parse this yet.

None of the fields touched above should be modified outside of the vcpu thread context (as long as that thread is inside the inner loop). Therefore, it should be safe to run that functions without any lock. Am I missing something?

Should have declared this [RFC] in the subject and CC'ed kvm...

Quoted 154 lines · Jan Kiszka

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

kvm-all.c +16 -2

index f8e4328..9c3e26f 100644
@@ -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);

target-i386/kvm.c +7 -0

index 0d0d8f6..0ad64d1 100644
@@ -1631,7 +1631,10 @@ void kvm_arch_pre_run(CPUX86State *env, struct kvm_run *run)
 
     /* Inject NMI */
     if (env->interrupt_request & CPU_INTERRUPT_NMI) {
+        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();
     }
 }
 

target-ppc/kvm.c +4 -0

index c09cc39..60d91a5 100644
@@ -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();
 }
 

target-ppc/kvm.c void kvm_arch_pre_run(CPUPPCState *env, struct kvm_run *run) · line 506

Quoted 2 lines

target-ppc/kvm.c +0 -0

 void kvm_arch_post_run(CPUPPCState *env, struct kvm_run *run)

On Sat, Jun 23, 2012 at 12:55:49AM +0200, Jan Kiszka wrote:
net.txt
--------

iothread flow
=============

1) Skip-work-if-device-locked

select(tap fd ready)
tap_send
   if (trylock(TAPState->NetClientState->dev))
       proceed;
   else
       continue; (data remains in queue)
   tap_read_packet
   qemu_send_packet_async

DRAWBACK: lock intensive.
DRAWBACK: VCPU threads can starve IOTHREAD (can be fixed with a scheme such as trylock() marks a flag saying "iothread wants lock".

2) Queue-work-to-vcpu-context

select(tap fd ready)
tap_send
   if (trylock(TAPState->NetClientState->dev)) {
       proceed;
   } else {
       AddToDeviceWorkQueue(work);
   }
   tap_read_packet
   qemu_send_packet_async

DRAWBACK: lock intensive
DRAWBACK: cache effects

Quoted 19 lines · Marcelo Tosatti

On Sat, Jun 23, 2012 at 12:55:49AM +0200, Jan Kiszka wrote:
net.txt
--------

iothread flow
=============

1) Skip-work-if-device-locked

select(tap fd ready)
tap_send
   if (trylock(TAPState->NetClientState->dev))
       proceed;
   else
       continue; (data remains in queue)
   tap_read_packet
   qemu_send_packet_async

Quoted 3 lines

DRAWBACK: lock intensive.
DRAWBACK: VCPU threads can starve IOTHREAD (can be fixed with a scheme such as trylock() marks a flag saying "iothread wants lock".

One alternative is to say the lock cannot be held across system calls or other long-running operations (similar to interrupt handler spinlocks in the kernel).  But QEMU threads are still at the mercy of the scheduler or page faults, so perhaps this is not a good idea because waiting on the lock could tie up the iothread.

Quoted 12 lines

2) Queue-work-to-vcpu-context

select(tap fd ready)
tap_send
   if (trylock(TAPState->NetClientState->dev)) {
       proceed;
   } else {
       AddToDeviceWorkQueue(work);
   }
   tap_read_packet
   qemu_send_packet_async

Quoted 2 lines

DRAWBACK: lock intensive
DRAWBACK: cache effects

This almost suggests we should invert packet reception for NICs.  tap fd ready should add a work item and the guest device calls into net/tap.c to pull out any packets from the work function:

tap_send
   dev_add_work(work);

virtio_net_rx_work_fn
   while ((req = virtqueue_pop(rx_queue))) {
       if (!net_receive_packet(netclient, req->buf)) { break; /* no more packets available */ }
       /* ...mark req complete and push onto virtqueue... */ }
   virtqueue_notify(rx_queue);

The idea is to avoid copies on the rx path and make the locking simple by always deferring to a work function (which can be invoked immediately if the device isn't locked).

Quoted 61 lines · Stefan Hajnoczi

Quoted 19 lines · Marcelo Tosatti

On Sat, Jun 23, 2012 at 12:55:49AM +0200, Jan Kiszka wrote:
net.txt
--------

iothread flow
=============

1) Skip-work-if-device-locked

select(tap fd ready)
tap_send
   if (trylock(TAPState->NetClientState->dev))
       proceed;
   else
       continue; (data remains in queue)
   tap_read_packet
   qemu_send_packet_async

Quoted 3 lines

DRAWBACK: lock intensive.
DRAWBACK: VCPU threads can starve IOTHREAD (can be fixed with a scheme such as trylock() marks a flag saying "iothread wants lock".

One alternative is to say the lock cannot be held across system calls or other long-running operations (similar to interrupt handler spinlocks in the kernel).  But QEMU threads are still at the mercy of the scheduler or page faults, so perhaps this is not a good idea because waiting on the lock could tie up the iothread.

Quoted 12 lines

2) Queue-work-to-vcpu-context

select(tap fd ready)
tap_send
   if (trylock(TAPState->NetClientState->dev)) {
       proceed;
   } else {
       AddToDeviceWorkQueue(work);
   }
   tap_read_packet
   qemu_send_packet_async

Quoted 2 lines

DRAWBACK: lock intensive
DRAWBACK: cache effects

This almost suggests we should invert packet reception for NICs.  tap fd ready should add a work item and the guest device calls into net/tap.c to pull out any packets from the work function:

tap_send
   dev_add_work(work);

virtio_net_rx_work_fn
   while ((req = virtqueue_pop(rx_queue))) {
       if (!net_receive_packet(netclient, req->buf)) { break; /* no more packets available */ }
       /* ...mark req complete and push onto virtqueue... */ }
   virtqueue_notify(rx_queue);

Quoted 3 lines

The idea is to avoid copies on the rx path and make the locking simple by always deferring to a work function (which can be invoked immediately if the device isn't locked).

I just realized this approach bypasses net/queue.c. I think it works really well for the peer model (no "VLANs"). Basically flow control is dictated by the peer (receiver) and we don't need to use NetQueue anymore. Whether this works elegantly for slirp and other backends, I'm not sure yet.

Quoted 61 lines · Stefan Hajnoczi

Quoted 19 lines · Marcelo Tosatti

On Sat, Jun 23, 2012 at 12:55:49AM +0200, Jan Kiszka wrote:
net.txt
--------

iothread flow
=============

1) Skip-work-if-device-locked

select(tap fd ready)
tap_send
   if (trylock(TAPState->NetClientState->dev))
       proceed;
   else
       continue; (data remains in queue)
   tap_read_packet
   qemu_send_packet_async

Quoted 3 lines

DRAWBACK: lock intensive.
DRAWBACK: VCPU threads can starve IOTHREAD (can be fixed with a scheme such as trylock() marks a flag saying "iothread wants lock".

One alternative is to say the lock cannot be held across system calls or other long-running operations (similar to interrupt handler spinlocks in the kernel).  But QEMU threads are still at the mercy of the scheduler or page faults, so perhaps this is not a good idea because waiting on the lock could tie up the iothread.

Quoted 12 lines

2) Queue-work-to-vcpu-context

select(tap fd ready)
tap_send
   if (trylock(TAPState->NetClientState->dev)) {
       proceed;
   } else {
       AddToDeviceWorkQueue(work);
   }
   tap_read_packet
   qemu_send_packet_async

Quoted 2 lines

DRAWBACK: lock intensive
DRAWBACK: cache effects

This almost suggests we should invert packet reception for NICs.  tap fd ready should add a work item and the guest device calls into net/tap.c to pull out any packets from the work function:

tap_send
   dev_add_work(work);

virtio_net_rx_work_fn
   while ((req = virtqueue_pop(rx_queue))) {
       if (!net_receive_packet(netclient, req->buf)) { break; /* no more packets available */ }
       /* ...mark req complete and push onto virtqueue... */ }
   virtqueue_notify(rx_queue);

Quoted 3 lines

The idea is to avoid copies on the rx path and make the locking simple by always deferring to a work function (which can be invoked immediately if the device isn't locked).

I just realized this approach bypasses net/queue.c. I think it works really well for the peer model (no "VLANs"). Basically flow control is dictated by the peer (receiver) and we don't need to use NetQueue anymore. Whether this works elegantly for slirp and other backends, I'm not sure yet.

Stefan

Quoted 66 lines · Stefan Hajnoczi

Quoted 61 lines · Stefan Hajnoczi

Quoted 19 lines · Marcelo Tosatti

On Sat, Jun 23, 2012 at 12:55:49AM +0200, Jan Kiszka wrote:
net.txt
--------

iothread flow
=============

1) Skip-work-if-device-locked

select(tap fd ready)
tap_send
   if (trylock(TAPState->NetClientState->dev))
       proceed;
   else
       continue; (data remains in queue)
   tap_read_packet
   qemu_send_packet_async

Quoted 3 lines

DRAWBACK: lock intensive.
DRAWBACK: VCPU threads can starve IOTHREAD (can be fixed with a scheme such as trylock() marks a flag saying "iothread wants lock".

One alternative is to say the lock cannot be held across system calls or other long-running operations (similar to interrupt handler spinlocks in the kernel).  But QEMU threads are still at the mercy of the scheduler or page faults, so perhaps this is not a good idea because waiting on the lock could tie up the iothread.

Quoted 12 lines

2) Queue-work-to-vcpu-context

select(tap fd ready)
tap_send
   if (trylock(TAPState->NetClientState->dev)) {
       proceed;
   } else {
       AddToDeviceWorkQueue(work);
   }
   tap_read_packet
   qemu_send_packet_async

Quoted 2 lines

DRAWBACK: lock intensive
DRAWBACK: cache effects

This almost suggests we should invert packet reception for NICs.  tap fd ready should add a work item and the guest device calls into net/tap.c to pull out any packets from the work function:

tap_send
   dev_add_work(work);

virtio_net_rx_work_fn
   while ((req = virtqueue_pop(rx_queue))) {
       if (!net_receive_packet(netclient, req->buf)) { break; /* no more packets available */ }
       /* ...mark req complete and push onto virtqueue... */ }
   virtqueue_notify(rx_queue);

Quoted 3 lines

The idea is to avoid copies on the rx path and make the locking simple by always deferring to a work function (which can be invoked immediately if the device isn't locked).

Quoted 7 lines

I just realized this approach bypasses net/queue.c.  I think it works really well for the peer model (no "VLANs").  Basically flow control is dictated by the peer (receiver) and we don't need to use NetQueue anymore.  Whether this works elegantly for slirp and other backends, I'm not sure yet.

Stefan

Right. The advantage is only backends where performance matters need to be converted (and only net hw drivers that matter performance wise need to be converted).

Apparently you guys have very different ideas on the higher level model here. It would be good to agree on one structure (including modifications on the one above) to follow and share the work on that direction.

1.  read_lock(memmap_lock)
2.  MemoryRegionSection mrs = lookup(addr)
3.  qom_ref(mrs.mr->dev)
4.  read_unlock(memmap_lock)

5.  mutex_lock(dev->lock)
6.  dispatch(&mrs, addr, data, size)
7.  mutex_unlock(dev->lock)

8. qom_unref(mrs.mr->object)

The following plan would allow progressive convertion to parallel operation.

Jan mentioned the MMIO handler->MMIO handler deadlock in a private message.

Jan: if there is recursive MMIO accesses, you can detect that and skip such MMIO handlers in dev_can_use_lock() ? Or blacklist.

Quoted 157 lines · Jan Kiszka

Should have declared this [RFC] in the subject and CC'ed kvm...

Quoted 154 lines · Jan Kiszka

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

kvm-all.c +16 -2

index f8e4328..9c3e26f 100644
@@ -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);

target-i386/kvm.c +7 -0

index 0d0d8f6..0ad64d1 100644
@@ -1631,7 +1631,10 @@ void kvm_arch_pre_run(CPUX86State *env, struct kvm_run *run)
 
      /* Inject NMI */
      if (env->interrupt_request&  CPU_INTERRUPT_NMI) {
+        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();
      }
  }
 

target-ppc/kvm.c +4 -0

index c09cc39..60d91a5 100644
@@ -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();
  }
 

Quoted 2 lines

target-ppc/kvm.c void kvm_arch_pre_run(CPUPPCState *env, struct kvm_run *run) · line 506

Quoted 2 lines

target-ppc/kvm.c +0 -0

  void kvm_arch_post_run(CPUPPCState *env, struct kvm_run *run)

The following plan would allow progressive convertion to parallel operation.

Jan mentioned the MMIO handler->MMIO handler deadlock in a private message.

Jan: if there is recursive MMIO accesses, you can detect that and skip such MMIO handlers in dev_can_use_lock() ? Or blacklist.

What i mentioned earlier about "unsolved issues" are the possibilities in "iothread flow" (belief is that it would be better determined at with execution experience / tuning).

OK, so, each document starts with xyz.txt. This is compatible with TCG, the critical part is not dropping/acquire locks for decent performance (that is, only drop memmap_lock from TCG VCPUS thread if IOTHREAD requests so).

This came out from discussions with Avi.

immediate-plan.txt
------------------

1.  read_lock(memmap_lock)
2.  MemoryRegionSection mrs = lookup(addr)
3.  qom_ref(mrs.mr->dev)
4.  read_unlock(memmap_lock)

5.  mutex_lock(dev->lock)
6.  dispatch(&mrs, addr, data, size)
7.  mutex_unlock(dev->lock)

8. qom_unref(mrs.mr->object)

A) Add Device object to MemoryRegions.

memory_region_add_subregion
memory_region_add_eventfd
memory_region_add_subregion_overlap

B) qom_ref details

Z) see device-hotplug.txt items

lock-model.txt
--------------

lock_device(dev) {
     if (dev_can_use_lock(dev))
         mutex_lock(dev->lock)
     else
         mutex_lock(qemu_global_mutex)
}

dev_can_use_lock(dev)
{
if (all services used by dev mmio handler have)
- qemu_global_mutex
- trylock(dev->lock) and fail

then yes
else
then no
}

That is, from the moment in which the device uses the order (dev->lock -> qemu_global_mutex), the qemu_global_mutex -> dev->lock is forbidden by the IOTHREAD or anyone else.

convert-to-unlocked.txt
-----------------------

1.  read_lock(memmap_lock)
2.  MemoryRegionSection mrs = lookup(addr)
3.  qom_ref(mrs.mr->dev)
4.  read_unlock(memmap_lock)

5.  mutex_lock(dev->lock)
6.  dispatch(&mrs, addr, data, size)
7.  mutex_unlock(dev->lock)

8. qom_unref(mrs.mr->object)

THE ITEMS BELOW SHOULD BE DOCUMENTATION TO CONVERT DEVICE TO UNLOCKED OPERATION.

1) qom_ref guarantees that the Device object does not disappear. However, these operations are allowed in parallel:

a) after 4. device memory and ioports might change or be unregistered.

Concurrent accesses of a device that is being hotunplugged or whose mappings change are responsability of the OS, so incorrect emulation is not QEMU's problem. However, device emulation is now subject to:
- cpu_physical_memory_map failing.
- stl_phys* failing.
- cpu_physical_memory_rw failing/reading from unassigned mem.

b) what should happen with a pending cpu_physical_memory_map pointer, when deleting the corresponding memory region?

net.txt
--------

iothread flow
=============

1) Skip-work-if-device-locked

select(tap fd ready)
tap_send

     if (trylock(TAPState->NetClientState->dev))
         proceed;
     else
         continue; (data remains in queue)
     tap_read_packet
     qemu_send_packet_async

DRAWBACK: lock intensive.
DRAWBACK: VCPU threads can starve IOTHREAD (can be fixed with a scheme such as trylock() marks a flag saying "iothread wants lock".

Checking each subsystem for possibility:

- tap_send: OK
- user,slirp: if not possible, use device->lock.
- qemu-char: OK
- interrupts: lock with qemu_global_mutex.
- qemutimer:
- read time: qemu_get_clock_ns (see later).

2) Queue-work-to-vcpu-context

select(tap fd ready)
tap_send

     if (trylock(TAPState->NetClientState->dev)) {
         proceed;
     } else {
         AddToDeviceWorkQueue(work);
     }
     tap_read_packet
     qemu_send_packet_async

DRAWBACK: lock intensive
DRAWBACK: cache effects

3) VCPU-thread-notify-device-unlocked

select(tap fd ready)
tap_send

     if (trylock(TAPState->NetClientState->dev)) {
         proceed;
     } else {
         signal VCPU thread to notify IOTHREAD when unlocked.
     }
     tap_read_packet

GENERAL OBJECTIVE: to avoid lock inversion. IOTHREAD should follow same order.

PCI HOTPLUG EJECT FLOW
----------------------

1) Monitor initiated.

monitor -> qmp_device_del -> qdev_unplug -> pci_unplug_device -> piix4_device_hotplug -> SCI interrupt -> OS removes application users from device -> OS runs _EJ0 ->

For hot removal, the device must be immediately ejected when OSPM calls the _EJ0 control method. The _EJ0 control method does not return until ejection is complete. After calling _EJ0, OSPM verifies the device no longer exists to determine if the eject succeeded. For _HID devices, OSPM evaluates the _STA method. For _ADR devices, OSPM checks with the bus driver for that device.

Avi: A pci eject guarantees that all accesses started after the eject completes do not see the device.

Can you do:

- qobject_remove()

and have the device dispatch running safely? NO, therefore pci_unplug_device must hold dev->lock.

Quoted 6 lines

1.  read_lock(memmap_lock)
2.  MemoryRegionSection mrs = lookup(addr)
3.  qom_ref(mrs.mr->dev)
4.  read_unlock(memmap_lock)

Quoted 3 lines

5.  mutex_lock(dev->lock)
6.  dispatch(&mrs, addr, data, size)
7.  mutex_unlock(dev->lock)

Just a detail, I don't think we should acquire a device specific lock in global code. Rather, I think we should acquire the global lock before dispatch unless a MemoryRegion is marked as being unlocked.

Quoted 188 lines · Marcelo Tosatti

Quoted 157 lines · Jan Kiszka

Should have declared this [RFC] in the subject and CC'ed kvm...

Quoted 154 lines · Jan Kiszka

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

kvm-all.c +16 -2

index f8e4328..9c3e26f 100644
@@ -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);

target-i386/kvm.c +7 -0

index 0d0d8f6..0ad64d1 100644
@@ -1631,7 +1631,10 @@ void kvm_arch_pre_run(CPUX86State *env, struct kvm_run *run)
 
     /* Inject NMI */
     if (env->interrupt_request&  CPU_INTERRUPT_NMI) {
+        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();
     }
 }
 

target-ppc/kvm.c +4 -0

index c09cc39..60d91a5 100644
@@ -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();
 }
 

Quoted 2 lines

target-ppc/kvm.c void kvm_arch_pre_run(CPUPPCState *env, struct kvm_run *run) · line 506

Quoted 2 lines

target-ppc/kvm.c +0 -0

 void kvm_arch_post_run(CPUPPCState *env, struct kvm_run *run)

The following plan would allow progressive convertion to parallel operation.

Jan mentioned the MMIO handler->MMIO handler deadlock in a private message.

Jan: if there is recursive MMIO accesses, you can detect that and skip such MMIO handlers in dev_can_use_lock() ? Or blacklist.

What i mentioned earlier about "unsolved issues" are the possibilities in "iothread flow" (belief is that it would be better determined at with execution experience / tuning).

OK, so, each document starts with xyz.txt. This is compatible with TCG, the critical part is not dropping/acquire locks for decent performance (that is, only drop memmap_lock from TCG VCPUS thread if IOTHREAD requests so).

This came out from discussions with Avi.

immediate-plan.txt
------------------

1.  read_lock(memmap_lock)
2.  MemoryRegionSection mrs = lookup(addr)
3.  qom_ref(mrs.mr->dev)
4.  read_unlock(memmap_lock)

Quoted 3 lines

5.  mutex_lock(dev->lock)
6.  dispatch(&mrs, addr, data, size)
7.  mutex_unlock(dev->lock)

Just a detail, I don't think we should acquire a device specific lock in global code.

Rather, I think we should acquire the global
lock before dispatch unless a MemoryRegion is marked as being unlocked.

Quoted 6 lines

1.  read_lock(memmap_lock)
2.  MemoryRegionSection mrs = lookup(addr)
3.  qom_ref(mrs.mr->dev)
4.  read_unlock(memmap_lock)

Quoted 3 lines

5.  mutex_lock(dev->lock)
6.  dispatch(&mrs, addr, data, size)
7.  mutex_unlock(dev->lock)

Just a detail, I don't think we should acquire a device specific lock in global code. Rather, I think we should acquire the global lock before dispatch unless a MemoryRegion is marked as being unlocked.

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

kvm-all.c +16 -2

index f8e4328..9c3e26f 100644
@@ -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);

target-i386/kvm.c +0 -0

index 0d0d8f6..0ad64d1 100644

target-i386/kvm.c +7 -0

+        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();
      }
  }
 

target-ppc/kvm.c +4 -0

index c09cc39..60d91a5 100644
@@ -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)

Quoted 108 lines · Jan Kiszka

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

kvm-all.c +3 -2

index f8e4328..9c3e26f 100644
@@ -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

kvm-all.c +13 -0

+         * 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);

target-i386/kvm.c +0 -0

index 0d0d8f6..0ad64d1 100644
@@ -1631,7 +1631,10 @@ void kvm_arch_pre_run(CPUX86State *env, struct
 kvm_run *run)
 

target-i386/kvm.c void kvm_arch_pre_run(CPUX86State *env, struct · line 1634

Quoted 2 lines

target-i386/kvm.c +0 -0

      /* 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.