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.
Suzuki K Poulose (3): 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/coresight-etm4x-core.c | 20 +++++++++---------- drivers/hwtracing/coresight/coresight-etm4x.h | 6 +++--- 2 files changed, 13 insertions(+), 13 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: f5bd523690d2f ("coresight: etm4x: Convert all register accesses") Reported-by: Yabin Cui yabinc@google.com 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..fae96151cdaf 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.
Fixes: f188b5e76aae9 ("coresight: etm4x: Save/restore state across CPU low power states") Reported-by: Yabin Cui yabinc@google.com Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 6 ------ drivers/hwtracing/coresight/coresight-etm4x.h | 3 --- 2 files changed, 9 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index fae96151cdaf..ff57aabb24c4 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..acd2b1db2052 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -907,9 +907,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;
On Thu, 11 Apr 2024 at 12:31, Suzuki K Poulose suzuki.poulose@arm.com wrote:
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.
Fixes: f188b5e76aae9 ("coresight: etm4x: Save/restore state across CPU low power states") Reported-by: Yabin Cui yabinc@google.com Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com
drivers/hwtracing/coresight/coresight-etm4x-core.c | 6 ------ drivers/hwtracing/coresight/coresight-etm4x.h | 3 --- 2 files changed, 9 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index fae96151cdaf..ff57aabb24c4 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..acd2b1db2052 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -907,9 +907,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;-- 2.34.1
this change means TRCVDCTLR, TRCVDSACCTLR, TRCVDARCCTLR #defines in ...etm4x.h, along with the CASE_##op.... macros are unused. Could be removed to prevent accidental re-introduction of access to these, perhaps replace with a note saying "we don't need these because...."
Either way
Reviewed-by: Mike Leach mike.leach@linaro.org
On 12/04/2024 13:56, Mike Leach wrote:
On Thu, 11 Apr 2024 at 12:31, Suzuki K Poulose suzuki.poulose@arm.com wrote:
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.
Fixes: f188b5e76aae9 ("coresight: etm4x: Save/restore state across CPU low power states") Reported-by: Yabin Cui yabinc@google.com Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com
drivers/hwtracing/coresight/coresight-etm4x-core.c | 6 ------ drivers/hwtracing/coresight/coresight-etm4x.h | 3 --- 2 files changed, 9 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index fae96151cdaf..ff57aabb24c4 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..acd2b1db2052 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -907,9 +907,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;-- 2.34.1
this change means TRCVDCTLR, TRCVDSACCTLR, TRCVDARCCTLR #defines in ...etm4x.h, along with the CASE_##op.... macros are unused. Could be removed to prevent accidental re-introduction of access to these, perhaps replace with a note saying "we don't need these because...."
Good point. I have removed all references to these registers in the sources. We should never touch them ever. I realise that we also have TRCDVCRn and TRCDVCMRn, that must not be touched. I will remove those CASEs.
Either way
Reviewed-by: Mike Leach mike.leach@linaro.org
Thanks Suzuki
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: f188b5e76aae9 ("coresight: etm4x: Save/restore state across CPU low power states") Reported-by: Yabin Cui yabinc@google.com 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 ff57aabb24c4..e0ebf7d7db91 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 acd2b1db2052..25dc8ffef5c0 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -136,6 +136,7 @@ * of _MASK for multi bit fields and BIT() for single bits. */ #define TRCIDR0_INSTP0_MASK GENMASK(2, 1) +#define TRCIDR0_QFILT BIT(4) #define TRCIDR0_TRCBB BIT(5) #define TRCIDR0_TRCCOND BIT(6) #define TRCIDR0_TRCCCI BIT(7) @@ -979,6 +980,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. @@ -1041,6 +1043,7 @@ struct etmv4_drvdata { bool boot_enable; bool os_unlock; bool instrp0; + bool q_filt; bool trcbb; bool trccond; bool retstack;
-
Mike Leach -
Suzuki K Poulose