Our CPUidle state save restore has some bugs which are triggered for an ETM with system instructions (e.g. ETE). This is a series of fixes to address them.
Changes since v1: - Fix inverted check for drvdata->csdev for Patch 1 (Yabin) - Remove Data trace register access macro cases (Mike Leach) - Fix QFILT field definition (Yabin) - (New patch) Fix Resource pair register access, reported-by Yabin
Suzuki K Poulose (4): coresight: etm4x: Do not hardcode IOMEM access for register restore coresight: etm4x: Do not save/restore Data trace control registers coresight: etm4x: Safe access for TRCQCLTR coresight: etm4x: Fix access to resource selector registers
.../coresight/coresight-etm4x-core.c | 26 +++++++++------- drivers/hwtracing/coresight/coresight-etm4x.h | 31 ++----------------- 2 files changed, 17 insertions(+), 40 deletions(-)
When we restore the register state for ETM4x, while coming back from CPU idle, we hardcode IOMEM access. This is wrong and could blow up for an ETM with system instructions access (and for ETE).
Fixes: f5bd523690d2 ("coresight: etm4x: Convert all register accesses") Reported-by: Yabin Cui yabinc@google.com Reviewed-by: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index c2ca4a02dfce..7bd849e28953 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1843,8 +1843,10 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) { int i; struct etmv4_save_state *state = drvdata->save_state; - struct csdev_access tmp_csa = CSDEV_ACCESS_IOMEM(drvdata->base); - struct csdev_access *csa = &tmp_csa; + struct csdev_access *csa = &drvdata->csdev->access; + + if (WARN_ON(!drvdata->csdev)) + return;
etm4_cs_unlock(drvdata, csa); etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);
ETM4x doesn't support Data trace on A class CPUs. As such do not access the Data trace control registers during CPU idle. This could cause problems for ETE. While at it, remove all references to the Data trace control registers.
Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU low power states") Reported-by: Yabin Cui yabinc@google.com Reviewed-by: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com --- .../coresight/coresight-etm4x-core.c | 6 ---- drivers/hwtracing/coresight/coresight-etm4x.h | 28 ------------------- 2 files changed, 34 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 7bd849e28953..81df753d8c15 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1739,9 +1739,6 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) state->trcvissctlr = etm4x_read32(csa, TRCVISSCTLR); if (drvdata->nr_pe_cmp) state->trcvipcssctlr = etm4x_read32(csa, TRCVIPCSSCTLR); - state->trcvdctlr = etm4x_read32(csa, TRCVDCTLR); - state->trcvdsacctlr = etm4x_read32(csa, TRCVDSACCTLR); - state->trcvdarcctlr = etm4x_read32(csa, TRCVDARCCTLR);
for (i = 0; i < drvdata->nrseqstate - 1; i++) state->trcseqevr[i] = etm4x_read32(csa, TRCSEQEVRn(i)); @@ -1872,9 +1869,6 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) etm4x_relaxed_write32(csa, state->trcvissctlr, TRCVISSCTLR); if (drvdata->nr_pe_cmp) etm4x_relaxed_write32(csa, state->trcvipcssctlr, TRCVIPCSSCTLR); - etm4x_relaxed_write32(csa, state->trcvdctlr, TRCVDCTLR); - etm4x_relaxed_write32(csa, state->trcvdsacctlr, TRCVDSACCTLR); - etm4x_relaxed_write32(csa, state->trcvdarcctlr, TRCVDARCCTLR);
for (i = 0; i < drvdata->nrseqstate - 1; i++) etm4x_relaxed_write32(csa, state->trcseqevr[i], TRCSEQEVRn(i)); diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 9ea678bc2e8e..9e430f72bbd6 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -43,9 +43,6 @@ #define TRCVIIECTLR 0x084 #define TRCVISSCTLR 0x088 #define TRCVIPCSSCTLR 0x08C -#define TRCVDCTLR 0x0A0 -#define TRCVDSACCTLR 0x0A4 -#define TRCVDARCCTLR 0x0A8 /* Derived resources registers */ #define TRCSEQEVRn(n) (0x100 + (n * 4)) /* n = 0-2 */ #define TRCSEQRSTEVR 0x118 @@ -90,9 +87,6 @@ /* Address Comparator registers n = 0-15 */ #define TRCACVRn(n) (0x400 + (n * 8)) #define TRCACATRn(n) (0x480 + (n * 8)) -/* Data Value Comparator Value registers, n = 0-7 */ -#define TRCDVCVRn(n) (0x500 + (n * 16)) -#define TRCDVCMRn(n) (0x580 + (n * 16)) /* ContextID/Virtual ContextID comparators, n = 0-7 */ #define TRCCIDCVRn(n) (0x600 + (n * 8)) #define TRCVMIDCVRn(n) (0x640 + (n * 8)) @@ -272,9 +266,6 @@ /* List of registers accessible via System instructions */ #define ETM4x_ONLY_SYSREG_LIST(op, val) \ CASE_##op((val), TRCPROCSELR) \ - CASE_##op((val), TRCVDCTLR) \ - CASE_##op((val), TRCVDSACCTLR) \ - CASE_##op((val), TRCVDARCCTLR) \ CASE_##op((val), TRCOSLAR)
#define ETM_COMMON_SYSREG_LIST(op, val) \ @@ -422,22 +413,6 @@ CASE_##op((val), TRCACATRn(13)) \ CASE_##op((val), TRCACATRn(14)) \ CASE_##op((val), TRCACATRn(15)) \ - CASE_##op((val), TRCDVCVRn(0)) \ - CASE_##op((val), TRCDVCVRn(1)) \ - CASE_##op((val), TRCDVCVRn(2)) \ - CASE_##op((val), TRCDVCVRn(3)) \ - CASE_##op((val), TRCDVCVRn(4)) \ - CASE_##op((val), TRCDVCVRn(5)) \ - CASE_##op((val), TRCDVCVRn(6)) \ - CASE_##op((val), TRCDVCVRn(7)) \ - CASE_##op((val), TRCDVCMRn(0)) \ - CASE_##op((val), TRCDVCMRn(1)) \ - CASE_##op((val), TRCDVCMRn(2)) \ - CASE_##op((val), TRCDVCMRn(3)) \ - CASE_##op((val), TRCDVCMRn(4)) \ - CASE_##op((val), TRCDVCMRn(5)) \ - CASE_##op((val), TRCDVCMRn(6)) \ - CASE_##op((val), TRCDVCMRn(7)) \ CASE_##op((val), TRCCIDCVRn(0)) \ CASE_##op((val), TRCCIDCVRn(1)) \ CASE_##op((val), TRCCIDCVRn(2)) \ @@ -907,9 +882,6 @@ struct etmv4_save_state { u32 trcviiectlr; u32 trcvissctlr; u32 trcvipcssctlr; - u32 trcvdctlr; - u32 trcvdsacctlr; - u32 trcvdarcctlr;
u32 trcseqevr[ETM_MAX_SEQ_STATES]; u32 trcseqrstevr;
ETM4x implements TRCQCLTR only when the Q elements are supported and the Q element filtering is supported (TRCIDR0.QFILT). Access to the register otherwise could be fatal. Fix this by tracking the availability, like the others.
Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU low power states") Reported-by: Yabin Cui yabinc@google.com Reviewed-by: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 8 ++++++-- drivers/hwtracing/coresight/coresight-etm4x.h | 3 +++ 2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 81df753d8c15..b4b84f2317cd 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1240,6 +1240,8 @@ static void etm4_init_arch_data(void *info) drvdata->nr_event = FIELD_GET(TRCIDR0_NUMEVENT_MASK, etmidr0); /* QSUPP, bits[16:15] Q element support field */ drvdata->q_support = FIELD_GET(TRCIDR0_QSUPP_MASK, etmidr0); + if (drvdata->q_support) + drvdata->q_filt = !!(etmidr0 & TRCIDR0_QFILT); /* TSSIZE, bits[28:24] Global timestamp size field */ drvdata->ts_size = FIELD_GET(TRCIDR0_TSSIZE_MASK, etmidr0);
@@ -1732,7 +1734,8 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) state->trcccctlr = etm4x_read32(csa, TRCCCCTLR); state->trcbbctlr = etm4x_read32(csa, TRCBBCTLR); state->trctraceidr = etm4x_read32(csa, TRCTRACEIDR); - state->trcqctlr = etm4x_read32(csa, TRCQCTLR); + if (drvdata->q_filt) + state->trcqctlr = etm4x_read32(csa, TRCQCTLR);
state->trcvictlr = etm4x_read32(csa, TRCVICTLR); state->trcviiectlr = etm4x_read32(csa, TRCVIIECTLR); @@ -1862,7 +1865,8 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) etm4x_relaxed_write32(csa, state->trcccctlr, TRCCCCTLR); etm4x_relaxed_write32(csa, state->trcbbctlr, TRCBBCTLR); etm4x_relaxed_write32(csa, state->trctraceidr, TRCTRACEIDR); - etm4x_relaxed_write32(csa, state->trcqctlr, TRCQCTLR); + if (drvdata->q_filt) + etm4x_relaxed_write32(csa, state->trcqctlr, TRCQCTLR);
etm4x_relaxed_write32(csa, state->trcvictlr, TRCVICTLR); etm4x_relaxed_write32(csa, state->trcviiectlr, TRCVIIECTLR); diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 9e430f72bbd6..9e9165f62e81 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -135,6 +135,7 @@ #define TRCIDR0_TRCCCI BIT(7) #define TRCIDR0_RETSTACK BIT(9) #define TRCIDR0_NUMEVENT_MASK GENMASK(11, 10) +#define TRCIDR0_QFILT BIT(14) #define TRCIDR0_QSUPP_MASK GENMASK(16, 15) #define TRCIDR0_TSSIZE_MASK GENMASK(28, 24)
@@ -954,6 +955,7 @@ struct etmv4_save_state { * @os_unlock: True if access to management registers is allowed. * @instrp0: Tracing of load and store instructions * as P0 elements is supported. + * @q_filt: Q element filtering support, if Q elements are supported. * @trcbb: Indicates if the trace unit supports branch broadcast tracing. * @trccond: If the trace unit supports conditional * instruction tracing. @@ -1016,6 +1018,7 @@ struct etmv4_drvdata { bool boot_enable; bool os_unlock; bool instrp0; + bool q_filt; bool trcbb; bool trccond; bool retstack;
Resource selector pair 0 is always implemented and reserved. We must not touch it, even during save/restore for CPU Idle. Rest of the driver is well behaved. Fix the offending ones.
Reported-by: Yabin Cui yabinc@google.com Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU low power states") Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index b4b84f2317cd..577cf485e761 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1758,7 +1758,8 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) state->trccntvr[i] = etm4x_read32(csa, TRCCNTVRn(i)); }
- for (i = 0; i < drvdata->nr_resource * 2; i++) + /* Resource selector pair 0 is reserved */ + for (i = 2; i < drvdata->nr_resource * 2; i++) state->trcrsctlr[i] = etm4x_read32(csa, TRCRSCTLRn(i));
for (i = 0; i < drvdata->nr_ss_cmp; i++) { @@ -1889,7 +1890,8 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) etm4x_relaxed_write32(csa, state->trccntvr[i], TRCCNTVRn(i)); }
- for (i = 0; i < drvdata->nr_resource * 2; i++) + /* Resource selector pair 0 is reserved */ + for (i = 2; i < drvdata->nr_resource * 2; i++) etm4x_relaxed_write32(csa, state->trcrsctlr[i], TRCRSCTLRn(i));
for (i = 0; i < drvdata->nr_ss_cmp; i++) {
On Fri, 12 Apr 2024 at 15:27, Suzuki K Poulose suzuki.poulose@arm.com wrote:
Resource selector pair 0 is always implemented and reserved. We must not touch it, even during save/restore for CPU Idle. Rest of the driver is well behaved. Fix the offending ones.
Reported-by: Yabin Cui yabinc@google.com Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU low power states") Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com
drivers/hwtracing/coresight/coresight-etm4x-core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index b4b84f2317cd..577cf485e761 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1758,7 +1758,8 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) state->trccntvr[i] = etm4x_read32(csa, TRCCNTVRn(i)); }
for (i = 0; i < drvdata->nr_resource * 2; i++)
/* Resource selector pair 0 is reserved */for (i = 2; i < drvdata->nr_resource * 2; i++) state->trcrsctlr[i] = etm4x_read32(csa, TRCRSCTLRn(i)); for (i = 0; i < drvdata->nr_ss_cmp; i++) {@@ -1889,7 +1890,8 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) etm4x_relaxed_write32(csa, state->trccntvr[i], TRCCNTVRn(i)); }
for (i = 0; i < drvdata->nr_resource * 2; i++)
/* Resource selector pair 0 is reserved */for (i = 2; i < drvdata->nr_resource * 2; i++) etm4x_relaxed_write32(csa, state->trcrsctlr[i], TRCRSCTLRn(i)); for (i = 0; i < drvdata->nr_ss_cmp; i++) {-- 2.34.1
Reviewed-by: Mike Leach mike.leach@linaro.org
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
On Fri, 12 Apr 2024 15:26:58 +0100, Suzuki K Poulose wrote:
Our CPUidle state save restore has some bugs which are triggered for an ETM with system instructions (e.g. ETE). This is a series of fixes to address them.
Changes since v1:
- Fix inverted check for drvdata->csdev for Patch 1 (Yabin)
- Remove Data trace register access macro cases (Mike Leach)
- Fix QFILT field definition (Yabin)
- (New patch) Fix Resource pair register access, reported-by Yabin
[...]
Applied, thanks!
[1/4] coresight: etm4x: Do not hardcode IOMEM access for register restore https://git.kernel.org/coresight/c/1e7ba33fa591de1cf60afffcabb45600b3607025 [2/4] coresight: etm4x: Do not save/restore Data trace control registers https://git.kernel.org/coresight/c/5eb3a0c2c52368cb9902e9a6ea04888e093c487d [3/4] coresight: etm4x: Safe access for TRCQCLTR https://git.kernel.org/coresight/c/46bf8d7cd8530eca607379033b9bc4ac5590a0cd [4/4] coresight: etm4x: Fix access to resource selector registers https://git.kernel.org/coresight/c/d6fc00d0f640d6010b51054aa8b0fd191177dbc9
Best regards,
-
Mike Leach -
Suzuki K Poulose