| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8aeb8ca9-5d3f-4154-bba5-1255b368bf1b@quicinc.com>
Date: Tue, 19 Nov 2024 16:58:50 +0800
From: Tao Zhang <quic_taozha@...cinc.com>
To: Suzuki K Poulose <suzuki.poulose@....com>,
Mike Leach
<mike.leach@...aro.org>, James Clark <james.clark@....com>,
Rob Herring
<robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Leo Yan
<leo.yan@...ux.dev>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>
CC: <coresight@...ts.linaro.org>, <linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v5 3/4] coresight: Add support for trace filtering by
source
On 11/18/2024 5:56 PM, Suzuki K Poulose wrote:
> On 18/11/2024 09:28, Tao Zhang wrote:
>>
>> On 11/15/2024 5:40 PM, Suzuki K Poulose wrote:
>>> On 15/11/2024 09:18, Tao Zhang wrote:
>>>>
>>>> On 11/13/2024 9:39 PM, Suzuki K Poulose wrote:
>>>>> On 30/10/2024 09:32, Tao Zhang wrote:
>>>>>> Some replicators have hard coded filtering of "trace" data, based
>>>>>> on the
>>>>>> source device. This is different from the trace filtering based on
>>>>>> TraceID, available in the standard programmable replicators. e.g.,
>>>>>> Qualcomm replicators have filtering based on custom trace protocol
>>>>>> format and is not programmable.
>>>>>>
>>>>>> The source device could be connected to the replicator via
>>>>>> intermediate
>>>>>> components (e.g., a funnel). Thus we need platform information from
>>>>>> the firmware tables to decide the source device corresponding to a
>>>>>> given output port from the replicator. Given this affects "trace
>>>>>> path building" and traversing the path back from the sink to source,
>>>>>> add the concept of "filtering by source" to the generic coresight
>>>>>> connection.
>>>>>>
>>>>>> The specified source will be marked like below in the Devicetree.
>>>>>> test-replicator {
>>>>>> ... ... ... ...
>>>>>> out-ports {
>>>>>> ... ... ... ...
>>>>>> port@0 {
>>>>>> reg = <0>;
>>>>>> xyz: endpoint {
>>>>>> remote-endpoint = <&zyx>;
>>>>>> filter-source = <&source_1>; <-- To specify the
>>>>>> source to
>>>>>> }; be filtered out here.
>>>>>> };
>>>>>>
>>>>>> port@1 {
>>>>>> reg = <1>;
>>>>>> abc: endpoint {
>>>>>> remote-endpoint = <&cba>;
>>>>>> filter-source = <&source_2>; <-- To specify the
>>>>>> source to
>>>>>> }; be filtered out here.
>>>>>> };
>>>>>> };
>>>>>> };
>>>>>>
>>>>>> Signed-off-by: Tao Zhang <quic_taozha@...cinc.com>
>>>>>> ---
>>>>>> drivers/hwtracing/coresight/coresight-core.c | 113
>>>>>> +++++++++++++ ++---
>>>>>> .../hwtracing/coresight/coresight-platform.c | 18 +++
>>>>>> include/linux/coresight.h | 5 +
>>>>>> 3 files changed, 117 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/
>>>>>> drivers/hwtracing/coresight/coresight-core.c
>>>>>> index ea38ecf26fcb..0a9380350fb5 100644
>>>>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>>>>> @@ -75,22 +75,54 @@ struct coresight_device
>>>>>> *coresight_get_percpu_sink(int cpu)
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(coresight_get_percpu_sink);
>>>>>> +static struct coresight_device *coresight_get_source(struct
>>>>>> list_head *path)
>>>>>> +{
>>>>>> + struct coresight_device *csdev;
>>>>>> +
>>>>>> + if (!path)
>>>>>> + return NULL;
>>>>>> +
>>>>>> + csdev = list_first_entry(path, struct coresight_node, link)-
>>>>>> >csdev;
>>>>>> + if (!coresight_is_device_source(csdev))
>>>>>> + return NULL;
>>>>>> +
>>>>>> + return csdev;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * coresight_blocks_source - checks whether the connection
>>>>>> matches the source
>>>>>> + * of path if connection is bound to specific source.
>>>>>> + * @src: The source device of the trace path
>>>>>> + * @conn: The connection of one outport
>>>>>> + *
>>>>>> + * Return false if the connection doesn't have a source binded
>>>>>> or source of the
>>>>>> + * path matches the source binds to connection.
>>>>>> + */
>>>>>> +static bool coresight_blocks_source(struct coresight_device *src,
>>>>>> + struct coresight_connection *conn)
>>>>>> +{
>>>>>> + return conn->filter_src_fwnode && (conn->filter_src_dev !=
>>>>>> src);
>>>>>> +}
>>>>>> +
>>>>>> static struct coresight_connection *
>>>>>> -coresight_find_out_connection(struct coresight_device *src_dev,
>>>>>> - struct coresight_device *dest_dev)
>>>>>> +coresight_find_out_connection(struct coresight_device *csdev,
>>>>>> + struct coresight_device *out_dev,
>>>>>> + struct coresight_device *trace_src)
>>>>>> {
>>>>>> int i;
>>>>>> struct coresight_connection *conn;
>>>>>> - for (i = 0; i < src_dev->pdata->nr_outconns; i++) {
>>>>>> - conn = src_dev->pdata->out_conns[i];
>>>>>> - if (conn->dest_dev == dest_dev)
>>>>>> + for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>>>>>> + conn = csdev->pdata->out_conns[i];
>>>>>> + if (coresight_blocks_source(trace_src, conn))
>>>>>> + continue;
>>>>>> + if (conn->dest_dev == out_dev)
>>>>>> return conn;
>>>>>> }
>>>>>> - dev_err(&src_dev->dev,
>>>>>> - "couldn't find output connection, src_dev: %s, dest_dev:
>>>>>> %s\n",
>>>>>> - dev_name(&src_dev->dev), dev_name(&dest_dev->dev));
>>>>>> + dev_err(&csdev->dev,
>>>>>> + "couldn't find output connection, csdev: %s, out_dev:
>>>>>> %s\n",
>>>>>> + dev_name(&csdev->dev), dev_name(&out_dev->dev));
>>>>>> return ERR_PTR(-ENODEV);
>>>>>> }
>>>>>> @@ -251,7 +283,8 @@ static void coresight_disable_sink(struct
>>>>>> coresight_device *csdev)
>>>>>> static int coresight_enable_link(struct coresight_device *csdev,
>>>>>> struct coresight_device *parent,
>>>>>> - struct coresight_device *child)
>>>>>> + struct coresight_device *child,
>>>>>> + struct coresight_device *source)
>>>>>> {
>>>>>> int link_subtype;
>>>>>> struct coresight_connection *inconn, *outconn;
>>>>>> @@ -259,8 +292,8 @@ static int coresight_enable_link(struct
>>>>>> coresight_device *csdev,
>>>>>> if (!parent || !child)
>>>>>> return -EINVAL;
>>>>>> - inconn = coresight_find_out_connection(parent, csdev);
>>>>>> - outconn = coresight_find_out_connection(csdev, child);
>>>>>> + inconn = coresight_find_out_connection(parent, csdev, source);
>>>>>> + outconn = coresight_find_out_connection(csdev, child, source);
>>>>>> link_subtype = csdev->subtype.link_subtype;
>>>>>> if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG &&
>>>>>> IS_ERR(inconn))
>>>>>> @@ -273,15 +306,16 @@ static int coresight_enable_link(struct
>>>>>> coresight_device *csdev,
>>>>>> static void coresight_disable_link(struct coresight_device
>>>>>> *csdev,
>>>>>> struct coresight_device *parent,
>>>>>> - struct coresight_device *child)
>>>>>> + struct coresight_device *child,
>>>>>> + struct coresight_device *source)
>>>>>> {
>>>>>> struct coresight_connection *inconn, *outconn;
>>>>>> if (!parent || !child)
>>>>>> return;
>>>>>> - inconn = coresight_find_out_connection(parent, csdev);
>>>>>> - outconn = coresight_find_out_connection(csdev, child);
>>>>>> + inconn = coresight_find_out_connection(parent, csdev, source);
>>>>>> + outconn = coresight_find_out_connection(csdev, child, source);
>>>>>> link_ops(csdev)->disable(csdev, inconn, outconn);
>>>>>> }
>>>>>> @@ -375,7 +409,8 @@ static void
>>>>>> coresight_disable_path_from(struct list_head *path,
>>>>>> case CORESIGHT_DEV_TYPE_LINK:
>>>>>> parent = list_prev_entry(nd, link)->csdev;
>>>>>> child = list_next_entry(nd, link)->csdev;
>>>>>> - coresight_disable_link(csdev, parent, child);
>>>>>> + coresight_disable_link(csdev, parent, child,
>>>>>> + coresight_get_source(path));
>>>>>> break;
>>>>>> default:
>>>>>> break;
>>>>>> @@ -418,7 +453,9 @@ int coresight_enable_path(struct list_head
>>>>>> *path, enum cs_mode mode,
>>>>>> u32 type;
>>>>>> struct coresight_node *nd;
>>>>>> struct coresight_device *csdev, *parent, *child;
>>>>>> + struct coresight_device *source;
>>>>>> + source = coresight_get_source(path);
>>>>>> list_for_each_entry_reverse(nd, path, link) {
>>>>>> csdev = nd->csdev;
>>>>>> type = csdev->type;
>>>>>> @@ -456,7 +493,7 @@ int coresight_enable_path(struct list_head
>>>>>> *path, enum cs_mode mode,
>>>>>> case CORESIGHT_DEV_TYPE_LINK:
>>>>>> parent = list_prev_entry(nd, link)->csdev;
>>>>>> child = list_next_entry(nd, link)->csdev;
>>>>>> - ret = coresight_enable_link(csdev, parent, child);
>>>>>> + ret = coresight_enable_link(csdev, parent, child,
>>>>>> source);
>>>>>> if (ret)
>>>>>> goto err;
>>>>>> break;
>>>>>> @@ -619,6 +656,7 @@ static void coresight_drop_device(struct
>>>>>> coresight_device *csdev)
>>>>>> /**
>>>>>> * _coresight_build_path - recursively build a path from a
>>>>>> @csdev to a sink.
>>>>>> * @csdev: The device to start from.
>>>>>> + * @source: The trace source device of the path.
>>>>>> * @sink: The final sink we want in this path.
>>>>>> * @path: The list to add devices to.
>>>>>> *
>>>>>> @@ -628,6 +666,7 @@ static void coresight_drop_device(struct
>>>>>> coresight_device *csdev)
>>>>>> * the source is the first device and the sink the last one.
>>>>>> */
>>>>>> static int _coresight_build_path(struct coresight_device *csdev,
>>>>>> + struct coresight_device *source,
>>>>>> struct coresight_device *sink,
>>>>>> struct list_head *path)
>>>>>> {
>>>>>> @@ -641,7 +680,7 @@ static int _coresight_build_path(struct
>>>>>> coresight_device *csdev,
>>>>>> if (coresight_is_percpu_source(csdev) &&
>>>>>> coresight_is_percpu_sink(sink) &&
>>>>>> sink == per_cpu(csdev_sink, source_ops(csdev)-
>>>>>> >cpu_id(csdev))) {
>>>>>> - if (_coresight_build_path(sink, sink, path) == 0) {
>>>>>> + if (_coresight_build_path(sink, source, sink, path) == 0) {
>>>>>> found = true;
>>>>>> goto out;
>>>>>> }
>>>>>> @@ -652,8 +691,12 @@ static int _coresight_build_path(struct
>>>>>> coresight_device *csdev,
>>>>>> struct coresight_device *child_dev;
>>>>>> child_dev = csdev->pdata->out_conns[i]->dest_dev;
>>>>>> +
>>>>>> + if (coresight_blocks_source(source, csdev->pdata-
>>>>>> >out_conns[i]))
>>>>>> + continue;
>>>>>> +
>>>>>> if (child_dev &&
>>>>>> - _coresight_build_path(child_dev, sink, path) == 0) {
>>>>>> + _coresight_build_path(child_dev, source, sink, path)
>>>>>> == 0) {
>>>>>> found = true;
>>>>>> break;
>>>>>> }
>>>>>> @@ -698,7 +741,7 @@ struct list_head *coresight_build_path(struct
>>>>>> coresight_device *source,
>>>>>> INIT_LIST_HEAD(path);
>>>>>> - rc = _coresight_build_path(source, sink, path);
>>>>>> + rc = _coresight_build_path(source, source, sink, path);
>>>>>> if (rc) {
>>>>>> kfree(path);
>>>>>> return ERR_PTR(rc);
>>>>>> @@ -927,6 +970,16 @@ static int coresight_orphan_match(struct
>>>>>> device *dev, void *data)
>>>>>> for (i = 0; i < src_csdev->pdata->nr_outconns; i++) {
>>>>>> conn = src_csdev->pdata->out_conns[i];
>>>>>> + /* Fix filter source device before skip the port */
>>>>>> + if (conn->filter_src_fwnode && !conn->filter_src_dev) {
>>>>>> + if (dst_csdev &&
>>>>>> + (conn->filter_src_fwnode ==
>>>>>> dst_csdev->dev.fwnode) &&
>>>>>> + !WARN_ON_ONCE(!coresight_is_device_source(dst_csdev)))
>>>>>> + conn->filter_src_dev = dst_csdev;
>>>>>> + else
>>>>>> + still_orphan = true;
>>>>>> + }
>>>>>> +
>>>>>> /* Skip the port if it's already connected. */
>>>>>> if (conn->dest_dev)
>>>>>> continue;
>>>>>> @@ -977,18 +1030,40 @@ static int
>>>>>> coresight_fixup_orphan_conns(struct coresight_device *csdev)
>>>>>> csdev, coresight_orphan_match);
>>>>>> }
>>>>>> +static int coresight_clear_filter_source(struct device *dev,
>>>>>> void *data)
>>>>>> +{
>>>>>> + int i;
>>>>>> + struct coresight_device *source = data;
>>>>>> + struct coresight_device *csdev = to_coresight_device(dev);
>>>>>> +
>>>>>> + for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
>>>>>> + if (csdev->pdata->out_conns[i]->filter_src_dev == source)
>>>>>> + csdev->pdata->out_conns[i]->filter_src_dev = NULL;
>>>>>> + }
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> /* coresight_remove_conns - Remove other device's references to
>>>>>> this device */
>>>>>> static void coresight_remove_conns(struct coresight_device *csdev)
>>>>>> {
>>>>>> int i, j;
>>>>>> struct coresight_connection *conn;
>>>>>> + if (coresight_is_device_source(csdev))
>>>>>> + bus_for_each_dev(&coresight_bustype, NULL, csdev,
>>>>>> + coresight_clear_filter_source);
>>>>>> +
>>>>>> /*
>>>>>> * Remove the input connection references from the
>>>>>> destination device
>>>>>> * for each output connection.
>>>>>> */
>>>>>> for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>>>>>> conn = csdev->pdata->out_conns[i];
>>>>>> + if (conn->filter_src_fwnode) {
>>>>>> + conn->filter_src_dev = NULL;
>>>>>> + fwnode_handle_put(conn->filter_src_fwnode);
>>>>>> + }
>>>>>> +
>>>>>> if (!conn->dest_dev)
>>>>>> continue;
>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c
>>>>>> b/ drivers/hwtracing/coresight/coresight-platform.c
>>>>>> index 64e171eaad82..d5532caa9e92 100644
>>>>>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>>>>>> @@ -243,6 +243,24 @@ static int
>>>>>> of_coresight_parse_endpoint(struct device *dev,
>>>>>> conn.dest_fwnode = fwnode_handle_get(rdev_fwnode);
>>>>>> conn.dest_port = rendpoint.port;
>>>>>> + /*
>>>>>> + * Get the firmware node of the filter source through the
>>>>>> + * reference. This could be used to filter the source in
>>>>>> + * building path.
>>>>>> + */
>>>>>> + conn.filter_src_fwnode =
>>>>>> + fwnode_find_reference(&ep->fwnode, "filter-source", 0);
>>>>>> + if (IS_ERR(conn.filter_src_fwnode)) {
>>>>>> + conn.filter_src_fwnode = NULL;
>>>>>> + } else {
>>>>>> + conn.filter_src_dev =
>>>>>> + coresight_find_csdev_by_fwnode(conn.filter_src_fwnode);
>>>>>> + if (conn.filter_src_dev &&
>>>>>> + !coresight_is_device_source(conn.filter_src_dev))
>>>>>> + dev_warn(&conn.filter_src_dev->dev,
>>>>>> + "Filter source is not a source device\n");
>>>>>
>>>>> Do we need to reset the conn.filter_src_fwnode ? Otherwise, we can
>>>>> warn from other places too.
>>>>
>>>> Agree, we need to reset conn.filter_src_fwnode and
>>>> conn.filter_src_dev here if it is not a SOURCE
>>>>
>>>> type device.
>>>>
>>>> I will update the following code to the next patch series. Could
>>>> you help check if it is fine to you?
>>>
>>> This looks good.
>>>
>>>>
>>>> + if (conn.filter_src_dev &&
>>>> + !coresight_is_device_source(conn.filter_src_dev)) {
>>>> + dev_warn(&conn.filter_src_dev->dev,
>>>> + "Filter source is not a source device\n");
>>>
>>> minor nit: It would be good to indicate which device has this
>>> "filtering". i.e,
>>> dev_warn(dev, "Filter source is not a trace source :
>>> %s\n", dev_name(&conn.filter_src_dev->dev));
>> Sure, I will update this to the next patch series.
>
> Also, it may be helpful, if we specify the "port number" which has this
> wrong handle. I guess something like:
>
> dev_warn(dev, "port %d: Filter handle is not a trace source : %s\n",
> conn.src_port, dev_name(&conn.filter_src_dev->dev));
Sure, I will update this to the next patch series.
Best,
Tao
>
> Suzuki
>
>>>
>>>> + conn.filter_src_dev = NULL；
>>>> + conn.filter_src_dev = NULL;
>>
>> Sorry for my mistake here. I also want to set "filter_src_fwnode " to
>> NULL if it is not a trace source.
>>
>> conn.filter_src_fwnode = NULL;
>
>
>>
>>
>> Best,
>>
>> Tao
>>
>>>
>>> Duplicated line ^
>>>
>>> Suzuki
>>>
>>>> + }
>>>>
>>>> Best,
>>>>
>>>> Tao
>>>>
>>>>>
>>>>> Suzuki
>>>>>
>>>>>> + }
>>>>>> +
>>>>>> new_conn = coresight_add_out_conn(dev, pdata, &conn);
>>>>>> if (IS_ERR_VALUE(new_conn)) {
>>>>>> fwnode_handle_put(conn.dest_fwnode);
>>>>>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>>>>>> index 9311df8538fc..f372c01ae2fc 100644
>>>>>> --- a/include/linux/coresight.h
>>>>>> +++ b/include/linux/coresight.h
>>>>>> @@ -172,6 +172,9 @@ struct coresight_desc {
>>>>>> * @dest_dev: a @coresight_device representation of the
>>>>>> component
>>>>>> connected to @src_port. NULL until the device is created
>>>>>> * @link: Representation of the connection as a sysfs link.
>>>>>> + * @filter_src_fwnode: filter source component's fwnode handle.
>>>>>> + * @filter_src_dev: a @coresight_device representation of the
>>>>>> component that
>>>>>> + needs to be filtered.
>>>>>> *
>>>>>> * The full connection structure looks like this, where
>>>>>> in_conns store
>>>>>> * references to same connection as the source device's out_conns.
>>>>>> @@ -200,6 +203,8 @@ struct coresight_connection {
>>>>>> struct coresight_device *dest_dev;
>>>>>> struct coresight_sysfs_link *link;
>>>>>> struct coresight_device *src_dev;
>>>>>> + struct fwnode_handle *filter_src_fwnode;
>>>>>> + struct coresight_device *filter_src_dev;
>>>>>> atomic_t src_refcnt;
>>>>>> atomic_t dest_refcnt;
>>>>>> };
>>>>>
>>>
>
Powered by blists - more mailing lists