Re: [PATCH v4 2/2] perf/arm-cmn: Add workarounds for CMN-S3 on Graviton5

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Setting the DTC node's logical ID to the parent XP's logical ID causes DTC nodes to be sorted out of domain order, breaking PMU event routing.
- [High] The hardcoded bit-shifts in arm_cmn_graviton5_dtc_domain() can produce DTC domains >= 4, leading to out-of-bounds writes in arm_cmn_event_init().

Pre-existing issues:
- [High] Memory allocated for `hw->dtm_idx` and `hw->wp_idx` is leaked if `arm_cmn_event_init` fails.
--

commit c34c59523c8b94745143d1d1114415bb33c12d3e
Author: Aviv Bakal <avivb@amazon.com>

perf/arm-cmn: Add workarounds for CMN-S3 on Graviton5

Graviton5 uses a customised CMN-S3 implementation where certain discovery
registers report zeroed fields. This patch adds workarounds to identify the
variant, derive the DTC domain from the XP node ID, and set the DTC logical
ID from the XP's logical ID.

> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index f1978a53d1c19..3fb71d9a57eb5 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -2250,6 +2253,18 @@ static unsigned int arm_cmn_dtc_domain(struct arm_cmn *cmn, void __iomem *xp_reg
>  	return FIELD_GET(CMN_DTM_UNIT_INFO_DTC_DOMAIN, readl_relaxed(xp_region + offset));
>  }
>  
> +static unsigned int arm_cmn_graviton5_dtc_domain(u16 xp_id)
> +{
> +	unsigned int x = (xp_id >> 7) & 0xf;
> +	unsigned int y = (xp_id >> 3) & 0xf;
> +
> +	/*
> +	 * The unit info register reads as zero; derive the DTC domain from
> +	 * the XP's mesh coordinates over the 10x14 mesh.
> +	 */
> +	return (x / 5) + (y / 7) * 2;
> +}

[Severity: High]
Can the hardcoded bit-shifts extract overlapping bits if the mesh dimensions
are smaller than 10x14?

If the system describes a smaller mesh (e.g., 8x8), arm_cmn_xyidbits
evaluates to 3. This would cause the hardcoded shifts (xp_id >> 7) and
(xp_id >> 3) to extract overlapping bits that inflate y (e.g., y = 15),
yielding a domain of 5.

If a 14x10 mesh is used, the x coordinate can natively reach 13, yielding
a domain index of 4 ((13 / 5) + (9 / 7) * 2).

When an unvalidated domain index >= 4 is assigned to dn->dtc, does it
cause an out-of-bounds write in arm_cmn_event_init()?

drivers/perf/arm-cmn.c:arm_cmn_event_init() {
    ...
    hw->dtc_idx[dn->dtc] = 0;
    ...
}

Since hw->dtc_idx is a fixed 4-byte array (s8 dtc_idx[CMN_MAX_DTCS]), an
index >= 4 would write past its bounds and corrupt adjacent fields like
hw->num_dns.

[ ... ]

> @@ -2472,6 +2497,10 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
>  
>  			switch (dn->type) {
>  			case CMN_TYPE_DTC:
> +				if (graviton5_workaround) {
> +					/* Node info logical ID is zeroed; use the XP's */
> +					dn->logid = xp->logid;
> +				}

[Severity: High]
Does overriding the DTC logical ID with the parent XP's logical ID cause
DTC nodes to be sorted out of domain order?

In arm_cmn_init_dtcs(), CMN_TYPE_DTC nodes are sorted by their logical ID
(dn->logid), which determines their index in the dynamically allocated
cmn->dtc array. Standard CMN parts have their logid read from hardware as
the exact domain index (0-3).

Because the domain index here is a 2D block distribution, the physical XP IDs
do not monotonically increase with the domain index. For example, an XP at
x=7, y=0 belongs to domain 1 but has a smaller logid than an XP at
x=2, y=3 which belongs to domain 0.

If cmn->dtc[0] points to the hardware base of domain 1 instead of domain 0,
will events programmed for domain 0 be routed to domain 1's hardware, breaking
PMU event counting?

Would it be better to explicitly set this as:

    dn->logid = arm_cmn_graviton5_dtc_domain(xp->id);


[Severity: High]
This is a pre-existing issue, but does arm_cmn_event_init() leak memory
if initialization fails?

drivers/perf/arm-cmn.c:arm_cmn_event_init() {
    ...
    if (arm_cmn_is_watchpoint(event))
        if (eventid != CMN_WP_UP && eventid != CMN_WP_DOWN)
            return -EINVAL;
    ...
    return arm_cmn_validate_group(cmn, event);
}

The driver dynamically allocates memory using arm_cmn_alloc_dtm_idx() and
arm_cmn_alloc_wp_idx(). If the initialization subsequently encounters an
error, such as an invalid eventid for a watchpoint or a failure in
arm_cmn_validate_group(), it returns an error code without freeing the
allocated memory.

The core perf code perf_try_init_event() does not call the driver's
event->destroy() callback if pmu->event_init() fails, expecting the driver
to clean up its own resources. Could this allow memory leaks if invalid events
are repeatedly created?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531110447.10095-1-avivb@amazon.com?part=2