diff mbox series

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

Message ID 1588771076-73790-3-git-send-email-suravee.suthikulpanit@amd.com (mailing list archive)
State New, archived
Headers show
Series KVM: SVM: Fix AVIC warning when enable irq window | expand

Commit Message

Suthikulpanit, Suravee May 6, 2020, 1:17 p.m. UTC 
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(-)

Comments

Paolo Bonzini May 6, 2020, 4:29 p.m. UTC | #1 
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
Suthikulpanit, Suravee May 7, 2020, 1:29 a.m. UTC | #2 
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
diff mbox series

Patch

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);