Re: [PATCH] drivers: perf: arm_pmuv3: Update 'pmc_width' based on actual HW event width

[Anshuman Khandual]

Hi James,

On 10/9/23 15:13, James Clark wrote:
> 
> 
> On 09/10/2023 05:37, Anshuman Khandual wrote:
>> This updates 'perf_event_mmap_page->pmc_width' based on actual HW event's
>> width that are currently missing i.e ARMPMU_EVT_63BIT and ARMPMU_EVT_47BIT.
>>
> 
> Might be worth adding why this is needed or what the actual effect is.

To have correct 'pmc_width' visible to the user space ?

> 
>> 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 | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
>> index fe4db1831662..94723d00548e 100644
>> --- a/drivers/perf/arm_pmuv3.c
>> +++ b/drivers/perf/arm_pmuv3.c
>> @@ -1375,6 +1375,10 @@ void arch_perf_update_userpage(struct perf_event *event,
>>  	if (userpg->cap_user_rdpmc) {
>>  		if (event->hw.flags & ARMPMU_EVT_64BIT)
>>  			userpg->pmc_width = 64;
>> +		else if (event->hw.flags & ARMPMU_EVT_63BIT)
>> +			userpg->pmc_width = 63;
>> +		else if (event->hw.flags & ARMPMU_EVT_47BIT)
>> +			userpg->pmc_width = 47;
> 
> Although it doesn't explicitly say it, the bit of the docs about
> pmc_width in Documentation/arch/arm64/perf.rst loosely implies that this
> is always either 64 or 32. Now that this isn't the case it could mislead
> someone in userspace that they don't have to handle the now arbitrary
> bit widths rather than just whole bytes/ints.

Are you suggesting that the user space would not handle pmc_width correctly
, once it deviates from a whole bytes/ints format ? In that case user space
handling might need some fixing.

> 
> I think the fix is as simple as adding something like "the width may not
> match the requested value or necessarily be a multiple of 8". Unless we
> think this is already widely known and I suppose we could leave it as
> is. (The existing bit in perf that uses it already handles it correctly).

This is from perf_event_mmap_page definition where it does not assert the
width to be multiple of bytes or ints. Hence the assumption should not be
made into the user space tools.

        /*
         * If cap_user_rdpmc this field provides the bit-width of the value
         * read using the rdpmc() or equivalent instruction. This can be used
         * to sign extend the result like:
         *
         *   pmc <<= 64 - width;
         *   pmc >>= 64 - width; // signed shift right
         *   count += pmc;
         */
        __u16   pmc_width;

Moreover, on x86 too 'userpg->pmc_width' gets assigned to different values
although multiple of 8.

userpg->pmc_width = x86_pmu.cntval_bits
arch/x86/events/amd/core.c:     .cntval_bits            = 48
arch/x86/events/intel/knc.c:    .cntval_bits            = 40
arch/x86/events/intel/p6.c:     .cntval_bits            = 32

> 
>>  		else
>>  			userpg->pmc_width = 32;
>>  	}