[PATCH v3 00/10] coresight: TRBE and Self-Hosted trace fixes

[PATCH v3 00/10] coresight: TRBE and Self-Hosted trace fixes

[Suzuki K Poulose]

This series fixes the following issues with the TRBE and Self-Hosted
trace for CoreSight.

The Self-hosted trace filter control registers are now save-restored
across CPU PM event. And more importantly the Trace Filtering is now
used to control per ETM session (rather than allowing the trace
throughout the life time of the system). i.e, ETM configuration of
the given run is used to enforce trace filtering (TRFCR) along with the
Trace Exclusion controls in TRCVICTLR.

For the TRBE, we were using the TRUNCATED flag in the AUX buffer on
every IRQ to indicate that we may have lost a few bytes of trace. But
this causes the event to be disabled until the userspace re-enables
it back, even when there is space left in the ring buffer. To make
things worse, we were restarting the AUX handle, which would soon
be disabled, potentially creating 0 sized records (without truncation),
which the perf tool tends to ignore. This might cause the event to be
disabled permanently. Also, sometimes we leave the buffer TRUNCATED,
but delay the closing of the handle to event schedule out, which could
cause significant black out in the trace capture. This was reported
by Tamas Zsoldos.

This series removes the use of TRUNCATED flag for every IRQ. Instead,
uses the COLLISION flag to indicate that there was a WRAP event in the
trace collection. A patch to report the COLLISIONs has been queued for
the perf tool. The TRUNCATED is only used if we really run out of space
in the buffer. And also we make sure the "handle" is closed immediately
on TRUNCATED case, which triggers the userspace to take action. The core
perf layer has been hardened to handle this case where a "handle" is
closed out.

Finally, we make sure that the CPU trace is prohibited, when the TRBE
is left disabled. The ETE/ETM driver will program the Trace Filtering
appropriately since we do this dynamically now with the first half
of the series.

Applies on coresigh/next.

The tree is also available here:

 https://gitlab.arm.com/linux-arm/linux-skp/-/tree/coresight/trbe-self-hosted-fixes/v3


Changes since v2 [1]:
 - Address comments from Anshuman
 - Perf tool patch queued, to report COLLISIONs in a session.

Changes since v1 [0]:
  - Moved TRFCR related accessors to a new header file
  - Following a discussion, dropped the TRUNCATED flag from
    the TRBE IRQ handler on WRAP. Instead mark COLLISION.
  - Added new patches to harden the ETM perf layer to handle
    an error in the sink driver.
  - Fix TRBE spurious IRQ handling
  - Cleanup TRBE driver to make the "TRUNCATE" cases managed
    at a central place.

[0] https://lkml.kernel.org/r/20210712113830.2803257-1-suzuki.poulose@arm.com
[1] https://lkml.kernel.org/r/20210723124611.3828908-1-suzuki.poulose@arm.com

Suzuki K Poulose (10):
  coresight: etm4x: Save restore TRFCR_EL1
  coresight: etm4x: Use Trace Filtering controls dynamically
  coresight: etm-pmu: Ensure the AUX handle is valid
  coresight: trbe: Ensure the format flag is set always
  coresight: trbe: Drop duplicate TRUNCATE flags
  coresight: trbe: Fix handling of spurious interrupts
  coresight: trbe: Do not truncate buffer on IRQ
  coresight: trbe: Unify the enabling sequence
  coresight: trbe: End the AUX handle on truncation
  coresight: trbe: Prohibit trace before disabling TRBE

 .../hwtracing/coresight/coresight-etm-perf.c  |  27 ++++-
 .../coresight/coresight-etm4x-core.c          | 100 ++++++++++++----
 drivers/hwtracing/coresight/coresight-etm4x.h |   9 +-
 .../coresight/coresight-self-hosted-trace.h   |  34 ++++++
 drivers/hwtracing/coresight/coresight-trbe.c  | 111 ++++++++++--------
 5 files changed, 200 insertions(+), 81 deletions(-)
 create mode 100644 drivers/hwtracing/coresight/coresight-self-hosted-trace.h

-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 01/10] 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.

The TRFCR related helpers are in a new header file, as we need to use
them for TRBE in the later patches.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 .../coresight/coresight-etm4x-core.c          | 43 +++++++++++++------
 drivers/hwtracing/coresight/coresight-etm4x.h |  2 +
 .../coresight/coresight-self-hosted-trace.h   | 25 +++++++++++
 3 files changed, 58 insertions(+), 12 deletions(-)
 create mode 100644 drivers/hwtracing/coresight/coresight-self-hosted-trace.h

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index e24252eaf8e4..537c0d7ee1ed 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -40,6 +40,7 @@
 #include "coresight-etm4x.h"
 #include "coresight-etm-perf.h"
 #include "coresight-etm4x-cfg.h"
+#include "coresight-self-hosted-trace.h"
 #include "coresight-syscfg.h"
 
 static int boot_enable;
@@ -1011,7 +1012,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)
@@ -1554,7 +1555,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;
@@ -1693,7 +1694,23 @@ 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;
+
+	/* Save the TRFCR irrespective of whether the ETM is ON */
+	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;
@@ -1789,6 +1806,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)
 {
@@ -1800,23 +1825,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;
diff --git a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
new file mode 100644
index 000000000000..53b35a28075e
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/*
+ * Arm v8 Self-Hosted trace support.
+ *
+ * Copyright (C) 2021 ARM Ltd.
+ */
+
+#ifndef __CORESIGHT_SELF_HOSTED_TRACE_H
+#define __CORESIGHT_SELF_HOSTED_TRACE_H
+
+#include <asm/sysreg.h>
+
+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();
+}
+
+#endif			/*  __CORESIGHT_SELF_HOSTED_TRACE_H */
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 02/10] 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, which doesn't prevent the ETM from generating Context
packets for an "excluded" EL. At the moment, we do a onetime
enable trace at user and kernel and leave it untouched for the
kernel life time. This implies that the ETM could potentially
generate trace packets containing the kernel addresses, and
thus leaking the kernel virtual address in the trace.

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 |  7 ++-
 .../coresight/coresight-self-hosted-trace.h   |  7 +++
 3 files changed, 59 insertions(+), 18 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 537c0d7ee1ed..d9a85ca25b60 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -239,6 +239,45 @@ struct etm4_enable_arg {
 	int rc;
 };
 
+/*
+ * 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 the CPU doesn't support FEAT_TRF, nothing to do */
+	if (!drvdata->trfcr)
+		return;
+	cpu_prohibit_trace();
+}
+
+/*
+ * 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
@@ -443,6 +482,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);
 
@@ -738,7 +778,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;
@@ -765,12 +804,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,
@@ -786,9 +820,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] =
@@ -990,15 +1021,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
@@ -1012,7 +1043,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)
@@ -1203,7 +1234,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)
@@ -1699,7 +1730,7 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
 	int ret = 0;
 
 	/* Save the TRFCR irrespective of whether the ETM is ON */
-	if (drvdata->trfc)
+	if (drvdata->trfcr)
 		drvdata->save_trfcr = read_trfcr();
 	/*
 	 * Save and restore the ETM Trace registers only if
@@ -1808,7 +1839,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..3c4d69b096ca 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -919,7 +919,10 @@ 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 supports FEAT_TRF, value of the TRFCR_ELx that
+ *		allows tracing at all ELs. We don't want to compute this
+ *		at runtime, due to the additional setting of TRFCR_CX when
+ *		in EL2. 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 +975,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;
diff --git a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
index 53b35a28075e..586d26e0cba3 100644
--- a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
+++ b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
@@ -22,4 +22,11 @@ static inline void write_trfcr(u64 val)
 	isb();
 }
 
+static inline void cpu_prohibit_trace(void)
+{
+	u64 trfcr = read_trfcr();
+
+	/* Prohibit tracing at EL0 & the kernel EL */
+	write_trfcr(trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE));
+}
 #endif			/*  __CORESIGHT_SELF_HOSTED_TRACE_H */
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 03/10] coresight: etm-pmu: Ensure the AUX handle is valid

[Suzuki K Poulose]

The ETM perf infrastructure closes out a handle during event_stop
or on an error in starting the event. In either case, it is possible
for a "sink" to update/close the handle, under certain circumstances.
(e.g no space in ring buffer.). So, ensure that we handle this
gracefully in the PMU driver by verifying the handle is still valid.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 .../hwtracing/coresight/coresight-etm-perf.c  | 27 ++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 8ebd728d3a80..9153c8f30568 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -481,8 +481,15 @@ static void etm_event_start(struct perf_event *event, int flags)
 fail_disable_path:
 	coresight_disable_path(path);
 fail_end_stop:
-	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
-	perf_aux_output_end(handle, 0);
+	/*
+	 * Check if the handle is still associated with the event,
+	 * to handle cases where if the sink failed to start the
+	 * trace and TRUNCATED the handle already.
+	 */
+	if (READ_ONCE(handle->event)) {
+		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
+		perf_aux_output_end(handle, 0);
+	}
 fail:
 	event->hw.state = PERF_HES_STOPPED;
 	goto out;
@@ -550,7 +557,21 @@ static void etm_event_stop(struct perf_event *event, int mode)
 
 		size = sink_ops(sink)->update_buffer(sink, handle,
 					      event_data->snk_config);
-		perf_aux_output_end(handle, size);
+		/*
+		 * Make sure the handle is still valid as the
+		 * sink could have closed it from an IRQ.
+		 * The sink driver must handle the race with
+		 * update_buffer() and IRQ. Thus either we
+		 * should get a valid handle and valid size
+		 * (which may be 0).
+		 *
+		 * But we should never get a non-zero size with
+		 * an invalid handle.
+		 */
+		if (READ_ONCE(handle->event))
+			perf_aux_output_end(handle, size);
+		else
+			WARN_ON(size);
 	}
 
 	/* Disabling the path make its elements available to other sessions */
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 04/10] coresight: trbe: Ensure the format flag is set always

[Suzuki K Poulose]

When the TRBE is stopped on truncating an event, we may not
set the FORMAT flag, even though the size of the record is 0.
Let us be consistent and not confuse the user.

To ensure that the format flag is always set on all the
records generated by TRBE, set the flag when we have a
new handle. Rather than deferring to the "end" operation,
which makes it clear. So, we can do this from

 -  arm_trbe_enable() -> When a new handle is provided by the
    CoreSight PMU, triggered via etm_event_start()
 -  trbe_handle_overflow() -> When we begin a new handle after
    closing the previous on overflow.

Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
Changes since v2:
  - Move the format flag setting to the beginning of a session
    than the ending (which can happen via various different paths)
---
 drivers/hwtracing/coresight/coresight-trbe.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 176868496879..81bf183a73a1 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -554,8 +554,6 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev,
 	if (cpudata->mode != CS_MODE_PERF)
 		return 0;
 
-	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
-
 	/*
 	 * We are about to disable the TRBE. And this could in turn
 	 * fill up the buffer triggering, an IRQ. This could be consumed
@@ -648,6 +646,7 @@ static int arm_trbe_enable(struct coresight_device *csdev, u32 mode, void *data)
 	if (mode != CS_MODE_PERF)
 		return -EINVAL;
 
+	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
 	*this_cpu_ptr(drvdata->handle) = handle;
 	cpudata->buf = buf;
 	cpudata->mode = mode;
@@ -710,8 +709,7 @@ 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.
 	 */
-	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW |
-				     PERF_AUX_FLAG_TRUNCATED);
+	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
 	perf_aux_output_end(handle, size);
 	event_data = perf_aux_output_begin(handle, event);
 	if (!event_data) {
@@ -725,6 +723,7 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
 		*this_cpu_ptr(buf->cpudata->drvdata->handle) = NULL;
 		return;
 	}
+	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
 	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) {
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 05/10] coresight: trbe: Drop duplicate TRUNCATE flags

[Suzuki K Poulose]

We mark the buffer as TRUNCATED when there is no space left
in the buffer. But we do it at different points.
    __trbe_normal_offset()
and also, at all the callers of the above function via
compute_trbe_buffer_limit(), when the limit == base (i.e
offset = 0 as returned by the __trbe_normal_offset()).

So, given that the callers already mark the buffer as TRUNCATED
drop the caller inside the __trbe_normal_offset().

This is in preparation to moving the handling of TRUNCATED
into a central place.

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>
---
Change since v2:
  - Added the flag for trbe_handle_spurious() -  Anshuman
---
 drivers/hwtracing/coresight/coresight-trbe.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 81bf183a73a1..5297b11f26b7 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -252,13 +252,9 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)
 	 * trbe_base				trbe_base + nr_pages
 	 *
 	 * Perf aux buffer does not have any space for the driver to write into.
-	 * Just communicate trace truncation event to the user space by marking
-	 * it with PERF_AUX_FLAG_TRUNCATED.
 	 */
-	if (!handle->size) {
-		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
+	if (!handle->size)
 		return 0;
-	}
 
 	/* Compute the tail and wakeup indices now that we've aligned head */
 	tail = PERF_IDX2OFF(handle->head + handle->size, buf);
@@ -360,7 +356,6 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)
 		return limit;
 
 	trbe_pad_buf(handle, handle->size);
-	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
 	return 0;
 }
 
@@ -688,6 +683,7 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
 	buf->trbe_write = buf->trbe_base + PERF_IDX2OFF(handle->head, buf);
 	if (buf->trbe_limit == buf->trbe_base) {
 		trbe_drain_and_disable_local();
+		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
 		return;
 	}
 	trbe_enable_hw(buf);
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 06/10] coresight: trbe: Fix handling of spurious interrupts

[Suzuki K Poulose]

On a spurious IRQ, right now we disable the TRBE and then re-enable
it back, resetting the "buffer" pointers(i.e BASE, LIMIT and more
importantly WRITE) to the original pointers from the AUX handle.
This implies that we overwrite any trace that was written so far,
(by overwriting TRBPTR) while we should have ignored the IRQ.

This patch cleans the behavior, by only stopping the TRBE if the
IRQ was indeed raised, as we can read the TRBSR without stopping
the TRBE (Only writes to the TRBSR requires the TRBE disabled).
And also, on detecting a spurious IRQ after examining the TRBSR,
we simply re-enable the TRBE without touching the other parameters.

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 | 30 ++++++++++----------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 5297b11f26b7..de99dd0aecd3 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -677,16 +677,16 @@ static int arm_trbe_disable(struct coresight_device *csdev)
 
 static void trbe_handle_spurious(struct perf_output_handle *handle)
 {
-	struct trbe_buf *buf = etm_perf_sink_config(handle);
+	u64 limitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
 
-	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_drain_and_disable_local();
-		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
-		return;
-	}
-	trbe_enable_hw(buf);
+	/*
+	 * If the IRQ was spurious, simply re-enable the TRBE
+	 * back without modifying the buffer parameters to
+	 * retain the trace collected so far.
+	 */
+	limitr |= TRBLIMITR_ENABLE;
+	write_sysreg_s(limitr, SYS_TRBLIMITR_EL1);
+	isb();
 }
 
 static void trbe_handle_overflow(struct perf_output_handle *handle)
@@ -759,12 +759,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
 	enum trbe_fault_action act;
 	u64 status;
 
-	/*
-	 * Ensure the trace is visible to the CPUs and
-	 * any external aborts have been resolved.
-	 */
-	trbe_drain_and_disable_local();
-
+	/* Reads to TRBSR_EL1 is fine when TRBE is active */
 	status = read_sysreg_s(SYS_TRBSR_EL1);
 	/*
 	 * If the pending IRQ was handled by update_buffer callback
@@ -773,6 +768,11 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
 	if (!is_trbe_irq(status))
 		return IRQ_NONE;
 
+	/*
+	 * Ensure the trace is visible to the CPUs and
+	 * any external aborts have been resolved.
+	 */
+	trbe_drain_and_disable_local();
 	clr_trbe_irq();
 	isb();
 
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 07/10] coresight: trbe: Do not truncate buffer on IRQ

[Suzuki K Poulose]

The TRBE driver marks the AUX buffer as TRUNCATED when we get an IRQ
on FILL event. This has rather unwanted side-effect of the event
being disabled when there may be more space in the ring buffer.

So, instead of TRUNCATE we need a different flag to indicate
that the trace may have lost a few bytes (i.e from the point of
generating the FILL event until the IRQ is consumed). Anyways, the
userspace must use the size from RECORD_AUX headers to restrict
the "trace" decoding.

Using PARTIAL flag causes the perf tool to generate the
following warning:

  Warning:
  AUX data had gaps in it XX times out of YY!

  Are you running a KVM guest in the background?

which is pointlessly scary for a user. The other remaining options
are :
  - COLLISION - Use by SPE to indicate samples collided
  - Add a new flag - Specifically for CoreSight, doesn't sound
    so good, if we can re-use something.

Given that we don't already use the "COLLISION" flag, the above
behavior can be notified using this flag for CoreSight.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: James Clark <james.clark@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Leo Yan <leo.yan@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
Changes since v2:
  - The perf tool patch for reporting collisions is queued.
---
 drivers/hwtracing/coresight/coresight-trbe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index de99dd0aecd3..a1a15fa6c4ae 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -612,7 +612,7 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev,
 		 * for correct size. Also, mark the buffer truncated.
 		 */
 		write = get_trbe_limit_pointer();
-		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
+		perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
 	}
 
 	offset = write - base;
@@ -705,7 +705,7 @@ 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.
 	 */
-	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
+	perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
 	perf_aux_output_end(handle, size);
 	event_data = perf_aux_output_begin(handle, event);
 	if (!event_data) {
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 08/10] coresight: trbe: Unify the enabling sequence

[Suzuki K Poulose]

Unify the sequence of enabling the TRBE. We do this from
event_start and also from the TRBE IRQ handler. Lets move
this to a common helper. The only minor functional change
is returning an error when we fail to enable the TRBE.
This should be handled already.

Since we now have unique entry point to trying to enable TRBE,
move the format flag setting to the central place.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
Changes since v2:
 - Removed redundant handle book keeping (Anshuman)
 - Moved the format flag setting to the helper
---
 drivers/hwtracing/coresight/coresight-trbe.c | 37 ++++++++++----------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index a1a15fa6c4ae..25c16d0f9e49 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -629,6 +629,21 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev,
 	return size;
 }
 
+static int __arm_trbe_enable(struct trbe_buf *buf,
+			     struct perf_output_handle *handle)
+{
+	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
+	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 -ENOSPC;
+	}
+	*this_cpu_ptr(buf->cpudata->drvdata->handle) = handle;
+	trbe_enable_hw(buf);
+	return 0;
+}
+
 static int arm_trbe_enable(struct coresight_device *csdev, u32 mode, void *data)
 {
 	struct trbe_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
@@ -641,19 +656,11 @@ static int arm_trbe_enable(struct coresight_device *csdev, u32 mode, void *data)
 	if (mode != CS_MODE_PERF)
 		return -EINVAL;
 
-	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
-	*this_cpu_ptr(drvdata->handle) = handle;
 	cpudata->buf = buf;
 	cpudata->mode = mode;
 	buf->cpudata = cpudata;
-	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 0;
-	}
-	trbe_enable_hw(buf);
-	return 0;
+
+	return __arm_trbe_enable(buf, handle);
 }
 
 static int arm_trbe_disable(struct coresight_device *csdev)
@@ -719,15 +726,7 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
 		*this_cpu_ptr(buf->cpudata->drvdata->handle) = NULL;
 		return;
 	}
-	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
-	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);
+	__arm_trbe_enable(buf, handle);
 }
 
 static bool is_perf_trbe(struct perf_output_handle *handle)
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 09/10] coresight: trbe: End the AUX handle on truncation

[Suzuki K Poulose]

When we detect that there isn't enough space left to start a meaningful
session, we disable the TRBE, marking the buffer as TRUNCATED. But we delay
the notification to the perf layer by perf_aux_output_end() until the event
is scheduled out, triggered from the kernel perf layer. This will cause
significant black outs in the trace. Now that the CoreSight PMU layer can
handle a closed "AUX" handle properly, we can close the handle as soon as
we detect the case, allowing the userspace to collect and re-enable the
event.

Also, while in the IRQ handler, move the irq_work_run() after we have
updated the handle, to make sure the "TRUNCATED" flag causes the event to
be disabled as soon as possible.

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>
Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-trbe.c | 26 ++++++++++++--------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 25c16d0f9e49..d05cb5ffc128 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -133,6 +133,7 @@ static void trbe_stop_and_truncate_event(struct perf_output_handle *handle)
 	 */
 	trbe_drain_and_disable_local();
 	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
+	perf_aux_output_end(handle, 0);
 	*this_cpu_ptr(buf->cpudata->drvdata->handle) = NULL;
 }
 
@@ -696,7 +697,7 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
 	isb();
 }
 
-static void trbe_handle_overflow(struct perf_output_handle *handle)
+static int trbe_handle_overflow(struct perf_output_handle *handle)
 {
 	struct perf_event *event = handle->event;
 	struct trbe_buf *buf = etm_perf_sink_config(handle);
@@ -724,9 +725,10 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
 		 */
 		trbe_drain_and_disable_local();
 		*this_cpu_ptr(buf->cpudata->drvdata->handle) = NULL;
-		return;
+		return -EINVAL;
 	}
-	__arm_trbe_enable(buf, handle);
+
+	return __arm_trbe_enable(buf, handle);
 }
 
 static bool is_perf_trbe(struct perf_output_handle *handle)
@@ -757,6 +759,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
 	struct perf_output_handle *handle = *handle_ptr;
 	enum trbe_fault_action act;
 	u64 status;
+	bool truncated = false;
 
 	/* Reads to TRBSR_EL1 is fine when TRBE is active */
 	status = read_sysreg_s(SYS_TRBSR_EL1);
@@ -781,24 +784,27 @@ 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:
-		trbe_handle_overflow(handle);
+		truncated = !!trbe_handle_overflow(handle);
 		break;
 	case TRBE_FAULT_ACT_SPURIOUS:
 		trbe_handle_spurious(handle);
 		break;
 	case TRBE_FAULT_ACT_FATAL:
 		trbe_stop_and_truncate_event(handle);
+		truncated = true;
 		break;
 	}
+
+	/*
+	 * If the buffer was truncated, ensure perf callbacks
+	 * have completed, which will disable the event.
+	 */
+	if (truncated)
+		irq_work_run();
+
 	return IRQ_HANDLED;
 }
 
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 10/10] coresight: trbe: Prohibit trace before disabling TRBE

[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, whenever
possible. We might also leave the TRBE disabled, if there is not
enough space left in the buffer. However, we do not touch the ETE at
all during all of this. This means 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 (even if it is for a short duration).

So, we move the CPU into trace prohibited state always before disabling
the TRBE, upon entering the IRQ handler. The state is restored if the
TRBE is enabled back. Otherwise the trace remains prohibited.

Since, the ETM/ETE driver now controls the TRFCR_EL1 per session, the
tracing can be restored/enabled back when the event is rescheduled
in.

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>
---
 .../hwtracing/coresight/coresight-self-hosted-trace.h    | 4 +++-
 drivers/hwtracing/coresight/coresight-trbe.c             | 9 +++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
index 586d26e0cba3..34372482a3cd 100644
--- a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
+++ b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
@@ -22,11 +22,13 @@ static inline void write_trfcr(u64 val)
 	isb();
 }
 
-static inline void cpu_prohibit_trace(void)
+static inline u64 cpu_prohibit_trace(void)
 {
 	u64 trfcr = read_trfcr();
 
 	/* Prohibit tracing at EL0 & the kernel EL */
 	write_trfcr(trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE));
+	/* Return the original value of the TRFCR */
+	return trfcr;
 }
 #endif			/*  __CORESIGHT_SELF_HOSTED_TRACE_H */
diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index d05cb5ffc128..d4c57aed05e5 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -16,6 +16,7 @@
 #define pr_fmt(fmt) DRVNAME ": " fmt
 
 #include <asm/barrier.h>
+#include "coresight-self-hosted-trace.h"
 #include "coresight-trbe.h"
 
 #define PERF_IDX2OFF(idx, buf) ((idx) % ((buf)->nr_pages << PAGE_SHIFT))
@@ -760,6 +761,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
 	enum trbe_fault_action act;
 	u64 status;
 	bool truncated = false;
+	u64 trfcr;
 
 	/* Reads to TRBSR_EL1 is fine when TRBE is active */
 	status = read_sysreg_s(SYS_TRBSR_EL1);
@@ -770,6 +772,8 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
 	if (!is_trbe_irq(status))
 		return IRQ_NONE;
 
+	/* Prohibit the CPU from tracing before we disable the TRBE */
+	trfcr = cpu_prohibit_trace();
 	/*
 	 * Ensure the trace is visible to the CPUs and
 	 * any external aborts have been resolved.
@@ -801,9 +805,14 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
 	/*
 	 * If the buffer was truncated, ensure perf callbacks
 	 * have completed, which will disable the event.
+	 *
+	 * Otherwise, restore the trace filter controls to
+	 * allow the tracing.
 	 */
 	if (truncated)
 		irq_work_run();
+	else
+		write_trfcr(trfcr);
 
 	return IRQ_HANDLED;
 }
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 02/10] coresight: etm4x: Use Trace Filtering controls dynamically

[Anshuman Khandual]

On 9/14/21 3:56 PM, Suzuki K Poulose wrote:
> 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, which doesn't prevent the ETM from generating Context
> packets for an "excluded" EL. At the moment, we do a onetime
> enable trace at user and kernel and leave it untouched for the
> kernel life time. This implies that the ETM could potentially
> generate trace packets containing the kernel addresses, and
> thus leaking the kernel virtual address in the trace.
> 
> 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>

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

> ---
>  .../coresight/coresight-etm4x-core.c          | 63 ++++++++++++++-----
>  drivers/hwtracing/coresight/coresight-etm4x.h |  7 ++-
>  .../coresight/coresight-self-hosted-trace.h   |  7 +++
>  3 files changed, 59 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 537c0d7ee1ed..d9a85ca25b60 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -239,6 +239,45 @@ struct etm4_enable_arg {
>  	int rc;
>  };
>  
> +/*
> + * 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 the CPU doesn't support FEAT_TRF, nothing to do */
> +	if (!drvdata->trfcr)
> +		return;
> +	cpu_prohibit_trace();
> +}
> +
> +/*
> + * 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
> @@ -443,6 +482,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);
>  
> @@ -738,7 +778,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;
> @@ -765,12 +804,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,
> @@ -786,9 +820,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] =
> @@ -990,15 +1021,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
> @@ -1012,7 +1043,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)
> @@ -1203,7 +1234,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)
> @@ -1699,7 +1730,7 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
>  	int ret = 0;
>  
>  	/* Save the TRFCR irrespective of whether the ETM is ON */
> -	if (drvdata->trfc)
> +	if (drvdata->trfcr)
>  		drvdata->save_trfcr = read_trfcr();
>  	/*
>  	 * Save and restore the ETM Trace registers only if
> @@ -1808,7 +1839,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..3c4d69b096ca 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -919,7 +919,10 @@ 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 supports FEAT_TRF, value of the TRFCR_ELx that
> + *		allows tracing at all ELs. We don't want to compute this
> + *		at runtime, due to the additional setting of TRFCR_CX when
> + *		in EL2. 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 +975,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;
> diff --git a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
> index 53b35a28075e..586d26e0cba3 100644
> --- a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
> +++ b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
> @@ -22,4 +22,11 @@ static inline void write_trfcr(u64 val)
>  	isb();
>  }
>  
> +static inline void cpu_prohibit_trace(void)
> +{
> +	u64 trfcr = read_trfcr();
> +
> +	/* Prohibit tracing at EL0 & the kernel EL */
> +	write_trfcr(trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE));
> +}
>  #endif			/*  __CORESIGHT_SELF_HOSTED_TRACE_H */
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 04/10] coresight: trbe: Ensure the format flag is set always

[Anshuman Khandual]

On 9/14/21 3:56 PM, Suzuki K Poulose wrote:
> When the TRBE is stopped on truncating an event, we may not
> set the FORMAT flag, even though the size of the record is 0.
> Let us be consistent and not confuse the user.
> 
> To ensure that the format flag is always set on all the
> records generated by TRBE, set the flag when we have a
> new handle. Rather than deferring to the "end" operation,
> which makes it clear. So, we can do this from
> 
>  -  arm_trbe_enable() -> When a new handle is provided by the
>     CoreSight PMU, triggered via etm_event_start()
>  -  trbe_handle_overflow() -> When we begin a new handle after
>     closing the previous on overflow.
> 
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> Changes since v2:
>   - Move the format flag setting to the beginning of a session
>     than the ending (which can happen via various different paths)

Right, this has improved from the last version.

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 176868496879..81bf183a73a1 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -554,8 +554,6 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev,
>  	if (cpudata->mode != CS_MODE_PERF)
>  		return 0;
>  
> -	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
> -
>  	/*
>  	 * We are about to disable the TRBE. And this could in turn
>  	 * fill up the buffer triggering, an IRQ. This could be consumed
> @@ -648,6 +646,7 @@ static int arm_trbe_enable(struct coresight_device *csdev, u32 mode, void *data)
>  	if (mode != CS_MODE_PERF)
>  		return -EINVAL;
>  
> +	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
>  	*this_cpu_ptr(drvdata->handle) = handle;
>  	cpudata->buf = buf;
>  	cpudata->mode = mode;
> @@ -710,8 +709,7 @@ 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.
>  	 */
> -	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW |
> -				     PERF_AUX_FLAG_TRUNCATED);
> +	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
>  	perf_aux_output_end(handle, size);
>  	event_data = perf_aux_output_begin(handle, event);
>  	if (!event_data) {
> @@ -725,6 +723,7 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
>  		*this_cpu_ptr(buf->cpudata->drvdata->handle) = NULL;
>  		return;
>  	}
> +	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
>  	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) {
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 05/10] coresight: trbe: Drop duplicate TRUNCATE flags

[Anshuman Khandual]

On 9/14/21 3:56 PM, Suzuki K Poulose wrote:
> We mark the buffer as TRUNCATED when there is no space left
> in the buffer. But we do it at different points.
>     __trbe_normal_offset()
> and also, at all the callers of the above function via
> compute_trbe_buffer_limit(), when the limit == base (i.e
> offset = 0 as returned by the __trbe_normal_offset()).
> 
> So, given that the callers already mark the buffer as TRUNCATED
> drop the caller inside the __trbe_normal_offset().
> 
> This is in preparation to moving the handling of TRUNCATED
> into a central place.
> 
> 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>

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

A small nit though.

> ---
> Change since v2:
>   - Added the flag for trbe_handle_spurious() -  Anshuman
> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 81bf183a73a1..5297b11f26b7 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -252,13 +252,9 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)
>  	 * trbe_base				trbe_base + nr_pages
>  	 *
>  	 * Perf aux buffer does not have any space for the driver to write into.
> -	 * Just communicate trace truncation event to the user space by marking
> -	 * it with PERF_AUX_FLAG_TRUNCATED.

Should not this comment be moved, where PERF_AUX_FLAG_TRUNCATED
gets marked after this change ?

>  	 */
> -	if (!handle->size) {
> -		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +	if (!handle->size)
>  		return 0;
> -	}
>  
>  	/* Compute the tail and wakeup indices now that we've aligned head */
>  	tail = PERF_IDX2OFF(handle->head + handle->size, buf);
> @@ -360,7 +356,6 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)
>  		return limit;
>  
>  	trbe_pad_buf(handle, handle->size);
> -	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
>  	return 0;
>  }
>  
> @@ -688,6 +683,7 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
>  	buf->trbe_write = buf->trbe_base + PERF_IDX2OFF(handle->head, buf);
>  	if (buf->trbe_limit == buf->trbe_base) {
>  		trbe_drain_and_disable_local();
> +		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
>  		return;
>  	}
>  	trbe_enable_hw(buf);
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 06/10] coresight: trbe: Fix handling of spurious interrupts

[Anshuman Khandual]

On 9/14/21 3:56 PM, Suzuki K Poulose wrote:
> On a spurious IRQ, right now we disable the TRBE and then re-enable
> it back, resetting the "buffer" pointers(i.e BASE, LIMIT and more
> importantly WRITE) to the original pointers from the AUX handle.
> This implies that we overwrite any trace that was written so far,
> (by overwriting TRBPTR) while we should have ignored the IRQ.
> 
> This patch cleans the behavior, by only stopping the TRBE if the
> IRQ was indeed raised, as we can read the TRBSR without stopping
> the TRBE (Only writes to the TRBSR requires the TRBE disabled).
> And also, on detecting a spurious IRQ after examining the TRBSR,
> we simply re-enable the TRBE without touching the other parameters.
> 
> 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>

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 30 ++++++++++----------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 5297b11f26b7..de99dd0aecd3 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -677,16 +677,16 @@ static int arm_trbe_disable(struct coresight_device *csdev)
>  
>  static void trbe_handle_spurious(struct perf_output_handle *handle)
>  {
> -	struct trbe_buf *buf = etm_perf_sink_config(handle);
> +	u64 limitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
>  
> -	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_drain_and_disable_local();
> -		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> -		return;
> -	}
> -	trbe_enable_hw(buf);
> +	/*
> +	 * If the IRQ was spurious, simply re-enable the TRBE
> +	 * back without modifying the buffer parameters to
> +	 * retain the trace collected so far.
> +	 */
> +	limitr |= TRBLIMITR_ENABLE;
> +	write_sysreg_s(limitr, SYS_TRBLIMITR_EL1);
> +	isb();
>  }
>  
>  static void trbe_handle_overflow(struct perf_output_handle *handle)
> @@ -759,12 +759,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>  	enum trbe_fault_action act;
>  	u64 status;
>  
> -	/*
> -	 * Ensure the trace is visible to the CPUs and
> -	 * any external aborts have been resolved.
> -	 */
> -	trbe_drain_and_disable_local();
> -
> +	/* Reads to TRBSR_EL1 is fine when TRBE is active */
>  	status = read_sysreg_s(SYS_TRBSR_EL1);
>  	/*
>  	 * If the pending IRQ was handled by update_buffer callback
> @@ -773,6 +768,11 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>  	if (!is_trbe_irq(status))
>  		return IRQ_NONE;
>  
> +	/*
> +	 * Ensure the trace is visible to the CPUs and
> +	 * any external aborts have been resolved.
> +	 */
> +	trbe_drain_and_disable_local();
>  	clr_trbe_irq();
>  	isb();
>  
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 07/10] coresight: trbe: Do not truncate buffer on IRQ

[Anshuman Khandual]

On 9/14/21 3:56 PM, Suzuki K Poulose wrote:
> The TRBE driver marks the AUX buffer as TRUNCATED when we get an IRQ
> on FILL event. This has rather unwanted side-effect of the event
> being disabled when there may be more space in the ring buffer.
> 
> So, instead of TRUNCATE we need a different flag to indicate
> that the trace may have lost a few bytes (i.e from the point of
> generating the FILL event until the IRQ is consumed). Anyways, the
> userspace must use the size from RECORD_AUX headers to restrict
> the "trace" decoding.
> 
> Using PARTIAL flag causes the perf tool to generate the
> following warning:
> 
>   Warning:
>   AUX data had gaps in it XX times out of YY!
> 
>   Are you running a KVM guest in the background?
> 
> which is pointlessly scary for a user. The other remaining options
> are :
>   - COLLISION - Use by SPE to indicate samples collided
>   - Add a new flag - Specifically for CoreSight, doesn't sound
>     so good, if we can re-use something.
> 
> Given that we don't already use the "COLLISION" flag, the above
> behavior can be notified using this flag for CoreSight.
> 
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: James Clark <james.clark@arm.com>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Leo Yan <leo.yan@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

> ---
> Changes since v2:
>   - The perf tool patch for reporting collisions is queued.
> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index de99dd0aecd3..a1a15fa6c4ae 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -612,7 +612,7 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev,
>  		 * for correct size. Also, mark the buffer truncated.
>  		 */
>  		write = get_trbe_limit_pointer();
> -		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +		perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
>  	}
>  
>  	offset = write - base;
> @@ -705,7 +705,7 @@ 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.
>  	 */
> -	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +	perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
>  	perf_aux_output_end(handle, size);
>  	event_data = perf_aux_output_begin(handle, event);
>  	if (!event_data) {
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 10/10] coresight: trbe: Prohibit trace before disabling TRBE

[Anshuman Khandual]

On 9/14/21 3:56 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, whenever
> possible. We might also leave the TRBE disabled, if there is not
> enough space left in the buffer. However, we do not touch the ETE at
> all during all of this. This means 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 (even if it is for a short duration).
> 
> So, we move the CPU into trace prohibited state always before disabling
> the TRBE, upon entering the IRQ handler. The state is restored if the
> TRBE is enabled back. Otherwise the trace remains prohibited.
> 
> Since, the ETM/ETE driver now controls the TRFCR_EL1 per session, the
> tracing can be restored/enabled back when the event is rescheduled
> in.
> 
> 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>

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

> ---
>  .../hwtracing/coresight/coresight-self-hosted-trace.h    | 4 +++-
>  drivers/hwtracing/coresight/coresight-trbe.c             | 9 +++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
> index 586d26e0cba3..34372482a3cd 100644
> --- a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
> +++ b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
> @@ -22,11 +22,13 @@ static inline void write_trfcr(u64 val)
>  	isb();
>  }
>  
> -static inline void cpu_prohibit_trace(void)
> +static inline u64 cpu_prohibit_trace(void)
>  {
>  	u64 trfcr = read_trfcr();
>  
>  	/* Prohibit tracing at EL0 & the kernel EL */
>  	write_trfcr(trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE));
> +	/* Return the original value of the TRFCR */
> +	return trfcr;
>  }
>  #endif			/*  __CORESIGHT_SELF_HOSTED_TRACE_H */
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index d05cb5ffc128..d4c57aed05e5 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -16,6 +16,7 @@
>  #define pr_fmt(fmt) DRVNAME ": " fmt
>  
>  #include <asm/barrier.h>
> +#include "coresight-self-hosted-trace.h"
>  #include "coresight-trbe.h"
>  
>  #define PERF_IDX2OFF(idx, buf) ((idx) % ((buf)->nr_pages << PAGE_SHIFT))
> @@ -760,6 +761,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>  	enum trbe_fault_action act;
>  	u64 status;
>  	bool truncated = false;
> +	u64 trfcr;
>  
>  	/* Reads to TRBSR_EL1 is fine when TRBE is active */
>  	status = read_sysreg_s(SYS_TRBSR_EL1);
> @@ -770,6 +772,8 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>  	if (!is_trbe_irq(status))
>  		return IRQ_NONE;
>  
> +	/* Prohibit the CPU from tracing before we disable the TRBE */
> +	trfcr = cpu_prohibit_trace();
>  	/*
>  	 * Ensure the trace is visible to the CPUs and
>  	 * any external aborts have been resolved.
> @@ -801,9 +805,14 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>  	/*
>  	 * If the buffer was truncated, ensure perf callbacks
>  	 * have completed, which will disable the event.
> +	 *
> +	 * Otherwise, restore the trace filter controls to
> +	 * allow the tracing.
>  	 */
>  	if (truncated)
>  		irq_work_run();
> +	else
> +		write_trfcr(trfcr);
>  
>  	return IRQ_HANDLED;
>  }
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 01/10] coresight: etm4x: Save restore TRFCR_EL1

[Mathieu Poirier]

Good morning,

On Tue, Sep 14, 2021 at 11:26:32AM +0100, Suzuki K Poulose wrote:
> 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.
> 
> The TRFCR related helpers are in a new header file, as we need to use
> them for TRBE in the later patches.
> 
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Leo Yan <leo.yan@linaro.org>
> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  .../coresight/coresight-etm4x-core.c          | 43 +++++++++++++------
>  drivers/hwtracing/coresight/coresight-etm4x.h |  2 +
>  .../coresight/coresight-self-hosted-trace.h   | 25 +++++++++++
>  3 files changed, 58 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/hwtracing/coresight/coresight-self-hosted-trace.h
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index e24252eaf8e4..537c0d7ee1ed 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -40,6 +40,7 @@
>  #include "coresight-etm4x.h"
>  #include "coresight-etm-perf.h"
>  #include "coresight-etm4x-cfg.h"
> +#include "coresight-self-hosted-trace.h"
>  #include "coresight-syscfg.h"
>  
>  static int boot_enable;
> @@ -1011,7 +1012,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)
> @@ -1554,7 +1555,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;
> @@ -1693,7 +1694,23 @@ 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;
> +
> +	/* Save the TRFCR irrespective of whether the ETM is ON */
> +	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;
> @@ -1789,6 +1806,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)
>  {
> @@ -1800,23 +1825,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;
> diff --git a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
> new file mode 100644
> index 000000000000..53b35a28075e
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +

Extra line

> +/*
> + * Arm v8 Self-Hosted trace support.
> + *
> + * Copyright (C) 2021 ARM Ltd.
> + */
> +
> +#ifndef __CORESIGHT_SELF_HOSTED_TRACE_H
> +#define __CORESIGHT_SELF_HOSTED_TRACE_H
> +
> +#include <asm/sysreg.h>
> +
> +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();
> +}
> +
> +#endif			/*  __CORESIGHT_SELF_HOSTED_TRACE_H */

This came out weird.

I fixed the above and applied the patch.

Thanks,
Mathieu

> -- 
> 2.24.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 06/10] coresight: trbe: Fix handling of spurious interrupts

[Mathieu Poirier]

On Tue, Sep 14, 2021 at 11:26:37AM +0100, Suzuki K Poulose wrote:
> On a spurious IRQ, right now we disable the TRBE and then re-enable
> it back, resetting the "buffer" pointers(i.e BASE, LIMIT and more
> importantly WRITE) to the original pointers from the AUX handle.
> This implies that we overwrite any trace that was written so far,
> (by overwriting TRBPTR) while we should have ignored the IRQ.
> 
> This patch cleans the behavior, by only stopping the TRBE if the
> IRQ was indeed raised, as we can read the TRBSR without stopping
> the TRBE (Only writes to the TRBSR requires the TRBE disabled).
> And also, on detecting a spurious IRQ after examining the TRBSR,
> we simply re-enable the TRBE without touching the other parameters.
> 
> 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 | 30 ++++++++++----------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 5297b11f26b7..de99dd0aecd3 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -677,16 +677,16 @@ static int arm_trbe_disable(struct coresight_device *csdev)
>  
>  static void trbe_handle_spurious(struct perf_output_handle *handle)
>  {
> -	struct trbe_buf *buf = etm_perf_sink_config(handle);
> +	u64 limitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
>  
> -	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_drain_and_disable_local();
> -		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> -		return;
> -	}
> -	trbe_enable_hw(buf);
> +	/*
> +	 * If the IRQ was spurious, simply re-enable the TRBE
> +	 * back without modifying the buffer parameters to
> +	 * retain the trace collected so far.
> +	 */
> +	limitr |= TRBLIMITR_ENABLE;
> +	write_sysreg_s(limitr, SYS_TRBLIMITR_EL1);
> +	isb();

I understand (and agree with) this part of the patch...

>  }
>  
>  static void trbe_handle_overflow(struct perf_output_handle *handle)
> @@ -759,12 +759,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>  	enum trbe_fault_action act;
>  	u64 status;
>  
> -	/*
> -	 * Ensure the trace is visible to the CPUs and
> -	 * any external aborts have been resolved.
> -	 */
> -	trbe_drain_and_disable_local();
> -
> +	/* Reads to TRBSR_EL1 is fine when TRBE is active */
>  	status = read_sysreg_s(SYS_TRBSR_EL1);
>  	/*
>  	 * If the pending IRQ was handled by update_buffer callback
> @@ -773,6 +768,11 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>  	if (!is_trbe_irq(status))
>  		return IRQ_NONE;
>  
> +	/*
> +	 * Ensure the trace is visible to the CPUs and
> +	 * any external aborts have been resolved.
> +	 */
> +	trbe_drain_and_disable_local();

But not this part...  I can see why you'd want to move this after the check for
is_trbe_irq(), but not how it relates to spurious interrupts.  To me it seems
like it is addressing another issue.  If those code snippets are related then a
good dose of comments is missing.

Thanks,
Mathieu

>  	clr_trbe_irq();
>  	isb();
>  
> -- 
> 2.24.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 07/10] coresight: trbe: Do not truncate buffer on IRQ

[Mathieu Poirier]

On Tue, Sep 14, 2021 at 11:26:38AM +0100, Suzuki K Poulose wrote:
> The TRBE driver marks the AUX buffer as TRUNCATED when we get an IRQ
> on FILL event. This has rather unwanted side-effect of the event
> being disabled when there may be more space in the ring buffer.
> 
> So, instead of TRUNCATE we need a different flag to indicate
> that the trace may have lost a few bytes (i.e from the point of
> generating the FILL event until the IRQ is consumed). Anyways, the
> userspace must use the size from RECORD_AUX headers to restrict
> the "trace" decoding.
> 
> Using PARTIAL flag causes the perf tool to generate the
> following warning:
> 
>   Warning:
>   AUX data had gaps in it XX times out of YY!
> 
>   Are you running a KVM guest in the background?
> 
> which is pointlessly scary for a user. The other remaining options
> are :
>   - COLLISION - Use by SPE to indicate samples collided
>   - Add a new flag - Specifically for CoreSight, doesn't sound
>     so good, if we can re-use something.
> 
> Given that we don't already use the "COLLISION" flag, the above
> behavior can be notified using this flag for CoreSight.
> 
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: James Clark <james.clark@arm.com>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Leo Yan <leo.yan@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> Changes since v2:
>   - The perf tool patch for reporting collisions is queued.
> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index de99dd0aecd3..a1a15fa6c4ae 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -612,7 +612,7 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev,
>  		 * for correct size. Also, mark the buffer truncated.
>  		 */
>  		write = get_trbe_limit_pointer();
> -		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +		perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);

The rational in the changelog should be added here as a comment.

>  	}
>  
>  	offset = write - base;
> @@ -705,7 +705,7 @@ 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.
>  	 */
> -	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +	perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);

Same.

>  	perf_aux_output_end(handle, size);
>  	event_data = perf_aux_output_begin(handle, event);
>  	if (!event_data) {
> -- 
> 2.24.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 07/10] coresight: trbe: Do not truncate buffer on IRQ

[Mathieu Poirier]

On Tue, Sep 21, 2021 at 11:41:10AM -0600, Mathieu Poirier wrote:
> On Tue, Sep 14, 2021 at 11:26:38AM +0100, Suzuki K Poulose wrote:
> > The TRBE driver marks the AUX buffer as TRUNCATED when we get an IRQ
> > on FILL event. This has rather unwanted side-effect of the event
> > being disabled when there may be more space in the ring buffer.
> > 
> > So, instead of TRUNCATE we need a different flag to indicate
> > that the trace may have lost a few bytes (i.e from the point of
> > generating the FILL event until the IRQ is consumed). Anyways, the
> > userspace must use the size from RECORD_AUX headers to restrict
> > the "trace" decoding.
> > 
> > Using PARTIAL flag causes the perf tool to generate the
> > following warning:
> > 
> >   Warning:
> >   AUX data had gaps in it XX times out of YY!
> > 
> >   Are you running a KVM guest in the background?
> > 
> > which is pointlessly scary for a user. The other remaining options
> > are :
> >   - COLLISION - Use by SPE to indicate samples collided
> >   - Add a new flag - Specifically for CoreSight, doesn't sound
> >     so good, if we can re-use something.
> > 
> > Given that we don't already use the "COLLISION" flag, the above
> > behavior can be notified using this flag for CoreSight.
> > 
> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Cc: James Clark <james.clark@arm.com>
> > Cc: Mike Leach <mike.leach@linaro.org>
> > Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> > Cc: Leo Yan <leo.yan@linaro.org>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > ---
> > Changes since v2:
> >   - The perf tool patch for reporting collisions is queued.
> > ---
> >  drivers/hwtracing/coresight/coresight-trbe.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> > index de99dd0aecd3..a1a15fa6c4ae 100644
> > --- a/drivers/hwtracing/coresight/coresight-trbe.c
> > +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> > @@ -612,7 +612,7 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev,
> >  		 * for correct size. Also, mark the buffer truncated.
> >  		 */
> >  		write = get_trbe_limit_pointer();
> > -		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> > +		perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
> 
> The rational in the changelog should be added here as a comment.
> 
> >  	}
> >  
> >  	offset = write - base;
> > @@ -705,7 +705,7 @@ 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.
> >  	 */
> > -	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> > +	perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
> 
> Same.

... and I'm out of time for today.  I will continue tomorrow.

Thanks,
Mathieu

> 
> >  	perf_aux_output_end(handle, size);
> >  	event_data = perf_aux_output_begin(handle, event);
> >  	if (!event_data) {
> > -- 
> > 2.24.1
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 06/10] coresight: trbe: Fix handling of spurious interrupts

[Suzuki K Poulose]

On 21/09/2021 18:24, Mathieu Poirier wrote:
> On Tue, Sep 14, 2021 at 11:26:37AM +0100, Suzuki K Poulose wrote:
>> On a spurious IRQ, right now we disable the TRBE and then re-enable
>> it back, resetting the "buffer" pointers(i.e BASE, LIMIT and more
>> importantly WRITE) to the original pointers from the AUX handle.
>> This implies that we overwrite any trace that was written so far,
>> (by overwriting TRBPTR) while we should have ignored the IRQ.
>>
>> This patch cleans the behavior, by only stopping the TRBE if the
>> IRQ was indeed raised, as we can read the TRBSR without stopping
>> the TRBE (Only writes to the TRBSR requires the TRBE disabled).
>> And also, on detecting a spurious IRQ after examining the TRBSR,
>> we simply re-enable the TRBE without touching the other parameters.
>>
>> 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 | 30 ++++++++++----------
>>   1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>> index 5297b11f26b7..de99dd0aecd3 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -677,16 +677,16 @@ static int arm_trbe_disable(struct coresight_device *csdev)
>>   
>>   static void trbe_handle_spurious(struct perf_output_handle *handle)
>>   {
>> -	struct trbe_buf *buf = etm_perf_sink_config(handle);
>> +	u64 limitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
>>   
>> -	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_drain_and_disable_local();
>> -		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
>> -		return;
>> -	}
>> -	trbe_enable_hw(buf);
>> +	/*
>> +	 * If the IRQ was spurious, simply re-enable the TRBE
>> +	 * back without modifying the buffer parameters to
>> +	 * retain the trace collected so far.
>> +	 */
>> +	limitr |= TRBLIMITR_ENABLE;
>> +	write_sysreg_s(limitr, SYS_TRBLIMITR_EL1);
>> +	isb();
> 
> I understand (and agree with) this part of the patch...
> 
>>   }
>>   
>>   static void trbe_handle_overflow(struct perf_output_handle *handle)
>> @@ -759,12 +759,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>>   	enum trbe_fault_action act;
>>   	u64 status;
>>   
>> -	/*
>> -	 * Ensure the trace is visible to the CPUs and
>> -	 * any external aborts have been resolved.
>> -	 */
>> -	trbe_drain_and_disable_local();
>> -
>> +	/* Reads to TRBSR_EL1 is fine when TRBE is active */
>>   	status = read_sysreg_s(SYS_TRBSR_EL1);
>>   	/*
>>   	 * If the pending IRQ was handled by update_buffer callback

[0] See below

>> @@ -773,6 +768,11 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
>>   	if (!is_trbe_irq(status))
>>   		return IRQ_NONE;
>>   
>> +	/*
>> +	 * Ensure the trace is visible to the CPUs and
>> +	 * any external aborts have been resolved.
>> +	 */
>> +	trbe_drain_and_disable_local();
> 
> But not this part...  I can see why you'd want to move this after the check for
> is_trbe_irq(), but not how it relates to spurious interrupts.  To me it seems
> like it is addressing another issue.  If those code snippets are related then a
> good dose of comments is missing.

This step is to make sure that we stop the TRBE only when there was
really something to process. (i.e, TRBSR indicates an IRQ was raised).
Also, there is a comment [0] above, for handling a case where the TRBE
event was consumed by the "update_buffer()" due to a race with IRQ
handler. Thus we stop the TRBE only when we need to analyse the cause
and take an action. I agree there is a bit of disconnect.

I can think of the following options:

  - Split the patch to 2. with
    1. Don't stop the trbe if there is no IRQ (the bit explained above)
    2. Don't reset the TRBE ptrs on spurious IRQ

    OR

  - Add the above comment to the section.

The commit description has a hint, "This patch cleans the behavior, by 
only stopping the TRBE if the IRQ was indeed raised", but I agree that
the code could be documented too.

Let me know what you think.

Thanks for the review.

Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 04/10] coresight: trbe: Ensure the format flag is set always

[Mathieu Poirier]

On Tue, Sep 14, 2021 at 11:26:35AM +0100, Suzuki K Poulose wrote:
> When the TRBE is stopped on truncating an event, we may not
> set the FORMAT flag, even though the size of the record is 0.
> Let us be consistent and not confuse the user.
> 
> To ensure that the format flag is always set on all the
> records generated by TRBE, set the flag when we have a
> new handle. Rather than deferring to the "end" operation,
> which makes it clear. So, we can do this from
> 
>  -  arm_trbe_enable() -> When a new handle is provided by the
>     CoreSight PMU, triggered via etm_event_start()
>  -  trbe_handle_overflow() -> When we begin a new handle after
>     closing the previous on overflow.
> 
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> Changes since v2:
>   - Move the format flag setting to the beginning of a session
>     than the ending (which can happen via various different paths)
> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>

I have applied patches 1 to 4.

> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 176868496879..81bf183a73a1 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -554,8 +554,6 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev,
>  	if (cpudata->mode != CS_MODE_PERF)
>  		return 0;
>  
> -	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
> -
>  	/*
>  	 * We are about to disable the TRBE. And this could in turn
>  	 * fill up the buffer triggering, an IRQ. This could be consumed
> @@ -648,6 +646,7 @@ static int arm_trbe_enable(struct coresight_device *csdev, u32 mode, void *data)
>  	if (mode != CS_MODE_PERF)
>  		return -EINVAL;
>  
> +	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
>  	*this_cpu_ptr(drvdata->handle) = handle;
>  	cpudata->buf = buf;
>  	cpudata->mode = mode;
> @@ -710,8 +709,7 @@ 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.
>  	 */
> -	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW |
> -				     PERF_AUX_FLAG_TRUNCATED);
> +	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
>  	perf_aux_output_end(handle, size);
>  	event_data = perf_aux_output_begin(handle, event);
>  	if (!event_data) {
> @@ -725,6 +723,7 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
>  		*this_cpu_ptr(buf->cpudata->drvdata->handle) = NULL;
>  		return;
>  	}
> +	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
>  	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) {
> -- 
> 2.24.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 05/10] coresight: trbe: Drop duplicate TRUNCATE flags

[Mathieu Poirier]

On Wed, Sep 15, 2021 at 10:56:17AM +0530, Anshuman Khandual wrote:
> 
> 
> On 9/14/21 3:56 PM, Suzuki K Poulose wrote:
> > We mark the buffer as TRUNCATED when there is no space left
> > in the buffer. But we do it at different points.
> >     __trbe_normal_offset()
> > and also, at all the callers of the above function via
> > compute_trbe_buffer_limit(), when the limit == base (i.e
> > offset = 0 as returned by the __trbe_normal_offset()).
> > 
> > So, given that the callers already mark the buffer as TRUNCATED
> > drop the caller inside the __trbe_normal_offset().
> > 
> > This is in preparation to moving the handling of TRUNCATED
> > into a central place.
> > 
> > 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>
> 
> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
> 
> A small nit though.
> 
> > ---
> > Change since v2:
> >   - Added the flag for trbe_handle_spurious() -  Anshuman
> > ---
> >  drivers/hwtracing/coresight/coresight-trbe.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> > index 81bf183a73a1..5297b11f26b7 100644
> > --- a/drivers/hwtracing/coresight/coresight-trbe.c
> > +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> > @@ -252,13 +252,9 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)
> >  	 * trbe_base				trbe_base + nr_pages
> >  	 *
> >  	 * Perf aux buffer does not have any space for the driver to write into.
> > -	 * Just communicate trace truncation event to the user space by marking
> > -	 * it with PERF_AUX_FLAG_TRUNCATED.
> 
> Should not this comment be moved, where PERF_AUX_FLAG_TRUNCATED
> gets marked after this change ?

I moved the comment and applied this patch.

> 
> >  	 */
> > -	if (!handle->size) {
> > -		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> > +	if (!handle->size)
> >  		return 0;
> > -	}
> >  
> >  	/* Compute the tail and wakeup indices now that we've aligned head */
> >  	tail = PERF_IDX2OFF(handle->head + handle->size, buf);
> > @@ -360,7 +356,6 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)
> >  		return limit;
> >  
> >  	trbe_pad_buf(handle, handle->size);
> > -	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> >  	return 0;
> >  }
> >  
> > @@ -688,6 +683,7 @@ static void trbe_handle_spurious(struct perf_output_handle *handle)
> >  	buf->trbe_write = buf->trbe_base + PERF_IDX2OFF(handle->head, buf);
> >  	if (buf->trbe_limit == buf->trbe_base) {
> >  		trbe_drain_and_disable_local();
> > +		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> >  		return;
> >  	}
> >  	trbe_enable_hw(buf);
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 08/10] coresight: trbe: Unify the enabling sequence

[Mathieu Poirier]

On Tue, Sep 14, 2021 at 11:26:39AM +0100, Suzuki K Poulose wrote:
> Unify the sequence of enabling the TRBE. We do this from
> event_start and also from the TRBE IRQ handler. Lets move
> this to a common helper. The only minor functional change
> is returning an error when we fail to enable the TRBE.
> This should be handled already.
> 
> Since we now have unique entry point to trying to enable TRBE,
> move the format flag setting to the central place.
> 
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Leo Yan <leo.yan@linaro.org>
> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> Changes since v2:
>  - Removed redundant handle book keeping (Anshuman)
>  - Moved the format flag setting to the helper
> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 37 ++++++++++----------
>  1 file changed, 18 insertions(+), 19 deletions(-)
>

Applied.

> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index a1a15fa6c4ae..25c16d0f9e49 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -629,6 +629,21 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev,
>  	return size;
>  }
>  
> +static int __arm_trbe_enable(struct trbe_buf *buf,
> +			     struct perf_output_handle *handle)
> +{
> +	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
> +	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 -ENOSPC;
> +	}
> +	*this_cpu_ptr(buf->cpudata->drvdata->handle) = handle;
> +	trbe_enable_hw(buf);
> +	return 0;
> +}
> +
>  static int arm_trbe_enable(struct coresight_device *csdev, u32 mode, void *data)
>  {
>  	struct trbe_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> @@ -641,19 +656,11 @@ static int arm_trbe_enable(struct coresight_device *csdev, u32 mode, void *data)
>  	if (mode != CS_MODE_PERF)
>  		return -EINVAL;
>  
> -	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
> -	*this_cpu_ptr(drvdata->handle) = handle;
>  	cpudata->buf = buf;
>  	cpudata->mode = mode;
>  	buf->cpudata = cpudata;
> -	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 0;
> -	}
> -	trbe_enable_hw(buf);
> -	return 0;
> +
> +	return __arm_trbe_enable(buf, handle);
>  }
>  
>  static int arm_trbe_disable(struct coresight_device *csdev)
> @@ -719,15 +726,7 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
>  		*this_cpu_ptr(buf->cpudata->drvdata->handle) = NULL;
>  		return;
>  	}
> -	perf_aux_output_flag(handle, PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
> -	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);
> +	__arm_trbe_enable(buf, handle);
>  }
>  
>  static bool is_perf_trbe(struct perf_output_handle *handle)
> -- 
> 2.24.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 06/10] coresight: trbe: Fix handling of spurious interrupts

[Mathieu Poirier]

On Tue, Sep 21, 2021 at 10:29:20PM +0100, Suzuki K Poulose wrote:
> On 21/09/2021 18:24, Mathieu Poirier wrote:
> > On Tue, Sep 14, 2021 at 11:26:37AM +0100, Suzuki K Poulose wrote:
> > > On a spurious IRQ, right now we disable the TRBE and then re-enable
> > > it back, resetting the "buffer" pointers(i.e BASE, LIMIT and more
> > > importantly WRITE) to the original pointers from the AUX handle.
> > > This implies that we overwrite any trace that was written so far,
> > > (by overwriting TRBPTR) while we should have ignored the IRQ.
> > > 
> > > This patch cleans the behavior, by only stopping the TRBE if the
> > > IRQ was indeed raised, as we can read the TRBSR without stopping
> > > the TRBE (Only writes to the TRBSR requires the TRBE disabled).
> > > And also, on detecting a spurious IRQ after examining the TRBSR,
> > > we simply re-enable the TRBE without touching the other parameters.
> > > 
> > > 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 | 30 ++++++++++----------
> > >   1 file changed, 15 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> > > index 5297b11f26b7..de99dd0aecd3 100644
> > > --- a/drivers/hwtracing/coresight/coresight-trbe.c
> > > +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> > > @@ -677,16 +677,16 @@ static int arm_trbe_disable(struct coresight_device *csdev)
> > >   static void trbe_handle_spurious(struct perf_output_handle *handle)
> > >   {
> > > -	struct trbe_buf *buf = etm_perf_sink_config(handle);
> > > +	u64 limitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
> > > -	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_drain_and_disable_local();
> > > -		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> > > -		return;
> > > -	}
> > > -	trbe_enable_hw(buf);
> > > +	/*
> > > +	 * If the IRQ was spurious, simply re-enable the TRBE
> > > +	 * back without modifying the buffer parameters to
> > > +	 * retain the trace collected so far.
> > > +	 */
> > > +	limitr |= TRBLIMITR_ENABLE;
> > > +	write_sysreg_s(limitr, SYS_TRBLIMITR_EL1);
> > > +	isb();
> > 
> > I understand (and agree with) this part of the patch...
> > 
> > >   }
> > >   static void trbe_handle_overflow(struct perf_output_handle *handle)
> > > @@ -759,12 +759,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
> > >   	enum trbe_fault_action act;
> > >   	u64 status;
> > > -	/*
> > > -	 * Ensure the trace is visible to the CPUs and
> > > -	 * any external aborts have been resolved.
> > > -	 */
> > > -	trbe_drain_and_disable_local();
> > > -
> > > +	/* Reads to TRBSR_EL1 is fine when TRBE is active */
> > >   	status = read_sysreg_s(SYS_TRBSR_EL1);
> > >   	/*
> > >   	 * If the pending IRQ was handled by update_buffer callback
> 
> [0] See below
> 
> > > @@ -773,6 +768,11 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
> > >   	if (!is_trbe_irq(status))
> > >   		return IRQ_NONE;
> > > +	/*
> > > +	 * Ensure the trace is visible to the CPUs and
> > > +	 * any external aborts have been resolved.
> > > +	 */
> > > +	trbe_drain_and_disable_local();
> > 
> > But not this part...  I can see why you'd want to move this after the check for
> > is_trbe_irq(), but not how it relates to spurious interrupts.  To me it seems
> > like it is addressing another issue.  If those code snippets are related then a
> > good dose of comments is missing.
> 
> This step is to make sure that we stop the TRBE only when there was
> really something to process. (i.e, TRBSR indicates an IRQ was raised).
> Also, there is a comment [0] above, for handling a case where the TRBE
> event was consumed by the "update_buffer()" due to a race with IRQ
> handler. Thus we stop the TRBE only when we need to analyse the cause
> and take an action. I agree there is a bit of disconnect.
> 
> I can think of the following options:
> 
>  - Split the patch to 2. with
>    1. Don't stop the trbe if there is no IRQ (the bit explained above)
>    2. Don't reset the TRBE ptrs on spurious IRQ

Please do two patches.

Other than this patch I commented on 07 and picked up 08.  Patches 09 and 10
won't apply if 06 and 07 aren't present so please address comments for 06 and 07
and resend all 4 patches (06, 07, 09, 10).

Thanks,
Mathieu

> 
>    OR
> 
>  - Add the above comment to the section.
> 
> The commit description has a hint, "This patch cleans the behavior, by only
> stopping the TRBE if the IRQ was indeed raised", but I agree that
> the code could be documented too.
> 
> Let me know what you think.
> 
> Thanks for the review.
> 
> Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel