Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Gleb Natapov <gleb@redhat.com>]

On Thu, Jul 19, 2012 at 02:12:13PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 19, 2012 at 01:54:53PM +0300, Gleb Natapov wrote:
> > On Thu, Jul 19, 2012 at 01:26:48PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jul 19, 2012 at 12:41:24PM +0300, Gleb Natapov wrote:
> > > > On Thu, Jul 19, 2012 at 12:33:29PM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Jul 19, 2012 at 12:21:07PM +0300, Gleb Natapov wrote:
> > > > > > On Thu, Jul 19, 2012 at 12:17:19PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Jul 19, 2012 at 10:53:37AM +0300, Gleb Natapov wrote:
> > > > > > > > On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> > > > > > > > > This creates a way to detect when kvm_set_irq(...,0) was run
> > > > > > > > > twice with the same source id by returning 0 in this case.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > > > ---
> > > > > > > > > 
> > > > > > > > > This is on top of my bugfix patch.  Uncompiled and untested.  Alex, I
> > > > > > > > > think something like this patch will make it possible for you to simply
> > > > > > > > > do
> > > > > > > > > 	if (kvm_set_irq(...., 0))
> > > > > > > > > 		eventfd_signal()
> > > > > > > > > 
> > > > > > > > Why caller can't track line state?
> > > > > > > 
> > > > > > > Why duplicate information? As we are finding it's not trivial to keep
> > > > > > > the two in sync. Think about migration etc ...
> > > > > > > 
> > > > > > We do not migrate irq_states. The caller already have to have enough
> > > > > > information to recreate its state and it should migrate the info, so why
> > > > > > should we go all the way down the call chain to find something that is
> > > > > > already known?
> > > > > 
> > > > > Hmm it's an interesting point. Looks like irqfds for level lose state
> > > > > across migration. Of course Alex wants to use them for assignment which
> > > > > currently disables migration, but we are talking about a generic API,
> > > > > so it's a problem that there's no way to retrieve the state.
> > > > > 
> > > > There is no any problem. Source knows what the line status is.
> > > 
> > > With EOIFD and level IRQFD, it does not.
> > > 
> > So this is again eventfd and level interrupts incompatibility problem?
> 
> At some level, yes.
> 
So may be we shouldn't do that especially since you claim migration will
not work.

> > > > Furthermore this is a (benign) bug if device calls irq_set with
> > > > the same level since it results in needless system calls. Qemu guilty
> > > > of it and _that_ should be fixed.
> > > 
> > > Fine but we are arguably returning a wrong result in that case:
> > > set_irq twice to 0 return 1 each time. I would expect 0 the
> > > second time.
> > It returns 0 if interrupt was coalesced. It was not.
> 
> Not really, if you call it with level 0 you always get 1 back.
> Look at kvm_ioapic_set_irq, see what happens if level is 0.
> It looks like a bug though a harmless one.
> 
May be. What kvm_set_irq() return in case of level=0 was never
important.

> > > 
> > > > > 
> > > > > Also migration is only one example. Duplicated state is generally
> > > > > nasty.  We would need extra locking too which is not nice.
> > > > > 
> > > > I don't know what extra locking you are talking about, but calling
> > > > kvm_set_irq() repeatedly with the same level will do a lot of unnecessary
> > > > locking in ioapic.
> > > 
> > > I am talking about Alex's EOIFD. This is what this patch is trying
> > > to help.
> > > 
> > Can you point me to exact problem in Alex's patch?
> 
> It's very simple. Alex adds an interface to clear the level
> automatically from guest on EOI.  So the caller has no way to know the
> current state for a given source ID and can not restore it after
> migration.
> 
Yes, but caller (read device emulation) knows what real state is. The
fact that EOI was called does not mean the line is at 0. Device should
reevaluate its state and re-trigger the line again if needed.

--
			Gleb.