Re: [PATCH] driver: perf: arm_pmuv3: Read PMMIR_EL1 unconditionally

[Anshuman Khandual]

Hi James,

On 10/9/23 14:29, James Clark wrote:
> 
> 
> On 09/10/2023 08:56, Anshuman Khandual wrote:
>> PMMIR_EL1 needs to be captured in 'armpmu->reg_pmmir', for all appropriate
>> PMU version implementations where the register is available and reading it
>> is valid . Hence checking for bus slot event presence is redundant and can
>> be dropped.
>>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
>> This applies on v6.6-rc5.
>>  
>>  drivers/perf/arm_pmuv3.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
>> index 1e72b486c033..9fc1b6da5106 100644
>> --- a/drivers/perf/arm_pmuv3.c
>> +++ b/drivers/perf/arm_pmuv3.c
>> @@ -1129,7 +1129,7 @@ static void __armv8pmu_probe_pmu(void *info)
>>  			     pmceid, ARMV8_PMUV3_MAX_COMMON_EVENTS);
>>  
>>  	/* store PMMIR register for sysfs */
>> -	if (is_pmuv3p4(pmuver) && (pmceid_raw[1] & BIT(31)))
>> +	if (is_pmuv3p4(pmuver))
>>  		cpu_pmu->reg_pmmir = read_pmmir();
>>  	else
>>  		cpu_pmu->reg_pmmir = 0;
> 
> 
> This does have the side effect of showing non-zero values in caps/slots
> even when the STALL_SLOT event isn't implemented. I think that's the
> scenario that the original commit (f5be3a61fd) was trying to avoid:

But the the sysfs interface is supposed to show all the PMMIR_EL1 based
HW attributes as captured irrespective of bus slots event's presence as
the register could be read on ARMv8.4-PMU without additional conditions
imposed upon from the architecture.

> 
>   /sys/bus/event_source/devices/armv8_pmuv3_0/caps/slots is exposed
>   under sysfs. [If] Both ARMv8.4-PMU and STALL_SLOT event are
>   implemented, it returns the slots from PMMIR_EL1, otherwise it will
>   return 0.

But that additional requirement of STALL_SLOT event is just SW mandated
without any architectural backing.

> 
> I can't really think of a scenario where that would be an issue, and the
> availability of the STALL_SLOT event is already discoverable from
> userspace through the events folder, so it's probably fine.

Absolutely.

> 
> Adding the original author just in case. But otherwise:
> 
> Reviewed-by: James Clark <james.clark@xxxxxxx>

Thanks !