memory-region 0: Reserved trace buffer memory

TMC ETR: When available, use this reserved memory region for trace data capture. Same region is used for trace data retention after a panic or watchdog reset.

TMC ETF: When available, use this reserved memory region for trace data retention synced from internal SRAM after a panic or watchdog reset.

memory-region 1: Reserved meta data memory

TMC ETR, ETF: When available, use this memory for register snapshot retention synced from hardware registers after a panic or watchdog reset.

Signed-off-by: Linu Cherian lcherian@marvell.com --- .../bindings/arm/arm,coresight-tmc.yaml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml index cb8dceaca70e..45ca4d02d73e 100644 --- a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml +++ b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml @@ -101,6 +101,22 @@ properties: and ETF configurations. $ref: /schemas/graph.yaml#/properties/port

+ memory-region: + items: + - description: Reserved trace buffer memory for ETR and ETF sinks. + For ETR, this reserved memory region is used for trace data capture. + Same region is used for trace data retention as well after a panic + or watchdog reset. + For ETF, this reserved memory region is used for retention of trace + data synced from internal SRAM after a panic or watchdog reset. + + - description: Reserved meta data memory. Used for ETR and ETF sinks. + + memory-region-names: + items: + - const: trace-mem + - const: metadata-mem + required: - compatible - reg @@ -115,6 +131,9 @@ examples: etr@20070000 { compatible = "arm,coresight-tmc", "arm,primecell"; reg = <0x20070000 0x1000>; + memory-region = <&etr_trace_mem_reserved>, + <&etr_mdata_mem_reserved>; + memory-region-names = "trace-mem", "metadata-mem";

clocks = <&oscclk6a>; clock-names = "apb_pclk";

Panic callback handlers allows coresight device drivers to sync relevant trace data and trace metadata to reserved memory regions so that they can be retrieved later in the subsequent boot or in the crashdump kernel.

Signed-off-by: Linu Cherian lcherian@marvell.com --- drivers/hwtracing/coresight/coresight-core.c | 32 ++++++++++++++++++++ include/linux/coresight.h | 12 ++++++++ 2 files changed, 44 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 9fabe00a40d6..dfa695c103de 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -20,6 +20,7 @@ #include <linux/property.h> #include <linux/delay.h> #include <linux/pm_runtime.h> +#include <linux/panic_notifier.h>

#include "coresight-etm-perf.h" #include "coresight-priv.h" @@ -1800,6 +1801,31 @@ struct bus_type coresight_bustype = { .name = "coresight", };

+static int coresight_panic_sync(struct device *dev, void *data) +{ + + struct coresight_device *csdev = container_of(dev, struct coresight_device, dev); + + /* Run through panic sync handlers for all enabled devices */ + if (csdev->enable && panic_ops(csdev)) + panic_ops(csdev)->sync(csdev); + + return 0; +} + +static int coresight_panic_cb(struct notifier_block *self, + unsigned long v, void *p) +{ + bus_for_each_dev(&coresight_bustype, NULL, NULL, + coresight_panic_sync); + + return 0; +} + +static struct notifier_block coresight_notifier = { + .notifier_call = coresight_panic_cb, +}; + static int __init coresight_init(void) { int ret; @@ -1812,6 +1838,10 @@ static int __init coresight_init(void) if (ret) goto exit_bus_unregister;

+ /* Register function to be called for panic */ + ret = atomic_notifier_chain_register(&panic_notifier_list, + &coresight_notifier); + /* initialise the coresight syscfg API */ ret = cscfg_init(); if (!ret) @@ -1826,6 +1856,8 @@ static int __init coresight_init(void) static void __exit coresight_exit(void) { cscfg_exit(); + atomic_notifier_chain_unregister(&panic_notifier_list, + &coresight_notifier); etm_perf_exit(); bus_unregister(&coresight_bustype); } diff --git a/include/linux/coresight.h b/include/linux/coresight.h index a269fffaf991..4fd518738958 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -301,6 +301,7 @@ enum cs_mode { #define link_ops(csdev) csdev->ops->link_ops #define helper_ops(csdev) csdev->ops->helper_ops #define ect_ops(csdev) csdev->ops->ect_ops +#define panic_ops(csdev) csdev->ops->panic_ops

/** * struct coresight_ops_sink - basic operations for a sink @@ -370,11 +371,22 @@ struct coresight_ops_helper { int (*disable)(struct coresight_device *csdev, void *data); };

+ +/** + * struct coresight_ops_panic - Generic device ops for panic handing + * + * @sync : Sync the device register state/trace data + */ +struct coresight_ops_panic { + int (*sync)(struct coresight_device *csdev); +}; + 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; };

#if IS_ENABLED(CONFIG_CORESIGHT)

* Introduce a new mode CS_MODE_READ_PREVBOOT for reading tracedata captured in previous boot.

* Add special handlers for preparing ETR/ETF for this special mode

* User can read the trace data as below

For example, for reading trace data from tmc_etf sink

1. cd /sys/bus/coresight/devices/tmc_etfXX/

2. Change mode to READ_PREVBOOT

#echo 1 > read_prevboot

3. Dump trace buffer data to a file,

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

4. Reset back to normal mode

#echo 0 > read_prevboot

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 --- .../coresight/coresight-etm4x-core.c | 1 + .../hwtracing/coresight/coresight-tmc-core.c | 81 +++++++++- .../hwtracing/coresight/coresight-tmc-etf.c | 62 ++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 145 +++++++++++++++++- drivers/hwtracing/coresight/coresight-tmc.h | 6 + include/linux/coresight.h | 13 ++ 6 files changed, 306 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 77b0271ce6eb..513baf681280 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1010,6 +1010,7 @@ static void etm4_disable(struct coresight_device *csdev,

switch (mode) { case CS_MODE_DISABLED: + case CS_MODE_READ_PREVBOOT: break; case CS_MODE_SYSFS: etm4_disable_sysfs(csdev); diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 6658ce76777b..65c15c9f821b 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -103,6 +103,45 @@ u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata) return mask; }

+int tmc_read_prepare_prevboot(struct tmc_drvdata *drvdata) +{ + int ret = 0; + struct tmc_register_snapshot *reg_ptr; + struct coresight_device *csdev = drvdata->csdev; + + if (!drvdata->metadata.vaddr) { + ret = -ENOMEM; + goto out; + } + + reg_ptr = drvdata->metadata.vaddr; + if (!reg_ptr->valid) { + dev_err(&drvdata->csdev->dev, + "Invalid metadata captured from previous boot\n"); + ret = -EINVAL; + goto out; + } + + /* Sink specific prevboot mode preparation */ + ret = prevboot_ops(csdev)->prepare(csdev); + if (ret) + goto out; + + if (reg_ptr->sts & 0x1) + coresight_insert_barrier_packet(drvdata->buf); + +out: + return ret; +} + +int tmc_read_unprepare_prevboot(struct tmc_drvdata *drvdata) +{ + struct coresight_device *csdev = drvdata->csdev; + + /* Sink specific prevboot mode preparation */ + return prevboot_ops(csdev)->unprepare(csdev); +} + static int tmc_read_prepare(struct tmc_drvdata *drvdata) { int ret = 0; @@ -153,6 +192,10 @@ static int tmc_open(struct inode *inode, struct file *file) struct tmc_drvdata *drvdata = container_of(file->private_data, struct tmc_drvdata, miscdev);

+ /* Advertise if we are opening with a special mode */ + if (drvdata->mode == CS_MODE_READ_PREVBOOT) + dev_dbg(&drvdata->csdev->dev, "TMC read mode for previous boot\n"); + ret = tmc_read_prepare(drvdata); if (ret) return ret; @@ -331,9 +374,44 @@ static ssize_t buffer_size_store(struct device *dev,

static DEVICE_ATTR_RW(buffer_size);

+static ssize_t read_prevboot_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); + + return sprintf(buf, "%#x\n", (drvdata->mode == CS_MODE_READ_PREVBOOT)); +} + +static ssize_t read_prevboot_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + int ret; + unsigned long val, flags; + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); + + ret = kstrtoul(buf, 0, &val); + if (ret) + return ret; + + spin_lock_irqsave(&drvdata->spinlock, flags); + + if (val && (drvdata->mode == CS_MODE_DISABLED)) + drvdata->mode = CS_MODE_READ_PREVBOOT; + else if (!val && (drvdata->mode == CS_MODE_READ_PREVBOOT)) + drvdata->mode = CS_MODE_DISABLED; + + spin_unlock_irqrestore(&drvdata->spinlock, flags); + + return size; +} + +static DEVICE_ATTR_RW(read_prevboot); + static struct attribute *coresight_tmc_attrs[] = { &dev_attr_trigger_cntr.attr, &dev_attr_buffer_size.attr, + &dev_attr_read_prevboot.attr, NULL, };

@@ -635,7 +713,8 @@ static void tmc_shutdown(struct amba_device *adev)

spin_lock_irqsave(&drvdata->spinlock, flags);

- if (drvdata->mode == CS_MODE_DISABLED) + if (drvdata->mode == CS_MODE_DISABLED || + drvdata->mode == CS_MODE_READ_PREVBOOT) goto out;

if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index 40204d5f200f..496b44aad56d 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -639,6 +639,45 @@ static int tmc_panic_sync_etf(struct coresight_device *csdev) return 0; }

+static int tmc_etb_setup_prevboot_buf(struct tmc_drvdata *drvdata) +{ + struct tmc_register_snapshot *reg_ptr; + + reg_ptr = drvdata->metadata.vaddr; + + drvdata->buf = memremap(reg_ptr->trc_paddr, reg_ptr->size, + MEMREMAP_WB); + if (IS_ERR(drvdata->buf)) + return -ENOMEM; + drvdata->len = reg_ptr->size; + return 0; +} + +static void tmc_etb_free_prevboot_buf(struct tmc_drvdata *drvdata) +{ + void *buf = drvdata->buf; + + if (!buf) + return; + memunmap(buf); + drvdata->buf = NULL; +} + +static int tmc_etb_prepare_prevboot(struct coresight_device *csdev) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + + return tmc_etb_setup_prevboot_buf(drvdata); +} + +static int tmc_etb_unprepare_prevboot(struct coresight_device *csdev) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + + tmc_etb_free_prevboot_buf(drvdata); + return 0; +} + static const struct coresight_ops_sink tmc_etf_sink_ops = { .enable = tmc_enable_etf_sink, .disable = tmc_disable_etf_sink, @@ -656,6 +695,11 @@ static const struct coresight_ops_panic tmc_etf_sync_ops = { .sync = tmc_panic_sync_etf, };

+static const struct coresight_ops_prevboot tmc_etf_prevboot_ops = { + .prepare = tmc_etb_prepare_prevboot, + .unprepare = tmc_etb_unprepare_prevboot, +}; + const struct coresight_ops tmc_etb_cs_ops = { .sink_ops = &tmc_etf_sink_ops, }; @@ -664,6 +708,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, + .prevboot_ops = &tmc_etf_prevboot_ops, };

int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) @@ -684,6 +729,14 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) goto out; }

+ if (drvdata->mode == CS_MODE_READ_PREVBOOT) { + ret = tmc_read_prepare_prevboot(drvdata); + if (ret) + goto out; + else + goto mode_valid; + } + /* Don't interfere if operated from Perf */ if (drvdata->mode == CS_MODE_PERF) { ret = -EINVAL; @@ -707,6 +760,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) __tmc_etb_disable_hw(drvdata); }

+mode_valid: drvdata->reading = true; out: spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -726,8 +780,16 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata) drvdata->config_type != TMC_CONFIG_TYPE_ETF)) return -EINVAL;

+ spin_lock_irqsave(&drvdata->spinlock, flags);

+ if (drvdata->mode == CS_MODE_READ_PREVBOOT) { + tmc_read_unprepare_prevboot(drvdata); + drvdata->reading = false; + spin_unlock_irqrestore(&drvdata->spinlock, flags); + return 0; + } + /* Re-enable the TMC if need be */ if (drvdata->mode == CS_MODE_SYSFS) { /* There is no point in reading a TMC in HW FIFO mode */ diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 27fc9fdb84cc..c31c71e02833 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1165,7 +1165,12 @@ ssize_t tmc_etr_get_sysfs_trace(struct tmc_drvdata *drvdata, { s64 offset; ssize_t actual = len; - struct etr_buf *etr_buf = drvdata->sysfs_buf; + struct etr_buf *etr_buf; + + if (drvdata->mode == CS_MODE_READ_PREVBOOT) + etr_buf = drvdata->prevboot_buf; + else + etr_buf = drvdata->sysfs_buf;

if (pos + actual > etr_buf->len) actual = etr_buf->len - pos; @@ -1864,6 +1869,121 @@ static int tmc_panic_sync_etr(struct coresight_device *csdev) return 0; }

+static int tmc_etr_setup_prevboot_buf(struct tmc_drvdata *drvdata) +{ + int rc = 0; + u64 trace_addr; + struct etr_buf *etr_buf; + struct etr_flat_buf *resrv_buf; + struct tmc_register_snapshot *reg_ptr; + + etr_buf = kzalloc(sizeof(*etr_buf), GFP_KERNEL); + if (!etr_buf) { + rc = -ENOMEM; + goto out; + } + etr_buf->size = drvdata->resrv_buf.size; + + resrv_buf = kzalloc(sizeof(*resrv_buf), GFP_KERNEL); + if (!resrv_buf) { + rc = -ENOMEM; + goto rmem_err; + } + + reg_ptr = drvdata->metadata.vaddr; + trace_addr = reg_ptr->trc_paddr; + + resrv_buf->vaddr = memremap(trace_addr, reg_ptr->size * 4, + MEMREMAP_WB); + if (IS_ERR(drvdata->buf)) { + rc = -ENOMEM; + goto map_err; + } + resrv_buf->size = etr_buf->size; + resrv_buf->dev = &drvdata->csdev->dev; + etr_buf->hwaddr = trace_addr; + etr_buf->mode = ETR_MODE_RESRV; + etr_buf->private = resrv_buf; + etr_buf->ops = etr_buf_ops[ETR_MODE_RESRV]; + + drvdata->prevboot_buf = etr_buf; + + return 0; + +map_err: + kfree(resrv_buf); + +rmem_err: + kfree(etr_buf); + +out: + return rc; +} + +static int tmc_etr_sync_prevboot_buf(struct tmc_drvdata *drvdata) +{ + u32 status; + u64 rrp, rwp, dba; + struct tmc_register_snapshot *reg_ptr; + struct etr_buf *etr_buf = drvdata->prevboot_buf; + + reg_ptr = drvdata->metadata.vaddr; + + rrp = reg_ptr->rrp; + rwp = reg_ptr->rwp; + dba = reg_ptr->dba; + status = reg_ptr->sts; + + etr_buf->full = !!(status & TMC_STS_FULL); + + /* Sync the buffer pointers */ + etr_buf->offset = rrp - dba; + if (etr_buf->full) + etr_buf->len = etr_buf->size; + else + etr_buf->len = rwp - rrp; + + /* Sanity checks for validating metadata */ + if ((etr_buf->offset > etr_buf->size) || + (etr_buf->len > etr_buf->size)) + return -EINVAL; + + return 0; +} + +static void tmc_etr_free_prevboot_buf(struct tmc_drvdata *drvdata) +{ + void *buf = drvdata->prevboot_buf; + + if (!buf) + return; + + memunmap(buf); + drvdata->prevboot_buf = NULL; +} + +static int tmc_etr_prepare_prevboot(struct coresight_device *csdev) +{ + int ret = 0; + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + + ret = tmc_etr_setup_prevboot_buf(drvdata); + if (ret) + goto out; + ret = tmc_etr_sync_prevboot_buf(drvdata); + +out: + return ret; +} + +static int tmc_etr_unprepare_prevboot(struct coresight_device *csdev) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + + tmc_etr_free_prevboot_buf(drvdata); + return 0; +} + static const struct coresight_ops_sink tmc_etr_sink_ops = { .enable = tmc_enable_etr_sink, .disable = tmc_disable_etr_sink, @@ -1876,9 +1996,15 @@ static const struct coresight_ops_panic tmc_etr_sync_ops = { .sync = tmc_panic_sync_etr, };

+static const struct coresight_ops_prevboot tmc_etr_prevboot_ops = { + .prepare = tmc_etr_prepare_prevboot, + .unprepare = tmc_etr_unprepare_prevboot, +}; + const struct coresight_ops tmc_etr_cs_ops = { .sink_ops = &tmc_etr_sink_ops, .panic_ops = &tmc_etr_sync_ops, + .prevboot_ops = &tmc_etr_prevboot_ops, };

int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) @@ -1890,12 +2016,21 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR)) return -EINVAL;

+ spin_lock_irqsave(&drvdata->spinlock, flags); if (drvdata->reading) { ret = -EBUSY; goto out; }

+ if (drvdata->mode == CS_MODE_READ_PREVBOOT) { + ret = tmc_read_prepare_prevboot(drvdata); + if (ret) + goto out; + else + goto mode_valid; + } + /* * We can safely allow reads even if the ETR is operating in PERF mode, * since the sysfs session is captured in mode specific data. @@ -1910,6 +2045,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) if (drvdata->mode == CS_MODE_SYSFS) __tmc_etr_disable_hw(drvdata);

+mode_valid: drvdata->reading = true; out: spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -1928,6 +2064,13 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)

spin_lock_irqsave(&drvdata->spinlock, flags);

+ if (drvdata->mode == CS_MODE_READ_PREVBOOT) { + tmc_read_unprepare_prevboot(drvdata); + drvdata->reading = false; + spin_unlock_irqrestore(&drvdata->spinlock, flags); + return 0; + } + /* RE-enable the TMC if need be */ if (drvdata->mode == CS_MODE_SYSFS) { /* diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 24ef7bbb0611..f8b79eaac0bd 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -213,6 +213,9 @@ struct tmc_resrv_buf { * @idr_mutex: Access serialisation for idr. * @sysfs_buf: SYSFS buffer for ETR. * @perf_buf: PERF buffer for ETR. + * @prevboot_buf: Previous boot buffer for ETR. This is a special purpose + * buffer that is used only for mapping the trace buffer from + * previous boot and not for capturing trace. * @resrv_buf: Reserved Memory for trace data buffer. Used by ETR/ETF. * @metadata: Reserved memory for metadata. Used by ETR/ETF. */ @@ -240,6 +243,7 @@ struct tmc_drvdata { struct mutex idr_mutex; struct etr_buf *sysfs_buf; struct etr_buf *perf_buf; + struct etr_buf *prevboot_buf; struct tmc_resrv_buf resrv_buf; struct tmc_resrv_buf metadata; }; @@ -291,6 +295,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_prevboot(struct tmc_drvdata *drvdata); +int tmc_read_unprepare_prevboot(struct tmc_drvdata *drvdata);

/* ETB/ETF functions */ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata); diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 4fd518738958..9f84decf1d7a 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -294,6 +294,7 @@ enum cs_mode { CS_MODE_DISABLED, CS_MODE_SYSFS, CS_MODE_PERF, + CS_MODE_READ_PREVBOOT, /* Trace data from previous boot */ };

#define source_ops(csdev) csdev->ops->source_ops @@ -302,6 +303,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 prevboot_ops(csdev) csdev->ops->prevboot_ops

/** * struct coresight_ops_sink - basic operations for a sink @@ -381,12 +383,23 @@ struct coresight_ops_panic { int (*sync)(struct coresight_device *csdev); };

+/** + * struct coresight_ops_prevboot - Generic device ops for prevboot mode + * + * @prepare : Preparation for prevboot mode + */ +struct coresight_ops_prevboot { + int (*prepare)(struct coresight_device *csdev); + int (*unprepare)(struct coresight_device *csdev); +}; + 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_prevboot *prevboot_ops; };

#if IS_ENABLED(CONFIG_CORESIGHT)

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 to be programmed as below.

$cd /config/cs-syscfg/features/gen_etrig/params $echo <panic_address> > address/value

Signed-off-by: Linu Cherian lcherian@marvell.com --- 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 995d3b2c76df..68b15c8d9462 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -5,7 +5,7 @@ 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 \ 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..037d6773fab8 --- /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) +#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 = 0x0, + }, +}; + +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) */

On 03/10/2023 17:43, James Clark wrote:

On 29/09/2023 14:37, Linu Cherian wrote:

...

• Introduce a new mode CS_MODE_READ_PREVBOOT for reading tracedata captured in previous boot.

• Add special handlers for preparing ETR/ETF for this special mode

• User can read the trace data as below

  For example, for reading trace data from tmc_etf sink

  1. cd /sys/bus/coresight/devices/tmc_etfXX/

  2. Change mode to READ_PREVBOOT

     #echo 1 > read_prevboot

  3. Dump trace buffer data to a file,

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

  4. Reset back to normal mode

     #echo 0 > read_prevboot

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

---

.../coresight/coresight-etm4x-core.c | 1 + .../hwtracing/coresight/coresight-tmc-core.c | 81 +++++++++- .../hwtracing/coresight/coresight-tmc-etf.c | 62 ++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 145 +++++++++++++++++- drivers/hwtracing/coresight/coresight-tmc.h | 6 + include/linux/coresight.h | 13 ++ 6 files changed, 306 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 77b0271ce6eb..513baf681280 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1010,6 +1010,7 @@ static void etm4_disable(struct coresight_device *csdev, switch (mode) { case CS_MODE_DISABLED:

• case CS_MODE_READ_PREVBOOT: break; case CS_MODE_SYSFS: etm4_disable_sysfs(csdev);

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 6658ce76777b..65c15c9f821b 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -103,6 +103,45 @@ u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata) return mask; } +int tmc_read_prepare_prevboot(struct tmc_drvdata *drvdata) +{

• int ret = 0;
• struct tmc_register_snapshot *reg_ptr;
• struct coresight_device *csdev = drvdata->csdev;
• if (!drvdata->metadata.vaddr) {

• ret = -ENOMEM;
  

• goto out;
  

• }
• reg_ptr = drvdata->metadata.vaddr;
• if (!reg_ptr->valid) {

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

• 	"Invalid metadata captured from previous boot\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }

I'm wondering if a more robust check is needed than the valid flag, like a checksum or something. I didn't debug it yet but I ended up with an invalid set of metadata after a panic reboot, see below. I'm not sure if it's just a logic bug or something got lost during the reboot, I didn't debug it yet. But I suppose unless you assume the panic didn't affect writing the metadata, then it could be partially written and shouldn't be trusted?

[...]

...

+static int tmc_etr_sync_prevboot_buf(struct tmc_drvdata *drvdata) +{

• u32 status;
• u64 rrp, rwp, dba;
• struct tmc_register_snapshot *reg_ptr;
• struct etr_buf *etr_buf = drvdata->prevboot_buf;
• reg_ptr = drvdata->metadata.vaddr;
• rrp = reg_ptr->rrp;
• rwp = reg_ptr->rwp;
• dba = reg_ptr->dba;
• status = reg_ptr->sts;
• etr_buf->full = !!(status & TMC_STS_FULL);
• /* Sync the buffer pointers */
• etr_buf->offset = rrp - dba;
• if (etr_buf->full)

• etr_buf->len = etr_buf->size;
  

• else

• etr_buf->len = rwp - rrp;
  

• /* Sanity checks for validating metadata */
• if ((etr_buf->offset > etr_buf->size) ||

•    (etr_buf->len > etr_buf->size))
  

• return -EINVAL;
  

The values I got here are 0x781b67182aa346f9 0x8000000 0x8000000 for offset, size and len respectively. This fails the first check. It would also be nice to have a dev_dbg here as well, it's basically the same as the valid check above which does have one.

So I debugged it and the issue is that after the panic I was doing a cold boot rather than a warm boot and the memory was being randomised.

The reason that 0x8000000 seemed to be initialised is because they are based on the reserved region size, rather than anything from the metadata. When I examined the metadata it was all randomised.

That leads me to think that the single bit for 'valid' is insufficient. There is a simple hashing function in include/linux/stringhash.h that we could use on the whole metadata struct, but that specifically says:

* These hash functions are NOT GUARANTEED STABLE between kernel * versions, architectures, or even repeated boots of the same kernel. * (E.g. they may depend on boot-time hardware detection or be * deliberately randomized.)

Although I'm not sure how true the repeated boots of the same kernel part is.

Maybe something in include/crypto/hash.h could be used instead, or make our own simple hash.

Hi James,

-----Original Message----- From: James Clark james.clark@arm.com Sent: Wednesday, October 4, 2023 7:18 PM To: Linu Cherian lcherian@marvell.com; suzuki.poulose@arm.com; mike.leach@linaro.org; leo.yan@linaro.org Cc: linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; linux- kernel@vger.kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; devicetree@vger.kernel.org; Sunil Kovvuri Goutham sgoutham@marvell.com; George Cherian gcherian@marvell.com; Anil Kumar Reddy H areddy3@marvell.com; Tanmay Jagdale tanmay@marvell.com Subject: [EXT] Re: [PATCH 5/7] coresight: tmc: Add support for reading tracedata from previous boot

External Email

---

On 03/10/2023 17:43, James Clark wrote:

...

On 29/09/2023 14:37, Linu Cherian wrote:

...

• Introduce a new mode CS_MODE_READ_PREVBOOT for reading

tracedata

...

...

captured in previous boot.

• Add special handlers for preparing ETR/ETF for this special mode

• User can read the trace data as below

  For example, for reading trace data from tmc_etf sink

  1. cd /sys/bus/coresight/devices/tmc_etfXX/

  2. Change mode to READ_PREVBOOT

     #echo 1 > read_prevboot

  3. Dump trace buffer data to a file,

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

  4. Reset back to normal mode

     #echo 0 > read_prevboot

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

---

.../coresight/coresight-etm4x-core.c | 1 + .../hwtracing/coresight/coresight-tmc-core.c | 81 +++++++++- .../hwtracing/coresight/coresight-tmc-etf.c | 62 ++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 145 +++++++++++++++++- drivers/hwtracing/coresight/coresight-tmc.h | 6 + include/linux/coresight.h | 13 ++ 6 files changed, 306 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 77b0271ce6eb..513baf681280 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1010,6 +1010,7 @@ static void etm4_disable(struct coresight_device *csdev,

switch (mode) { case CS_MODE_DISABLED:

• case CS_MODE_READ_PREVBOOT: break; case CS_MODE_SYSFS: etm4_disable_sysfs(csdev);

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 6658ce76777b..65c15c9f821b 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -103,6 +103,45 @@ u32 tmc_get_memwidth_mask(struct

tmc_drvdata *drvdata)

...

...

return mask; }

+int tmc_read_prepare_prevboot(struct tmc_drvdata *drvdata) {

• int ret = 0;
• struct tmc_register_snapshot *reg_ptr;
• struct coresight_device *csdev = drvdata->csdev;
• if (!drvdata->metadata.vaddr) {

• ret = -ENOMEM;
  

• goto out;
  

• }
• reg_ptr = drvdata->metadata.vaddr;
• if (!reg_ptr->valid) {

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

• 	"Invalid metadata captured from previous boot\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }

I'm wondering if a more robust check is needed than the valid flag, like a checksum or something. I didn't debug it yet but I ended up with an invalid set of metadata after a panic reboot, see below. I'm not sure if it's just a logic bug or something got lost during the reboot, I didn't debug it yet. But I suppose unless you assume the panic didn't affect writing the metadata, then it could be partially written and shouldn't be trusted?

[...]

...

+static int tmc_etr_sync_prevboot_buf(struct tmc_drvdata *drvdata) {

• u32 status;
• u64 rrp, rwp, dba;
• struct tmc_register_snapshot *reg_ptr;
• struct etr_buf *etr_buf = drvdata->prevboot_buf;
• reg_ptr = drvdata->metadata.vaddr;
• rrp = reg_ptr->rrp;
• rwp = reg_ptr->rwp;
• dba = reg_ptr->dba;
• status = reg_ptr->sts;
• etr_buf->full = !!(status & TMC_STS_FULL);
• /* Sync the buffer pointers */
• etr_buf->offset = rrp - dba;
• if (etr_buf->full)

• etr_buf->len = etr_buf->size;
  

• else

• etr_buf->len = rwp - rrp;
  

• /* Sanity checks for validating metadata */
• if ((etr_buf->offset > etr_buf->size) ||

•    (etr_buf->len > etr_buf->size))
  

• return -EINVAL;
  

The values I got here are 0x781b67182aa346f9 0x8000000 0x8000000 for offset, size and len respectively. This fails the first check. It would also be nice to have a dev_dbg here as well, it's basically the same as the valid check above which does have one.

So I debugged it and the issue is that after the panic I was doing a cold boot rather than a warm boot and the memory was being randomised.

The reason that 0x8000000 seemed to be initialised is because they are based on the reserved region size, rather than anything from the metadata. When I examined the metadata it was all randomised.

That leads me to think that the single bit for 'valid' is insufficient. There is a simple hashing function in include/linux/stringhash.h that we could use on the whole metadata struct, but that specifically says:

• These hash functions are NOT GUARANTEED STABLE between kernel
• versions, architectures, or even repeated boots of the same kernel.
• (E.g. they may depend on boot-time hardware detection or be
• deliberately randomized.)

Although I'm not sure how true the repeated boots of the same kernel part is.

Maybe something in include/crypto/hash.h could be used instead, or make our own simple hash.

Thanks for the pointers. Will take a look at it.

Hi Mike,

-----Original Message----- From: Linu Cherian Sent: Saturday, October 14, 2023 5:07 PM To: Mike Leach mike.leach@linaro.org Cc: suzuki.poulose@arm.com; james.clark@arm.com; leo.yan@linaro.org; linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; linux- kernel@vger.kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; devicetree@vger.kernel.org; Sunil Kovvuri Goutham sgoutham@marvell.com; George Cherian gcherian@marvell.com Subject: RE: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add "memory-region" property

Hi Mike,

...

-----Original Message----- From: Mike Leach mike.leach@linaro.org Sent: Friday, October 6, 2023 4:33 PM To: Linu Cherian lcherian@marvell.com Cc: suzuki.poulose@arm.com; james.clark@arm.com; leo.yan@linaro.org; linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; linux- kernel@vger.kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; devicetree@vger.kernel.org; Sunil Kovvuri Goutham sgoutham@marvell.com; George Cherian gcherian@marvell.com Subject: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add "memory-region" property

External Email

---

Hi Linu

On Fri, 29 Sept 2023 at 14:38, Linu Cherian lcherian@marvell.com wrote:

...

memory-region 0: Reserved trace buffer memory

TMC ETR: When available, use this reserved memory region for trace data capture. Same region is used for trace data retention after a panic or watchdog reset.

TMC ETF: When available, use this reserved memory region for trace data retention synced from internal SRAM after a panic or watchdog reset.

memory-region 1: Reserved meta data memory

TMC ETR, ETF: When available, use this memory for register snapshot retention synced from hardware registers after a panic or watchdog reset.

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

.../bindings/arm/arm,coresight-tmc.yaml | 19

+++++++++++++++++++

...

...

1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml index cb8dceaca70e..45ca4d02d73e 100644 --- a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml +++ b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml @@ -101,6 +101,22 @@ properties: and ETF configurations. $ref: /schemas/graph.yaml#/properties/port

• memory-region:
• items:

•  - description: Reserved trace buffer memory for ETR and ETF sinks.
  

•      For ETR, this reserved memory region is used for trace data
  

capture.

...

...

•      Same region is used for trace data retention as well after a panic
  

•      or watchdog reset.
  

•      For ETF, this reserved memory region is used for retention of trace
  

•      data synced from internal SRAM after a panic or watchdog reset.
  

Is there a valid use case for ETR where we use these areas when there is not a panic/reset situation?

Currently its meant to be used only for the panic/reset situations and gets enabled only with a special buffer mode.

...

Either way - the description should perhaps mention that these areas are only used if specifically selected by the driver - the default memory usage models for ETR / perf are otherwise unaltered.

Ack.

...

...

•  - description: Reserved meta data memory. Used for ETR and ETF
  

sinks.

...

...

• memory-region-names:
• items:

•  - const: trace-mem
  

•  - const: metadata-mem
  

Is there a constraint required here? If we are using the memory area for trace in a panic situation, then we must have the meta data memory area defined? Perhaps a set of names such as "etr-trace-mem", "panic-trace-mem" , "panic-metadata-mem", were the first is for general ETR trace in non-panic situation and then constrain the "panic-" areas to appear

together.

...

The "etr-trace-mem", "panic-trace-mem" could easily point to the same area.

As noted above, we do not have other generic use case for these reserved regions now. Hence two regions/names, panic-trace-mem and panic-metadata-mem with constraints kept as minItems: 2 and maxItems: 2 would suffice ?

Looking this further, realized that by default minItems and maxItems are 2 in this case. IIUC, nothing additional required to enforce this constraint.

Also tested this, with the below dts change with just one memory-region,

diff --git a/arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi b/arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi index 09d2b692e9e1..03ff6b2f7269 100644 --- a/arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi +++ b/arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi @@ -30,6 +30,9 @@ etf_sys1: etf@20140000 { /* etf1 */ clocks = <&soc_smc50mhz>; clock-names = "apb_pclk"; power-domains = <&scpi_devpd 0>; + memory-region = <&etf1_trace>; + memory-region-names = "tracedata"; + in-ports {

Validation of DTS files using below command identified this. # make CHECK_DTBS=y ARCH=arm64 arm/juno-r2-scmi.dtb

from schema $id: http://devicetree.org/schemas/thermal/thermal-zones.yaml# /home/lcherian/scode/mainline/linux/arch/arm64/boot/dts/arm/juno-r2-scmi.dtb: etf@20140000: memory-region: [[72]] is too short from schema $id: http://devicetree.org/schemas/arm/arm,coresight-tmc.yaml# /home/lcherian/scode/mainline/linux/arch/arm64/boot/dts/arm/juno-r2-scmi.dtb: etf@20140000: memory-region-names: ['tracedata'] is too short from schema $id: http://devicetree.org/schemas/arm/arm,coresight-tmc.yaml# /home/lcherian/scode/mainline/linux/arch/arm64/boot/dts/arm/juno-r2-scmi.dtb: etf@20140000: Unevaluated properties are not allowed ('memory-region', 'memory-region-names' were unexp ected)

On 09/11/2023 01:08, Linu Cherian wrote:

Hi James,

...

-----Original Message----- From: Linu Cherian lcherian@marvell.com Sent: Tuesday, October 10, 2023 6:53 PM To: James Clark james.clark@arm.com; suzuki.poulose@arm.com; mike.leach@linaro.org; leo.yan@linaro.org Cc: linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; linux- kernel@vger.kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; devicetree@vger.kernel.org; Sunil Kovvuri Goutham sgoutham@marvell.com; George Cherian gcherian@marvell.com; Anil Kumar Reddy H areddy3@marvell.com Subject: RE: [EXT] Re: [PATCH 5/7] coresight: tmc: Add support for reading tracedata from previous boot

Hi James,

...

-----Original Message----- From: James Clark james.clark@arm.com Sent: Wednesday, October 4, 2023 7:18 PM To: Linu Cherian lcherian@marvell.com; suzuki.poulose@arm.com; mike.leach@linaro.org; leo.yan@linaro.org Cc: linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; linux- kernel@vger.kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; devicetree@vger.kernel.org; Sunil Kovvuri Goutham sgoutham@marvell.com; George Cherian gcherian@marvell.com; Anil Kumar Reddy H areddy3@marvell.com; Tanmay Jagdale tanmay@marvell.com Subject: [EXT] Re: [PATCH 5/7] coresight: tmc: Add support for reading tracedata from previous boot

External Email

---

On 03/10/2023 17:43, James Clark wrote:

...

On 29/09/2023 14:37, Linu Cherian wrote:

...

• Introduce a new mode CS_MODE_READ_PREVBOOT for reading

tracedata

...

...

captured in previous boot.

• Add special handlers for preparing ETR/ETF for this special mode

• User can read the trace data as below

  For example, for reading trace data from tmc_etf sink

  1. cd /sys/bus/coresight/devices/tmc_etfXX/

  2. Change mode to READ_PREVBOOT

     #echo 1 > read_prevboot

  3. Dump trace buffer data to a file,

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

  4. Reset back to normal mode

     #echo 0 > read_prevboot

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

---

.../coresight/coresight-etm4x-core.c | 1 + .../hwtracing/coresight/coresight-tmc-core.c | 81 +++++++++- .../hwtracing/coresight/coresight-tmc-etf.c | 62 ++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 145

+++++++++++++++++-

...

...

...

drivers/hwtracing/coresight/coresight-tmc.h | 6 + include/linux/coresight.h | 13 ++ 6 files changed, 306 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 77b0271ce6eb..513baf681280 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1010,6 +1010,7 @@ static void etm4_disable(struct coresight_device *csdev,

switch (mode) { case CS_MODE_DISABLED:

• case CS_MODE_READ_PREVBOOT: break; case CS_MODE_SYSFS: etm4_disable_sysfs(csdev);

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 6658ce76777b..65c15c9f821b 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -103,6 +103,45 @@ u32 tmc_get_memwidth_mask(struct

tmc_drvdata *drvdata)

...

...

return mask; }

+int tmc_read_prepare_prevboot(struct tmc_drvdata *drvdata) {

• int ret = 0;
• struct tmc_register_snapshot *reg_ptr;
• struct coresight_device *csdev = drvdata->csdev;
• if (!drvdata->metadata.vaddr) {

• ret = -ENOMEM;
  

• goto out;
  

• }
• reg_ptr = drvdata->metadata.vaddr;
• if (!reg_ptr->valid) {

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

• 	"Invalid metadata captured from previous boot\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }

I'm wondering if a more robust check is needed than the valid flag, like a checksum or something. I didn't debug it yet but I ended up with an invalid set of metadata after a panic reboot, see below. I'm not sure if it's just a logic bug or something got lost during the reboot, I didn't debug it yet. But I suppose unless you assume the panic didn't affect writing the metadata, then it could be partially written and shouldn't be trusted?

[...]

...

+static int tmc_etr_sync_prevboot_buf(struct tmc_drvdata *drvdata) {

• u32 status;
• u64 rrp, rwp, dba;
• struct tmc_register_snapshot *reg_ptr;
• struct etr_buf *etr_buf = drvdata->prevboot_buf;
• reg_ptr = drvdata->metadata.vaddr;
• rrp = reg_ptr->rrp;
• rwp = reg_ptr->rwp;
• dba = reg_ptr->dba;
• status = reg_ptr->sts;
• etr_buf->full = !!(status & TMC_STS_FULL);
• /* Sync the buffer pointers */
• etr_buf->offset = rrp - dba;
• if (etr_buf->full)

• etr_buf->len = etr_buf->size;
  

• else

• etr_buf->len = rwp - rrp;
  

• /* Sanity checks for validating metadata */
• if ((etr_buf->offset > etr_buf->size) ||

•    (etr_buf->len > etr_buf->size))
  

• return -EINVAL;
  

The values I got here are 0x781b67182aa346f9 0x8000000 0x8000000 for offset, size and len respectively. This fails the first check. It would also be nice to have a dev_dbg here as well, it's basically the same as the valid check above which does have one.

So I debugged it and the issue is that after the panic I was doing a cold boot rather than a warm boot and the memory was being randomised.

The reason that 0x8000000 seemed to be initialised is because they are based on the reserved region size, rather than anything from the metadata. When I examined the metadata it was all randomised.

That leads me to think that the single bit for 'valid' is insufficient. There is a simple hashing function in include/linux/stringhash.h that we could use on the whole metadata struct, but that specifically says:

• These hash functions are NOT GUARANTEED STABLE between kernel
• versions, architectures, or even repeated boots of the same kernel.
• (E.g. they may depend on boot-time hardware detection or be
• deliberately randomized.)

Although I'm not sure how true the repeated boots of the same kernel part is.

Maybe something in include/crypto/hash.h could be used instead, or make our own simple hash.

Thanks for the pointers. Will take a look at it.

Since the purpose is to identify any data corruption, crc32(using crc32_le API) looks okay to me. Any thoughts on this ? May be we could add crc32 checks for trace data as well ?

Thanks.

Seems fine to me. Maybe doing it on the trace data is overkill if you already know the metadata is fine, but at the same time it might not do any harm either. It might catch some edge case where the firmware or device is doing something strange.

...

---

CoreSight mailing list -- coresight@lists.linaro.org To unsubscribe send an email to coresight-leave@lists.linaro.org