kvm: don't clear level from irqfd

PATCH In review Michael S. Tsirkin 3 participants 9 messages Jun 27, 2012 KVM

lore

Share & tools

lore

Michael S. Tsirkin

author

2012-06-27 09:13 UTC #0 lore raw

PATCH 1/1 kvm: don't clear level from irqfd

irqfd sets and clears the level immediately.
This used to be necessary for MSI interrupts but isn't anymore.

Note! We never officially supported irqfd for level interrupts but it usually seemed to kind of work, and this change will break it. There are no known real users of that feature though - only old vfio prototypes. Removing this line will make it easier to add
real support for level irqfds in the future.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 virt/kvm/eventfd.c | 1 -
 1 file changed, 1 deletion(-)

virt/kvm/eventfd.c +0 -1

index f59c1e8..4db6b01 100644
@@ -68,7 +68,6 @@ irqfd_inject(struct work_struct *work)
 	struct kvm *kvm = irqfd->kvm;
 
 	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 }
 
 /*

Signature · 1 line

MST

Gleb Natapov

2012-06-27 10:03 UTC #1 lore raw

Quoted 10 lines · Michael S. Tsirkin

irqfd sets and clears the level immediately.
This used to be necessary for MSI interrupts but isn't anymore.
Note! We never officially supported irqfd for level interrupts but it usually seemed to kind of work, and this change will break it. There are no known real users of that feature though - only old vfio prototypes. Removing this line will make it easier to add
real support for level irqfds in the future.

Did we officially supported irqfd for ioapic/pic edge interrupts? Because those will break to.

Quoted 19 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 virt/kvm/eventfd.c | 1 -
 1 file changed, 1 deletion(-)

virt/kvm/eventfd.c +0 -1

index f59c1e8..4db6b01 100644
@@ -68,7 +68,6 @@ irqfd_inject(struct work_struct *work)
 	struct kvm *kvm = irqfd->kvm;
 
 	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 }
 
 /*

Signature · 1 line

MST

--
Gleb.

Michael S. Tsirkin

author

2012-06-27 10:20 UTC #2 lore raw

Quoted 11 lines · Gleb Natapov

Quoted 10 lines · Michael S. Tsirkin
irqfd sets and clears the level immediately.
This used to be necessary for MSI interrupts but isn't anymore.
Note! We never officially supported irqfd for level interrupts but it usually seemed to kind of work, and this change will break it. There are no known real users of that feature though - only old vfio prototypes. Removing this line will make it easier to add
real support for level irqfds in the future.

Quoted 2 lines

Did we officially supported irqfd for ioapic/pic edge interrupts? Because those will break to.

Hard to say - as Alex points out it was never documented. But at least they actually worked reliably. Hmm. One way to address this is to add kvm_trigger_irq which will trigger edge interrupts unconditionally.

Not sure it's worth it though.

Quoted 22 lines

Quoted 19 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 virt/kvm/eventfd.c | 1 -
 1 file changed, 1 deletion(-)

virt/kvm/eventfd.c +0 -1

index f59c1e8..4db6b01 100644
@@ -68,7 +68,6 @@ irqfd_inject(struct work_struct *work)
 	struct kvm *kvm = irqfd->kvm;
 
 	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 }
 
 /*

Signature · 1 line

MST

--
Gleb.

Gleb Natapov

2012-06-27 10:22 UTC #3 lore raw

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Gleb Natapov
Quoted 10 lines · Michael S. Tsirkin
irqfd sets and clears the level immediately.
This used to be necessary for MSI interrupts but isn't anymore.
Note! We never officially supported irqfd for level interrupts but it usually seemed to kind of work, and this change will break it. There are no known real users of that feature though - only old vfio prototypes. Removing this line will make it easier to add
real support for level irqfds in the future.

Quoted 4 lines

Quoted 2 lines
Did we officially supported irqfd for ioapic/pic edge interrupts? Because those will break to.
Hard to say - as Alex points out it was never documented.

If we will start dropping everything that was never documented we will hardly have working VMM at the end :)

Quoted 4 lines

But at least they actually worked reliably. Hmm. One way to address this is to add kvm_trigger_irq which will trigger edge interrupts unconditionally.

Quoted 24 lines

Not sure it's worth it though.

Quoted 22 lines

Quoted 19 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 virt/kvm/eventfd.c | 1 -
 1 file changed, 1 deletion(-)

virt/kvm/eventfd.c +0 -1

index f59c1e8..4db6b01 100644
@@ -68,7 +68,6 @@ irqfd_inject(struct work_struct *work)
 	struct kvm *kvm = irqfd->kvm;
 
 	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 }
 
 /*

Signature · 1 line

MST

--
Gleb.

Michael S. Tsirkin

author

2012-06-27 11:38 UTC #4 lore raw

Quoted 13 lines · Gleb Natapov

Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Gleb Natapov
Quoted 10 lines · Michael S. Tsirkin
irqfd sets and clears the level immediately.
This used to be necessary for MSI interrupts but isn't anymore.
Note! We never officially supported irqfd for level interrupts but it usually seemed to kind of work, and this change will break it. There are no known real users of that feature though - only old vfio prototypes. Removing this line will make it easier to add
real support for level irqfds in the future.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Did we officially supported irqfd for ioapic/pic edge interrupts? Because those will break to.
Hard to say - as Alex points out it was never documented.
If we will start dropping everything that was never documented we will hardly have working VMM at the end :)

Generally true.

However I just realized that this interface does not work reliably for ioapic/pic edge either. More or less for the same reason that it does not work for edge: we set bit in IRR, try to inject, then clear even if injection failed (e.g. because it is low priority). Guest will never get an interrupt in this case.

So what happened e.g. for Alex's vfio prototype is he never hit an interrupt window in light testing.

What we would need to do to fix it is:

Quoted 4 lines

Review

Quoted 4 lines

(quoted diff) +2 -2

-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);

Which will fix edge but break level same as this patch. But is it worth fixing? Maybe declare it broken and be done with it?

Quoted 31 lines

Quoted 4 lines
But at least they actually worked reliably. Hmm. One way to address this is to add kvm_trigger_irq which will trigger edge interrupts unconditionally.
Quoted 24 lines
Not sure it's worth it though.
Quoted 22 lines
Quoted 19 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 virt/kvm/eventfd.c | 1 -
 1 file changed, 1 deletion(-)
virt/kvm/eventfd.c +0 -1
index f59c1e8..4db6b01 100644
@@ -68,7 +68,6 @@ irqfd_inject(struct work_struct *work)
 	struct kvm *kvm = irqfd->kvm;
 
 	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 }
 
 /*
Signature · 1 line
MST
--
Gleb.
--
Gleb.

Gleb Natapov

2012-06-27 11:50 UTC #5 lore raw

Quoted 21 lines · Michael S. Tsirkin

Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Gleb Natapov
Quoted 10 lines · Michael S. Tsirkin
irqfd sets and clears the level immediately.
This used to be necessary for MSI interrupts but isn't anymore.
Note! We never officially supported irqfd for level interrupts but it usually seemed to kind of work, and this change will break it. There are no known real users of that feature though - only old vfio prototypes. Removing this line will make it easier to add
real support for level irqfds in the future.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Did we officially supported irqfd for ioapic/pic edge interrupts? Because those will break to.
Hard to say - as Alex points out it was never documented.
If we will start dropping everything that was never documented we will hardly have working VMM at the end :)

Quoted 8 lines

Generally true.
However I just realized that this interface does not work reliably for ioapic/pic edge either. More or less for the same reason that it does not work for edge: we set bit in IRR, try to inject, then clear even if injection failed (e.g. because it is low priority). Guest will never get an interrupt in this case.

That's not how ioapic/pic edge interrupts work.

Quoted 4 lines

So what happened e.g. for Alex's vfio prototype is he never hit an interrupt window in light testing.
What we would need to do to fix it is:

Quoted 41 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 4 lines
(quoted diff) +2 -2
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
Which will fix edge but break level same as this patch. But is it worth fixing? Maybe declare it broken and be done with it?
Quoted 31 lines
Quoted 4 lines
But at least they actually worked reliably. Hmm. One way to address this is to add kvm_trigger_irq which will trigger edge interrupts unconditionally.
Quoted 24 lines
Not sure it's worth it though.
Quoted 22 lines
Quoted 19 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 virt/kvm/eventfd.c | 1 -
 1 file changed, 1 deletion(-)
virt/kvm/eventfd.c +0 -1
index f59c1e8..4db6b01 100644
@@ -68,7 +68,6 @@ irqfd_inject(struct work_struct *work)
 	struct kvm *kvm = irqfd->kvm;
 
 	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 }
 
 /*
Signature · 1 line
MST
--
Gleb.
--
Gleb.

--
Gleb.

Alex Williamson

2012-06-27 14:41 UTC #6 lore raw

In reply to Michael S. Tsirkin

Quoted 33 lines · Michael S. Tsirkin

Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Gleb Natapov
Quoted 10 lines · Michael S. Tsirkin
irqfd sets and clears the level immediately.
This used to be necessary for MSI interrupts but isn't anymore.
Note! We never officially supported irqfd for level interrupts but it usually seemed to kind of work, and this change will break it. There are no known real users of that feature though - only old vfio prototypes. Removing this line will make it easier to add
real support for level irqfds in the future.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Did we officially supported irqfd for ioapic/pic edge interrupts? Because those will break to.
Hard to say - as Alex points out it was never documented.
If we will start dropping everything that was never documented we will hardly have working VMM at the end :)
Generally true.
However I just realized that this interface does not work reliably for ioapic/pic edge either. More or less for the same reason that it does not work for edge: we set bit in IRR, try to inject, then clear even if injection failed (e.g. because it is low priority). Guest will never get an interrupt in this case.
So what happened e.g. for Alex's vfio prototype is he never hit an interrupt window in light testing.
What we would need to do to fix it is:

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 4 lines
(quoted diff) +2 -2
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
Which will fix edge but break level same as this patch. But is it worth fixing? Maybe declare it broken and be done with it?

Ugh, so if I understand correctly, the case where kvm_set_irq() returns <0, we're broken. VFIO has masked the physical device and has no error path to know that the interrupt inject was unsuccessful. We almost need an error eventfd that bounces the interrupt back to userspace for the qemu interrupt model to handle, but that has it's own set of complications. Suggestions? Declaring the interface broken isn't very useful. Thanks,

Alex

Michael S. Tsirkin

author

2012-06-27 15:00 UTC #7 lore raw

Quoted 43 lines · Alex Williamson

Quoted 33 lines · Michael S. Tsirkin
Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Gleb Natapov
Quoted 10 lines · Michael S. Tsirkin
irqfd sets and clears the level immediately.
This used to be necessary for MSI interrupts but isn't anymore.
Note! We never officially supported irqfd for level interrupts but it usually seemed to kind of work, and this change will break it. There are no known real users of that feature though - only old vfio prototypes. Removing this line will make it easier to add
real support for level irqfds in the future.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Did we officially supported irqfd for ioapic/pic edge interrupts? Because those will break to.
Hard to say - as Alex points out it was never documented.
If we will start dropping everything that was never documented we will hardly have working VMM at the end :)
Generally true.
However I just realized that this interface does not work reliably for ioapic/pic edge either. More or less for the same reason that it does not work for edge: we set bit in IRR, try to inject, then clear even if injection failed (e.g. because it is low priority). Guest will never get an interrupt in this case.
So what happened e.g. for Alex's vfio prototype is he never hit an interrupt window in light testing.
What we would need to do to fix it is:
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 4 lines
(quoted diff) +2 -2
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
Which will fix edge but break level same as this patch. But is it worth fixing? Maybe declare it broken and be done with it?

Quoted 2 lines

Ugh, so if I understand correctly, the case where kvm_set_irq() returns <0, we're broken.

Gleb says it's not broken. Maybe I misread the code. Need more investigation.

Quoted 8 lines

VFIO has masked the physical device and has no error
path to know that the interrupt inject was unsuccessful. We almost need an error eventfd that bounces the interrupt back to userspace for the qemu interrupt model to handle, but that has it's own set of complications. Suggestions? Declaring the interface broken isn't very useful. Thanks,
Alex

Gleb Natapov

2012-06-27 15:03 UTC #8 lore raw

Quoted 44 lines · Michael S. Tsirkin

Quoted 43 lines · Alex Williamson
Quoted 41 lines · Michael S. Tsirkin
Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Gleb Natapov
Quoted 10 lines · Michael S. Tsirkin
irqfd sets and clears the level immediately.
This used to be necessary for MSI interrupts but isn't anymore.
Note! We never officially supported irqfd for level interrupts but it usually seemed to kind of work, and this change will break it. There are no known real users of that feature though - only old vfio prototypes. Removing this line will make it easier to add
real support for level irqfds in the future.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Did we officially supported irqfd for ioapic/pic edge interrupts? Because those will break to.
Hard to say - as Alex points out it was never documented.
If we will start dropping everything that was never documented we will hardly have working VMM at the end :)
Generally true.
However I just realized that this interface does not work reliably for ioapic/pic edge either. More or less for the same reason that it does not work for edge: we set bit in IRR, try to inject, then clear even if injection failed (e.g. because it is low priority). Guest will never get an interrupt in this case.
So what happened e.g. for Alex's vfio prototype is he never hit an interrupt window in light testing.
What we would need to do to fix it is:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 4 lines
(quoted diff) +2 -2
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
Which will fix edge but break level same as this patch. But is it worth fixing? Maybe declare it broken and be done with it?

Quoted 6 lines

Quoted 2 lines
Ugh, so if I understand correctly, the case where kvm_set_irq() returns <0, we're broken.
Gleb says it's not broken. Maybe I misread the code. Need more investigation.

IRR in ioapic/pic is used for edge detection logic for edge interrupts. It have to be cleared for next set_irq(1) to be detected.

Quoted 8 lines

Quoted 8 lines
VFIO has masked the physical device and has no error
path to know that the interrupt inject was unsuccessful. We almost need an error eventfd that bounces the interrupt back to userspace for the qemu interrupt model to handle, but that has it's own set of complications. Suggestions? Declaring the interface broken isn't very useful. Thanks,
Alex

--
Gleb.