On Thu, 14 Mar 2019 at 05:16, Suzuki K Poulose suzuki.poulose@arm.com wrote:
On 07/03/2019 17:53, Mathieu Poirier wrote:
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index 08ce37c9475d..f067f7ee08c7 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -239,6 +239,11 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata, if (attr->config & BIT(ETM_OPT_TS)) /* bit[11], Global timestamp tracing bit */ config->cfg |= BIT(11);
if (attr->config & BIT(ETM_OPT_CTXTID))/* bit[6], Context ID tracing bit */config->cfg |= BIT(6);nit: Could we use the ETM4_CFG_BIT_CTXTID here rather than hard coding the value we already define ?
We certainly can.
Otherwise:
Reviewed-by: Suzuki K Poulose suzuki.poulose@arm.com
Btw, someday, we could make this an array of attr->config bits defs and their corresponding drvdata->config bit defs and be done with it, rather than adding separate checks like.
A conversion must be made because configuration bits differ between ETMv3.x and ETMv4. Since ETMv3.x came first I kept the bits specified for that architecture but it is only a matter of time before that strategy doesn't work anymore. I envision a time in the not so distant future where new options will come out with configuration bits completely orthogonal to what we currently have. When that happens user space will have to set attr->config for each ETM flavour, essentially removing the need for bit conversion. At that point attr->config and drvdata->config will be the same for each ETM architecture.
diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h index a1a959ba24ff..b0e35eec6499 100644 --- a/tools/include/linux/coresight-pmu.h +++ b/tools/include/linux/coresight-pmu.h @@ -12,11 +12,13 @@
/* 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
/* ETMv4 CONFIGR programming bits for the ETM OPTs */ #define ETM4_CFG_BIT_CYCACC 4 +#define ETM4_CFG_BIT_CTXTID 6 #define ETM4_CFG_BIT_TS 11 #define ETM4_CFG_BIT_RETSTK 12
nit: Should this be really exposed to the perf tool ? The tool must always rely on the "format" strings, isn't it ? That way, we are free to change the kernel definitions and get the tool working everywhere ?
We don't have a choice to copy coresight-pmu.h over to the tools' side. Otherwise there is a script that compares the perf headers with the kernel headers that complains bitterly, something that makes Acme very unhappy.
Ok. But, my question is, why do the perf tool need to know this from the header file. AFAICS, all of this must be exposed via sysfs format. We could certainly remove the CFG bits.
I see your point and it is a valid one. On the flip side doing so would introduce a fair amount of churn in the perf tools that is not related to this feature. Given the already substantial amount of modifications there that are related to this feature, it should probably be treated separately.
Thanks, Mathieu
Cheers Suzuki