[PATCH 0/5] coresight: TRBE and Self-Hosted trace fixes

[PATCH 0/5] coresight: TRBE and Self-Hosted trace fixes

[Suzuki K Poulose]

This is a series of fixes addressing the issues in the way we handle
  - Self-Hosted trace filter control register for ETM/ETE
  - AUX buffer and event handling of TRBE at overflow.

The use of TRUNCATED flag on an IRQ for the TRBE driver is
something that needs to be rexamined. Please see Patch 3 for
more details.

Suzuki K Poulose (5):
  coresight: etm4x: Save restore TRFCR_EL1
  coresight: etm4x: Use Trace Filtering controls dynamically
  coresight: trbe: Keep TRBE disabled on overflow irq
  coresight: trbe: Move irq_work queue processing
  coresight: trbe: Prohibit tracing while handling an IRQ

 .../coresight/coresight-etm4x-core.c          | 109 ++++++++++++++----
 drivers/hwtracing/coresight/coresight-etm4x.h |   7 +-
 drivers/hwtracing/coresight/coresight-trbe.c  |  91 ++++++++++-----
 3 files changed, 149 insertions(+), 58 deletions(-)

-- 
2.24.1

[PATCH 1/5] coresight: etm4x: Save restore TRFCR_EL1

[Suzuki K Poulose]

When the CPU enters a low power mode, the TRFCR_EL1 contents could be
reset. Thus we need to save/restore the TRFCR_EL1 along with the ETM4x
registers to allow the tracing.

Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 .../coresight/coresight-etm4x-core.c          | 52 ++++++++++++++-----
 drivers/hwtracing/coresight/coresight-etm4x.h |  2 +
 2 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index da27cd4a3c38..6bbe4da31d9b 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -236,6 +236,17 @@ struct etm4_enable_arg {
 	int rc;
 };
 
+static inline u64 read_trfcr(void)
+{
+	return read_sysreg_s(SYS_TRFCR_EL1);
+}
+
+static inline void write_trfcr(u64 val)
+{
+	write_sysreg_s(val, SYS_TRFCR_EL1);
+	isb();
+}
+
 #ifdef CONFIG_ETM4X_IMPDEF_FEATURE
 
 #define HISI_HIP08_AMBA_ID		0x000b6d01
@@ -985,7 +996,7 @@ static void cpu_enable_tracing(struct etmv4_drvdata *drvdata)
 	if (is_kernel_in_hyp_mode())
 		trfcr |= TRFCR_EL2_CX;
 
-	write_sysreg_s(trfcr, SYS_TRFCR_EL1);
+	write_trfcr(trfcr);
 }
 
 static void etm4_init_arch_data(void *info)
@@ -1528,7 +1539,7 @@ static void etm4_init_trace_id(struct etmv4_drvdata *drvdata)
 	drvdata->trcid = coresight_get_trace_id(drvdata->cpu);
 }
 
-static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
+static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
 {
 	int i, ret = 0;
 	struct etmv4_save_state *state;
@@ -1667,7 +1678,22 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
 	return ret;
 }
 
-static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
+static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
+{
+	int ret = 0;
+
+	if (drvdata->trfc)
+		drvdata->save_trfcr = read_trfcr();
+	/*
+	 * Save and restore the ETM Trace registers only if
+	 * the ETM is active.
+	 */
+	if (local_read(&drvdata->mode) && drvdata->save_state)
+		ret = __etm4_cpu_save(drvdata);
+	return ret;
+}
+
+static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
 {
 	int i;
 	struct etmv4_save_state *state = drvdata->save_state;
@@ -1763,6 +1789,14 @@ static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
 	etm4_cs_lock(drvdata, csa);
 }
 
+static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
+{
+	if (drvdata->trfc)
+		write_trfcr(drvdata->save_trfcr);
+	if (drvdata->state_needs_restore)
+		__etm4_cpu_restore(drvdata);
+}
+
 static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
 			      void *v)
 {
@@ -1774,23 +1808,17 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
 
 	drvdata = etmdrvdata[cpu];
 
-	if (!drvdata->save_state)
-		return NOTIFY_OK;
-
 	if (WARN_ON_ONCE(drvdata->cpu != cpu))
 		return NOTIFY_BAD;
 
 	switch (cmd) {
 	case CPU_PM_ENTER:
-		/* save the state if self-hosted coresight is in use */
-		if (local_read(&drvdata->mode))
-			if (etm4_cpu_save(drvdata))
-				return NOTIFY_BAD;
+		if (etm4_cpu_save(drvdata))
+			return NOTIFY_BAD;
 		break;
 	case CPU_PM_EXIT:
 	case CPU_PM_ENTER_FAILED:
-		if (drvdata->state_needs_restore)
-			etm4_cpu_restore(drvdata);
+		etm4_cpu_restore(drvdata);
 		break;
 	default:
 		return NOTIFY_DONE;
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index e5b79bdb9851..82cba16b73a6 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -921,6 +921,7 @@ struct etmv4_save_state {
  * @lpoverride:	If the implementation can support low-power state over.
  * @trfc:	If the implementation supports Arm v8.4 trace filter controls.
  * @config:	structure holding configuration parameters.
+ * @save_trfcr:	Saved TRFCR_EL1 register during a CPU PM event.
  * @save_state:	State to be preserved across power loss
  * @state_needs_restore: True when there is context to restore after PM exit
  * @skip_power_up: Indicates if an implementation can skip powering up
@@ -973,6 +974,7 @@ struct etmv4_drvdata {
 	bool				lpoverride;
 	bool				trfc;
 	struct etmv4_config		config;
+	u64				save_trfcr;
 	struct etmv4_save_state		*save_state;
 	bool				state_needs_restore;
 	bool				skip_power_up;
-- 
2.24.1

[PATCH 2/5] coresight: etm4x: Use Trace Filtering controls dynamically

[Suzuki K Poulose]

The Trace Filtering support (FEAT_TRF) ensures that the ETM
can be prohibited from generating any trace for a given EL.
This is much stricter knob, than the TRCVICTLR exception level
masks. At the moment, we do a onetime enable trace at user and
kernel and leave it untouched for the kernel life time.

This patch makes the switch dynamic, by honoring the filters
set by the user and enforcing them in the TRFCR controls.
We also rename the cpu_enable_tracing() appropriately to
cpu_detect_trace_filtering() and the drvdata member
trfc => trfcr to indicate the "value" of the TRFCR_EL1.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Al Grant <al.grant@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 .../coresight/coresight-etm4x-core.c          | 63 ++++++++++++++-----
 drivers/hwtracing/coresight/coresight-etm4x.h |  5 +-
 2 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 6bbe4da31d9b..9b9cebae8bb5 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -247,6 +247,45 @@ static inline void write_trfcr(u64 val)
 	isb();
 }
 
+/*
+ * etm4x_prohibit_trace - Prohibit the CPU from tracing at all ELs.
+ * When the CPU supports FEAT_TRF, we could move the ETM to a trace
+ * prohibited state by filtering the Exception levels via TRFCR_EL1.
+ */
+static void etm4x_prohibit_trace(struct etmv4_drvdata *drvdata)
+{
+	if (drvdata->trfcr) {
+		u64 val = read_trfcr();
+
+		write_trfcr(val & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE));
+	}
+}
+
+/*
+ * etm4x_allow_trace - Allow CPU tracing in the respective ELs,
+ * as configured by the drvdata->config.mode for the current
+ * session. Even though we have TRCVICTLR bits to filter the
+ * trace in the ELs, it doesn't prevent the ETM from generating
+ * a packet (e.g, TraceInfo) that might contain the addresses from
+ * the excluded levels. Thus we use the additional controls provided
+ * via the Trace Filtering controls (FEAT_TRF) to make sure no trace
+ * is generated for the excluded ELs.
+ */
+static void etm4x_allow_trace(struct etmv4_drvdata *drvdata)
+{
+	u64 trfcr = drvdata->trfcr;
+
+	/* If the CPU doesn't support FEAT_TRF, nothing to do */
+	if (!trfcr)
+		return;
+
+	if (drvdata->config.mode & ETM_MODE_EXCL_KERN)
+		trfcr &= ~TRFCR_ELx_ExTRE;
+	if (drvdata->config.mode & ETM_MODE_EXCL_USER)
+		trfcr &= ~TRFCR_ELx_E0TRE;
+	write_trfcr(trfcr);
+}
+
 #ifdef CONFIG_ETM4X_IMPDEF_FEATURE
 
 #define HISI_HIP08_AMBA_ID		0x000b6d01
@@ -451,6 +490,7 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
 	if (etm4x_is_ete(drvdata))
 		etm4x_relaxed_write32(csa, TRCRSR_TA, TRCRSR);
 
+	etm4x_allow_trace(drvdata);
 	/* Enable the trace unit */
 	etm4x_relaxed_write32(csa, 1, TRCPRGCTLR);
 
@@ -729,7 +769,6 @@ static int etm4_enable(struct coresight_device *csdev,
 static void etm4_disable_hw(void *info)
 {
 	u32 control;
-	u64 trfcr;
 	struct etmv4_drvdata *drvdata = info;
 	struct etmv4_config *config = &drvdata->config;
 	struct coresight_device *csdev = drvdata->csdev;
@@ -756,12 +795,7 @@ static void etm4_disable_hw(void *info)
 	 * If the CPU supports v8.4 Trace filter Control,
 	 * set the ETM to trace prohibited region.
 	 */
-	if (drvdata->trfc) {
-		trfcr = read_sysreg_s(SYS_TRFCR_EL1);
-		write_sysreg_s(trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE),
-			       SYS_TRFCR_EL1);
-		isb();
-	}
+	etm4x_prohibit_trace(drvdata);
 	/*
 	 * Make sure everything completes before disabling, as recommended
 	 * by section 7.3.77 ("TRCVICTLR, ViewInst Main Control Register,
@@ -777,9 +811,6 @@ static void etm4_disable_hw(void *info)
 	if (coresight_timeout(csa, TRCSTATR, TRCSTATR_PMSTABLE_BIT, 1))
 		dev_err(etm_dev,
 			"timeout while waiting for PM stable Trace Status\n");
-	if (drvdata->trfc)
-		write_sysreg_s(trfcr, SYS_TRFCR_EL1);
-
 	/* read the status of the single shot comparators */
 	for (i = 0; i < drvdata->nr_ss_cmp; i++) {
 		config->ss_status[i] =
@@ -974,15 +1005,15 @@ static bool etm4_init_csdev_access(struct etmv4_drvdata *drvdata,
 	return false;
 }
 
-static void cpu_enable_tracing(struct etmv4_drvdata *drvdata)
+static void cpu_detect_trace_filtering(struct etmv4_drvdata *drvdata)
 {
 	u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
 	u64 trfcr;
 
+	drvdata->trfcr = 0;
 	if (!cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_TRACE_FILT_SHIFT))
 		return;
 
-	drvdata->trfc = true;
 	/*
 	 * If the CPU supports v8.4 SelfHosted Tracing, enable
 	 * tracing at the kernel EL and EL0, forcing to use the
@@ -996,7 +1027,7 @@ static void cpu_enable_tracing(struct etmv4_drvdata *drvdata)
 	if (is_kernel_in_hyp_mode())
 		trfcr |= TRFCR_EL2_CX;
 
-	write_trfcr(trfcr);
+	drvdata->trfcr = trfcr;
 }
 
 static void etm4_init_arch_data(void *info)
@@ -1187,7 +1218,7 @@ static void etm4_init_arch_data(void *info)
 	/* NUMCNTR, bits[30:28] number of counters available for tracing */
 	drvdata->nr_cntr = BMVAL(etmidr5, 28, 30);
 	etm4_cs_lock(drvdata, csa);
-	cpu_enable_tracing(drvdata);
+	cpu_detect_trace_filtering(drvdata);
 }
 
 static inline u32 etm4_get_victlr_access_type(struct etmv4_config *config)
@@ -1682,7 +1713,7 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
 {
 	int ret = 0;
 
-	if (drvdata->trfc)
+	if (drvdata->trfcr)
 		drvdata->save_trfcr = read_trfcr();
 	/*
 	 * Save and restore the ETM Trace registers only if
@@ -1791,7 +1822,7 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
 
 static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
 {
-	if (drvdata->trfc)
+	if (drvdata->trfcr)
 		write_trfcr(drvdata->save_trfcr);
 	if (drvdata->state_needs_restore)
 		__etm4_cpu_restore(drvdata);
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 82cba16b73a6..2a7943e2bfcd 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -919,7 +919,8 @@ struct etmv4_save_state {
  * @nooverflow:	Indicate if overflow prevention is supported.
  * @atbtrig:	If the implementation can support ATB triggers
  * @lpoverride:	If the implementation can support low-power state over.
- * @trfc:	If the implementation supports Arm v8.4 trace filter controls.
+ * @trfcr:	If the CPU supportfs FEAT_TRF, value of the TRFCR_ELx with
+ * 		trace allowed at user and kernel ELs. Otherwise, 0.
  * @config:	structure holding configuration parameters.
  * @save_trfcr:	Saved TRFCR_EL1 register during a CPU PM event.
  * @save_state:	State to be preserved across power loss
@@ -972,7 +973,7 @@ struct etmv4_drvdata {
 	bool				nooverflow;
 	bool				atbtrig;
 	bool				lpoverride;
-	bool				trfc;
+	u64				trfcr;
 	struct etmv4_config		config;
 	u64				save_trfcr;
 	struct etmv4_save_state		*save_state;
-- 
2.24.1

[PATCH 3/5] coresight: trbe: Keep TRBE disabled on overflow irq

[Suzuki K Poulose]

When an AUX buffer is marked TRUNCATED, the kernel will disable
the event (via irq_work) and can only be re-enabled by the
userspace tool.

Also, since we *always* mark the buffer as TRUNCATED (which is
needs to be reconsidered, see below), we need not re-enable the
TRBE as the event is going to be now disabled. This follows the
SPE driver behavior.

Without this change, we could leave the event disabled for
ever under certain conditions. i.e, when we try to re-enable
in the IRQ handler, if there is no space left in the buffer,
we "TRUNCATE" the buffer and create a record of size 0.
The perf tool skips such records and the event will remain
disabled forever.

Regarding the use of TRUNCATED flag:
With FILL mode, the TRBE stops collection when the buffer is
full, raising the IRQ. Now, since the IRQ is asynchronous,
we could loose some amount of trace, after the buffer was
full until the IRQ is handled. Also the last packet may
have been trimmed. The decoder can figure this out and
cope with this. The side effect of this approach is:

 We always disable the event when there is an IRQ, even
 when the other half of the ring buffer is free ! This
 is not ideal.

Now, we should switch to using PARTIAL to indicate that there
was potentially some partial trace packets in the buffer and
some data was lost. We should use TRUNCATED only when there
is absolutely no space in the ring buffer. This change would
also require some additional changes in the CoreSight PMU
framework to allow, sinks to "close" the handle (rather
than the PMU driver closing the handle upon event_stop).
So, until that is sorted, let us keep the TRUNCATED flag
and the rework can be addressed separately.

Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
Reported-by: Tamas Zsoldos <Tamas.Zsoldos@arm.com>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-trbe.c | 34 +++++++-------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 176868496879..ec38cf17b693 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -696,10 +696,8 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
 
 static void trbe_handle_overflow(struct perf_output_handle *handle)
 {
-	struct perf_event *event = handle->event;
 	struct trbe_buf *buf = etm_perf_sink_config(handle);
 	unsigned long offset, size;
-	struct etm_event_data *event_data;
 
 	offset = get_trbe_limit_pointer() - get_trbe_base_pointer();
 	size = offset - PERF_IDX2OFF(handle->head, buf);
@@ -709,30 +707,22 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
 	/*
 	 * Mark the buffer as truncated, as we have stopped the trace
 	 * collection upon the WRAP event, without stopping the source.
+	 *
+	 * We don't re-enable the TRBE here, as the event is
+	 * bound to be disabled due to the TRUNCATED flag.
+	 * This is not ideal, as we could use the available space in
+	 * the ring buffer and continue the tracing.
+	 *
+	 * TODO: Revisit the use of TRUNCATED flag and may be instead use
+	 * PARTIAL, to indicate trace may contain partial packets.
+	 * And TRUNCATED can be used only if we do not have enough space
+	 * in the buffer. This would need additional changes in
+	 * etm_event_stop() to allow the sinks to leave a closed
+	 * aux_handle.
 	 */
 	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW |
 				     PERF_AUX_FLAG_TRUNCATED);
 	perf_aux_output_end(handle, size);
-	event_data = perf_aux_output_begin(handle, event);
-	if (!event_data) {
-		/*
-		 * We are unable to restart the trace collection,
-		 * thus leave the TRBE disabled. The etm-perf driver
-		 * is able to detect this with a disconnected handle
-		 * (handle->event = NULL).
-		 */
-		trbe_drain_and_disable_local();
-		*this_cpu_ptr(buf->cpudata->drvdata->handle) = NULL;
-		return;
-	}
-	buf->trbe_limit = compute_trbe_buffer_limit(handle);
-	buf->trbe_write = buf->trbe_base + PERF_IDX2OFF(handle->head, buf);
-	if (buf->trbe_limit == buf->trbe_base) {
-		trbe_stop_and_truncate_event(handle);
-		return;
-	}
-	*this_cpu_ptr(buf->cpudata->drvdata->handle) = handle;
-	trbe_enable_hw(buf);
 }
 
 static bool is_perf_trbe(struct perf_output_handle *handle)
-- 
2.24.1

Re: [PATCH 3/5] coresight: trbe: Keep TRBE disabled on overflow irq

[Anshuman Khandual]

On 7/12/21 5:08 PM, Suzuki K Poulose wrote:
> When an AUX buffer is marked TRUNCATED, the kernel will disable
> the event (via irq_work) and can only be re-enabled by the
> userspace tool.

Will it be renabled via normal perf event enable callback OR an
explicit perf event restart is required upon the TRUNCATED flag ?

> 
> Also, since we *always* mark the buffer as TRUNCATED (which is
> needs to be reconsidered, see below), we need not re-enable the
> TRBE as the event is going to be now disabled. This follows the
> SPE driver behavior.
> 
> Without this change, we could leave the event disabled for
> ever under certain conditions. i.e, when we try to re-enable
> in the IRQ handler, if there is no space left in the buffer,
> we "TRUNCATE" the buffer and create a record of size 0.
> The perf tool skips such records and the event will remain
> disabled forever.

Why ? Should not the user space tool explicitly start back the
tracing event after detecting zero sized record with buffer
marked with TRUNCATE flag ? What prevents it to restart the
event ?

> 
> Regarding the use of TRUNCATED flag:
> With FILL mode, the TRBE stops collection when the buffer is
> full, raising the IRQ. Now, since the IRQ is asynchronous,
> we could loose some amount of trace, after the buffer was
> full until the IRQ is handled. Also the last packet may
> have been trimmed. The decoder can figure this out and
> cope with this. The side effect of this approach is:
> 
>  We always disable the event when there is an IRQ, even
>  when the other half of the ring buffer is free ! This
>  is not ideal.
> 
> Now, we should switch to using PARTIAL to indicate that there
> was potentially some partial trace packets in the buffer and
> some data was lost. We should use TRUNCATED only when there
> is absolutely no space in the ring buffer. This change would
> also require some additional changes in the CoreSight PMU
> framework to allow, sinks to "close" the handle (rather
> than the PMU driver closing the handle upon event_stop).
> So, until that is sorted, let us keep the TRUNCATED flag
> and the rework can be addressed separately.

But I guess this is a separate problem all together.

> 
> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
> Reported-by: Tamas Zsoldos <Tamas.Zsoldos@arm.com>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 34 +++++++-------------
>  1 file changed, 12 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 176868496879..ec38cf17b693 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -696,10 +696,8 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
>  
>  static void trbe_handle_overflow(struct perf_output_handle *handle)
>  {
> -	struct perf_event *event = handle->event;
>  	struct trbe_buf *buf = etm_perf_sink_config(handle);
>  	unsigned long offset, size;
> -	struct etm_event_data *event_data;
>  
>  	offset = get_trbe_limit_pointer() - get_trbe_base_pointer();
>  	size = offset - PERF_IDX2OFF(handle->head, buf);
> @@ -709,30 +707,22 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
>  	/*
>  	 * Mark the buffer as truncated, as we have stopped the trace
>  	 * collection upon the WRAP event, without stopping the source.
> +	 *
> +	 * We don't re-enable the TRBE here, as the event is
> +	 * bound to be disabled due to the TRUNCATED flag.
> +	 * This is not ideal, as we could use the available space in
> +	 * the ring buffer and continue the tracing.
> +	 *
> +	 * TODO: Revisit the use of TRUNCATED flag and may be instead use
> +	 * PARTIAL, to indicate trace may contain partial packets.
> +	 * And TRUNCATED can be used only if we do not have enough space
> +	 * in the buffer. This would need additional changes in
> +	 * etm_event_stop() to allow the sinks to leave a closed
> +	 * aux_handle.
>  	 */
>  	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW |
>  				     PERF_AUX_FLAG_TRUNCATED);
>  	perf_aux_output_end(handle, size);
> -	event_data = perf_aux_output_begin(handle, event);
> -	if (!event_data) {
> -		/*
> -		 * We are unable to restart the trace collection,
> -		 * thus leave the TRBE disabled. The etm-perf driver
> -		 * is able to detect this with a disconnected handle
> -		 * (handle->event = NULL).
> -		 */
> -		trbe_drain_and_disable_local();
> -		*this_cpu_ptr(buf->cpudata->drvdata->handle) = NULL;
> -		return;
> -	}
> -	buf->trbe_limit = compute_trbe_buffer_limit(handle);
> -	buf->trbe_write = buf->trbe_base + PERF_IDX2OFF(handle->head, buf);
> -	if (buf->trbe_limit == buf->trbe_base) {
> -		trbe_stop_and_truncate_event(handle);
> -		return;
> -	}
> -	*this_cpu_ptr(buf->cpudata->drvdata->handle) = handle;
> -	trbe_enable_hw(buf);
>  }

The change here just stops the event restart after handling the IRQ
and marking the buffer with PERF_AUX_FLAG_TRUNCATED which helps the
event from being disabled for ever (still need to understand how !).

I guess it might be better to separate out the problems with using
PERF_AUX_FLAG_TRUNCATED and related aspects, in a subsequent patch
which just updates the code with the above TODO comment ?

>  
>  static bool is_perf_trbe(struct perf_output_handle *handle)
> 

Re: [PATCH 3/5] coresight: trbe: Keep TRBE disabled on overflow irq

[Suzuki K Poulose]

On 15/07/2021 04:54, Anshuman Khandual wrote:
> 
> 
> On 7/12/21 5:08 PM, Suzuki K Poulose wrote:
>> When an AUX buffer is marked TRUNCATED, the kernel will disable
>> the event (via irq_work) and can only be re-enabled by the
>> userspace tool.
> 
> Will it be renabled via normal perf event enable callback OR an
> explicit perf event restart is required upon the TRUNCATED flag ?

The perf event moves to a DISABLED state. At this state an
explicit restart from the userspace tool is required, via
PERF_EVENT_IOC_ENABLE.

> 
>>
>> Also, since we *always* mark the buffer as TRUNCATED (which is
>> needs to be reconsidered, see below), we need not re-enable the
>> TRBE as the event is going to be now disabled. This follows the
>> SPE driver behavior.
>>
>> Without this change, we could leave the event disabled for
>> ever under certain conditions. i.e, when we try to re-enable
>> in the IRQ handler, if there is no space left in the buffer,
>> we "TRUNCATE" the buffer and create a record of size 0.
>> The perf tool skips such records and the event will remain
>> disabled forever.
> 
> Why ? Should not the user space tool explicitly start back the
> tracing event after detecting zero sized record with buffer
> marked with TRUNCATE flag ? What prevents it to restart the
> event ?

The perf tool discards a 0 sized packet. While I agree that
this is something that should be fixed in perf tool too, this
deviation from the "expected driver" behavior (see SPE driver
which this one was inspired from) will break the existing
perf tools (not an ABI break, but a functional issue
which is not nice and generally discouraged in the kernel).

> 
>>
>> Regarding the use of TRUNCATED flag:
>> With FILL mode, the TRBE stops collection when the buffer is
>> full, raising the IRQ. Now, since the IRQ is asynchronous,
>> we could loose some amount of trace, after the buffer was
>> full until the IRQ is handled. Also the last packet may
>> have been trimmed. The decoder can figure this out and
>> cope with this. The side effect of this approach is:
>>
>>   We always disable the event when there is an IRQ, even
>>   when the other half of the ring buffer is free ! This
>>   is not ideal.
>>
>> Now, we should switch to using PARTIAL to indicate that there
>> was potentially some partial trace packets in the buffer and
>> some data was lost. We should use TRUNCATED only when there
>> is absolutely no space in the ring buffer. This change would
>> also require some additional changes in the CoreSight PMU
>> framework to allow, sinks to "close" the handle (rather
>> than the PMU driver closing the handle upon event_stop).
>> So, until that is sorted, let us keep the TRUNCATED flag
>> and the rework can be addressed separately.
> 
> But I guess this is a separate problem all together.

Yes, it is. As mentioned above, we need changes to the
coresight PMU framework to be able to use the available
buffer. And the message already is clear, that this
is fixing the "odd" behavior.

> 
>>
>> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
>> Reported-by: Tamas Zsoldos <Tamas.Zsoldos@arm.com>
>> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: Leo Yan <leo.yan@linaro.org>
>> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
>> Cc: Will Deacon <will@kernel.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-trbe.c | 34 +++++++-------------
>>   1 file changed, 12 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>> index 176868496879..ec38cf17b693 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -696,10 +696,8 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
>>   
>>   static void trbe_handle_overflow(struct perf_output_handle *handle)
>>   {
>> -	struct perf_event *event = handle->event;
>>   	struct trbe_buf *buf = etm_perf_sink_config(handle);
>>   	unsigned long offset, size;
>> -	struct etm_event_data *event_data;
>>   
>>   	offset = get_trbe_limit_pointer() - get_trbe_base_pointer();
>>   	size = offset - PERF_IDX2OFF(handle->head, buf);
>> @@ -709,30 +707,22 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
>>   	/*
>>   	 * Mark the buffer as truncated, as we have stopped the trace
>>   	 * collection upon the WRAP event, without stopping the source.
>> +	 *
>> +	 * We don't re-enable the TRBE here, as the event is
>> +	 * bound to be disabled due to the TRUNCATED flag.
>> +	 * This is not ideal, as we could use the available space in
>> +	 * the ring buffer and continue the tracing.
>> +	 *
>> +	 * TODO: Revisit the use of TRUNCATED flag and may be instead use
>> +	 * PARTIAL, to indicate trace may contain partial packets.
>> +	 * And TRUNCATED can be used only if we do not have enough space
>> +	 * in the buffer. This would need additional changes in
>> +	 * etm_event_stop() to allow the sinks to leave a closed
>> +	 * aux_handle.
>>   	 */
>>   	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW |
>>   				     PERF_AUX_FLAG_TRUNCATED);
>>   	perf_aux_output_end(handle, size);
>> -	event_data = perf_aux_output_begin(handle, event);
>> -	if (!event_data) {
>> -		/*
>> -		 * We are unable to restart the trace collection,
>> -		 * thus leave the TRBE disabled. The etm-perf driver
>> -		 * is able to detect this with a disconnected handle
>> -		 * (handle->event = NULL).
>> -		 */
>> -		trbe_drain_and_disable_local();
>> -		*this_cpu_ptr(buf->cpudata->drvdata->handle) = NULL;
>> -		return;
>> -	}
>> -	buf->trbe_limit = compute_trbe_buffer_limit(handle);
>> -	buf->trbe_write = buf->trbe_base + PERF_IDX2OFF(handle->head, buf);
>> -	if (buf->trbe_limit == buf->trbe_base) {
>> -		trbe_stop_and_truncate_event(handle);
>> -		return;
>> -	}
>> -	*this_cpu_ptr(buf->cpudata->drvdata->handle) = handle;
>> -	trbe_enable_hw(buf);
>>   }
> 
> The change here just stops the event restart after handling the IRQ
> and marking the buffer with PERF_AUX_FLAG_TRUNCATED which helps the
> event from being disabled for ever (still need to understand how !).

The real issue is unnecessary starting of the event with new buffer
after we have "TRUNCATED" the buffer. This can lead to occassionally
hitting "0" sized buffer, because the "irq_work_run()" could kick
in and disable the event, leaving no trace generated (because we
were tracing only userspace). So, we now have a 0 sized record
with the event in disabled state, which the perf tool ignores.

> 
> I guess it might be better to separate out the problems with using
> PERF_AUX_FLAG_TRUNCATED and related aspects, in a subsequent patch
> which just updates the code with the above TODO comment ?

The problems with the driver using TRUNCATED flag must be addressed
in a different patch. This patch is only to comply with the behavior,
of "TRUNCATED" indicates the event is disabled. So do not start a
session.

Does that help ?

Suzuki

Re: [PATCH 3/5] coresight: trbe: Keep TRBE disabled on overflow irq

[Anshuman Khandual]

On 7/15/21 1:58 PM, Suzuki K Poulose wrote:
> On 15/07/2021 04:54, Anshuman Khandual wrote:
>>
>>
>> On 7/12/21 5:08 PM, Suzuki K Poulose wrote:
>>> When an AUX buffer is marked TRUNCATED, the kernel will disable
>>> the event (via irq_work) and can only be re-enabled by the
>>> userspace tool.
>>
>> Will it be renabled via normal perf event enable callback OR an
>> explicit perf event restart is required upon the TRUNCATED flag ?
> 
> The perf event moves to a DISABLED state. At this state an
> explicit restart from the userspace tool is required, via
> PERF_EVENT_IOC_ENABLE.

Got it.

> 
>>
>>>
>>> Also, since we *always* mark the buffer as TRUNCATED (which is
>>> needs to be reconsidered, see below), we need not re-enable the
>>> TRBE as the event is going to be now disabled. This follows the
>>> SPE driver behavior.
>>>
>>> Without this change, we could leave the event disabled for
>>> ever under certain conditions. i.e, when we try to re-enable
>>> in the IRQ handler, if there is no space left in the buffer,
>>> we "TRUNCATE" the buffer and create a record of size 0.
>>> The perf tool skips such records and the event will remain
>>> disabled forever.
>>
>> Why ? Should not the user space tool explicitly start back the
>> tracing event after detecting zero sized record with buffer
>> marked with TRUNCATE flag ? What prevents it to restart the
>> event ?
> 
> The perf tool discards a 0 sized packet. While I agree that
> this is something that should be fixed in perf tool too, this
> deviation from the "expected driver" behavior (see SPE driver
> which this one was inspired from) will break the existing
> perf tools (not an ABI break, but a functional issue
> which is not nice and generally discouraged in the kernel).

Makes sense not to cause deviation from the expected driver behaviour
for now, though perf tool should eventually accommodate a zero sized
trace record in the buffer and restart the event.

> 
>>
>>>
>>> Regarding the use of TRUNCATED flag:
>>> With FILL mode, the TRBE stops collection when the buffer is
>>> full, raising the IRQ. Now, since the IRQ is asynchronous,
>>> we could loose some amount of trace, after the buffer was
>>> full until the IRQ is handled. Also the last packet may
>>> have been trimmed. The decoder can figure this out and
>>> cope with this. The side effect of this approach is:
>>>
>>>   We always disable the event when there is an IRQ, even
>>>   when the other half of the ring buffer is free ! This
>>>   is not ideal.
>>>
>>> Now, we should switch to using PARTIAL to indicate that there
>>> was potentially some partial trace packets in the buffer and
>>> some data was lost. We should use TRUNCATED only when there
>>> is absolutely no space in the ring buffer. This change would
>>> also require some additional changes in the CoreSight PMU
>>> framework to allow, sinks to "close" the handle (rather
>>> than the PMU driver closing the handle upon event_stop).
>>> So, until that is sorted, let us keep the TRUNCATED flag
>>> and the rework can be addressed separately.
>>
>> But I guess this is a separate problem all together.
> 
> Yes, it is. As mentioned above, we need changes to the
> coresight PMU framework to be able to use the available
> buffer. And the message already is clear, that this
> is fixing the "odd" behavior.

Got it.

> 
>>
>>>
>>> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
>>> Reported-by: Tamas Zsoldos <Tamas.Zsoldos@arm.com>
>>> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Cc: Mike Leach <mike.leach@linaro.org>
>>> Cc: Leo Yan <leo.yan@linaro.org>
>>> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
>>> Cc: Will Deacon <will@kernel.org>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-trbe.c | 34 +++++++-------------
>>>   1 file changed, 12 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>>> index 176868496879..ec38cf17b693 100644
>>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>>> @@ -696,10 +696,8 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
>>>     static void trbe_handle_overflow(struct perf_output_handle *handle)
>>>   {
>>> -    struct perf_event *event = handle->event;
>>>       struct trbe_buf *buf = etm_perf_sink_config(handle);
>>>       unsigned long offset, size;
>>> -    struct etm_event_data *event_data;
>>>         offset = get_trbe_limit_pointer() - get_trbe_base_pointer();
>>>       size = offset - PERF_IDX2OFF(handle->head, buf);
>>> @@ -709,30 +707,22 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
>>>       /*
>>>        * Mark the buffer as truncated, as we have stopped the trace
>>>        * collection upon the WRAP event, without stopping the source.
>>> +     *
>>> +     * We don't re-enable the TRBE here, as the event is
>>> +     * bound to be disabled due to the TRUNCATED flag.
>>> +     * This is not ideal, as we could use the available space in
>>> +     * the ring buffer and continue the tracing.
>>> +     *
>>> +     * TODO: Revisit the use of TRUNCATED flag and may be instead use
>>> +     * PARTIAL, to indicate trace may contain partial packets.
>>> +     * And TRUNCATED can be used only if we do not have enough space
>>> +     * in the buffer. This would need additional changes in
>>> +     * etm_event_stop() to allow the sinks to leave a closed
>>> +     * aux_handle.
>>>        */
>>>       perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW |
>>>                        PERF_AUX_FLAG_TRUNCATED);
>>>       perf_aux_output_end(handle, size);
>>> -    event_data = perf_aux_output_begin(handle, event);
>>> -    if (!event_data) {
>>> -        /*
>>> -         * We are unable to restart the trace collection,
>>> -         * thus leave the TRBE disabled. The etm-perf driver
>>> -         * is able to detect this with a disconnected handle
>>> -         * (handle->event = NULL).
>>> -         */
>>> -        trbe_drain_and_disable_local();
>>> -        *this_cpu_ptr(buf->cpudata->drvdata->handle) = NULL;
>>> -        return;
>>> -    }
>>> -    buf->trbe_limit = compute_trbe_buffer_limit(handle);
>>> -    buf->trbe_write = buf->trbe_base + PERF_IDX2OFF(handle->head, buf);
>>> -    if (buf->trbe_limit == buf->trbe_base) {
>>> -        trbe_stop_and_truncate_event(handle);
>>> -        return;
>>> -    }
>>> -    *this_cpu_ptr(buf->cpudata->drvdata->handle) = handle;
>>> -    trbe_enable_hw(buf);
>>>   }
>>
>> The change here just stops the event restart after handling the IRQ
>> and marking the buffer with PERF_AUX_FLAG_TRUNCATED which helps the
>> event from being disabled for ever (still need to understand how !).
> 
> The real issue is unnecessary starting of the event with new buffer
> after we have "TRUNCATED" the buffer. This can lead to occassionally
> hitting "0" sized buffer, because the "irq_work_run()" could kick
> in and disable the event, leaving no trace generated (because we
> were tracing only userspace). So, we now have a 0 sized record
> with the event in disabled state, which the perf tool ignores.

Got it.

> 
>>
>> I guess it might be better to separate out the problems with using
>> PERF_AUX_FLAG_TRUNCATED and related aspects, in a subsequent patch
>> which just updates the code with the above TODO comment ?
> 
> The problems with the driver using TRUNCATED flag must be addressed
> in a different patch. This patch is only to comply with the behavior,
> of "TRUNCATED" indicates the event is disabled. So do not start a
> session.
> 
> Does that help ?

Yes but I was just suggesting about listing out the problems with current
PERF_AUX_FLAG_TRUNCATED and possible usage of PERF_AUX_FLAG_PARTIAL instead,
along with the comments (as proposed here in this patch) in a separate patch
without any code change. This might help keep this patch's objective clear.

Re: [PATCH 3/5] coresight: trbe: Keep TRBE disabled on overflow irq

[Suzuki K Poulose]

On 12/07/2021 12:38, Suzuki K Poulose wrote:
> When an AUX buffer is marked TRUNCATED, the kernel will disable
> the event (via irq_work) and can only be re-enabled by the
> userspace tool.
> 
> Also, since we *always* mark the buffer as TRUNCATED (which is
> needs to be reconsidered, see below), we need not re-enable the
> TRBE as the event is going to be now disabled. This follows the
> SPE driver behavior.
> 
> Without this change, we could leave the event disabled for
> ever under certain conditions. i.e, when we try to re-enable
> in the IRQ handler, if there is no space left in the buffer,
> we "TRUNCATE" the buffer and create a record of size 0.
> The perf tool skips such records and the event will remain
> disabled forever.
> 
> Regarding the use of TRUNCATED flag:
> With FILL mode, the TRBE stops collection when the buffer is
> full, raising the IRQ. Now, since the IRQ is asynchronous,
> we could loose some amount of trace, after the buffer was
> full until the IRQ is handled. Also the last packet may
> have been trimmed. The decoder can figure this out and
> cope with this. The side effect of this approach is:
> 
>   We always disable the event when there is an IRQ, even
>   when the other half of the ring buffer is free ! This
>   is not ideal.
> 
> Now, we should switch to using PARTIAL to indicate that there
> was potentially some partial trace packets in the buffer and
> some data was lost. We should use TRUNCATED only when there
> is absolutely no space in the ring buffer. This change would
> also require some additional changes in the CoreSight PMU
> framework to allow, sinks to "close" the handle (rather
> than the PMU driver closing the handle upon event_stop).
> So, until that is sorted, let us keep the TRUNCATED flag
> and the rework can be addressed separately.

Based on an offline discussion, we have decided to drop the
TRUNCATED flag and use PARTIAL to indicate the IRQ fired
and some trace packets may be trimmed. So, I will rework
the fix to accommodate these changes and resend the series.
Please ignore the patches 3-5 in the series.

The first two patches are still valid, but I will send them
in as separate for ease of merging.

Kind regards
Suzuki


> 
> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
> Reported-by: Tamas Zsoldos <Tamas.Zsoldos@arm.com>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-trbe.c | 34 +++++++-------------
>   1 file changed, 12 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 176868496879..ec38cf17b693 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -696,10 +696,8 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
>   
>   static void trbe_handle_overflow(struct perf_output_handle *handle)
>   {
> -	struct perf_event *event = handle->event;
>   	struct trbe_buf *buf = etm_perf_sink_config(handle);
>   	unsigned long offset, size;
> -	struct etm_event_data *event_data;
>   
>   	offset = get_trbe_limit_pointer() - get_trbe_base_pointer();
>   	size = offset - PERF_IDX2OFF(handle->head, buf);
> @@ -709,30 +707,22 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
>   	/*
>   	 * Mark the buffer as truncated, as we have stopped the trace
>   	 * collection upon the WRAP event, without stopping the source.
> +	 *
> +	 * We don't re-enable the TRBE here, as the event is
> +	 * bound to be disabled due to the TRUNCATED flag.
> +	 * This is not ideal, as we could use the available space in
> +	 * the ring buffer and continue the tracing.
> +	 *
> +	 * TODO: Revisit the use of TRUNCATED flag and may be instead use
> +	 * PARTIAL, to indicate trace may contain partial packets.
> +	 * And TRUNCATED can be used only if we do not have enough space
> +	 * in the buffer. This would need additional changes in
> +	 * etm_event_stop() to allow the sinks to leave a closed
> +	 * aux_handle.
>   	 */
>   	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW |
>   				     PERF_AUX_FLAG_TRUNCATED);
>   	perf_aux_output_end(handle, size);
> -	event_data = perf_aux_output_begin(handle, event);
> -	if (!event_data) {
> -		/*
> -		 * We are unable to restart the trace collection,
> -		 * thus leave the TRBE disabled. The etm-perf driver
> -		 * is able to detect this with a disconnected handle
> -		 * (handle->event = NULL).
> -		 */
> -		trbe_drain_and_disable_local();
> -		*this_cpu_ptr(buf->cpudata->drvdata->handle) = NULL;
> -		return;
> -	}
> -	buf->trbe_limit = compute_trbe_buffer_limit(handle);
> -	buf->trbe_write = buf->trbe_base + PERF_IDX2OFF(handle->head, buf);
> -	if (buf->trbe_limit == buf->trbe_base) {
> -		trbe_stop_and_truncate_event(handle);
> -		return;
> -	}
> -	*this_cpu_ptr(buf->cpudata->drvdata->handle) = handle;
> -	trbe_enable_hw(buf);
>   }
>   
>   static bool is_perf_trbe(struct perf_output_handle *handle)
> 

[PATCH 4/5] coresight: trbe: Move irq_work queue processing

[Suzuki K Poulose]

The TRBE driver issues the irq_work_run() to ensure that
any pending disable event callback has been processed. But this
is done before we mark the AUX buffer as TRUNCATED, making
the call pointless. Move the call after the current handle
has been closed.

Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
Reported-by: Tamas Zsoldos <Tamas.Zsoldos@arm.com>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-trbe.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index ec38cf17b693..c0c264264427 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -723,6 +723,14 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
 	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW |
 				     PERF_AUX_FLAG_TRUNCATED);
 	perf_aux_output_end(handle, size);
+
+	/*
+	 * Ensure perf callbacks have completed. Since we
+	 * always TRUNCATE the buffer on IRQ, the event
+	 * is scheduled to be disabled. Make sure that happens
+	 * as soon as possible.
+	 */
+	irq_work_run();
 }
 
 static bool is_perf_trbe(struct perf_output_handle *handle)
@@ -777,12 +785,6 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
 	if (!is_perf_trbe(handle))
 		return IRQ_NONE;
 
-	/*
-	 * Ensure perf callbacks have completed, which may disable
-	 * the trace buffer in response to a TRUNCATION flag.
-	 */
-	irq_work_run();
-
 	act = trbe_get_fault_act(status);
 	switch (act) {
 	case TRBE_FAULT_ACT_WRAP:
-- 
2.24.1

Re: [PATCH 4/5] coresight: trbe: Move irq_work queue processing

[Anshuman Khandual]

On 7/12/21 5:08 PM, Suzuki K Poulose wrote:
> The TRBE driver issues the irq_work_run() to ensure that
> any pending disable event callback has been processed. But this
> is done before we mark the AUX buffer as TRUNCATED, making
> the call pointless. Move the call after the current handle
> has been closed.

So there is a possibility that a disable event gets missed before
the buffer is marked TRUNCATED ? But even then would not another
disable event be triggered again subsequently ? Just trying to
understand what is the actual problem because of the current
placement of irq_work_run() ?

> 
> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
> Reported-by: Tamas Zsoldos <Tamas.Zsoldos@arm.com>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index ec38cf17b693..c0c264264427 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -723,6 +723,14 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
>  	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW |
>  				     PERF_AUX_FLAG_TRUNCATED);
>  	perf_aux_output_end(handle, size);
> +
> +	/*
> +	 * Ensure perf callbacks have completed. Since we
> +	 * always TRUNCATE the buffer on IRQ, the event
> +	 * is scheduled to be disabled. Make sure that happens
> +	 * as soon as possible.
> +	 */
> +	irq_work_run();
>  }
>  
>  static bool is_perf_trbe(struct perf_output_handle *handle)
> @@ -777,12 +785,6 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>  	if (!is_perf_trbe(handle))
>  		return IRQ_NONE;
>  
> -	/*
> -	 * Ensure perf callbacks have completed, which may disable
> -	 * the trace buffer in response to a TRUNCATION flag.
> -	 */
> -	irq_work_run();
> -
>  	act = trbe_get_fault_act(status);
>  	switch (act) {
>  	case TRBE_FAULT_ACT_WRAP:
> 

Re: [PATCH 4/5] coresight: trbe: Move irq_work queue processing

[Suzuki K Poulose]

On 15/07/2021 04:23, Anshuman Khandual wrote:
> 
> 
> On 7/12/21 5:08 PM, Suzuki K Poulose wrote:
>> The TRBE driver issues the irq_work_run() to ensure that
>> any pending disable event callback has been processed. But this
>> is done before we mark the AUX buffer as TRUNCATED, making
>> the call pointless. Move the call after the current handle
>> has been closed.
> 
> So there is a possibility that a disable event gets missed before
> the buffer is marked TRUNCATED ? But even then would not another

No, it is the other way around. A disable event is triggered
in response to the TRUNCATED flag.

> disable event be triggered again subsequently ? Just trying to
> understand what is the actual problem because of the current
> placement of irq_work_run() ?

That begs the question, What is the purpose of irq_work_run() ?
See below.

> 
>>
>> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
>> Reported-by: Tamas Zsoldos <Tamas.Zsoldos@arm.com>
>> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
>> Cc: Leo Yan <leo.yan@linaro.org>
>> Cc: Will Deacon <will@kernel.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-trbe.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>> index ec38cf17b693..c0c264264427 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -723,6 +723,14 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
>>   	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW |
>>   				     PERF_AUX_FLAG_TRUNCATED);
>>   	perf_aux_output_end(handle, size);
>> +
>> +	/*
>> +	 * Ensure perf callbacks have completed. Since we
>> +	 * always TRUNCATE the buffer on IRQ, the event
>> +	 * is scheduled to be disabled. Make sure that happens
>> +	 * as soon as possible.
>> +	 */
>> +	irq_work_run();
>>   }
>>   
>>   static bool is_perf_trbe(struct perf_output_handle *handle)
>> @@ -777,12 +785,6 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>>   	if (!is_perf_trbe(handle))
>>   		return IRQ_NONE;
>>   
>> -	/*
>> -	 * Ensure perf callbacks have completed, which may disable
>> -	 * the trace buffer in response to a TRUNCATION flag.
>> -	 */

This comment here explains what the irq_work_run() is supposed to do.
This clearly indicates that the event will be disabled in response
to TRUNCATION. Now, we haven't updated the buffer flags yet here.
It happens *after* this call, in handle_overflow(). So, this is placed
incorrectly. The purpose of this patch is to make sure that we actually
complete the callbacks in response to the TRUNCATED flag we set with
this IRQ.

>> -	irq_work_run();
>> -
>>   	act = trbe_get_fault_act(status);
>>   	switch (act) {
>>   	case TRBE_FAULT_ACT_WRAP:
>>

Suzuki

Re: [PATCH 4/5] coresight: trbe: Move irq_work queue processing

[Anshuman Khandual]

On 7/15/21 2:03 PM, Suzuki K Poulose wrote:
> On 15/07/2021 04:23, Anshuman Khandual wrote:
>>
>>
>> On 7/12/21 5:08 PM, Suzuki K Poulose wrote:
>>> The TRBE driver issues the irq_work_run() to ensure that
>>> any pending disable event callback has been processed. But this
>>> is done before we mark the AUX buffer as TRUNCATED, making
>>> the call pointless. Move the call after the current handle
>>> has been closed.
>>
>> So there is a possibility that a disable event gets missed before
>> the buffer is marked TRUNCATED ? But even then would not another
> 
> No, it is the other way around. A disable event is triggered
> in response to the TRUNCATED flag.
> 
>> disable event be triggered again subsequently ? Just trying to
>> understand what is the actual problem because of the current
>> placement of irq_work_run() ?
> 
> That begs the question, What is the purpose of irq_work_run() ?
> See below.
> 
>>
>>>
>>> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
>>> Reported-by: Tamas Zsoldos <Tamas.Zsoldos@arm.com>
>>> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Cc: Mike Leach <mike.leach@linaro.org>
>>> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
>>> Cc: Leo Yan <leo.yan@linaro.org>
>>> Cc: Will Deacon <will@kernel.org>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-trbe.c | 14 ++++++++------
>>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>>> index ec38cf17b693..c0c264264427 100644
>>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>>> @@ -723,6 +723,14 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
>>>       perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW |
>>>                        PERF_AUX_FLAG_TRUNCATED);
>>>       perf_aux_output_end(handle, size);
>>> +
>>> +    /*
>>> +     * Ensure perf callbacks have completed. Since we
>>> +     * always TRUNCATE the buffer on IRQ, the event
>>> +     * is scheduled to be disabled. Make sure that happens
>>> +     * as soon as possible.
>>> +     */
>>> +    irq_work_run();
>>>   }
>>>     static bool is_perf_trbe(struct perf_output_handle *handle)
>>> @@ -777,12 +785,6 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>>>       if (!is_perf_trbe(handle))
>>>           return IRQ_NONE;
>>>   -    /*
>>> -     * Ensure perf callbacks have completed, which may disable
>>> -     * the trace buffer in response to a TRUNCATION flag.
>>> -     */
> 
> This comment here explains what the irq_work_run() is supposed to do.
> This clearly indicates that the event will be disabled in response
> to TRUNCATION. Now, we haven't updated the buffer flags yet here.
> It happens *after* this call, in handle_overflow(). So, this is placed
> incorrectly. The purpose of this patch is to make sure that we actually
> complete the callbacks in response to the TRUNCATED flag we set with
> this IRQ.

Got it.

> 
>>> -    irq_work_run();
>>> -
>>>       act = trbe_get_fault_act(status);
>>>       switch (act) {
>>>       case TRBE_FAULT_ACT_WRAP:
>>>
> 
> Suzuki
> 

[PATCH 5/5] coresight: trbe: Prohibit tracing while handling an IRQ

[Suzuki K Poulose]

When the TRBE generates an IRQ, we stop the TRBE, collect the trace
and then reprogram the TRBE with the updated buffer pointers in case
of a spurious IRQ. We might also leave the TRBE disabled, on an
overflow interrupt, without touching the ETE. This means the
the ETE is only disabled when the event is disabled later (via irq_work).
This is incorrect, as the ETE trace is still ON without actually being
captured and may be routed to the ATB.

So, we move the CPU into trace prohibited state (for all exception
levels) upon entering the IRQ handler. The state is restored before
enabling the TRBE back. Otherwise the trace remains prohibited.
Since, the ETM/ETE driver controls the TRFCR_EL1 per session,
(from commit "coresight: etm4x: Use Trace Filtering controls dynamically")
the tracing can be restored/enabled back when the event is rescheduled
in.

Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-trbe.c | 43 ++++++++++++++++++--
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index c0c264264427..e4d88e0de2a8 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -83,6 +83,31 @@ struct trbe_drvdata {
 	struct platform_device *pdev;
 };
 
+static inline void write_trfcr(u64 val)
+{
+	write_sysreg_s(val, SYS_TRFCR_EL1);
+	isb();
+}
+
+/*
+ * Prohibit the CPU tracing at all ELs, in preparation to collect
+ * the trace buffer.
+ *
+ * Returns the original value of the trfcr for restoring later.
+ */
+static u64 cpu_prohibit_tracing(void)
+{
+	u64 trfcr = read_sysreg_s(SYS_TRFCR_EL1);
+
+	write_trfcr(trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE));
+	return trfcr;
+}
+
+static void cpu_restore_tracing(u64 trfcr)
+{
+	write_trfcr(trfcr);
+}
+
 static int trbe_alloc_node(struct perf_event *event)
 {
 	if (event->cpu == -1)
@@ -681,7 +706,7 @@ static int arm_trbe_disable(struct coresight_device *csdev)
 	return 0;
 }
 
-static void trbe_handle_spurious(struct perf_output_handle *handle)
+static void trbe_handle_spurious(struct perf_output_handle *handle, u64 trfcr)
 {
 	struct trbe_buf *buf = etm_perf_sink_config(handle);
 
@@ -691,6 +716,7 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
 		trbe_drain_and_disable_local();
 		return;
 	}
+	cpu_restore_tracing(trfcr);
 	trbe_enable_hw(buf);
 }
 
@@ -760,7 +786,18 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
 	struct perf_output_handle **handle_ptr = dev;
 	struct perf_output_handle *handle = *handle_ptr;
 	enum trbe_fault_action act;
-	u64 status;
+	u64 status, trfcr;
+
+	/*
+	 * Prohibit the tracing, while we process this. We turn
+	 * things back right, if we get to enabling the TRBE
+	 * back again. Otherwise, the tracing still remains
+	 * prohibited, until the perf event state changes
+	 * or another event is scheduled. This ensures that
+	 * the trace is not generated when it cannot be
+	 * captured.
+	 */
+	trfcr = cpu_prohibit_tracing();
 
 	/*
 	 * Ensure the trace is visible to the CPUs and
@@ -791,7 +828,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
 		trbe_handle_overflow(handle);
 		break;
 	case TRBE_FAULT_ACT_SPURIOUS:
-		trbe_handle_spurious(handle);
+		trbe_handle_spurious(handle, trfcr);
 		break;
 	case TRBE_FAULT_ACT_FATAL:
 		trbe_stop_and_truncate_event(handle);
-- 
2.24.1

Re: [PATCH 5/5] coresight: trbe: Prohibit tracing while handling an IRQ

[Anshuman Khandual]

A small nit. Paragraphs in the commit message do not seem to be aligned
properly to a maximum 75 characters width.

On 7/12/21 5:08 PM, Suzuki K Poulose wrote:
> When the TRBE generates an IRQ, we stop the TRBE, collect the trace
> and then reprogram the TRBE with the updated buffer pointers in case
> of a spurious IRQ. We might also leave the TRBE disabled, on an
> overflow interrupt, without touching the ETE. This means the
> the ETE is only disabled when the event is disabled later (via irq_work).
> This is incorrect, as the ETE trace is still ON without actually being
> captured and may be routed to the ATB.

I had an assumption that when the TRBE is stopped, ETE would also stop
implicitly given that the trace packets are not being accepted anymore.
But if that assumption does not always hold true, then yes trace must
be stopped upon a TRBE IRQ.

> 
> So, we move the CPU into trace prohibited state (for all exception
> levels) upon entering the IRQ handler. The state is restored before
> enabling the TRBE back. Otherwise the trace remains prohibited.
> Since, the ETM/ETE driver controls the TRFCR_EL1 per session,
> (from commit "coresight: etm4x: Use Trace Filtering controls dynamically")

commit SHA ID ?

> the tracing can be restored/enabled back when the event is rescheduled
> in.

Makes sense.

> 
> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Leo Yan <leo.yan@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 43 ++++++++++++++++++--
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index c0c264264427..e4d88e0de2a8 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -83,6 +83,31 @@ struct trbe_drvdata {
>  	struct platform_device *pdev;
>  };
>  
> +static inline void write_trfcr(u64 val)
> +{
> +	write_sysreg_s(val, SYS_TRFCR_EL1);
> +	isb();
> +}
> +

There is another instance of write_trfcr() in coresight-etm4x-core.c and
some other writes into SYS_TRFCR_EL1 elsewhere. write_trfcr() should be
factored out and moved to a common place.

> +/*
> + * Prohibit the CPU tracing at all ELs, in preparation to collect
> + * the trace buffer.
> + *
> + * Returns the original value of the trfcr for restoring later.
> + */
> +static u64 cpu_prohibit_tracing(void)
> +{
> +	u64 trfcr = read_sysreg_s(SYS_TRFCR_EL1);
> +
> +	write_trfcr(trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE));
> +	return trfcr;
> +}

This also should be factored out along with etm4x_prohibit_trace()
usage and moved to a common header instead.

> +
> +static void cpu_restore_tracing(u64 trfcr)
> +{
> +	write_trfcr(trfcr);
> +}
> +
>  static int trbe_alloc_node(struct perf_event *event)
>  {
>  	if (event->cpu == -1)
> @@ -681,7 +706,7 @@ static int arm_trbe_disable(struct coresight_device *csdev)
>  	return 0;
>  }
>  
> -static void trbe_handle_spurious(struct perf_output_handle *handle)
> +static void trbe_handle_spurious(struct perf_output_handle *handle, u64 trfcr)
>  {
>  	struct trbe_buf *buf = etm_perf_sink_config(handle);
>  
> @@ -691,6 +716,7 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
>  		trbe_drain_and_disable_local();
>  		return;
>  	}

A small comment here would be great because this will be the only
IRQ handler path, where it actually restores the tracing back.

> +	cpu_restore_tracing(trfcr);
>  	trbe_enable_hw(buf);
>  }
>  
> @@ -760,7 +786,18 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>  	struct perf_output_handle **handle_ptr = dev;
>  	struct perf_output_handle *handle = *handle_ptr;
>  	enum trbe_fault_action act;
> -	u64 status;
> +	u64 status, trfcr;
> +
> +	/*
> +	 * Prohibit the tracing, while we process this. We turn
> +	 * things back right, if we get to enabling the TRBE
> +	 * back again. Otherwise, the tracing still remains
> +	 * prohibited, until the perf event state changes
> +	 * or another event is scheduled. This ensures that
> +	 * the trace is not generated when it cannot be
> +	 * captured.
> +	 */

Right.

But a small nit though. Please keep the comments here formatted and
aligned with the existing ones.

> +	trfcr = cpu_prohibit_tracing();
>  
>  	/*
>  	 * Ensure the trace is visible to the CPUs and
> @@ -791,7 +828,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>  		trbe_handle_overflow(handle);
>  		break;
>  	case TRBE_FAULT_ACT_SPURIOUS:
> -		trbe_handle_spurious(handle);
> +		trbe_handle_spurious(handle, trfcr);
>  		break;
>  	case TRBE_FAULT_ACT_FATAL:
>  		trbe_stop_and_truncate_event(handle);
> 

But stopping the trace (even though from a sink IRQ handler) is a source
device action. Should not this be done via a new coresight_ops_source
callback instead ?

Re: [PATCH 5/5] coresight: trbe: Prohibit tracing while handling an IRQ

[Suzuki K Poulose]

On 15/07/2021 04:09, Anshuman Khandual wrote:
> A small nit. Paragraphs in the commit message do not seem to be aligned
> properly to a maximum 75 characters width.
> 
> On 7/12/21 5:08 PM, Suzuki K Poulose wrote:
>> When the TRBE generates an IRQ, we stop the TRBE, collect the trace
>> and then reprogram the TRBE with the updated buffer pointers in case
>> of a spurious IRQ. We might also leave the TRBE disabled, on an
>> overflow interrupt, without touching the ETE. This means the
>> the ETE is only disabled when the event is disabled later (via irq_work).
>> This is incorrect, as the ETE trace is still ON without actually being
>> captured and may be routed to the ATB.
> 
> I had an assumption that when the TRBE is stopped, ETE would also stop
> implicitly given that the trace packets are not being accepted anymore.
> But if that assumption does not always hold true, then yes trace must
> be stopped upon a TRBE IRQ.

No, the ETE never stops, until it is stopped. The ETE doesn't care who
is the consumer of the trace. Be it TRBE or ATB or any other sink.

> 
>>
>> So, we move the CPU into trace prohibited state (for all exception
>> levels) upon entering the IRQ handler. The state is restored before
>> enabling the TRBE back. Otherwise the trace remains prohibited.
>> Since, the ETM/ETE driver controls the TRFCR_EL1 per session,
>> (from commit "coresight: etm4x: Use Trace Filtering controls dynamically")
> 
> commit SHA ID ?
> 

The patch is in this series, not committed yet.

>> the tracing can be restored/enabled back when the event is rescheduled
>> in.
> 
> Makes sense.
> 
>>
>> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
>> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: Leo Yan <leo.yan@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-trbe.c | 43 ++++++++++++++++++--
>>   1 file changed, 40 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>> index c0c264264427..e4d88e0de2a8 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -83,6 +83,31 @@ struct trbe_drvdata {
>>   	struct platform_device *pdev;
>>   };
>>   
>> +static inline void write_trfcr(u64 val)
>> +{
>> +	write_sysreg_s(val, SYS_TRFCR_EL1);
>> +	isb();
>> +}
>> +
> 
> There is another instance of write_trfcr() in coresight-etm4x-core.c and
> some other writes into SYS_TRFCR_EL1 elsewhere. write_trfcr() should be
> factored out and moved to a common place.

Agreed, but I couldn't find a right candidate for this. Welcome
to suggestions. May be we could add something like:

asm/self-hosted.h

> 
>> +/*
>> + * Prohibit the CPU tracing at all ELs, in preparation to collect
>> + * the trace buffer.
>> + *
>> + * Returns the original value of the trfcr for restoring later.
>> + */
>> +static u64 cpu_prohibit_tracing(void)
>> +{
>> +	u64 trfcr = read_sysreg_s(SYS_TRFCR_EL1);
>> +
>> +	write_trfcr(trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE));
>> +	return trfcr;
>> +}
> 
> This also should be factored out along with etm4x_prohibit_trace()
> usage and moved to a common header instead.
> 
>> +
>> +static void cpu_restore_tracing(u64 trfcr)
>> +{
>> +	write_trfcr(trfcr);
>> +}
>> +
>>   static int trbe_alloc_node(struct perf_event *event)
>>   {
>>   	if (event->cpu == -1)
>> @@ -681,7 +706,7 @@ static int arm_trbe_disable(struct coresight_device *csdev)
>>   	return 0;
>>   }
>>   
>> -static void trbe_handle_spurious(struct perf_output_handle *handle)
>> +static void trbe_handle_spurious(struct perf_output_handle *handle, u64 trfcr)
>>   {
>>   	struct trbe_buf *buf = etm_perf_sink_config(handle);
>>   
>> @@ -691,6 +716,7 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
>>   		trbe_drain_and_disable_local();
>>   		return;
>>   	}
> 
> A small comment here would be great because this will be the only
> IRQ handler path, where it actually restores the tracing back.

Agreed

> 
>> +	cpu_restore_tracing(trfcr);
>>   	trbe_enable_hw(buf);
>>   }
>>   
>> @@ -760,7 +786,18 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>>   	struct perf_output_handle **handle_ptr = dev;
>>   	struct perf_output_handle *handle = *handle_ptr;
>>   	enum trbe_fault_action act;
>> -	u64 status;
>> +	u64 status, trfcr;
>> +
>> +	/*
>> +	 * Prohibit the tracing, while we process this. We turn
>> +	 * things back right, if we get to enabling the TRBE
>> +	 * back again. Otherwise, the tracing still remains
>> +	 * prohibited, until the perf event state changes
>> +	 * or another event is scheduled. This ensures that
>> +	 * the trace is not generated when it cannot be
>> +	 * captured.
>> +	 */
> 
> Right.
> 
> But a small nit though. Please keep the comments here formatted and
> aligned with the existing ones.
> 

ok

>> +	trfcr = cpu_prohibit_tracing();
>>   
>>   	/*
>>   	 * Ensure the trace is visible to the CPUs and
>> @@ -791,7 +828,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>>   		trbe_handle_overflow(handle);
>>   		break;
>>   	case TRBE_FAULT_ACT_SPURIOUS:
>> -		trbe_handle_spurious(handle);
>> +		trbe_handle_spurious(handle, trfcr);
>>   		break;
>>   	case TRBE_FAULT_ACT_FATAL:
>>   		trbe_stop_and_truncate_event(handle);
>>
> 
> But stopping the trace (even though from a sink IRQ handler) is a source
> device action. Should not this be done via a new coresight_ops_source
> callback instead ?

It is a valid point. But that has limitations.
Here is the list:

   * Stopping the source is a heavy hammer, especially if we
     are about to continue the trace soon. (e.g, spurious
     interrupt and possibly soon for FILL events with reworking
     the flags)

   * Stopping the source, via source_ops() is doing things
     under the driving mode of the session, perf vs sysfs.
     We only support perf though, but if there is another
     user.

   * This is agnostic to the mode (as above), the TRBE driver
     doesn't need to be taught, how to find the path and
     stop the current session for the given mode.

   * If the tracing is enabled in kernel mode, the ETE still
     generates the trace until we trigger the longer route
     for disabling, which is not nice.

Suzuki

Re: [PATCH 5/5] coresight: trbe: Prohibit tracing while handling an IRQ

[Anshuman Khandual]

On 7/15/21 2:22 PM, Suzuki K Poulose wrote:
> On 15/07/2021 04:09, Anshuman Khandual wrote:
>> A small nit. Paragraphs in the commit message do not seem to be aligned
>> properly to a maximum 75 characters width.
>>
>> On 7/12/21 5:08 PM, Suzuki K Poulose wrote:
>>> When the TRBE generates an IRQ, we stop the TRBE, collect the trace
>>> and then reprogram the TRBE with the updated buffer pointers in case
>>> of a spurious IRQ. We might also leave the TRBE disabled, on an
>>> overflow interrupt, without touching the ETE. This means the
>>> the ETE is only disabled when the event is disabled later (via irq_work).
>>> This is incorrect, as the ETE trace is still ON without actually being
>>> captured and may be routed to the ATB.
>>
>> I had an assumption that when the TRBE is stopped, ETE would also stop
>> implicitly given that the trace packets are not being accepted anymore.
>> But if that assumption does not always hold true, then yes trace must
>> be stopped upon a TRBE IRQ.
> 
> No, the ETE never stops, until it is stopped. The ETE doesn't care who
> is the consumer of the trace. Be it TRBE or ATB or any other sink.

Right. In that case, ETE must be stopped explicitly.

> 
>>
>>>
>>> So, we move the CPU into trace prohibited state (for all exception
>>> levels) upon entering the IRQ handler. The state is restored before
>>> enabling the TRBE back. Otherwise the trace remains prohibited.
>>> Since, the ETM/ETE driver controls the TRFCR_EL1 per session,
>>> (from commit "coresight: etm4x: Use Trace Filtering controls dynamically")
>>
>> commit SHA ID ?
>>
> 
> The patch is in this series, not committed yet.

Ahh missed, it might be better to address it like 'previous patch ("")
in the series' instead than 'commit ("")'.

> 
>>> the tracing can be restored/enabled back when the event is rescheduled
>>> in.
>>
>> Makes sense.
>>
>>>
>>> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
>>> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Cc: Mike Leach <mike.leach@linaro.org>
>>> Cc: Leo Yan <leo.yan@linaro.org>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-trbe.c | 43 ++++++++++++++++++--
>>>   1 file changed, 40 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>>> index c0c264264427..e4d88e0de2a8 100644
>>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>>> @@ -83,6 +83,31 @@ struct trbe_drvdata {
>>>       struct platform_device *pdev;
>>>   };
>>>   +static inline void write_trfcr(u64 val)
>>> +{
>>> +    write_sysreg_s(val, SYS_TRFCR_EL1);
>>> +    isb();
>>> +}
>>> +
>>
>> There is another instance of write_trfcr() in coresight-etm4x-core.c and
>> some other writes into SYS_TRFCR_EL1 elsewhere. write_trfcr() should be
>> factored out and moved to a common place.
> 
> Agreed, but I couldn't find a right candidate for this. Welcome
> to suggestions. May be we could add something like:
> 
> asm/self-hosted.h

Why in asm ? Like other similar coresight headers, just creating 'self-hosted.h'
in the drivers/hwtracing/coresight/ path should do.

> 
>>
>>> +/*
>>> + * Prohibit the CPU tracing at all ELs, in preparation to collect
>>> + * the trace buffer.
>>> + *
>>> + * Returns the original value of the trfcr for restoring later.
>>> + */
>>> +static u64 cpu_prohibit_tracing(void)
>>> +{
>>> +    u64 trfcr = read_sysreg_s(SYS_TRFCR_EL1);
>>> +
>>> +    write_trfcr(trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE));
>>> +    return trfcr;
>>> +}
>>
>> This also should be factored out along with etm4x_prohibit_trace()
>> usage and moved to a common header instead.
>>
>>> +
>>> +static void cpu_restore_tracing(u64 trfcr)
>>> +{
>>> +    write_trfcr(trfcr);
>>> +}
>>> +
>>>   static int trbe_alloc_node(struct perf_event *event)
>>>   {
>>>       if (event->cpu == -1)
>>> @@ -681,7 +706,7 @@ static int arm_trbe_disable(struct coresight_device *csdev)
>>>       return 0;
>>>   }
>>>   -static void trbe_handle_spurious(struct perf_output_handle *handle)
>>> +static void trbe_handle_spurious(struct perf_output_handle *handle, u64 trfcr)
>>>   {
>>>       struct trbe_buf *buf = etm_perf_sink_config(handle);
>>>   @@ -691,6 +716,7 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
>>>           trbe_drain_and_disable_local();
>>>           return;
>>>       }
>>
>> A small comment here would be great because this will be the only
>> IRQ handler path, where it actually restores the tracing back.
> 
> Agreed
> 
>>
>>> +    cpu_restore_tracing(trfcr);
>>>       trbe_enable_hw(buf);
>>>   }
>>>   @@ -760,7 +786,18 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>>>       struct perf_output_handle **handle_ptr = dev;
>>>       struct perf_output_handle *handle = *handle_ptr;
>>>       enum trbe_fault_action act;
>>> -    u64 status;
>>> +    u64 status, trfcr;
>>> +
>>> +    /*
>>> +     * Prohibit the tracing, while we process this. We turn
>>> +     * things back right, if we get to enabling the TRBE
>>> +     * back again. Otherwise, the tracing still remains
>>> +     * prohibited, until the perf event state changes
>>> +     * or another event is scheduled. This ensures that
>>> +     * the trace is not generated when it cannot be
>>> +     * captured.
>>> +     */
>>
>> Right.
>>
>> But a small nit though. Please keep the comments here formatted and
>> aligned with the existing ones.
>>
> 
> ok
> 
>>> +    trfcr = cpu_prohibit_tracing();
>>>         /*
>>>        * Ensure the trace is visible to the CPUs and
>>> @@ -791,7 +828,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>>>           trbe_handle_overflow(handle);
>>>           break;
>>>       case TRBE_FAULT_ACT_SPURIOUS:
>>> -        trbe_handle_spurious(handle);
>>> +        trbe_handle_spurious(handle, trfcr);
>>>           break;
>>>       case TRBE_FAULT_ACT_FATAL:
>>>           trbe_stop_and_truncate_event(handle);
>>>
>>
>> But stopping the trace (even though from a sink IRQ handler) is a source
>> device action. Should not this be done via a new coresight_ops_source
>> callback instead ?
> 
> It is a valid point. But that has limitations.
> Here is the list:
> 
>   * Stopping the source is a heavy hammer, especially if we
>     are about to continue the trace soon. (e.g, spurious
>     interrupt and possibly soon for FILL events with reworking
>     the flags)
> 
>   * Stopping the source, via source_ops() is doing things
>     under the driving mode of the session, perf vs sysfs.
>     We only support perf though, but if there is another
>     user.
> 
>   * This is agnostic to the mode (as above), the TRBE driver
>     doesn't need to be taught, how to find the path and
>     stop the current session for the given mode.
> 
>   * If the tracing is enabled in kernel mode, the ETE still
>     generates the trace until we trigger the longer route
>     for disabling, which is not nice.
Okay, makes sense. But should not this be included in the commit message
as well - why a source_ops() is not being added for this operation, even
though it sounds intuitive.

Re: [PATCH 0/5] coresight: TRBE and Self-Hosted trace fixes

[Mathieu Poirier]

On Mon, Jul 12, 2021 at 12:38:25PM +0100, Suzuki K Poulose wrote:
> This is a series of fixes addressing the issues in the way we handle
>   - Self-Hosted trace filter control register for ETM/ETE
>   - AUX buffer and event handling of TRBE at overflow.
> 
> The use of TRUNCATED flag on an IRQ for the TRBE driver is
> something that needs to be rexamined. Please see Patch 3 for
> more details.
> 
> Suzuki K Poulose (5):
>   coresight: etm4x: Save restore TRFCR_EL1
>   coresight: etm4x: Use Trace Filtering controls dynamically
>   coresight: trbe: Keep TRBE disabled on overflow irq
>   coresight: trbe: Move irq_work queue processing
>   coresight: trbe: Prohibit tracing while handling an IRQ
> 
>  .../coresight/coresight-etm4x-core.c          | 109 ++++++++++++++----
>  drivers/hwtracing/coresight/coresight-etm4x.h |   7 +-
>  drivers/hwtracing/coresight/coresight-trbe.c  |  91 ++++++++++-----
>  3 files changed, 149 insertions(+), 58 deletions(-)

Anshuman - please have a look when you have a minutes.

Thanks,
Mathieu

> 
> -- 
> 2.24.1
> 

Re: [PATCH 0/5] coresight: TRBE and Self-Hosted trace fixes

[Anshuman Khandual]

On 7/12/21 10:32 PM, Mathieu Poirier wrote:
> On Mon, Jul 12, 2021 at 12:38:25PM +0100, Suzuki K Poulose wrote:
>> This is a series of fixes addressing the issues in the way we handle
>>   - Self-Hosted trace filter control register for ETM/ETE
>>   - AUX buffer and event handling of TRBE at overflow.
>>
>> The use of TRUNCATED flag on an IRQ for the TRBE driver is
>> something that needs to be rexamined. Please see Patch 3 for
>> more details.
>>
>> Suzuki K Poulose (5):
>>   coresight: etm4x: Save restore TRFCR_EL1
>>   coresight: etm4x: Use Trace Filtering controls dynamically
>>   coresight: trbe: Keep TRBE disabled on overflow irq
>>   coresight: trbe: Move irq_work queue processing
>>   coresight: trbe: Prohibit tracing while handling an IRQ
>>
>>  .../coresight/coresight-etm4x-core.c          | 109 ++++++++++++++----
>>  drivers/hwtracing/coresight/coresight-etm4x.h |   7 +-
>>  drivers/hwtracing/coresight/coresight-trbe.c  |  91 ++++++++++-----
>>  3 files changed, 149 insertions(+), 58 deletions(-)
> 
> Anshuman - please have a look when you have a minutes.

Sure, will do.