Re: [PATCH 4/5] perf: add MIPS Coherence Manager 3 (CM3) PMU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential
issues to consider:
- [High] perf/mips_cm3: 2^32 counts lost on counter overflow in
  cm3_pmu_update()
- [Medium] perf/mips_cm3: 64-bit qualifier truncated in hwc->config_base
  on 32-bit
- [Medium] perf/mips_cm3: cm3_pmu_event_init() validates group before
  setting config
- [Medium] perf/mips_cm3: NULL ctx deref in cm3_pmu_validate_group()
  with lockdep
- [Medium] perf/mips_cm3: cpuhp_state_remove_instance() migrates
  unregistered PMU
- [Medium] perf/mips_cm3: cm3_pmu_hw_init() skips power check when
  initialized
- [Medium] perf/mips_cm3: invalid wait context acquiring cm_core_lock in
  cm3_pmu_lock()
--

commit 7d10c5a1b5055212bfd02b13d5581ab06e3a9b14
Author: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>

perf: add MIPS Coherence Manager 3 (CM3) PMU driver

This commit adds an uncore PMU driver and documentation for the performance
counters in the MIPS Coherence Manager 3 (CM3).

> diff --git a/drivers/perf/mips_cm3_pmu.c b/drivers/perf/mips_cm3_pmu.c
> new file mode 100644
> index 0000000000000..d4be23e79d6f0
> --- /dev/null
> +++ b/drivers/perf/mips_cm3_pmu.c

[ ... ]

> +static bool cm3_pmu_lock(struct cm3_pmu *cm3_pmu)
> +{
> +	preempt_disable();
> +	if (cm3_pmu->cluster == cpu_cluster(&current_cpu_data))
> +		return false;
> +
> +	mips_cm_lock_other(cm3_pmu->cluster, 0, 0, CM_GCR_Cx_OTHER_BLOCK_DEBUG);
> +	return true;
> +}

[Severity: Medium]
Can calling mips_cm_lock_other() here trigger a lockdep wait-context
splat under CONFIG_PROVE_RAW_LOCK_NESTING?

mips_cm_lock_other() acquires the per-CPU cm_core_lock, which is a
non-raw spinlock_t:

arch/mips/kernel/mips-cm.c:mips_cm_lock_other() {
	...
	spin_lock_irqsave(this_cpu_ptr(&cm_core_lock),
			  *this_cpu_ptr(&cm_core_lock_flags));
	...
}

CM3_PMU_READ() and CM3_PMU_WRITE() call cm3_pmu_lock() from the PMU
callbacks (add, start, stop, del, read), which run while holding the
raw_spinlock_t perf_event_context::lock, as well as from the hardirq
hrtimer callback cm3_pmu_poll().  Acquiring a spinlock_t inside a
raw_spinlock_t or hardirq context is an invalid wait-context transition
when accessing a remote cluster's PMU.

[ ... ]

> +static int cm3_pmu_hw_init(struct cm3_pmu *cm3_pmu)
> +{
> +	guard(mutex)(&cm3_pmu->init_lock);
> +
> +	if (cm3_pmu->initialized)
> +		return 0;
> +
> +	if (!cm3_pmu_cluster_powered(cm3_pmu))
> +		return -ENODEV;
> +
> +	cm3_pmu->num_pcs = FIELD_GET(CM_GCR_DB_PC_CTL_PERF_NUM_CNT,
> +				     CM3_PMU_READ(cm3_pmu, pc_ctl));
> +	cm3_pmu->num_pcs = min_t(unsigned int, cm3_pmu->num_pcs,
> +				 CM3_PMU_MAX_PCS);
> +	cm3_pmu_hw_reset(cm3_pmu);
> +	cm3_pmu->initialized = true;
> +
> +	return 0;
> +}

[Severity: Medium]
Does returning early when cm3_pmu->initialized is true skip the power
check if a remote cluster is powered down after probe or an earlier
event?  As cm3_pmu_unregister() notes by checking both
cm3_pmu->initialized and cm3_pmu_cluster_powered(), a cluster that was
once initialized can later power down; opening a new event on it will
then pass cm3_pmu_hw_init() and fault when cm3_pmu_start() accesses its
GCRs.

[ ... ]

> +static void cm3_pmu_update(struct perf_event *event)
> +{
> +	struct cm3_pmu *cm3_pmu = to_cm3_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	u64 prev, new;
> +
> +	do {
> +		prev = local64_read(&hwc->prev_count);
> +		new = cm3_pmu_read_counter(cm3_pmu, hwc->idx);
> +	} while (local64_cmpxchg(&hwc->prev_count, prev, new) != prev);
> +
> +	local64_add((new - prev) & CM3_PMU_COUNTER_MASK, &event->count);
> +}
> +
> +static void cm3_pmu_start(struct perf_event *event, int flags)
> +{
> +	struct cm3_pmu *cm3_pmu = to_cm3_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int idx = hwc->idx;
> +
> +	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
> +		return;
> +
> +	hwc->state = 0;
> +
> +	if (idx != CM3_PMU_IDX_CYCLES)
> +		cm3_pmu_program_counter(cm3_pmu, idx, hwc->config,
> +					hwc->config_base);
> +
> +	/*
> +	 * Setting the reset bit together with the count-on bit resets the
> +	 * counter and its overflow status before counting is started.
> +	 */
> +	local64_set(&hwc->prev_count, 0);
> +	cm3_pmu_set_ctl(cm3_pmu, 0,
> +			cm3_pmu_reset_bit(idx) | cm3_pmu_counton_bit(idx));
> +
> +	perf_event_update_userpage(event);
> +}

[Severity: High]
When operating in overflow-interrupt mode (cm3_pmu->irq > 0, where the
polling timer is not started), does cm3_pmu_update() lose 2^32 counts on
counter wraparound?

cm3_pmu_start() resets both hwc->prev_count and the 32-bit hardware
counter to 0.  When the counter reaches 0xffffffff, raises the overflow
interrupt, and wraps past 0 to a small value new before
cm3_pmu_handle_irq() calls cm3_pmu_update(), (new - prev) & 0xffffffff
evaluates to just new (or on subsequent overflows, whenever new >= prev),
discarding the full 2^32 wraparound.

[ ... ]

> +static void cm3_pmu_count_group_event(struct perf_event *event,
> +				      struct perf_event *member,
> +				      unsigned int *cycles, unsigned int *pcs)
> +{
> +	if (member->pmu != event->pmu)
> +		return;
> +
> +	if (member->hw.config == CM3_PMU_EV_CYCLES)
> +		(*cycles)++;
> +	else
> +		(*pcs)++;
> +}
> +
> +static bool cm3_pmu_validate_group(struct perf_event *event)
> +{
> +	struct cm3_pmu *cm3_pmu = to_cm3_pmu(event->pmu);
> +	struct perf_event *leader = event->group_leader;
> +	struct perf_event *sibling;
> +	unsigned int cycles = 0, pcs = 0;
> +
> +	/* Hardware events of the group must all be ours */
> +	if (leader->pmu != event->pmu && !is_software_event(leader))
> +		return false;
> +
> +	cm3_pmu_count_group_event(event, event, &cycles, &pcs);
> +	if (leader != event)
> +		cm3_pmu_count_group_event(event, leader, &cycles, &pcs);
> +
> +	for_each_sibling_event(sibling, leader) {
> +		if (sibling->pmu != event->pmu && !is_software_event(sibling))
> +			return false;
> +
> +		cm3_pmu_count_group_event(event, sibling, &cycles, &pcs);
> +	}
> +
> +	return cycles <= 1 && pcs <= cm3_pmu->num_pcs;
> +}

[Severity: Medium]
Can for_each_sibling_event(sibling, leader) cause a NULL pointer
dereference when leader == event and CONFIG_PROVE_LOCKING is enabled?

When initializing a standalone event or group leader in
perf_try_init_event(), leader->ctx is still NULL, and
for_each_sibling_event() expands lockdep_assert_event_ctx(leader), which
dereferences leader->ctx->mutex.  Returning true before the loop when
leader == event avoids this.

> +
> +static int cm3_pmu_event_init(struct perf_event *event)
> +{

[ ... ]

> +	if (!cm3_pmu_validate_group(event))
> +		return -EINVAL;
> +
> +	hwc->idx = -1;
> +	hwc->config = config;

[Severity: Medium]
Is cm3_pmu_validate_group(event) called before hwc->config is set?

Because event->hw.config is still 0 during cm3_pmu_validate_group(),
cm3_pmu_count_group_event(event, event, &cycles, &pcs) always counts the
new event in pcs instead of cycles.  Adding a CM3_PMU_EV_CYCLES event to
a group that already has two programmable events fails with -EINVAL,
while a group with two CM3_PMU_EV_CYCLES events passes validation.

> +	hwc->config_base = event->attr.config1 ?: cm3_pmu_default_qual(config);
> +	event->cpu = cm3_pmu->on_cpu;
> +
> +	return 0;
> +}

[Severity: Medium]
Does storing the 64-bit qualifier in hwc->config_base truncate bits 63:32
on 32-bit kernels?

hwc->config_base is an unsigned long, whereas CM3 qualifiers are 64-bit
values (and events such as CM3_PMU_EV_CM_REQ, CM3_PMU_EV_IO_REQ, and
CM3_PMU_EV_MEM_REQ require bits in 63:32 to be set in order to match any
requests).  Storing the qualifier in hwc->config1 (which is u64 in
struct hw_perf_event) preserves all 64 bits on CONFIG_32BIT.

[ ... ]

> +static void cm3_pmu_unregister(struct cm3_pmu *cm3_pmu)
> +{
> +	perf_pmu_unregister(&cm3_pmu->pmu);
> +	cpuhp_state_remove_instance(cm3_pmu_cpuhp_state, &cm3_pmu->node);
> +	hrtimer_cancel(&cm3_pmu->hrtimer);
> +	if (cm3_pmu->initialized && cm3_pmu_cluster_powered(cm3_pmu)) {
> +		cm3_pmu->irq = 0;
> +		cm3_pmu_hw_reset(cm3_pmu);
> +	}
> +}
> +
> +static int cm3_pmu_register(struct device *dev, struct cm3_pmu *cm3_pmu)
> +{

[ ... ]

> +	ret = perf_pmu_register(&cm3_pmu->pmu, name, -1);
> +	if (ret) {
> +		cpuhp_state_remove_instance(cm3_pmu_cpuhp_state, &cm3_pmu->node);
> +		return dev_err_probe(dev, ret, "failed to register %s\n", name);
> +	}

[Severity: Medium]
Should cm3_pmu_unregister() and the error path in cm3_pmu_register() use
cpuhp_state_remove_instance_nocalls() instead of
cpuhp_state_remove_instance()?

cpuhp_state_remove_instance() invokes cm3_pmu_offline_cpu() for each
online CPU while all CPUs remain in cpu_online_mask.  When more than one
CPU is online, cpumask_any_but(cpu_online_mask, cpu) finds a target CPU
and calls perf_pmu_migrate_context() on a PMU that has already been
unregistered (or failed perf_pmu_register()).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-i6500-cm3-pmu-v1-0-bd119584891a@mobileye.com?part=4