KVM: Switch assigned device IRQ forwarding to threaded handler

This improves the IRQ forwarding for assigned devices: By using the kernel's threaded IRQ scheme, we can get rid of the latency-prone work queue and simplify the code in the same run. Moreover, we no longer have to hold assigned_dev_lock while raising the guest IRQ, which can be a lenghty operation as we may have to iterate over all VCPUs. The lock is now only used for synchronizing masking vs. unmasking of INTx-type IRQs, thus is renames to intx_lock. Acked-by: Alex Williamson <alex.williamson@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

KVM: Switch assigned device IRQ forwarding to threaded handler
This improves the IRQ forwarding for assigned devices: By using the kernel's threaded IRQ scheme, we can get rid of the latency-prone work queue and simplify the code in the same run. Moreover, we no longer have to hold assigned_dev_lock while raising the guest IRQ, which can be a lenghty operation as we may have to iterate over all VCPUs. The lock is now only used for synchronizing masking vs. unmasking of INTx-type IRQs, thus is renames to intx_lock. Acked-by: Alex Williamson <alex.williamson@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Jan Kiszka · Avi Kivity
1 parent 0c106b5aaa
Showing 2 changed files with 36 additions and 83 deletions Side-by-side Diff
include/linux/kvm_host.h
virt/kvm/assigned-dev.c
@@ -470,16 +470,8 @@
 	void (*irq_acked)(struct kvm_irq_ack_notifier *kian);
 };
  
-#define KVM_ASSIGNED_MSIX_PENDING		0x1
-struct kvm_guest_msix_entry {
-	u32 vector;
-	u16 entry;
-	u16 flags;
-};
-
 struct kvm_assigned_dev_kernel {
 	struct kvm_irq_ack_notifier ack_notifier;
-	struct work_struct interrupt_work;
 	struct list_head list;
 	int assigned_dev_id;
 	int host_segnr;
  
@@ -490,13 +482,13 @@
 	bool host_irq_disabled;
 	struct msix_entry *host_msix_entries;
 	int guest_irq;
-	struct kvm_guest_msix_entry *guest_msix_entries;
+	struct msix_entry *guest_msix_entries;
 	unsigned long irq_requested_type;
 	int irq_source_id;
 	int flags;
 	struct pci_dev *dev;
 	struct kvm *kvm;
-	spinlock_t assigned_dev_lock;
+	spinlock_t intx_lock;
 };
  
 struct kvm_irq_mask_notifier {
@@ -55,58 +55,31 @@
 	return index;
 }
  
-static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
+static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
 {
-	struct kvm_assigned_dev_kernel *assigned_dev;
-	int i;
+	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
+	u32 vector;
+	int index;
  
-	assigned_dev = container_of(work, struct kvm_assigned_dev_kernel,
-				    interrupt_work);
+	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX) {
+		spin_lock(&assigned_dev->intx_lock);
+		disable_irq_nosync(irq);
+		assigned_dev->host_irq_disabled = true;
+		spin_unlock(&assigned_dev->intx_lock);
+	}
  
-	spin_lock_irq(&assigned_dev->assigned_dev_lock);
 	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
-		struct kvm_guest_msix_entry *guest_entries =
-			assigned_dev->guest_msix_entries;
-		for (i = 0; i < assigned_dev->entries_nr; i++) {
-			if (!(guest_entries[i].flags &
-					KVM_ASSIGNED_MSIX_PENDING))
-				continue;
-			guest_entries[i].flags &= ~KVM_ASSIGNED_MSIX_PENDING;
+		index = find_index_from_host_irq(assigned_dev, irq);
+		if (index >= 0) {
+			vector = assigned_dev->
+					guest_msix_entries[index].vector;
 			kvm_set_irq(assigned_dev->kvm,
-				    assigned_dev->irq_source_id,
-				    guest_entries[i].vector, 1);
+				    assigned_dev->irq_source_id, vector, 1);
 		}
 	} else
 		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
 			    assigned_dev->guest_irq, 1);
  
-	spin_unlock_irq(&assigned_dev->assigned_dev_lock);
-}
-
-static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
-{
-	unsigned long flags;
-	struct kvm_assigned_dev_kernel *assigned_dev =
-		(struct kvm_assigned_dev_kernel *) dev_id;
-
-	spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags);
-	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
-		int index = find_index_from_host_irq(assigned_dev, irq);
-		if (index < 0)
-			goto out;
-		assigned_dev->guest_msix_entries[index].flags |=
-			KVM_ASSIGNED_MSIX_PENDING;
-	}
-
-	schedule_work(&assigned_dev->interrupt_work);
-
-	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) {
-		disable_irq_nosync(irq);
-		assigned_dev->host_irq_disabled = true;
-	}
-
-out:
-	spin_unlock_irqrestore(&assigned_dev->assigned_dev_lock, flags);
 	return IRQ_HANDLED;
 }
  
@@ -114,7 +87,6 @@
 static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 {
 	struct kvm_assigned_dev_kernel *dev;
-	unsigned long flags;
  
 	if (kian->gsi == -1)
 		return;
  
@@ -127,12 +99,12 @@
 	/* The guest irq may be shared so this ack may be
 	 * from another device.
 	 */
-	spin_lock_irqsave(&dev->assigned_dev_lock, flags);
+	spin_lock(&dev->intx_lock);
 	if (dev->host_irq_disabled) {
 		enable_irq(dev->host_irq);
 		dev->host_irq_disabled = false;
 	}
-	spin_unlock_irqrestore(&dev->assigned_dev_lock, flags);
+	spin_unlock(&dev->intx_lock);
 }
  
 static void deassign_guest_irq(struct kvm *kvm,
  
  
  
  
@@ -155,29 +127,20 @@
 			      struct kvm_assigned_dev_kernel *assigned_dev)
 {
 	/*
-	 * In kvm_free_device_irq, cancel_work_sync return true if:
-	 * 1. work is scheduled, and then cancelled.
-	 * 2. work callback is executed.
+	 * We disable irq here to prevent further events.
 	 *
-	 * The first one ensured that the irq is disabled and no more events
-	 * would happen. But for the second one, the irq may be enabled (e.g.
-	 * for MSI). So we disable irq here to prevent further events.
-	 *
 	 * Notice this maybe result in nested disable if the interrupt type is
 	 * INTx, but it's OK for we are going to free it.
 	 *
 	 * If this function is a part of VM destroy, please ensure that till
 	 * now, the kvm state is still legal for probably we also have to wait
-	 * interrupt_work done.
+	 * on a currently running IRQ handler.
 	 */
 	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
 		int i;
 		for (i = 0; i < assigned_dev->entries_nr; i++)
-			disable_irq_nosync(assigned_dev->
-					   host_msix_entries[i].vector);
+			disable_irq(assigned_dev->host_msix_entries[i].vector);
  
-		cancel_work_sync(&assigned_dev->interrupt_work);
-
 		for (i = 0; i < assigned_dev->entries_nr; i++)
 			free_irq(assigned_dev->host_msix_entries[i].vector,
 				 (void *)assigned_dev);
@@ -188,8 +151,7 @@
 		pci_disable_msix(assigned_dev->dev);
 	} else {
 		/* Deal with MSI and INTx */
-		disable_irq_nosync(assigned_dev->host_irq);
-		cancel_work_sync(&assigned_dev->interrupt_work);
+		disable_irq(assigned_dev->host_irq);
  
 		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
  
@@ -268,8 +230,9 @@
 	 * on the same interrupt line is not a happy situation: there
 	 * are going to be long delays in accepting, acking, etc.
 	 */
-	if (request_irq(dev->host_irq, kvm_assigned_dev_intr,
-			0, "kvm_assigned_intx_device", (void *)dev))
+	if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
+				 IRQF_ONESHOT, "kvm_assigned_intx_device",
+				 (void *)dev))
 		return -EIO;
 	return 0;
 }
@@ -287,8 +250,8 @@
 	}
  
 	dev->host_irq = dev->dev->irq;
-	if (request_irq(dev->host_irq, kvm_assigned_dev_intr, 0,
-			"kvm_assigned_msi_device", (void *)dev)) {
+	if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
+				 0, "kvm_assigned_msi_device", (void *)dev)) {
 		pci_disable_msi(dev->dev);
 		return -EIO;
 	}
@@ -313,10 +276,10 @@
 		return r;
  
 	for (i = 0; i < dev->entries_nr; i++) {
-		r = request_irq(dev->host_msix_entries[i].vector,
-				kvm_assigned_dev_intr, 0,
-				"kvm_assigned_msix_device",
-				(void *)dev);
+		r = request_threaded_irq(dev->host_msix_entries[i].vector,
+					 NULL, kvm_assigned_dev_thread,
+					 0, "kvm_assigned_msix_device",
+					 (void *)dev);
 		if (r)
 			goto err;
 	}
  
@@ -557,12 +520,10 @@
 	match->host_devfn = assigned_dev->devfn;
 	match->flags = assigned_dev->flags;
 	match->dev = dev;
-	spin_lock_init(&match->assigned_dev_lock);
+	spin_lock_init(&match->intx_lock);
 	match->irq_source_id = -1;
 	match->kvm = kvm;
 	match->ack_notifier.irq_acked = kvm_assigned_dev_ack_irq;
-	INIT_WORK(&match->interrupt_work,
-		  kvm_assigned_dev_interrupt_work_handler);
  
 	list_add(&match->list, &kvm->arch.assigned_dev_head);
  
@@ -654,9 +615,9 @@
 			r = -ENOMEM;
 			goto msix_nr_out;
 		}
-		adev->guest_msix_entries = kzalloc(
-				sizeof(struct kvm_guest_msix_entry) *
-				entry_nr->entry_nr, GFP_KERNEL);
+		adev->guest_msix_entries =
+			kzalloc(sizeof(struct msix_entry) * entry_nr->entry_nr,
+				GFP_KERNEL);
 		if (!adev->guest_msix_entries) {
 			kfree(adev->host_msix_entries);
 			r = -ENOMEM;
...	...	@@ -470,16 +470,8 @@
470	470	void (irq_acked)(struct kvm_irq_ack_notifier kian);
471	471	};
472	472
473		-#define KVM_ASSIGNED_MSIX_PENDING 0x1
474		-struct kvm_guest_msix_entry {
475		- u32 vector;
476		- u16 entry;
477		- u16 flags;
478		-};
479		-
480	473	struct kvm_assigned_dev_kernel {
481	474	struct kvm_irq_ack_notifier ack_notifier;
482		- struct work_struct interrupt_work;
483	475	struct list_head list;
484	476	int assigned_dev_id;
485	477	int host_segnr;
486	478
...	...	@@ -490,13 +482,13 @@
490	482	bool host_irq_disabled;
491	483	struct msix_entry *host_msix_entries;
492	484	int guest_irq;
493		- struct kvm_guest_msix_entry *guest_msix_entries;
	485	+ struct msix_entry *guest_msix_entries;
494	486	unsigned long irq_requested_type;
495	487	int irq_source_id;
496	488	int flags;
497	489	struct pci_dev *dev;
498	490	struct kvm *kvm;
499		- spinlock_t assigned_dev_lock;
	491	+ spinlock_t intx_lock;
500	492	};
501	493
502	494	struct kvm_irq_mask_notifier {
...	...	@@ -55,58 +55,31 @@
55	55	return index;
56	56	}
57	57
58		-static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
	58	+static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
59	59	{
60		- struct kvm_assigned_dev_kernel *assigned_dev;
61		- int i;
	60	+ struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
	61	+ u32 vector;
	62	+ int index;
62	63
63		- assigned_dev = container_of(work, struct kvm_assigned_dev_kernel,
64		- interrupt_work);
	64	+ if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX) {
	65	+ spin_lock(&assigned_dev->intx_lock);
	66	+ disable_irq_nosync(irq);
	67	+ assigned_dev->host_irq_disabled = true;
	68	+ spin_unlock(&assigned_dev->intx_lock);
	69	+ }
65	70
66		- spin_lock_irq(&assigned_dev->assigned_dev_lock);
67	71	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
68		- struct kvm_guest_msix_entry *guest_entries =
69		- assigned_dev->guest_msix_entries;
70		- for (i = 0; i < assigned_dev->entries_nr; i++) {
71		- if (!(guest_entries[i].flags &
72		- KVM_ASSIGNED_MSIX_PENDING))
73		- continue;
74		- guest_entries[i].flags &= ~KVM_ASSIGNED_MSIX_PENDING;
	72	+ index = find_index_from_host_irq(assigned_dev, irq);
	73	+ if (index >= 0) {
	74	+ vector = assigned_dev->
	75	+ guest_msix_entries[index].vector;
75	76	kvm_set_irq(assigned_dev->kvm,
76		- assigned_dev->irq_source_id,
77		- guest_entries[i].vector, 1);
	77	+ assigned_dev->irq_source_id, vector, 1);
78	78	}
79	79	} else
80	80	kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
81	81	assigned_dev->guest_irq, 1);
82	82
83		- spin_unlock_irq(&assigned_dev->assigned_dev_lock);
84		-}
85		-
86		-static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
87		-{
88		- unsigned long flags;
89		- struct kvm_assigned_dev_kernel *assigned_dev =
90		- (struct kvm_assigned_dev_kernel *) dev_id;
91		-
92		- spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags);
93		- if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
94		- int index = find_index_from_host_irq(assigned_dev, irq);
95		- if (index < 0)
96		- goto out;
97		- assigned_dev->guest_msix_entries[index].flags \|=
98		- KVM_ASSIGNED_MSIX_PENDING;
99		- }
100		-
101		- schedule_work(&assigned_dev->interrupt_work);
102		-
103		- if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) {
104		- disable_irq_nosync(irq);
105		- assigned_dev->host_irq_disabled = true;
106		- }
107		-
108		-out:
109		- spin_unlock_irqrestore(&assigned_dev->assigned_dev_lock, flags);
110	83	return IRQ_HANDLED;
111	84	}
112	85
...	...	@@ -114,7 +87,6 @@
114	87	static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
115	88	{
116	89	struct kvm_assigned_dev_kernel *dev;
117		- unsigned long flags;
118	90
119	91	if (kian->gsi == -1)
120	92	return;
121	93
...	...	@@ -127,12 +99,12 @@
127	99	/* The guest irq may be shared so this ack may be
128	100	* from another device.
129	101	*/
130		- spin_lock_irqsave(&dev->assigned_dev_lock, flags);
	102	+ spin_lock(&dev->intx_lock);
131	103	if (dev->host_irq_disabled) {
132	104	enable_irq(dev->host_irq);
133	105	dev->host_irq_disabled = false;
134	106	}
135		- spin_unlock_irqrestore(&dev->assigned_dev_lock, flags);
	107	+ spin_unlock(&dev->intx_lock);
136	108	}
137	109
138	110	static void deassign_guest_irq(struct kvm *kvm,
139	111
140	112
141	113
142	114
...	...	@@ -155,29 +127,20 @@
155	127	struct kvm_assigned_dev_kernel *assigned_dev)
156	128	{
157	129	/*
158		- * In kvm_free_device_irq, cancel_work_sync return true if:
159		- * 1. work is scheduled, and then cancelled.
160		- * 2. work callback is executed.
	130	+ * We disable irq here to prevent further events.
161	131	*
162		- * The first one ensured that the irq is disabled and no more events
163		- * would happen. But for the second one, the irq may be enabled (e.g.
164		- * for MSI). So we disable irq here to prevent further events.
165		- *
166	132	* Notice this maybe result in nested disable if the interrupt type is
167	133	* INTx, but it's OK for we are going to free it.
168	134	*
169	135	* If this function is a part of VM destroy, please ensure that till
170	136	* now, the kvm state is still legal for probably we also have to wait
171		- * interrupt_work done.
	137	+ * on a currently running IRQ handler.
172	138	*/
173	139	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
174	140	int i;
175	141	for (i = 0; i < assigned_dev->entries_nr; i++)
176		- disable_irq_nosync(assigned_dev->
177		- host_msix_entries[i].vector);
	142	+ disable_irq(assigned_dev->host_msix_entries[i].vector);
178	143
179		- cancel_work_sync(&assigned_dev->interrupt_work);
180		-
181	144	for (i = 0; i < assigned_dev->entries_nr; i++)
182	145	free_irq(assigned_dev->host_msix_entries[i].vector,
183	146	(void *)assigned_dev);
...	...	@@ -188,8 +151,7 @@
188	151	pci_disable_msix(assigned_dev->dev);
189	152	} else {
190	153	/* Deal with MSI and INTx */
191		- disable_irq_nosync(assigned_dev->host_irq);
192		- cancel_work_sync(&assigned_dev->interrupt_work);
	154	+ disable_irq(assigned_dev->host_irq);
193	155
194	156	free_irq(assigned_dev->host_irq, (void *)assigned_dev);
195	157
...	...	@@ -268,8 +230,9 @@
268	230	* on the same interrupt line is not a happy situation: there
269	231	* are going to be long delays in accepting, acking, etc.
270	232	*/
271		- if (request_irq(dev->host_irq, kvm_assigned_dev_intr,
272		- 0, "kvm_assigned_intx_device", (void *)dev))
	233	+ if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
	234	+ IRQF_ONESHOT, "kvm_assigned_intx_device",
	235	+ (void *)dev))
273	236	return -EIO;
274	237	return 0;
275	238	}
...	...	@@ -287,8 +250,8 @@
287	250	}
288	251
289	252	dev->host_irq = dev->dev->irq;
290		- if (request_irq(dev->host_irq, kvm_assigned_dev_intr, 0,
291		- "kvm_assigned_msi_device", (void *)dev)) {
	253	+ if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
	254	+ 0, "kvm_assigned_msi_device", (void *)dev)) {
292	255	pci_disable_msi(dev->dev);
293	256	return -EIO;
294	257	}
...	...	@@ -313,10 +276,10 @@
313	276	return r;
314	277
315	278	for (i = 0; i < dev->entries_nr; i++) {
316		- r = request_irq(dev->host_msix_entries[i].vector,
317		- kvm_assigned_dev_intr, 0,
318		- "kvm_assigned_msix_device",
319		- (void *)dev);
	279	+ r = request_threaded_irq(dev->host_msix_entries[i].vector,
	280	+ NULL, kvm_assigned_dev_thread,
	281	+ 0, "kvm_assigned_msix_device",
	282	+ (void *)dev);
320	283	if (r)
321	284	goto err;
322	285	}
323	286
...	...	@@ -557,12 +520,10 @@
557	520	match->host_devfn = assigned_dev->devfn;
558	521	match->flags = assigned_dev->flags;
559	522	match->dev = dev;
560		- spin_lock_init(&match->assigned_dev_lock);
	523	+ spin_lock_init(&match->intx_lock);
561	524	match->irq_source_id = -1;
562	525	match->kvm = kvm;
563	526	match->ack_notifier.irq_acked = kvm_assigned_dev_ack_irq;
564		- INIT_WORK(&match->interrupt_work,
565		- kvm_assigned_dev_interrupt_work_handler);
566	527
567	528	list_add(&match->list, &kvm->arch.assigned_dev_head);
568	529
...	...	@@ -654,9 +615,9 @@
654	615	r = -ENOMEM;
655	616	goto msix_nr_out;
656	617	}
657		- adev->guest_msix_entries = kzalloc(
658		- sizeof(struct kvm_guest_msix_entry) *
659		- entry_nr->entry_nr, GFP_KERNEL);
	618	+ adev->guest_msix_entries =
	619	+ kzalloc(sizeof(struct msix_entry) * entry_nr->entry_nr,
	620	+ GFP_KERNEL);
660	621	if (!adev->guest_msix_entries) {
661	622	kfree(adev->host_msix_entries);
662	623	r = -ENOMEM;