Re: [PATCH] arm64: add support for Self-hosted trace

[Shaokun Zhang <zhangshaokun@hisilicon.com>]

Hi Suzuki,

在 2020/7/2 8:06, Suzuki K Poulose 写道:
> On 07/01/2020 10:18 AM, Shaokun Zhang wrote:
>> Hi Suzuki,
>>
>> 在 2020/6/30 21:28, Suzuki K Poulose 写道:
>>> Hi
>>>
>>> On 06/30/2020 01:32 PM, Shaokun Zhang 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: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>>>> Signed-off-by: Jonathan Zhou <jonathan.zhouwen@huawei.com>
>>>> ---
>>>>    arch/arm64/Kconfig               | 11 +++++++++++
>>>>    arch/arm64/include/asm/cpucaps.h |  3 ++-
>>>>    arch/arm64/include/asm/sysreg.h  |  6 ++++++
>>>>    arch/arm64/kernel/cpufeature.c   | 25 ++++++++++++++++++++++++-
>>>>    4 files changed, 43 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>>> index a4a094bedcb2..328188f7962c 100644
>>>> --- a/arch/arm64/Kconfig
>>>> +++ b/arch/arm64/Kconfig
>>>> @@ -1596,6 +1596,17 @@ config ARM64_AMU_EXTN
>>>>          correctly reflect reality. Most commonly, the value read will
>>>> be 0,
>>>>          indicating that the counter is not enabled.
>>>>    +config ARM64_SELF_HOSTED_TRACE
>>>> +    bool "Enable support for the Self-hosted Trace Extension"
>>>> +    default y
>>>> +    help
>>>> +      The Self-hosted Trace is introduced by ARMv8.4 CPU architecture.
>>>> +          It adds controls of trace in a self-hosted system through
>>>> system
>>>> +      registers.
>>>> +
>>>> +      Selecting this option will control trace filter and allow trace
>>>> +      at EL1 and EL0.
>>>> +
>>>>    endmenu
>>>>      menu "ARMv8.5 architectural features"
>>>> diff --git a/arch/arm64/include/asm/cpucaps.h
>>>> b/arch/arm64/include/asm/cpucaps.h
>>>> index d7b3bb0cb180..99ffd7b5117a 100644
>>>> --- a/arch/arm64/include/asm/cpucaps.h
>>>> +++ b/arch/arm64/include/asm/cpucaps.h
>>>> @@ -62,7 +62,8 @@
>>>>    #define ARM64_HAS_GENERIC_AUTH            52
>>>>    #define ARM64_HAS_32BIT_EL1            53
>>>>    #define ARM64_BTI                54
>>>> +#define ARM64_HAS_SELF_HOSTED_TRACE        55
>>>>    -#define ARM64_NCAPS                55
>>>> +#define ARM64_NCAPS                56
>>>>      #endif /* __ASM_CPUCAPS_H */
>>>> diff --git a/arch/arm64/include/asm/sysreg.h
>>>> b/arch/arm64/include/asm/sysreg.h
>>>> index 463175f80341..137e26bd3cc4 100644
>>>> --- a/arch/arm64/include/asm/sysreg.h
>>>> +++ b/arch/arm64/include/asm/sysreg.h
>>>> @@ -183,6 +183,7 @@
>>>>    #define SYS_CPACR_EL1            sys_reg(3, 0, 1, 0, 2)
>>>>      #define SYS_ZCR_EL1            sys_reg(3, 0, 1, 2, 0)
>>>> +#define SYS_TRFCR_EL1            sys_reg(3, 0, 1, 2, 1)
>>>>      #define SYS_TTBR0_EL1            sys_reg(3, 0, 2, 0, 0)
>>>>    #define SYS_TTBR1_EL1            sys_reg(3, 0, 2, 0, 1)
>>>> @@ -755,6 +756,8 @@
>>>>    #define ID_AA64MMFR2_CNP_SHIFT        0
>>>>      /* id_aa64dfr0 */
>>>> +#define ID_AA64DFR0_SELF_HOSTED_SHIFT    40
>>>> +#define ID_AA64DFR0_DOUBLELOCK_SHIFT    36
>>>>    #define ID_AA64DFR0_PMSVER_SHIFT    32
>>>>    #define ID_AA64DFR0_CTX_CMPS_SHIFT    28
>>>>    #define ID_AA64DFR0_WRPS_SHIFT        20
>>>> @@ -875,6 +878,9 @@
>>>>    #define CPACR_EL1_ZEN_EL0EN    (BIT(17)) /* enable EL0 access, if
>>>> EL1EN set */
>>>>    #define CPACR_EL1_ZEN        (CPACR_EL1_ZEN_EL1EN |
>>>> CPACR_EL1_ZEN_EL0EN)
>>>>    +#define TRFCR_EL1_E0TRE        (BIT(0)) /* Trace is allowed at
>>>> EL0 */
>>>> +#define TRFCR_EL1_E1TRE        (BIT(1)) /* Trace is allowed at EL1 */
>>>> +#define TRFCR_EL1_TRE        (TRFCR_EL1_E0TRE | TRFCR_EL1_E1TRE)
>>>>      /* Safe value for MPIDR_EL1: Bit31:RES1, Bit30:U:0, Bit24:MT:0 */
>>>>    #define SYS_MPIDR_SAFE_VAL    (BIT(31))
>>>> diff --git a/arch/arm64/kernel/cpufeature.c
>>>> b/arch/arm64/kernel/cpufeature.c
>>>> index 4ae41670c2e6..6008f06ce2c4 100644
>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>> @@ -368,7 +368,8 @@ static const struct arm64_ftr_bits
>>>> ftr_id_mmfr0[] = {
>>>>    };
>>>>      static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>>>> -    S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 36, 4,
>>>> 0),
>>>> +    S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 44, 4,
>>>> 0),
>>>> +    ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE,
>>>> ID_AA64DFR0_SELF_HOSTED_SHIFT, 4, 0),
>>>>        ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE,
>>>> ID_AA64DFR0_PMSVER_SHIFT, 4, 0),
>>>>        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE,
>>>> ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
>>>>        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE,
>>>> ID_AA64DFR0_WRPS_SHIFT, 4, 0),
>>>> @@ -1655,6 +1656,15 @@ static void bti_enable(const struct
>>>> arm64_cpu_capabilities *__unused)
>>>>    }
>>>>    #endif /* CONFIG_ARM64_BTI */
>>>>    +#ifdef CONFIG_ARM64_SELF_HOSTED_TRACE
>>>> +static void self_hosted_trace_enable(const struct
>>>> arm64_cpu_capabilities *__unused)
>>>> +{
>>>> +    /* Enable E0TRE and E1TRE in TRFCR_EL1 */
>>>> +    write_sysreg_s(read_sysreg_s(SYS_TRFCR_EL1) | TRFCR_EL1_TRE,
>>>> SYS_TRFCR_EL1);
>>>> +    isb();
>>>> +}
>>>
>>>
>>> Is this really sufficient ? If we are running with VHE, don't we need
>>> to set the TRFCR_EL2.E0HTRE ? Similarly we need to set E2TRE for
>>
>> Correct, I missed it.
> 
> Please ignore the comment above. Looking at the psuedo code for
> TRFCR_EL1 access, with VHE the access is routed to TRFCR_EL2. So
> what you have is fine. But, please see below.
> 
>>
>>> enabling kernel tracing.
>>>
>>>> +#endif /* CONFIG_ARM64_SELF_HOSTED_TRACE */
>>>> +
>>>>    /* Internal helper functions to match cpu capability type */
>>>>    static bool
>>>>    cpucap_late_cpu_optional(const struct arm64_cpu_capabilities *cap)
>>>> @@ -2054,6 +2064,19 @@ static const struct arm64_cpu_capabilities
>>>> arm64_features[] = {
>>>>            .sign = FTR_UNSIGNED,
>>>>        },
>>>>    #endif
>>>> +#ifdef CONFIG_ARM64_SELF_HOSTED_TRACE
>>>> +    {
>>>> +        .desc = "Self-hosted Trace",
>>>> +        .capability = ARM64_HAS_SELF_HOSTED_TRACE,
>>>> +        .type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>>
>>> Also, if we are running on a system with a mix of CPUs, we would never
>>> set the TRFCRx bits, which implies the ETMs won't be able to provide
>>
>> Sorry, Maybe I don't follow it correctly, what do you mean about "mix of
>> CPUs", do you mean some CPU support ETM and other may don't support ETM?
>>
> 
> I meant, if you have a mix of CPUs with and without the TraceFiltering
> support, the CPUs with TraceFiltering support will not be able to trace
> unless they enable the tracing explicitly in the TRFCR_ELx.
> 
>>> useful trace information.
>>>
>>> And finally, do we need a special config for this ? This can be a
>>
>> Do you mean that firmware can do it if the user wants to enable trace?
> 
> No. We should do this always, for a CPU capable of Trace filtering, by
> checking the ID register. The ETM tracing is an existing functionality
> used by the cores. So having a new config and hiding the control behind
> it is not going to help the existing users. Also, given that the actual
> trace functionality is controlled CORESIGHT configs, we should either
> use that or leave the CoreSight drivers to do this setup.
> 
> My preference is to leave this to the CoreSight drivers to select the

I appreciate your comments and I check the coresight driver quickly.
Hopefully I shall not miss some important things. I'm not sure whether
it is appropriate to check CPU ID register in coresight driver?

Thanks,
Shaokun

> appropriate filtering based on the "individual" trace sessions.
> 
> Suzuki
> 
> .


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