[PATCH] Revert "svm: Fix AVIC incomplete IPI emulation"

[PATCH] Revert "svm: Fix AVIC incomplete IPI emulation"

[Suthikulpanit, Suravee]

This reverts commit bb218fbcfaaa3b115d4cd7a43c0ca164f3a96e57.

As Oren Twaig pointed out the old discussion:

  https://patchwork.kernel.org/patch/8292231/

that the change coud potentially cause an extra IPI to be sent to
the destination vcpu because the AVIC hardware already set the IRR bit
before the incomplete IPI #VMEXIT with id=1 (target vcpu is not running).
Since writting to ICR and ICR2 will also set the IRR. If something triggers
the destination vcpu to get scheduled before the emulation finishes, then
this could result in an additional IPI.

Also, the issue mentioned in the commit bb218fbcfaaa was misdiagnosed.

Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Reported-by: Oren Twaig <oren@scalemp.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index f13a3a24d360..47c4993448c7 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -4512,14 +4512,25 @@ static int avic_incomplete_ipi_interception(struct vcpu_svm *svm)
 		kvm_lapic_reg_write(apic, APIC_ICR, icrl);
 		break;
 	case AVIC_IPI_FAILURE_TARGET_NOT_RUNNING: {
+		int i;
+		struct kvm_vcpu *vcpu;
+		struct kvm *kvm = svm->vcpu.kvm;
 		struct kvm_lapic *apic = svm->vcpu.arch.apic;
 
 		/*
-		 * Update ICR high and low, then emulate sending IPI,
-		 * which is handled when writing APIC_ICR.
+		 * At this point, we expect that the AVIC HW has already
+		 * set the appropriate IRR bits on the valid target
+		 * vcpus. So, we just need to kick the appropriate vcpu.
 		 */
-		kvm_lapic_reg_write(apic, APIC_ICR2, icrh);
-		kvm_lapic_reg_write(apic, APIC_ICR, icrl);
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			bool m = kvm_apic_match_dest(vcpu, apic,
+						     icrl & KVM_APIC_SHORT_MASK,
+						     GET_APIC_DEST_FIELD(icrh),
+						     icrl & KVM_APIC_DEST_MASK);
+
+			if (m && !avic_vcpu_is_running(vcpu))
+				kvm_vcpu_wake_up(vcpu);
+		}
 		break;
 	}
 	case AVIC_IPI_FAILURE_INVALID_TARGET:
-- 
2.17.1

Re: [PATCH] Revert "svm: Fix AVIC incomplete IPI emulation"

[Suthikulpanit, Suravee]

Ping

On 3/20/19 3:12 PM, Suthikulpanit, Suravee wrote:
> This reverts commit bb218fbcfaaa3b115d4cd7a43c0ca164f3a96e57.
> 
> As Oren Twaig pointed out the old discussion:
> 
>    https://patchwork.kernel.org/patch/8292231/
> 
> that the change coud potentially cause an extra IPI to be sent to
> the destination vcpu because the AVIC hardware already set the IRR bit
> before the incomplete IPI #VMEXIT with id=1 (target vcpu is not running).
> Since writting to ICR and ICR2 will also set the IRR. If something triggers
> the destination vcpu to get scheduled before the emulation finishes, then
> this could result in an additional IPI.
> 
> Also, the issue mentioned in the commit bb218fbcfaaa was misdiagnosed.
> 
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Reported-by: Oren Twaig <oren@scalemp.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   arch/x86/kvm/svm.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index f13a3a24d360..47c4993448c7 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -4512,14 +4512,25 @@ static int avic_incomplete_ipi_interception(struct vcpu_svm *svm)
>   		kvm_lapic_reg_write(apic, APIC_ICR, icrl);
>   		break;
>   	case AVIC_IPI_FAILURE_TARGET_NOT_RUNNING: {
> +		int i;
> +		struct kvm_vcpu *vcpu;
> +		struct kvm *kvm = svm->vcpu.kvm;
>   		struct kvm_lapic *apic = svm->vcpu.arch.apic;
>   
>   		/*
> -		 * Update ICR high and low, then emulate sending IPI,
> -		 * which is handled when writing APIC_ICR.
> +		 * At this point, we expect that the AVIC HW has already
> +		 * set the appropriate IRR bits on the valid target
> +		 * vcpus. So, we just need to kick the appropriate vcpu.
>   		 */
> -		kvm_lapic_reg_write(apic, APIC_ICR2, icrh);
> -		kvm_lapic_reg_write(apic, APIC_ICR, icrl);
> +		kvm_for_each_vcpu(i, vcpu, kvm) {
> +			bool m = kvm_apic_match_dest(vcpu, apic,
> +						     icrl & KVM_APIC_SHORT_MASK,
> +						     GET_APIC_DEST_FIELD(icrh),
> +						     icrl & KVM_APIC_DEST_MASK);
> +
> +			if (m && !avic_vcpu_is_running(vcpu))
> +				kvm_vcpu_wake_up(vcpu);
> +		}
>   		break;
>   	}
>   	case AVIC_IPI_FAILURE_INVALID_TARGET:
>