[PATCH 0/4] KVM: SVM: Fix AVIC warning when enable irq window

[PATCH 0/4] KVM: SVM: Fix AVIC warning when enable irq window

[Suravee Suthikulpanit]

Introduce kvm_make_all_cpus_request_except(), which is used
in the subsequent patch 2 to fix AVIC warning.

Also include miscelleneous clean ups.

Thanks,
Suravee

Suravee Suthikulpanit (4):
  KVM: Introduce kvm_make_all_cpus_request_except()
  KVM: SVM: Fixes setting V_IRQ while AVIC is still enabled
  KVM: SVM: Merge svm_enable_vintr into svm_set_vintr
  KVM: SVM: Remove unnecessary V_IRQ unsetting

 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/hyperv.c           |  6 ++++--
 arch/x86/kvm/i8254.c            |  4 ++--
 arch/x86/kvm/svm/avic.c         |  2 +-
 arch/x86/kvm/svm/svm.c          | 18 ++++++------------
 arch/x86/kvm/x86.c              | 16 +++++++++++++---
 include/linux/kvm_host.h        |  3 +++
 virt/kvm/kvm_main.c             | 14 +++++++++++---
 8 files changed, 41 insertions(+), 24 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] KVM: Introduce kvm_make_all_cpus_request_except()

[Suravee Suthikulpanit]

This allows making request to all other vcpus except the one
specified in the parameter.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/hyperv.c    |  2 +-
 arch/x86/kvm/x86.c       |  2 +-
 include/linux/kvm_host.h |  3 +++
 virt/kvm/kvm_main.c      | 14 +++++++++++---
 4 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index bcefa9d..54d4b98 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1427,7 +1427,7 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *current_vcpu, u64 ingpa,
 	 */
 	kvm_make_vcpus_request_mask(kvm,
 				    KVM_REQ_TLB_FLUSH | KVM_REQUEST_NO_WAKEUP,
-				    vcpu_mask, &hv_vcpu->tlb_flush);
+				    NULL, vcpu_mask, &hv_vcpu->tlb_flush);
 
 ret_success:
 	/* We always do full TLB flush, set rep_done = rep_cnt. */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7900ea8..df473f9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8055,7 +8055,7 @@ void kvm_make_scan_ioapic_request_mask(struct kvm *kvm,
 	zalloc_cpumask_var(&cpus, GFP_ATOMIC);
 
 	kvm_make_vcpus_request_mask(kvm, KVM_REQ_SCAN_IOAPIC,
-				    vcpu_bitmap, cpus);
+				    NULL, vcpu_bitmap, cpus);
 
 	free_cpumask_var(cpus);
 }
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 01276e3..131cc15 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -813,8 +813,11 @@ int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data,
 void kvm_reload_remote_mmus(struct kvm *kvm);
 
 bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req,
+				 struct kvm_vcpu *except,
 				 unsigned long *vcpu_bitmap, cpumask_var_t tmp);
 bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req);
+bool kvm_make_all_cpus_request_except(struct kvm *kvm, unsigned int req,
+				      struct kvm_vcpu *except);
 bool kvm_make_cpus_request_mask(struct kvm *kvm, unsigned int req,
 				unsigned long *vcpu_bitmap);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 74bdb7b..df73605 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -259,6 +259,7 @@ static inline bool kvm_kick_many_cpus(const struct cpumask *cpus, bool wait)
 }
 
 bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req,
+				 struct kvm_vcpu *except,
 				 unsigned long *vcpu_bitmap, cpumask_var_t tmp)
 {
 	int i, cpu, me;
@@ -268,7 +269,8 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req,
 	me = get_cpu();
 
 	kvm_for_each_vcpu(i, vcpu, kvm) {
-		if (vcpu_bitmap && !test_bit(i, vcpu_bitmap))
+		if ((vcpu_bitmap && !test_bit(i, vcpu_bitmap)) ||
+		    (vcpu == except))
 			continue;
 
 		kvm_make_request(req, vcpu);
@@ -288,19 +290,25 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req,
 	return called;
 }
 
-bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
+bool kvm_make_all_cpus_request_except(struct kvm *kvm, unsigned int req,
+				      struct kvm_vcpu *except)
 {
 	cpumask_var_t cpus;
 	bool called;
 
 	zalloc_cpumask_var(&cpus, GFP_ATOMIC);
 
-	called = kvm_make_vcpus_request_mask(kvm, req, NULL, cpus);
+	called = kvm_make_vcpus_request_mask(kvm, req, except, NULL, cpus);
 
 	free_cpumask_var(cpus);
 	return called;
 }
 
+bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
+{
+	return kvm_make_all_cpus_request_except(kvm, req, NULL);
+}
+
 #ifndef CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL
 void kvm_flush_remote_tlbs(struct kvm *kvm)
 {
-- 
1.8.3.1

[PATCH 2/4] KVM: SVM: Fixes setting V_IRQ while AVIC is still enabled

[Suravee Suthikulpanit]

The commit 64b5bd270426 ("KVM: nSVM: ignore L1 interrupt window
while running L2 with V_INTR_MASKING=1") introduced a WARN_ON,
which checks if AVIC is enabled when trying to set V_IRQ
in the VMCB for enabling irq window.

The following warning is triggered because the requesting vcpu
(to deactivate AVIC) does not get to process APICv update request
for itself until the next #vmexit.

WARNING: CPU: 0 PID: 118232 at arch/x86/kvm/svm/svm.c:1372 enable_irq_window+0x6a/0xa0 [kvm_amd]
 RIP: 0010:enable_irq_window+0x6a/0xa0 [kvm_amd]
 Call Trace:
  kvm_arch_vcpu_ioctl_run+0x6e3/0x1b50 [kvm]
  ? kvm_vm_ioctl_irq_line+0x27/0x40 [kvm]
  ? _copy_to_user+0x26/0x30
  ? kvm_vm_ioctl+0xb3e/0xd90 [kvm]
  ? set_next_entity+0x78/0xc0
  kvm_vcpu_ioctl+0x236/0x610 [kvm]
  ksys_ioctl+0x8a/0xc0
  __x64_sys_ioctl+0x1a/0x20
  do_syscall_64+0x58/0x210
  entry_SYSCALL_64_after_hwframe+0x44/0xa9

Fixes by sending APICV update request to all other vcpus, and
immediately update APIC for itself.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Link: https://lkml.org/lkml/2020/5/2/167
Fixes: 64b5bd270426 ("KVM: nSVM: ignore L1 interrupt window while running L2 with V_INTR_MASKING=1")
---
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/hyperv.c           |  4 +++-
 arch/x86/kvm/i8254.c            |  4 ++--
 arch/x86/kvm/svm/avic.c         |  2 +-
 arch/x86/kvm/svm/svm.c          |  6 ++++--
 arch/x86/kvm/x86.c              | 14 ++++++++++++--
 6 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index dd84085..e7115dd 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1503,7 +1503,7 @@ gpa_t kvm_mmu_gva_to_gpa_system(struct kvm_vcpu *vcpu, gva_t gva,
 void kvm_apicv_init(struct kvm *kvm, bool enable);
 void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu);
 void kvm_request_apicv_update(struct kvm *kvm, bool activate,
-			      unsigned long bit);
+			      unsigned long bit, struct kvm_vcpu *except);
 
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
 
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 54d4b98..a007ca5 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -779,7 +779,9 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages)
 	 * not compatible with APICV, so request
 	 * to deactivate APICV permanently.
 	 */
-	kvm_request_apicv_update(vcpu->kvm, false, APICV_INHIBIT_REASON_HYPERV);
+	kvm_request_apicv_update(vcpu->kvm, false,
+				 APICV_INHIBIT_REASON_HYPERV,
+				 vcpu);
 	synic->active = true;
 	synic->dont_zero_synic_pages = dont_zero_synic_pages;
 	return 0;
diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index febca33..c524081 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -305,14 +305,14 @@ void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject)
 	 */
 	if (reinject) {
 		kvm_request_apicv_update(kvm, false,
-					 APICV_INHIBIT_REASON_PIT_REINJ);
+					 APICV_INHIBIT_REASON_PIT_REINJ, NULL);
 		/* The initial state is preserved while ps->reinject == 0. */
 		kvm_pit_reset_reinject(pit);
 		kvm_register_irq_ack_notifier(kvm, &ps->irq_ack_notifier);
 		kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
 	} else {
 		kvm_request_apicv_update(kvm, true,
-					 APICV_INHIBIT_REASON_PIT_REINJ);
+					 APICV_INHIBIT_REASON_PIT_REINJ, NULL);
 		kvm_unregister_irq_ack_notifier(kvm, &ps->irq_ack_notifier);
 		kvm_unregister_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
 	}
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index e80daa9..4b1574e 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -594,7 +594,7 @@ void svm_toggle_avic_for_irq_window(struct kvm_vcpu *vcpu, bool activate)
 
 	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
 	kvm_request_apicv_update(vcpu->kvm, activate,
-				 APICV_INHIBIT_REASON_IRQWIN);
+				 APICV_INHIBIT_REASON_IRQWIN, vcpu);
 	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 }
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 2f379ba..c2117cd 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3502,7 +3502,8 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
 	 */
 	if (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC))
 		kvm_request_apicv_update(vcpu->kvm, false,
-					 APICV_INHIBIT_REASON_X2APIC);
+					 APICV_INHIBIT_REASON_X2APIC,
+					 vcpu);
 
 	/*
 	 * Currently, AVIC does not work with nested virtualization.
@@ -3510,7 +3511,8 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
 	 */
 	if (nested && guest_cpuid_has(vcpu, X86_FEATURE_SVM))
 		kvm_request_apicv_update(vcpu->kvm, false,
-					 APICV_INHIBIT_REASON_NESTED);
+					 APICV_INHIBIT_REASON_NESTED,
+					 vcpu);
 }
 
 static bool svm_has_wbinvd_exit(void)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index df473f9..8f69ad0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8083,7 +8083,8 @@ void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
  * locked, because it calls __x86_set_memory_region() which does
  * synchronize_srcu(&kvm->srcu).
  */
-void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
+void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit,
+			      struct kvm_vcpu *except)
 {
 	unsigned long old, new, expected;
 
@@ -8110,7 +8111,16 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
 	trace_kvm_apicv_update_request(activate, bit);
 	if (kvm_x86_ops.pre_update_apicv_exec_ctrl)
 		kvm_x86_ops.pre_update_apicv_exec_ctrl(kvm, activate);
-	kvm_make_all_cpus_request(kvm, KVM_REQ_APICV_UPDATE);
+
+	/*
+	 * Sending request to update APICV for all other vcpus,
+	 * while update the calling vcpu immediately instead of
+	 * waiting for another #VMEXIT to handle the request.
+	 */
+	kvm_make_all_cpus_request_except(kvm, KVM_REQ_APICV_UPDATE,
+					 except);
+	if (except)
+		kvm_vcpu_update_apicv(except);
 }
 EXPORT_SYMBOL_GPL(kvm_request_apicv_update);
 
-- 
1.8.3.1

Re: [PATCH 2/4] KVM: SVM: Fixes setting V_IRQ while AVIC is still enabled

[Paolo Bonzini]

On 06/05/20 15:17, Suravee Suthikulpanit wrote:
>   */
> -void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
> +void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit,
> +			      struct kvm_vcpu *except)
>  {
>  	unsigned long old, new, expected;
>  
> @@ -8110,7 +8111,16 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
>  	trace_kvm_apicv_update_request(activate, bit);
>  	if (kvm_x86_ops.pre_update_apicv_exec_ctrl)
>  		kvm_x86_ops.pre_update_apicv_exec_ctrl(kvm, activate);
> -	kvm_make_all_cpus_request(kvm, KVM_REQ_APICV_UPDATE);
> +
> +	/*
> +	 * Sending request to update APICV for all other vcpus,
> +	 * while update the calling vcpu immediately instead of
> +	 * waiting for another #VMEXIT to handle the request.
> +	 */
> +	kvm_make_all_cpus_request_except(kvm, KVM_REQ_APICV_UPDATE,
> +					 except);
> +	if (except)
> +		kvm_vcpu_update_apicv(except);

Can you use kvm_get_running_vcpu() instead of touching all callers?
Also a slightly better subject would be "KVM: SVM: Disable AVIC before
setting V_IRQ".

Paolo

Re: [PATCH 2/4] KVM: SVM: Fixes setting V_IRQ while AVIC is still enabled

[Suravee Suthikulpanit]

On 5/6/20 11:29 PM, Paolo Bonzini wrote:
> On 06/05/20 15:17, Suravee Suthikulpanit wrote:
>>    */
>> -void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
>> +void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit,
>> +			      struct kvm_vcpu *except)
>>   {
>>   	unsigned long old, new, expected;
>>   
>> @@ -8110,7 +8111,16 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
>>   	trace_kvm_apicv_update_request(activate, bit);
>>   	if (kvm_x86_ops.pre_update_apicv_exec_ctrl)
>>   		kvm_x86_ops.pre_update_apicv_exec_ctrl(kvm, activate);
>> -	kvm_make_all_cpus_request(kvm, KVM_REQ_APICV_UPDATE);
>> +
>> +	/*
>> +	 * Sending request to update APICV for all other vcpus,
>> +	 * while update the calling vcpu immediately instead of
>> +	 * waiting for another #VMEXIT to handle the request.
>> +	 */
>> +	kvm_make_all_cpus_request_except(kvm, KVM_REQ_APICV_UPDATE,
>> +					 except);
>> +	if (except)
>> +		kvm_vcpu_update_apicv(except);
> 
> Can you use kvm_get_running_vcpu() instead of touching all callers?
> Also a slightly better subject would be "KVM: SVM: Disable AVIC before
> setting V_IRQ".
> 
> Paolo
> 

Right, that's better idea. I'll send out V2 separately.

Thanks,
Suravee

[PATCH 3/4] KVM: SVM: Merge svm_enable_vintr into svm_set_vintr

[Suravee Suthikulpanit]

Code clean up and remove unnecessary intercept check for
INTERCEPT_VINTR.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm/svm.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index c2117cd..8ac00b2 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1364,12 +1364,13 @@ static void svm_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
 	}
 }
 
-static inline void svm_enable_vintr(struct vcpu_svm *svm)
+static void svm_set_vintr(struct vcpu_svm *svm)
 {
 	struct vmcb_control_area *control;
 
 	/* The following fields are ignored when AVIC is enabled */
 	WARN_ON(kvm_vcpu_apicv_active(&svm->vcpu));
+	set_intercept(svm, INTERCEPT_VINTR);
 
 	/*
 	 * This is just a dummy VINTR to actually cause a vmexit to happen.
@@ -1383,13 +1384,6 @@ static inline void svm_enable_vintr(struct vcpu_svm *svm)
 	mark_dirty(svm->vmcb, VMCB_INTR);
 }
 
-static void svm_set_vintr(struct vcpu_svm *svm)
-{
-	set_intercept(svm, INTERCEPT_VINTR);
-	if (is_intercept(svm, INTERCEPT_VINTR))
-		svm_enable_vintr(svm);
-}
-
 static void svm_clear_vintr(struct vcpu_svm *svm)
 {
 	clr_intercept(svm, INTERCEPT_VINTR);
-- 
1.8.3.1

[PATCH 4/4] KVM: SVM: Remove unnecessary V_IRQ unsetting

[Suravee Suthikulpanit]

This has already been handled in the prior call to svm_clear_vintr().

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm/svm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 8ac00b2..553d4b6 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2662,8 +2662,6 @@ static int interrupt_window_interception(struct vcpu_svm *svm)
 	 */
 	svm_toggle_avic_for_irq_window(&svm->vcpu, true);
 
-	svm->vmcb->control.int_ctl &= ~V_IRQ_MASK;
-	mark_dirty(svm->vmcb, VMCB_INTR);
 	++svm->vcpu.stat.irq_window_exits;
 	return 1;
 }
-- 
1.8.3.1

Re: [PATCH 0/4] KVM: SVM: Fix AVIC warning when enable irq window

[Paolo Bonzini]

On 06/05/20 15:17, Suravee Suthikulpanit wrote:
> Introduce kvm_make_all_cpus_request_except(), which is used
> in the subsequent patch 2 to fix AVIC warning.
> 
> Also include miscelleneous clean ups.
> 
> Thanks,
> Suravee
> 
> Suravee Suthikulpanit (4):
>   KVM: Introduce kvm_make_all_cpus_request_except()
>   KVM: SVM: Fixes setting V_IRQ while AVIC is still enabled
>   KVM: SVM: Merge svm_enable_vintr into svm_set_vintr
>   KVM: SVM: Remove unnecessary V_IRQ unsetting
> 
>  arch/x86/include/asm/kvm_host.h |  2 +-
>  arch/x86/kvm/hyperv.c           |  6 ++++--
>  arch/x86/kvm/i8254.c            |  4 ++--
>  arch/x86/kvm/svm/avic.c         |  2 +-
>  arch/x86/kvm/svm/svm.c          | 18 ++++++------------
>  arch/x86/kvm/x86.c              | 16 +++++++++++++---
>  include/linux/kvm_host.h        |  3 +++
>  virt/kvm/kvm_main.c             | 14 +++++++++++---
>  8 files changed, 41 insertions(+), 24 deletions(-)
> 

I merged patches 1-3-4, you can send out 2 independently.

Thanks,

Paolo