[PATCH] coresight: etm4x: Ensure default perf settings filter user/kernel

[PATCH] coresight: etm4x: Ensure default perf settings filter user/kernel

[Mike Leach]

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(+)

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 */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] coresight: etm4x: Ensure default perf settings filter user/kernel

[Leo Yan]

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] coresight: etm4x: Ensure default perf settings filter user/kernel

[Mike Leach]

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
> >



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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel