linux-kernel - Re: [PATCH 2/8] perf/arm-cmn: Fix CCLA register offset

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zr8jJq3l9sfvJuil@J2N7QTR9R3>
Date: Fri, 16 Aug 2024 11:00:06 +0100
From: Mark Rutland <mark.rutland@....com>
To: Robin Murphy <robin.murphy@....com>
Cc: will@...nel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, ilkka@...amperecomputing.com
Subject: Re: [PATCH 2/8] perf/arm-cmn: Fix CCLA register offset

On Fri, Aug 09, 2024 at 08:15:41PM +0100, Robin Murphy wrote:
> Apparently pmu_event_sel is offset by 8 for all CCLA nodes, not just
> the CCLA_RNI combination type.

Was there some reason we used to think that was specific to CCLA_RNI
nodes, or was that just an oversight?

Looking at the CMN-700 TRM and scanning for pmu_event_sel, we have:

	16'h2000	por_ccg_ha_pmu_event_sel
	16'h2000	por_ccg_ra_pmu_event_sel
	16'h2008	por_ccla_pmu_event_sel
	16'h2000	por_dn_pmu_event_sel
	16'h2000	cmn_hns_pmu_event_sel
	16'h2000	por_hni_pmu_event_sel
	16'h2008	por_hnp_pmu_event_sel
	16'h2000	por_mxp_pmu_event_sel
	16'h2000	por_rnd_pmu_event_sel
	16'h2000	por_rni_pmu_event_sel
	16'h2000	por_sbsx_pmu_event_sel

> Fixes: 23760a014417 ("perf/arm-cmn: Add CMN-700 support")
> Signed-off-by: Robin Murphy <robin.murphy@....com>
> ---
>  drivers/perf/arm-cmn.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index fd2122a37f22..0e2e12e2f4fb 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -2393,10 +2393,13 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
>  			case CMN_TYPE_CXHA:
>  			case CMN_TYPE_CCRA:
>  			case CMN_TYPE_CCHA:
> -			case CMN_TYPE_CCLA:
>  			case CMN_TYPE_HNS:
>  				dn++;
>  				break;
> +			case CMN_TYPE_CCLA:
> +				dn->pmu_base += CMN_HNP_PMU_EVENT_SEL;
> +				dn++;
> +				break;

When reading this for the first time, it looks like a copy-paste error
since CMN_HNP_PMU_EVENT_SEL doesn't have any obvious relationship with
CCLA nodes.

I reckon it'd be worth adding CMN_CCLA_PMU_EVENT_SEL, and replacing the
existing comment above the definition of CMN_HNP_PMU_EVENT_SEL, e.g.

/*
 * Some nodes place common registers at different offsets from most
 * other nodes.
 */
#define CMN_HNP_PMU_EVENT_SEL		0x008
#define CMN_CCLA_PMU_EVENT_SEL		0x008

That way the switch looks less suspicious, and the comment is a bit more
helpful to anyone trying to figure out what's going on here.

With that:

Acked-by: Mark Rutland <mark.rutland@....com>

Mark.

>  			/* Nothing to see here */
>  			case CMN_TYPE_MPAM_S:
>  			case CMN_TYPE_MPAM_NS:
> -- 
> 2.39.2.101.g768bb238c484.dirty
>