[PATCH 0/3] KVM: x86: Address performance degradation due to APICv inhibits

[PATCH 0/3] KVM: x86: Address performance degradation due to APICv inhibits

[Naveen N Rao (AMD)]

When AVIC is enabled (kvm_amd avic=1), guests with kernel-irqchip=on and 
PIT in reinject mode (default configuration) show degraded performance.  
This is because even though APICv is inhibited, we see other inhibits 
being set/cleared causing contention on apicv_update_lock. Rework 
inhibit code so that apicv_update_lock is not required unless APICv 
state is changing. More details in patch 3.

In a test setup with two guests (-smp 16, -netdev tap,vhost=on, -device 
virtio-net-pci,mq=on), multiple instances of netperf TCP_RR sending 
traffic to the other guest, sar shows:

Without this patch series, kvm_amd avic=0 (default):
Average:        IFACE   rxpck/s   txpck/s    rxkB/s    txkB/s   rxcmp/s   txcmp/s  rxmcst/s   %ifutil
Average:           lo      0.00      0.00      0.00      0.00      0.00      0.00      0.00      0.00
Average:         ens2 1323407.59 1323406.87 250722.47 126654.46      0.00      0.00      0.00      0.00

Without this patch series, kvm_amd avic=1:
Average:        IFACE   rxpck/s   txpck/s    rxkB/s    txkB/s   rxcmp/s   txcmp/s  rxmcst/s   %ifutil
Average:           lo      0.00      0.00      0.00      0.00      0.00      0.00      0.00      0.00
Average:         ens2 1057711.25 1057697.51 200385.83 101225.23      0.00      0.00      0.00      0.00

We see ~20% degradation in packet rate.

With this patch series, kvm_amd avic=1:
Average:        IFACE   rxpck/s   txpck/s    rxkB/s    txkB/s   rxcmp/s   txcmp/s  rxmcst/s   %ifutil
Average:           lo      0.00      0.00      0.00      0.00      0.00      0.00      0.00      0.00
Average:         ens2 1315433.91 1315434.46 249211.99 125891.47      0.00      0.00      0.00      0.00


- Naveen



Naveen N Rao (AMD) (3):
  KVM: x86: hyper-v: Convert synic_auto_eoi_used to an atomic
  KVM: x86: Remove use of apicv_update_lock when toggling guest debug
    state
  KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

 arch/x86/include/asm/kvm_host.h |  14 ++--
 arch/x86/kvm/hyperv.c           |  17 ++---
 arch/x86/kvm/x86.c              | 125 ++++++++++++++++----------------
 3 files changed, 72 insertions(+), 84 deletions(-)


base-commit: eb723766b1030a23c38adf2348b7c3d1409d11f0
-- 
2.48.1

[PATCH 1/3] KVM: x86: hyper-v: Convert synic_auto_eoi_used to an atomic

[Naveen N Rao (AMD)]

apicv_update_lock is primarily meant for protecting updates to the apicv
state, and is not necessary for guarding updates to synic_auto_eoi_used.
Convert synic_auto_eoi_used to an atomic and use
kvm_set_or_clear_apicv_inhibit() helper to simplify the logic.

Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
---
 arch/x86/include/asm/kvm_host.h |  7 ++-----
 arch/x86/kvm/hyperv.c           | 17 +++++------------
 2 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5193c3dfbce1..fb93563714c2 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1150,11 +1150,8 @@ struct kvm_hv {
 	/* How many vCPUs have VP index != vCPU index */
 	atomic_t num_mismatched_vp_indexes;
 
-	/*
-	 * How many SynICs use 'AutoEOI' feature
-	 * (protected by arch.apicv_update_lock)
-	 */
-	unsigned int synic_auto_eoi_used;
+	/* How many SynICs use 'AutoEOI' feature */
+	atomic_t synic_auto_eoi_used;
 
 	struct kvm_hv_syndbg hv_syndbg;
 
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 6a6dd5a84f22..7a4554ea1d16 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -131,25 +131,18 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
 	if (auto_eoi_old == auto_eoi_new)
 		return;
 
-	if (!enable_apicv)
-		return;
-
-	down_write(&vcpu->kvm->arch.apicv_update_lock);
-
 	if (auto_eoi_new)
-		hv->synic_auto_eoi_used++;
+		atomic_inc(&hv->synic_auto_eoi_used);
 	else
-		hv->synic_auto_eoi_used--;
+		atomic_dec(&hv->synic_auto_eoi_used);
 
 	/*
 	 * Inhibit APICv if any vCPU is using SynIC's AutoEOI, which relies on
 	 * the hypervisor to manually inject IRQs.
 	 */
-	__kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
-					 APICV_INHIBIT_REASON_HYPERV,
-					 !!hv->synic_auto_eoi_used);
-
-	up_write(&vcpu->kvm->arch.apicv_update_lock);
+	kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
+				       APICV_INHIBIT_REASON_HYPERV,
+				       !!atomic_read(&hv->synic_auto_eoi_used));
 }
 
 static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
-- 
2.48.1

Re: [PATCH 1/3] KVM: x86: hyper-v: Convert synic_auto_eoi_used to an atomic

[Maxim Levitsky]

On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
> apicv_update_lock is primarily meant for protecting updates to the apicv
> state, and is not necessary for guarding updates to synic_auto_eoi_used.
> Convert synic_auto_eoi_used to an atomic and use
> kvm_set_or_clear_apicv_inhibit() helper to simplify the logic.
> 
> Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
> ---
>  arch/x86/include/asm/kvm_host.h |  7 ++-----
>  arch/x86/kvm/hyperv.c           | 17 +++++------------
>  2 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 5193c3dfbce1..fb93563714c2 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1150,11 +1150,8 @@ struct kvm_hv {
>  	/* How many vCPUs have VP index != vCPU index */
>  	atomic_t num_mismatched_vp_indexes;
>  
> -	/*
> -	 * How many SynICs use 'AutoEOI' feature
> -	 * (protected by arch.apicv_update_lock)
> -	 */
> -	unsigned int synic_auto_eoi_used;
> +	/* How many SynICs use 'AutoEOI' feature */
> +	atomic_t synic_auto_eoi_used;
>  
>  	struct kvm_hv_syndbg hv_syndbg;
>  
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 6a6dd5a84f22..7a4554ea1d16 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -131,25 +131,18 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
>  	if (auto_eoi_old == auto_eoi_new)
>  		return;
>  
> -	if (!enable_apicv)
> -		return;
> -
> -	down_write(&vcpu->kvm->arch.apicv_update_lock);
> -
>  	if (auto_eoi_new)
> -		hv->synic_auto_eoi_used++;
> +		atomic_inc(&hv->synic_auto_eoi_used);
>  	else
> -		hv->synic_auto_eoi_used--;
> +		atomic_dec(&hv->synic_auto_eoi_used);
>  
>  	/*
>  	 * Inhibit APICv if any vCPU is using SynIC's AutoEOI, which relies on
>  	 * the hypervisor to manually inject IRQs.
>  	 */
> -	__kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
> -					 APICV_INHIBIT_REASON_HYPERV,
> -					 !!hv->synic_auto_eoi_used);
> -
> -	up_write(&vcpu->kvm->arch.apicv_update_lock);
> +	kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
> +				       APICV_INHIBIT_REASON_HYPERV,
> +				       !!atomic_read(&hv->synic_auto_eoi_used));

Hi,

This introduces a race, because there is a race window between
the moment we read hv->synic_auto_eoi_used, and decide to set/clear the inhibit.

After we read hv->synic_auto_eoi_used, but before we call the kvm_set_or_clear_apicv_inhibit,
other core might also run synic_update_vector and change hv->synic_auto_eoi_used, 
finish setting the inhibit in kvm_set_or_clear_apicv_inhibit,
and only then we will call kvm_set_or_clear_apicv_inhibit with the stale value of hv->synic_auto_eoi_used and clear it.

IMHO, knowing that this code is mostly a precaution and that modern windows doesn't use AutoEOI
(at least when AutoEOI deprecation bit is set), instead of counting, we can unconditionally
inhibit the APICv when the guest attempts to use AutoEOI once.
But as usual I won't be surprised that this breaks *some* old and/or odd windows versions.

Best regards,
	Maxim Levitsky





>  }
>  
>  static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,

Re: [PATCH 1/3] KVM: x86: hyper-v: Convert synic_auto_eoi_used to an atomic

[Naveen N Rao]

Hi Maxim,

On Mon, Feb 03, 2025 at 08:30:13PM -0500, Maxim Levitsky wrote:
> On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 6a6dd5a84f22..7a4554ea1d16 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -131,25 +131,18 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >  	if (auto_eoi_old == auto_eoi_new)
> >  		return;
> >  
> > -	if (!enable_apicv)
> > -		return;
> > -
> > -	down_write(&vcpu->kvm->arch.apicv_update_lock);
> > -
> >  	if (auto_eoi_new)
> > -		hv->synic_auto_eoi_used++;
> > +		atomic_inc(&hv->synic_auto_eoi_used);
> >  	else
> > -		hv->synic_auto_eoi_used--;
> > +		atomic_dec(&hv->synic_auto_eoi_used);
> >  
> >  	/*
> >  	 * Inhibit APICv if any vCPU is using SynIC's AutoEOI, which relies on
> >  	 * the hypervisor to manually inject IRQs.
> >  	 */
> > -	__kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
> > -					 APICV_INHIBIT_REASON_HYPERV,
> > -					 !!hv->synic_auto_eoi_used);
> > -
> > -	up_write(&vcpu->kvm->arch.apicv_update_lock);
> > +	kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
> > +				       APICV_INHIBIT_REASON_HYPERV,
> > +				       !!atomic_read(&hv->synic_auto_eoi_used));
> 
> Hi,
> 
> This introduces a race, because there is a race window between
> the moment we read hv->synic_auto_eoi_used, and decide to set/clear the inhibit.
> 
> After we read hv->synic_auto_eoi_used, but before we call the kvm_set_or_clear_apicv_inhibit,
> other core might also run synic_update_vector and change hv->synic_auto_eoi_used, 
> finish setting the inhibit in kvm_set_or_clear_apicv_inhibit,
> and only then we will call kvm_set_or_clear_apicv_inhibit with the stale value of hv->synic_auto_eoi_used and clear it.

Ah, indeed. Thanks for the explanation.

I wonder if we can switch to using kvm_hv->hv_lock in place of 
apicv_update_lock. That lock is already used to guard updates to 
partition-wide MSRs in kvm_hv_set_msr_common(). So, that might be ok 
too?


Thanks,
Naveen

Re: [PATCH 1/3] KVM: x86: hyper-v: Convert synic_auto_eoi_used to an atomic

[Sean Christopherson]

On Tue, Feb 04, 2025, Naveen N Rao wrote:
> Hi Maxim,
> 
> On Mon, Feb 03, 2025 at 08:30:13PM -0500, Maxim Levitsky wrote:
> > On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
> > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > > index 6a6dd5a84f22..7a4554ea1d16 100644
> > > --- a/arch/x86/kvm/hyperv.c
> > > +++ b/arch/x86/kvm/hyperv.c
> > > @@ -131,25 +131,18 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> > >  	if (auto_eoi_old == auto_eoi_new)
> > >  		return;
> > >  
> > > -	if (!enable_apicv)
> > > -		return;
> > > -
> > > -	down_write(&vcpu->kvm->arch.apicv_update_lock);
> > > -
> > >  	if (auto_eoi_new)
> > > -		hv->synic_auto_eoi_used++;
> > > +		atomic_inc(&hv->synic_auto_eoi_used);
> > >  	else
> > > -		hv->synic_auto_eoi_used--;
> > > +		atomic_dec(&hv->synic_auto_eoi_used);
> > >  
> > >  	/*
> > >  	 * Inhibit APICv if any vCPU is using SynIC's AutoEOI, which relies on
> > >  	 * the hypervisor to manually inject IRQs.
> > >  	 */
> > > -	__kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
> > > -					 APICV_INHIBIT_REASON_HYPERV,
> > > -					 !!hv->synic_auto_eoi_used);
> > > -
> > > -	up_write(&vcpu->kvm->arch.apicv_update_lock);
> > > +	kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
> > > +				       APICV_INHIBIT_REASON_HYPERV,
> > > +				       !!atomic_read(&hv->synic_auto_eoi_used));
> > 
> > Hi,
> > 
> > This introduces a race, because there is a race window between the moment
> > we read hv->synic_auto_eoi_used, and decide to set/clear the inhibit.
> > 
> > After we read hv->synic_auto_eoi_used, but before we call the
> > kvm_set_or_clear_apicv_inhibit, other core might also run
> > synic_update_vector and change hv->synic_auto_eoi_used, finish setting the
> > inhibit in kvm_set_or_clear_apicv_inhibit, and only then we will call
> > kvm_set_or_clear_apicv_inhibit with the stale value of
> > hv->synic_auto_eoi_used and clear it.
> 
> Ah, indeed. Thanks for the explanation.
> 
> I wonder if we can switch to using kvm_hv->hv_lock in place of 
> apicv_update_lock. That lock is already used to guard updates to 
> partition-wide MSRs in kvm_hv_set_msr_common(). So, that might be ok 
> too?

Why?  All that would do is add complexity (taking two locks, or ensuring there
is no race when juggling locks), because if the guest is actually toggling AutoEOI
at a meaningful rate on multiple vCPUs, then there is going to be lock contention
regardless of which lock is taken.

Re: [PATCH 1/3] KVM: x86: hyper-v: Convert synic_auto_eoi_used to an atomic

[Naveen N Rao]

On Tue, Feb 04, 2025 at 11:33:01AM -0800, Sean Christopherson wrote:
> On Tue, Feb 04, 2025, Naveen N Rao wrote:
> > Hi Maxim,
> > 
> > On Mon, Feb 03, 2025 at 08:30:13PM -0500, Maxim Levitsky wrote:
> > > On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
> > > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > > > index 6a6dd5a84f22..7a4554ea1d16 100644
> > > > --- a/arch/x86/kvm/hyperv.c
> > > > +++ b/arch/x86/kvm/hyperv.c
> > > > @@ -131,25 +131,18 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> > > >  	if (auto_eoi_old == auto_eoi_new)
> > > >  		return;
> > > >  
> > > > -	if (!enable_apicv)
> > > > -		return;
> > > > -
> > > > -	down_write(&vcpu->kvm->arch.apicv_update_lock);
> > > > -
> > > >  	if (auto_eoi_new)
> > > > -		hv->synic_auto_eoi_used++;
> > > > +		atomic_inc(&hv->synic_auto_eoi_used);
> > > >  	else
> > > > -		hv->synic_auto_eoi_used--;
> > > > +		atomic_dec(&hv->synic_auto_eoi_used);
> > > >  
> > > >  	/*
> > > >  	 * Inhibit APICv if any vCPU is using SynIC's AutoEOI, which relies on
> > > >  	 * the hypervisor to manually inject IRQs.
> > > >  	 */
> > > > -	__kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
> > > > -					 APICV_INHIBIT_REASON_HYPERV,
> > > > -					 !!hv->synic_auto_eoi_used);
> > > > -
> > > > -	up_write(&vcpu->kvm->arch.apicv_update_lock);
> > > > +	kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
> > > > +				       APICV_INHIBIT_REASON_HYPERV,
> > > > +				       !!atomic_read(&hv->synic_auto_eoi_used));
> > > 
> > > Hi,
> > > 
> > > This introduces a race, because there is a race window between the moment
> > > we read hv->synic_auto_eoi_used, and decide to set/clear the inhibit.
> > > 
> > > After we read hv->synic_auto_eoi_used, but before we call the
> > > kvm_set_or_clear_apicv_inhibit, other core might also run
> > > synic_update_vector and change hv->synic_auto_eoi_used, finish setting the
> > > inhibit in kvm_set_or_clear_apicv_inhibit, and only then we will call
> > > kvm_set_or_clear_apicv_inhibit with the stale value of
> > > hv->synic_auto_eoi_used and clear it.
> > 
> > Ah, indeed. Thanks for the explanation.
> > 
> > I wonder if we can switch to using kvm_hv->hv_lock in place of 
> > apicv_update_lock. That lock is already used to guard updates to 
> > partition-wide MSRs in kvm_hv_set_msr_common(). So, that might be ok 
> > too?
> 
> Why?  All that would do is add complexity (taking two locks, or ensuring there
> is no race when juggling locks), because if the guest is actually 
> toggling AutoEOI
> at a meaningful rate on multiple vCPUs, then there is going to be lock contention
> regardless of which lock is taken.

Yes, indeed.

The rationale for switching to a different lock was to address the 
original goal with this patch, which is to restrict use of 
apicv_update_lock to only toggling the APICv state. But, that is only 
relevant if we want to attempt that.

I do see why hv_lock won't work in this scenario though, so yes, we 
either need to retain use of apicv_update_lock, or introduce a new mutex 
for protecting updates to synic_auto_eoi_used.


Thanks,
Naveen

[PATCH 2/3] KVM: x86: Remove use of apicv_update_lock when toggling guest debug state

[Naveen N Rao (AMD)]

apicv_update_lock is not required when querying the state of guest
debug in all the vcpus. Remove usage of the same, and switch to
kvm_set_or_clear_apicv_inhibit() helper to simplify the code.

Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
---
 arch/x86/kvm/x86.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b2d9a16fd4d3..11235e91ae90 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12038,19 +12038,14 @@ static void kvm_arch_vcpu_guestdbg_update_apicv_inhibit(struct kvm *kvm)
 	struct kvm_vcpu *vcpu;
 	unsigned long i;
 
-	if (!enable_apicv)
-		return;
-
-	down_write(&kvm->arch.apicv_update_lock);
-
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		if (vcpu->guest_debug & KVM_GUESTDBG_BLOCKIRQ) {
 			set = true;
 			break;
 		}
 	}
-	__kvm_set_or_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_BLOCKIRQ, set);
-	up_write(&kvm->arch.apicv_update_lock);
+
+	kvm_set_or_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_BLOCKIRQ, set);
 }
 
 int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
-- 
2.48.1

Re: [PATCH 2/3] KVM: x86: Remove use of apicv_update_lock when toggling guest debug state

[Maxim Levitsky]

On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
> apicv_update_lock is not required when querying the state of guest
> debug in all the vcpus. Remove usage of the same, and switch to
> kvm_set_or_clear_apicv_inhibit() helper to simplify the code.

It might be worth to mention that the reason why the lock is not needed,
is because kvm_vcpu_ioctl from which this function is called takes 'vcpu->mutex'
and thus concurrent execution of this function is not really possible.

Besides this:

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

> 
> Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
> ---
>  arch/x86/kvm/x86.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b2d9a16fd4d3..11235e91ae90 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12038,19 +12038,14 @@ static void kvm_arch_vcpu_guestdbg_update_apicv_inhibit(struct kvm *kvm)
>  	struct kvm_vcpu *vcpu;
>  	unsigned long i;
>  
> -	if (!enable_apicv)
> -		return;
> -
> -	down_write(&kvm->arch.apicv_update_lock);
> -
>  	kvm_for_each_vcpu(i, vcpu, kvm) {
>  		if (vcpu->guest_debug & KVM_GUESTDBG_BLOCKIRQ) {
>  			set = true;
>  			break;
>  		}
>  	}
> -	__kvm_set_or_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_BLOCKIRQ, set);
> -	up_write(&kvm->arch.apicv_update_lock);
> +
> +	kvm_set_or_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_BLOCKIRQ, set);
>  }
>  
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,

Re: [PATCH 2/3] KVM: x86: Remove use of apicv_update_lock when toggling guest debug state

[Naveen N Rao]

On Mon, Feb 03, 2025 at 09:00:05PM -0500, Maxim Levitsky wrote:
> On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
> > apicv_update_lock is not required when querying the state of guest
> > debug in all the vcpus. Remove usage of the same, and switch to
> > kvm_set_or_clear_apicv_inhibit() helper to simplify the code.
> 
> It might be worth to mention that the reason why the lock is not needed,
> is because kvm_vcpu_ioctl from which this function is called takes 'vcpu->mutex'
> and thus concurrent execution of this function is not really possible.
> 
> Besides this:
> 
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Sure, thanks for the review!


- Naveen

Re: [PATCH 2/3] KVM: x86: Remove use of apicv_update_lock when toggling guest debug state

[Naveen N Rao]

On Mon, Feb 03, 2025 at 09:00:05PM -0500, Maxim Levitsky wrote:
> On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
> > apicv_update_lock is not required when querying the state of guest
> > debug in all the vcpus. Remove usage of the same, and switch to
> > kvm_set_or_clear_apicv_inhibit() helper to simplify the code.
> 
> It might be worth to mention that the reason why the lock is not needed,
> is because kvm_vcpu_ioctl from which this function is called takes 'vcpu->mutex'
> and thus concurrent execution of this function is not really possible.

Looking at this again, that looks to be a vcpu-specific lock, so I guess 
it is possible for multiple vcpus to run this concurrently?

In reality, this looks to be coming in from a vcpu ioctl from userspace, 
so this is probably not being invoked concurrently today.

Regardless, I wonder if moving this to a per-vcpu inhibit might be a 
better way to address this.


Thanks,
Naveen

Re: [PATCH 2/3] KVM: x86: Remove use of apicv_update_lock when toggling guest debug state

[Sean Christopherson]

On Tue, Feb 04, 2025, Naveen N Rao wrote:
> On Mon, Feb 03, 2025 at 09:00:05PM -0500, Maxim Levitsky wrote:
> > On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
> > > apicv_update_lock is not required when querying the state of guest
> > > debug in all the vcpus. Remove usage of the same, and switch to
> > > kvm_set_or_clear_apicv_inhibit() helper to simplify the code.
> > 
> > It might be worth to mention that the reason why the lock is not needed,
> > is because kvm_vcpu_ioctl from which this function is called takes 'vcpu->mutex'
> > and thus concurrent execution of this function is not really possible.
> 
> Looking at this again, that looks to be a vcpu-specific lock, so I guess 
> it is possible for multiple vcpus to run this concurrently?

Correct.

> In reality, this looks to be coming in from a vcpu ioctl from userspace, 
> so this is probably not being invoked concurrently today.
> 
> Regardless, I wonder if moving this to a per-vcpu inhibit might be a 
> better way to address this.

No, this is a slow path.

Re: [PATCH 2/3] KVM: x86: Remove use of apicv_update_lock when toggling guest debug state

[Paolo Bonzini]

On 2/4/25 18:51, Sean Christopherson wrote:
> On Tue, Feb 04, 2025, Naveen N Rao wrote:
>> On Mon, Feb 03, 2025 at 09:00:05PM -0500, Maxim Levitsky wrote:
>>> On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
>>>> apicv_update_lock is not required when querying the state of guest
>>>> debug in all the vcpus. Remove usage of the same, and switch to
>>>> kvm_set_or_clear_apicv_inhibit() helper to simplify the code.
>>>
>>> It might be worth to mention that the reason why the lock is not needed,
>>> is because kvm_vcpu_ioctl from which this function is called takes 'vcpu->mutex'
>>> and thus concurrent execution of this function is not really possible.
>>
>> Looking at this again, that looks to be a vcpu-specific lock, so I guess
>> it is possible for multiple vcpus to run this concurrently?
> 
> Correct.

And this patch is incorrect. Because there is a store and many loads, 
you have the typical race when two vCPUs set blockirq at the same time

	vcpu 0				vcpu 1
	---------------			--------------
	set vcpu0->guest_debug
					clear vcpu1->guest_debug
	read vcpu0->guest_debug
	read vcpu1->guest_debug	
	set inhibit
					read stale vcpu0->guest_debug
					read vcpu1->guest_debug
					clear inhibit

But since this is really a slow path, why even bother optimizing it?

Paolo

Re: [PATCH 2/3] KVM: x86: Remove use of apicv_update_lock when toggling guest debug state

[Maxim Levitsky]

On Tue, 2025-02-04 at 18:58 +0100, Paolo Bonzini wrote:
> On 2/4/25 18:51, Sean Christopherson wrote:
> > On Tue, Feb 04, 2025, Naveen N Rao wrote:
> > > On Mon, Feb 03, 2025 at 09:00:05PM -0500, Maxim Levitsky wrote:
> > > > On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
> > > > > apicv_update_lock is not required when querying the state of guest
> > > > > debug in all the vcpus. Remove usage of the same, and switch to
> > > > > kvm_set_or_clear_apicv_inhibit() helper to simplify the code.
> > > > 
> > > > It might be worth to mention that the reason why the lock is not needed,
> > > > is because kvm_vcpu_ioctl from which this function is called takes 'vcpu->mutex'
> > > > and thus concurrent execution of this function is not really possible.
> > > 
> > > Looking at this again, that looks to be a vcpu-specific lock, so I guess
> > > it is possible for multiple vcpus to run this concurrently?
> > 
> > Correct.
> 
> And this patch is incorrect. Because there is a store and many loads, 
> you have the typical race when two vCPUs set blockirq at the same time
> 
> 	vcpu 0				vcpu 1
> 	---------------			--------------
> 	set vcpu0->guest_debug
> 					clear vcpu1->guest_debug
> 	read vcpu0->guest_debug
> 	read vcpu1->guest_debug	
> 	set inhibit
> 					read stale vcpu0->guest_debug
> 					read vcpu1->guest_debug
> 					clear inhibit
> 
> But since this is really a slow path, why even bother optimizing it?
> 
> Paolo
> 


Paolo, you are absolutely right! the vcpu mutex only prevents concurrent ioctl
on a same vcpu, but not on different vcpus, and without locking of course
this patch isn't going to work. The per-vcpu mutex is not something I know well,
and I only recently made aware of it, so I mixed this thing up.

So yes, some kind of lock is needed here.

Best regards,
     Maxim Levitsky

Re: [PATCH 2/3] KVM: x86: Remove use of apicv_update_lock when toggling guest debug state

[Naveen N Rao]

On Tue, Feb 04, 2025 at 09:51:22AM -0800, Sean Christopherson wrote:
> On Tue, Feb 04, 2025, Naveen N Rao wrote:
> > On Mon, Feb 03, 2025 at 09:00:05PM -0500, Maxim Levitsky wrote:
> > > On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
> > > > apicv_update_lock is not required when querying the state of guest
> > > > debug in all the vcpus. Remove usage of the same, and switch to
> > > > kvm_set_or_clear_apicv_inhibit() helper to simplify the code.
> > > 
> > > It might be worth to mention that the reason why the lock is not needed,
> > > is because kvm_vcpu_ioctl from which this function is called takes 'vcpu->mutex'
> > > and thus concurrent execution of this function is not really possible.
> > 
> > Looking at this again, that looks to be a vcpu-specific lock, so I guess 
> > it is possible for multiple vcpus to run this concurrently?
> 
> Correct.
> 
> > In reality, this looks to be coming in from a vcpu ioctl from userspace, 
> > so this is probably not being invoked concurrently today.
> > 
> > Regardless, I wonder if moving this to a per-vcpu inhibit might be a 
> > better way to address this.
> 
> No, this is a slow path.

My comment was more from the point of view of correctness, rather than 
performance (with the goal of removing use of apicv_update_lock) -- 
similar to the issue with IRQWIN needing to maintain per-vcpu state. My 
naive understanding of Maxim's mail was that we would introduce per-vcpu 
inhibit field to maintain per-vcpu inhibit state, but not actually 
inhibit AVIC on a per-vcpu basis :)


Thanks,
Naveen

[PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Naveen N Rao (AMD)]

apicv_inhibit_reasons is used to determine if APICv is active, and if
not, the reason(s) why it may be inhibited. In some scenarios, inhibit
reasons can be set and cleared often, resulting in increased contention
on apicv_update_lock used to guard updates to apicv_inhibit_reasons.

In particular, if a guest is using PIT in reinject mode (the default)
and if AVIC is enabled in kvm_amd kernel module, we inhibit AVIC during
kernel PIT creation (APICV_INHIBIT_REASON_PIT_REINJ), resulting in KVM
emulating x2APIC for the guest. In that case, since AVIC is enabled in
the kvm_amd kernel module, KVM additionally inhibits AVIC for requesting
a IRQ window every time it has to inject external interrupts resulting
in a barrage of inhibits being set and cleared. This shows significant
performance degradation compared to AVIC being disabled, due to high
contention on apicv_update_lock.

Though apicv_update_lock is being used to guard updates to
apicv_inhibit_reasons, it is only necessary if the APICv activation
state changes. Introduce a separate boolean, apicv_activated, to track
if APICv is active or not, and limit use of apicv_update_lock for when
APICv is being (de)activated. Convert apicv_inhibit_reasons to an atomic
and use atomic operations to fetch/update it.

Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
---
 arch/x86/include/asm/kvm_host.h |   7 +-
 arch/x86/kvm/x86.c              | 116 +++++++++++++++++---------------
 2 files changed, 63 insertions(+), 60 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index fb93563714c2..bc4fb3c9d54c 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1359,9 +1359,10 @@ struct kvm_arch {
 	bool apic_access_memslot_enabled;
 	bool apic_access_memslot_inhibited;
 
-	/* Protects apicv_inhibit_reasons */
+	bool apicv_activated;
+	/* Protects apicv_activated */
 	struct rw_semaphore apicv_update_lock;
-	unsigned long apicv_inhibit_reasons;
+	atomic_t apicv_inhibit_reasons;
 
 	gpa_t wall_clock;
 
@@ -2183,8 +2184,6 @@ gpa_t kvm_mmu_gva_to_gpa_system(struct kvm_vcpu *vcpu, gva_t gva,
 bool kvm_apicv_activated(struct kvm *kvm);
 bool kvm_vcpu_apicv_activated(struct kvm_vcpu *vcpu);
 void __kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu);
-void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
-				      enum kvm_apicv_inhibit reason, bool set);
 void kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
 				    enum kvm_apicv_inhibit reason, bool set);
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 11235e91ae90..6c8f9a9d6548 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9917,33 +9917,42 @@ static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
 
 bool kvm_apicv_activated(struct kvm *kvm)
 {
-	return (READ_ONCE(kvm->arch.apicv_inhibit_reasons) == 0);
+	return READ_ONCE(kvm->arch.apicv_activated);
 }
 EXPORT_SYMBOL_GPL(kvm_apicv_activated);
 
 bool kvm_vcpu_apicv_activated(struct kvm_vcpu *vcpu)
 {
-	ulong vm_reasons = READ_ONCE(vcpu->kvm->arch.apicv_inhibit_reasons);
+	ulong vm_apicv_activated = READ_ONCE(vcpu->kvm->arch.apicv_activated);
 	ulong vcpu_reasons =
 			kvm_x86_call(vcpu_get_apicv_inhibit_reasons)(vcpu);
 
-	return (vm_reasons | vcpu_reasons) == 0;
+	return vm_apicv_activated && vcpu_reasons == 0;
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_apicv_activated);
 
-static void set_or_clear_apicv_inhibit(unsigned long *inhibits,
-				       enum kvm_apicv_inhibit reason, bool set)
+static unsigned long set_or_clear_apicv_inhibit(atomic_t *inhibits, enum kvm_apicv_inhibit reason,
+						bool set, unsigned long *new_inhibits)
 {
 	const struct trace_print_flags apicv_inhibits[] = { APICV_INHIBIT_REASONS };
+	unsigned long old, new;
 
 	BUILD_BUG_ON(ARRAY_SIZE(apicv_inhibits) != NR_APICV_INHIBIT_REASONS);
 
-	if (set)
-		__set_bit(reason, inhibits);
-	else
-		__clear_bit(reason, inhibits);
+	if (set) {
+		old = new = atomic_fetch_or(BIT(reason), inhibits);
+		__set_bit(reason, &new);
+	} else {
+		old = new = atomic_fetch_andnot(BIT(reason), inhibits);
+		__clear_bit(reason, &new);
+	}
 
-	trace_kvm_apicv_inhibit_changed(reason, set, *inhibits);
+	trace_kvm_apicv_inhibit_changed(reason, set, new);
+
+	if (new_inhibits)
+		*new_inhibits = new;
+
+	return old;
 }
 
 static void kvm_apicv_init(struct kvm *kvm)
@@ -9951,7 +9960,7 @@ static void kvm_apicv_init(struct kvm *kvm)
 	enum kvm_apicv_inhibit reason = enable_apicv ? APICV_INHIBIT_REASON_ABSENT :
 						       APICV_INHIBIT_REASON_DISABLED;
 
-	set_or_clear_apicv_inhibit(&kvm->arch.apicv_inhibit_reasons, reason, true);
+	set_or_clear_apicv_inhibit(&kvm->arch.apicv_inhibit_reasons, reason, true, NULL);
 
 	init_rwsem(&kvm->arch.apicv_update_lock);
 }
@@ -10592,56 +10601,51 @@ static void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
 	__kvm_vcpu_update_apicv(vcpu);
 }
 
-void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
-				      enum kvm_apicv_inhibit reason, bool set)
-{
-	unsigned long old, new;
-
-	lockdep_assert_held_write(&kvm->arch.apicv_update_lock);
-
-	if (!(kvm_x86_ops.required_apicv_inhibits & BIT(reason)))
-		return;
-
-	old = new = kvm->arch.apicv_inhibit_reasons;
-
-	set_or_clear_apicv_inhibit(&new, reason, set);
-
-	if (!!old != !!new) {
-		/*
-		 * Kick all vCPUs before setting apicv_inhibit_reasons to avoid
-		 * false positives in the sanity check WARN in vcpu_enter_guest().
-		 * This task will wait for all vCPUs to ack the kick IRQ before
-		 * updating apicv_inhibit_reasons, and all other vCPUs will
-		 * block on acquiring apicv_update_lock so that vCPUs can't
-		 * redo vcpu_enter_guest() without seeing the new inhibit state.
-		 *
-		 * Note, holding apicv_update_lock and taking it in the read
-		 * side (handling the request) also prevents other vCPUs from
-		 * servicing the request with a stale apicv_inhibit_reasons.
-		 */
-		kvm_make_all_cpus_request(kvm, KVM_REQ_APICV_UPDATE);
-		kvm->arch.apicv_inhibit_reasons = new;
-		if (new) {
-			unsigned long gfn = gpa_to_gfn(APIC_DEFAULT_PHYS_BASE);
-			int idx = srcu_read_lock(&kvm->srcu);
-
-			kvm_zap_gfn_range(kvm, gfn, gfn+1);
-			srcu_read_unlock(&kvm->srcu, idx);
-		}
-	} else {
-		kvm->arch.apicv_inhibit_reasons = new;
-	}
-}
-
 void kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
 				    enum kvm_apicv_inhibit reason, bool set)
 {
-	if (!enable_apicv)
+	unsigned long old, new;
+
+	if (!enable_apicv || !(kvm_x86_ops.required_apicv_inhibits & BIT(reason)))
 		return;
 
-	down_write(&kvm->arch.apicv_update_lock);
-	__kvm_set_or_clear_apicv_inhibit(kvm, reason, set);
-	up_write(&kvm->arch.apicv_update_lock);
+	old = set_or_clear_apicv_inhibit(&kvm->arch.apicv_inhibit_reasons, reason, set, &new);
+
+	if (!old != !new) {
+		down_write(&kvm->arch.apicv_update_lock);
+
+		/*
+		 * Someone else may have updated the inhibit reason and the flag
+		 * between when we do the update above and take the lock. Confirm
+		 * the state change needed before proceeding.
+		 */
+		new = atomic_read(&kvm->arch.apicv_inhibit_reasons);
+		if (!new != kvm->arch.apicv_activated) {
+			/*
+			 * Kick all vCPUs before setting apicv_activated to avoid false
+			 * positives in the sanity check WARN in vcpu_enter_guest().
+			 * This task will wait for all vCPUs to ack the kick IRQ before
+			 * updating apicv_activated, and all other vCPUs will block on
+			 * acquiring apicv_update_lock so that vCPUs can't redo
+			 * vcpu_enter_guest() without seeing the new inhibit state.
+			 *
+			 * Note, holding apicv_update_lock and taking it in the read side
+			 * (handling the request) also prevents other vCPUs from servicing
+			 * the request with a stale apicv_activated value.
+			 */
+			kvm_make_all_cpus_request(kvm, KVM_REQ_APICV_UPDATE);
+			kvm->arch.apicv_activated = !new;
+			if (new) {
+				unsigned long gfn = gpa_to_gfn(APIC_DEFAULT_PHYS_BASE);
+				int idx = srcu_read_lock(&kvm->srcu);
+
+				kvm_zap_gfn_range(kvm, gfn, gfn+1);
+				srcu_read_unlock(&kvm->srcu, idx);
+			}
+		}
+
+		up_write(&kvm->arch.apicv_update_lock);
+	}
 }
 EXPORT_SYMBOL_GPL(kvm_set_or_clear_apicv_inhibit);
 
-- 
2.48.1

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Sean Christopherson]

On Mon, Feb 03, 2025, Naveen N Rao (AMD) wrote:
> apicv_inhibit_reasons is used to determine if APICv is active, and if
> not, the reason(s) why it may be inhibited. In some scenarios, inhibit
> reasons can be set and cleared often, resulting in increased contention
> on apicv_update_lock used to guard updates to apicv_inhibit_reasons.
> 
> In particular, if a guest is using PIT in reinject mode (the default)
> and if AVIC is enabled in kvm_amd kernel module, we inhibit AVIC during
> kernel PIT creation (APICV_INHIBIT_REASON_PIT_REINJ), resulting in KVM
> emulating x2APIC for the guest. In that case, since AVIC is enabled in
> the kvm_amd kernel module, KVM additionally inhibits AVIC for requesting
> a IRQ window every time it has to inject external interrupts resulting
> in a barrage of inhibits being set and cleared. This shows significant
> performance degradation compared to AVIC being disabled, due to high
> contention on apicv_update_lock.
> 
> Though apicv_update_lock is being used to guard updates to
> apicv_inhibit_reasons, it is only necessary if the APICv activation
> state changes. Introduce a separate boolean, apicv_activated, to track
> if APICv is active or not, and limit use of apicv_update_lock for when
> APICv is being (de)activated. Convert apicv_inhibit_reasons to an atomic
> and use atomic operations to fetch/update it.

It's a moot point, but there is too much going on in this patch.  For a change
like this, it should be split into at ~4 patches:

 1. Add an kvm_x86_ops.required_apicv_inhibits check outside outside of the lock,
    which we can and should do anyways.  I would probably vote to turn the one
    inside the lock into a WARN, as that "optimized" path is only an optimization
    if the inhibit applies to both SVM and VMX.
 2. Use a bool to track the activated state.
 3. Use an atomic.
 4. Implement the optimized updates.

#3 is optional, #1 and #2 are not.

It's a moot point though, because toggling APICv inhibits on IRQ windows is
laughably broken.  I'm guessing it "works" because the only time it triggers in
practice is in conjunction with PIT re-injection.

 1. vCPU0 waits for an IRQ window, APICV_INHIBIT_REASON_IRQWIN = 1
 2. vCPU1 waits for an IRQ window, APICV_INHIBIT_REASON_IRQWIN = 1
 3. vCPU1 gets its IRQ window, APICV_INHIBIT_REASON_IRQWIN = 0
 4. vCPU0 is sad

APICV_INHIBIT_REASON_BLOCKIRQ is also tied to per-vCPU state, but is a-ok because
KVM walks all vCPUs to generate inhibit.  That approach won't work for IRQ windows
though, because it's obviously not a slow path.

What is generating so many external interrupts?  E.g. is the guest using the PIT
for TSC or APIC calibration?  I suppose it doesn't matter terribly in this case
since APICV_INHIBIT_REASON_PIT_REINJ is already set.

Unless there's a very, very good reason to support a use case that generates
ExtInts during boot, but _only_ during boot, and otherwise doesn't have any APICv
ihibits, I'm leaning towards making SVM's IRQ window inhibit sticky, i.e. never
clear it.  And then we can more simply optimize kvm_set_or_clear_apicv_inhibit()
to do nothing if the inhibit is sticky and already set.

E.g. something like this:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6d4a6734b2d6..6f926fd6fc1c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10629,6 +10629,14 @@ void kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
        if (!enable_apicv)
                return;
 
+       if (kvm_is_apicv_inhibit_sticky(reason)) {
+               if (WARN_ON_ONCE(!set))
+                       return;
+
+               if (kvm_test_apicv_inhibit(reason))
+                       return;
+       }
+
        down_write(&kvm->arch.apicv_update_lock);
        __kvm_set_or_clear_apicv_inhibit(kvm, reason, set);
        up_write(&kvm->arch.apicv_update_lock);

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Paolo Bonzini]

On 2/3/25 19:45, Sean Christopherson wrote:
> Unless there's a very, very good reason to support a use case that generates
> ExtInts during boot, but _only_ during boot, and otherwise doesn't have any APICv
> ihibits, I'm leaning towards making SVM's IRQ window inhibit sticky, i.e. never
> clear it.

BIOS tends to use PIT, so that may be too much.  With respect to Naveen's report
of contention on apicv_update_lock, I would go with the sticky-bit idea but apply
it to APICV_INHIBIT_REASON_PIT_REINJ.

Plus, to avoid crazy ExtINT configurations, something like this:

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 3ec6197b1386..3e358d55b676 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1295,6 +1295,11 @@ enum kvm_apicv_inhibit {
  	 */
  	APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED,
  
+	/*
+	 * AVIC is disabled because more than one vCPU has extint unmasked
+	 */
+	APICV_INHIBIT_REASON_EXTINT,
+
  	/*********************************************************/
  	/* INHIBITs that are relevant only to the Intel's APICv. */
  	/*********************************************************/
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 71544b0f6301..33a5f4ef42bd 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -377,6 +377,7 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
  	struct kvm_apic_map *new, *old = NULL;
  	struct kvm_vcpu *vcpu;
  	unsigned long i;
+	int extint_cnt = 0;
  	u32 max_id = 255; /* enough space for any xAPIC ID */
  	bool xapic_id_mismatch;
  	int r;
@@ -432,6 +433,8 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
  		if (!kvm_apic_present(vcpu))
  			continue;
  
+		extint_cnt += kvm_apic_accept_pic_intr(vcpu);
+
  		r = kvm_recalculate_phys_map(new, vcpu, &xapic_id_mismatch);
  		if (r) {
  			kvfree(new);
@@ -457,6 +460,11 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
  	else
  		kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED);
  
+	if (extint_cnt > 1)
+		kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_EXTINT);
+	else
+		kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_EXTINT);
+
  	if (!new || new->logical_mode == KVM_APIC_MODE_MAP_DISABLED)
  		kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED);
  	else
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 57ff79bc02a4..ba2fc7dd8ca2 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -676,6 +676,7 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
  	BIT(APICV_INHIBIT_REASON_HYPERV) |		\
  	BIT(APICV_INHIBIT_REASON_NESTED) |		\
  	BIT(APICV_INHIBIT_REASON_IRQWIN) |		\
+	BIT(APICV_INHIBIT_REASON_EXTINT) |		\
  	BIT(APICV_INHIBIT_REASON_PIT_REINJ) |		\
  	BIT(APICV_INHIBIT_REASON_BLOCKIRQ) |		\
  	BIT(APICV_INHIBIT_REASON_SEV)      |		\


I don't love adding another inhibit reason but, together, these two should
remove the contention on apicv_update_lock.  Another idea could be to move
IRQWIN to per-vCPU reason but Maxim tells me that it's not so easy.

Paolo

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Sean Christopherson]

On Mon, Feb 03, 2025, Paolo Bonzini wrote:
> On 2/3/25 19:45, Sean Christopherson wrote:
> > Unless there's a very, very good reason to support a use case that generates
> > ExtInts during boot, but _only_ during boot, and otherwise doesn't have any APICv
> > ihibits, I'm leaning towards making SVM's IRQ window inhibit sticky, i.e. never
> > clear it.
> 
> BIOS tends to use PIT, so that may be too much.  With respect to Naveen's report
> of contention on apicv_update_lock, I would go with the sticky-bit idea but apply
> it to APICV_INHIBIT_REASON_PIT_REINJ.

That won't work, at least not with yet more changes, because KVM creates the
in-kernel PIT with reinjection enabled by default.  The stick-bit idea is that
if a bit is set and can never be cleared, then there's no need to track new
updates.  Since userspace needs to explicitly disable reinjection, the inhibit
can't be sticky.

I assume We could fudge around that easily enough by deferring the inhibit until
a vCPU is created (or run?), but piggybacking PIT_REINJ won't help the userspace
I/O APIC case.

> I don't love adding another inhibit reason but, together, these two should
> remove the contention on apicv_update_lock.  Another idea could be to move
> IRQWIN to per-vCPU reason but Maxim tells me that it's not so easy.

Oh, yeah, that reminds me of the other reason I would vote for a sticky flag:
if inhibition really is toggling rapidly, performance is going to be quite bad
because inhibiting APICv requires (a) zapping APIC SPTEs and (b) serializing
writers if multiple vCPUs trigger the 0=>1 transition.

And there's some amount of serialization even if there's only a single writer,
as KVM kicks all vCPUs to toggle APICv (and again to flush TLBs, if necessary).

Hmm, something doesn't add up.  Naveen's changelog says:

  KVM additionally inhibits AVIC for requesting a IRQ window every time it has
  to inject external interrupts resulting in a barrage of inhibits being set and
  cleared. This shows significant performance degradation compared to AVIC being
  disabled, due to high contention on apicv_update_lock.

But if this is a "real world" use case where the only source of ExtInt is the
PIT, and kernels typically only wire up the PIT to the BSP, why is there
contention on apicv_update_lock?  APICv isn't actually being toggled, so readers
blocking writers to handle KVM_REQ_APICV_UPDATE shouldn't be a problem.

Naveen, do you know why there's a contention on apicv_update_lock?  Are multiple
vCPUs actually trying to inject ExtInt?

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Maxim Levitsky]

On Mon, 2025-02-03 at 15:46 -0800, Sean Christopherson wrote:
> On Mon, Feb 03, 2025, Paolo Bonzini wrote:
> > On 2/3/25 19:45, Sean Christopherson wrote:
> > > Unless there's a very, very good reason to support a use case that generates
> > > ExtInts during boot, but _only_ during boot, and otherwise doesn't have any APICv
> > > ihibits, I'm leaning towards making SVM's IRQ window inhibit sticky, i.e. never
> > > clear it.
> > 
> > BIOS tends to use PIT, so that may be too much.  With respect to Naveen's report
> > of contention on apicv_update_lock, I would go with the sticky-bit idea but apply
> > it to APICV_INHIBIT_REASON_PIT_REINJ.
> 
> That won't work, at least not with yet more changes, because KVM creates the
> in-kernel PIT with reinjection enabled by default.  The stick-bit idea is that
> if a bit is set and can never be cleared, then there's no need to track new
> updates.  Since userspace needs to explicitly disable reinjection, the inhibit
> can't be sticky.
I confirmed this with a trace, this is indeed the case.

> 
> I assume We could fudge around that easily enough by deferring the inhibit until
> a vCPU is created (or run?), but piggybacking PIT_REINJ won't help the userspace
> I/O APIC case.
> 
> > I don't love adding another inhibit reason but, together, these two should
> > remove the contention on apicv_update_lock.  Another idea could be to move
> > IRQWIN to per-vCPU reason but Maxim tells me that it's not so easy.

I retract this statement, it was based on my knowledge from back when I implemented it.

Looking at the current code again, this should be possible and can be a nice cleanup regardless.

(Or I just might have forgotten the reason that made me think back then that this is not worth it,
because I do remember well that I wanted to make IRQWIN inhibit to be per vcpu)

Basically to do so we need to introduce per-vcpu inhibit field (instead of .vcpu_get_apicv_inhibit_reasons callback)
set the inhibit bit there in svm_enable_irq_window, and raise KVM_REQ_APICV_UPDATE.

Same thing in interrupt_window_interception().

Nested code can be updated to do so as well very easily. IMHO this is a very nice cleanup.

I'll prepare a patch soon for this.

Also regardless, I strongly support Paolo's idea to inhibit APICv/AVIC when more than one ExtINT entry is
enabled, although this might not be enough:

In fact multiple vCPUs with ExtINT enabled I think can trigger the WARN_ON_ONCE below:

kvm_cpu_has_extint will be true on both vCPUs, so kvm_cpu_has_injectable_intr will be true on both
vCPUs as well, and thus both vCPUs can try to pull the interrupt from PIC, with second one likely getting -1,
and that not to mention the possibility of corrupting PIC state due to the concurrent access.


I am talking about this code:


	if (kvm_cpu_has_injectable_intr(vcpu)) {
		r = can_inject ? kvm_x86_call(interrupt_allowed)(vcpu, true) :
				 -EBUSY;
		if (r < 0)
			goto out;
		if (r) {
			int irq = kvm_cpu_get_interrupt(vcpu);

			if (!WARN_ON_ONCE(irq == -1)) {
				kvm_queue_interrupt(vcpu, irq, false);
				kvm_x86_call(inject_irq)(vcpu, false);
				WARN_ON(kvm_x86_call(interrupt_allowed)(vcpu, true) < 0);
			}
		}
		if (kvm_cpu_has_injectable_intr(vcpu))
			kvm_x86_call(enable_irq_window)(vcpu);
	}


So we might need to do something stronger than only inhibiting APICv/AVIC, we might
want to ignore second ExtINT entry, or maybe even better ignore both ExtInt entries and refuse to deliver ExtINT at all? 
(the guest is broken (Intel says that this configuration is frowned upon), so IMHO it deserves to keep both pieces. Do you agree?)


Best regards,
	Maxim Levitsky



> 
> Oh, yeah, that reminds me of the other reason I would vote for a sticky flag:
> if inhibition really is toggling rapidly, performance is going to be quite bad
> because inhibiting APICv requires (a) zapping APIC SPTEs and (b) serializing
> writers if multiple vCPUs trigger the 0=>1 transition.
> 
> And there's some amount of serialization even if there's only a single writer,
> as KVM kicks all vCPUs to toggle APICv (and again to flush TLBs, if necessary).
> 
> Hmm, something doesn't add up.  Naveen's changelog says:
> 
>   KVM additionally inhibits AVIC for requesting a IRQ window every time it has
>   to inject external interrupts resulting in a barrage of inhibits being set and
>   cleared. This shows significant performance degradation compared to AVIC being
>   disabled, due to high contention on apicv_update_lock.
> 
> But if this is a "real world" use case where the only source of ExtInt is the
> PIT, and kernels typically only wire up the PIT to the BSP, why is there
> contention on apicv_update_lock?  APICv isn't actually being toggled, so readers
> blocking writers to handle KVM_REQ_APICV_UPDATE shouldn't be a problem.
> 
> Naveen, do you know why there's a contention on apicv_update_lock?  Are multiple
> vCPUs actually trying to inject ExtInt?
> 

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Sean Christopherson]

On Mon, Feb 03, 2025, Maxim Levitsky wrote:
> On Mon, 2025-02-03 at 15:46 -0800, Sean Christopherson wrote:
> > On Mon, Feb 03, 2025, Paolo Bonzini wrote:
> > > On 2/3/25 19:45, Sean Christopherson wrote:
> > > > Unless there's a very, very good reason to support a use case that generates
> > > > ExtInts during boot, but _only_ during boot, and otherwise doesn't have any APICv
> > > > ihibits, I'm leaning towards making SVM's IRQ window inhibit sticky, i.e. never
> > > > clear it.
> > > 
> > > BIOS tends to use PIT, so that may be too much.  With respect to Naveen's report
> > > of contention on apicv_update_lock, I would go with the sticky-bit idea but apply
> > > it to APICV_INHIBIT_REASON_PIT_REINJ.
> > 
> > That won't work, at least not with yet more changes, because KVM creates the
> > in-kernel PIT with reinjection enabled by default.  The stick-bit idea is that
> > if a bit is set and can never be cleared, then there's no need to track new
> > updates.  Since userspace needs to explicitly disable reinjection, the inhibit
> > can't be sticky.
> I confirmed this with a trace, this is indeed the case.
> 
> > 
> > I assume We could fudge around that easily enough by deferring the inhibit until
> > a vCPU is created (or run?), but piggybacking PIT_REINJ won't help the userspace
> > I/O APIC case.
> > 
> > > I don't love adding another inhibit reason but, together, these two should
> > > remove the contention on apicv_update_lock.  Another idea could be to move
> > > IRQWIN to per-vCPU reason but Maxim tells me that it's not so easy.
> 
> I retract this statement, it was based on my knowledge from back when I
> implemented it.
> 
> Looking at the current code again, this should be possible and can be a nice
> cleanup regardless.
> 
> (Or I just might have forgotten the reason that made me think back then that
> this is not worth it, because I do remember well that I wanted to make IRQWIN
> inhibit to be per vcpu)

The complication is the APIC page.  That's not a problem for vCPUs running in L2
because they'll use a different MMU, i.e. a different set of SPTEs that never map
the APIC backing page.  At least, that's how it's supposed to work[*].  ;-)

For IRQWIN, turning off APICv for the current vCPU will leave the APIC SPTEs in
place and so KVM will fail to intercept accesses to the APIC page.  And making
IRQWIN a per-vCPU inhibit won't help performance in the case where there is no
other inhibit, because (a) toggling it on/off requires taking mmu_lock for writing
and doing a remote TLB flush, and (b) unless the guest is doing something bizarre,
only one vCPU will be receiving ExtInt IRQs.  I.e. I don't think trying to make
IRQWIN a pure per-vCPU inhibit is necessary for performance.

After fiddling with a bunch of ideas, I think the best approach to address both
issues is to add a counter for the IRQ window (addresses the per-vCPU aspect of
IRQ windows), set/clear the IRQWIN inhibit according to the counter when *any*
inhibit changes, and then force an immediate update if and only if the count hits
a 0<=>1 transition *and* there is no other inhibit.  That would address the flaw
Naveen found without needing to make PIT_REINJ sticky.

Guarding the count with apicv_update_lock held for read ensures that if there is
a racing change to a different inhibit, that either kvm_inc_or_dec_irq_window_inhibit()
will see no inhibits and go down the slow path, or __kvm_set_or_clear_apicv_inhibit()
will set IRQWIN accordingly.

Compile tested only (and probably needs to be split into multiple patches).  I'll
try to take it for a spin later today.

[*] https://lore.kernel.org/all/20250130010825.220346-1-seanjc@google.com

---
 arch/x86/include/asm/kvm_host.h | 13 ++++++++++
 arch/x86/kvm/svm/svm.c          | 43 +++++++++------------------------
 arch/x86/kvm/svm/svm.h          |  1 +
 arch/x86/kvm/x86.c              | 36 ++++++++++++++++++++++++++-
 4 files changed, 61 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5193c3dfbce1..9e3465e70a0a 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1365,6 +1365,7 @@ struct kvm_arch {
 	/* Protects apicv_inhibit_reasons */
 	struct rw_semaphore apicv_update_lock;
 	unsigned long apicv_inhibit_reasons;
+	atomic_t apicv_irq_window;
 
 	gpa_t wall_clock;
 
@@ -2203,6 +2204,18 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
 	kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
 }
 
+void kvm_inc_or_dec_irq_window_inhibit(struct kvm *kvm, bool inc);
+
+static inline void kvm_inc_apicv_irq_window(struct kvm *kvm)
+{
+	kvm_inc_or_dec_irq_window_inhibit(kvm, true);
+}
+
+static inline void kvm_dec_apicv_irq_window(struct kvm *kvm)
+{
+	kvm_inc_or_dec_irq_window_inhibit(kvm, false);
+}
+
 unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 				      unsigned long a0, unsigned long a1,
 				      unsigned long a2, unsigned long a3,
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7640a84e554a..668db3bfff3d 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1636,9 +1636,13 @@ static void svm_set_vintr(struct vcpu_svm *svm)
 	struct vmcb_control_area *control;
 
 	/*
-	 * The following fields are ignored when AVIC is enabled
+	 * vIRQ is ignored by hardware AVIC is enabled, and so AVIC must be
+	 * inhibited to detect the interrupt window.
 	 */
-	WARN_ON(kvm_vcpu_apicv_activated(&svm->vcpu));
+	if (enable_apicv && !is_guest_mode(&svm->vcpu)) {
+		svm->avic_irq_window = true;
+		kvm_inc_apicv_irq_window(svm->vcpu.kvm);
+	}
 
 	svm_set_intercept(svm, INTERCEPT_VINTR);
 
@@ -1666,6 +1670,11 @@ static void svm_set_vintr(struct vcpu_svm *svm)
 
 static void svm_clear_vintr(struct vcpu_svm *svm)
 {
+	if (svm->avic_irq_window && !is_guest_mode(&svm->vcpu)) {
+		svm->avic_irq_window = false;
+		kvm_dec_apicv_irq_window(svm->vcpu.kvm);
+	}
+
 	svm_clr_intercept(svm, INTERCEPT_VINTR);
 
 	/* Drop int_ctl fields related to VINTR injection.  */
@@ -3219,20 +3228,6 @@ static int interrupt_window_interception(struct kvm_vcpu *vcpu)
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	svm_clear_vintr(to_svm(vcpu));
 
-	/*
-	 * If not running nested, for AVIC, the only reason to end up here is ExtINTs.
-	 * In this case AVIC was temporarily disabled for
-	 * requesting the IRQ window and we have to re-enable it.
-	 *
-	 * If running nested, still remove the VM wide AVIC inhibit to
-	 * support case in which the interrupt window was requested when the
-	 * vCPU was not running nested.
-
-	 * All vCPUs which run still run nested, will remain to have their
-	 * AVIC still inhibited due to per-cpu AVIC inhibition.
-	 */
-	kvm_clear_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_IRQWIN);
-
 	++vcpu->stat.irq_window_exits;
 	return 1;
 }
@@ -3879,22 +3874,8 @@ static void svm_enable_irq_window(struct kvm_vcpu *vcpu)
 	 * enabled, the STGI interception will not occur. Enable the irq
 	 * window under the assumption that the hardware will set the GIF.
 	 */
-	if (vgif || gif_set(svm)) {
-		/*
-		 * IRQ window is not needed when AVIC is enabled,
-		 * unless we have pending ExtINT since it cannot be injected
-		 * via AVIC. In such case, KVM needs to temporarily disable AVIC,
-		 * and fallback to injecting IRQ via V_IRQ.
-		 *
-		 * If running nested, AVIC is already locally inhibited
-		 * on this vCPU, therefore there is no need to request
-		 * the VM wide AVIC inhibition.
-		 */
-		if (!is_guest_mode(vcpu))
-			kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_IRQWIN);
-
+	if (vgif || gif_set(svm))
 		svm_set_vintr(svm);
-	}
 }
 
 static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 9d7cdb8fbf87..8eefed0a865a 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -323,6 +323,7 @@ struct vcpu_svm {
 
 	bool guest_state_loaded;
 
+	bool avic_irq_window;
 	bool x2avic_msrs_intercepted;
 
 	/* Guest GIF value, used when vGIF is not enabled */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b2d9a16fd4d3..7388f4cfe468 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10604,7 +10604,11 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
 
 	old = new = kvm->arch.apicv_inhibit_reasons;
 
-	set_or_clear_apicv_inhibit(&new, reason, set);
+	if (reason != APICV_INHIBIT_REASON_IRQWIN)
+		set_or_clear_apicv_inhibit(&new, reason, set);
+
+	set_or_clear_apicv_inhibit(&new, APICV_INHIBIT_REASON_IRQWIN,
+				   atomic_read(&kvm->arch.apicv_irq_window));
 
 	if (!!old != !!new) {
 		/*
@@ -10645,6 +10649,36 @@ void kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
 }
 EXPORT_SYMBOL_GPL(kvm_set_or_clear_apicv_inhibit);
 
+void kvm_inc_or_dec_irq_window_inhibit(struct kvm *kvm, bool inc)
+{
+	bool toggle;
+
+	/*
+	 * The IRQ window inhibit has a cyclical dependency of sorts, as KVM
+	 * needs to manually inject IRQs and thus detect interrupt windows if
+	 * APICv is disabled/inhibitied.  To avoid thrashing if the IRQ window
+	 * is being requested because APICv is already inhibited, toggle the
+	 * actual inhibit (and take the lock for write) if and only if there's
+	 * no other inhibit.  KVM evaluates the IRQ window count when _any_
+	 * inhibit changes, i.e. the IRQ window inhibit can be lazily updated
+	 * on the next inhibit change (if one ever occurs).
+	 */
+	down_read(&kvm->arch.apicv_update_lock);
+
+	if (inc)
+		toggle = atomic_inc_return(&kvm->arch.apicv_irq_window) == 1;
+	else
+		toggle = atomic_dec_return(&kvm->arch.apicv_irq_window) == 0;
+
+	toggle = toggle && !(kvm->arch.apicv_inhibit_reasons & ~BIT(APICV_INHIBIT_REASON_IRQWIN));
+
+	up_read(&kvm->arch.apicv_update_lock);
+
+	if (toggle)
+		kvm_set_or_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_IRQWIN, inc);
+}
+EXPORT_SYMBOL_GPL(kvm_inc_or_dec_irq_window_inhibit);
+
 static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
 {
 	if (!kvm_apic_present(vcpu))

base-commit: eb723766b1030a23c38adf2348b7c3d1409d11f0
-- 

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Maxim Levitsky]

On Tue, 2025-02-04 at 11:18 -0800, Sean Christopherson wrote:
> On Mon, Feb 03, 2025, Maxim Levitsky wrote:
> > On Mon, 2025-02-03 at 15:46 -0800, Sean Christopherson wrote:
> > > On Mon, Feb 03, 2025, Paolo Bonzini wrote:
> > > > On 2/3/25 19:45, Sean Christopherson wrote:
> > > > > Unless there's a very, very good reason to support a use case that generates
> > > > > ExtInts during boot, but _only_ during boot, and otherwise doesn't have any APICv
> > > > > ihibits, I'm leaning towards making SVM's IRQ window inhibit sticky, i.e. never
> > > > > clear it.
> > > > 
> > > > BIOS tends to use PIT, so that may be too much.  With respect to Naveen's report
> > > > of contention on apicv_update_lock, I would go with the sticky-bit idea but apply
> > > > it to APICV_INHIBIT_REASON_PIT_REINJ.
> > > 
> > > That won't work, at least not with yet more changes, because KVM creates the
> > > in-kernel PIT with reinjection enabled by default.  The stick-bit idea is that
> > > if a bit is set and can never be cleared, then there's no need to track new
> > > updates.  Since userspace needs to explicitly disable reinjection, the inhibit
> > > can't be sticky.
> > I confirmed this with a trace, this is indeed the case.
> > 
> > > I assume We could fudge around that easily enough by deferring the inhibit until
> > > a vCPU is created (or run?), but piggybacking PIT_REINJ won't help the userspace
> > > I/O APIC case.
> > > 
> > > > I don't love adding another inhibit reason but, together, these two should
> > > > remove the contention on apicv_update_lock.  Another idea could be to move
> > > > IRQWIN to per-vCPU reason but Maxim tells me that it's not so easy.
> > 
> > I retract this statement, it was based on my knowledge from back when I
> > implemented it.
> > 
> > Looking at the current code again, this should be possible and can be a nice
> > cleanup regardless.
> > 
> > (Or I just might have forgotten the reason that made me think back then that
> > this is not worth it, because I do remember well that I wanted to make IRQWIN
> > inhibit to be per vcpu)
> 
> The complication is the APIC page.  That's not a problem for vCPUs running in L2
> because they'll use a different MMU, i.e. a different set of SPTEs that never map
> the APIC backing page.  At least, that's how it's supposed to work[*].  ;-)

Yes it is that thing that I forgot. Because of this AVIC can't be inhibited on a single vCPU, 
and the only exception is nesting.
I need to write this down somewhere so that I won't forget this again.


> 
> For IRQWIN, turning off APICv for the current vCPU will leave the APIC SPTEs in
> place and so KVM will fail to intercept accesses to the APIC page.  And making
> IRQWIN a per-vCPU inhibit won't help performance in the case where there is no
> other inhibit, because (a) toggling it on/off requires taking mmu_lock for writing
> and doing a remote TLB flush, and (b) unless the guest is doing something bizarre,
> only one vCPU will be receiving ExtInt IRQs.  I.e. I don't think trying to make
> IRQWIN a pure per-vCPU inhibit is necessary for performance.
> 
> After fiddling with a bunch of ideas, I think the best approach to address both
> issues is to add a counter for the IRQ window (addresses the per-vCPU aspect of
> IRQ windows), set/clear the IRQWIN inhibit according to the counter when *any*
> inhibit changes, and then force an immediate update if and only if the count hits
> a 0<=>1 transition *and* there is no other inhibit.  That would address the flaw
> Naveen found without needing to make PIT_REINJ sticky.
> 
> Guarding the count with apicv_update_lock held for read ensures that if there is
> a racing change to a different inhibit, that either kvm_inc_or_dec_irq_window_inhibit()
> will see no inhibits and go down the slow path, or __kvm_set_or_clear_apicv_inhibit()
> will set IRQWIN accordingly.
> 
> Compile tested only (and probably needs to be split into multiple patches).  I'll
> try to take it for a spin later today.
> 
> [*] https://lore.kernel.org/all/20250130010825.220346-1-seanjc@google.com
> 
> ---
>  arch/x86/include/asm/kvm_host.h | 13 ++++++++++
>  arch/x86/kvm/svm/svm.c          | 43 +++++++++------------------------
>  arch/x86/kvm/svm/svm.h          |  1 +
>  arch/x86/kvm/x86.c              | 36 ++++++++++++++++++++++++++-
>  4 files changed, 61 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 5193c3dfbce1..9e3465e70a0a 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1365,6 +1365,7 @@ struct kvm_arch {
>  	/* Protects apicv_inhibit_reasons */
>  	struct rw_semaphore apicv_update_lock;
>  	unsigned long apicv_inhibit_reasons;
> +	atomic_t apicv_irq_window;
>  
>  	gpa_t wall_clock;
>  
> @@ -2203,6 +2204,18 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
>  	kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
>  }
>  
> +void kvm_inc_or_dec_irq_window_inhibit(struct kvm *kvm, bool inc);
> +
> +static inline void kvm_inc_apicv_irq_window(struct kvm *kvm)
> +{
> +	kvm_inc_or_dec_irq_window_inhibit(kvm, true);
> +}
> +
> +static inline void kvm_dec_apicv_irq_window(struct kvm *kvm)
> +{
> +	kvm_inc_or_dec_irq_window_inhibit(kvm, false);
> +}
> +
>  unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>  				      unsigned long a0, unsigned long a1,
>  				      unsigned long a2, unsigned long a3,
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7640a84e554a..668db3bfff3d 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1636,9 +1636,13 @@ static void svm_set_vintr(struct vcpu_svm *svm)
>  	struct vmcb_control_area *control;
>  
>  	/*
> -	 * The following fields are ignored when AVIC is enabled
> +	 * vIRQ is ignored by hardware AVIC is enabled, and so AVIC must be
> +	 * inhibited to detect the interrupt window.
>  	 */
> -	WARN_ON(kvm_vcpu_apicv_activated(&svm->vcpu));
> +	if (enable_apicv && !is_guest_mode(&svm->vcpu)) {
> +		svm->avic_irq_window = true;
> +		kvm_inc_apicv_irq_window(svm->vcpu.kvm);
> +	}
>  
>  	svm_set_intercept(svm, INTERCEPT_VINTR);
>  
> @@ -1666,6 +1670,11 @@ static void svm_set_vintr(struct vcpu_svm *svm)
>  
>  static void svm_clear_vintr(struct vcpu_svm *svm)
>  {
> +	if (svm->avic_irq_window && !is_guest_mode(&svm->vcpu)) {
> +		svm->avic_irq_window = false;
> +		kvm_dec_apicv_irq_window(svm->vcpu.kvm);
> +	}
> +
>  	svm_clr_intercept(svm, INTERCEPT_VINTR);
>  
>  	/* Drop int_ctl fields related to VINTR injection.  */
> @@ -3219,20 +3228,6 @@ static int interrupt_window_interception(struct kvm_vcpu *vcpu)
>  	kvm_make_request(KVM_REQ_EVENT, vcpu);
>  	svm_clear_vintr(to_svm(vcpu));
>  
> -	/*
> -	 * If not running nested, for AVIC, the only reason to end up here is ExtINTs.
> -	 * In this case AVIC was temporarily disabled for
> -	 * requesting the IRQ window and we have to re-enable it.
> -	 *
> -	 * If running nested, still remove the VM wide AVIC inhibit to
> -	 * support case in which the interrupt window was requested when the
> -	 * vCPU was not running nested.
> -
> -	 * All vCPUs which run still run nested, will remain to have their
> -	 * AVIC still inhibited due to per-cpu AVIC inhibition.
> -	 */

Please keep these comment that explain why inhibit has to be cleared here,
and the code as well.

The reason is that IRQ window can be requested before nested entry, which will lead to
VM wide inhibit, and the interrupt window can happen while nested because nested hypervisor
can opt to not intercept interrupts. If that happens, the AVIC will remain inhibited forever.
 


> -	kvm_clear_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_IRQWIN);
> -
>  	++vcpu->stat.irq_window_exits;
>  	return 1;
>  }
> @@ -3879,22 +3874,8 @@ static void svm_enable_irq_window(struct kvm_vcpu *vcpu)
>  	 * enabled, the STGI interception will not occur. Enable the irq
>  	 * window under the assumption that the hardware will set the GIF.
>  	 */
> -	if (vgif || gif_set(svm)) {
> -		/*
> -		 * IRQ window is not needed when AVIC is enabled,
> -		 * unless we have pending ExtINT since it cannot be injected
> -		 * via AVIC. In such case, KVM needs to temporarily disable AVIC,
> -		 * and fallback to injecting IRQ via V_IRQ.
> -		 *
> -		 * If running nested, AVIC is already locally inhibited
> -		 * on this vCPU, therefore there is no need to request
> -		 * the VM wide AVIC inhibition.
> -		 */

Please keep this comment because it explains why this is needed.

As far as I remember there was a reason, not only related to performance,
to avoid inhibiting AVIC here VM wide.


Best regards,
	Maxim Levitsky




> -		if (!is_guest_mode(vcpu))
> -			kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_IRQWIN);
> -
> +	if (vgif || gif_set(svm))
>  		svm_set_vintr(svm);
> -	}
>  }
>  
>  static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 9d7cdb8fbf87..8eefed0a865a 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -323,6 +323,7 @@ struct vcpu_svm {
>  
>  	bool guest_state_loaded;
>  
> +	bool avic_irq_window;
>  	bool x2avic_msrs_intercepted;
>  
>  	/* Guest GIF value, used when vGIF is not enabled */
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b2d9a16fd4d3..7388f4cfe468 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10604,7 +10604,11 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
>  
>  	old = new = kvm->arch.apicv_inhibit_reasons;
>  
> -	set_or_clear_apicv_inhibit(&new, reason, set);
> +	if (reason != APICV_INHIBIT_REASON_IRQWIN)
> +		set_or_clear_apicv_inhibit(&new, reason, set);
> +
> +	set_or_clear_apicv_inhibit(&new, APICV_INHIBIT_REASON_IRQWIN,
> +				   atomic_read(&kvm->arch.apicv_irq_window));
>  
>  	if (!!old != !!new) {
>  		/*
> @@ -10645,6 +10649,36 @@ void kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
>  }
>  EXPORT_SYMBOL_GPL(kvm_set_or_clear_apicv_inhibit);
>  
> +void kvm_inc_or_dec_irq_window_inhibit(struct kvm *kvm, bool inc)
> +{
> +	bool toggle;
> +
> +	/*
> +	 * The IRQ window inhibit has a cyclical dependency of sorts, as KVM
> +	 * needs to manually inject IRQs and thus detect interrupt windows if
> +	 * APICv is disabled/inhibitied.  To avoid thrashing if the IRQ window
> +	 * is being requested because APICv is already inhibited, toggle the
> +	 * actual inhibit (and take the lock for write) if and only if there's
> +	 * no other inhibit.  KVM evaluates the IRQ window count when _any_
> +	 * inhibit changes, i.e. the IRQ window inhibit can be lazily updated
> +	 * on the next inhibit change (if one ever occurs).
> +	 */
> +	down_read(&kvm->arch.apicv_update_lock);
> +
> +	if (inc)
> +		toggle = atomic_inc_return(&kvm->arch.apicv_irq_window) == 1;
> +	else
> +		toggle = atomic_dec_return(&kvm->arch.apicv_irq_window) == 0;
> +
> +	toggle = toggle && !(kvm->arch.apicv_inhibit_reasons & ~BIT(APICV_INHIBIT_REASON_IRQWIN));
> +
> +	up_read(&kvm->arch.apicv_update_lock);
> +
> +	if (toggle)
> +		kvm_set_or_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_IRQWIN, inc);
> +}
> +EXPORT_SYMBOL_GPL(kvm_inc_or_dec_irq_window_inhibit);
> +
>  static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>  {
>  	if (!kvm_apic_present(vcpu))
> 
> base-commit: eb723766b1030a23c38adf2348b7c3d1409d11f0

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Sean Christopherson]

On Tue, Feb 04, 2025, Maxim Levitsky wrote:
> On Tue, 2025-02-04 at 11:18 -0800, Sean Christopherson wrote:
> > On Mon, Feb 03, 2025, Maxim Levitsky wrote:
> > @@ -3219,20 +3228,6 @@ static int interrupt_window_interception(struct kvm_vcpu *vcpu)
> >  	kvm_make_request(KVM_REQ_EVENT, vcpu);
> >  	svm_clear_vintr(to_svm(vcpu));
> >  
> > -	/*
> > -	 * If not running nested, for AVIC, the only reason to end up here is ExtINTs.
> > -	 * In this case AVIC was temporarily disabled for
> > -	 * requesting the IRQ window and we have to re-enable it.
> > -	 *
> > -	 * If running nested, still remove the VM wide AVIC inhibit to
> > -	 * support case in which the interrupt window was requested when the
> > -	 * vCPU was not running nested.
> > -
> > -	 * All vCPUs which run still run nested, will remain to have their
> > -	 * AVIC still inhibited due to per-cpu AVIC inhibition.
> > -	 */
> 
> Please keep these comment that explain why inhibit has to be cleared here,

Ya, I'll make sure there are good comments that capture everything before posting
anything.

> and the code as well.
> 
> The reason is that IRQ window can be requested before nested entry, which
> will lead to VM wide inhibit, and the interrupt window can happen while
> nested because nested hypervisor can opt to not intercept interrupts. If that
> happens, the AVIC will remain inhibited forever.

Ah, because the svm_clear_vintr() triggered by enter_svm_guest_mode()'s

	svm_set_gif(svm, true);

will occur in_guest_mode() == true.  But doesn't that bug exist today?  Clearing
the inhibit relies on interrupt_window_interception() being reached, and so if
the ExtInt is injected into L2, KVM will never reach interrupt_window_interception().

KVM will evaluate pending events in that case:

		    svm->vcpu.arch.nmi_pending ||
		    kvm_cpu_has_injectable_intr(&svm->vcpu) ||
		    kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
			kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);

but the nested pending VMRUN will block events and not get to the point where
KVM enables an IRQ window:

		r = can_inject ? kvm_x86_call(interrupt_allowed)(vcpu, true) :
				 -EBUSY;
		if (r < 0)
			goto out; <======= Taken for nested VMRUN pending
		if (r) {
			int irq = kvm_cpu_get_interrupt(vcpu);

			if (!WARN_ON_ONCE(irq == -1)) {
				kvm_queue_interrupt(vcpu, irq, false);
				kvm_x86_call(inject_irq)(vcpu, false);
				WARN_ON(kvm_x86_call(interrupt_allowed)(vcpu, true) < 0);
			}
		}
		if (kvm_cpu_has_injectable_intr(vcpu))
			kvm_x86_call(enable_irq_window)(vcpu);

and if L2 has IRQs enabled, the subsequent KVM_REQ_EVENT processing after the
immediate exit will inject into L2, without re-opening a window and thus without
triggering interrupt_window_interception().

Ha!  I was going to suggest hooking svm_leave_nested(), and lo and behold KVM
already does that for KVM_REQ_APICV_UPDATE to ensure it has an up-to-date view
of the inhibits.

After much staring, I suspect the reason KVM hooks interrupt_window_interception()
and not svm_clear_vintr() is to avoid thrashing the inhibit and thus the VM-wide
lock/state on nested entry/exit, e.g. on a typical:

	gif=0 => VMRUN => gif=1 => #VMEXIT => gif=0

sequence, KVM would clear the inhibit while running L2, and then re-enable the
inhibit when control transers back to L1.  But that doesn't gel with this comment
in interrupt_window_interception():

	* If running nested, still remove the VM wide AVIC inhibit to
	* support case in which the interrupt window was requested when the
	* vCPU was not running nested.

That code/comment exists specifically for the case where L1 is not intercepting
IRQs, and so KVM is effectively using V_IRQ in vmcb02 to detect interrupt windows
for L1 IRQs.

If L1 _is_ intercepting IRQs, then interrupt_window_interception() will never be
reached while L2 is active, because the only reason KVM would set the V_IRQ intercept
in vmcb02 would be on behalf of L1, i.e. because of vmcs12.  svm_clear_vintr()
always operates on (at least) vmcb01, and VMRUN unconditionally sets GIF=1, which
means that enter_svm_guest_mode() will always do svm_clear_vintr() via
svm_set_gif(svm, true).  I.e. KVM will keep the VM-wide inhibit set until control
transfers back to L1 *and* an interrupt window is triggered.

Even the "L1 doesn't intercept IRQs" case is incongruous, because if IRQs are
enabled in L2 at the time of VMRUN, KVM will immediately inject L1's ExtINT into
L2 without taking an interrupt window #VMEXIT.

I don't see a perfect solution.  Barring a rather absurd number of checks, KVM
will inevitably leave the VM-wide inhibit in place while running L2, or KVM will
risk thrashing the VM-wide inhibit across VMRUN.

/facepalm

It's still not perfect, but the obvious-in-hindsight answer is to clear the
IRQ window inhibit when KVM actually injects an interrupt and there's no longer
a injectable interrupt.

That optimizes all the paths: if L1 isn't intercept IRQs, KVM will drop the
inhibit as soon as an interrupt is injected into L2.  If L1 is intercepting IRQs,
KVM will keep the inhibit until the IRQ is injected into L2.  Unless I'm missing
something, the inhibit itself should prevent an injectable IRQ from disappearing,
i.e. AVIC won't be left inhibited.

So this for the SVM changes?

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7640a84e554a..2a5cf7029b26 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3219,20 +3219,6 @@ static int interrupt_window_interception(struct kvm_vcpu *vcpu)
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	svm_clear_vintr(to_svm(vcpu));
 
-	/*
-	 * If not running nested, for AVIC, the only reason to end up here is ExtINTs.
-	 * In this case AVIC was temporarily disabled for
-	 * requesting the IRQ window and we have to re-enable it.
-	 *
-	 * If running nested, still remove the VM wide AVIC inhibit to
-	 * support case in which the interrupt window was requested when the
-	 * vCPU was not running nested.
-
-	 * All vCPUs which run still run nested, will remain to have their
-	 * AVIC still inhibited due to per-cpu AVIC inhibition.
-	 */
-	kvm_clear_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_IRQWIN);
-
 	++vcpu->stat.irq_window_exits;
 	return 1;
 }
@@ -3670,6 +3656,23 @@ static void svm_inject_irq(struct kvm_vcpu *vcpu, bool reinjected)
 	struct vcpu_svm *svm = to_svm(vcpu);
 	u32 type;
 
+	/*
+	 * If AVIC was inhibited in order to detect an IRQ window, and there's
+	 * no other injectable interrupts pending or L2 is active (see below),
+	 * then drop the inhibit as the window has served its purpose.
+	 *
+	 * If L2 is active, this path is reachable if L1 is not intercepting
+	 * IRQs, i.e. if KVM is injecting L1 IRQs into L2.  AVIC is locally
+	 * inhibited while L2 is active; drop the VM-wide inhibit to optimize
+	 * the case in which the interrupt window was requested while L1 was
+	 * active (the vCPU was not running nested).
+	 */
+	if (svm->avic_irq_window &&
+	    (!kvm_cpu_has_injectable_intr(vcpu) || is_guest_mode(vcpu))) {
+		svm->avic_irq_window = false;
+		kvm_dec_apicv_irq_window(svm->vcpu.kvm);
+	}
+
 	if (vcpu->arch.interrupt.soft) {
 		if (svm_update_soft_interrupt_rip(vcpu))
 			return;
@@ -3881,17 +3884,28 @@ static void svm_enable_irq_window(struct kvm_vcpu *vcpu)
 	 */
 	if (vgif || gif_set(svm)) {
 		/*
-		 * IRQ window is not needed when AVIC is enabled,
-		 * unless we have pending ExtINT since it cannot be injected
-		 * via AVIC. In such case, KVM needs to temporarily disable AVIC,
-		 * and fallback to injecting IRQ via V_IRQ.
+		 * KVM only enables IRQ windows when AVIC is enabled if there's
+		 * pending ExtINT since it cannot be injected via AVIC (ExtINT
+		 * bypasses the local APIC).  V_IRQ is ignored by hardware when
+		 * AVIC is enabled, and so KVM needs to temporarily disable
+		 * AVIC in order to detect when it's ok to inject the ExtINT.
 		 *
-		 * If running nested, AVIC is already locally inhibited
-		 * on this vCPU, therefore there is no need to request
-		 * the VM wide AVIC inhibition.
+		 * If running nested, AVIC is already locally inhibited on this
+		 * vCPU (L2 vCPUs use a different MMU that never maps the AVIC
+		 * backing page), therefore there is no need to increment the
+		 * VM-wide AVIC inhibit.  KVM will re-evaluate events when the
+		 * vCPU exits to L1 and enable an IRQ window if the ExtINT is
+		 * still pending.
+		 *
+		 * Note, the IRQ window inhibit needs to be updated even if
+		 * AVIC is inhibited for a different reason, as KVM needs to
+		 * keep AVIC inhibited if the other reason is cleared and there
+		 * is still an injectable interrupt pending.
 		 */
-		if (!is_guest_mode(vcpu))
-			kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_IRQWIN);
+		if (enable_apicv && !svm->avic_irq_window && !is_guest_mode(vcpu)) {
+			svm->avic_irq_window = true;
+			kvm_inc_apicv_irq_window(vcpu->kvm);
+		}
 
 		svm_set_vintr(svm);
 	}

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Naveen N Rao]

On Tue, Feb 04, 2025 at 05:41:35PM -0800, Sean Christopherson wrote:
> On Tue, Feb 04, 2025, Maxim Levitsky wrote:
> > On Tue, 2025-02-04 at 11:18 -0800, Sean Christopherson wrote:
> > > On Mon, Feb 03, 2025, Maxim Levitsky wrote:
> > > @@ -3219,20 +3228,6 @@ static int interrupt_window_interception(struct kvm_vcpu *vcpu)
> > >  	kvm_make_request(KVM_REQ_EVENT, vcpu);
> > >  	svm_clear_vintr(to_svm(vcpu));
> > >  
> > > -	/*
> > > -	 * If not running nested, for AVIC, the only reason to end up here is ExtINTs.
> > > -	 * In this case AVIC was temporarily disabled for
> > > -	 * requesting the IRQ window and we have to re-enable it.
> > > -	 *
> > > -	 * If running nested, still remove the VM wide AVIC inhibit to
> > > -	 * support case in which the interrupt window was requested when the
> > > -	 * vCPU was not running nested.
> > > -
> > > -	 * All vCPUs which run still run nested, will remain to have their
> > > -	 * AVIC still inhibited due to per-cpu AVIC inhibition.
> > > -	 */
> > 
> > Please keep these comment that explain why inhibit has to be cleared here,
> 
> Ya, I'll make sure there are good comments that capture everything before posting
> anything.
> 
> > and the code as well.
> > 
> > The reason is that IRQ window can be requested before nested entry, which
> > will lead to VM wide inhibit, and the interrupt window can happen while
> > nested because nested hypervisor can opt to not intercept interrupts. If that
> > happens, the AVIC will remain inhibited forever.

<snip>

> It's still not perfect, but the obvious-in-hindsight answer is to 
> clear the
> IRQ window inhibit when KVM actually injects an interrupt and there's no longer
> a injectable interrupt.
> 
> That optimizes all the paths: if L1 isn't intercept IRQs, KVM will drop the
> inhibit as soon as an interrupt is injected into L2.  If L1 is intercepting IRQs,
> KVM will keep the inhibit until the IRQ is injected into L2.  Unless I'm missing
> something, the inhibit itself should prevent an injectable IRQ from disappearing,
> i.e. AVIC won't be left inhibited.
> 
> So this for the SVM changes?

I tested this in my setup (below changes to svm.c, along with the rest 
of the changes from your earlier posting), and it ensures proper update 
of the APICv inhibit. That is, with the below changes, AVIC is properly 
enabled in the virtual machine in pit=discard mode (which wasn't the 
case with the earlier changes to svm_[set|clear]_vintr()).

This improves performance in my test by ~10%:
Average:        IFACE   rxpck/s   txpck/s    rxkB/s    txkB/s   rxcmp/s   txcmp/s  rxmcst/s   %ifutil
Average:           lo      0.12      0.12      0.01      0.01      0.00      0.00      0.00      0.00
Average:       enp0s2 1197141.51 1197158.89 226801.33 114572.12      0.00      0.00      0.00      0.00

This is still ~10% below what I see with avic=0 though.


Thanks,
Naveen

> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7640a84e554a..2a5cf7029b26 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3219,20 +3219,6 @@ static int interrupt_window_interception(struct kvm_vcpu *vcpu)
>  	kvm_make_request(KVM_REQ_EVENT, vcpu);
>  	svm_clear_vintr(to_svm(vcpu));
>  
> -	/*
> -	 * If not running nested, for AVIC, the only reason to end up here is ExtINTs.
> -	 * In this case AVIC was temporarily disabled for
> -	 * requesting the IRQ window and we have to re-enable it.
> -	 *
> -	 * If running nested, still remove the VM wide AVIC inhibit to
> -	 * support case in which the interrupt window was requested when the
> -	 * vCPU was not running nested.
> -
> -	 * All vCPUs which run still run nested, will remain to have their
> -	 * AVIC still inhibited due to per-cpu AVIC inhibition.
> -	 */
> -	kvm_clear_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_IRQWIN);
> -
>  	++vcpu->stat.irq_window_exits;
>  	return 1;
>  }
> @@ -3670,6 +3656,23 @@ static void svm_inject_irq(struct kvm_vcpu *vcpu, bool reinjected)
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	u32 type;
>  
> +	/*
> +	 * If AVIC was inhibited in order to detect an IRQ window, and there's
> +	 * no other injectable interrupts pending or L2 is active (see below),
> +	 * then drop the inhibit as the window has served its purpose.
> +	 *
> +	 * If L2 is active, this path is reachable if L1 is not intercepting
> +	 * IRQs, i.e. if KVM is injecting L1 IRQs into L2.  AVIC is locally
> +	 * inhibited while L2 is active; drop the VM-wide inhibit to optimize
> +	 * the case in which the interrupt window was requested while L1 was
> +	 * active (the vCPU was not running nested).
> +	 */
> +	if (svm->avic_irq_window &&
> +	    (!kvm_cpu_has_injectable_intr(vcpu) || is_guest_mode(vcpu))) {
> +		svm->avic_irq_window = false;
> +		kvm_dec_apicv_irq_window(svm->vcpu.kvm);
> +	}
> +
>  	if (vcpu->arch.interrupt.soft) {
>  		if (svm_update_soft_interrupt_rip(vcpu))
>  			return;
> @@ -3881,17 +3884,28 @@ static void svm_enable_irq_window(struct kvm_vcpu *vcpu)
>  	 */
>  	if (vgif || gif_set(svm)) {
>  		/*
> -		 * IRQ window is not needed when AVIC is enabled,
> -		 * unless we have pending ExtINT since it cannot be injected
> -		 * via AVIC. In such case, KVM needs to temporarily disable AVIC,
> -		 * and fallback to injecting IRQ via V_IRQ.
> +		 * KVM only enables IRQ windows when AVIC is enabled if there's
> +		 * pending ExtINT since it cannot be injected via AVIC (ExtINT
> +		 * bypasses the local APIC).  V_IRQ is ignored by hardware when
> +		 * AVIC is enabled, and so KVM needs to temporarily disable
> +		 * AVIC in order to detect when it's ok to inject the ExtINT.
>  		 *
> -		 * If running nested, AVIC is already locally inhibited
> -		 * on this vCPU, therefore there is no need to request
> -		 * the VM wide AVIC inhibition.
> +		 * If running nested, AVIC is already locally inhibited on this
> +		 * vCPU (L2 vCPUs use a different MMU that never maps the AVIC
> +		 * backing page), therefore there is no need to increment the
> +		 * VM-wide AVIC inhibit.  KVM will re-evaluate events when the
> +		 * vCPU exits to L1 and enable an IRQ window if the ExtINT is
> +		 * still pending.
> +		 *
> +		 * Note, the IRQ window inhibit needs to be updated even if
> +		 * AVIC is inhibited for a different reason, as KVM needs to
> +		 * keep AVIC inhibited if the other reason is cleared and there
> +		 * is still an injectable interrupt pending.
>  		 */
> -		if (!is_guest_mode(vcpu))
> -			kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_IRQWIN);
> +		if (enable_apicv && !svm->avic_irq_window && !is_guest_mode(vcpu)) {
> +			svm->avic_irq_window = true;
> +			kvm_inc_apicv_irq_window(vcpu->kvm);
> +		}
>  
>  		svm_set_vintr(svm);
>  	}

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Paolo Bonzini]

On 2/4/25 20:18, Sean Christopherson wrote:
> On Mon, Feb 03, 2025, Maxim Levitsky wrote:
>> On Mon, 2025-02-03 at 15:46 -0800, Sean Christopherson wrote:
>>> On Mon, Feb 03, 2025, Paolo Bonzini wrote:
>>>> On 2/3/25 19:45, Sean Christopherson wrote:
>>>>> Unless there's a very, very good reason to support a use case that generates
>>>>> ExtInts during boot, but _only_ during boot, and otherwise doesn't have any APICv
>>>>> ihibits, I'm leaning towards making SVM's IRQ window inhibit sticky, i.e. never
>>>>> clear it.
>>>>
>>>> BIOS tends to use PIT, so that may be too much.  With respect to Naveen's report
>>>> of contention on apicv_update_lock, I would go with the sticky-bit idea but apply
>>>> it to APICV_INHIBIT_REASON_PIT_REINJ.
>>>
>>> That won't work, at least not with yet more changes, because KVM creates the
>>> in-kernel PIT with reinjection enabled by default.  The stick-bit idea is that
>>> if a bit is set and can never be cleared, then there's no need to track new
>>> updates.  Since userspace needs to explicitly disable reinjection, the inhibit
>>> can't be sticky.
>> I confirmed this with a trace, this is indeed the case.
>>
>>>
>>> I assume We could fudge around that easily enough by deferring the inhibit until
>>> a vCPU is created (or run?), but piggybacking PIT_REINJ won't help the userspace
>>> I/O APIC case.
>>>
>>>> I don't love adding another inhibit reason but, together, these two should
>>>> remove the contention on apicv_update_lock.  Another idea could be to move
>>>> IRQWIN to per-vCPU reason but Maxim tells me that it's not so easy.
>>
>> I retract this statement, it was based on my knowledge from back when I
>> implemented it.
>>
>> Looking at the current code again, this should be possible and can be a nice
>> cleanup regardless.
>>
>> (Or I just might have forgotten the reason that made me think back then that
>> this is not worth it, because I do remember well that I wanted to make IRQWIN
>> inhibit to be per vcpu)
> 
> The complication is the APIC page.  That's not a problem for vCPUs running in L2
> because they'll use a different MMU, i.e. a different set of SPTEs that never map
> the APIC backing page.  At least, that's how it's supposed to work[*].  ;-)
> 
> For IRQWIN, turning off APICv for the current vCPU will leave the APIC SPTEs in
> place and so KVM will fail to intercept accesses to the APIC page.  And making
> IRQWIN a per-vCPU inhibit won't help performance in the case where there is no
> other inhibit, because (a) toggling it on/off requires taking mmu_lock for writing
> and doing a remote TLB flush, and (b) unless the guest is doing something bizarre,
> only one vCPU will be receiving ExtInt IRQs.  I.e. I don't think trying to make
> IRQWIN a pure per-vCPU inhibit is necessary for performance.
> 
> After fiddling with a bunch of ideas, I think the best approach to address both
> issues is to add a counter for the IRQ window (addresses the per-vCPU aspect of
> IRQ windows), set/clear the IRQWIN inhibit according to the counter when *any*
> inhibit changes, and then force an immediate update if and only if the count hits
> a 0<=>1 transition *and* there is no other inhibit.  That would address the flaw
> Naveen found without needing to make PIT_REINJ sticky.
> 
> Guarding the count with apicv_update_lock held for read ensures that if there is
> a racing change to a different inhibit, that either kvm_inc_or_dec_irq_window_inhibit()
> will see no inhibits and go down the slow path, or __kvm_set_or_clear_apicv_inhibit()
> will set IRQWIN accordingly.
> 
> Compile tested only (and probably needs to be split into multiple patches).  I'll
> try to take it for a spin later today.
> 
> [*] https://lore.kernel.org/all/20250130010825.220346-1-seanjc@google.com
> 
> ---
>   arch/x86/include/asm/kvm_host.h | 13 ++++++++++
>   arch/x86/kvm/svm/svm.c          | 43 +++++++++------------------------
>   arch/x86/kvm/svm/svm.h          |  1 +
>   arch/x86/kvm/x86.c              | 36 ++++++++++++++++++++++++++-
>   4 files changed, 61 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 5193c3dfbce1..9e3465e70a0a 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1365,6 +1365,7 @@ struct kvm_arch {
>   	/* Protects apicv_inhibit_reasons */
>   	struct rw_semaphore apicv_update_lock;
>   	unsigned long apicv_inhibit_reasons;
> +	atomic_t apicv_irq_window;
>   
>   	gpa_t wall_clock;
>   
> @@ -2203,6 +2204,18 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
>   	kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
>   }
>   
> +void kvm_inc_or_dec_irq_window_inhibit(struct kvm *kvm, bool inc);
> +
> +static inline void kvm_inc_apicv_irq_window(struct kvm *kvm)
> +{
> +	kvm_inc_or_dec_irq_window_inhibit(kvm, true);
> +}
> +
> +static inline void kvm_dec_apicv_irq_window(struct kvm *kvm)
> +{
> +	kvm_inc_or_dec_irq_window_inhibit(kvm, false);
> +}
> +
>   unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   				      unsigned long a0, unsigned long a1,
>   				      unsigned long a2, unsigned long a3,
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7640a84e554a..668db3bfff3d 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1636,9 +1636,13 @@ static void svm_set_vintr(struct vcpu_svm *svm)
>   	struct vmcb_control_area *control;
>   
>   	/*
> -	 * The following fields are ignored when AVIC is enabled
> +	 * vIRQ is ignored by hardware AVIC is enabled, and so AVIC must be
> +	 * inhibited to detect the interrupt window.
>   	 */
> -	WARN_ON(kvm_vcpu_apicv_activated(&svm->vcpu));
> +	if (enable_apicv && !is_guest_mode(&svm->vcpu)) {
> +		svm->avic_irq_window = true;
> +		kvm_inc_apicv_irq_window(svm->vcpu.kvm);
> +	}
>   
>   	svm_set_intercept(svm, INTERCEPT_VINTR);
>   
> @@ -1666,6 +1670,11 @@ static void svm_set_vintr(struct vcpu_svm *svm)
>   
>   static void svm_clear_vintr(struct vcpu_svm *svm)
>   {
> +	if (svm->avic_irq_window && !is_guest_mode(&svm->vcpu)) {
> +		svm->avic_irq_window = false;
> +		kvm_dec_apicv_irq_window(svm->vcpu.kvm);
> +	}
> +
>   	svm_clr_intercept(svm, INTERCEPT_VINTR);
>   
>   	/* Drop int_ctl fields related to VINTR injection.  */
> @@ -3219,20 +3228,6 @@ static int interrupt_window_interception(struct kvm_vcpu *vcpu)
>   	kvm_make_request(KVM_REQ_EVENT, vcpu);
>   	svm_clear_vintr(to_svm(vcpu));
>   
> -	/*
> -	 * If not running nested, for AVIC, the only reason to end up here is ExtINTs.
> -	 * In this case AVIC was temporarily disabled for
> -	 * requesting the IRQ window and we have to re-enable it.
> -	 *
> -	 * If running nested, still remove the VM wide AVIC inhibit to
> -	 * support case in which the interrupt window was requested when the
> -	 * vCPU was not running nested.
> -
> -	 * All vCPUs which run still run nested, will remain to have their
> -	 * AVIC still inhibited due to per-cpu AVIC inhibition.
> -	 */
> -	kvm_clear_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_IRQWIN);
> -
>   	++vcpu->stat.irq_window_exits;
>   	return 1;
>   }
> @@ -3879,22 +3874,8 @@ static void svm_enable_irq_window(struct kvm_vcpu *vcpu)
>   	 * enabled, the STGI interception will not occur. Enable the irq
>   	 * window under the assumption that the hardware will set the GIF.
>   	 */
> -	if (vgif || gif_set(svm)) {
> -		/*
> -		 * IRQ window is not needed when AVIC is enabled,
> -		 * unless we have pending ExtINT since it cannot be injected
> -		 * via AVIC. In such case, KVM needs to temporarily disable AVIC,
> -		 * and fallback to injecting IRQ via V_IRQ.
> -		 *
> -		 * If running nested, AVIC is already locally inhibited
> -		 * on this vCPU, therefore there is no need to request
> -		 * the VM wide AVIC inhibition.
> -		 */
> -		if (!is_guest_mode(vcpu))
> -			kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_IRQWIN);
> -
> +	if (vgif || gif_set(svm))
>   		svm_set_vintr(svm);
> -	}
>   }
>   
>   static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 9d7cdb8fbf87..8eefed0a865a 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -323,6 +323,7 @@ struct vcpu_svm {
>   
>   	bool guest_state_loaded;
>   
> +	bool avic_irq_window;
>   	bool x2avic_msrs_intercepted;
>   
>   	/* Guest GIF value, used when vGIF is not enabled */
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b2d9a16fd4d3..7388f4cfe468 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10604,7 +10604,11 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
>   
>   	old = new = kvm->arch.apicv_inhibit_reasons;
>   
> -	set_or_clear_apicv_inhibit(&new, reason, set);
> +	if (reason != APICV_INHIBIT_REASON_IRQWIN)
> +		set_or_clear_apicv_inhibit(&new, reason, set);
> +
> +	set_or_clear_apicv_inhibit(&new, APICV_INHIBIT_REASON_IRQWIN,
> +				   atomic_read(&kvm->arch.apicv_irq_window));
>   
>   	if (!!old != !!new) {
>   		/*
> @@ -10645,6 +10649,36 @@ void kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
>   }
>   EXPORT_SYMBOL_GPL(kvm_set_or_clear_apicv_inhibit);
>   
> +void kvm_inc_or_dec_irq_window_inhibit(struct kvm *kvm, bool inc)
> +{
> +	bool toggle;
> +
> +	/*
> +	 * The IRQ window inhibit has a cyclical dependency of sorts, as KVM
> +	 * needs to manually inject IRQs and thus detect interrupt windows if
> +	 * APICv is disabled/inhibitied.  To avoid thrashing if the IRQ window
> +	 * is being requested because APICv is already inhibited, toggle the
> +	 * actual inhibit (and take the lock for write) if and only if there's
> +	 * no other inhibit.  KVM evaluates the IRQ window count when _any_
> +	 * inhibit changes, i.e. the IRQ window inhibit can be lazily updated
> +	 * on the next inhibit change (if one ever occurs).
> +	 */
> +	down_read(&kvm->arch.apicv_update_lock);
> +
> +	if (inc)
> +		toggle = atomic_inc_return(&kvm->arch.apicv_irq_window) == 1;
> +	else
> +		toggle = atomic_dec_return(&kvm->arch.apicv_irq_window) == 0;
> +
> +	toggle = toggle && !(kvm->arch.apicv_inhibit_reasons & ~BIT(APICV_INHIBIT_REASON_IRQWIN));
> +
> +	up_read(&kvm->arch.apicv_update_lock);
> +
> +	if (toggle)
> +		kvm_set_or_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_IRQWIN, inc);

I'm not super confident in breaking the critical section...  Another possibility:

void kvm_inc_or_dec_irq_window_inhibit(struct kvm *kvm, bool inc)
{
         int add = inc ? 1 : -1;

	if (!enable_apicv)
		return;

         /*
         * IRQ windows happen either because of ExtINT injections, or because
	* APICv is already disabled/inhibited for another reason.  While ExtINT
	* injections are rare and should not happen while the vCPU is running
	* its actual workload, it's worth avoiding thrashing if the IRQ window
         * is being requested because APICv is already inhibited.  So, toggle the
         * the actual inhibit (which requires taking the lock forwrite) if and
	* only if there's no other inhibit.  kvm_set_or_clear_apicv_inhibit()
         * always evaluates the IRQ window count; thus the IRQ window inhibit
         * call _will_ be lazily updated on the next call, if it ever happens.
         */
         if (READ_ONCE(kvm->arch.apicv_inhibit_reasons) & ~BIT(APICV_INHIBIT_REASON_IRQWIN)) {
                 guard(rwsem_read)(&kvm->arch.apicv_update_lock);
                 if (READ_ONCE(kvm->arch.apicv_inhibit_reasons) & ~BIT(APICV_INHIBIT_REASON_IRQWIN)) {
                         atomic_add(add, &kvm->arch.apicv_irq_window);
                         return;
                 }
         }

	/*
	 * Strictly speaking the lock is only needed if going 0->1 or 1->0,
	 * a la atomic_dec_and_mutex_lock.  However, ExtINTs are rare and
	 * only target a single CPU, so that is the common case; do not
	 * bother eliding the down_write()/up_write() pair.
	 */
         guard(rwsem_write)(&kvm->arch.apicv_update_lock);
         if (atomic_add_return(add, &kvm->arch.apicv_irq_window) == inc)
                 __kvm_set_or_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_IRQWIN, inc);
}
EXPORT_SYMBOL_GPL(kvm_inc_or_dec_irq_window_inhibit);

Paolo

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Naveen N Rao]

On Wed, Feb 05, 2025 at 12:36:21PM +0100, Paolo Bonzini wrote:
> On 2/4/25 20:18, Sean Christopherson wrote:
> > On Mon, Feb 03, 2025, Maxim Levitsky wrote:
> > > On Mon, 2025-02-03 at 15:46 -0800, Sean Christopherson wrote:
> > > > On Mon, Feb 03, 2025, Paolo Bonzini wrote:
> > > > > On 2/3/25 19:45, Sean Christopherson wrote:

<snip>

> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index b2d9a16fd4d3..7388f4cfe468 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10604,7 +10604,11 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
> >   	old = new = kvm->arch.apicv_inhibit_reasons;
> > -	set_or_clear_apicv_inhibit(&new, reason, set);
> > +	if (reason != APICV_INHIBIT_REASON_IRQWIN)
> > +		set_or_clear_apicv_inhibit(&new, reason, set);
> > +
> > +	set_or_clear_apicv_inhibit(&new, APICV_INHIBIT_REASON_IRQWIN,
> > +				   atomic_read(&kvm->arch.apicv_irq_window));
> >   	if (!!old != !!new) {
> >   		/*
> > @@ -10645,6 +10649,36 @@ void kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
> >   }
> >   EXPORT_SYMBOL_GPL(kvm_set_or_clear_apicv_inhibit);
> > +void kvm_inc_or_dec_irq_window_inhibit(struct kvm *kvm, bool inc)
> > +{
> > +	bool toggle;
> > +
> > +	/*
> > +	 * The IRQ window inhibit has a cyclical dependency of sorts, as KVM
> > +	 * needs to manually inject IRQs and thus detect interrupt windows if
> > +	 * APICv is disabled/inhibitied.  To avoid thrashing if the IRQ window
> > +	 * is being requested because APICv is already inhibited, toggle the
> > +	 * actual inhibit (and take the lock for write) if and only if there's
> > +	 * no other inhibit.  KVM evaluates the IRQ window count when _any_
> > +	 * inhibit changes, i.e. the IRQ window inhibit can be lazily updated
> > +	 * on the next inhibit change (if one ever occurs).
> > +	 */
> > +	down_read(&kvm->arch.apicv_update_lock);
> > +
> > +	if (inc)
> > +		toggle = atomic_inc_return(&kvm->arch.apicv_irq_window) == 1;
> > +	else
> > +		toggle = atomic_dec_return(&kvm->arch.apicv_irq_window) == 0;
> > +
> > +	toggle = toggle && !(kvm->arch.apicv_inhibit_reasons & ~BIT(APICV_INHIBIT_REASON_IRQWIN));
> > +
> > +	up_read(&kvm->arch.apicv_update_lock);
> > +
> > +	if (toggle)
> > +		kvm_set_or_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_IRQWIN, inc);
> 
> I'm not super confident in breaking the critical section...  Another possibility:
> 
> void kvm_inc_or_dec_irq_window_inhibit(struct kvm *kvm, bool inc)
> {
>         int add = inc ? 1 : -1;
> 
> 	if (!enable_apicv)
> 		return;
> 
>         /*
>         * IRQ windows happen either because of ExtINT injections, or because
> 	* APICv is already disabled/inhibited for another reason.  While ExtINT
> 	* injections are rare and should not happen while the vCPU is running
> 	* its actual workload, it's worth avoiding thrashing if the IRQ window
>         * is being requested because APICv is already inhibited.  So, toggle the
>         * the actual inhibit (which requires taking the lock forwrite) if and
> 	* only if there's no other inhibit.  kvm_set_or_clear_apicv_inhibit()
>         * always evaluates the IRQ window count; thus the IRQ window inhibit
>         * call _will_ be lazily updated on the next call, if it ever happens.
>         */
>         if (READ_ONCE(kvm->arch.apicv_inhibit_reasons) & ~BIT(APICV_INHIBIT_REASON_IRQWIN)) {
>                 guard(rwsem_read)(&kvm->arch.apicv_update_lock);
>                 if (READ_ONCE(kvm->arch.apicv_inhibit_reasons) & ~BIT(APICV_INHIBIT_REASON_IRQWIN)) {
>                         atomic_add(add, &kvm->arch.apicv_irq_window);
>                         return;
>                 }
>         }
> 
> 	/*
> 	 * Strictly speaking the lock is only needed if going 0->1 or 1->0,
> 	 * a la atomic_dec_and_mutex_lock.  However, ExtINTs are rare and
> 	 * only target a single CPU, so that is the common case; do not
> 	 * bother eliding the down_write()/up_write() pair.
> 	 */
>         guard(rwsem_write)(&kvm->arch.apicv_update_lock);
>         if (atomic_add_return(add, &kvm->arch.apicv_irq_window) == inc)
>                 __kvm_set_or_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_IRQWIN, inc);
> }
> EXPORT_SYMBOL_GPL(kvm_inc_or_dec_irq_window_inhibit);

I haven't analyzed this yet, but moving apicv_irq_window into a separate 
cacheline is improving the performance in my tests by ~7 to 8%:

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 9e3465e70a0a..d8a40ac49226 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1355,6 +1355,9 @@ struct kvm_arch {
        struct kvm_ioapic *vioapic;
        struct kvm_pit *vpit;
        atomic_t vapics_in_nmi_mode;
+
+       atomic_t apicv_irq_window;
+
        struct mutex apic_map_lock;
        struct kvm_apic_map __rcu *apic_map;
        atomic_t apic_map_dirty;
@@ -1365,7 +1368,6 @@ struct kvm_arch {
        /* Protects apicv_inhibit_reasons */
        struct rw_semaphore apicv_update_lock;
        unsigned long apicv_inhibit_reasons;
-       atomic_t apicv_irq_window;

        gpa_t wall_clock;


I chose that spot before apic_map_lock simply because there was a 4 byte 
hole there. This happens to also help performance in the AVIC disabled 
case by a few percentage points (rather, restores the performance in the 
AVIC disabled case).

Before this change, I was trying to see if we could entirely elide the 
rwsem read lock in the specific scenario we are seeing the bottleneck.  
That is, instead of checking for any other inhibit being set, can we 
specifically test for PIT_REINJ while setting the IRQWIN inhibit? Then, 
update the inhibit change logic if PIT_REINJ is cleared to re-check the 
irq window count.

There's probably a race here somewhere, but FWIW, along with the above 
change to 'struct kvm_arch', this helps improve performance by a few 
more percentage points helping close the gap to within 2% of the AVIC 
disabled case.


- Naveen




diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 274eb99aa97b..bd861342b949 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10596,6 +10596,7 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
                                      enum kvm_apicv_inhibit reason, bool set)
 {
        unsigned long old, new;
+       bool retried = false;
 
        lockdep_assert_held_write(&kvm->arch.apicv_update_lock);
 
@@ -10607,6 +10608,7 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
        if (reason != APICV_INHIBIT_REASON_IRQWIN)
                set_or_clear_apicv_inhibit(&new, reason, set);
 
+again:
        set_or_clear_apicv_inhibit(&new, APICV_INHIBIT_REASON_IRQWIN,
                                   atomic_read(&kvm->arch.apicv_irq_window));
 
@@ -10635,6 +10637,14 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
        } else {
                kvm->arch.apicv_inhibit_reasons = new;
        }
+
+       /* If PIT_REINJ inhibit is being cleared, ensure we have updated copy of apicv_irq_window */
+       if (reason == APICV_INHIBIT_REASON_PIT_REINJ && !set && !retried) {
+               smp_mb();
+               retried = true;
+               old = new = kvm->arch.apicv_inhibit_reasons;
+               goto again;
+       }
 }
 
 void kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
@@ -10656,34 +10666,10 @@ void kvm_inc_or_dec_irq_window_inhibit(struct kvm *kvm, bool inc)
        if (!enable_apicv)
                return;
 
-       /*
-        * IRQ windows happen either because of ExtINT injections, or because
-        * APICv is already disabled/inhibited for another reason.  While ExtINT
-        * injections are rare and should not happen while the vCPU is running
-        * its actual workload, it's worth avoiding thrashing if the IRQ window
-        * is being requested because APICv is already inhibited.  So, toggle the
-        * the actual inhibit (which requires taking the lock forwrite) if and
-        * only if there's no other inhibit.  kvm_set_or_clear_apicv_inhibit()
-        * always evaluates the IRQ window count; thus the IRQ window inhibit
-        * call _will_ be lazily updated on the next call, if it ever happens.
-        */
-       if (READ_ONCE(kvm->arch.apicv_inhibit_reasons) & ~BIT(APICV_INHIBIT_REASON_IRQWIN)) {
-               guard(rwsem_read)(&kvm->arch.apicv_update_lock);
-               if (READ_ONCE(kvm->arch.apicv_inhibit_reasons) & ~BIT(APICV_INHIBIT_REASON_IRQWIN)) {
-                       atomic_add(add, &kvm->arch.apicv_irq_window);
-                       return;
-               }
-       }
-
-       /*
-        * Strictly speaking the lock is only needed if going 0->1 or 1->0,
-        * a la atomic_dec_and_mutex_lock.  However, ExtINTs are rare and
-        * only target a single CPU, so that is the common case; do not
-        * bother eliding the down_write()/up_write() pair.
-        */
-       guard(rwsem_write)(&kvm->arch.apicv_update_lock);
-       if (atomic_add_return(add, &kvm->arch.apicv_irq_window) == inc)
-               __kvm_set_or_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_IRQWIN, inc);
+       atomic_add(add, &kvm->arch.apicv_irq_window);
+       smp_mb();
+       if (!(READ_ONCE(kvm->arch.apicv_inhibit_reasons) & BIT(APICV_INHIBIT_REASON_PIT_REINJ)))
+               kvm_set_or_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_IRQWIN, inc);
 }
 EXPORT_SYMBOL_GPL(kvm_inc_or_dec_irq_window_inhibit);

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Sean Christopherson]

On Tue, Feb 11, 2025, Naveen N Rao wrote:
> On Wed, Feb 05, 2025 at 12:36:21PM +0100, Paolo Bonzini wrote:
> I haven't analyzed this yet, but moving apicv_irq_window into a separate 
> cacheline is improving the performance in my tests by ~7 to 8%:
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 9e3465e70a0a..d8a40ac49226 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1355,6 +1355,9 @@ struct kvm_arch {
>         struct kvm_ioapic *vioapic;
>         struct kvm_pit *vpit;
>         atomic_t vapics_in_nmi_mode;
> +
> +       atomic_t apicv_irq_window;
> +
>         struct mutex apic_map_lock;
>         struct kvm_apic_map __rcu *apic_map;
>         atomic_t apic_map_dirty;
> @@ -1365,7 +1368,6 @@ struct kvm_arch {
>         /* Protects apicv_inhibit_reasons */
>         struct rw_semaphore apicv_update_lock;
>         unsigned long apicv_inhibit_reasons;
> -       atomic_t apicv_irq_window;
> 
>         gpa_t wall_clock;
> 
> 
> I chose that spot before apic_map_lock simply because there was a 4 byte 
> hole there. This happens to also help performance in the AVIC disabled 
> case by a few percentage points (rather, restores the performance in the 
> AVIC disabled case).
> 
> Before this change, I was trying to see if we could entirely elide the 
> rwsem read lock in the specific scenario we are seeing the bottleneck.  
> That is, instead of checking for any other inhibit being set, can we 
> specifically test for PIT_REINJ while setting the IRQWIN inhibit? Then, 
> update the inhibit change logic if PIT_REINJ is cleared to re-check the 
> irq window count.
> 
> There's probably a race here somewhere, but FWIW, along with the above 
> change to 'struct kvm_arch', this helps improve performance by a few 
> more percentage points helping close the gap to within 2% of the AVIC 
> disabled case.

I suspect the issue is that apicv_inhibit_reasons is in the same cache line.  That
field is read on at least every entry

		/*
		 * Assert that vCPU vs. VM APICv state is consistent.  An APICv
		 * update must kick and wait for all vCPUs before toggling the
		 * per-VM state, and responding vCPUs must wait for the update
		 * to complete before servicing KVM_REQ_APICV_UPDATE.
		 */
		WARN_ON_ONCE((kvm_vcpu_apicv_activated(vcpu) != kvm_vcpu_apicv_active(vcpu)) &&
			     (kvm_get_apic_mode(vcpu) != LAPIC_MODE_DISABLED));

and when opening an IRQ window in svm_set_vintr()

	WARN_ON(kvm_vcpu_apicv_activated(&svm->vcpu));

and when handling emulated APIC MMIO in kvm_mmu_faultin_pfn():

		/*
		 * If the APIC access page exists but is disabled, go directly
		 * to emulation without caching the MMIO access or creating a
		 * MMIO SPTE.  That way the cache doesn't need to be purged
		 * when the AVIC is re-enabled.
		 */
		if (!kvm_apicv_activated(vcpu->kvm))
			return RET_PF_EMULATE;

Hmm, now that I think about it, lack of emulated MMIO caching that might explain
the 2% gap.  Do you see the same gap if the guest is using x2APIC?

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Naveen N Rao]

On Tue, Feb 11, 2025 at 08:37:17AM -0800, Sean Christopherson wrote:
> On Tue, Feb 11, 2025, Naveen N Rao wrote:
> > On Wed, Feb 05, 2025 at 12:36:21PM +0100, Paolo Bonzini wrote:
> > I haven't analyzed this yet, but moving apicv_irq_window into a separate 
> > cacheline is improving the performance in my tests by ~7 to 8%:
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 9e3465e70a0a..d8a40ac49226 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1355,6 +1355,9 @@ struct kvm_arch {
> >         struct kvm_ioapic *vioapic;
> >         struct kvm_pit *vpit;
> >         atomic_t vapics_in_nmi_mode;
> > +
> > +       atomic_t apicv_irq_window;
> > +
> >         struct mutex apic_map_lock;
> >         struct kvm_apic_map __rcu *apic_map;
> >         atomic_t apic_map_dirty;
> > @@ -1365,7 +1368,6 @@ struct kvm_arch {
> >         /* Protects apicv_inhibit_reasons */
> >         struct rw_semaphore apicv_update_lock;
> >         unsigned long apicv_inhibit_reasons;
> > -       atomic_t apicv_irq_window;
> > 
> >         gpa_t wall_clock;
> > 
> > 
> > I chose that spot before apic_map_lock simply because there was a 4 byte 
> > hole there. This happens to also help performance in the AVIC disabled 
> > case by a few percentage points (rather, restores the performance in the 
> > AVIC disabled case).
> > 
> > Before this change, I was trying to see if we could entirely elide the 
> > rwsem read lock in the specific scenario we are seeing the bottleneck.  
> > That is, instead of checking for any other inhibit being set, can we 
> > specifically test for PIT_REINJ while setting the IRQWIN inhibit? Then, 
> > update the inhibit change logic if PIT_REINJ is cleared to re-check the 
> > irq window count.
> > 
> > There's probably a race here somewhere, but FWIW, along with the above 
> > change to 'struct kvm_arch', this helps improve performance by a few 
> > more percentage points helping close the gap to within 2% of the AVIC 
> > disabled case.
> 
> I suspect the issue is that apicv_inhibit_reasons is in the same cache line.  That
> field is read on at least every entry
> 
> 		/*
> 		 * Assert that vCPU vs. VM APICv state is consistent.  An APICv
> 		 * update must kick and wait for all vCPUs before toggling the
> 		 * per-VM state, and responding vCPUs must wait for the update
> 		 * to complete before servicing KVM_REQ_APICV_UPDATE.
> 		 */
> 		WARN_ON_ONCE((kvm_vcpu_apicv_activated(vcpu) != kvm_vcpu_apicv_active(vcpu)) &&
> 			     (kvm_get_apic_mode(vcpu) != LAPIC_MODE_DISABLED));
> 
> and when opening an IRQ window in svm_set_vintr()
> 
> 	WARN_ON(kvm_vcpu_apicv_activated(&svm->vcpu));

Possibly, but we also write to apicv_update_lock every time we update 
apicv_irq_window in kvm_inc_or_dec_irq_window_inhibit(), and that is in 
the same cacheline as apicv_inhibit_reasons:

	if (READ_ONCE(kvm->arch.apicv_inhibit_reasons) & ~BIT(APICV_INHIBIT_REASON_IRQWIN)) {
		guard(rwsem_read)(&kvm->arch.apicv_update_lock);
		if (READ_ONCE(kvm->arch.apicv_inhibit_reasons) & ~BIT(APICV_INHIBIT_REASON_IRQWIN)) {
			atomic_add(add, &kvm->arch.apicv_irq_window);
			return;
		}
	}

Also, note that introducing apicv_irq_window after apicv_inhibit_reasons 
is degrading performance in the AVIC disabled case too. So, it is likely 
that some other cacheline below apicv_inhibit_reasons in kvm_arch may 
also be contributing to this.

> 
> and when handling emulated APIC MMIO in kvm_mmu_faultin_pfn():
> 
> 		/*
> 		 * If the APIC access page exists but is disabled, go directly
> 		 * to emulation without caching the MMIO access or creating a
> 		 * MMIO SPTE.  That way the cache doesn't need to be purged
> 		 * when the AVIC is re-enabled.
> 		 */
> 		if (!kvm_apicv_activated(vcpu->kvm))
> 			return RET_PF_EMULATE;
> 
> Hmm, now that I think about it, lack of emulated MMIO caching that might explain
> the 2% gap.  Do you see the same gap if the guest is using x2APIC?

Are you referring to hybrid-AVIC? I am testing this on a Turin system 
that has x2AVIC support, so the guest is running in x2APIC/x2AVIC mode 
(kvm_amd avic=0/avic=1)


- Naveen

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Naveen N Rao]

On Mon, Feb 03, 2025 at 03:46:48PM -0800, Sean Christopherson wrote:
> On Mon, Feb 03, 2025, Paolo Bonzini wrote:
> > On 2/3/25 19:45, Sean Christopherson wrote:
> > > Unless there's a very, very good reason to support a use case that generates
> > > ExtInts during boot, but _only_ during boot, and otherwise doesn't have any APICv
> > > ihibits, I'm leaning towards making SVM's IRQ window inhibit sticky, i.e. never
> > > clear it.
> > 
> > BIOS tends to use PIT, so that may be too much.  With respect to Naveen's report
> > of contention on apicv_update_lock, I would go with the sticky-bit idea but apply
> > it to APICV_INHIBIT_REASON_PIT_REINJ.
> 
> That won't work, at least not with yet more changes, because KVM creates the
> in-kernel PIT with reinjection enabled by default.  The stick-bit idea is that
> if a bit is set and can never be cleared, then there's no need to track new
> updates.  Since userspace needs to explicitly disable reinjection, the inhibit
> can't be sticky.
> 
> I assume We could fudge around that easily enough by deferring the inhibit until
> a vCPU is created (or run?), but piggybacking PIT_REINJ won't help the userspace
> I/O APIC case.

As a separate change, I have been testing a patch that moves the 
PIT_REINJ inhibit from PIT creation to the point at which the guest 
actually programs it so that default guest configurations can utilize 
AVIC:

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index cd57a517d04a..8f959de7ff32 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -235,6 +235,7 @@ static void destroy_pit_timer(struct kvm_pit *pit)
 {
        hrtimer_cancel(&pit->pit_state.timer);
        kthread_flush_work(&pit->expired);
+       kvm_clear_apicv_inhibit(pit->kvm, APICV_INHIBIT_REASON_PIT_REINJ);
 }
 
 static void pit_do_work(struct kthread_work *work)
@@ -296,22 +297,12 @@ void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject)
        if (atomic_read(&ps->reinject) == reinject)
                return;
 
-       /*
-        * AMD SVM AVIC accelerates EOI write and does not trap.
-        * This cause in-kernel PIT re-inject mode to fail
-        * since it checks ps->irq_ack before kvm_set_irq()
-        * and relies on the ack notifier to timely queue
-        * the pt->worker work iterm and reinject the missed tick.
-        * So, deactivate APICv when PIT is in reinject mode.
-        */
        if (reinject) {
-               kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PIT_REINJ);
                /* 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_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PIT_REINJ);
                kvm_unregister_irq_ack_notifier(kvm, &ps->irq_ack_notifier);
                kvm_unregister_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
        }
@@ -358,6 +349,16 @@ static void create_pit_timer(struct kvm_pit *pit, u32 val, int is_period)
                }
        }
 
+       /*
+        * AMD SVM AVIC accelerates EOI write and does not trap. This causes
+        * in-kernel PIT re-inject mode to fail since it checks ps->irq_ack
+        * before kvm_set_irq() and relies on the ack notifier to timely queue
+        * the pt->worker work item and reinject the missed tick. So,
+        * deactivate APICv when PIT is in reinject mode, but only when the
+        * timer is actually armed.
+        */
+       kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PIT_REINJ);
+
        hrtimer_start(&ps->timer, ktime_add_ns(ktime_get(), interval),
                      HRTIMER_MODE_ABS);
 }


Is that reasonable?

If it is, or if we choose to delay PIT_REINJ inhibit to vcpu creation time, 
then making PT_REINJ or IRQWIN inhibits sticky will prevent AVIC from being 
enabled later on. I can see in my tests that BIOS (both seabios and edk2) 
programs the PIT though Linux guest itself doesn't (unless -no-hpet is used).  
The above change will allow AVIC to be used in Linux guests even when a PIT is 
present.

> 
> > I don't love adding another inhibit reason but, together, these two should
> > remove the contention on apicv_update_lock.  Another idea could be to move
> > IRQWIN to per-vCPU reason but Maxim tells me that it's not so easy.
> 
> Oh, yeah, that reminds me of the other reason I would vote for a sticky flag:
> if inhibition really is toggling rapidly, performance is going to be quite bad
> because inhibiting APICv requires (a) zapping APIC SPTEs and (b) serializing
> writers if multiple vCPUs trigger the 0=>1 transition.
> 
> And there's some amount of serialization even if there's only a single writer,
> as KVM kicks all vCPUs to toggle APICv (and again to flush TLBs, if necessary).
> 
> Hmm, something doesn't add up.  Naveen's changelog says:
> 
>   KVM additionally inhibits AVIC for requesting a IRQ window every time it has
>   to inject external interrupts resulting in a barrage of inhibits being set and
>   cleared. This shows significant performance degradation compared to AVIC being
>   disabled, due to high contention on apicv_update_lock.
> 
> But if this is a "real world" use case where the only source of ExtInt is the
> PIT, and kernels typically only wire up the PIT to the BSP, why is there
> contention on apicv_update_lock?  APICv isn't actually being toggled, so readers
> blocking writers to handle KVM_REQ_APICV_UPDATE shouldn't be a problem.
> 
> Naveen, do you know why there's a contention on apicv_update_lock?  Are multiple
> vCPUs actually trying to inject ExtInt?

Apologies for the confusion, I should probably have said "device 
interrupts" (sorry, I'm still trying to get my terminology right). I 
have described the test scenario in the cover letter, but to add more 
details: the test involves two guests on the same host (using network 
tap devices with vhost and mq) with one guest running netperf TCP_RR to 
the other guest. Trimmed qemu command:

qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=on \
  -smp 16,cores=8,threads=2 -cpu host,topoext=on,x2apic=on -m 32G \
  -drive file=~/ubuntu_vm1.img,if=virtio,format=qcow2 \
  -netdev tap,id=v0,br=virbr0,script=~/qemu-ifup,downscript=~/qemu-ifdown,vhost=on,queues=16 \
  -device virtio-net-pci,netdev=v0,mac=52:54:00:12:34:11,mq=on,vectors=34

So, not a real world workload per se, though there may be use cases for guests 
having to handle large number of packets.

You're right -- APICv isn't actually being toggled, but IRQWIN inhibit is 
constantly being set and cleared while trying to inject device interrupts into 
the guests. The places where we set/clear IRQWIN inhibit has comments 
indicating that it is only required for ExtINT, though that's not actually the 
case here.

What is actually happening is that since the PIT is in reinject mode, APICv is 
not active in the guest. When that happens, kvm_cpu_has_injectable_intr() 
returns true when any interrupt is pending:

    /*
     * check if there is injectable interrupt:
     * when virtual interrupt delivery enabled,
     * interrupt from apic will handled by hardware,
     * we don't need to check it here.
     */
    int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v)
    {
	    if (kvm_cpu_has_extint(v))
		    return 1;

	    if (!is_guest_mode(v) && kvm_vcpu_apicv_active(v))
		    return 0;

	    return kvm_apic_has_interrupt(v) != -1; /* LAPIC */
    }

The second if condition fails since APICv is not active. So, 
kvm_check_and_inject_events() calls enable_irq_window() to request for an IRQ 
window to inject those interrupts. That results in all vCPUs trying to acquire 
apicv_update_lock for updating the inhibit reason, though APICv state itself 
isn't changing and we are not requesting for KVM_REQ_APICV_UPDATE.


Thanks,
Naveen

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Paolo Bonzini]

On Tue, Feb 4, 2025 at 12:15 PM Naveen N Rao <naveen@kernel.org> wrote:
> As a separate change, I have been testing a patch that moves the
> PIT_REINJ inhibit from PIT creation to the point at which the guest
> actually programs it so that default guest configurations can utilize
> AVIC:

In-kernel PIC and PIT is sort of a legacy setup; the so-called
"split irqchip" (LAPIC in KVM, PIC/PIT in userspace) is strongly
preferred.  So I don't think it's particularly important to cater
for PIT_REINJ.

> If it is, or if we choose to delay PIT_REINJ inhibit to vcpu creation time,
> then making PT_REINJ or IRQWIN inhibits sticky will prevent AVIC from being
> enabled later on. I can see in my tests that BIOS (both seabios and edk2)
> programs the PIT though Linux guest itself doesn't (unless -no-hpet is used).

Even with -no-hpet, Linux should turn off the PIT relatively soon
and only rely on the local APIC's timer.

> You're right -- APICv isn't actually being toggled, but IRQWIN inhibit is
> constantly being set and cleared while trying to inject device interrupts into
> the guests. The places where we set/clear IRQWIN inhibit has comments
> indicating that it is only required for ExtINT, though that's not actually the
> case here.
>
> What is actually happening is that since the PIT is in reinject mode, APICv is
> not active in the guest. When that happens, kvm_cpu_has_injectable_intr()
> returns true when any interrupt is pending:
>
>     /*
>      * check if there is injectable interrupt:
>      * when virtual interrupt delivery enabled,
>      * interrupt from apic will handled by hardware,
>      * we don't need to check it here.
>      */
>     int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v)
>     {
>             if (kvm_cpu_has_extint(v))
>                     return 1;
>
>             if (!is_guest_mode(v) && kvm_vcpu_apicv_active(v))
>                     return 0;
>
>             return kvm_apic_has_interrupt(v) != -1; /* LAPIC */
>     }
>
> The second if condition fails since APICv is not active. So,
> kvm_check_and_inject_events() calls enable_irq_window() to request for an IRQ
> window to inject those interrupts.

Ok, that's due solely to the presence of *another* active inhibit.
Since sticky inhibits cannot work, making the IRQWIN inhibit per-CPU
will still cause vCPUs to pound on the apicv_update_lock, but only on
the read side of the rwsem so that should be more tolerable.

Using atomics is considerably more complicated and I'd rather avoid it.

Paolo

Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Naveen N Rao]

On Tue, Feb 04, 2025 at 03:08:57PM +0100, Paolo Bonzini wrote:
> On Tue, Feb 4, 2025 at 12:15 PM Naveen N Rao <naveen@kernel.org> wrote:
> > As a separate change, I have been testing a patch that moves the
> > PIT_REINJ inhibit from PIT creation to the point at which the guest
> > actually programs it so that default guest configurations can utilize
> > AVIC:
> 
> In-kernel PIC and PIT is sort of a legacy setup; the so-called
> "split irqchip" (LAPIC in KVM, PIC/PIT in userspace) is strongly
> preferred.  So I don't think it's particularly important to cater
> for PIT_REINJ.

Sure, though it would be nice if we can enable AVIC to function in wider 
configurations especially if the guest doesn't use the PIT :)

> 
> > If it is, or if we choose to delay PIT_REINJ inhibit to vcpu creation time,
> > then making PT_REINJ or IRQWIN inhibits sticky will prevent AVIC from being
> > enabled later on. I can see in my tests that BIOS (both seabios and edk2)
> > programs the PIT though Linux guest itself doesn't (unless -no-hpet is used).
> 
> Even with -no-hpet, Linux should turn off the PIT relatively soon
> and only rely on the local APIC's timer.

I am not seeing that. With -no-hpet, I see that the guest continues to 
use the PIT, as well as the local APIC timer.

> 
> > You're right -- APICv isn't actually being toggled, but IRQWIN inhibit is
> > constantly being set and cleared while trying to inject device interrupts into
> > the guests. The places where we set/clear IRQWIN inhibit has comments
> > indicating that it is only required for ExtINT, though that's not actually the
> > case here.
> >
> > What is actually happening is that since the PIT is in reinject mode, APICv is
> > not active in the guest. When that happens, kvm_cpu_has_injectable_intr()
> > returns true when any interrupt is pending:
> >
> >     /*
> >      * check if there is injectable interrupt:
> >      * when virtual interrupt delivery enabled,
> >      * interrupt from apic will handled by hardware,
> >      * we don't need to check it here.
> >      */
> >     int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v)
> >     {
> >             if (kvm_cpu_has_extint(v))
> >                     return 1;
> >
> >             if (!is_guest_mode(v) && kvm_vcpu_apicv_active(v))
> >                     return 0;
> >
> >             return kvm_apic_has_interrupt(v) != -1; /* LAPIC */
> >     }
> >
> > The second if condition fails since APICv is not active. So,
> > kvm_check_and_inject_events() calls enable_irq_window() to request for an IRQ
> > window to inject those interrupts.
> 
> Ok, that's due solely to the presence of *another* active inhibit.
> Since sticky inhibits cannot work, making the IRQWIN inhibit per-CPU
> will still cause vCPUs to pound on the apicv_update_lock, but only on
> the read side of the rwsem so that should be more tolerable.
> 
> Using atomics is considerably more complicated and I'd rather avoid it.

Understood, thanks!


- Naveen