| Message ID | 20210921134121.2423546-4-suzuki.poulose@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Self-hosted trace related errata workarounds | expand |
Hi Suzuki, On Tue, Sep 21, 2021 at 02:41:07PM +0100, Suzuki K Poulose wrote: > We collect the trace from the TRBE on FILL event from IRQ context > and when via update_buffer(), when the event is stopped. Let us s/"and when via"/"and via" > consolidate how we calculate the trace generated into a helper. > > 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> > --- > drivers/hwtracing/coresight/coresight-trbe.c | 48 ++++++++++++-------- > 1 file changed, 30 insertions(+), 18 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c > index 63f7edd5fd1f..063c4505a203 100644 > --- a/drivers/hwtracing/coresight/coresight-trbe.c > +++ b/drivers/hwtracing/coresight/coresight-trbe.c > @@ -527,6 +527,30 @@ static enum trbe_fault_action trbe_get_fault_act(u64 trbsr) > return TRBE_FAULT_ACT_SPURIOUS; > } > > +static unsigned long trbe_get_trace_size(struct perf_output_handle *handle, > + struct trbe_buf *buf, > + bool wrap) Stacking > +{ > + u64 write; > + u64 start_off, end_off; > + > + /* > + * If the TRBE has wrapped around the write pointer has > + * wrapped and should be treated as limit. > + */ > + if (wrap) > + write = get_trbe_limit_pointer(); > + else > + write = get_trbe_write_pointer(); > + > + end_off = write - buf->trbe_base; In both arm_trbe_alloc_buffer() and trbe_handle_overflow() the base address is acquired using get_trbe_base_pointer() but here it is referenced directly - any reason for that? It certainly makes reviewing this simple patch quite difficult because I keep wondering if I am missing something subtle... > + start_off = PERF_IDX2OFF(handle->head, buf); > + > + if (WARN_ON_ONCE(end_off < start_off)) > + return 0; > + return (end_off - start_off); > +} > + > static void *arm_trbe_alloc_buffer(struct coresight_device *csdev, > struct perf_event *event, void **pages, > int nr_pages, bool snapshot) > @@ -588,9 +612,9 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev, > struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev); > struct trbe_buf *buf = config; > enum trbe_fault_action act; > - unsigned long size, offset; > - unsigned long write, base, status; > + unsigned long size, status; > unsigned long flags; > + bool wrap = false; > > WARN_ON(buf->cpudata != cpudata); > WARN_ON(cpudata->cpu != smp_processor_id()); > @@ -630,8 +654,6 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev, > * handle gets freed in etm_event_stop(). > */ > trbe_drain_and_disable_local(); > - write = get_trbe_write_pointer(); > - base = get_trbe_base_pointer(); > > /* Check if there is a pending interrupt and handle it here */ > status = read_sysreg_s(SYS_TRBSR_EL1); > @@ -655,20 +677,11 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev, > goto done; > } > > - /* > - * Otherwise, the buffer is full and the write pointer > - * has reached base. Adjust this back to the Limit pointer > - * for correct size. Also, mark the buffer truncated. > - */ > - write = get_trbe_limit_pointer(); > perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION); > + wrap = true; > } > > - offset = write - base; > - if (WARN_ON_ONCE(offset < PERF_IDX2OFF(handle->head, buf))) > - size = 0; > - else > - size = offset - PERF_IDX2OFF(handle->head, buf); > + size = trbe_get_trace_size(handle, buf, wrap); > > done: > local_irq_restore(flags); > @@ -749,11 +762,10 @@ 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); > - unsigned long offset, size; > + unsigned long size; > struct etm_event_data *event_data; > > - offset = get_trbe_limit_pointer() - get_trbe_base_pointer(); > - size = offset - PERF_IDX2OFF(handle->head, buf); > + size = trbe_get_trace_size(handle, buf, true); > if (buf->snapshot) > handle->head += size; > > -- > 2.24.1 >
On 30/09/2021 18:54, Mathieu Poirier wrote: > Hi Suzuki, > > On Tue, Sep 21, 2021 at 02:41:07PM +0100, Suzuki K Poulose wrote: >> We collect the trace from the TRBE on FILL event from IRQ context >> and when via update_buffer(), when the event is stopped. Let us > > s/"and when via"/"and via" > >> consolidate how we calculate the trace generated into a helper. >> >> 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> >> --- >> drivers/hwtracing/coresight/coresight-trbe.c | 48 ++++++++++++-------- >> 1 file changed, 30 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c >> index 63f7edd5fd1f..063c4505a203 100644 >> --- a/drivers/hwtracing/coresight/coresight-trbe.c >> +++ b/drivers/hwtracing/coresight/coresight-trbe.c >> @@ -527,6 +527,30 @@ static enum trbe_fault_action trbe_get_fault_act(u64 trbsr) >> return TRBE_FAULT_ACT_SPURIOUS; >> } >> >> +static unsigned long trbe_get_trace_size(struct perf_output_handle *handle, >> + struct trbe_buf *buf, >> + bool wrap) > > Stacking > Ack >> +{ >> + u64 write; >> + u64 start_off, end_off; >> + >> + /* >> + * If the TRBE has wrapped around the write pointer has >> + * wrapped and should be treated as limit. >> + */ >> + if (wrap) >> + write = get_trbe_limit_pointer(); >> + else >> + write = get_trbe_write_pointer(); >> + >> + end_off = write - buf->trbe_base; > > In both arm_trbe_alloc_buffer() and trbe_handle_overflow() the base address is > acquired using get_trbe_base_pointer() but here it is referenced directly - any > reason for that? It certainly makes reviewing this simple patch quite > difficult because I keep wondering if I am missing something subtle... Very good observation. So far, we always prgrammed the TRBBASER with the the VA(ring_buffer[0]). And thus reading the BASER and using the buf->trbe_base is all fine. But going forward, we are going to use different values for the TRBBASER to work around erratum. Thus to make the computation of the "offsets" within the ring buffer, it is always correct to use this field. I could move this to the patch where the work around is introduced, and put in a comment there. Thanks for the review Suzuki
On Fri, Oct 01, 2021 at 09:36:24AM +0100, Suzuki K Poulose wrote: > On 30/09/2021 18:54, Mathieu Poirier wrote: > > Hi Suzuki, > > > > On Tue, Sep 21, 2021 at 02:41:07PM +0100, Suzuki K Poulose wrote: > > > We collect the trace from the TRBE on FILL event from IRQ context > > > and when via update_buffer(), when the event is stopped. Let us > > > > s/"and when via"/"and via" > > > > > consolidate how we calculate the trace generated into a helper. > > > > > > 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> > > > --- > > > drivers/hwtracing/coresight/coresight-trbe.c | 48 ++++++++++++-------- > > > 1 file changed, 30 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c > > > index 63f7edd5fd1f..063c4505a203 100644 > > > --- a/drivers/hwtracing/coresight/coresight-trbe.c > > > +++ b/drivers/hwtracing/coresight/coresight-trbe.c > > > @@ -527,6 +527,30 @@ static enum trbe_fault_action trbe_get_fault_act(u64 trbsr) > > > return TRBE_FAULT_ACT_SPURIOUS; > > > } > > > +static unsigned long trbe_get_trace_size(struct perf_output_handle *handle, > > > + struct trbe_buf *buf, > > > + bool wrap) > > > > Stacking > > > > Ack > > > > +{ > > > + u64 write; > > > + u64 start_off, end_off; > > > + > > > + /* > > > + * If the TRBE has wrapped around the write pointer has > > > + * wrapped and should be treated as limit. > > > + */ > > > + if (wrap) > > > + write = get_trbe_limit_pointer(); > > > + else > > > + write = get_trbe_write_pointer(); > > > + > > > + end_off = write - buf->trbe_base; > > > > In both arm_trbe_alloc_buffer() and trbe_handle_overflow() the base address is > > acquired using get_trbe_base_pointer() but here it is referenced directly - any > > reason for that? It certainly makes reviewing this simple patch quite > > difficult because I keep wondering if I am missing something subtle... > > Very good observation. So far, we always prgrammed the TRBBASER with the > the VA(ring_buffer[0]). And thus reading the BASER and using the > buf->trbe_base is all fine. > > But going forward, we are going to use different values for the TRBBASER > to work around erratum. Thus to make the computation of the "offsets" > within the ring buffer, it is always correct to use this field. I could > move this to the patch where the work around is introduced, and put in > a comment there. That will be greatly appreciated. > > Thanks for the review > > Suzuki >
On 01/10/2021 16:15, Mathieu Poirier wrote: > On Fri, Oct 01, 2021 at 09:36:24AM +0100, Suzuki K Poulose wrote: >> On 30/09/2021 18:54, Mathieu Poirier wrote: >>> Hi Suzuki, >>> >>> On Tue, Sep 21, 2021 at 02:41:07PM +0100, Suzuki K Poulose wrote: >>>> We collect the trace from the TRBE on FILL event from IRQ context >>>> and when via update_buffer(), when the event is stopped. Let us >>> >>> s/"and when via"/"and via" >>> >>>> consolidate how we calculate the trace generated into a helper. >>>> >>>> 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> >>>> --- >>>> drivers/hwtracing/coresight/coresight-trbe.c | 48 ++++++++++++-------- >>>> 1 file changed, 30 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c >>>> index 63f7edd5fd1f..063c4505a203 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-trbe.c >>>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c >>>> @@ -527,6 +527,30 @@ static enum trbe_fault_action trbe_get_fault_act(u64 trbsr) >>>> return TRBE_FAULT_ACT_SPURIOUS; >>>> } >>>> +static unsigned long trbe_get_trace_size(struct perf_output_handle *handle, >>>> + struct trbe_buf *buf, >>>> + bool wrap) >>> >>> Stacking >>> >> >> Ack >> >>>> +{ >>>> + u64 write; >>>> + u64 start_off, end_off; >>>> + >>>> + /* >>>> + * If the TRBE has wrapped around the write pointer has >>>> + * wrapped and should be treated as limit. >>>> + */ >>>> + if (wrap) >>>> + write = get_trbe_limit_pointer(); >>>> + else >>>> + write = get_trbe_write_pointer(); >>>> + >>>> + end_off = write - buf->trbe_base; >>> >>> In both arm_trbe_alloc_buffer() and trbe_handle_overflow() the base address is >>> acquired using get_trbe_base_pointer() but here it is referenced directly - any >>> reason for that? It certainly makes reviewing this simple patch quite >>> difficult because I keep wondering if I am missing something subtle... >> >> Very good observation. So far, we always prgrammed the TRBBASER with the >> the VA(ring_buffer[0]). And thus reading the BASER and using the >> buf->trbe_base is all fine. >> >> But going forward, we are going to use different values for the TRBBASER >> to work around erratum. Thus to make the computation of the "offsets" >> within the ring buffer, it is always correct to use this field. I could >> move this to the patch where the work around is introduced, and put in >> a comment there. > > That will be greatly appreciated. I have moved this to the patch, which introduces the concept of "TRBE using" a different BASE address than the beginning of the ring buffer. Thanks Suzuki
diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c index 63f7edd5fd1f..063c4505a203 100644 --- a/drivers/hwtracing/coresight/coresight-trbe.c +++ b/drivers/hwtracing/coresight/coresight-trbe.c @@ -527,6 +527,30 @@ static enum trbe_fault_action trbe_get_fault_act(u64 trbsr) return TRBE_FAULT_ACT_SPURIOUS; } +static unsigned long trbe_get_trace_size(struct perf_output_handle *handle, + struct trbe_buf *buf, + bool wrap) +{ + u64 write; + u64 start_off, end_off; + + /* + * If the TRBE has wrapped around the write pointer has + * wrapped and should be treated as limit. + */ + if (wrap) + write = get_trbe_limit_pointer(); + else + write = get_trbe_write_pointer(); + + end_off = write - buf->trbe_base; + start_off = PERF_IDX2OFF(handle->head, buf); + + if (WARN_ON_ONCE(end_off < start_off)) + return 0; + return (end_off - start_off); +} + static void *arm_trbe_alloc_buffer(struct coresight_device *csdev, struct perf_event *event, void **pages, int nr_pages, bool snapshot) @@ -588,9 +612,9 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev, struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev); struct trbe_buf *buf = config; enum trbe_fault_action act; - unsigned long size, offset; - unsigned long write, base, status; + unsigned long size, status; unsigned long flags; + bool wrap = false; WARN_ON(buf->cpudata != cpudata); WARN_ON(cpudata->cpu != smp_processor_id()); @@ -630,8 +654,6 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev, * handle gets freed in etm_event_stop(). */ trbe_drain_and_disable_local(); - write = get_trbe_write_pointer(); - base = get_trbe_base_pointer(); /* Check if there is a pending interrupt and handle it here */ status = read_sysreg_s(SYS_TRBSR_EL1); @@ -655,20 +677,11 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev, goto done; } - /* - * Otherwise, the buffer is full and the write pointer - * has reached base. Adjust this back to the Limit pointer - * for correct size. Also, mark the buffer truncated. - */ - write = get_trbe_limit_pointer(); perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION); + wrap = true; } - offset = write - base; - if (WARN_ON_ONCE(offset < PERF_IDX2OFF(handle->head, buf))) - size = 0; - else - size = offset - PERF_IDX2OFF(handle->head, buf); + size = trbe_get_trace_size(handle, buf, wrap); done: local_irq_restore(flags); @@ -749,11 +762,10 @@ 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); - unsigned long offset, size; + unsigned long size; struct etm_event_data *event_data; - offset = get_trbe_limit_pointer() - get_trbe_base_pointer(); - size = offset - PERF_IDX2OFF(handle->head, buf); + size = trbe_get_trace_size(handle, buf, true); if (buf->snapshot) handle->head += size;