Re: [PATCH v3 1/9] perf/x86/intel: Ensure guest PEBS path doesn't set unwanted PERF_GLOBAL_CTRL bits

On Tue, May 12, 2026, Dapeng Mi wrote:
> 
> On 5/9/2026 7:40 AM, [email protected] wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [High] The host PEBS collision check fails to mask non-counter bits, incorrectly disabling guest PEBS for events like Store Latency.
> > --
> >
> > Note: The format of this report has been altered due to recitation
> > restrictions. The original patch code is not quoted directly, and findings are
> > provided as a free-form summary.
> >
> > commit 39adc53b360e88595a3be6fc59b18b048d9e9921
> > Author: Sean Christopherson <[email protected]>
> >
> > perf/x86/intel: Ensure guest PEBS path doesn't set unwanted PERF_GLOBAL_CTRL bits
> >
> > This commit masks the pebs counter values being reinstated into the global
> > control msr for a guest with perf's desired value. This is done to ensure the
> > hypervisor doesn't unintentionally enable counters.
> >
> > In intel_guest_get_msrs(), the host pebs collision check evaluates the host
> > field of the pebs_enable array entry to determine if the host is actively
> > using pebs. If it is, guest pebs is disabled.
> >
> > Is it possible for this check to falsely detect a host collision due to
> > non-counter bits being set?
> >
> > When a guest configures a store latency pebs event, a backing perf event is
> > created. In intel_pmu_pebs_enable(), this sets both the counter index bit
> > and bit 63 in cpuc->pebs_enabled.
> >
> > Because the host field is calculated as cpuc->pebs_enabled masked against
> > the inverse of cpuc->intel_ctrl_guest_mask, and the guest mask exclusively
> > tracks valid counter indices (never including bit 63), bit 63 remains set in
> > the host field.
> >
> > Consequently, the check for whether the host is actively using pebs will
> > evaluate to true even when only the guest is using pebs. This introduces a
> > regression where guest pebs is completely disabled, silently breaking guest
> > store latency events.
> >
> > Should the host pebs usage evaluation also be masked by intel_ctrl to prevent
> > non-counter bits from triggering a false collision?
> 
> Hmm, I suspect if the issue could happen on real hardwares.  

OMG, it took me _so_ long to understand what Sashiko was complaining about.  I'm
pretty sure I read Sashiko's response like four times, and it didn't click until
I read your response.

In case anyone is feeling as dense as me, Sashiko is saying this:

	/*
	 * FIXME: Allow guest and host usage of PEBS events to co-exist instead
	 *        of disabling guest PEBS entirely if the host is using PEBS.
	 *        What exactly goes wrong if guest and host are using PEBS is
	 *        unknown.
	 */
	if (pebs_mask & ~cpuc->intel_ctrl_exclude_host_mask)
		guest_pebs_mask = 0;

should probably be masked by intel_ctrl as well.

> The function intel_pmu_pebs_enable() indeed sets extra bits into
> cpuc->pebs_enabled for load latency and specific store events, like below
> code shows.
> 
> ```
> 
> ......
> 
>     if ((event->hw.flags & PERF_X86_EVENT_PEBS_LDLAT) && (x86_pmu.version < 5))
>         cpuc->pebs_enabled |= 1ULL << (hwc->idx + 32);
>     else if (event->hw.flags & PERF_X86_EVENT_PEBS_ST)
>         cpuc->pebs_enabled |= 1ULL << 63;
> 
> ......
> 
> ```
> 
> But these 2 cases should only be hit on quite old platforms (prior to
> Icelake). On these platforms, only GP counters support PEBS sampling and
> pebs_capable would be set PEBS_COUNTER_MASK, and so these extra bits would
> be filtered out by the pebs_capable and pebs_mask won't really contain
> these extra bits. 

Thanks for digging into this!  I was staring and just going "huh!?" over and over
in my head :-)

> Anyway, we could optimize the code further like below and thoroughly filter
> away these extra bits. (only building, not test on real HW)

Hmm, I think I'd rather figure out what it would take to drop the FIXME entirely.
And if we need to keep the check, I'm a-ok risking false positives until we have
a better understanding of why the check exists.

> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 854881b5e696..6eee7636a822 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -4997,7 +4997,7 @@ static struct perf_guest_switch_msr
> *intel_guest_get_msrs(int *nr,
>         struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>         struct perf_guest_switch_msr *arr = cpuc->guest_switch_msrs;
>         u64 intel_ctrl = hybrid(cpuc->pmu, intel_ctrl);
> -       u64 pebs_mask = cpuc->pebs_enabled & x86_pmu.pebs_capable;
> +       u64 pebs_mask = cpuc->pebs_enabled & x86_pmu.pebs_capable & intel_ctrl;
>         u64 guest_pebs_mask;
>         int global_ctrl;
> 
> @@ -5049,7 +5049,7 @@ static struct perf_guest_switch_msr
> *intel_guest_get_msrs(int *nr,
>          * the guest wants to use for PEBS, (c) are not excluded from counting
>          * in the guest, and (d) _are_ excluded from counting in the host.
>          */
> -       guest_pebs_mask = pebs_mask & intel_ctrl & guest_pebs->enable &
> +       guest_pebs_mask = pebs_mask & guest_pebs->enable &
>                           ~cpuc->intel_ctrl_exclude_guest_mask &
>                           cpuc->intel_ctrl_exclude_host_mask;
> 
> >