[PATCH v2 1/2] KVM: nVMX: Fix virtual interrupt delivery injection

[PATCH v2 1/2] KVM: nVMX: Fix virtual interrupt delivery injection

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@linux.intel.com>

This patch fix bug reported in https://bugzilla.kernel.org/show_bug.cgi?id=73331, 
after the patch http://www.spinics.net/lists/kvm/msg105230.html applied, there is
some progress and the L2 can boot up, however, slowly. The original idea of this 
fix vid injection patch is from "Zhang, Yang Z" <yang.z.zhang@intel.com>.

Interrupt which delivered by vid should be injected to L1 by L0 if current is in 
L1, or should be injected to L2 by L0 through the old injection way if L1 doesn't 
have set External-interrupt exiting bit. The current logic doen't consider these 
cases. This patch fix it by vid intr to L1 if current is L1 or L2 through old 
injection way if L1 doen't have External-interrupt exiting bit set.

Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
Signed-off-by: "Zhang, Yang Z" <yang.z.zhang@intel.com>
---
v1 -> v2:
 * fix the typo in patch description

 arch/x86/kvm/vmx.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 021d84a..ad36646 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -7112,8 +7112,22 @@ static void vmx_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr)
 {
 	if (max_irr == -1)
 		return;
-
-	vmx_set_rvi(max_irr);
+	if (!is_guest_mode(vcpu)) {
+		vmx_set_rvi(max_irr);
+	} else if (is_guest_mode(vcpu) && !nested_exit_on_intr(vcpu)) {
+		/*
+		 * Fall back to old way to inject the interrupt since there
+		 * is no vAPIC-v for L2.
+		 */
+		if (vcpu->arch.exception.pending ||
+				vcpu->arch.nmi_injected ||
+				vcpu->arch.interrupt.pending)
+			return;
+		else if (vmx_interrupt_allowed(vcpu)) {
+			kvm_queue_interrupt(vcpu, max_irr, false);
+			vmx_inject_irq(vcpu);
+		}
+	}
 }
 
 static void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
-- 
1.9.1

[PATCH v2 2/2] KVM: nVMX: Fix fail to get nested ack intr's vector during nested vmexit

[Wanpeng Li]

WARNING: CPU: 9 PID: 7251 at arch/x86/kvm/vmx.c:8719 nested_vmx_vmexit+0xa4/0x233 [kvm_intel]()
Modules linked in: tun nfsv3 nfs_acl auth_rpcgss oid_registry nfsv4 dns_resolver nfs fscache lockd
sunrpc pci_stub netconsole kvm_intel kvm bridge stp llc autofs4 8021q ipv6 uinput joydev microcode
pcspkr igb i2c_algo_bit ehci_pci ehci_hcd e1000e ixgbe ptp pps_core hwmon mdio i2c_i801 i2c_core
tpm_tis tpm ipmi_si ipmi_msghandler isci libsas scsi_transport_sas button dm_mirror dm_region_hash
dm_log dm_mod
CPU: 9 PID: 7251 Comm: qemu-system-x86 Tainted: G        W     3.16.0-rc1 #2
Hardware name: Intel Corporation S2600CP/S2600CP, BIOS RMLSDP.86I.00.29.D696.1311111329 11/11/2013
 000000000000220f ffff880ffd107bf8 ffffffff81493563 000000000000220f
 0000000000000000 ffff880ffd107c38 ffffffff8103f0eb ffff880ffd107c48
 ffffffffa059709a ffff881ffc9e0040 ffff8800b74b8000 00000000ffffffff
Call Trace:
 [<ffffffff81493563>] dump_stack+0x49/0x5e
 [<ffffffff8103f0eb>] warn_slowpath_common+0x7c/0x96
 [<ffffffffa059709a>] ? nested_vmx_vmexit+0xa4/0x233 [kvm_intel]
 [<ffffffff8103f11a>] warn_slowpath_null+0x15/0x17
 [<ffffffffa059709a>] nested_vmx_vmexit+0xa4/0x233 [kvm_intel]
 [<ffffffffa0594295>] ? nested_vmx_exit_handled+0x6a/0x39e [kvm_intel]
 [<ffffffffa0537931>] ? kvm_apic_has_interrupt+0x80/0xd5 [kvm]
 [<ffffffffa05972ec>] vmx_check_nested_events+0xc3/0xd3 [kvm_intel]
 [<ffffffffa051ebe9>] inject_pending_event+0xd0/0x16e [kvm]
 [<ffffffffa051efa0>] vcpu_enter_guest+0x319/0x704 [kvm]

After commit 77b0f5d (KVM: nVMX: Ack and write vector info to intr_info if L1
asks us to), "Acknowledge interrupt on exit" behavior can be emulated. Current
logic will ask for intr vector if it is nested vmexit and VM_EXIT_ACK_INTR_ON_EXIT
is set by L1. However, intr vector for posted intr can't be got by generic read
pending interrupt vector and intack routine, there is a requirement to sync from
pir to irr. This patch fix it by ask the intr vector after sync pir to irr.

Reviewed-by: Yang Zhang <yang.z.zhang@intel.com>
Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
---
v1 -> v2:
 * replace kvm_get_apic_interrupt() by kvm_lapic_find_highest_irr()

 arch/x86/kvm/lapic.c | 1 +
 arch/x86/kvm/vmx.c   | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 0069118..b7d45dc 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1637,6 +1637,7 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 	apic_clear_irr(vector, apic);
 	return vector;
 }
+EXPORT_SYMBOL_GPL(kvm_get_apic_interrupt);
 
 void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu,
 		struct kvm_lapic_state *s)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4ae5ad8..a704f71 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8697,6 +8697,9 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 	if ((exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
 	    && nested_exit_intr_ack_set(vcpu)) {
 		int irq = kvm_cpu_get_interrupt(vcpu);
+
+		if (irq < 0 && kvm_apic_vid_enabled(vcpu->kvm))
+			irq = kvm_lapic_find_highest_irr(vcpu);
 		WARN_ON(irq < 0);
 		vmcs12->vm_exit_intr_info = irq |
 			INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR;
-- 
1.9.1

Re: [PATCH v2 2/2] KVM: nVMX: Fix fail to get nested ack intr's vector during nested vmexit

[Paolo Bonzini]

Il 17/07/2014 13:03, Wanpeng Li ha scritto:
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4ae5ad8..a704f71 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8697,6 +8697,9 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>  	if ((exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>  	    && nested_exit_intr_ack_set(vcpu)) {
>  		int irq = kvm_cpu_get_interrupt(vcpu);
> +
> +		if (irq < 0 && kvm_apic_vid_enabled(vcpu->kvm))
> +			irq = kvm_lapic_find_highest_irr(vcpu);
>  		WARN_ON(irq < 0);
>  		vmcs12->vm_exit_intr_info = irq |
>  			INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR;

I wonder if this should be kvm_apic_has_interrupt, so that the PPR
register is taken into consideration?

If so, the same change can also be written like this:

diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index bd0da43..a1ec6a5 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -108,7 +108,7 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
 
 	vector = kvm_cpu_get_extint(v);
 
-	if (kvm_apic_vid_enabled(v->kvm) || vector != -1)
+	if (vector != -1)
 		return vector;			/* PIC */
 
 	return kvm_get_apic_interrupt(v);	/* APIC */
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 3855103..92a0a58 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1627,10 +1627,13 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 	int vector = kvm_apic_has_interrupt(vcpu);
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	/* Note that we never get here with APIC virtualization enabled.  */
+	/*
+	 * With APIC virtualization enabled, just pass back the
+	 * vector, the processor will take care of delivery.
+	 */
 
-	if (vector == -1)
-		return -1;
+	if (vector == -1 || kvm_apic_vid_enabled(vcpu->kvm))
+		return vector;
 
 	apic_set_isr(vector, apic);
 	apic_update_ppr(apic);

The idea is that kvm_cpu_get_interrupt always return the interrupt.  If
you are injecting an interrupt you will test kvm_cpu_has_injectable_intr
outside the call to kvm_cpu_get_interrupt, and kvm_get_apic_interrupt
will never be reached anyway.  Instead, if you are reporting the interrupt,
any interrupt will be okay.

Yang, Wanpeng, what do you think?  Can you test both variants,
that is:

- you patch with kvm_apic_has_interrupt instead of
kvm_lapic_find_highest_irr

- the above untested patch of mine?

Thanks,

Paolo

Re: [PATCH v2 2/2] KVM: nVMX: Fix fail to get nested ack intr's vector during nested vmexit

[Paolo Bonzini]

Il 17/07/2014 13:28, Paolo Bonzini ha scritto:
> Il 17/07/2014 13:03, Wanpeng Li ha scritto:
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 4ae5ad8..a704f71 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -8697,6 +8697,9 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>>  	if ((exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>>  	    && nested_exit_intr_ack_set(vcpu)) {
>>  		int irq = kvm_cpu_get_interrupt(vcpu);
>> +
>> +		if (irq < 0 && kvm_apic_vid_enabled(vcpu->kvm))
>> +			irq = kvm_lapic_find_highest_irr(vcpu);
>>  		WARN_ON(irq < 0);
>>  		vmcs12->vm_exit_intr_info = irq |
>>  			INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR;
> 
> I wonder if this should be kvm_apic_has_interrupt, so that the PPR
> register is taken into consideration?


And actually, I think the acknowledging should include the three steps to
set-ISR/update-PPR/clear-IRR.  (With APICv update PPR is not strictly
necessary, but it doesn't hurt either).

You cannot let the processor do these because it would deliver the interrupt
through the IDT,  but you still must do it in the hypervisor.

This gives this patch:

diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index bd0da43..a1ec6a5 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -108,7 +108,7 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
 
 	vector = kvm_cpu_get_extint(v);
 
-	if (kvm_apic_vid_enabled(v->kvm) || vector != -1)
+	if (vector != -1)
 		return vector;			/* PIC */
 
 	return kvm_get_apic_interrupt(v);	/* APIC */
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 3855103..6cbc7af 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -360,10 +360,20 @@ static inline void apic_clear_irr(int vec, struct kvm_lapic *apic)
 
 static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
 {
-	/* Note that we never get here with APIC virtualization enabled.  */
+	if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR)) {
+		/*
+		 * With APIC virtualization enabled, all caching is disabled
+		 * because the processor can modify ISR under the hood.  Instead
+		 * just set SVI.
+		 */
+		if (kvm_apic_vid_enabled(vcpu->kvm)) {
+			kvm_x86_ops->hwapic_isr_update(vcpu->kvm, vec);
+			return;
+		}
 
-	if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
 		++apic->isr_count;
+	}
+
 	BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
 	/*
 	 * ISR (in service register) bit is set when injecting an interrupt.
@@ -1627,11 +1637,16 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 	int vector = kvm_apic_has_interrupt(vcpu);
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	/* Note that we never get here with APIC virtualization enabled.  */
-
 	if (vector == -1)
 		return -1;
 
+	/*
+	 * We get here even with APIC virtualization enabled, if doing
+	 * nested virtualization and L1 runs with the "acknowledge interrupt 
+	 * on exit" mode.  Then we cannot inject the interrupt via RVI,
+	 * because the process would deliver it through the IDT.
+	 */
+
 	apic_set_isr(vector, apic);
 	apic_update_ppr(apic);
 	apic_clear_irr(vector, apic);


I think the right way to do it must be something like this; you cannot
do it just in nested_vmx_vmexit.  Testing is welcome since I don't have
easy access to APICv-capable hardware (it would take a few days).

Paolo

Re: [PATCH v2 2/2] KVM: nVMX: Fix fail to get nested ack intr's vector during nested vmexit

[Wanpeng Li]

On Thu, Jul 17, 2014 at 02:04:11PM +0200, Paolo Bonzini wrote:
>Il 17/07/2014 13:28, Paolo Bonzini ha scritto:
>> Il 17/07/2014 13:03, Wanpeng Li ha scritto:
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index 4ae5ad8..a704f71 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -8697,6 +8697,9 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>>>  	if ((exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>>>  	    && nested_exit_intr_ack_set(vcpu)) {
>>>  		int irq = kvm_cpu_get_interrupt(vcpu);
>>> +
>>> +		if (irq < 0 && kvm_apic_vid_enabled(vcpu->kvm))
>>> +			irq = kvm_lapic_find_highest_irr(vcpu);
>>>  		WARN_ON(irq < 0);
>>>  		vmcs12->vm_exit_intr_info = irq |
>>>  			INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR;
>> 
>> I wonder if this should be kvm_apic_has_interrupt, so that the PPR
>> register is taken into consideration?
>
>
>And actually, I think the acknowledging should include the three steps to
>set-ISR/update-PPR/clear-IRR.  (With APICv update PPR is not strictly
>necessary, but it doesn't hurt either).
>
>You cannot let the processor do these because it would deliver the interrupt
>through the IDT,  but you still must do it in the hypervisor.
>
>This gives this patch:
>
>diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
>index bd0da43..a1ec6a5 100644
>--- a/arch/x86/kvm/irq.c
>+++ b/arch/x86/kvm/irq.c
>@@ -108,7 +108,7 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
> 
> 	vector = kvm_cpu_get_extint(v);
> 
>-	if (kvm_apic_vid_enabled(v->kvm) || vector != -1)
>+	if (vector != -1)
> 		return vector;			/* PIC */
> 
> 	return kvm_get_apic_interrupt(v);	/* APIC */
>diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>index 3855103..6cbc7af 100644
>--- a/arch/x86/kvm/lapic.c
>+++ b/arch/x86/kvm/lapic.c
>@@ -360,10 +360,20 @@ static inline void apic_clear_irr(int vec, struct kvm_lapic *apic)
> 
> static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> {
>-	/* Note that we never get here with APIC virtualization enabled.  */
>+	if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR)) {
>+		/*
>+		 * With APIC virtualization enabled, all caching is disabled
>+		 * because the processor can modify ISR under the hood.  Instead
>+		 * just set SVI.
>+		 */
>+		if (kvm_apic_vid_enabled(vcpu->kvm)) {
>+			kvm_x86_ops->hwapic_isr_update(vcpu->kvm, vec);
>+			return;
>+		}
> 
>-	if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
> 		++apic->isr_count;
>+	}
>+
> 	BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
> 	/*
> 	 * ISR (in service register) bit is set when injecting an interrupt.
>@@ -1627,11 +1637,16 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
> 	int vector = kvm_apic_has_interrupt(vcpu);
> 	struct kvm_lapic *apic = vcpu->arch.apic;
> 
>-	/* Note that we never get here with APIC virtualization enabled.  */
>-
> 	if (vector == -1)
> 		return -1;
> 
>+	/*
>+	 * We get here even with APIC virtualization enabled, if doing
>+	 * nested virtualization and L1 runs with the "acknowledge interrupt 
>+	 * on exit" mode.  Then we cannot inject the interrupt via RVI,
>+	 * because the process would deliver it through the IDT.
>+	 */
>+
> 	apic_set_isr(vector, apic);
> 	apic_update_ppr(apic);
> 	apic_clear_irr(vector, apic);
>
>
>I think the right way to do it must be something like this; you cannot
>do it just in nested_vmx_vmexit.  Testing is welcome since I don't have
>easy access to APICv-capable hardware (it would take a few days).

I will test it tomorrow, it's late today for me. ;-)

Regards,
Wanpeng Li 

>
>Paolo

Re: [PATCH v2 1/2] KVM: nVMX: Fix virtual interrupt delivery injection

[Paolo Bonzini]

Il 17/07/2014 13:03, Wanpeng Li ha scritto:
> +		/*
> +		 * Fall back to old way to inject the interrupt since there
> +		 * is no vAPIC-v for L2.
> +		 */
> +		if (vcpu->arch.exception.pending ||
> +				vcpu->arch.nmi_injected ||
> +				vcpu->arch.interrupt.pending)
> +			return;

This is just

	if (kvm_event_needs_reinjection(vcpu))
		return;

but apart from this the patch is okay.  I'll make the change and apply 
it, thanks.

Paolo

Re: [PATCH v2 1/2] KVM: nVMX: Fix virtual interrupt delivery injection

[Wanpeng Li]

On Thu, Jul 17, 2014 at 01:31:06PM +0200, Paolo Bonzini wrote:
>Il 17/07/2014 13:03, Wanpeng Li ha scritto:
>>+		/*
>>+		 * Fall back to old way to inject the interrupt since there
>>+		 * is no vAPIC-v for L2.
>>+		 */
>>+		if (vcpu->arch.exception.pending ||
>>+				vcpu->arch.nmi_injected ||
>>+				vcpu->arch.interrupt.pending)
>>+			return;
>
>This is just
>
>	if (kvm_event_needs_reinjection(vcpu))
>		return;
>
>but apart from this the patch is okay.  I'll make the change and
>apply it, thanks.
>

Thanks.

Regards,
Wanpeng Li 

>Paolo