KVM in-kernel APIC update

My current thoughts are that we at least move the IOAPIC into the kernel as well. That will give sufficient control to generate ISA bus interrupts for guests that understand APICs. If we want to be able to generate ISA interrupts for legacy guests which talk to the 8259s that will prove to be insufficient. The good news is that moving the 8259s down as well is probably not a huge deal either, especially since I have already prepped the usermode side. Thoughts?

So heres a question for you guys out there. What is the expected use of the in-kernel APIC? My interests lie in the ability to send IPIs for SMP, as well as being able to inject asynchronous hypercall interrupts. I assume there are other reasons too, such as PV device interrupts, etc and I would like to make sure I am seeing the big picture before making any bad design decisions. My question is, how do we expect the PV devices to look from a bus perspective?

The current Bochs/QEMU system model paints a fairly simple ISA architecture utilizing a single IOAPIC + dual 8259 setup. Do we expect in-kernel injected IRQs to follow the ISA model (e.g. either legacy or PCI interrupts only limited to IRQ0-15) or do we want to expand on this? The PCI hypercall device introduced a while back would be an example of something ISA based. Alternatives would be to utilize unused "pins" (such as IRQ16-23) on IOAPIC #0, or introducing new an entirely new bus/IOAPICs just for KVM, etc.

If the latter, we also need to decide what the resource conveyance model and vector allocation policy should be. For instance, do we publish said resources formally in the MP/ACPI tables in Bochs? Doing so would allow MP/ACPI compliant OSs like linux to naturally route the IRQ. Conversely, do we do something more direct just like we do for KVM discovery via wrmsr?

struct kvm_vcpu;

drivers/kvm/kvm.h +2 -0

+struct kvm_irqinfo {
+	int         vector;

drivers/kvm/kvm.h +25 -4

+	atomic_t		timer_pending;
+	u64	           	apic_base_msr;
+	unsigned long      	base_address;
+	u32	           	timer_divide_count;
+	struct hrtimer       	apic_timer;
+	int                	intr_pending_count[MAX_APIC_INT_VECTOR];
+	ktime_t           	timer_last_update;
+	struct {
+		int deliver_mode;
+		int source[6];
+	} direct_intr;
+	u32	           	err_status;
+	u32	           	err_write_count;
+	struct kvm_vcpu        	*vcpu;
+	struct page	   	*regs_page;
+	void               	*regs;
+	struct kvm_irqdevice    ext; /* Used for external/NMI interrupts */
+};
+
 /*
  * 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

@@ -236,6 +281,11 @@ struct kvm_pio_request {
 	int rep;
 };
 
+#define KVM_VCPU_INIT_SIPI_SIPI_STATE_NORM          0
+#define KVM_VCPU_INIT_SIPI_SIPI_STATE_WAIT_SIPI     1
+
+#define NR_IRQ_WORDS KVM_IRQ_BITMAP_SIZE(unsigned long)
+
 struct kvm_vcpu {
 	struct kvm *kvm;
 	union {

@@ -248,12 +298,9 @@ 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];
 	unsigned long regs[NR_VCPU_REGS]; /* for rsp: vcpu_load_rsp_rip() */
 	unsigned long rip;      /* needs vcpu_load_rsp_rip() */
-
+	struct kvm_irqdevice irq_dev;
 	unsigned long cr0;
 	unsigned long cr2;
 	unsigned long cr3;

drivers/kvm/kvm.h +0 -1

 	u64 pdptrs[4]; /* pae */
 	u64 shadow_efer;
-	u64 apic_base;
 	u64 ia32_misc_enable_msr;
 	int nmsrs;

drivers/kvm/kvm.h +5 -0

 	struct vmx_msr_entry *guest_msrs;

@@ -298,6 +343,11 @@ struct kvm_vcpu {
 	int sigset_active;
 	sigset_t sigset;
 
+	struct kvm_apic apic;
+	wait_queue_head_t halt_wq;
+	/* For AP startup */
+	unsigned long init_sipi_sipi_state;
+
 	struct {
 		int active;
 		u8 save_iopl;

drivers/kvm/kvm_main.c +4 -0

+	case KVM_GET_OPTIONS: {
+		r = -EFAULT;
+		if (copy_to_user(&kvm->options, argp, sizeof(kvm->options)))
+		    goto out;

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

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

drivers/kvm/svm.c +0 -8

@@ -108,20 +109,12 @@ 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;

Quoted 1 line

Quoted 1 line

On Wed, Apr 4, 2007 at 3:40 AM, in message <46135686.4090905-atKUWr5tajBWk0Htik3J/[email protected]>,

I would avoid moving down anything that's not strictly necessary.

I still don't have an opinion as to whether it is necessary; I'll need to study the details. Xen pushes most of the platform into the hypervisor, but then Xen's communication path to qemu is much more expensive (involving the scheduler and a potential cpu switch) than kvm. We'd need to balance possible performance improvements (I'd expect negligible) and interface simplification (possibly non-negligible) against further diverging from qemu.

There are two extreme models, which I think are both needed. On one end, support for closed OSes (e.g. Windows) requires fairly strict conformance to the PCI model, which means going through the IOAPIC or PIC or however the interrupt lines are wired in qemu. This seems to indicate that an in-kernel IOAPIC is needed. On the other end (Linux),
a legacy-free and emulation-free device can just inject interrupts directly and use shared memory to ack interrupts and indicate their source.

Quoted 2 lines

If the latter, we also need to decide what the resource conveyance model and

vector allocation policy should be. For instance, do we publish said resources formally in the MP/ACPI tables in Bochs? Doing so would allow MP/ACPI compliant OSs like linux to naturally route the IRQ. Conversely, do we do something more direct just like we do for KVM discovery via wrmsr?

Quoted 1 line

I think we can go the direct route for cooperative guests.

I also suggest doing the work in stages and measuring; that is, first push the local apic, determine what the remaining bottlenecks are and tackle them. I'm pretty sure that Linux guests would only require the local apic but Windows (and older Linux kernels) might require more.

Quoted 2 lines

struct kvm_vcpu;

drivers/kvm/kvm.h struct vmcs { · line 171

Quoted 13 lines

drivers/kvm/kvm.h +12 -0

+struct kvm_irqinfo {
+	int         vector;
+	int         nmi;
+};
+
+#define KVM_IRQFLAGS_NMI  (1 << 0)
+#define KVM_IRQFLAGS_PEEK (1 << 1)
+
+struct kvm_irqdevice {
+	int  (*pending)(struct kvm_irqdevice *this, int flags);
+	int  (*read)(struct kvm_irqdevice *this, int flags, 
+		     struct kvm_irqinfo *info);

Aren't pending() and read() + PEEK overlapping?

drivers/kvm/kvm.h struct vmcs { · line 177

Quoted 8 lines

drivers/kvm/kvm.h +6 -0

+	int  (*inject)(struct kvm_irqdevice *this, int irq, int flags);
+	int  (*summary)(struct kvm_irqdevice *this, void *data);
+	void (*destructor)(struct kvm_irqdevice *this);
+
+	void *private;
+};

Consider using container_of() to simulate C++ inheritance. Messier but
less indirections. Also consider a kvm_irqdevice_operations structure.

drivers/kvm/kvm.h struct vmcs { · line 185

Quoted 10 lines

drivers/kvm/kvm.h +8 -0

+
+#define MAX_APIC_INT_VECTOR      256
+
+struct kvm_apic {
+	u32	           	status;
+	u32	           	vcpu_id;
+	spinlock_t 		lock;
+	u32			pcpu_lock_owner;

Isn't this vcpu->cpu?

Quoted 49 lines

drivers/kvm/kvm.h +25 -4

+	atomic_t		timer_pending;
+	u64	           	apic_base_msr;
+	unsigned long      	base_address;
+	u32	           	timer_divide_count;
+	struct hrtimer       	apic_timer;
+	int                	intr_pending_count[MAX_APIC_INT_VECTOR];
+	ktime_t           	timer_last_update;
+	struct {
+		int deliver_mode;
+		int source[6];
+	} direct_intr;
+	u32	           	err_status;
+	u32	           	err_write_count;
+	struct kvm_vcpu        	*vcpu;
+	struct page	   	*regs_page;
+	void               	*regs;
+	struct kvm_irqdevice    ext; /* Used for external/NMI interrupts */
+};
+
 /*
  * 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

@@ -236,6 +281,11 @@ struct kvm_pio_request {
 	int rep;
 };
 
+#define KVM_VCPU_INIT_SIPI_SIPI_STATE_NORM          0
+#define KVM_VCPU_INIT_SIPI_SIPI_STATE_WAIT_SIPI     1
+
+#define NR_IRQ_WORDS KVM_IRQ_BITMAP_SIZE(unsigned long)
+
 struct kvm_vcpu {
 	struct kvm *kvm;
 	union {

@@ -248,12 +298,9 @@ 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];
 	unsigned long regs[NR_VCPU_REGS]; /* for rsp: vcpu_load_rsp_rip() */
 	unsigned long rip;      /* needs vcpu_load_rsp_rip() */
-
+	struct kvm_irqdevice irq_dev;
 	unsigned long cr0;
 	unsigned long cr2;
 	unsigned long cr3;

drivers/kvm/kvm.h struct kvm_vcpu { · line 311

Quoted 6 lines

drivers/kvm/kvm.h +0 -1

@@ -261,10 +308,8 @@ struct kvm_vcpu {
 	struct page *para_state_page;
 	gpa_t hypercall_gpa;
 	unsigned long cr4;
-	unsigned long cr8;
 

I think we should keep cr8 here and accept the duplication.

Quoted 23 lines

drivers/kvm/kvm.h +0 -1

 	u64 pdptrs[4]; /* pae */
 	u64 shadow_efer;
-	u64 apic_base;
 	u64 ia32_misc_enable_msr;
 	int nmsrs;

drivers/kvm/kvm.h +5 -0

 	struct vmx_msr_entry *guest_msrs;

@@ -298,6 +343,11 @@ struct kvm_vcpu {
 	int sigset_active;
 	sigset_t sigset;
 
+	struct kvm_apic apic;
+	wait_queue_head_t halt_wq;
+	/* For AP startup */
+	unsigned long init_sipi_sipi_state;
+
 	struct {
 		int active;
 		u8 save_iopl;

@@ -319,6 +369,23 @@ struct kvm_mem_alias {
 	gfn_t target_gfn;
 };
 

drivers/kvm/kvm.h struct kvm_mem_alias {

Quoted 5 lines

drivers/kvm/kvm.h +4 -0

+#define kvm_irq_pending(dev, flags)     (dev)->pending(dev, flags)
+#define kvm_irq_read(dev, flags, info)  (dev)->read(dev, flags, info)
+#define kvm_irq_inject(dev, irq, flags) (dev)->inject(dev, irq, flags)
+#define kvm_irq_summary(dev, data)      (dev)->summary(dev, data)
 

Static inlines please, no #defines. In this case, I'd just call the functions directly.

drivers/kvm/kvm.h struct kvm_mem_alias {

Quoted 12 lines

drivers/kvm/kvm.h +10 -0

+
+#define kvm_vcpu_irq_pending(vcpu, flags) \
+   kvm_irq_pending(&vcpu->irq_dev, flags)
+#define kvm_vcpu_irq_read(vcpu, flags, info) \
+   kvm_irq_read(&vcpu->irq_dev, flags, info)
+#define kvm_vcpu_irq_inject(vcpu, irq, flags) \
+   kvm_irq_inject(&vcpu->irq_dev, irq, flags)
+#define kvm_vcpu_irq_summary(vcpu, data)  \
+   kvm_irq_summary(&vcpu->irq_dev, data)
+
 

Ditto.

drivers/kvm/kvm_main.c static int emulator_write_std(unsigned long addr, · line 1044

Quoted 13 lines

drivers/kvm/kvm_main.c +10 -0

+struct kvm_mmio_handler {
+	unsigned long (*read)(struct kvm_vcpu *v,
+			      unsigned long addr,
+			      unsigned long length);
+	void (*write)(struct kvm_vcpu *v,
+			   unsigned long addr,
+			   unsigned long length,
+			   unsigned long val);
+	int (*in_range)(struct kvm_vcpu *v, unsigned long addr);
+};

This would need to be split out into a separate patch. Addresses are gpa_t.

drivers/kvm/kvm_main.c static int emulator_write_std(unsigned long addr,

Quoted 4 lines

drivers/kvm/kvm_main.c +3 -0

+
+#define KVM_MMIO_HANDLER_NR_ARRAY_SIZE 1
+static struct kvm_mmio_handler 

*kvm_mmio_handlers[KVM_MMIO_HANDLER_NR_ARRAY_SIZE] =

drivers/kvm/kvm_main.c static int emulator_write_std(unsigned long addr, · line 1057

Quoted 5 lines

drivers/kvm/kvm_main.c +4 -0

+{
+    &apic_mmio_handler,
+};
+

We'll need dynamic registration if we want kernel apic support to be optional.

Also, this needs to be per-vcpu as each vcpu's apic can be placed at a different address.

Review int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32

Quoted 3 lines

(quoted diff) +0 -0

@@ -1479,7 +1533,11 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 

msr, u64 *pdata)

drivers/kvm/kvm_main.c int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)

Quoted 11 lines

drivers/kvm/kvm_main.c +5 -1

 		data = 3;
 		break;
 	case MSR_IA32_APICBASE:
-		data = vcpu->apic_base;
+		data = vcpu->apic.apic_base_msr;
+		break;
+	case MSR_IA32_TIME_STAMP_COUNTER:
+		// FIXME
+                //data = guest_read_tsc();
 		break;
 

The tsc stuff looks spurious.

Quoted 11 lines

drivers/kvm/kvm_main.c +10 -0

+	case KVM_GET_OPTIONS: {
+		r = -EFAULT;
+		if (copy_to_user(&kvm->options, argp, sizeof(kvm->options)))
+		    goto out;
+		r = 0;
+		break;
+	}
+	case KVM_SET_OPTIONS: {
+		r = -EFAULT;
+		if (copy_from_user(&kvm->options, argp, sizeof(kvm->options)))

drivers/kvm/kvm_main.c static long kvm_vm_ioctl(struct file *filp, · line 2741

Quoted 5 lines

drivers/kvm/kvm_main.c +4 -0

+		    goto out;
+		r = 0;
+		break;
+	}

This is difficult to expand over time. I prefer a separate ioctl() for each option.

Quoted 10 lines

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

+
+struct bitarray {
+    unsigned long summary; /* 1 per word in pending */
+    unsigned long pending[NR_IRQ_WORDS];
+};
+
+static int bitarray_pending(struct bitarray *this)
+{
+	return this->summary ? 1 : 0;	

drivers/kvm/kvm_userint.c line 45

Quoted 6 lines

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

+}
+
+static int bitarray_findhighest(struct bitarray *this)
+{
+	if(!this->summary)

Space after if, here and elsewhere.

Quoted 4 lines

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

+		return 0;
+

drivers/kvm/kvm_userint.c line 57

Quoted 10 lines

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

+	int word_index = __ffs(this->summary);
+	int bit_index = __ffs(this->pending[word_index]);
+
+	return word_index * BITS_PER_LONG + bit_index;	
+}
+
+static void bitarray_set(struct bitarray *this, int nr)
+{
+	set_bit(nr, &this->pending);
+	set_bit(nr / BITS_PER_LONG, &this->summary); 

Since you need locking anyway, best to use the unlocked versions (__set_bit()).

drivers/kvm/kvm_userint.c line 80

Quoted 5 lines

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

+typedef struct {
+	struct bitarray irq_pending;
+	struct bitarray nmi_pending;

Why is nmi_pending a bitarray?

Quoted 16 lines

drivers/kvm/svm.c +1 -8

@@ -108,20 +109,12 @@ 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_vcpu_irq_read(vcpu, 0, NULL);
 }
 

drivers/kvm/svm.c static unsigned get_addr_size(struct kvm_vcpu *vcpu) · line 119

Quoted 7 lines

drivers/kvm/svm.c +1 -2

 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);
+	kvm_vcpu_irq_inject(vcpu, irq, 0);
 }
 

It would be helpful to unify the vmx and svm irq code first (I can merge something like that immediately), so this patch becomes simpler.

Signature · 3 lines

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to 
panic.

What I was planning on doing was using that QEMU patch I provided to intercept all pic_send_irq() calls and forward them directly to the kernel via a new ioctl(). This ioctl would be directed at the VM fd, not the VCPU, since its a pure ISA global pin reference and wont know the targeted vcpu until the 8259/IOAPIC perform their translation.

So that being said, I think the interface between userspace and kernel would be no more complex than it is today. I.e. we would just be passing a single int via an ioctl.

The danger, as you have pointed out, is accepting the QEMU patch that I submitted that potentially diverges the pic code from QEMU upstream. What this buys us, however, is is that any code (kernel or userspace) would be able to inject an ISA interrupt. Using an ISA interrupt has the advantage of already being represented in the ACPI/MP tables presented by Bochs, and thus any compliant OS will automatically route the IRQ.

Where things do get potentially complicated with the interface is if we go with a hybrid solution. Leaving the LAPIC in kernel, but the IOAPIC/8259 in userspace requires a much wider interface so the IOAPIC can deliver APIC style messages to the kernel. Also, IOAPIC EOI for level sensitive interrupts become more difficult and complex. Putting LAPIC + IOAPIC#0 in the kernel and leaving 8259 outside might actually work fairly well, but in-kernel ISA interrupts will only work with OSs which enable the APIC. This may or may not be an issue and may be an acceptable tradeoff.

Quoted 8 lines

There are two extreme models, which I think are both needed. On one end, support for closed OSes (e.g. Windows) requires fairly strict conformance to the PCI model, which means going through the IOAPIC or PIC or however the interrupt lines are wired in qemu. This seems to indicate that an in-kernel IOAPIC is needed. On the other end (Linux),
a legacy-free and emulation-free device can just inject interrupts directly and use shared memory to ack interrupts and indicate their source.

Well, sort of. The problem as I see it in both cases is still IRQ routing. If you knew of a free vector to take (regardless of OS) you could forgo the ACPI/MP declarations all together and register the vector with your "device" (e.g. via a hypercall, etc) and have the device issue direct LAPIC messages with the proper vector. I think this would work assuming the device used edge triggered interrupts (which don't require IOAPIC IRR registration or EOI).

Quoted 5 lines

Quoted 2 lines

Since you need locking anyway, best to use the unlocked versions (__set_bit()).

Ack. Yes, locking needs to be added. Votes for appropriate mechanism? (e.g. spinlock, mutex, etc?)

Quoted 16 lines

Quoted 16 lines

drivers/kvm/svm.c +1 -8

@@ -108,20 +109,12 @@ 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_vcpu_irq_read(vcpu, 0, NULL);
 }
 

Quoted 11 lines

drivers/kvm/svm.c static unsigned get_addr_size(struct kvm_vcpu *vcpu)

Quoted 8 lines

drivers/kvm/svm.c +1 -2

 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);
+	kvm_vcpu_irq_inject(vcpu, irq, 0);
 }
 
 

It would be helpful to unify the vmx and svm irq code first (I can merge something like that immediately), so this patch becomes simpler.

Well, I could just change any occurrence of "pop_irq" with kvm_vcpu_irq_read() and any occurrence of push_irq() to kvm_vcpu_irq_inject(), but I don't get the impression that this is what you were referring to. Could you elaborate?

Quoted 1 line

Quoted 1 line

On Wed, Apr 4, 2007 at 12:49 PM, in message <4613D736.1080207-atKUWr5tajBWk0Htik3J/[email protected]>,

Gregory Haskins wrote:

Hmm. If the ioapic is in the kernel, then it's a platform- wide resource and you would need a vm ioctl. If ioapic emulation is in userspace, then the ioapic logic will have decided which cpu is targeted and you would issue a vcpu ioctl.

Quoted 1 line

So that being said, I think the interface between userspace and kernel would

be no more complex than it is today. I.e. we would just be passing a single int via an ioctl.

I think the interface should mirror the hardware interface at the point of the "cut". For example, if we keep the ioapic in userspace, the interface is ioapic/apic bus messages. If we push the ioapic into the kernel, the interface describes the various ioapic pins and how the ioapics are connected to each other and to the processors (i.e. the topology).

Everything should keep working, that is a must. We just need the interfaces to follow the hardware faithfully. The issue with the ioapic eoi is worrying me performance wise, though; it looks like we need to push the ioapic too if we are to have no- compromise performance on unmodified OSes.

For unmodified guests, use the existing pci irq routing. I certainly wouldn't want to debug anything else. For modified guests, there's no real problem.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

Since you need locking anyway, best to use the unlocked versions (__set_bit()).

Quoted 1 line

Ack. Yes, locking needs to be added. Votes for appropriate mechanism?

(e.g. spinlock, mutex, etc?)

Quoted 2 lines

spin_lock_irq(), as this lock will frequently have to be taken in host irq handlers. Need to be extra careful with the locking in {vmx,svm}_vcpu_run().

Quoted 1 line

Well, I could just change any occurrence of "pop_irq" with

kvm_vcpu_irq_read() and any occurrence of push_irq() to kvm_vcpu_irq_inject(), but I don't get the impression that this is what you were referring to. Could you elaborate?

Quoted 1 line

I meant having a cleanup patch before that pushes irq handling into common code, then the apic patch could modify that to call the kernel apic code if necessary.

Agreed. I was thinking that the interface for the "IOAPIC in kernel" model would look something like the way the pic_send_irq() function looks, except it would also convey BUS/IOAPIC id.

so: kvm_inject_interrupt(int bus, int pin, int value);

and the "kvmpic" driver would currently translate as bus = 0 (giving us IRQ0-23). E.g.

kvmpic_send_irq(int irq, int value)

{
   kvm_inject_interrupt(0, irq, value);
}

Quoted 8 lines

Everything should keep working, that is a must. We just need the interfaces to follow the hardware faithfully. The issue with the ioapic eoi is worrying me performance wise, though; it looks like we need to push the ioapic too if we are to have no- compromise performance on unmodified OSes.

Not sure if this will make you feel better, but it appears as though both the QEMU and the in-kernel model that I inherited don't accurately support IOAPIC EOI already. From that you can infer that there must not be any level-sensitive interrupts in use today. Since the system seems to only have the legacy ISA model (which dictates edge triggers, IIRC), this makes sense. However, if we want to support level triggers in the future, we will have to address this. I would be uncomfortable designing something that doesn't take the IOAPIC EOI into account, even if its not immediately used.

Quoted 4 lines

For unmodified guests, use the existing pci irq routing. I certainly wouldn't want to debug anything else. For modified guests, there's no real problem.

Ill take your word for it ;) I spent a few minutes looking through the linux kernel trying to figure out how to get a hint about a free vector without assigning one statically in the header files and came up empty handed. Im sure there is a way (or maybe static is ok?).

Quoted 1 line

Quoted 1 line

On Wed, Apr 4, 2007 at 1:43 PM, in message <4613E3CB.6000904-atKUWr5tajBWk0Htik3J/[email protected]>,

Quoted 1 line · Gregory Haskins

Agreed. I was thinking that the interface for the "IOAPIC in kernel" model

would look something like the way the pic_send_irq() function looks, except it would also convey BUS/IOAPIC id.

Quoted 4 lines

so: kvm_inject_interrupt(int bus, int pin, int value);

and the "kvmpic" driver would currently translate as bus = 0 (giving us

IRQ0- 23). E.g.

Quoted 1 line

Quoted 5 lines

kvmpic_send_irq(int irq, int value)

{
   kvm_inject_interrupt(0, irq, value);
}

With appropriate modeling of edge vs level triggered, and translation of pin to vector, yes.

pci is level triggered, so maybe the guests just handle the inaccuracy.

Quoted 3 lines

pci is level triggered, so maybe the guests just handle the inaccuracy.

Good point. I'm not sure how this works today. Perhaps we just get lucky that nothing checks the IRR in the IOAPIC coupled with a bug in the IOAPIC model that an APIC message is sent out even if the interrupt is not acknowledged. It would explain why it works today, anyway. Either way, I would like to get this modeled "right" this go-round, so the point is moot.

there is a remote possibility that some OSs depend on certain devices being level-triggered: for example if you get an IRQ from a level-triggered device and _dont_ deassert that signal from the IRQ handler (intentionally so), then the semantics of current hardware will cause a second interrupt to be sent by the PIC, after the APIC message has been EOI-ed in the local APIC. While such "repeat interrupts" would be pure madness to rely on i think, i'm not sure it's not being done. Note that if the same IO-APIC pin is set up to detect edges then not deasserting the signal would not cause a 'repeat interrupt'. Whether such accurate emulation of signalling is needed depends on the hardware semantics of the devices we emulate.

Quoted 4 lines

Gregory Haskins wrote:

Hmm. If the ioapic is in the kernel, then it's a platform- wide

Quoted 4 lines

and you would need a vm ioctl. If ioapic emulation is in userspace, then the ioapic logic will have decided which cpu is targeted and you would issue a vcpu ioctl.

Thats exactly in line with my thinking.

Quoted 2 lines

Quoted 2 lines

So that being said, I think the interface between userspace and

would

Quoted 1 line

be no more complex than it is today. I.e. we would just be passing a

single

Quoted 3 lines

int via an ioctl.

I think the interface should mirror the hardware interface at the

Quoted 2 lines

of the "cut". For example, if we keep the ioapic in userspace, the interface is ioapic/apic bus messages. If we push the ioapic into

Quoted 3 lines

kernel, the interface describes the various ioapic pins and how the ioapics are connected to each other and to the processors (i.e. the topology).

Agreed. I was thinking that the interface for the "IOAPIC in kernel"

would look something like the way the pic_send_irq() function looks,

it would also convey BUS/IOAPIC id.

Quoted 8 lines

My current thoughts are that we at least move the IOAPIC into the kernel as well. That will give sufficient control to generate ISA bus interrupts for guests that understand APICs. If we want to be able to generate ISA interrupts for legacy guests which talk to the 8259s that will prove to be insufficient. The good news is that moving the 8259s down as well is probably not a huge deal either, especially since I have already prepped the usermode side. Thoughts?

I would avoid moving down anything that's not strictly necessary. If we want to keep the PIC in qemu, for example, just export the APIC-PIC interface to qemu.

we should move all the PICs into KVM proper - and that includes the i8259A PIC too. Qemu-space drivers are then wired to pins on these

but nothing in Qemu does vector generation or vector prioritization - that task is purely up to KVM. There are mixed i8259A+lapic models possible too and the simplest model is to have all vector handling in KVM.

any 'cut' of the interface to allow both qemu and KVM generate vectors is unnecessary (and harmful) complexity. The interface cut should be at the 'pin' level, with Qemu raising a signal on a pin and lowering a signal on a pin, but otherwise not dealing with IRQ routing and IRQ vectors.

Ingo

-----------------------------------------------------------------------

Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVD

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

we should move all the PICs into KVM proper - and that includes the i8259A PIC too. Qemu-space drivers are then wired to pins on these PICs, but nothing in Qemu does vector generation or vector prioritization - that task is purely up to KVM. There are mixed i8259A+lapic models possible too and the simplest model is to have all vector handling in KVM.

any 'cut' of the interface to allow both qemu and KVM generate vectors is unnecessary (and harmful) complexity. The interface cut should be at the 'pin' level, with Qemu raising a signal on a pin and lowering a signal on a pin, but otherwise not dealing with IRQ routing and IRQ vectors.

Quoted 2 lines · Ingo Molnar

we should move all the PICs into KVM proper - and that includes the i8259A PIC too. Qemu-space drivers are then wired to pins on these

Quoted 6 lines

but nothing in Qemu does vector generation or vector prioritization - that task is purely up to KVM. There are mixed i8259A+lapic models possible too and the simplest model is to have all vector handling in KVM.

any 'cut' of the interface to allow both qemu and KVM generate

Quoted 1 line

is unnecessary (and harmful) complexity. The interface cut should be

Quoted 4 lines

the 'pin' level, with Qemu raising a signal on a pin and lowering a signal on a pin, but otherwise not dealing with IRQ routing and IRQ vectors.

Following is my view of the possible cuts:

- everything in userspace: worst performance, but needed for

- tpr in kernel: minimal effort, bad badly defined interface

- lapic in kernel: well defined (all on-chip stuff in kernel, off-chip in userspace), fixes main Windows problems, interrupts still need userspace. Interface is the processor's LINT pins and APIC bus.

- *pic in kernel: most effort, easiest irq synchronization. Interface is pic/ioapic pin level, plus a topology description that userspace

to tell the hardware how the pins are connected (logically required,

practically not).

- *pic in kernel: most effort, easiest irq synchronization. Interface is pic/ioapic pin level, plus a topology description that userspace uses to tell the hardware how the pins are connected (logically required, but practically not).

there's one thing i havent seen mentioned (maybe i missed it): the ability to save/restore the in-kernel *PIC state. That would be required for migration/snapshotting/etc.

* Avi Kivity <avi-atKUWr5tajBWk0Htik3J/[email protected]> wrote:

any 'cut' of the interface to allow both qemu and KVM generate vectors is unnecessary (and harmful) complexity. The interface cut should be at
the 'pin' level, with Qemu raising a signal on a pin and lowering a signal on a pin, but otherwise not dealing with IRQ routing and IRQ vectors.

Hi all,

Attached is a snapshot of my current efforts on the kernel side for the in-kernel APIC work. Feedback welcome.

My current thoughts are that we at least move the IOAPIC into the kernel as well. [...]

The current Bochs/QEMU system model paints a fairly simple ISA architecture utilizing a single IOAPIC + dual 8259 setup. Do we expect in-kernel injected IRQs to follow the ISA model (e.g. either legacy or PCI interrupts only limited to IRQ0-15) or do we want to expand on this? [...]

If the latter, we also need to decide what the resource conveyance model and vector allocation policy should be. For instance, do we publish said resources formally in the MP/ACPI tables in Bochs? Doing so would allow MP/ACPI compliant OSs like linux to naturally route the IRQ. Conversely, do we do something more direct just like we do for KVM discovery via wrmsr?

Quoted 1 line

Quoted 1 line

On Wed, Apr 4, 2007 at 4:32 PM, in message <[email protected]>,

Quoted 3 lines

My current thoughts are that we at least move the IOAPIC into the kernel as well. [...]

yes. And then do the final 10% move of handling the i8529A in KVM too.

for PV/accel drivers we dont need any extra ACPI enumeration - the hypercall API is good enough to connect to the hypervisor, and i suspect all guest OSs we care about allow drivers to allocate an IRQ vector for a new device, without having that device enumerated in ACPI.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

On Wed, Apr 4, 2007 at 4:32 PM, in message <[email protected]>,

Quoted 3 lines · Ingo Molnar

Quoted 3 lines

My current thoughts are that we at least move the IOAPIC into the kernel as well. [...]

Quoted 2 lines

yes. And then do the final 10% move of handling the i8529A in KVM too.

Hi Ingo,
We are in full agreement on this point, and has been my preferred model from the beginning. The only issue with this approach is that it requires a fairly disruptive patch to QEMUs "pic_set_irq()" feature which many people have drawn exception to so far. (In case you weren't following from the beginning, its the "QEMU PIC indirection patch...." thread).

If we dont care about supporting "--no-kvm" anymore, this problem becomes trivially easy.....

we can just link in a different pic module into QEMU and be done with it. The problem as I see it is that we really have a lot of value in being able to switch between kvm and pure qemu mode via --no-kvm, especially for debugging. Therefore, IMHO we need to be able to dynamically switch between PIC emulation code.

If we *do* want to go with this model, *and* we decide that the approach I have taken with QEMU is a reasonable way to do it, then I would suggest we go about it by getting the patch accepted in QEMU upstream. I would gladly take on this duty if we all agree this is the right approach.

Quoted 6 lines

for PV/accel drivers we dont need any extra ACPI enumeration - the hypercall API is good enough to connect to the hypervisor, and i suspect all guest OSs we care about allow drivers to allocate an IRQ vector for a new device, without having that device enumerated in ACPI.

If you know how to do this in Linux, please share! I was looking for this earlier and came up empty handed. All I could find was the places where the PCI/MP/ACPI type things assigned vectors to devices they new about. It's probably "operator-ignorance" ;)

Regards,
-Greg

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

On Wed, Apr 4, 2007 at 4:32 PM, in message <[email protected]>,

Ingo Molnar <[email protected]> wrote:

Quoted 5 lines

Quoted 3 lines

My current thoughts are that we at least move the IOAPIC into the kernel as well. [...]

yes. And then do the final 10% move of handling the i8529A in KVM too.

Hi Ingo,
We are in full agreement on this point, and has been my preferred model from the beginning. The only issue with this approach is that it requires a fairly disruptive patch to QEMUs "pic_set_irq()" feature which many people have drawn exception to so far. (In case you weren't following from the beginning, its the "QEMU PIC indirection patch...." thread).

If we dont care about supporting "--no-kvm" anymore, this problem becomes trivially easy.....we can just link in a different pic module into QEMU and be done with it. The problem as I see it is that we really have a lot of value in being able to switch between kvm and pure qemu mode via --no-kvm, especially for debugging. Therefore, IMHO we need to be able to dynamically switch between PIC emulation code.

If we *do* want to go with this model, *and* we decide that the approach I have taken with QEMU is a reasonable way to do it, then I would suggest we go about it by getting the patch accepted in QEMU upstream. I would gladly take on this duty if we all agree this is the right approach.