Re: [PATCH v2 5/6] x86: Enable ack interrupt on vmexit

[Gleb Natapov <gleb@redhat.com>]

On Fri, Nov 23, 2012 at 05:41:49AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2012-11-22:
> > On Wed, Nov 21, 2012 at 04:09:38PM +0800, Yang Zhang wrote:
> >> Ack interrupt on vmexit is required by Posted Interrupt. With it,
> >> when external interrupt caused vmexit, the cpu will acknowledge the
> >> interrupt controller and save the interrupt's vector in vmcs.
> >> 
> >> There are several approaches to enable it. This patch uses a simply
> >> way: re-generate an interrupt via self ipi.
> >> 
> >> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> >> ---
> >>  arch/x86/kvm/vmx.c |   11 ++++++++++-
> >>  1 files changed, 10 insertions(+), 1 deletions(-)
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> index 7949d21..f6ef090 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -2525,7 +2525,8 @@ static __init int setup_vmcs_config(struct
> > vmcs_config *vmcs_conf)
> >>  #ifdef CONFIG_X86_64
> >>  	min |= VM_EXIT_HOST_ADDR_SPACE_SIZE;
> >>  #endif
> >> -	opt = VM_EXIT_SAVE_IA32_PAT | VM_EXIT_LOAD_IA32_PAT;
> >> +	opt = VM_EXIT_SAVE_IA32_PAT | VM_EXIT_LOAD_IA32_PAT |
> >> +		VM_EXIT_ACK_INTR_ON_EXIT;
> > Always? Do it only if posted interrupts are actually available
> > and going to be used.
> 
> Right. 
> But the currently interrupt handler path is too long:
> vm exit -> KVM vmexit handler(interrupt disabled) -> KVM re-enable interrupt -> cpu ack the interrupt and interrupt deliver through the host IDT
> This will bring extra cost for interrupt belongs to guest. After enable "acknowledge interrupt on exit", we can inject the interrupt right way after vm exit if the interrupt is for the guest(This patch doesn't do this).
> 
> Since we only want to enable "acknowledge interrupt on exit" for Posted Interrupt, probably, we can enable it when PI is available.
> 
> >>  	if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_EXIT_CTLS,
> >>  				&_vmexit_control) < 0)
> >>  		return -EIO;
> >> @@ -4457,6 +4458,14 @@ static int handle_exception(struct kvm_vcpu *vcpu)
> >> 
> >>  static int handle_external_interrupt(struct kvm_vcpu *vcpu)
> >>  {
> >> +	unsigned int vector;
> >> +
> >> +	vector = vmcs_read32(VM_EXIT_INTR_INFO);
> >> +	vector &= INTR_INFO_VECTOR_MASK;
> > Valid bit is guarantied to be set here?
> > 
> >> +
> >> +	apic_eoi();
> > This is way to late. handle_external_interrupt() is called longs after
> > preemption and local irqs are enabled. vcpu process may be scheduled out
> > and apic_eoi() will not be called for a long time leaving interrupt
> > stuck in ISR and blocking other interrupts.
> 
> I will move it to vmx_complete_atomic_exit().
> 
> >> +	apic->send_IPI_self(vector);
> > For level interrupt this is not needed, no?
> 
> If we enable "ack interrupt on exit" only when apicv is available, then all interrupt is edge(interrupt remapping will setup all remap entries to deliver edge interrupt. interrupt remapping is required by x2apic, x2apic is required by PI)
Why x2apic is required by PI? Is this architectural? Cannot find it in SDM.

If interrupts will be delivered without self ipi like Avi suggests then
level will not be the issue.

> 
> /*
>          * Trigger mode in the IRTE will always be edge, and for IO-APIC,
> the
>          * actual level or edge trigger will be setup in the IO-APIC
>          * RTE. This will help simplify level triggered irq migration.
>          * For more details, see the comments (in io_apic.c) explainig
> IO-APIC
>          * irq migration in the presence of interrupt-remapping.
>         */
> 
> >> +
> >>  	++vcpu->stat.irq_exits;
> >>  	return 1;
> >>  }
> >> --
> >> 1.7.1
> > 
> > --
> > 			Gleb.
> 
> 
> Best regards,
> Yang
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.