[PATCH v2 4/4] coresight: etm4x: Hook panic dump callback for etmv4

[Leo Yan]

On Thu, Nov 23, 2017 at 10:03:33AM -0700, Mathieu Poirier wrote:
> On 22 November 2017 at 12:09, Mathieu Poirier
> <mathieu.poirier at linaro.org> wrote:
> > On Tue, Nov 21, 2017 at 11:08:44AM +0800, Leo Yan wrote:
> >> We need to dump etmv4 info as metadata, so the data can be used for
> >> offline checking for configuration and generate 'perf' compatible
> >> file.  This commit hooks etmv4 callback for panic dump.
> >>
> >> Signed-off-by: Leo Yan <leo.yan at linaro.org>
> >> ---
> >>  drivers/hwtracing/coresight/coresight-etm4x.c | 22 ++++++++++++++++++++++
> >>  drivers/hwtracing/coresight/coresight-etm4x.h | 15 +++++++++++++++
> >>  2 files changed, 37 insertions(+)
> >>
> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> >> index cf364a5..bf9608b 100644
> >> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> >> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> >> @@ -418,11 +418,33 @@ static void etm4_disable(struct coresight_device *csdev,
> >>               local_set(&drvdata->mode, CS_MODE_DISABLED);
> >>  }
> >>
> >> +static void etm4_panic_cb(void *data)
> >> +{
> >> +     struct coresight_device *csdev = (struct coresight_device *)data;
> >> +     int cpu = smp_processor_id();
> >> +     struct etmv4_drvdata *drvdata = etmdrvdata[cpu];
> >> +     struct etmv4_config *config = &drvdata->config;
> >> +     struct etmv4_metadata *metadata = &drvdata->metadata;
> >> +
> >> +     metadata->magic = ETM4_METADATA_MAGIC;
> >> +     metadata->cpu = cpu;
> >> +     metadata->trcconfigr = config->cfg;
> >> +     metadata->trctraceidr = drvdata->trcid;
> >> +     metadata->trcidr0 = readl_relaxed(drvdata->base + TRCIDR0);
> >> +     metadata->trcidr1 = readl_relaxed(drvdata->base + TRCIDR1);
> >> +     metadata->trcidr2 = readl_relaxed(drvdata->base + TRCIDR2);
> >> +     metadata->trcidr8 = readl_relaxed(drvdata->base + TRCIDR8);
> >
> > I'm pretty sure this won't work on all architecture.  This tracer may not be
> > powered when this is called and depending on the power domain the TRCIDRx
> > registers are in it may lock.
> >
> > Simply read those at boot time as part of the _probe() function.
> 
> Thinking about this will work when operating from sysFS but
> theoretically inaccurate from perf.  Say we have a 4 CPU system where
> for a perf session only 2 CPUs were used for trace acquisition - the
> process was simply not scheduled on the other 2 CPU.  With the above
> code 2 CPUs (the ones that were actually used) will have the right
> configuration while the other two will have the default tracer
> configuration (since etm4_parse_event_config() was never called on
> them).

This is quite important for how to launch tracing you have reminded me
on IRC and I'd like to list all possible tracer configurations:

- Use sysFS to enable some or all ETM tracers;

- Use perf + '--per-thread' to open one session, as you said the
  session may only use some of CPUs but not all of them;

- Use perf + 'snapshot' mode, this mode now doesn't work but we can
  expect this mode can work later so can enable all tracers;

These three scenarios are what I can think out and panic dump should
handle them correctly, if I miss or misunderstand anything, please feel
free correct me.

> Values in TRCIDR{0, 1, 2, 8} and TRCAUTHSTATUS is RO and won't change.
> So those can be filled at _probe() time when tracers are instantiated.
> Values for trcconfigr ang trctraceidr are trickier.
> 
> When operating from sysFS tracer configuration should be recorded in
> etm4_enable_sysfs(), probably just before smp_call_function_single().
> When operating from perf adding all tracers to the dump list should be
> done in etm_setup_aux() and an update for the configuration in
> etm4_enable_perf(), just before etm4_enable_hw().  When circling
> through tracers to communicate the metadata to the kdump mechanic
> check if buf != NULL, what way we only communicate information about
> tracers that were used.

I'm not sure I catch all your points, so I want check if I can
understand your suggestion as below?

We need allocate dump buffer and save registers value when tracer is
enabled, also add their node into dump list (for sysFS, this is done
in etm4_enable_sysfs() and for perf is in etm_setup_aux() and
etm4_enable_perf()).  After panic happens we iterate all tracer dump
nodes and only update buffer info if (buf != NULL).

> Removal of the tracers from the dump list should be done in etm_free_aux()
> 
> This will need testing :o)

Thanks a lot for the detailed suggestion, will verify them.

[...]

Thanks,
Leo Yan