[PATCH] kvm: don't clear level from irqfd

[PATCH] kvm: don't clear level from irqfd

[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.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 virt/kvm/eventfd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index f59c1e8..4db6b01 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -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);
 }
 
 /*
-- 
MST

Re: [PATCH] kvm: don't clear level from irqfd

[Gleb Natapov]

On Wed, Jun 27, 2012 at 12:13:43PM +0300, Michael S. Tsirkin wrote:
> 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.

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  virt/kvm/eventfd.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index f59c1e8..4db6b01 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -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);
>  }
>  
>  /*
> -- 
> MST

--
			Gleb.

Re: [PATCH] kvm: don't clear level from irqfd

[Michael S. Tsirkin]

On Wed, Jun 27, 2012 at 01:03:46PM +0300, Gleb Natapov wrote:
> On Wed, Jun 27, 2012 at 12:13:43PM +0300, Michael S. Tsirkin wrote:
> > 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.

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.

> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  virt/kvm/eventfd.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> > index f59c1e8..4db6b01 100644
> > --- a/virt/kvm/eventfd.c
> > +++ b/virt/kvm/eventfd.c
> > @@ -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);
> >  }
> >  
> >  /*
> > -- 
> > MST
> 
> --
> 			Gleb.

Re: [PATCH] kvm: don't clear level from irqfd

[Gleb Natapov]

On Wed, Jun 27, 2012 at 01:20:24PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 27, 2012 at 01:03:46PM +0300, Gleb Natapov wrote:
> > On Wed, Jun 27, 2012 at 12:13:43PM +0300, Michael S. Tsirkin wrote:
> > > 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.
> 
> 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 :)

> 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.
> 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  virt/kvm/eventfd.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> > > index f59c1e8..4db6b01 100644
> > > --- a/virt/kvm/eventfd.c
> > > +++ b/virt/kvm/eventfd.c
> > > @@ -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);
> > >  }
> > >  
> > >  /*
> > > -- 
> > > MST
> > 
> > --
> > 			Gleb.

--
			Gleb.

Re: [PATCH] kvm: don't clear level from irqfd

[Michael S. Tsirkin]

On Wed, Jun 27, 2012 at 01:22:54PM +0300, Gleb Natapov wrote:
> On Wed, Jun 27, 2012 at 01:20:24PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 27, 2012 at 01:03:46PM +0300, Gleb Natapov wrote:
> > > On Wed, Jun 27, 2012 at 12:13:43PM +0300, Michael S. Tsirkin wrote:
> > > > 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.
> > 
> > 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:
> > > > -	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?


> > 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.
> > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  virt/kvm/eventfd.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > > 
> > > > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> > > > index f59c1e8..4db6b01 100644
> > > > --- a/virt/kvm/eventfd.c
> > > > +++ b/virt/kvm/eventfd.c
> > > > @@ -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);
> > > >  }
> > > >  
> > > >  /*
> > > > -- 
> > > > MST
> > > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.

Re: [PATCH] kvm: don't clear level from irqfd

[Gleb Natapov]

On Wed, Jun 27, 2012 at 02:38:43PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 27, 2012 at 01:22:54PM +0300, Gleb Natapov wrote:
> > On Wed, Jun 27, 2012 at 01:20:24PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 27, 2012 at 01:03:46PM +0300, Gleb Natapov wrote:
> > > > On Wed, Jun 27, 2012 at 12:13:43PM +0300, Michael S. Tsirkin wrote:
> > > > > 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.
> > > 
> > > 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.
> 
That's not how ioapic/pic edge interrupts work.

> 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:
> > > > > -	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?
> 
> 
> > > 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.
> > > 
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >  virt/kvm/eventfd.c | 1 -
> > > > >  1 file changed, 1 deletion(-)
> > > > > 
> > > > > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> > > > > index f59c1e8..4db6b01 100644
> > > > > --- a/virt/kvm/eventfd.c
> > > > > +++ b/virt/kvm/eventfd.c
> > > > > @@ -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);
> > > > >  }
> > > > >  
> > > > >  /*
> > > > > -- 
> > > > > MST
> > > > 
> > > > --
> > > > 			Gleb.
> > 
> > --
> > 			Gleb.

--
			Gleb.

Re: [PATCH] kvm: don't clear level from irqfd

[Alex Williamson]

On Wed, 2012-06-27 at 14:38 +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 27, 2012 at 01:22:54PM +0300, Gleb Natapov wrote:
> > On Wed, Jun 27, 2012 at 01:20:24PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 27, 2012 at 01:03:46PM +0300, Gleb Natapov wrote:
> > > > On Wed, Jun 27, 2012 at 12:13:43PM +0300, Michael S. Tsirkin wrote:
> > > > > 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.
> > > 
> > > 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:
> > > > > -	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

Re: [PATCH] kvm: don't clear level from irqfd

[Michael S. Tsirkin]

On Wed, Jun 27, 2012 at 08:41:57AM -0600, Alex Williamson wrote:
> On Wed, 2012-06-27 at 14:38 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 27, 2012 at 01:22:54PM +0300, Gleb Natapov wrote:
> > > On Wed, Jun 27, 2012 at 01:20:24PM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 27, 2012 at 01:03:46PM +0300, Gleb Natapov wrote:
> > > > > On Wed, Jun 27, 2012 at 12:13:43PM +0300, Michael S. Tsirkin wrote:
> > > > > > 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.
> > > > 
> > > > 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:
> > > > > > -	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.

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

>  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

Re: [PATCH] kvm: don't clear level from irqfd

[Gleb Natapov]

On Wed, Jun 27, 2012 at 06:00:57PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 27, 2012 at 08:41:57AM -0600, Alex Williamson wrote:
> > On Wed, 2012-06-27 at 14:38 +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 27, 2012 at 01:22:54PM +0300, Gleb Natapov wrote:
> > > > On Wed, Jun 27, 2012 at 01:20:24PM +0300, Michael S. Tsirkin wrote:
> > > > > On Wed, Jun 27, 2012 at 01:03:46PM +0300, Gleb Natapov wrote:
> > > > > > On Wed, Jun 27, 2012 at 12:13:43PM +0300, Michael S. Tsirkin wrote:
> > > > > > > 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.
> > > > > 
> > > > > 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:
> > > > > > > -	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.
> 
> 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.

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