KVM: do not call kvm_set_irq from irq disabled section

Hi Marcelo,

Thanks for the patch.
I put it into my kernel source tree and tested the freshly build kernel in
my testing environment. The problem is now - almost - gone. The only suspicious message (ONE occurrence immediate after starting the Server 2008 R2 VM) in syslog is now:

BUG: scheduling while atomic: qemu/3674/0x00000002
Modules linked in: tun bridge stp llc ext3 jbd uhci_hcd
 snd_hda_codec_realtek snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq
 snd_seq_device snd_pcm_oss snd_mixer_oss snd_hda_intel snd_hda_codec
 snd_hwdep snd_pcm snd_timer snd soundcore snd_page_alloc i915
 drm_kms_helper drm i2c_algo_bit video output i2c_i801 i2c_core
 ide_pci_generic ide_core button intel_agp ehci_hcd thermal e1000e iTCO_wdt
 iTCO_vendor_support processor usbcore kvm_intel evdev sg psmouse pcspkr
 serio_raw kvm rtc_cmos rtc_core rtc_lib ext4 mbcache jbd2 crc16 dm_mod
 sr_mod cdrom sd_mod ata_generic pata_acpi ahci libata scsi_mod Pid: 3674,
 comm: qemu Not tainted 2.6.33.2-KVM_patch #4
Call Trace:
 [<ffffffff8133d414>] ? schedule+0x544/0xa60
 [<ffffffffa01839ad>] ? mmu_zap_unsync_children+0x17d/0x200 [kvm]
 [<ffffffff8133e612>] ? __mutex_lock_slowpath+0x162/0x350
 [<ffffffff8133e809>] ? mutex_lock+0x9/0x20

 [<ffffffffa016cc87>] ? kvm_ioapic_set_irq+0x47/0x160 [kvm]
 [<ffffffffa016d914>] ? kvm_set_irq+0xf4/0x190 [kvm]
 [<ffffffffa016d9b0>] ? kvm_set_ioapic_irq+0x0/0x50 [kvm]
 [<ffffffffa016da00>] ? kvm_set_pic_irq+0x0/0x50 [kvm]
 [<ffffffffa018587f>] ? paging64_walk_addr+0x25f/0x750 [kvm]
 [<ffffffff8133e70d>] ? __mutex_lock_slowpath+0x25d/0x350
 [<ffffffffa016e905>] ? kvm_assigned_dev_ack_irq+0x35/0x90 [kvm]
 [<ffffffffa016d771>] ? kvm_notify_acked_irq+0x71/0x120 [kvm]

[<ffffffffa016ce13>] ? kvm_ioapic_update_eoi+0x73/0xd0 [kvm] [<ffffffffa0192689>] ? apic_reg_write+0x569/0x700 [kvm] [<ffffffffa0192939>] ? apic_mmio_write+0x69/0x70 [kvm] [<ffffffffa0178fec>] ? emulator_write_emulated_onepage+0xac/0x1b0 [kvm] [<ffffffffa018d9c5>] ? x86_emulate_insn+0x1d25/0x4d20 [kvm] [<ffffffffa018b98e>] ? x86_decode_insn+0x96e/0xba0 [kvm] [<ffffffffa018442c>] ? kvm_mmu_unprotect_page_virt+0xec/0x100 [kvm] [<ffffffffa0178c13>] ? emulate_instruction+0xd3/0x380 [kvm] [<ffffffff8133fe3e>] ? __down_read+0xce/0xd0 [<ffffffffa0191c69>] ? apic_update_ppr+0x29/0x70 [kvm] [<ffffffffa01fcc08>] ? handle_apic_access+0x18/0x40 [kvm_intel] [<ffffffffa017ba7d>] ? kvm_arch_vcpu_ioctl_run+0x3ad/0xcf0 [kvm] [<ffffffff81081af7>] ? wake_futex+0x37/0x70 [<ffffffffa0168d5c>] ? kvm_vcpu_ioctl+0x53c/0x910 [kvm] [<ffffffff81013ab3>] ? __switch_to_xtra+0x163/0x1a0 [<ffffffff810088b1>] ? __switch_to+0x271/0x340 [<ffffffff81121405>] ? vfs_ioctl+0x35/0xd0 [<ffffffff811215c8>] ? do_vfs_ioctl+0x88/0x570 [<ffffffff8133d1c9>] ? schedule+0x2f9/0xa60 [<ffffffff81121b30>] ? sys_ioctl+0x80/0xa0 [<ffffffff810cf4ea>] ? fire_user_return_notifiers+0x3a/0x70 [<ffffffff8100a042>] ? system_call_fastpath+0x16/0x1b
kvm: emulating exchange as write

Honestly my knowledge of the kvm internals is not sufficient to decide if
this bug report still belongs to my problem or is something different. So if you need more information or additional testing please let me know..

Best regards
Ralf Bonenkamp

-----Ursprüngliche Nachricht-----
Von: Marcelo Tosatti [mailto:[email protected]]
Gesendet: Dienstag, 20. April 2010 17:54
An: kvm
Cc: Ralf Bonenkamp; Chris Wright; Yang, Sheng
Betreff: *** GMX Spamverdacht *** [UNTESTED] KVM: do not call kvm_set_irq
from irq disabled section

The assigned device interrupt work handler calls kvm_set_irq, which can sleep, for example, waiting for the ioapic mutex, from irq disabled section.

https://bugzilla.kernel.org/show_bug.cgi?id=15725

Fix by dropping assigned_dev_lock (and re-enabling interrupts) before invoking kvm_set_irq for the KVM_DEV_IRQ_HOST_MSIX case. Other cases do not require the lock or interrupts disabled (a new work instance will be queued in case of concurrent interrupt).

KVM-Stable-Tag.

Signed-off-by Marcelo Tosatti <[email protected]>

The assigned device interrupt work handler calls kvm_set_irq, which can sleep, for example, waiting for the ioapic mutex, from irq disabled section.

https://bugzilla.kernel.org/show_bug.cgi?id=15725

Fix by dropping assigned_dev_lock (and re-enabling interrupts) before invoking kvm_set_irq for the KVM_DEV_IRQ_HOST_MSIX case. Other cases do not require the lock or interrupts disabled (a new work instance will be queued in case of concurrent interrupt).

KVM-Stable-Tag.

Signed-off-by Marcelo Tosatti <[email protected]>

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

index 47ca447..7ac7bbe 100644

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

+
+		spin_lock_irq(&assigned_dev->assigned_dev_lock);
 		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;
+			/*
+ 			 * If kvm_assigned_dev_intr sets pending for an
+ 			 * entry smaller than this work instance is
+ 			 * currently processing, a new work instance
+ 			 * will be queued.
+ 			 */

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

-	spin_unlock_irq(&assigned_dev->assigned_dev_lock);
 }
 
 static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)

Quoted 6 lines · On Tuesday 20 April 2010 23:54:01 Marcelo Tosatti wrote:

The assigned device interrupt work handler calls kvm_set_irq, which can sleep, for example, waiting for the ioapic mutex, from irq disabled section.

https://bugzilla.kernel.org/show_bug.cgi?id=15725

Quoted 4 lines

Fix by dropping assigned_dev_lock (and re-enabling interrupts) before invoking kvm_set_irq for the KVM_DEV_IRQ_HOST_MSIX case. Other cases do not require the lock or interrupts disabled (a new work instance will be queued in case of concurrent interrupt).

Looks fine, but depends on the new work would be queued sounds a little tricky...

How about a local_irq_disable() at the beginning? It can ensure no concurrent interrupts would happen as well I think.

Quoted 13 lines

KVM-Stable-Tag.

Signed-off-by Marcelo Tosatti <[email protected]>

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

index 47ca447..7ac7bbe 100644
@@ -64,24 +64,33 @@ static void
 kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 interrupt_work);
 	kvm = assigned_dev->kvm;
 

virt/kvm/assigned-dev.c static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) · line 69

Quoted 4 lines

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

-	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;

irq_requested_type and guest_msix_entries should also protected by the lock. So how about another spin_lock()/unlock() pair wraps the second kvm_set_irq()?

Quoted 18 lines

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

+
+		spin_lock_irq(&assigned_dev->assigned_dev_lock);
 		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;
+			/*
+ 			 * If kvm_assigned_dev_intr sets pending for an
+ 			 * entry smaller than this work instance is
+ 			 * currently processing, a new work instance
+ 			 * will be queued.
+ 			 */
+			spin_unlock_irq(&assigned_dev->assigned_dev_lock);
 			kvm_set_irq(assigned_dev->kvm,
 				    assigned_dev->irq_source_id,
 				    guest_entries[i].vector, 1);
+			spin_lock_irq(&assigned_dev->assigned_dev_lock);

virt/kvm/assigned-dev.c static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) · line 92

Quoted 5 lines

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

 		}
+		spin_unlock_irq(&assigned_dev->assigned_dev_lock);
 	} else
 		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
 			    assigned_dev->guest_irq, 1);

Or could we make kvm_set_irq() atomic? Though the code path is a little long for spinlock.

Quoted 2 lines

Or could we make kvm_set_irq() atomic? Though the code path is a little long for spinlock.

Yes, given the sleep-inside-RCU-protected section bug from kvm_notify_acked_irq, either that or convert IRQ locking to SRCU.

But as you said, the code paths are long and potentially slow, so probably SRCU is a better alternative.

Gleb?

Quoted 6 lines · Marcelo Tosatti

Quoted 2 lines

Or could we make kvm_set_irq() atomic? Though the code path is a little long for spinlock.

Yes, given the sleep-inside-RCU-protected section bug from kvm_notify_acked_irq, either that or convert IRQ locking to SRCU.

Quoted 4 lines

But as you said, the code paths are long and potentially slow, so probably SRCU is a better alternative.

Gleb?

kvm_set_irq() was converted to rcu from mutex to make msix interrupt injection scalable.

Quoted 7 lines · Gleb Natapov

Quoted 6 lines · Marcelo Tosatti

Quoted 2 lines

Or could we make kvm_set_irq() atomic? Though the code path is a little long for spinlock.

Yes, given the sleep-inside-RCU-protected section bug from kvm_notify_acked_irq, either that or convert IRQ locking to SRCU.

Quoted 6 lines

Quoted 4 lines

But as you said, the code paths are long and potentially slow, so probably SRCU is a better alternative.

Gleb?

kvm_set_irq() was converted to rcu from mutex to make msix interrupt injection scalable.

We meant ioapic lock. See the last report from Ralf on this thread.

Quoted 11 lines · Marcelo Tosatti

Quoted 10 lines · Gleb Natapov

Quoted 9 lines · Marcelo Tosatti

Quoted 2 lines

Or could we make kvm_set_irq() atomic? Though the code path is a little long for spinlock.

Yes, given the sleep-inside-RCU-protected section bug from kvm_notify_acked_irq, either that or convert IRQ locking to SRCU.

But as you said, the code paths are long and potentially slow, so probably SRCU is a better alternative.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Gleb?

kvm_set_irq() was converted to rcu from mutex to make msix interrupt injection scalable.

We meant ioapic lock. See the last report from Ralf on this thread.

Can we solve the problem by calling ack notifier outside rcu read section in kvm_notify_acked_irq()?

Quoted 12 lines · Gleb Natapov

Quoted 11 lines · Marcelo Tosatti

Quoted 10 lines · Gleb Natapov

Quoted 9 lines · Marcelo Tosatti

Quoted 2 lines

Or could we make kvm_set_irq() atomic? Though the code path is a little long for spinlock.

Yes, given the sleep-inside-RCU-protected section bug from kvm_notify_acked_irq, either that or convert IRQ locking to SRCU.

But as you said, the code paths are long and potentially slow, so probably SRCU is a better alternative.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Gleb?

kvm_set_irq() was converted to rcu from mutex to make msix interrupt injection scalable.

We meant ioapic lock. See the last report from Ralf on this thread.

Can we solve the problem by calling ack notifier outside rcu read section in kvm_notify_acked_irq()?

The unregister path does

- remove_from_list(entry)
- synchronize_rcu
- kfree(entry)

So if kvm_notify_acked_irq sleeps, synchronize_rcu can succeed, and the notifier entry can be freed.

Quoted 27 lines · Marcelo Tosatti

Quoted 12 lines · Gleb Natapov

Quoted 11 lines · Marcelo Tosatti

Quoted 10 lines · Gleb Natapov

Quoted 9 lines · Marcelo Tosatti

Quoted 2 lines

Or could we make kvm_set_irq() atomic? Though the code path is a little long for spinlock.

Yes, given the sleep-inside-RCU-protected section bug from kvm_notify_acked_irq, either that or convert IRQ locking to SRCU.

But as you said, the code paths are long and potentially slow, so probably SRCU is a better alternative.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Gleb?

kvm_set_irq() was converted to rcu from mutex to make msix interrupt injection scalable.

We meant ioapic lock. See the last report from Ralf on this thread.

Can we solve the problem by calling ack notifier outside rcu read section in kvm_notify_acked_irq()?

The unregister path does

- remove_from_list(entry)
- synchronize_rcu
- kfree(entry)

Quoted 2 lines

So if kvm_notify_acked_irq sleeps, synchronize_rcu can succeed, and the notifier entry can be freed.

What I mean is kvm_notify_acked_irq() will iterate over all ack entries in rcu read protected section, but instead of calling callback it will collect them into the array and call them outside rcu read section. At this point it doesn't matter if entry is unregistered since the copy is used to actually call the notifier.

Quoted 28 lines · Gleb Natapov

Quoted 27 lines · Marcelo Tosatti

Quoted 12 lines · Gleb Natapov

Quoted 11 lines · Marcelo Tosatti

Quoted 10 lines · Gleb Natapov

Quoted 9 lines · Marcelo Tosatti

Quoted 2 lines

Or could we make kvm_set_irq() atomic? Though the code path is a little long for spinlock.

Yes, given the sleep-inside-RCU-protected section bug from kvm_notify_acked_irq, either that or convert IRQ locking to SRCU.

But as you said, the code paths are long and potentially slow, so probably SRCU is a better alternative.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Gleb?

kvm_set_irq() was converted to rcu from mutex to make msix interrupt injection scalable.

We meant ioapic lock. See the last report from Ralf on this thread.

Can we solve the problem by calling ack notifier outside rcu read section in kvm_notify_acked_irq()?

The unregister path does

- remove_from_list(entry)
- synchronize_rcu
- kfree(entry)

Quoted 7 lines

Quoted 2 lines

So if kvm_notify_acked_irq sleeps, synchronize_rcu can succeed, and the notifier entry can be freed.

What I mean is kvm_notify_acked_irq() will iterate over all ack entries in rcu read protected section, but instead of calling callback it will collect them into the array and call them outside rcu read section. At this point it doesn't matter if entry is unregistered since the copy is used to actually call the notifier.

Here it is, but no, this trick can't be done safely for ack notifiers because they are unregistered at runtime by device assignment.

How does the freeing path knows it can proceed to free its entry if there is no reliable way to know if there is a reference to itself? (think "priv" gets freed between rcu_read_unlock and ->irq_acked with the patch below).

I can convert to SRCU if you have no objections.

Quoted 37 lines · Marcelo Tosatti

Quoted 28 lines · Gleb Natapov

Quoted 27 lines · Marcelo Tosatti

Quoted 12 lines · Gleb Natapov

Quoted 11 lines · Marcelo Tosatti

Quoted 10 lines · Gleb Natapov

Quoted 9 lines · Marcelo Tosatti

Quoted 2 lines

Or could we make kvm_set_irq() atomic? Though the code path is a little long for spinlock.

Yes, given the sleep-inside-RCU-protected section bug from kvm_notify_acked_irq, either that or convert IRQ locking to SRCU.

But as you said, the code paths are long and potentially slow, so probably SRCU is a better alternative.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Gleb?

kvm_set_irq() was converted to rcu from mutex to make msix interrupt injection scalable.

We meant ioapic lock. See the last report from Ralf on this thread.

Can we solve the problem by calling ack notifier outside rcu read section in kvm_notify_acked_irq()?

The unregister path does

- remove_from_list(entry)
- synchronize_rcu
- kfree(entry)

Quoted 7 lines

Quoted 2 lines

So if kvm_notify_acked_irq sleeps, synchronize_rcu can succeed, and the notifier entry can be freed.

What I mean is kvm_notify_acked_irq() will iterate over all ack entries in rcu read protected section, but instead of calling callback it will collect them into the array and call them outside rcu read section. At this point it doesn't matter if entry is unregistered since the copy is used to actually call the notifier.

Quoted 8 lines

Here it is, but no, this trick can't be done safely for ack notifiers because they are unregistered at runtime by device assignment.

How does the freeing path knows it can proceed to free its entry if there is no reliable way to know if there is a reference to itself? (think "priv" gets freed between rcu_read_unlock and ->irq_acked with the patch below).

Yeah, I see :(

Quoted 2 lines

I can convert to SRCU if you have no objections.

AFAIR there was a disadvantage comparing to RCU, but I don't remember what it was exactly.

What about converting PIC/IOAPIC mutexes into spinlocks?

Quoted 46 lines · Gleb Natapov

Quoted 37 lines · Marcelo Tosatti

Quoted 28 lines · Gleb Natapov

Quoted 27 lines · Marcelo Tosatti

Quoted 12 lines · Gleb Natapov

Quoted 11 lines · Marcelo Tosatti

Quoted 10 lines · Gleb Natapov

Quoted 9 lines · Marcelo Tosatti

Quoted 2 lines

Or could we make kvm_set_irq() atomic? Though the code path is a little long for spinlock.

Yes, given the sleep-inside-RCU-protected section bug from kvm_notify_acked_irq, either that or convert IRQ locking to SRCU.

But as you said, the code paths are long and potentially slow, so probably SRCU is a better alternative.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Gleb?

kvm_set_irq() was converted to rcu from mutex to make msix interrupt injection scalable.

We meant ioapic lock. See the last report from Ralf on this thread.

Can we solve the problem by calling ack notifier outside rcu read section in kvm_notify_acked_irq()?

The unregister path does

- remove_from_list(entry)
- synchronize_rcu
- kfree(entry)

Quoted 7 lines

Quoted 2 lines

So if kvm_notify_acked_irq sleeps, synchronize_rcu can succeed, and the notifier entry can be freed.

What I mean is kvm_notify_acked_irq() will iterate over all ack entries in rcu read protected section, but instead of calling callback it will collect them into the array and call them outside rcu read section. At this point it doesn't matter if entry is unregistered since the copy is used to actually call the notifier.

Quoted 8 lines

Here it is, but no, this trick can't be done safely for ack notifiers because they are unregistered at runtime by device assignment.

How does the freeing path knows it can proceed to free its entry if there is no reliable way to know if there is a reference to itself? (think "priv" gets freed between rcu_read_unlock and ->irq_acked with the patch below).

Quoted 6 lines

Yeah, I see :(

Quoted 2 lines

I can convert to SRCU if you have no objections.

AFAIR there was a disadvantage comparing to RCU, but I don't remember what it was exactly.

http://www.mentby.com/paul-e-mckenney/kvm-patch-v3-13-kvm-fix-race-in-irqrouting-logic.html

But for KVM IRQ path's it should not matter much since usage of grace periods is rare (registration/unregistration is very rare compared to read side), and the IRQ path's are already large and slow, so the added overhead should not be noticeable.

Quoted 1 line

What about converting PIC/IOAPIC mutexes into spinlocks?

Works for me, but on large guests the spinning will be noticeable. I believe.

Avi?

Quoted 7 lines

Quoted 4 lines

What about converting PIC/IOAPIC mutexes into spinlocks?

Works for me, but on large guests the spinning will be noticeable. I believe.

For interrupts going through IOPIC, but we know this is not scalable anyway.

Quoted 7 lines · Gleb Natapov

Quoted 7 lines

Quoted 4 lines

What about converting PIC/IOAPIC mutexes into spinlocks?

Works for me, but on large guests the spinning will be noticeable. I believe.

Quoted 3 lines

For interrupts going through IOPIC, but we know this is not scalable anyway.

Yes. We also wanted to convert the ioapic/pic to spinlocks so we could queue the interrupt from the PIT directly instead of using KVM_REQ_PENDING_TIMER which keeps confusing me. Chris Lalancette posted a patchset for this a while back but it was never completed.

Quoted 11 lines · Avi Kivity

Quoted 9 lines · Gleb Natapov

Quoted 9 lines

Quoted 5 lines

What about converting PIC/IOAPIC mutexes into spinlocks?

Works for me, but on large guests the spinning will be noticeable. I believe.

Quoted 9 lines

Quoted 4 lines

For interrupts going through IOPIC, but we know this is not scalable anyway.

Yes. We also wanted to convert the ioapic/pic to spinlocks so we could queue the interrupt from the PIT directly instead of using KVM_REQ_PENDING_TIMER which keeps confusing me. Chris Lalancette posted a patchset for this a while back but it was never completed.

Yeah, I'm sorry I never completed it. It turns out that with the HPET changes that went in around the time I was looking at it, that set of patches wasn't really required to fix the problem I was seeing with kdump.

That being said, if it's useful to somebody, I can repost the patches (though they are woefully out-of-date now). Let me know if you want to see them again.

The assigned device interrupt work handler calls kvm_set_irq, which can sleep, for example, waiting for the ioapic mutex, from irq disabled section.

https://bugzilla.kernel.org/show_bug.cgi?id=15725

Fix by dropping assigned_dev_lock (and re-enabling interrupts) before invoking kvm_set_irq for the KVM_DEV_IRQ_HOST_MSIX case. Other cases do not require the lock or interrupts disabled (a new work instance will be queued in case of concurrent interrupt).

KVM-Stable-Tag.

Signed-off-by Marcelo Tosatti<[email protected]>

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

index 47ca447..7ac7bbe 100644
@@ -64,24 +64,33 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
  				    interrupt_work);
  	kvm = assigned_dev->kvm;
 
-	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;
+
+		spin_lock_irq(&assigned_dev->assigned_dev_lock);
  		for (i = 0; i<  assigned_dev->entries_nr; i++) {
  			if (!(guest_entries[i].flags&
  					KVM_ASSIGNED_MSIX_PENDING))

Quoted 34 lines · Marcelo Tosatti

The assigned device interrupt work handler calls kvm_set_irq, which can sleep, for example, waiting for the ioapic mutex, from irq disabled section.

https://bugzilla.kernel.org/show_bug.cgi?id=15725

Fix by dropping assigned_dev_lock (and re-enabling interrupts) before invoking kvm_set_irq for the KVM_DEV_IRQ_HOST_MSIX case. Other cases do not require the lock or interrupts disabled (a new work instance will be queued in case of concurrent interrupt).

KVM-Stable-Tag.

Signed-off-by Marcelo Tosatti<[email protected]>

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

index 47ca447..7ac7bbe 100644
@@ -64,24 +64,33 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 				    interrupt_work);
 	kvm = assigned_dev->kvm;
 
-	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;
+
+		spin_lock_irq(&assigned_dev->assigned_dev_lock);
 		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;
+			/*

virt/kvm/assigned-dev.c static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) · line 83

Quoted 6 lines

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

+ 			 * If kvm_assigned_dev_intr sets pending for an
+ 			 * entry smaller than this work instance is
+ 			 * currently processing, a new work instance
+ 			 * will be queued.
+ 			 */
+			spin_unlock_irq(&assigned_dev->assigned_dev_lock);

What happens if assigned_dev->entries_nr is changed here?

Quoted 10 lines

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

 			kvm_set_irq(assigned_dev->kvm,
 				    assigned_dev->irq_source_id,
 				    guest_entries[i].vector, 1);
+			spin_lock_irq(&assigned_dev->assigned_dev_lock);
 		}
+		spin_unlock_irq(&assigned_dev->assigned_dev_lock);
 	} else
 		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
 			    assigned_dev->guest_irq, 1);
 

virt/kvm/assigned-dev.c static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) · line 94

Quoted 2 lines

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

-	spin_unlock_irq(&assigned_dev->assigned_dev_lock);
 }

A less dangerous fix is to copy all vectors to be triggered into a local array, drop the lock, and replay the array into kvm_set_irq(). The else branch could do this as well.

Quoted 20 lines

Quoted 20 lines

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

index 47ca447..7ac7bbe 100644
@@ -64,24 +64,33 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
  				    interrupt_work);
  	kvm = assigned_dev->kvm;
 
-	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;
+
+		spin_lock_irq(&assigned_dev->assigned_dev_lock);
  		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;
+			/*

Quoted 9 lines

virt/kvm/assigned-dev.c static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) · line 93

Quoted 7 lines

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

+ 			 * If kvm_assigned_dev_intr sets pending for an
+ 			 * entry smaller than this work instance is
+ 			 * currently processing, a new work instance
+ 			 * will be queued.
+ 			 */
+			spin_unlock_irq(&assigned_dev->assigned_dev_lock);
 

What happens if assigned_dev->entries_nr is changed here?

It does not. Interrupts and work instances are stopped before irq_requested_type or entries_nr can change.

Quoted 11 lines

Quoted 11 lines

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

  			kvm_set_irq(assigned_dev->kvm,
  				    assigned_dev->irq_source_id,
  				    guest_entries[i].vector, 1);
+			spin_lock_irq(&assigned_dev->assigned_dev_lock);
  		}
+		spin_unlock_irq(&assigned_dev->assigned_dev_lock);
  	} else
  		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
  			    assigned_dev->guest_irq, 1);
 

Quoted 7 lines

virt/kvm/assigned-dev.c static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) · line 95

Quoted 3 lines

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

-	spin_unlock_irq(&assigned_dev->assigned_dev_lock);
  }
 

A less dangerous fix is to copy all vectors to be triggered into a local array, drop the lock, and replay the array into kvm_set_irq(). The else branch could do this as well.

Its a bit large, 256 max vectors.