Re: [Patch v8 20/23] perf/x86/intel: Support arch-PEBS based SIMD/eGPRs/SSP sampling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Requesting the SSP (Shadow Stack Pointer) register improperly allows large PEBS to be enabled on legacy PEBS hardware, resulting in stale and identical SSP values across all samples in the buffer.
--

commit 2a27d3f31fac15c7d594a6588f8dacfa2939bc9f
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86/intel: Support arch-PEBS based SIMD/eGPRs/SSP sampling

This commit adds support for sampling SIMD, eGPRs, and SSP registers using
arch-PEBS. It defines the layout for these extended registers and manages
their sampling via MSRs and the XSTATE_BV bitmap.

> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c

[ ... ]

> @@ -4695,21 +4710,113 @@ static void intel_pebs_aliases_skl(struct perf_event *event)

[ ... ]

> +static bool __regs_support_large_pebs(struct perf_event *event, bool intr)
> +{
> +	u64 regs = intr ? event->attr.sample_regs_intr :
> +			  event->attr.sample_regs_user;
> +	u64 vec_regs = intr ? event->attr.sample_simd_vec_reg_intr :
> +			      event->attr.sample_simd_vec_reg_user;
> +	u64 pred_regs = intr ? event->attr.sample_simd_pred_reg_intr :
> +			       event->attr.sample_simd_pred_reg_user;
> +	u64 xregs_mask = PEBS_GP_REGS | PERF_X86_EGPRS_MASK |
> +			 BIT_ULL(PERF_REG_X86_SSP);
> +
> +	if (regs & ~xregs_mask)
> +		return false;
> +
> +	if ((regs & (PEBS_GP_REGS | BIT_ULL(PERF_REG_X86_SSP))) &&
> +	    !intel_pebs_support_regs(event, PEBS_DATACFG_GP))
> +		return false;

[Severity: High]
Does this logic unintentionally allow large PEBS to be enabled on legacy PEBS
hardware when the SSP register is requested?

Looking at intel_pebs_support_regs(), it returns true for PEBS_DATACFG_GP
unconditionally on legacy PEBS hardware. If a user requests SSP on legacy
PEBS, __regs_support_large_pebs() will return true and allow large PEBS.

Since legacy PEBS hardware records do not contain the SSP, the perf core
fetches it from the thread's XSAVE buffer during the PMU interrupt via
x86_pmu_update_perf_regs(). 

Because large PEBS batches multiple samples and delays the interrupt, won't
every sample in the batch end up with the exact same stale SSP value from
the time of the interrupt?

Should large PEBS be explicitly disabled on legacy PEBS when SSP is
requested, perhaps similarly to how eGPRs are handled?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529075645.580362-1-dapeng1.mi@linux.intel.com?part=20