[PATCH 2/4] perf/arm-cmn: Add CMN-650 support
Ilkka Koskinen
ilkka at os.amperecomputing.com
Thu Apr 21 00:09:09 PDT 2022
On Wed, 20 Apr 2022, Robin Murphy wrote:
> On 2022-04-20 00:05, Ilkka Koskinen wrote:
>>
>> Hi Robin,
>>
>> I need to go through your patches more carefully, but I do have a couple of
>> comments already:
>
> Thanks for having a look!
>
>> On Mon, 18 Apr 2022, Robin Murphy wrote:
>>> Add the identifiers and events for CMN-650, which slots into its
>>> evolutionary position between CMN-600 and the 700-series products.
>>> Imagine CMN-600 made bigger, and with most of the rough edges smoothed
>>> off, but that then balanced out by some bonkers PMU functionality for
>>> the new HN-P enhancement in CMN-650r2.
>>>
>>> Most of the CXG events are actually common to newer revisions of CMN-600
>>> too, so they're arguably a little late; oh well.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy at arm.com>
>>> ---
>>> drivers/perf/arm-cmn.c | 222 ++++++++++++++++++++++++++++++++---------
>>> 1 file changed, 176 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
>>> index 9c1d82be7a2f..cce8516d465c 100644
>>> --- a/drivers/perf/arm-cmn.c
>>> +++ b/drivers/perf/arm-cmn.c
>>> @@ -39,7 +39,7 @@
>>> #define CMN_CHILD_NODE_ADDR GENMASK(27, 0)
>>> #define CMN_CHILD_NODE_EXTERNAL BIT(31)
>>>
>>> -#define CMN_MAX_DIMENSION 8
>>> +#define CMN_MAX_DIMENSION 12
>>
>> I wonder if it made sense to dynamically allocate the arrays later in the
>> code instead of allocating them in stack, especially if mesh topologies
>> keeps growing fast. That would probably avoid setting max dimension
>> altogether if one could use num_xps, num_dns etc. Just for future
>> thoughts...
>
> Note that the group validation structure *is* dynamically allocated since the
> last update, since it was already getting a bit big for the stack; it's just
> not dynamically *sized*. That's a compromise to keep the validation code as
> simple and efficient as it reasonably can be. I'm not entirely convinced that
> extra complexity and runtime overhead for everyone is worth it for the sake
> of making it slightly easier to catch an obvious bug if someone makes
> out-of-tree hacks to the driver. This is not the only place which needs
> updating (or at least checking) if the maximum number of possible DTMs really
> does increase again.
Probably not. If the mesh size grows remarkably, then it might make sense
to revisit but it should be ok now.
>
>>> #define CMN_MAX_XPS (CMN_MAX_DIMENSION * CMN_MAX_DIMENSION)
>>> #define CMN_MAX_DTMS (CMN_MAX_XPS + (CMN_MAX_DIMENSION - 1) *
>>> 4)
>>
>> <snip>
>>
>>> @@ -1692,8 +1802,13 @@ static int arm_cmn_discover(struct arm_cmn *cmn,
>>> unsigned int rgn_offset)
>>> cmn->num_dns += FIELD_GET(CMN_CI_CHILD_COUNT, reg);
>>
>> How about checking if child count is bigger than the maximum mesh size
>> before this for loop? It would help in case someone would work on enabling
>> support for new, bigger models and would forget to update
>> CMN_MAX_DIMENSION...
>
> Hmm, I guess we do already warn and bail out if we find a mystery node type
> that implies we're being lied to, but TBH that was always more to avoid
> compilers moaning about the switch statement lacking a default case than
> because I thought it's a necessary check in itself. Ultimately if someone
> lies to the driver and claims that a CMN product is a different CMN product,
> it's already not going to work properly due to the subtle differences in the
> hardware, so I'd argue that potential memory corruption due to overrunning
> array bounds in various places is really just part and parcel of "not working
> properly".
>
> CMN-700 r0 and r2 seemed clear that 12x12 is the largest supported dimension;
> r1 seemed a bit ambiguous between what the TRM said and what I could find in
> the actual product deliverables, so I can double-check with the hardware team
> if you like - or if you already know better, please do feel free to correct
> my assumption :)
That's fair point. I'm fine as it is now.
Cheers, Ilkka
>>> }
>>>
>>> - /* Cheeky +1 to help terminate pointer-based iteration later */
>>> - dn = devm_kcalloc(cmn->dev, cmn->num_dns + 1, sizeof(*dn),
>>> GFP_KERNEL);
>>> + /*
>>> + * Some nodes effectively have two separate types, which we'll handle
>>> + * by creating one of each internally. For a (very) safe initial
>>> upper
>>> + * bound, account for double the number of non-XP nodes.
>>> + */
>>> + dn = devm_kcalloc(cmn->dev, cmn->num_dns * 2 - cmn->num_xps,
>>> + sizeof(*dn), GFP_KERNEL);
>>> if (!dn)
>>> return -ENOMEM;
>>>
>>
>> <snip>
>>
>>> @@ -1970,6 +2098,7 @@ static int arm_cmn_remove(struct platform_device
>>> *pdev)
>>> #ifdef CONFIG_OF
>>> static const struct of_device_id arm_cmn_of_match[] = {
>>> { .compatible = "arm,cmn-600", .data = (void *)CMN600 },
>>> + { .compatible = "arm,cmn-650", .data = (void *)CMN650 },
>>> { .compatible = "arm,ci-700", .data = (void *)CI700 },
>>> {}
>>> };
>>> @@ -1979,6 +2108,7 @@ MODULE_DEVICE_TABLE(of, arm_cmn_of_match);
>>> #ifdef CONFIG_ACPI
>>> static const struct acpi_device_id arm_cmn_acpi_match[] = {
>>> { "ARMHC600", CMN600 },
>>> + { "ARMHC650", CMN650 },
>>
>> Not the great place for this comment but there probably isn't any better.
>>
>> Based on DEN0093 v1.1, CMN's DSDT entries have been changed since CMN-600.
>> ROOTNODEBASE seems to be specific for CMN-600. Moreover, if you compare the
>> address maps in TRMs' Discovery chapters, you can see the difference.
>>
>> I'm thinking, at the minimal the second platform_get_resource() call has to
>> be skipped and zero returned in arm_cmn600_acpi_probe(), if the model is
>> cmn650 (probably also for cmn-700)
>
> As you've already found, things prefixed with "arm_cmn600_" vs. "arm_cmn_"
> *are* specific to CMN-600, and the CI-700 update reworked this area such that
> everything else simply probes in a firmware-agnostic manner. It may have been
> a bit subtle since CI-700 doesn't have an ACPI binding, but it was very much
> intended to cover these future ACPI additions as well.
>
> Thanks,
> Robin.
>
More information about the linux-arm-kernel
mailing list