| Message ID | 20230804085219.260790-2-james.clark@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: Allow guests to be traced when FEAT_TRF and VHE are present | expand |
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>
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
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).
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; }
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(-)