KVM: Improve IRQ assignment for device passthrough

From: Jan Kiszka <[email protected]>

The complete work handler runs with assigned_dev_lock acquired and interrupts disabled, so there is nothing to gain pushing this work out of the actually IRQ handler. Fold them together.

Signed-off-by Jan Kiszka <[email protected]>

---
 include/linux/kvm_host.h |    1 -
 virt/kvm/assigned-dev.c  |   71 +++++++++++++++-------------------------------
 2 files changed, 23 insertions(+), 49 deletions(-)

include/linux/kvm_host.h +0 -1

index 462b982..df5497f 100644
@@ -465,7 +465,6 @@ struct kvm_guest_msix_entry {
 
 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;

virt/kvm/assigned-dev.c +23 -48

index 7c98928..5c1b56a 100644
@@ -55,18 +55,24 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
 	return index;
 }
 
-static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
+static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 {
-	struct kvm_assigned_dev_kernel *assigned_dev;
-	int i;
-
-	assigned_dev = container_of(work, struct kvm_assigned_dev_kernel,
-				    interrupt_work);
+	struct kvm_assigned_dev_kernel *assigned_dev =
+		(struct kvm_assigned_dev_kernel *) dev_id;
+	unsigned long flags;
 
-	spin_lock_irq(&assigned_dev->assigned_dev_lock);
+	spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags);
 	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
 		struct kvm_guest_msix_entry *guest_entries =
 			assigned_dev->guest_msix_entries;
+		int index = find_index_from_host_irq(assigned_dev, irq);
+		int i;
+
+		if (index < 0)
+			goto out;
+
+		guest_entries[index].flags |= KVM_ASSIGNED_MSIX_PENDING;
+
 		for (i = 0; i < assigned_dev->entries_nr; i++) {
 			if (!(guest_entries[i].flags &
 					KVM_ASSIGNED_MSIX_PENDING))

@@ -76,33 +82,15 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 				    assigned_dev->irq_source_id,
 				    guest_entries[i].vector, 1);
 		}
-	} else
+	} 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;
+		if (assigned_dev->irq_requested_type &
+				KVM_DEV_IRQ_GUEST_INTX) {
+			disable_irq_nosync(irq);
+			assigned_dev->host_irq_disabled = true;
+		}
 	}
 
 out:

@@ -147,33 +135,23 @@ static void deassign_guest_irq(struct kvm *kvm,
 	assigned_dev->irq_requested_type &= ~(KVM_DEV_IRQ_GUEST_MASK);
 }
 
-/* The function implicit hold kvm->lock mutex due to cancel_work_sync() */
 static void deassign_host_irq(struct kvm *kvm,
 			      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.
-	 *
-	 * 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.
+	 * 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);
-
-		cancel_work_sync(&assigned_dev->interrupt_work);
+			disable_irq(assigned_dev->host_msix_entries[i].vector);
 
 		for (i = 0; i < assigned_dev->entries_nr; i++)
 			free_irq(assigned_dev->host_msix_entries[i].vector,

@@ -185,8 +163,7 @@ static void deassign_host_irq(struct kvm *kvm,
 		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);
 

@@ -558,8 +535,6 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	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);
 

From: Jan Kiszka <[email protected]>

PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest.

Signed-off-by Jan Kiszka <[email protected]>

---
 include/linux/kvm_host.h |    1 +
 virt/kvm/assigned-dev.c  |  153 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 140 insertions(+), 14 deletions(-)

include/linux/kvm_host.h +1 -0

index df5497f..fcdc849 100644
@@ -473,6 +473,7 @@ struct kvm_assigned_dev_kernel {
 	unsigned int entries_nr;
 	int host_irq;
 	bool host_irq_disabled;
+	bool pci_2_3;
 	struct msix_entry *host_msix_entries;
 	int guest_irq;
 	struct kvm_guest_msix_entry *guest_msix_entries;

virt/kvm/assigned-dev.c +75 -0

index d3ddfea..411643c 100644
@@ -55,10 +55,96 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
 	return index;
 }
 
+/*
+ * Verify that the device supports Interrupt Disable bit in command register,
+ * per PCI 2.3, by flipping this bit and reading it back: this bit was readonly
+ * in PCI 2.2.
+ */
+static bool pci_2_3_supported(struct pci_dev *pdev)
+{
+	u16 orig, new;
+	bool supported = false;
+
+	pci_block_user_cfg_access(pdev);
+	pci_read_config_word(pdev, PCI_COMMAND, &orig);
+	pci_write_config_word(pdev, PCI_COMMAND,
+			      orig ^ PCI_COMMAND_INTX_DISABLE);
+	pci_read_config_word(pdev, PCI_COMMAND, &new);
+	
+	/*
+	 * There's no way to protect against
+	 * hardware bugs or detect them reliably, but as long as we know
+	 * what the value should be, let's go ahead and check it.
+	 */
+	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
+		dev_err(&pdev->dev, "Command changed from 0x%x to 0x%x: "
+			"driver or HW bug?\n", orig, new);
+		goto out;
+	}
+	if (!((new ^ orig) & PCI_COMMAND_INTX_DISABLE)) {
+		dev_warn(&pdev->dev, "Device does not support "
+			 "disabling interrupts: unable to bind.\n");
+		goto out;
+	}
+	supported = true;
+
+	/* Now restore the original value. */
+	pci_write_config_word(pdev, PCI_COMMAND, orig);
+
+out:
+	pci_unblock_user_cfg_access(pdev);
+	return supported;
+}
+
+static void
+pci_2_3_mask_irq(struct pci_dev *dev, int mask, unsigned int *irq_status)
+{
+	u32 cmd_status_dword;
+	u16 origcmd, newcmd;
+
+	/*
+	 * We do a single dword read to retrieve both command and status.
+	 * Document assumptions that make this possible.
+	 */
+	BUILD_BUG_ON(PCI_COMMAND % 4);
+	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
+
+	pci_block_user_cfg_access(dev);
+
+	/*
+	 * Read both command and status registers in a single 32-bit operation.
+	 * Note: we could cache the value for command and move the status read
+	 * out of the lock if there was a way to get notified of user changes
+	 * to command register through sysfs. Should be good for shared irqs.
+	 */
+	pci_read_config_dword(dev, PCI_COMMAND, &cmd_status_dword);
+	origcmd = cmd_status_dword;
+
+	if (irq_status) {
+		/*
+		* Check interrupt status register to see whether our device triggered
+		* the interrupt.
+		*/
+		*irq_status = (cmd_status_dword >> 16) & PCI_STATUS_INTERRUPT;
+		if (*irq_status == 0)
+			goto done;
+	}
+

virt/kvm/assigned-dev.c +5 -4

 static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev =
 		(struct kvm_assigned_dev_kernel *) dev_id;
+	int ret = IRQ_HANDLED;
 	unsigned long flags;
 
 	spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags);

@@ -83,19 +169,34 @@ static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 				    guest_entries[i].vector, 1);
 		}
 	} else {
-		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
-			    assigned_dev->guest_irq, 1);
-
 		if (assigned_dev->irq_requested_type &
 				KVM_DEV_IRQ_GUEST_INTX) {
-			disable_irq_nosync(irq);
+			if (assigned_dev->pci_2_3) {
+				unsigned int irq_status;
+
+				if (assigned_dev->host_irq_disabled) {

virt/kvm/assigned-dev.c +6 -1

+			} else
+				disable_irq_nosync(irq);
 			assigned_dev->host_irq_disabled = true;
 		}
+
+		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
+			    assigned_dev->guest_irq, 1);
 	}
 
 out:
 	spin_unlock_irqrestore(&assigned_dev->assigned_dev_lock, flags);
-	return IRQ_HANDLED;
+	return ret;
 }
 
 /* Ack the irq line for an assigned device */

virt/kvm/assigned-dev.c +24 -5

 	}
 	spin_unlock_irqrestore(&dev->assigned_dev_lock, flags);

@@ -166,7 +270,11 @@ static void deassign_host_irq(struct kvm *kvm,
 		pci_disable_msix(assigned_dev->dev);
 	} else {
 		/* Deal with MSI and INTx */
-		disable_irq(assigned_dev->host_irq);
+		if (assigned_dev->pci_2_3) {
+			pci_2_3_mask_irq(assigned_dev->dev, 1, NULL);
+			synchronize_irq(assigned_dev->host_irq);
+		} else
+			disable_irq(assigned_dev->host_irq);
 
 		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
 

@@ -214,6 +322,13 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 
 	pci_reset_function(assigned_dev->dev);
 
+	/*
+	 * Unmask the IRQ at PCI level once the reset is done - the next user
+	 * may not expect the IRQ being masked.
+	 */
+	if (assigned_dev->pci_2_3)
+		pci_2_3_mask_irq(assigned_dev->dev, 0, NULL);
+
 	pci_release_regions(assigned_dev->dev);
 	pci_disable_device(assigned_dev->dev);
 	pci_dev_put(assigned_dev->dev);

@@ -239,15 +354,26 @@ void kvm_free_all_assigned_devices(struct kvm *kvm)
 static int assigned_device_enable_host_intx(struct kvm *kvm,
 					    struct kvm_assigned_dev_kernel *dev)
 {
+	unsigned long flags = 0;
+
 	dev->host_irq = dev->dev->irq;
-	/* Even though this is PCI, we don't want to use shared
-	 * interrupts. Sharing host devices with guest-assigned devices
-	 * on the same interrupt line is not a happy situation: there
-	 * are going to be long delays in accepting, acking, etc.
+
+	/*
+	 * We can only share the IRQ line with other host devices if we are
+	 * able to disable the IRQ source at device-level - independently of
+	 * the guest driver. Otherwise host devices may suffer from unbounded
+	 * IRQ latencies when the guest keeps the line asserted.
 	 */
+	dev->pci_2_3 = pci_2_3_supported(dev->dev);
+	if (dev->pci_2_3)
+		flags = IRQF_SHARED;
+

virt/kvm/assigned-dev.c +1 -2

@@ -324,7 +450,6 @@ static int assigned_device_enable_guest_msi(struct kvm *kvm,
 {
 	dev->guest_irq = irq->guest_irq;
 	dev->ack_notifier.gsi = -1;
-	dev->host_irq_disabled = false;
 	return 0;
 }
 #endif

@@ -336,7 +461,6 @@ static int assigned_device_enable_guest_msix(struct kvm *kvm,
 {
 	dev->guest_irq = irq->guest_irq;
 	dev->ack_notifier.gsi = -1;
-	dev->host_irq_disabled = false;
 	return 0;
 }
 #endif

@@ -367,6 +491,7 @@ static int assign_host_irq(struct kvm *kvm,
 	default:
 		r = -EINVAL;
 	}
+	dev->host_irq_disabled = false;
 
 	if (!r)
 		dev->irq_requested_type |= host_irq_type;

Signature · 1 line

1.7.1

Quoted 112 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest.

Signed-off-by Jan Kiszka <[email protected]>

---
 include/linux/kvm_host.h |    1 +
 virt/kvm/assigned-dev.c  |  153 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 140 insertions(+), 14 deletions(-)

include/linux/kvm_host.h +1 -0

index df5497f..fcdc849 100644
@@ -473,6 +473,7 @@ struct kvm_assigned_dev_kernel {
 	unsigned int entries_nr;
 	int host_irq;
 	bool host_irq_disabled;
+	bool pci_2_3;
 	struct msix_entry *host_msix_entries;
 	int guest_irq;
 	struct kvm_guest_msix_entry *guest_msix_entries;

virt/kvm/assigned-dev.c +80 -0

index d3ddfea..411643c 100644
@@ -55,10 +55,96 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
 	return index;
 }
 
+/*
+ * Verify that the device supports Interrupt Disable bit in command register,
+ * per PCI 2.3, by flipping this bit and reading it back: this bit was readonly
+ * in PCI 2.2.
+ */
+static bool pci_2_3_supported(struct pci_dev *pdev)
+{
+	u16 orig, new;
+	bool supported = false;
+
+	pci_block_user_cfg_access(pdev);
+	pci_read_config_word(pdev, PCI_COMMAND, &orig);
+	pci_write_config_word(pdev, PCI_COMMAND,
+			      orig ^ PCI_COMMAND_INTX_DISABLE);
+	pci_read_config_word(pdev, PCI_COMMAND, &new);
+	
+	/*
+	 * There's no way to protect against
+	 * hardware bugs or detect them reliably, but as long as we know
+	 * what the value should be, let's go ahead and check it.
+	 */
+	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
+		dev_err(&pdev->dev, "Command changed from 0x%x to 0x%x: "
+			"driver or HW bug?\n", orig, new);
+		goto out;
+	}
+	if (!((new ^ orig) & PCI_COMMAND_INTX_DISABLE)) {
+		dev_warn(&pdev->dev, "Device does not support "
+			 "disabling interrupts: unable to bind.\n");
+		goto out;
+	}
+	supported = true;
+
+	/* Now restore the original value. */
+	pci_write_config_word(pdev, PCI_COMMAND, orig);
+
+out:
+	pci_unblock_user_cfg_access(pdev);
+	return supported;
+}
+
+static void
+pci_2_3_mask_irq(struct pci_dev *dev, int mask, unsigned int *irq_status)
+{
+	u32 cmd_status_dword;
+	u16 origcmd, newcmd;
+
+	/*
+	 * We do a single dword read to retrieve both command and status.
+	 * Document assumptions that make this possible.
+	 */
+	BUILD_BUG_ON(PCI_COMMAND % 4);
+	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
+
+	pci_block_user_cfg_access(dev);
+
+	/*
+	 * Read both command and status registers in a single 32-bit operation.
+	 * Note: we could cache the value for command and move the status read
+	 * out of the lock if there was a way to get notified of user changes
+	 * to command register through sysfs. Should be good for shared irqs.
+	 */
+	pci_read_config_dword(dev, PCI_COMMAND, &cmd_status_dword);
+	origcmd = cmd_status_dword;
+
+	if (irq_status) {
+		/*
+		* Check interrupt status register to see whether our device triggered
+		* the interrupt.
+		*/
+		*irq_status = (cmd_status_dword >> 16) & PCI_STATUS_INTERRUPT;
+		if (*irq_status == 0)
+			goto done;
+	}
+
+	newcmd = origcmd & ~PCI_COMMAND_INTX_DISABLE;
+	if (mask)
+		newcmd |= PCI_COMMAND_INTX_DISABLE;
+	if (newcmd != origcmd)
+		pci_write_config_word(dev, PCI_COMMAND, newcmd);

virt/kvm/assigned-dev.c static int find_index_from_host_irq(struct kvm_assigned_dev_kernel · line 142

Quoted 5 lines

virt/kvm/assigned-dev.c +5 -0

+
+done:
+	pci_unblock_user_cfg_access(dev);
+}
+

Let's return irq_status instead of returning through a pointer? Will save a branch and generally make the code a bit cleaner.

Quoted 23 lines

virt/kvm/assigned-dev.c +5 -4

 static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev =
 		(struct kvm_assigned_dev_kernel *) dev_id;
+	int ret = IRQ_HANDLED;
 	unsigned long flags;
 
 	spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags);

@@ -83,19 +169,34 @@ static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 				    guest_entries[i].vector, 1);
 		}
 	} else {
-		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
-			    assigned_dev->guest_irq, 1);
-
 		if (assigned_dev->irq_requested_type &
 				KVM_DEV_IRQ_GUEST_INTX) {
-			disable_irq_nosync(irq);
+			if (assigned_dev->pci_2_3) {
+				unsigned int irq_status;
+
+				if (assigned_dev->host_irq_disabled) {

virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id) · line 187

Quoted 10 lines

virt/kvm/assigned-dev.c +10 -0

+					ret = IRQ_NONE;
+					goto out;
+				}
+
+				pci_2_3_mask_irq(assigned_dev->dev, 1,
+						 &irq_status);
+				if (irq_status == 0) {
+					ret = IRQ_NONE;
+					goto out;
+				}

This will be cleaner if pci_2_3_mask_irq returns irqreturn_t.

Quoted 15 lines

virt/kvm/assigned-dev.c +6 -1

+			} else
+				disable_irq_nosync(irq);
 			assigned_dev->host_irq_disabled = true;
 		}
+
+		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
+			    assigned_dev->guest_irq, 1);
 	}
 
 out:
 	spin_unlock_irqrestore(&assigned_dev->assigned_dev_lock, flags);
-	return IRQ_HANDLED;
+	return ret;
 }
 

virt/kvm/assigned-dev.c static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian) · line 225

Quoted 11 lines

virt/kvm/assigned-dev.c +4 -1

 /* Ack the irq line for an assigned device */

@@ -117,7 +218,10 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 	 */
 	spin_lock_irqsave(&dev->assigned_dev_lock, flags);
 	if (dev->host_irq_disabled) {
-		enable_irq(dev->host_irq);
+		if (dev->pci_2_3)
+			pci_2_3_mask_irq(dev->dev, 0, NULL);
+		else
+			enable_irq(dev->host_irq);
 		dev->host_irq_disabled = false;

So what happens here is that if interrupt is still pending we will set level to 0, then get another interrupt from device which will set it back to 1. An obvious optimization is avoid all this, check pending bit and just keep level at 1.

Quoted 57 lines

virt/kvm/assigned-dev.c +26 -6

 	}
 	spin_unlock_irqrestore(&dev->assigned_dev_lock, flags);

@@ -166,7 +270,11 @@ static void deassign_host_irq(struct kvm *kvm,
 		pci_disable_msix(assigned_dev->dev);
 	} else {
 		/* Deal with MSI and INTx */
-		disable_irq(assigned_dev->host_irq);
+		if (assigned_dev->pci_2_3) {
+			pci_2_3_mask_irq(assigned_dev->dev, 1, NULL);
+			synchronize_irq(assigned_dev->host_irq);
+		} else
+			disable_irq(assigned_dev->host_irq);
 
 		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
 

@@ -214,6 +322,13 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 
 	pci_reset_function(assigned_dev->dev);
 
+	/*
+	 * Unmask the IRQ at PCI level once the reset is done - the next user
+	 * may not expect the IRQ being masked.
+	 */
+	if (assigned_dev->pci_2_3)
+		pci_2_3_mask_irq(assigned_dev->dev, 0, NULL);
+
 	pci_release_regions(assigned_dev->dev);
 	pci_disable_device(assigned_dev->dev);
 	pci_dev_put(assigned_dev->dev);

@@ -239,15 +354,26 @@ void kvm_free_all_assigned_devices(struct kvm *kvm)
 static int assigned_device_enable_host_intx(struct kvm *kvm,
 					    struct kvm_assigned_dev_kernel *dev)
 {
+	unsigned long flags = 0;
+
 	dev->host_irq = dev->dev->irq;
-	/* Even though this is PCI, we don't want to use shared
-	 * interrupts. Sharing host devices with guest-assigned devices
-	 * on the same interrupt line is not a happy situation: there
-	 * are going to be long delays in accepting, acking, etc.
+
+	/*
+	 * We can only share the IRQ line with other host devices if we are
+	 * able to disable the IRQ source at device-level - independently of
+	 * the guest driver. Otherwise host devices may suffer from unbounded
+	 * IRQ latencies when the guest keeps the line asserted.
 	 */
+	dev->pci_2_3 = pci_2_3_supported(dev->dev);
+	if (dev->pci_2_3)
+		flags = IRQF_SHARED;
+
 	if (request_irq(dev->host_irq, kvm_assigned_dev_intr,
-			0, "kvm_assigned_intx_device", (void *)dev))
+			flags, "kvm_assigned_intx_device", (void *)dev))
 		return -EIO;
+

virt/kvm/assigned-dev.c void kvm_free_all_assigned_devices(struct kvm *kvm) · line 379

Quoted 5 lines

virt/kvm/assigned-dev.c +2 -0

+	if (dev->pci_2_3)
+		pci_2_3_mask_irq(dev->dev, 0, NULL);
 	return 0;
 }
 

Let's reverse the logic and try non-shared first, 2.3 is that fails? This way we are backwards compatible ...

Quoted 23 lines

virt/kvm/assigned-dev.c +1 -2

@@ -324,7 +450,6 @@ static int assigned_device_enable_guest_msi(struct kvm *kvm,
 {
 	dev->guest_irq = irq->guest_irq;
 	dev->ack_notifier.gsi = -1;
-	dev->host_irq_disabled = false;
 	return 0;
 }
 #endif

@@ -336,7 +461,6 @@ static int assigned_device_enable_guest_msix(struct kvm *kvm,
 {
 	dev->guest_irq = irq->guest_irq;
 	dev->ack_notifier.gsi = -1;
-	dev->host_irq_disabled = false;
 	return 0;
 }
 #endif

@@ -367,6 +491,7 @@ static int assign_host_irq(struct kvm *kvm,
 	default:
 		r = -EINVAL;
 	}
+	dev->host_irq_disabled = false;
 

Quoted 4 lines

virt/kvm/assigned-dev.c +0 -0

 	if (!r)
 		dev->irq_requested_type |= host_irq_type;

Signature · 1 line

1.7.1

Quoted 113 lines · Michael S. Tsirkin

Quoted 112 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest.

Signed-off-by Jan Kiszka <[email protected]>

---
 include/linux/kvm_host.h |    1 +
 virt/kvm/assigned-dev.c  |  153 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 140 insertions(+), 14 deletions(-)

include/linux/kvm_host.h +1 -0

index df5497f..fcdc849 100644
@@ -473,6 +473,7 @@ struct kvm_assigned_dev_kernel {
 	unsigned int entries_nr;
 	int host_irq;
 	bool host_irq_disabled;
+	bool pci_2_3;
 	struct msix_entry *host_msix_entries;
 	int guest_irq;
 	struct kvm_guest_msix_entry *guest_msix_entries;

virt/kvm/assigned-dev.c +80 -0

index d3ddfea..411643c 100644
@@ -55,10 +55,96 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
 	return index;
 }
 
+/*
+ * Verify that the device supports Interrupt Disable bit in command register,
+ * per PCI 2.3, by flipping this bit and reading it back: this bit was readonly
+ * in PCI 2.2.
+ */
+static bool pci_2_3_supported(struct pci_dev *pdev)
+{
+	u16 orig, new;
+	bool supported = false;
+
+	pci_block_user_cfg_access(pdev);
+	pci_read_config_word(pdev, PCI_COMMAND, &orig);
+	pci_write_config_word(pdev, PCI_COMMAND,
+			      orig ^ PCI_COMMAND_INTX_DISABLE);
+	pci_read_config_word(pdev, PCI_COMMAND, &new);
+	
+	/*
+	 * There's no way to protect against
+	 * hardware bugs or detect them reliably, but as long as we know
+	 * what the value should be, let's go ahead and check it.
+	 */
+	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
+		dev_err(&pdev->dev, "Command changed from 0x%x to 0x%x: "
+			"driver or HW bug?\n", orig, new);
+		goto out;
+	}
+	if (!((new ^ orig) & PCI_COMMAND_INTX_DISABLE)) {
+		dev_warn(&pdev->dev, "Device does not support "
+			 "disabling interrupts: unable to bind.\n");
+		goto out;
+	}
+	supported = true;
+
+	/* Now restore the original value. */
+	pci_write_config_word(pdev, PCI_COMMAND, orig);
+
+out:
+	pci_unblock_user_cfg_access(pdev);
+	return supported;
+}
+
+static void
+pci_2_3_mask_irq(struct pci_dev *dev, int mask, unsigned int *irq_status)
+{
+	u32 cmd_status_dword;
+	u16 origcmd, newcmd;
+
+	/*
+	 * We do a single dword read to retrieve both command and status.
+	 * Document assumptions that make this possible.
+	 */
+	BUILD_BUG_ON(PCI_COMMAND % 4);
+	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
+
+	pci_block_user_cfg_access(dev);
+
+	/*
+	 * Read both command and status registers in a single 32-bit operation.
+	 * Note: we could cache the value for command and move the status read
+	 * out of the lock if there was a way to get notified of user changes
+	 * to command register through sysfs. Should be good for shared irqs.
+	 */
+	pci_read_config_dword(dev, PCI_COMMAND, &cmd_status_dword);
+	origcmd = cmd_status_dword;
+
+	if (irq_status) {
+		/*
+		* Check interrupt status register to see whether our device triggered
+		* the interrupt.
+		*/
+		*irq_status = (cmd_status_dword >> 16) & PCI_STATUS_INTERRUPT;
+		if (*irq_status == 0)
+			goto done;
+	}
+
+	newcmd = origcmd & ~PCI_COMMAND_INTX_DISABLE;
+	if (mask)
+		newcmd |= PCI_COMMAND_INTX_DISABLE;
+	if (newcmd != origcmd)
+		pci_write_config_word(dev, PCI_COMMAND, newcmd);

Quoted 8 lines

virt/kvm/assigned-dev.c static int find_index_from_host_irq(struct kvm_assigned_dev_kernel · line 142

Quoted 5 lines

virt/kvm/assigned-dev.c +5 -0

+
+done:
+	pci_unblock_user_cfg_access(dev);
+}
+

Let's return irq_status instead of returning through a pointer? Will save a branch and generally make the code a bit cleaner.

I'm open for a better API suggestion,

but the current one goes like
this: if irq_status is non-null, only mask the IRQ if the status bit indicates an interrupt. But we also have a user that wants to mask unconditionally.

Quoted 52 lines

Quoted 23 lines

virt/kvm/assigned-dev.c +5 -4

 static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev =
 		(struct kvm_assigned_dev_kernel *) dev_id;
+	int ret = IRQ_HANDLED;
 	unsigned long flags;
 
 	spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags);

@@ -83,19 +169,34 @@ static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 				    guest_entries[i].vector, 1);
 		}
 	} else {
-		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
-			    assigned_dev->guest_irq, 1);
-
 		if (assigned_dev->irq_requested_type &
 				KVM_DEV_IRQ_GUEST_INTX) {
-			disable_irq_nosync(irq);
+			if (assigned_dev->pci_2_3) {
+				unsigned int irq_status;
+
+				if (assigned_dev->host_irq_disabled) {

virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id) · line 187

Quoted 10 lines

virt/kvm/assigned-dev.c +10 -0

+					ret = IRQ_NONE;
+					goto out;
+				}
+
+				pci_2_3_mask_irq(assigned_dev->dev, 1,
+						 &irq_status);
+				if (irq_status == 0) {
+					ret = IRQ_NONE;
+					goto out;
+				}

This will be cleaner if pci_2_3_mask_irq returns irqreturn_t.

Quoted 15 lines

virt/kvm/assigned-dev.c +6 -1

+			} else
+				disable_irq_nosync(irq);
 			assigned_dev->host_irq_disabled = true;
 		}
+
+		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
+			    assigned_dev->guest_irq, 1);
 	}
 
 out:
 	spin_unlock_irqrestore(&assigned_dev->assigned_dev_lock, flags);
-	return IRQ_HANDLED;
+	return ret;
 }
 

Quoted 16 lines

virt/kvm/assigned-dev.c static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian) · line 225

Quoted 11 lines

virt/kvm/assigned-dev.c +4 -1

 /* Ack the irq line for an assigned device */

@@ -117,7 +218,10 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 	 */
 	spin_lock_irqsave(&dev->assigned_dev_lock, flags);
 	if (dev->host_irq_disabled) {
-		enable_irq(dev->host_irq);
+		if (dev->pci_2_3)
+			pci_2_3_mask_irq(dev->dev, 0, NULL);
+		else
+			enable_irq(dev->host_irq);
 		dev->host_irq_disabled = false;

So what happens here is that if interrupt is still pending we will set level to 0, then get another interrupt from device which will set it back to 1. An obvious optimization is avoid all this, check pending bit and just keep level at 1.

Isn't this an unrelated optimization, independent of this patch? But I'll think about it. What pending bit are you referring to?

Quoted 57 lines

Quoted 57 lines

virt/kvm/assigned-dev.c +26 -6

 	}
 	spin_unlock_irqrestore(&dev->assigned_dev_lock, flags);

@@ -166,7 +270,11 @@ static void deassign_host_irq(struct kvm *kvm,
 		pci_disable_msix(assigned_dev->dev);
 	} else {
 		/* Deal with MSI and INTx */
-		disable_irq(assigned_dev->host_irq);
+		if (assigned_dev->pci_2_3) {
+			pci_2_3_mask_irq(assigned_dev->dev, 1, NULL);
+			synchronize_irq(assigned_dev->host_irq);
+		} else
+			disable_irq(assigned_dev->host_irq);
 
 		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
 

@@ -214,6 +322,13 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 
 	pci_reset_function(assigned_dev->dev);
 
+	/*
+	 * Unmask the IRQ at PCI level once the reset is done - the next user
+	 * may not expect the IRQ being masked.
+	 */
+	if (assigned_dev->pci_2_3)
+		pci_2_3_mask_irq(assigned_dev->dev, 0, NULL);
+
 	pci_release_regions(assigned_dev->dev);
 	pci_disable_device(assigned_dev->dev);
 	pci_dev_put(assigned_dev->dev);

@@ -239,15 +354,26 @@ void kvm_free_all_assigned_devices(struct kvm *kvm)
 static int assigned_device_enable_host_intx(struct kvm *kvm,
 					    struct kvm_assigned_dev_kernel *dev)
 {
+	unsigned long flags = 0;
+
 	dev->host_irq = dev->dev->irq;
-	/* Even though this is PCI, we don't want to use shared
-	 * interrupts. Sharing host devices with guest-assigned devices
-	 * on the same interrupt line is not a happy situation: there
-	 * are going to be long delays in accepting, acking, etc.
+
+	/*
+	 * We can only share the IRQ line with other host devices if we are
+	 * able to disable the IRQ source at device-level - independently of
+	 * the guest driver. Otherwise host devices may suffer from unbounded
+	 * IRQ latencies when the guest keeps the line asserted.
 	 */
+	dev->pci_2_3 = pci_2_3_supported(dev->dev);
+	if (dev->pci_2_3)
+		flags = IRQF_SHARED;
+
 	if (request_irq(dev->host_irq, kvm_assigned_dev_intr,
-			0, "kvm_assigned_intx_device", (void *)dev))
+			flags, "kvm_assigned_intx_device", (void *)dev))
 		return -EIO;
+

Quoted 8 lines

virt/kvm/assigned-dev.c void kvm_free_all_assigned_devices(struct kvm *kvm) · line 379

Quoted 5 lines

virt/kvm/assigned-dev.c +2 -0

+	if (dev->pci_2_3)
+		pci_2_3_mask_irq(dev->dev, 0, NULL);
 	return 0;
 }
 

Let's reverse the logic and try non-shared first, 2.3 is that fails? This way we are backwards compatible ...

Compatible with what?

I thought about trying non-shared IRQs first, but that would break host devices arriving later - including other VMs that want to pass a device sitting on the same IRQ line. It's better (from management POV) to have IRQF_SHARED available.

Quoted 23 lines

Quoted 23 lines

virt/kvm/assigned-dev.c +1 -2

@@ -324,7 +450,6 @@ static int assigned_device_enable_guest_msi(struct kvm *kvm,
 {
 	dev->guest_irq = irq->guest_irq;
 	dev->ack_notifier.gsi = -1;
-	dev->host_irq_disabled = false;
 	return 0;
 }
 #endif

@@ -336,7 +461,6 @@ static int assigned_device_enable_guest_msix(struct kvm *kvm,
 {
 	dev->guest_irq = irq->guest_irq;
 	dev->ack_notifier.gsi = -1;
-	dev->host_irq_disabled = false;
 	return 0;
 }
 #endif

@@ -367,6 +491,7 @@ static int assign_host_irq(struct kvm *kvm,
 	default:
 		r = -EINVAL;
 	}
+	dev->host_irq_disabled = false;
 

Quoted 4 lines

Quoted 4 lines

virt/kvm/assigned-dev.c +0 -0

 	if (!r)
 		dev->irq_requested_type |= host_irq_type;

Signature · 1 line

1.7.1

Thanks for the comment,
Jan

Quoted 123 lines · Jan Kiszka

Quoted 113 lines · Michael S. Tsirkin

Quoted 112 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest.

Signed-off-by Jan Kiszka <[email protected]>

---
 include/linux/kvm_host.h |    1 +
 virt/kvm/assigned-dev.c  |  153 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 140 insertions(+), 14 deletions(-)

include/linux/kvm_host.h +1 -0

index df5497f..fcdc849 100644
@@ -473,6 +473,7 @@ struct kvm_assigned_dev_kernel {
 	unsigned int entries_nr;
 	int host_irq;
 	bool host_irq_disabled;
+	bool pci_2_3;
 	struct msix_entry *host_msix_entries;
 	int guest_irq;
 	struct kvm_guest_msix_entry *guest_msix_entries;

virt/kvm/assigned-dev.c +80 -0

index d3ddfea..411643c 100644
@@ -55,10 +55,96 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
 	return index;
 }
 
+/*
+ * Verify that the device supports Interrupt Disable bit in command register,
+ * per PCI 2.3, by flipping this bit and reading it back: this bit was readonly
+ * in PCI 2.2.
+ */
+static bool pci_2_3_supported(struct pci_dev *pdev)
+{
+	u16 orig, new;
+	bool supported = false;
+
+	pci_block_user_cfg_access(pdev);
+	pci_read_config_word(pdev, PCI_COMMAND, &orig);
+	pci_write_config_word(pdev, PCI_COMMAND,
+			      orig ^ PCI_COMMAND_INTX_DISABLE);
+	pci_read_config_word(pdev, PCI_COMMAND, &new);
+	
+	/*
+	 * There's no way to protect against
+	 * hardware bugs or detect them reliably, but as long as we know
+	 * what the value should be, let's go ahead and check it.
+	 */
+	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
+		dev_err(&pdev->dev, "Command changed from 0x%x to 0x%x: "
+			"driver or HW bug?\n", orig, new);
+		goto out;
+	}
+	if (!((new ^ orig) & PCI_COMMAND_INTX_DISABLE)) {
+		dev_warn(&pdev->dev, "Device does not support "
+			 "disabling interrupts: unable to bind.\n");
+		goto out;
+	}
+	supported = true;
+
+	/* Now restore the original value. */
+	pci_write_config_word(pdev, PCI_COMMAND, orig);
+
+out:
+	pci_unblock_user_cfg_access(pdev);
+	return supported;
+}
+
+static void
+pci_2_3_mask_irq(struct pci_dev *dev, int mask, unsigned int *irq_status)
+{
+	u32 cmd_status_dword;
+	u16 origcmd, newcmd;
+
+	/*
+	 * We do a single dword read to retrieve both command and status.
+	 * Document assumptions that make this possible.
+	 */
+	BUILD_BUG_ON(PCI_COMMAND % 4);
+	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
+
+	pci_block_user_cfg_access(dev);
+
+	/*
+	 * Read both command and status registers in a single 32-bit operation.
+	 * Note: we could cache the value for command and move the status read
+	 * out of the lock if there was a way to get notified of user changes
+	 * to command register through sysfs. Should be good for shared irqs.
+	 */
+	pci_read_config_dword(dev, PCI_COMMAND, &cmd_status_dword);
+	origcmd = cmd_status_dword;
+
+	if (irq_status) {
+		/*
+		* Check interrupt status register to see whether our device triggered
+		* the interrupt.
+		*/
+		*irq_status = (cmd_status_dword >> 16) & PCI_STATUS_INTERRUPT;
+		if (*irq_status == 0)
+			goto done;
+	}
+
+	newcmd = origcmd & ~PCI_COMMAND_INTX_DISABLE;
+	if (mask)
+		newcmd |= PCI_COMMAND_INTX_DISABLE;
+	if (newcmd != origcmd)
+		pci_write_config_word(dev, PCI_COMMAND, newcmd);

Quoted 8 lines

virt/kvm/assigned-dev.c static int find_index_from_host_irq(struct kvm_assigned_dev_kernel · line 142

Quoted 5 lines

virt/kvm/assigned-dev.c +5 -0

+
+done:
+	pci_unblock_user_cfg_access(dev);
+}
+

Let's return irq_status instead of returning through a pointer? Will save a branch and generally make the code a bit cleaner.

Quoted 1 line

I'm open for a better API suggestion,

Maybe use separate functions for this.
pci_2_3_mask_irq
pci_2_3_unmask_irq
pci_2_3_disable_irq

Common code can go into subfunctions.

Quoted 4 lines

but the current one goes like
this: if irq_status is non-null, only mask the IRQ if the status bit indicates an interrupt. But we also have a user that wants to mask unconditionally.

Why do you ever want to do that?

Quoted 70 lines

Quoted 69 lines

Quoted 24 lines

virt/kvm/assigned-dev.c +5 -4

 static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev =
 		(struct kvm_assigned_dev_kernel *) dev_id;
+	int ret = IRQ_HANDLED;
 	unsigned long flags;
 
 	spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags);

@@ -83,19 +169,34 @@ static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 				    guest_entries[i].vector, 1);
 		}
 	} else {
-		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
-			    assigned_dev->guest_irq, 1);
-
 		if (assigned_dev->irq_requested_type &
 				KVM_DEV_IRQ_GUEST_INTX) {
-			disable_irq_nosync(irq);
+			if (assigned_dev->pci_2_3) {
+				unsigned int irq_status;
+
+				if (assigned_dev->host_irq_disabled) {

virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id) · line 187

Quoted 10 lines

virt/kvm/assigned-dev.c +10 -0

+					ret = IRQ_NONE;
+					goto out;
+				}
+
+				pci_2_3_mask_irq(assigned_dev->dev, 1,
+						 &irq_status);
+				if (irq_status == 0) {
+					ret = IRQ_NONE;
+					goto out;
+				}

This will be cleaner if pci_2_3_mask_irq returns irqreturn_t.

Quoted 17 lines

virt/kvm/assigned-dev.c +6 -1

+			} else
+				disable_irq_nosync(irq);
 			assigned_dev->host_irq_disabled = true;
 		}
+
+		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
+			    assigned_dev->guest_irq, 1);
 	}
 
 out:
 	spin_unlock_irqrestore(&assigned_dev->assigned_dev_lock, flags);
-	return IRQ_HANDLED;
+	return ret;
 }
 
 /* Ack the irq line for an assigned device */

virt/kvm/assigned-dev.c static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian) · line 225

Quoted 9 lines

virt/kvm/assigned-dev.c +4 -1

@@ -117,7 +218,10 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 	 */
 	spin_lock_irqsave(&dev->assigned_dev_lock, flags);
 	if (dev->host_irq_disabled) {
-		enable_irq(dev->host_irq);
+		if (dev->pci_2_3)
+			pci_2_3_mask_irq(dev->dev, 0, NULL);
+		else
+			enable_irq(dev->host_irq);
 		dev->host_irq_disabled = false;

So what happens here is that if interrupt is still pending we will set level to 0, then get another interrupt from device which will set it back to 1. An obvious optimization is avoid all this, check pending bit and just keep level at 1.

Quoted 2 lines

Isn't this an unrelated optimization, independent of this patch? But I'll think about it. What pending bit are you referring to?

The one in PCI_STATUS.

Quoted 67 lines

Quoted 66 lines

Quoted 53 lines

virt/kvm/assigned-dev.c +24 -5

 	}
 	spin_unlock_irqrestore(&dev->assigned_dev_lock, flags);

@@ -166,7 +270,11 @@ static void deassign_host_irq(struct kvm *kvm,
 		pci_disable_msix(assigned_dev->dev);
 	} else {
 		/* Deal with MSI and INTx */
-		disable_irq(assigned_dev->host_irq);
+		if (assigned_dev->pci_2_3) {
+			pci_2_3_mask_irq(assigned_dev->dev, 1, NULL);
+			synchronize_irq(assigned_dev->host_irq);
+		} else
+			disable_irq(assigned_dev->host_irq);
 
 		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
 

@@ -214,6 +322,13 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 
 	pci_reset_function(assigned_dev->dev);
 
+	/*
+	 * Unmask the IRQ at PCI level once the reset is done - the next user
+	 * may not expect the IRQ being masked.
+	 */
+	if (assigned_dev->pci_2_3)
+		pci_2_3_mask_irq(assigned_dev->dev, 0, NULL);
+
 	pci_release_regions(assigned_dev->dev);
 	pci_disable_device(assigned_dev->dev);
 	pci_dev_put(assigned_dev->dev);

@@ -239,15 +354,26 @@ void kvm_free_all_assigned_devices(struct kvm *kvm)
 static int assigned_device_enable_host_intx(struct kvm *kvm,
 					    struct kvm_assigned_dev_kernel *dev)
 {
+	unsigned long flags = 0;
+
 	dev->host_irq = dev->dev->irq;
-	/* Even though this is PCI, we don't want to use shared
-	 * interrupts. Sharing host devices with guest-assigned devices
-	 * on the same interrupt line is not a happy situation: there
-	 * are going to be long delays in accepting, acking, etc.
+
+	/*
+	 * We can only share the IRQ line with other host devices if we are
+	 * able to disable the IRQ source at device-level - independently of
+	 * the guest driver. Otherwise host devices may suffer from unbounded
+	 * IRQ latencies when the guest keeps the line asserted.
 	 */
+	dev->pci_2_3 = pci_2_3_supported(dev->dev);
+	if (dev->pci_2_3)
+		flags = IRQF_SHARED;
+

virt/kvm/assigned-dev.c void kvm_free_all_assigned_devices(struct kvm *kvm) · line 379

Quoted 10 lines

virt/kvm/assigned-dev.c +4 -1

 	if (request_irq(dev->host_irq, kvm_assigned_dev_intr,
-			0, "kvm_assigned_intx_device", (void *)dev))
+			flags, "kvm_assigned_intx_device", (void *)dev))
 		return -EIO;
+
+	if (dev->pci_2_3)
+		pci_2_3_mask_irq(dev->dev, 0, NULL);
 	return 0;
 }
 

Let's reverse the logic and try non-shared first, 2.3 is that fails? This way we are backwards compatible ...

Quoted 1 line

Compatible with what?

With the status quo :)

Quoted 5 lines

I thought about trying non-shared IRQs first, but that would break host devices arriving later - including other VMs that want to pass a device sitting on the same IRQ line. It's better (from management POV) to have IRQF_SHARED available.

OTOH non-shared might be faster as we don't need to do config writes/reads on data path ... Add a knob for management to control this?

Quoted 138 lines · Michael S. Tsirkin

Quoted 123 lines · Jan Kiszka

Quoted 113 lines · Michael S. Tsirkin

Quoted 112 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices between on the host side when passing them to a guest.

Signed-off-by Jan Kiszka <[email protected]>

---
 include/linux/kvm_host.h |    1 +
 virt/kvm/assigned-dev.c  |  153 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 140 insertions(+), 14 deletions(-)

include/linux/kvm_host.h +1 -0

index df5497f..fcdc849 100644
@@ -473,6 +473,7 @@ struct kvm_assigned_dev_kernel {
 	unsigned int entries_nr;
 	int host_irq;
 	bool host_irq_disabled;
+	bool pci_2_3;
 	struct msix_entry *host_msix_entries;
 	int guest_irq;
 	struct kvm_guest_msix_entry *guest_msix_entries;

virt/kvm/assigned-dev.c +80 -0

index d3ddfea..411643c 100644
@@ -55,10 +55,96 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
 	return index;
 }
 
+/*
+ * Verify that the device supports Interrupt Disable bit in command register,
+ * per PCI 2.3, by flipping this bit and reading it back: this bit was readonly
+ * in PCI 2.2.
+ */
+static bool pci_2_3_supported(struct pci_dev *pdev)
+{
+	u16 orig, new;
+	bool supported = false;
+
+	pci_block_user_cfg_access(pdev);
+	pci_read_config_word(pdev, PCI_COMMAND, &orig);
+	pci_write_config_word(pdev, PCI_COMMAND,
+			      orig ^ PCI_COMMAND_INTX_DISABLE);
+	pci_read_config_word(pdev, PCI_COMMAND, &new);
+	
+	/*
+	 * There's no way to protect against
+	 * hardware bugs or detect them reliably, but as long as we know
+	 * what the value should be, let's go ahead and check it.
+	 */
+	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
+		dev_err(&pdev->dev, "Command changed from 0x%x to 0x%x: "
+			"driver or HW bug?\n", orig, new);
+		goto out;
+	}
+	if (!((new ^ orig) & PCI_COMMAND_INTX_DISABLE)) {
+		dev_warn(&pdev->dev, "Device does not support "
+			 "disabling interrupts: unable to bind.\n");
+		goto out;
+	}
+	supported = true;
+
+	/* Now restore the original value. */
+	pci_write_config_word(pdev, PCI_COMMAND, orig);
+
+out:
+	pci_unblock_user_cfg_access(pdev);
+	return supported;
+}
+
+static void
+pci_2_3_mask_irq(struct pci_dev *dev, int mask, unsigned int *irq_status)
+{
+	u32 cmd_status_dword;
+	u16 origcmd, newcmd;
+
+	/*
+	 * We do a single dword read to retrieve both command and status.
+	 * Document assumptions that make this possible.
+	 */
+	BUILD_BUG_ON(PCI_COMMAND % 4);
+	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
+
+	pci_block_user_cfg_access(dev);
+
+	/*
+	 * Read both command and status registers in a single 32-bit operation.
+	 * Note: we could cache the value for command and move the status read
+	 * out of the lock if there was a way to get notified of user changes
+	 * to command register through sysfs. Should be good for shared irqs.
+	 */
+	pci_read_config_dword(dev, PCI_COMMAND, &cmd_status_dword);
+	origcmd = cmd_status_dword;
+
+	if (irq_status) {
+		/*
+		* Check interrupt status register to see whether our device triggered
+		* the interrupt.
+		*/
+		*irq_status = (cmd_status_dword >> 16) & PCI_STATUS_INTERRUPT;
+		if (*irq_status == 0)
+			goto done;
+	}
+
+	newcmd = origcmd & ~PCI_COMMAND_INTX_DISABLE;
+	if (mask)
+		newcmd |= PCI_COMMAND_INTX_DISABLE;
+	if (newcmd != origcmd)
+		pci_write_config_word(dev, PCI_COMMAND, newcmd);

Quoted 8 lines

virt/kvm/assigned-dev.c static int find_index_from_host_irq(struct kvm_assigned_dev_kernel · line 142

Quoted 5 lines

virt/kvm/assigned-dev.c +5 -0

+
+done:
+	pci_unblock_user_cfg_access(dev);
+}
+

Let's return irq_status instead of returning through a pointer? Will save a branch and generally make the code a bit cleaner.

Quoted 1 line

I'm open for a better API suggestion,

Maybe use separate functions for this.
pci_2_3_mask_irq
pci_2_3_unmask_irq
pci_2_3_disable_irq

Common code can go into subfunctions.

Quoted 4 lines

but the current one goes like
this: if irq_status is non-null, only mask the IRQ if the status bit indicates an interrupt. But we also have a user that wants to mask unconditionally.

Quoted 1 line

Why do you ever want to do that?

During device shutdown (was disable_irq in the unshared case so far). The alternative would be to reset the device first, clearing any potentially pending events. If we can reorder the reset, that is likely better.

Quoted 70 lines

Quoted 70 lines

Quoted 69 lines

Quoted 24 lines

virt/kvm/assigned-dev.c +5 -4

 static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev =
 		(struct kvm_assigned_dev_kernel *) dev_id;
+	int ret = IRQ_HANDLED;
 	unsigned long flags;
 
 	spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags);

@@ -83,19 +169,34 @@ static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 				    guest_entries[i].vector, 1);
 		}
 	} else {
-		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
-			    assigned_dev->guest_irq, 1);
-
 		if (assigned_dev->irq_requested_type &
 				KVM_DEV_IRQ_GUEST_INTX) {
-			disable_irq_nosync(irq);
+			if (assigned_dev->pci_2_3) {
+				unsigned int irq_status;
+
+				if (assigned_dev->host_irq_disabled) {

virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id) · line 187

Quoted 10 lines

virt/kvm/assigned-dev.c +10 -0

+					ret = IRQ_NONE;
+					goto out;
+				}
+
+				pci_2_3_mask_irq(assigned_dev->dev, 1,
+						 &irq_status);
+				if (irq_status == 0) {
+					ret = IRQ_NONE;
+					goto out;
+				}

This will be cleaner if pci_2_3_mask_irq returns irqreturn_t.

Quoted 17 lines

virt/kvm/assigned-dev.c +6 -1

+			} else
+				disable_irq_nosync(irq);
 			assigned_dev->host_irq_disabled = true;
 		}
+
+		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
+			    assigned_dev->guest_irq, 1);
 	}
 
 out:
 	spin_unlock_irqrestore(&assigned_dev->assigned_dev_lock, flags);
-	return IRQ_HANDLED;
+	return ret;
 }
 
 /* Ack the irq line for an assigned device */

virt/kvm/assigned-dev.c static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian) · line 225

Quoted 9 lines

virt/kvm/assigned-dev.c +4 -1

@@ -117,7 +218,10 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 	 */
 	spin_lock_irqsave(&dev->assigned_dev_lock, flags);
 	if (dev->host_irq_disabled) {
-		enable_irq(dev->host_irq);
+		if (dev->pci_2_3)
+			pci_2_3_mask_irq(dev->dev, 0, NULL);
+		else
+			enable_irq(dev->host_irq);
 		dev->host_irq_disabled = false;

So what happens here is that if interrupt is still pending we will set level to 0, then get another interrupt from device which will set it back to 1. An obvious optimization is avoid all this, check pending bit and just keep level at 1.

Quoted 4 lines

Quoted 2 lines

Isn't this an unrelated optimization, independent of this patch? But I'll think about it. What pending bit are you referring to?

The one in PCI_STATUS.

Ah, OK.

Quoted 67 lines

Quoted 67 lines

Quoted 66 lines

Quoted 53 lines

virt/kvm/assigned-dev.c +24 -5

 	}
 	spin_unlock_irqrestore(&dev->assigned_dev_lock, flags);

@@ -166,7 +270,11 @@ static void deassign_host_irq(struct kvm *kvm,
 		pci_disable_msix(assigned_dev->dev);
 	} else {
 		/* Deal with MSI and INTx */
-		disable_irq(assigned_dev->host_irq);
+		if (assigned_dev->pci_2_3) {
+			pci_2_3_mask_irq(assigned_dev->dev, 1, NULL);
+			synchronize_irq(assigned_dev->host_irq);
+		} else
+			disable_irq(assigned_dev->host_irq);
 
 		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
 

@@ -214,6 +322,13 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 
 	pci_reset_function(assigned_dev->dev);
 
+	/*
+	 * Unmask the IRQ at PCI level once the reset is done - the next user
+	 * may not expect the IRQ being masked.
+	 */
+	if (assigned_dev->pci_2_3)
+		pci_2_3_mask_irq(assigned_dev->dev, 0, NULL);
+
 	pci_release_regions(assigned_dev->dev);
 	pci_disable_device(assigned_dev->dev);
 	pci_dev_put(assigned_dev->dev);

@@ -239,15 +354,26 @@ void kvm_free_all_assigned_devices(struct kvm *kvm)
 static int assigned_device_enable_host_intx(struct kvm *kvm,
 					    struct kvm_assigned_dev_kernel *dev)
 {
+	unsigned long flags = 0;
+
 	dev->host_irq = dev->dev->irq;
-	/* Even though this is PCI, we don't want to use shared
-	 * interrupts. Sharing host devices with guest-assigned devices
-	 * on the same interrupt line is not a happy situation: there
-	 * are going to be long delays in accepting, acking, etc.
+
+	/*
+	 * We can only share the IRQ line with other host devices if we are
+	 * able to disable the IRQ source at device-level - independently of
+	 * the guest driver. Otherwise host devices may suffer from unbounded
+	 * IRQ latencies when the guest keeps the line asserted.
 	 */
+	dev->pci_2_3 = pci_2_3_supported(dev->dev);
+	if (dev->pci_2_3)
+		flags = IRQF_SHARED;
+

virt/kvm/assigned-dev.c void kvm_free_all_assigned_devices(struct kvm *kvm) · line 379

Quoted 10 lines

virt/kvm/assigned-dev.c +4 -1

 	if (request_irq(dev->host_irq, kvm_assigned_dev_intr,
-			0, "kvm_assigned_intx_device", (void *)dev))
+			flags, "kvm_assigned_intx_device", (void *)dev))
 		return -EIO;
+
+	if (dev->pci_2_3)
+		pci_2_3_mask_irq(dev->dev, 0, NULL);
 	return 0;
 }
 

Let's reverse the logic and try non-shared first, 2.3 is that fails? This way we are backwards compatible ...

Quoted 3 lines

Quoted 1 line

Compatible with what?

With the status quo :)

There is no incompatibility except for a potential slow-down of a path that was often usable (exclusive legacy interrupts belong to a very rare species).

Quoted 9 lines

Quoted 5 lines

I thought about trying non-shared IRQs first, but that would break host devices arriving later - including other VMs that want to pass a device sitting on the same IRQ line. It's better (from management POV) to have IRQF_SHARED available.

OTOH non-shared might be faster as we don't need to do config writes/reads on data path ... Add a knob for management to control this?

Depends on how fast config writes actually are. I know they were slow (up to awfully slow if bios was involved) on old hardware, but does this still apply? I mean, a config knob would involve the whole stack, so it should be worth that effort.

Jan