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

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

[Anshuman Khandual]

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.

Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
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;
 		else
 			userpg->pmc_width = 32;
 	}
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[James Clark]

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.

> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> 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.

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).

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> 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;
>>  	}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[James Clark]

On 10/10/2023 04:03, Anshuman Khandual wrote:
> 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 ?

Well yeah, but for example I didn't know what that was. And it's not
clear why it needs updating at this point in time without a link to any
other commit or relevant section from the Arm ARM. So I had a kind of a
"why now" question.

"To have correct 'pmc_width' visible to the user space" is definitely
more of a what than a why.

> 
>>
>>> Cc: Will Deacon <will@kernel.org>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>> 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.
> 

Not really, I'm just suggesting that anyone writing a new tool and only
reading the docs could make that assumption. Seeing as only 32 and 64
bit options are mentioned. So it's more to avoid misleading someone in
the future than about fixing any existing code, as updating the docs
wouldn't have that effect.

>>
>> 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.
> 

Yeah I know its already ok for Perf which is why I mentioned it. But
there are more tools out there than Perf, and ones that don't even exist
yet, which people would normally read the documentation before writing.

>         /*
>          * 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;
>>>  	}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[James Clark]

On 10/10/2023 09:28, James Clark wrote:
> 
> 
> On 10/10/2023 04:03, Anshuman Khandual wrote:
>> 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 ?
> 
> Well yeah, but for example I didn't know what that was. And it's not
> clear why it needs updating at this point in time without a link to any
> other commit or relevant section from the Arm ARM. So I had a kind of a
> "why now" question.
> 
> "To have correct 'pmc_width' visible to the user space" is definitely
> more of a what than a why.
> 

If anything this should at least have a fixes: tag on it. If you're
saying that it's now correct.

>>
>>>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>> ---
>>>> 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.
>>
> 
> Not really, I'm just suggesting that anyone writing a new tool and only
> reading the docs could make that assumption. Seeing as only 32 and 64
> bit options are mentioned. So it's more to avoid misleading someone in
> the future than about fixing any existing code, as updating the docs
> wouldn't have that effect.
> 
>>>
>>> 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.
>>
> 
> Yeah I know its already ok for Perf which is why I mentioned it. But
> there are more tools out there than Perf, and ones that don't even exist
> yet, which people would normally read the documentation before writing.
> 
>>         /*
>>          * 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;
>>>>  	}
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Robin Murphy]

On 09/10/2023 5:37 am, 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.
> 
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> 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;

I think this will give the wrong behaviour in at least some cases - if 
the user has not requested a "long" event which would lead to 
ARMPMU_EVT_64BIT being set, we will bias the values to only count 32 
effective bits regardless of how wide the physical counters are. I 
believe this is what "the valid width of the counter" in the 
documentation is trying to refer to.

Thanks,
Robin.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Rutland]

On Mon, Oct 09, 2023 at 10:07:24AM +0530, 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.
> 
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> 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;

The PMUv3 driver *never* uses 63-bit or 47-bit counters, so the PMUv3 driver
doesn't need this. The ARMPMU_EVT_63BIT and ARMPMU_EVT_47BIT flags are only
used by the Apple PMU driver, which doesn't support user access, and similarly
doesn't need to handle this.

This code is not necessary as the two new branches can never run. It will also
confuse people into thinking the PMUv3 driver can use these widths when it
cannot.

NAK to this patch for the reasons above.

Mark

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel