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

[Ilkka Koskinen <ilkka@os.amperecomputing.com>]

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