Re: [RFC][PATCH 0/2] Fix guest shared interrupt with in-kernel irqchip

[Sheng Yang <sheng@linux.intel.com>]

On Mon, 2008-10-06 at 14:23 +0800, Sheng Yang wrote:
> On Sat, 2008-10-04 at 11:47 +0300, Avi Kivity wrote:
> > Sheng Yang wrote:
> >> Derived from Avi's suggestion, now every call of kvm_set_irq() should offer
> >> a irq_source_id, which is allocated by kvm_allocate_irq_sources(). We based
> >> on irq_source_id to identify irq source and implement logical OR for shared
> >> level interrupts.
> >>
> >> The allocated irq_source_id can be freed by kvm_free_irq_sources().
> >>
> >>
> >> +
> >> +	u32 irq_sources_bitmap;
> >> +	u32 irq_states[KVM_IOAPIC_NUM_PINS];
> >>   
> >
> > You're using the bit manipulation functions on this, which use unsigned  
> > long parameters.  This will generate unaligned accesses, which will  
> > probably annoy ia64 when this code is ported.  So it may be better to  
> > wast some space and use unsigned longs here.
> >
> > Other than that, looks a nice and clean to me.
> >
> Yes, of course. Thanks for point it out. :)
> 
> Update the patch with unsigned long.
> 
Oops, miss one harmless point...
Continue updating.

--
From: Sheng Yang <sheng@linux.intel.com>
Date: Mon, 6 Oct 2008 14:32:06 +0800
Subject: [PATCH 1/1] KVM: Fix guest shared interrupt with in-kernel irqchip

Derived from Avi's suggestion, now every call of kvm_set_irq() should offer
a irq_source_id, which is allocated by kvm_allocate_irq_sources(). We based
on irq_source_id to identify irq source and implement logical OR for shared
level interrupts.

The allocated irq_source_id can be freed by kvm_free_irq_sources().

Now we support sizeof(unsigned long) different irq sources at most.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/kvm/i8254.c       |   11 +++++++++--
 arch/x86/kvm/i8254.h       |    1 +
 arch/x86/kvm/irq.c         |   42 +++++++++++++++++++++++++++++++++++++++---
 arch/x86/kvm/irq.h         |    7 ++++++-
 arch/x86/kvm/x86.c         |   24 ++++++++++++++++++------
 include/asm-x86/kvm_host.h |    3 +++
 include/linux/kvm_host.h   |    1 +
 7 files changed, 77 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 6144d3f..da06919 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -545,6 +545,12 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm)
 	if (!pit)
 		return NULL;
 
+	mutex_lock(&kvm->lock);
+	pit->irq_source_id = kvm_request_irq_source_id(kvm);
+	mutex_unlock(&kvm->lock);
+	if (pit->irq_source_id < 0)
+		return NULL;
+
 	mutex_init(&pit->pit_state.lock);
 	mutex_lock(&pit->pit_state.lock);
 	spin_lock_init(&pit->pit_state.inject_lock);
@@ -587,6 +593,7 @@ void kvm_free_pit(struct kvm *kvm)
 		mutex_lock(&kvm->arch.vpit->pit_state.lock);
 		timer = &kvm->arch.vpit->pit_state.pit_timer.timer;
 		hrtimer_cancel(timer);
+		kvm_free_irq_source_id(kvm, kvm->arch.vpit->irq_source_id);
 		mutex_unlock(&kvm->arch.vpit->pit_state.lock);
 		kfree(kvm->arch.vpit);
 	}
@@ -598,8 +605,8 @@ static void __inject_pit_timer_intr(struct kvm *kvm)
 	int i;
 
 	mutex_lock(&kvm->lock);
-	kvm_set_irq(kvm, 0, 1);
-	kvm_set_irq(kvm, 0, 0);
+	kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
+	kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
 	mutex_unlock(&kvm->lock);
 
 	/*
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index e436d49..4178022 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -44,6 +44,7 @@ struct kvm_pit {
 	struct kvm_io_device speaker_dev;
 	struct kvm *kvm;
 	struct kvm_kpit_state pit_state;
+	int irq_source_id;
 };
 
 #define KVM_PIT_BASE_ADDRESS	    0x40
diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 8c1b9c5..69d6e40 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -101,14 +101,22 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu)
 }
 
 /* This should be called with the kvm->lock mutex held */
-void kvm_set_irq(struct kvm *kvm, int irq, int level)
+void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
 {
+	unsigned long *irq_state = &kvm->arch.irq_states[irq];
+
+	/* Logical OR for level trig interrupt */
+	if (level)
+		set_bit(irq_source_id, irq_state);
+	else
+		clear_bit(irq_source_id, irq_state);
+
 	/* Not possible to detect if the guest uses the PIC or the
 	 * IOAPIC.  So set the bit in both. The guest will ignore
 	 * writes to the unused one.
 	 */
-	kvm_ioapic_set_irq(kvm->arch.vioapic, irq, level);
-	kvm_pic_set_irq(pic_irqchip(kvm), irq, level);
+	kvm_ioapic_set_irq(kvm->arch.vioapic, irq, !!(*irq_state));
+	kvm_pic_set_irq(pic_irqchip(kvm), irq, !!(*irq_state));
 }
 
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi)
@@ -132,3 +140,31 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
 {
 	hlist_del(&kian->link);
 }
+
+/* The caller must hold kvm->lock mutex */
+int kvm_request_irq_source_id(struct kvm *kvm)
+{
+	unsigned long *bitmap = &kvm->arch.irq_sources_bitmap;
+	int irq_source_id = find_first_zero_bit(bitmap,
+				sizeof(kvm->arch.irq_sources_bitmap));
+	if (irq_source_id >= sizeof(kvm->arch.irq_sources_bitmap)) {
+		printk(KERN_WARNING "kvm: exhaust allocatable IRQ sources!\n");
+		irq_source_id = -EFAULT;
+	} else
+		set_bit(irq_source_id, bitmap);
+	return irq_source_id;
+}
+
+void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
+{
+	int i;
+
+	if (irq_source_id <= 0 ||
+	    irq_source_id >= sizeof(kvm->arch.irq_sources_bitmap)) {
+		printk(KERN_ERR "kvm: IRQ source ID out of range!\n");
+		return;
+	}
+	for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
+		clear_bit(irq_source_id, &kvm->arch.irq_states[i]);
+	clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
+}
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 9f157c9..b1987f5 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -32,6 +32,8 @@
 
 #define PIC_NUM_PINS 16
 
+#define KVM_USERSPACE_IRQ_SOURCE_ID	0
+
 struct kvm;
 struct kvm_vcpu;
 
@@ -85,7 +87,7 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
 
 void kvm_pic_reset(struct kvm_kpic_state *s);
 
-void kvm_set_irq(struct kvm *kvm, int irq, int level);
+void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);
@@ -103,4 +105,7 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu);
 int pit_has_pending_timer(struct kvm_vcpu *vcpu);
 int apic_has_pending_timer(struct kvm_vcpu *vcpu);
 
+int kvm_request_irq_source_id(struct kvm *kvm);
+void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id);
+
 #endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 675fcc1..6c3ac18 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -136,14 +136,12 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 	 */
 	mutex_lock(&assigned_dev->kvm->lock);
 	kvm_set_irq(assigned_dev->kvm,
+		    assigned_dev->irq_source_id,
 		    assigned_dev->guest_irq, 1);
 	mutex_unlock(&assigned_dev->kvm->lock);
 	kvm_put_kvm(assigned_dev->kvm);
 }
 
-/* FIXME: Implement the OR logic needed to make shared interrupts on
- * this line behave properly
- */
 static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev =
@@ -165,7 +163,7 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 
 	dev = container_of(kian, struct kvm_assigned_dev_kernel,
 			   ack_notifier);
-	kvm_set_irq(dev->kvm, dev->guest_irq, 0);
+	kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 0);
 	enable_irq(dev->host_irq);
 }
 
@@ -178,6 +176,8 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 
 	kvm_unregister_irq_ack_notifier(kvm, &assigned_dev->ack_notifier);
 
+	kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
+
 	if (cancel_work_sync(&assigned_dev->interrupt_work))
 		/* We had pending work. That means we will have to take
 		 * care of kvm_put_kvm.
@@ -319,15 +319,23 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 
 	list_add(&match->list, &kvm->arch.assigned_dev_head);
 
+	r = kvm_request_irq_source_id(kvm);
+	if (r < 0)
+		goto out_list_del;
+	else
+		match->irq_source_id = r;
+
 	if (assigned_dev->flags & KVM_DEV_ASSIGN_ENABLE_IOMMU) {
 		r = kvm_iommu_map_guest(kvm, match);
 		if (r)
-			goto out_list_del;
+			goto out_free_irq_source_id;
 	}
 
 out:
 	mutex_unlock(&kvm->lock);
 	return r;
+out_free_irq_source_id:
+	kvm_free_irq_source_id(kvm, match->irq_source_id);
 out_list_del:
 	list_del(&match->list);
 	pci_release_regions(dev);
@@ -1993,7 +2001,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 		if (irqchip_in_kernel(kvm)) {
 			mutex_lock(&kvm->lock);
-			kvm_set_irq(kvm, irq_event.irq, irq_event.level);
+			kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
+				    irq_event.irq, irq_event.level);
 			mutex_unlock(&kvm->lock);
 			r = 0;
 		}
@@ -4313,6 +4322,9 @@ struct  kvm *kvm_arch_create_vm(void)
 	INIT_LIST_HEAD(&kvm->arch.active_mmu_pages);
 	INIT_LIST_HEAD(&kvm->arch.assigned_dev_head);
 
+	/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
+	kvm->arch.irq_sources_bitmap = 1;
+
 	return kvm;
 }
 
diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
index 53995a8..dfee063 100644
--- a/include/asm-x86/kvm_host.h
+++ b/include/asm-x86/kvm_host.h
@@ -365,6 +365,9 @@ struct kvm_arch{
 
 	struct page *ept_identity_pagetable;
 	bool ept_identity_pagetable_done;
+
+	unsigned long irq_sources_bitmap;
+	unsigned long irq_states[KVM_IOAPIC_NUM_PINS];
 };
 
 struct kvm_vm_stat {
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 73b7c52..61c378c 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -303,6 +303,7 @@ struct kvm_assigned_dev_kernel {
 	int host_irq;
 	int guest_irq;
 	int irq_requested;
+	int irq_source_id;
 	struct pci_dev *dev;
 	struct kvm *kvm;
 };
-- 
1.5.4.5