| 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: <aacf3c7c-8a47-4d9f-95ce-58dcbc94a66c@arm.com>
Date: Wed, 7 Aug 2024 15:59:51 +0100
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 v2 2/3] coresight: Add source filtering for multi-port
output
On 18/07/2024 03:22, Tao Zhang wrote:
>
> On 7/11/2024 9:05 PM, Suzuki K Poulose wrote:
>> On 11/07/2024 09:17, Tao Zhang wrote:
>>> In order to enable the output ports of multi-port output devices,
>>
>> This has nothing to do with multi-port. You wanted to pain the picture
>> of a multi-port device, while the reality is different.
>>
>>> such as static replicator, to correspond to designated sources,
>>> a mechanism for filtering data sources is introduced for the
>>> output ports.
>>>
>>> The specified source will be marked like below in the Devicetree.
>>> test-replicator {
>>> ... ... ... ...
>>> out-ports {
>>> ... ... ... ...
>>> port@0 {
>>> reg = <0>;
>>> xxx: endpoint {
>>> remote-endpoint = <&xxx>;
>>> filter_src = <&xxx>; <-- To specify the source to
>>> }; be filtered out here.
>>> };
>>>
>>> port@1 {
>>> reg = <1>;
>>> yyy: endpoint {
>>> remote-endpoint = <&yyy>;
>>> filter_src = <&yyy>; <-- To specify the source to
>>> }; be filtered out here.
>>> };
>>> };
>>> };
>>>
>>> Then driver will find the expected source marked in the Devicetree, and
>>> save it to the coresight path. When the function needs to filter the
>>> source, it could obtain it from coresight path parameter. Finally,
>>> the output port knows which source it corresponds to, and it also knows
>>> which input port it corresponds to.
>>
>> Minor nit: I think the commit description is full of "How" you are
>> doing something, while it must rather be: "What and Why?"
>>
>> I would prefer something like :
>>
>> Subject: coresight: Add support for trace filtering by source
>>
>> 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.
>>
> I will update according to your comments to the next version.
>>
>>>
>>> Signed-off-by: Tao Zhang <quic_taozha@...cinc.com>
>>> ---
>>> drivers/hwtracing/coresight/coresight-core.c | 125 ++++++++++++++++--
>>> .../hwtracing/coresight/coresight-platform.c | 18 +++
>>> include/linux/coresight.h | 5 +
>>> 3 files changed, 135 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c
>>> b/drivers/hwtracing/coresight/coresight-core.c
>>> index 9fc6f6b863e0..3f02a31b9328 100644
>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>> @@ -75,15 +75,60 @@ 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 (csdev->type != CORESIGHT_DEV_TYPE_SOURCE)
>>> + return NULL;
>>> +
>>> + return csdev;
>>> +}
>>> +
>>
>> Why is this still here ? Didn't agree to remove this and pass the source
>> directly ?
>
> I agree pass the source directly, but how can we get the source device
> of the path
>
> without this function? The solution I can think of is to pass source
> directly from
>
> "coresight_enable"like below.
>
> coresight_enable ---pass source---> coresight_enable_path ->
> coresight_enable_link
>
> -> coresight_find_out_connection -> coresight_blocks_source
>
> Does this solution meet your expectations? Or do you have a better
> suggestion?
>
>>
>>> +/**
>>> + * coresight_source_filter - checks whether the connection matches
>>> the source
>>> + * of path if connection is binded to specific source.
>>> + * @trace_source: The source device of the trace path
>>> + * @conn: The connection of one outport
>>> + *
>>> + * Return zero if the connection doesn't have a source binded or
>>> source of the
>>> + * path matches the source binds to connection.
>>> + */
>>> +static int coresight_source_filter(struct coresight_device
>>> *trace_source,
>>> + struct coresight_connection *conn)
>>
>> This name is a bit confusing. It doesn't tell you, whether it
>> allows the trace or blocks it.
>>
>>> +{
>>> + int ret = 0;
>>> +
>>> + if (!conn->filter_src_dev)
>>> + return ret;
>>
>> This is not the correct check. It must be :
>>
>> if (!conn->filter_src_fwnode)
>>
>> Because, the device could have disappeared (or not appeared yet).
>> e.g., (TPDM) driver module unloaded/not - loaded.
>>
>>> +
>>> + if (!trace_source)
>>
>> Is that possible ?
>>
>>> + return -1;
>>> +
>>> + if (conn->filter_src_dev == trace_source)
>>> + ret = 0;
>>> + else
>>> + ret = -1;
>>
>> Couldn't this simply be :
>>
>> /*
>> * Returns true, if the trace path is not possible through @conn
>> * for trace originating from @src
>> */
>> static bool coresight_blocks_source(src, conn)
>> {
>> return conn->filter_src_fwnode &&
>> (conn->filter_src_dev != src);
>> }
> I will update to the next version.
>>
>>> +
>>> + return ret;
>>> +}
>>> +
>>> static struct coresight_connection *
>>> coresight_find_out_connection(struct coresight_device *src_dev,
>>> - struct coresight_device *dest_dev)
>>> + struct coresight_device *dest_dev,
>>> + struct coresight_device *trace_source)
>>
>> Please could you rename :
>>
>> src_dev => csdev
>> dest_dev => out_dev
>> trace_source => trace_src ?
>>
>> Having src_dev and trace_source in the same list is confusing.
> I will update to the next version.
>>
>>> {
>>> int i;
>>> struct coresight_connection *conn;
>>> for (i = 0; i < src_dev->pdata->nr_outconns; i++) {
>>> conn = src_dev->pdata->out_conns[i];
>>> + if (coresight_source_filter(trace_source, conn))
>>
>> if (coresight_blocks_source(trace_source, conn))
>> continue;
>>
>>> + continue;
>>> if (conn->dest_dev == dest_dev)
>>> return conn;
>>> }
>>> @@ -251,7 +296,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 *trace_source)
>>> {
>>> int link_subtype;
>>> struct coresight_connection *inconn, *outconn;
>>> @@ -259,8 +305,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,
>>> trace_source);
>>> + outconn = coresight_find_out_connection(csdev, child,
>>> trace_source);
>>> link_subtype = csdev->subtype.link_subtype;
>>> if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG &&
>>> IS_ERR(inconn))
>>> @@ -273,15 +319,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 *trace_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,
>>> trace_source);
>>> + outconn = coresight_find_out_connection(csdev, child,
>>> trace_source);
>>> link_ops(csdev)->disable(csdev, inconn, outconn);
>>> }
>>> @@ -341,6 +388,9 @@ static void coresight_disable_path_from(struct
>>> list_head *path,
>>> {
>>> u32 type;
>>> struct coresight_device *csdev, *parent, *child;
>>> + struct coresight_device *source;
>>> +
>>> + source = coresight_get_source(path);
>>
>> Grr! This must come from the "caller of the
>> coresight_disable_path_from()", which was my comment. Please go back
>> and double check the comment on previous version.
>
> How about if I pass the source to " coresight_disable_path_from"
> directly from
>
> "coresight_enable" or "coresight_disable" like below?
>
> coresight_enable ----| |--->
> coresight_disable_patch_from
>
> |---pass source---|
>
> coresight_disable----| |--->
> coresight_disable_path -> coresight_disable_patch_from
>
> This can ensure that the source passed into coresight_disable_patch_from is
>
> the correct source.
coresight_disable_path_from() must be given the "source" separately,
as the "path" could be "partial". Rest could stay as it is in your
patch.
>
>>
>>
>>> if (!nd)
>>> nd = list_first_entry(path, struct coresight_node, link);
>>> @@ -375,7 +425,7 @@ 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, source);
>>> break;
>>> default:
>>> break;
>>> @@ -418,6 +468,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;
>>> @@ -456,7 +509,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 +672,7 @@ static void coresight_drop_device(struct
>>> coresight_device *csdev)
>>> * @csdev: The device to start from.
>>> * @sink: The final sink we want in this path.
>>> * @path: The list to add devices to.
>>> + * @trace_source: The trace source device of the path.
>>> *
>>> * The tree of Coresight device is traversed until @sink is found.
>>> * From there the sink is added to the list along with all the
>>> devices that led
>>> @@ -627,7 +681,8 @@ static void coresight_drop_device(struct
>>> coresight_device *csdev)
>>> */
>>> static int _coresight_build_path(struct coresight_device *csdev,
>>> struct coresight_device *sink,
>>> - struct list_head *path)
>>> + struct list_head *path,
>>> + struct coresight_device *trace_source)
>>> {
>>> int i, ret;
>>> bool found = false;
>>> @@ -639,7 +694,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, sink, path, trace_source) ==
>>> 0) {
>>> found = true;
>>> goto out;
>>> }
>>> @@ -650,8 +705,13 @@ static int _coresight_build_path(struct
>>> coresight_device *csdev,
>>> struct coresight_device *child_dev;
>>> child_dev = csdev->pdata->out_conns[i]->dest_dev;
>>> +
>>> + if (csdev->pdata->out_conns[i]->filter_src_dev
>>> + && (csdev->pdata->out_conns[i]->filter_src_dev !=
>>> trace_source))
>>> + continue;
>>> +
>>> if (child_dev &&
>>> - _coresight_build_path(child_dev, sink, path) == 0) {
>>> + _coresight_build_path(child_dev, sink, path,
>>> trace_source) == 0) {
>>> found = true;
>>> break;
>>> }
>>> @@ -696,7 +756,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, sink, path, source);
>>> if (rc) {
>>> kfree(path);
>>> return ERR_PTR(rc);
>>> @@ -957,6 +1017,14 @@ static int coresight_orphan_match(struct device
>>> *dev, void *data)
>>> /* This component still has an orphan */
>>> still_orphan = true;
>>> }
>>
>>
>>> + if ((conn->filter_src_fwnode) && dst_csdev
>>> + && (conn->filter_src_fwnode == dst_csdev->dev.fwnode)) {
>>> + conn->filter_src_dev = dst_csdev;
>>> + if (conn->filter_src_dev->type
>>> + != CORESIGHT_DEV_TYPE_SOURCE)
>>> + dev_warn(&conn->filter_src_dev->dev,
>>> + "Filter source is not a source device\n");
>>> + }
>>
>> Have you tested your code with the following order :
>>
>> 1) Build everything as module
>> 2) Load the replicator driver.
>> 3) Load the TPDM driver
>>
>> And are you able to get the trace working ? I suspect it doesn't work.
>> The csdev->orphan must be used to track if there is any missing link
>> with unresolved "filter_src_dev".
>
> I really didn't consider this test case in my testing. There was a
> problem with
>
> the logic of the code I modified. If it is modified to the following,
> can it solve the
>
> test case you proposed?
>
> 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];
>
> /*Add the following code to fix filter source device before
> skip the port*/
> + if ((conn->filter_src_fwnode) && dst_csdev
> + && (conn->filter_src_fwnode == dst_csdev->dev.fwnode)) {
> + conn->filter_src_dev = dst_csdev;
> + if (conn->filter_src_dev->type
> + != CORESIGHT_DEV_TYPE_SOURCE)
> + dev_warn(&conn->filter_src_dev->dev,
> + "Filter source is not a source device\n");
And we should not set the device as the filter_src_dev in this case.
> + }
>
> /* Skip the port if it's already connected. */
> if (conn->dest_dev)
> continue;
> ... ... ...
> ret = coresight_add_in_conn(conn);
> if (ret)
> return ret;
>
> /* Add the following code to make sure the device still
> orphan if there is a
>
> * filter source firmware node but the filter source is
> null.*/
>
> + if (conn->filter_src_fwnode && !conn->filter_src_dev)
> + still_orphan = true;
> } else {
> /* This component still has an orphan */
> still_orphan = true;
> }
> }
>
> src_csdev->orphan = still_orphan;
> ... ... ...
> }
>
>>
>>
>>> }
>>> src_csdev->orphan = still_orphan;
>>> @@ -974,6 +1042,30 @@ static int coresight_fixup_orphan_conns(struct
>>> coresight_device *csdev)
>>> csdev, coresight_orphan_match);
>>> }
>>> +/* reset_filter_src - Reset the filter source if the source is
>>> being removed */
>>> +static int reset_filter_src(struct coresight_device *trace_source,
>>> + struct coresight_connection *conn)
>>> +{
>>> + int i;
>>> + int ret = -1;
>>> + struct coresight_device *csdev;
>>> + struct coresight_connection *output_conn;
>>
>> I don't think this is sufficient. I would rather walk through the
>> entire coresight bus and fixup the filter_src_dev, rather than
>> traversing the
>> paths. e.g., one of the devices in the path could have been removed.
>> See coresight_clear_default_sink() for e.g. You should have something
>> similar :
>>
>> coresight_clear_filter_source();
>
> That's right. My current understanding is that there are two cases that
> need to be considered.
>
> #1 Unregister the source device
>
> In this case, we need to walk through the entire coresight bus and fixup
> the filter_src_dev
>
> #2 Unregister the replicator
>
> Iterate over all output connections to set filter_src_dev and
> filter_src_fwnode to NULL.
>
> Is this logically correct? I have attached my sample code below, please
> help review it.
>
> +static void 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;
> + }
>
> +}
That looks good to me.
>
> /* 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 (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) //walk through the
> entire coresight bus and fixup the filter_src_dev
> + 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) { //Iterate over all output
> connections to set filter_src_dev and filter_src_fwnode to NULL.
> + conn->filter_src_dev = NULL;
> + conn->filter_src_fwnode = NULL;
We may need to "drop" the refcount for this one ?
Suzuki
> + }
>
> if (!conn->dest_dev)
> continue;
>
> ... ... ...
> }
>
>
> Best,
>
> Tao
>
>>
>>
>> Suzuki
>>
>>
>>
>>> +
>>> + csdev = conn->dest_dev;
>>> + for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>>> + output_conn = csdev->pdata->out_conns[i];
>>> + if (output_conn->filter_src_dev
>>> + && (output_conn->filter_src_dev == trace_source)) {
>>> + output_conn->filter_src_dev = NULL;
>>> + return 0;
>>> + }
>>> + ret = reset_filter_src(trace_source, output_conn);
>>> + if (!ret)
>>> + return ret;
>>> + }
>>> + return ret;
>>> +}
>>> +
>>> /* coresight_remove_conns - Remove other device's references to
>>> this device */
>>> static void coresight_remove_conns(struct coresight_device *csdev)
>>> {
>>> @@ -986,6 +1078,13 @@ static void coresight_remove_conns(struct
>>> coresight_device *csdev)
>>> */
>>
>>
>>> for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>>> conn = csdev->pdata->out_conns[i];
>>> + if (csdev->type == CORESIGHT_DEV_TYPE_SOURCE)
>>> + reset_filter_src(csdev, conn);
>>> + if (conn->filter_src_fwnode) {
>>> + conn->filter_src_dev = NULL;
>>> + conn->filter_src_fwnode = NULL;
>>> + }
>>> +
>>> if (!conn->dest_dev)
>>> continue;
>>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c
>>> b/drivers/hwtracing/coresight/coresight-platform.c
>>> index 64e171eaad82..b3c3e2361d07 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-src", 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 && (conn.filter_src_dev->type
>>> + != CORESIGHT_DEV_TYPE_SOURCE))
>>> + dev_warn(&conn.filter_src_dev->dev,
>>> + "Filter source %s is not a source device\n");
>>> + }
>>> +
>>> 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 f09ace92176e..91a689b4514b 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