Re: [PATCH v2] KVM: VMX: enable acknowledge interupt on vmexit

[Gleb Natapov <gleb@redhat.com>]

On Thu, Jan 24, 2013 at 12:47:14AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-01-23:
> > On Tue, Jan 22, 2013 at 01:49:31PM +0800, Yang Zhang wrote:
> >> From: Yang Zhang <yang.z.zhang@Intel.com>
> >> 
> >> The "acknowledge interrupt on exit" feature controls processor behavior
> >> for external interrupt acknowledgement. When this control is set, the
> >> processor acknowledges the interrupt controller to acquire the
> >> interrupt vector on VM exit.
> >> 
> >> After enabling this feature, an interrupt which arrived when target cpu
> >> is running in vmx non-root mode will be handled by vmx handler instead
> >> of handler in idt. Currently, vmx handler only fakes an interrupt stack
> >> and jump to idt table to let real handler to handle it. Further, we
> >> will recognize the interrupt and only delivery the interrupt which not
> >> belong to current vcpu through idt table. The interrupt which belonged
> >> to current vcpu will be handled inside vmx handler. This will reduce
> >> the interrupt handle cost of KVM.
> >> 
> >> Refer to Intel SDM volum 3, chapter 33.2.
> >> 
> >> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> >> ---
> >>  arch/x86/include/asm/kvm_host.h |    2 + arch/x86/kvm/svm.c           
> >>    |    6 ++++ arch/x86/kvm/vmx.c              |   52
> >>  ++++++++++++++++++++++++++++++++++++++- arch/x86/kvm/x86.c            
> >>   |    2 + 4 files changed, 61 insertions(+), 1 deletions(-)
> >> diff --git a/arch/x86/include/asm/kvm_host.h
> >> b/arch/x86/include/asm/kvm_host.h index c431b33..0b73602 100644 ---
> >> a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h
> >> @@ -345,6 +345,7 @@ struct kvm_vcpu_arch {
> >>  	unsigned long cr8;
> >>  	u32 hflags;
> >>  	u64 efer;
> >> +	struct desc_ptr host_idt;
> > Enough to save only host_idt.address.
> > 
> >>  	u64 apic_base; 	struct kvm_lapic *apic;    /* kernel irqchip context
> >>  */ 	unsigned long apic_attention; @@ -723,6 +724,7 @@ struct
> >>  kvm_x86_ops { 	int (*check_intercept)(struct kvm_vcpu *vcpu, 			      
> >>  struct x86_instruction_info *info, 			       enum x86_intercept_stage
> >>  stage); +	void (*handle_external_intr)(struct kvm_vcpu *vcpu); };
> >>  
> >>  struct kvm_arch_async_pf {
> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >> index d29d3cd..e286600 100644
> >> --- a/arch/x86/kvm/svm.c
> >> +++ b/arch/x86/kvm/svm.c
> >> @@ -4227,6 +4227,11 @@ out:
> >>  	return ret;
> >>  }
> >> +static void svm_handle_external_intr(struct kvm_vcpu *vcpu)
> >> +{
> >> +	return;
> >> +}
> >> +
> >>  static struct kvm_x86_ops svm_x86_ops = { 	.cpu_has_kvm_support =
> >>  has_svm, 	.disabled_by_bios = is_disabled, @@ -4318,6 +4323,7 @@
> >>  static struct kvm_x86_ops svm_x86_ops = { 	.set_tdp_cr3 = set_tdp_cr3,
> >>  
> >>  	.check_intercept = svm_check_intercept, +	.handle_external_intr =
> >>  svm_handle_external_intr, };
> >>  
> >>  static int __init svm_init(void)
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> index dd2a85c..ef98392 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -2565,7 +2565,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;
> >>  	if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_EXIT_CTLS,
> >>  				&_vmexit_control) < 0)
> >>  		return -EIO;
> >> @@ -3933,6 +3934,7 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
> >> 
> >>  	vmcs_writel(CR0_GUEST_HOST_MASK, ~0UL);
> >>  	set_cr4_guest_host_mask(vmx);
> >> +	native_store_idt(&vmx->vcpu.arch.host_idt);
> >> 
> > We already call native_store_idt() in vmx_set_constant_host_state(). No
> > need to do it twice. Add vcpu parameter to vmx_set_constant_host_state()
> > to get idt address from there.
> Sure.
> 
> >>  	return 0;
> >>  }
> >> @@ -6096,6 +6098,53 @@ static void vmx_complete_atomic_exit(struct
> > vcpu_vmx *vmx)
> >>  	}
> >>  }
> >> + +static void vmx_handle_external_intr(struct kvm_vcpu *vcpu) +{ +	u32
> >> exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO); +	if ((exit_intr_info
> >> & (INTR_INFO_VALID_MASK | INTR_INFO_INTR_TYPE_MASK)) +			==
> >> (INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR)) { +		unsigned int vector;
> >> +		unsigned long entry; +		gate_desc *desc; + +		vector = 
> >> exit_intr_info & INTR_INFO_VECTOR_MASK; +#ifdef CONFIG_X86_64 +		desc =
> >> (void *)vcpu->arch.host_idt.address + vector * 16; +#else +		desc =
> >> (void *)vcpu->arch.host_idt.address + vector * 8; +#endif + +		entry =
> >> gate_offset(*desc); +		asm( +			"mov %0, %%" _ASM_DX " \n\t" +#ifdef
> >> CONFIG_X86_64 +			"mov %%" _ASM_SP ", %%" _ASM_BX " \n\t" +			"and
> >> $0xfffffffffffffff0, %%" _ASM_SP " \n\t" +			"mov %%ss, %%" _ASM_AX "
> >> \n\t" +			"push %%" _ASM_AX " \n\t" +			"push %%" _ASM_BX " \n\t"
> >> +#endif +			"pushf \n\t" +			"mov %%cs, %%" _ASM_AX " \n\t" +			"push
> >> %%" _ASM_AX " \n\t" +			"push intr_return \n\t" +			"jmp *%% " _ASM_DX
> >> " \n\t" +			"1: \n\t" +			".pushsection .rodata \n\t" +			".global
> >> intr_return \n\t" +			"intr_return: " _ASM_PTR " 1b \n\t"
> >> +			".popsection \n\t" +			: :"m"(entry) : +#ifdef CONFIG_X86_64
> >> +			"rax", "rbx", "rdx" +#else +			"eax", "ebx", "edx"
> > ebx is not clobbered on 32bit as far as I see.
> Right. ebx is not touched.
> 
> >> +#endif
> >> +			);
> >> +	}
> >> +}
> >> +
> >>  static void vmx_recover_nmi_blocking(struct vcpu_vmx *vmx) { 	u32
> >>  exit_intr_info; @@ -7363,6 +7412,7 @@ static struct kvm_x86_ops
> >>  vmx_x86_ops = { 	.set_tdp_cr3 = vmx_set_cr3,
> >>  
> >>  	.check_intercept = vmx_check_intercept, +	.handle_external_intr =
> >>  vmx_handle_external_intr, };
> >>  
> >>  static int __init vmx_init(void)
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> index 1c9c834..000140b 100644
> >> --- a/arch/x86/kvm/x86.c
> >> +++ b/arch/x86/kvm/x86.c
> >> @@ -5728,6 +5728,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >>  							   native_read_tsc());
> >>  
> >>  	vcpu->mode = OUTSIDE_GUEST_MODE;
> >> +	kvm_x86_ops->handle_external_intr(vcpu);
> >> +
> > Move it after smp_wmb(). Also to be close to how real interrupt is
> > injected we can set IF in eflags copy that we push on a stack in
> > vmx_handle_external_intr() and get rid of local_irq_enable() here
> > (adding comment about it of course). svm_handle_external_intr() will
> > call local_irq_enable().
> Yes, we can set IF in this case. But for non external interrupt caused vmexit, we still need the local_irq_enable() to open interrupt.
> 
Of course. You can do it in vmx_handle_external_intr() if not external
interrupt detected.

> >>  	smp_wmb();
> >>  	local_irq_enable();
> >> --
> >> 1.7.1
> > 
> > 
> > --
> > 			Gleb.
> 
> 
> Best regards,
> Yang
> 

--
			Gleb.