| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ae66124f-e839-4696-8d84-7be44e7e5ea9@arm.com>
Date: Tue, 14 Jan 2025 16:03:37 +0000
From: Suzuki K Poulose <suzuki.poulose@....com>
To: yuanfang zhang <quic_yuanfang@...cinc.com>, mike.leach@...aro.org,
james.clark@...aro.org, alexander.shishkin@...ux.intel.com
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] coresight-etm4x: add isb() before reading the TRCSTATR
Hi Yuanfang Zhang,
On 06/12/2024 08:02, yuanfang zhang wrote:
> From: Yuanfang Zhang <quic_yuanfang@...cinc.com>
>
> As recommended by section 4.3.7 ("Synchronization when using system
> instructions to progrom the trace unit") of ARM IHI 0064H.b, the
> self-hosted trace analyzer must perform a Context synchronization
> event between writing to the TRCPRGCTLR and reading the TRCSTATR.
>
> Fixes: ebddaad09e10 ("coresight: etm4x: Add missing single-shot control API to sysfs")
This looks wrong. This should rather be :
Fixes: 1ab3bb9df5e3 ("coresight: etm4x: Add necessary synchronization
for sysreg access")
> Signed-off-by: Yuanfang Zhang <quic_yuanfang@...cinc.com>
> ---
> Changes in V3:
> 1. Remove dsb(sy) after polling TRCSTATR.
Please separate this into a different patch.
> 2. Add isb() after polling TRCSTATR.
> ---
> .../hwtracing/coresight/coresight-etm4x-core.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 66d44a404ad0..c6ea00bba0cc 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -531,7 +531,6 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
> * As recommended by section 4.3.7 ("Synchronization when using the
> * memory-mapped interface") of ARM IHI 0064D
> */
> - dsb(sy);
> isb();
>
This should be a separate patch, as the main issue needs to be ported
back to stable.
> done:
> @@ -906,10 +905,25 @@ static void etm4_disable_hw(void *info)
> tsb_csync();
> etm4x_relaxed_write32(csa, control, TRCPRGCTLR);
>
> + /*
> + * As recommended by section 4.3.7 ("Synchronization when using system
> + * instructions to progrom the trace unit") of ARM IHI 0064H.b, the
> + * self-hosted trace analyzer must perform a Context synchronization
> + * event between writing to the TRCPRGCTLR and reading the TRCSTATR.
> + */
> + if (!csa->io_mem)
> + isb();
> +
> /* wait for TRCSTATR.PMSTABLE to go to '1' */
> if (coresight_timeout(csa, TRCSTATR, TRCSTATR_PMSTABLE_BIT, 1))
> dev_err(etm_dev,
> "timeout while waiting for PM stable Trace Status\n");
Additionally, we need an ISB between the each read of TRCSTATR (in
coresight_timeout() above), as per the secion 4.3.7, Synchronization
when using system instructions to program the trace unit, Note secion.
Please could you fix that as well ? That would require, changing the
coresight_timeout(), or add something on our own here for etm4x driver
to do that.
Something like (untested):
diff --git a/drivers/hwtracing/coresight/coresight-core.c
b/drivers/hwtracing/coresight/coresight-core.c
index 0a9380350fb5..f579181362b4 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1092,18 +1092,20 @@ static void coresight_remove_conns(struct
coresight_device *csdev)
}
/**
- * coresight_timeout - loop until a bit has changed to a specific register
- * state.
+ * coresight_timeout_action - loop until a bit has changed to a
specific register
+ * state, with a callback after every trial.
* @csa: coresight device access for the device
* @offset: Offset of the register from the base of the device.
* @position: the position of the bit of interest.
* @value: the value the bit should have.
+ * @cb: Call back after each trial
*
* Return: 0 as soon as the bit has taken the desired state or -EAGAIN if
* TIMEOUT_US has elapsed, which ever happens first.
*/
-int coresight_timeout(struct csdev_access *csa, u32 offset,
- int position, int value)
+int coresight_timeout_action(struct csdev_access *csa, u32 offset,
+ int position, int value,
+ coresight_timeout_cb_t cb)
{
int i;
u32 val;
@@ -1119,6 +1121,8 @@ int coresight_timeout(struct csdev_access *csa,
u32 offset,
if (!(val & BIT(position)))
return 0;
}
+ if (cb)
+ cb(csa, offset, position, value);
/*
* Delay is arbitrary - the specification doesn't say how long
@@ -1131,6 +1135,13 @@ int coresight_timeout(struct csdev_access *csa,
u32 offset,
return -EAGAIN;
}
+EXPORT_SYMBOL_GPL(coresight_timeout_action);
+
+int coresight_timeout(struct csdev_access *csa, u32 offset,
+ int position, int value)
+{
+ return coresight_timeout_action(csa, offset, position, value, NULL);
+}
EXPORT_SYMBOL_GPL(coresight_timeout);
u32 coresight_relaxed_read32(struct coresight_device *csdev, u32 offset)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c
b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index dd8c74f893db..c60f145bffe2 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -399,6 +399,29 @@ static void etm4_check_arch_features(struct
etmv4_drvdata *drvdata,
}
#endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
+static void etm4x_sys_ins_barrier(struct csdev_access *csa, u32 offset,
int pos, int val)
+{
+ if (!csa->io_mem)
+ isb();
+}
+
+/*
+ * etm4x_wait_status: Poll for TRCSTATR.<pos> == <val>. While using system
+ * instruction to access the trace unit, each access must be separated by a
+ * synchronization barrier. See ARM IHI0064H.b section "4.3.7
Synchronization of
+ * register updates", for system instructions section, in "Notes":
+ *
+ * "In particular, whenever disabling or enabling the trace unit, a
poll of
+ * TRCSTATR needs explicit synchronization between each read of
TRCSTATR"
+ */
+static int etm4x_wait_status(struct csdev_access *csa, int pos, int val)
+{
+ if (!csa->io_mem)
+ return coresight_timeout_action(csa, TRCSTATR, pos, val,
+ etm4x_sys_ins_barrier);
+ return coresight_timeout(csa, TRCSTATR, pos, val);
+}
+
static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
{
int i, rc;
@@ -430,7 +453,7 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
isb();
/* wait for TRCSTATR.IDLE to go up */
- if (coresight_timeout(csa, TRCSTATR, TRCSTATR_IDLE_BIT, 1))
+ if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 1))
dev_err(etm_dev,
"timeout while waiting for Idle Trace Status\n");
if (drvdata->nr_pe)
@@ -518,12 +541,14 @@ static int etm4_enable_hw(struct etmv4_drvdata
*drvdata)
/* Enable the trace unit */
etm4x_relaxed_write32(csa, 1, TRCPRGCTLR);
+ if (csa->io_mem)
+ dsb(sy);
/* Synchronize the register updates for sysreg access */
if (!csa->io_mem)
isb();
/* wait for TRCSTATR.IDLE to go back down to '0' */
- if (coresight_timeout(csa, TRCSTATR, TRCSTATR_IDLE_BIT, 0))
+ if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 0))
dev_err(etm_dev,
"timeout while waiting for Idle Trace Status\n");
@@ -531,7 +556,6 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
* As recommended by section 4.3.7 ("Synchronization when using the
* memory-mapped interface") of ARM IHI 0064D
*/
- dsb(sy);
isb();
done:
@@ -907,7 +931,7 @@ static void etm4_disable_hw(void *info)
etm4x_relaxed_write32(csa, control, TRCPRGCTLR);
/* wait for TRCSTATR.PMSTABLE to go to '1' */
- if (coresight_timeout(csa, TRCSTATR, TRCSTATR_PMSTABLE_BIT, 1))
+ if (etm4x_wait_status(csa, TRCSTATR_PMSTABLE_BIT, 1))
dev_err(etm_dev,
"timeout while waiting for PM stable Trace Status\n");
/* read the status of the single shot comparators */
@@ -1711,7 +1735,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata
*drvdata)
etm4_os_lock(drvdata);
/* wait for TRCSTATR.PMSTABLE to go up */
- if (coresight_timeout(csa, TRCSTATR, TRCSTATR_PMSTABLE_BIT, 1)) {
+ if (etm4x_wait_status(csa, TRCSTATR_PMSTABLE_BIT, 1)) {
dev_err(etm_dev,
"timeout while waiting for PM Stable Status\n");
etm4_os_unlock(drvdata);
@@ -1802,7 +1826,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata
*drvdata)
state->trcpdcr = etm4x_read32(csa, TRCPDCR);
/* wait for TRCSTATR.IDLE to go up */
- if (coresight_timeout(csa, TRCSTATR, TRCSTATR_IDLE_BIT, 1)) {
+ if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 1)) {
dev_err(etm_dev,
"timeout while waiting for Idle Trace Status\n");
etm4_os_unlock(drvdata);
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 17276965ff1d..03dd53fe724a 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -649,6 +649,10 @@ extern int coresight_enable_sysfs(struct
coresight_device *csdev);
extern void coresight_disable_sysfs(struct coresight_device *csdev);
extern int coresight_timeout(struct csdev_access *csa, u32 offset,
int position, int value);
+typedef void (*coresight_timeout_cb_t) (struct csdev_access *, u32,
int, int);
+extern int coresight_timeout_action(struct csdev_access *csa, u32 offset,
+ int position, int value,
+ coresight_timeout_cb_t cb);
extern int coresight_claim_device(struct coresight_device *csdev);
extern int coresight_claim_device_unlocked(struct coresight_device
*csdev);
> + /*
> + * As recommended by section 4.3.7 (Synchronization of register updates)
> + * of ARM IHI 0064H.b.
> + */
> + isb();
> +
> /* read the status of the single shot comparators */
> for (i = 0; i < drvdata->nr_ss_cmp; i++) {
> config->ss_status[i] =
Powered by blists - more mailing lists