CoreSight

coresight@lists.linaro.org

5 participants
2513 discussions

RE: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add "memory-region" property

by Linu Cherian

Hi Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski(a)linaro.org> > Sent: Tuesday, October 3, 2023 12:02 PM > To: Linu Cherian <lcherian(a)marvell.com>; suzuki.poulose(a)arm.com; > mike.leach(a)linaro.org; james.clark(a)arm.com; leo.yan(a)linaro.org > Cc: linux-arm-kernel(a)lists.infradead.org; coresight(a)lists.linaro.org; linux- > kernel(a)vger.kernel.org; robh+dt(a)kernel.org; > krzysztof.kozlowski+dt(a)linaro.org; conor+dt(a)kernel.org; > devicetree(a)vger.kernel.org; Sunil Kovvuri Goutham > <sgoutham(a)marvell.com>; George Cherian <gcherian(a)marvell.com> > Subject: Re: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add > "memory-region" property > > On 03/10/2023 06:33, Linu Cherian wrote: > > Hi Krzysztof, > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzysztof.kozlowski(a)linaro.org> > >> Sent: Saturday, September 30, 2023 8:59 PM > >> To: Linu Cherian <lcherian(a)marvell.com>; suzuki.poulose(a)arm.com; > >> mike.leach(a)linaro.org; james.clark(a)arm.com; leo.yan(a)linaro.org > >> Cc: linux-arm-kernel(a)lists.infradead.org; coresight(a)lists.linaro.org; > >> linux- kernel(a)vger.kernel.org; robh+dt(a)kernel.org; > >> krzysztof.kozlowski+dt(a)linaro.org; conor+dt(a)kernel.org; > >> devicetree(a)vger.kernel.org; Sunil Kovvuri Goutham > >> <sgoutham(a)marvell.com>; George Cherian <gcherian(a)marvell.com> > >> Subject: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add > >> "memory-region" property > >> > >> External Email > >> > >> --------------------------------------------------------------------- > >> - On 29/09/2023 15:37, Linu Cherian 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(a)marvell.com> > >>> --- > >> > >> Where is the changelog? This is supposed to be v4 or something later. > >> Please, keep proper versioning and changelog. > > > > Sure, will add the changelog from next version onwards. > > > > Yeah, the last version was RFC v3 and the RFC tag has been dropped from > this version onwards. > > Hence started this version with V1. > > v1 says it is the first version, but you already had three others. > Please keep continuous version log, regardless whether you call it RFC or RFT > or RFsomething. Okay. Will continue with Patch V5 from next series and add necessary notes in cover letter. > > > > >> > >>> .../bindings/arm/arm,coresight-tmc.yaml | 19 > +++++++++++++++++++ > >>> 1 file changed, 19 insertions(+) > >>> > > Best regards, > Krzysztof

2 years

[PATCH] MAINTAINERS: coresight: Add filename catchall for Coresight

by James Clark

There are a few files missing from the list like test_arm_coresight.sh and arm-coresight.txt. These could be picked up just with a name match. Signed-off-by: James Clark <james.clark(a)arm.com> --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 90f13281d297..3ee45066b7e4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2066,6 +2066,7 @@ F: tools/perf/arch/arm/util/pmu.c F: tools/perf/tests/shell/coresight/* F: tools/perf/util/cs-etm-decoder/* F: tools/perf/util/cs-etm.* +N: coresight ARM/CORTINA SYSTEMS GEMINI ARM ARCHITECTURE M: Hans Ulli Kroll <ulli.kroll(a)googlemail.com> -- 2.34.1

2 years

Re: [PATCH v2] perf cs-etm: Fix incorrect or missing decoder for raw trace

by James Clark

On 04/10/2023 01:32, Besar Wicaksono wrote: > The decoder creation for raw trace uses metadata from the first CPU. > On per-cpu mode, this metadata is incorrectly used for every decoder. > On per-process/per-thread traces, the first CPU is CPU0. If CPU0 trace > is not enabled, its metadata will be marked unused and the decoder is > not created. Perf report dump skips the decoding part because the > decoder is missing. > > To fix this, use metadata of the CPU associated with sample object. > > Signed-off-by: Besar Wicaksono <bwicaksono(a)nvidia.com> > --- > > Changes from v1: > * Update commit message > * Add fallback to CPU-0 metadata if sample CPU id is not available > * Preserve cs_etm__set_trace_param_* arguments and just breakdown the index > parameter into trace-param and metadata indexes > Thanks to Mike and James for the feedback. > v1: https://lore.kernel.org/lkml/20230919224553.1658-1-bwicaksono@nvidia.com/T/… > > --- > tools/perf/util/cs-etm.c | 106 ++++++++++++++++++++++++--------------- > 1 file changed, 65 insertions(+), 41 deletions(-) > There is a trivial conflict on the perf-tools-next repo that it's probably worth rebasing for, otherwise: Reviewed-by: James Clark <james.clark(a)arm.com> Also should probably include linux-perf-users(a)vger.kernel.org as tools changes actually go through the perf tree rather than the coresight one. > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > index 1419b40dfbe8..3abe68a9981e 100644 > --- a/tools/perf/util/cs-etm.c > +++ b/tools/perf/util/cs-etm.c > @@ -293,22 +293,31 @@ static int cs_etm__metadata_set_trace_id(u8 trace_chan_id, u64 *cpu_metadata) > }) > > /* > - * Get a metadata for a specific cpu from an array. > + * Get a metadata index for a specific cpu from an array. > * > */ > -static u64 *get_cpu_data(struct cs_etm_auxtrace *etm, int cpu) > +static int get_cpu_data_idx(struct cs_etm_auxtrace *etm, int cpu) > { > int i; > - u64 *metadata = NULL; > > for (i = 0; i < etm->num_cpu; i++) { > if (etm->metadata[i][CS_ETM_CPU] == (u64)cpu) { > - metadata = etm->metadata[i]; > - break; > + return i; > } > } > > - return metadata; > + return -1; > +} > + > +/* > + * Get a metadata for a specific cpu from an array. > + * > + */ > +static u64 *get_cpu_data(struct cs_etm_auxtrace *etm, int cpu) > +{ > + int idx = get_cpu_data_idx(etm, cpu); > + > + return (idx != -1) ? etm->metadata[idx] : NULL; > } > > /* > @@ -651,66 +660,80 @@ static void cs_etm__packet_dump(const char *pkt_string) > } > > static void cs_etm__set_trace_param_etmv3(struct cs_etm_trace_params *t_params, > - struct cs_etm_auxtrace *etm, int idx, > - u32 etmidr) > + struct cs_etm_auxtrace *etm, int t_idx, > + int m_idx, u32 etmidr) > { > u64 **metadata = etm->metadata; > > - t_params[idx].protocol = cs_etm__get_v7_protocol_version(etmidr); > - t_params[idx].etmv3.reg_ctrl = metadata[idx][CS_ETM_ETMCR]; > - t_params[idx].etmv3.reg_trc_id = metadata[idx][CS_ETM_ETMTRACEIDR]; > + t_params[t_idx].protocol = cs_etm__get_v7_protocol_version(etmidr); > + t_params[t_idx].etmv3.reg_ctrl = metadata[m_idx][CS_ETM_ETMCR]; > + t_params[t_idx].etmv3.reg_trc_id = metadata[m_idx][CS_ETM_ETMTRACEIDR]; > } > > static void cs_etm__set_trace_param_etmv4(struct cs_etm_trace_params *t_params, > - struct cs_etm_auxtrace *etm, int idx) > + struct cs_etm_auxtrace *etm, int t_idx, > + int m_idx) > { > u64 **metadata = etm->metadata; > > - t_params[idx].protocol = CS_ETM_PROTO_ETMV4i; > - t_params[idx].etmv4.reg_idr0 = metadata[idx][CS_ETMV4_TRCIDR0]; > - t_params[idx].etmv4.reg_idr1 = metadata[idx][CS_ETMV4_TRCIDR1]; > - t_params[idx].etmv4.reg_idr2 = metadata[idx][CS_ETMV4_TRCIDR2]; > - t_params[idx].etmv4.reg_idr8 = metadata[idx][CS_ETMV4_TRCIDR8]; > - t_params[idx].etmv4.reg_configr = metadata[idx][CS_ETMV4_TRCCONFIGR]; > - t_params[idx].etmv4.reg_traceidr = metadata[idx][CS_ETMV4_TRCTRACEIDR]; > + t_params[t_idx].protocol = CS_ETM_PROTO_ETMV4i; > + t_params[t_idx].etmv4.reg_idr0 = metadata[m_idx][CS_ETMV4_TRCIDR0]; > + t_params[t_idx].etmv4.reg_idr1 = metadata[m_idx][CS_ETMV4_TRCIDR1]; > + t_params[t_idx].etmv4.reg_idr2 = metadata[m_idx][CS_ETMV4_TRCIDR2]; > + t_params[t_idx].etmv4.reg_idr8 = metadata[m_idx][CS_ETMV4_TRCIDR8]; > + t_params[t_idx].etmv4.reg_configr = metadata[m_idx][CS_ETMV4_TRCCONFIGR]; > + t_params[t_idx].etmv4.reg_traceidr = metadata[m_idx][CS_ETMV4_TRCTRACEIDR]; > } > > static void cs_etm__set_trace_param_ete(struct cs_etm_trace_params *t_params, > - struct cs_etm_auxtrace *etm, int idx) > + struct cs_etm_auxtrace *etm, int t_idx, > + int m_idx) > { > u64 **metadata = etm->metadata; > > - t_params[idx].protocol = CS_ETM_PROTO_ETE; > - t_params[idx].ete.reg_idr0 = metadata[idx][CS_ETE_TRCIDR0]; > - t_params[idx].ete.reg_idr1 = metadata[idx][CS_ETE_TRCIDR1]; > - t_params[idx].ete.reg_idr2 = metadata[idx][CS_ETE_TRCIDR2]; > - t_params[idx].ete.reg_idr8 = metadata[idx][CS_ETE_TRCIDR8]; > - t_params[idx].ete.reg_configr = metadata[idx][CS_ETE_TRCCONFIGR]; > - t_params[idx].ete.reg_traceidr = metadata[idx][CS_ETE_TRCTRACEIDR]; > - t_params[idx].ete.reg_devarch = metadata[idx][CS_ETE_TRCDEVARCH]; > + t_params[t_idx].protocol = CS_ETM_PROTO_ETE; > + t_params[t_idx].ete.reg_idr0 = metadata[m_idx][CS_ETE_TRCIDR0]; > + t_params[t_idx].ete.reg_idr1 = metadata[m_idx][CS_ETE_TRCIDR1]; > + t_params[t_idx].ete.reg_idr2 = metadata[m_idx][CS_ETE_TRCIDR2]; > + t_params[t_idx].ete.reg_idr8 = metadata[m_idx][CS_ETE_TRCIDR8]; > + t_params[t_idx].ete.reg_configr = metadata[m_idx][CS_ETE_TRCCONFIGR]; > + t_params[t_idx].ete.reg_traceidr = metadata[m_idx][CS_ETE_TRCTRACEIDR]; > + t_params[t_idx].ete.reg_devarch = metadata[m_idx][CS_ETE_TRCDEVARCH]; > } > > static int cs_etm__init_trace_params(struct cs_etm_trace_params *t_params, > struct cs_etm_auxtrace *etm, > + bool formatted, > + int sample_cpu, > int decoders) > { > - int i; > + int t_idx, m_idx; > u32 etmidr; > u64 architecture; > > - for (i = 0; i < decoders; i++) { > - architecture = etm->metadata[i][CS_ETM_MAGIC]; > + for (t_idx = 0; t_idx < decoders; t_idx++) { > + if (formatted) > + m_idx = t_idx; > + else { > + m_idx = get_cpu_data_idx(etm, sample_cpu); > + if (m_idx == -1) { > + pr_warning("CS_ETM: unknown CPU, falling back to first metadata\n"); > + m_idx = 0; > + } > + } > + > + architecture = etm->metadata[m_idx][CS_ETM_MAGIC]; > > switch (architecture) { > case __perf_cs_etmv3_magic: > - etmidr = etm->metadata[i][CS_ETM_ETMIDR]; > - cs_etm__set_trace_param_etmv3(t_params, etm, i, etmidr); > + etmidr = etm->metadata[m_idx][CS_ETM_ETMIDR]; > + cs_etm__set_trace_param_etmv3(t_params, etm, t_idx, m_idx, etmidr); > break; > case __perf_cs_etmv4_magic: > - cs_etm__set_trace_param_etmv4(t_params, etm, i); > + cs_etm__set_trace_param_etmv4(t_params, etm, t_idx, m_idx); > break; > case __perf_cs_ete_magic: > - cs_etm__set_trace_param_ete(t_params, etm, i); > + cs_etm__set_trace_param_ete(t_params, etm, t_idx, m_idx); > break; > default: > return -EINVAL; > @@ -1026,7 +1049,7 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id, > } > > static struct cs_etm_queue *cs_etm__alloc_queue(struct cs_etm_auxtrace *etm, > - bool formatted) > + bool formatted, int sample_cpu) > { > struct cs_etm_decoder_params d_params; > struct cs_etm_trace_params *t_params = NULL; > @@ -1051,7 +1074,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, formatted, sample_cpu, decoders)) > goto out_free; > > /* Set decoder parameters to decode trace packets */ > @@ -1091,14 +1114,15 @@ static struct cs_etm_queue *cs_etm__alloc_queue(struct cs_etm_auxtrace *etm, > static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm, > struct auxtrace_queue *queue, > unsigned int queue_nr, > - bool formatted) > + bool formatted, > + int sample_cpu) > { > struct cs_etm_queue *etmq = queue->priv; > > if (list_empty(&queue->head) || etmq) > return 0; > > - etmq = cs_etm__alloc_queue(etm, formatted); > + etmq = cs_etm__alloc_queue(etm, formatted, sample_cpu); > > if (!etmq) > return -ENOMEM; > @@ -2826,7 +2850,7 @@ static int cs_etm__process_auxtrace_event(struct perf_session *session, > * formatted in piped mode (true). > */ > err = cs_etm__setup_queue(etm, &etm->queues.queue_array[idx], > - idx, true); > + idx, true, -1); > if (err) > return err; > > @@ -3032,7 +3056,7 @@ static int cs_etm__queue_aux_fragment(struct perf_session *session, off_t file_o > idx = auxtrace_event->idx; > formatted = !(aux_event->flags & PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW); > return cs_etm__setup_queue(etm, &etm->queues.queue_array[idx], > - idx, formatted); > + idx, formatted, sample->cpu); > } > > /* Wasn't inside this buffer, but there were no parse errors. 1 == 'not found' */

2 years

Re: [PATCH 1/3] perf tests test_arm_coresight: Fix the shellcheck warning in latest test_arm_coresight.sh

by James Clark

On 29/09/2023 05:11, Athira Rajeev wrote: > Running shellcheck on tests/shell/test_arm_coresight.sh > throws below warnings: > > In tests/shell/test_arm_coresight.sh line 15: > cs_etm_path=$(find /sys/bus/event_source/devices/cs_etm/ -name cpu* -print -quit) > ^--^ SC2061: Quote the parameter to -name so the shell won't interpret it. > > In tests/shell/test_arm_coresight.sh line 20: > if [ $archhver -eq 5 -a "$(printf "0x%X\n" $archpart)" = "0xA13" ] ; then > ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined > > This warning is observed after commit: > "commit bb350847965d ("perf test: Update cs_etm testcase for Arm ETE")" > > Fixed this issue by using quoting 'cpu*' for SC2061 and > using "&&" in line number 20 for SC2166 warning > > Fixes: bb350847965d ("perf test: Update cs_etm testcase for Arm ETE") > Signed-off-by: Athira Rajeev <atrajeev(a)linux.vnet.ibm.com> > --- > tools/perf/tests/shell/test_arm_coresight.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/tests/shell/test_arm_coresight.sh b/tools/perf/tests/shell/test_arm_coresight.sh > index fe78c4626e45..f2115dfa24a5 100755 > --- a/tools/perf/tests/shell/test_arm_coresight.sh > +++ b/tools/perf/tests/shell/test_arm_coresight.sh > @@ -12,12 +12,12 @@ > glb_err=0 > > cs_etm_dev_name() { > - cs_etm_path=$(find /sys/bus/event_source/devices/cs_etm/ -name cpu* -print -quit) > + cs_etm_path=$(find /sys/bus/event_source/devices/cs_etm/ -name 'cpu*' -print -quit) > trcdevarch=$(cat ${cs_etm_path}/mgmt/trcdevarch) > archhver=$((($trcdevarch >> 12) & 0xf)) > archpart=$(($trcdevarch & 0xfff)) > > - if [ $archhver -eq 5 -a "$(printf "0x%X\n" $archpart)" = "0xA13" ] ; then > + if [ $archhver -eq 5 ] && [ "$(printf "0x%X\n" $archpart)" = "0xA13" ] ; then > echo "ete" > else > echo "etm" Reviewed-by: James Clark <james.clark(a)arm.com>

2 years

RE: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add "memory-region" property

by Linu Cherian

Hi Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski(a)linaro.org> > Sent: Saturday, September 30, 2023 8:59 PM > To: Linu Cherian <lcherian(a)marvell.com>; suzuki.poulose(a)arm.com; > mike.leach(a)linaro.org; james.clark(a)arm.com; leo.yan(a)linaro.org > Cc: linux-arm-kernel(a)lists.infradead.org; coresight(a)lists.linaro.org; linux- > kernel(a)vger.kernel.org; robh+dt(a)kernel.org; > krzysztof.kozlowski+dt(a)linaro.org; conor+dt(a)kernel.org; > devicetree(a)vger.kernel.org; Sunil Kovvuri Goutham > <sgoutham(a)marvell.com>; George Cherian <gcherian(a)marvell.com> > Subject: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add > "memory-region" property > > External Email > > ---------------------------------------------------------------------- > On 29/09/2023 15:37, Linu Cherian 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(a)marvell.com> > > --- > > Where is the changelog? This is supposed to be v4 or something later. > Please, keep proper versioning and changelog. Sure, will add the changelog from next version onwards. Yeah, the last version was RFC v3 and the RFC tag has been dropped from this version onwards. Hence started this version with V1. > > > .../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 > > Drop the 'mem' suffixes. > Ack. Will remove it in next version.

2 years

[PATCH] coresight: Add mode check when enable/disable csdev source

by Junhao He

The commmit [1] replace "source_ops(csdev)::enable()" with coresight_enable_source(). After this patch, when enable ETM through perf mode, the csdev::enable will be set to true. Then if we to disable the ETM by sysfs mode at the time, the resource will be released incorrectly, kernel will report the following call trace. perf record -e cs_etm/@ultra_smb0/ -C 2 sleep 30 & echo 1 > /sys/bus/coresight/devices/ultra_smb0/enable_sink echo 1 > /sys/bus/coresight/devices/etm0/enable_source echo 0 > /sys/bus/coresight/devices/etm0/enable_source Unable to handle kernel NULL pointer dereference at virtual address 00000000000001d0 Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP ... Call trace: etm4_disable+0x54/0x150 [coresight_etm4x] coresight_disable_source+0x6c/0x98 [coresight] coresight_disable+0x74/0x1c0 [coresight] enable_source_store+0x88/0xa0 [coresight] dev_attr_store+0x20/0x40 sysfs_kf_write+0x4c/0x68 kernfs_fop_write_iter+0x120/0x1b8 vfs_write+0x2dc/0x3b0 ksys_write+0x70/0x108 __arm64_sys_write+0x24/0x38 invoke_syscall+0x50/0x128 el0_svc_common.constprop.0+0x104/0x130 do_el0_svc+0x40/0xb8 el0_svc+0x2c/0xb8 el0t_64_sync_handler+0xc0/0xc8 el0t_64_sync+0x1a4/0x1a8 Code: d53cd042 91002000 b9402a81 b8626800 (f940ead5) ---[ end trace 0000000000000000 ]--- The csdev::enable specify the device is currently part of an active path, when enabling source via perf mode, csdev::enable also should be set to true. We can add mode check in etm4_disable() to fix that, if it's done, the sysfs cannot report busy when sysfs to enable ETM that has been enabled by perf. Struct coresight_device add member of mode to check device busy in coresight_(enable/disable)_source function to fixes handle kernel NULL pointer, and sysfs report busy if perf session is already using the ETM. By the way, inperhaps another cleanup patch may be upload to remove the etmv4_drvdata::mode tmc_drvdata::mode or others. Test: perf record -e cs_etm/@ultra_smb0/ -C 2 sleep 30 & cat /sys/bus/coresight/devices/etm2/enable_source 1 echo 1 > /sys/bus/coresight/devices/etm2/enable_source -bash: echo: write error: Device or resource busy echo 0 > /sys/bus/coresight/devices/etm2/enable_source coresight etm2: Someone is already using the tracer cat /sys/bus/coresight/devices/etm2/enable_source 1 [1] "coresight: Enable and disable helper devices adjacent to the path" Fixes: 6148652807ba ("coresight: Enable and disable helper devices adjacent to the path") Signed-off-by: Junhao He <hejunhao3(a)huawei.com> --- drivers/hwtracing/coresight/coresight-core.c | 18 ++++++++++++++++-- .../hwtracing/coresight/coresight-etm-perf.c | 6 +++++- drivers/hwtracing/coresight/coresight-priv.h | 3 ++- include/linux/coresight.h | 14 ++++++++------ 4 files changed, 31 insertions(+), 10 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 206f17826399..7b485fc2ed19 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -388,6 +388,7 @@ int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode, return ret; } csdev->enable = true; + csdev->mode = mode; } atomic_inc(&csdev->refcnt); @@ -446,18 +447,26 @@ static void coresight_disable_helpers(struct coresight_device *csdev) * the device if there are no users left. * * @csdev: The coresight device to disable + * @mode: How the coresight device is being used, perf mode or sysfs mode. * @data: Opaque data to pass on to the disable function of the source device. * For example in perf mode this is a pointer to the struct perf_event. * * Returns true if the device has been disabled. */ -bool coresight_disable_source(struct coresight_device *csdev, void *data) +bool coresight_disable_source(struct coresight_device *csdev, enum cs_mode mode, + void *data) { + if (csdev->mode && csdev->mode != mode) { + dev_err(&csdev->dev, "Someone is already using the tracer\n"); + return false; + } + if (atomic_dec_return(&csdev->refcnt) == 0) { if (source_ops(csdev)->disable) source_ops(csdev)->disable(csdev, data); coresight_disable_helpers(csdev); csdev->enable = false; + csdev->mode = CS_MODE_DISABLED; } return !csdev->enable; } @@ -1117,6 +1126,11 @@ int coresight_enable(struct coresight_device *csdev) if (ret) goto out; + if (csdev->mode == CS_MODE_PERF) { + ret = -EBUSY; + goto out; + } + if (csdev->enable) { /* * There could be multiple applications driving the software @@ -1202,7 +1216,7 @@ void coresight_disable(struct coresight_device *csdev) if (ret) goto out; - if (!csdev->enable || !coresight_disable_source(csdev, NULL)) + if (!csdev->enable || !coresight_disable_source(csdev, CS_MODE_SYSFS, NULL)) goto out; switch (csdev->subtype.source_subtype) { diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 5ca6278baff4..015aea9f2b22 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -459,6 +459,10 @@ static void etm_event_start(struct perf_event *event, int flags) if (WARN_ON(ctxt->event_data)) goto fail; + /* Sysfs is already using the tracer */ + if (csdev->mode == CS_MODE_SYSFS) + goto fail; + /* * Deal with the ring buffer API and get a handle on the * session's information. @@ -587,7 +591,7 @@ static void etm_event_stop(struct perf_event *event, int mode) return; /* stop tracer */ - coresight_disable_source(csdev, event); + coresight_disable_source(csdev, CS_MODE_PERF, event); /* tell the core */ event->hw.state = PERF_HES_STOPPED; diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index 767076e07970..1f8b512ba5ac 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -233,6 +233,7 @@ void coresight_set_percpu_sink(int cpu, struct coresight_device *csdev); struct coresight_device *coresight_get_percpu_sink(int cpu); int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode, void *data); -bool coresight_disable_source(struct coresight_device *csdev, void *data); +bool coresight_disable_source(struct coresight_device *csdev, enum cs_mode mode, + void *data); #endif diff --git a/include/linux/coresight.h b/include/linux/coresight.h index a269fffaf991..acbbedb9abf3 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -216,6 +216,12 @@ struct coresight_sysfs_link { const char *target_name; }; +enum cs_mode { + CS_MODE_DISABLED, + CS_MODE_SYSFS, + CS_MODE_PERF, +}; + /** * struct coresight_device - representation of a device as used by the framework * @pdata: Platform data with device connections associated to this device. @@ -228,6 +234,7 @@ struct coresight_sysfs_link { * @refcnt: keep track of what is in use. * @orphan: true if the component has connections that haven't been linked. * @enable: 'true' if component is currently part of an active path. + * @mode: This tracer's mode, i.e sysFS, Perf or disabled. * @activated: 'true' only if a _sink_ has been activated. A sink can be * activated but not yet enabled. Enabling for a _sink_ * happens when a source has been selected and a path is enabled @@ -252,6 +259,7 @@ struct coresight_device { atomic_t refcnt; bool orphan; bool enable; /* true only if configured as part of a path */ + enum cs_mode mode; /* sink specific fields */ bool activated; /* true only if a sink is part of a path */ struct dev_ext_attribute *ea; @@ -290,12 +298,6 @@ static struct coresight_dev_list (var) = { \ #define to_coresight_device(d) container_of(d, struct coresight_device, dev) -enum cs_mode { - CS_MODE_DISABLED, - CS_MODE_SYSFS, - CS_MODE_PERF, -}; - #define source_ops(csdev) csdev->ops->source_ops #define sink_ops(csdev) csdev->ops->sink_ops #define link_ops(csdev) csdev->ops->link_ops -- 2.30.0

2 years

[GIT PULL] coresight: Fixes for v6.6

by Suzuki K Poulose

Hi Greg Please find a couple of fixes for coresight subsystem targetting v6.6 Kindly pull Suzuki The following changes since commit 0bb80ecc33a8fb5a682236443c1e740d5c917d1d: Linux 6.6-rc1 (2023-09-10 16:28:41 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git tags/coresight-fixes-v6.6-1 for you to fetch changes up to e5028011885a85032aa3c1b7e3e493bcdacb4a0a: coresight: tmc-etr: Disable warnings for allocation failures (2023-09-20 10:46:30 +0100) ---------------------------------------------------------------- coresight: Fixes for v6.6 Couple of fixes for the CoreSight self-hosted tracing targeting v6.6. Includes : - Fix runtime warnings while reusing the TMC-ETR buffer - Fix (disable) warning when a large buffer is allocated in the flat mode. ---------------------------------------------------------------- Linu Cherian (1): coresight: Fix run time warnings while reusing ETR buffer Suzuki K Poulose (1): coresight: tmc-etr: Disable warnings for allocation failures drivers/hwtracing/coresight/coresight-tmc-etr.c | 27 ++++++++++++++----------- 1 file changed, 15 insertions(+), 12 deletions(-)

2 years

Re: [PATCH v1 3/5] arm64: KVM: Add interface to set guest value for TRFCR register

by James Clark

On 29/09/2023 11:17, Marc Zyngier wrote: > On Thu, 28 Sep 2023 16:16:07 +0100, > James Clark <james.clark(a)arm.com> wrote: >> >> Add an interface for the Coresight driver to use to set the value of the >> TRFCR register for the guest. This register controls the exclude >> settings for trace at different exception levels, and is used to >> honor the exclude_host and exclude_guest parameters from the Perf >> session. This will be used to later write TRFCR_EL1 on nVHE at guest >> switch. For VHE, TRFCR_EL1 is written immediately. Because guest writes >> to the register are trapped, the value will persist and can't be >> modified. >> >> The settings must be copied to the vCPU before each run in the same >> way that PMU events are because the per-cpu struct isn't accessible in >> protected mode. >> >> Now that both guest and host values are saved, rename trfcr_el1 to >> host_trfcr_el1 to make it clear that's the value that should be restored >> on return to the host. >> >> Reviewed-by: Mark Brown <broonie(a)kernel.org> (sysreg) >> Signed-off-by: James Clark <james.clark(a)arm.com> >> --- >> arch/arm64/include/asm/kvm_host.h | 6 +++++- >> arch/arm64/kvm/arm.c | 1 + >> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ >> arch/arm64/kvm/hyp/nvhe/debug-sr.c | 4 ++-- >> arch/arm64/tools/sysreg | 4 ++++ >> 5 files changed, 33 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 498f922f4f41..0e57827a0cf2 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -547,7 +547,8 @@ struct kvm_vcpu_arch { >> /* Statistical profiling extension */ >> u64 pmscr_el1; >> /* Self-hosted trace */ >> - u64 trfcr_el1; >> + u64 host_trfcr_el1; >> + u64 guest_trfcr_el1; >> } host_debug_state; > > I think it is high time we stop having *guest* state in a structure > that is obviously for the host, starting by moving the breakpoint and > watchpoint out, and into the sysreg array. > It looks like host_debug_state::regs is actually the host debug state, despite the type being called "struct kvm_guest_debug_arch". But yeah I can move pmscr_el1 and trfcr_el1 to the guest and host sysreg arrays, that would make sense. host_debug_state::regs seems to be a bit more intertwined with the kvm_arch_vcpu_ioctl_set_guest_debug() ioctl and the pattern to support two different sets of guest debug state, so I'm not sure if it's worth touching that at this point: * We maintain more than a single set of debug registers to support * debugging the guest ... * debug_ptr points to the set of debug registers that should be loaded * onto the hardware when running the guest. */ struct kvm_guest_debug_arch *debug_ptr; struct kvm_guest_debug_arch vcpu_debug_state; struct kvm_guest_debug_arch external_debug_state; > And then TRFCR_EL1 can join the fun. But it is pretty unclear whether > that's actually the guest state. > It has an effect on the guest, but can never actually be read or written by the guest. Not sure if that pattern exists elsewhere... >> >> /* VGIC state */ >> @@ -1097,6 +1098,8 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu); >> void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr); >> void kvm_clr_pmu_events(u32 clr); >> bool kvm_set_pmuserenr(u64 val); >> +void kvm_etm_set_guest_trfcr(u64 trfcr_guest); >> +void kvm_etm_update_vcpu_events(struct kvm_vcpu *vcpu); >> #else >> static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {} >> static inline void kvm_clr_pmu_events(u32 clr) {} >> @@ -1104,6 +1107,7 @@ static inline bool kvm_set_pmuserenr(u64 val) >> { >> return false; >> } >> +static inline void kvm_etm_set_guest_trfcr(u64 trfcr_guest) {} >> #endif >> >> void kvm_vcpu_load_sysregs_vhe(struct kvm_vcpu *vcpu); >> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c >> index 1bfdd583b261..65e805dc1d7a 100644 >> --- a/arch/arm64/kvm/arm.c >> +++ b/arch/arm64/kvm/arm.c >> @@ -958,6 +958,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) >> kvm_vgic_flush_hwstate(vcpu); >> >> kvm_pmu_update_vcpu_events(vcpu); >> + kvm_etm_update_vcpu_events(vcpu); >> >> /* >> * Ensure we set mode to IN_GUEST_MODE after we disable >> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >> index 6a1bad1a921b..379d2677961f 100644 >> --- a/arch/arm64/kvm/debug.c >> +++ b/arch/arm64/kvm/debug.c >> @@ -22,6 +22,7 @@ >> DBG_MDSCR_MDE) >> >> static DEFINE_PER_CPU(u64, mdcr_el2); >> +static DEFINE_PER_CPU(u64, guest_trfcr); > > Hold on a sec. Why is that global? I'd expect the tracing to be > specific to a vcpu, and not to affect *everything*. > I suppose because it is a property of the host CPU rather than the vcpu. The vcpu might not even exist yet when the Perf session starts, and Perf sessions only care about the host CPU's perspective. It's defining the guest trfcr value for any vcpu that might or might not run on this core in the future, rather than any one specific vcpu. I copied the pattern from the existing Perf PMU settings in arch/arm64/kvm/pmu.c: static DEFINE_PER_CPU(struct kvm_pmu_events, kvm_pmu_events); We could flip it around and make KVM say "what's the guest trfcr value for this core?" to the Coresight driver at the point of guest switch, but all that would mean is that the global would be in the Coresight driver instead. >> >> /** >> * save/restore_guest_debug_regs >> @@ -342,3 +343,23 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu) >> vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE); >> vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRFCR); >> } >> + >> +void kvm_etm_set_guest_trfcr(u64 trfcr_guest) >> +{ >> + if (has_vhe()) >> + write_sysreg_s(trfcr_guest, SYS_TRFCR_EL12); >> + else >> + *this_cpu_ptr(&guest_trfcr) = trfcr_guest; >> +} >> +EXPORT_SYMBOL_GPL(kvm_etm_set_guest_trfcr); > > In what context can this be called? What if we want to expose TRFCR to > guests instead? It looks to me that this ultimately precludes such > use. > Yes it does, but currently Coresight isn't exposed to guests at all so it's not a problem. In the future if we did decide to do it, there would be significant work to somehow share the components between the host and the guest. But even if they were shared, in the case where the guest isn't using trace the trfcr and exclude guest settings would still need to work, so something of this form would still be required. >> + >> +/* >> + * Updates the vcpu's view of the etm events for this cpu. Must be >> + * called before every vcpu run after disabling interrupts, to ensure >> + * that an interrupt cannot fire and update the structure. >> + */ >> +void kvm_etm_update_vcpu_events(struct kvm_vcpu *vcpu) >> +{ >> + if (!has_vhe() && vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR)) >> + vcpu->arch.host_debug_state.guest_trfcr_el1 = *this_cpu_ptr(&guest_trfcr); > > But what is the point of this per-vcpu field if all you care about is > some per-CPU data? > This was the only way I could find to share data between the host and have it available to EL2 on the guest switch. I saw it was also done that way for PMUs in kvm_pmu_update_vcpu_events(). Maybe it could be done by exposing the guest_trfcr global to both sides with some annotation? But I assumed that wasn't idomatic as it wasn't done for PMUs. >> +} >> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c >> index 89c208112eb7..55bc01e9808f 100644 >> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c >> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c >> @@ -86,7 +86,7 @@ void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu) >> __debug_save_spe(&vcpu->arch.host_debug_state.pmscr_el1); >> /* Disable and flush Self-Hosted Trace generation */ >> if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR)) >> - __debug_save_trace(&vcpu->arch.host_debug_state.trfcr_el1); >> + __debug_save_trace(&vcpu->arch.host_debug_state.host_trfcr_el1); >> } > `<> >> void __debug_switch_to_guest(struct kvm_vcpu *vcpu) >> @@ -99,7 +99,7 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu) >> if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE)) >> __debug_restore_spe(vcpu->arch.host_debug_state.pmscr_el1); >> if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR)) >> - __debug_restore_trace(vcpu->arch.host_debug_state.trfcr_el1); >> + __debug_restore_trace(vcpu->arch.host_debug_state.host_trfcr_el1); >> } >> >> void __debug_switch_to_host(struct kvm_vcpu *vcpu) >> diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg >> index 992722c0c23b..295a4a625b8b 100644 >> --- a/arch/arm64/tools/sysreg >> +++ b/arch/arm64/tools/sysreg >> @@ -2661,3 +2661,7 @@ EndSysreg >> Sysreg TRFCR_EL2 3 4 1 2 1 >> Fields TRFCR_EL2 >> EndSysreg >> + >> +Sysreg TRFCR_EL12 3 5 1 2 1 >> +Fields TRFCR_ELx >> +EndSysreg > > Please move this to the first patch. > > M. > Will do. James

2 years

Re: [PATCH] coresight: etm4x: Remove bogous __exit annotation for some functions

by James Clark

On 29/09/2023 09:16, Uwe Kleine-König wrote: > etm4_platform_driver (which lives in ".data" contains a reference to > etm4_remove_platform_dev(). So the latter must not be marked with __exit > which results in the function being discarded for a build with > CONFIG_CORESIGHT_SOURCE_ETM4X=y which in turn makes the remove pointer > contain invalid data. > > etm4x_amba_driver referencing etm4_remove_amba() has the same issue. > > Drop the __exit annotations for the two affected functions and a third > one that is called by the other two. > Makes sense to me, I can't see any other struct platform_driver callbacks marked with __init or __exit. Reviewed-by: James Clark <james.clark(a)arm.com> > For reasons I don't understand this isn't catched by building with > CONFIG_DEBUG_SECTION_MISMATCH=y. > > Fixes: c23bc382ef0e ("coresight: etm4x: Refactor probing routine") > Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig(a)pengutronix.de> > --- > drivers/hwtracing/coresight/coresight-etm4x-core.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index 77b0271ce6eb..34aee59dd147 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -2224,7 +2224,7 @@ static void clear_etmdrvdata(void *info) > per_cpu(delayed_probe, cpu) = NULL; > } > > -static void __exit etm4_remove_dev(struct etmv4_drvdata *drvdata) > +static void etm4_remove_dev(struct etmv4_drvdata *drvdata) > { > bool had_delayed_probe; > /* > @@ -2253,7 +2253,7 @@ static void __exit etm4_remove_dev(struct etmv4_drvdata *drvdata) > } > } > > -static void __exit etm4_remove_amba(struct amba_device *adev) > +static void etm4_remove_amba(struct amba_device *adev) > { > struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev); > > @@ -2261,7 +2261,7 @@ static void __exit etm4_remove_amba(struct amba_device *adev) > etm4_remove_dev(drvdata); > } > > -static int __exit etm4_remove_platform_dev(struct platform_device *pdev) > +static int etm4_remove_platform_dev(struct platform_device *pdev) > { > struct etmv4_drvdata *drvdata = dev_get_drvdata(&pdev->dev); > > > base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d

2 years

[PATCH v1 0/5] kvm/coresight: Support exclude guest and exclude host

by James Clark

This is a combination of the RFC for nVHE here [1] and v3 of VHE version here [2]. After a few of the review comments it seemed much simpler for both versions to use the same interface and be in the same patchset. 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. nVHE: 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. With this change, the guest filtering rules from the Perf session are 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. The first commit moves the register to sysreg because I add the EL12 version in a later commit. --- Changes since nVHE RFC [1]: * Re-write just in terms of the register value to be written for the host and the guest. This removes some logic from the hyp code and a value of kvm_vcpu_arch:trfcr_el1 = 0 no longer means "don't restore". * Remove all the conditional compilation and new files. * Change the kvm_etm_update_vcpu_events macro to a function. * Re-use DEBUG_STATE_SAVE_TRFCR so iflags don't need to be expanded anymore. * Expand the cover letter. Changes since VHE v3 [2]: * Use the same interface as nVHE mode so TRFCR_EL12 is now written by kvm. [1]: https://lore.kernel.org/kvmarm/20230804101317.460697-1-james.clark@arm.com/ [2]: https://lore.kernel.org/kvmarm/20230905102117.2011094-1-james.clark@arm.com/ James Clark (5): arm64/sysreg: Move TRFCR definitions to sysreg arm64: KVM: Rename DEBUG_STATE_SAVE_TRBE to DEBUG_STATE_SAVE_TRFCR arm64: KVM: Add interface to set guest value for TRFCR register arm64: KVM: Write TRFCR value on guest switch with nVHE coresight: Pass guest TRFCR value to KVM arch/arm64/include/asm/kvm_host.h | 10 +++- arch/arm64/include/asm/sysreg.h | 12 ---- arch/arm64/kvm/arm.c | 1 + arch/arm64/kvm/debug.c | 38 +++++++++++- arch/arm64/kvm/hyp/nvhe/debug-sr.c | 60 +++++++++++-------- arch/arm64/tools/sysreg | 41 +++++++++++++ .../coresight/coresight-etm4x-core.c | 42 +++++++++++-- drivers/hwtracing/coresight/coresight-etm4x.h | 2 +- drivers/hwtracing/coresight/coresight-priv.h | 3 + 9 files changed, 160 insertions(+), 49 deletions(-) -- 2.34.1

2 years, 1 month

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight