Date:   Tue, 12 Jan 2021 23:43:55 +0000
From:   Mike Leach <mike.leach@...aro.org>
To:     Leo Yan <leo.yan@...aro.org>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        John Garry <john.garry@...wei.com>,
        Will Deacon <will@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Daniel Kiss <Daniel.Kiss@....com>,
        Denis Nikitin <denik@...omium.org>,
        Coresight ML <coresight@...ts.linaro.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Al Grant <al.grant@....com>
Subject: Re: [PATCH v1 1/7] coresight: etm-perf: Add support for PID tracing
 for kernel at EL2

Hi Leo,

On Tue, 12 Jan 2021 at 14:15, Leo Yan <leo.yan@...aro.org> wrote:
>
> Hi Mike,
>
> On Tue, Jan 12, 2021 at 11:23:03AM +0000, Mike Leach wrote:
> > Hi Leo,
> >
> > On Tue, 12 Jan 2021 at 08:58, Leo Yan <leo.yan@...aro.org> wrote:
> > >
> > > Hi Mike,
> > >
> > > On Mon, Jan 11, 2021 at 04:22:39PM +0000, Mike Leach wrote:
> > >
> > > [...]
> > >
> > > > > diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h
> > > > > index b0e35eec6499..927c6285ce5d 100644
> > > > > --- a/include/linux/coresight-pmu.h
> > > > > +++ b/include/linux/coresight-pmu.h
> > > > > @@ -11,16 +11,19 @@
> > > > >  #define CORESIGHT_ETM_PMU_SEED  0x10
> > > > >
> > > > >  /* ETMv3.5/PTM's ETMCR config bit */
> > > > > -#define ETM_OPT_CYCACC  12
> > > > > -#define ETM_OPT_CTXTID 14
> > > > > -#define ETM_OPT_TS      28
> > > > > -#define ETM_OPT_RETSTK 29
> > > > > +#define ETM_OPT_CYCACC         12
> > > > > +#define ETM_OPT_CTXTID         14
> > > > > +#define ETM_OPT_CTXTID_IN_VMID 15
> > > >
> > > > Minor issue here - ETMv3.x / PTM cannot trace CXTID in VMID so this
> > > > may better be named ETM4_OPT_CTXTID_IN_VMID, rather than be grouped
> > > > with the ETM3.5 options?
> > >
> > > I looked into this suggestion but found it's complex than I assumed.
> > > This config bits are not only used for ETMv3.x / PTM, it's also used
> > > as an configuration interface between user space in Perf and kernel
> > > drivers.
> > >
> > > For example, in the userspace, perf tool sets bit ETM_OPT_TS to enable
> > > timestamp [1], this is same for ETMv3 and ETMv4.  In the kernel side,
> > > the configuration is directly used ETMv3 (in coresight-etm3x-core.c),
> > > but the configuration bits are converted for ETMv4 in the function
> > > etm4_parse_event_config() [2].
> > >
> > > So this is a historical issue, at the early period ETMv3 and ETMv4 can
> > > be compatible with each other for configurations, but after evoluation,
> > > some configs only belong to ETMv4 and cannot be applied on ETMv3
> > > anymore, but we still use ETMv3.5 config bits as the interface between
> > > kernel and userspace.
> > >
> >
> > I was aware that etm3/ptm used these bits as both the options and the
> > bit values for direct writing to the ETMCR register for ETMv3, and
> > re-mapped to appropriate register values in ETMv4.
> > In the past we have re-used etmv3.5 bit definitions ETM_xxx  when
> > appropriate, but where unique to ETM4 we  have used a ETM4_xxx naming
> > convention.
>
> I am concern this approach is not friendly for extension; for example,
> let's say IP ETM5 with defined bit 28 as CTXTID, if add a new option
> for it, we need to define macro as:
>
>         #define ETM5_OPT_CTXTID         28
>
> This will result in confliction with the existed option ETM_OPT_TS
> and it is hard for maintenance for options, since there have different
> prefixes (like ETM_OPT_xxx, ETM4_OPT_xxx, ETM5_OPT_xxx, etc).
>

No it will not - we don't need a new option for CTXTID in a
hypothetical ETM5 - as we use the existing one for ETM3 and map it to
the correct bit, just as ETM4 does.


> I'd like to take option as knob to enable or disable hardware
> feature; the low level drivers should set the appropriate values for
> registers based on different options.
>
> Furthermore, ETM driver should report error when detect any option is
> not supported, I.e. ETM3 driver should report failure if user wrongly
> set the option ETM_OPT_CTXTID_IN_VMID.
>
> > I am not suggesting re-factoring the options completely, just
> > re-naming this single option to make it clear it is unique to ETM4+.
>
> Here I perfer Suzuki's suggestion to simply refine comments, something
> like below:
>
> /*
>  * Below are bit offsets for perf options, most of them are orignally
>  * coming from ETMv3.5/PTM's ETMCR config bits (so far except
>  * ETM_OPT_CTXTID_IN_VMID is only used for ETMv4).
>  *
>  * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and
>  * directly use below macros as config bits.
>  */
> #define ETM_OPT_CYCACC          12
> #define ETM_OPT_CTXTID          14
> #define ETM_OPT_CTXTID_IN_VMID  15
> #define ETM_OPT_TS              28
> #define ETM_OPT_RETSTK          29
>
> > Looking at the etmv3 driver, at present it does not actually appear to
> > support contextid tracing - and when it does, both bits 14 and 15 will
> > be required to be used - as ETMCR defines these bits as ContextID
> > size.
> > Should this ever get fixed.
>
> Good catch!  Seems to me, this is a good example that we should
> distinguish the definition between Perf options and config bits :)
>
> > then having an overlapping option bit -
> > that appears to be valid for ETMv3 will be confusing.
>
> I hope the the proposed change can avoid the confusion, if have
> concern, please let me know.
>
> Thanks a lot for suggestions,
> Leo

If you think that clarification via comment is better than a change of
name then go ahead.

Regards

Mike

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

Powered by blists - more mailing lists