[PATCH] KVM: x86/pmu: Don't retry a counter whose config was rejected

[PATCH] KVM: x86/pmu: Don't retry a counter whose config was rejected

[Luka Absandze]

kvm_pmu_handle_event() re-arms the reprogram bit for every failed
reprogram, on the assumption that the failure is transient and a later
refresh will succeed.  That is true for contention, e.g. the -EBUSY from
x86_reserve_hardware(), but not for a configuration the host PMU driver
rejects outright.  A rejected config can never succeed on retry, so the
counter is reprogrammed on every PMU refresh for as long as the guest
leaves it enabled, and every attempt fails the same way.

Skip the re-arm for -EINVAL, one of the errnos the x86 PMU drivers use
for a config they will never accept.  Note this becomes reachable on AMD
only with the patch linked below, which starts rejecting the Merge event
(PMCxFFF) a guest programs as part of a Large Increment per Cycle pair;
on Intel it is already reachable today via the INTEL_FIXED_VLBR_EVENT
check in intel_pmu_hw_config(), where the config is likewise a function
of fixed guest state and can never start being accepted.

Link: https://lore.kernel.org/all/20260916123315.89042-1-absandze@amazon.de/
Signed-off-by: Luka Absandze <absandze@amazon.de>
---
 arch/x86/kvm/pmu.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index a7d60c8785cd..b9945a6256ed 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -680,8 +680,14 @@ void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
 		 * reprogram bit, i.e. opportunistically try again on the next
 		 * PMU refresh.  Don't make a new request as doing so can stall
 		 * the guest if reprogramming repeatedly fails.
+		 *
+		 * -EINVAL means the event's config was rejected outright and
+		 * can never succeed on retry, so don't re-arm; the guest can
+		 * still do so itself by rewriting the event selector.
 		 */
-		if (reprogram_counter(pmc))
+		int r = reprogram_counter(pmc);
+
+		if (r && r != -EINVAL)
 			set_bit(pmc->idx, pmu->reprogram_pmi);
 	}
 
-- 
2.47.3

Re: [PATCH] KVM: x86/pmu: Don't retry a counter whose config was rejected

[Sean Christopherson]

On Fri, Sep 18, 2026, Luka Absandze wrote:
> kvm_pmu_handle_event() re-arms the reprogram bit for every failed
> reprogram, on the assumption that the failure is transient and a later
> refresh will succeed.  That is true for contention, e.g. the -EBUSY from
> x86_reserve_hardware(), but not for a configuration the host PMU driver
> rejects outright.  A rejected config can never succeed on retry, so the
> counter is reprogrammed on every PMU refresh for as long as the guest
> leaves it enabled, and every attempt fails the same way.
> 
> Skip the re-arm for -EINVAL, one of the errnos the x86 PMU drivers use
> for a config they will never accept.  Note this becomes reachable on AMD
> only with the patch linked below, which starts rejecting the Merge event
> (PMCxFFF) a guest programs as part of a Large Increment per Cycle pair;
> on Intel it is already reachable today via the INTEL_FIXED_VLBR_EVENT
> check in intel_pmu_hw_config(), where the config is likewise a function
> of fixed guest state and can never start being accepted.
> 
> Link: https://lore.kernel.org/all/20260916123315.89042-1-absandze@amazon.de/
> Signed-off-by: Luka Absandze <absandze@amazon.de>
> ---
>  arch/x86/kvm/pmu.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index a7d60c8785cd..b9945a6256ed 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -680,8 +680,14 @@ void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
>  		 * reprogram bit, i.e. opportunistically try again on the next
>  		 * PMU refresh.  Don't make a new request as doing so can stall
>  		 * the guest if reprogramming repeatedly fails.
> +		 *
> +		 * -EINVAL means the event's config was rejected outright and
> +		 * can never succeed on retry, so don't re-arm; the guest can
> +		 * still do so itself by rewriting the event selector.
>  		 */
> -		if (reprogram_counter(pmc))
> +		int r = reprogram_counter(pmc);
> +
> +		if (r && r != -EINVAL)
>  			set_bit(pmc->idx, pmu->reprogram_pmi);

Hmm, would it instead make more sense to explicitly check for -EBUSY?  -EINVAL
isn't the only fatal error code.

diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index a7d60c8785cd..5296ee8f32af 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -681,7 +681,7 @@ void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
 		 * PMU refresh.  Don't make a new request as doing so can stall
 		 * the guest if reprogramming repeatedly fails.
 		 */
-		if (reprogram_counter(pmc))
+		if (reprogram_counter(pmc) == -EBUSY)
 			set_bit(pmc->idx, pmu->reprogram_pmi);
 	}
 
Though I guess one could argue -ENOMEM is also transient?  I definitely prefer
an "allow"-list though.  And I find it easier to read if 'r' is declared outside
the loop (and less of a chance of variable shadowing; the odds of returning a
stale value are quite low given there is no return value, and probably never will
be a return value).

E.g. this? (completely untested)

diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index a7d60c8785cd..03a470c49a74 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -662,7 +662,7 @@ void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
 	DECLARE_BITMAP(bitmap, X86_PMC_IDX_MAX);
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
 	struct kvm_pmc *pmc;
-	int bit;
+	int bit, r;
 
 	bitmap_copy(bitmap, pmu->reprogram_pmi, X86_PMC_IDX_MAX);
 
@@ -676,12 +676,14 @@ void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
 
 	kvm_for_each_pmc(pmu, pmc, bit, bitmap) {
 		/*
-		 * If reprogramming fails, e.g. due to contention, re-set the
-		 * reprogram bit, i.e. opportunistically try again on the next
-		 * PMU refresh.  Don't make a new request as doing so can stall
-		 * the guest if reprogramming repeatedly fails.
+		 * If reprogramming fails on a transient condition, e.g. due to
+		 * contention, re-set the reprogram bit, i.e. opportunistically
+		 * try again on the next PMU refresh.  Don't make a new request
+		 * as doing so can stall the guest if reprogramming repeatedly
+		 * fails.
 		 */
-		if (reprogram_counter(pmc))
+		r = reprogram_counter(pmc);
+		if (r == -EBUSY || r == -ENOMEM)
 			set_bit(pmc->idx, pmu->reprogram_pmi);
 	}
 

Re: [PATCH] KVM: x86/pmu: Don't retry a counter whose config was rejected

[Absandze, Luka]

On 2026-09-18 09:16, Sean Christopherson wrote:
> Hmm, would it instead make more sense to explicitly check for -EBUSY?  -EINVAL
> isn't the only fatal error code.
> 
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index a7d60c8785cd..5296ee8f32af 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -681,7 +681,7 @@ void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
>                  * PMU refresh.  Don't make a new request as doing so can stall
>                  * the guest if reprogramming repeatedly fails.
>                  */
> -               if (reprogram_counter(pmc))
> +               if (reprogram_counter(pmc) == -EBUSY)
>                         set_bit(pmc->idx, pmu->reprogram_pmi);
>         }
> 
> Though I guess one could argue -ENOMEM is also transient?

I had contemplated this, but could not convince myself at a glance that
perf and KVM agreed on what constituted a transient error.
I have now concluded I was seeing ghosts :)

> E.g. this? (completely untested)
> [diff]

Tested against the fault described in the linked patch and selftests on
AMD Zen 3 (although I don't think there's much coverage in this regard).

If no other objections, would you be fine with taking the diff you
provided into the tree or would you prefer a v2?

Re: [PATCH] KVM: x86/pmu: Don't retry a counter whose config was rejected

[Sean Christopherson]

On Fri, Sep 18, 2026, Luka Absandze wrote:
> On 2026-09-18 09:16, Sean Christopherson wrote:
> > Hmm, would it instead make more sense to explicitly check for -EBUSY?  -EINVAL
> > isn't the only fatal error code.
> > 
> > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> > index a7d60c8785cd..5296ee8f32af 100644
> > --- a/arch/x86/kvm/pmu.c
> > +++ b/arch/x86/kvm/pmu.c
> > @@ -681,7 +681,7 @@ void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
> >                  * PMU refresh.  Don't make a new request as doing so can stall
> >                  * the guest if reprogramming repeatedly fails.
> >                  */
> > -               if (reprogram_counter(pmc))
> > +               if (reprogram_counter(pmc) == -EBUSY)
> >                         set_bit(pmc->idx, pmu->reprogram_pmi);
> >         }
> > 
> > Though I guess one could argue -ENOMEM is also transient?
> 
> I had contemplated this, but could not convince myself at a glance that
> perf and KVM agreed on what constituted a transient error.
> I have now concluded I was seeing ghosts :)
> 
> > E.g. this? (completely untested)
> > [diff]
> 
> Tested against the fault described in the linked patch and selftests on
> AMD Zen 3 (although I don't think there's much coverage in this regard).
> 
> If no other objections, would you be fine with taking the diff you
> provided into the tree or would you prefer a v2?

Go ahead and send a v2, I want to see what Sashiko thinks.

Re: [PATCH] KVM: x86/pmu: Don't retry a counter whose config was rejected

[Sandipan Das]

On 19-09-2026 03:09, Sean Christopherson wrote:
> On Fri, Sep 18, 2026, Luka Absandze wrote:
>> On 2026-09-18 09:16, Sean Christopherson wrote:
>>> Hmm, would it instead make more sense to explicitly check for -EBUSY?  -EINVAL
>>> isn't the only fatal error code.
>>>
>>> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
>>> index a7d60c8785cd..5296ee8f32af 100644
>>> --- a/arch/x86/kvm/pmu.c
>>> +++ b/arch/x86/kvm/pmu.c
>>> @@ -681,7 +681,7 @@ void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
>>>                  * PMU refresh.  Don't make a new request as doing so can stall
>>>                  * the guest if reprogramming repeatedly fails.
>>>                  */
>>> -               if (reprogram_counter(pmc))
>>> +               if (reprogram_counter(pmc) == -EBUSY)
>>>                         set_bit(pmc->idx, pmu->reprogram_pmi);
>>>         }
>>>
>>> Though I guess one could argue -ENOMEM is also transient?
>>
>> I had contemplated this, but could not convince myself at a glance that
>> perf and KVM agreed on what constituted a transient error.
>> I have now concluded I was seeing ghosts :)
>>
>>> E.g. this? (completely untested)
>>> [diff]
>>
>> Tested against the fault described in the linked patch and selftests on
>> AMD Zen 3 (although I don't think there's much coverage in this regard).
>>
>> If no other objections, would you be fine with taking the diff you
>> provided into the tree or would you prefer a v2?
> 
> Go ahead and send a v2, I want to see what Sashiko thinks.

From a quick glance, it seems EBUSY and ENOMEM might just be sufficient.
The former occurs when a pinned event cannot be created cause all counters
are exhausted and the latter due to various event and context struct
related alloc failures.