[PATCH v3] Coresight: etm4x: Add support for Self-hosted trace

[Jonathan Zhou]

Hi, Suzuki.
Thanks for your review.

On 03/11/2020 20:44, Suzuki K Poulose wrote:
> Jonathan
> 
> On 9/18/20 3:30 AM, Jonathan Zhou wrote:
>> ARMv8.4 architecture extension introduces ARMv8.4-Trace, Armv8.4
>> Self-hosted Trace Extensions. It provides control of exception
>> levels and security states. Let's add this feature detection and
>> enable E1TRE and E0TRE in TRFCR_EL1 if Self-hosted Trace is
>> supported.
>>
>> Cc: Mathieu Poirier <mathieu.poirier at linaro.org>
>> Cc: Catalin Marinas <catalin.marinas at arm.com>
>> Cc: Will Deacon <will at kernel.org>
>> Cc: Suzuki K Poulose <suzuki.poulose at arm.com>
>> Cc: Shaokun Zhang <zhangshaokun at hisilicon.com>
>> Signed-off-by: Jonathan Zhou <jonathan.zhouwen at huawei.com>
>> ---
>> ChangeLog in v3:
>>      * address Suzuki's comments
>>
>>   arch/arm64/include/asm/sysreg.h               | 17 +++++++++++++
>>   drivers/hwtracing/coresight/coresight-etm4x.c | 35
>> +++++++++++++++++++++++++++
>>   2 files changed, 52 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/sysreg.h
>> b/arch/arm64/include/asm/sysreg.h
>> index 554a7e8ecb07..31b84a5fa35d 100644
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -184,6 +184,22 @@
>>     #define SYS_ZCR_EL1            sys_reg(3, 0, 1, 2, 0)
>>   +/* Trace Filter control EL1*/
>> +#define SYS_TRFCR_EL1            sys_reg(3, 0, 1, 2, 1)
>> +/* Trace Filter control EL2 */
>> +#define SYS_TRFCR_EL2            sys_reg(3, 4, 1, 2, 1)
> 
> Please keep the ordering of the definitions.
OK.
> 
>> +
>> +/* Trace is allowed at EL0 */
>> +#define TRFCR_EL1_E0TRE            BIT(0)
> 
> TRFCR_ELx_E0TRE ?
> 
Yes.

>> +/* Trace is allowed at EL1 */
>> +#define TRFCR_EL1_E1TRE            BIT(1)
> 
> May be TRFCR_ELx_ELxTRE  ?
> 
Yes, it's better.
>> +#define TRFCR_TS_SHIFT            5
>> +#define TRFCR_TS_MASK            GENMASK(6, 5)
>> +/* Virtual timestamp */
>> +#define TRFCR_TS_VT            (0x1UL << TRFCR_TS_SHIFT)
> 
>> +/* Context idx is allowed at EL2 */
>> +#define TRFCR_EL2_CX            BIT(3)
>> +
>>   #define SYS_TTBR0_EL1            sys_reg(3, 0, 2, 0, 0)
>>   #define SYS_TTBR1_EL1            sys_reg(3, 0, 2, 0, 1)
>>   #define SYS_TCR_EL1            sys_reg(3, 0, 2, 0, 2)
>> @@ -772,6 +788,7 @@
>>   #define ID_AA64MMFR2_CNP_SHIFT        0
>>     /* id_aa64dfr0 */
>> +#define ID_AA64DFR0_SELF_HOSTED_SHIFT    40
> 
> Please stick to the architcted name for the field, TRACE_FILT
> 
OK, thanks.

>>   #define ID_AA64DFR0_DOUBLELOCK_SHIFT    36
>>   #define ID_AA64DFR0_PMSVER_SHIFT    32
>>   #define ID_AA64DFR0_CTX_CMPS_SHIFT    28
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c
>> b/drivers/hwtracing/coresight/coresight-etm4x.c
>> index 96425e818fc2..82648a3c7a8e 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
>> @@ -28,6 +28,7 @@
>>   #include <linux/perf_event.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/property.h>
>> +#include <asm/sysreg.h>
>>   #include <asm/sections.h>
>>   #include <asm/local.h>
>>   #include <asm/virt.h>
>> @@ -785,6 +786,36 @@ static void etm4_init_arch_data(void *info)
>>       CS_LOCK(drvdata->base);
>>   }
>>   +static void etm4_init_sysctrl(void *info)
>> +{
>> +    u64 sys_trfcr_el1, dfr0;
>> +    int trace_filt;
>> +
>> +    dfr0 = read_sysreg(id_aa64dfr0_el1);
>> +
>> +    trace_filt = cpuid_feature_extract_unsigned_field(dfr0,
>> +                    ID_AA64DFR0_SELF_HOSTED_SHIFT);
>> +    if (trace_filt == 0x1) {
>> +        sys_trfcr_el1 = read_sysreg_s(SYS_TRFCR_EL1);
>> +        sys_trfcr_el1 &= (~TRFCR_TS_MASK);
>> +        /* Enable EL1 & EL0 tracing. */
>> +        sys_trfcr_el1 |= TRFCR_EL1_E0TRE;
>> +        sys_trfcr_el1 |= TRFCR_EL1_E1TRE;
>> +        /* Use virtual timestamp */
>> +        sys_trfcr_el1 |= TRFCR_TS_VT;
> 
> 
> Could we simply do :
> 
>         u64 trcfr = (TRFCR_ELx_E0TRE |
>                  TRFCR_ELx_ExTRE |
>                  TRFCR_ELx_TS_VT)
>  
Got it.
   
>> +        /* enable contextid_el2 and vmid tracing. */
>> +        if (is_kernel_in_hyp_mode())
>> +            sys_trfcr_el1 |= TRFCR_EL2_CX;
>> +        /*
>> +         * If pe is in EL2, we assume HCR_EL2.E2H == 1. Thus, accessing
>> +         * TRFCR_EL1 is routed to TRFCR_EL2 and leave TRFCR_EL1 to be
>> +         * configured by the kvm.
>> +         */
>> +        write_sysreg_s(sys_trfcr_el1, SYS_TRFCR_EL1);
>> +        isb();
> 
> 
> 
>> +    }
>> +}
> 
> 
>> +
>>   static void etm4_set_default_config(struct etmv4_config *config)
>>   {
>>       /* disable all events tracing */
>> @@ -1504,6 +1535,10 @@ static int etm4_probe(struct amba_device *adev,
>> const struct amba_id *id)
>>                   etm4_init_arch_data,  drvdata, 1))
>>           dev_err(dev, "ETM arch init failed\n");
>>   +    if (smp_call_function_single(drvdata->cpu,
>> +                etm4_init_sysctrl, drvdata, 1))
>> +        dev_err(dev, "ETM sysctrl init failed\n");
> 
> We could as well move this to etm4_init_arch_data(), which is called
> already on the CPU.
> 
OK.
> Suzuki
> 
> .
Jonathan.