Re: [PATCH v1 37/41] perf pmu: Probe user-space mmap capabilities

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf pmu: probe_mmap_caps() skips perf_user_access sysctl on RISC-V
- [Medium] perf pmu: non-advancing counter check in probe_mmap_caps()
--

commit 4c52f2a70c884425ea972ad4877898e6764c69b2
Author: Ian Rogers <irogers@google.com>

perf pmu: Probe user-space mmap capabilities

This commit extends perf_pmu__probe() with probe_mmap_caps() to inspect the
perf_event_mmap_page of an opened PMU event, recording user-space counter
reading support (rdpmc/mrs/csrr), counter bit-width, and user time conversion
capabilities.

> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index bc3c821153b6a..e26563342ad19 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -3820,6 +3820,106 @@ static bool probe_format_pack(const struct perf_pmu *pmu, struct perf_event_attr
>  	return true;
>  }
>  
> +/*
> + * Probe user-space perf_event_mmap_page capabilities, including user-space
> + * counter reading (rdpmc/mrs/csrr), counter bit-width, and user time fields.
> + */
> +static void probe_mmap_caps(struct perf_pmu *pmu,
> +			    const struct perf_event_attr *base_attr,
> +			    pid_t pid, int cpu, unsigned long flags)
> +{

[ ... ]

> +	/*
> +	 * On architectures such as arm64, user-space counter reading must be
> +	 * explicitly requested via format/rdpmc (e.g. config1:1).
> +	 */
> +	if (perf_pmu__has_format(pmu, "rdpmc")) {
> +		struct perf_event_attr r_attr = attr;
> +		int uval;
> +
> +		has_rdpmc_cap = true;
> +		if (pmu->probed.rdpmc_sysfs < 0 &&
> +		    sysctl__read_int("kernel/perf_user_access", &uval) == 0)
> +			pmu->probed.rdpmc_sysfs = uval;
> +
> +		if (probe_format_pack(pmu, &r_attr, "rdpmc", 1))
> +			fd = sys_perf_event_open(&r_attr, 0, -1, -1, flags);
> +	}

[Severity: Medium]
In probe_mmap_caps(), reading kernel/perf_user_access and setting
has_rdpmc_cap are gated on perf_pmu__has_format(pmu, "rdpmc").

While arm64 PMUs expose format/rdpmc, the RISC-V SBI PMU driver in
drivers/perf/riscv_pmu_sbi.c registers the kernel/perf_user_access sysctl in
sbi_pmu_sysctl_table without exposing a per-PMU sysfs rdpmc file or a
format/rdpmc attribute in riscv_arch_formats_attr:

drivers/perf/riscv_pmu_sbi.c:
    static struct attribute *riscv_arch_formats_attr[] = {
        &format_attr_event.attr,
        &format_attr_firmware.attr,
        NULL,
    };

When perf_pmu__probe() -> __perf_pmu__probe() -> probe_mmap_caps() runs on a
RISC-V core PMU, won't this skip reading kernel/perf_user_access, leaving
pmu->probed.rdpmc_sysfs at -1 and leaving pmu->probed.rdpmc_available false
whenever kernel/perf_user_access is 0?

> +
> +	if (fd < 0 && !pmu->is_uncore)
> +		fd = sys_perf_event_open(&attr, 0, -1, -1, flags);
> +	if (fd < 0)
> +		fd = sys_perf_event_open(&attr, pid, cpu, -1, flags);
> +
> +	if (fd >= 0) {
> +		pc = mmap(NULL, page_sz, PROT_READ, MAP_SHARED, fd, 0);
> +		if (pc != MAP_FAILED) {
> +			if (has_rdpmc_cap || pc->cap_user_rdpmc) {
> +				pmu->probed.rdpmc_available = true;
> +				pmu->probed.pmc_width = pc->pmc_width;
> +			}
> +			if (pc->cap_user_rdpmc && pc->index != 0) {
> +				struct perf_mmap map = { .base = pc };
> +				struct perf_counts_values start = { .val = 0 };
> +				struct perf_counts_values end = { .val = 0 };
> +
> +				pmu->probed.can_user_rdpmc = true;
> +				if (perf_mmap__read_self(&map, &start) == 0) {
> +					for (int spin = 0; spin < 10000; spin++)
> +						barrier();
> +					if (perf_mmap__read_self(&map, &end) == 0 &&
> +					    end.val >= start.val && end.val > 0)
> +						pmu->probed.rdpmc_works = true;
> +				}
> +			}

[Severity: Medium]
Should the check in probe_mmap_caps() be end.val > start.val instead of
end.val >= start.val && end.val > 0?

When an active hardware event is opened and its perf_event_mmap_page is
mapped, the kernel's period initialization (such as x86_perf_event_set_period,
armpmu_event_set_period, and riscv_pmu_event_set_period) sets
event->hw.prev_count to (u64)-left, and perf_event_update_userpage() sets
userpg->offset to +left > 0.

In user space, perf_mmap__read_self() computes:

    cnt = pc->offset + evcnt;

Because pc->offset is already positive, if read_perf_counter() returns 0 (for
instance via the fallback stub in tools/lib/perf/mmap.c) or if the hardware
counter does not advance across the 10,000-iteration spin loop, start.val and
end.val will both equal the same positive constant, causing end.val >=
start.val && end.val > 0 to evaluate to true and set pmu->probed.rdpmc_works
even when the counter did not advance.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103234502.1008382-1-irogers@google.com?part=37