[PATCH v2 0/4] KVM: Improve IRQ assignment for device passthrough

[PATCH v2 0/4] KVM: Improve IRQ assignment for device passthrough

[Jan Kiszka]

Four patches to improve "classic" device assigment /wrt IRQs. Highlight
is the last one that resolves the host IRQ sharing issue for all PCI 2.3
devices. Quite essential when passing non-MSI-ready devices like many
USB host controllers.

Changes in v2:
 - Reworked IRQ forwarding path to use threaded IRQs (direct signalling
   from IRQ context does not work out of the box and may be too lengthy)
 - Refactored host IRQ naming of assigned devices (cosmetic change)
 - Avoid unmask on ack when the next IRQ is pending, rather reassert the
   guest line (PCI-2.3 patch)
 - Refactored PCI-2.3 patch (but still no control knob for shared mode -
   is that a must?)

Jan Kiszka (4):
  KVM: Clear assigned guest IRQ on release
  KVM: Switch assigned device IRQ forwarding to threaded handler
  KVM: Refactor IRQ names of assigned devices
  KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

 include/linux/kvm_host.h |   14 +--
 virt/kvm/assigned-dev.c  |  279 +++++++++++++++++++++++++++++++++------------
 2 files changed, 208 insertions(+), 85 deletions(-)

[PATCH v2 1/4] KVM: Clear assigned guest IRQ on release

[Jan Kiszka]

When we deassign a guest IRQ, clear the potentially asserted guest line.
There might be no chance for the guest to do this, specifically if we
switch from INTx to MSI mode.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 virt/kvm/assigned-dev.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index 7c98928..ecc4419 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -141,6 +141,9 @@ static void deassign_guest_irq(struct kvm *kvm,
 	kvm_unregister_irq_ack_notifier(kvm, &assigned_dev->ack_notifier);
 	assigned_dev->ack_notifier.gsi = -1;
 
+	kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
+		    assigned_dev->guest_irq, 0);
+
 	if (assigned_dev->irq_source_id != -1)
 		kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
 	assigned_dev->irq_source_id = -1;
-- 
1.7.1

[PATCH v2 2/4] KVM: Switch assigned device IRQ forwarding to threaded handler

[Jan Kiszka]

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 include/linux/kvm_host.h |   12 +----
 virt/kvm/assigned-dev.c  |  106 ++++++++++++++-------------------------------
 2 files changed, 35 insertions(+), 83 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 462b982..a786419 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -456,16 +456,8 @@ struct kvm_irq_ack_notifier {
 	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;
@@ -476,13 +468,13 @@ struct kvm_assigned_dev_kernel {
 	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 {
diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index ecc4419..d0b07ea 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -55,58 +55,31 @@ 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_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 @@ out:
 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 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 	/* 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,28 +127,19 @@ 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,
@@ -188,8 +151,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);
 
@@ -268,8 +230,8 @@ static int assigned_device_enable_host_intx(struct kvm *kvm,
 	 * 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,
+				 0, "kvm_assigned_intx_device", (void *)dev))
 		return -EIO;
 	return 0;
 }
@@ -287,8 +249,8 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
 	}
 
 	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 +275,10 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,
 		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 +519,10 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	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 +614,9 @@ static int kvm_vm_ioctl_set_msix_nr(struct kvm *kvm,
 			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;
-- 
1.7.1

Re: [PATCH v2 2/4] KVM: Switch assigned device IRQ forwarding to threaded handler

[Michael S. Tsirkin]

On Tue, Nov 02, 2010 at 04:49:18PM +0100, Jan Kiszka wrote:
> 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.

Interesting. Do you see any latency improvements from this?

> 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.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  include/linux/kvm_host.h |   12 +----
>  virt/kvm/assigned-dev.c  |  106 ++++++++++++++-------------------------------
>  2 files changed, 35 insertions(+), 83 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 462b982..a786419 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -456,16 +456,8 @@ struct kvm_irq_ack_notifier {
>  	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;
> @@ -476,13 +468,13 @@ struct kvm_assigned_dev_kernel {
>  	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 {
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index ecc4419..d0b07ea 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -55,58 +55,31 @@ 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_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 @@ out:
>  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 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
>  	/* 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,28 +127,19 @@ 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,
> @@ -188,8 +151,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);
>  
> @@ -268,8 +230,8 @@ static int assigned_device_enable_host_intx(struct kvm *kvm,
>  	 * 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,
> +				 0, "kvm_assigned_intx_device", (void *)dev))
>  		return -EIO;
>  	return 0;
>  }
> @@ -287,8 +249,8 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
>  	}
>  
>  	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 +275,10 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,
>  		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 +519,10 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
>  	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 +614,9 @@ static int kvm_vm_ioctl_set_msix_nr(struct kvm *kvm,
>  			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;
> -- 
> 1.7.1

Re: [PATCH v2 2/4] KVM: Switch assigned device IRQ forwarding to threaded handler

[Jan Kiszka]

Am 02.11.2010 18:44, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2010 at 04:49:18PM +0100, Jan Kiszka wrote:
>> 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.
> 
> Interesting. Do you see any latency improvements from this?

Haven't measured, but I was primarily thinking of real-time scenarios,
ie. setups where you may want adjust the threaded IRQ priority -
something you cannot do with the shared kevent thread.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[PATCH v2 3/4] KVM: Refactor IRQ names of assigned devices

[Jan Kiszka]

Cosmetic change, but it helps to correlate IRQs with PCI devices.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 include/linux/kvm_host.h |    1 +
 virt/kvm/assigned-dev.c  |   10 ++++++----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index a786419..46120da 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -475,6 +475,7 @@ struct kvm_assigned_dev_kernel {
 	struct pci_dev *dev;
 	struct kvm *kvm;
 	spinlock_t intx_lock;
+	char irq_name[32];
 };
 
 struct kvm_irq_mask_notifier {
diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index d0b07ea..ca402ed 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -231,7 +231,7 @@ static int assigned_device_enable_host_intx(struct kvm *kvm,
 	 * are going to be long delays in accepting, acking, etc.
 	 */
 	if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
-				 0, "kvm_assigned_intx_device", (void *)dev))
+				 0, dev->irq_name, (void *)dev))
 		return -EIO;
 	return 0;
 }
@@ -250,7 +250,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
 
 	dev->host_irq = dev->dev->irq;
 	if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
-				 0, "kvm_assigned_msi_device", (void *)dev)) {
+				 0, dev->irq_name, (void *)dev)) {
 		pci_disable_msi(dev->dev);
 		return -EIO;
 	}
@@ -277,8 +277,7 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,
 	for (i = 0; i < dev->entries_nr; i++) {
 		r = request_threaded_irq(dev->host_msix_entries[i].vector,
 					 NULL, kvm_assigned_dev_thread,
-					 0, "kvm_assigned_msix_device",
-					 (void *)dev);
+					 0, dev->irq_name, (void *)dev);
 		if (r)
 			goto err;
 	}
@@ -335,6 +334,9 @@ static int assign_host_irq(struct kvm *kvm,
 	if (dev->irq_requested_type & KVM_DEV_IRQ_HOST_MASK)
 		return r;
 
+	snprintf(dev->irq_name, sizeof(dev->irq_name), "kvm:%s",
+		 pci_name(dev->dev));
+
 	switch (host_irq_type) {
 	case KVM_DEV_IRQ_HOST_INTX:
 		r = assigned_device_enable_host_intx(kvm, dev);
-- 
1.7.1

[PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

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 <jan.kiszka@siemens.com>
---
 include/linux/kvm_host.h |    1 +
 virt/kvm/assigned-dev.c  |  194 ++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 180 insertions(+), 15 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 46120da..fdc2bd9 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -466,6 +466,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 msix_entry *guest_msix_entries;
diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index ca402ed..91fe9c8 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -55,17 +55,145 @@ 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)
+{
+	bool supported = false;
+	u16 orig, new;
+
+	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 unsigned int
+pci_2_3_set_irq_mask(struct pci_dev *dev, bool mask, bool check_status)
+{
+	u32 cmd_status_dword;
+	u16 origcmd, newcmd;
+	unsigned int status;
+
+	/*
+	 * 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;
+	status = cmd_status_dword >> 16;
+
+	if (check_status) {
+		bool irq_pending = status & PCI_STATUS_INTERRUPT;
+
+		/*
+		 * Check interrupt status register to see whether our device
+		 * triggered the interrupt (when masking) or the next IRQ is
+		 * already pending (when unmasking).
+		 */
+		if (!(mask == irq_pending))
+			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);
+
+done:
+	pci_unblock_user_cfg_access(dev);
+	return status;
+}
+
+static void pci_2_3_irq_mask(struct pci_dev *dev)
+{
+	pci_2_3_set_irq_mask(dev, true, false);
+}
+
+static unsigned int pci_2_3_irq_check_and_mask(struct pci_dev *dev)
+{
+	return pci_2_3_set_irq_mask(dev, true, true);
+}
+
+static void pci_2_3_irq_unmask(struct pci_dev *dev)
+{
+	pci_2_3_set_irq_mask(dev, false, false);
+}
+
+static unsigned int pci_2_3_irq_check_and_unmask(struct pci_dev *dev)
+{
+	return pci_2_3_set_irq_mask(dev, false, true);
+}
+
+static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
+{
+	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
+	int ret = IRQ_WAKE_THREAD;
+
+	spin_lock(&assigned_dev->intx_lock);
+	if (assigned_dev->host_irq_disabled ||
+	    !(pci_2_3_irq_check_and_mask(assigned_dev->dev) &
+			PCI_STATUS_INTERRUPT))
+		ret = IRQ_NONE;
+	else
+		assigned_dev->host_irq_disabled = true;
+	spin_unlock(&assigned_dev->intx_lock);
+
+	return ret;
+}
+
 static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
 	u32 vector;
 	int index;
 
-	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX) {
-		spin_lock(&assigned_dev->intx_lock);
+	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX &&
+	    !assigned_dev->pci_2_3) {
+		spin_lock_irq(&assigned_dev->intx_lock);
 		disable_irq_nosync(irq);
 		assigned_dev->host_irq_disabled = true;
-		spin_unlock(&assigned_dev->intx_lock);
+		spin_unlock_irq(&assigned_dev->intx_lock);
 	}
 
 	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
@@ -87,6 +215,7 @@ static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
 static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 {
 	struct kvm_assigned_dev_kernel *dev;
+	bool reassert = false;
 
 	if (kian->gsi == -1)
 		return;
@@ -99,12 +228,23 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 	/* The guest irq may be shared so this ack may be
 	 * from another device.
 	 */
-	spin_lock(&dev->intx_lock);
+	spin_lock_irq(&dev->intx_lock);
 	if (dev->host_irq_disabled) {
-		enable_irq(dev->host_irq);
+		if (dev->pci_2_3) {
+			if (pci_2_3_irq_check_and_unmask(dev->dev) &
+			    PCI_STATUS_INTERRUPT) {
+				reassert = true;
+				goto out;
+			}
+		} else
+			enable_irq(dev->host_irq);
 		dev->host_irq_disabled = false;
 	}
-	spin_unlock(&dev->intx_lock);
+out:
+	spin_unlock_irq(&dev->intx_lock);
+
+	if (reassert)
+		kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 1);
 }
 
 static void deassign_guest_irq(struct kvm *kvm,
@@ -151,7 +291,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_irq_mask(assigned_dev->dev);
+			synchronize_irq(assigned_dev->host_irq);
+		} else
+			disable_irq(assigned_dev->host_irq);
 
 		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
 
@@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
+
 	pci_release_regions(assigned_dev->dev);
 	pci_disable_device(assigned_dev->dev);
 	pci_dev_put(assigned_dev->dev);
@@ -224,15 +375,29 @@ 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)
 {
+	irq_handler_t irq_handler = NULL;
+	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.
 	 */
-	if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
-				 0, dev->irq_name, (void *)dev))
+	dev->pci_2_3 = pci_2_3_supported(dev->dev);
+	if (dev->pci_2_3) {
+		irq_handler = kvm_assigned_dev_intr;
+		flags = IRQF_SHARED;
+	}
+	if (request_threaded_irq(dev->host_irq, irq_handler,
+				 kvm_assigned_dev_thread, flags,
+				 dev->irq_name, (void *)dev))
 		return -EIO;
+
+	if (dev->pci_2_3)
+		pci_2_3_irq_unmask(dev->dev);
 	return 0;
 }
 
@@ -308,7 +473,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
@@ -320,7 +484,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
@@ -354,6 +517,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;
-- 
1.7.1

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Michael S. Tsirkin]

On Tue, Nov 02, 2010 at 04:49:20PM +0100, Jan Kiszka wrote:
> 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 <jan.kiszka@siemens.com>
> ---
>  include/linux/kvm_host.h |    1 +
>  virt/kvm/assigned-dev.c  |  194 ++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 180 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 46120da..fdc2bd9 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -466,6 +466,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 msix_entry *guest_msix_entries;
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index ca402ed..91fe9c8 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -55,17 +55,145 @@ 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)
> +{
> +	bool supported = false;
> +	u16 orig, new;
> +
> +	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 unsigned int
> +pci_2_3_set_irq_mask(struct pci_dev *dev, bool mask, bool check_status)
> +{
> +	u32 cmd_status_dword;
> +	u16 origcmd, newcmd;
> +	unsigned int status;
> +
> +	/*
> +	 * 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;
> +	status = cmd_status_dword >> 16;
> +
> +	if (check_status) {
> +		bool irq_pending = status & PCI_STATUS_INTERRUPT;
> +
> +		/*
> +		 * Check interrupt status register to see whether our device
> +		 * triggered the interrupt (when masking) or the next IRQ is
> +		 * already pending (when unmasking).
> +		 */
> +		if (!(mask == irq_pending))

Same as mask != irq_pending?

> +			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);
> +
> +done:
> +	pci_unblock_user_cfg_access(dev);
> +	return status;
> +}
> +
> +static void pci_2_3_irq_mask(struct pci_dev *dev)
> +{
> +	pci_2_3_set_irq_mask(dev, true, false);
> +}
> +
> +static unsigned int pci_2_3_irq_check_and_mask(struct pci_dev *dev)
> +{
> +	return pci_2_3_set_irq_mask(dev, true, true);
> +}
> +
> +static void pci_2_3_irq_unmask(struct pci_dev *dev)
> +{
> +	pci_2_3_set_irq_mask(dev, false, false);
> +}
> +
> +static unsigned int pci_2_3_irq_check_and_unmask(struct pci_dev *dev)
> +{
> +	return pci_2_3_set_irq_mask(dev, false, true);
> +}
> +

IMO this is not a terribly good interface: all users check the pending bit
(PCI_STATUS_INTERRUPT) which is what the function pci_2_3_set_irq_mask
did anyway.  I'd suggest returning irqreturn_t or bool and not unsigned
int.


> +static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
> +{
> +	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> +	int ret = IRQ_WAKE_THREAD;
> +
> +	spin_lock(&assigned_dev->intx_lock);
> +	if (assigned_dev->host_irq_disabled ||
> +	    !(pci_2_3_irq_check_and_mask(assigned_dev->dev) &
> +			PCI_STATUS_INTERRUPT))
> +		ret = IRQ_NONE;
> +	else
> +		assigned_dev->host_irq_disabled = true;

This is a bug I think.  For pci 2.3 we should never track interrupt
state in kvm IMO.  For example, if userspace unmasks an interrupt
through a config write, we will get an interrupt while host_irq_disabled
is set. If we then fail to mask it, kaboom.

> +	spin_unlock(&assigned_dev->intx_lock);
> +
> +	return ret;
> +}
> +
>  static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
>  {
>  	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
>  	u32 vector;
>  	int index;
>  
> -	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX) {
> -		spin_lock(&assigned_dev->intx_lock);
> +	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX &&
> +	    !assigned_dev->pci_2_3) {
> +		spin_lock_irq(&assigned_dev->intx_lock);
>  		disable_irq_nosync(irq);
>  		assigned_dev->host_irq_disabled = true;
> -		spin_unlock(&assigned_dev->intx_lock);
> +		spin_unlock_irq(&assigned_dev->intx_lock);
>  	}
>  
>  	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
> @@ -87,6 +215,7 @@ static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
>  static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
>  {
>  	struct kvm_assigned_dev_kernel *dev;
> +	bool reassert = false;
>  
>  	if (kian->gsi == -1)
>  		return;
> @@ -99,12 +228,23 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
>  	/* The guest irq may be shared so this ack may be
>  	 * from another device.
>  	 */
> -	spin_lock(&dev->intx_lock);
> +	spin_lock_irq(&dev->intx_lock);
>  	if (dev->host_irq_disabled) {
> -		enable_irq(dev->host_irq);
> +		if (dev->pci_2_3) {
> +			if (pci_2_3_irq_check_and_unmask(dev->dev) &
> +			    PCI_STATUS_INTERRUPT) {
> +				reassert = true;
> +				goto out;
> +			}
> +		} else
> +			enable_irq(dev->host_irq);

Or

		if (!dev->pci_2_3)
			enable_irq(dev->host_irq);
		else if (pci_2_3_irq_check_and_unmask(dev->dev) & PCI_STATUS_INTERRUPT) {
			reassert = true;
			goto out;
		}

to reduce nesting.

>  		dev->host_irq_disabled = false;
>  	}
> -	spin_unlock(&dev->intx_lock);
> +out:
> +	spin_unlock_irq(&dev->intx_lock);
> +
> +	if (reassert)
> +		kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 1);

Hmm, I think this still has more overhead than it needs to have.
Instead of setting level to 0 and then back to 1, can't we just
avoid set to 1 in the first place? This would need a different
interface than pci_2_3_irq_check_and_unmask to avoid a race
where interrupt is received while we are acking another one:

	block userspace access
	check pending bit
	if (!pending)
		set irq (0)
	clear pending
	block userspace access

Would be worth it for high volume devices.

>  }
>  
>  static void deassign_guest_irq(struct kvm *kvm,
> @@ -151,7 +291,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_irq_mask(assigned_dev->dev);
> +			synchronize_irq(assigned_dev->host_irq);
> +		} else
> +			disable_irq(assigned_dev->host_irq);
>  
>  		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
>  
> @@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
> +

Doesn't pci_reset_function clear mask bit? It seems to ...

>  	pci_release_regions(assigned_dev->dev);
>  	pci_disable_device(assigned_dev->dev);
>  	pci_dev_put(assigned_dev->dev);
> @@ -224,15 +375,29 @@ 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)
>  {
> +	irq_handler_t irq_handler = NULL;
> +	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.
>  	 */
> -	if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
> -				 0, dev->irq_name, (void *)dev))
> +	dev->pci_2_3 = pci_2_3_supported(dev->dev);
> +	if (dev->pci_2_3) {
> +		irq_handler = kvm_assigned_dev_intr;
> +		flags = IRQF_SHARED;
> +	}

I would prefer and else clause here instead of initializing
variables at the top and overwriting here. Makes it easier
to see which value is valid when.

> +	if (request_threaded_irq(dev->host_irq, irq_handler,
> +				 kvm_assigned_dev_thread, flags,
> +				 dev->irq_name, (void *)dev))
>  		return -EIO;
> +
> +	if (dev->pci_2_3)
> +		pci_2_3_irq_unmask(dev->dev);
>  	return 0;
>  }
>  
> @@ -308,7 +473,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
> @@ -320,7 +484,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
> @@ -354,6 +517,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;
> -- 
> 1.7.1

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

Am 02.11.2010 18:41, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2010 at 04:49:20PM +0100, Jan Kiszka wrote:
>> 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 <jan.kiszka@siemens.com>
>> ---
>>  include/linux/kvm_host.h |    1 +
>>  virt/kvm/assigned-dev.c  |  194 ++++++++++++++++++++++++++++++++++++++++++----
>>  2 files changed, 180 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 46120da..fdc2bd9 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -466,6 +466,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 msix_entry *guest_msix_entries;
>> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
>> index ca402ed..91fe9c8 100644
>> --- a/virt/kvm/assigned-dev.c
>> +++ b/virt/kvm/assigned-dev.c
>> @@ -55,17 +55,145 @@ 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)
>> +{
>> +	bool supported = false;
>> +	u16 orig, new;
>> +
>> +	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 unsigned int
>> +pci_2_3_set_irq_mask(struct pci_dev *dev, bool mask, bool check_status)
>> +{
>> +	u32 cmd_status_dword;
>> +	u16 origcmd, newcmd;
>> +	unsigned int status;
>> +
>> +	/*
>> +	 * 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;
>> +	status = cmd_status_dword >> 16;
>> +
>> +	if (check_status) {
>> +		bool irq_pending = status & PCI_STATUS_INTERRUPT;
>> +
>> +		/*
>> +		 * Check interrupt status register to see whether our device
>> +		 * triggered the interrupt (when masking) or the next IRQ is
>> +		 * already pending (when unmasking).
>> +		 */
>> +		if (!(mask == irq_pending))
> 
> Same as mask != irq_pending?

Yes. Relict of various refactorings.

> 
>> +			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);
>> +
>> +done:
>> +	pci_unblock_user_cfg_access(dev);
>> +	return status;
>> +}
>> +
>> +static void pci_2_3_irq_mask(struct pci_dev *dev)
>> +{
>> +	pci_2_3_set_irq_mask(dev, true, false);
>> +}
>> +
>> +static unsigned int pci_2_3_irq_check_and_mask(struct pci_dev *dev)
>> +{
>> +	return pci_2_3_set_irq_mask(dev, true, true);
>> +}
>> +
>> +static void pci_2_3_irq_unmask(struct pci_dev *dev)
>> +{
>> +	pci_2_3_set_irq_mask(dev, false, false);
>> +}
>> +
>> +static unsigned int pci_2_3_irq_check_and_unmask(struct pci_dev *dev)
>> +{
>> +	return pci_2_3_set_irq_mask(dev, false, true);
>> +}
>> +
> 
> IMO this is not a terribly good interface: all users check the pending bit
> (PCI_STATUS_INTERRUPT) which is what the function pci_2_3_set_irq_mask
> did anyway.  I'd suggest returning irqreturn_t or bool and not unsigned
> int.

Agreed. Originally, I thought there are more bits in the status word the
caller may make use of. But there are in fact none.

> 
> 
>> +static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
>> +{
>> +	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
>> +	int ret = IRQ_WAKE_THREAD;
>> +
>> +	spin_lock(&assigned_dev->intx_lock);
>> +	if (assigned_dev->host_irq_disabled ||
>> +	    !(pci_2_3_irq_check_and_mask(assigned_dev->dev) &
>> +			PCI_STATUS_INTERRUPT))
>> +		ret = IRQ_NONE;
>> +	else
>> +		assigned_dev->host_irq_disabled = true;
> 
> This is a bug I think.  For pci 2.3 we should never track interrupt
> state in kvm IMO.  For example, if userspace unmasks an interrupt
> through a config write, we will get an interrupt while host_irq_disabled
> is set. If we then fail to mask it, kaboom.

Good point. There is no way around evaluating the status word as long as
user space can fiddle with INTX_DISABLE.

> 
>> +	spin_unlock(&assigned_dev->intx_lock);
>> +
>> +	return ret;
>> +}
>> +
>>  static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
>>  {
>>  	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
>>  	u32 vector;
>>  	int index;
>>  
>> -	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX) {
>> -		spin_lock(&assigned_dev->intx_lock);
>> +	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX &&
>> +	    !assigned_dev->pci_2_3) {
>> +		spin_lock_irq(&assigned_dev->intx_lock);
>>  		disable_irq_nosync(irq);
>>  		assigned_dev->host_irq_disabled = true;
>> -		spin_unlock(&assigned_dev->intx_lock);
>> +		spin_unlock_irq(&assigned_dev->intx_lock);
>>  	}
>>  
>>  	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
>> @@ -87,6 +215,7 @@ static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
>>  static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
>>  {
>>  	struct kvm_assigned_dev_kernel *dev;
>> +	bool reassert = false;
>>  
>>  	if (kian->gsi == -1)
>>  		return;
>> @@ -99,12 +228,23 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
>>  	/* The guest irq may be shared so this ack may be
>>  	 * from another device.
>>  	 */
>> -	spin_lock(&dev->intx_lock);
>> +	spin_lock_irq(&dev->intx_lock);
>>  	if (dev->host_irq_disabled) {
>> -		enable_irq(dev->host_irq);
>> +		if (dev->pci_2_3) {
>> +			if (pci_2_3_irq_check_and_unmask(dev->dev) &
>> +			    PCI_STATUS_INTERRUPT) {
>> +				reassert = true;
>> +				goto out;
>> +			}
>> +		} else
>> +			enable_irq(dev->host_irq);
> 
> Or
> 
> 		if (!dev->pci_2_3)
> 			enable_irq(dev->host_irq);
> 		else if (pci_2_3_irq_check_and_unmask(dev->dev) & PCI_STATUS_INTERRUPT) {
> 			reassert = true;
> 			goto out;
> 		}
> 
> to reduce nesting.

Yeah.

> 
>>  		dev->host_irq_disabled = false;
>>  	}
>> -	spin_unlock(&dev->intx_lock);
>> +out:
>> +	spin_unlock_irq(&dev->intx_lock);
>> +
>> +	if (reassert)
>> +		kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 1);
> 
> Hmm, I think this still has more overhead than it needs to have.
> Instead of setting level to 0 and then back to 1, can't we just
> avoid set to 1 in the first place? This would need a different
> interface than pci_2_3_irq_check_and_unmask to avoid a race
> where interrupt is received while we are acking another one:
> 
> 	block userspace access
> 	check pending bit
> 	if (!pending)
> 		set irq (0)
> 	clear pending
> 	block userspace access
> 
> Would be worth it for high volume devices.

The problem is that we can't reorder guest IRQ line clearing and host
IRQ line enabling without taking a lock across host IRQ disable + guest
IRQ raise - and that is now distributed across hard and threaded IRQ
handlers and we don't want to hold and IRQ-safe lock during kvm_set_irq.

> 
>>  }
>>  
>>  static void deassign_guest_irq(struct kvm *kvm,
>> @@ -151,7 +291,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_irq_mask(assigned_dev->dev);
>> +			synchronize_irq(assigned_dev->host_irq);
>> +		} else
>> +			disable_irq(assigned_dev->host_irq);
>>  
>>  		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
>>  
>> @@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
>> +
> 
> Doesn't pci_reset_function clear mask bit? It seems to ...

I was left with non-functional devices for the host here if I was not
doing this. Need to recheck, but I think it was required.

> 
>>  	pci_release_regions(assigned_dev->dev);
>>  	pci_disable_device(assigned_dev->dev);
>>  	pci_dev_put(assigned_dev->dev);
>> @@ -224,15 +375,29 @@ 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)
>>  {
>> +	irq_handler_t irq_handler = NULL;
>> +	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.
>>  	 */
>> -	if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
>> -				 0, dev->irq_name, (void *)dev))
>> +	dev->pci_2_3 = pci_2_3_supported(dev->dev);
>> +	if (dev->pci_2_3) {
>> +		irq_handler = kvm_assigned_dev_intr;
>> +		flags = IRQF_SHARED;
>> +	}
> 
> I would prefer and else clause here instead of initializing
> variables at the top and overwriting here. Makes it easier
> to see which value is valid when.

OK.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Michael S. Tsirkin]

On Tue, Nov 02, 2010 at 06:56:14PM +0100, Jan Kiszka wrote:
> >> @@ -99,12 +228,23 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
> >>  	/* The guest irq may be shared so this ack may be
> >>  	 * from another device.
> >>  	 */
> >> -	spin_lock(&dev->intx_lock);
> >> +	spin_lock_irq(&dev->intx_lock);
> >>  	if (dev->host_irq_disabled) {
> >> -		enable_irq(dev->host_irq);
> >> +		if (dev->pci_2_3) {
> >> +			if (pci_2_3_irq_check_and_unmask(dev->dev) &
> >> +			    PCI_STATUS_INTERRUPT) {
> >> +				reassert = true;
> >> +				goto out;
> >> +			}
> >> +		} else
> >> +			enable_irq(dev->host_irq);
> > 
> > Or
> > 
> > 		if (!dev->pci_2_3)
> > 			enable_irq(dev->host_irq);
> > 		else if (pci_2_3_irq_check_and_unmask(dev->dev) & PCI_STATUS_INTERRUPT) {
> > 			reassert = true;
> > 			goto out;
> > 		}
> > 
> > to reduce nesting.
> 
> Yeah.
> 
> > 
> >>  		dev->host_irq_disabled = false;
> >>  	}
> >> -	spin_unlock(&dev->intx_lock);
> >> +out:
> >> +	spin_unlock_irq(&dev->intx_lock);
> >> +
> >> +	if (reassert)
> >> +		kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 1);
> > 
> > Hmm, I think this still has more overhead than it needs to have.
> > Instead of setting level to 0 and then back to 1, can't we just
> > avoid set to 1 in the first place? This would need a different
> > interface than pci_2_3_irq_check_and_unmask to avoid a race
> > where interrupt is received while we are acking another one:
> > 
> > 	block userspace access
> > 	check pending bit
> > 	if (!pending)
> > 		set irq (0)
> > 	clear pending
> > 	block userspace access
> > 
> > Would be worth it for high volume devices.
> 
> The problem is that we can't reorder guest IRQ line clearing and host
> IRQ line enabling without taking a lock across host IRQ disable + guest
> IRQ raise - and that is now distributed across hard and threaded IRQ
> handlers and we don't want to hold and IRQ-safe lock during kvm_set_irq.

Oh I think I confused you.
What I mean is:

 	block userspace access
 	check interrupt status bit
 	if (!interrupt status bit set)
 		set irq (0)
 	clear interrupt disable bit
 	block userspace access

This way we enable interrupt after set irq so not need for
extra locks I think.

Hmm one thing I noticed is that pci_block_user_cfg_access
will BUG_ON if it was already blocked. So I think we have
a bug here when interrupt handler kicks in right after
we unmask interrupts.

Probably need some kind of lock to protect against this.


> > 
> >>  }
> >>  
> >>  static void deassign_guest_irq(struct kvm *kvm,
> >> @@ -151,7 +291,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_irq_mask(assigned_dev->dev);
> >> +			synchronize_irq(assigned_dev->host_irq);
> >> +		} else
> >> +			disable_irq(assigned_dev->host_irq);
> >>  
> >>  		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
> >>  
> >> @@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
> >> +
> > 
> > Doesn't pci_reset_function clear mask bit? It seems to ...
> 
> I was left with non-functional devices for the host here if I was not
> doing this. Need to recheck, but I think it was required.

Interesting. Could you check why please?


> > 
> >>  	pci_release_regions(assigned_dev->dev);
> >>  	pci_disable_device(assigned_dev->dev);
> >>  	pci_dev_put(assigned_dev->dev);
> >> @@ -224,15 +375,29 @@ 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)
> >>  {
> >> +	irq_handler_t irq_handler = NULL;
> >> +	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.
> >>  	 */
> >> -	if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
> >> -				 0, dev->irq_name, (void *)dev))
> >> +	dev->pci_2_3 = pci_2_3_supported(dev->dev);
> >> +	if (dev->pci_2_3) {
> >> +		irq_handler = kvm_assigned_dev_intr;
> >> +		flags = IRQF_SHARED;
> >> +	}
> > 
> > I would prefer and else clause here instead of initializing
> > variables at the top and overwriting here. Makes it easier
> > to see which value is valid when.
> 
> OK.
> 
> Thanks,
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

Am 02.11.2010 19:24, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2010 at 06:56:14PM +0100, Jan Kiszka wrote:
>>>>  		dev->host_irq_disabled = false;
>>>>  	}
>>>> -	spin_unlock(&dev->intx_lock);
>>>> +out:
>>>> +	spin_unlock_irq(&dev->intx_lock);
>>>> +
>>>> +	if (reassert)
>>>> +		kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 1);
>>>
>>> Hmm, I think this still has more overhead than it needs to have.
>>> Instead of setting level to 0 and then back to 1, can't we just
>>> avoid set to 1 in the first place? This would need a different
>>> interface than pci_2_3_irq_check_and_unmask to avoid a race
>>> where interrupt is received while we are acking another one:
>>>
>>> 	block userspace access
>>> 	check pending bit
>>> 	if (!pending)
>>> 		set irq (0)
>>> 	clear pending
>>> 	block userspace access
>>>
>>> Would be worth it for high volume devices.
>>
>> The problem is that we can't reorder guest IRQ line clearing and host
>> IRQ line enabling without taking a lock across host IRQ disable + guest
>> IRQ raise - and that is now distributed across hard and threaded IRQ
>> handlers and we don't want to hold and IRQ-safe lock during kvm_set_irq.
> 
> Oh I think I confused you.
> What I mean is:
> 
>  	block userspace access
>  	check interrupt status bit
>  	if (!interrupt status bit set)
>  		set irq (0)
>  	clear interrupt disable bit
>  	block userspace access
> 
> This way we enable interrupt after set irq so not need for
> extra locks I think.

OK. Would require some serious refactoring again.

But what about edge IRQs? Don't we need to toggle the bit for them? And
as we do not differentiate between level and edge, we currently have to
do this unconditionally.

> 
> Hmm one thing I noticed is that pci_block_user_cfg_access
> will BUG_ON if it was already blocked. So I think we have
> a bug here when interrupt handler kicks in right after
> we unmask interrupts.
> 
> Probably need some kind of lock to protect against this.
> 

Or an atomic counter. Will have a look.

Alex, does VFIO take care of this already?

> 
>>>
>>>>  }
>>>>  
>>>>  static void deassign_guest_irq(struct kvm *kvm,
>>>> @@ -151,7 +291,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_irq_mask(assigned_dev->dev);
>>>> +			synchronize_irq(assigned_dev->host_irq);
>>>> +		} else
>>>> +			disable_irq(assigned_dev->host_irq);
>>>>  
>>>>  		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
>>>>  
>>>> @@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
>>>> +
>>>
>>> Doesn't pci_reset_function clear mask bit? It seems to ...
>>
>> I was left with non-functional devices for the host here if I was not
>> doing this. Need to recheck, but I think it was required.
> 
> Interesting. Could you check why please?
> 

Can't reproduce anymore. This was early code, maybe affected by some
bits or buts that no longer exist.

Spec says it's cleared on reset, so I removed those lines now.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

Am 02.11.2010 19:40, Jan Kiszka wrote:
>>>>> @@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
>>>>> +
>>>>
>>>> Doesn't pci_reset_function clear mask bit? It seems to ...
>>>
>>> I was left with non-functional devices for the host here if I was not
>>> doing this. Need to recheck, but I think it was required.
>>
>> Interesting. Could you check why please?
>>
> 
> Can't reproduce anymore. This was early code, maybe affected by some
> bits or buts that no longer exist.
> 
> Spec says it's cleared on reset, so I removed those lines now.
> 

Hmpf, it just happened again: Guest was using my ath9k, I killed the
guest, lspci says DisINTx+. Strange. Is anyone (qemu) restoring the
config space after the reset, bringing the disable bit back?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

Am 02.11.2010 19:48, Jan Kiszka wrote:
> Am 02.11.2010 19:40, Jan Kiszka wrote:
>>>>>> @@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
>>>>>> +
>>>>>
>>>>> Doesn't pci_reset_function clear mask bit? It seems to ...
>>>>
>>>> I was left with non-functional devices for the host here if I was not
>>>> doing this. Need to recheck, but I think it was required.
>>>
>>> Interesting. Could you check why please?
>>>
>>
>> Can't reproduce anymore. This was early code, maybe affected by some
>> bits or buts that no longer exist.
>>
>> Spec says it's cleared on reset, so I removed those lines now.
>>
> 
> Hmpf, it just happened again: Guest was using my ath9k, I killed the
> guest, lspci says DisINTx+. Strange. Is anyone (qemu) restoring the
> config space after the reset, bringing the disable bit back?

Or does the kernel cache the control word?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Michael S. Tsirkin]

On Tue, Nov 02, 2010 at 07:51:39PM +0100, Jan Kiszka wrote:
> Am 02.11.2010 19:48, Jan Kiszka wrote:
> > Am 02.11.2010 19:40, Jan Kiszka wrote:
> >>>>>> @@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
> >>>>>> +
> >>>>>
> >>>>> Doesn't pci_reset_function clear mask bit? It seems to ...
> >>>>
> >>>> I was left with non-functional devices for the host here if I was not
> >>>> doing this. Need to recheck, but I think it was required.
> >>>
> >>> Interesting. Could you check why please?
> >>>
> >>
> >> Can't reproduce anymore. This was early code, maybe affected by some
> >> bits or buts that no longer exist.
> >>
> >> Spec says it's cleared on reset, so I removed those lines now.
> >>
> > 
> > Hmpf, it just happened again: Guest was using my ath9k, I killed the
> > guest, lspci says DisINTx+. Strange. Is anyone (qemu) restoring the
> > config space after the reset, bringing the disable bit back?
> 
> Or does the kernel cache the control word?
> 
> Jan

Maybe it does, need to dig in drivers/pci. If it does this
might have other implications.

> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

Am 02.11.2010 19:54, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2010 at 07:51:39PM +0100, Jan Kiszka wrote:
>> Am 02.11.2010 19:48, Jan Kiszka wrote:
>>> Am 02.11.2010 19:40, Jan Kiszka wrote:
>>>>>>>> @@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
>>>>>>>> +
>>>>>>>
>>>>>>> Doesn't pci_reset_function clear mask bit? It seems to ...
>>>>>>
>>>>>> I was left with non-functional devices for the host here if I was not
>>>>>> doing this. Need to recheck, but I think it was required.
>>>>>
>>>>> Interesting. Could you check why please?
>>>>>
>>>>
>>>> Can't reproduce anymore. This was early code, maybe affected by some
>>>> bits or buts that no longer exist.
>>>>
>>>> Spec says it's cleared on reset, so I removed those lines now.
>>>>
>>>
>>> Hmpf, it just happened again: Guest was using my ath9k, I killed the
>>> guest, lspci says DisINTx+. Strange. Is anyone (qemu) restoring the
>>> config space after the reset, bringing the disable bit back?
>>
>> Or does the kernel cache the control word?
>>
>> Jan
> 
> Maybe it does, need to dig in drivers/pci. If it does this
> might have other implications.

OK, that mystery is resolved now: pci_reset_function saves & restores
the device state.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Michael S. Tsirkin]

On Tue, Nov 02, 2010 at 08:30:25PM +0100, Jan Kiszka wrote:
> Am 02.11.2010 19:54, Michael S. Tsirkin wrote:
> > On Tue, Nov 02, 2010 at 07:51:39PM +0100, Jan Kiszka wrote:
> >> Am 02.11.2010 19:48, Jan Kiszka wrote:
> >>> Am 02.11.2010 19:40, Jan Kiszka wrote:
> >>>>>>>> @@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
> >>>>>>>> +
> >>>>>>>
> >>>>>>> Doesn't pci_reset_function clear mask bit? It seems to ...
> >>>>>>
> >>>>>> I was left with non-functional devices for the host here if I was not
> >>>>>> doing this. Need to recheck, but I think it was required.
> >>>>>
> >>>>> Interesting. Could you check why please?
> >>>>>
> >>>>
> >>>> Can't reproduce anymore. This was early code, maybe affected by some
> >>>> bits or buts that no longer exist.
> >>>>
> >>>> Spec says it's cleared on reset, so I removed those lines now.
> >>>>
> >>>
> >>> Hmpf, it just happened again: Guest was using my ath9k, I killed the
> >>> guest, lspci says DisINTx+. Strange. Is anyone (qemu) restoring the
> >>> config space after the reset, bringing the disable bit back?
> >>
> >> Or does the kernel cache the control word?
> >>
> >> Jan
> > 
> > Maybe it does, need to dig in drivers/pci. If it does this
> > might have other implications.
> 
> OK, that mystery is resolved now: pci_reset_function saves & restores
> the device state.
> 
> Jan

Aha. I wonder what other state we need to clear.

> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1935 bytes --]

Am 02.11.2010 20:53, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2010 at 08:30:25PM +0100, Jan Kiszka wrote:
>> Am 02.11.2010 19:54, Michael S. Tsirkin wrote:
>>> On Tue, Nov 02, 2010 at 07:51:39PM +0100, Jan Kiszka wrote:
>>>> Am 02.11.2010 19:48, Jan Kiszka wrote:
>>>>> Am 02.11.2010 19:40, Jan Kiszka wrote:
>>>>>>>>>> @@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> Doesn't pci_reset_function clear mask bit? It seems to ...
>>>>>>>>
>>>>>>>> I was left with non-functional devices for the host here if I was not
>>>>>>>> doing this. Need to recheck, but I think it was required.
>>>>>>>
>>>>>>> Interesting. Could you check why please?
>>>>>>>
>>>>>>
>>>>>> Can't reproduce anymore. This was early code, maybe affected by some
>>>>>> bits or buts that no longer exist.
>>>>>>
>>>>>> Spec says it's cleared on reset, so I removed those lines now.
>>>>>>
>>>>>
>>>>> Hmpf, it just happened again: Guest was using my ath9k, I killed the
>>>>> guest, lspci says DisINTx+. Strange. Is anyone (qemu) restoring the
>>>>> config space after the reset, bringing the disable bit back?
>>>>
>>>> Or does the kernel cache the control word?
>>>>
>>>> Jan
>>>
>>> Maybe it does, need to dig in drivers/pci. If it does this
>>> might have other implications.
>>
>> OK, that mystery is resolved now: pci_reset_function saves & restores
>> the device state.
>>
>> Jan
> 
> Aha. I wonder what other state we need to clear.
> 

Maybe just save/restore before/after assigning the device?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Michael S. Tsirkin]

On Tue, Nov 02, 2010 at 08:58:36PM +0100, Jan Kiszka wrote:
> Am 02.11.2010 20:53, Michael S. Tsirkin wrote:
> > On Tue, Nov 02, 2010 at 08:30:25PM +0100, Jan Kiszka wrote:
> >> Am 02.11.2010 19:54, Michael S. Tsirkin wrote:
> >>> On Tue, Nov 02, 2010 at 07:51:39PM +0100, Jan Kiszka wrote:
> >>>> Am 02.11.2010 19:48, Jan Kiszka wrote:
> >>>>> Am 02.11.2010 19:40, Jan Kiszka wrote:
> >>>>>>>>>> @@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
> >>>>>>>>>> +
> >>>>>>>>>
> >>>>>>>>> Doesn't pci_reset_function clear mask bit? It seems to ...
> >>>>>>>>
> >>>>>>>> I was left with non-functional devices for the host here if I was not
> >>>>>>>> doing this. Need to recheck, but I think it was required.
> >>>>>>>
> >>>>>>> Interesting. Could you check why please?
> >>>>>>>
> >>>>>>
> >>>>>> Can't reproduce anymore. This was early code, maybe affected by some
> >>>>>> bits or buts that no longer exist.
> >>>>>>
> >>>>>> Spec says it's cleared on reset, so I removed those lines now.
> >>>>>>
> >>>>>
> >>>>> Hmpf, it just happened again: Guest was using my ath9k, I killed the
> >>>>> guest, lspci says DisINTx+. Strange. Is anyone (qemu) restoring the
> >>>>> config space after the reset, bringing the disable bit back?
> >>>>
> >>>> Or does the kernel cache the control word?
> >>>>
> >>>> Jan
> >>>
> >>> Maybe it does, need to dig in drivers/pci. If it does this
> >>> might have other implications.
> >>
> >> OK, that mystery is resolved now: pci_reset_function saves & restores
> >> the device state.
> >>
> >> Jan
> > 
> > Aha. I wonder what other state we need to clear.
> > 
> 
> Maybe just save/restore before/after assigning the device?
> 
> Jan
> 

Yea. Sounds good.

-- 
MST

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Michael S. Tsirkin]

On Tue, Nov 02, 2010 at 07:40:46PM +0100, Jan Kiszka wrote:
> Am 02.11.2010 19:24, Michael S. Tsirkin wrote:
> > On Tue, Nov 02, 2010 at 06:56:14PM +0100, Jan Kiszka wrote:
> >>>>  		dev->host_irq_disabled = false;
> >>>>  	}
> >>>> -	spin_unlock(&dev->intx_lock);
> >>>> +out:
> >>>> +	spin_unlock_irq(&dev->intx_lock);
> >>>> +
> >>>> +	if (reassert)
> >>>> +		kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 1);
> >>>
> >>> Hmm, I think this still has more overhead than it needs to have.
> >>> Instead of setting level to 0 and then back to 1, can't we just
> >>> avoid set to 1 in the first place? This would need a different
> >>> interface than pci_2_3_irq_check_and_unmask to avoid a race
> >>> where interrupt is received while we are acking another one:
> >>>
> >>> 	block userspace access
> >>> 	check pending bit
> >>> 	if (!pending)
> >>> 		set irq (0)
> >>> 	clear pending
> >>> 	block userspace access
> >>>
> >>> Would be worth it for high volume devices.
> >>
> >> The problem is that we can't reorder guest IRQ line clearing and host
> >> IRQ line enabling without taking a lock across host IRQ disable + guest
> >> IRQ raise - and that is now distributed across hard and threaded IRQ
> >> handlers and we don't want to hold and IRQ-safe lock during kvm_set_irq.
> > 
> > Oh I think I confused you.
> > What I mean is:
> > 
> >  	block userspace access
> >  	check interrupt status bit
> >  	if (!interrupt status bit set)
> >  		set irq (0)
> >  	clear interrupt disable bit
> >  	block userspace access
> > 
> > This way we enable interrupt after set irq so not need for
> > extra locks I think.
> 
> OK. Would require some serious refactoring again.

That would also mean we can't just solve the nested block/unblock
problem with a simple lock.  Not sure this is worth the effort.

> But what about edge IRQs? Don't we need to toggle the bit for them? And
> as we do not differentiate between level and edge, we currently have to
> do this unconditionally.

AFAIK PCI IRQs are level, so I don't think we need to bother.

> > 
> > Hmm one thing I noticed is that pci_block_user_cfg_access
> > will BUG_ON if it was already blocked. So I think we have
> > a bug here when interrupt handler kicks in right after
> > we unmask interrupts.
> > 
> > Probably need some kind of lock to protect against this.
> > 
> 
> Or an atomic counter.

BTW block userspace access uses a global spinlock which will likely hurt
us on multi-CPU. Switching that to something more SMP friendly, e.g. a
per-device spinlock, might be a good idea: I don't see why that lock and
queue are global.

> Will have a look.

Need to also consider an interrupt running in parallel
with unmasking in thread.

> Alex, does VFIO take care of this already?

VFIO does this all under spin_lock_irq.

> > 
> >>>
> >>>>  }
> >>>>  
> >>>>  static void deassign_guest_irq(struct kvm *kvm,
> >>>> @@ -151,7 +291,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_irq_mask(assigned_dev->dev);
> >>>> +			synchronize_irq(assigned_dev->host_irq);
> >>>> +		} else
> >>>> +			disable_irq(assigned_dev->host_irq);
> >>>>  
> >>>>  		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
> >>>>  
> >>>> @@ -199,6 +343,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_irq_unmask(assigned_dev->dev);
> >>>> +
> >>>
> >>> Doesn't pci_reset_function clear mask bit? It seems to ...
> >>
> >> I was left with non-functional devices for the host here if I was not
> >> doing this. Need to recheck, but I think it was required.
> > 
> > Interesting. Could you check why please?
> > 
> 
> Can't reproduce anymore. This was early code, maybe affected by some
> bits or buts that no longer exist.
> 
> Spec says it's cleared on reset, so I removed those lines now.
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

Am 02.11.2010 19:52, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2010 at 07:40:46PM +0100, Jan Kiszka wrote:
>> Am 02.11.2010 19:24, Michael S. Tsirkin wrote:
>>> On Tue, Nov 02, 2010 at 06:56:14PM +0100, Jan Kiszka wrote:
>>>>>>  		dev->host_irq_disabled = false;
>>>>>>  	}
>>>>>> -	spin_unlock(&dev->intx_lock);
>>>>>> +out:
>>>>>> +	spin_unlock_irq(&dev->intx_lock);
>>>>>> +
>>>>>> +	if (reassert)
>>>>>> +		kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 1);
>>>>>
>>>>> Hmm, I think this still has more overhead than it needs to have.
>>>>> Instead of setting level to 0 and then back to 1, can't we just
>>>>> avoid set to 1 in the first place? This would need a different
>>>>> interface than pci_2_3_irq_check_and_unmask to avoid a race
>>>>> where interrupt is received while we are acking another one:
>>>>>
>>>>> 	block userspace access
>>>>> 	check pending bit
>>>>> 	if (!pending)
>>>>> 		set irq (0)
>>>>> 	clear pending
>>>>> 	block userspace access
>>>>>
>>>>> Would be worth it for high volume devices.
>>>>
>>>> The problem is that we can't reorder guest IRQ line clearing and host
>>>> IRQ line enabling without taking a lock across host IRQ disable + guest
>>>> IRQ raise - and that is now distributed across hard and threaded IRQ
>>>> handlers and we don't want to hold and IRQ-safe lock during kvm_set_irq.
>>>
>>> Oh I think I confused you.
>>> What I mean is:
>>>
>>>  	block userspace access
>>>  	check interrupt status bit
>>>  	if (!interrupt status bit set)
>>>  		set irq (0)
>>>  	clear interrupt disable bit
>>>  	block userspace access
>>>
>>> This way we enable interrupt after set irq so not need for
>>> extra locks I think.
>>
>> OK. Would require some serious refactoring again.
> 
> That would also mean we can't just solve the nested block/unblock
> problem with a simple lock.  Not sure this is worth the effort.

Can't follow: what can be nested and how?

> 
>> But what about edge IRQs? Don't we need to toggle the bit for them? And
>> as we do not differentiate between level and edge, we currently have to
>> do this unconditionally.
> 
> AFAIK PCI IRQs are level, so I don't think we need to bother.

Ah, indeed.

> 
>>>
>>> Hmm one thing I noticed is that pci_block_user_cfg_access
>>> will BUG_ON if it was already blocked. So I think we have
>>> a bug here when interrupt handler kicks in right after
>>> we unmask interrupts.
>>>
>>> Probably need some kind of lock to protect against this.
>>>
>>
>> Or an atomic counter.
> 
> BTW block userspace access uses a global spinlock which will likely hurt
> us on multi-CPU. Switching that to something more SMP friendly, e.g. a
> per-device spinlock, might be a good idea: I don't see why that lock and
> queue are global.

Been through that code recently, hairy stuff. pci_lock also protects the
bus operation which can be overloaded (e.g. for error injection - if
that is not the only use case). So we need a per-bus lock, but that can
still cause contentions if devices on the same bus are handled on
different cpus.

I think the whole PCI config interface is simply not designed for
performance. It's considered a slow path, which it normally is.

> 
>> Will have a look.
> 
> Need to also consider an interrupt running in parallel
> with unmasking in thread.
> 
>> Alex, does VFIO take care of this already?
> 
> VFIO does this all under spin_lock_irq.

Everything has its pros and cons...

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Michael S. Tsirkin]

On Tue, Nov 02, 2010 at 08:11:31PM +0100, Jan Kiszka wrote:
> Am 02.11.2010 19:52, Michael S. Tsirkin wrote:
> > On Tue, Nov 02, 2010 at 07:40:46PM +0100, Jan Kiszka wrote:
> >> Am 02.11.2010 19:24, Michael S. Tsirkin wrote:
> >>> On Tue, Nov 02, 2010 at 06:56:14PM +0100, Jan Kiszka wrote:
> >>>>>>  		dev->host_irq_disabled = false;
> >>>>>>  	}
> >>>>>> -	spin_unlock(&dev->intx_lock);
> >>>>>> +out:
> >>>>>> +	spin_unlock_irq(&dev->intx_lock);
> >>>>>> +
> >>>>>> +	if (reassert)
> >>>>>> +		kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 1);
> >>>>>
> >>>>> Hmm, I think this still has more overhead than it needs to have.
> >>>>> Instead of setting level to 0 and then back to 1, can't we just
> >>>>> avoid set to 1 in the first place? This would need a different
> >>>>> interface than pci_2_3_irq_check_and_unmask to avoid a race
> >>>>> where interrupt is received while we are acking another one:
> >>>>>
> >>>>> 	block userspace access
> >>>>> 	check pending bit
> >>>>> 	if (!pending)
> >>>>> 		set irq (0)
> >>>>> 	clear pending
> >>>>> 	block userspace access
> >>>>>
> >>>>> Would be worth it for high volume devices.
> >>>>
> >>>> The problem is that we can't reorder guest IRQ line clearing and host
> >>>> IRQ line enabling without taking a lock across host IRQ disable + guest
> >>>> IRQ raise - and that is now distributed across hard and threaded IRQ
> >>>> handlers and we don't want to hold and IRQ-safe lock during kvm_set_irq.
> >>>
> >>> Oh I think I confused you.
> >>> What I mean is:
> >>>
> >>>  	block userspace access
> >>>  	check interrupt status bit
> >>>  	if (!interrupt status bit set)
> >>>  		set irq (0)
> >>>  	clear interrupt disable bit
> >>>  	block userspace access
> >>>
> >>> This way we enable interrupt after set irq so not need for
> >>> extra locks I think.
> >>
> >> OK. Would require some serious refactoring again.
> > 
> > That would also mean we can't just solve the nested block/unblock
> > problem with a simple lock.  Not sure this is worth the effort.
> 
> Can't follow: what can be nested and how?

I just mean interrupt trying to block userspace when thread
did that already.

> > 
> >> But what about edge IRQs? Don't we need to toggle the bit for them? And
> >> as we do not differentiate between level and edge, we currently have to
> >> do this unconditionally.
> > 
> > AFAIK PCI IRQs are level, so I don't think we need to bother.
> 
> Ah, indeed.
> 
> > 
> >>>
> >>> Hmm one thing I noticed is that pci_block_user_cfg_access
> >>> will BUG_ON if it was already blocked. So I think we have
> >>> a bug here when interrupt handler kicks in right after
> >>> we unmask interrupts.
> >>>
> >>> Probably need some kind of lock to protect against this.
> >>>
> >>
> >> Or an atomic counter.
> > 
> > BTW block userspace access uses a global spinlock which will likely hurt
> > us on multi-CPU. Switching that to something more SMP friendly, e.g. a
> > per-device spinlock, might be a good idea: I don't see why that lock and
> > queue are global.
> 
> Been through that code recently, hairy stuff. pci_lock also protects the
> bus operation which can be overloaded (e.g. for error injection - if
> that is not the only use case). So we need a per-bus lock, but that can
> still cause contentions if devices on the same bus are handled on
> different cpus.

Right. So this lock got reused for blocking userspace, I do not suggest
we rip it all out, just make userspace blocking use
a finer-grained lock.

> I think the whole PCI config interface is simply not designed for
> performance. It's considered a slow path, which it normally is.

So I guess we'll need to fix that now, at least if we
want to make the 2.3 way the default.

> > 
> >> Will have a look.
> > 
> > Need to also consider an interrupt running in parallel
> > with unmasking in thread.
> > 
> >> Alex, does VFIO take care of this already?
> > 
> > VFIO does this all under spin_lock_irq.
> 
> Everything has its pros and cons...
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1512 bytes --]

Am 02.11.2010 20:14, Michael S. Tsirkin wrote:
>>> BTW block userspace access uses a global spinlock which will likely hurt
>>> us on multi-CPU. Switching that to something more SMP friendly, e.g. a
>>> per-device spinlock, might be a good idea: I don't see why that lock and
>>> queue are global.
>>
>> Been through that code recently, hairy stuff. pci_lock also protects the
>> bus operation which can be overloaded (e.g. for error injection - if
>> that is not the only use case). So we need a per-bus lock, but that can
>> still cause contentions if devices on the same bus are handled on
>> different cpus.
> 
> Right. So this lock got reused for blocking userspace, I do not suggest
> we rip it all out, just make userspace blocking use
> a finer-grained lock.
> 
>> I think the whole PCI config interface is simply not designed for
>> performance. It's considered a slow path, which it normally is.
> 
> So I guess we'll need to fix that now, at least if we
> want to make the 2.3 way the default.
> 

On many systems (those with "direct" PCI config access), there is
another lock down the road: pci_config_lock. That can't be broken up as
the protected resource is unique. So we do not gain much improving the
higher-level lock.

BTW, accessing the interrupt controller for IRQ line fiddling is not a
per-device thing either. So as long as the latency of the code under the
locks is not significantly worse with PCI-level masking, we don't lose
scalability here.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Alex Williamson]

On Tue, 2010-11-02 at 19:40 +0100, Jan Kiszka wrote:
> Am 02.11.2010 19:24, Michael S. Tsirkin wrote:
> > On Tue, Nov 02, 2010 at 06:56:14PM +0100, Jan Kiszka wrote:
> >>>>  		dev->host_irq_disabled = false;
> >>>>  	}
> >>>> -	spin_unlock(&dev->intx_lock);
> >>>> +out:
> >>>> +	spin_unlock_irq(&dev->intx_lock);
> >>>> +
> >>>> +	if (reassert)
> >>>> +		kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 1);
> >>>
> >>> Hmm, I think this still has more overhead than it needs to have.
> >>> Instead of setting level to 0 and then back to 1, can't we just
> >>> avoid set to 1 in the first place? This would need a different
> >>> interface than pci_2_3_irq_check_and_unmask to avoid a race
> >>> where interrupt is received while we are acking another one:
> >>>
> >>> 	block userspace access
> >>> 	check pending bit
> >>> 	if (!pending)
> >>> 		set irq (0)
> >>> 	clear pending
> >>> 	block userspace access
> >>>
> >>> Would be worth it for high volume devices.
> >>
> >> The problem is that we can't reorder guest IRQ line clearing and host
> >> IRQ line enabling without taking a lock across host IRQ disable + guest
> >> IRQ raise - and that is now distributed across hard and threaded IRQ
> >> handlers and we don't want to hold and IRQ-safe lock during kvm_set_irq.
> > 
> > Oh I think I confused you.
> > What I mean is:
> > 
> >  	block userspace access
> >  	check interrupt status bit
> >  	if (!interrupt status bit set)
> >  		set irq (0)
> >  	clear interrupt disable bit
> >  	block userspace access
> > 
> > This way we enable interrupt after set irq so not need for
> > extra locks I think.
> 
> OK. Would require some serious refactoring again.
> 
> But what about edge IRQs? Don't we need to toggle the bit for them? And
> as we do not differentiate between level and edge, we currently have to
> do this unconditionally.
> 
> > 
> > Hmm one thing I noticed is that pci_block_user_cfg_access
> > will BUG_ON if it was already blocked. So I think we have
> > a bug here when interrupt handler kicks in right after
> > we unmask interrupts.
> > 
> > Probably need some kind of lock to protect against this.
> > 
> 
> Or an atomic counter. Will have a look.
> 
> Alex, does VFIO take care of this already?

Yes, VFIO has a lock used by the interrupt handler and the EOI handler
that prevents them from both blocking user cfg access at the same time.

Alex

Re: [PATCH v2 0/4] KVM: Improve IRQ assignment for device passthrough

[Michael S. Tsirkin]

>  - Refactored PCI-2.3 patch (but still no control knob for shared mode -
>    is that a must?)

I think it's the prudent thing to do.

-- 
MST

Re: [PATCH v2 0/4] KVM: Improve IRQ assignment for device passthrough

[Jan Kiszka]

Am 02.11.2010 18:11, Michael S. Tsirkin wrote:
>>  - Refactored PCI-2.3 patch (but still no control knob for shared mode -
>>    is that a must?)
> 
> I think it's the prudent thing to do.
> 

What a pity...

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux