CoreSight

coresight@lists.linaro.org

4 participants
2387 discussions

Re: [PATCH v8 2/5] Coresight: Add trace_id function to retrieving the trace ID

by James Clark

On 26/12/2024 1:10 am, Jie Gan wrote: > Add 'trace_id' function pointer in ops. It's responsible for > retrieving the device's trace ID. > > Add 'struct cs_sink_data' to store the data that is needed by > coresight_enable_path/coresight_disable_path. The structure > will be transmitted to the helper and sink device to enable > related funcationalities. > The new cs_sink_data struct is quite specific to this change. Can we start passing the path around to enable/disable functions, that will allow devices to gather anything they want in the future. Because we already have coresight_get_sink(path), coresight_get_source(path) etc. And see below, but for this case we can also change the path struct to contain the trace ID. Then all the new functions, allocations and searches for the trace ID are unecessary. The CTCU will have access to the path, and by the time its enable function is called the trace ID is already assigned. It's also easier to understand at which point a trace ID is allocated, rather than adding the trace_id() callbacks from everywhere which could potentially either read or allocate. I suppose that's "safer" because maybe it's not allocated, but I can't see what case it would happen in reverse. > Signed-off-by: Jie Gan <quic_jiegan(a)quicinc.com> > --- > drivers/hwtracing/coresight/coresight-core.c | 59 +++++++++++++++---- > drivers/hwtracing/coresight/coresight-etb10.c | 3 +- > .../hwtracing/coresight/coresight-etm-perf.c | 37 ++++++++++-- > .../coresight/coresight-etm3x-core.c | 30 ++++++++++ > .../coresight/coresight-etm4x-core.c | 29 +++++++++ > drivers/hwtracing/coresight/coresight-priv.h | 13 +++- > drivers/hwtracing/coresight/coresight-stm.c | 22 +++++++ > drivers/hwtracing/coresight/coresight-sysfs.c | 24 +++++++- > .../hwtracing/coresight/coresight-tmc-etf.c | 3 +- > .../hwtracing/coresight/coresight-tmc-etr.c | 6 +- > drivers/hwtracing/coresight/coresight-tpda.c | 20 +++++++ > drivers/hwtracing/coresight/coresight-trbe.c | 4 +- > drivers/hwtracing/coresight/ultrasoc-smb.c | 3 +- > include/linux/coresight.h | 6 ++ > 14 files changed, 234 insertions(+), 25 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 0a9380350fb5..2e560b425fd4 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -23,6 +23,7 @@ > #include "coresight-etm-perf.h" > #include "coresight-priv.h" > #include "coresight-syscfg.h" > +#include "coresight-trace-id.h" > > /* > * Mutex used to lock all sysfs enable and disable actions and loading and > @@ -331,12 +332,12 @@ static int coresight_enable_helper(struct coresight_device *csdev, > return helper_ops(csdev)->enable(csdev, mode, data); > } > > -static void coresight_disable_helper(struct coresight_device *csdev) > +static void coresight_disable_helper(struct coresight_device *csdev, void *data) > { > - helper_ops(csdev)->disable(csdev, NULL); > + helper_ops(csdev)->disable(csdev, data); > } > > -static void coresight_disable_helpers(struct coresight_device *csdev) > +static void coresight_disable_helpers(struct coresight_device *csdev, void *data) > { > int i; > struct coresight_device *helper; > @@ -344,7 +345,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev) > for (i = 0; i < csdev->pdata->nr_outconns; ++i) { > helper = csdev->pdata->out_conns[i]->dest_dev; > if (helper && coresight_is_helper(helper)) > - coresight_disable_helper(helper); > + coresight_disable_helper(helper, data); > } > } > > @@ -361,7 +362,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev) > void coresight_disable_source(struct coresight_device *csdev, void *data) > { > source_ops(csdev)->disable(csdev, data); > - coresight_disable_helpers(csdev); > + coresight_disable_helpers(csdev, NULL); > } > EXPORT_SYMBOL_GPL(coresight_disable_source); > > @@ -371,7 +372,8 @@ EXPORT_SYMBOL_GPL(coresight_disable_source); > * disabled. > */ > static void coresight_disable_path_from(struct list_head *path, > - struct coresight_node *nd) > + struct coresight_node *nd, > + void *sink_data) > { > u32 type; > struct coresight_device *csdev, *parent, *child; > @@ -417,13 +419,13 @@ static void coresight_disable_path_from(struct list_head *path, > } > > /* Disable all helpers adjacent along the path last */ > - coresight_disable_helpers(csdev); > + coresight_disable_helpers(csdev, sink_data); > } > } > > -void coresight_disable_path(struct list_head *path) > +void coresight_disable_path(struct list_head *path, void *sink_data) > { > - coresight_disable_path_from(path, NULL); > + coresight_disable_path_from(path, NULL, sink_data); > } > EXPORT_SYMBOL_GPL(coresight_disable_path); > > @@ -505,10 +507,47 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode, > out: > return ret; > err: > - coresight_disable_path_from(path, nd); > + coresight_disable_path_from(path, nd, sink_data); > goto out; > } > > +int coresight_read_traceid(struct list_head *path, enum cs_mode mode, > + struct coresight_trace_id_map *id_map) > +{ > + int trace_id, type; > + struct coresight_device *csdev; > + struct coresight_node *nd; > + > + list_for_each_entry(nd, path, link) { What do you think about also changing the path to this: struct coresight_path { struct list_head *path, u8 trace_id }; That would avoid having to traverse the path on every enable and would remove this function. You could also cache the trace ID in the CTCU for a similar benefit, but it wouldn't remove the need to call this at least once. The expensive part should be the create path part, after that enable and disable should be cheap because they happen on schedule for Perf mode. We should be avoiding allocations and searches. > + csdev = nd->csdev; > + type = csdev->type; > + > + switch (type) { > + case CORESIGHT_DEV_TYPE_SOURCE: > + if (source_ops(csdev)->trace_id != NULL) { > + trace_id = source_ops(csdev)->trace_id(csdev, > + mode, > + id_map); > + if (IS_VALID_CS_TRACE_ID(trace_id)) > + goto out; > + } > + break; > + case CORESIGHT_DEV_TYPE_LINK: > + if (link_ops(csdev)->trace_id != NULL) { > + trace_id = link_ops(csdev)->trace_id(csdev); > + if (IS_VALID_CS_TRACE_ID(trace_id)) > + goto out; > + } > + break; > + default: > + break; > + } > + } > + return -EINVAL; > +out: > + return trace_id; > +} > + > struct coresight_device *coresight_get_sink(struct list_head *path) > { > struct coresight_device *csdev; > diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c > index aea9ac9c4bd0..904b5531c256 100644 > --- a/drivers/hwtracing/coresight/coresight-etb10.c > +++ b/drivers/hwtracing/coresight/coresight-etb10.c > @@ -173,7 +173,8 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data) > pid_t pid; > unsigned long flags; > struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > - struct perf_output_handle *handle = data; > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + struct perf_output_handle *handle = sink_data->handle; > struct cs_buffers *buf = etm_perf_sink_config(handle); > > spin_lock_irqsave(&drvdata->spinlock, flags); > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > index ad6a8f4b70b6..e676edd42ddc 100644 > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > @@ -459,6 +459,7 @@ static void etm_event_start(struct perf_event *event, int flags) > struct perf_output_handle *handle = &ctxt->handle; > struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu); > struct list_head *path; > + struct cs_sink_data *sink_data = NULL; > u64 hw_id; > u8 trace_id; > > @@ -498,9 +499,20 @@ static void etm_event_start(struct perf_event *event, int flags) > if (WARN_ON_ONCE(!sink)) > goto fail_end_stop; > > + sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL); kzalloc can't be called from here. Check dmesg for the warning. That's another reason to do this change on the path. Because the path is allocated on etm_setup_aux() where allocations are allowed. > + if (!sink_data) > + goto fail_end_stop; > + > + sink_data->sink = sink; > + sink_data->traceid = coresight_read_traceid(path, CS_MODE_PERF, > + &sink->perf_sink_id_map); > + sink_data->handle = handle; > + > /* Nothing will happen without a path */ > - if (coresight_enable_path(path, CS_MODE_PERF, handle)) > + if (coresight_enable_path(path, CS_MODE_PERF, sink_data)) { > + kfree(sink_data); > goto fail_end_stop; > + } > > /* Finally enable the tracer */ > if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF, > @@ -526,6 +538,7 @@ static void etm_event_start(struct perf_event *event, int flags) > perf_report_aux_output_id(event, hw_id); > } > > + kfree(sink_data); > out: > /* Tell the perf core the event is alive */ > event->hw.state = 0; > @@ -534,7 +547,8 @@ static void etm_event_start(struct perf_event *event, int flags) > return; > > fail_disable_path: > - coresight_disable_path(path); > + coresight_disable_path(path, sink_data); > + kfree(sink_data); > fail_end_stop: > /* > * Check if the handle is still associated with the event, > @@ -559,6 +573,7 @@ static void etm_event_stop(struct perf_event *event, int mode) > struct perf_output_handle *handle = &ctxt->handle; > struct etm_event_data *event_data; > struct list_head *path; > + struct cs_sink_data *sink_data = NULL; > > /* > * If we still have access to the event_data via handle, > @@ -603,6 +618,10 @@ static void etm_event_stop(struct perf_event *event, int mode) > if (!sink) > return; > > + sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL); Same problem as above. > + if (!sink_data) > + return; > + > /* stop tracer */ > coresight_disable_source(csdev, event); > > @@ -616,12 +635,16 @@ static void etm_event_stop(struct perf_event *event, int mode) > * have to do anything here. > */ > if (handle->event && (mode & PERF_EF_UPDATE)) { > - if (WARN_ON_ONCE(handle->event != event)) > + if (WARN_ON_ONCE(handle->event != event)) { > + kfree(sink_data); > return; > + } > > /* update trace information */ > - if (!sink_ops(sink)->update_buffer) > + if (!sink_ops(sink)->update_buffer) { > + kfree(sink_data); > return; > + } > > size = sink_ops(sink)->update_buffer(sink, handle, > event_data->snk_config); > @@ -642,8 +665,12 @@ static void etm_event_stop(struct perf_event *event, int mode) > WARN_ON(size); > } > > + sink_data->sink = sink; > + sink_data->traceid = coresight_read_traceid(path, CS_MODE_PERF, > + &sink->perf_sink_id_map); > /* Disabling the path make its elements available to other sessions */ > - coresight_disable_path(path); > + coresight_disable_path(path, sink_data); > + kfree(sink_data); > } > > static int etm_event_add(struct perf_event *event, int mode) > diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c > index c103f4c70f5d..0f095fab7e0a 100644 > --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c > @@ -697,10 +697,40 @@ static void etm_disable(struct coresight_device *csdev, > coresight_set_mode(csdev, CS_MODE_DISABLED); > } > > +static int etm_trace_id(struct coresight_device *csdev, > + enum cs_mode mode, > + struct coresight_trace_id_map *id_map) > +{ > + int trace_id; > + struct etm_drvdata *drvdata; > + > + if (csdev == NULL) > + return -EINVAL; > + > + drvdata = dev_get_drvdata(csdev->dev.parent); > + switch (mode) { > + case CS_MODE_SYSFS: > + trace_id = etm_read_alloc_trace_id(drvdata); > + break; > + case CS_MODE_PERF: > + trace_id = coresight_trace_id_read_cpu_id_map(drvdata->cpu, id_map); > + if (IS_VALID_CS_TRACE_ID(trace_id)) > + drvdata->traceid = (u8)trace_id; > + break; > + default: > + trace_id = -EINVAL; > + break; > + } > + > + return trace_id; > +} > + > + > static const struct coresight_ops_source etm_source_ops = { > .cpu_id = etm_cpu_id, > .enable = etm_enable, > .disable = etm_disable, > + .trace_id = etm_trace_id, > }; > > static const struct coresight_ops etm_cs_ops = { > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index dd8c74f893db..bac9ea371688 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -1025,10 +1025,39 @@ static void etm4_disable(struct coresight_device *csdev, > coresight_set_mode(csdev, CS_MODE_DISABLED); > } > > +static int etm4_trace_id(struct coresight_device *csdev, > + enum cs_mode mode, > + struct coresight_trace_id_map *id_map) > +{ > + int trace_id; > + struct etmv4_drvdata *drvdata; > + > + if (csdev == NULL) > + return -EINVAL; > + > + drvdata = dev_get_drvdata(csdev->dev.parent); > + switch (mode) { > + case CS_MODE_SYSFS: > + trace_id = etm4_read_alloc_trace_id(drvdata); > + break; > + case CS_MODE_PERF: > + trace_id = coresight_trace_id_read_cpu_id_map(drvdata->cpu, id_map); > + if (IS_VALID_CS_TRACE_ID(trace_id)) > + drvdata->trcid = (u8)trace_id; > + break; > + default: > + trace_id = -EINVAL; > + break; > + } > + > + return trace_id; > +} > + > static const struct coresight_ops_source etm4_source_ops = { > .cpu_id = etm4_cpu_id, > .enable = etm4_enable, > .disable = etm4_disable, > + .trace_id = etm4_trace_id, > }; > > static const struct coresight_ops etm4_cs_ops = { > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h > index 05f891ca6b5c..6a95a3ba9339 100644 > --- a/drivers/hwtracing/coresight/coresight-priv.h > +++ b/drivers/hwtracing/coresight/coresight-priv.h > @@ -105,6 +105,15 @@ struct cs_buffers { > void **data_pages; > }; > > +/** > + * struct cs_sink_data - data used by coresight_enable_path/coresight_disable_path > + */ > +struct cs_sink_data { > + struct perf_output_handle *handle; > + struct coresight_device *sink; > + u8 traceid; > +}; > + > static inline void coresight_insert_barrier_packet(void *buf) > { > if (buf) > @@ -129,9 +138,11 @@ static inline void CS_UNLOCK(void __iomem *addr) > } while (0); > } > > -void coresight_disable_path(struct list_head *path); > +void coresight_disable_path(struct list_head *path, void *sink_data); > int coresight_enable_path(struct list_head *path, enum cs_mode mode, > void *sink_data); > +int coresight_read_traceid(struct list_head *path, enum cs_mode mode, > + struct coresight_trace_id_map *id_map); > struct coresight_device *coresight_get_sink(struct list_head *path); > struct coresight_device *coresight_get_sink_by_id(u32 id); > struct coresight_device * > diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c > index b581a30a1cd9..5709eda77d9b 100644 > --- a/drivers/hwtracing/coresight/coresight-stm.c > +++ b/drivers/hwtracing/coresight/coresight-stm.c > @@ -281,9 +281,31 @@ static void stm_disable(struct coresight_device *csdev, > } > } > > +static int stm_trace_id(struct coresight_device *csdev, > + enum cs_mode mode, > + struct coresight_trace_id_map *id_map) > +{ > + int trace_id; > + struct stm_drvdata *drvdata; > + > + if (csdev == NULL) > + return -EINVAL; > + > + drvdata = dev_get_drvdata(csdev->dev.parent); > + trace_id = drvdata->traceid; > + if (!IS_VALID_CS_TRACE_ID(trace_id)) { Trace ID is static and always set for STM so this isn't necessary, just return drvdata->traceid which would already be valid otherwise the device wouldn't be registered. > + trace_id = coresight_trace_id_get_system_id(); > + if (IS_VALID_CS_TRACE_ID(trace_id)) > + drvdata->traceid = (u8)trace_id; > + } > + > + return trace_id; > +} > + > static const struct coresight_ops_source stm_source_ops = { > .enable = stm_enable, > .disable = stm_disable, > + .trace_id = stm_trace_id, > }; > > static const struct coresight_ops stm_cs_ops = { > diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c > index a01c9e54e2ed..8816bccba374 100644 > --- a/drivers/hwtracing/coresight/coresight-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-sysfs.c > @@ -168,6 +168,7 @@ int coresight_enable_sysfs(struct coresight_device *csdev) > int cpu, ret = 0; > struct coresight_device *sink; > struct list_head *path; > + struct cs_sink_data *sink_data; > enum coresight_dev_subtype_source subtype; > u32 hash; > > @@ -209,7 +210,14 @@ int coresight_enable_sysfs(struct coresight_device *csdev) > goto out; > } > > - ret = coresight_enable_path(path, CS_MODE_SYSFS, NULL); > + sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL); > + if (!sink_data) { > + ret = -ENOMEM; > + goto out; > + } > + sink_data->traceid = coresight_read_traceid(path, CS_MODE_SYSFS, NULL); > + sink_data->sink = sink; > + ret = coresight_enable_path(path, CS_MODE_SYSFS, sink_data); > if (ret) > goto err_path; > > @@ -246,15 +254,17 @@ int coresight_enable_sysfs(struct coresight_device *csdev) > break; > } > > + kfree(sink_data); > out: > mutex_unlock(&coresight_mutex); > return ret; > > err_source: > - coresight_disable_path(path); > + coresight_disable_path(path, sink_data); > > err_path: > coresight_release_path(path); > + kfree(sink_data); > goto out; > } > EXPORT_SYMBOL_GPL(coresight_enable_sysfs); > @@ -263,6 +273,7 @@ void coresight_disable_sysfs(struct coresight_device *csdev) > { > int cpu, ret; > struct list_head *path = NULL; > + struct cs_sink_data *sink_data = NULL; > u32 hash; > > mutex_lock(&coresight_mutex); > @@ -274,6 +285,10 @@ void coresight_disable_sysfs(struct coresight_device *csdev) > if (!coresight_disable_source_sysfs(csdev, NULL)) > goto out; > > + sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL); > + if (!sink_data) > + goto out; > + > switch (csdev->subtype.source_subtype) { > case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: > cpu = source_ops(csdev)->cpu_id(csdev); > @@ -297,8 +312,11 @@ void coresight_disable_sysfs(struct coresight_device *csdev) > break; > } > > - coresight_disable_path(path); > + sink_data->sink = coresight_find_activated_sysfs_sink(csdev); > + sink_data->traceid = coresight_read_traceid(path, CS_MODE_SYSFS, NULL); > + coresight_disable_path(path, sink_data); > coresight_release_path(path); > + kfree(sink_data); > > out: > mutex_unlock(&coresight_mutex); > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c > index d4f641cd9de6..7dc536eba3e2 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c > @@ -250,7 +250,8 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data) > pid_t pid; > unsigned long flags; > struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > - struct perf_output_handle *handle = data; > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + struct perf_output_handle *handle = sink_data->handle; > struct cs_buffers *buf = etm_perf_sink_config(handle); > > spin_lock_irqsave(&drvdata->spinlock, flags); > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index a48bb85d0e7f..90a4058c9959 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -1254,7 +1254,8 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) > struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev, > enum cs_mode mode, void *data) > { > - struct perf_output_handle *handle = data; > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + struct perf_output_handle *handle = sink_data->handle; > struct etr_perf_buffer *etr_perf; > > switch (mode) { > @@ -1648,7 +1649,8 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) > pid_t pid; > unsigned long flags; > struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > - struct perf_output_handle *handle = data; > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + struct perf_output_handle *handle = sink_data->handle; > struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle); > > spin_lock_irqsave(&drvdata->spinlock, flags); > diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c > index 189a4abc2561..05622333f2d2 100644 > --- a/drivers/hwtracing/coresight/coresight-tpda.c > +++ b/drivers/hwtracing/coresight/coresight-tpda.c > @@ -241,9 +241,29 @@ static void tpda_disable(struct coresight_device *csdev, > dev_dbg(drvdata->dev, "TPDA inport %d disabled\n", in->dest_port); > } > > +static int tpda_trace_id(struct coresight_device *csdev) > +{ > + int trace_id; > + struct tpda_drvdata *drvdata; > + > + if (csdev == NULL) > + return -EINVAL; > + > + drvdata = dev_get_drvdata(csdev->dev.parent); > + trace_id = drvdata->atid; > + if (!IS_VALID_CS_TRACE_ID(trace_id)) { Same comment as STM. This is always set so no ned to try to assign another one. > + trace_id = coresight_trace_id_get_system_id(); > + if (IS_VALID_CS_TRACE_ID(trace_id)) > + drvdata->atid = (u8)trace_id; > + } > + > + return trace_id; > +} > + > static const struct coresight_ops_link tpda_link_ops = { > .enable = tpda_enable, > .disable = tpda_disable, > + .trace_id = tpda_trace_id, > }; > > static const struct coresight_ops tpda_cs_ops = { > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c > index 919804b12a67..6680d3c69906 100644 > --- a/drivers/hwtracing/coresight/coresight-trbe.c > +++ b/drivers/hwtracing/coresight/coresight-trbe.c > @@ -21,6 +21,7 @@ > > #include "coresight-self-hosted-trace.h" > #include "coresight-trbe.h" > +#include "coresight-priv.h" > > #define PERF_IDX2OFF(idx, buf) ((idx) % ((buf)->nr_pages << PAGE_SHIFT)) > > @@ -1012,7 +1013,8 @@ static int arm_trbe_enable(struct coresight_device *csdev, enum cs_mode mode, > { > struct trbe_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev); > - struct perf_output_handle *handle = data; > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + struct perf_output_handle *handle = sink_data->handle; > struct trbe_buf *buf = etm_perf_sink_config(handle); > > WARN_ON(cpudata->cpu != smp_processor_id()); > diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c > index dc3c9504dd7c..99e0eb33c01f 100644 > --- a/drivers/hwtracing/coresight/ultrasoc-smb.c > +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c > @@ -216,7 +216,8 @@ static void smb_enable_sysfs(struct coresight_device *csdev) > static int smb_enable_perf(struct coresight_device *csdev, void *data) > { > struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent); > - struct perf_output_handle *handle = data; > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + struct perf_output_handle *handle = sink_data->handle; > struct cs_buffers *buf = etm_perf_sink_config(handle); > pid_t pid; > > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 157c4bd009a1..1929f0088c0f 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -368,6 +368,7 @@ struct coresight_ops_sink { > * Operations available for links. > * @enable: enables flow between iport and oport. > * @disable: disables flow between iport and oport. > + * @trace_id: Collect the traceid. > */ > struct coresight_ops_link { > int (*enable)(struct coresight_device *csdev, > @@ -376,6 +377,7 @@ struct coresight_ops_link { > void (*disable)(struct coresight_device *csdev, > struct coresight_connection *in, > struct coresight_connection *out); > + int (*trace_id)(struct coresight_device *csdev); > }; > > /** > @@ -385,6 +387,7 @@ struct coresight_ops_link { > * is associated to. > * @enable: enables tracing for a source. > * @disable: disables tracing for a source. > + * @trace_id: collect the traceid. > */ > struct coresight_ops_source { > int (*cpu_id)(struct coresight_device *csdev); > @@ -392,6 +395,9 @@ struct coresight_ops_source { > enum cs_mode mode, struct coresight_trace_id_map *id_map); > void (*disable)(struct coresight_device *csdev, > struct perf_event *event); > + int (*trace_id)(struct coresight_device *csdev, > + enum cs_mode mode, > + struct coresight_trace_id_map *id_map); > }; > > /**

5 months, 3 weeks

Re: [PATCH v8 1/5] Coresight: Add support for new APB clock name

by James Clark

On 26/12/2024 1:10 am, Jie Gan wrote: > Add support for new APB clock-name. If the function fails > to obtain the clock with the name "apb_pclk", it will > attempt to acquire the clock with the name "apb". > > Signed-off-by: Jie Gan <quic_jiegan(a)quicinc.com> > --- > include/linux/coresight.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 17276965ff1d..157c4bd009a1 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -459,8 +459,11 @@ static inline struct clk *coresight_get_enable_apb_pclk(struct device *dev) > int ret; > > pclk = clk_get(dev, "apb_pclk"); > - if (IS_ERR(pclk)) > - return NULL; > + if (IS_ERR(pclk)) { > + pclk = clk_get(dev, "apb"); > + if (IS_ERR(pclk)) > + return NULL; > + } > > ret = clk_prepare_enable(pclk); > if (ret) { Reviewed-by: James Clark <james.clark(a)linaro.org>

5 months, 3 weeks

Re: [PATCH] coresight: catu: Fix number of pages while using 64k pages

by Suzuki K Poulose

On 09/01/2025 21:53, Ilkka Koskinen wrote: > Trying to record a trace on kernel with 64k pages resulted in -ENOMEM. > This happens due to a bug in calculating the number of table pages, which > returns zero. Fix the issue by rounding up. > > $ perf record --kcore -e cs_etm/@tmc_etr55,cycacc,branch_broadcast/k --per-thread taskset --cpu-list 1 dd if=/dev/zero of=/dev/null > failed to mmap with 12 (Cannot allocate memory) > Needs a Fixes tag. Fixes : 8ed536b1e283 ("coresight: catu: Add support for scatter gather tables") > Signed-off-by: Ilkka Koskinen <ilkka(a)os.amperecomputing.com> > --- > drivers/hwtracing/coresight/coresight-catu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c > index 275cc0d9f505..3378bb77e6b4 100644 > --- a/drivers/hwtracing/coresight/coresight-catu.c > +++ b/drivers/hwtracing/coresight/coresight-catu.c > @@ -269,7 +269,7 @@ catu_init_sg_table(struct device *catu_dev, int node, > * Each table can address upto 1MB and we can have > * CATU_PAGES_PER_SYSPAGE tables in a system page. > */ > - nr_tpages = DIV_ROUND_UP(size, SZ_1M) / CATU_PAGES_PER_SYSPAGE; > + nr_tpages = DIV_ROUND_UP(size, CATU_PAGES_PER_SYSPAGE * SZ_1M); > catu_table = tmc_alloc_sg_table(catu_dev, node, nr_tpages, > size >> PAGE_SHIFT, pages); > if (IS_ERR(catu_table)) Looks good to me, I will queue this later for v6.15. Suzuki

5 months, 3 weeks

[GIT PULL] coresight: Updates for Linux v6.14

by Suzuki K Poulose

Greg, Please find the changes targeting v6.14 for the CoreSight self hosted tracing subsystem. Kindly pull Suzuki The following changes since commit fac04efc5c793dccbd07e2d59af9f90b7fc0dca4: Linux 6.13-rc2 (2024-12-08 14:03:39 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git tags/coresight-next-v6.14 for you to fetch changes up to 56e14a21cee4ea6074b956a9ff2e406de5ad6548: coresight-tpda: Optimize the function of reading element size (2024-12-13 10:25:15 +0000) ---------------------------------------------------------------- coresight: Updates for Linux v6.14 Coresight self-hosted tracing subsystem updates for v6.14 includes: - Support for static traceid allocation for devices - Support for impdef, static trace filtering in Qualcomm replicators - Miscellaneous fixes Signed-off-by: Suzuki K Poulose <suzuki.poulose(a)arm.com> ---------------------------------------------------------------- James Clark (1): coresight: Drop atomics in connection refcounts Mao Jinlong (3): dt-bindings: arm: Add arm,static-trace-id for coresight dummy source coresight: Add support to get static id for system trace sources coresight: dummy: Add static trace id support for dummy source Pei Xiao (1): coresight: Fix dsb_mode_store() unsigned val is never less than zero Songwei Chai (1): Coresight: Narrow down the matching range of tpdm Tao Zhang (4): dt-bindings: arm: qcom,coresight-static-replicator: Add property for source filtering coresight: Add a helper to check if a device is source coresight: Add support for trace filtering by source coresight-tpda: Optimize the function of reading element size .../sysfs-bus-coresight-devices-dummy-source | 15 +++ .../bindings/arm/arm,coresight-dummy-source.yaml | 6 ++ .../arm/arm,coresight-static-replicator.yaml | 19 +++- drivers/hwtracing/coresight/coresight-core.c | 113 +++++++++++++++++---- drivers/hwtracing/coresight/coresight-dummy.c | 81 +++++++++++++-- drivers/hwtracing/coresight/coresight-funnel.c | 6 +- drivers/hwtracing/coresight/coresight-platform.c | 27 +++++ drivers/hwtracing/coresight/coresight-replicator.c | 6 +- drivers/hwtracing/coresight/coresight-tpda.c | 19 +++- drivers/hwtracing/coresight/coresight-tpdm.c | 7 +- drivers/hwtracing/coresight/coresight-trace-id.c | 43 +++++--- drivers/hwtracing/coresight/coresight-trace-id.h | 9 ++ include/linux/coresight.h | 17 +++- 13 files changed, 309 insertions(+), 59 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source

5 months, 3 weeks

Re: [PATCH 1/4] coresight/etm4x: disallow altering config via sysfs while enabled

by Suzuki K Poulose

On 09/01/2025 12:01, Yeoreum Yun wrote: > Hi Suzuki, > >> Is it not better to have separate "configs" for perf and sysfs ? >> And etmX driver can populate the "running" config, based on the >> mode specific config. That way, the configs can be updated >> independently without affecting the running config or the perf one. >> > > That was i've tried but I've accepted Mike's opinion that > it's enough to check whether CS_MODE_DISABLED via coresight_get_mode() > in *_store(). > > "the .._store functions in sysfs should use coresight_get_mode() to ensure > this is set to CS_MODE_DISABLED before altering the config, > which ensures that the trace system is inactive. > We don't' really care about reading the config if trace is running." There are two issues with that : 1. Sprinkling the get_mode call in each sysfs stor function doesn't look good to me. 2. Someone preparing for a sysfs session must not be prevented from doing so just because there is a perf session running. Suzuki > Thanks. >

5 months, 3 weeks

Re: [PATCH 1/4] coresight/etm4x: disallow altering config via sysfs while enabled

by Suzuki K Poulose

Hi Levi, On 21/12/2024 16:59, Yeoreum Yun wrote: > When etm4x configuration is modified via sysfs while etm4x is being > enabled via perf, enabled etm4x could run with different configuration > from perf_event. > > To address this, disallow altering config via sysfs while csdev is enabled. > > Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com> > --- > .../coresight/coresight-etm4x-sysfs.c | 132 +++++++++++++++++- > 1 file changed, 128 insertions(+), 4 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > index 11e865b8e824..cc1f112921d7 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > @@ -174,6 +174,9 @@ static ssize_t reset_store(struct device *dev, > if (kstrtoul(buf, 16, &val)) > return -EINVAL; > > + if (coresight_get_mode(drvdata->csdev)) > + return -EBUSY; > + Is it not better to have separate "configs" for perf and sysfs ? And etmX driver can populate the "running" config, based on the mode specific config. That way, the configs can be updated independently without affecting the running config or the perf one. Suzuki

5 months, 3 weeks

Re: [PATCH] hwtracing: Switch back to struct platform_driver::remove()

by Suzuki K Poulose

Hi On 11/11/2024 14:12, Alexander Shishkin wrote: > Uwe Kleine-König <u.kleine-koenig(a)baylibre.com> writes: > >> After commit 0edb555a65d1 ("platform: Make platform_driver::remove() >> return void") .remove() is (again) the right callback to implement for >> platform drivers. >> >> Convert all platform drivers below drivers/hwtracing to use .remove(), >> with the eventual goal to drop struct platform_driver::remove_new(). As >> .remove() and .remove_new() have the same prototypes, conversion is done >> by just changing the structure member name in the driver initializer. >> >> Also adapt some whitespace to make indention consistent. >> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig(a)baylibre.com> > > Acked-by: Alexander Shishkin <alexander.shishkin(a)linux.intel.com> > >> --- >> Hello, >> >> I did a single patch for all of drivers/hwtracing. While I usually >> prefer to do one logical change per patch, this seems to be >> overengineering here as the individual changes are really trivial and >> shouldn't be much in the way for stable backports. But I'll happily >> split the patch if you prefer it split. Maybe split for coresight vs. >> intel_th? Also if you object the indentation stuff, I can rework that. > > I'm fine with it as is. > >> This is based on today's next, if conflicts arise when you apply it at >> some later time and don't want to resolve them, feel free to just drop >> the changes to the conflicting files. I'll notice and followup at a >> later time then. Or ask me for a fixed resend. (Having said that, I >> recommend b4 am -3 + git am -3 which should resolve most conflicts just >> fine.) > > Does anybody want to pick this up or should I? I'm fine either way, but > if there are any conflicts they won't be from my end of things, so it > might make sense to take it via the coresight path. I am happy to take them via coresight tree and queue them for v6.14 Suzuki > > Thanks, > -- > Alex

5 months, 3 weeks

Re: [PATCH v2] coresight: prevent deactivate active config while enable the config

by Suzuki K Poulose

On 07/01/2025 13:01, Yeoreum Yun wrote: > Hi Suzuki, > >> Hi Levi >> >> On 23/12/2024 18:53, Yeoreum Yun wrote: >>> While enable active config via cscfg_csdev_enable_active_config(), >>> active config could be deactivated via configfs' sysfs interface. >>> This could make UAF issue in below scenario: >>> >>> CPU0 CPU1 >>> (sysfs enable) load module >>> cscfg_load_config_sets() >>> activate config. // sysfs >>> (sys_active_cnt == 1) >>> ... >>> cscfg_csdev_enable_active_config() >>> lock(csdev->cscfg_csdev_lock) >>> // here load config activate by CPU1 >>> unlock(csdev->cscfg_csdev_lock) >>> >>> deactivate config // sysfs >>> (sys_activec_cnt == 0) >>> cscfg_unload_config_sets() >>> unload module >>> >>> // access to config_desc which freed >>> // while unloading module. >>> cfs_csdev_enable_config >>> >>> To address this, introduce sys_enable_cnt in cscfg_mgr to prevent >>> deactivate while there is enabled configuration. >> >> Thanks for the finding the problem and the detailed description + patch. I >> have some concerns on the fix, please find it below. >> >>> >> >> >> So we have 3 atomic counters now ! >> cscfg_mgr->sys_active_cnt // Global count >> config->active_cnt // Per config count, >> >> And another one which this one introduces. >> >> cscfg_mgr->sys_enable_cnt // ? >> >> >> And config->active_cnt is always ever 0 or 1. i.e., it is not really a >> reference counter at the moment but it indicates whether it is active or >> not. Could we not use that for tracking the references on the specific >> config ? >> >> i.e., every time we "enable_active_config" atomic_inc(config->active_cnt) >> >> and disable_active_config() always decrements the config. These could be >> wrapped in cscfg_get() cscfg_put() which would, inc/dec the refcounts >> and also drop the "module" reference when the active_cnt == 0 > > This action is done via _cscfg_activate_config() already but its > activation is done via "sysfs". > and the checking active_cnt, I think it would increase lots of complex. I don't understand. We have this today : cscfg_config_desc which is getting de-activated. This has reference to the module (owner). If someone is using this config_desc in running session (be it perf or sysfg), it must be refcounted. As such I don't see that the cscfg is refcounted anywhere. (The active_cnt indicates if this has been activated, but not used in a session). If we have a refcount on the "cscfg_config_desc" for each active session, we could prevent/delay unloading the module until the last one drops. Something like this is what I was checking ? coresight: cscfg: Hold refcount on the config for active sessions Signed-off-by: Suzuki K Poulose <suzuki.poulose(a)arm.com> --- .../hwtracing/coresight/coresight-config.h | 2 +- .../hwtracing/coresight/coresight-syscfg.c | 24 +++++++++++++------ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h index 6ba013975741..84cdde6f0e4d 100644 --- a/drivers/hwtracing/coresight/coresight-config.h +++ b/drivers/hwtracing/coresight/coresight-config.h @@ -228,7 +228,7 @@ struct cscfg_feature_csdev { * @feats_csdev:references to the device features to enable. */ struct cscfg_config_csdev { - const struct cscfg_config_desc *config_desc; + struct cscfg_config_desc *config_desc; struct coresight_device *csdev; bool enabled; struct list_head node; diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c index 11138a9762b0..53baeaaf907f 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.c +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -914,15 +914,20 @@ static int _cscfg_activate_config(unsigned long cfg_hash) return err; } +static void cscfg_config_desc_put(struct cscfg_config_desc *config_desc) +{ + if (!atomic_dec_return(&config_desc->active_cnt)) + cscfg_owner_put(config_desc->load_owner); +} + static void _cscfg_deactivate_config(unsigned long cfg_hash) { struct cscfg_config_desc *config_desc; list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) { if ((unsigned long)config_desc->event_ea->var == cfg_hash) { - atomic_dec(&config_desc->active_cnt); atomic_dec(&cscfg_mgr->sys_active_cnt); - cscfg_owner_put(config_desc->load_owner); + cscfg_config_desc_put(config_desc); dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name); break; } @@ -1047,7 +1052,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev, unsigned long cfg_hash, int preset) { struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item; - const struct cscfg_config_desc *config_desc; + struct cscfg_config_desc *config_desc; unsigned long flags; int err = 0; @@ -1062,8 +1067,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev, spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags); list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) { config_desc = config_csdev_item->config_desc; - if ((atomic_read(&config_desc->active_cnt)) && - ((unsigned long)config_desc->event_ea->var == cfg_hash)) { + if ((unsigned long)config_desc->event_ea->var == cfg_hash && + atomic_inc_not_zero(&config_desc->active_cnt)) { config_csdev_active = config_csdev_item; csdev->active_cscfg_ctxt = (void *)config_csdev_active; break; @@ -1091,12 +1096,15 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev, * Set enabled if OK, err if not. */ spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags); - if (csdev->active_cscfg_ctxt) + if (csdev->active_cscfg_ctxt == config_csdev_active) config_csdev_active->enabled = true; else err = -EBUSY; spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags); } + if (err) + cscfg_config_desc_put(config_desc); + } return err; } @@ -1136,8 +1144,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev) spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags); /* true if there was an enabled active config */ - if (config_csdev) + if (config_csdev) { cscfg_csdev_disable_config(config_csdev); + cscfg_config_desc_put(config_csdev->config_desc); + } } EXPORT_SYMBOL_GPL(cscfg_csdev_disable_active_config); -- > because, if so, it should iterate all config in each csdev. > So, I believe it is the reason why the activation and module_cnt get via "sysfs" > to prevent iterating every config in csdev when config unload. > > although, active_cnt in each config added to list in csdev be 0 or 1, > the module could be >= 1 (by sum of active_cnt which have the same > module owner). So I'm skeptical to use active_cnt like "reference cnt" > and That's why I decide to use "sys_enable_cnt" >> >>> Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com> >>> --- >>> from v1 to v2: >>> - modify commit message. >>> --- >>> .../hwtracing/coresight/coresight-etm4x-core.c | 3 +++ >>> drivers/hwtracing/coresight/coresight-syscfg.c | 18 ++++++++++++++++-- >>> drivers/hwtracing/coresight/coresight-syscfg.h | 2 ++ >>> 3 files changed, 21 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> index 86893115df17..6218ef40acbc 100644 >>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> @@ -986,6 +986,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev) >>> smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1); >>> >>> raw_spin_unlock(&drvdata->spinlock); >>> + >>> + cscfg_csdev_disable_active_config(csdev); >> >> This looks like a separate "fix" from what you are trying to address. Please >> could split this ? > > I don't think so, because without this calling, the "sys_enable_cnt" > never down, It makes error. My point is, we need to "disable_active_config" irrespective of this race, in the sysfs path. So this should be a fix apart from fixing the race. Suzuki > >> Also, would like to hear what Mike has to say about this change. > > IIRC, I followed his suggestion. > > Thanks! >

5 months, 3 weeks

Re: [PATCH v2] coresight: prevent deactivate active config while enable the config

by Suzuki K Poulose

Hi Levi On 23/12/2024 18:53, Yeoreum Yun wrote: > While enable active config via cscfg_csdev_enable_active_config(), > active config could be deactivated via configfs' sysfs interface. > This could make UAF issue in below scenario: > > CPU0 CPU1 > (sysfs enable) load module > cscfg_load_config_sets() > activate config. // sysfs > (sys_active_cnt == 1) > ... > cscfg_csdev_enable_active_config() > lock(csdev->cscfg_csdev_lock) > // here load config activate by CPU1 > unlock(csdev->cscfg_csdev_lock) > > deactivate config // sysfs > (sys_activec_cnt == 0) > cscfg_unload_config_sets() > unload module > > // access to config_desc which freed > // while unloading module. > cfs_csdev_enable_config > > To address this, introduce sys_enable_cnt in cscfg_mgr to prevent > deactivate while there is enabled configuration. Thanks for the finding the problem and the detailed description + patch. I have some concerns on the fix, please find it below. > So we have 3 atomic counters now ! cscfg_mgr->sys_active_cnt // Global count config->active_cnt // Per config count, And another one which this one introduces. cscfg_mgr->sys_enable_cnt // ? And config->active_cnt is always ever 0 or 1. i.e., it is not really a reference counter at the moment but it indicates whether it is active or not. Could we not use that for tracking the references on the specific config ? i.e., every time we "enable_active_config" atomic_inc(config->active_cnt) and disable_active_config() always decrements the config. These could be wrapped in cscfg_get() cscfg_put() which would, inc/dec the refcounts and also drop the "module" reference when the active_cnt == 0 > Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com> > --- > from v1 to v2: > - modify commit message. > --- > .../hwtracing/coresight/coresight-etm4x-core.c | 3 +++ > drivers/hwtracing/coresight/coresight-syscfg.c | 18 ++++++++++++++++-- > drivers/hwtracing/coresight/coresight-syscfg.h | 2 ++ > 3 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index 86893115df17..6218ef40acbc 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -986,6 +986,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev) > smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1); > > raw_spin_unlock(&drvdata->spinlock); > + > + cscfg_csdev_disable_active_config(csdev); This looks like a separate "fix" from what you are trying to address. Please could split this ? Also, would like to hear what Mike has to say about this change. Suzuki > + > cpus_read_unlock(); > > /* > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c > index a70c1454b410..dfa7dcbaf25d 100644 > --- a/drivers/hwtracing/coresight/coresight-syscfg.c > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c > @@ -953,7 +953,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti > cscfg_mgr->sysfs_active_config = cfg_hash; > } else { > /* disable if matching current value */ > - if (cscfg_mgr->sysfs_active_config == cfg_hash) { > + if (cscfg_mgr->sysfs_active_config == cfg_hash && > + !atomic_read(&cscfg_mgr->sys_enable_cnt)) { > _cscfg_deactivate_config(cfg_hash); > cscfg_mgr->sysfs_active_config = 0; > } else > @@ -1055,6 +1056,12 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev, > if (!atomic_read(&cscfg_mgr->sys_active_cnt)) > return 0; > > + /* > + * increment sys_enable_cnt first to prevent deactivate the config > + * while enable active config. > + */ > + atomic_inc(&cscfg_mgr->sys_enable_cnt); > + > /* > * Look for matching configuration - set the active configuration > * context if found. > @@ -1098,6 +1105,10 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev, > raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags); > } > } > + > + if (!config_csdev_active || err) > + atomic_dec(&cscfg_mgr->sys_enable_cnt); > + > return err; > } > EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config); > @@ -1129,8 +1140,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev) > if (config_csdev) { > if (!config_csdev->enabled) > config_csdev = NULL; > - else > + else { > config_csdev->enabled = false; > + atomic_dec(&cscfg_mgr->sys_enable_cnt); > + } > } > csdev->active_cscfg_ctxt = NULL; > raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags); > @@ -1179,6 +1192,7 @@ static int cscfg_create_device(void) > INIT_LIST_HEAD(&cscfg_mgr->config_desc_list); > INIT_LIST_HEAD(&cscfg_mgr->load_order_list); > atomic_set(&cscfg_mgr->sys_active_cnt, 0); > + atomic_set(&cscfg_mgr->sys_enable_cnt, 0); > cscfg_mgr->load_state = CSCFG_NONE; > > /* setup the device */ > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h > index 66e2db890d82..2fc397919985 100644 > --- a/drivers/hwtracing/coresight/coresight-syscfg.h > +++ b/drivers/hwtracing/coresight/coresight-syscfg.h > @@ -38,6 +38,7 @@ enum cscfg_load_ops { > * @config_desc_list: List of system configuration descriptors to load into registered devices. > * @load_order_list: Ordered list of owners for dynamically loaded configurations. > * @sys_active_cnt: Total number of active config descriptor references. > + * @sys_enable_cnt: Total number of enable of active config descriptor references. > * @cfgfs_subsys: configfs subsystem used to manage configurations. > * @sysfs_active_config:Active config hash used if CoreSight controlled from sysfs. > * @sysfs_active_preset:Active preset index used if CoreSight controlled from sysfs. > @@ -50,6 +51,7 @@ struct cscfg_manager { > struct list_head config_desc_list; > struct list_head load_order_list; > atomic_t sys_active_cnt; > + atomic_t sys_enable_cnt; > struct configfs_subsystem cfgfs_subsys; > u32 sysfs_active_config; > int sysfs_active_preset; > -- > LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7} >

5 months, 4 weeks

Re: [PATCH v9 0/7] kvm/coresight: Support exclude guest and exclude host

by James Clark

On 06/01/2025 2:48 pm, Marc Zyngier wrote: > On Mon, 06 Jan 2025 14:24:35 +0000, > James Clark <james.clark(a)linaro.org> wrote: >> >> FEAT_TRF is a Coresight feature that allows trace capture to be >> completely filtered at different exception levels, unlike the existing >> TRCVICTLR controls which may still emit target addresses of branches, >> even if the following trace is filtered. >> >> Without FEAT_TRF, it was possible to start a trace session on a host and >> also collect trace from the guest as TRCVICTLR was never programmed to >> exclude guests (and it could still emit target addresses even if it >> was). >> >> With FEAT_TRF, the current behavior of trace in guests exists depends on >> whether nVHE or VHE are being used. Both of the examples below are from >> the host's point of view, as Coresight isn't accessible from guests. >> This patchset is only relevant to when FEAT_TRF exists, otherwise there >> is no change. >> >> Current behavior: >> >> nVHE/pKVM: >> >> Because the host and the guest are both using TRFCR_EL1, trace will be >> generated in guests depending on the same filter rules the host is >> using. For example if the host is tracing userspace only, then guest >> userspace trace will also be collected. >> >> (This is further limited by whether TRBE is used because an issue >> with TRBE means that it's completely disabled in nVHE guests, but it's >> possible to have other tracing components.) >> >> VHE: >> >> With VHE, the host filters will be in TRFCR_EL2, but the filters in >> TRFCR_EL1 will be active when the guest is running. Because we don't >> write to TRFCR_EL1, guest trace will be completely disabled. >> >> New behavior: >> >> The guest filtering rules from the Perf session are now honored for both >> nVHE and VHE modes. This is done by either writing to TRFCR_EL12 at the >> start of the Perf session and doing nothing else further, or caching the >> guest value and writing it at guest switch for nVHE. In pKVM, trace is >> now be disabled for both protected and unprotected guests. >> >> There is also an optimization where the Coresight drivers pass their >> enabled state to KVM. This means in the common case KVM doesn't have to >> touch any sysregs when the feature isn't in use. >> >> Applies to kvmarm/next (00163be8bb59). > > Can you *PLEASE* stop this absolutely nonsense of posting patches > based on top of random commits? Please look at how we integrate new > developments: they are *always* based on an early -rc tag (usually > -rc3). > > If you depend on other patches, add them to your series and post the > whole thing. > > Thanks, > > M. > Sure, I re-posted it on the latest -rc with a few commits picked up. Thanks James

5 months, 4 weeks

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight