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 <[email protected]>
> Cc: Jim Mattson <[email protected]>
> Cc: Mingwei Zhang <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
>
> 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 :/