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

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

[Yang, Sheng]

To deal with guest shared interrupt bug in in-kernel irqchip, we should:

1. Identify each level trig interrupt source.
2. Implement logical OR on the same IRQ line for each interrupt source.

Here I chose a simple method: the caller of kvm_set_irq() has responsiblity
to identify interrupt sources, and IOAPIC/PIC ensure logical OR on IRQ line.

The alternative method of identify can be: a process to
request/allocate/free device identity, then kvm_set_irq() has responsibility
to identify interrupt sources. But I think it's too complicate and
unnecessary, for the caller of set_irq() should aware of the IRQ state.

The patch treats all userspace devices as one source. This have been ensured
by QEmu, which would ensure logical OR on IRQ line if IRQ line is sharing in
userspace.

Comments are welcome! And patches are untested, due to our boxes are down
during the holiday(and my Linux Desktop in the company also down, have
to post patch with outlook)...

Thanks!
--
regards
Yang, Sheng

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

[Avi Kivity]

Yang, Sheng wrote:
> To deal with guest shared interrupt bug in in-kernel irqchip, we should:
>
> 1. Identify each level trig interrupt source.
> 2. Implement logical OR on the same IRQ line for each interrupt source.
>
> Here I chose a simple method: the caller of kvm_set_irq() has responsiblity
> to identify interrupt sources, and IOAPIC/PIC ensure logical OR on IRQ line.
>   

The downside is that every caller has to do this edge detection.

How about allocating a vector of u32s (one per irq), and each source 
will allocate a bit within this vector.  The 'or' operation becomes 
(word != 0).

For example:

   irq_src = kvm_irq_allocate_source(kvm);  /* allocate bit within irq 
vector */

    ...
 
   kvm_set_irq(kvm, irq, 1, irq_src);

kvm_set_irq(...)
{
    // locking?
    if (level)
        set_bit(irq_src, &kvm->irq_state[irq]);
    else
        clear_bit(irq_src, &kvm->irq_state[irq]);
   kvm_pic_set_irq(kvm, irq, !!kvm->irq_state[irq]);
   kvm_ioapic_set_irq(kvm, irq, !!kvm->irq_state[irq]);
}

kvm_irq_allocate_source(kvm)
{
     irq_src = find_first_clear_bit(kvm->irq_sources);
     set_bit(irq_src, &kvm->irq_sources);
     return irq_src;
}

This also keeps the pic and ioapic out of the picture, which is good.  
It also allows us to implement negative polarity easily in the future.

-- 
error compiling committee.c: too many arguments to function

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

[Sheng Yang]

On Thu, Oct 02, 2008 at 04:27:18PM +0300, Avi Kivity wrote:
> Yang, Sheng wrote:
>> To deal with guest shared interrupt bug in in-kernel irqchip, we should:
>>
>> 1. Identify each level trig interrupt source.
>> 2. Implement logical OR on the same IRQ line for each interrupt source.
>>
>> Here I chose a simple method: the caller of kvm_set_irq() has responsiblity
>> to identify interrupt sources, and IOAPIC/PIC ensure logical OR on IRQ line.
>>   
>
> The downside is that every caller has to do this edge detection.
>
> How about allocating a vector of u32s (one per irq), and each source  
> will allocate a bit within this vector.  The 'or' operation becomes  
> (word != 0).

Oh, that's what I called the "alternative" one... I just think the
request/allocation/free make thing complicate and unnecessary for now, for
we are already ensure that kvm_set_irq() are called in pair.

I once think if we use resource allocating method, the irqs should bind with
the resources, and it's unnecessary. But I think your method is better,
reserve one bit for each source on every irq_state make things simpler.

OK, I will update the patch following this method.

--
regards
Yang, Sheng

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

[Sheng Yang]

How about this one? Still untested.

--
From: Sheng Yang <sheng@linux.intel.com>
Date: Fri, 3 Oct 2008 00:07:20 +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_request_irq_source_id(). 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_source_id().

Now we support 32 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         |   44 +++++++++++++++++++++++++++++++++++++++++---
 arch/x86/kvm/irq.h         |    5 ++++-
 arch/x86/kvm/x86.c         |   26 +++++++++++++++++++-------
 include/asm-x86/kvm_host.h |    3 +++
 include/linux/kvm_host.h   |    1 +
 7 files changed, 78 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 6144d3f..392089c 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, 0, 1, kvm->arch.vpit->irq_source_id);
+	kvm_set_irq(kvm, 0, 0, kvm->arch.vpit->irq_source_id);
 	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..6d8eaef 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, int level, int irq_source_id)
 {
+	unsigned long *irq_state = (unsigned long *)&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,33 @@ 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 = (unsigned long *)&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,
+			  (unsigned long *)&kvm->arch.irq_states[i]);
+	clear_bit(irq_source_id,
+		  (unsigned long *)&kvm->arch.irq_sources_bitmap);
+}
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 9f157c9..0cefa7d 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -85,7 +85,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, int level, int irq_source_id);
 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 +103,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 ad7a227..9142533 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->guest_irq, 1);
+		    assigned_dev->guest_irq, 1,
+		    assigned_dev->irq_source_id);
 	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->guest_irq, 0, dev->irq_source_id);
 	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);
+			/* irq_source_id 0 reserved for userspace */
+			kvm_set_irq(kvm, irq_event.irq, irq_event.level, 0);
 			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 635f50e..7114c56 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;
+
+	u32 irq_sources_bitmap;
+	u32 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.3

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

[Amit Shah]

* On Thursday 02 Oct 2008 22:15:59 Sheng Yang wrote:

>  /* 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, int level, int irq_source_id)

Just one thing: can you make the irq_source_id argument the 3rd one and 
the 'level' as the last one? It's clearer to think of it that way.

I'll test this next week.

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

[Sheng Yang]

On Fri, Oct 03, 2008 at 01:06:53PM +0530, Amit Shah wrote:
> * On Thursday 02 Oct 2008 22:15:59 Sheng Yang wrote:
> 
> >  /* 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, int level, int irq_source_id)
> 
> Just one thing: can you make the irq_source_id argument the 3rd one and 
> the 'level' as the last one? It's clearer to think of it that way.

Yeah, put the irq_source_id at the end of parameter list is a little ugly.
But I think it's better to keep "irq" and "level" together, so I move
irq_souce_id to the 2nd place. :)

> I'll test this next week.

Thanks!
--
regards
Yang, Sheng

----
From: Sheng Yang <sheng@linux.intel.com>
Date: Fri, 3 Oct 2008 18:20:43 +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 32 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         |   44 +++++++++++++++++++++++++++++++++++++++++---
 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, 79 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..653c59b 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 = (unsigned long *)&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,33 @@ 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 = (unsigned long *)&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,
+			  (unsigned long *)&kvm->arch.irq_states[i]);
+	clear_bit(irq_source_id,
+		  (unsigned long *)&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 ad7a227..3ace41f 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 635f50e..7114c56 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;
+
+	u32 irq_sources_bitmap;
+	u32 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.3

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

[Amit Shah]

* On Friday 03 Oct 2008 16:07:54 Sheng Yang wrote:
> > Just one thing: can you make the irq_source_id argument the 3rd one and
> > the 'level' as the last one? It's clearer to think of it that way.
>
> Yeah, put the irq_source_id at the end of parameter list is a little ugly.
> But I think it's better to keep "irq" and "level" together, so I move
> irq_souce_id to the 2nd place. :)

I think that's even better.

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

[Avi Kivity]

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.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Avi Kivity]

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.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Sheng Yang]

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.

--

From: Sheng Yang <sheng@linux.intel.com>
Date: Mon, 6 Oct 2008 13:57:14 +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..5762738 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 = (unsigned long *)&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

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

[Sheng Yang]

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

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

[Sheng Yang]

On Friday 03 October 2008 15:36:53 Amit Shah wrote:
> * On Thursday 02 Oct 2008 22:15:59 Sheng Yang wrote:
> >  /* 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, int level, int irq_source_id)
>
> Just one thing: can you make the irq_source_id argument the 3rd one and
> the 'level' as the last one? It's clearer to think of it that way.
>
> I'll test this next week.

Hi, Amit

Do you have any update on this patch?

Thanks!
--
regards
Yang, Sheng

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

[Amit Shah]

* On Friday 03 Oct 2008 16:07:54 Sheng Yang wrote:

> @@ -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);

This should be done in the kvm_vm_ioctl_assign_irq call, just after 
registering the ack notifier.

Amit.