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

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

[Mi, Dapeng]

On 5/9/2026 7:40 AM, sashiko-bot@kernel.org 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 <seanjc@google.com>
>
> 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.  

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. 

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

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;

>