[PATCH 0/2] coresight: Allow guests to be traced when FEAT_TRF and VHE are present

[PATCH 0/2] coresight: Allow guests to be traced when FEAT_TRF and VHE are present

[James Clark]

The first commit moves the register to sysreg because I add the EL12
version in the second commit. I couldn't really think of the best way
to make it fit in sysreg without any changes, because all the fields are
shared between EL1 and EL2 except for the CX bit which was named
differently for that reason in the original definition. The CX bit only
exists in TRFCR_EL2.

The test results have some single spurious EL2 addresses, but I don't
think this is an issue with this patchset because it happens in the
host-userspace case which maintains the existing programming of
TRFCR. It's likely an issue with the model but I will follow it up
separately.

James Clark (2):
  arm64/sysreg: Move TRFCR definitions to sysreg
  coresight: Allow guests to be traced when FEAT_TRF and VHE are present

 arch/arm64/include/asm/sysreg.h               | 12 -----
 arch/arm64/tools/sysreg                       | 26 ++++++++++
 .../coresight/coresight-etm4x-core.c          | 47 ++++++++++++++++---
 drivers/hwtracing/coresight/coresight-etm4x.h |  2 +-
 drivers/hwtracing/coresight/coresight-priv.h  |  3 ++
 5 files changed, 70 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH 1/2] arm64/sysreg: Move TRFCR definitions to sysreg

[James Clark]

TRFCR_EL2_CX needs to become TRFCR_ELx_CX to avoid unnecessary
duplication and make the SysregFields block re-usable.

Signed-off-by: James Clark <james.clark@arm.com>
---
 arch/arm64/include/asm/sysreg.h               | 12 ----------
 arch/arm64/tools/sysreg                       | 22 +++++++++++++++++++
 .../coresight/coresight-etm4x-core.c          |  2 +-
 3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index b481935e9314..fc9a5a09fa04 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -171,8 +171,6 @@
 #define SYS_RGSR_EL1			sys_reg(3, 0, 1, 0, 5)
 #define SYS_GCR_EL1			sys_reg(3, 0, 1, 0, 6)
 
-#define SYS_TRFCR_EL1			sys_reg(3, 0, 1, 2, 1)
-
 #define SYS_TCR_EL1			sys_reg(3, 0, 2, 0, 2)
 
 #define SYS_APIAKEYLO_EL1		sys_reg(3, 0, 2, 1, 0)
@@ -382,7 +380,6 @@
 #define SYS_VTTBR_EL2			sys_reg(3, 4, 2, 1, 0)
 #define SYS_VTCR_EL2			sys_reg(3, 4, 2, 1, 2)
 
-#define SYS_TRFCR_EL2			sys_reg(3, 4, 1, 2, 1)
 #define SYS_HDFGRTR_EL2			sys_reg(3, 4, 3, 1, 4)
 #define SYS_HDFGWTR_EL2			sys_reg(3, 4, 3, 1, 5)
 #define SYS_HAFGRTR_EL2			sys_reg(3, 4, 3, 1, 6)
@@ -640,15 +637,6 @@
 /* Safe value for MPIDR_EL1: Bit31:RES1, Bit30:U:0, Bit24:MT:0 */
 #define SYS_MPIDR_SAFE_VAL	(BIT(31))
 
-#define TRFCR_ELx_TS_SHIFT		5
-#define TRFCR_ELx_TS_MASK		((0x3UL) << TRFCR_ELx_TS_SHIFT)
-#define TRFCR_ELx_TS_VIRTUAL		((0x1UL) << TRFCR_ELx_TS_SHIFT)
-#define TRFCR_ELx_TS_GUEST_PHYSICAL	((0x2UL) << TRFCR_ELx_TS_SHIFT)
-#define TRFCR_ELx_TS_PHYSICAL		((0x3UL) << TRFCR_ELx_TS_SHIFT)
-#define TRFCR_EL2_CX			BIT(3)
-#define TRFCR_ELx_ExTRE			BIT(1)
-#define TRFCR_ELx_E0TRE			BIT(0)
-
 /* GIC Hypervisor interface registers */
 /* ICH_MISR_EL2 bit definitions */
 #define ICH_MISR_EOI		(1 << 0)
diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index 65866bf819c3..fe1f824977d9 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -2495,3 +2495,25 @@ Field	5	F
 Field	4	P
 Field	3:0	Align
 EndSysreg
+
+SysregFields TRFCR_ELx
+Res0	63:7
+UnsignedEnum	6:5	TS
+	0b0001	VIRTUAL
+	0b0010	GUEST_PHYSICAL
+	0b0011	PHYSICAL
+EndEnum
+Res0	4
+Field	3	CX
+Res0	2
+Field	1	ExTRE
+Field	0	E0TRE
+EndSysregFields
+
+Sysreg	TRFCR_EL1	3	0	1	2	1
+Fields	TRFCR_ELx
+EndSysreg
+
+Sysreg	TRFCR_EL2	3	4	1	2	1
+Fields	TRFCR_ELx
+EndSysreg
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 703b6fcbb6a5..257e5c1a4b52 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1145,7 +1145,7 @@ static void cpu_detect_trace_filtering(struct etmv4_drvdata *drvdata)
 
 	/* If we are running at EL2, allow tracing the CONTEXTIDR_EL2. */
 	if (is_kernel_in_hyp_mode())
-		trfcr |= TRFCR_EL2_CX;
+		trfcr |= TRFCR_ELx_CX;
 
 	drvdata->trfcr = trfcr;
 }
-- 
2.34.1

Re: [PATCH 1/2] arm64/sysreg: Move TRFCR definitions to sysreg

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 588 bytes --]

On Fri, Aug 04, 2023 at 09:52:16AM +0100, James Clark wrote:

> TRFCR_EL2_CX needs to become TRFCR_ELx_CX to avoid unnecessary
> duplication and make the SysregFields block re-usable.

That field is only present in the EL2 version.  I would tend to leave
the registers split for that reason, there's some minor potential for
confusion if people refer to the sysreg file rather than the docs, or
potentially confuse some future automation.  However that's not a super
strongly held opinion.

Otherwise this checks out against DDI0601 2023-06:

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] arm64/sysreg: Move TRFCR definitions to sysreg

[James Clark]

On 04/08/2023 13:10, Mark Brown wrote:
> On Fri, Aug 04, 2023 at 09:52:16AM +0100, James Clark wrote:
> 
>> TRFCR_EL2_CX needs to become TRFCR_ELx_CX to avoid unnecessary
>> duplication and make the SysregFields block re-usable.
> 
> That field is only present in the EL2 version.  I would tend to leave
> the registers split for that reason, there's some minor potential for
> confusion if people refer to the sysreg file rather than the docs, or
> potentially confuse some future automation.  However that's not a super
> strongly held opinion.
> 

True, the potential for confusion is a good reason to not try to avoid
duplication. Probably helps if it is ever auto generated or validated as
well.

I could update it on the next version. But do I leave all the existing
_ELx usages in the code, or change them all to _EL1 (Except CX_EL2)? To
leave them as _ELx sysreg would look like this, even though _EL1 would
probably be more accurate:

  SysregFields TRFCR_EL2
  Res0	63:7
  UnsignedEnum	6:5	TS
  	0b0001	VIRTUAL
  	0b0010	GUEST_PHYSICAL
  	0b0011	PHYSICAL
  EndEnum
  Res0	4
  Field	3	CX
  Res0	2
  Field	1	E2TRE
  Field	0	E0TRE
  EndSysregFields

  SysregFields TRFCR_ELx
  Res0	63:7
  UnsignedEnum	6:5	TS
  	0b0001	VIRTUAL
  	0b0010	GUEST_PHYSICAL
  	0b0011	PHYSICAL
  EndEnum
  Res0	4:2
  Field	1	ExTRE
  Field	0	E0TRE
  EndSysregFields

  Sysreg	TRFCR_EL1	3	0	1	2	1
  Fields	TRFCR_ELx
  EndSysreg

  Sysreg	TRFCR_EL2	3	4	1	2	1
  Fields	TRFCR_EL2
  EndSysreg

  Sysreg TRFCR_EL12	3	5	1	2	1
  Fields	TRFCR_ELx
  EndSysreg


> Otherwise this checks out against DDI0601 2023-06:
> 
> Reviewed-by: Mark Brown <broonie@kernel.org>

Thanks for the review

Re: [PATCH 1/2] arm64/sysreg: Move TRFCR definitions to sysreg

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

On Fri, Aug 04, 2023 at 04:55:19PM +0100, James Clark wrote:
> On 04/08/2023 13:10, Mark Brown wrote:
> > On Fri, Aug 04, 2023 at 09:52:16AM +0100, James Clark wrote:

> >> TRFCR_EL2_CX needs to become TRFCR_ELx_CX to avoid unnecessary
> >> duplication and make the SysregFields block re-usable.

> > That field is only present in the EL2 version.  I would tend to leave
> > the registers split for that reason, there's some minor potential for
> > confusion if people refer to the sysreg file rather than the docs, or
> > potentially confuse some future automation.  However that's not a super
> > strongly held opinion.

> True, the potential for confusion is a good reason to not try to avoid
> duplication. Probably helps if it is ever auto generated or validated as
> well.

> I could update it on the next version. But do I leave all the existing
> _ELx usages in the code, or change them all to _EL1 (Except CX_EL2)? To
> leave them as _ELx sysreg would look like this, even though _EL1 would
> probably be more accurate:

>   SysregFields TRFCR_EL2

You could just leave this as _ELx and simply not reference it for the
EL1 definition which is proably fair?  Perhaps with a comment saying why
there's an expanded definition for EL1.  I don't think it fundamentally
matters which way it's done so long as EL1 stays a subset of the EL2
definition (which seems likely, and we can always revisit should that
happen).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/2] coresight: Allow guests to be traced when FEAT_TRF and VHE are present

[James Clark]

Currently the userspace and kernel filters for guests are never set, so
no trace will be generated for them. Add it by writing to the guest
filters when exclude_guest isn't set. By writing either E1TRE or E0TRE,
filtering on either guest kernel or guest userspace is also supported.

Since TRFCR_EL1 access is trapped, this can't be  modified by the guest.

This change also brings exclude_host support which is difficult to add
as a separate commit without excess churn and resulting in no trace at
all.

Testing
=======

The addresses were counted with the following:

  $ perf report -D | grep -Eo 'EL2|EL1|EL0' | sort | uniq -c

Guest kernel only:

  $ perf record -e cs_etm//Gk -a -- true
    535 EL1
      1 EL2

Guest user only (0 addresses expected because the guest OS hasn't reached
userspace yet):

  $ perf record -e cs_etm//Gu -a -- true

Host kernel only:

  $  perf record -e cs_etm//Hk -a -- true
   3501 EL2

Host userspace only:

  $  perf record -e cs_etm//Hu -a -- true
    408 EL0
      1 EL2

Signed-off-by: James Clark <james.clark@arm.com>
---
 arch/arm64/tools/sysreg                       |  4 ++
 .../coresight/coresight-etm4x-core.c          | 45 ++++++++++++++++---
 drivers/hwtracing/coresight/coresight-etm4x.h |  2 +-
 drivers/hwtracing/coresight/coresight-priv.h  |  3 ++
 4 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index fe1f824977d9..81028ef08415 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -2517,3 +2517,7 @@ EndSysreg
 Sysreg	TRFCR_EL2	3	4	1	2	1
 Fields	TRFCR_ELx
 EndSysreg
+
+Sysreg TRFCR_EL12	3	5	1	2	1
+Fields	TRFCR_ELx
+EndSysreg
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 257e5c1a4b52..8c0356ec905e 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -274,6 +274,18 @@ static void etm4x_prohibit_trace(struct etmv4_drvdata *drvdata)
 	cpu_prohibit_trace();
 }
 
+static u64 etm4x_get_kern_user_filter(struct etmv4_drvdata *drvdata)
+{
+	u64 trfcr = drvdata->trfcr;
+
+	if (drvdata->config.mode & ETM_MODE_EXCL_KERN)
+		trfcr &= ~TRFCR_ELx_ExTRE;
+	if (drvdata->config.mode & ETM_MODE_EXCL_USER)
+		trfcr &= ~TRFCR_ELx_E0TRE;
+
+	return trfcr;
+}
+
 /*
  * etm4x_allow_trace - Allow CPU tracing in the respective ELs,
  * as configured by the drvdata->config.mode for the current
@@ -286,18 +298,33 @@ static void etm4x_prohibit_trace(struct etmv4_drvdata *drvdata)
  */
 static void etm4x_allow_trace(struct etmv4_drvdata *drvdata)
 {
-	u64 trfcr = drvdata->trfcr;
+	u64 trfcr;
 
 	/* If the CPU doesn't support FEAT_TRF, nothing to do */
-	if (!trfcr)
+	if (!drvdata->trfcr)
 		return;
 
-	if (drvdata->config.mode & ETM_MODE_EXCL_KERN)
-		trfcr &= ~TRFCR_ELx_ExTRE;
-	if (drvdata->config.mode & ETM_MODE_EXCL_USER)
-		trfcr &= ~TRFCR_ELx_E0TRE;
+	if (drvdata->config.mode & ETM_MODE_EXCL_HOST)
+		trfcr = drvdata->trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE);
+	else
+		trfcr = etm4x_get_kern_user_filter(drvdata);
 
 	write_trfcr(trfcr);
+
+	/*
+	 * Filters for EL1 and EL0 (when running a guest) are stored in
+	 * TRFCR_EL1 so write it there for VHE. For nVHE, the filters in
+	 * have to be re-applied when switching to the guest instead.
+	 */
+	if (!is_kernel_in_hyp_mode())
+		return;
+
+	if (drvdata->config.mode & ETM_MODE_EXCL_GUEST)
+		trfcr = drvdata->trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE);
+	else
+		trfcr = etm4x_get_kern_user_filter(drvdata);
+
+	write_sysreg_s(trfcr, SYS_TRFCR_EL12);
 }
 
 #ifdef CONFIG_ETM4X_IMPDEF_FEATURE
@@ -655,6 +682,12 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
 	if (attr->exclude_user)
 		config->mode = ETM_MODE_EXCL_USER;
 
+	if (attr->exclude_host)
+		config->mode |= ETM_MODE_EXCL_HOST;
+
+	if (attr->exclude_guest)
+		config->mode |= ETM_MODE_EXCL_GUEST;
+
 	/* Always start from the default config */
 	etm4_set_default_config(config);
 
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 20e2e4cb7614..3f170599822f 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -841,7 +841,7 @@ enum etm_impdef_type {
  * @s_ex_level: Secure ELs where tracing is supported.
  */
 struct etmv4_config {
-	u32				mode;
+	u64				mode;
 	u32				pe_sel;
 	u32				cfg;
 	u32				eventctrl0;
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 767076e07970..727dd27ba800 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -39,6 +39,9 @@
 
 #define ETM_MODE_EXCL_KERN	BIT(30)
 #define ETM_MODE_EXCL_USER	BIT(31)
+#define ETM_MODE_EXCL_HOST	BIT(32)
+#define ETM_MODE_EXCL_GUEST	BIT(33)
+
 struct cs_pair_attribute {
 	struct device_attribute attr;
 	u32 lo_off;
-- 
2.34.1

Re: [PATCH 2/2] coresight: Allow guests to be traced when FEAT_TRF and VHE are present

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 231 bytes --]

On Fri, Aug 04, 2023 at 09:52:17AM +0100, James Clark wrote:

> +
> +Sysreg TRFCR_EL12	3	5	1	2	1
> +Fields	TRFCR_ELx
> +EndSysreg

This bit checks out against DDI0601 2023-06:

Reviewed-by: Mark Brown <broonie@kernel.org> (sysreg)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]