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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Fri, Feb 08, 2013 at 02:28:44PM +0200, Gleb Natapov wrote:
> On Thu, Feb 07, 2013 at 07:49:47PM -0200, Marcelo Tosatti wrote:
> > 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).
> > 
> And incorrect result will be returned for an interrupt injected by an emulated device
> in the scenario above.
>
> > 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
> > 
> I think this is exactly same solution we are discussing above:
> 
> apic_accept_interrupt() {
>  if (PIR || IRR)
>    return coalesced;
>  else
>    set PIR;
> }

Its not exactly the same. Without test_and_set_bit(PIR) two vcpus can race.

> with the same self-IPI problem. IMO this is the best we can do and will
> work correctly for RTC interrupt re-injection case.

Yes.