perf/x86: Don't enforce minimum period for KVM guest-only events

  Don't apply minimum period workarounds/requirements to events that are
being created by KVM to virtualize PMCs for guests, i.e. skip limit
enforcement for events that exclude the host.  Perf's somewhat arbitrary
limits prevents KVM from correctly virtualizing counter overflow, e.g. if
the guest sets a counter to have an effective period of '1', forcing a
minimum period of '2' results in overflow occurring at the incorrect time.

Whether or not a "real" profiling use case is affected is debatable, but
the incorrect behavior is trivially easy to observe and reproduce, and is
deterministic enough to make the PMU appear to be broken from the guest's
perspective.

Furthermore, the "period" set by KVM isn't actually a period, as KVM won't
automatically reprogram the event with the same period on overflow.  KVM
will synthesize a PMI into the guest when appropriate, but what the guest
does in response to the PMI is purely a guest decision.  In other words,
KVM effectively operates in a one-shot mode, not a periodic mode.

Letting KVM and/or the guest program "too small" periods is safe for the
host, as events that exclude the host are atomically disabled with respect
to VM-Exit, i.e. are guaranteed to stop counting upon transitioning to the
host.  And whether or not *explicitly* programming a short period is safe
is somewhat of a moot point, as transitions to/from the guest effectively
yield the same effect, e.g. an unrelated VM-Exit => VM-Enter transition
will re-enable guest PMCs with whatever count happened to be in the PMC at
the time of VM-Exit.

Cc: Like Xu <likexu@tencent.com>
Cc: Jim Mattson <jmattson@google.com>
Cc: Mingwei Zhang <mizhang@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---

Disclaimer: I've only tested this from KVM's side of things.

 arch/x86/events/core.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)


base-commit: 744940f1921c8feb90e3c4bcc1e153fdd6e10fe2
  

On Tue, Nov 07, 2023, Sean Christopherson wrote:
> Don't apply minimum period workarounds/requirements to events that are
> being created by KVM to virtualize PMCs for guests, i.e. skip limit
> enforcement for events that exclude the host.  Perf's somewhat arbitrary
> limits prevents KVM from correctly virtualizing counter overflow, e.g. if
> the guest sets a counter to have an effective period of '1', forcing a
> minimum period of '2' results in overflow occurring at the incorrect time.
> 
> Whether or not a "real" profiling use case is affected is debatable, but
> the incorrect behavior is trivially easy to observe and reproduce, and is
> deterministic enough to make the PMU appear to be broken from the guest's
> perspective.
> 
> Furthermore, the "period" set by KVM isn't actually a period, as KVM won't
> automatically reprogram the event with the same period on overflow.  KVM
> will synthesize a PMI into the guest when appropriate, but what the guest
> does in response to the PMI is purely a guest decision.  In other words,
> KVM effectively operates in a one-shot mode, not a periodic mode.
> 
> Letting KVM and/or the guest program "too small" periods is safe for the
> host, as events that exclude the host are atomically disabled with respect
> to VM-Exit, i.e. are guaranteed to stop counting upon transitioning to the
> host.  And whether or not *explicitly* programming a short period is safe
> is somewhat of a moot point, as transitions to/from the guest effectively
> yield the same effect, e.g. an unrelated VM-Exit => VM-Enter transition
> will re-enable guest PMCs with whatever count happened to be in the PMC at
> the time of VM-Exit.
> 
> Cc: Like Xu <likexu@tencent.com>
> Cc: Jim Mattson <jmattson@google.com>
> Cc: Mingwei Zhang <mizhang@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> 
> Disclaimer: I've only tested this from KVM's side of things.
> 
>  arch/x86/events/core.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 40ad1425ffa2..f8a8a4ea4d47 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -1388,16 +1388,25 @@ int x86_perf_event_set_period(struct perf_event *event)
>  		hwc->last_period = period;
>  		ret = 1;
>  	}
> -	/*
> -	 * Quirk: certain CPUs dont like it if just 1 hw_event is left:
> -	 */
> -	if (unlikely(left < 2))
> -		left = 2;
>  
>  	if (left > x86_pmu.max_period)
>  		left = x86_pmu.max_period;
>  
> -	static_call_cond(x86_pmu_limit_period)(event, &left);
> +	/*
> +	 * Exempt KVM guest events from the minimum period requirements.  It's
> +	 * the guest's responsibility to ensure it can make forward progress,
> +	 * and it's KVM's responsibility to configure an appropriate "period"
> +	 * to correctly virtualize overflow for the guest's PMCs.
> +	 */
> +	if (!event->attr.exclude_host) {
> +		/*
> +		 * Quirk: certain CPUs dont like it if just 1 event is left:
> +		 */
> +		if (unlikely(left < 2))
> +			left = 2;
> +
> +		static_call_cond(x86_pmu_limit_period)(event, &left);
> +	}
>  
>  	this_cpu_write(pmc_prev_left[idx], left);
>  

Nice one. I am curious how you tested this one? I would like to
reproduce that one on my side.

>
> base-commit: 744940f1921c8feb90e3c4bcc1e153fdd6e10fe2
> -- 
> 2.42.0.869.gea05f2083d-goog
>
  

On Tue, Nov 07, 2023, Mingwei Zhang wrote:
> On Tue, Nov 07, 2023, Sean Christopherson wrote:
> >  arch/x86/events/core.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> > index 40ad1425ffa2..f8a8a4ea4d47 100644
> > --- a/arch/x86/events/core.c
> > +++ b/arch/x86/events/core.c
> > @@ -1388,16 +1388,25 @@ int x86_perf_event_set_period(struct perf_event *event)
> >  		hwc->last_period = period;
> >  		ret = 1;
> >  	}
> > -	/*
> > -	 * Quirk: certain CPUs dont like it if just 1 hw_event is left:
> > -	 */
> > -	if (unlikely(left < 2))
> > -		left = 2;
> >  
> >  	if (left > x86_pmu.max_period)
> >  		left = x86_pmu.max_period;
> >  
> > -	static_call_cond(x86_pmu_limit_period)(event, &left);
> > +	/*
> > +	 * Exempt KVM guest events from the minimum period requirements.  It's
> > +	 * the guest's responsibility to ensure it can make forward progress,
> > +	 * and it's KVM's responsibility to configure an appropriate "period"
> > +	 * to correctly virtualize overflow for the guest's PMCs.
> > +	 */
> > +	if (!event->attr.exclude_host) {
> > +		/*
> > +		 * Quirk: certain CPUs dont like it if just 1 event is left:
> > +		 */
> > +		if (unlikely(left < 2))
> > +			left = 2;
> > +
> > +		static_call_cond(x86_pmu_limit_period)(event, &left);
> > +	}
> >  
> >  	this_cpu_write(pmc_prev_left[idx], left);
> >  
> 
> Nice one. I am curious how you tested this one? I would like to
> reproduce that one on my side.

The check_emulated_instr() sub-test in KVM-Unit-Tests's x86/pmu.c fails when run
with "my" (which is really yours) fix for the KVM's handling of emulated PMC
events[*].  If KVM synthesizes an "instructions retired" event that bumps the
PMC to all ones, i.e. -1 for all intents and purposes, the test fails because
KVM creates a sample_period of '1', but perf programs a period of '2'.

I suspect a very simple test of writing -1 to a PMC from the guest would exhibit
the same behavior.

[*] https://lkml.kernel.org/r/ZUWAg3WP2XESCAR4%40google.com
  

On Tue, Nov 7, 2023 at 3:02 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Nov 07, 2023, Mingwei Zhang wrote:
> > On Tue, Nov 07, 2023, Sean Christopherson wrote:
> > >  arch/x86/events/core.c | 21 +++++++++++++++------
> > >  1 file changed, 15 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> > > index 40ad1425ffa2..f8a8a4ea4d47 100644
> > > --- a/arch/x86/events/core.c
> > > +++ b/arch/x86/events/core.c
> > > @@ -1388,16 +1388,25 @@ int x86_perf_event_set_period(struct perf_event *event)
> > >             hwc->last_period = period;
> > >             ret = 1;
> > >     }
> > > -   /*
> > > -    * Quirk: certain CPUs dont like it if just 1 hw_event is left:
> > > -    */
> > > -   if (unlikely(left < 2))
> > > -           left = 2;
> > >
> > >     if (left > x86_pmu.max_period)
> > >             left = x86_pmu.max_period;
> > >
> > > -   static_call_cond(x86_pmu_limit_period)(event, &left);
> > > +   /*
> > > +    * Exempt KVM guest events from the minimum period requirements.  It's
> > > +    * the guest's responsibility to ensure it can make forward progress,
> > > +    * and it's KVM's responsibility to configure an appropriate "period"
> > > +    * to correctly virtualize overflow for the guest's PMCs.
> > > +    */
> > > +   if (!event->attr.exclude_host) {
> > > +           /*
> > > +            * Quirk: certain CPUs dont like it if just 1 event is left:
> > > +            */
> > > +           if (unlikely(left < 2))
> > > +                   left = 2;
> > > +
> > > +           static_call_cond(x86_pmu_limit_period)(event, &left);
> > > +   }
> > >
> > >     this_cpu_write(pmc_prev_left[idx], left);
> > >
> >
> > Nice one. I am curious how you tested this one? I would like to
> > reproduce that one on my side.
>
> The check_emulated_instr() sub-test in KVM-Unit-Tests's x86/pmu.c fails when run
> with "my" (which is really yours) fix for the KVM's handling of emulated PMC
> events[*].  If KVM synthesizes an "instructions retired" event that bumps the
> PMC to all ones, i.e. -1 for all intents and purposes, the test fails because
> KVM creates a sample_period of '1', but perf programs a period of '2'.
>
> I suspect a very simple test of writing -1 to a PMC from the guest would exhibit
> the same behavior.
>
> [*] https://lkml.kernel.org/r/ZUWAg3WP2XESCAR4%40google.com

Nice, I will try that and see if I can reproduce. Will give
Reviewed-by after testing it on my side.

Thanks.
-Mingwei
  

On Tue, Nov 07, 2023 at 10:36:05AM -0800, Sean Christopherson wrote:
> Don't apply minimum period workarounds/requirements to events that are
> being created by KVM to virtualize PMCs for guests, i.e. skip limit
> enforcement for events that exclude the host.  Perf's somewhat arbitrary
> limits prevents KVM from correctly virtualizing counter overflow, e.g. if
> the guest sets a counter to have an effective period of '1', forcing a
> minimum period of '2' results in overflow occurring at the incorrect time.
> 
> Whether or not a "real" profiling use case is affected is debatable, but
> the incorrect behavior is trivially easy to observe and reproduce, and is
> deterministic enough to make the PMU appear to be broken from the guest's
> perspective.
> 
> Furthermore, the "period" set by KVM isn't actually a period, as KVM won't
> automatically reprogram the event with the same period on overflow.  KVM
> will synthesize a PMI into the guest when appropriate, but what the guest
> does in response to the PMI is purely a guest decision.  In other words,
> KVM effectively operates in a one-shot mode, not a periodic mode.
> 
> Letting KVM and/or the guest program "too small" periods is safe for the
> host, as events that exclude the host are atomically disabled with respect
> to VM-Exit, i.e. are guaranteed to stop counting upon transitioning to the
> host.  And whether or not *explicitly* programming a short period is safe
> is somewhat of a moot point, as transitions to/from the guest effectively
> yield the same effect, e.g. an unrelated VM-Exit => VM-Enter transition
> will re-enable guest PMCs with whatever count happened to be in the PMC at
> the time of VM-Exit.
> 
> Cc: Like Xu <likexu@tencent.com>
> Cc: Jim Mattson <jmattson@google.com>
> Cc: Mingwei Zhang <mizhang@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> 
> Disclaimer: I've only tested this from KVM's side of things.
> 
>  arch/x86/events/core.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 40ad1425ffa2..f8a8a4ea4d47 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -1388,16 +1388,25 @@ int x86_perf_event_set_period(struct perf_event *event)
>  		hwc->last_period = period;
>  		ret = 1;
>  	}
> -	/*
> -	 * Quirk: certain CPUs dont like it if just 1 hw_event is left:
> -	 */
> -	if (unlikely(left < 2))
> -		left = 2;
>  
>  	if (left > x86_pmu.max_period)
>  		left = x86_pmu.max_period;
>  
> -	static_call_cond(x86_pmu_limit_period)(event, &left);
> +	/*
> +	 * Exempt KVM guest events from the minimum period requirements.  It's
> +	 * the guest's responsibility to ensure it can make forward progress,
> +	 * and it's KVM's responsibility to configure an appropriate "period"
> +	 * to correctly virtualize overflow for the guest's PMCs.
> +	 */
> +	if (!event->attr.exclude_host) {
> +		/*
> +		 * Quirk: certain CPUs dont like it if just 1 event is left:
> +		 */
> +		if (unlikely(left < 2))
> +			left = 2;
> +
> +		static_call_cond(x86_pmu_limit_period)(event, &left);
> +	}

Hmm, IIRC we can disable that left < 2 thing for anything that doesn't
have x86_pmu.pebs_no_isolation IIRC.

I'm not sure about taking out the limit_period call, why does it make
sense to allow the guest to program obviously invalid settings?

That is, would something like the below work for you?

---
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 40ad1425ffa2..5543a0bab1f8 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -152,6 +152,14 @@ u64 x86_perf_event_update(struct perf_event *event)
 	return new_raw_count;
 }
 
+static void x86_perf_limit_period(struct perf_event *event, s64 *left)
+{
+	/*
+	 * Quirk: certain CPUs dont like it if just 1 hw_event is left:
+	 */
+	*left = max(*left, 2);
+}
+
 /*
  * Find and validate any extra registers to set up.
  */
@@ -1388,11 +1396,6 @@ int x86_perf_event_set_period(struct perf_event *event)
 		hwc->last_period = period;
 		ret = 1;
 	}
-	/*
-	 * Quirk: certain CPUs dont like it if just 1 hw_event is left:
-	 */
-	if (unlikely(left < 2))
-		left = 2;
 
 	if (left > x86_pmu.max_period)
 		left = x86_pmu.max_period;
@@ -2130,6 +2133,10 @@ static int __init init_hw_perf_events(void)
 	if (!x86_pmu.update)
 		x86_pmu.update = x86_perf_event_update;
 
+	// XXX check non-Intel
+	if (!x86_pmu.limit_period && x86_pmu.pebs_no_isolation)
+		x86_pmu.limit_update = x86_perf_limit_period;
+
 	x86_pmu_static_call_update();
 
 	/*
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index a08f794a0e79..9fe0f241779e 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -4471,7 +4471,10 @@ static void bdw_limit_period(struct perf_event *event, s64 *left)
 		if (*left < 128)
 			*left = 128;
 		*left &= ~0x3fULL;
+		return;
 	}
+	if (unlikely(x86_pmu.pebs_no_isolation))
+		*left = max(*left, 2);
 }
 
 static void nhm_limit_period(struct perf_event *event, s64 *left)
  

On Fri, Nov 17, 2023, Peter Zijlstra wrote:
> On Tue, Nov 07, 2023 at 10:36:05AM -0800, Sean Christopherson wrote:
> > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> > index 40ad1425ffa2..f8a8a4ea4d47 100644
> > --- a/arch/x86/events/core.c
> > +++ b/arch/x86/events/core.c
> > @@ -1388,16 +1388,25 @@ int x86_perf_event_set_period(struct perf_event *event)
> >  		hwc->last_period = period;
> >  		ret = 1;
> >  	}
> > -	/*
> > -	 * Quirk: certain CPUs dont like it if just 1 hw_event is left:
> > -	 */
> > -	if (unlikely(left < 2))
> > -		left = 2;
> >  
> >  	if (left > x86_pmu.max_period)
> >  		left = x86_pmu.max_period;
> >  
> > -	static_call_cond(x86_pmu_limit_period)(event, &left);
> > +	/*
> > +	 * Exempt KVM guest events from the minimum period requirements.  It's
> > +	 * the guest's responsibility to ensure it can make forward progress,
> > +	 * and it's KVM's responsibility to configure an appropriate "period"
> > +	 * to correctly virtualize overflow for the guest's PMCs.
> > +	 */
> > +	if (!event->attr.exclude_host) {
> > +		/*
> > +		 * Quirk: certain CPUs dont like it if just 1 event is left:
> > +		 */
> > +		if (unlikely(left < 2))
> > +			left = 2;
> > +
> > +		static_call_cond(x86_pmu_limit_period)(event, &left);
> > +	}
> 
> Hmm, IIRC we can disable that left < 2 thing for anything that doesn't
> have x86_pmu.pebs_no_isolation IIRC.
> 
> I'm not sure about taking out the limit_period call, why does it make
> sense to allow the guest to program obviously invalid settings?

I don't see how the guest behavior is obviously invalid.  Architecturally, writing
-1 to a counter should result in overflow after a single event.  Underlying uarch
goofiness shouldn't enter into that equation.

Honoring the guest's programming *might* cause oddness for the guest, whereas
not honoring the architecture is guaranteed to cause visible issues.

If programming a "period" of 1 puts the host at risk in some way, then I agree
that this is unsafe and we need a different solution.  But if the worst case
scenario is non-determinstic or odd behavior from the guest's perspective, then
that's the guest's problem (with the caveat that the guest might not have accurate
Family/Model/Stepping data to make informed decisions).

> That is, would something like the below work for you?

No, because the fix ideally wouldn't require fancy hardware, i.e. would work for
all CPUs for which KVM supports a virtual PMU.
  

On Tue, Nov 28, 2023 at 05:33:16PM -0800, Sean Christopherson wrote:

> If programming a "period" of 1 puts the host at risk in some way, then I agree
> that this is unsafe and we need a different solution. 

IIRC if you put in -1 on a Nehalem, you end up with an NMI-storm which
wasn't trivial to recover from if at all (it's too long ago and I don't
have ancient hardware like that anymore :/)

> But if the worst case
> scenario is non-determinstic or odd behavior from the guest's perspective, then
> that's the guest's problem (with the caveat that the guest might not have accurate
> Family/Model/Stepping data to make informed decisions).

Things like bdm_limit_period() will cause odd behaviour IIRC, it does
daft things like generate extra PEBS records on overflow and gives
otherwise daft results for PDIR.

glc_limit_period() lacks a useful comment :/