Messages in this thread | | | From | Mike Leach <> | Date | Tue, 12 Jan 2021 23:43:55 +0000 | 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@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
| |