Re: [PATCH v7 2/3] x86, apicv: add virtual interrupt delivery support

[Gleb Natapov <gleb@redhat.com>]

On Thu, Dec 20, 2012 at 10:53:16AM -0200, Marcelo Tosatti wrote:
> On Thu, Dec 20, 2012 at 08:42:06AM +0200, Gleb Natapov wrote:
> > On Wed, Dec 19, 2012 at 10:59:36PM -0200, Marcelo Tosatti wrote:
> > > On Mon, Dec 17, 2012 at 01:30:49PM +0800, Yang Zhang wrote:
> > > > From: Yang Zhang <yang.z.zhang@Intel.com>
> > > > 
> > > > Virtual interrupt delivery avoids KVM to inject vAPIC interrupts
> > > > manually, which is fully taken care of by the hardware. This needs
> > > > some special awareness into existing interrupr injection path:
> > > > 
> > > > - for pending interrupt, instead of direct injection, we may need
> > > >   update architecture specific indicators before resuming to guest.
> > > > 
> > > > - A pending interrupt, which is masked by ISR, should be also
> > > >   considered in above update action, since hardware will decide
> > > >   when to inject it at right time. Current has_interrupt and
> > > >   get_interrupt only returns a valid vector from injection p.o.v.
> > > > 
> > > > Signed-off-by: Kevin Tian <kevin.tian@intel.com>
> > > > Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> > > 
> > > 
> > > Resuming previous discussion:
> > > 
> > > > > How about to recaculate irr_pending according the VIRR on each
> > > > > vmexit?
> > > > > 
> > > > No need really. Since HW can only clear VIRR the only situation that
> > > > may
> > > > happen is that irr_pending will be true but VIRR is empty and
> > > > apic_find_highest_irr() will return correct result in this case.
> > > 
> > > Self-IPI does cause VIRR to be set, see "29.1.5 Self-IPI 
> > > Virtualization".
> > >
> > True. But as I said later in that discussion once irr_pending is set
> > to true it never becomes false, so the optimization is effectively
> > disable. We can set it to true doing apic initialization to make it
> > explicit.
> 
> Its just confusing, to have a variable which has different meanings
> in different configurations. I would rather have it explicit that 
> its not used rather than check every time the i read the code.
> 
> if (apic_vid() == 0 && !apic->irr_pending)
> 	return -1;
> 
I'd prefer to avoid this additional if() especially as its sole purpose
is documentation.  We can add comment instead. Note that irr_pending
is just a hint anyway.  It can be true when no interrupt is pending in
irr. We can even rename it to irr_pending_hint or something.

> > > Also, an example of problem with ISR caching optimization (which was written
> > > with ISR/IRR management entirely in software):
> > > 
> > > isr_count variable is never incremented (because HW sets ISR):
> > > kvm_cpu_get_interrupt does not call into kvm_get_apic_interrupt 
> > > if virtual interrupt delivery enabled.
> > > 
> > > Therefore apic_find_highest_isr can return -1 even though VISR
> > > is not zero.
> > > 
> > > In that case (VISR non-zero but apic_find_highest_isr() == -1), 
> > > apic_update_ppr does not correctly set
> > > 
> > > PPR = max(ISRV, TPR)
> > > 
> > > Which can result in kvm_arch_vcpu_runnable returning 1 when it should not.
> > > 
> > > Please disable usage of isr_count if virtual interrupt delivery enabled.
> > > Given that self-IPI writes to VIRR, also please disable usage of
> > > highest_isr_cache and irr_pending.
> > > 
> > Good catch about isr_count. We can set it to 1 during apic
> > initialization too, or skip call to apic_update_ppr() in
> > kvm_apic_has_interrupt() if vid is enabled.
> 
> Not sure if you can skip it, its probably necessary to calculate it
> before HW does so (say migration etc).
kvm_apic_has_interrupt() is not called during migration and
kvm_apic_post_state_restore() calls apic_update_ppr() explicitly. I am
not sure it is needed though since migrated value should be already
correct anyway.

--
			Gleb.