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

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

[Yang Zhang]

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;
 	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);
 
 	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"
+#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);
+
 	smp_wmb();
 	local_irq_enable();
 
-- 
1.7.1

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

[Gleb Natapov]

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.

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

> +#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().

>  	smp_wmb();
>  	local_irq_enable();
>  
> -- 
> 1.7.1


--
			Gleb.

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

[Zhang, Yang Z]

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.

>>  	smp_wmb();
>>  	local_irq_enable();
>> --
>> 1.7.1
> 
> 
> --
> 			Gleb.


Best regards,
Yang

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

[Gleb Natapov]

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.

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

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-01-24:
> 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.
Sure.

Best regards,
Yang