[PATCH 0/3] KVM: SVM: fix race between interrupt delivery and AVIC inhibition

[PATCH 0/3] KVM: SVM: fix race between interrupt delivery and AVIC inhibition

[Paolo Bonzini]

This is a split and cleaned-up version of Maxim's patch to fix the
SVM race between interrupt delivery and AVIC inhibition.  The final
difference is just code movement and formatting.

Maxim Levitsky (2):
  KVM: SVM: extract avic_ring_doorbell
  KVM: SVM: fix race between interrupt delivery and AVIC inhibition

Paolo Bonzini (1):
  KVM: SVM: set IRR in svm_deliver_interrupt

 arch/x86/kvm/svm/avic.c | 73 ++++++++++++++---------------------------
 arch/x86/kvm/svm/svm.c  | 48 +++++++++++++++++++++++----
 arch/x86/kvm/svm/svm.h  |  4 ++-
 arch/x86/kvm/x86.c      |  4 ++-
 4 files changed, 72 insertions(+), 57 deletions(-)

-- 
2.31.1

[PATCH 1/3] KVM: SVM: extract avic_ring_doorbell

[Paolo Bonzini]

From: Maxim Levitsky <mlevitsk@redhat.com>

The check on the current CPU adds an extra level of indentation to
svm_deliver_avic_intr and conflates documentation on what happens
if the vCPU exits (of interest to svm_deliver_avic_intr) and migrates
(only of interest to avic_ring_doorbell, which calls get/put_cpu()).
Extract the wrmsr to a separate function and rewrite the
comment in svm_deliver_avic_intr().

Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm/avic.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 3f9b48732aea..4d1baf5c8f6a 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -269,6 +269,24 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+
+static void avic_ring_doorbell(struct kvm_vcpu *vcpu)
+{
+	/*
+	 * Note, the vCPU could get migrated to a different pCPU at any
+	 * point, which could result in signalling the wrong/previous
+	 * pCPU.  But if that happens the vCPU is guaranteed to do a
+	 * VMRUN (after being migrated) and thus will process pending
+	 * interrupts, i.e. a doorbell is not needed (and the spurious
+	 * one is harmless).
+	 */
+	int cpu = READ_ONCE(vcpu->cpu);
+
+	if (cpu != get_cpu())
+		wrmsrl(MSR_AMD64_SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpu));
+	put_cpu();
+}
+
 static void avic_kick_target_vcpus(struct kvm *kvm, struct kvm_lapic *source,
 				   u32 icrl, u32 icrh)
 {
@@ -669,19 +687,12 @@ int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec)
 	 * automatically process AVIC interrupts at VMRUN.
 	 */
 	if (vcpu->mode == IN_GUEST_MODE) {
-		int cpu = READ_ONCE(vcpu->cpu);
-
 		/*
-		 * Note, the vCPU could get migrated to a different pCPU at any
-		 * point, which could result in signalling the wrong/previous
-		 * pCPU.  But if that happens the vCPU is guaranteed to do a
-		 * VMRUN (after being migrated) and thus will process pending
-		 * interrupts, i.e. a doorbell is not needed (and the spurious
-		 * one is harmless).
+		 * Signal the doorbell to tell hardware to inject the IRQ if the vCPU
+		 * is in the guest.  If the vCPU is not in the guest, hardware will
+		 * automatically process AVIC interrupts at VMRUN.
 		 */
-		if (cpu != get_cpu())
-			wrmsrl(MSR_AMD64_SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpu));
-		put_cpu();
+		avic_ring_doorbell(vcpu);
 	} else {
 		/*
 		 * Wake the vCPU if it was blocking.  KVM will then detect the
-- 
2.31.1

[PATCH 2/3] KVM: SVM: set IRR in svm_deliver_interrupt

[Paolo Bonzini]

SVM has to set IRR for both the AVIC and the software-LAPIC case,
so pull it up to the common function that handles both configurations.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm/avic.c | 2 --
 arch/x86/kvm/svm/svm.c  | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 4d1baf5c8f6a..1e1890721634 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -670,8 +670,6 @@ int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec)
 	if (!vcpu->arch.apicv_active)
 		return -1;
 
-	kvm_lapic_set_irr(vec, vcpu->arch.apic);
-
 	/*
 	 * Pairs with the smp_mb_*() after setting vcpu->guest_mode in
 	 * vcpu_enter_guest() to ensure the write to the vIRR is ordered before
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 52e4130110f3..cd769ff8af16 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3304,8 +3304,8 @@ static void svm_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode,
 {
 	struct kvm_vcpu *vcpu = apic->vcpu;
 
+	kvm_lapic_set_irr(vector, apic);
 	if (svm_deliver_avic_intr(vcpu, vector)) {
-		kvm_lapic_set_irr(vector, apic);
 		kvm_make_request(KVM_REQ_EVENT, vcpu);
 		kvm_vcpu_kick(vcpu);
 	} else {
-- 
2.31.1

[PATCH 3/3] KVM: SVM: fix race between interrupt delivery and AVIC inhibition

[Paolo Bonzini]

From: Maxim Levitsky <mlevitsk@redhat.com>

If svm_deliver_avic_intr is called just after the target vcpu's AVIC got
inhibited, it might read a stale value of vcpu->arch.apicv_active
which can lead to the target vCPU not noticing the interrupt.

To fix this use load-acquire/store-release so that, if the target vCPU
is IN_GUEST_MODE, we're guaranteed to see a previous disabling of the
AVIC.  If AVIC has been disabled in the meanwhile, proceed with the
KVM_REQ_EVENT-based delivery.

Incomplete IPI vmexit has the same races as svm_deliver_avic_intr, and
in fact it can be handled in exactly the same way; the only difference
lies in who has set IRR, whether svm_deliver_interrupt or the processor.
Therefore, svm_complete_interrupt_delivery can be used to fix incomplete
IPI vmexits as well.

Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm/avic.c | 48 +++++++----------------------------------
 arch/x86/kvm/svm/svm.c  | 48 +++++++++++++++++++++++++++++++++++------
 arch/x86/kvm/svm/svm.h  |  4 +++-
 arch/x86/kvm/x86.c      |  4 +++-
 4 files changed, 55 insertions(+), 49 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 1e1890721634..c32c46b15cb9 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -270,7 +270,7 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
 }
 
 
-static void avic_ring_doorbell(struct kvm_vcpu *vcpu)
+void avic_ring_doorbell(struct kvm_vcpu *vcpu)
 {
 	/*
 	 * Note, the vCPU could get migrated to a different pCPU at any
@@ -302,8 +302,13 @@ static void avic_kick_target_vcpus(struct kvm *kvm, struct kvm_lapic *source,
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		if (kvm_apic_match_dest(vcpu, source, icrl & APIC_SHORT_MASK,
 					GET_APIC_DEST_FIELD(icrh),
-					icrl & APIC_DEST_MASK))
-			kvm_vcpu_wake_up(vcpu);
+					icrl & APIC_DEST_MASK)) {
+			vcpu->arch.apic->irr_pending = true;
+			svm_complete_interrupt_delivery(vcpu,
+							icrl & APIC_MODE_MASK,
+							icrl & APIC_INT_LEVELTRIG,
+							icrl & APIC_VECTOR_MASK);
+		}
 	}
 }
 
@@ -665,43 +670,6 @@ void svm_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
 	return;
 }
 
-int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec)
-{
-	if (!vcpu->arch.apicv_active)
-		return -1;
-
-	/*
-	 * Pairs with the smp_mb_*() after setting vcpu->guest_mode in
-	 * vcpu_enter_guest() to ensure the write to the vIRR is ordered before
-	 * the read of guest_mode, which guarantees that either VMRUN will see
-	 * and process the new vIRR entry, or that the below code will signal
-	 * the doorbell if the vCPU is already running in the guest.
-	 */
-	smp_mb__after_atomic();
-
-	/*
-	 * Signal the doorbell to tell hardware to inject the IRQ if the vCPU
-	 * is in the guest.  If the vCPU is not in the guest, hardware will
-	 * automatically process AVIC interrupts at VMRUN.
-	 */
-	if (vcpu->mode == IN_GUEST_MODE) {
-		/*
-		 * Signal the doorbell to tell hardware to inject the IRQ if the vCPU
-		 * is in the guest.  If the vCPU is not in the guest, hardware will
-		 * automatically process AVIC interrupts at VMRUN.
-		 */
-		avic_ring_doorbell(vcpu);
-	} else {
-		/*
-		 * Wake the vCPU if it was blocking.  KVM will then detect the
-		 * pending IRQ when checking if the vCPU has a wake event.
-		 */
-		kvm_vcpu_wake_up(vcpu);
-	}
-
-	return 0;
-}
-
 bool svm_dy_apicv_has_pending_interrupt(struct kvm_vcpu *vcpu)
 {
 	return false;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index cd769ff8af16..2ad158b27e91 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3299,21 +3299,55 @@ static void svm_set_irq(struct kvm_vcpu *vcpu)
 		SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR;
 }
 
-static void svm_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode,
-				  int trig_mode, int vector)
+void svm_complete_interrupt_delivery(struct kvm_vcpu *vcpu, int delivery_mode,
+				     int trig_mode, int vector)
 {
-	struct kvm_vcpu *vcpu = apic->vcpu;
+	/*
+	 * vcpu->arch.apicv_active must be read after vcpu->mode.
+	 * Pairs with smp_store_release in vcpu_enter_guest.
+	 */
+	bool in_guest_mode = (smp_load_acquire(&vcpu->mode) == IN_GUEST_MODE);
 
-	kvm_lapic_set_irr(vector, apic);
-	if (svm_deliver_avic_intr(vcpu, vector)) {
+	if (!READ_ONCE(vcpu->arch.apicv_active)) {
+		/* Process the interrupt with a vmexit.  */
 		kvm_make_request(KVM_REQ_EVENT, vcpu);
 		kvm_vcpu_kick(vcpu);
+		return;
+	}
+
+	trace_kvm_apicv_accept_irq(vcpu->vcpu_id, delivery_mode, trig_mode, vector);
+	if (in_guest_mode) {
+		/*
+		 * Signal the doorbell to tell hardware to inject the IRQ if the vCPU
+		 * is in the guest.  If the vCPU is not in the guest, hardware will
+		 * automatically process AVIC interrupts at VMRUN.
+		 */
+		avic_ring_doorbell(vcpu);
 	} else {
-		trace_kvm_apicv_accept_irq(vcpu->vcpu_id, delivery_mode,
-					   trig_mode, vector);
+		/*
+		 * Wake the vCPU if it was blocking.  KVM will then detect the
+		 * pending IRQ when checking if the vCPU has a wake event.
+		 */
+		kvm_vcpu_wake_up(vcpu);
 	}
 }
 
+static void svm_deliver_interrupt(struct kvm_lapic *apic,  int delivery_mode,
+				  int trig_mode, int vector)
+{
+	kvm_lapic_set_irr(vector, apic);
+
+	/*
+	 * Pairs with the smp_mb_*() after setting vcpu->guest_mode in
+	 * vcpu_enter_guest() to ensure the write to the vIRR is ordered before
+	 * the read of guest_mode.  This guarantees that either VMRUN will see
+	 * and process the new vIRR entry, or that svm_complete_interrupt_delivery
+	 * will signal the doorbell if the CPU has already performed vmentry.
+	 */
+	smp_mb__after_atomic();
+	svm_complete_interrupt_delivery(apic->vcpu, delivery_mode, trig_mode, vector);
+}
+
 static void svm_update_cr8_intercept(struct kvm_vcpu *vcpu, int tpr, int irr)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 8cc45f27fcbd..dd895f0f5569 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -489,6 +489,8 @@ void svm_set_gif(struct vcpu_svm *svm, bool value);
 int svm_invoke_exit_handler(struct kvm_vcpu *vcpu, u64 exit_code);
 void set_msr_interception(struct kvm_vcpu *vcpu, u32 *msrpm, u32 msr,
 			  int read, int write);
+void svm_complete_interrupt_delivery(struct kvm_vcpu *vcpu, int delivery_mode,
+		  int trig_mode, int vec);
 
 /* nested.c */
 
@@ -572,12 +574,12 @@ bool svm_check_apicv_inhibit_reasons(ulong bit);
 void svm_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
 void svm_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr);
 void svm_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr);
-int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec);
 bool svm_dy_apicv_has_pending_interrupt(struct kvm_vcpu *vcpu);
 int svm_update_pi_irte(struct kvm *kvm, unsigned int host_irq,
 		       uint32_t guest_irq, bool set);
 void avic_vcpu_blocking(struct kvm_vcpu *vcpu);
 void avic_vcpu_unblocking(struct kvm_vcpu *vcpu);
+void avic_ring_doorbell(struct kvm_vcpu *vcpu);
 
 /* sev.c */
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7131d735b1ef..641044db415d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9983,7 +9983,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	 * result in virtual interrupt delivery.
 	 */
 	local_irq_disable();
-	vcpu->mode = IN_GUEST_MODE;
+
+	/* Store vcpu->apicv_active before vcpu->mode.  */
+	smp_store_release(&vcpu->mode, IN_GUEST_MODE);
 
 	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
 
-- 
2.31.1

Re: [PATCH 1/3] KVM: SVM: extract avic_ring_doorbell

[Sean Christopherson]

On Fri, Feb 11, 2022, Paolo Bonzini wrote:
> From: Maxim Levitsky <mlevitsk@redhat.com>
> 
> The check on the current CPU adds an extra level of indentation to
> svm_deliver_avic_intr and conflates documentation on what happens
> if the vCPU exits (of interest to svm_deliver_avic_intr) and migrates
> (only of interest to avic_ring_doorbell, which calls get/put_cpu()).
> Extract the wrmsr to a separate function and rewrite the
> comment in svm_deliver_avic_intr().
> 
> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Bad SoB chain, should be:

  Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
  Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
  Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Interestingly, git-apply drops the second, redundant SoB and yields

  Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
  Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
  Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>

Which will probably get you yelled at by Stephen's scripts :-)

A few nits below...

Reviewed-by: Sean Christopherson <seanjc@google.com>

> ---
>  arch/x86/kvm/svm/avic.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 3f9b48732aea..4d1baf5c8f6a 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -269,6 +269,24 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> +

Spurious newline.

> +static void avic_ring_doorbell(struct kvm_vcpu *vcpu)
> +{
> +	/*
> +	 * Note, the vCPU could get migrated to a different pCPU at any
> +	 * point, which could result in signalling the wrong/previous
> +	 * pCPU.  But if that happens the vCPU is guaranteed to do a
> +	 * VMRUN (after being migrated) and thus will process pending
> +	 * interrupts, i.e. a doorbell is not needed (and the spurious
> +	 * one is harmless).

Please run these out to 80 chars, it saves a whole line!

	/*
	 * Note, the vCPU could get migrated to a different pCPU at any point,
	 * which could result in signalling the wrong/previous pCPU.  But if
	 * that happens the vCPU is guaranteed to do a VMRUN (after being
	 * migrated) and thus will process pending interrupts, i.e. a doorbell
	 * is not needed (and the spurious one is harmless).
	 */

> +	 */
> +	int cpu = READ_ONCE(vcpu->cpu);
> +
> +	if (cpu != get_cpu())
> +		wrmsrl(MSR_AMD64_SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpu));
> +	put_cpu();
> +}
> +
>  static void avic_kick_target_vcpus(struct kvm *kvm, struct kvm_lapic *source,
>  				   u32 icrl, u32 icrh)
>  {

Re: [PATCH 2/3] KVM: SVM: set IRR in svm_deliver_interrupt

[Sean Christopherson]

On Fri, Feb 11, 2022, Paolo Bonzini wrote:
> SVM has to set IRR for both the AVIC and the software-LAPIC case,
> so pull it up to the common function that handles both configurations.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH 3/3] KVM: SVM: fix race between interrupt delivery and AVIC inhibition

[Sean Christopherson]

On Fri, Feb 11, 2022, Paolo Bonzini wrote:
> From: Maxim Levitsky <mlevitsk@redhat.com>
> 
> If svm_deliver_avic_intr is called just after the target vcpu's AVIC got
> inhibited, it might read a stale value of vcpu->arch.apicv_active
> which can lead to the target vCPU not noticing the interrupt.
> 
> To fix this use load-acquire/store-release so that, if the target vCPU
> is IN_GUEST_MODE, we're guaranteed to see a previous disabling of the
> AVIC.  If AVIC has been disabled in the meanwhile, proceed with the
> KVM_REQ_EVENT-based delivery.
> 
> Incomplete IPI vmexit has the same races as svm_deliver_avic_intr, and
> in fact it can be handled in exactly the same way; the only difference
> lies in who has set IRR, whether svm_deliver_interrupt or the processor.
> Therefore, svm_complete_interrupt_delivery can be used to fix incomplete
> IPI vmexits as well.
> 
> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Same SoB issues.

Several comments on non-functional things, with those addressed:

Reviewed-by: Sean Christopherson <seanjc@google.com>

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index cd769ff8af16..2ad158b27e91 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3299,21 +3299,55 @@ static void svm_set_irq(struct kvm_vcpu *vcpu)
>  		SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR;
>  }
>  
> -static void svm_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode,
> -				  int trig_mode, int vector)
> +void svm_complete_interrupt_delivery(struct kvm_vcpu *vcpu, int delivery_mode,
> +				     int trig_mode, int vector)
>  {
> -	struct kvm_vcpu *vcpu = apic->vcpu;
> +	/*
> +	 * vcpu->arch.apicv_active must be read after vcpu->mode.
> +	 * Pairs with smp_store_release in vcpu_enter_guest.
> +	 */
> +	bool in_guest_mode = (smp_load_acquire(&vcpu->mode) == IN_GUEST_MODE);
>  
> -	kvm_lapic_set_irr(vector, apic);
> -	if (svm_deliver_avic_intr(vcpu, vector)) {
> +	if (!READ_ONCE(vcpu->arch.apicv_active)) {
> +		/* Process the interrupt with a vmexit.  */

Double spaces at the end.  But I would prefer we omit the comment entirely,
there is no guarantee the vCPU is in the guest or even running.

>  		kvm_make_request(KVM_REQ_EVENT, vcpu);
>  		kvm_vcpu_kick(vcpu);
> +		return;
> +	}
> +
> +	trace_kvm_apicv_accept_irq(vcpu->vcpu_id, delivery_mode, trig_mode, vector);
> +	if (in_guest_mode) {
> +		/*
> +		 * Signal the doorbell to tell hardware to inject the IRQ if the vCPU
> +		 * is in the guest.  If the vCPU is not in the guest, hardware will
> +		 * automatically process AVIC interrupts at VMRUN.

This is a bit confusing because KVM has _just_ checked if the vCPU is in the guest.
Something like this?

		/*
		 * Signal the doorbell to tell hardware to inject the IRQ.  If
		 * the vCPU exits the guest before the doorbell chimes, hardware
		 * will automatically process AVIC interrupts at the next VMRUN.
		 */

> +		 */
> +		avic_ring_doorbell(vcpu);
>  	} else {
> -		trace_kvm_apicv_accept_irq(vcpu->vcpu_id, delivery_mode,
> -					   trig_mode, vector);
> +		/*
> +		 * Wake the vCPU if it was blocking.  KVM will then detect the
> +		 * pending IRQ when checking if the vCPU has a wake event.
> +		 */
> +		kvm_vcpu_wake_up(vcpu);
>  	}
>  }
>  
> +static void svm_deliver_interrupt(struct kvm_lapic *apic,  int delivery_mode,
> +				  int trig_mode, int vector)
> +{
> +	kvm_lapic_set_irr(vector, apic);
> +
> +	/*
> +	 * Pairs with the smp_mb_*() after setting vcpu->guest_mode in
> +	 * vcpu_enter_guest() to ensure the write to the vIRR is ordered before
> +	 * the read of guest_mode.  This guarantees that either VMRUN will see
> +	 * and process the new vIRR entry, or that svm_complete_interrupt_delivery
> +	 * will signal the doorbell if the CPU has already performed vmentry.

How about "if the CPU has already entered the guest" instead of "performed vmentry"?
Mixing VMRUN and vmentry/VM-Entry is confusing because KVM often uses VM-Enter/VM-Entry
to refer to VMRESUME/VMLAUNCH/VMRUN as a single concept (though I agree vmentry is better
than VMRUN here, because ucode checks the vIRR in the middle of VMRUN before "VM entry").
And for that usage, KVM is the one that performs VM-Entry.

> +	 */
> +	smp_mb__after_atomic();
> +	svm_complete_interrupt_delivery(apic->vcpu, delivery_mode, trig_mode, vector);
> +}
> +
>  static void svm_update_cr8_intercept(struct kvm_vcpu *vcpu, int tpr, int irr)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 8cc45f27fcbd..dd895f0f5569 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -489,6 +489,8 @@ void svm_set_gif(struct vcpu_svm *svm, bool value);
>  int svm_invoke_exit_handler(struct kvm_vcpu *vcpu, u64 exit_code);
>  void set_msr_interception(struct kvm_vcpu *vcpu, u32 *msrpm, u32 msr,
>  			  int read, int write);
> +void svm_complete_interrupt_delivery(struct kvm_vcpu *vcpu, int delivery_mode,
> +		  int trig_mode, int vec);

Please align the params.

>  
>  /* nested.c */
>  
> @@ -572,12 +574,12 @@ bool svm_check_apicv_inhibit_reasons(ulong bit);
>  void svm_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
>  void svm_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr);
>  void svm_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr);
> -int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec);
>  bool svm_dy_apicv_has_pending_interrupt(struct kvm_vcpu *vcpu);
>  int svm_update_pi_irte(struct kvm *kvm, unsigned int host_irq,
>  		       uint32_t guest_irq, bool set);
>  void avic_vcpu_blocking(struct kvm_vcpu *vcpu);
>  void avic_vcpu_unblocking(struct kvm_vcpu *vcpu);
> +void avic_ring_doorbell(struct kvm_vcpu *vcpu);
>  
>  /* sev.c */
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7131d735b1ef..641044db415d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9983,7 +9983,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  	 * result in virtual interrupt delivery.
>  	 */
>  	local_irq_disable();
> -	vcpu->mode = IN_GUEST_MODE;
> +
> +	/* Store vcpu->apicv_active before vcpu->mode.  */
> +	smp_store_release(&vcpu->mode, IN_GUEST_MODE);
>  
>  	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
>  
> -- 
> 2.31.1
> 

Re: [PATCH 3/3] KVM: SVM: fix race between interrupt delivery and AVIC inhibition

[Paolo Bonzini]

On 2/11/22 18:11, Sean Christopherson wrote:
>> +		/* Process the interrupt with a vmexit.  */
>
> Double spaces at the end.  But I would prefer we omit the comment entirely,
> there is no guarantee the vCPU is in the guest or even running.

Sure, or perhaps "process the interrupt in inject_pending_event".

Regarding the two spaces, it used to a pretty strict rule in the US with 
typewriters.  It helps readability of monospaced fonts 
(https://www.cultofpedagogy.com/two-spaces-after-period/), and code is 
mostly monospaced...  But well, the title of the article says it all.

Paolo

Re: [PATCH 3/3] KVM: SVM: fix race between interrupt delivery and AVIC inhibition

[Sean Christopherson]

On Fri, Feb 11, 2022, Paolo Bonzini wrote:
> On 2/11/22 18:11, Sean Christopherson wrote:
> > > +		/* Process the interrupt with a vmexit.  */
> > 
> > Double spaces at the end.  But I would prefer we omit the comment entirely,
> > there is no guarantee the vCPU is in the guest or even running.
> 
> Sure, or perhaps "process the interrupt in inject_pending_event".

s/in/via?

> Regarding the two spaces, it used to a pretty strict rule in the US with
> typewriters.  It helps readability of monospaced fonts
> (https://www.cultofpedagogy.com/two-spaces-after-period/), and code is
> mostly monospaced...  But well, the title of the article says it all.

Preaching to the choir, I'm a firm believer that there should always be two spaces
after a full stop, monospace or not.  Unless it's the end of a comment. :-D

Re: [PATCH 3/3] KVM: SVM: fix race between interrupt delivery and AVIC inhibition

[Maxim Levitsky]

On Fri, 2022-02-11 at 17:35 +0000, Sean Christopherson wrote:
> On Fri, Feb 11, 2022, Paolo Bonzini wrote:
> > On 2/11/22 18:11, Sean Christopherson wrote:
> > > > +		/* Process the interrupt with a vmexit.  */
> > > 
> > > Double spaces at the end.  But I would prefer we omit the comment entirely,
> > > there is no guarantee the vCPU is in the guest or even running.
> > 
> > Sure, or perhaps "process the interrupt in inject_pending_event".
> 
> s/in/via?
> 
> > Regarding the two spaces, it used to a pretty strict rule in the US with
> > typewriters.  It helps readability of monospaced fonts
> > (https://www.cultofpedagogy.com/two-spaces-after-period/), and code is
> > mostly monospaced...  But well, the title of the article says it all.
> 
> Preaching to the choir, I'm a firm believer that there should always be two spaces
> after a full stop, monospace or not.  Unless it's the end of a comment. :-D
> 
As someone who gotten into typewritter repair hobby lately, I sure understand what you mean.
But for some reason I still use one space after period even on a typewritter and
that doesn't bother me :-)

Best regards,
	Maxim Levitsky

Re: [PATCH 0/3] KVM: SVM: fix race between interrupt delivery and AVIC inhibition

[Maxim Levitsky]

On Fri, 2022-02-11 at 06:01 -0500, Paolo Bonzini wrote:
> This is a split and cleaned-up version of Maxim's patch to fix the
> SVM race between interrupt delivery and AVIC inhibition.  The final
> difference is just code movement and formatting.
> 
> Maxim Levitsky (2):
>   KVM: SVM: extract avic_ring_doorbell
>   KVM: SVM: fix race between interrupt delivery and AVIC inhibition
> 
> Paolo Bonzini (1):
>   KVM: SVM: set IRR in svm_deliver_interrupt
> 
>  arch/x86/kvm/svm/avic.c | 73 ++++++++++++++---------------------------
>  arch/x86/kvm/svm/svm.c  | 48 +++++++++++++++++++++++----
>  arch/x86/kvm/svm/svm.h  |  4 ++-
>  arch/x86/kvm/x86.c      |  4 ++-
>  4 files changed, 72 insertions(+), 57 deletions(-)
> 

I know I am beeing late as these patches are already merged,
but as far as I can see, they all look fine.
Thanks!

Best regards,
	Maxim Levitsky