Re: [PATCH 05/21] perf: arm_pmuv3: Check cntr_mask before using pmccntr

Hi Robin, thanks for the review

Robin Murphy <[email protected]> writes:

> On 12/06/2026 8:28 pm, Colton Lewis wrote:
>> Check cntr_mask before using pmccntr to ensure it's available. With a
>> partitioned PMU, there may be instances where pmccntr is being used by
>> the guest and will be absent from cntr_mask.

>> Signed-off-by: Colton Lewis <[email protected]>
>> ---
>>    drivers/perf/arm_pmuv3.c | 3 ++-
>>    1 file changed, 2 insertions(+), 1 deletion(-)

>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
>> index 1cceb1f614515..17bb1cfdc271c 100644
>> --- a/drivers/perf/arm_pmuv3.c
>> +++ b/drivers/perf/arm_pmuv3.c
>> @@ -1028,7 +1028,8 @@ static int armv8pmu_get_event_idx(struct  
>> pmu_hw_events *cpuc,

>>    	/* Always prefer to place a cycle counter into the cycle counter. */
>>    	if (armv8pmu_can_use_pmccntr(cpuc, event)) {

> Something smells wrong here - we can use PMCCNTR but we can't?

>> -		if (!test_and_set_bit(ARMV8_PMU_CYCLE_IDX, cpuc->used_mask))
>> +		if (test_bit(ARMV8_PMU_CYCLE_IDX, cpu_pmu->cntr_mask) &&

> ...i.e. this test would seem to belong in the helper function.
>> +		    !test_and_set_bit(ARMV8_PMU_CYCLE_IDX, cpuc->used_mask))
>>    			return ARMV8_PMU_CYCLE_IDX;
>>    		else if (armv8pmu_event_is_64bit(event) &&
>>    			   armv8pmu_event_want_user_access(event) &&

> Plus it also doesn't make much sense to fall through to the "we _could_
> accommodate this event if only PMCCNTR was free" case here if we know
> PMCCNTR can never be free because it's not even available.

I agree it would be a good idea to refactor these if statements.

Probably something like:

/* tests moved to `armv8pmu_can_use_pmccntr` */
if (armv8pmu_can_use_pmccntr()) {
	set_bit(ARMV8_PMU_CYCLE_IDX, cpu_pmu->used_mask);
         return ARMV8_PMU_CYCLE_IDX;
}

if (armv8pmu_event_is_64bit(event) &&
     armv8pmu_event_want_user_access(event) &&
     !armv8pmu_has_long_event(cpu_pmu))
	return -EAGAIN;