From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-23 12:46:24
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,
it 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.
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
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 on truncation
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 | 98 ++++++++++++----
drivers/hwtracing/coresight/coresight-etm4x.h | 7 +-
.../coresight/coresight-self-hosted-trace.h | 34 ++++++
drivers/hwtracing/coresight/coresight-trbe.c | 109 ++++++++++--------
5 files changed, 197 insertions(+), 78 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
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-23 12:46:26
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 <redacted>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
Changes since v1:
- Moved the TRFCR helpers in to a new header file
---
.../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
@@ -1667,7 +1668,23 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)returnret;}-staticvoidetm4_cpu_restore(structetmv4_drvdata*drvdata)+staticintetm4_cpu_save(structetmv4_drvdata*drvdata)+{+intret=0;++/* Save the TRFCR irrespective of whether the ETM is ON */+if(drvdata->trfc)+drvdata->save_trfcr=read_trfcr();+/*+*SaveandrestoretheETMTraceregistersonlyif+*theETMisactive.+*/+if(local_read(&drvdata->mode)&&drvdata->save_state)+ret=__etm4_cpu_save(drvdata);+returnret;+}++staticvoid__etm4_cpu_restore(structetmv4_drvdata*drvdata){inti;structetmv4_save_state*state=drvdata->save_state;
@@ -1774,23 +1799,17 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,drvdata=etmdrvdata[cpu];-if(!drvdata->save_state)-returnNOTIFY_OK;-if(WARN_ON_ONCE(drvdata->cpu!=cpu))returnNOTIFY_BAD;switch(cmd){caseCPU_PM_ENTER:-/* save the state if self-hosted coresight is in use */-if(local_read(&drvdata->mode))-if(etm4_cpu_save(drvdata))-returnNOTIFY_BAD;+if(etm4_cpu_save(drvdata))+returnNOTIFY_BAD;break;caseCPU_PM_EXIT:caseCPU_PM_ENTER_FAILED:-if(drvdata->state_needs_restore)-etm4_cpu_restore(drvdata);+etm4_cpu_restore(drvdata);break;default:returnNOTIFY_DONE;
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-23 12:46:29
The Trace Filtering support (FEAT_TRF) ensures that the ETM
can be prohibited from generating any trace for a given EL.
This is much stricter knob, than the TRCVICTLR exception level
masks. At the moment, we do a onetime enable trace at user and
kernel and leave it untouched for the kernel life time.
This patch makes the switch dynamic, by honoring the filters
set by the user and enforcing them in the TRFCR controls.
We also rename the cpu_enable_tracing() appropriately to
cpu_detect_trace_filtering() and the drvdata member
trfc => trfcr to indicate the "value" of the TRFCR_EL1.
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Al Grant <redacted>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../coresight/coresight-etm4x-core.c | 61 ++++++++++++++-----
drivers/hwtracing/coresight/coresight-etm4x.h | 5 +-
.../coresight/coresight-self-hosted-trace.h | 7 +++
3 files changed, 55 insertions(+), 18 deletions(-)
@@ -237,6 +237,43 @@ struct etm4_enable_arg {intrc;};+/*+*etm4x_prohibit_trace-ProhibittheCPUfromtracingatallELs.+*WhentheCPUsupportsFEAT_TRF,wecouldmovetheETMtoatrace+*prohibitedstatebyfilteringtheExceptionlevelsviaTRFCR_EL1.+*/+staticvoidetm4x_prohibit_trace(structetmv4_drvdata*drvdata)+{+if(drvdata->trfcr)+cpu_prohibit_trace();+}++/*+*etm4x_allow_trace-AllowCPUtracingintherespectiveELs,+*asconfiguredbythedrvdata->config.modeforthecurrent+*session.EventhoughwehaveTRCVICTLRbitstofilterthe+*traceintheELs,itdoesn'tpreventtheETMfromgenerating+*apacket(e.g,TraceInfo)thatmightcontaintheaddressesfrom+*theexcludedlevels.Thusweusetheadditionalcontrolsprovided+*viatheTraceFilteringcontrols(FEAT_TRF)tomakesurenotrace+*isgeneratedfortheexcludedELs.+*/+staticvoidetm4x_allow_trace(structetmv4_drvdata*drvdata)+{+u64trfcr=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
@@ -441,6 +478,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);
@@ -719,7 +757,6 @@ static int etm4_enable(struct coresight_device *csdev,staticvoidetm4_disable_hw(void*info){u32control;-u64trfcr;structetmv4_drvdata*drvdata=info;structetmv4_config*config=&drvdata->config;structcoresight_device*csdev=drvdata->csdev;
@@ -746,12 +783,7 @@ static void etm4_disable_hw(void *info)*IftheCPUsupportsv8.4TracefilterControl,*settheETMtotraceprohibitedregion.*/-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);/**Makesureeverythingcompletesbeforedisabling,asrecommended*bysection7.3.77("TRCVICTLR, ViewInst Main Control Register,
@@ -767,9 +799,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]=
@@ -1177,7 +1206,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);}staticinlineu32etm4_get_victlr_access_type(structetmv4_config*config)
@@ -1673,7 +1702,7 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)intret=0;/* Save the TRFCR irrespective of whether the ETM is ON */-if(drvdata->trfc)+if(drvdata->trfcr)drvdata->save_trfcr=read_trfcr();/**SaveandrestoretheETMTraceregistersonlyif
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-23 12:46:30
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 <redacted>
Cc: Anshuman Khandual <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../hwtracing/coresight/coresight-etm-perf.c | 27 ++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
@@ -519,7 +526,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);+/*+*Makesurethehandleisstillvalidasthe+*sinkcouldhavecloseditfromanIRQ.+*Thesinkdrivermusthandletheracewith+*update_buffer()andIRQ.Thuseitherwe+*shouldgetavalidhandleandvalidsize+*(whichmaybe0).+*+*Butweshouldnevergetanon-zerosizewith+*aninvalidhandle.+*/+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
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-23 12:46:32
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. Always set the
format flag for TRBE generated records.
Cc: Anshuman Khandual <redacted>
Cc: Leo Yan <redacted>
Cc: Mike Leach <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-23 12:46:36
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 <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
@@ -253,13 +253,9 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)*trbe_basetrbe_base+nr_pages**Perfauxbufferdoesnothaveanyspaceforthedrivertowriteinto.-*Justcommunicatetracetruncationeventtotheuserspacebymarking-*itwithPERF_AUX_FLAG_TRUNCATED.*/-if(!handle->size){-perf_aux_output_flag(handle,PERF_AUX_FLAG_TRUNCATED);+if(!handle->size)return0;-}/* Compute the tail and wakeup indices now that we've aligned head */tail=PERF_IDX2OFF(handle->head+handle->size,buf);
@@ -361,7 +357,6 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)returnlimit;trbe_pad_buf(handle,handle->size);-perf_aux_output_flag(handle,PERF_AUX_FLAG_TRUNCATED);return0;}
--
2.24.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-23 12:46:38
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 <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 29 ++++++++++----------
1 file changed, 15 insertions(+), 14 deletions(-)
@@ -679,15 +679,16 @@ static int arm_trbe_disable(struct coresight_device *csdev)staticvoidtrbe_handle_spurious(structperf_output_handle*handle){-structtrbe_buf*buf=etm_perf_sink_config(handle);+u64limitr=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();-return;-}-trbe_enable_hw(buf);+/*+*IftheIRQwasspurious,simplyre-enabletheTRBE+*backwithoutmodifiyingthebufferparametersto+*retainthetracecollectedsofar.+*/+limitr|=TRBLIMITR_ENABLE;+write_sysreg_s(limitr,SYS_TRBLIMITR_EL1);+isb();}staticvoidtrbe_handle_overflow(structperf_output_handle*handle)
@@ -760,12 +761,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)enumtrbe_fault_actionact;u64status;-/*-*EnsurethetraceisvisibletotheCPUsand-*anyexternalabortshavebeenresolved.-*/-trbe_drain_and_disable_local();-+/* Reads to TRBSR_EL1 is fine when TRBE is active */status=read_sysreg_s(SYS_TRBSR_EL1);/**IfthependingIRQwashandledbyupdate_buffercallback
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-23 12:46:40
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 <redacted>
Cc: Mike Leach <redacted>
Cc: Anshuman Khandual <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-23 12:46:43
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.
Cc: Anshuman Khandual <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 34 +++++++++++---------
1 file changed, 18 insertions(+), 16 deletions(-)
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-23 12:46:45
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.
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 <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 25 ++++++++++++--------
1 file changed, 15 insertions(+), 10 deletions(-)
@@ -762,6 +763,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)structperf_output_handle*handle=*handle_ptr;enumtrbe_fault_actionact;u64status;+booltruncated=false;/* Reads to TRBSR_EL1 is fine when TRBE is active */status=read_sysreg_s(SYS_TRBSR_EL1);
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-23 12:46:47
We must prohibit the CPU from tracing before we disable
the TRBE and only re-enable it when we are sure the TRBE
has been enabled back. Otherwise, leave the CPU in
prohibited state.
Cc: Anshuman Khandual <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
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(-)
@@ -22,11 +22,13 @@ static inline void write_trfcr(u64 val)isb();}-staticinlinevoidcpu_prohibit_trace(void)+staticinlineu64cpu_prohibit_trace(void){u64trfcr=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 */+returntrfcr;}#endif /* __CORESIGHT_SELF_HOSTED_TRACE_H */
@@ -764,6 +765,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)enumtrbe_fault_actionact;u64status;booltruncated=false;+u64trfcr;/* Reads to TRBSR_EL1 is fine when TRBE is active */status=read_sysreg_s(SYS_TRBSR_EL1);
@@ -774,6 +776,8 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)if(!is_trbe_irq(status))returnIRQ_NONE;+/* Prohibit the CPU from tracing before we disable the TRBE */+trfcr=cpu_prohibit_trace();/**EnsurethetraceisvisibletotheCPUsand*anyexternalabortshavebeenresolved.
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-23 13:45:50
On 23/07/2021 13:46, Suzuki K Poulose wrote:
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,
it 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.
From: Mike Leach <hidden> Date: 2021-07-26 12:34:53
Hi Suzuki,
On Fri, 23 Jul 2021 at 13:46, Suzuki K Poulose [off-list ref] 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.
What is the user visible behaviour when using COLLISION?
The TRUNCATE warning is at least accurate - even if the KVM thing is
something of a red herring.
It is easier to explain a "scary" warning, than try to debug someones
problems if perf is silent or misleading when using the COLLISION
flag.
Regards
Mike
quoted hunk
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 <redacted>
Cc: Mike Leach <redacted>
Cc: Anshuman Khandual <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-26 16:16:53
Hi Mike,
On 26/07/2021 13:34, Mike Leach wrote:
Hi Suzuki,
On Fri, 23 Jul 2021 at 13:46, Suzuki K Poulose [off-list ref] wrote:
quoted
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.
What is the user visible behaviour when using COLLISION?
If you meant a Warning from the perf tool (similar to TRUNCATE or
PARTIAL), the answer is none. We could add one in the perf tool
if you think this is necessary.
The TRUNCATE warning is at least accurate - even if the KVM thing is
something of a red herring.
It is easier to explain a "scary" warning, than try to debug someones
problems if perf is silent or misleading when using the COLLISION
flag.
The RECORD_AUX still has this flag. So, if someone really wanted to
know how many times the TRBE fired the IRQ and thus potentially lost a
few bytes of the trace, they could always look at this.
Definitely this is not something similar to "TRUNCATED", which we
realized the hard way, nor the PARTIAL. But the perf tool could
report something similar. Please remember that the perf tool always
uses the "size" field from the RECORD_AUX to limit the trace decoding.
So, I am not sure how this could create new problems.
Suzuki
Regards
Mike
quoted
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 <redacted>
Cc: Mike Leach <redacted>
Cc: Anshuman Khandual <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Mike Leach <hidden> Date: 2021-07-27 10:46:39
HI Suzuki,
On Mon, 26 Jul 2021 at 17:01, Suzuki K Poulose [off-list ref] wrote:
Hi Mike,
On 26/07/2021 13:34, Mike Leach wrote:
quoted
Hi Suzuki,
On Fri, 23 Jul 2021 at 13:46, Suzuki K Poulose [off-list ref] wrote:
quoted
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.
What is the user visible behaviour when using COLLISION?
If you meant a Warning from the perf tool (similar to TRUNCATE or
PARTIAL), the answer is none. We could add one in the perf tool
if you think this is necessary.
I do - the problem is that we have replaced a visible warning with a
silent failure.
While we agree that the side effects of TRUNCATE mean it unfeasible as
a solution here - at least the PARTIAL message does give some
indication.
The average perf user is going to rely on the output from the tool -
if there is no warning they will assume all is good, but they have
possible non-contiguous trace and no indication of such.
Since we are using a collision flag in a particular context - i.e.
coresight trace - we have the chance to provide an appropriate message
for this context.
quoted
The TRUNCATE warning is at least accurate - even if the KVM thing is
something of a red herring.
Sorry - I meant PARTIAL here - but the comment stands otherwise.
quoted
It is easier to explain a "scary" warning, than try to debug someones
problems if perf is silent or misleading when using the COLLISION
flag.
The RECORD_AUX still has this flag. So, if someone really wanted to
know how many times the TRBE fired the IRQ and thus potentially lost a
few bytes of the trace, they could always look at this.
They could - but how would they know that they needed to - what
indicators would they have that the trace was not continuous?
The point of the perf tool is that it presents an accurate picture to
the user, based on the data collected. Most users aren't going to
start digging into the intricacies of the perf data file formats and
nor should they have to.
Definitely this is not something similar to "TRUNCATED", which we
realized the hard way, nor the PARTIAL. But the perf tool could
report something similar. Please remember that the perf tool always
uses the "size" field from the RECORD_AUX to limit the trace decoding.
So, I am not sure how this could create new problems.
There is no issue with decode - but if a user is investigating a
problem using trace, they need to be aware that some trace might be
dropped.
That way they can take mitigating action.
Regards
Mike
Suzuki
quoted
Regards
Mike
quoted
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 <redacted>
Cc: Mike Leach <redacted>
Cc: Anshuman Khandual <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-27 13:06:39
On 27/07/2021 11:46, Mike Leach wrote:
HI Suzuki,
On Mon, 26 Jul 2021 at 17:01, Suzuki K Poulose [off-list ref] wrote:
quoted
Hi Mike,
On 26/07/2021 13:34, Mike Leach wrote:
quoted
Hi Suzuki,
On Fri, 23 Jul 2021 at 13:46, Suzuki K Poulose [off-list ref] wrote:
quoted
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.
What is the user visible behaviour when using COLLISION?
If you meant a Warning from the perf tool (similar to TRUNCATE or
PARTIAL), the answer is none. We could add one in the perf tool
if you think this is necessary.
I do - the problem is that we have replaced a visible warning with a
silent failure.
While we agree that the side effects of TRUNCATE mean it unfeasible as
a solution here - at least the PARTIAL message does give some
indication.
The average perf user is going to rely on the output from the tool -
if there is no warning they will assume all is good, but they have
possible non-contiguous trace and no indication of such.
Since we are using a collision flag in a particular context - i.e.
coresight trace - we have the chance to provide an appropriate message
for this context.
quoted
quoted
The TRUNCATE warning is at least accurate - even if the KVM thing is
something of a red herring.
Sorry - I meant PARTIAL here - but the comment stands otherwise.
quoted
quoted
It is easier to explain a "scary" warning, than try to debug someones
problems if perf is silent or misleading when using the COLLISION
flag.
The RECORD_AUX still has this flag. So, if someone really wanted to
know how many times the TRBE fired the IRQ and thus potentially lost a
few bytes of the trace, they could always look at this.
They could - but how would they know that they needed to - what
indicators would they have that the trace was not continuous?
The point of the perf tool is that it presents an accurate picture to
the user, based on the data collected. Most users aren't going to
start digging into the intricacies of the perf data file formats and
nor should they have to.
quoted
Definitely this is not something similar to "TRUNCATED", which we
realized the hard way, nor the PARTIAL. But the perf tool could
report something similar. Please remember that the perf tool always
uses the "size" field from the RECORD_AUX to limit the trace decoding.
So, I am not sure how this could create new problems.
There is no issue with decode - but if a user is investigating a
problem using trace, they need to be aware that some trace might be
dropped.
That way they can take mitigating action.
Agreed. This is something that can be done by the perf tool.
Kind regards
Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-28 09:24:46
On 07/27/2021 02:06 PM, Suzuki K Poulose wrote:
On 27/07/2021 11:46, Mike Leach wrote:
quoted
HI Suzuki,
On Mon, 26 Jul 2021 at 17:01, Suzuki K Poulose [off-list ref] wrote:
quoted
Hi Mike,
On 26/07/2021 13:34, Mike Leach wrote:
quoted
Hi Suzuki,
On Fri, 23 Jul 2021 at 13:46, Suzuki K Poulose [off-list ref] wrote:
quoted
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.
What is the user visible behaviour when using COLLISION?
If you meant a Warning from the perf tool (similar to TRUNCATE or
PARTIAL), the answer is none. We could add one in the perf tool
if you think this is necessary.
I do - the problem is that we have replaced a visible warning with a
silent failure.
While we agree that the side effects of TRUNCATE mean it unfeasible as
a solution here - at least the PARTIAL message does give some
indication.
The average perf user is going to rely on the output from the tool -
if there is no warning they will assume all is good, but they have
possible non-contiguous trace and no indication of such.
Since we are using a collision flag in a particular context - i.e.
coresight trace - we have the chance to provide an appropriate message
for this context.
quoted
quoted
The TRUNCATE warning is at least accurate - even if the KVM thing is
something of a red herring.
Sorry - I meant PARTIAL here - but the comment stands otherwise.
quoted
quoted
It is easier to explain a "scary" warning, than try to debug someones
problems if perf is silent or misleading when using the COLLISION
flag.
The RECORD_AUX still has this flag. So, if someone really wanted to
know how many times the TRBE fired the IRQ and thus potentially lost a
few bytes of the trace, they could always look at this.
They could - but how would they know that they needed to - what
indicators would they have that the trace was not continuous?
The point of the perf tool is that it presents an accurate picture to
the user, based on the data collected. Most users aren't going to
start digging into the intricacies of the perf data file formats and
nor should they have to.
quoted
Definitely this is not something similar to "TRUNCATED", which we
realized the hard way, nor the PARTIAL. But the perf tool could
report something similar. Please remember that the perf tool always
uses the "size" field from the RECORD_AUX to limit the trace decoding.
So, I am not sure how this could create new problems.
There is no issue with decode - but if a user is investigating a
problem using trace, they need to be aware that some trace might be
dropped.
That way they can take mitigating action.
Agreed. This is something that can be done by the perf tool.
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 <redacted>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
Changes since v1:
- Moved the TRFCR helpers in to a new header file
---
.../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
@@ -1667,7 +1668,23 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)returnret;}-staticvoidetm4_cpu_restore(structetmv4_drvdata*drvdata)+staticintetm4_cpu_save(structetmv4_drvdata*drvdata)+{+intret=0;++/* Save the TRFCR irrespective of whether the ETM is ON */+if(drvdata->trfc)+drvdata->save_trfcr=read_trfcr();+/*+*SaveandrestoretheETMTraceregistersonlyif+*theETMisactive.+*/+if(local_read(&drvdata->mode)&&drvdata->save_state)+ret=__etm4_cpu_save(drvdata);+returnret;+}++staticvoid__etm4_cpu_restore(structetmv4_drvdata*drvdata){inti;structetmv4_save_state*state=drvdata->save_state;
@@ -1774,23 +1799,17 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,drvdata=etmdrvdata[cpu];-if(!drvdata->save_state)-returnNOTIFY_OK;-if(WARN_ON_ONCE(drvdata->cpu!=cpu))returnNOTIFY_BAD;switch(cmd){caseCPU_PM_ENTER:-/* save the state if self-hosted coresight is in use */-if(local_read(&drvdata->mode))-if(etm4_cpu_save(drvdata))-returnNOTIFY_BAD;+if(etm4_cpu_save(drvdata))+returnNOTIFY_BAD;break;caseCPU_PM_EXIT:caseCPU_PM_ENTER_FAILED:-if(drvdata->state_needs_restore)-etm4_cpu_restore(drvdata);+etm4_cpu_restore(drvdata);break;default:returnNOTIFY_DONE;
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
Could you please explain 'stricter' ? Are you suggesting that TRCVICTLR
based exception filtering some times might not implement the filtering
even if configured ?
masks. At the moment, we do a onetime enable trace at user and
kernel and leave it untouched for the kernel life time.
This patch makes the switch dynamic, by honoring the filters
set by the user and enforcing them in the TRFCR controls.
TRFCR actually helps in making the exception level filtering dynamic
which was not possible earlier with TRCVICTLR.
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.
Makes sense.
quoted hunk
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Al Grant <redacted>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../coresight/coresight-etm4x-core.c | 61 ++++++++++++++-----
drivers/hwtracing/coresight/coresight-etm4x.h | 5 +-
.../coresight/coresight-self-hosted-trace.h | 7 +++
3 files changed, 55 insertions(+), 18 deletions(-)
Should it be as etm4x_allow_trace() instead, where drvdata->trfcr
indicates the presence of FEAT_TRF - just to be clear ?
/* If the CPU doesn't support FEAT_TRF, nothing to do */
if (!drvdata->trfcr)
return;
cpu_prohibit_trace();
quoted hunk
+}
+
+/*
+ * 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
@@ -441,6 +478,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);
@@ -746,12 +783,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,
@@ -767,9 +799,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] =
@@ -964,15 +993,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
@@ -1177,7 +1206,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)
@@ -1673,7 +1702,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
@@ -1782,7 +1811,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);
+ * trace allowed at user and kernel ELs. Otherwise, 0.
The sentence here does not make sense. Is not the exception level ELx and EL0
can be filtered out independently ? Should this be something like ...
"If the CPU supports FEAT_TRF, value of the TRFCR_ELx - indicating whether
trace is allowed at user [and/or] kernel ELs. Otherwise, 0."
quoted hunk
* @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
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 <redacted>
Cc: Anshuman Khandual <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../hwtracing/coresight/coresight-etm-perf.c | 27 ++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
@@ -519,7 +526,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);
Right.
}
/* Disabling the path make its elements available to other sessions */
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. Always set the
format flag for TRBE generated records.
Cc: Anshuman Khandual <redacted>
Cc: Leo Yan <redacted>
Cc: Mike Leach <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
But why should not PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW also be set on
zero sized records as well ? Otherwise there are two instances during
TRBE buffer management, where PERF_AUX_FLAG_TRUNCATED is marked alone
without PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW. Those could be changed as
well.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
@@ -253,13 +253,9 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)*trbe_basetrbe_base+nr_pages**Perfauxbufferdoesnothaveanyspaceforthedrivertowriteinto.-*Justcommunicatetracetruncationeventtotheuserspacebymarking-*itwithPERF_AUX_FLAG_TRUNCATED.*/-if(!handle->size){-perf_aux_output_flag(handle,PERF_AUX_FLAG_TRUNCATED);+if(!handle->size)return0;-}/* Compute the tail and wakeup indices now that we've aligned head */tail=PERF_IDX2OFF(handle->head+handle->size,buf);
@@ -361,7 +357,6 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)returnlimit;trbe_pad_buf(handle,handle->size);-perf_aux_output_flag(handle,PERF_AUX_FLAG_TRUNCATED);return0;}
What about in trbe_handle_spurious() path which used to set the flag via
compute_trbe_buffer_limit(), but would not any more after this change. I
guess following additional change would be required to preserve the past
behaviour.
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.
The ideas was that a state (pointers) reset would improve the chances
of not getting the spurious IRQ once again. This is assuming that some
thing during this current state machine, had caused the spurious IRQ.
Hence just restart it back from the beginning. Yes, it does lose some
trace data but whats the real possibility of such spurious IRQs in the
first place ?
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.
This makes sense. I was not sure if TRBSR could be safely read without
actually stopping the TRBE.
quoted hunk
Cc: Anshuman Khandual <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 29 ++++++++++----------
1 file changed, 15 insertions(+), 14 deletions(-)
@@ -760,12 +761,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
Warn here that a non-related IRQ has been delivered to this handler ?
But moving the trbe_drain_and_disable_local() later, enables it to
return back immediately after detecting an unrelated IRQ.
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();
Actually there are two types of spurious interrupts here.
1. Non-TRBE spurious interrupt
Fails is_trbe_irq() test and needs to be returned immediately from
arm_trbe_irq_handler(), after an warning for the platform IRQ
delivery wiring.
2. TRBE spurious interrupt
Clears is_trbe_irq() and get handled in trbe_handle_spurious(). I
still think leaving this unchanged might be better as it reduces
the chance of getting further spurious TRBE interrupts.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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.
Cc: Anshuman Khandual <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 34 +++++++++++---------
1 file changed, 18 insertions(+), 16 deletions(-)
With that, this clean up makes sense.
Reviewed-by: Anshuman Khandual <redacted>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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.
via the CoreSight PMU layer's stop event ?
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.
Makes sense.
Minor nit. Commit message here should be reformatted to be expanded
upto 75 character width.
quoted hunk
Cc: Anshuman Khandual <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 25 ++++++++++++--------
1 file changed, 15 insertions(+), 10 deletions(-)
@@ -762,6 +763,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)structperf_output_handle*handle=*handle_ptr;enumtrbe_fault_actionact;u64status;+booltruncated=false;/* Reads to TRBSR_EL1 is fine when TRBE is active */status=read_sysreg_s(SYS_TRBSR_EL1);
We must prohibit the CPU from tracing before we disable
the TRBE and only re-enable it when we are sure the TRBE
has been enabled back. Otherwise, leave the CPU in
prohibited state.
Cc: Anshuman Khandual <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
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(-)
@@ -22,11 +22,13 @@ static inline void write_trfcr(u64 val)isb();}-staticinlinevoidcpu_prohibit_trace(void)+staticinlineu64cpu_prohibit_trace(void){u64trfcr=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 */+returntrfcr;}#endif /* __CORESIGHT_SELF_HOSTED_TRACE_H */
@@ -764,6 +765,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)enumtrbe_fault_actionact;u64status;booltruncated=false;+u64trfcr;/* Reads to TRBSR_EL1 is fine when TRBE is active */status=read_sysreg_s(SYS_TRBSR_EL1);
@@ -774,6 +776,8 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)if(!is_trbe_irq(status))returnIRQ_NONE;+/* Prohibit the CPU from tracing before we disable the TRBE */+trfcr=cpu_prohibit_trace();/**EnsurethetraceisvisibletotheCPUsand*anyexternalabortshavebeenresolved.
The change LGTM. But the commit message needs to add some more details
like that in V2 which explained how traces from ETE could be routed to
ATB if not put in trace prohibited state, for all exception levels etc.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-30 11:29:58
On 30/07/2021 04:48, Anshuman Khandual wrote:
On 7/23/21 6:16 PM, Suzuki K Poulose wrote:
quoted
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
Could you please explain 'stricter' ? Are you suggesting that TRCVICTLR
based exception filtering some times might not implement the filtering
even if configured ?
Sure, the TRVICTLR only ensures that the ETM doesn't generate
any "branch" trace packets. But that doesn't prevent it from
generating the "Context" packets which may contain the kernel
addresses, if they are generated while in Kernel.
But, the FEAT_TRF strictly prevents the trace unit from generating
any packets while it is "prohibited". Thus it is a much better
control to prevent kernel address leaks via the trace.
quoted
masks. At the moment, we do a onetime enable trace at user and
kernel and leave it untouched for the kernel life time.
This patch makes the switch dynamic, by honoring the filters
set by the user and enforcing them in the TRFCR controls.
TRFCR actually helps in making the exception level filtering dynamic
which was not possible earlier with TRCVICTLR.
quoted
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.
Makes sense.
quoted
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Al Grant <redacted>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../coresight/coresight-etm4x-core.c | 61 ++++++++++++++-----
drivers/hwtracing/coresight/coresight-etm4x.h | 5 +-
.../coresight/coresight-self-hosted-trace.h | 7 +++
3 files changed, 55 insertions(+), 18 deletions(-)
Should it be as etm4x_allow_trace() instead, where drvdata->trfcr
indicates the presence of FEAT_TRF - just to be clear ?
/* If the CPU doesn't support FEAT_TRF, nothing to do */
if (!drvdata->trfcr)
return;
cpu_prohibit_trace();
OK
quoted
+}
+
+/*
+ * 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
@@ -441,6 +478,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);
@@ -746,12 +783,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,
@@ -767,9 +799,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] =
@@ -964,15 +993,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
@@ -1177,7 +1206,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)
@@ -1673,7 +1702,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
@@ -1782,7 +1811,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);
+ * trace allowed at user and kernel ELs. Otherwise, 0.
The sentence here does not make sense. Is not the exception level ELx and EL0
can be filtered out independently ? Should this be something like ...
The value holds a superset of the possible "allowed" configurations.
We do this to avoid setting the TRFCR_CX everytime depending on the
kernel EL (only possible from EL2). So we initialize the field
with value of TRCR_ELx with all the ELs enabled. This can be filtered
later by the driver accordingly. This will also serve as marker
to check the availability of the feature.
Thanks
Suzuki
"If the CPU supports FEAT_TRF, value of the TRFCR_ELx - indicating whether
trace is allowed at user [and/or] kernel ELs. Otherwise, 0."
quoted
* @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
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-30 11:37:25
On 30/07/2021 05:26, Anshuman Khandual wrote:
On 7/23/21 6:16 PM, Suzuki K Poulose wrote:
quoted
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. Always set the
format flag for TRBE generated records.
Cc: Anshuman Khandual <redacted>
Cc: Leo Yan <redacted>
Cc: Mike Leach <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
But why should not PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW also be set on
zero sized records as well ? Otherwise there are two instances during
TRBE buffer management, where PERF_AUX_FLAG_TRUNCATED is marked alone
without PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW. Those could be changed as
well.
All records (irrespective of the size) generated by the TRBE must
contain the "RAW" flag. Did I miss another instance where we don't
do this ?
Suzuki
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-30 12:57:53
On 30/07/2021 06:15, Anshuman Khandual wrote:
On 7/23/21 6:16 PM, Suzuki K Poulose wrote:
quoted
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.
The ideas was that a state (pointers) reset would improve the chances
of not getting the spurious IRQ once again. This is assuming that some
thing during this current state machine, had caused the spurious IRQ.
Hence just restart it back from the beginning. Yes, it does lose some
trace data but whats the real possibility of such spurious IRQs in the
first place ?
quoted
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.
This makes sense. I was not sure if TRBSR could be safely read without
actually stopping the TRBE.
quoted
Cc: Anshuman Khandual <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 29 ++++++++++----------
1 file changed, 15 insertions(+), 14 deletions(-)
@@ -760,12 +761,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
Warn here that a non-related IRQ has been delivered to this handler ?
But moving the trbe_drain_and_disable_local() later, enables it to
return back immediately after detecting an unrelated IRQ.
Not really. There could be race with the update_buffer(), see the
comment right above that. When that happens, we have disabled the
TRBE in the update_buffer(). Either case, we have nothing to do.
quoted
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();
Actually there are two types of spurious interrupts here.
1. Non-TRBE spurious interrupt
Fails is_trbe_irq() test and needs to be returned immediately from
arm_trbe_irq_handler(), after an warning for the platform IRQ
delivery wiring.
Not necessarily warrant a WARNING. See above.
2. TRBE spurious interrupt
Clears is_trbe_irq() and get handled in trbe_handle_spurious(). I
still think leaving this unchanged might be better as it reduces
the chance of getting further spurious TRBE interrupts.
How does it reduce the chances of getting another spurious interrupt ?
If the TRBE gets a spurious IRQ, that we cannot decode, I would rather
leave it as NOP.
Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2021-07-30 12:58:55
On 30/07/2021 05:47, Anshuman Khandual wrote:
quoted hunk
On 7/23/21 6:16 PM, Suzuki K Poulose wrote:
quoted
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 <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <redacted>
Cc: Leo Yan <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
@@ -253,13 +253,9 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)*trbe_basetrbe_base+nr_pages**Perfauxbufferdoesnothaveanyspaceforthedrivertowriteinto.-*Justcommunicatetracetruncationeventtotheuserspacebymarking-*itwithPERF_AUX_FLAG_TRUNCATED.*/-if(!handle->size){-perf_aux_output_flag(handle,PERF_AUX_FLAG_TRUNCATED);+if(!handle->size)return0;-}/* Compute the tail and wakeup indices now that we've aligned head */tail=PERF_IDX2OFF(handle->head+handle->size,buf);
@@ -361,7 +357,6 @@ static unsigned long __trbe_normal_offset(struct perf_output_handle *handle)returnlimit;trbe_pad_buf(handle,handle->size);-perf_aux_output_flag(handle,PERF_AUX_FLAG_TRUNCATED);return0;}
What about in trbe_handle_spurious() path which used to set the flag via
compute_trbe_buffer_limit(), but would not any more after this change. I
guess following additional change would be required to preserve the past
behaviour.