Re: [PATCH v2 1/6] coresight: Add config flag to enable branch broadcast

[Suzuki K Poulose]

On 22/04/2022 11:18, James Clark wrote:
> On 11/03/2022 15:56, Mike Leach wrote:
> > Hi James,
> >
> > On Fri, 11 Mar 2022 at 14:58, James Clark <james.clark@xxxxxxx> wrote:
> > > On 02/02/2022 20:25, Mike Leach wrote:
> > > > Hi James, Suzuki
> > > >
> > > > On Fri, 28 Jan 2022 at 11:19, Suzuki K Poulose <suzuki.poulose@xxxxxxx> wrote:
> > > > > On 13/01/2022 09:10, James Clark wrote:
> > > > > > When enabled, all taken branch addresses are output, even if the branch
> > > > > > was because of a direct branch instruction. This enables reconstruction
> > > > > > of the program flow without having access to the memory image of the
> > > > > > code being executed.
> > > > > >
> > > > > > Use bit 8 for the config option which would be the correct bit for
> > > > > > programming ETMv3. Although branch broadcast can't be enabled on ETMv3
> > > > > > because it's not in the define ETM3X_SUPPORTED_OPTIONS, using the
> > > > > > correct bit might help prevent future collisions or allow it to be
> > > > > > enabled if needed.
> > > > > >
> > > > > > Signed-off-by: James Clark <james.clark@xxxxxxx>
> > > > > > ---
> > > > > >    drivers/hwtracing/coresight/coresight-etm-perf.c   |  2 ++
> > > > > >    drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 ++++++++++
> > > > > >    include/linux/coresight-pmu.h                      |  2 ++
> > > > > >    3 files changed, 14 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > > > index c039b6ae206f..43bbd5dc3d3b 100644
> > > > > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > > > @@ -52,6 +52,7 @@ static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
> > > > > >     * The PMU formats were orignally for ETMv3.5/PTM's ETMCR 'config';
> > > > > >     * now take them as general formats and apply on all ETMs.
> > > > > >     */
> > > > > > +PMU_FORMAT_ATTR(branch_broadcast, "config:"__stringify(ETM_OPT_BRANCH_BROADCAST));
> > > > > >    PMU_FORMAT_ATTR(cycacc,             "config:" __stringify(ETM_OPT_CYCACC));
> > > > > >    /* contextid1 enables tracing CONTEXTIDR_EL1 for ETMv4 */
> > > > > >    PMU_FORMAT_ATTR(contextid1, "config:" __stringify(ETM_OPT_CTXTID));
> > > > > > @@ -97,6 +98,7 @@ static struct attribute *etm_config_formats_attr[] = {
> > > > > >        &format_attr_sinkid.attr,
> > > > > >        &format_attr_preset.attr,
> > > > > >        &format_attr_configid.attr,
> > > > > > +     &format_attr_branch_broadcast.attr,
> > > > >
> > > > > Does it make sense to hide the option if the bb is not supported ? I
> > > > > guess it will be tricky as we don't track the common feature set. So,
> > > > > that said...>>>>
> > > > > >        NULL,
> > > > > >    };
> > > > > >
> > > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > > > > index bf18128cf5de..04669ecc0efa 100644
> > > > > > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > > > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > > > > @@ -692,6 +692,16 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
> > > > > >                ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset);
> > > > > >        }
> > > > > >
> > > > > > +     /* branch broadcast - enable if selected and supported */
> > > > > > +     if (attr->config & BIT(ETM_OPT_BRANCH_BROADCAST)) {
> > > > > > +             if (!drvdata->trcbb) {
> > > > > > +                     ret = -EINVAL;
> > > > >
> > > > > Should we fail here ? We could simply ignore this and generate the trace
> > > > > normally. This would work on a big.LITTLE system with one set missing
> > > > > the branch broadcast, while the others support.
>
> Hi Suzuki,
>
> I think if the user really needs branch broadcast because they've modified their
> binaries then they would want feedback that one of the cores doesn't support it.
> Otherwise their decode would silently go wrong and they could get stuck.
>
> Perf already opens an event per core, so it wouldn't be much effort to manually
> modify the command line to only select cores where it is supported.
>
> I think for that scenario the current patch already works that way because the
> feature is checked on the active core when the event is enabled?

Fair enough. We could let the user pin the perf to the CPUs where this
is supported. This information is available to the user via sysfs.

So, let us proceed with the changes here.

Cheers
Suzuki