[PATCH] KVM: x86/vmx: Suppress posted interrupt notification when CPU is in host

[PATCH] KVM: x86/vmx: Suppress posted interrupt notification when CPU is in host

[Chao Gao]

PIN (Posted interrupt notification) is useless to host as KVM always syncs
pending guest interrupts in PID to guest's vAPIC before each VM entry. In
fact, Sending PINs to a CPU running in host will lead to additional
overhead due to interrupt handling.

Currently, software path, vmx_deliver_posted_interrupt(), is optimized to
issue PINs only if target vCPU is in IN_GUEST_MODE. But hardware paths
(VT-d and Intel IPI virtualization) aren't optimized.

Set PID.SN right after VM exits and clear it before VM entry to minimize
the chance of hardware issuing PINs to a CPU when it's in host.

Also honour PID.SN bit in vmx_deliver_posted_interrupt().

When IPI virtualization is enabled, this patch increases "perf bench" [*]
by 4% from 8.12 us/ops to 7.80 us/ops.

[*] test cmd: perf bench sched pipe -T. Note that we change the source
code to pin two threads to two different vCPUs so that it can reproduce
stable results.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/kvm/vmx/posted_intr.c | 28 ++--------------------------
 arch/x86/kvm/vmx/vmx.c         | 24 +++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c
index 237a1f40f939..a0458f72df99 100644
--- a/arch/x86/kvm/vmx/posted_intr.c
+++ b/arch/x86/kvm/vmx/posted_intr.c
@@ -70,12 +70,6 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
 	 * needs to be changed.
 	 */
 	if (pi_desc->nv != POSTED_INTR_WAKEUP_VECTOR && vcpu->cpu == cpu) {
-		/*
-		 * Clear SN if it was set due to being preempted.  Again, do
-		 * this even if there is no assigned device for simplicity.
-		 */
-		if (pi_test_and_clear_sn(pi_desc))
-			goto after_clear_sn;
 		return;
 	}
 
@@ -99,12 +93,8 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
 	do {
 		old.control = new.control = READ_ONCE(pi_desc->control);
 
-		/*
-		 * Clear SN (as above) and refresh the destination APIC ID to
-		 * handle task migration (@cpu != vcpu->cpu).
-		 */
 		new.ndst = dest;
-		new.sn = 0;
+		new.sn = 1;
 
 		/*
 		 * Restore the notification vector; in the blocking case, the
@@ -114,19 +104,6 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
 	} while (pi_try_set_control(pi_desc, old.control, new.control));
 
 	local_irq_restore(flags);
-
-after_clear_sn:
-
-	/*
-	 * Clear SN before reading the bitmap.  The VT-d firmware
-	 * writes the bitmap and reads SN atomically (5.2.3 in the
-	 * spec), so it doesn't really have a memory barrier that
-	 * pairs with this, but we cannot do that and we need one.
-	 */
-	smp_mb__after_atomic();
-
-	if (!pi_is_pir_empty(pi_desc))
-		pi_set_on(pi_desc);
 }
 
 static bool vmx_can_use_vtd_pi(struct kvm *kvm)
@@ -154,13 +131,12 @@ static void pi_enable_wakeup_handler(struct kvm_vcpu *vcpu)
 		      &per_cpu(wakeup_vcpus_on_cpu, vcpu->cpu));
 	raw_spin_unlock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
 
-	WARN(pi_desc->sn, "PI descriptor SN field set before blocking");
-
 	do {
 		old.control = new.control = READ_ONCE(pi_desc->control);
 
 		/* set 'NV' to 'wakeup vector' */
 		new.nv = POSTED_INTR_WAKEUP_VECTOR;
+		new.sn = 0;
 	} while (pi_try_set_control(pi_desc, old.control, new.control));
 
 	/*
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index a3c5504601a8..fa915b1680eb 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4036,6 +4036,9 @@ static int vmx_deliver_posted_interrupt(struct kvm_vcpu *vcpu, int vector)
 	if (pi_test_and_set_pir(vector, &vmx->pi_desc))
 		return 0;
 
+	if (pi_test_sn(&vmx->pi_desc))
+		return 0;
+
 	/* If a previous notification has sent the IPI, nothing to do.  */
 	if (pi_test_and_set_on(&vmx->pi_desc))
 		return 0;
@@ -6520,8 +6523,17 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu)
 	if (KVM_BUG_ON(!enable_apicv, vcpu->kvm))
 		return -EIO;
 
-	if (pi_test_on(&vmx->pi_desc)) {
+	if (pi_test_on(&vmx->pi_desc) || pi_test_sn(&vmx->pi_desc)) {
 		pi_clear_on(&vmx->pi_desc);
+
+		/*
+		 * IN_GUEST_MODE means we are about to enter vCPU. Allow
+		 * PIN (posted interrupt notification) to deliver is key
+		 * to interrupt posting. Clear PID.SN.
+		 */
+		if (vcpu->mode == IN_GUEST_MODE)
+			pi_clear_sn(&vmx->pi_desc);
+
 		/*
 		 * IOMMU can write to PID.ON, so the barrier matters even on UP.
 		 * But on x86 this is just a compiler barrier anyway.
@@ -6976,6 +6988,16 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	/* The actual VMENTER/EXIT is in the .noinstr.text section. */
 	vmx_vcpu_enter_exit(vcpu, vmx);
 
+	/*
+	 * Suppress notification right after VM exits to minimize the
+	 * window where VT-d or remote CPU may send a useless notification
+	 * when posting interrupts to a VM. Note that the notification is
+	 * useless because KVM syncs pending interrupts in PID.IRR to vAPIC
+	 * IRR before VM entry.
+	 */
+	if (kvm_vcpu_apicv_active(vcpu))
+		pi_set_sn(&vmx->pi_desc);
+
 	/*
 	 * We do not use IBRS in the kernel. If this vCPU has used the
 	 * SPEC_CTRL MSR it may have left it on; save the value and
-- 
2.25.1

RE: [PATCH] KVM: x86/vmx: Suppress posted interrupt notification when CPU is in host

[Tian, Kevin]

> From: Chao Gao <chao.gao@intel.com>
> Sent: Friday, June 17, 2022 7:47 PM
> 
> PIN (Posted interrupt notification) is useless to host as KVM always syncs
> pending guest interrupts in PID to guest's vAPIC before each VM entry. In
> fact, Sending PINs to a CPU running in host will lead to additional
> overhead due to interrupt handling.
> 
> Currently, software path, vmx_deliver_posted_interrupt(), is optimized to
> issue PINs only if target vCPU is in IN_GUEST_MODE. But hardware paths
> (VT-d and Intel IPI virtualization) aren't optimized.
> 
> Set PID.SN right after VM exits and clear it before VM entry to minimize
> the chance of hardware issuing PINs to a CPU when it's in host.
> 
> Also honour PID.SN bit in vmx_deliver_posted_interrupt().
> 
> When IPI virtualization is enabled, this patch increases "perf bench" [*]
> by 4% from 8.12 us/ops to 7.80 us/ops.
> 
> [*] test cmd: perf bench sched pipe -T. Note that we change the source
> code to pin two threads to two different vCPUs so that it can reproduce
> stable results.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
>  arch/x86/kvm/vmx/posted_intr.c | 28 ++--------------------------
>  arch/x86/kvm/vmx/vmx.c         | 24 +++++++++++++++++++++++-
>  2 files changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/posted_intr.c
> b/arch/x86/kvm/vmx/posted_intr.c
> index 237a1f40f939..a0458f72df99 100644
> --- a/arch/x86/kvm/vmx/posted_intr.c
> +++ b/arch/x86/kvm/vmx/posted_intr.c
> @@ -70,12 +70,6 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int
> cpu)
>  	 * needs to be changed.
>  	 */
>  	if (pi_desc->nv != POSTED_INTR_WAKEUP_VECTOR && vcpu->cpu ==
> cpu) {
> -		/*
> -		 * Clear SN if it was set due to being preempted.  Again, do
> -		 * this even if there is no assigned device for simplicity.
> -		 */
> -		if (pi_test_and_clear_sn(pi_desc))
> -			goto after_clear_sn;
>  		return;
>  	}
> 
> @@ -99,12 +93,8 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int
> cpu)
>  	do {
>  		old.control = new.control = READ_ONCE(pi_desc->control);
> 
> -		/*
> -		 * Clear SN (as above) and refresh the destination APIC ID to
> -		 * handle task migration (@cpu != vcpu->cpu).
> -		 */
>  		new.ndst = dest;
> -		new.sn = 0;
> +		new.sn = 1;

A comment is appreciated.

> 
>  		/*
>  		 * Restore the notification vector; in the blocking case, the
> @@ -114,19 +104,6 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int
> cpu)
>  	} while (pi_try_set_control(pi_desc, old.control, new.control));
> 
>  	local_irq_restore(flags);
> -
> -after_clear_sn:
> -
> -	/*
> -	 * Clear SN before reading the bitmap.  The VT-d firmware
> -	 * writes the bitmap and reads SN atomically (5.2.3 in the
> -	 * spec), so it doesn't really have a memory barrier that
> -	 * pairs with this, but we cannot do that and we need one.
> -	 */
> -	smp_mb__after_atomic();
> -
> -	if (!pi_is_pir_empty(pi_desc))
> -		pi_set_on(pi_desc);
>  }
> 
>  static bool vmx_can_use_vtd_pi(struct kvm *kvm)
> @@ -154,13 +131,12 @@ static void pi_enable_wakeup_handler(struct
> kvm_vcpu *vcpu)
>  		      &per_cpu(wakeup_vcpus_on_cpu, vcpu->cpu));
>  	raw_spin_unlock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu-
> >cpu));
> 
> -	WARN(pi_desc->sn, "PI descriptor SN field set before blocking");
> -
>  	do {
>  		old.control = new.control = READ_ONCE(pi_desc->control);
> 
>  		/* set 'NV' to 'wakeup vector' */
>  		new.nv = POSTED_INTR_WAKEUP_VECTOR;
> +		new.sn = 0;
>  	} while (pi_try_set_control(pi_desc, old.control, new.control));
> 

there is a problem a few lines downwards:

	/*
	 * Send a wakeup IPI to this CPU if an interrupt may have been posted
	 * before the notification vector was updated, in which case the IRQ
	 * will arrive on the non-wakeup vector.  An IPI is needed as calling
	 * try_to_wake_up() from ->sched_out() isn't allowed (IRQs are not
	 * enabled until it is safe to call try_to_wake_up() on the task being
	 * scheduled out).
	 */
	if (pi_test_on(&new))
		apic->send_IPI_self(POSTED_INTR_WAKEUP_VECTOR);

'on' is not set when SN is set. This is different from original assumption
which has SN cleared in above window. In this case pi_test_on()
should be replaced with pi_is_pir_empty().

There is another simplification possible in vmx_vcpu_pi_put():

	if (kvm_vcpu_is_blocking(vcpu) && !vmx_interrupt_blocked(vcpu))
		pi_enable_wakeup_handler(vcpu);

	/*
	 * Set SN when the vCPU is preempted.  Note, the vCPU can both be seen
	 * as blocking and preempted, e.g. if it's preempted between setting
	 * its wait state and manually scheduling out.
	 */
	if (vcpu->preempted)
		pi_set_sn(pi_desc);

With this patch 'sn' is already set when a runnable vcpu is preempted
hence above is required only for a blocking vcpu. And in the
blocking case if the notification is anyway suppressed it's pointless to
further change the notification vector. Then it could be simplified as:

	if (!vcpu->preempted && kvm_vcpu_is_blocking(vcpu) &&
		!vmx_interrupt_blocked(vcpu))
		pi_enable_wakeup_handler(vcpu);

>  	/*
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index a3c5504601a8..fa915b1680eb 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4036,6 +4036,9 @@ static int vmx_deliver_posted_interrupt(struct
> kvm_vcpu *vcpu, int vector)
>  	if (pi_test_and_set_pir(vector, &vmx->pi_desc))
>  		return 0;
> 
> +	if (pi_test_sn(&vmx->pi_desc))
> +		return 0;
> +
>  	/* If a previous notification has sent the IPI, nothing to do.  */
>  	if (pi_test_and_set_on(&vmx->pi_desc))
>  		return 0;
> @@ -6520,8 +6523,17 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu
> *vcpu)
>  	if (KVM_BUG_ON(!enable_apicv, vcpu->kvm))
>  		return -EIO;
> 
> -	if (pi_test_on(&vmx->pi_desc)) {
> +	if (pi_test_on(&vmx->pi_desc) || pi_test_sn(&vmx->pi_desc)) {

this has potential side-effect in vmexit/vmentry path where pi_desc is
always scanned now. While reducing interrupts help the IPC test case,
do you observe any impact on other scenarios where interrupts are not
the main cause of vmexits?

>  		pi_clear_on(&vmx->pi_desc);
> +
> +		/*
> +		 * IN_GUEST_MODE means we are about to enter vCPU.
> Allow
> +		 * PIN (posted interrupt notification) to deliver is key
> +		 * to interrupt posting. Clear PID.SN.
> +		 */
> +		if (vcpu->mode == IN_GUEST_MODE)
> +			pi_clear_sn(&vmx->pi_desc);

I wonder whether it makes more sense to have 'sn' closely sync-ed
with vcpu->mode, e.g. having a kvm_x86_set_vcpu_mode() ops
to translate vcpu->mode into vmx/svm specific hardware bits like
'sn' here. Then call it in common place when vcpu->mode is changed. 

> +
>  		/*
>  		 * IOMMU can write to PID.ON, so the barrier matters even
> on UP.
>  		 * But on x86 this is just a compiler barrier anyway.
> @@ -6976,6 +6988,16 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu
> *vcpu)
>  	/* The actual VMENTER/EXIT is in the .noinstr.text section. */
>  	vmx_vcpu_enter_exit(vcpu, vmx);
> 
> +	/*
> +	 * Suppress notification right after VM exits to minimize the
> +	 * window where VT-d or remote CPU may send a useless notification
> +	 * when posting interrupts to a VM. Note that the notification is
> +	 * useless because KVM syncs pending interrupts in PID.IRR to vAPIC
> +	 * IRR before VM entry.
> +	 */
> +	if (kvm_vcpu_apicv_active(vcpu))
> +		pi_set_sn(&vmx->pi_desc);
> +
>  	/*
>  	 * We do not use IBRS in the kernel. If this vCPU has used the
>  	 * SPEC_CTRL MSR it may have left it on; save the value and
> --
> 2.25.1

Re: [PATCH] KVM: x86/vmx: Suppress posted interrupt notification when CPU is in host

[Chao Gao]

On Tue, Jun 21, 2022 at 02:59:10PM +0800, Tian, Kevin wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> Sent: Friday, June 17, 2022 7:47 PM
>>
>> PIN (Posted interrupt notification) is useless to host as KVM always syncs
>> pending guest interrupts in PID to guest's vAPIC before each VM entry. In
>> fact, Sending PINs to a CPU running in host will lead to additional
>> overhead due to interrupt handling.
>>
>> Currently, software path, vmx_deliver_posted_interrupt(), is optimized to
>> issue PINs only if target vCPU is in IN_GUEST_MODE. But hardware paths
>> (VT-d and Intel IPI virtualization) aren't optimized.
>>
>> Set PID.SN right after VM exits and clear it before VM entry to minimize
>> the chance of hardware issuing PINs to a CPU when it's in host.
>>
>> Also honour PID.SN bit in vmx_deliver_posted_interrupt().
>>
>> When IPI virtualization is enabled, this patch increases "perf bench" [*]
>> by 4% from 8.12 us/ops to 7.80 us/ops.
>>
>> [*] test cmd: perf bench sched pipe -T. Note that we change the source
>> code to pin two threads to two different vCPUs so that it can reproduce
>> stable results.
>>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>>  arch/x86/kvm/vmx/posted_intr.c | 28 ++--------------------------
>>  arch/x86/kvm/vmx/vmx.c         | 24 +++++++++++++++++++++++-
>>  2 files changed, 25 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/posted_intr.c
>> b/arch/x86/kvm/vmx/posted_intr.c
>> index 237a1f40f939..a0458f72df99 100644
>> --- a/arch/x86/kvm/vmx/posted_intr.c
>> +++ b/arch/x86/kvm/vmx/posted_intr.c
>> @@ -70,12 +70,6 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int
>> cpu)
>>        * needs to be changed.
>>        */
>>       if (pi_desc->nv != POSTED_INTR_WAKEUP_VECTOR && vcpu->cpu ==
>> cpu) {
>> -             /*
>> -              * Clear SN if it was set due to being preempted.  Again, do
>> -              * this even if there is no assigned device for simplicity.
>> -              */
>> -             if (pi_test_and_clear_sn(pi_desc))
>> -                     goto after_clear_sn;
>>               return;
>>       }
>>
>> @@ -99,12 +93,8 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int
>> cpu)
>>       do {
>>               old.control = new.control = READ_ONCE(pi_desc->control);
>>
>> -             /*
>> -              * Clear SN (as above) and refresh the destination APIC ID to
>> -              * handle task migration (@cpu != vcpu->cpu).
>> -              */
>>               new.ndst = dest;
>> -             new.sn = 0;
>> +             new.sn = 1;
>
>A comment is appreciated.

Will add a comment like:
/*
 * Set SN and refresh the destination APIC ID to handle task migration
 * (@cpu != vcpu->cpu).
 *
 * SN was cleared when a vCPU goes to blocked state so that KVM can wake
 * up the blocked vCPU on receving a notification. For a running/runnable
 * vCPU, such notifications are useless. Set SN bit to suppress them.
 */

>there is a problem a few lines downwards:
>
>        /*
>         * Send a wakeup IPI to this CPU if an interrupt may have been posted
>         * before the notification vector was updated, in which case the IRQ
>         * will arrive on the non-wakeup vector.  An IPI is needed as calling
>         * try_to_wake_up() from ->sched_out() isn't allowed (IRQs are not
>         * enabled until it is safe to call try_to_wake_up() on the task being
>         * scheduled out).
>         */
>        if (pi_test_on(&new))
>                apic->send_IPI_self(POSTED_INTR_WAKEUP_VECTOR);
>
>'on' is not set when SN is set. This is different from original assumption
>which has SN cleared in above window. In this case pi_test_on()
>should be replaced with pi_is_pir_empty().

Nice catch. Will fix it.

>
>There is another simplification possible in vmx_vcpu_pi_put():
>
>        if (kvm_vcpu_is_blocking(vcpu) && !vmx_interrupt_blocked(vcpu))
>                pi_enable_wakeup_handler(vcpu);
>
>        /*
>         * Set SN when the vCPU is preempted.  Note, the vCPU can both be seen
>         * as blocking and preempted, e.g. if it's preempted between setting
>         * its wait state and manually scheduling out.
>         */
>        if (vcpu->preempted)
>                pi_set_sn(pi_desc);
>
>With this patch 'sn' is already set when a runnable vcpu is preempted
>hence above is required only for a blocking vcpu. And in the

There are two corner cases to be fixed before we can assume this.

Currently, this patch clears SN bit in vmx_deliver_posted_interrupt().
If kvm decides to cancel vCPU entry (i.e., bail out) after one of two call sites
of it in vcpu_enter_guest(), SN is cleared for a running vCPU.
I was thinking a running vCPU with SN cleared won't have any functional
issue. So I didn't add a new static_call for SN bit and manipulate it
when KVM decides to cancel vCPU entry.

>blocking case if the notification is anyway suppressed it's pointless to
>further change the notification vector. Then it could be simplified as:
>
>        if (!vcpu->preempted && kvm_vcpu_is_blocking(vcpu) &&
>                !vmx_interrupt_blocked(vcpu))
>                pi_enable_wakeup_handler(vcpu);
>
>>       /*
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index a3c5504601a8..fa915b1680eb 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -4036,6 +4036,9 @@ static int vmx_deliver_posted_interrupt(struct
>> kvm_vcpu *vcpu, int vector)
>>       if (pi_test_and_set_pir(vector, &vmx->pi_desc))
>>               return 0;
>>
>> +     if (pi_test_sn(&vmx->pi_desc))
>> +             return 0;
>> +
>>       /* If a previous notification has sent the IPI, nothing to do.  */
>>       if (pi_test_and_set_on(&vmx->pi_desc))
>>               return 0;
>> @@ -6520,8 +6523,17 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu
>> *vcpu)
>>       if (KVM_BUG_ON(!enable_apicv, vcpu->kvm))
>>               return -EIO;
>>
>> -     if (pi_test_on(&vmx->pi_desc)) {
>> +     if (pi_test_on(&vmx->pi_desc) || pi_test_sn(&vmx->pi_desc)) {
>
>this has potential side-effect in vmexit/vmentry path where pi_desc is
>always scanned now. While reducing interrupts help the IPC test case,
>do you observe any impact on other scenarios where interrupts are not
>the main cause of vmexits?

Good question. I will run cpuid loop tests to measure the impact.
SN/ON/IRR are in the same cacheline. So, supposing the impact would be
negligible.

>
>>               pi_clear_on(&vmx->pi_desc);
>> +
>> +             /*
>> +              * IN_GUEST_MODE means we are about to enter vCPU.
>> Allow
>> +              * PIN (posted interrupt notification) to deliver is key
>> +              * to interrupt posting. Clear PID.SN.
>> +              */
>> +             if (vcpu->mode == IN_GUEST_MODE)
>> +                     pi_clear_sn(&vmx->pi_desc);
>
>I wonder whether it makes more sense to have 'sn' closely sync-ed
>with vcpu->mode, e.g. having a kvm_x86_set_vcpu_mode() ops
>to translate vcpu->mode into vmx/svm specific hardware bits like
>'sn' here. Then call it in common place when vcpu->mode is changed.

It makes sense to me because it can fix the two corner cases described above.