Re: [PATCH v5 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] The `arm_cmn_graviton5_dtc_domain` function computes a DTC domain index without upper bounds, which can exceed the `CMN_MAX_DTCS` limit and cause memory corruption.
- [High] The Graviton5 workaround assigns the XP's logical ID to the DTC node's logical ID, breaking the domain-based sorting order and causing DTC resources to be assigned incorrectly.

Pre-existing issues:
- [High] Lack of bounds checking on `cmn->num_dtcs` allows a `memset()` call to overflow the `dtc_idx` array.
--

commit fd0610a07c973430036fe29d5eab5bfbb5c0eb4c
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 introduces a
dedicated ACPI HID, derives the DTC domain from the XP node ID, and
sets 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 this calculation exceed the CMN_MAX_DTCS limit?

If a compromised or malformed firmware reports an XP node ID with
coordinate components up to 15 (e.g., x=15, y=15), this calculation
yields a domain index of 7.

The domain index is later inherited by the DTC node (dn->dtc) and used
directly as an array index in arm_cmn_event_init():

    hw->dtc_idx[dn->dtc] = 0;

Since the hw->dtc_idx array has a fixed size of CMN_MAX_DTCS (4), an
index of 4 or higher would overwrite adjacent struct fields like num_dns,
leading to memory corruption.

[ ... ]

> @@ -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 setting dn->logid to xp->logid break the domain-based sorting
order for DTC resources?

The CMN driver sequentially populates the cmn->dtc array based on the
sorted order of DTC nodes:

drivers/perf/arm-cmn.c:arm_cmn_init_dtcs() {
    ...
    int err = arm_cmn_init_dtc(cmn, dn, dtc_idx++);
    ...
}

Standard hardware sets a DTC node's logid to its domain ID (0..3),
ensuring the array is populated in domain order. However, XP logical
IDs do not monotonically increase with domain IDs (e.g., an XP in Domain 1
at X=5, Y=0 has a lower logid than an XP in Domain 0 at X=0, Y=1).

Will this cause the DTC nodes to sort out of order, misaligning the
cmn->dtc array and causing PMU events to be programmed into the wrong
hardware domain counters? Would it be safer to use dn->logid = xp->dtc?

>  				cmn->num_dtcs++;

[Severity: High]
This is a pre-existing issue, but does cmn->num_dtcs lack bounds checking?

During hardware discovery, the driver increments cmn->num_dtcs for every
CMN_TYPE_DTC node reported by the firmware, without capping it at
CMN_MAX_DTCS (4).

Later in arm_cmn_event_init(), the driver uses this unbounded value:

    memset(hw->dtc_idx, 0, cmn->num_dtcs);

Since hw->dtc_idx is a 4-byte array, if the firmware provides more than
4 DTC nodes, will the memset overflow the array and corrupt the
arm_cmn_hw_event struct?

>  				dn++;
>  				break;

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