Re: [PATCH 12/19] coresight: etm4x: Cleanup secure exception level masks

From: Suzuki K Poulose <suzuki.poulose@arm.com>
To: mike.leach@linaro.org
Cc: coresight@lists.linaro.org, anshuman.khandual@arm.com,
	mathieu.poirier@linaro.org, linux-arm-kernel@lists.infradead.org,
	leo.yan@linaro.org
Subject: Re: [PATCH 12/19] coresight: etm4x: Cleanup secure exception level masks
Date: Wed, 30 Sep 2020 11:32:25 +0100	[thread overview]
Message-ID: <72dd83d2-462a-c68b-290f-533d17a1e8fa@arm.com> (raw)
In-Reply-To: <CAJ9a7VjoB9L8HrKhffaAmn+SUY6p=AYU8ZEy+n8h1q04f+abEA@mail.gmail.com>

Hi Mike,

On 09/22/2020 01:47 PM, Mike Leach wrote:
> Hi Suzuki,
>
>>>> @@ -935,16 +935,9 @@ static u64 etm4_get_ns_access_type(struct etmv4_config *config)
>>>>    static u64 etm4_get_access_type(struct etmv4_config *config)
>>>>    {
>>>>           u64 access_type = etm4_get_ns_access_type(config);
>>>> -       u64 s_hyp = (config->arch & 0x0f) >= 0x4 ? ETM_EXLEVEL_S_HYP : 0;
>>>>
>>>> -       /*
>>>> -        * EXLEVEL_S, bits[11:8], don't trace anything happening
>>>> -        * in secure state.
>>>> -        */
>>>> -       access_type |= (ETM_EXLEVEL_S_APP       |
>>>> -                       ETM_EXLEVEL_S_OS        |
>>>> -                       s_hyp                   |
>>>> -                       ETM_EXLEVEL_S_MON);
>>>> +       /* All supported secure ELs are excluded */
>>>> +       access_type |= (u64)config->s_ex_level << TRCACATR_EXLEVEL_SHIFT;
>>>>
>>>
>>> What is the << TRCACATR_EXLEVEL_SHIFT doing here?
>>
>> The config->s_ex_level is the EXLVEL mask from the TRCIDR3 shifted to bit 0, as above.
>> We need to make sure that we use the mask in the correct position for TRCACATR register.
>> Basically, we simply exclude all the secure levels supported by the ETM.
>>
> 
> Sorry, should have been a little more explicit. This breaks the next patch!
> 
>>>
>>>>           return access_type;
>>>>    }
>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
>>>> index efd903688edd..407ad6647f36 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
>>>> @@ -522,6 +522,8 @@
>>>>    /* PowerDown Control Register bits */
>>>>    #define TRCPDCR_PU                     BIT(3)
>>>>
>>>> +#define TRCACATR_EXLEVEL_SHIFT         8
>>>> +
>>>>    /* secure state access levels - TRCACATRn */
>>>>    #define ETM_EXLEVEL_S_APP              BIT(8)
>>>>    #define ETM_EXLEVEL_S_OS               BIT(9)
>>>> @@ -604,7 +606,7 @@
>>>>     * @vmid_mask0:        VM ID comparator mask for comparator 0-3.
>>>>     * @vmid_mask1:        VM ID comparator mask for comparator 4-7.
>>>>     * @ext_inp:   External input selection.
>>>> - * @arch:      ETM architecture version (for arch dependent config).
>>>> + * @s_ex_level: Secure ELs where tracing is supported.
>>>>     */
>>>>    struct etmv4_config {
>>>>           u32                             mode;
>>>> @@ -648,7 +650,7 @@ struct etmv4_config {
>>>>           u32                             vmid_mask0;
>>>>           u32                             vmid_mask1;
>>>>           u32                             ext_inp;
>>>> -       u8                              arch;
>>>> +       u8                              s_ex_level;
>>>>    };
>>>>
>>>>    /**
>>>> --
>>>> 2.24.1
>>>>
>>>
>>> Perhaps this patch could be combined with the next patch as it
>>> operates on the same set of flags.
>>>
>>
>> I agree that they both deal with the same set of masks. However, functionally
>> they have separate purposes.
>>
>> 1) This patch disconnects the usage of drvdata->arch field to determine
>> the secure exception level mask. This is more of a correctness, as a given
>> v4.4 implementation may not have a Secure EL2.
>>
>> 2) The next patch cleans up the way we define and use all the exception level
>> masks, both secure and non-secure.
>>
> 
> 
> 
> Applying this and the next patch which moves to the bits being indexed
> from 0 for a common reusable field gets you the following code
> sequence:-

Good catch.

> 
> static u64 etm4_get_ns_access_type(struct etmv4_config *config)
> {
>      u64 access_type = 0;
> 
>      /*
>       * EXLEVEL_NS, bits[15:12]
>       * The Exception levels are:
>       *   Bit[12] Exception level 0 - Application
>       *   Bit[13] Exception level 1 - OS
>       *   Bit[14] Exception level 2 - Hypervisor
>       *   Bit[15] Never implemented
>       */
> 

Updated the comments.

> 
> [ MJL: at this point the comment is true to an extent but no longer
> applies to the values #defines below - which have been shifted by the
> change to the #defines to form a field that is indexed from bit 0]
> 

> 
> static u64 etm4_get_access_type(struct etmv4_config *config)
> {
>      u64 access_type = etm4_get_ns_access_type(config);
> 
>      /* All supported secure ELs are excluded */
>      access_type |= (u64)config->s_ex_level << TRCACATR_EXLEVEL_SHIFT;
> 
> [MJL: Now we are OR ing a 0 bit index based field (NS access type)
> with another 0 index based field - but shifting it for some reason?]

That was a mistake. I will drop this shift from here to make get_access_type() and
this should work.

Suzuki

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