Re: [PATCH v4 04/10] coresight: etm-perf: update to handle configuration selection

[Mike Leach]

Hi

On Thu, 4 Mar 2021 at 14:25, Suzuki K Poulose <suzuki.poulose@xxxxxxx> wrote:
>
> On 3/4/21 2:19 PM, Mike Leach wrote:
> > Hi Suzuki,
> >
> > On Thu, 4 Mar 2021 at 12:13, Suzuki K Poulose <suzuki.poulose@xxxxxxx> wrote:
> >>
> >> On 1/28/21 5:09 PM, Mike Leach wrote:
> >>> Loaded coresight configurations are registered in the cs_etm\cs_config sub
> >>> directory. This extends the etm-perf code to handle these registrations,
> >>> and the cs_syscfg driver to perform the registration on load.
> >>>
> >>> Signed-off-by: Mike Leach <mike.leach@xxxxxxxxxx>
> >>> ---
> >>>    .../hwtracing/coresight/coresight-config.h    |   5 +-
> >>>    .../hwtracing/coresight/coresight-etm-perf.c  | 164 +++++++++++++++---
> >>>    .../hwtracing/coresight/coresight-etm-perf.h  |   8 +
> >>>    .../hwtracing/coresight/coresight-syscfg.c    |  13 +-
> >>>    4 files changed, 166 insertions(+), 24 deletions(-)
> >>>
>
>
> >>> +static ssize_t etm_perf_cscfg_event_show(struct device *dev,
> >>> +                                      struct device_attribute *dattr,
> >>> +                                      char *buf)
> >>> +{
> >>> +     struct dev_ext_attribute *ea;
> >>> +
> >>> +     ea = container_of(dattr, struct dev_ext_attribute, attr);
> >>> +     return scnprintf(buf, PAGE_SIZE, "%s\n", (const char *)(ea->var));
> >>> +}
> >>
> >> "configid=0x%lx", (unsigned long)ea->var ?
> >>
> >
> > ea->var _is_ "configid=0x%lx" due to the way perf handles the events
> > sub-dir entries.
> >
>
> This must be combined with the suggestion below.
>
> >>> +
> >>> +static int etm_perf_add_cscfg_event(struct device *dev, struct cscfg_config_desc *cs_cfg)
> >>> +{
> >>> +     struct dev_ext_attribute *ea;
> >>> +     unsigned long hash;
> >>> +     int ret;
> >>> +     struct device *pmu_dev = etm_pmu.dev;
> >>> +
> >>> +     ea = devm_kzalloc(dev, sizeof(*ea), GFP_KERNEL);
> >>> +     if (!ea)
> >>> +             return -ENOMEM;
> >>> +
> >>> +     hash = (unsigned long)cs_cfg->id_ea->var;
> >>> +
> >>> +     sysfs_attr_init(&ea->attr.attr);
> >>> +     ea->attr.attr.name = devm_kstrdup(dev, cs_cfg->name, GFP_KERNEL);
> >>> +     if (!ea->attr.attr.name)
> >>> +             return -ENOMEM;
> >>> +
> >>> +     /*
> >>> +      * attribute value is "configid=<hash>".
> >>> +      * this will be what perf evaluates when the config name is used
> >>> +      * on the command line.
> >>> +      */
> >>> +     ea->var = devm_kzalloc(dev, CSCFG_EVENT_STR_SIZE, GFP_KERNEL);
> >>> +     if (!ea->var)
> >>> +             return -ENOMEM;
> >>
> >> Could we drop this string and use the "hash" instead ?
> >>
> >
> > No. My understanding is that we have added an events directory to
> > cs_etm, and add the configurations in there:-
> >
> > cs_etm/events/autofdo
> >
> > Now the contents of autofdo are "configid=0x<hash-value>" - where
> > hash-value is the hash of "autofdo".
> >
> > On the perf command line:-
> >
> > perf record -e cs_etm/autofdo/ .....
> >
> > will result in perf parsing autofdo, looking in the events dir for
> > cs_etm, seeing the configid=-string, and parsing that to assign to
> > configid attribute - which we have allocated to config2:63:32 - this
> > will then appear as a value in the perf_event and we can load the
> > configuration when starting up the event on the ETM etc.
>
> Sorry, I was not explicit in my comments. You could drop the string and
> have ea->var = hash. And the _show() could simply do
>
> "configid=0x%lx" , hash
>
> as mentioned above.
>
> That would avoid another string allocation, with the same interface.
>

OK, that makes sense.

Mike

> Suzuki



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK