| Message ID | 20200818214312.30793-1-mike.leach@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: etm4x: Ensure default perf settings filter user/kernel | expand |
Hi Mike, On Tue, Aug 18, 2020 at 10:43:12PM +0100, Mike Leach wrote: > Moving from using an address filter to trace the default "all addresses" > range to no filtering to acheive the same result, has caused the perf > filtering of kernel/user address spaces from not working unless an > explicit address filter was used. > > This due to the global TRCVICTLR exception level filtering not being set > correctly. > > This patch fixes this. Very cool! > Reported-by: Leo Yan <leo.yan@linaro.org> > Signed-off-by: Mike Leach <mike.leach@linaro.org> > --- > drivers/hwtracing/coresight/coresight-etm4x.c | 11 +++++++++++ > drivers/hwtracing/coresight/coresight-etm4x.h | 3 +++ > 2 files changed, 14 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c > index 8b0634ebef77..d0f800753590 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x.c > @@ -52,6 +52,7 @@ static struct etmv4_drvdata *etmdrvdata[NR_CPUS]; > static void etm4_set_default_config(struct etmv4_config *config); > static int etm4_set_event_filters(struct etmv4_drvdata *drvdata, > struct perf_event *event); > +static u64 etm4_get_access_type(struct etmv4_config *config); > > static enum cpuhp_state hp_online; > > @@ -785,6 +786,8 @@ static void etm4_init_arch_data(void *info) > > static void etm4_set_default_config(struct etmv4_config *config) > { > + u64 access_type; > + > /* disable all events tracing */ > config->eventctrl0 = 0x0; > config->eventctrl1 = 0x0; > @@ -800,6 +803,14 @@ static void etm4_set_default_config(struct etmv4_config *config) > > /* TRCVICTLR::EVENT = 0x01, select the always on logic */ > config->vinst_ctrl = BIT(0); > + > + /* > + * TRCVICTLR::EXLEVEL_NS:EXLEVELS: Set kernel / user filtering > + * bits in vinst, same bit pattern as address comparator acc values > + * but offset in this register > + */ > + access_type = etm4_get_access_type(config) << ETM_EXLEVEL_VINST_OFFSET; > + config->vinst_ctrl |= (u32)access_type; Just a minor comment, here the definition for ETM_EXLEVEL_VINST_OFFSET is easily to introduce confusion. In the register TRCACATRn, EXLEVEL_NS:EXLEVELS offset is 8, and in the registser TRCVICTLR the EXLEVEL_NS:EXLEVELS offset is 16. So for more explict expression, we need to define below two macros: #define ETM_EXLEVEL_TRCVICTLR_OFFSET 16 #define ETM_EXLEVEL_TRCACATRn_OFFSET 8 access_type = etm4_get_access_type(config) >> ETM_EXLEVEL_TRCACATRn_OFFSET; access_type = access_type << ETM_EXLEVEL_VINST_OFFSET; config->vinst_ctrl |= (u32)access_type; Otherwise, the change is good for me: Reviewed-by: Leo Yan <leo.yan@linaro.org> Tested-by: Leo Yan <leo.yan@linaro.org> And thanks a lot for the quick fixing! Leo > static u64 etm4_get_ns_access_type(struct etmv4_config *config) > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h > index b8283e1d6d88..369abc25c597 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x.h > +++ b/drivers/hwtracing/coresight/coresight-etm4x.h > @@ -192,6 +192,9 @@ > #define ETM_EXLEVEL_NS_HYP BIT(14) > #define ETM_EXLEVEL_NS_NA BIT(15) > > +/* access level control in VINST - same bits as TRCACATRn but offset */ > +#define ETM_EXLEVEL_VINST_OFFSET 8 > + > /* secure / non secure masks - TRCVICTLR, IDR3 */ > #define ETM_EXLEVEL_S_VICTLR_MASK GENMASK(19, 16) > /* NS MON (EL3) mode never implemented */ > -- > 2.17.1 >
Hi Leo On Wed, 19 Aug 2020 at 03:11, Leo Yan <leo.yan@linaro.org> wrote: > > Hi Mike, > > On Tue, Aug 18, 2020 at 10:43:12PM +0100, Mike Leach wrote: > > Moving from using an address filter to trace the default "all addresses" > > range to no filtering to acheive the same result, has caused the perf > > filtering of kernel/user address spaces from not working unless an > > explicit address filter was used. > > > > This due to the global TRCVICTLR exception level filtering not being set > > correctly. > > > > This patch fixes this. > > Very cool! > > > Reported-by: Leo Yan <leo.yan@linaro.org> > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > > --- > > drivers/hwtracing/coresight/coresight-etm4x.c | 11 +++++++++++ > > drivers/hwtracing/coresight/coresight-etm4x.h | 3 +++ > > 2 files changed, 14 insertions(+) > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c > > index 8b0634ebef77..d0f800753590 100644 > > --- a/drivers/hwtracing/coresight/coresight-etm4x.c > > +++ b/drivers/hwtracing/coresight/coresight-etm4x.c > > @@ -52,6 +52,7 @@ static struct etmv4_drvdata *etmdrvdata[NR_CPUS]; > > static void etm4_set_default_config(struct etmv4_config *config); > > static int etm4_set_event_filters(struct etmv4_drvdata *drvdata, > > struct perf_event *event); > > +static u64 etm4_get_access_type(struct etmv4_config *config); > > > > static enum cpuhp_state hp_online; > > > > @@ -785,6 +786,8 @@ static void etm4_init_arch_data(void *info) > > > > static void etm4_set_default_config(struct etmv4_config *config) > > { > > + u64 access_type; > > + > > /* disable all events tracing */ > > config->eventctrl0 = 0x0; > > config->eventctrl1 = 0x0; > > @@ -800,6 +803,14 @@ static void etm4_set_default_config(struct etmv4_config *config) > > > > /* TRCVICTLR::EVENT = 0x01, select the always on logic */ > > config->vinst_ctrl = BIT(0); > > + > > + /* > > + * TRCVICTLR::EXLEVEL_NS:EXLEVELS: Set kernel / user filtering > > + * bits in vinst, same bit pattern as address comparator acc values > > + * but offset in this register > > + */ > > + access_type = etm4_get_access_type(config) << ETM_EXLEVEL_VINST_OFFSET; > > + config->vinst_ctrl |= (u32)access_type; > > Just a minor comment, here the definition for ETM_EXLEVEL_VINST_OFFSET > is easily to introduce confusion. In the register TRCACATRn, > EXLEVEL_NS:EXLEVELS offset is 8, and in the registser TRCVICTLR the > EXLEVEL_NS:EXLEVELS offset is 16. So for more explict expression, we > need to define below two macros: > > #define ETM_EXLEVEL_TRCVICTLR_OFFSET 16 > #define ETM_EXLEVEL_TRCACATRn_OFFSET 8 > > access_type = etm4_get_access_type(config) >> ETM_EXLEVEL_TRCACATRn_OFFSET; > access_type = access_type << ETM_EXLEVEL_VINST_OFFSET; > config->vinst_ctrl |= (u32)access_type; > > Otherwise, the change is good for me: > > Reviewed-by: Leo Yan <leo.yan@linaro.org> > Tested-by: Leo Yan <leo.yan@linaro.org> > Thanks for the review and test. I am not sure the above makes things clearer. Looking at it, I think a better comment and more consistent use of the TRCVICTLR name would make things better. I 've spotted a second instance where this needs to be set, so the next spin will have a helper fn updating config->vinst_ctrl. Regards Mike > And thanks a lot for the quick fixing! > > Leo > > > static u64 etm4_get_ns_access_type(struct etmv4_config *config) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h > > index b8283e1d6d88..369abc25c597 100644 > > --- a/drivers/hwtracing/coresight/coresight-etm4x.h > > +++ b/drivers/hwtracing/coresight/coresight-etm4x.h > > @@ -192,6 +192,9 @@ > > #define ETM_EXLEVEL_NS_HYP BIT(14) > > #define ETM_EXLEVEL_NS_NA BIT(15) > > > > +/* access level control in VINST - same bits as TRCACATRn but offset */ > > +#define ETM_EXLEVEL_VINST_OFFSET 8 > > > > + > > /* secure / non secure masks - TRCVICTLR, IDR3 */ > > #define ETM_EXLEVEL_S_VICTLR_MASK GENMASK(19, 16) > > /* NS MON (EL3) mode never implemented */ > > -- > > 2.17.1 > >
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index 8b0634ebef77..d0f800753590 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -52,6 +52,7 @@ static struct etmv4_drvdata *etmdrvdata[NR_CPUS]; static void etm4_set_default_config(struct etmv4_config *config); static int etm4_set_event_filters(struct etmv4_drvdata *drvdata, struct perf_event *event); +static u64 etm4_get_access_type(struct etmv4_config *config); static enum cpuhp_state hp_online; @@ -785,6 +786,8 @@ static void etm4_init_arch_data(void *info) static void etm4_set_default_config(struct etmv4_config *config) { + u64 access_type; + /* disable all events tracing */ config->eventctrl0 = 0x0; config->eventctrl1 = 0x0; @@ -800,6 +803,14 @@ static void etm4_set_default_config(struct etmv4_config *config) /* TRCVICTLR::EVENT = 0x01, select the always on logic */ config->vinst_ctrl = BIT(0); + + /* + * TRCVICTLR::EXLEVEL_NS:EXLEVELS: Set kernel / user filtering + * bits in vinst, same bit pattern as address comparator acc values + * but offset in this register + */ + access_type = etm4_get_access_type(config) << ETM_EXLEVEL_VINST_OFFSET; + config->vinst_ctrl |= (u32)access_type; } static u64 etm4_get_ns_access_type(struct etmv4_config *config) diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index b8283e1d6d88..369abc25c597 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -192,6 +192,9 @@ #define ETM_EXLEVEL_NS_HYP BIT(14) #define ETM_EXLEVEL_NS_NA BIT(15) +/* access level control in VINST - same bits as TRCACATRn but offset */ +#define ETM_EXLEVEL_VINST_OFFSET 8 + /* secure / non secure masks - TRCVICTLR, IDR3 */ #define ETM_EXLEVEL_S_VICTLR_MASK GENMASK(19, 16) /* NS MON (EL3) mode never implemented */
Moving from using an address filter to trace the default "all addresses" range to no filtering to acheive the same result, has caused the perf filtering of kernel/user address spaces from not working unless an explicit address filter was used. This due to the global TRCVICTLR exception level filtering not being set correctly. This patch fixes this. Reported-by: Leo Yan <leo.yan@linaro.org> Signed-off-by: Mike Leach <mike.leach@linaro.org> --- drivers/hwtracing/coresight/coresight-etm4x.c | 11 +++++++++++ drivers/hwtracing/coresight/coresight-etm4x.h | 3 +++ 2 files changed, 14 insertions(+)