irqdevice INTR example

Hi All,
Attached are the first three patches in my queue. The first two you are likely familiar with at this point (though I have made some more of the requested changes to 02-irqdevice.patch). The last item (03-preemptible-cpu.patch) adds an implementation to the previously unused kvm_vcpu_intr() callback. This acts as a functional example of the INTR callback mechanism as Avi requested. Note that the work related to IF/NMI/TPR classification of interrupts happens later in my queue and is not mature enough to share yet, but hopefully soon.

KVM: Preemptible VCPU

From: <>

This adds support for interrupting an executing CPU

drivers/kvm/condvar.c new file +2 -0

new file mode 100644
index 0000000..87e464a
@@ -0,0 +1,109 @@
+/*
+ * Condition Variable

drivers/kvm/condvar.c new file +1 -3

+
 	/*
-	 * FIXME: Implement this or the CPU wont notice the interrupt until
-	 * the next natural VMEXIT.  Note that this is how the system
-	 * has always worked, so nothing is broken here.  This is a future

drivers/kvm/condvar.c new file +5 -0

+	 *
+	 * This condvar/spinlock/timeout/retry eliminate the race in a safe
+	 * manner, at the expense of making the INTR delivery synchronous
 	 */
+	spin_lock(&vcpu->irq.lock);
+	

If you want condition variables, activate your cryogenically-cooled suit and post on it on lkml. This cannot be added to the kernel via kvm.

Quoted 1 line

Quoted 1 line

On Thu, Apr 12, 2007 at 4:02 AM, in message <461DE791.1040707-atKUWr5tajBWk0Htik3J/[email protected]>,

Quoted 2 lines · Gregory Haskins

Hi All,
Attached are the first three patches in my queue. The first two you are

likely familiar with at this point (though I have made some more of the requested changes to 02- irqdevice.patch). The last item
(03- preemptible- cpu.patch) adds an implementation to the previously unused kvm_vcpu_intr() callback. This acts as a functional example of the INTR callback mechanism as Avi requested. Note that the work related to IF/NMI/TPR classification of interrupts happens later in my queue and is not mature enough to share yet, but hopefully soon.

Quoted 22 lines

KVM: Preemptible VCPU

From: <>

This adds support for interrupting an executing CPU

diff -- git a/drivers/kvm/condvar.c b/drivers/kvm/condvar.c new file mode 100644
index 0000000..87e464a

drivers/kvm/condvar.c renamed from dev/null +0 -0

@@ - 0,0 +1,109 @@
+/*
+ * Condition Variable
+ *
+ * Copyright (C) 2007, Novell
+ *
+ * Authors:
+ * Gregory Haskins <[email protected]>
+ *

Quoted 5 lines

+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top- level directory.
+ *
+ */

If you want condition variables, activate your cryogenically- cooled suit and post on it on lkml. This cannot be added to the kernel via kvm.

Quoted 13 lines

drivers/kvm/condvar.c renamed from dev/null +5 -4

+
 	/*
-	 * FIXME: Implement this or the CPU wont notice the interrupt until
-	 * the next natural VMEXIT.  Note that this is how the system
-	 * has always worked, so nothing is broken here.  This is a future
-	 * enhancement
+	 * HACK ALERT!
+	 *
+	 * We want to send a virtual interrupt signal to the task that owns
+	 * the guest.  However, the signal will only force a VMEXIT (via

drivers/kvm/condvar.c

Quoted 6 lines

drivers/kvm/condvar.c renamed from dev/null +5 -0

+	 * a reschedule IPI) if the task is currently in GUEST mode.  There
+	 * is a race condition between the time that we mark the vcpu as
+	 * running and the time the system actually enter guest mode.  Since
+	 * there doesnt appear to be any way to help with this situation from
+	 * the VT hardware, we are forced to wait to make sure the guest 

We support AMD too.

drivers/kvm/condvar.c

Quoted 5 lines

drivers/kvm/condvar.c renamed from dev/null +3 -0

+	 * actually gets interrupted in a reasonable amount of time.  If it
+	 * does not, we assume that the IPI failed because it was too early
+	 * and must try again until it does.

Waiting seems totally broken. I'm not even sure what we are waiting for -- certainly you can't wait for re- entry into guest mode, since there's not guarantee that's coming.

If it is elsewhere, we need to use one of the many (alas) standard wakeup mechanisms to wake up userspace and let it know the vcpu needs service.

Qemu uses signals, but I think that making the vcpu fd writable is more generic. For qemu, we can attach a signal to the fd (using fcntl(2)); that has the benefit of leaving the choice of which signal to use to userspace.

Quoted 12 lines

drivers/kvm/condvar.c renamed from dev/null +10 -0

+	 *
+	 * This condvar/spinlock/timeout/retry eliminate the race in a safe
+	 * manner, at the expense of making the INTR delivery synchronous
 	 */
+	spin_lock(&vcpu- >irq.lock);
+	
+	if (vcpu- >irq.task) {
+		struct timespec tmo = {
+			.tv_sec  = 0,
+			.tv_nsec = 100000 /* 100us */
+		};

drivers/kvm/condvar.c

Quoted 6 lines

drivers/kvm/condvar.c renamed from dev/null +5 -0

+
+		BUG_ON(vcpu- >irq.task == current);
+			
+		while (vcpu- >irq.task) {
+			send_sig(SIGSTOP, vcpu- >irq.task, 0);

SIGSTOP?! I lost the plot here.

Quoted 5 lines

Quoted 2 lines

If you want condition variables, activate your cryogenically- cooled suit and post on it on lkml. This cannot be added to the kernel via kvm.

Hehe. I wasn't aware of the contention surrounding them but that does explain why they are notably missing. I will rip it out this code in favor of a pure waitqueue as Christof suggested to avoid more controversy.

Quoted 12 lines

Quoted 12 lines

drivers/kvm/kvm_main.c +5 -4

+
 	/*
-	 * FIXME: Implement this or the CPU wont notice the interrupt until
-	 * the next natural VMEXIT.  Note that this is how the system
-	 * has always worked, so nothing is broken here.  This is a future
-	 * enhancement
+	 * HACK ALERT!
+	 *
+	 * We want to send a virtual interrupt signal to the task that owns
+	 * the guest.  However, the signal will only force a VMEXIT (via

Quoted 9 lines

drivers/kvm/kvm_main.c static void kvm_vcpu_intr(struct kvm_irqsink *this, · line 2339

Quoted 7 lines

drivers/kvm/kvm_main.c +5 -0

+	 * a reschedule IPI) if the task is currently in GUEST mode.  There
+	 * is a race condition between the time that we mark the vcpu as
+	 * running and the time the system actually enter guest mode.  Since
+	 * there doesnt appear to be any way to help with this situation from
+	 * the VT hardware, we are forced to wait to make sure the guest 

We support AMD too.

My patch actually supports both, and I didn't mean to infer that AMD was skipped. I was speaking generally when I said "VT". Is there a better term to refer to the concept of VMX/SVN?

Quoted 10 lines

drivers/kvm/kvm_main.c static void kvm_vcpu_intr(struct kvm_irqsink *this, · line 2342

Quoted 6 lines

drivers/kvm/kvm_main.c +3 -0

+	 * actually gets interrupted in a reasonable amount of time.  If it
+	 * does not, we assume that the IPI failed because it was too early
+	 * and must try again until it does.

Waiting seems totally broken. I'm not even sure what we are waiting for -- certainly you can't wait for re- entry into guest mode, since there's not guarantee that's coming.

Actually, there is ;) (unless I missed something) If you look at the other side of this implementation in vcpu_XXX_run(), I only set the vcpu->irq.task when the vcpu is just about to enter guest mode. And I clear it when it leaves. In other words, the kvn_vcpu_intr() function is a no-op if the vcpu is not in guest mode (or close to it).

Quoted 3 lines

If it is elsewhere, we need to use one of the many (alas) standard wakeup mechanisms to wake up userspace and let it know the vcpu needs service.

My code assumes we only need to kick the vcpu if its in guest mode because it figures it will just pick up the interrupt on the next run cycle otherwise. I haven't studied QEMU deep enough to know that it can suspend the execution of the run-cycle...i just thought it looped indefinitely.

This makes sense now that I think about it because something like hlt should cause a suspension of CPU activity until further notice.

Quoted 12 lines

Quoted 12 lines

drivers/kvm/kvm_main.c +10 -0

+	 *
+	 * This condvar/spinlock/timeout/retry eliminate the race in a safe
+	 * manner, at the expense of making the INTR delivery synchronous
 	 */
+	spin_lock(&vcpu- >irq.lock);
+	
+	if (vcpu- >irq.task) {
+		struct timespec tmo = {
+			.tv_sec  = 0,
+			.tv_nsec = 100000 /* 100us */
+		};

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +5 -0

+
+		BUG_ON(vcpu- >irq.task == current);
+			
+		while (vcpu- >irq.task) {
+			send_sig(SIGSTOP, vcpu- >irq.task, 0);

SIGSTOP?! I lost the plot here.

Well, I needed a signal to send that would reliably IPI the guest. I originally wrote the code to send RT signal 33 (which I defined as KVM_SIGNAL_VIRTUAL_INTERRUPT). In both cases, the design calls for sending a signal (purely for the IPI function of the kernel) and then to remove the signal before userspace sees it. I had some trouble with the code that used dequeue_signal() to strip out the RT signal. So I then changed to sending SIGSTOP to IPI, and SIGCONT to effectively strip out the pending SIGSTOP.

Keep in mind that this is really just proof-of-concept code. The general idea of this POC is to demonstrate how I envisioned the irqdevice API to be used to deliver interrupts even once concurrent access is possible (e.g., SMP/PV). The details of which signal are used, etc, are incidental for now until we agree that the irqdevice API approach is acceptable.

Quoted 1 line

Quoted 1 line

On Thu, Apr 12, 2007 at 8:49 AM, in message <461E2AD5.7070905-atKUWr5tajBWk0Htik3J/[email protected]>,

Actually, I am in favor of having well- defined synchronization primitives. The only issues I see with condition variables are:

- there are many variants of mutexes in the kernel (mutex, spinlock, spinlock with irqs disabled, ...), so many condvar variants are needed.

- they're very close to waitqueues, so perhaps there's not much justification

I'm no synchronization primitive expert, though. My comment had nothing to do with my opinion on condition variables or your implementation thereof. kvm is simply not the place to introduce them.

I referred to the comment. Maybe just "the hardware"?

Ah, ok -- I misunderstood the whole thing. The way to avoid the race is to disable interrupts before entering the guest. This way the IPI is delayed until you enter guest mode:

    irq_disable();

    spin_lock();
    vcpu- >guest_mode = 1;
    check_whether_an_irq_is_pending_and_if_so_inject_it();
    spin_unlock();

    asm ("vm_entry_sequence");

    vcpu- >guest_mode = 0;
   
    irq_enable(); // currently in the vm entry sequence

    // recheck here?

If the interrupt happens before the spin_lock(), we'll get a non- ipi wakeup and then see it in check_whether(). If it happens after it we'll get an IPI which will be ignored until we're snugly in guest mode.

In general I find it useful to pretend there are many userspaces being written for kvm, otherwise we get locked into qemu's current mode of operation.

Quoted 2 lines

This makes sense now that I think about it because something like hlt should

cause a suspension of CPU activity until further notice.

An alternative is to handle hlt in the kernel in a place where we're ready for the IPI wakeup. The downside to that is that we have to be prepared for external wakeup sources as well (signal, poll, aio... messy).

It's best not to use signals internally. Qemu relies on them and we have to support them, but in kernel we should use existing kernel mechanisms.

I was interested in how - >pending() and - >read_vector() and the raise callback interact, but got distracted by the, err, uniqueness of the signal thing.

Quoted 18 lines

Ah, ok -- I misunderstood the whole thing. The way to avoid the race is to disable interrupts before entering the guest. This way the IPI is delayed until you enter guest mode:

    irq_disable();

    spin_lock();
    vcpu- >guest_mode = 1;
    check_whether_an_irq_is_pending_and_if_so_inject_it();
    spin_unlock();

    asm ("vm_entry_sequence");

    vcpu- >guest_mode = 0;
   
    irq_enable(); // currently in the vm entry sequence

Quoted 6 lines

    // recheck here?

If the interrupt happens before the spin_lock(), we'll get a non- ipi wakeup and then see it in check_whether(). If it happens after it we'll get an IPI which will be ignored until we're snugly in guest mode.

When I first read this I thought "whoa! you want to disable interrupts during the whole time we are in GUEST mode?" But then it dawned on me what you are suggesting: Interrupts would be re-enabled after the context switch because we re-load the processor state? Cool! The thing I can't wrap my head around is what happens when the guest has IF=0 and and external interrupt comes in? Would we still exit?

But that aside, a problem that I see is that (IIUC) IPIs use NMIs not EXT-INTs. Assuming that is right, I suppose we might be able to do a similar trick except we also disable NMIs first (is this possible/recommended/forbidden?).

Quoted 4 lines

It's best not to use signals internally. Qemu relies on them and we have to support them, but in kernel we should use existing kernel mechanisms.

But Avi, this was *your* idea to use signals ;) But in all seriousness, I don't know if I have a choice. I have two requirements which constrain me:

1) I need an IPI to cause a VMEXIT

2) I need to support 2.6.16, strongly preferable as a loadable module.

According to my research (which is undoubtedly not 100% definitive), 2.6.16 or even the newer kernels do not export most of the IPI facilities. send_sig() happens to be exported and it happens to invoke a reschedule_IPI under the hood, so its convenient. If there is another way to get access to the IPI facility without playing games with signals, I am all ears. But until then I don't know what else to do. If I had the luxury of modifying the kernel source, we could just export what we needed and be done with it.

Quoted 4 lines

I was interested in how - >pending() and - >read_vector() and the raise callback interact, but got distracted by the, err, uniqueness of the signal thing.

You still haven't weighed in here ;) Hopefully you have a clearer picture of what I was trying to do now at least. Whether you agree or not is another matter.

Quoted 1 line

Quoted 1 line

On Thu, Apr 12, 2007 at 10:14 AM, in message <461E3EDB.3080002-atKUWr5tajBWk0Htik3J/[email protected]>,

It's really subtle.

With respect to interrupts, VT^H^Hthe hardware provides an override over IF. If an interrupt happens while this override is enabled, we exit guest mode regardless of the state of IF. In effect interrupts are enabled but _delivery_ is disabled. Once we exit guest mode, interrupts become disabled again until we set IF explicitly.

Quoted 1 line

Quoted 1 line

On Thu, Apr 12, 2007 at 12:01 PM, in message <[email protected]>,

Quoted 1 line

Quoted 1 line

Quoted 1 line

On Thu, Apr 12, 2007 at 10:14 AM, in message <461E3EDB.3080002-atKUWr5tajBWk0Htik3J/[email protected]>,

Quoted 8 lines · Avi Kivity

It's really subtle.

With respect to interrupts, VT^H^Hthe hardware provides an override over IF. If an interrupt happens while this override is enabled, we exit guest mode regardless of the state of IF. In effect interrupts are enabled but _delivery_ is disabled. Once we exit guest mode, interrupts become disabled again until we set IF explicitly.

Heres a version that implements that idea. Note that its still a POC, as the kick_process still needs to be exported and handled in extern-module-compat (or whatever its called)

Quoted 8 lines

It's really subtle.

With respect to interrupts, VT^H^Hthe hardware provides an override over IF. If an interrupt happens while this override is enabled, we exit guest mode regardless of the state of IF. In effect interrupts are enabled but _delivery_ is disabled. Once we exit guest mode, interrupts become disabled again until we set IF explicitly.

Heres a version that implements that idea. Note that its still a POC, as the kick_process still needs to be exported and handled in extern-module-compat (or whatever its called)

drivers/kvm/kvm_main.c +4 -0

@@ -1866,6 +1874,19 @@ static int kvm_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 		kvm_arch_ops->decache_regs(vcpu);
 	}
 
+	/*
+	 * Make sure the current task is accurately recorded.  Most of the 
+	 * time, it will be so we first check if its necessary without taking
+	 * the lock.  If there is a mismatch, we must aquire the lock and 

drivers/kvm/svm.c +11 -0

+	 */
+	local_irq_save(irq_flags);
+
+	spin_lock(&vcpu->irq.lock);
+
+	/*
+	 * There are optimizations we can make when signaling interrupts
+	 * if we know the VCPU is in GUEST mode, so mark that here
+	 */
+	vcpu->irq.guest_mode = 1;
+

drivers/kvm/svm.c +0 -0

@@ -1617,6 +1650,16 @@ again:
 	reload_tss(vcpu);
 
 	/*

drivers/kvm/vmx.c +3 -0

 	asm ("mov %0, %%ds; mov %0, %%es" : : "r"(__USER_DS));
 
+	/*
+	 * Signal that we have transitioned back to host mode 
+	 */

Quoted 1 line

Quoted 1 line

On Sat, Apr 14, 2007 at 10:30 AM, in message <4620E56A.7040207-atKUWr5tajBWk0Htik3J/[email protected]>,

Quoted 9 lines · Gregory Haskins

Quoted 8 lines

It's really subtle.

With respect to interrupts, VT^H^Hthe hardware provides an override over IF. If an interrupt happens while this override is enabled, we exit guest mode regardless of the state of IF. In effect interrupts are enabled but _delivery_ is disabled. Once we exit guest mode, interrupts become disabled again until we set IF explicitly.

Quoted 1 line

Heres a version that implements that idea. Note that its still a POC, as

the kick_process still needs to be exported and handled in extern-module-compat (or whatever its called)

Quoted 1 line

Hopefully Ingo will ack such a patch, as he suggested kick_process() originally.

Review static int kvm_vcpu_ioctl_run(struct kvm_vcpu *vcpu,

Quoted 3 lines

(quoted diff) +0 -0

@@ -1866,6 +1874,19 @@ static int kvm_vcpu_ioctl_run(struct kvm_vcpu *vcpu, 

struct kvm_run *kvm_run)

Quoted 7 lines

drivers/kvm/kvm_main.c +4 -0

 		kvm_arch_ops->decache_regs(vcpu);
 	}
 
+	/*
+	 * Make sure the current task is accurately recorded.  Most of the 
+	 * time, it will be so we first check if its necessary without taking
+	 * the lock.  If there is a mismatch, we must aquire the lock and 

drivers/kvm/kvm_main.c static int kvm_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) · line 1891

Quoted 10 lines

drivers/kvm/kvm_main.c +9 -0

+	 * check again to eliminate races
+	 */
+	if (unlikely(vcpu->irq.task != current)) {
+		spin_lock(&vcpu->irq.lock);
+		if (vcpu->irq.task != current)
+			vcpu->irq.task = current;
+		spin_unlock(&vcpu->irq.lock);
+	}
+
 

Isn't this equivalent to

    vcpu->irq.task = current

?

btw, these double-check things are broken, see
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html.
It will usually work on x86, and can be made to work elsewhere with memory barriers, but it's less simple than it appears.

stgi() / clgi() is what implements interrupt handling under svm. I think you got this right, but please tell me you read the relevant sections in the manual a few times.

Quoted 5 lines

drivers/kvm/svm.c +0 -0

@@ -1617,6 +1650,16 @@ again:
 	reload_tss(vcpu);
 

drivers/kvm/svm.c again:

Quoted 11 lines

drivers/kvm/svm.c +9 -0

 	/*
+	 * Signal that we have transitioned back to host mode 
+	 */
+	spin_lock(&vcpu->irq.lock);
+
+	vcpu->irq.guest_mode = 0;
+	vcpu->irq.pending = 0;
+	
+	spin_unlock(&vcpu->irq.lock);
+
 

If an interrupt happened just before here, then we need to check it, otherwise it's lost. Also an opportunity to see ->read_vector in action, which was the purpose of the exercise IIRC.

Quoted 8 lines

drivers/kvm/vmx.c +3 -0

 	asm ("mov %0, %%ds; mov %0, %%es" : : "r"(__USER_DS));
 
+	/*
+	 * Signal that we have transitioned back to host mode 
+	 */

drivers/kvm/vmx.c again: · line 1959

Quoted 10 lines

drivers/kvm/vmx.c +7 -0

+	spin_lock(&vcpu->irq.lock);
+
+	vcpu->irq.guest_mode = 0;
+	vcpu->irq.pending = 0;
+
+	spin_unlock(&vcpu->irq.lock);
+
 	if (fail) {
 		kvm_run->exit_reason = KVM_EXIT_FAIL_ENTRY;
 		kvm_run->fail_entry.hardware_entry_failure_reason

Ditto.

You also want spin_lock_irq() when interrupts are enabled, since some callers will lock it in interrupt context.

Quoted 1 line

Quoted 1 line

On Sun, Apr 15, 2007 at 6:32 PM, in message <[email protected]>,

However, you do highlight a bug in this code that could cause the interrupt to be deferred indefinitely until a second interrupt came in if the processor happens to halt after this VMEXIT. I.e., since a signal isn't being delivered while in irq.guest_mode = 1, we do need to handle this at this layer by looping back into the CPU.

Out of curiosity, did you review my kick_process() version or the newer smp_call_function_single() based version of the patch?

Quoted 25 lines

Review static int kvm_vcpu_ioctl_run(struct kvm_vcpu *vcpu, · line 1874

Quoted 3 lines

(quoted diff) +0 -0

@@ -1866,6 +1874,19 @@ static int kvm_vcpu_ioctl_run(struct kvm_vcpu *vcpu, 
 

struct kvm_run *kvm_run)

Quoted 8 lines

drivers/kvm/kvm_main.c +5 -0

 		kvm_arch_ops->decache_regs(vcpu);
 	}
 
+	/*
+	 * Make sure the current task is accurately recorded.  Most of the 
+	 * time, it will be so we first check if its necessary without taking
+	 * the lock.  If there is a mismatch, we must aquire the lock and 
+	 * check again to eliminate races

drivers/kvm/kvm_main.c static int kvm_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)

Quoted 10 lines

drivers/kvm/kvm_main.c +8 -0

+	 */
+	if (unlikely(vcpu->irq.task != current)) {
+		spin_lock(&vcpu->irq.lock);
+		if (vcpu->irq.task != current)
+			vcpu->irq.task = current;
+		spin_unlock(&vcpu->irq.lock);
+	}
+
 
 

Isn't this equivalent to

Quoted 2 lines

    vcpu->irq.task = current

Yeah, good point. I originally had a printk in there to report when it changes, thus the conditional. I didn't collapse it when I took the printk out. I suppose you could argue that updating the variable indiscriminately will unnecessarily dirty a cache-line, but I doubt it would matter in the grand scheme of things.