| 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: <94096b6e-1a4c-4fc4-8294-316b38b35e99@arm.com>
Date: Mon, 18 Nov 2024 09:56:09 +0000
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Tao Zhang <quic_taozha@...cinc.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 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));
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