| 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
| ||
|
Date: Wed, 18 Mar 2015 09:07:27 -0600
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"linux-kernel@...r.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
On 17 March 2015 at 04:37, Alexander Shishkin
<alexander.shishkin@...ux.intel.com> wrote:
> 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,
I forgot to mention in my previous email... I think the hierarchy of
our respective tracing module along with the generic-stm probably
needs a review.
Currently we have drivers/coresight, drivers/intel_th and drivers/stm.
To me it doesn't scale - what happens when other architectures come
out with their own hw tracing technologies?
I suggest we move everything under drivers/hwtracing and as such have:
drivers/hwtracing
drivers/hwtracing/intel_ht
drivers/hwtracing/coresight
drivers/hwtracing/stm
That way other architectures can add drivers for their own hw tracing
technology without further polluting the drivers/ directory and
concentrating everything in the same area. What's your view on that?
> --
> 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