From: Suzuki K Poulose <suzuki.poulose@arm.com> To: Mike Leach <mike.leach@linaro.org> Cc: linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, Coresight ML <coresight@lists.linaro.org>, Mathieu Poirier <mathieu.poirier@linaro.org>, Anshuman Khandual <anshuman.khandual@arm.com>, Leo Yan <leo.yan@linaro.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Jonathan Zhou <jonathan.zhouwen@huawei.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org> Subject: Re: [PATCH v7 28/28] coresight: Add support for v8.4 SelfHosted tracing Date: Fri, 12 Feb 2021 15:36:33 +0000 [thread overview] Message-ID: <72f85de4-5b39-c963-78cf-2f8347e21268@arm.com> (raw) In-Reply-To: <CAJ9a7ViwNA__+5f7-DehM6OtGer3DjdZfWsgHc9EOcr5-vQXgA@mail.gmail.com> Hi Mike On 2/12/21 10:34 AM, Mike Leach wrote: > Hi Mathieu, Suzuki, > > Sorry for the really late response on this patch, but I noticed a > problem while doing a review of the ETE / TRBE set. (TRBE specs > mention TRFCR_ELx, so I was confirming a couple of things). > > On Sun, 10 Jan 2021 at 22:49, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: >> >> From: Jonathan Zhou <jonathan.zhouwen@huawei.com> >> >> v8.4 tracing extensions added support for trace filtering controlled >> by TRFCR_ELx. This must be programmed to allow tracing at EL1/EL2 and >> EL0. The timestamp used is the virtual time. Also enable CONTEXIDR_EL2 >> tracing if we are running the kernel at EL2. >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Mike Leach <mike.leach@linaro.org> >> Cc: Will Deacon <will@kernel.org> >> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> >> Signed-off-by: Jonathan Zhou <jonathan.zhouwen@huawei.com> >> [ Move the trace filtering setup etm_init_arch_data() and >> clean ups] >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> .../coresight/coresight-etm4x-core.c | 25 +++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> index 3d3165dd09d4..18c1a80abab8 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> @@ -859,6 +859,30 @@ static bool etm4_init_csdev_access(struct etmv4_drvdata *drvdata, >> return false; >> } >> >> +static void cpu_enable_tracing(void) >> +{ >> + u64 dfr0 = read_sysreg(id_aa64dfr0_el1); >> + u64 trfcr; >> + >> + if (!cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_TRACE_FILT_SHIFT)) >> + return; >> + >> + /* >> + * If the CPU supports v8.4 SelfHosted Tracing, enable >> + * tracing at the kernel EL and EL0, forcing to use the >> + * virtual time as the timestamp. >> + */ >> + trfcr = (TRFCR_ELx_TS_VIRTUAL | >> + TRFCR_ELx_ExTRE | >> + TRFCR_ELx_E0TRE); >> + >> + /* If we are running at EL2, allow tracing the CONTEXTIDR_EL2. */ >> + if (is_kernel_in_hyp_mode()) >> + trfcr |= TRFCR_EL2_CX; >> + > > This is wrong - CX bit is present on TRFCR_EL2, not TRFCR_EL1. Why is this wrong ? We do this only when we are in EL2. > Moreover, TRFCR_EL2 has a separate enables for tracing at EL0 and EL2. > True, that is for EL0&2 translation regimes. i.e, tracing EL0 with the kernel running at EL2. But bits TRFCR_EL2.E2TRE == TRFCR_EL1.E1TRE If notice, we name the bit TRFCR_ELx_ExTRE. And E0TRE == E0HTRE. So we do the following : 1) When kernel running at EL2: Enable tracing at EL2 and EL0 and context tracking 2) When kernel running at EL1: Enable tracing at EL1 and EL0. > Secondly - is this correct in principal? Should the driver not be > reading the access it is permitted by the kernel, rather than giving > itself unfettered access to trace where it wants to. I dont follow the "access permitted by the kernel" here. What are we referrring to ? > Surely TRFCR_ELx levels should be chosen in KConfig and then should > be set up in kernel initialisation? I disagree with yet another Kconfig. This basic requirement for enabling the trace collection. It is not something that we can optionally use from the architecture. So we should transparently do the right thing for making sure that we set up the system for something that didn't require any other steps. Or in other words, if we add a Kconfig option for TRFCR programming, if someone forgets to select it when they upgraded the kernel they are in for a surprisingly long debugging to find why the trace doesnt work. As for the TRFCR programming, we have two choices. etm4x driver or generic boot up for the CPU. I preferred to do this in the driver as we can enable it only if trace drivers are available. Cheers Suzuki > > Regards > > Mike > > > >> + write_sysreg_s(trfcr, SYS_TRFCR_EL1); >> +} >> + >> static void etm4_init_arch_data(void *info) >> { >> u32 etmidr0; >> @@ -1044,6 +1068,7 @@ static void etm4_init_arch_data(void *info) >> /* NUMCNTR, bits[30:28] number of counters available for tracing */ >> drvdata->nr_cntr = BMVAL(etmidr5, 28, 30); >> etm4_cs_lock(drvdata, csa); >> + cpu_enable_tracing(); >> } >> >> static inline u32 etm4_get_victlr_access_type(struct etmv4_config *config) >> -- >> 2.24.1 >> > > > -- > Mike Leach > Principal Engineer, ARM Ltd. > Manchester Design Centre. UK >
WARNING: multiple messages have this Message-ID (diff)
From: Suzuki K Poulose <suzuki.poulose@arm.com> To: Mike Leach <mike.leach@linaro.org> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>, Anshuman Khandual <anshuman.khandual@arm.com>, Catalin Marinas <catalin.marinas@arm.com>, Coresight ML <coresight@lists.linaro.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Jonathan Zhou <jonathan.zhouwen@huawei.com>, Leo Yan <leo.yan@linaro.org>, Will Deacon <will@kernel.org>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH v7 28/28] coresight: Add support for v8.4 SelfHosted tracing Date: Fri, 12 Feb 2021 15:36:33 +0000 [thread overview] Message-ID: <72f85de4-5b39-c963-78cf-2f8347e21268@arm.com> (raw) In-Reply-To: <CAJ9a7ViwNA__+5f7-DehM6OtGer3DjdZfWsgHc9EOcr5-vQXgA@mail.gmail.com> Hi Mike On 2/12/21 10:34 AM, Mike Leach wrote: > Hi Mathieu, Suzuki, > > Sorry for the really late response on this patch, but I noticed a > problem while doing a review of the ETE / TRBE set. (TRBE specs > mention TRFCR_ELx, so I was confirming a couple of things). > > On Sun, 10 Jan 2021 at 22:49, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: >> >> From: Jonathan Zhou <jonathan.zhouwen@huawei.com> >> >> v8.4 tracing extensions added support for trace filtering controlled >> by TRFCR_ELx. This must be programmed to allow tracing at EL1/EL2 and >> EL0. The timestamp used is the virtual time. Also enable CONTEXIDR_EL2 >> tracing if we are running the kernel at EL2. >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Mike Leach <mike.leach@linaro.org> >> Cc: Will Deacon <will@kernel.org> >> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> >> Signed-off-by: Jonathan Zhou <jonathan.zhouwen@huawei.com> >> [ Move the trace filtering setup etm_init_arch_data() and >> clean ups] >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> .../coresight/coresight-etm4x-core.c | 25 +++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> index 3d3165dd09d4..18c1a80abab8 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> @@ -859,6 +859,30 @@ static bool etm4_init_csdev_access(struct etmv4_drvdata *drvdata, >> return false; >> } >> >> +static void cpu_enable_tracing(void) >> +{ >> + u64 dfr0 = read_sysreg(id_aa64dfr0_el1); >> + u64 trfcr; >> + >> + if (!cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_TRACE_FILT_SHIFT)) >> + return; >> + >> + /* >> + * If the CPU supports v8.4 SelfHosted Tracing, enable >> + * tracing at the kernel EL and EL0, forcing to use the >> + * virtual time as the timestamp. >> + */ >> + trfcr = (TRFCR_ELx_TS_VIRTUAL | >> + TRFCR_ELx_ExTRE | >> + TRFCR_ELx_E0TRE); >> + >> + /* If we are running at EL2, allow tracing the CONTEXTIDR_EL2. */ >> + if (is_kernel_in_hyp_mode()) >> + trfcr |= TRFCR_EL2_CX; >> + > > This is wrong - CX bit is present on TRFCR_EL2, not TRFCR_EL1. Why is this wrong ? We do this only when we are in EL2. > Moreover, TRFCR_EL2 has a separate enables for tracing at EL0 and EL2. > True, that is for EL0&2 translation regimes. i.e, tracing EL0 with the kernel running at EL2. But bits TRFCR_EL2.E2TRE == TRFCR_EL1.E1TRE If notice, we name the bit TRFCR_ELx_ExTRE. And E0TRE == E0HTRE. So we do the following : 1) When kernel running at EL2: Enable tracing at EL2 and EL0 and context tracking 2) When kernel running at EL1: Enable tracing at EL1 and EL0. > Secondly - is this correct in principal? Should the driver not be > reading the access it is permitted by the kernel, rather than giving > itself unfettered access to trace where it wants to. I dont follow the "access permitted by the kernel" here. What are we referrring to ? > Surely TRFCR_ELx levels should be chosen in KConfig and then should > be set up in kernel initialisation? I disagree with yet another Kconfig. This basic requirement for enabling the trace collection. It is not something that we can optionally use from the architecture. So we should transparently do the right thing for making sure that we set up the system for something that didn't require any other steps. Or in other words, if we add a Kconfig option for TRFCR programming, if someone forgets to select it when they upgraded the kernel they are in for a surprisingly long debugging to find why the trace doesnt work. As for the TRFCR programming, we have two choices. etm4x driver or generic boot up for the CPU. I preferred to do this in the driver as we can enable it only if trace drivers are available. Cheers Suzuki > > Regards > > Mike > > > >> + write_sysreg_s(trfcr, SYS_TRFCR_EL1); >> +} >> + >> static void etm4_init_arch_data(void *info) >> { >> u32 etmidr0; >> @@ -1044,6 +1068,7 @@ static void etm4_init_arch_data(void *info) >> /* NUMCNTR, bits[30:28] number of counters available for tracing */ >> drvdata->nr_cntr = BMVAL(etmidr5, 28, 30); >> etm4_cs_lock(drvdata, csa); >> + cpu_enable_tracing(); >> } >> >> static inline u32 etm4_get_victlr_access_type(struct etmv4_config *config) >> -- >> 2.24.1 >> > > > -- > Mike Leach > Principal Engineer, ARM Ltd. > Manchester Design Centre. UK > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-02-12 15:38 UTC|newest] Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-10 22:48 [PATCH v7 00/28] coresight: etm4x: Support for system instructions Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 01/28] coresight: etm4x: Handle access to TRCSSPCICRn Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 02/28] coresight: etm4x: Skip accessing TRCPDCR in save/restore Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-25 18:39 ` Mathieu Poirier 2021-01-25 18:39 ` Mathieu Poirier 2021-01-10 22:48 ` [PATCH v7 03/28] coresight: Introduce device access abstraction Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-25 18:42 ` Mathieu Poirier 2021-01-25 18:42 ` Mathieu Poirier 2021-01-10 22:48 ` [PATCH v7 04/28] coresight: tpiu: Prepare for using coresight " Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 05/28] coresight: Convert coresight_timeout to use " Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 06/28] coresight: Convert claim/disclaim operations to use access wrappers Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 07/28] coresight: etm4x: Always read the registers on the host CPU Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 08/28] coresight: etm4x: Convert all register accesses Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 09/28] coresight: etm4x: Make offset available for sysfs attributes Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 10/28] coresight: etm4x: Add commentary on the registers Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 11/28] coresight: etm4x: Add sysreg access helpers Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 12/28] coresight: etm4x: Hide sysfs attributes for unavailable registers Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 13/28] coresight: etm4x: Define DEVARCH register fields Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 14/28] coresight: etm4x: Check for Software Lock Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 15/28] coresight: etm4x: Cleanup secure exception level masks Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 16/28] coresight: etm4x: Clean up " Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 17/28] coresight: etm4x: Handle ETM architecture version Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 18/28] coresight: etm4x: Detect access early on the target CPU Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 19/28] coresight: etm4x: Use TRCDEVARCH for component discovery Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 20/28] coresight: etm4x: Expose trcdevarch via sysfs Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 21/28] coresight: etm4x: Add necessary synchronization for sysreg access Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 22/28] coresight: etm4x: Detect system instructions support Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 23/28] coresight: etm4x: Refactor probing routine Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 24/28] coresight: etm4x: Run arch feature detection on the CPU Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 25/28] coresight: etm4x: Add support for sysreg only devices Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 26/28] dts: bindings: coresight: ETM system register access only units Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 27/28] arm64: Add TRFCR_ELx definitions Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-01-10 22:48 ` [PATCH v7 28/28] coresight: Add support for v8.4 SelfHosted tracing Suzuki K Poulose 2021-01-10 22:48 ` Suzuki K Poulose 2021-02-12 10:34 ` Mike Leach 2021-02-12 10:34 ` Mike Leach 2021-02-12 15:36 ` Suzuki K Poulose [this message] 2021-02-12 15:36 ` Suzuki K Poulose 2021-02-12 17:30 ` Mike Leach 2021-02-12 17:30 ` Mike Leach 2021-02-18 14:51 ` Suzuki K Poulose 2021-02-18 14:51 ` Suzuki K Poulose 2021-01-25 18:49 ` [PATCH v7 00/28] coresight: etm4x: Support for system instructions Mathieu Poirier 2021-01-25 18:49 ` Mathieu Poirier 2021-01-25 22:05 ` Suzuki K Poulose 2021-01-25 22:05 ` Suzuki K Poulose
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=72f85de4-5b39-c963-78cf-2f8347e21268@arm.com \ --to=suzuki.poulose@arm.com \ --cc=anshuman.khandual@arm.com \ --cc=catalin.marinas@arm.com \ --cc=coresight@lists.linaro.org \ --cc=jonathan.zhouwen@huawei.com \ --cc=leo.yan@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mathieu.poirier@linaro.org \ --cc=mike.leach@linaro.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.