* [PATCH 2/2] KVM: Device Assignment: Fix guest shared interrupts with in-kernel irqchip
2008-10-11 8:10 ` [PATCH 1/2] KVM: Handle multiple interrupt sources Amit Shah
@ 2008-10-11 8:10 ` Amit Shah
2008-10-13 2:32 ` [PATCH 1/2] KVM: Handle multiple interrupt sources Sheng Yang
1 sibling, 0 replies; 8+ messages in thread
From: Amit Shah @ 2008-10-11 8:10 UTC (permalink / raw)
To: avi; +Cc: kvm, xiantao.zhang, Sheng Yang, Amit Shah
From: Sheng Yang <sheng@linux.intel.com>
Derived from Avi's suggestion, now every call of kvm_set_irq() should offer
an 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_sources().
As of now we support sizeof(unsigned long) different irq sources at most.
[Amit: - rebase to kvm.git HEAD
- move kvm_request_irq_source_id to the update_irq ioctl]
Signed-off-by: Sheng Yang <sheng@linux.intel.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
arch/x86/kvm/i8254.c | 11 +++++++++--
arch/x86/kvm/i8254.h | 1 +
arch/x86/kvm/irq.h | 5 +++++
arch/x86/kvm/x86.c | 7 +++++--
include/asm-x86/kvm_host.h | 3 +++
include/linux/kvm_host.h | 3 ++-
virt/kvm/irq_comm.c | 42 +++++++++++++++++++++++++++++++++++++++---
virt/kvm/kvm_main.c | 13 ++++++++-----
8 files changed, 72 insertions(+), 13 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.h b/arch/x86/kvm/irq.h
index 71e37a5..9c6d9a5 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;
@@ -95,4 +97,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 6f45428..4388c44 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1823,8 +1823,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
*/
if (kvm->userspace_intrsource_states[irq_event.irq]
!= irq_event.level) {
- 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);
kvm->userspace_intrsource_states[irq_event.irq]
= irq_event.level;
}
@@ -4126,6 +4126,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 4b06ca8..f01e92e 100644
--- a/include/asm-x86/kvm_host.h
+++ b/include/asm-x86/kvm_host.h
@@ -368,6 +368,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 d392e31..c6e2cc3 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -308,11 +308,12 @@ struct kvm_assigned_dev_kernel {
int host_irq;
int guest_irq;
int irq_requested;
+ int irq_source_id;
int irq_state;
struct pci_dev *dev;
struct kvm *kvm;
};
-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);
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index d0169f5..a014831 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -25,15 +25,23 @@
#include "ioapic.h"
/* 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_ioapic_set_irq(kvm->arch.vioapic, irq, !!(*irq_state));
#ifdef CONFIG_X86
- kvm_pic_set_irq(pic_irqchip(kvm), irq, level);
+ kvm_pic_set_irq(pic_irqchip(kvm), irq, !!(*irq_state));
#endif
}
@@ -58,3 +66,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/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index faa56fb..0479898 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -106,6 +106,7 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
mutex_lock(&assigned_dev->kvm->lock);
if (assigned_dev->irq_state == 0) {
kvm_set_irq(assigned_dev->kvm,
+ assigned_dev->irq_source_id,
assigned_dev->guest_irq, 1);
assigned_dev->irq_state = 1;
}
@@ -113,9 +114,6 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
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 =
@@ -138,7 +136,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);
if (dev->irq_state == 1) {
- kvm_set_irq(dev->kvm, dev->guest_irq, 0);
+ kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 0);
dev->irq_state = 0;
}
enable_irq(dev->host_irq);
@@ -152,6 +150,7 @@ static void kvm_free_assigned_device(struct kvm *kvm,
free_irq(assigned_dev->host_irq, (void *)assigned_dev);
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
@@ -221,6 +220,11 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
match->ack_notifier.gsi = assigned_irq->guest_irq;
match->ack_notifier.irq_acked = kvm_assigned_dev_ack_irq;
kvm_register_irq_ack_notifier(kvm, &match->ack_notifier);
+ r = kvm_request_irq_source_id(kvm);
+ if (r < 0)
+ goto out_release;
+ else
+ match->irq_source_id = r;
/* Even though this is PCI, we don't want to use shared
* interrupts. Sharing host devices with guest-assigned devices
@@ -233,7 +237,6 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
goto out_release;
}
}
-
match->irq_requested = true;
mutex_unlock(&kvm->lock);
return r;
--
1.5.4.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] KVM: Handle multiple interrupt sources
2008-10-11 8:10 ` [PATCH 1/2] KVM: Handle multiple interrupt sources Amit Shah
2008-10-11 8:10 ` [PATCH 2/2] KVM: Device Assignment: Fix guest shared interrupts with in-kernel irqchip Amit Shah
@ 2008-10-13 2:32 ` Sheng Yang
1 sibling, 0 replies; 8+ messages in thread
From: Sheng Yang @ 2008-10-13 2:32 UTC (permalink / raw)
To: Amit Shah; +Cc: avi, kvm, xiantao.zhang
On Saturday 11 October 2008 16:10:51 Amit Shah wrote:
> From: Sheng Yang <sheng@linux.intel.com>
>
> Keep a record of current interrupt state before injecting. Don't
> assert/deassert repeatedly, so that every caller of kvm_set_irq()
> can be identified as a separate interrupt source for the IOAPIC/PIC
> to implement logical OR of level triggered interrupts on one IRQ line.
>
> Notice that userspace devices are treated as one device for each IRQ
> line. The correctness of sharing interrupt for each IRQ line should be
> ensured by the userspace program (QEmu).
>
> [Amit: rebase to kvm.git HEAD]
Hi, Amit
Thanks for your work!
But maybe I miss something. I suppose my later patch can work indepently? I
think the second patch should solve the whole problem (sorry to reply it to
the second rather than [0/2] which made confusion...). Can you have a check?
Thanks!
--
regards
Yang, Sheng
>
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
> arch/x86/kvm/x86.c | 13 ++++++++++++-
> include/linux/kvm_host.h | 3 +++
> virt/kvm/kvm_main.c | 12 +++++++++---
> 3 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index dda478e..6f45428 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1816,7 +1816,18 @@ 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);
> + /*
> + * Take one IRQ line as from one device, shared IRQ
> + * line should also be handled in the userspace before
> + * use KVM_IRQ_LINE ioctl to change IRQ line state.
> + */
> + if (kvm->userspace_intrsource_states[irq_event.irq]
> + != irq_event.level) {
> + kvm_set_irq(kvm, irq_event.irq,
> + irq_event.level);
> + kvm->userspace_intrsource_states[irq_event.irq]
> + = irq_event.level;
> + }
> mutex_unlock(&kvm->lock);
> r = 0;
> }
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 3833c48..d392e31 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -129,6 +129,8 @@ struct kvm {
> unsigned long mmu_notifier_seq;
> long mmu_notifier_count;
> #endif
> +
> + int userspace_intrsource_states[KVM_IOAPIC_NUM_PINS];
> };
>
> /* The guest did something we don't support. */
> @@ -306,6 +308,7 @@ struct kvm_assigned_dev_kernel {
> int host_irq;
> int guest_irq;
> int irq_requested;
> + int irq_state;
> struct pci_dev *dev;
> struct kvm *kvm;
> };
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index cf0ab8e..faa56fb 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -104,8 +104,11 @@ static void
> kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) *
> finer-grained lock, update this
> */
> mutex_lock(&assigned_dev->kvm->lock);
> - kvm_set_irq(assigned_dev->kvm,
> - assigned_dev->guest_irq, 1);
> + if (assigned_dev->irq_state == 0) {
> + kvm_set_irq(assigned_dev->kvm,
> + assigned_dev->guest_irq, 1);
> + assigned_dev->irq_state = 1;
> + }
> mutex_unlock(&assigned_dev->kvm->lock);
> kvm_put_kvm(assigned_dev->kvm);
> }
> @@ -134,7 +137,10 @@ 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);
> + if (dev->irq_state == 1) {
> + kvm_set_irq(dev->kvm, dev->guest_irq, 0);
> + dev->irq_state = 0;
> + }
> enable_irq(dev->host_irq);
> }
^ permalink raw reply [flat|nested] 8+ messages in thread