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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 27 Jun 2012 14:38:43 +0300
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Gleb Natapov <gleb@...hat.com>
Cc:	Avi Kivity <avi@...hat.com>, Marcelo Tosatti <mtosatti@...hat.com>,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	Alex Williamson <alex.williamson@...hat.com>
Subject: Re: [PATCH] kvm: don't clear level from irqfd

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@...hat.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/