Hi Linu

On 16/09/2024 11:34, Linu Cherian wrote:

• Get reserved region from device tree node for metadata
• Define metadata format for TMC
• Add TMC ETR panic sync handler that syncs register snapshot to metadata region
• Add TMC ETF panic sync handler that syncs register snapshot to metadata region and internal SRAM to reserved trace buffer region.

The patch looks good overall. Some minor comments below.

Signed-off-by: Linu Cherian lcherian@marvell.com

Changelog from v9:

• Add common helper function of_tmc_get_reserved_resource_by_name for better code reuse
• Inorder to keep the reserved buffer validity and crashdata validity independent, is_tmc_reserved_region_valid renamed to tmc_has_reserved_buffer
• drvdata->crash_tbuf renamed to drvdata->resrv_buf
• New fields added to crash metadata: version, ffcr, ffsr, mode
• Defined crashdata version with Major version 1, Minor version 0
• Set version while creating crashdata record
• Removed Reviewed-by tag due to the above changes .../hwtracing/coresight/coresight-tmc-core.c | 14 ++++ .../hwtracing/coresight/coresight-tmc-etf.c | 77 ++++++++++++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 78 +++++++++++++++++++ drivers/hwtracing/coresight/coresight-tmc.h | 66 ++++++++++++++++ 4 files changed, 235 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 0764c21aba0f..54bf8ae2bff8 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -445,6 +445,20 @@ static void tmc_get_reserved_region(struct device *parent) drvdata->resrv_buf.paddr = res.start; drvdata->resrv_buf.size = resource_size(&res);

• if (of_tmc_get_reserved_resource_by_name(parent, "metadata", &res))

• return;
  

• drvdata->crash_mdata.vaddr = memremap(res.start,

• 			       resource_size(&res),
  

• 			       MEMREMAP_WC);
  

• if (IS_ERR_OR_NULL(drvdata->crash_mdata.vaddr)) {

• dev_err(parent, "Metadata memory mapping failed\n");
  

• return;
  

• }
• drvdata->crash_mdata.paddr = res.start;
• drvdata->crash_mdata.size = resource_size(&res); }

/* Detect and initialise the capabilities of a TMC ETR */ diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index d4f641cd9de6..d77ec9307e98 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -590,6 +590,78 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, return to_read; } +static int tmc_panic_sync_etf(struct coresight_device *csdev) +{

• u32 val;
• struct csdev_access *csa;
• struct tmc_crash_metadata *mdata;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• csa = &drvdata->csdev->access;
• mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
• /* Make sure we have valid reserved memory */
• if (!tmc_has_reserved_buffer(drvdata) ||

•    !tmc_has_crash_mdata_buffer(drvdata))
  

• return 0;
  

• tmc_crashdata_set_invalid(drvdata);
• CS_UNLOCK(drvdata->base);
• /* Proceed only if ETF is enabled or configured as sink */
• val = readl(drvdata->base + TMC_CTL);
• if (!(val & TMC_CTL_CAPT_EN))

• goto out;
  

minor nit : Since the check below is "covered" by the same comment above, please drop the extra line here to make it clear that "we check for sink" by checking the "MODE == CIRCULAR_BUFFER".

• val = readl(drvdata->base + TMC_MODE);
• if (val != TMC_MODE_CIRCULAR_BUFFER)

• goto out;
  

• val = readl(drvdata->base + TMC_FFSR);
• /* Do manual flush and stop only if its not auto-stopped */
• if (!(val & TMC_FFSR_FT_STOPPED)) {

• dev_info(&csdev->dev,
  

• 	 "%s: Triggering manual flush\n", __func__);
  

Please drop the ^^^ line. We don't want to do anything like that from a panic callback.

• tmc_flush_and_stop(drvdata);
  

• } else

• tmc_wait_for_tmcready(drvdata);
  

• /* Sync registers from hardware to metadata region */
• mdata->sts = csdev_access_relaxed_read32(csa, TMC_STS);

Why are we using "csa" here and not for TMC_CTL etc ? Simply drop the "csa" and use the raw reads like above. TMC doesn't have anyother modes of access.

• mdata->mode = csdev_access_relaxed_read32(csa, TMC_MODE);
• mdata->ffcr = csdev_access_relaxed_read32(csa, TMC_FFCR);
• mdata->ffsr = csdev_access_relaxed_read32(csa, TMC_FFSR);
• mdata->trace_paddr = drvdata->resrv_buf.paddr;
• /* Sync Internal SRAM to reserved trace buffer region */
• drvdata->buf = drvdata->resrv_buf.vaddr;
• tmc_etb_dump_hw(drvdata);
• /* Store as per RSZ register convention */
• mdata->size = drvdata->len >> 2;
• mdata->version = CS_CRASHDATA_VERSION;
• /*

• * Make sure all previous writes are completed,
  

• * before we mark valid
  

• */
  

• dsb(sy);

I don't think this matters much, as this would only be read by a secondary kernel. In the worst case, you only need `dmb(ish)` to make sure the writes are visible before valid is set to true.

• mdata->valid = true;
• /*

• * Below order need to maintained, since crc of metadata
  

• * is dependent on first
  

• */
  

• mdata->crc32_tdata = find_crash_tracedata_crc(drvdata, mdata);
• mdata->crc32_mdata = find_crash_metadata_crc(mdata);
• tmc_disable_hw(drvdata);
• dev_info(&csdev->dev, "%s: success\n", __func__);

Please no "prints" from a panic call back, unless it absolutely CRITICAL.

+out:

• CS_UNLOCK(drvdata->base);
• return 0;

+}

• static const struct coresight_ops_sink tmc_etf_sink_ops = { .enable = tmc_enable_etf_sink, .disable = tmc_disable_etf_sink,

@@ -603,6 +675,10 @@ static const struct coresight_ops_link tmc_etf_link_ops = { .disable = tmc_disable_etf_link, }; +static const struct coresight_ops_panic tmc_etf_sync_ops = {

• .sync = tmc_panic_sync_etf,

+};

• const struct coresight_ops tmc_etb_cs_ops = { .sink_ops = &tmc_etf_sink_ops, };

@@ -610,6 +686,7 @@ const struct coresight_ops tmc_etb_cs_ops = { const struct coresight_ops tmc_etf_cs_ops = { .sink_ops = &tmc_etf_sink_ops, .link_ops = &tmc_etf_link_ops,

• .panic_ops = &tmc_etf_sync_ops, };

int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 8bca5b36334a..8228d7aaa361 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1814,6 +1814,79 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev) return 0; } +static int tmc_panic_sync_etr(struct coresight_device *csdev) +{

• u32 val;
• struct csdev_access *csa;
• struct tmc_crash_metadata *mdata;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• csa = &drvdata->csdev->access;

As earlier, drop the csa.

• mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
• if (!drvdata->etr_buf)

• return 0;
  

• /* Being in RESRV mode implies valid reserved memory as well */
• if (drvdata->etr_buf->mode != ETR_MODE_RESRV)

• return 0;
  

• if (!tmc_has_reserved_buffer(drvdata) ||

Do we need to check this again ? We wouldn't be in ETR_MODE_RESRV otherwise, also indicated by the comment.

•    !tmc_has_crash_mdata_buffer(drvdata))
  

• return 0;
  

• tmc_crashdata_set_invalid(drvdata);
• CS_UNLOCK(drvdata->base);
• /* Proceed only if ETR is enabled */
• val = readl(drvdata->base + TMC_CTL);
• if (!(val & TMC_CTL_CAPT_EN))

• goto out;
  

• val = readl(drvdata->base + TMC_FFSR);
• /* Do manual flush and stop only if its not auto-stopped */
• if (!(val & TMC_FFSR_FT_STOPPED)) {

• dev_info(&csdev->dev,
  

• 	 "%s: Triggering manual flush\n", __func__);
  

Drop the info

• tmc_flush_and_stop(drvdata);
  

• } else

• tmc_wait_for_tmcready(drvdata);
  

• /* Sync registers from hardware to metadata region */
• mdata->size = csdev_access_relaxed_read32(csa, TMC_RSZ);
• mdata->sts = csdev_access_relaxed_read32(csa, TMC_STS);
• mdata->mode = csdev_access_relaxed_read32(csa, TMC_MODE);
• mdata->ffcr = csdev_access_relaxed_read32(csa, TMC_FFCR);
• mdata->ffsr = csdev_access_relaxed_read32(csa, TMC_FFSR);

Please use raw reads, don't mix csa and raw reads.

• mdata->rrp = tmc_read_rrp(drvdata);
• mdata->rwp = tmc_read_rwp(drvdata);
• mdata->dba = tmc_read_dba(drvdata);
• mdata->trace_paddr = drvdata->resrv_buf.paddr;
• mdata->version = CS_CRASHDATA_VERSION;
• /*

• * Make sure all previous writes are completed,
  

• * before we mark valid
  

• */
  

• dsb(sy);

Same as earlier, doesn't buy us much

• mdata->valid = true;
• /*

• * Below order need to maintained, since crc of metadata
  

• * is dependent on first
  

• */
  

• mdata->crc32_tdata = find_crash_tracedata_crc(drvdata, mdata);
• mdata->crc32_mdata = find_crash_metadata_crc(mdata);
• tmc_disable_hw(drvdata);
• dev_info(&csdev->dev, "%s: success\n", __func__);

Drop

+out:

• CS_UNLOCK(drvdata->base);
• return 0;

+}

• static const struct coresight_ops_sink tmc_etr_sink_ops = { .enable = tmc_enable_etr_sink, .disable = tmc_disable_etr_sink,

@@ -1822,8 +1895,13 @@ static const struct coresight_ops_sink tmc_etr_sink_ops = { .free_buffer = tmc_free_etr_buffer, }; +static const struct coresight_ops_panic tmc_etr_sync_ops = {

• .sync = tmc_panic_sync_etr,

+};

• const struct coresight_ops tmc_etr_cs_ops = { .sink_ops = &tmc_etr_sink_ops,
• .panic_ops = &tmc_etr_sync_ops, };

int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index d2261eddab71..75e504e51956 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -12,6 +12,7 @@ #include <linux/miscdevice.h> #include <linux/mutex.h> #include <linux/refcount.h> +#include <linux/crc32.h> #define TMC_RSZ 0x004 #define TMC_STS 0x00c @@ -76,6 +77,9 @@ #define TMC_AXICTL_AXCACHE_OS (0xf << 2) #define TMC_AXICTL_ARCACHE_OS (0xf << 16) +/* TMC_FFSR - 0x300 */ +#define TMC_FFSR_FT_STOPPED BIT(1)

• /* TMC_FFCR - 0x304 */ #define TMC_FFCR_FLUSHMAN_BIT 6 #define TMC_FFCR_EN_FMT BIT(0)

@@ -94,6 +98,9 @@ #define TMC_AUTH_NSID_MASK GENMASK(1, 0) +/* Major version 1 Minor version 0 */ +#define CS_CRASHDATA_VERSION (1 << 16)

• enum tmc_config_type { TMC_CONFIG_TYPE_ETB, TMC_CONFIG_TYPE_ETR,

@@ -131,6 +138,25 @@ enum tmc_mem_intf_width { #define CORESIGHT_SOC_600_ETR_CAPS \ (TMC_ETR_SAVE_RESTORE | TMC_ETR_AXI_ARCACHE) +/* TMC metadata region for ETR and ETF configurations */ +struct tmc_crash_metadata {

• uint32_t crc32_mdata; /* crc of metadata */
• uint32_t crc32_tdata; /* crc of tracedata */
• uint32_t version; /* 31:16 Major version, 15:0 Minor version */
• uint32_t valid; /* Indicate if this ETF/ETR was enabled */
• uint32_t size; /* Ram Size register */

Please could you not keep this "plain bytes" ? Or rename the field to tmc_ram_size if we want to stick to TMC RAM SIZE register. It is very easy to confuse it with "normal" bytes.

• uint32_t sts; /* Status register */

tmc_sts

• uint32_t mode; /* Mode register */

tmc_mode

This doesn't look packed. Please could you add a padding here to make sure the fields are 64bit aligned ?

• uint64_t ffcr; /* Formatter and flush control register */

tmc_ffcr

• uint64_t ffsr; /* Formatter and flush status register */

tmc_ffsr

Also, why are they both 64bit ? They are all 32bit for sure ?

• uint32_t reserved32[3];

Why do we have reserved bits here ? They should be near the 32bit fields.

I think, once you fix the type of ffcr and ffsr things, everything will be in order.

• uint64_t rrp; /* Ram Read pointer register */
• uint64_t rwp; /* Ram Write pointer register */
• uint64_t dba; /* Data buffer address register */
• uint64_t trace_paddr; /* Phys address of trace buffer */
• uint64_t reserved64[3];

+};

Suzuki

• enum etr_mode { ETR_MODE_FLAT, /* Uses contiguous flat buffer */ ETR_MODE_ETR_SG, /* Uses in-built TMC ETR SG mechanism */

@@ -205,6 +231,8 @@ struct tmc_resrv_buf {

• retention (after crash) only when ETR_MODE_RESRV buffer
  

• mode is enabled. Used by ETF for trace data retention
  

• (after crash) by default.
  

• • @crash_mdata: Reserved memory for storing tmc crash metadata.

• •  Used by ETR/ETF.
    

  */ struct tmc_drvdata { struct clk *pclk;

@@ -231,6 +259,7 @@ struct tmc_drvdata { struct etr_buf *sysfs_buf; struct etr_buf *perf_buf; struct tmc_resrv_buf resrv_buf;

• struct tmc_resrv_buf crash_mdata; };

struct etr_buf_operations { @@ -356,6 +385,43 @@ static inline bool tmc_has_reserved_buffer(struct tmc_drvdata *drvdata) return false; } +static inline bool tmc_has_crash_mdata_buffer(struct tmc_drvdata *drvdata) +{

• if (drvdata->crash_mdata.vaddr &&

•    drvdata->crash_mdata.size)
  

• return true;
  

• return false;

+}

+static inline void tmc_crashdata_set_invalid(struct tmc_drvdata *drvdata) +{

• struct tmc_crash_metadata *mdata;
• mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
• if (tmc_has_crash_mdata_buffer(drvdata))

• mdata->valid = false;
  

+}

+static inline uint32_t find_crash_metadata_crc(struct tmc_crash_metadata *md) +{

• unsigned long crc_size;
• crc_size = sizeof(struct tmc_crash_metadata) -

• offsetof(struct tmc_crash_metadata, crc32_tdata);
  

• return crc32_le(0, (void *)&md->crc32_tdata, crc_size);

+}

+static inline uint32_t find_crash_tracedata_crc(struct tmc_drvdata *drvdata,

• 				struct tmc_crash_metadata *md)
  

+{

• unsigned long crc_size;
• /* Take CRC of configured buffer size to keep it simple */
• crc_size = md->size << 2;
• return crc32_le(0, (void *)drvdata->resrv_buf.vaddr, crc_size);

+}

• struct coresight_device *tmc_etr_get_catu_device(struct tmc_drvdata *drvdata);

void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu);

Hi Suzuki,

On 2024-10-17 at 17:42:21, Linu Cherian (lcherian@marvell.com) wrote:

Hi Suzuki,

On 2024-10-01 at 22:13:12, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

...

Hi Linu

On 16/09/2024 11:34, Linu Cherian wrote:

...

• Get reserved region from device tree node for metadata
• Define metadata format for TMC
• Add TMC ETR panic sync handler that syncs register snapshot to metadata region
• Add TMC ETF panic sync handler that syncs register snapshot to metadata region and internal SRAM to reserved trace buffer region.

The patch looks good overall. Some minor comments below.

...

Signed-off-by: Linu Cherian lcherian@marvell.com

Changelog from v9:

• Add common helper function of_tmc_get_reserved_resource_by_name for better code reuse
• Inorder to keep the reserved buffer validity and crashdata validity independent, is_tmc_reserved_region_valid renamed to tmc_has_reserved_buffer
• drvdata->crash_tbuf renamed to drvdata->resrv_buf
• New fields added to crash metadata: version, ffcr, ffsr, mode
• Defined crashdata version with Major version 1, Minor version 0
• Set version while creating crashdata record
• Removed Reviewed-by tag due to the above changes .../hwtracing/coresight/coresight-tmc-core.c | 14 ++++ .../hwtracing/coresight/coresight-tmc-etf.c | 77 ++++++++++++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 78 +++++++++++++++++++ drivers/hwtracing/coresight/coresight-tmc.h | 66 ++++++++++++++++ 4 files changed, 235 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 0764c21aba0f..54bf8ae2bff8 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -445,6 +445,20 @@ static void tmc_get_reserved_region(struct device *parent) drvdata->resrv_buf.paddr = res.start; drvdata->resrv_buf.size = resource_size(&res);

• if (of_tmc_get_reserved_resource_by_name(parent, "metadata", &res))

• return;
  

• drvdata->crash_mdata.vaddr = memremap(res.start,

• 			       resource_size(&res),
  

• 			       MEMREMAP_WC);
  

• if (IS_ERR_OR_NULL(drvdata->crash_mdata.vaddr)) {

• dev_err(parent, "Metadata memory mapping failed\n");
  

• return;
  

• }
• drvdata->crash_mdata.paddr = res.start;
• drvdata->crash_mdata.size = resource_size(&res); } /* Detect and initialise the capabilities of a TMC ETR */

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index d4f641cd9de6..d77ec9307e98 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -590,6 +590,78 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, return to_read; } +static int tmc_panic_sync_etf(struct coresight_device *csdev) +{

• u32 val;
• struct csdev_access *csa;
• struct tmc_crash_metadata *mdata;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• csa = &drvdata->csdev->access;
• mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
• /* Make sure we have valid reserved memory */
• if (!tmc_has_reserved_buffer(drvdata) ||

•    !tmc_has_crash_mdata_buffer(drvdata))
  

• return 0;
  

• tmc_crashdata_set_invalid(drvdata);
• CS_UNLOCK(drvdata->base);
• /* Proceed only if ETF is enabled or configured as sink */
• val = readl(drvdata->base + TMC_CTL);
• if (!(val & TMC_CTL_CAPT_EN))

• goto out;
  

minor nit : Since the check below is "covered" by the same comment above, please drop the extra line here to make it clear that "we check for sink" by checking the "MODE == CIRCULAR_BUFFER".

Ack.

...

...

• val = readl(drvdata->base + TMC_MODE);
• if (val != TMC_MODE_CIRCULAR_BUFFER)

• goto out;
  

• val = readl(drvdata->base + TMC_FFSR);
• /* Do manual flush and stop only if its not auto-stopped */
• if (!(val & TMC_FFSR_FT_STOPPED)) {

• dev_info(&csdev->dev,
  

• 	 "%s: Triggering manual flush\n", __func__);
  

Please drop the ^^^ line. We don't want to do anything like that from a panic callback.

Ack.

Are we okay to change this dev_dbg instead of removing it. Have found this quite useful to understand if the CTI trigger has fired.

...

...

• tmc_flush_and_stop(drvdata);
  

• } else

• tmc_wait_for_tmcready(drvdata);
  

• /* Sync registers from hardware to metadata region */
• mdata->sts = csdev_access_relaxed_read32(csa, TMC_STS);

Why are we using "csa" here and not for TMC_CTL etc ? Simply drop the "csa" and use the raw reads like above. TMC doesn't have anyother modes of access.

Okay.

...

...

• mdata->mode = csdev_access_relaxed_read32(csa, TMC_MODE);
• mdata->ffcr = csdev_access_relaxed_read32(csa, TMC_FFCR);
• mdata->ffsr = csdev_access_relaxed_read32(csa, TMC_FFSR);
• mdata->trace_paddr = drvdata->resrv_buf.paddr;
• /* Sync Internal SRAM to reserved trace buffer region */
• drvdata->buf = drvdata->resrv_buf.vaddr;
• tmc_etb_dump_hw(drvdata);
• /* Store as per RSZ register convention */
• mdata->size = drvdata->len >> 2;
• mdata->version = CS_CRASHDATA_VERSION;
• /*

• * Make sure all previous writes are completed,
  

• * before we mark valid
  

• */
  

• dsb(sy);

I don't think this matters much, as this would only be read by a secondary kernel. In the worst case, you only need `dmb(ish)` to make sure the writes are visible before valid is set to true.

Ack. Will change that.

...

...

• mdata->valid = true;
• /*

• * Below order need to maintained, since crc of metadata
  

• * is dependent on first
  

• */
  

• mdata->crc32_tdata = find_crash_tracedata_crc(drvdata, mdata);
• mdata->crc32_mdata = find_crash_metadata_crc(mdata);
• tmc_disable_hw(drvdata);
• dev_info(&csdev->dev, "%s: success\n", __func__);

Please no "prints" from a panic call back, unless it absolutely CRITICAL.

Ack.

Would it be okay to convert this to dev_dbg? Helps to understand if the panic call handler has run just in case if we want to debug if the metadata saving happened or not.

...

...

+out:

• CS_UNLOCK(drvdata->base);
• return 0;

+}

• static const struct coresight_ops_sink tmc_etf_sink_ops = { .enable = tmc_enable_etf_sink, .disable = tmc_disable_etf_sink,

@@ -603,6 +675,10 @@ static const struct coresight_ops_link tmc_etf_link_ops = { .disable = tmc_disable_etf_link, }; +static const struct coresight_ops_panic tmc_etf_sync_ops = {

• .sync = tmc_panic_sync_etf,

+};

• const struct coresight_ops tmc_etb_cs_ops = { .sink_ops = &tmc_etf_sink_ops, };

@@ -610,6 +686,7 @@ const struct coresight_ops tmc_etb_cs_ops = { const struct coresight_ops tmc_etf_cs_ops = { .sink_ops = &tmc_etf_sink_ops, .link_ops = &tmc_etf_link_ops,

• .panic_ops = &tmc_etf_sync_ops, }; int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 8bca5b36334a..8228d7aaa361 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1814,6 +1814,79 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev) return 0; } +static int tmc_panic_sync_etr(struct coresight_device *csdev) +{

• u32 val;
• struct csdev_access *csa;
• struct tmc_crash_metadata *mdata;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• csa = &drvdata->csdev->access;

As earlier, drop the csa.

Okay.

...

...

• mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
• if (!drvdata->etr_buf)

• return 0;
  

• /* Being in RESRV mode implies valid reserved memory as well */
• if (drvdata->etr_buf->mode != ETR_MODE_RESRV)

• return 0;
  

• if (!tmc_has_reserved_buffer(drvdata) ||

Do we need to check this again ? We wouldn't be in ETR_MODE_RESRV otherwise, also indicated by the comment.

Will drop.

...

...

•    !tmc_has_crash_mdata_buffer(drvdata))
  

• return 0;
  

• tmc_crashdata_set_invalid(drvdata);
• CS_UNLOCK(drvdata->base);
• /* Proceed only if ETR is enabled */
• val = readl(drvdata->base + TMC_CTL);
• if (!(val & TMC_CTL_CAPT_EN))

• goto out;
  

• val = readl(drvdata->base + TMC_FFSR);
• /* Do manual flush and stop only if its not auto-stopped */
• if (!(val & TMC_FFSR_FT_STOPPED)) {

• dev_info(&csdev->dev,
  

• 	 "%s: Triggering manual flush\n", __func__);
  

Drop the info

Ack.

...

...

• tmc_flush_and_stop(drvdata);
  

• } else

• tmc_wait_for_tmcready(drvdata);
  

• /* Sync registers from hardware to metadata region */
• mdata->size = csdev_access_relaxed_read32(csa, TMC_RSZ);
• mdata->sts = csdev_access_relaxed_read32(csa, TMC_STS);
• mdata->mode = csdev_access_relaxed_read32(csa, TMC_MODE);
• mdata->ffcr = csdev_access_relaxed_read32(csa, TMC_FFCR);
• mdata->ffsr = csdev_access_relaxed_read32(csa, TMC_FFSR);

Please use raw reads, don't mix csa and raw reads.

Ack.

...

...

• mdata->rrp = tmc_read_rrp(drvdata);
• mdata->rwp = tmc_read_rwp(drvdata);
• mdata->dba = tmc_read_dba(drvdata);
• mdata->trace_paddr = drvdata->resrv_buf.paddr;
• mdata->version = CS_CRASHDATA_VERSION;
• /*

• * Make sure all previous writes are completed,
  

• * before we mark valid
  

• */
  

• dsb(sy);

Same as earlier, doesn't buy us much

Will convert to dmb.

...

...

• mdata->valid = true;
• /*

• * Below order need to maintained, since crc of metadata
  

• * is dependent on first
  

• */
  

• mdata->crc32_tdata = find_crash_tracedata_crc(drvdata, mdata);
• mdata->crc32_mdata = find_crash_metadata_crc(mdata);
• tmc_disable_hw(drvdata);
• dev_info(&csdev->dev, "%s: success\n", __func__);

Drop

...

+out:

• CS_UNLOCK(drvdata->base);
• return 0;

+}

• static const struct coresight_ops_sink tmc_etr_sink_ops = { .enable = tmc_enable_etr_sink, .disable = tmc_disable_etr_sink,

@@ -1822,8 +1895,13 @@ static const struct coresight_ops_sink tmc_etr_sink_ops = { .free_buffer = tmc_free_etr_buffer, }; +static const struct coresight_ops_panic tmc_etr_sync_ops = {

• .sync = tmc_panic_sync_etr,

+};

• const struct coresight_ops tmc_etr_cs_ops = { .sink_ops = &tmc_etr_sink_ops,
• .panic_ops = &tmc_etr_sync_ops, }; int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)

diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index d2261eddab71..75e504e51956 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -12,6 +12,7 @@ #include <linux/miscdevice.h> #include <linux/mutex.h> #include <linux/refcount.h> +#include <linux/crc32.h> #define TMC_RSZ 0x004 #define TMC_STS 0x00c @@ -76,6 +77,9 @@ #define TMC_AXICTL_AXCACHE_OS (0xf << 2) #define TMC_AXICTL_ARCACHE_OS (0xf << 16) +/* TMC_FFSR - 0x300 */ +#define TMC_FFSR_FT_STOPPED BIT(1)

• /* TMC_FFCR - 0x304 */ #define TMC_FFCR_FLUSHMAN_BIT 6 #define TMC_FFCR_EN_FMT BIT(0)

@@ -94,6 +98,9 @@ #define TMC_AUTH_NSID_MASK GENMASK(1, 0) +/* Major version 1 Minor version 0 */ +#define CS_CRASHDATA_VERSION (1 << 16)

• enum tmc_config_type { TMC_CONFIG_TYPE_ETB, TMC_CONFIG_TYPE_ETR,

@@ -131,6 +138,25 @@ enum tmc_mem_intf_width { #define CORESIGHT_SOC_600_ETR_CAPS \ (TMC_ETR_SAVE_RESTORE | TMC_ETR_AXI_ARCACHE) +/* TMC metadata region for ETR and ETF configurations */ +struct tmc_crash_metadata {

• uint32_t crc32_mdata; /* crc of metadata */
• uint32_t crc32_tdata; /* crc of tracedata */
• uint32_t version; /* 31:16 Major version, 15:0 Minor version */
• uint32_t valid; /* Indicate if this ETF/ETR was enabled */
• uint32_t size; /* Ram Size register */

Please could you not keep this "plain bytes" ? Or rename the field to tmc_ram_size if we want to stick to TMC RAM SIZE register. It is very easy to confuse it with "normal" bytes.

Okay. Will change the name.

...

...

• uint32_t sts; /* Status register */

tmc_sts

...

• uint32_t mode; /* Mode register */

tmc_mode

This doesn't look packed. Please could you add a padding here to make sure the fields are 64bit aligned ?

...

• uint64_t ffcr; /* Formatter and flush control register */

tmc_ffcr

...

• uint64_t ffsr; /* Formatter and flush status register */

tmc_ffsr

Also, why are they both 64bit ? They are all 32bit for sure ?

...

• uint32_t reserved32[3];

Why do we have reserved bits here ? They should be near the 32bit fields.

Have added them for the sake of future extensions, just in case if we wish to add additional registers without changing the overall metadata size.

...

I think, once you fix the type of ffcr and ffsr things, everything will be in order.

Okay will fix.

...

...

• uint64_t rrp; /* Ram Read pointer register */
• uint64_t rwp; /* Ram Write pointer register */
• uint64_t dba; /* Data buffer address register */
• uint64_t trace_paddr; /* Phys address of trace buffer */
• uint64_t reserved64[3];

+};

Suzuki

...

• enum etr_mode { ETR_MODE_FLAT, /* Uses contiguous flat buffer */ ETR_MODE_ETR_SG, /* Uses in-built TMC ETR SG mechanism */

@@ -205,6 +231,8 @@ struct tmc_resrv_buf {

• retention (after crash) only when ETR_MODE_RESRV buffer
  

• mode is enabled. Used by ETF for trace data retention
  

• (after crash) by default.
  

• • @crash_mdata: Reserved memory for storing tmc crash metadata.

• •  Used by ETR/ETF.
    

  */ struct tmc_drvdata { struct clk *pclk;

@@ -231,6 +259,7 @@ struct tmc_drvdata { struct etr_buf *sysfs_buf; struct etr_buf *perf_buf; struct tmc_resrv_buf resrv_buf;

• struct tmc_resrv_buf crash_mdata; }; struct etr_buf_operations {

@@ -356,6 +385,43 @@ static inline bool tmc_has_reserved_buffer(struct tmc_drvdata *drvdata) return false; } +static inline bool tmc_has_crash_mdata_buffer(struct tmc_drvdata *drvdata) +{

• if (drvdata->crash_mdata.vaddr &&

•    drvdata->crash_mdata.size)
  

• return true;
  

• return false;

+}

+static inline void tmc_crashdata_set_invalid(struct tmc_drvdata *drvdata) +{

• struct tmc_crash_metadata *mdata;
• mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
• if (tmc_has_crash_mdata_buffer(drvdata))

• mdata->valid = false;
  

+}

+static inline uint32_t find_crash_metadata_crc(struct tmc_crash_metadata *md) +{

• unsigned long crc_size;
• crc_size = sizeof(struct tmc_crash_metadata) -

• offsetof(struct tmc_crash_metadata, crc32_tdata);
  

• return crc32_le(0, (void *)&md->crc32_tdata, crc_size);

+}

+static inline uint32_t find_crash_tracedata_crc(struct tmc_drvdata *drvdata,

• 				struct tmc_crash_metadata *md)
  

+{

• unsigned long crc_size;
• /* Take CRC of configured buffer size to keep it simple */
• crc_size = md->size << 2;
• return crc32_le(0, (void *)drvdata->resrv_buf.vaddr, crc_size);

+}

• struct coresight_device *tmc_etr_get_catu_device(struct tmc_drvdata *drvdata); void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu);

Hi,

Please ignore this message, was sent by mistake. Atually a repetition of message sent earlier.

Thanks.

On 2024-10-29 at 11:54:41, Linu Cherian (lcherian@marvell.com) wrote:

Hi Suzuki,

On 2024-10-17 at 17:42:21, Linu Cherian (lcherian@marvell.com) wrote:

...

Hi Suzuki,

On 2024-10-01 at 22:13:12, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

...

Hi Linu

On 16/09/2024 11:34, Linu Cherian wrote:

...

• Get reserved region from device tree node for metadata
• Define metadata format for TMC
• Add TMC ETR panic sync handler that syncs register snapshot to metadata region
• Add TMC ETF panic sync handler that syncs register snapshot to metadata region and internal SRAM to reserved trace buffer region.

The patch looks good overall. Some minor comments below.

...

Signed-off-by: Linu Cherian lcherian@marvell.com

Changelog from v9:

• Add common helper function of_tmc_get_reserved_resource_by_name for better code reuse
• Inorder to keep the reserved buffer validity and crashdata validity independent, is_tmc_reserved_region_valid renamed to tmc_has_reserved_buffer
• drvdata->crash_tbuf renamed to drvdata->resrv_buf
• New fields added to crash metadata: version, ffcr, ffsr, mode
• Defined crashdata version with Major version 1, Minor version 0
• Set version while creating crashdata record
• Removed Reviewed-by tag due to the above changes .../hwtracing/coresight/coresight-tmc-core.c | 14 ++++ .../hwtracing/coresight/coresight-tmc-etf.c | 77 ++++++++++++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 78 +++++++++++++++++++ drivers/hwtracing/coresight/coresight-tmc.h | 66 ++++++++++++++++ 4 files changed, 235 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 0764c21aba0f..54bf8ae2bff8 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -445,6 +445,20 @@ static void tmc_get_reserved_region(struct device *parent) drvdata->resrv_buf.paddr = res.start; drvdata->resrv_buf.size = resource_size(&res);

• if (of_tmc_get_reserved_resource_by_name(parent, "metadata", &res))

• return;
  

• drvdata->crash_mdata.vaddr = memremap(res.start,

• 			       resource_size(&res),
  

• 			       MEMREMAP_WC);
  

• if (IS_ERR_OR_NULL(drvdata->crash_mdata.vaddr)) {

• dev_err(parent, "Metadata memory mapping failed\n");
  

• return;
  

• }
• drvdata->crash_mdata.paddr = res.start;
• drvdata->crash_mdata.size = resource_size(&res); } /* Detect and initialise the capabilities of a TMC ETR */

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index d4f641cd9de6..d77ec9307e98 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -590,6 +590,78 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, return to_read; } +static int tmc_panic_sync_etf(struct coresight_device *csdev) +{

• u32 val;
• struct csdev_access *csa;
• struct tmc_crash_metadata *mdata;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• csa = &drvdata->csdev->access;
• mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
• /* Make sure we have valid reserved memory */
• if (!tmc_has_reserved_buffer(drvdata) ||

•    !tmc_has_crash_mdata_buffer(drvdata))
  

• return 0;
  

• tmc_crashdata_set_invalid(drvdata);
• CS_UNLOCK(drvdata->base);
• /* Proceed only if ETF is enabled or configured as sink */
• val = readl(drvdata->base + TMC_CTL);
• if (!(val & TMC_CTL_CAPT_EN))

• goto out;
  

minor nit : Since the check below is "covered" by the same comment above, please drop the extra line here to make it clear that "we check for sink" by checking the "MODE == CIRCULAR_BUFFER".

Ack.

...

...

• val = readl(drvdata->base + TMC_MODE);
• if (val != TMC_MODE_CIRCULAR_BUFFER)

• goto out;
  

• val = readl(drvdata->base + TMC_FFSR);
• /* Do manual flush and stop only if its not auto-stopped */
• if (!(val & TMC_FFSR_FT_STOPPED)) {

• dev_info(&csdev->dev,
  

• 	 "%s: Triggering manual flush\n", __func__);
  

Please drop the ^^^ line. We don't want to do anything like that from a panic callback.

Ack.

Are we okay with converting this to dev_dbg, as this could be quite helpful to understand if the CTI trigger has occured or not.

...

...

...

• tmc_flush_and_stop(drvdata);
  

• } else

• tmc_wait_for_tmcready(drvdata);
  

• /* Sync registers from hardware to metadata region */
• mdata->sts = csdev_access_relaxed_read32(csa, TMC_STS);

Why are we using "csa" here and not for TMC_CTL etc ? Simply drop the "csa" and use the raw reads like above. TMC doesn't have anyother modes of access.

Okay.

...

...

• mdata->mode = csdev_access_relaxed_read32(csa, TMC_MODE);
• mdata->ffcr = csdev_access_relaxed_read32(csa, TMC_FFCR);
• mdata->ffsr = csdev_access_relaxed_read32(csa, TMC_FFSR);
• mdata->trace_paddr = drvdata->resrv_buf.paddr;
• /* Sync Internal SRAM to reserved trace buffer region */
• drvdata->buf = drvdata->resrv_buf.vaddr;
• tmc_etb_dump_hw(drvdata);
• /* Store as per RSZ register convention */
• mdata->size = drvdata->len >> 2;
• mdata->version = CS_CRASHDATA_VERSION;
• /*

• * Make sure all previous writes are completed,
  

• * before we mark valid
  

• */
  

• dsb(sy);

I don't think this matters much, as this would only be read by a secondary kernel. In the worst case, you only need `dmb(ish)` to make sure the writes are visible before valid is set to true.

Ack. Will change that.

...

...

• mdata->valid = true;
• /*

• * Below order need to maintained, since crc of metadata
  

• * is dependent on first
  

• */
  

• mdata->crc32_tdata = find_crash_tracedata_crc(drvdata, mdata);
• mdata->crc32_mdata = find_crash_metadata_crc(mdata);
• tmc_disable_hw(drvdata);
• dev_info(&csdev->dev, "%s: success\n", __func__);

Please no "prints" from a panic call back, unless it absolutely CRITICAL.

Ack.

Are we okay with converting this to dev_dbg, as this could aid in debugging to understand if kernel has populated valid metadata.

Thanks. Linu Cherian.

On 2024-10-03 at 18:55:54, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

Hi Linu

On 16/09/2024 11:34, Linu Cherian wrote:

...

• Add support for reading crashdata using special device files. The special device files /dev/crash_tmc_xxx would be available for read file operation only when the crash data is valid.

• User can read the crash data as below

  For example, for reading crash data from tmc_etf sink

  #dd if=/dev/crash_tmc_etfXX of=~/cstrace.bin

There are some comments below, please take a look.

...

Signed-off-by: Anil Kumar Reddy areddy3@marvell.com Signed-off-by: Tanmay Jagdale tanmay@marvell.com Signed-off-by: Linu Cherian lcherian@marvell.com

---

Changelog from v9:

• Removed READ_CRASHDATA mode meant for special casing crashdata read
• Added new fields full, len, offset to struct tmc_resrv_buf

Why do we need "full" ? See more on that below.

...

so as to have a common read function for ETR and ETF

• Introduced read file operation, tmc_crashdata_read specific to crashdata reads common for both ETR and ETF

• Introduced is_tmc_crashdata_valid function Special device file /dev/crash_tmc_xxx will be available only when crashdata is valid.

• Version checks added to crashdata validity checks

• Mark crashdata as invalid when user starts tracing with ETR sink in "resrv" buffer mode

  .../hwtracing/coresight/coresight-tmc-core.c | 206 +++++++++++++++++- .../hwtracing/coresight/coresight-tmc-etf.c | 36 +++ .../hwtracing/coresight/coresight-tmc-etr.c | 63 ++++++ drivers/hwtracing/coresight/coresight-tmc.h | 18 +- include/linux/coresight.h | 12 + 5 files changed, 333 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 54bf8ae2bff8..47b6b3f88750 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -105,6 +105,125 @@ u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata) return mask; } +bool is_tmc_crashdata_valid(struct tmc_drvdata *drvdata) +{

• struct tmc_crash_metadata *mdata;
• if (!tmc_has_reserved_buffer(drvdata) ||

•    !tmc_has_crash_mdata_buffer(drvdata))
  

• return false;
  

• mdata = drvdata->crash_mdata.vaddr;
• /* Check version match */
• if (mdata->version != CS_CRASHDATA_VERSION)

• return false;
  

• /* Check data integrity of metadata */
• if (mdata->crc32_mdata != find_crash_metadata_crc(mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash metadata\n");
  

• return false;
  

• }
• /* Check data integrity of tracedata */
• if (mdata->crc32_tdata != find_crash_tracedata_crc(drvdata, mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash tracedata\n");
  

• return false;
  

• }
• /* Check for valid metadata */
• if (!mdata->valid) {

minor nit: This could be checked right after the VERSION and we verify the CRC anyway later and thus could skip all the CRC calculations if !valid.

Ack.

...

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Data invalid in tmc crash metadata\n");
  

• return false;
  

• }
• return true;

+}

+int tmc_read_prepare_crashdata(struct tmc_drvdata *drvdata) +{

• int ret = 0;
• unsigned long flags;
• struct tmc_crash_metadata *mdata;
• struct coresight_device *csdev = drvdata->csdev;
• spin_lock_irqsave(&drvdata->spinlock, flags);
• if (!is_tmc_crashdata_valid(drvdata)) {

• ret = -ENXIO;
  

• goto out;
  

• }
• mdata = drvdata->crash_mdata.vaddr;
• /*

• * Buffer address given by metadata for retrieval of trace data
  

• * from previous boot is expected to be same as the reserved
  

• * trace buffer memory region provided through DTS
  

• */
  

• if (drvdata->resrv_buf.paddr != mdata->trace_paddr) {

• dev_dbg(&csdev->dev, "Trace buffer address of previous boot invalid\n");
  

Couldn't this be made part of the "is_tmc_crashdata_valid()" and not repeated everytime we do the read ? Surely, this can't change after boot.

Ack. Will move.

...

• ret = -EINVAL;
  

• goto out;
  

• }
• /* Sink specific crashdata mode preparation */
• ret = crashdata_ops(csdev)->prepare(csdev);
• if (ret)

• goto out;
  

• if (mdata->sts & 0x1)

• coresight_insert_barrier_packet(drvdata->buf);
  

• drvdata->reading = true;

Why are we dealing with drvdata->reading ? That is supposed to be only for the normal trace reading ?

Ack. Will remove, we dont need this.

...

+out:

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
• return ret;

+}

+int tmc_read_unprepare_crashdata(struct tmc_drvdata *drvdata) +{

• int ret;
• unsigned long flags;
• struct coresight_device *csdev = drvdata->csdev;
• spin_lock_irqsave(&drvdata->spinlock, flags);
• /* Sink specific crashdata mode preparation */
• ret = crashdata_ops(csdev)->unprepare(csdev);
• drvdata->reading = false;

...

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
• return ret;

+}

+static inline ssize_t tmc_get_resvbuf_trace(struct tmc_drvdata *drvdata,

• 			  loff_t pos, size_t len, char **bufpp)
  

+{

• s64 offset;
• ssize_t actual = len;
• struct tmc_resrv_buf *rbuf = &drvdata->resrv_buf;
• if (pos + actual > rbuf->len)

• actual = rbuf->len - pos;
  

• if (actual <= 0)

• return actual;
  

return 0 ? Because, we went beyond the file position, not because there was an error. So, that it doesn't look like we are suppressing an ERROR ?

return 0 looks fine to me. Will recheck on this.

...

• /* Compute the offset from which we read the data */
• offset = rbuf->offset + pos;
• if (offset >= rbuf->size)

• offset -= rbuf->size;
  

• /* Adjust the length to limit this transaction to end of buffer */
• actual = (actual < (rbuf->size - offset)) ?

• actual : rbuf->size - offset;
  

• *bufpp = (char *)rbuf->vaddr + offset;
• return actual;

+}

• static int tmc_read_prepare(struct tmc_drvdata *drvdata) { int ret = 0;

@@ -224,6 +343,70 @@ static const struct file_operations tmc_fops = { .llseek = no_llseek, }; +static int tmc_crashdata_open(struct inode *inode, struct file *file) +{

• int ret;
• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• ret = tmc_read_prepare_crashdata(drvdata);

I don't see the point of this "prepare" and unprepare callbacks, as they can be made generic by populating the mdata->rrp,rwp fields accordingly ?

i.e., while populating the mdata-> fields, for ETR, do what you do now. For ETF you could :

mdata->rrp = 0; mdata->dba = 0; mdata->rwp = drvdata->len; mdata->size = drvdata->len >> 2; mdata->sts = TMC_STS_FULL;

Agree with your point that this would get rid of sink specific callbacks.

But few points to consider before we go with the above approach,

* mdata register snapshots wont be true to their definition, with such encodings.

We had a similar discussion on this earlier regarding mdata->size, ie. We decided to stick to register format instead of storing bytes. https://lore.kernel.org/linux-arm-kernel/20240620041054.GC125816@hyd1403.cav...

* We will have more comparable code between normal trace buffer reading and reading trace buffer after panic with sink specific callbacks, even though we keep the code seperate.

Please let me know your thoughts on this.

...

• if (ret)

• return ret;
  

• nonseekable_open(inode, file);
• dev_dbg(&drvdata->csdev->dev, "%s: successfully opened\n", __func__);
• return 0;

+}

+static ssize_t tmc_crashdata_read(struct file *file, char __user *data,

• 		  size_t len, loff_t *ppos)
  

+{

• char *bufp;
• ssize_t actual;
• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• actual = tmc_get_resvbuf_trace(drvdata, *ppos, len, &bufp);
• if (actual <= 0)

• return 0;
  

• if (copy_to_user(data, bufp, actual)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"%s: copy_to_user failed\n", __func__);
  

• return -EFAULT;
  

• }
• *ppos += actual;
• dev_dbg(&drvdata->csdev->dev, "%zu bytes copied\n", actual);
• return actual;

+}

+static int tmc_crashdata_release(struct inode *inode, struct file *file) +{

• int ret = 0;
• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• ret = tmc_read_unprepare_crashdata(drvdata);
• dev_dbg(&drvdata->csdev->dev, "%s: released\n", __func__);
• return ret;

+}

+static const struct file_operations tmc_crashdata_fops = {

• .owner = THIS_MODULE,
• .open = tmc_crashdata_open,
• .read = tmc_crashdata_read,
• .release = tmc_crashdata_release,
• .llseek = no_llseek,

+};

• static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid) { enum tmc_mem_intf_width memwidth;

@@ -533,6 +716,20 @@ static u32 tmc_etr_get_max_burst_size(struct device *dev) return burst_size; } +static void register_crash_dev_interface(struct tmc_drvdata *drvdata,

• 			 const char *name)
  

+{

• drvdata->crashdev.name =

• devm_kasprintf(&drvdata->csdev->dev, GFP_KERNEL, "%s_%s", "crash", name);
  

• drvdata->crashdev.minor = MISC_DYNAMIC_MINOR;
• drvdata->crashdev.fops = &tmc_crashdata_fops;
• if (misc_register(&drvdata->crashdev)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Failed to setup user interface for crashdata\n");
  

• drvdata->crashdev.fops = NULL;
  

• }

+}

• static int __tmc_probe(struct device *dev, struct resource *res) { int ret = 0;

@@ -633,8 +830,13 @@ static int __tmc_probe(struct device *dev, struct resource *res) drvdata->miscdev.minor = MISC_DYNAMIC_MINOR; drvdata->miscdev.fops = &tmc_fops; ret = misc_register(&drvdata->miscdev);

• if (ret)

• if (ret) { coresight_unregister(drvdata->csdev);

• goto out;
  

• }
• if (is_tmc_crashdata_valid(drvdata))

• register_crash_dev_interface(drvdata, desc.name);
  

  out: return ret; }

@@ -688,6 +890,8 @@ static void __tmc_remove(struct device *dev) * handler to this device is closed. */ misc_deregister(&drvdata->miscdev);

• if (drvdata->crashdev.fops)

• misc_deregister(&drvdata->crashdev);
  

  coresight_unregister(drvdata->csdev); }

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index d77ec9307e98..2b4639a1def2 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -662,6 +662,36 @@ static int tmc_panic_sync_etf(struct coresight_device *csdev) return 0; } +static int tmc_etb_prepare_crashdata(struct coresight_device *csdev) +{

• unsigned long size;
• struct tmc_resrv_buf *rbuf;
• struct tmc_crash_metadata *mdata;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• rbuf = &drvdata->resrv_buf;
• mdata = drvdata->crash_mdata.vaddr;
• size = mdata->size << 2;
• rbuf->len = size;
• rbuf->offset = 0;
• rbuf->full = 0;
• return 0;

+}

+static int tmc_etb_unprepare_crashdata(struct coresight_device *csdev) +{

• struct tmc_resrv_buf *rbuf;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• rbuf = &drvdata->resrv_buf;
• /* Reset valid length */
• rbuf->len = 0;
• return 0;

+}

• static const struct coresight_ops_sink tmc_etf_sink_ops = { .enable = tmc_enable_etf_sink, .disable = tmc_disable_etf_sink,

@@ -679,6 +709,11 @@ static const struct coresight_ops_panic tmc_etf_sync_ops = { .sync = tmc_panic_sync_etf, }; +static const struct coresight_ops_crashdata tmc_etf_crashdata_ops = {

• .prepare = tmc_etb_prepare_crashdata,
• .unprepare = tmc_etb_unprepare_crashdata,

+};

• const struct coresight_ops tmc_etb_cs_ops = { .sink_ops = &tmc_etf_sink_ops, };

@@ -687,6 +722,7 @@ const struct coresight_ops tmc_etf_cs_ops = { .sink_ops = &tmc_etf_sink_ops, .link_ops = &tmc_etf_link_ops, .panic_ops = &tmc_etf_sync_ops,

• .crashdata_ops = &tmc_etf_crashdata_ops, }; int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 8228d7aaa361..7688b8d70ced 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1887,6 +1887,55 @@ static int tmc_panic_sync_etr(struct coresight_device *csdev) return 0; } +static int tmc_etr_prepare_crashdata(struct coresight_device *csdev) +{

• u32 status;
• u64 rrp, rwp, dba;
• struct tmc_resrv_buf *rbuf;
• struct tmc_crash_metadata *mdata;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• rbuf = &drvdata->resrv_buf;
• mdata = drvdata->crash_mdata.vaddr;
• rrp = mdata->rrp;
• rwp = mdata->rwp;
• dba = mdata->dba;
• status = mdata->sts;
• rbuf->full = !!(status & TMC_STS_FULL);
• /* Sync the buffer pointers */
• rbuf->offset = rrp - dba;
• if (rbuf->full)

• rbuf->len = rbuf->size;
  

Why are we nto using mdata->size << 2 ? What if the "reserved buf" size was changes by the firmware, keeping the base same ?

Ack. Will change this.

...

• else

• rbuf->len = rwp - rrp;
  

We don't seem to be using "full" anywhere else other than this routine. So we could simply drop it ?

As I mentioned above, I don't see why the "rbuf" cannot be constructed from a generic "metadata" (provided we fix the TMC-ETF mdata storage) and drop these ETF/ETR specific prepare/unprepare callbacks.

Few concerns on this approach is posted above.

...

• /* Additional sanity checks for validating metadata */
• if ((rbuf->offset > rbuf->size) ||

•    (rbuf->len > rbuf->size)) {
  

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Offset and length invalid in tmc crash metadata\n");
  

• return -EINVAL;
  

• }
• return 0;

+}

+static int tmc_etr_unprepare_crashdata(struct coresight_device *csdev) +{

• struct tmc_resrv_buf *rbuf;
• struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• rbuf = &drvdata->resrv_buf;
• /* Reset valid length */
• rbuf->len = 0;
• return 0;

+}

• static const struct coresight_ops_sink tmc_etr_sink_ops = { .enable = tmc_enable_etr_sink, .disable = tmc_disable_etr_sink,

@@ -1899,9 +1948,15 @@ static const struct coresight_ops_panic tmc_etr_sync_ops = { .sync = tmc_panic_sync_etr, }; +static const struct coresight_ops_crashdata tmc_etr_crashdata_ops = {

• .prepare = tmc_etr_prepare_crashdata,
• .unprepare = tmc_etr_unprepare_crashdata,

+};

• const struct coresight_ops tmc_etr_cs_ops = { .sink_ops = &tmc_etr_sink_ops, .panic_ops = &tmc_etr_sync_ops,
• .crashdata_ops = &tmc_etr_crashdata_ops, }; int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)

@@ -1934,6 +1989,14 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) __tmc_etr_disable_hw(drvdata); drvdata->reading = true;

• /*

• * The only other place we mark the metadata invalid is during
  

• * panic handler. Normally this won't race with panic handler,
  

• * as all cpus would be stopped before running panic handler.
  

• */
  

• if (drvdata->etr_mode == ETR_MODE_RESRV)

• tmc_crashdata_set_invalid(drvdata);
  

  out: spin_unlock_irqrestore(&drvdata->spinlock, flags);

diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 75e504e51956..a6daa35c7cc1 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -195,11 +195,17 @@ struct etr_buf {

• @paddr : Start address of reserved memory region.
• @vaddr : Corresponding CPU virtual address.
• @size : Size of reserved memory region.

• • @full : Trace data overflow
• • @offset : Offset of the trace data in the buffer for consumption.
• • @len : Available trace data @buf (may round up to the beginning).

super minor nit: extra TAB

Ack.

...

*/ struct tmc_resrv_buf { phys_addr_t paddr; void *vaddr; size_t size;

• bool full;

As mentioned above, drop it ^^

...

• unsigned long offset;
• s64 len; }; /**

@@ -208,6 +214,8 @@ struct tmc_resrv_buf {

• @base: memory mapped base address for this component.
• @csdev: component vitals needed by the framework.
• @miscdev: specifics to handle "/dev/xyz.tmc" entry.

• • @crashdev: specifics to handle "/dev/crash_tmc_xyz" entry for reading

• • crash tracedata.
    

  • @spinlock: only one at a time pls.
  • @pid: Process ID of the process that owns the session that is using

  • this component. For example this would be the pid of the Perf
    

@@ -227,7 +235,10 @@ struct tmc_resrv_buf {

• @idr_mutex: Access serialisation for idr.
• @sysfs_buf: SYSFS buffer for ETR.
• @perf_buf: PERF buffer for ETR.

• • @resrv_buf: Used by ETR as hardware trace buffer and for trace data

• • @sysfs_crash_buf: Sysfs crashdata buffer for ETR. This is a special purpose

• • buffer that is used only for mapping the trace buffer from
    

• • previous crash and not for capturing trace.
    

This looks like a left over from the previous version. Please drop.

Ack.

...

• • @resrv_buf: Used by ETR as hardware trace buffer and for trace data

  • retention (after crash) only when ETR_MODE_RESRV buffer
    

  • mode is enabled. Used by ETF for trace data retention
    

  • (after crash) by default.
    

@@ -239,6 +250,7 @@ struct tmc_drvdata { void __iomem *base; struct coresight_device *csdev; struct miscdevice miscdev;

• struct miscdevice crashdev; spinlock_t spinlock; pid_t pid; bool reading;

@@ -309,6 +321,8 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata); void tmc_enable_hw(struct tmc_drvdata *drvdata); void tmc_disable_hw(struct tmc_drvdata *drvdata); u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata); +int tmc_read_prepare_crashdata(struct tmc_drvdata *drvdata); +int tmc_read_unprepare_crashdata(struct tmc_drvdata *drvdata); /* ETB/ETF functions */ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata); @@ -371,6 +385,8 @@ void tmc_sg_table_sync_data_range(struct tmc_sg_table *table, u64 offset, u64 size); ssize_t tmc_sg_table_get_data(struct tmc_sg_table *sg_table, u64 offset, size_t len, char **bufpp);

+bool is_tmc_crashdata_valid(struct tmc_drvdata *drvdata); static inline unsigned long tmc_sg_table_buf_size(struct tmc_sg_table *sg_table) { diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 2e63f7614551..f04e40828885 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -336,6 +336,7 @@ enum cs_mode { #define helper_ops(csdev) csdev->ops->helper_ops #define ect_ops(csdev) csdev->ops->ect_ops #define panic_ops(csdev) csdev->ops->panic_ops +#define crashdata_ops(csdev) csdev->ops->crashdata_ops /**

• struct coresight_ops_sink - basic operations for a sink

@@ -415,12 +416,23 @@ struct coresight_ops_panic { int (*sync)(struct coresight_device *csdev); }; +/**

• • struct coresight_ops_crashdata - Generic device ops for crashdata mode
• • @prepare : Preparation for reading crashdata mode
• */

+struct coresight_ops_crashdata {

• int (*prepare)(struct coresight_device *csdev);
• int (*unprepare)(struct coresight_device *csdev);

+};

Drop these please ^^

...

struct coresight_ops { const struct coresight_ops_sink *sink_ops; const struct coresight_ops_link *link_ops; const struct coresight_ops_source *source_ops; const struct coresight_ops_helper *helper_ops; const struct coresight_ops_panic *panic_ops;

• const struct coresight_ops_crashdata *crashdata_ops; }; static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,

Hi Suzuki,

On 2024-10-18 at 15:16:17, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

On 17/10/2024 12:40, Linu Cherian wrote:

...

On 2024-10-03 at 18:55:54, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

...

Hi Linu

On 16/09/2024 11:34, Linu Cherian wrote:

...

• Add support for reading crashdata using special device files. The special device files /dev/crash_tmc_xxx would be available for read file operation only when the crash data is valid.

• User can read the crash data as below

  For example, for reading crash data from tmc_etf sink

  #dd if=/dev/crash_tmc_etfXX of=~/cstrace.bin

There are some comments below, please take a look.

...

Signed-off-by: Anil Kumar Reddy areddy3@marvell.com Signed-off-by: Tanmay Jagdale tanmay@marvell.com Signed-off-by: Linu Cherian lcherian@marvell.com

---

Changelog from v9:

• Removed READ_CRASHDATA mode meant for special casing crashdata read
• Added new fields full, len, offset to struct tmc_resrv_buf

Why do we need "full" ? See more on that below.

...

so as to have a common read function for ETR and ETF

• Introduced read file operation, tmc_crashdata_read specific to crashdata reads common for both ETR and ETF

• Introduced is_tmc_crashdata_valid function Special device file /dev/crash_tmc_xxx will be available only when crashdata is valid.

• Version checks added to crashdata validity checks

• Mark crashdata as invalid when user starts tracing with ETR sink in "resrv" buffer mode

  .../hwtracing/coresight/coresight-tmc-core.c | 206 +++++++++++++++++- .../hwtracing/coresight/coresight-tmc-etf.c | 36 +++ .../hwtracing/coresight/coresight-tmc-etr.c | 63 ++++++ drivers/hwtracing/coresight/coresight-tmc.h | 18 +- include/linux/coresight.h | 12 + 5 files changed, 333 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 54bf8ae2bff8..47b6b3f88750 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -105,6 +105,125 @@ u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata) return mask; } +bool is_tmc_crashdata_valid(struct tmc_drvdata *drvdata) +{

• struct tmc_crash_metadata *mdata;
• if (!tmc_has_reserved_buffer(drvdata) ||

•    !tmc_has_crash_mdata_buffer(drvdata))
  

• return false;
  

• mdata = drvdata->crash_mdata.vaddr;
• /* Check version match */
• if (mdata->version != CS_CRASHDATA_VERSION)

• return false;
  

• /* Check data integrity of metadata */
• if (mdata->crc32_mdata != find_crash_metadata_crc(mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash metadata\n");
  

• return false;
  

• }
• /* Check data integrity of tracedata */
• if (mdata->crc32_tdata != find_crash_tracedata_crc(drvdata, mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash tracedata\n");
  

• return false;
  

• }
• /* Check for valid metadata */
• if (!mdata->valid) {

minor nit: This could be checked right after the VERSION and we verify the CRC anyway later and thus could skip all the CRC calculations if !valid.

Ack.

...

...

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Data invalid in tmc crash metadata\n");
  

• return false;
  

• }
• return true;

+}

+int tmc_read_prepare_crashdata(struct tmc_drvdata *drvdata) +{

• int ret = 0;
• unsigned long flags;
• struct tmc_crash_metadata *mdata;
• struct coresight_device *csdev = drvdata->csdev;
• spin_lock_irqsave(&drvdata->spinlock, flags);
• if (!is_tmc_crashdata_valid(drvdata)) {

• ret = -ENXIO;
  

• goto out;
  

• }
• mdata = drvdata->crash_mdata.vaddr;
• /*

• * Buffer address given by metadata for retrieval of trace data
  

• * from previous boot is expected to be same as the reserved
  

• * trace buffer memory region provided through DTS
  

• */
  

• if (drvdata->resrv_buf.paddr != mdata->trace_paddr) {

• dev_dbg(&csdev->dev, "Trace buffer address of previous boot invalid\n");
  

Couldn't this be made part of the "is_tmc_crashdata_valid()" and not repeated everytime we do the read ? Surely, this can't change after boot.

Ack. Will move.

...

...

• ret = -EINVAL;
  

• goto out;
  

• }
• /* Sink specific crashdata mode preparation */
• ret = crashdata_ops(csdev)->prepare(csdev);
• if (ret)

• goto out;
  

• if (mdata->sts & 0x1)

• coresight_insert_barrier_packet(drvdata->buf);
  

• drvdata->reading = true;

Why are we dealing with drvdata->reading ? That is supposed to be only for the normal trace reading ?

Ack. Will remove, we dont need this.

...

...

+out:

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
• return ret;

+}

+int tmc_read_unprepare_crashdata(struct tmc_drvdata *drvdata) +{

• int ret;
• unsigned long flags;
• struct coresight_device *csdev = drvdata->csdev;
• spin_lock_irqsave(&drvdata->spinlock, flags);
• /* Sink specific crashdata mode preparation */
• ret = crashdata_ops(csdev)->unprepare(csdev);
• drvdata->reading = false;

...

• spin_unlock_irqrestore(&drvdata->spinlock, flags);
• return ret;

+}

+static inline ssize_t tmc_get_resvbuf_trace(struct tmc_drvdata *drvdata,

• 			  loff_t pos, size_t len, char **bufpp)
  

+{

• s64 offset;
• ssize_t actual = len;
• struct tmc_resrv_buf *rbuf = &drvdata->resrv_buf;
• if (pos + actual > rbuf->len)

• actual = rbuf->len - pos;
  

• if (actual <= 0)

• return actual;
  

return 0 ? Because, we went beyond the file position, not because there was an error. So, that it doesn't look like we are suppressing an ERROR ?

return 0 looks fine to me. Will recheck on this.

...

...

• /* Compute the offset from which we read the data */
• offset = rbuf->offset + pos;
• if (offset >= rbuf->size)

• offset -= rbuf->size;
  

• /* Adjust the length to limit this transaction to end of buffer */
• actual = (actual < (rbuf->size - offset)) ?

• actual : rbuf->size - offset;
  

• *bufpp = (char *)rbuf->vaddr + offset;
• return actual;

+}

• static int tmc_read_prepare(struct tmc_drvdata *drvdata) { int ret = 0;

@@ -224,6 +343,70 @@ static const struct file_operations tmc_fops = { .llseek = no_llseek, }; +static int tmc_crashdata_open(struct inode *inode, struct file *file) +{

• int ret;
• struct tmc_drvdata *drvdata = container_of(file->private_data,

• 				   struct tmc_drvdata,
  

• 				   crashdev);
  

• ret = tmc_read_prepare_crashdata(drvdata);

I don't see the point of this "prepare" and unprepare callbacks, as they can be made generic by populating the mdata->rrp,rwp fields accordingly ?

i.e., while populating the mdata-> fields, for ETR, do what you do now. For ETF you could :

mdata->rrp = 0; mdata->dba = 0; mdata->rwp = drvdata->len; mdata->size = drvdata->len >> 2; mdata->sts = TMC_STS_FULL;

Agree with your point that this would get rid of sink specific callbacks.

But few points to consider before we go with the above approach,

• mdata register snapshots wont be true to their definition, with such encodings.

  We had a similar discussion on this earlier regarding mdata->size, ie. We decided to stick to register format instead of storing bytes. https://lore.kernel.org/linux-arm-kernel/20240620041054.GC125816@hyd1403.cav...

Understood. But whoever fills in the metdata does need to fill the mdata information above ? Including calculating the hash. So, I think it is fair to say that mdata is populated in a way that makes sense just by looking at it. In fact, we should :

mdata->dba = <address of the tracedata> mdata->rrp = mdata->dba; mdata->rwp = mdata->rrp + drvdata->len; mdata->size = drvdata->len >> 2; mdata->sts = TMC_STS_FULL;

Rather than filling in 0's.

Okay, will make this change for ETF and avoid the sink specific callbacks.

...

• We will have more comparable code between normal trace buffer reading and reading trace buffer after panic with sink specific callbacks, even though we keep the code seperate.

Lets not create "customs" here. We have a callback for a reason. i.e, the ETF has an internal SRAM which is not accessible directly by the CPU and ETR uses normal RAM.

While in this case, we have the data in CPU accessible RAM and the "driver" in this case is crash handling and can be agnostic of where the data was captured (ETF vs ETR)

Okay fine.

Suzuki

...

Please let me know your thoughts on this.

Hi Suzuki,

On 2024-10-29 at 11:51:12, Linu Cherian (lcherian@marvell.com) wrote:

Hi Suzuki,

On 2024-10-21 at 18:10:40, Linu Cherian (lcherian@marvell.com) wrote:

...

Hi Suzuki,

On 2024-10-18 at 15:16:17, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

...

On 17/10/2024 12:40, Linu Cherian wrote:

...

On 2024-10-03 at 18:55:54, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

...

Hi Linu

On 16/09/2024 11:34, Linu Cherian wrote:

...

• Add support for reading crashdata using special device files. The special device files /dev/crash_tmc_xxx would be available for read file operation only when the crash data is valid.

• User can read the crash data as below

  For example, for reading crash data from tmc_etf sink

  #dd if=/dev/crash_tmc_etfXX of=~/cstrace.bin

There are some comments below, please take a look.

...

Signed-off-by: Anil Kumar Reddy areddy3@marvell.com Signed-off-by: Tanmay Jagdale tanmay@marvell.com Signed-off-by: Linu Cherian lcherian@marvell.com

---

Changelog from v9:

• Removed READ_CRASHDATA mode meant for special casing crashdata read
• Added new fields full, len, offset to struct tmc_resrv_buf

Why do we need "full" ? See more on that below.

...

so as to have a common read function for ETR and ETF

• Introduced read file operation, tmc_crashdata_read specific to crashdata reads common for both ETR and ETF

• Introduced is_tmc_crashdata_valid function Special device file /dev/crash_tmc_xxx will be available only when crashdata is valid.

• Version checks added to crashdata validity checks

• Mark crashdata as invalid when user starts tracing with ETR sink in "resrv" buffer mode

  .../hwtracing/coresight/coresight-tmc-core.c | 206 +++++++++++++++++- .../hwtracing/coresight/coresight-tmc-etf.c | 36 +++ .../hwtracing/coresight/coresight-tmc-etr.c | 63 ++++++ drivers/hwtracing/coresight/coresight-tmc.h | 18 +- include/linux/coresight.h | 12 + 5 files changed, 333 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 54bf8ae2bff8..47b6b3f88750 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -105,6 +105,125 @@ u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata) return mask; } +bool is_tmc_crashdata_valid(struct tmc_drvdata *drvdata) +{

• struct tmc_crash_metadata *mdata;
• if (!tmc_has_reserved_buffer(drvdata) ||

•    !tmc_has_crash_mdata_buffer(drvdata))
  

• return false;
  

• mdata = drvdata->crash_mdata.vaddr;
• /* Check version match */
• if (mdata->version != CS_CRASHDATA_VERSION)

• return false;
  

• /* Check data integrity of metadata */
• if (mdata->crc32_mdata != find_crash_metadata_crc(mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash metadata\n");
  

• return false;
  

• }
• /* Check data integrity of tracedata */
• if (mdata->crc32_tdata != find_crash_tracedata_crc(drvdata, mdata)) {

• dev_dbg(&drvdata->csdev->dev,
  

• 	"CRC mismatch in tmc crash tracedata\n");
  

• return false;
  

• }
• /* Check for valid metadata */
• if (!mdata->valid) {

minor nit: This could be checked right after the VERSION and we verify the CRC anyway later and thus could skip all the CRC calculations if !valid.

Ack.

...

...

• dev_dbg(&drvdata->csdev->dev,
  

• 	"Data invalid in tmc crash metadata\n");
  

• return false;
  

• }
• return true;

+}

+int tmc_read_prepare_crashdata(struct tmc_drvdata *drvdata) +{

• int ret = 0;
• unsigned long flags;
• struct tmc_crash_metadata *mdata;
• struct coresight_device *csdev = drvdata->csdev;
• spin_lock_irqsave(&drvdata->spinlock, flags);
• if (!is_tmc_crashdata_valid(drvdata)) {

• ret = -ENXIO;
  

• goto out;
  

• }
• mdata = drvdata->crash_mdata.vaddr;
• /*

• * Buffer address given by metadata for retrieval of trace data
  

• * from previous boot is expected to be same as the reserved
  

• * trace buffer memory region provided through DTS
  

• */
  

• if (drvdata->resrv_buf.paddr != mdata->trace_paddr) {

• dev_dbg(&csdev->dev, "Trace buffer address of previous boot invalid\n");
  

Couldn't this be made part of the "is_tmc_crashdata_valid()" and not repeated everytime we do the read ? Surely, this can't change after boot.

Ack. Will move.

...

...

• ret = -EINVAL;
  

• goto out;
  

• }
• /* Sink specific crashdata mode preparation */
• ret = crashdata_ops(csdev)->prepare(csdev);
• if (ret)

• goto out;
  

• if (mdata->sts & 0x1)

• coresight_insert_barrier_packet(drvdata->buf);
  

• drvdata->reading = true;

Why are we dealing with drvdata->reading ? That is supposed to be only for the normal trace reading ?

Ack. Will remove, we dont need this.

Looked into this further and it seems like, retaining the drvdata->reading flags would be useful to avoid simultaneous crashdata reads and regular trace capture sessions using reserve buffers.

Something like this,

+static int buf_mode_set_resrv(struct tmc_drvdata *drvdata) +{ + unsigned long flags; + int err = 0; + + /* Ensure there are no active crashdata read sessions */ + spin_lock_irqsave(&drvdata->spinlock, flags); + if (!drvdata->reading) { + tmc_crashdata_set_invalid(drvdata); + drvdata->etr_mode = ETR_MODE_RESRV; + + } else + err = -EINVAL; + spin_unlock_irqrestore(&drvdata->spinlock, flags); + return err; +} + static ssize_t buf_mode_preferred_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t size) { struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); struct etr_buf_hw buf_hw;

get_etr_buf_hw(dev, &buf_hw); @@ -2039,7 +2050,7 @@ static ssize_t buf_mode_preferred_store(struct device *dev, else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_CATU]) && buf_hw.has_catu) drvdata->etr_mode = ETR_MODE_CATU; else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_RESRV]) && buf_hw.has_resrv) - drvdata->etr_mode = ETR_MODE_RESRV; + return buf_mode_set_resrv(drvdata); else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_AUTO])) drvdata->etr_mode = ETR_MODE_AUTO; else

Was acutally calling the tmc_crashdata_set_invalid in the wrong place in v10 patch.

On 2024-10-03 at 18:59:30, Suzuki K Poulose (suzuki.poulose@arm.com) wrote:

On 16/09/2024 11:34, Linu Cherian wrote:

...

Add a preloaded configuration for generating external trigger on address match. This can be used by CTI and ETR blocks to stop trace capture on kernel panic.

Kernel address for "panic" function is used as the default trigger address.

This new configuration is available as, /sys/kernel/config/cs-syscfg/configurations/panicstop

Signed-off-by: Linu Cherian lcherian@marvell.com Reviewed-by: James Clark james.clark@arm.com

---

Changelog from v9: No changes

drivers/hwtracing/coresight/Makefile | 2 +- .../coresight/coresight-cfg-preload.c | 2 + .../coresight/coresight-cfg-preload.h | 2 + .../hwtracing/coresight/coresight-cfg-pstop.c | 83 +++++++++++++++++++ 4 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 drivers/hwtracing/coresight/coresight-cfg-pstop.c

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 4ba478211b31..46ce7f39d05f 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -25,7 +25,7 @@ subdir-ccflags-y += $(condflags) obj-$(CONFIG_CORESIGHT) += coresight.o coresight-y := coresight-core.o coresight-etm-perf.o coresight-platform.o \ coresight-sysfs.o coresight-syscfg.o coresight-config.o \

• coresight-cfg-preload.o coresight-cfg-afdo.o \
  

• coresight-cfg-preload.o coresight-cfg-afdo.o coresight-cfg-pstop.o \
  

Please could you only build it when ETM4X is selected ? That way you could drop the "CONFIG" check in the code ?

coresight-cfg-pstop feature follows the approach taken in existing features like preload, afdo etc. Hence, shall we keep the same format ? Please let me know if you think otherwise.

...

coresight-syscfg-configfs.o coresight-trace-id.o

obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \ diff --git a/drivers/hwtracing/coresight/coresight-cfg-preload.c b/drivers/hwtracing/coresight/coresight-cfg-preload.c index e237a4edfa09..4980e68483c5 100644 --- a/drivers/hwtracing/coresight/coresight-cfg-preload.c +++ b/drivers/hwtracing/coresight/coresight-cfg-preload.c @@ -13,6 +13,7 @@ static struct cscfg_feature_desc *preload_feats[] = { #if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X) &strobe_etm4x,

• &gen_etrig_etm4x, #endif NULL };

@@ -20,6 +21,7 @@ static struct cscfg_feature_desc *preload_feats[] = { static struct cscfg_config_desc *preload_cfgs[] = { #if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X) &afdo_etm4x,

• &pstop_etm4x, #endif NULL };

diff --git a/drivers/hwtracing/coresight/coresight-cfg-preload.h b/drivers/hwtracing/coresight/coresight-cfg-preload.h index 21299e175477..291ba530a6a5 100644 --- a/drivers/hwtracing/coresight/coresight-cfg-preload.h +++ b/drivers/hwtracing/coresight/coresight-cfg-preload.h @@ -10,4 +10,6 @@ #if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X) extern struct cscfg_feature_desc strobe_etm4x; extern struct cscfg_config_desc afdo_etm4x; +extern struct cscfg_feature_desc gen_etrig_etm4x; +extern struct cscfg_config_desc pstop_etm4x; #endif diff --git a/drivers/hwtracing/coresight/coresight-cfg-pstop.c b/drivers/hwtracing/coresight/coresight-cfg-pstop.c new file mode 100644 index 000000000000..c2bfbd07bfaf --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-cfg-pstop.c @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright(C) 2023 Marvell.
• • Based on coresight-cfg-afdo.c
• */

+#include "coresight-config.h"

+/* ETMv4 includes and features */ +#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)

Could we not drop this check, if we only build it when CONFIG_CORESIGHT_SOURCE_ETM4X is selected. It is not useful otherwise anyways.

IIUC, this arrangement might be helpful to add register configurations for future ETM versions in the same file.

Rest looks fine to me

Suzuki

...

+#include "coresight-etm4x-cfg.h"

+/* preload configurations and features */

+/* preload in features for ETMv4 */

+/* panic_stop feature */ +static struct cscfg_parameter_desc gen_etrig_params[] = {

• {

• .name = "address",
  

• .value = (u64)panic,
  

• },

+};

+static struct cscfg_regval_desc gen_etrig_regs[] = {

• /* resource selector */
• {

• .type = CS_CFG_REG_TYPE_RESOURCE,
  

• .offset = TRCRSCTLRn(2),
  

• .hw_info = ETM4_CFG_RES_SEL,
  

• .val32 = 0x40001,
  

• },
• /* single address comparator */
• {

• .type = CS_CFG_REG_TYPE_RESOURCE | CS_CFG_REG_TYPE_VAL_64BIT |
  

• 	CS_CFG_REG_TYPE_VAL_PARAM,
  

• .offset =  TRCACVRn(0),
  

• .val32 = 0x0,
  

• },
• {

• .type = CS_CFG_REG_TYPE_RESOURCE,
  

• .offset = TRCACATRn(0),
  

• .val64 = 0xf00,
  

• },
• /* Driver external output[0] with comparator out */
• {

• .type = CS_CFG_REG_TYPE_RESOURCE,
  

• .offset = TRCEVENTCTL0R,
  

• .val32 = 0x2,
  

• },
• /* end of regs */

+};

+struct cscfg_feature_desc gen_etrig_etm4x = {

• .name = "gen_etrig",
• .description = "Generate external trigger on address match\n"

•        "parameter \'address\': address of kernel address\n",
  

• .match_flags = CS_CFG_MATCH_CLASS_SRC_ETM4,
• .nr_params = ARRAY_SIZE(gen_etrig_params),
• .params_desc = gen_etrig_params,
• .nr_regs = ARRAY_SIZE(gen_etrig_regs),
• .regs_desc = gen_etrig_regs,

+};

+/* create a panic stop configuration */

+/* the total number of parameters in used features */ +#define PSTOP_NR_PARAMS ARRAY_SIZE(gen_etrig_params)

+static const char *pstop_ref_names[] = {

• "gen_etrig",

+};

+struct cscfg_config_desc pstop_etm4x = {

• .name = "panicstop",
• .description = "Stop ETM on kernel panic\n",
• .nr_feat_refs = ARRAY_SIZE(pstop_ref_names),
• .feat_ref_names = pstop_ref_names,
• .nr_total_params = PSTOP_NR_PARAMS,

+};

+/* end of ETM4x configurations */ +#endif /* IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X) */

Hi Linu

On 16/09/2024 11:34, Linu Cherian wrote:

Add documentation on using coresight during panic and watchdog.

Thank you so much for the documentation, this will be quite useful !

Some minor comments below.

Signed-off-by: Linu Cherian lcherian@marvell.com

Changelog from v9: This patch has been newly introduced.

Documentation/trace/coresight/panic.rst | 356 ++++++++++++++++++++++++ 1 file changed, 356 insertions(+) create mode 100644 Documentation/trace/coresight/panic.rst

diff --git a/Documentation/trace/coresight/panic.rst b/Documentation/trace/coresight/panic.rst new file mode 100644 index 000000000000..3b53d91cace8 --- /dev/null +++ b/Documentation/trace/coresight/panic.rst @@ -0,0 +1,356 @@ +=================================================== +Using Coresight for Kernel panic and Watchdog reset +===================================================

+Introduction +------------ +This documentation is about using Linux coresight trace support to +debug kernel panic and watchdog reset scenarios.

+Coresight trace during Kernel panic +----------------------------------- +From the coresight driver point of view, addressing the kernel panic +situation has four main requirements.

+a. Support for allocation of trace buffer pages from reserved memory area.

• Platform can advertise this using a new device tree property added to
• relevant coresight nodes.

+b. Support for stopping coresight blocks at the time of panic

+c. Saving required metadata in the specified format

+d. Support for reading trace data captured at the time of panic

+Allocation of trace buffer pages from reserved RAM +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +A new optional device tree property "memory-region" is added to the +ETR/ETF device nodes, that would give the base address and size of trace

ETR/ETF => CoreSight TMC

+buffer.

+Static allocation of trace buffers would ensure that both IOMMU enabled +and disabled cases are handled. Also, platforms that support persistent +RAM will allow users to read trace data in the subsequent boot without +booting the crashdump kernel.

+Note: +For ETR sink devices, this reserved region will be used for both trace +capture and trace data retrieval. +For ETF sink devices, internal SRAM would be used for trace capture, +and they would be synced to reserved region for retrieval.

+Disabling coresight blocks at the time of panic +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +In order to avoid the situation of losing relevant trace data after a +kernel panic, it would be desirable to stop the coresight blocks at the +time of panic.

+This can be achieved by configuring the comparator, CTI and sink +devices as below::

•       Trigger on panic
  

• Comparator --->External out --->CTI -->External In---->ETR/ETF stop

+Saving metadata at the time of kernel panic +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +Coresight metadata involves all additional data that are required for a +successful trace decode in addition to the trace data. This involves +ETR/ETF, ETE register snapshot etc.

s/ETE/ETB ?

+A new optional device property "memory-region" is added to +the ETR/ETF/ETE device nodes for this.

same here ^^

+Reading trace data captured at the time of panic +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +Trace data captured at the time of panic, can be read from rebooted kernel +or from crashdump kernel using a special device file /dev/crash_tmc_xxx. +This device file is created only when there is a valid crashdata available.

+General flow of trace capture and decode incase of kernel panic +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +1. Enable source and sink on all the cores using the sysfs interface.

• ETR sinks should have trace buffers allocated from reserved memory,
• by selecting "resrv" buffer mode from sysfs.

+2. Run relevant tests.

+3. On a kernel panic, all coresight blocks are disabled, necessary

• metadata is synced by kernel panic handler.
• System would eventually reboot or boot a crashdump kernel.

+4. For platforms that supports crashdump kernel, raw trace data can be

• dumped using the coresight sysfs interface from the crashdump kernel
• itself. Persistent RAM is not a requirement in this case.

+5. For platforms that supports persistent RAM, trace data can be dumped

• using the coresight sysfs interface in the subsequent Linux boot.
• Crashdump kernel is not a requirement in this case. Persistent RAM
• ensures that trace data is intact across reboot.

+Coresight trace during Watchdog reset +------------------------------------- +The main difference between addressing the watchdog reset and kernel panic +case are below,

+a. Saving coresight metadata need to be taken care by the

• SCP(system control processor) firmware in the specified format,
• instead of kernel.

+b. Reserved memory region given by firmware for trace buffer and metadata

• has to be in persistent RAM.
• Note: This is a requirement for watchdog reset case but optional
• in kernel panic case.

+Watchdog reset can be supported only on platforms that meet the above +two requirements.

+Sample commands for testing a Kernel panic case with ETR sink +-------------------------------------------------------------

+1. Boot Linux kernel with "crash_kexec_post_notifiers" added to the kernel

• bootargs. This is mandatory if the user would like to read the tracedata
• from the crashdump kernel.

+2. Enable the preloaded ETM configuration

• #echo 1 > /sys/kernel/config/cs-syscfg/configurations/panicstop/enable

+3. Configure CTI using sysfs interface::

• #./cti_setup.sh
• #cat cti_setup.sh
• cd /sys/bus/coresight/devices/
• ap_cti_config () {

•  #ETM trig out[0] trigger to Channel 0
  

•  echo 0 4 > channels/trigin_attach
  

• }
• etf_cti_config () {

•  #ETF Flush in trigger from Channel 0
  

•  echo 0 1 > channels/trigout_attach
  

•  echo 1 > channels/trig_filter_enable
  

• }
• etr_cti_config () {

•  #ETR Flush in from Channel 0
  

•  echo 0 1 > channels/trigout_attach
  

•  echo 1 > channels/trig_filter_enable
  

• }
• ctidevs=`find . -name "cti*"`
• for i in $ctidevs
• do

•        cd $i
  

•        connection=`find . -name "ete*"`
  

minor nit: this could be ete or etm

•        if [ ! -z "$connection" ]
  

•        then
  

•                echo "AP CTI config for $i"
  

•                ap_cti_config
  

•        fi
  

•        connection=`find . -name "tmc_etf*"`
  

•        if [ ! -z "$connection" ]
  

•        then
  

•                echo "ETF CTI config for $i"
  

•                etf_cti_config
  

•        fi
  

•        connection=`find . -name "tmc_etr*"`
  

•        if [ ! -z "$connection" ]
  

•        then
  

•                echo "ETR CTI config for $i"
  

•                etr_cti_config
  

•        fi
  

•        cd ..
  

• done

+Note: CTI connections are SOC specific and hence the above script is +added just for reference.

+4. Choose reserved buffer mode for ETR buffer

• #echo "resrv" > /sys/bus/coresight/devices/tmc_etr0/buf_mode_preferred

+5. Enable stop on flush trigger configuration

• #echo 1 > /sys/bus/coresight/devices/tmc_etr0/stop_on_flush

+6. Start Coresight tracing on cores 1 and 2 using sysfs interface

+7. Run some application on core 1

• #taskset -c 1 dd if=/dev/urandom of=/dev/null &

+8. Invoke kernel panic on core 2

• #echo 1 > /proc/sys/kernel/panic
• #taskset -c 2 echo c > /proc/sysrq-trigger

+9. From rebooted kernel or crashdump kernel, read crashdata

• #dd if=/dev/crash_tmc_etr0 of=/trace/cstrace.bin

+10. Run opencsd decoder tools/scripts to generate the instruction trace.

+Sample instruction trace dump +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

+Core1 dump::

• A etm4_enable_hw: ffff800008ae1dd4
• CONTEXT EL2 etm4_enable_hw: ffff800008ae1dd4
• I etm4_enable_hw: ffff800008ae1dd4:
• d503201f nop
• I etm4_enable_hw: ffff800008ae1dd8:
• d503201f nop
• I etm4_enable_hw: ffff800008ae1ddc:
• d503201f nop
• I etm4_enable_hw: ffff800008ae1de0:
• d503201f nop
• I etm4_enable_hw: ffff800008ae1de4:
• d503201f nop
• I etm4_enable_hw: ffff800008ae1de8:
• d503233f paciasp
• I etm4_enable_hw: ffff800008ae1dec:
• a9be7bfd stp x29, x30, [sp, #-32]!
• I etm4_enable_hw: ffff800008ae1df0:
• 910003fd mov x29, sp
• I etm4_enable_hw: ffff800008ae1df4:
• a90153f3 stp x19, x20, [sp, #16]
• I etm4_enable_hw: ffff800008ae1df8:
• 2a0003f4 mov w20, w0
• I etm4_enable_hw: ffff800008ae1dfc:
• 900085b3 adrp x19, ffff800009b95000 <reserved_mem+0xc48>
• I etm4_enable_hw: ffff800008ae1e00:
• 910f4273 add x19, x19, #0x3d0
• I etm4_enable_hw: ffff800008ae1e04:
• f8747a60 ldr x0, [x19, x20, lsl #3]
• E etm4_enable_hw: ffff800008ae1e08:
• b4000140 cbz x0, ffff800008ae1e30 <etm4_starting_cpu+0x50>
• I 149.039572921 etm4_enable_hw: ffff800008ae1e30:
• a94153f3 ldp x19, x20, [sp, #16]
• I 149.039572921 etm4_enable_hw: ffff800008ae1e34:
• 52800000 mov w0, #0x0 // #0
• I 149.039572921 etm4_enable_hw: ffff800008ae1e38:
• a8c27bfd ldp x29, x30, [sp], #32
• ..snip

•    149.052324811           chacha_block_generic: ffff800008642d80:
  

• 9100a3e0 add x0,
• I 149.052324811 chacha_block_generic: ffff800008642d84:
• b86178a2 ldr w2, [x5, x1, lsl #2]
• I 149.052324811 chacha_block_generic: ffff800008642d88:
• 8b010803 add x3, x0, x1, lsl #2
• I 149.052324811 chacha_block_generic: ffff800008642d8c:
• b85fc063 ldur w3, [x3, #-4]
• I 149.052324811 chacha_block_generic: ffff800008642d90:
• 0b030042 add w2, w2, w3
• I 149.052324811 chacha_block_generic: ffff800008642d94:
• b8217882 str w2, [x4, x1, lsl #2]
• I 149.052324811 chacha_block_generic: ffff800008642d98:
• 91000421 add x1, x1, #0x1
• I 149.052324811 chacha_block_generic: ffff800008642d9c:
• f100443f cmp x1, #0x11

+Core 2 dump::

• A etm4_enable_hw: ffff800008ae1dd4
• CONTEXT EL2 etm4_enable_hw: ffff800008ae1dd4
• I etm4_enable_hw: ffff800008ae1dd4:
• d503201f nop
• I etm4_enable_hw: ffff800008ae1dd8:
• d503201f nop
• I etm4_enable_hw: ffff800008ae1ddc:
• d503201f nop
• I etm4_enable_hw: ffff800008ae1de0:
• d503201f nop
• I etm4_enable_hw: ffff800008ae1de4:
• d503201f nop
• I etm4_enable_hw: ffff800008ae1de8:
• d503233f paciasp
• I etm4_enable_hw: ffff800008ae1dec:
• a9be7bfd stp x29, x30, [sp, #-32]!
• I etm4_enable_hw: ffff800008ae1df0:
• 910003fd mov x29, sp
• I etm4_enable_hw: ffff800008ae1df4:
• a90153f3 stp x19, x20, [sp, #16]
• I etm4_enable_hw: ffff800008ae1df8:
• 2a0003f4 mov w20, w0
• I etm4_enable_hw: ffff800008ae1dfc:
• 900085b3 adrp x19, ffff800009b95000 <reserved_mem+0xc48>
• I etm4_enable_hw: ffff800008ae1e00:
• 910f4273 add x19, x19, #0x3d0
• I etm4_enable_hw: ffff800008ae1e04:
• f8747a60 ldr x0, [x19, x20, lsl #3]
• E etm4_enable_hw: ffff800008ae1e08:
• b4000140 cbz x0, ffff800008ae1e30 <etm4_starting_cpu+0x50>
• I 149.046243445 etm4_enable_hw: ffff800008ae1e30:
• a94153f3 ldp x19, x20, [sp, #16]
• I 149.046243445 etm4_enable_hw: ffff800008ae1e34:
• 52800000 mov w0, #0x0 // #0
• I 149.046243445 etm4_enable_hw: ffff800008ae1e38:
• a8c27bfd ldp x29, x30, [sp], #32
• I 149.046243445 etm4_enable_hw: ffff800008ae1e3c:
• d50323bf autiasp
• E 149.046243445 etm4_enable_hw: ffff800008ae1e40:
• d65f03c0 ret
• A ete_sysreg_write: ffff800008adfa18
• ..snip
• I 149.05422547 panic: ffff800008096300:
• a90363f7 stp x23, x24, [sp, #48]
• I 149.05422547 panic: ffff800008096304:
• 6b00003f cmp w1, w0
• I 149.05422547 panic: ffff800008096308:
• 3a411804 ccmn w0, #0x1, #0x4, ne // ne = any
• N 149.05422547 panic: ffff80000809630c:
• 540001e0 b.eq ffff800008096348 <panic+0xe0> // b.none
• I 149.05422547 panic: ffff800008096310:
• f90023f9 str x25, [sp, #64]
• E 149.05422547 panic: ffff800008096314:
• 97fe44ef bl ffff8000080276d0 <panic_smp_self_stop>
• A panic: ffff80000809634c
• I 149.05422547 panic: ffff80000809634c:
• 910102d5 add x21, x22, #0x40
• I 149.05422547 panic: ffff800008096350:
• 52800020 mov w0, #0x1 // #1
• E 149.05422547 panic: ffff800008096354:
• 94166b8b bl ffff800008631180 <bust_spinlocks>
• N 149.054225518 bust_spinlocks: ffff800008631180:
• 340000c0 cbz w0, ffff800008631198 <bust_spinlocks+0x18>
• I 149.054225518 bust_spinlocks: ffff800008631184:
• f000a321 adrp x1, ffff800009a98000 <pbufs.0+0xbb8>
• I 149.054225518 bust_spinlocks: ffff800008631188:
• b9405c20 ldr w0, [x1, #92]
• I 149.054225518 bust_spinlocks: ffff80000863118c:
• 11000400 add w0, w0, #0x1
• I 149.054225518 bust_spinlocks: ffff800008631190:
• b9005c20 str w0, [x1, #92]
• E 149.054225518 bust_spinlocks: ffff800008631194:
• d65f03c0 ret
• A panic: ffff800008096358

+Perf based testing +------------------

+Starting perf session +~~~~~~~~~~~~~~~~~~~~~ +ETF: +perf record -e cs_etm/panicstop,@tmc_etf1/ -C 1 +perf record -e cs_etm/panicstop,@tmc_etf2/ -C 2

+ETR: +perf record -e cs_etm/panicstop,@tmc_etr0/ -C 1,2

+Reading trace data after panic +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +Same sysfs based method explained above can be used to retrieve and +decode the trace data after the reboot on kernel panic.