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 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;

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