Re: [PATCH 2/2] x86, apicv: Add Posted Interrupt supporting

[Marcelo Tosatti <mtosatti@redhat.com>]

On Thu, Feb 07, 2013 at 04:01:11PM +0200, Gleb Natapov wrote:
> On Wed, Feb 06, 2013 at 08:49:23PM -0200, Marcelo Tosatti wrote:
> > > Second is that interrupt may be
> > > reported as delivered, but it will be coalesced (possible only with the self
> > > IPI with the same vector):
> > > 
> > > Starting condition: PIR=0, IRR=0 vcpu is in a guest mode
> > > 
> > > io thread                 |           vcpu
> > > accept_apic_interrupt()   |
> > >  PIR and IRR is zero      |
> > >  set PIR                  |
> > >  return delivered         |
> > >                           |      self IPI
> > >                           |      set IRR
> > >                           |     merge PIR to IRR (*)
> > > 
> > > At (*) interrupt that was reported as delivered is coalesced.
> > 
> > Only vcpu itself should send self-IPI, so its fine.
> > 
> It is fine only because this is not happening in practice (I hope) for single interrupt
> we care about. Otherwise this is serious problem.

Coalesced information is only interesting for non IPI cases, that
is, device emulation (at the moment, at least).

The above cause can happen when loading APIC registers, but delivered
is not interesting in that case. Good to document, however.

> > > > Or:
> > > > 
> > > > apic_accept_interrupt() {
> > > > 
> > > > 1. Read ORIG_PIR=PIR, ORIG_IRR=IRR.
> > > > Never set IRR when HWAPIC enabled, even if outside of guest mode.
> > > > 2. Set PIR and let HW or SW VM-entry transfer it to IRR.
> > > > 3. set_irq return value: (ORIG_PIR or ORIG_IRR set).
> > > > }
> > > > 
> > > This can report interrupt as coalesced, but it will be eventually delivered
> > > as separate interrupt:
> > > 
> > > Starting condition: PIR=0, IRR=1 vcpu is in a guest mode
> > > 
> > >  io thread              |         vcpu
> > >                         |         
> > > accept_apic_interrupt() |
> > > ORIG_PIR=0, ORIG_IRR=1  |
> > >                         |    EOI
> > >                         |    clear IRR, set ISR
> > > set PIR                 |
> > > return coalesced        |
> > >                         |    clear PIR, set IRR
> > >                         |    EOI
> > >                         |    clear IRR, set ISR (*)
> > > 
> > > At (*) interrupt that was reported as coalesced is delivered.
> > > 
> > > 
> > > So still no perfect solution. But first one has much less serious
> > > problems for our practical needs.
> > > 
> > > > Two or more concurrent set_irq can race with each other, though. Can
> > > > either document the race or add a lock.
> > > > 
> > > 
> > > --
> > > 			Gleb.
> > 
> > Ok, then:
> > 
> > accept_apic_irq:
> > 1. coalesced = test_and_set_bit(PIR)
> > 2. set KVM_REQ_EVENT bit 	(*)
> > 3. if (vcpu->in_guest_mode)
> > 4.	if (test_and_set_bit(pir notification bit))
> > 5.		send PIR IPI
> > 6. return coalesced
> Do not see how it will help.
> 
> Starting condition: PIR=0, IRR=1 vcpu is in a guest mode
> 
> io thread                  |              vcpu
> accept_apic_interrupt()    |
> coalesced = 0, PIR=1       |
> vcpu in a guest mode:      |
>     send PIR IPI           |
>                            |      receive PIR IPI
>                            |      merge PIR to IRR (*)
> return not coalesced       |
> 
> At (*) interrupt that was reported as delivered is coalesced.

Of course!

> The point is that we need to check PIR and IRR atomically and this is
> impossible.

Ok, next try:

1. orig_irr = read irr from vapic page
2. if (orig_irr == 0)
3. 	return test_and_test_bit(pir)
4. return 0

To summarize, given irr and pir (the bit for the vector in question) possibilities:

irr=0, pir=0, inject, return coalesced=0.
irr=0, pir=1, not injected, return coalesced=1.
irr=1, pir=0, not injected, return coalesced=1.
irr=1, pir=1, not injected, return coalesced=1.

Note the behaviour regarding whether to inject or not is the same as
today: it depends on IRR being set. If PIR=1 and IRR=0, IRR will be
eventually set.

> > Other sites:
> > A: On VM-entry, after disabling interrupts, but before
> > the last check for ->requests, clear pir notification bit 
> > (unconditionally).
> > 
> > (*) This is _necessary_ also because during VM-exit a PIR IPI interrupt can 
> > be missed, so the KVM_REQ_EVENT indicates that SW is responsible for
> > PIR->IRR transfer.
> > 
> 
> --
> 			Gleb.