Re: [PATCH v5 16/17] coresight: Emit sink ID in the HW_ID packets

[James Clark <james.clark@linaro.org>]

On 17/07/2024 4:03 pm, Mike Leach wrote:
> On Fri, 12 Jul 2024 at 11:23, James Clark <james.clark@linaro.org> wrote:
>>
>> From: James Clark <james.clark@arm.com>
>>
>> For Perf to be able to decode when per-sink trace IDs are used, emit the
>> sink that's being written to for each ETM.
>>
>> Perf currently errors out if it sees a newer packet version so instead
>> of bumping it, add a new minor version field. This can be used to
>> signify new versions that have backwards compatible fields. Considering
>> this change is only for high core count machines, it doesn't make sense
>> to make a breaking change for everyone.
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/hwtracing/coresight/coresight-core.c  | 26 ++++++++++---------
>>   .../hwtracing/coresight/coresight-etm-perf.c  | 16 ++++++++----
>>   drivers/hwtracing/coresight/coresight-priv.h  |  1 +
>>   include/linux/coresight-pmu.h                 | 17 +++++++++---
>>   4 files changed, 39 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
>> index faf560ba8d64..c427e9344a84 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -487,23 +487,25 @@ struct coresight_device *coresight_get_sink(struct list_head *path)
>>          return csdev;
>>   }
>>
>> +u32 coresight_get_sink_id(struct coresight_device *csdev)
>> +{
>> +       if (!csdev->ea)
>> +               return 0;
>> +
>> +       /*
>> +        * See function etm_perf_add_symlink_sink() to know where
>> +        * this comes from.
>> +        */
>> +       return (u32) (unsigned long) csdev->ea->var;
>> +}
>> +
>>   static int coresight_sink_by_id(struct device *dev, const void *data)
>>   {
>>          struct coresight_device *csdev = to_coresight_device(dev);
>> -       unsigned long hash;
>>
>>          if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
>> -            csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
>> -
>> -               if (!csdev->ea)
>> -                       return 0;
>> -               /*
>> -                * See function etm_perf_add_symlink_sink() to know where
>> -                * this comes from.
>> -                */
>> -               hash = (unsigned long)csdev->ea->var;
>> -
>> -               if ((u32)hash == *(u32 *)data)
>> +           csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
>> +               if (coresight_get_sink_id(csdev) == *(u32 *)data)
>>                          return 1;
>>          }
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> index 70c99f0409b2..ad6a8f4b70b6 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> @@ -460,6 +460,7 @@ static void etm_event_start(struct perf_event *event, int flags)
>>          struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu);
>>          struct list_head *path;
>>          u64 hw_id;
>> +       u8 trace_id;
>>
>>          if (!csdev)
>>                  goto fail;
>> @@ -512,11 +513,16 @@ static void etm_event_start(struct perf_event *event, int flags)
>>           */
>>          if (!cpumask_test_cpu(cpu, &event_data->aux_hwid_done)) {
>>                  cpumask_set_cpu(cpu, &event_data->aux_hwid_done);
>> -               hw_id = FIELD_PREP(CS_AUX_HW_ID_VERSION_MASK,
>> -                                  CS_AUX_HW_ID_CURR_VERSION);
>> -               hw_id |= FIELD_PREP(CS_AUX_HW_ID_TRACE_ID_MASK,
>> -                                   coresight_trace_id_read_cpu_id_map(cpu,
>> -                                                                      &sink->perf_sink_id_map));
>> +
>> +               trace_id = coresight_trace_id_read_cpu_id_map(cpu, &sink->perf_sink_id_map);
>> +
>> +               hw_id = FIELD_PREP(CS_AUX_HW_ID_MAJOR_VERSION_MASK,
>> +                               CS_AUX_HW_ID_MAJOR_VERSION);
>> +               hw_id |= FIELD_PREP(CS_AUX_HW_ID_MINOR_VERSION_MASK,
>> +                               CS_AUX_HW_ID_MINOR_VERSION);
>> +               hw_id |= FIELD_PREP(CS_AUX_HW_ID_TRACE_ID_MASK, trace_id);
>> +               hw_id |= FIELD_PREP(CS_AUX_HW_ID_SINK_ID_MASK, coresight_get_sink_id(sink));
>> +
>>                  perf_report_aux_output_id(event, hw_id);
>>          }
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
>> index 61a46d3bdcc8..05f891ca6b5c 100644
>> --- a/drivers/hwtracing/coresight/coresight-priv.h
>> +++ b/drivers/hwtracing/coresight/coresight-priv.h
>> @@ -148,6 +148,7 @@ int coresight_make_links(struct coresight_device *orig,
>>                           struct coresight_device *target);
>>   void coresight_remove_links(struct coresight_device *orig,
>>                              struct coresight_connection *conn);
>> +u32 coresight_get_sink_id(struct coresight_device *csdev);
>>
>>   #if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM3X)
>>   extern int etm_readl_cp14(u32 off, unsigned int *val);
>> diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h
>> index 51ac441a37c3..89b0ac0014b0 100644
>> --- a/include/linux/coresight-pmu.h
>> +++ b/include/linux/coresight-pmu.h
>> @@ -49,12 +49,21 @@
>>    * Interpretation of the PERF_RECORD_AUX_OUTPUT_HW_ID payload.
>>    * Used to associate a CPU with the CoreSight Trace ID.
>>    * [07:00] - Trace ID - uses 8 bits to make value easy to read in file.
>> - * [59:08] - Unused (SBZ)
>> - * [63:60] - Version
>> + * [39:08] - Sink ID - as reported in /sys/bus/event_source/devices/cs_etm/sinks/
>> + *           Added in minor version 1.
>> + * [55:40] - Unused (SBZ)
>> + * [59:56] - Minor Version - previously existing fields are compatible with
>> + *           all minor versions.
>> + * [63:60] - Major Version - previously existing fields mean different things
>> + *           in new major versions.
>>    */
>>   #define CS_AUX_HW_ID_TRACE_ID_MASK     GENMASK_ULL(7, 0)
>> -#define CS_AUX_HW_ID_VERSION_MASK      GENMASK_ULL(63, 60)
>> +#define CS_AUX_HW_ID_SINK_ID_MASK      GENMASK_ULL(39, 8)
>>
>> -#define CS_AUX_HW_ID_CURR_VERSION 0
>> +#define CS_AUX_HW_ID_MINOR_VERSION_MASK        GENMASK_ULL(59, 56)
>> +#define CS_AUX_HW_ID_MAJOR_VERSION_MASK        GENMASK_ULL(63, 60)
>> +
>> +#define CS_AUX_HW_ID_MAJOR_VERSION 0
>> +#define CS_AUX_HW_ID_MINOR_VERSION 1
>>
>>   #endif
>> --
>> 2.34.1
>>
> 
> 
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK

Hi Mike,

I think you miss-sent this one