Re: [PATCH v4 2/2] KVM: VMX: Add Posted Interrupt supporting

[Marcelo Tosatti <mtosatti@redhat.com>]

On Sun, Feb 24, 2013 at 04:19:17PM +0200, Gleb Natapov wrote:
> On Sun, Feb 24, 2013 at 01:55:07PM +0000, Zhang, Yang Z wrote:
> > > I do not think it fixes it. There is no guaranty that IPI will be
> > > processed by remote cpu while sending cpu is still in locked section, so
> > > the same race may happen regardless. As you say above there are 3
> > > contexts, but only two use locks.
> > See following logic, I think the problem you mentioned will not happened with current logic.
> > 
> > get lock
> > if test_pir (this will ensure there is no in flight IPI for same interrupt. Since we are taking the lock now, no IPI will be sent before we release the lock and no pir->irr is performed by hardware for same interrupt.)

Does the PIR IPI interrupt returns synchronously _after_ PIR->IRR transfer
is made? Don't think so.

PIR IPI interrupt returns after remote CPU has acked interrupt receival,
not after remote CPU has acked _and_ performed PIR->IRR transfer.

Right? (yes, right, see step 4. below).

Should try to make it easier to drop the lock later, so depend on it as
little as possible (also please document what it protects in detail).

Also note:

"
3. The processor clears the outstanding-notification bit in the
posted-interrupt descriptor. This is done atomically
so as to leave the remainder of the descriptor unmodified (e.g., with a
locked AND operation).
4. The processor writes zero to the EOI register in the local APIC; this
dismisses the interrupt with the postedinterrupt
notification vector from the local APIC.
5. The logical processor performs a logical-OR of PIR into VIRR and
clears PIR. No other agent can read or write a
PIR bit (or group of bits) between the time it is read (to determine
what to OR into VIRR) and when it is cleared.
"

So checking the ON bit does not mean the HW has finished performing
PIR->VIRR transfer (which means ON bit can only be used as an indication 
of whether to send interrupt, not whether PIR->VIRR transfer is
finished).

So its fine to do

-> atomic set pir
-> if (atomic_test_and_set(PIR ON bit))
->	send IPI

But HW can transfer two distinct bits, set by different serialized instances
of kvm_set_irq (protected with a lock), in a single PIR->IRR pass.

> I do not see where those assumptions are coming from. Testing pir does
> not guaranty that the IPI is not processed by VCPU right now.
> 
> iothread:                           vcpu:
> send_irq()
> lock(pir)
> check pir and irr
> set pir
> send IPI (*)
> unlock(pir)
> 
> send_irq()
> lock(pir)
>                                  receive IPI (*)
>                                  atomic {
>                                    pir_tmp = pir
>                                    pir = 0
>                                  }
> check pir and irr
>                                  irr &= pir_tmp
> set pir
> send IPI
> unlock(pir)
> 
> At this point both pir and irr are set and interrupt may be coalesced,
> but it is reported as delivered.

s/"set pir"/"injected = !t_a_s(pir)"/

> So what prevents the scenario above from happening?
> 
> --
> 			Gleb.