diff mbox series

[3/3] perf/arm-cmn: Enable support for tertiary match group

Message ID 20240126221215.1537377-4-ilkka@os.amperecomputing.com
State New
Headers
Series perf/arm-cmn: Add support for tertiary match group |

Commit Message

Ilkka Koskinen Jan. 26, 2024, 10:12 p.m. UTC 
  Add support for tertiary match group.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 drivers/perf/arm-cmn.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

Comments

Robin Murphy Jan. 29, 2024, 5:07 p.m. UTC | #1 
On 2024-01-26 10:12 pm, Ilkka Koskinen wrote:
> Add support for tertiary match group.
 >
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> ---
>   drivers/perf/arm-cmn.c | 23 +++++++++++++++++++----
>   1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index dc6370396ad0..ce9fbdcf6144 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -91,10 +91,13 @@
>   #define CMN600_WPn_CONFIG_WP_COMBINE	BIT(6)
>   #define CMN600_WPn_CONFIG_WP_EXCLUSIVE	BIT(5)
>   #define CMN_DTM_WPn_CONFIG_WP_GRP	GENMASK_ULL(5, 4)
> +#define CMN600_WPn_CONFIG_WP_GRP	BIT(4)
>   #define CMN_DTM_WPn_CONFIG_WP_CHN_SEL	GENMASK_ULL(3, 1)
>   #define CMN_DTM_WPn_CONFIG_WP_DEV_SEL	BIT(0)
>   #define CMN_DTM_WPn_VAL(n)		(CMN_DTM_WPn(n) + 0x08)
>   #define CMN_DTM_WPn_MASK(n)		(CMN_DTM_WPn(n) + 0x10)
> +#define CMN_DTM_WP_CHN_SEL_REQ_VC	0
> +#define CMN_DTM_WP_GRP_TERTIARY		0x2
>   
>   #define CMN_DTM_PMU_CONFIG		0x210
>   #define CMN__PMEVCNT0_INPUT_SEL		GENMASK_ULL(37, 32)
> @@ -175,8 +178,8 @@
>   #define CMN_CONFIG_WP_DEV_SEL		GENMASK_ULL(50, 48)
>   #define CMN_CONFIG_WP_CHN_SEL		GENMASK_ULL(55, 51)
>   /* Note that we don't yet support the tertiary match group on newer IPs */
> -#define CMN_CONFIG_WP_GRP		BIT_ULL(56)
> -#define CMN_CONFIG_WP_EXCLUSIVE		BIT_ULL(57)
> +#define CMN_CONFIG_WP_GRP		GENMASK_ULL(57, 56)
> +#define CMN_CONFIG_WP_EXCLUSIVE		BIT_ULL(58)
>   #define CMN_CONFIG1_WP_VAL		GENMASK_ULL(63, 0)
>   #define CMN_CONFIG2_WP_MASK		GENMASK_ULL(63, 0)
>   
> @@ -1298,7 +1301,9 @@ static struct attribute *arm_cmn_format_attrs[] = {
>   
>   	CMN_FORMAT_ATTR(CMN_ANY, wp_dev_sel, CMN_CONFIG_WP_DEV_SEL),
>   	CMN_FORMAT_ATTR(CMN_ANY, wp_chn_sel, CMN_CONFIG_WP_CHN_SEL),
> -	CMN_FORMAT_ATTR(CMN_ANY, wp_grp, CMN_CONFIG_WP_GRP),
> +	CMN_FORMAT_ATTR(CMN600, wp_grp, CMN600_WPn_CONFIG_WP_GRP),

Perhaps an easy confusion, but 4 != 56: CMN_CONFIG_WP_* represent 
perf_event->config{,1,2} attribute fields per the CMN_CONFIG_* pattern, 
whereas CMN*_WPn_CONFIG_* are hardware register fields where "config" is 
just annoygingly part of the register name.

> +	CMN_FORMAT_ATTR(NOT_CMN600, wp_grp, CMN_CONFIG_WP_GRP),

Hmm, I'm sure last time I tried something like this, sysfs wouldn't let 
two attributes with the same name exist, regardless of whether one was 
meant to be hidden :/

TBH I think that either we change ABI for everyone consistently, or we 
extend the field in a backwards-compatible way. If you think an ABI 
break would affect existing CMN-600 users, then surely at stands to 
affect existing CMN-650 and CMN-700 users just as much?

> +
>   	CMN_FORMAT_ATTR(CMN_ANY, wp_exclusive, CMN_CONFIG_WP_EXCLUSIVE),
>   	CMN_FORMAT_ATTR(CMN_ANY, wp_combine, CMN_CONFIG_WP_COMBINE),
>   
> @@ -1398,8 +1403,11 @@ static u32 arm_cmn_wp_config(struct perf_event *event)
>   
>   	config = FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_DEV_SEL, dev) |
>   		 FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_CHN_SEL, chn) |
> -		 FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_GRP, grp) |
>   		 FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_DEV_SEL2, dev >> 1);
> +
> +	if (grp)
> +		config |= is_cmn600 ? CMN600_WPn_CONFIG_WP_GRP :
> +				      FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_GRP, grp);

FWIW I think something more like "if (is_cmn600) grp &= 1;" before the 
existing assignent might be clearer. Note that that *is* effectively how 
this works already since CMN_DTM_WPn_CONFIG_WP_GRP was updated, it's 
just currently implicit in CMN_EVENT_WP_GRP().

>   	if (exc)
>   		config |= is_cmn600 ? CMN600_WPn_CONFIG_WP_EXCLUSIVE :
>   				      CMN_DTM_WPn_CONFIG_WP_EXCLUSIVE;

You've missed the "(combine && !grp)" logic below this point, which also 
needs to get rather more involved if a combined match across groups 1 
and 2 is going to work correctly.

> @@ -1764,6 +1772,13 @@ static int arm_cmn_event_init(struct perf_event *event)
>   		/* ...and we need a "real" direction */
>   		if (eventid != CMN_WP_UP && eventid != CMN_WP_DOWN)
>   			return -EINVAL;
> +
> +		if (cmn->part != PART_CMN600)
> +			if (CMN_EVENT_WP_GRP(event) > CMN_DTM_WP_GRP_TERTIARY ||
> +			    (CMN_EVENT_WP_GRP(event) == CMN_DTM_WP_GRP_TERTIARY &&
> +			     CMN_EVENT_WP_CHN_SEL(event) != CMN_DTM_WP_CHN_SEL_REQ_VC))
> +				return -EINVAL;
> +

We already don't attempt to sanity-check watchpoint arguments (e.g. 
chn>3 or chn=1,grp=1), so I'm not really inclined to start. The aim here 
has always been not to try to understand watchpoints at all, and 
effectively just pass through the register interface to the user.

Thanks,
Robin.

>   		/* ...but the DTM may depend on which port we're watching */
>   		if (cmn->multi_dtm)
>   			hw->dtm_offset = CMN_EVENT_WP_DEV_SEL(event) / 2;
Ilkka Koskinen Jan. 30, 2024, 5:35 a.m. UTC | #2 
On Mon, 29 Jan 2024, Robin Murphy wrote:

> On 2024-01-26 10:12 pm, Ilkka Koskinen wrote:
>> Add support for tertiary match group.
>>
>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> ---
>>   drivers/perf/arm-cmn.c | 23 +++++++++++++++++++----
>>   1 file changed, 19 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
>> index dc6370396ad0..ce9fbdcf6144 100644
>> --- a/drivers/perf/arm-cmn.c
>> +++ b/drivers/perf/arm-cmn.c
>> @@ -91,10 +91,13 @@
>>   #define CMN600_WPn_CONFIG_WP_COMBINE	BIT(6)
>>   #define CMN600_WPn_CONFIG_WP_EXCLUSIVE	BIT(5)
>>   #define CMN_DTM_WPn_CONFIG_WP_GRP	GENMASK_ULL(5, 4)
>> +#define CMN600_WPn_CONFIG_WP_GRP	BIT(4)
>>   #define CMN_DTM_WPn_CONFIG_WP_CHN_SEL	GENMASK_ULL(3, 1)
>>   #define CMN_DTM_WPn_CONFIG_WP_DEV_SEL	BIT(0)
>>   #define CMN_DTM_WPn_VAL(n)		(CMN_DTM_WPn(n) + 0x08)
>>   #define CMN_DTM_WPn_MASK(n)		(CMN_DTM_WPn(n) + 0x10)
>> +#define CMN_DTM_WP_CHN_SEL_REQ_VC	0
>> +#define CMN_DTM_WP_GRP_TERTIARY		0x2
>>     #define CMN_DTM_PMU_CONFIG		0x210
>>   #define CMN__PMEVCNT0_INPUT_SEL		GENMASK_ULL(37, 32)
>> @@ -175,8 +178,8 @@
>>   #define CMN_CONFIG_WP_DEV_SEL		GENMASK_ULL(50, 48)
>>   #define CMN_CONFIG_WP_CHN_SEL		GENMASK_ULL(55, 51)
>>   /* Note that we don't yet support the tertiary match group on newer IPs 
>> */
>> -#define CMN_CONFIG_WP_GRP		BIT_ULL(56)
>> -#define CMN_CONFIG_WP_EXCLUSIVE		BIT_ULL(57)
>> +#define CMN_CONFIG_WP_GRP		GENMASK_ULL(57, 56)
>> +#define CMN_CONFIG_WP_EXCLUSIVE		BIT_ULL(58)
>>   #define CMN_CONFIG1_WP_VAL		GENMASK_ULL(63, 0)
>>   #define CMN_CONFIG2_WP_MASK		GENMASK_ULL(63, 0)
>>   @@ -1298,7 +1301,9 @@ static struct attribute *arm_cmn_format_attrs[] = {
>>     	CMN_FORMAT_ATTR(CMN_ANY, wp_dev_sel, CMN_CONFIG_WP_DEV_SEL),
>>   	CMN_FORMAT_ATTR(CMN_ANY, wp_chn_sel, CMN_CONFIG_WP_CHN_SEL),
>> -	CMN_FORMAT_ATTR(CMN_ANY, wp_grp, CMN_CONFIG_WP_GRP),
>> +	CMN_FORMAT_ATTR(CMN600, wp_grp, CMN600_WPn_CONFIG_WP_GRP),
>
> Perhaps an easy confusion, but 4 != 56: CMN_CONFIG_WP_* represent 
> perf_event->config{,1,2} attribute fields per the CMN_CONFIG_* pattern, 
> whereas CMN*_WPn_CONFIG_* are hardware register fields where "config" is just 
> annoygingly part of the register name.

Ah, true.

>
>> +	CMN_FORMAT_ATTR(NOT_CMN600, wp_grp, CMN_CONFIG_WP_GRP),
>
> Hmm, I'm sure last time I tried something like this, sysfs wouldn't let two 
> attributes with the same name exist, regardless of whether one was meant to 
> be hidden :/
>
> TBH I think that either we change ABI for everyone consistently, or we extend 
> the field in a backwards-compatible way. If you think an ABI break would 
> affect existing CMN-600 users, then surely at stands to affect existing 
> CMN-650 and CMN-700 users just as much?

Well, I doubt it would really affect. Sounds like extending would be just 
fine.

>> +
>>   	CMN_FORMAT_ATTR(CMN_ANY, wp_exclusive, CMN_CONFIG_WP_EXCLUSIVE),
>>   	CMN_FORMAT_ATTR(CMN_ANY, wp_combine, CMN_CONFIG_WP_COMBINE),
>>   @@ -1398,8 +1403,11 @@ static u32 arm_cmn_wp_config(struct perf_event 
>> *event)
>>     	config = FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_DEV_SEL, dev) |
>>   		 FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_CHN_SEL, chn) |
>> -		 FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_GRP, grp) |
>>   		 FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_DEV_SEL2, dev >> 1);
>> +
>> +	if (grp)
>> +		config |= is_cmn600 ? CMN600_WPn_CONFIG_WP_GRP :
>> +				      FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_GRP, 
>> grp);
>
> FWIW I think something more like "if (is_cmn600) grp &= 1;" before the 
> existing assignent might be clearer. Note that that *is* effectively how this 
> works already since CMN_DTM_WPn_CONFIG_WP_GRP was updated, it's just 
> currently implicit in CMN_EVENT_WP_GRP().

Seems reasonable

>
>>   	if (exc)
>>   		config |= is_cmn600 ? CMN600_WPn_CONFIG_WP_EXCLUSIVE :
>>   				      CMN_DTM_WPn_CONFIG_WP_EXCLUSIVE;
>
> You've missed the "(combine && !grp)" logic below this point, which also 
> needs to get rather more involved if a combined match across groups 1 and 2 
> is going to work correctly.

Ah, that's right

>
>> @@ -1764,6 +1772,13 @@ static int arm_cmn_event_init(struct perf_event 
>> *event)
>>   		/* ...and we need a "real" direction */
>>   		if (eventid != CMN_WP_UP && eventid != CMN_WP_DOWN)
>>   			return -EINVAL;
>> +
>> +		if (cmn->part != PART_CMN600)
>> +			if (CMN_EVENT_WP_GRP(event) > CMN_DTM_WP_GRP_TERTIARY 
>> ||
>> +			    (CMN_EVENT_WP_GRP(event) == 
>> CMN_DTM_WP_GRP_TERTIARY &&
>> +			     CMN_EVENT_WP_CHN_SEL(event) != 
>> CMN_DTM_WP_CHN_SEL_REQ_VC))
>> +				return -EINVAL;
>> +
>
> We already don't attempt to sanity-check watchpoint arguments (e.g. chn>3 or 
> chn=1,grp=1), so I'm not really inclined to start. The aim here has always 
> been not to try to understand watchpoints at all, and effectively just pass 
> through the register interface to the user.

Yep, I noticed that. I'm fine with either way

Cheers, Ilkka

>
> Thanks,
> Robin.
>
>>   		/* ...but the DTM may depend on which port we're watching */
>>   		if (cmn->multi_dtm)
>>   			hw->dtm_offset = CMN_EVENT_WP_DEV_SEL(event) / 2;
>
diff mbox series

Patch

diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index dc6370396ad0..ce9fbdcf6144 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -91,10 +91,13 @@ 
 #define CMN600_WPn_CONFIG_WP_COMBINE	BIT(6)
 #define CMN600_WPn_CONFIG_WP_EXCLUSIVE	BIT(5)
 #define CMN_DTM_WPn_CONFIG_WP_GRP	GENMASK_ULL(5, 4)
+#define CMN600_WPn_CONFIG_WP_GRP	BIT(4)
 #define CMN_DTM_WPn_CONFIG_WP_CHN_SEL	GENMASK_ULL(3, 1)
 #define CMN_DTM_WPn_CONFIG_WP_DEV_SEL	BIT(0)
 #define CMN_DTM_WPn_VAL(n)		(CMN_DTM_WPn(n) + 0x08)
 #define CMN_DTM_WPn_MASK(n)		(CMN_DTM_WPn(n) + 0x10)
+#define CMN_DTM_WP_CHN_SEL_REQ_VC	0
+#define CMN_DTM_WP_GRP_TERTIARY		0x2
 
 #define CMN_DTM_PMU_CONFIG		0x210
 #define CMN__PMEVCNT0_INPUT_SEL		GENMASK_ULL(37, 32)
@@ -175,8 +178,8 @@ 
 #define CMN_CONFIG_WP_DEV_SEL		GENMASK_ULL(50, 48)
 #define CMN_CONFIG_WP_CHN_SEL		GENMASK_ULL(55, 51)
 /* Note that we don't yet support the tertiary match group on newer IPs */
-#define CMN_CONFIG_WP_GRP		BIT_ULL(56)
-#define CMN_CONFIG_WP_EXCLUSIVE		BIT_ULL(57)
+#define CMN_CONFIG_WP_GRP		GENMASK_ULL(57, 56)
+#define CMN_CONFIG_WP_EXCLUSIVE		BIT_ULL(58)
 #define CMN_CONFIG1_WP_VAL		GENMASK_ULL(63, 0)
 #define CMN_CONFIG2_WP_MASK		GENMASK_ULL(63, 0)
 
@@ -1298,7 +1301,9 @@  static struct attribute *arm_cmn_format_attrs[] = {
 
 	CMN_FORMAT_ATTR(CMN_ANY, wp_dev_sel, CMN_CONFIG_WP_DEV_SEL),
 	CMN_FORMAT_ATTR(CMN_ANY, wp_chn_sel, CMN_CONFIG_WP_CHN_SEL),
-	CMN_FORMAT_ATTR(CMN_ANY, wp_grp, CMN_CONFIG_WP_GRP),
+	CMN_FORMAT_ATTR(CMN600, wp_grp, CMN600_WPn_CONFIG_WP_GRP),
+	CMN_FORMAT_ATTR(NOT_CMN600, wp_grp, CMN_CONFIG_WP_GRP),
+
 	CMN_FORMAT_ATTR(CMN_ANY, wp_exclusive, CMN_CONFIG_WP_EXCLUSIVE),
 	CMN_FORMAT_ATTR(CMN_ANY, wp_combine, CMN_CONFIG_WP_COMBINE),
 
@@ -1398,8 +1403,11 @@  static u32 arm_cmn_wp_config(struct perf_event *event)
 
 	config = FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_DEV_SEL, dev) |
 		 FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_CHN_SEL, chn) |
-		 FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_GRP, grp) |
 		 FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_DEV_SEL2, dev >> 1);
+
+	if (grp)
+		config |= is_cmn600 ? CMN600_WPn_CONFIG_WP_GRP :
+				      FIELD_PREP(CMN_DTM_WPn_CONFIG_WP_GRP, grp);
 	if (exc)
 		config |= is_cmn600 ? CMN600_WPn_CONFIG_WP_EXCLUSIVE :
 				      CMN_DTM_WPn_CONFIG_WP_EXCLUSIVE;
@@ -1764,6 +1772,13 @@  static int arm_cmn_event_init(struct perf_event *event)
 		/* ...and we need a "real" direction */
 		if (eventid != CMN_WP_UP && eventid != CMN_WP_DOWN)
 			return -EINVAL;
+
+		if (cmn->part != PART_CMN600)
+			if (CMN_EVENT_WP_GRP(event) > CMN_DTM_WP_GRP_TERTIARY ||
+			    (CMN_EVENT_WP_GRP(event) == CMN_DTM_WP_GRP_TERTIARY &&
+			     CMN_EVENT_WP_CHN_SEL(event) != CMN_DTM_WP_CHN_SEL_REQ_VC))
+				return -EINVAL;
+
 		/* ...but the DTM may depend on which port we're watching */
 		if (cmn->multi_dtm)
 			hw->dtm_offset = CMN_EVENT_WP_DEV_SEL(event) / 2;