Add irqdevice interface + userint implementation

Quoted 1 line

Quoted 1 line

On Thu, Apr 5, 2007 at 2:53 PM, in message <[email protected]>,

Hi All,
The following is another patch that I broke out of the in-kernel-apic
patch that I sent earlier. This focuses purely on the irqdevice abstraction without introducing the concept of the in-kernel LAPIC. It also provides an implementation of a "userint" model, which should support a system with a QEMU based emulation of the (A)PIC (e.g. current behavior). I have cleaned up quite a few things based on all of your comments as well, so hopefully its a little more polished than last time.

As far as testing, I have confirmed that I can boot a 64-bit linux guest with no discernable difference in behavior.

Note that this patch applies after the in-kernel-mmio.patch that I sent earlier today which has not yet been accepted/commited. This patch should not depend on it. However, if you do have problems applying it let me know and I can generate another one that is not after in-kernel-mmio in the series.

Comments please!

-Greg

--
KVM: Add irqdevice interface + userint implementation

The current code is geared towards using a user-mode (A)PIC. This patch

adds an "irqdevice" abstraction, and implements a "userint" model to
handle the
duties of the original code. Later, we can develop other irqdevice
models
to handle objects like LAPIC, IOAPIC, i8259, etc, as appropriate

Signed-off-by Gregory Haskins <[email protected]>

---

drivers/kvm/Makefile +1 -1

index c0a789f..beb6f51 100644
@@ -2,7 +2,7 @@
 # Makefile for Kernel-based Virtual Machine module
 #
 
-kvm-objs := kvm_main.o mmu.o x86_emulate.o
+kvm-objs := kvm_main.o mmu.o x86_emulate.o kvm_userint.o
 obj-$(CONFIG_KVM) += kvm.o
 kvm-intel-objs = vmx.o
 obj-$(CONFIG_KVM_INTEL) += kvm-intel.o

drivers/kvm/kvm.h +3 -0

index c1923df..6caf60b 100644
@@ -13,6 +13,7 @@
 #include <linux/mm.h>
 
 #include "vmx.h"
+#include "kvm_irqdevice.h"
 #include <linux/kvm.h>
 #include <linux/kvm_para.h>
 

@@ -157,6 +158,8 @@ struct vmcs {
 
 struct kvm_vcpu;
 
+int kvm_userint_init(struct kvm_irqdevice *dev);
+
 /*
  * x86 supports 3 paging modes (4-level 64-bit, 3-level 64-bit, and 2-level
  * 32-bit).  The kvm_mmu structure abstracts the details of the current mmu

@@ -286,6 +289,8 @@ static inline void kvm_io_bus_register_dev(struct 

kvm_io_bus *bus, struct kvm_io

drivers/kvm/kvm.h +3 -3

 	bus->devs[bus->dev_count++] = dev;
 }
 
+#define NR_IRQ_WORDS KVM_IRQ_BITMAP_SIZE(unsigned long)
+
 struct kvm_vcpu {
 	struct kvm *kvm;
 	union {

@@ -298,9 +303,7 @@ struct kvm_vcpu {
 	u64 host_tsc;
 	struct kvm_run *run;
 	int interrupt_window_open;
-	unsigned long irq_summary; /* bit vector: 1 per word in irq_pending */
-#define NR_IRQ_WORDS KVM_IRQ_BITMAP_SIZE(unsigned long)
-	unsigned long irq_pending[NR_IRQ_WORDS];
+	struct kvm_irqdevice irq_dev;
 	unsigned long regs[NR_VCPU_REGS]; /* for rsp: vcpu_load_rsp_rip() */
 	unsigned long rip;      /* needs vcpu_load_rsp_rip() */
 

drivers/kvm/kvm_irqdevice.h new file +8 -0

new file mode 100644
index 0000000..9c91d15
@@ -0,0 +1,206 @@
+/*
+ * kvm_irqdevice.h
+ *
+ * Defines an interface for an abstract interrupt controller.  The model 
+ * consists of a unit with an arbitrary number of input lines (IRQ0-N), an
+ * output line (INTR), and methods for completing an interrupt-acknowledge
+ * cycle (INTA).  A particular implementation of this model will define
+ * various policies, such as irq-to-vector translation, INTA/auto-EOI 

policy,
+ * etc.
+ *
+ * In addition, the INTR callback mechanism allows the unit to be "wired" to
+ * an interruptible source in a very flexible manner. For instance, an
+ * irqdevice could have its INTR wired to a VCPU (ala LAPIC), or another
+ * interrupt controller (ala cascaded i8259s)
+ *
+ * Copyright (C) 2007 Novell
+ *
+ * Authors:
+ * Gregory Haskins <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ *
+ */

drivers/kvm/kvm_irqdevice.h new file +33 -0

+
+#ifndef __KVM_IRQDEVICE_H
+#define __KVM_IRQDEVICE_H
+
+#define KVM_IRQFLAGS_NMI  (1 << 0)
+#define KVM_IRQFLAGS_PEEK (1 << 1)
+
+struct kvm_irqdevice;
+
+struct kvm_irqsink {
+	void (*raise_intr)(struct kvm_irqsink *this, 
+			   struct kvm_irqdevice *dev);
+
+	void *private;
+};
+
+struct kvm_irqdevice {
+	int  (*pending)(struct kvm_irqdevice *this, int flags);
+	int  (*read_vector)(struct kvm_irqdevice *this, int flags); 
+	int  (*set_pin)(struct kvm_irqdevice *this, int pin, int level, 
+			int flags);
+	int  (*summary)(struct kvm_irqdevice *this, void *data);
+	void (*destructor)(struct kvm_irqdevice *this);
+
+	void               *private;
+	struct kvm_irqsink  sink;
+};
+
+/*
+ * kvm_irq_init()
+ *
+ * Description:
+ *    Initialized the kvm_irqdevice for use.  Should be called before 

calling
+ * any derived implementation init functions
+ *
+ * Parameters:
+ * struct kvm_irqdevice *dev: This device
+ *
+ * Returns: (void)
+ */

drivers/kvm/kvm_irqdevice.h new file +102 -0

+static inline void kvm_irq_init(struct kvm_irqdevice *dev)
+{
+	memset(dev, 0, sizeof(*dev));
+}
+
+/*
+ * kvm_irq_pending()
+ *
+ * Description:
+ *    Efficiently determines if an interrupt is pending on an irqdevice
+ *
+ * Parameters:
+ *    struct kvm_irqdevice *dev: The device
+ *    int flags: Modifies the behavior as follows:
+ * 
+ *                   KVM_IRQFLAGS_NMI: Mask everything but NMIs
+ *
+ * Returns: (int)
+ *   -1 = failure
+ *    0 = no iterrupts pending (per "flags" criteria)
+ *   >0 = one or more interrupts are pending
+ */
+static inline int kvm_irq_pending(struct kvm_irqdevice *dev, int flags)
+{
+	return dev->pending(dev, flags);
+}
+
+/*
+ * kvm_irq_read_vector()
+ *
+ * Description:
+ *    Read the highest priority pending vector from the device, potentially
+ *    invoking auto-EOI depending on device policy
+ *
+ * Parameters:
+ *    struct kvm_irqdevice *dev: The device
+ *    int flags: Modifies the behavior as follows:
+ *
+ *                   KVM_IRQFLAGS_NMI: Mask everything but NMIs
+ *                   KVM_IRQFLAGS_PEEK: Do not auto-acknowledge interrupt
+ *
+ * Returns: (int)
+ *   -1 = no interrupts pending (per "flags" criteria)
+ *  >=0 = the highest pending vector
+ */
+static inline int kvm_irq_read_vector(struct kvm_irqdevice *dev, int flags)
+{
+	return dev->read_vector(dev, flags);
+}
+
+/*
+ * kvm_irq_set_pin()
+ *
+ * Description:
+ *    Allows the caller to assert/deassert an IRQ input pin to the device 
+ *    according to device policy.
+ *
+ * Parameters:
+ *    struct kvm_irqdevice *dev: The device
+ *    int pin: The input pin to alter
+ *    int level: The value to set (1 = assert, 0 = deassert)
+ *    int flags: Reserved, must be 0
+ *
+ * Returns: (int)
+ *   -1 = failure
+ *    0 = success
+ */
+static inline int kvm_irq_set_pin(struct kvm_irqdevice *dev, int pin, 
+				  int level, int flags)
+{
+	return dev->set_pin(dev, pin, level, flags);
+}
+
+/*
+ * kvm_irq_summary()
+ *
+ * Description:
+ *    Loads a summary bitmask of all pending vectors (0-255)
+ *
+ * Parameters:
+ *    struct kvm_irqdevice *dev: The device
+ *    void *data: A pointer to a region capable of holding a 256 bit bitmap
+ *
+ * Returns: (int)
+ *   -1 = failure
+ *    0 = success
+ */
+static inline int kvm_irq_summary(struct kvm_irqdevice *dev, void *data)
+{
+	return dev->summary(dev, data);
+}
+
+/*
+ * kvm_irq_register_sink()
+ *
+ * Description:
+ *    Registers an kvm_irqsink object as an INTR callback
+ *
+ * Parameters:
+ *    struct kvm_irqdevice *dev: The device
+ *    struct kvm_irqsink *sink: The sink to register.  Data will be copied
+ *                              so building object from transient storage 

is
+ * ok.
+ *
+ * Returns: (void)
+ */

drivers/kvm/kvm_irqdevice.h new file +26 -0

+static inline void kvm_irq_register_sink(struct kvm_irqdevice *dev, 
+					 const struct kvm_irqsink *sink)
+{
+	dev->sink = *sink;
+}
+
+/*
+ * kvm_irq_raise_intr()
+ *
+ * Description:
+ *    Invokes a registered INTR callback (if present).  This function is
+ *    meant to be used privately by a irqdevice implementation. 
+ *
+ * Parameters:
+ *    struct kvm_irqdevice *dev: The device
+ *
+ * Returns: (void)
+ */
+static inline void kvm_irq_raise_intr(struct kvm_irqdevice *dev)
+{
+	struct kvm_irqsink *sink = &dev->sink;
+	if (sink->raise_intr)
+		sink->raise_intr(sink, dev);
+}
+
+#endif /*  __KVM_IRQDEVICE_H */

drivers/kvm/kvm_main.c +13 -8

index 9ca0ad3..ac8d4f4 100644
@@ -1989,8 +1989,7 @@ static int kvm_vcpu_ioctl_get_sregs(struct kvm_vcpu 
 *vcpu,
 	sregs->efer = vcpu->shadow_efer;
 	sregs->apic_base = vcpu->apic_base;
 
-	memcpy(sregs->interrupt_bitmap, vcpu->irq_pending,
-	       sizeof sregs->interrupt_bitmap);
+	kvm_irq_summary(&vcpu->irq_dev, &sregs->interrupt_bitmap);
 
 	vcpu_put(vcpu);

@@ -2044,12 +2043,18 @@ static int kvm_vcpu_ioctl_set_sregs(struct kvm_vcpu 
 *vcpu,
 	if (mmu_reset_needed)
 		kvm_mmu_reset_context(vcpu);
 
-	memcpy(vcpu->irq_pending, sregs->interrupt_bitmap,
-	       sizeof vcpu->irq_pending);
-	vcpu->irq_summary = 0;
-	for (i = 0; i < NR_IRQ_WORDS; ++i)
-		if (vcpu->irq_pending[i])
-			__set_bit(i, &vcpu->irq_summary);
+	/* walk the interrupt-bitmap and inject an IRQ for each bit found */
+	for (i = 0; i < NR_IRQ_WORDS; ++i) {
+		unsigned long word = sregs->interrupt_bitmap[i];
+		while(word) {
+			int bit_index = __ffs(word);
+			int irq = i * BITS_PER_LONG + bit_index;
+			
+			kvm_irq_set_pin(&vcpu->irq_dev, irq, 1, 0);
+
+			clear_bit(bit_index, &word);
+		}
+	}
 
 	set_segment(vcpu, &sregs->cs, VCPU_SREG_CS);

drivers/kvm/kvm_main.c +1 -7

 	set_segment(vcpu, &sregs->ds, VCPU_SREG_DS);

@@ -2210,14 +2215,8 @@ static int kvm_vcpu_ioctl_interrupt(struct kvm_vcpu 
 *vcpu,
 {
 	if (irq->irq < 0 || irq->irq >= 256)
 		return -EINVAL;
-	vcpu_load(vcpu);
-
-	set_bit(irq->irq, vcpu->irq_pending);
-	set_bit(irq->irq / BITS_PER_LONG, &vcpu->irq_summary);
 
-	vcpu_put(vcpu);
-
-	return 0;
+	return kvm_irq_set_pin(&vcpu->irq_dev, irq->irq, 1, 0);
 }
 

static int kvm_vcpu_ioctl_debug_guest(struct kvm_vcpu *vcpu,

drivers/kvm/kvm_main.c +27 -0

@@ -2318,6 +2317,33 @@ out1:
 	return r;
 }
 
+/* This function will be invoked whenever the vcpu->irq_dev raises its INTR 
 
+ * line
+ */
+static void kvm_vcpu_intr(struct kvm_irqsink *this, 
+			  struct kvm_irqdevice *dev)
+{
+	/* Our irq device is requesting to interrupt the vcpu.  If it is
+	 * currently running, we should inject a host IPI to force a VMEXIT 
+	 */
+	
+	/* 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
+	 */
+}
+
+static void kvm_vcpu_irqsink_init(struct kvm_vcpu *vcpu)
+{
+	struct kvm_irqsink sink = {
+		.raise_intr = kvm_vcpu_intr,
+		.private    = vcpu
+	};
+	
+	kvm_irq_register_sink(&vcpu->irq_dev, &sink);
+}
+
 /*
  * Creates some virtual cpus.  Good luck creating more than one.

*/

drivers/kvm/kvm_main.c +4 -0

@@ -2364,6 +2390,10 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, 
 int n)
 	if (r < 0)
 		goto out_free_vcpus;
 
+	kvm_irq_init(&vcpu->irq_dev);
+	kvm_vcpu_irqsink_init(vcpu);
+	kvm_userint_init(&vcpu->irq_dev);
+
 	kvm_arch_ops->vcpu_load(vcpu);
 	r = kvm_mmu_setup(vcpu);

drivers/kvm/kvm_main.c +0 -0

 	if (r >= 0)

drivers/kvm/kvm_userint.c new file +4 -0

new file mode 100644
index 0000000..ded3098
@@ -0,0 +1,201 @@
+/*
+ * kvm_userint.c: User Interrupts IRQ device 
+ *
+ * This acts as an extention of an interrupt controller that exists 

elsewhere
+ * (typically in userspace/QEMU). Because this PIC is a pseudo device that
+ * is downstream from a real emulated PIC, the "IRQ-to-vector" mapping has
+ * already occured. Therefore, this PIC has the following unusal properties:
+ *
+ * 1) It has 256 "pins" which are literal vectors (e.g.no translation)
+ * 2) It only supports "auto-EOI" behavior since it is expected that the
+ * upstream emulated PIC will handle the real EOIs (if applicable)
+ * 3) It only listens to "asserts" on the pins (deasserts are dropped)
+ * because its an auto-EOI device anyway.
+ *
+ * Copyright (C) 2007 Novell
+ *
+ * bitarray code based on original vcpu->irq_pending code,
+ * Copyright (C) 2007 Qumranet
+ *
+ * Authors:
+ * Gregory Haskins <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ *
+ */

drivers/kvm/kvm_userint.c new file +165 -0

+
+#include "kvm.h"
+
+/*----------------------------------------------------------------------
+ * optimized bitarray object - works like bitarrays in bitops, but uses 
+ * a summary field to accelerate lookups.  Assumes external locking 
+ *---------------------------------------------------------------------*/
+
+struct bitarray {
+	unsigned long summary; /* 1 per word in pending */
+	unsigned long pending[NR_IRQ_WORDS];
+};
+
+static inline int bitarray_pending(struct bitarray *this)
+{
+	return this->summary ? 1 : 0;	
+}
+
+static inline int bitarray_findhighest(struct bitarray *this)
+{
+	if (!this->summary)
+		return -1;
+	else {
+	    int word_index = __ffs(this->summary);
+	    int bit_index  = __ffs(this->pending[word_index]);
+
+	    return word_index * BITS_PER_LONG + bit_index;	
+	}
+}
+
+static inline void bitarray_set(struct bitarray *this, int nr)
+{
+	__set_bit(nr, &this->pending);
+	__set_bit(nr / BITS_PER_LONG, &this->summary); 
+} 
+
+static inline void bitarray_clear(struct bitarray *this, int nr)
+{
+	int word = nr / BITS_PER_LONG;
+
+	__clear_bit(nr, &this->pending);
+	if (!this->pending[word])
+		__clear_bit(word, &this->summary);
+}
+
+static inline int bitarray_test(struct bitarray *this, int nr)
+{
+	return test_bit(nr, &this->pending);
+}
+
+/*----------------------------------------------------------------------
+ * userint interface - provides the actual kvm_irqdevice implementation
+ *---------------------------------------------------------------------*/
+
+typedef struct {
+	spinlock_t      lock;
+	struct bitarray irq_pending;
+	int             nmi_pending;
+}kvm_userint;
+
+static int userint_pending(struct kvm_irqdevice *this, int flags)
+{
+	kvm_userint *s = (kvm_userint*)this->private;
+	int ret;
+
+	spin_lock_irq(&s->lock);
+
+	if (flags & KVM_IRQFLAGS_NMI)
+		ret = s->nmi_pending;
+	else
+		ret = bitarray_pending(&s->irq_pending);
+
+	spin_unlock_irq(&s->lock);
+
+	return ret;
+}
+
+static int userint_read_vector(struct kvm_irqdevice *this, int flags)
+{
+	kvm_userint *s = (kvm_userint*)this->private;
+	int          irq;
+
+	spin_lock_irq(&s->lock);
+
+	/* NMIs take priority, so if there is an NMI pending, or
+	 * if we are filtering out NMIs, only consider them 
+	 */
+	if (s->nmi_pending || (flags & KVM_IRQFLAGS_NMI))
+		irq = s->nmi_pending ? 2 : -1;
+	else
+		irq = bitarray_findhighest(&s->irq_pending);
+	
+	if (!(flags & KVM_IRQFLAGS_PEEK)) {
+		/* If the "peek" flag is not set, automatically clear the 
+		 * interrupt as the EOI mechanism (if any) will take place 
+		 * in userspace 
+		 */
+		bitarray_clear(&s->irq_pending, irq);
+		if (irq == 2)
+			s->nmi_pending = 0;
+	}
+
+	spin_unlock_irq(&s->lock);
+
+	return irq;
+}
+
+static int userint_set_pin(struct kvm_irqdevice* this, int irq, int level, 
+			   int flags)
+{
+	kvm_userint *s = (kvm_userint*)this->private;
+
+	if(!level)
+	    return 0; /* We dont care about deasserts */
+
+	spin_lock_irq(&s->lock);
+
+	/* Update the local state */
+	bitarray_set(&s->irq_pending, irq);
+	if (irq == 2)
+		s->nmi_pending = 1;
+
+	spin_unlock_irq(&s->lock);
+
+	/* And then alert the higher layer software we have changes */
+	kvm_irq_raise_intr(this);
+
+	return 0;
+}
+
+static int userint_summary(struct kvm_irqdevice* this, void *data)
+{	
+	kvm_userint *s = (kvm_userint*)this->private;
+
+	spin_lock_irq(&s->lock);
+	memcpy(data, s->irq_pending.pending, sizeof s->irq_pending.pending);
+	spin_unlock_irq(&s->lock);
+
+	return 0;
+}
+
+static void userint_destructor(struct kvm_irqdevice *this)
+{
+	kfree(this->private);
+}
+
+int kvm_userint_init(struct kvm_irqdevice *dev)
+{
+	kvm_userint *s;
+
+	s = (kvm_userint *)kzalloc(sizeof(kvm_userint), GFP_KERNEL);
+     
+	spin_lock_init(&s->lock);
+
+	dev->pending     = userint_pending;
+	dev->read_vector = userint_read_vector;
+	dev->set_pin     = userint_set_pin;
+	dev->summary     = userint_summary;
+	dev->destructor  = userint_destructor;
+
+	dev->private = s;
+
+	return 0;
+}
+

drivers/kvm/svm.c +7 -11

index b7e1410..04189ad 100644
@@ -108,20 +108,16 @@ static unsigned get_addr_size(struct kvm_vcpu *vcpu)
 
 static inline u8 pop_irq(struct kvm_vcpu *vcpu)
 {
-	int word_index = __ffs(vcpu->irq_summary);
-	int bit_index = __ffs(vcpu->irq_pending[word_index]);
-	int irq = word_index * BITS_PER_LONG + bit_index;
-
-	clear_bit(bit_index, &vcpu->irq_pending[word_index]);
-	if (!vcpu->irq_pending[word_index])
-		clear_bit(word_index, &vcpu->irq_summary);
-	return irq;
+	return kvm_irq_read_vector(&vcpu->irq_dev, 0);
 }
 
 static inline void push_irq(struct kvm_vcpu *vcpu, u8 irq)
 {
-	set_bit(irq, vcpu->irq_pending);
-	set_bit(irq / BITS_PER_LONG, &vcpu->irq_summary);
+	/* FIXME: We probably want to reserve the "set_pin" verb for
+	 * actual interrupt requests, not for putting back something
+	 * previously pending.  Lets revisit this
+	 */
+	kvm_irq_set_pin(&vcpu->irq_dev, irq, 1, 0);
 }
 
 static inline void clgi(void)

@@ -1092,7 +1088,7 @@ static int halt_interception(struct kvm_vcpu *vcpu, 
 struct kvm_run *kvm_run)
 {
 	vcpu->svm->next_rip = vcpu->svm->vmcb->save.rip + 1;
 	skip_emulated_instruction(vcpu);
-	if (vcpu->irq_summary)
+	if (kvm_irq_pending(&vcpu->irq_dev, 0))
 		return 1;
 

drivers/kvm/svm.c +1 -1

 	kvm_run->exit_reason = KVM_EXIT_HLT;

@@ -1263,7 +1259,7 @@ static int interrupt_window_interception(struct 
 kvm_vcpu *vcpu,
 	 * possible
 	 */
 	if (kvm_run->request_interrupt_window &&
-	    !vcpu->irq_summary) {
+	    !kvm_irq_pending(&vcpu->irq_dev, 0)) {
 		++kvm_stat.irq_window_exits;
 		kvm_run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN;

drivers/kvm/svm.c +1 -1

 		return 0;

@@ -1399,7 +1395,7 @@ static void do_interrupt_requests(struct kvm_vcpu 
 *vcpu,
 		(!(control->int_state & SVM_INTERRUPT_SHADOW_MASK) &&
 		 (vcpu->svm->vmcb->save.rflags & X86_EFLAGS_IF));
 
-	if (vcpu->interrupt_window_open && vcpu->irq_summary)
+	if (vcpu->interrupt_window_open && kvm_irq_pending(&vcpu->irq_dev, 0))
 		/*
 		 * If interrupts enabled, and not blocked by sti or mov ss. Good.

drivers/kvm/svm.c +2 -1

 		 */

@@ -1409,7 +1405,8 @@ static void do_interrupt_requests(struct kvm_vcpu 
 *vcpu,
 	 * Interrupts blocked.  Wait for unblock.
 	 */
 	if (!vcpu->interrupt_window_open &&
-	    (vcpu->irq_summary || kvm_run->request_interrupt_window)) {
+	    (kvm_irq_pending(&vcpu->irq_dev, 0) || 
+	     kvm_run->request_interrupt_window)) {
 		control->intercept |= 1ULL << INTERCEPT_VINTR;
 	} else

drivers/kvm/svm.c +3 -2

 		control->intercept &= ~(1ULL << INTERCEPT_VINTR);

@@ -1418,8 +1415,9 @@ static void do_interrupt_requests(struct kvm_vcpu 
 *vcpu,
 static void post_kvm_run_save(struct kvm_vcpu *vcpu,
 			      struct kvm_run *kvm_run)
 {
-	kvm_run->ready_for_interrupt_injection = (vcpu->interrupt_window_open &&
-						  vcpu->irq_summary == 0);
+	kvm_run->ready_for_interrupt_injection = 
+		(vcpu->interrupt_window_open && 
+		 !kvm_irq_pending(&vcpu->irq_dev, 0));
 	kvm_run->if_flag = (vcpu->svm->vmcb->save.rflags & X86_EFLAGS_IF) != 0;
 	kvm_run->cr8 = vcpu->cr8;

drivers/kvm/svm.c +1 -1

 	kvm_run->apic_base = vcpu->apic_base;

@@ -1434,7 +1432,7 @@ static void post_kvm_run_save(struct kvm_vcpu *vcpu,
 static int dm_request_for_irq_injection(struct kvm_vcpu *vcpu,
 					  struct kvm_run *kvm_run)
 {
-	return (!vcpu->irq_summary &&
+	return (!kvm_irq_pending(&vcpu->irq_dev, 0) &&
 		kvm_run->request_interrupt_window &&
 		vcpu->interrupt_window_open &&
 		(vcpu->svm->vmcb->save.rflags & X86_EFLAGS_IF));

drivers/kvm/vmx.c +2 -7

index 61a6116..0d285eb 100644
@@ -1219,13 +1219,8 @@ static void inject_rmode_irq(struct kvm_vcpu *vcpu, 
 int irq)
 
 static void kvm_do_inject_irq(struct kvm_vcpu *vcpu)
 {
-	int word_index = __ffs(vcpu->irq_summary);
-	int bit_index = __ffs(vcpu->irq_pending[word_index]);
-	int irq = word_index * BITS_PER_LONG + bit_index;
-
-	clear_bit(bit_index, &vcpu->irq_pending[word_index]);
-	if (!vcpu->irq_pending[word_index])
-		clear_bit(word_index, &vcpu->irq_summary);
+	int irq = kvm_irq_read_vector(&vcpu->irq_dev, 0);
+	BUG_ON(irq < 0);
 
 	if (vcpu->rmode.active) {

drivers/kvm/vmx.c +1 -1

 		inject_rmode_irq(vcpu, irq);

@@ -1246,7 +1241,7 @@ static void do_interrupt_requests(struct kvm_vcpu 
 *vcpu,
 		 (vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) & 3) == 0);
 
 	if (vcpu->interrupt_window_open &&
-	    vcpu->irq_summary &&
+	    kvm_irq_pending(&vcpu->irq_dev, 0) &&
 	    !(vmcs_read32(VM_ENTRY_INTR_INFO_FIELD) & INTR_INFO_VALID_MASK))
 		/*

drivers/kvm/vmx.c +2 -1

 		 * If interrupts enabled, and not blocked by sti or mov ss. Good.

@@ -1255,7 +1250,8 @@ static void do_interrupt_requests(struct kvm_vcpu 
 *vcpu,
 
 	cpu_based_vm_exec_control = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
 	if (!vcpu->interrupt_window_open &&
-	    (vcpu->irq_summary || kvm_run->request_interrupt_window))
+	    (kvm_irq_pending(&vcpu->irq_dev, 0) ||
+	     kvm_run->request_interrupt_window))
 		/*
 		 * Interrupts blocked.  Wait for unblock.

drivers/kvm/vmx.c +2 -2

 		 */

@@ -1314,8 +1310,8 @@ static int handle_exception(struct kvm_vcpu *vcpu, 
 struct kvm_run *kvm_run)
 
 	if (is_external_interrupt(vect_info)) {
 		int irq = vect_info & VECTORING_INFO_VECTOR_MASK;
-		set_bit(irq, vcpu->irq_pending);
-		set_bit(irq / BITS_PER_LONG, &vcpu->irq_summary);
+		/* FIXME: Is this right? */
+		kvm_irq_set_pin(&vcpu->irq_dev, irq, 1, 0); 
 	}
 

drivers/kvm/vmx.c +4 -3

 	if ((intr_info & INTR_INFO_INTR_TYPE_MASK) == 0x200) { /* nmi */

@@ -1619,8 +1615,9 @@ static void post_kvm_run_save(struct kvm_vcpu *vcpu,
 	kvm_run->if_flag = (vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF) != 0;
 	kvm_run->cr8 = vcpu->cr8;
 	kvm_run->apic_base = vcpu->apic_base;
-	kvm_run->ready_for_interrupt_injection = (vcpu->interrupt_window_open &&
-						  vcpu->irq_summary == 0);
+	kvm_run->ready_for_interrupt_injection = 
+		(vcpu->interrupt_window_open && 
+		 !kvm_irq_pending(&vcpu->irq_dev, 0));
 }
 
 static int handle_interrupt_window(struct kvm_vcpu *vcpu,

@@ -1631,7 +1628,7 @@ static int handle_interrupt_window(struct kvm_vcpu 
 *vcpu,
 	 * possible
 	 */
 	if (kvm_run->request_interrupt_window &&
-	    !vcpu->irq_summary) {
+	    !kvm_irq_pending(&vcpu->irq_dev, 0)) {
 		kvm_run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN;
 		++kvm_stat.irq_window_exits;

drivers/kvm/vmx.c +1 -1

 		return 0;

@@ -1642,7 +1639,7 @@ static int handle_interrupt_window(struct kvm_vcpu 
 *vcpu,
 static int handle_halt(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 {
 	skip_emulated_instruction(vcpu);
-	if (vcpu->irq_summary)
+	if (kvm_irq_pending(&vcpu->irq_dev, 0))
 		return 1;
 

drivers/kvm/vmx.c +1 -1

 	kvm_run->exit_reason = KVM_EXIT_HLT;

@@ -1713,7 +1710,7 @@ static int kvm_handle_exit(struct kvm_run *kvm_run, 
 struct kvm_vcpu *vcpu)
 static int dm_request_for_irq_injection(struct kvm_vcpu *vcpu,
 					  struct kvm_run *kvm_run)
 {
-	return (!vcpu->irq_summary &&
+	return (!kvm_irq_pending(&vcpu->irq_dev, 0) &&
 		kvm_run->request_interrupt_window &&
 		vcpu->interrupt_window_open &&

(vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF));

Hi All,
The following is another patch that I broke out of the in-kernel-apic patch that I sent earlier. This focuses purely on the irqdevice abstraction without introducing the concept of the in-kernel LAPIC. It also provides an implementation of a "userint" model, which should support a system with a QEMU based emulation of the (A)PIC (e.g. current behavior). I have cleaned up quite a few things based on all of your comments as well, so hopefully its a little more polished than last time.

As far as testing, I have confirmed that I can boot a 64-bit linux guest with no discernable difference in behavior.

Note that this patch applies after the in-kernel-mmio.patch that I sent earlier today which has not yet been accepted/commited. This patch should not depend on it. However, if you do have problems applying it let me know and I can generate another one that is not after in-kernel-mmio in the series.

drivers/kvm/kvm.h +3 -0

+
+struct kvm_irqdevice;
+

drivers/kvm/kvm.h +3 -0

+
+/*
+ * kvm_irq_init()

drivers/kvm/kvm.h +16 -0

+static inline void kvm_irq_init(struct kvm_irqdevice *dev)
+{
+	memset(dev, 0, sizeof(*dev));
+}
+
+/*
+ * kvm_irq_pending()
+ *
+ * Description:
+ *    Efficiently determines if an interrupt is pending on an irqdevice
+ *
+ * Parameters:
+ *    struct kvm_irqdevice *dev: The device
+ *    int flags: Modifies the behavior as follows:
+ * 
+ *                   KVM_IRQFLAGS_NMI: Mask everything but NMIs

drivers/kvm/kvm.h +3 -0

+
+/*
+ * kvm_irq_set_pin()

drivers/kvm/kvm.h +8 -0

+			int bit_index = __ffs(word);
+			int irq = i * BITS_PER_LONG + bit_index;
+			
+			kvm_irq_set_pin(&vcpu->irq_dev, irq, 1, 0);
+
+			clear_bit(bit_index, &word);
+		}
+	}
 
 	set_segment(vcpu, &sregs->cs, VCPU_SREG_CS);
 	set_segment(vcpu, &sregs->ds, VCPU_SREG_DS);

@@ -2318,6 +2317,33 @@ out1:
 
 

drivers/kvm/kvm_userint.c new file +3 -0

new file mode 100644
index 0000000..ded3098
@@ -0,0 +1,201 @@
+/*
+ * kvm_userint.c: User Interrupts IRQ device 
+ *

drivers/kvm/kvm_userint.c new file +4 -0

+static int userint_read_vector(struct kvm_irqdevice *this, int flags)
+{
+	kvm_userint *s = (kvm_userint*)this->private;
+	int          irq;

Quoted 1 line

Quoted 1 line

On Sun, Apr 8, 2007 at 2:47 AM, in message <46189005.2090609-atKUWr5tajBWk0Htik3J/[email protected]>,

Quoted 2 lines · Gregory Haskins

Hi All,
The following is another patch that I broke out of the in- kernel- apic patch

that I sent earlier. This focuses purely on the irqdevice abstraction without introducing the concept of the in- kernel LAPIC. It also provides an implementation of a "userint" model, which should support a system with a QEMU based emulation of the (A)PIC (e.g. current behavior). I have cleaned up quite a few things based on all of your comments as well, so hopefully its a little more polished than last time.

Quoted 2 lines

As far as testing, I have confirmed that I can boot a 64- bit linux guest with

no discernable difference in behavior.

Quoted 2 lines

Note that this patch applies after the in- kernel- mmio.patch that I sent

earlier today which has not yet been accepted/commited. This patch should not depend on it. However, if you do have problems applying it let me know and I can generate another one that is not after in- kernel- mmio in the series.

Quoted 1 line

A patchset is fine.

Quoted 6 lines

drivers/kvm/Makefile +0 -0

@@ - 2,7 +2,7 @@
# Makefile for Kernel- based Virtual Machine module #

Quoted 3 lines

- kvm- objs := kvm_main.o mmu.o x86_emulate.o +kvm- objs := kvm_main.o mmu.o x86_emulate.o kvm_userint.o

kvm_ prefixes are not needed for drivers/kvm/. kvm_main is an historical exception.

Quoted 11 lines

obj- $(CONFIG_KVM) += kvm.o
kvm- intel- objs = vmx.o
obj- $(CONFIG_KVM_INTEL) += kvm- intel.o
diff -- git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h index c1923df..6caf60b 100644

drivers/kvm/kvm.h +0 -0

@@ - 13,6 +13,7 @@
#include <linux/mm.h>

Quoted 3 lines

#include "vmx.h"
+#include "kvm_irqdevice.h"

Ditto.

drivers/kvm/kvm.h

Quoted 7 lines

drivers/kvm/kvm.h +6 -0

+
+#ifndef __KVM_IRQDEVICE_H
+#define __KVM_IRQDEVICE_H
+
+#define KVM_IRQFLAGS_NMI  (1 << 0)
+#define KVM_IRQFLAGS_PEEK (1 << 1)

is PEEK still needed?

Quoted 10 lines

drivers/kvm/kvm.h +9 -0

+
+struct kvm_irqdevice;
+
+struct kvm_irqsink {
+	void (*raise_intr)(struct kvm_irqsink *this, 
+			   struct kvm_irqdevice *dev);
+
+	void *private;
+};

drivers/kvm/kvm.h

Quoted 5 lines

drivers/kvm/kvm.h +4 -0

+
+struct kvm_irqdevice {
+	int  (*pending)(struct kvm_irqdevice *this, int flags);
+	int  (*read_vector)(struct kvm_irqdevice *this, int flags); 

I'm a bit confused here. The kvm_irqsink mechanism implies a push mechanism. - >pending and - >read_vector imply a pull mechanism.

drivers/kvm/kvm.h

Quoted 7 lines

drivers/kvm/kvm.h +6 -0

+
+/*
+ * kvm_irq_init()
+ *
+ * Description:
+ *    Initialized the kvm_irqdevice for use.  Should be called before 

calling

Quoted 8 lines

+ * any derived implementation init functions
+ *
+ * Parameters:
+ * struct kvm_irqdevice *dev: This device
+ *
+ * Returns: (void)
+ */

This is infinitely better than the usual kvm documentation, but it would be better still to follow kerneldoc.

Quoted 23 lines

drivers/kvm/kvm.h +22 -0

+static inline void kvm_irq_init(struct kvm_irqdevice *dev)
+{
+	memset(dev, 0, sizeof(*dev));
+}
+
+/*
+ * kvm_irq_pending()
+ *
+ * Description:
+ *    Efficiently determines if an interrupt is pending on an irqdevice
+ *
+ * Parameters:
+ *    struct kvm_irqdevice *dev: The device
+ *    int flags: Modifies the behavior as follows:
+ * 
+ *                   KVM_IRQFLAGS_NMI: Mask everything but NMIs
+ *
+ * Returns: (int)
+ *   - 1 = failure
+ *    0 = no iterrupts pending (per "flags" criteria)
+ *   >0 = one or more interrupts are pending
+ */

drivers/kvm/kvm.h

Quoted 5 lines

drivers/kvm/kvm.h +4 -0

+static inline int kvm_irq_pending(struct kvm_irqdevice *dev, int flags)
+{
+	return dev- >pending(dev, flags);
+}

Usually - errno is returned for failure. Can this reasonably fail, though? it would simplify upper layers if it couldn't.

Quoted 10 lines

drivers/kvm/kvm.h +9 -0

+
+/*
+ * kvm_irq_set_pin()
+ *
+ * Description:
+ *    Allows the caller to assert/deassert an IRQ input pin to the device 
+ *    according to device policy.
+ *
+ * Parameters:

Quoted 5 lines

+ *    struct kvm_irqdevice *dev: The device
+ *    int pin: The input pin to alter
+ *    int level: The value to set (1 = assert, 0 = deassert)
+ *    int flags: Reserved, must be 0

Just remove it for now. When we find a use for it, we can add it. It's only userspace interfaces that are brittle.

drivers/kvm/kvm.h

Quoted 7 lines

drivers/kvm/kvm.h +4 -0

+	/* walk the interrupt- bitmap and inject an IRQ for each bit found */
+	for (i = 0; i < NR_IRQ_WORDS; ++i) {
+		unsigned long word = sregs- >interrupt_bitmap[i];
+		while(word) {

spaceafterwhile. Also, can just use a single loop with __test_bit(), as we're not performance critical here.

Quoted 20 lines

drivers/kvm/kvm.h +8 -0

+			int bit_index = __ffs(word);
+			int irq = i * BITS_PER_LONG + bit_index;
+			
+			kvm_irq_set_pin(&vcpu- >irq_dev, irq, 1, 0);
+
+			clear_bit(bit_index, &word);
+		}
+	}
 
 	set_segment(vcpu, &sregs- >cs, VCPU_SREG_CS);
 	set_segment(vcpu, &sregs- >ds, VCPU_SREG_DS);

@@ - 2318,6 +2317,33 @@ out1:

diff -- git a/drivers/kvm/kvm_userint.c b/drivers/kvm/kvm_userint.c new file mode 100644
index 0000000..ded3098

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

Quoted 5 lines

@@ - 0,0 +1,201 @@
+/*
+ * kvm_userint.c: User Interrupts IRQ device
+ *
+ * This acts as an extention of an interrupt controller that exists

elsewhere

Quoted 3 lines

+ * (typically in userspace/QEMU). Because this PIC is a pseudo device that
+ * is downstream from a real emulated PIC, the "IRQ- to- vector" mapping has
+ * already occured. Therefore, this PIC has the following unusal

properties:

Quoted 7 lines

+ *
+ * 1) It has 256 "pins" which are literal vectors (e.g.no translation)
+ * 2) It only supports "auto- EOI" behavior since it is expected that the
+ * upstream emulated PIC will handle the real EOIs (if applicable)
+ * 3) It only listens to "asserts" on the pins (deasserts are dropped)
+ * because its an auto- EOI device anyway.

Wierd, but well explained.

drivers/kvm/kvm_userint.c line 65

Quoted 12 lines

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

+static inline int bitarray_findhighest(struct bitarray *this)
+{
+	if (!this- >summary)
+		return - 1;
+	else {
+	    int word_index = __ffs(this- >summary);
+	    int bit_index  = __ffs(this- >pending[word_index]);
+
+	    return word_index * BITS_PER_LONG + bit_index;	
+	}
+}

Um, this is the lowest?

drivers/kvm/kvm_userint.c line 95

Quoted 12 lines

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

+
+/*----------------------------------------------------------------------
+ * userint interface -  provides the actual kvm_irqdevice implementation
+ *--------------------------------------------------------------------- */
+
+typedef struct {
+	spinlock_t      lock;
+	struct bitarray irq_pending;
+	int             nmi_pending;
+}kvm_userint;

No typedefs in kernel code for ordinary structs.

Quoted 4 lines

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

+
+static int userint_pending(struct kvm_irqdevice *this, int flags)

drivers/kvm/kvm_userint.c

Quoted 10 lines

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

+{
+	kvm_userint *s = (kvm_userint*)this- >private;
+	int ret;
+
+	spin_lock_irq(&s- >lock);
+
+	if (flags & KVM_IRQFLAGS_NMI)
+		ret = s- >nmi_pending;
+	else
+		ret = bitarray_pending(&s- >irq_pending);

You probably want:

    ret = s- >nmi_pending;
    if (!(flags & KVM_IRQFLAGS_NMI))
          ret |= bitarray_pending(...);

To avoid calling this twice when interrupts are enabled.

Quoted 11 lines

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

+static int userint_read_vector(struct kvm_irqdevice *this, int flags)
+{
+	kvm_userint *s = (kvm_userint*)this- >private;
+	int          irq;
+
+	spin_lock_irq(&s- >lock);
+
+	/* NMIs take priority, so if there is an NMI pending, or
+	 * if we are filtering out NMIs, only consider them 
+	 */

drivers/kvm/kvm_userint.c

Quoted 5 lines

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

+	if (s- >nmi_pending || (flags & KVM_IRQFLAGS_NMI))
+		irq = s- >nmi_pending ? 2 : - 1;
+	else
+		irq = bitarray_findhighest(&s- >irq_pending);

Same here. Actually the comment is correct (even though it does not start with a /* on a line by itself).

In general, looks very good. The interface is a bit over- rich (- >pending, - >read_vector, and irq_sink) but that's not a showstopper.

Quoted 9 lines

Quoted 9 lines

drivers/kvm/kvm_irqdevice.h new file +9 -0

+
+struct kvm_irqdevice;
+
+struct kvm_irqsink {
+	void (*raise_intr)(struct kvm_irqsink *this, 
+			   struct kvm_irqdevice *dev);
+
+	void *private;
+};

Quoted 9 lines

drivers/kvm/kvm_irqdevice.h line 53

Quoted 6 lines

drivers/kvm/kvm_irqdevice.h new file +4 -0

+
+struct kvm_irqdevice {
+	int  (*pending)(struct kvm_irqdevice *this, int flags);
+	int  (*read_vector)(struct kvm_irqdevice *this, int flags); 

I'm a bit confused here. The kvm_irqsink mechanism implies a push mechanism. - >pending and - >read_vector imply a pull mechanism.

What I am modeling is the concept of an PIC INTR + INTA. INTR serves to just cause the processor to realize it needs to service an interrupt. The INTA cycle is what then gets invoked after INTR to figure out which vector is appropriate. Finally, the vector is called out of the IDT.

However, there is no actual INTA cycle on a virtualized guest. We emulate an INTA cycle by calling our pending/read routines and injecting the vector into the VMCS. Whats missing from the current HEAD is that there is no concept of INTR. This is whats new, and its why we have both a push and a pull mechanism. The old way only had the pull. Does this make sense?

Quoted 6 lines

Quoted 6 lines

drivers/kvm/kvm_userint.c new file +6 -0

+static inline int bitarray_findhighest(struct bitarray *this)
+{
+	if (!this- >summary)
+		return - 1;
+	else {
+	    int word_index = __ffs(this- >summary);

Quoted 9 lines

drivers/kvm/kvm_userint.c line 65

Quoted 7 lines

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

+	    int bit_index  = __ffs(this- >pending[word_index]);
+
+	    return word_index * BITS_PER_LONG + bit_index;	
+	}
+}

Um, this is the lowest?

Opps...carry over from the original code. I forgot to reverse the polarity.

Quoted 17 lines

Quoted 5 lines

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

+
+static int userint_pending(struct kvm_irqdevice *this, int flags)

drivers/kvm/kvm_userint.c

Quoted 10 lines

drivers/kvm/kvm_userint.c new file +10 -0

+{
+	kvm_userint *s = (kvm_userint*)this- >private;
+	int ret;
+
+	spin_lock_irq(&s- >lock);
+
+	if (flags & KVM_IRQFLAGS_NMI)
+		ret = s- >nmi_pending;
+	else
+		ret = bitarray_pending(&s- >irq_pending);

You probably want:

Quoted 6 lines

    ret = s- >nmi_pending;
    if (!(flags & KVM_IRQFLAGS_NMI))
          ret |= bitarray_pending(...);

To avoid calling this twice when interrupts are enabled.

I think there is a misunderstanding. The return value from this function should just be 0 for false, and non-zero for true. The bitarray still holds a bit for IRQ2 so you technically wouldn't need to call this twice when interrupts are enabled. Simply calling this without the flag set will give you the proper behavior, so I think it is fine as written. It also doesn't hurt to do it the way you wrote it either. Let me know if I misunderstood you.

Quoted 1 line

Quoted 1 line

On Tue, Apr 10, 2007 at 3:52 AM, in message <461B4241.5030101-atKUWr5tajBWk0Htik3J/[email protected]>,

Hmm. The current code is synchronous in nature (the vcpu is never executing while we raise an interrupt, so the INTA is never needed, as we can ensure the cpu can process the interrupt when we inject it. With smp/paravirt/whatever (I realize now), this is no longer true.

The NIC raises an interrupt, we have to interrupt the guest to see if its current IF/cr8 permit interrupt injection, and if not, we have to keep the interrupt in the irqdevice and request an exit when IF/cr8 permit injection.

This means that - >pending() and - >read_vector() have to be called in a critical section, otherwise the pending interrupt might be change between the first and second call, resulting in an injected interrupt to software that is not ready to accept it. If we replace read_vector by
- >ack(this, int vector) we avoid this need (and also save a pointless recalculation of the vector).

Note that the original code did not work as intended. Because of the IF/tpr issues, there is at most interrupt queued, and only when it is ready to be accepted.

With the in- kernel apic that changes of course.

Quoted 5 lines

The NIC raises an interrupt, we have to interrupt the guest to see if its current IF/cr8 permit interrupt injection, and if not, we have to keep the interrupt in the irqdevice and request an exit when IF/cr8 permit injection.

Exactly. The INTR really just serves to kick the cpu into a VMEXIT so we can evaluate the IF/TPR. In many cases this may reveal that the CPU might not be able to take the interrupt right then and there, so it will remain queued by the irqdevice model until it can. If the interrupt cannot be dispatched, we would turn on any relevant EXIT feature (e.g., interrupt-window-exiting) if we dont do this already (I believe this is already in there)

Quoted 7 lines

This means that - >pending() and - >read_vector() have to be called in a critical section, otherwise the pending interrupt might be change between the first and second call, resulting in an injected interrupt to software that is not ready to accept it. If we replace read_vector by
- >ack(this, int vector) we avoid this need (and also save a pointless recalculation of the vector).

I am a little confused as to what you are proposing. The current design has pending only returning a boolean if there is something pending, and read_vector actually does the INTA cycle (i.e., returns the vector # and marks it accepted). I can certainly see how a lack of critical section between pending and read_vector could allow a new interrupt to be injected, but I am not seeing why this is a problem. If a new higher-priority interrupt comes in between the pending and read_vector, the new one will be returned, but I think this is desirable?

And if we replace read_vector() with ack(), how do we learn of the vector in the first place? I am guessing that you are thinking that both pending and read_vector return a vector and thus the confusion. Otherwise, I am missing your point completely :) Could you elaborate?

Quoted 1 line

Quoted 1 line

On Tue, Apr 10, 2007 at 8:01 AM, in message <461B7C81.7040102-atKUWr5tajBWk0Htik3J/[email protected]>,

Quoted 2 lines · Gregory Haskins

And if we replace read_vector() with ack(), how do we learn of the vector in

the first place? I am guessing that you are thinking that both pending and read_vector return a vector and thus the confusion. Otherwise, I am missing your point completely :) Could you elaborate?

Quoted 1 line

That's what I thought, but it doesn't really matter: what's important is that accepting an interrupt has several stages, and that they must act as a transaction.

The stages are:
1. determine which interrupt is to be injected (if not, abort)
2. check if IF and tpr allow injection
3. one of
- inject the interrupt and mark it as in- service
- request an exit on IF and/or tpr threshold

Simply indicating that an interrupt may be pending is already performed by the irq_sink callback.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

On Tue, Apr 10, 2007 at 8:01 AM, in message <461B7C81.7040102-atKUWr5tajBWk0Htik3J/[email protected]>,

Quoted 21 lines · Avi Kivity

Quoted 2 lines · Gregory Haskins

And if we replace read_vector() with ack(), how do we learn of the vector in

the first place? I am guessing that you are thinking that both pending and read_vector return a vector and thus the confusion. Otherwise, I am missing your point completely :) Could you elaborate?

Quoted 2 lines

That's what I thought, but it doesn't really matter: what's important is that accepting an interrupt has several stages, and that they must act as a transaction.

The stages are:
1. determine which interrupt is to be injected (if not, abort)
2. check if IF and tpr allow injection
3. one of
- inject the interrupt and mark it as in- service
- request an exit on IF and/or tpr threshold

Quoted 3 lines

Simply indicating that an interrupt may be pending is already performed by the irq_sink callback.

Yes, but the operative words here are "may be". INTR tells the system that an interrupt *may be* pending, but we don't yet actually know which vector (if any) will be eligible for injection. Thinking down the road when we will have SMP/PV, we must assume that the CPU could be actually running concurrently. If so, it must be stopped first before we can evaluate the state (e.g., IF, TPR, etc.) to know which vector (if any) can be dispatched. The eligibility of a vector (or the vector itself if a new, higher-pri line is raised in between) may change up until the point the EXIT actually occurs.

This is analogous to how the 8259 INTR/INTA cycle works, IIUC. The INTR line is raised to get the CPUs attention asynchronously independent of CPU state, but its the processor controlled INTA cycle that retrieves the vector to use. IMHO, we want to do the same thing here. The thread that invokes the set_pin and thus indirectly the INTR is one context. You can think of this as the PIC. The thread that is running the guest and VMEXITs is another context. You can think of this as the processor. The PIC signals the processor that it needs attention, but the vector is conveyed on the processor's terms when its ready. In cases where there is no parallelism (e.g., todays sync design, or of the vcpu happens to not be executing), the INTR is a no-op.

Now as far as the rest of the API is concerned, pending() tells us definitively (and efficiently) after we have stopped that there really is something pending given the current state of the system. read_vector() tells us what that vector actually is. Really, read_vector() is the authoritative source of data and acts like our processor controlled INTA cycle. The important thing is that this read_vector() method is called in the same context as the guest CPU thread. The other two (irq_sink, pending) are just helpers to get us to that point.

Where I really think we need to have a critical section is between the vcpu-INTR callback, and the part of the vcpu logic that evaluates whether interrupts are pending (to avoid missing a newly injected interrupt while we are processing). However, I really see this as an vcpu issue more than an issue against the irqdevice interface. Does this make sense?

I will put together a follow-on patch that implements my thinking to help illustrate the concept.

Quoted 1 line

Quoted 1 line

On Tue, Apr 10, 2007 at 10:46 AM, in message <461BA363.5080308-atKUWr5tajBWk0Htik3J/[email protected]>,

Both the vcpu and the irqdevice are involved in the decision:

(quoted diff) +0 -3

-  the vcpu holds the IF (or rather, the "interrupt window")
-  the lapic holds the tpr
-  but the vcpu contains the controls that allow exiting on tpr 

modification (on 64- bit and with next- gen Intel processors)

IIUC, you are suggesting (pseudocode):

spin_lock(vcpu- >irq_lock)
nmi = !irq_window_open(vcpu)
if (vcpu- >irq- >pending(nmi))
inject(vcpu, vcpu- >irq- >read_vector(nmi))
spin_unlock(vcpu- >irq_lock)

where - >pending() has to program a tpr exit if current tpr masks an interrupt.

The alternative I'm suggesting is to move all this mess into the irqdevice code in a single callback. While this has less reuse potential, in fact the hardware itself is messy and attempts to generalize it may turn into a mess as well.

Quoted 1 line

I will put together a follow- on patch that implements my thinking to help

illustrate the concept.

Quoted 1 line

That will be best. After all, we're not trying to design a generic interface, we have a know an limited set of users. Seeing the users will help evaluate the API.

Quoted 1 line

Quoted 1 line

On Tue, Apr 10, 2007 at 11:39 AM, in message <[email protected]>,

We could change the API to handle the pending condition as a tri-level

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

On Tue, Apr 10, 2007 at 11:39 AM, in message <[email protected]>,

Quoted 2 lines · Gregory Haskins

We could change the API to handle the pending condition as a tri-level

I just thought of a better way. Leave the return value as a boolean, but use the concept of the mask_reasons. It simplifies the logic, and allows the irqdevice to register for additional VMEXITs regardless of whether its currently injecting something or not. So we have:

  spin_lock(vcpu- >irq_lock)
  vcpu->exit_reasons = 0;
  nmi = !irq_window_open(vcpu)
  if (vcpu- >irq- >pending(nmi, &mask_reasons))
      inject(vcpu, vcpu- >irq- >read_vector(nmi))
  if (mask_reasons) {
        if (mask_reasons & MASKREASON_TPR)
          vcpu->exit_reasons &= TPR;
      /* etc */
  }
  spin_unlock(vcpu- >irq_lock)

Quoted 1 line

Quoted 1 line

On Tue, Apr 10, 2007 at 12:11 PM, in message <461BB738.7040206-atKUWr5tajBWk0Htik3J/[email protected]>,

It's more complex than that;

you need to program the hardware to exit on
a specific threshold level of tpr values (e.g. exit when tpr <= n).

It's a little sad to encode a complex condition (wait for nmi window opening/wait for irq window opening/wait for irq window opening with tpr <= n) in irqdevice, then decode it in the vcpu and program the hardware accordingly.

Quoted 4 lines

where - >pending() has to program a tpr exit if current tpr masks an interrupt.

I wasn't planning on the irqdevice having this type of control because I assumed we would always have these exit types set, but you have highlighted an interesting point. If we want to selectively enable these different types of exits (and I agree that it's probably a good idea), we need more intelligence from the irqdevice. Now the question is how to handle/detect the different *types* of masking that can occur. My current API is really just designed to return a boolean: true = "there are injectable vectors", false = "there are no injectable vectors (but there may be masked vectors).

I don't like the idea of this programming being handled directly by the irqdevice because I think its potentially the wrong layer. However, we do need to account for this somehow. We could change the API to handle the pending condition as a tri-level: no-interrupts, masked-interrupts, injectable-interrupts? For "no" and "injectable", the operation is as your psuedo-code indicates. For the masked case, we could enumerate different classes of reasons (e.g. TPR, etc) that could be used to drive the programming. So the pseudo-code would now be:

   spin_lock(vcpu- >irq_lock)
   nmi = !irq_window_open(vcpu)
   switch (vcpu- >irq- >pending(nmi, &mask_reasons)) {
   case NONE_PENDING:
      break;
   case MASKED_PENDING:
      if (mask_reasons & MASKREASON_TPR)
          vcpu->exit_reasons &= TPR;
      /* etc */
      break;
    case PENDING:
       inject(vcpu, vcpu- >irq- >read_vector(nmi))
       break;
   }
   spin_unlock(vcpu- >irq_lock)

Thoughts?

Quoted 6 lines

The alternative I'm suggesting is to move all this mess into the irqdevice code in a single callback. While this has less reuse potential, in fact the hardware itself is messy and attempts to generalize it may turn into a mess as well.

I cant quite wrap my head around how I could work out all of the issues as I (think) have done with the current proposal, but I am open to suggestion. Can you elaborate more?

drivers/kvm/kvm_irqdevice.h new file +20 -0

+ *
+ * Defines an interface for an abstract interrupt controller.  The model 
+ * consists of a unit with an arbitrary number of input lines (IRQ0-N), an
+ * output line (INTR), and methods for completing an interrupt-acknowledge
+ * cycle (INTA).  A particular implementation of this model will define
+ * various policies, such as irq-to-vector translation, INTA/auto-EOI policy,
+ * etc.  
+ * 
+ * In addition, the INTR callback mechanism allows the unit to be "wired" to
+ * an interruptible source in a very flexible manner. For instance, an 
+ * irqdevice could have its INTR wired to a VCPU (ala LAPIC), or another 
+ * interrupt controller (ala cascaded i8259s)
+ *
+ * Copyright (C) 2007 Novell
+ *
+ * Authors:
+ *   Gregory Haskins <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,

drivers/kvm/kvm_irqdevice.h new file +36 -0

+/*----------------------------------------------------------------------
+ * optimized bitarray object - works like bitarrays in bitops, but uses 
+ * a summary field to accelerate lookups.  Assumes external locking 
+ *---------------------------------------------------------------------*/
+
+struct bitarray {
+	unsigned long summary; /* 1 per word in pending */
+	unsigned long pending[NR_IRQ_WORDS];
+};
+
+static inline int bitarray_pending(struct bitarray *this)
+{
+	return this->summary ? 1 : 0;	
+}
+
+static inline int bitarray_findhighest(struct bitarray *this)
+{
+	if (!this->summary)
+		return -1;
+	else {
+	    int word_index = __ffs(this->summary);
+	    int bit_index  = __ffs(this->pending[word_index]);
+
+	    return word_index * BITS_PER_LONG + bit_index;	
+	}
+}
+
+static inline void bitarray_set(struct bitarray *this, int nr)
+{
+	__set_bit(nr, &this->pending);
+	__set_bit(nr / BITS_PER_LONG, &this->summary); 
+} 
+
+static inline void bitarray_clear(struct bitarray *this, int nr)
+{
+	int word = nr / BITS_PER_LONG;

Quoted 4 lines

+/*----------------------------------------------------------------------
+ * optimized bitarray object - works like bitarrays in bitops, but uses
+ * a summary field to accelerate lookups. Assumes external locking
+ *---------------------------------------------------------------------*/

drivers/kvm/kvm_userint.c line 49

Quoted 7 lines

drivers/kvm/kvm_userint.c new file +6 -0

+
+struct bitarray {
+	unsigned long summary; /* 1 per word in pending */
+	unsigned long pending[NR_IRQ_WORDS];
+};
+

This should go into a separate header, probably even in include/linux/

Quoted 4 lines · Christoph Hellwig

Quoted 4 lines

+/*----------------------------------------------------------------------
+ * optimized bitarray object - works like bitarrays in bitops, but uses
+ * a summary field to accelerate lookups. Assumes external locking
+ *---------------------------------------------------------------------*/

Quoted 9 lines

drivers/kvm/kvm_userint.c line 49

Quoted 7 lines

drivers/kvm/kvm_userint.c new file +6 -0

+
+struct bitarray {
+	unsigned long summary; /* 1 per word in pending */
+	unsigned long pending[NR_IRQ_WORDS];
+};
+

This should go into a separate header, probably even in include/linux/

It's limited to BITS_PER_LONG * BITS_PER_LONG bits (1K or 4K, depending on arch). Do you see any potential users?

Making it generic would be difficult in C without preprocessor abuse.

Quoted 1 line

Quoted 1 line

On Sun, Apr 8, 2007 at 5:58 AM, in message

Quoted 4 lines

diff -- git a/drivers/kvm/kvm_irqdevice.h b/drivers/kvm/kvm_irqdevice.h
new file mode 100644
index 0000000..9c91d15
--- /dev/null

Quoted 4 lines

+++ b/drivers/kvm/kvm_irqdevice.h
@@ - 0,0 +1,206 @@
+/*
+ * kvm_irqdevice.h

No need to mention the file name in the top of file comment. Quite contrary,
this comment can easily get out of sync and thus is not encouraged.

Quoted 10 lines

+ *
+ * Defines an interface for an abstract interrupt controller. The model
+ * consists of a unit with an arbitrary number of input lines (IRQ0- N), an
+ * output line (INTR), and methods for completing an interrupt- acknowledge
+ * cycle (INTA). A particular implementation of this model will define
+ * various policies, such as irq- to- vector translation, INTA/auto- EOI policy,
+ * etc.
+ *
+ * In addition, the INTR callback mechanism allows the unit to be "wired"

to

Quoted 8 lines

+ * an interruptible source in a very flexible manner. For instance, an
+ * irqdevice could have its INTR wired to a VCPU (ala LAPIC), or another
+ * interrupt controller (ala cascaded i8259s)
+ *
+ * Copyright (C) 2007 Novell
+ *
+ * Authors:
+ * Gregory Haskins <[email protected]>

Quoted 8 lines

+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License

for

Quoted 3 lines

+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along

with

Quoted 1 line

+ * this program; if not, write to the Free Software Foundation, Inc., 59

Temple

Quoted 1 line

+ * Place - Suite 330, Boston, MA 02111- 1307 USA.

It would be nice if you could follow the top of file comment style used in the rest of the kvm code instead of using a very different one.

Quoted 6 lines

+/*
+ * kvm_irq_init()
+ *
+ * Description:
+ * Initialized the kvm_irqdevice for use. Should be called before

calling

Quoted 7 lines

+ * any derived implementation init functions
+ *
+ * Parameters:
+ * struct kvm_irqdevice *dev: This device
+ *
+ * Returns: (void)
+ */

Please use kernel- doc style comments that can be used to auto- generate documentation.

drivers/kvm/kvm_irqdevice.h line 100

Quoted 5 lines

drivers/kvm/kvm_irqdevice.h new file +4 -0

+static inline int kvm_irq_pending(struct kvm_irqdevice *dev, int flags)
+{
+	return dev- >pending(dev, flags);
+}

This wrapper is not needed at all, just call the method directly.

drivers/kvm/kvm_irqdevice.h line 123

Quoted 5 lines

drivers/kvm/kvm_irqdevice.h new file +4 -0

+static inline int kvm_irq_read_vector(struct kvm_irqdevice *dev, int flags)
+{
+	return dev- >read_vector(dev, flags);
+}

ditto

drivers/kvm/kvm_irqdevice.h line 146

Quoted 5 lines

drivers/kvm/kvm_irqdevice.h new file +5 -0

+static inline int kvm_irq_set_pin(struct kvm_irqdevice *dev, int pin, 
+				  int level, int flags)
+{
+	return dev- >set_pin(dev, pin, level, flags);
+}

ditto

drivers/kvm/kvm_irqdevice.h line 165

Quoted 4 lines

drivers/kvm/kvm_irqdevice.h new file +4 -0

+static inline int kvm_irq_summary(struct kvm_irqdevice *dev, void *data)
+{
+	return dev- >summary(dev, data);
+}

ditto.

drivers/kvm/kvm_irqdevice.h line 185

Quoted 5 lines

drivers/kvm/kvm_irqdevice.h new file +5 -0

+static inline void kvm_irq_register_sink(struct kvm_irqdevice *dev, 
+					 const struct kvm_irqsink *sink)
+{
+	dev- >sink = *sink;
+}

Completely useless wrapper, just do the assignment in the caller.

Also the convention of copying the operation vectors is rather unnatural for linux, just set the pointer (and make sure the operations regustired are file- scope not local- scope as in your current code)

drivers/kvm/kvm_irqdevice.h line 204

Quoted 7 lines

drivers/kvm/kvm_irqdevice.h new file +6 -0

+static inline void kvm_irq_raise_intr(struct kvm_irqdevice *dev)
+{
+	struct kvm_irqsink *sink = &dev- >sink;
+	if (sink- >raise_intr)
+		sink- >raise_intr(sink, dev);
+}

Similarly this is rather pointless and could be opencoded in the only caller.

Quoted 41 lines

drivers/kvm/kvm_userint.c new file +40 -0

+/*----------------------------------------------------------------------
+ * optimized bitarray object -  works like bitarrays in bitops, but uses 
+ * a summary field to accelerate lookups.  Assumes external locking 
+ *--------------------------------------------------------------------- */
+
+struct bitarray {
+	unsigned long summary; /* 1 per word in pending */
+	unsigned long pending[NR_IRQ_WORDS];
+};
+
+static inline int bitarray_pending(struct bitarray *this)
+{
+	return this- >summary ? 1 : 0;	
+}
+
+static inline int bitarray_findhighest(struct bitarray *this)
+{
+	if (!this- >summary)
+		return - 1;
+	else {
+	    int word_index = __ffs(this- >summary);
+	    int bit_index  = __ffs(this- >pending[word_index]);
+
+	    return word_index * BITS_PER_LONG + bit_index;	
+	}
+}
+
+static inline void bitarray_set(struct bitarray *this, int nr)
+{
+	__set_bit(nr, &this- >pending);
+	__set_bit(nr / BITS_PER_LONG, &this- >summary); 
+} 
+
+static inline void bitarray_clear(struct bitarray *this, int nr)
+{
+	int word = nr / BITS_PER_LONG;
+
+	__clear_bit(nr, &this- >pending);
+	if (!this- >pending[word])
+		__clear_bit(word, &this- >summary);

drivers/kvm/kvm_userint.c line 85

Quoted 6 lines

drivers/kvm/kvm_userint.c new file +6 -0

+}
+
+static inline int bitarray_test(struct bitarray *this, int nr)
+{
+	return test_bit(nr, &this- >pending);
+}

This should go into a separate header, probably even in include/linux/

drivers/kvm/kvm_userint.c line 95

Quoted 10 lines

drivers/kvm/kvm_userint.c new file +9 -0

+/*----------------------------------------------------------------------
+ * userint interface -  provides the actual kvm_irqdevice implementation
+ *--------------------------------------------------------------------- */
+
+typedef struct {
+	spinlock_t      lock;
+	struct bitarray irq_pending;
+	int             nmi_pending;
+}kvm_userint;

Please just use a struct type instead of the typedef.

drivers/kvm/kvm_userint.c line 187

Quoted 6 lines

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

+int kvm_userint_init(struct kvm_irqdevice *dev)
+{
+	kvm_userint *s;
+
+	s = (kvm_userint *)kzalloc(sizeof(kvm_userint), GFP_KERNEL);

No need to case the kzalloc return value. But you need to check the return value for NULL and handle the error.