| 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: <87mw3bewq8.fsf@ashishki-desk.ger.corp.intel.com>
Date: Tue, 17 Mar 2015 12:37:03 +0200
From: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
Pratik Patel <pratikp@...eaurora.org>, peter.lachner@...el.com,
norbert.schulz@...el.com, keven.boell@...el.com,
yann.fouassier@...el.com, laurent.fert@...el.com,
linux-api@...r.kernel.org
Subject: Re: [PATCH v0 01/11] stm class: Introduce an abstraction for System Trace Module devices
Mathieu Poirier <mathieu.poirier@...aro.org> writes:
> On 7 March 2015 at 04:35, Alexander Shishkin
> <alexander.shishkin@...ux.intel.com> wrote:
>> A System Trace Module (STM) is a device exporting data in System Trace
>> Protocol (STP) format as defined by MIPI STP standards. Examples of such
>> devices are Intel Trace Hub and Coresight STM.
>>
>> This abstraction provides a unified interface for software trace sources
>> to send their data over an STM device to a debug host. In order to do
>> that, such a trace source needs to be assigned a pair of master/channel
>> identifiers that all the data from this source will be tagged with. The
>> STP decoder on the debug host side will use these master/channel tags to
>> distinguish different trace streams from one another inside one STP
>> stream.
>>
>> This abstraction provides a configfs-based policy management mechanism
>> for dynamic allocation of these master/channel pairs based on trace
>> source-supplied string identifier. It has the flexibility of being
>> defined at runtime and at the same time (provided that the policy
>> definition is aligned with the decoding end) consistency.
>>
>> For userspace trace sources, this abstraction provides write()-based and
>> mmap()-based (if the underlying stm device allows this) output mechanism.
>>
>> For kernel-side trace sources, we provide "stm_source" device class that
>> can be connected to an stm device at run time.
>>
>> Cc: linux-api@...r.kernel.org
>> Cc: Pratik Patel <pratikp@...eaurora.org>
>> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
>> Signed-off-by: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
>> ---
>> Documentation/ABI/testing/configfs-stp-policy | 44 ++
>> Documentation/ABI/testing/sysfs-class-stm | 14 +
>> Documentation/ABI/testing/sysfs-class-stm_source | 11 +
>> Documentation/trace/stm.txt | 77 +++
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 1 +
>> drivers/stm/Kconfig | 8 +
>> drivers/stm/Makefile | 3 +
>> drivers/stm/core.c | 839 +++++++++++++++++++++++
>> drivers/stm/policy.c | 470 +++++++++++++
>> drivers/stm/stm.h | 77 +++
>> include/linux/stm.h | 87 +++
>> include/uapi/linux/stm.h | 47 ++
>> 13 files changed, 1680 insertions(+)
>> create mode 100644 Documentation/ABI/testing/configfs-stp-policy
>> create mode 100644 Documentation/ABI/testing/sysfs-class-stm
>> create mode 100644 Documentation/ABI/testing/sysfs-class-stm_source
>> create mode 100644 Documentation/trace/stm.txt
>> create mode 100644 drivers/stm/Kconfig
>> create mode 100644 drivers/stm/Makefile
>> create mode 100644 drivers/stm/core.c
>> create mode 100644 drivers/stm/policy.c
>> create mode 100644 drivers/stm/stm.h
>> create mode 100644 include/linux/stm.h
>> create mode 100644 include/uapi/linux/stm.h
>>
>
> [Snip...]
>
>> +
>> +static int stm_output_assign(struct stm_device *stm, unsigned int width,
>> + struct stp_policy_node *policy_node,
>> + struct stm_output *output)
>> +{
>> + unsigned int midx, cidx, mend, cend;
>> + int ret = -EBUSY;
>> +
>> + if (width > stm->data->sw_nchannels)
>> + return -EINVAL;
>> +
>> + if (policy_node) {
>
> Where does this get set? All I found is code that is switching on it.
It comes from stp_policy_node_lookup() in stm_file_assign() or
stm_source_link_add().
>> + stp_policy_node_get_ranges(policy_node,
>> + &midx, &mend, &cidx, &cend);
>> + } else {
>> + midx = stm->data->sw_start;
>> + cidx = 0;
>> + mend = stm->data->sw_end;
>> + cend = stm->data->sw_nchannels - 1;
>> + }
>> +
>> + spin_lock(&stm->mc_lock);
>> + if (output->nr_chans)
>> + goto unlock;
>> +
>> + ret = stm_find_master_chan(stm, width, &midx, mend, &cidx, cend);
>> + if (ret)
>> + goto unlock;
>> +
>> + output->master = midx;
>> + output->channel = cidx;
>> + output->nr_chans = width;
>> + stm_output_claim(stm, output);
>> + dev_dbg(stm->dev, "assigned %u:%u (+%u)\n", midx, cidx, width);
>> +
>> + ret = 0;
>> +unlock:
>> + spin_unlock(&stm->mc_lock);
>> +
>> + return ret;
>> +}
>> +
>
> [Snip...]
>
>> +
>> +/**
>> + * stm_source_register_device() - register an stm_source device
>> + * @parent: parent device
>> + * @data: device description structure
>> + *
>> + * This will create a device of stm_source class that can write
>> + * data to an stm device once linked.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +int stm_source_register_device(struct device *parent,
>> + struct stm_source_data *data)
>> +{
>> + struct stm_source_device *src;
>> + struct device *dev;
>> +
>> + if (!stm_core_up)
>> + return -EPROBE_DEFER;
>> +
>> + src = kzalloc(sizeof(*src), GFP_KERNEL);
>> + if (!src)
>> + return -ENOMEM;
>> +
>> + dev = device_create(&stm_source_class, parent, MKDEV(0, 0), NULL, "%s",
>> + data->name);
>> + if (IS_ERR(dev)) {
>> + kfree(src);
>> + return PTR_ERR(dev);
>> + }
>> +
>> + spin_lock_init(&src->link_lock);
>> + INIT_LIST_HEAD(&src->link_entry);
>> + src->dev = dev;
>> + src->data = data;
>> + data->src = src;
>> + dev_set_drvdata(dev, src);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(stm_source_register_device);
>> +
>
> [Snip...]
>
> It's really not clear (at least to me) how stm_source_device works.
> They make a bridge between the kernel and the STM devices but the
> "link" between them is not obvious. More documentation perhaps?
Sure, I need to elaborate on this a bit. The idea the stm_source device
gets "linked" to an stm device by way of writing that stm device name to
its stm_source_link attribute and if everything's fine, stm_sounrce's
.link callback is called, after which it can start sending data out with
stm_sounce_write().
>> +/**
>> + * struct stm_data - STM device description and callbacks
>> + * @name: device name
>> + * @stm: internal structure, only used by stm class code
>> + * @sw_start: first STP master
>> + * @sw_end: last STP master
>> + * @sw_nchannels: number of STP channels per master
>> + * @sw_mmiosz: size of one channel's IO space, for mmap, optional
>> + * @write: write callback
>> + * @mmio_addr: mmap callback, optional
>> + *
>> + * Fill out this structure before calling stm_register_device() to create
>> + * an STM device and stm_unregister_device() to destroy it. It will also be
>> + * passed back to write() and mmio_addr() callbacks.
>> + */
>> +struct stm_data {
>> + const char *name;
>> + struct stm_device *stm;
>> + unsigned int sw_start;
>> + unsigned int sw_end;
>
> The above kernel doc is the only place where "sw_start" and "sw_end"
> are described as first and last STP masters. Other than that it takes
> a really long time to figure out what they really are. I think a
> better naming convention can be chosen here.
The idea is that only a certain subset of masters is available for
software (others being statically assigned to different hw blocks or
otherwise unavailable to software). These two mark start and end of this
subset.
>
>> + unsigned int sw_nchannels;
>> + unsigned int sw_mmiosz;
>> + ssize_t (*write)(struct stm_data *, unsigned int,
>> + unsigned int, const char *, size_t);
>> + phys_addr_t (*mmio_addr)(struct stm_data *, unsigned int,
>> + unsigned int, unsigned int);
>> + void (*link)(struct stm_data *, unsigned int,
>> + unsigned int);
>> + void (*unlink)(struct stm_data *, unsigned int,
>> + unsigned int);
>
> It is really not clear to me what the "link" and "unlink" functions do
> - documenting what they're for and explain when to use them and (not
> use them) would be appreciated.
Will do.
> I think we should also add two things to this structure: 1) a private
> field and 2) a (*stm_drv_ioctl) stub.
> The private field would be filled by the registrant and left alone by
> the generic-stm core. When parsing the commands in
> "stm_char_ioctl()", data->stm_drv_ioctl(private, cmd, arg) could be
> called to let architecture specific drivers deal with it. That way
> applications can deal with a single configuration file descriptor.
Indeed, good idea.
>
>> +};
>> +
>> +int stm_register_device(struct device *parent, struct stm_data *stm_data,
>> + struct module *owner);
>> +void stm_unregister_device(struct stm_data *stm_data);
>> +
>> +struct stm_source_device;
>> +
>> +/**
>> + * struct stm_source_data - STM source device description and callbacks
>> + * @name: device name, will be used for policy lookup
>> + * @src: internal structure, only used by stm class code
>> + * @nr_chans: number of channels to allocate
>> + * @link: called when STM device gets linked to this source
>> + * @unlink: called when STH device is about to be unlinked
>> + *
>> + * Fill in this structure before calling stm_source_register_device() to
>> + * register a source device. Also pass it to unregister and write calls.
>> + */
>> +struct stm_source_data {
>> + const char *name;
>> + struct stm_source_device *src;
>> + unsigned int percpu;
>> + unsigned int nr_chans;
>> + int (*link)(struct stm_source_data *data);
>> + void (*unlink)(struct stm_source_data *data);
>> +};
>> +
>
> I didn't get the linking/unlinking process - it is also not clear as
> to why we need struct stm_data and struct stm_source_data. More
> explanation on this would be good.
My idea is that connecting stm to stm_source device is done via sysfs
attribute write like so:
$ echo dummy_stm > /sys/class/stm_source/stm_console/stm_source_link
This will result in stm_source's .link getting called, at which point
stm_console, for example, will do a register_console().
stm_data is there for stm devices to pass their parameters and callbacks
to stm_register_device(); stm_source data is that for stm_source
devices. I'll try to be more elaborate about these in my followup.
>> +int stm_source_register_device(struct device *parent,
>> + struct stm_source_data *data);
>> +void stm_source_unregister_device(struct stm_source_data *data);
>> +
>> +int stm_source_write(struct stm_source_data *data, unsigned int chan,
>> + const char *buf, size_t count);
>> +
>> +#endif /* _STM_H_ */
>> diff --git a/include/uapi/linux/stm.h b/include/uapi/linux/stm.h
>> new file mode 100644
>> index 0000000000..042b58b53b
>> --- /dev/null
>> +++ b/include/uapi/linux/stm.h
>> @@ -0,0 +1,47 @@
>> +/*
>> + * System Trace Module (STM) userspace interfaces
>> + * Copyright (c) 2014, Intel Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * STM class implements generic infrastructure for System Trace Module devices
>> + * as defined in MIPI STPv2 specification.
>> + */
>> +
>> +#ifndef _UAPI_LINUX_STM_H
>> +#define _UAPI_LINUX_STM_H
>> +
>> +/**
>> + * struct stp_policy_id - identification for the STP policy
>> + * @size: size of the structure including real id[] length
>> + * @master: assigned master
>> + * @channel: first assigned channel
>> + * @width: number of requested channels
>> + * @id: identification string
>> + *
>> + * User must calculate the total size of the structure and put it into
>> + * @size field, fill out the @id and desired @width. In return, kernel
>> + * fills out @master, @channel and @width.
>> + */
>> +struct stp_policy_id {
>> + __u32 size;
>> + __u16 master;
>> + __u16 channel;
>> + __u16 width;
>> + /* padding */
>> + __u16 __reserved_0;
>> + __u32 __reserved_1;
>> + char id[0];
>> +};
>> +
>> +#define STP_POLICY_ID_SET _IOWR('%', 0, struct stp_policy_id)
>> +#define STP_POLICY_ID_GET _IOR('%', 1, struct stp_policy_id)
>> +
>> +#endif /* _UAPI_LINUX_STM_H */
>> --
>> 2.1.4
>>
>
> Aside from the above points I think this is all very good work. The
> patchset should likely be broken up in two sets, one for the generic
> STM architecture wrapper and another for the Intel TH part. That way
> things can be worked on independently.
The Intel TH part is very much dependent on the STM part; I could split
out the stm-dependant patch from Intel TH, but that just seems to make
it more complex for me and reviewers. I'd prefer to keep them together
unless there are strong objections.
> On my side I will get a prototype going that folds the current
> coresight-stm driver in this new mindset - I'll let you know how
> things go.
Thanks a lot!
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists