Re: [PATCH 2/4] perf/arm-cmn: Add CMN-650 support

[Ilkka Koskinen <ilkka@os.amperecomputing.com>]

[-- Attachment #1: Type: text/plain, Size: 6329 bytes --]



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@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.
>

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel