Re: [PATCH 1/2] x86: Enable ack interrupt on vmexit

[Gleb Natapov <gleb@redhat.com>]

On Thu, Dec 13, 2012 at 08:19:01AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2012-12-13:
> > On Thu, Dec 13, 2012 at 08:03:06AM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2012-12-13:
> >>> On Thu, Dec 13, 2012 at 07:54:35AM +0000, Zhang, Yang Z wrote:
> >>>> Gleb Natapov wrote on 2012-12-13:
> >>>>> On Thu, Dec 13, 2012 at 03:29:39PM +0800, Yang Zhang wrote:
> >>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>> 
> >>>>>> 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. Only
> >>>>>> enable it when posted interrupt is enabled.
> >>>>>> 
> >>>>>> 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 |   20 +++++++++++++++++---
> >>>>>>  1 files changed, 17 insertions(+), 3 deletions(-)
> >>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >>>>>> index 8cd9eb7..6b6bd03 100644
> >>>>>> --- a/arch/x86/kvm/vmx.c
> >>>>>> +++ b/arch/x86/kvm/vmx.c
> >>>>>> @@ -2549,7 +2549,7 @@ 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;
> >>>>>>  	if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_EXIT_CTLS,
> >>>>>>  				&_vmexit_control) < 0) 		return -EIO; @@ -3913,6 +3913,7 @@
> >>>>>>  static int vmx_vcpu_setup(struct vcpu_vmx *vmx) 	unsigned long a;
> >>>>>>  #endif 	int i;
> >>>>>> +	u32 vmexit_ctrl = vmcs_config.vmexit_ctrl;
> >>>>>> 
> >>>>>>  	/* I/O */ 	vmcs_write64(IO_BITMAP_A, __pa(vmx_io_bitmap_a)); @@
> >>>>>>  -3996,8 +3997,10 @@ static int vmx_vcpu_setup(struct vcpu_vmx
> >>>>>>  *vmx) 		vmx->guest_msrs[j].mask = -1ull; 		++vmx->nmsrs; 	}
> >>>>>> -
> >>>>>> -	vmcs_write32(VM_EXIT_CONTROLS, vmcs_config.vmexit_ctrl);
> >>>>>> +
> >>>>>> +	if(!enable_apicv_pi)
> >>>>>> +		vmexit_ctrl &= ~VM_EXIT_ACK_INTR_ON_EXIT;
> >>>>>> +	vmcs_write32(VM_EXIT_CONTROLS, vmexit_ctrl);
> >>>>>> 
> >>>>>>  	/* 22.2.1, 20.8.1 */
> >>>>>>  	vmcs_write32(VM_ENTRY_CONTROLS, vmcs_config.vmentry_ctrl);
> >>>>>> @@ -6267,6 +6270,17 @@ static void vmx_complete_atomic_exit(struct
> >>>>> vcpu_vmx *vmx)
> >>>>>>  		asm("int $2");
> >>>>>>  		kvm_after_handle_nmi(&vmx->vcpu);
> >>>>>>  	}
> >>>>>> +	if ((exit_intr_info & INTR_INFO_INTR_TYPE_MASK) ==
> >>>>>> INTR_TYPE_EXT_INTR && +	    (exit_intr_info & INTR_INFO_VALID_MASK)
> >>>>>> && enable_apicv_pi) { +		unsigned int vector, tmr; + + 	vector =
> >>>>>> exit_intr_info & INTR_INFO_VECTOR_MASK; +		tmr = apic_read(APIC_TMR
> >>>>>> + ((vector &
> >>> ~0x1f)
> >>>>>>>> 1)); +		apic_eoi(); +		if ( !((1 << (vector % 32)) & tmr) )
> >>>>>> +			apic->send_IPI_self(vector); +	}
> >>>>> What happen with the idea to dispatch interrupt through idt without IPI?
> >>>> I am not sure upstream guys will allow to export idt to a module. If it
> >>>> is not a problem, then can do it as you suggested.
> >>>> 
> >>> I replied to that before. No need to export idt to modules. Add function
> >>> to entry_32/64.S that does dispatching and export it instead.
> >> It still need to touch common code. Do you think upstream guys will
> >> buy-in this?
> >> 
> > What's the problem with touching common code? Show the code, get the
> > acks. But wait for merge window to close.
> You are right. We hope to push the PI patch ASAP. If touch common code, it may need long time to discuss to get final decision.
> As we discussion early, I will enable this feature in kvm not just when PI is enabled later. At that time, we can get some performance data and to see whether self ipi has big problem. Before the data ready, I think to limit all changes inside KVM modules should be a better way. How do you think?
> 
I think we have plenty of time till 3.9. We should do it right, not
quick.

--
			Gleb.