Re: [PATCH v9 18/20] RISC-V: perf: Add Qemu virt machine events

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `counterid_mask` for `cycles` and `instructions` events in the QEMU PMU vendor events table incorrectly excludes fixed counters, unnecessarily consuming limited programmable counters.
--

--- Patch [18]: [PATCH v9 18/20] RISC-V: perf: Add Qemu virt machine events ---
commit c3b03b30f6d05637f0ec5ea8c549bff42e2bbc49
Author: Atish Patra <[email protected]>

RISC-V: perf: Add Qemu virt machine events

Qemu virt machine supports a very minimal set of legacy perf events.
Add them to the vendor table so that users can use them when
counter delegation is enabled.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 5eb49db8bc7c1..9220c75651a5c 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c

[ ... ]

> @@ -468,7 +469,45 @@ struct riscv_vendor_pmu_events {
>  	  .hw_event_map = _hw_event_map, .cache_event_map = _cache_event_map, \
>  	  .attrs_events = _attrs },
>  
> +/* QEMU virt PMU events */
> +static const struct riscv_pmu_event qemu_virt_hw_event_map[PERF_COUNT_HW_MAX] = {
> +	PERF_MAP_ALL_UNSUPPORTED,
> +	[PERF_COUNT_HW_CPU_CYCLES]		= {0x01, 0xFFFFFFF8},
> +	[PERF_COUNT_HW_INSTRUCTIONS]		= {0x02, 0xFFFFFFF8}
> +};

[Severity: Medium]
Does the counterid_mask of 0xFFFFFFF8 inadvertently exclude the fixed hardware
counters for these events?

By clearing bits 0, 1, and 2, this mask prevents the driver from using the
dedicated fixed counters (counter 0 for cycles, counter 2 for instructions).

When checking if a fixed counter is permitted, get_deleg_fixed_hw_idx() 
verifies event->attr.config2 against RISCV_PMU_CYCLE_FIXED_CTR_MASK (0x01)
or RISCV_PMU_INSTRUCTION_FIXED_CTR_MASK (0x04). Because 0xFFFFFFF8 & 0x01 == 0
and 0xFFFFFFF8 & 0x04 == 0, the driver will bypass the fixed counters and 
unnecessarily consume scarce programmable HPM counters instead.

Would it be better to include the respective fixed counter bits in the mask
(e.g., 0xFFFFFFF9 for cycles and 0xFFFFFFFD for instructions), or perhaps
use 0xFFFFFFFF?

> +
> +static const struct riscv_pmu_event qemu_virt_cache_event_map[PERF_COUNT_HW_CACHE_MAX]
> +						[PERF_COUNT_HW_CACHE_OP_MAX]
> +						[PERF_COUNT_HW_CACHE_RESULT_MAX] = {

[ ... ]

> +	[C(ITLB)][C(OP_READ)][C(RESULT_MISS)]	= {0x10021, 0xFFFFFFF8},
> +};
> +
> +RVPMU_EVENT_CMASK_ATTR(cycles, cycles, 0x01, 0xFFFFFFF8);
> +RVPMU_EVENT_CMASK_ATTR(instructions, instructions, 0x02, 0xFFFFFFF8);

[Severity: Medium]
Since this macro exposes the counterid_mask to userspace via sysfs aliases, 
does providing 0xFFFFFFF8 here cause the perf tool to populate config2 with 
0xFFFFFFF8?

If so, this will enforce the same fixed-counter bypass on the counter 
delegation path. Updating these masks to include the fixed counter bits
would ensure they can be fully utilized.

> +RVPMU_EVENT_CMASK_ATTR(dTLB-load-misses, dTLB_load_miss, 0x10019, 0xFFFFFFF8);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=18