CoreSight

coresight@lists.linaro.org

7 participants
2393 discussions

Re: [PATCH 2/7] perf tools: Add util function for overriding user set config values

by James Clark

On 24/04/2023 16:36, Adrian Hunter wrote: > On 24/04/23 16:47, James Clark wrote: >> There is some duplicated code to only override config values if they >> haven't already been set by the user so make a util function for this. >> >> Signed-off-by: James Clark <james.clark(a)arm.com> > > One minor comment, nevertheless: > > Acked-by: Adrian Hunter <adrian.hunter(a)intel.com> > Thanks for the review >> --- >> tools/perf/arch/arm64/util/arm-spe.c | 26 ++----------------------- >> tools/perf/arch/x86/util/intel-pt.c | 22 ++------------------- >> tools/perf/util/evsel.c | 29 ++++++++++++++++++++++++++++ >> tools/perf/util/evsel.h | 3 +++ >> 4 files changed, 36 insertions(+), 44 deletions(-) >> [...] >> } >> } >> + >> +/* >> + * Set @config_name to @val as long as the user hasn't already set or cleared it >> + * by passing a config term on the command line. >> + * >> + * @val is the value to put into the bits specified by @config_name rather than >> + * the bit pattern. It is shifted into position by this function, so to set >> + * something to true, pass 1 for val rather than a pre shifted value. >> + */ >> +#define field_prep(_mask, _val) (((_val) << (ffsll(_mask) - 1)) & (_mask)) > > I notice there is already tools/include/linux/bitfield.h > so may FIELD_PREP from there could be used? I tried that first, but it seems like quite a lot of effort went into it to make it work on const only values so it doesn't work here. There is this [1] change to make a non const one but it doesn't look like it went anywhere: [1]: https://patchwork.kernel.org/project/linux-omap/patch/3a54a6703879d10f08cf0…

2 years, 2 months

Re: [PATCH v3 1/3] Coresight: Add coresight dummy driver

by Suzuki K Poulose

On 22/04/2023 08:37, Hao Zhang wrote: > Some Coresight devices that kernel don't have permission to access or > configure. So there need driver to register dummy devices as Coresight > devices. It may also be used to define components that may not have > any programming interfaces (e.g, static links), so that paths can be > established in the driver. Provide Coresight API for dummy device > operations, such as enabling and disabling dummy devices. Build the > Coresight path for dummy sink or dummy source for debugging. > > Signed-off-by: Hao Zhang <quic_hazha(a)quicinc.com> > --- > drivers/hwtracing/coresight/Kconfig | 11 ++ > drivers/hwtracing/coresight/Makefile | 1 + > drivers/hwtracing/coresight/coresight-dummy.c | 179 ++++++++++++++++++ > include/linux/coresight.h | 1 + > 4 files changed, 192 insertions(+) > create mode 100644 drivers/hwtracing/coresight/coresight-dummy.c > > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig > index 2b5bbfffbc4f..06f0a7594169 100644 > --- a/drivers/hwtracing/coresight/Kconfig > +++ b/drivers/hwtracing/coresight/Kconfig > @@ -236,4 +236,15 @@ config CORESIGHT_TPDA > > To compile this driver as a module, choose M here: the module will be > called coresight-tpda. > + > +config CORESIGHT_DUMMY > + tristate "Dummy driver support" > + help > + Enables support for dummy driver. Dummy driver can be used for > + CoreSight sources/sinks that are owned and configured by some > + other subsystem and use Linux drivers to configure rest of trace > + path. > + > + To compile this driver as a module, choose M here: the module will be > + called coresight-dummy. > endif > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile > index 33bcc3f7b8ae..995d3b2c76df 100644 > --- a/drivers/hwtracing/coresight/Makefile > +++ b/drivers/hwtracing/coresight/Makefile > @@ -30,3 +30,4 @@ obj-$(CONFIG_CORESIGHT_TPDA) += coresight-tpda.o > coresight-cti-y := coresight-cti-core.o coresight-cti-platform.o \ > coresight-cti-sysfs.o > obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o > +obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o > diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/hwtracing/coresight/coresight-dummy.c > new file mode 100644 > index 000000000000..1fb8b3d1c170 > --- /dev/null > +++ b/drivers/hwtracing/coresight/coresight-dummy.c > @@ -0,0 +1,179 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/coresight.h> > +#include <linux/of.h> > +#include <linux/pm_runtime.h> > + > +#include "coresight-priv.h" > +#include "coresight-trace-id.h" > + > +struct dummy_drvdata { > + struct device *dev; > + struct coresight_device *csdev; > + int traceid; > +}; > + > +DEFINE_CORESIGHT_DEVLIST(source_devs, "dummy_source"); > +DEFINE_CORESIGHT_DEVLIST(sink_devs, "dummy_sink"); > + > +static int dummy_source_enable(struct coresight_device *csdev, > + struct perf_event *event, u32 mode) > +{ > + struct dummy_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + > + dev_dbg(drvdata->dev, "Dummy source enabled\n"); > + > + return 0; > +} > + > +static void dummy_source_disable(struct coresight_device *csdev, > + struct perf_event *event) > +{ > + struct dummy_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + > + dev_dbg(drvdata->dev, "Dummy source disabled\n"); > +} > + > +static int dummy_sink_enable(struct coresight_device *csdev, u32 mode, > + void *data) > +{ > + struct dummy_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + > + dev_dbg(drvdata->dev, "Dummy sink enabled\n"); > + > + return 0; > +} > + > +static int dummy_sink_disable(struct coresight_device *csdev) > +{ > + struct dummy_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + > + dev_dbg(drvdata->dev, "Dummy sink disabled\n"); > + > + return 0; > +} > + > +static const struct coresight_ops_source dummy_source_ops = { > + .enable = dummy_source_enable, > + .disable = dummy_source_disable, > +}; > + > +static const struct coresight_ops dummy_source_cs_ops = { > + .source_ops = &dummy_source_ops, > +}; > + > +static const struct coresight_ops_sink dummy_sink_ops = { > + .enable = dummy_sink_enable, > + .disable = dummy_sink_disable, > +}; > + > +static const struct coresight_ops dummy_sink_cs_ops = { > + .sink_ops = &dummy_sink_ops, > +}; > + > +static int dummy_probe(struct platform_device *pdev) > +{ > + int trace_id; > + struct device *dev = &pdev->dev; > + struct device_node *node = dev->of_node; > + struct coresight_platform_data *pdata; > + struct dummy_drvdata *drvdata; > + struct coresight_desc desc = { 0 }; > + > + if (of_device_is_compatible(node, "arm,coresight-dummy-source")) { > + trace_id = coresight_trace_id_get_system_id(); Why is this needed ? If at all we need something, this must be explicitly asked for. How is this used ? > + if (trace_id < 0) > + return trace_id; > + > + desc.name = coresight_alloc_device_name(&source_devs, dev); > + if (!desc.name) > + return -ENOMEM; > + > + desc.type = CORESIGHT_DEV_TYPE_SOURCE; > + desc.subtype.source_subtype = > + CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS; > + desc.ops = &dummy_source_cs_ops; > + } else if (of_device_is_compatible(node, "arm,coresight-dummy-sink")) { > + desc.name = coresight_alloc_device_name(&sink_devs, dev); > + if (!desc.name) > + return -ENOMEM; > + > + desc.type = CORESIGHT_DEV_TYPE_SINK; > + desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_DUMMY; > + desc.ops = &dummy_sink_cs_ops; > + } else { > + dev_err(dev, "Device type not set\n"); > + return -EINVAL; > + } > + > + pdata = coresight_get_platform_data(dev); > + if (IS_ERR(pdata)) > + return PTR_ERR(pdata); > + pdev->dev.platform_data = pdata; > + > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > + if (!drvdata) > + return -ENOMEM; If we have allocated a traceid, we must clean it up here and/or at device removal. > + > + drvdata->dev = &pdev->dev; > + platform_set_drvdata(pdev, drvdata); > + > + desc.pdata = pdev->dev.platform_data; > + desc.dev = &pdev->dev; > + drvdata->csdev = coresight_register(&desc); > + if (IS_ERR(drvdata->csdev)) > + return PTR_ERR(drvdata->csdev); > + > + drvdata->traceid = (u8)trace_id; Where/how is this used ? Suzuki

2 years, 2 months

Re: [PATCH] coresight: Fix signedness bug in tmc_etr_buf_insert_barrier_packet()

by Suzuki K Poulose

Hi Dan On 21/04/2023 11:42, Dan Carpenter wrote: > This code generates a Smatch warning: > > drivers/hwtracing/coresight/coresight-tmc-etr.c:947 tmc_etr_buf_insert_barrier_packet() > error: uninitialized symbol 'bufp'. > > The problem is that if tmc_sg_table_get_data() returns -EINVAL, then > when we test if "len < CORESIGHT_BARRIER_PKT_SIZE", the negative "len" > value is type promoted to a high unsigned long value which is greater > than CORESIGHT_BARRIER_PKT_SIZE. Fix this bug by adding an explicit > check for error codes. > > Fixes: 75f4e3619fe2 ("coresight: tmc-etr: Add transparent buffer management") > Signed-off-by: Dan Carpenter <dan.carpenter(a)linaro.org> > --- > drivers/hwtracing/coresight/coresight-tmc-etr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index 918d461fcf4a..eaa296ced167 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -942,7 +942,7 @@ tmc_etr_buf_insert_barrier_packet(struct etr_buf *etr_buf, u64 offset) > > len = tmc_etr_buf_get_data(etr_buf, offset, > CORESIGHT_BARRIER_PKT_SIZE, &bufp); > - if (WARN_ON(len < CORESIGHT_BARRIER_PKT_SIZE)) > + if (WARN_ON(len < 0 || len < CORESIGHT_BARRIER_PKT_SIZE)) > return -EINVAL; > coresight_insert_barrier_packet(bufp); > return offset + CORESIGHT_BARRIER_PKT_SIZE; Thanks for the fix, I will send this as fixes at 6.4-rc1, as I have already sent the PULL request for 6.4 Suzuki

2 years, 2 months

Re: [PATCH] perf cs-etm: Add support for coresight trace for any range of CPUs

by Suzuki K Poulose

On 20/04/2023 13:37, Ganapatrao Kulkarni wrote: > > > On 20-04-2023 06:00 pm, James Clark wrote: >> >> >> On 20/04/2023 12:47, Ganapatrao Kulkarni wrote: >>> ... >>> My patch is rebased on 6.3-RC7 codebase with Mike's 3 perf patches >>> related to dynamic id [1] support(queued for 6.4). >>> >>> "perf report -D" works for me. >> >> I was referring to sparse CPU lists, which I think you mentioned above >> doesn't work even with this patch. >> >>> >>> [1] https://www.spinics.net/lists/linux-perf-users/msg27452.html >>> >> >> It should be based on the next branch here: >> git://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git > > OK. It need not be. Since this patch is purely perf tools patch and has nothing to do with the kernel drivers, it should be beased on whatever the tip of the perf tool tree is. Otherwise we risk rebasing to that eventually. Cheers Suzuki

2 years, 2 months

Re: [PATCH] perf cs-etm: Add support for coresight trace for any range of CPUs

by James Clark

On 20/04/2023 12:47, Ganapatrao Kulkarni wrote: > > Hi James, > > On 20-04-2023 03:13 pm, James Clark wrote: >> >> >> On 19/04/2023 18:21, Ganapatrao Kulkarni wrote: >>> The current implementation supports coresight trace for a range of >>> CPUs, if the first CPU is CPU0. >>> >>> Adding changes to enable coresight trace for any range of CPUs by >>> decoding the first CPU also from the header. >>> Later, first CPU id is used instead of CPU0 across the decoder >>> functions. >>> >>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni(a)os.amperecomputing.com> >>> --- >>> .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 4 +- >>> .../perf/util/cs-etm-decoder/cs-etm-decoder.h | 3 +- >>> tools/perf/util/cs-etm.c | 62 ++++++++++++------- >>> 3 files changed, 42 insertions(+), 27 deletions(-) >>> >>> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >>> b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >>> index 82a27ab90c8b..41ab299b643b 100644 >>> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >>> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >>> @@ -724,7 +724,7 @@ cs_etm_decoder__create_etm_decoder(struct >>> cs_etm_decoder_params *d_params, >>> } >>> struct cs_etm_decoder * >>> -cs_etm_decoder__new(int decoders, struct cs_etm_decoder_params >>> *d_params, >>> +cs_etm_decoder__new(int first_decoder, int decoders, struct >>> cs_etm_decoder_params *d_params, >>> struct cs_etm_trace_params t_params[]) >>> { >>> struct cs_etm_decoder *decoder; >>> @@ -769,7 +769,7 @@ cs_etm_decoder__new(int decoders, struct >>> cs_etm_decoder_params *d_params, >>> /* init raw frame logging if required */ >>> cs_etm_decoder__init_raw_frame_logging(d_params, decoder); >>> - for (i = 0; i < decoders; i++) { >>> + for (i = first_decoder; i < decoders; i++) { >>> ret = cs_etm_decoder__create_etm_decoder(d_params, >>> &t_params[i], >>> decoder); >>> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h >>> b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h >>> index 92a855fbe5b8..b06193fc75b4 100644 >>> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h >>> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h >>> @@ -90,7 +90,8 @@ int cs_etm_decoder__process_data_block(struct >>> cs_etm_decoder *decoder, >>> size_t len, size_t *consumed); >>> struct cs_etm_decoder * >>> -cs_etm_decoder__new(int num_cpu, >>> +cs_etm_decoder__new(int first_decoder, >>> + int decoders, >>> struct cs_etm_decoder_params *d_params, >>> struct cs_etm_trace_params t_params[]); >>> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c >>> index 94e2d02009eb..2619513ae088 100644 >>> --- a/tools/perf/util/cs-etm.c >>> +++ b/tools/perf/util/cs-etm.c >>> @@ -55,6 +55,8 @@ struct cs_etm_auxtrace { >>> u8 has_virtual_ts; /* Virtual/Kernel timestamps in the trace. */ >>> int num_cpu; >>> + int first_cpu; >>> + int last_cpu; >>> u64 latest_kernel_timestamp; >>> u32 auxtrace_type; >>> u64 branches_sample_type; >>> @@ -638,14 +640,13 @@ static void cs_etm__set_trace_param_ete(struct >>> cs_etm_trace_params *t_params, >>> } >>> static int cs_etm__init_trace_params(struct cs_etm_trace_params >>> *t_params, >>> - struct cs_etm_auxtrace *etm, >>> - int decoders) >>> + struct cs_etm_auxtrace *etm) >>> { >>> int i; >>> u32 etmidr; >>> u64 architecture; >>> - for (i = 0; i < decoders; i++) { >>> + for (i = etm->first_cpu; i < etm->last_cpu; i++) { >>> architecture = etm->metadata[i][CS_ETM_MAGIC]; >>> switch (architecture) { >>> @@ -817,7 +818,7 @@ static void cs_etm__free(struct perf_session >>> *session) >>> /* Then the RB tree itself */ >>> intlist__delete(traceid_list); >>> - for (i = 0; i < aux->num_cpu; i++) >>> + for (i = aux->first_cpu; i < aux->last_cpu; i++) >>> zfree(&aux->metadata[i]); >>> thread__zput(aux->unknown_thread); >>> @@ -921,7 +922,8 @@ static struct cs_etm_queue >>> *cs_etm__alloc_queue(struct cs_etm_auxtrace *etm, >>> * Each queue can only contain data from one CPU when >>> unformatted, so only one decoder is >>> * needed. >>> */ >>> - int decoders = formatted ? etm->num_cpu : 1; >>> + int first_decoder = formatted ? etm->first_cpu : 0; >>> + int decoders = first_decoder + (formatted ? etm->num_cpu : 1); >>> etmq = zalloc(sizeof(*etmq)); >>> if (!etmq) >>> @@ -937,7 +939,7 @@ static struct cs_etm_queue >>> *cs_etm__alloc_queue(struct cs_etm_auxtrace *etm, >>> if (!t_params) >>> goto out_free; >>> - if (cs_etm__init_trace_params(t_params, etm, decoders)) >>> + if (cs_etm__init_trace_params(t_params, etm)) >>> goto out_free; >>> /* Set decoder parameters to decode trace packets */ >>> @@ -947,8 +949,7 @@ static struct cs_etm_queue >>> *cs_etm__alloc_queue(struct cs_etm_auxtrace *etm, >>> formatted)) >>> goto out_free; >>> - etmq->decoder = cs_etm_decoder__new(decoders, &d_params, >>> - t_params); >>> + etmq->decoder = cs_etm_decoder__new(first_decoder, decoders, >>> &d_params, t_params); >>> if (!etmq->decoder) >>> goto out_free; >>> @@ -2959,11 +2960,11 @@ static int cs_etm__queue_aux_records(struct >>> perf_session *session) >>> * Loop through the ETMs and complain if we find at least one where >>> ts_source != 1 (virtual >>> * timestamps). >>> */ >>> -static bool cs_etm__has_virtual_ts(u64 **metadata, int num_cpu) >>> +static bool cs_etm__has_virtual_ts(u64 **metadata, struct >>> cs_etm_auxtrace *etm) >>> { >>> int j; >>> - for (j = 0; j < num_cpu; j++) { >>> + for (j = etm->first_cpu; j < etm->last_cpu; j++) { >>> switch (metadata[j][CS_ETM_MAGIC]) { >>> case __perf_cs_etmv4_magic: >>> if (HAS_PARAM(j, ETMV4, TS_SOURCE) || >>> metadata[j][CS_ETMV4_TS_SOURCE] != 1) >>> @@ -2982,13 +2983,14 @@ static bool cs_etm__has_virtual_ts(u64 >>> **metadata, int num_cpu) >>> } >>> /* map trace ids to correct metadata block, from information in >>> metadata */ >>> -static int cs_etm__map_trace_ids_metadata(int num_cpu, u64 **metadata) >>> +static int cs_etm__map_trace_ids_metadata(struct cs_etm_auxtrace *etm) >>> { >>> u64 cs_etm_magic; >>> + u64 **metadata = etm->metadata; >>> u8 trace_chan_id; >>> int i, err; >>> - for (i = 0; i < num_cpu; i++) { >>> + for (i = etm->first_cpu; i < etm->last_cpu; i++) { >>> cs_etm_magic = metadata[i][CS_ETM_MAGIC]; >>> switch (cs_etm_magic) { >>> case __perf_cs_etmv3_magic: >>> @@ -3015,12 +3017,13 @@ static int cs_etm__map_trace_ids_metadata(int >>> num_cpu, u64 **metadata) >>> * If we found AUX_HW_ID packets, then set any metadata marked as >>> unused to the >>> * unused value to reduce the number of unneeded decoders created. >>> */ >>> -static int cs_etm__clear_unused_trace_ids_metadata(int num_cpu, u64 >>> **metadata) >>> +static int cs_etm__clear_unused_trace_ids_metadata(struct >>> cs_etm_auxtrace *etm) >>> { >>> u64 cs_etm_magic; >>> + u64 **metadata = etm->metadata; >>> int i; >>> - for (i = 0; i < num_cpu; i++) { >>> + for (i = etm->first_cpu; i < etm->last_cpu; i++) { >>> cs_etm_magic = metadata[i][CS_ETM_MAGIC]; >>> switch (cs_etm_magic) { >>> case __perf_cs_etmv3_magic: >>> @@ -3049,7 +3052,7 @@ int cs_etm__process_auxtrace_info_full(union >>> perf_event *event, >>> int event_header_size = sizeof(struct perf_event_header); >>> int total_size = auxtrace_info->header.size; >>> int priv_size = 0; >>> - int num_cpu; >>> + int num_cpu, first_cpu = 0, last_cpu; >>> int err = 0; >>> int aux_hw_id_found; >>> int i, j; >>> @@ -3068,22 +3071,31 @@ int cs_etm__process_auxtrace_info_full(union >>> perf_event *event, >>> /* First the global part */ >>> ptr = (u64 *) auxtrace_info->priv; >>> num_cpu = ptr[CS_PMU_TYPE_CPUS] & 0xffffffff; >>> - metadata = zalloc(sizeof(*metadata) * num_cpu); >>> + >>> + /* Start parsing after the common part of the header */ >>> + i = CS_HEADER_VERSION_MAX; >>> + >>> + /*Get CPU id of first event */ >>> + first_cpu = ptr[i + CS_ETM_CPU]; >>> + last_cpu = first_cpu + num_cpu; >>> + >>> + if (first_cpu > cpu__max_cpu().cpu || >>> + last_cpu > cpu__max_cpu().cpu) >>> + return -EINVAL; >>> + >>> + metadata = zalloc(sizeof(*metadata) * last_cpu); >> >> Hi Ganapatrao, >> >> I think I see what the problem is, but I'm wondering if a better fix >> would be to further decouple the CPU ID from the index in the array. >> >> With your change it's not clear what happens with sparse recordings, for >> example 'perf record -e cs_etm// -C 1,3,5'. And it seems like there is > > This patch fixes for any range of CPUs. > Record with sparse list of CPUs will not work with current code still. > Is there a major issue that means sparse can't be done? I'm thinking it would be best to fix both issues with one change while we understand this part rather than a half fix that might have do be completely re-understood and re-done later anyway. Unless there is some big blocker but I can't see it? >> some wastage in the zalloc here for example if only CPU 256 is traced >> then we'd still make 256 decoders but 255 of them would be unused? >> >> I tried to test sparse recordings, but your change doesn't apply to the >> latest coresight/next branch. I did notice that 'perf report -D' doesn't >> work with them on coresight/next (it just quits), but I couldn't see if >> that's fixed with your change. > > My patch is rebased on 6.3-RC7 codebase with Mike's 3 perf patches > related to dynamic id [1] support(queued for 6.4). > > "perf report -D" works for me. I was referring to sparse CPU lists, which I think you mentioned above doesn't work even with this patch. > > [1] https://www.spinics.net/lists/linux-perf-users/msg27452.html > It should be based on the next branch here: git://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git >> >> Would a better fix not be to keep the metadata loops from 0-N and >> instead save the CPU ID in cs_etm_decoder_params or the decoder. That >> way it would support both sparse and not starting from 0 cases? I think > > Yep, I though this initially, it got complicated due to for more > for-loops. I will try again and post V2. > I can't imagine it would need any extra for loops off the top of my head. Just saving the CPU ID in a few structs and using it wherever it's needed instead of the loop index. I imagine most of the loops would actually stay the same rather than be changed like you have in V1. >> the code would be better if it's worded like "i < recorded_cpus" rather >> than "i < cpu" to make it clear that i isn't actually the CPU ID it's >> just an index. > > Yes, makes sense to call it "recorded_cpus". > >> >> Also a new test for this scenario would probably be a good idea. >> >> Thanks >> James >> > Thanks, > Ganapat

2 years, 2 months

Re: [PATCH] perf cs-etm: Add support for coresight trace for any range of CPUs

by James Clark

On 19/04/2023 18:21, Ganapatrao Kulkarni wrote: > The current implementation supports coresight trace for a range of > CPUs, if the first CPU is CPU0. > > Adding changes to enable coresight trace for any range of CPUs by > decoding the first CPU also from the header. > Later, first CPU id is used instead of CPU0 across the decoder functions. > > Signed-off-by: Ganapatrao Kulkarni <gankulkarni(a)os.amperecomputing.com> > --- > .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 4 +- > .../perf/util/cs-etm-decoder/cs-etm-decoder.h | 3 +- > tools/perf/util/cs-etm.c | 62 ++++++++++++------- > 3 files changed, 42 insertions(+), 27 deletions(-) > > diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > index 82a27ab90c8b..41ab299b643b 100644 > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > @@ -724,7 +724,7 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params, > } > > struct cs_etm_decoder * > -cs_etm_decoder__new(int decoders, struct cs_etm_decoder_params *d_params, > +cs_etm_decoder__new(int first_decoder, int decoders, struct cs_etm_decoder_params *d_params, > struct cs_etm_trace_params t_params[]) > { > struct cs_etm_decoder *decoder; > @@ -769,7 +769,7 @@ cs_etm_decoder__new(int decoders, struct cs_etm_decoder_params *d_params, > /* init raw frame logging if required */ > cs_etm_decoder__init_raw_frame_logging(d_params, decoder); > > - for (i = 0; i < decoders; i++) { > + for (i = first_decoder; i < decoders; i++) { > ret = cs_etm_decoder__create_etm_decoder(d_params, > &t_params[i], > decoder); > diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h > index 92a855fbe5b8..b06193fc75b4 100644 > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h > @@ -90,7 +90,8 @@ int cs_etm_decoder__process_data_block(struct cs_etm_decoder *decoder, > size_t len, size_t *consumed); > > struct cs_etm_decoder * > -cs_etm_decoder__new(int num_cpu, > +cs_etm_decoder__new(int first_decoder, > + int decoders, > struct cs_etm_decoder_params *d_params, > struct cs_etm_trace_params t_params[]); > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > index 94e2d02009eb..2619513ae088 100644 > --- a/tools/perf/util/cs-etm.c > +++ b/tools/perf/util/cs-etm.c > @@ -55,6 +55,8 @@ struct cs_etm_auxtrace { > u8 has_virtual_ts; /* Virtual/Kernel timestamps in the trace. */ > > int num_cpu; > + int first_cpu; > + int last_cpu; > u64 latest_kernel_timestamp; > u32 auxtrace_type; > u64 branches_sample_type; > @@ -638,14 +640,13 @@ static void cs_etm__set_trace_param_ete(struct cs_etm_trace_params *t_params, > } > > static int cs_etm__init_trace_params(struct cs_etm_trace_params *t_params, > - struct cs_etm_auxtrace *etm, > - int decoders) > + struct cs_etm_auxtrace *etm) > { > int i; > u32 etmidr; > u64 architecture; > > - for (i = 0; i < decoders; i++) { > + for (i = etm->first_cpu; i < etm->last_cpu; i++) { > architecture = etm->metadata[i][CS_ETM_MAGIC]; > > switch (architecture) { > @@ -817,7 +818,7 @@ static void cs_etm__free(struct perf_session *session) > /* Then the RB tree itself */ > intlist__delete(traceid_list); > > - for (i = 0; i < aux->num_cpu; i++) > + for (i = aux->first_cpu; i < aux->last_cpu; i++) > zfree(&aux->metadata[i]); > > thread__zput(aux->unknown_thread); > @@ -921,7 +922,8 @@ static struct cs_etm_queue *cs_etm__alloc_queue(struct cs_etm_auxtrace *etm, > * Each queue can only contain data from one CPU when unformatted, so only one decoder is > * needed. > */ > - int decoders = formatted ? etm->num_cpu : 1; > + int first_decoder = formatted ? etm->first_cpu : 0; > + int decoders = first_decoder + (formatted ? etm->num_cpu : 1); > > etmq = zalloc(sizeof(*etmq)); > if (!etmq) > @@ -937,7 +939,7 @@ static struct cs_etm_queue *cs_etm__alloc_queue(struct cs_etm_auxtrace *etm, > if (!t_params) > goto out_free; > > - if (cs_etm__init_trace_params(t_params, etm, decoders)) > + if (cs_etm__init_trace_params(t_params, etm)) > goto out_free; > > /* Set decoder parameters to decode trace packets */ > @@ -947,8 +949,7 @@ static struct cs_etm_queue *cs_etm__alloc_queue(struct cs_etm_auxtrace *etm, > formatted)) > goto out_free; > > - etmq->decoder = cs_etm_decoder__new(decoders, &d_params, > - t_params); > + etmq->decoder = cs_etm_decoder__new(first_decoder, decoders, &d_params, t_params); > > if (!etmq->decoder) > goto out_free; > @@ -2959,11 +2960,11 @@ static int cs_etm__queue_aux_records(struct perf_session *session) > * Loop through the ETMs and complain if we find at least one where ts_source != 1 (virtual > * timestamps). > */ > -static bool cs_etm__has_virtual_ts(u64 **metadata, int num_cpu) > +static bool cs_etm__has_virtual_ts(u64 **metadata, struct cs_etm_auxtrace *etm) > { > int j; > > - for (j = 0; j < num_cpu; j++) { > + for (j = etm->first_cpu; j < etm->last_cpu; j++) { > switch (metadata[j][CS_ETM_MAGIC]) { > case __perf_cs_etmv4_magic: > if (HAS_PARAM(j, ETMV4, TS_SOURCE) || metadata[j][CS_ETMV4_TS_SOURCE] != 1) > @@ -2982,13 +2983,14 @@ static bool cs_etm__has_virtual_ts(u64 **metadata, int num_cpu) > } > > /* map trace ids to correct metadata block, from information in metadata */ > -static int cs_etm__map_trace_ids_metadata(int num_cpu, u64 **metadata) > +static int cs_etm__map_trace_ids_metadata(struct cs_etm_auxtrace *etm) > { > u64 cs_etm_magic; > + u64 **metadata = etm->metadata; > u8 trace_chan_id; > int i, err; > > - for (i = 0; i < num_cpu; i++) { > + for (i = etm->first_cpu; i < etm->last_cpu; i++) { > cs_etm_magic = metadata[i][CS_ETM_MAGIC]; > switch (cs_etm_magic) { > case __perf_cs_etmv3_magic: > @@ -3015,12 +3017,13 @@ static int cs_etm__map_trace_ids_metadata(int num_cpu, u64 **metadata) > * If we found AUX_HW_ID packets, then set any metadata marked as unused to the > * unused value to reduce the number of unneeded decoders created. > */ > -static int cs_etm__clear_unused_trace_ids_metadata(int num_cpu, u64 **metadata) > +static int cs_etm__clear_unused_trace_ids_metadata(struct cs_etm_auxtrace *etm) > { > u64 cs_etm_magic; > + u64 **metadata = etm->metadata; > int i; > > - for (i = 0; i < num_cpu; i++) { > + for (i = etm->first_cpu; i < etm->last_cpu; i++) { > cs_etm_magic = metadata[i][CS_ETM_MAGIC]; > switch (cs_etm_magic) { > case __perf_cs_etmv3_magic: > @@ -3049,7 +3052,7 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event, > int event_header_size = sizeof(struct perf_event_header); > int total_size = auxtrace_info->header.size; > int priv_size = 0; > - int num_cpu; > + int num_cpu, first_cpu = 0, last_cpu; > int err = 0; > int aux_hw_id_found; > int i, j; > @@ -3068,22 +3071,31 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event, > /* First the global part */ > ptr = (u64 *) auxtrace_info->priv; > num_cpu = ptr[CS_PMU_TYPE_CPUS] & 0xffffffff; > - metadata = zalloc(sizeof(*metadata) * num_cpu); > + > + /* Start parsing after the common part of the header */ > + i = CS_HEADER_VERSION_MAX; > + > + /*Get CPU id of first event */ > + first_cpu = ptr[i + CS_ETM_CPU]; > + last_cpu = first_cpu + num_cpu; > + > + if (first_cpu > cpu__max_cpu().cpu || > + last_cpu > cpu__max_cpu().cpu) > + return -EINVAL; > + > + metadata = zalloc(sizeof(*metadata) * last_cpu); Hi Ganapatrao, I think I see what the problem is, but I'm wondering if a better fix would be to further decouple the CPU ID from the index in the array. With your change it's not clear what happens with sparse recordings, for example 'perf record -e cs_etm// -C 1,3,5'. And it seems like there is some wastage in the zalloc here for example if only CPU 256 is traced then we'd still make 256 decoders but 255 of them would be unused? I tried to test sparse recordings, but your change doesn't apply to the latest coresight/next branch. I did notice that 'perf report -D' doesn't work with them on coresight/next (it just quits), but I couldn't see if that's fixed with your change. Would a better fix not be to keep the metadata loops from 0-N and instead save the CPU ID in cs_etm_decoder_params or the decoder. That way it would support both sparse and not starting from 0 cases? I think the code would be better if it's worded like "i < recorded_cpus" rather than "i < cpu" to make it clear that i isn't actually the CPU ID it's just an index. Also a new test for this scenario would probably be a good idea. Thanks James

2 years, 2 months

[GIT PULL] coresight: Updates for v6.4

by Suzuki K Poulose

Hi Greg, Please find the updates for coresight self hosted tracing targeting v6.4 Kindly pull. Suzuki The following changes since commit 197b6b60ae7bc51dd0814953c562833143b292aa: Linux 6.3-rc4 (2023-03-26 14:40:20 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git tags/coresight-next-v6.4 for you to fetch changes up to 18996a113f2567aef3057e300e3193ce2df1684c: coresight: etm_pmu: Set the module field (2023-04-14 12:14:09 +0100) ---------------------------------------------------------------- coresight: Updates for v6.4 This is a relatively smaller update for CoreSight tracing subsystem targeting v6.4, with the following changes: - Removing Mathieu Poirier as MAINTAINER for the subsystem, with updates to CREDITS for his contributions. - Fix CoreSight ETM PMU to set the module field Signed-off-by: Suzuki K Poulose <suzuki.poulose(a)arm.com> ---------------------------------------------------------------- Suzuki K Poulose (2): MAINTAINERS: Remove Mathieu Poirier as coresight maintainer coresight: etm_pmu: Set the module field CREDITS | 5 +++++ MAINTAINERS | 1 - drivers/hwtracing/coresight/coresight-etm-perf.c | 1 + 3 files changed, 6 insertions(+), 1 deletion(-)

2 years, 2 months

[PATCH] perf tools: Fix use before NULL check introduced by map__dso() introduction

by Arnaldo Carvalho de Melo

James Clark noticed that the recent 63df0e4bc368adbd ("perf map: Add accessor for dso") patch accessed map->dso before the 'map' variable was NULL checked, which is a change in logic that leads to segmentation faults, so comb thru that patch to fix similar cases. Fixes: 63df0e4bc368adbd ("perf map: Add accessor for dso") Cc: Adrian Hunter <adrian.hunter(a)intel.com> Cc: Alexander Shishkin <alexander.shishkin(a)linux.intel.com> Cc: Ian Rogers <irogers(a)google.com> Cc: Ingo Molnar <mingo(a)redhat.com> Cc: Jiri Olsa <jolsa(a)kernel.org> Cc: John Garry <john.g.garry(a)oracle.com> Cc: Leo Yan <leo.yan(a)linaro.org> Cc: Mark Rutland <mark.rutland(a)arm.com> Cc: Mathieu Poirier <mathieu.poirier(a)linaro.org> Cc: Mike Leach <mike.leach(a)linaro.org> Cc: Namhyung Kim <namhyung(a)kernel.org> Cc: Peter Zijlstra <peterz(a)infradead.org Cc: Suzuki K Poulose <suzuki.poulose(a)arm.com> Cc: Will Deacon <will(a)kernel.org> To: James Clark <james.clark(a)arm.com> Link: https://lore.kernel.org/lkml/ Signed-off-by: Arnaldo Carvalho de Melo <acme(a)redhat.com> --- tools/perf/builtin-script.c | 7 +++---- tools/perf/ui/browsers/hists.c | 4 ++-- tools/perf/util/sort.c | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c index 8fba247b798ca307..006f522d0e7f6a18 100644 --- a/tools/perf/builtin-script.c +++ b/tools/perf/builtin-script.c @@ -1075,8 +1075,7 @@ static int grab_bb(u8 *buffer, u64 start, u64 end, return 0; } - dso = map__dso(al.map); - if (!thread__find_map(thread, *cpumode, start, &al) || !dso) { + if (!thread__find_map(thread, *cpumode, start, &al) || (dso = map__dso(al.map)) == NULL) { pr_debug("\tcannot resolve %" PRIx64 "-%" PRIx64 "\n", start, end); return 0; } @@ -1106,9 +1105,9 @@ static int map__fprintf_srccode(struct map *map, u64 addr, FILE *fp, struct srcc unsigned line; int len; char *srccode; - struct dso *dso = map__dso(map); + struct dso *dso; - if (!map || !dso) + if (!map || (dso = map__dso(map)) == NULL) return 0; srcfile = get_srcline_split(dso, map__rip_2objdump(map, addr), diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c index ab70e5f5fad236e5..69c81759a64f938f 100644 --- a/tools/perf/ui/browsers/hists.c +++ b/tools/perf/ui/browsers/hists.c @@ -2499,9 +2499,9 @@ add_annotate_opt(struct hist_browser *browser __maybe_unused, struct map_symbol *ms, u64 addr) { - struct dso *dso = map__dso(ms->map); + struct dso *dso; - if (!ms->map || !dso || dso->annotate_warned) + if (!ms->map || (dso = map__dso(ms->map)) == NULL || dso->annotate_warned) return 0; if (!ms->sym) diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c index f2ffaf90648e469e..31b1cd0935e277ba 100644 --- a/tools/perf/util/sort.c +++ b/tools/perf/util/sort.c @@ -1568,7 +1568,7 @@ static int hist_entry__dcacheline_snprintf(struct hist_entry *he, char *bf, if (he->mem_info) { struct map *map = he->mem_info->daddr.ms.map; - struct dso *dso = map__dso(map); + struct dso *dso = map ? map__dso(map) : NULL; addr = cl_address(he->mem_info->daddr.al_addr, chk_double_cl); ms = &he->mem_info->daddr.ms; -- 2.39.2

2 years, 2 months

[PATCH] perf: cs-etm: Fix segfault in dso lookup

by James Clark

map__dso() is called before thread__find_map() which always results in a null pointer dereference. Fix it by finding first, then checking if it exists. Fixes: 63df0e4bc368 ("perf map: Add accessor for dso") Signed-off-by: James Clark <james.clark(a)arm.com> --- tools/perf/util/cs-etm.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 103865968700..8dd81ddd9e4e 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -885,9 +885,11 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id, thread = etmq->etm->unknown_thread; } - dso = map__dso(al.map); + if (!thread__find_map(thread, cpumode, address, &al)) + return 0; - if (!thread__find_map(thread, cpumode, address, &al) || !dso) + dso = map__dso(al.map); + if (!dso) return 0; if (dso->data.status == DSO_DATA_STATUS_ERROR && -- 2.34.1

2 years, 2 months

[PATCH] coresight: etm_pmu: Set the module field

by Suzuki K Poulose

struct pmu::module must be set to the module owning the PMU driver. Set this for the coresight etm_pmu. Signed-off-by: Suzuki K Poulose <suzuki.poulose(a)arm.com> --- drivers/hwtracing/coresight/coresight-etm-perf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index a48c97da8165..711f451b6946 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -901,6 +901,7 @@ int __init etm_perf_init(void) etm_pmu.addr_filters_sync = etm_addr_filters_sync; etm_pmu.addr_filters_validate = etm_addr_filters_validate; etm_pmu.nr_addr_filters = ETM_ADDR_CMP_MAX; + etm_pmu.module = THIS_MODULE; ret = perf_pmu_register(&etm_pmu, CORESIGHT_ETM_PMU_NAME, -1); if (ret == 0) -- 2.34.1

2 years, 2 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight