Re: [PATCH 1/3] perf/x86/amd/uncore: Add common PMU helper functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unconditional and improperly ordered context hijacking in `uncore_common_ctx_move` permanently breaks the PMU.
--

commit 43a9758bb17897ce6b1730e5454983946f9d832d
Author: Qi Liu <liuqi@hygon.cn>

perf/x86/amd/uncore: Add common PMU helper functions

This patch introduces common helper functions for AMD-family uncore PMU
handling. It provides infrastructure for event initialization, counter
allocation, reading, and context management for reuse by similar drivers.

> diff --git a/arch/x86/events/amd/uncore_common.c b/arch/x86/events/amd/uncore_common.c
> new file mode 100644
> index 0000000000000..81affaab319a2
> --- /dev/null
> +++ b/arch/x86/events/amd/uncore_common.c
[ ... ]
> +static enum hrtimer_restart uncore_common_hrtimer(struct hrtimer *hrtimer)
> +{
> +	struct uncore_common_ctx *ctx;
> +	struct perf_event *event;
> +	int bit;
> +
> +	ctx = container_of(hrtimer, struct uncore_common_ctx, hrtimer);
> +
> +	if (!ctx->nr_active || ctx->cpu != smp_processor_id())
> +		return HRTIMER_NORESTART;

[Severity: High]
This is a pre-existing issue, but could this condition trigger incorrectly and
permanently stop the hrtimer?

If ctx->cpu is left pointing to an old CPU or assigned to a different sibling,
this will evaluate to true and prevent the timer from restarting.

[ ... ]
> +void uncore_common_ctx_move(struct uncore_common *uncore, unsigned int cpu)
> +{
> +	struct uncore_common_ctx *curr, *next;
> +	struct uncore_common_pmu *pmu;
> +	int i, j;
> +
> +	if (!uncore->init_done)
> +		return;
> +
> +	for (i = 0; i < uncore->num_pmus; i++) {
> +		pmu = &uncore->pmus[i];
> +		if (!pmu->ctx)
> +			continue;
> +
> +		curr = *per_cpu_ptr(pmu->ctx, cpu);
> +		if (!curr)
> +			continue;
> +
> +		for_each_online_cpu(j) {
> +			if (cpu == j)
> +				continue;
> +
> +			next = *per_cpu_ptr(pmu->ctx, j);
> +			if (!next)
> +				continue;
> +
> +			if (curr == next) {
> +				perf_pmu_migrate_context(&pmu->pmu, cpu, j);
> +				cpumask_clear_cpu(cpu, &pmu->active_mask);
> +				cpumask_set_cpu(j, &pmu->active_mask);
> +				next->cpu = j;
> +				break;
> +			}
> +		}
> +	}
> +}

[Severity: High]
This isn't a bug introduced by this patch, but does the way this context is
moved cause the hrtimer to permanently break in two scenarios?

First, if a non-active sibling CPU goes offline, this code unconditionally
assigns next->cpu = j without verifying if (cpu == curr->cpu). If the first
online sibling it finds is not the currently active CPU (where events are
running), the active CPU gets hijacked. When the hrtimer fires on the active
CPU, ctx->cpu != smp_processor_id() will be true, permanently stopping the
timer.

Second, if the active CPU is offlining, the next->cpu = j assignment happens
after perf_pmu_migrate_context(). If the thread is preempted after migration
but before the assignment, could the newly armed timer on CPU j fire before
next->cpu is updated? This would also permanently stop the timer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701034413.718872-1-liuqi@hygon.cn?part=1