[PATCH] kvm: ioapic: Introduce arch-specific check for lazy update EOI mechanism

[PATCH] kvm: ioapic: Introduce arch-specific check for lazy update EOI mechanism

[Suravee Suthikulpanit]

commit f458d039db7e ("kvm: ioapic: Lazy update IOAPIC EOI") introduces
the following regression on Intel VMX APICv.

BUG: stack guard page was hit at 000000008f595917 \
(stack is 00000000bdefe5a4..00000000ae2b06f5)
kernel stack overflow (double-fault): 0000 [#1] SMP NOPTI
RIP: 0010:kvm_set_irq+0x51/0x160 [kvm]
Call Trace:
 irqfd_resampler_ack+0x32/0x90 [kvm]
 kvm_notify_acked_irq+0x62/0xd0 [kvm]
 kvm_ioapic_update_eoi_one.isra.0+0x30/0x120 [kvm]
 ioapic_set_irq+0x20e/0x240 [kvm]
 kvm_ioapic_set_irq+0x5c/0x80 [kvm]
 kvm_set_irq+0xbb/0x160 [kvm]
 ? kvm_hv_set_sint+0x20/0x20 [kvm]
 irqfd_resampler_ack+0x32/0x90 [kvm]
 kvm_notify_acked_irq+0x62/0xd0 [kvm]
 kvm_ioapic_update_eoi_one.isra.0+0x30/0x120 [kvm]
 ioapic_set_irq+0x20e/0x240 [kvm]
 kvm_ioapic_set_irq+0x5c/0x80 [kvm]
 kvm_set_irq+0xbb/0x160 [kvm]
 ? kvm_hv_set_sint+0x20/0x20 [kvm]
....

This is due to the logic always force IOAPIC lazy update EOI mechanism
when APICv is activated, which is only needed by AMD SVM AVIC.

Fixes by introducing struct kvm_arch.use_lazy_eoi variable to specify
whether the architecture needs lazy update EOI support.

Reported-by: borisvk@bstnet.org
Link: https://www.spinics.net/lists/kvm/msg213512.html
Fixes: f458d039db7e ("kvm: ioapic: Lazy update IOAPIC EOI")
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/include/asm/kvm_host.h | 2 ++
 arch/x86/kvm/ioapic.c           | 5 +++--
 arch/x86/kvm/svm/svm.c          | 8 ++++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 42a2d0d..eedfb15 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -982,6 +982,8 @@ struct kvm_arch {
 
 	struct kvm_pmu_event_filter *pmu_event_filter;
 	struct task_struct *nx_lpage_recovery_thread;
+
+	bool use_lazy_eoi;
 };
 
 struct kvm_vm_stat {
diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 750ff0b..94567c0 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -228,9 +228,10 @@ static int ioapic_set_irq(struct kvm_ioapic *ioapic, unsigned int irq,
 	 * AMD SVM AVIC accelerate EOI write and do not trap,
 	 * in-kernel IOAPIC will not be able to receive the EOI.
 	 * In this case, we do lazy update of the pending EOI when
-	 * trying to set IOAPIC irq.
+	 * trying to set IOAPIC irq if specified by the archtecture.
 	 */
-	if (kvm_apicv_activated(ioapic->kvm))
+	if (kvm_apicv_activated(ioapic->kvm) &&
+	    ioapic->kvm->arch.use_lazy_eoi)
 		ioapic_lazy_update_eoi(ioapic, irq);
 
 	/*
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 2f379ba..b560319 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3882,8 +3882,16 @@ static int svm_vm_init(struct kvm *kvm)
 {
 	if (avic) {
 		int ret = avic_vm_init(kvm);
+
 		if (ret)
 			return ret;
+		/*
+		 * AMD SVM AVIC accelerate EOI write and do not trap,
+		 * in-kernel IOAPIC will not be able to receive the EOI.
+		 * Therefore, specify lazy update of the pending EOI for IOAPIC.
+		 * (Please see in arch/x86/kvm/ioapic.c: ioapic_set_irq().)
+		 */
+		kvm->arch.use_lazy_eoi = true;
 	}
 
 	kvm_apicv_init(kvm, avic);
-- 
1.8.3.1

Re: [PATCH] kvm: ioapic: Introduce arch-specific check for lazy update EOI mechanism

[Paolo Bonzini]

On 24/04/20 07:08, Suravee Suthikulpanit wrote:
> commit f458d039db7e ("kvm: ioapic: Lazy update IOAPIC EOI") introduces
> the following regression on Intel VMX APICv.
> 
> BUG: stack guard page was hit at 000000008f595917 \
> (stack is 00000000bdefe5a4..00000000ae2b06f5)
> kernel stack overflow (double-fault): 0000 [#1] SMP NOPTI
> RIP: 0010:kvm_set_irq+0x51/0x160 [kvm]
> Call Trace:
>  irqfd_resampler_ack+0x32/0x90 [kvm]
>  kvm_notify_acked_irq+0x62/0xd0 [kvm]
>  kvm_ioapic_update_eoi_one.isra.0+0x30/0x120 [kvm]
>  ioapic_set_irq+0x20e/0x240 [kvm]
>  kvm_ioapic_set_irq+0x5c/0x80 [kvm]
>  kvm_set_irq+0xbb/0x160 [kvm]
>  ? kvm_hv_set_sint+0x20/0x20 [kvm]
>  irqfd_resampler_ack+0x32/0x90 [kvm]
>  kvm_notify_acked_irq+0x62/0xd0 [kvm]
>  kvm_ioapic_update_eoi_one.isra.0+0x30/0x120 [kvm]
>  ioapic_set_irq+0x20e/0x240 [kvm]
>  kvm_ioapic_set_irq+0x5c/0x80 [kvm]
>  kvm_set_irq+0xbb/0x160 [kvm]
>  ? kvm_hv_set_sint+0x20/0x20 [kvm]
> ....
> 
> This is due to the logic always force IOAPIC lazy update EOI mechanism
> when APICv is activated, which is only needed by AMD SVM AVIC.
> 
> Fixes by introducing struct kvm_arch.use_lazy_eoi variable to specify
> whether the architecture needs lazy update EOI support.

You are not explaining why the same infinite loop cannot happen on AMD.
 It seems to me that it is also fixed by adding a check for re-entrancy
in ioapic_lazy_update_eoi.  It's easy to add one since
ioapic_lazy_update_eoi is called with the ioapic->lock taken.

Paolo

Re: [PATCH] kvm: ioapic: Introduce arch-specific check for lazy update EOI mechanism

[Suravee Suthikulpanit]

Paolo,

On 4/25/20 4:52 PM, Paolo Bonzini wrote:
> On 24/04/20 07:08, Suravee Suthikulpanit wrote:
>> commit f458d039db7e ("kvm: ioapic: Lazy update IOAPIC EOI") introduces
>> the following regression on Intel VMX APICv.
>>
>> BUG: stack guard page was hit at 000000008f595917 \
>> (stack is 00000000bdefe5a4..00000000ae2b06f5)
>> kernel stack overflow (double-fault): 0000 [#1] SMP NOPTI
>> RIP: 0010:kvm_set_irq+0x51/0x160 [kvm]
>> Call Trace:
>>   irqfd_resampler_ack+0x32/0x90 [kvm]
>>   kvm_notify_acked_irq+0x62/0xd0 [kvm]
>>   kvm_ioapic_update_eoi_one.isra.0+0x30/0x120 [kvm]
>>   ioapic_set_irq+0x20e/0x240 [kvm]
>>   kvm_ioapic_set_irq+0x5c/0x80 [kvm]
>>   kvm_set_irq+0xbb/0x160 [kvm]
>>   ? kvm_hv_set_sint+0x20/0x20 [kvm]
>>   irqfd_resampler_ack+0x32/0x90 [kvm]
>>   kvm_notify_acked_irq+0x62/0xd0 [kvm]
>>   kvm_ioapic_update_eoi_one.isra.0+0x30/0x120 [kvm]
>>   ioapic_set_irq+0x20e/0x240 [kvm]
>>   kvm_ioapic_set_irq+0x5c/0x80 [kvm]
>>   kvm_set_irq+0xbb/0x160 [kvm]
>>   ? kvm_hv_set_sint+0x20/0x20 [kvm]
>> ....
>>
>> This is due to the logic always force IOAPIC lazy update EOI mechanism
>> when APICv is activated, which is only needed by AMD SVM AVIC.
>>
>> Fixes by introducing struct kvm_arch.use_lazy_eoi variable to specify
>> whether the architecture needs lazy update EOI support.
> 
> You are not explaining why the same infinite loop cannot happen on AMD.
>   It seems to me that it is also fixed by adding a check for re-entrancy
> in ioapic_lazy_update_eoi.  It's easy to add one since
> ioapic_lazy_update_eoi is called with the ioapic->lock taken.
> 
> Paolo
> 

I finally reproduced on AMD system as well. I'll send out a new patch for this based on your suggestion.

Suravee