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

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

[Suzuki K Poulose]

Here is the updated patches from the v3 of the series [0], parts of which
have been queued to coresight/next. This series now applies on the
coresight/next branch.

Changes since v3:
 - Split the spurious IRQ handling patch to :
   a) coresight: trbe: irq handler: Do not disable TRBE if no action is needed
   b) coresight: trbe: Fix handling of spurious interrupts

 - Added a helper to mark the ring buffer when there is WRAP event
   and added a comment to explain.

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

Suzuki K Poulose (5):
  coresight: trbe: irq handler: Do not disable TRBE if no action is needed
  coresight: trbe: Fix handling of spurious interrupts
  coresight: trbe: Do not truncate buffer on IRQ
  coresight: trbe: End the AUX handle on truncation
  coresight: trbe: Prohibit trace before disabling TRBE

 .../coresight/coresight-self-hosted-trace.h   |  4 +-
 drivers/hwtracing/coresight/coresight-trbe.c  | 96 ++++++++++++-------
 2 files changed, 64 insertions(+), 36 deletions(-)

-- 
2.24.1


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

[PATCH v4 1/5] coresight: trbe: irq handler: Do not disable TRBE if no action is needed

[Suzuki K Poulose]

The IRQ handler of the TRBE driver could race against the update_buffer()
in consuming the  IRQ. So, if the update_buffer() gets to processing the
TRBE irq, the TRBSR will be cleared. Thus by the time IRQ handler is
triggered, there is nothing to do there. Handle these cases and do not
disable the TRBE unnecessarily. Since the TRBSR can be read without
stopping the TRBE, we can check that before disabling the TRBE.

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

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 2cf28de2e447..e78800ba5b5b 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -762,12 +762,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
@@ -776,6 +771,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 v4 2/5] 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.

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 | 22 ++++++++------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index e78800ba5b5b..47120d263639 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -684,20 +684,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();
-		/*
-		 * Just communicate trace truncation event to the user space by
-		 * marking it with PERF_AUX_FLAG_TRUNCATED.
-		 */
-		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)
-- 
2.24.1


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

[PATCH v4 3/5] 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 v3:
 - Moved setting the COLLISION flag to a helper and added
   comments to explain.
---
 drivers/hwtracing/coresight/coresight-trbe.c | 27 +++++++++++++++-----
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 47120d263639..0a9106c15639 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -120,6 +120,25 @@ static void trbe_reset_local(void)
 	write_sysreg_s(0, SYS_TRBSR_EL1);
 }
 
+static void trbe_report_wrap_event(struct perf_output_handle *handle)
+{
+	/*
+	 * Mark the buffer to indicate that there was a WRAP event by
+	 * setting the COLLISION flag. This indicates to the user that
+	 * the TRBE trace collection was stopped without stopping the
+	 * ETE and thus there might be some amount of trace that was
+	 * lost between the time the WRAP was detected and the IRQ
+	 * was consumed by the CPU.
+	 *
+	 * Setting the TRUNCATED flag would move the event to STOPPED
+	 * state unnecessarily, even when there is space left in the
+	 * ring buffer. Using the COLLISION flag doesn't have this side
+	 * effect. We only set TRUNCATED flag when there is no space
+	 * left in the ring buffer.
+	 */
+	perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
+}
+
 static void trbe_stop_and_truncate_event(struct perf_output_handle *handle)
 {
 	struct trbe_buf *buf = etm_perf_sink_config(handle);
@@ -612,7 +631,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);
+		trbe_report_wrap_event(handle);
 	}
 
 	offset = write - base;
@@ -708,11 +727,7 @@ static void trbe_handle_overflow(struct perf_output_handle *handle)
 	if (buf->snapshot)
 		handle->head += size;
 
-	/*
-	 * 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);
+	trbe_report_wrap_event(handle);
 	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 v4 4/5] 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 0a9106c15639..4174300f1344 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -152,6 +152,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;
 }
 
@@ -715,7 +716,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);
@@ -739,9 +740,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)
@@ -772,6 +774,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);
@@ -796,24 +799,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 v4 5/5] 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>
Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
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 23f05df3f173..53840a2c41f2 100644
--- a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
+++ b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
@@ -21,11 +21,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 4174300f1344..a53ee98f312f 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))
@@ -775,6 +776,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);
@@ -785,6 +787,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.
@@ -816,9 +820,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 v4 0/5] coresight: TRBE and Self-Hosted trace fixes

[Mathieu Poirier]

On Thu, Sep 23, 2021 at 03:39:14PM +0100, Suzuki K Poulose wrote:
> Here is the updated patches from the v3 of the series [0], parts of which
> have been queued to coresight/next. This series now applies on the
> coresight/next branch.
> 
> Changes since v3:
>  - Split the spurious IRQ handling patch to :
>    a) coresight: trbe: irq handler: Do not disable TRBE if no action is needed
>    b) coresight: trbe: Fix handling of spurious interrupts
> 
>  - Added a helper to mark the ring buffer when there is WRAP event
>    and added a comment to explain.
> 
> [0] https://lkml.kernel.org/r/20210914102641.1852544-1-suzuki.poulose@arm.com
> 
> Suzuki K Poulose (5):
>   coresight: trbe: irq handler: Do not disable TRBE if no action is needed
>   coresight: trbe: Fix handling of spurious interrupts
>   coresight: trbe: Do not truncate buffer on IRQ
>   coresight: trbe: End the AUX handle on truncation
>   coresight: trbe: Prohibit trace before disabling TRBE
> 
>  .../coresight/coresight-self-hosted-trace.h   |  4 +-
>  drivers/hwtracing/coresight/coresight-trbe.c  | 96 ++++++++++++-------
>  2 files changed, 64 insertions(+), 36 deletions(-)

Applied and pushed - thanks for the modifications.

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 v4 0/5] coresight: TRBE and Self-Hosted trace fixes

[Suzuki K Poulose]

On 23/09/2021 17:55, Mathieu Poirier wrote:
> On Thu, Sep 23, 2021 at 03:39:14PM +0100, Suzuki K Poulose wrote:
>> Here is the updated patches from the v3 of the series [0], parts of which
>> have been queued to coresight/next. This series now applies on the
>> coresight/next branch.
>>
>> Changes since v3:
>>   - Split the spurious IRQ handling patch to :
>>     a) coresight: trbe: irq handler: Do not disable TRBE if no action is needed
>>     b) coresight: trbe: Fix handling of spurious interrupts
>>
>>   - Added a helper to mark the ring buffer when there is WRAP event
>>     and added a comment to explain.
>>
>> [0] https://lkml.kernel.org/r/20210914102641.1852544-1-suzuki.poulose@arm.com
>>
>> Suzuki K Poulose (5):
>>    coresight: trbe: irq handler: Do not disable TRBE if no action is needed
>>    coresight: trbe: Fix handling of spurious interrupts
>>    coresight: trbe: Do not truncate buffer on IRQ
>>    coresight: trbe: End the AUX handle on truncation
>>    coresight: trbe: Prohibit trace before disabling TRBE
>>
>>   .../coresight/coresight-self-hosted-trace.h   |  4 +-
>>   drivers/hwtracing/coresight/coresight-trbe.c  | 96 ++++++++++++-------
>>   2 files changed, 64 insertions(+), 36 deletions(-)
> 
> Applied and pushed - thanks for the modifications.

Thank you for the review and support !

Suzuki

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