Re: [PATCH v3 18/26] coresight: etm4x: Clean up exception level masks

[Suzuki K Poulose]

On 11/6/20 6:52 PM, Mathieu Poirier wrote:
> Good morning,
>
> On Wed, Oct 28, 2020 at 10:09:37PM +0000, Suzuki K Poulose wrote:
> > etm4_get_access_type() calculates the exception level bits
> > for use in address comparator registers. This is also used
> > by the TRCVICTLR register by shifting to the required position.
> >
> > This patch cleans up the logic to make etm4_get_access_type()
> > calcualte a generic mask which can be used by all users by
> > shifting to their field.
> >
> > No functional changes, only code cleanups.
> >
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > ---
> > Changes since previous version:
> >    - Fix the duplicate shift. More commentary
> > ---


> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> > index 2ac4ecb0af61..f1251ddf1984 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> > @@ -548,24 +548,38 @@
> >   
> >   #define TRCACATR_EXLEVEL_SHIFT		8
> >   
> > -/* secure state access levels - TRCACATRn */
> > -#define ETM_EXLEVEL_S_APP		BIT(8)
> > -#define ETM_EXLEVEL_S_OS		BIT(9)
> > -#define ETM_EXLEVEL_S_HYP		BIT(10)
> > -#define ETM_EXLEVEL_S_MON		BIT(11)
> > -/* non-secure state access levels - TRCACATRn */
> > -#define ETM_EXLEVEL_NS_APP		BIT(12)
> > -#define ETM_EXLEVEL_NS_OS		BIT(13)
> > -#define ETM_EXLEVEL_NS_HYP		BIT(14)
> > -#define ETM_EXLEVEL_NS_NA		BIT(15)
> > -
> > -/* access level control in TRCVICTLR - same bits as TRCACATRn but shifted */
> > -#define ETM_EXLEVEL_LSHIFT_TRCVICTLR	8
> > +/*
> > + * Exception level mask for Secure and Non-Secure ELs.
> > + * ETM defines the bits for EL control (e.g, TRVICTLR, TRCACTRn).
> > + * The Secure and Non-Secure ELs are always to gether.
> > + * Non-secure EL3 is never implemented.
> > + * We use the following generic mask as they appear in different
> > + * registers and this can be shifted for the appropriate
> > + * fields.
> > + */
> > +#define ETM_EXLEVEL_S_APP		BIT(0)	/* Secure EL0		*/
> > +#define ETM_EXLEVEL_S_OS		BIT(1)	/* Secure EL1		*/
> > +#define ETM_EXLEVEL_S_HYP		BIT(2)	/* Secure EL2		*/
> > +#define ETM_EXLEVEL_S_MON		BIT(3)	/* Secure EL3/Montor	*/
>
> s/Montor/Monitor
>
> > +#define ETM_EXLEVEL_NS_APP		BIT(4)	/* NonSecure EL0	*/
> > +#define ETM_EXLEVEL_NS_OS		BIT(5)	/* NonSecure EL1	*/
> > +#define ETM_EXLEVEL_NS_HYP		BIT(6)	/* NonSecure EL2	*/
> > +
> > +#define ETM_EXLEVEL_MASK		(GENMASK(6, 0))
>
> Not used.

I have updated the patch to use this for creating the TRCVICTLR_EXLEVEL_MASK
(see below), which is used to clear all the EXLEVEL bits from TRCVICTLR.

> > +#define ETM_EXLEVEL_S_MASK		(GENMASK(3, 0))
> > +#define ETM_EXLEVEL_NS_MASK		(GENMASK(6, 4))
>
> This needs to be GENMASK(2, 0) in order TRCVICTLR_EXLEVEL_NS_SHIFT to be 20.
> Otherwise the resulting mask is 4 bit off to the left.

I have retained the ETM_EXLEVEL_NS_MASK as it is above, to keep the defintions
above consistent. I have fixed the problem by using TRCVICTLR_EXLEVEL_SHIFT whenever
we use ETM_EXLEVEL_*_MASK.

And TRCVICTLR_EXLEVEL_*_SHIFT is used when we set a given value from the sysfs, respectively.
e.g :

@@ -795,10 +795,10 @@ static ssize_t ns_exlevel_vinst_store(struct device *dev,

        spin_lock(&drvdata->spinlock);
        /* clear EXLEVEL_NS bits  */
-       config->vinst_ctrl &= ~(ETM_EXLEVEL_NS_VICTLR_MASK);
+       config->vinst_ctrl &= ~(TRCVICTLR_EXLEVEL_NS_MASK);
        /* enable instruction tracing for corresponding exception level */
        val &= drvdata->ns_ex_level;
-       config->vinst_ctrl |= (val << 20);
+       config->vinst_ctrl |= (val << TRCVICTLR_EXLEVEL_NS_SHIFT);
        spin_unlock(&drvdata->spinlock);
        return size;
 }

> > +
> > +/* access level controls in TRCACATRn */
> > +#define TRCACATR_EXLEVEL_SHIFT		8
> > +
> > +/* access level control in TRCVICTLR */
> > +#define TRCVICTLR_EXLEVEL_SHIFT		16
> > +#define TRCVICTLR_EXLEVEL_S_SHIFT	16
> > +#define TRCVICTLR_EXLEVEL_NS_SHIFT	20
> >   
> >   /* secure / non secure masks - TRCVICTLR, IDR3 */
> > -#define ETM_EXLEVEL_S_VICTLR_MASK	GENMASK(19, 16)
> > -/* NS MON (EL3) mode never implemented */
> > -#define ETM_EXLEVEL_NS_VICTLR_MASK	GENMASK(22, 20)
> > +#define TRCVICTLR_EXLEVEL_S_MASK	(ETM_EXLEVEL_S_MASK << TRCVICTLR_EXLEVEL_S_SHIFT)
> > +#define TRCVICTLR_EXLEVEL_NS_MASK	(ETM_EXLEVEL_NS_MASK << TRCVICTLR_EXLEVEL_NS_SHIFT)

And the above has been updated to :

+#define TRCVICTLR_EXLEVEL_MASK         (ETM_EXLEVEL_MASK << TRCVICTLR_EXLEVEL_SHIFT)
+#define TRCVICTLR_EXLEVEL_S_MASK       (ETM_EXLEVEL_S_MASK << TRCVICTLR_EXLEVEL_SHIFT)
+#define TRCVICTLR_EXLEVEL_NS_MASK      (ETM_EXLEVEL_NS_MASK << TRCVICTLR_EXLEVEL_SHIFT)

Suzuki