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

From: Gleb Natapov <gleb@redhat.com>
To: Marcelo Tosatti <mtosatti@redhat.com>
Cc: "Zhang, Yang Z" <yang.z.zhang@intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Shan, Haitao" <haitao.shan@intel.com>,
	"Zhang, Xiantao" <xiantao.zhang@intel.com>,
	"Nakajima, Jun" <jun.nakajima@intel.com>,
	"Anvin, H Peter" <h.peter.anvin@intel.com>
Subject: Re: [PATCH 2/2] x86, apicv: Add Posted Interrupt supporting
Date: Thu, 7 Feb 2013 16:01:11 +0200	[thread overview]
Message-ID: <20130207140111.GL7837@redhat.com> (raw)
In-Reply-To: <20130206224923.GA12266@amt.cnet>

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.

> > > 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.

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

> 
> 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.