lkml.org 
[lkml]   [2021]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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@linaro.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@linaro.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

    \
     
     \ /
      Last update: 2021-01-13 01:56    [W:2.515 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site