[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 at 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 at linaro.org>
> > Signed-off-by: Mike Leach <mike.leach at 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 at linaro.org>
> Tested-by: Leo Yan <leo.yan at 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