| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7cea725-940-4728-2879-529a49bceda@os.amperecomputing.com>
Date: Mon, 29 Jan 2024 21:35:51 -0800 (PST)
From: Ilkka Koskinen <ilkka@...amperecomputing.com>
To: Robin Murphy <robin.murphy@....com>
cc: Ilkka Koskinen <ilkka@...amperecomputing.com>,
Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] perf/arm-cmn: Enable support for tertiary match
group
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@...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;
>
Powered by blists - more mailing lists