| 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: <f149618c-799c-4809-9648-bd522c6fbaf0@quicinc.com>
Date: Thu, 16 Jan 2025 17:08:16 +0800
From: Yuanfang Zhang <quic_yuanfang@...cinc.com>
To: Suzuki K Poulose <suzuki.poulose@....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
On 1/15/2025 12:03 AM, Suzuki K Poulose wrote:
> 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")
>
done.
>
>> 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.
done.
>
>> 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.
>
>> 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.
>
done.
>
> 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