KVM: No disable_irq for MSI/MSI-X interrupt on device assignment

PATCH Applied Sheng Yang 2 participants 6 messages Apr 30, 2009 – May 31, 2009 KVM

Share & tools

Sheng Yang

author

2009-04-30 02:58 UTC #0 lore raw

PATCH 1/1 KVM: No disable_irq for MSI/MSI-X interrupt on device assignment

Disable interrupt at interrupt handler and enable it when guest ack is for the level triggered interrupt, to prevent reinjected interrupt. MSI/MSI-X don't need it.

One possible problem is multiply same vector interrupt injected between irq handler and scheduled work handler would be merged as one for MSI/MSI-X. But AFAIK, the drivers handle it well.

The patch fixed the oplin card performance issue(MSI-X performance is half of MSI/INTx).

Signed-off-by Sheng Yang <[email protected]>

---
 virt/kvm/kvm_main.c |   22 ++++++++--------------
 1 files changed, 8 insertions(+), 14 deletions(-)

virt/kvm/kvm_main.c +8 ~~-14~~

index eebc5ed..de8f205 100644
@@ -120,7 +120,7 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev;
 	struct kvm *kvm;
-	int irq, i;
+	int i;
 
 	assigned_dev = container_of(work, struct kvm_assigned_dev_kernel,
 				    interrupt_work);

@@ -143,20 +143,10 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 			kvm_set_irq(assigned_dev->kvm,
 				    assigned_dev->irq_source_id,
 				    guest_entries[i].vector, 1);
-			irq = assigned_dev->host_msix_entries[i].vector;
-			if (irq != 0)
-				enable_irq(irq);
-			assigned_dev->host_irq_disabled = false;
 		}
-	} else {
+	} else
 		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
 			    assigned_dev->guest_irq, 1);
-		if (assigned_dev->irq_requested_type &
-				KVM_DEV_IRQ_GUEST_MSI) {
-			enable_irq(assigned_dev->host_irq);
-			assigned_dev->host_irq_disabled = false;
-		}
-	}
 
 	spin_unlock_irq(&assigned_dev->assigned_dev_lock);
 	mutex_unlock(&assigned_dev->kvm->lock);

@@ -179,8 +169,10 @@ static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 
 	schedule_work(&assigned_dev->interrupt_work);
 
-	disable_irq_nosync(irq);
-	assigned_dev->host_irq_disabled = true;
+	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) {
+		disable_irq_nosync(irq);
+		assigned_dev->host_irq_disabled = true;
+	}
 
 out:
 	spin_unlock_irqrestore(&assigned_dev->assigned_dev_lock, flags);

@@ -417,6 +409,7 @@ 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

@@ -427,6 +420,7 @@ 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

Signature · 1 line

1.5.4.5

Avi Kivity

2009-05-04 08:26 UTC #1 lore raw

Quoted 14 lines · Sheng Yang

Disable interrupt at interrupt handler and enable it when guest ack is for the level triggered interrupt, to prevent reinjected interrupt. MSI/MSI-X don't need it.
One possible problem is multiply same vector interrupt injected between irq handler and scheduled work handler would be merged as one for MSI/MSI-X. But AFAIK, the drivers handle it well.
The patch fixed the oplin card performance issue(MSI-X performance is half of MSI/INTx).
virt/kvm/kvm_main.c +0 -0
 	schedule_work(&assigned_dev->interrupt_work);

virt/kvm/kvm_main.c static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id) · line 178

Quoted 10 lines

virt/kvm/kvm_main.c +4 -2

 
-	disable_irq_nosync(irq);
-	assigned_dev->host_irq_disabled = true;
+	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) {
+		disable_irq_nosync(irq);
+		assigned_dev->host_irq_disabled = true;
+	}
 
 out:
 	spin_unlock_irqrestore(&assigned_dev->assigned_dev_lock, flags);

I don't have the spin_lock_irqrestore() here. The patch applies, but with fuzz. Am I missing a patch?

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Sheng Yang

author

2009-05-04 08:30 UTC #2 lore raw

Quoted 22 lines · On Monday 04 May 2009 16:25:55 Avi Kivity wrote:

Quoted 21 lines · Sheng Yang
Disable interrupt at interrupt handler and enable it when guest ack is for the level triggered interrupt, to prevent reinjected interrupt. MSI/MSI-X don't need it.
One possible problem is multiply same vector interrupt injected between irq handler and scheduled work handler would be merged as one for MSI/MSI-X. But AFAIK, the drivers handle it well.
The patch fixed the oplin card performance issue(MSI-X performance is half of MSI/INTx).
virt/kvm/kvm_main.c +4 -2
 	schedule_work(&assigned_dev->interrupt_work);
 
-	disable_irq_nosync(irq);
-	assigned_dev->host_irq_disabled = true;
+	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) {
+		disable_irq_nosync(irq);
+		assigned_dev->host_irq_disabled = true;
+	}

Quoted 5 lines

Quoted 2 lines
out:
spin_unlock_irqrestore(&assigned_dev->assigned_dev_lock, flags);
I don't have the spin_lock_irqrestore() here. The patch applies, but with fuzz. Am I missing a patch?

Oh, it's Marcelo's patchset...

[patch 0/4] use smp_send_reschedule in vcpu_kick / assigned dev host intx race fix

Signature · 2 lines

regards
Yang, Sheng

Avi Kivity

2009-05-04 08:34 UTC #3 lore raw

Quoted 32 lines · Sheng Yang

Quoted 23 lines · On Monday 04 May 2009 16:25:55 Avi Kivity wrote:
Quoted 21 lines · Sheng Yang
Disable interrupt at interrupt handler and enable it when guest ack is for the level triggered interrupt, to prevent reinjected interrupt. MSI/MSI-X don't need it.
One possible problem is multiply same vector interrupt injected between irq handler and scheduled work handler would be merged as one for MSI/MSI-X. But AFAIK, the drivers handle it well.
The patch fixed the oplin card performance issue(MSI-X performance is half of MSI/INTx).
virt/kvm/kvm_main.c +4 -2
 	schedule_work(&assigned_dev->interrupt_work);
 
-	disable_irq_nosync(irq);
-	assigned_dev->host_irq_disabled = true;
+	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) {
+		disable_irq_nosync(irq);
+		assigned_dev->host_irq_disabled = true;
+	}
Quoted 6 lines
Quoted 3 lines
out:
spin_unlock_irqrestore(&assigned_dev->assigned_dev_lock, flags);
I don't have the spin_lock_irqrestore() here. The patch applies, but with fuzz. Am I missing a patch?

Quoted 4 lines

Oh, it's Marcelo's patchset...
[patch 0/4] use smp_send_reschedule in vcpu_kick / assigned dev host intx race fix

Okay, I am working my queue in reverse order :)

But please note dependencies on other patches for me.

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Sheng Yang

author

2009-05-04 08:37 UTC #4 lore raw

Quoted 30 lines · On Monday 04 May 2009 16:34:00 Avi Kivity wrote:

Quoted 29 lines · Sheng Yang
Quoted 27 lines · On Monday 04 May 2009 16:25:55 Avi Kivity wrote:
Quoted 13 lines · Sheng Yang
Disable interrupt at interrupt handler and enable it when guest ack is for the level triggered interrupt, to prevent reinjected interrupt. MSI/MSI-X don't need it.
One possible problem is multiply same vector interrupt injected between irq handler and scheduled work handler would be merged as one for MSI/MSI-X. But AFAIK, the drivers handle it well.
The patch fixed the oplin card performance issue(MSI-X performance is half of MSI/INTx).
virt/kvm/kvm_main.c +0 -0
 	schedule_work(&assigned_dev->interrupt_work);
virt/kvm/kvm_main.c static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id) · line 178
Quoted 10 lines
virt/kvm/kvm_main.c +4 -2
 
-	disable_irq_nosync(irq);
-	assigned_dev->host_irq_disabled = true;
+	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) {
+		disable_irq_nosync(irq);
+		assigned_dev->host_irq_disabled = true;
+	}
 
 out:
 	spin_unlock_irqrestore(&assigned_dev->assigned_dev_lock, flags);
I don't have the spin_lock_irqrestore() here. The patch applies, but with fuzz. Am I missing a patch?

Quoted 8 lines

Quoted 4 lines
Oh, it's Marcelo's patchset...
[patch 0/4] use smp_send_reschedule in vcpu_kick / assigned dev host intx race fix
Okay, I am working my queue in reverse order :)
But please note dependencies on other patches for me.

Yes, sorry for totally forgot (mine based on his patchset)... :(

Signature · 2 lines

regards
Yang, Sheng

Avi Kivity

Applied

2009-05-31 09:48 UTC #5 lore raw

In reply to Sheng Yang

Quoted 11 lines · Sheng Yang

Disable interrupt at interrupt handler and enable it when guest ack is for the level triggered interrupt, to prevent reinjected interrupt. MSI/MSI-X don't need it.
One possible problem is multiply same vector interrupt injected between irq handler and scheduled work handler would be merged as one for MSI/MSI-X. But AFAIK, the drivers handle it well.
The patch fixed the oplin card performance issue(MSI-X performance is half of MSI/INTx).

Applied, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function