Re: [PATCH v2] stm class: Document the stm_ftrace
Chunyan Zhang writes: > Hi Alex, Hi Chunyan, > Could you take this patch please if there's no further comments? Sorry for the delay. Yes, I'm picking this patch into my tree very soon. Thanks, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] stm class: Document the stm_ftrace
On 21 March 2017 at 07:57, Chunyan Zhang wrote: > On 20 March 2017 at 19:09, Alexander Shishkin > wrote: >> Chunyan Zhang writes: >> >>> Hi Alex, >>> >>> On 20 March 2017 at 16:49, Alexander Shishkin >>> wrote: >>>> Hi Chunyan, >>>> >>>> A couple of clarifications: iirc this applies to the function tracer >>>> of ftrace, right? Does it make sense to mention that? Also, are you >>> >>> Right, only applies to the function tracer currently (actually only >>> function address and parent function address of Function tracer is >>> recorded into STM, I mean it doesn't include like "pid" "task name" >>> "cpu-id" these information right now). It makes sense to mention >>> function tracer, I will address that. >> >> Thanks! >> >>>> planning to support other ftrace payloads like trace_printk()s? >>> >>> No plan so far, but I think I can consider to do that, it depends on >>> how many people think that are helpful. >>> What do you think? >> >> Well, I myself almost never use function tracer, but I do use >> tracepoints/trace_printk()s. I'm *guessing* that everybody who's > > In fact I had implemented exporting tracepoints to STM and tried > upstreaming that, but Steven Rostedt and Ingo expressed their worries > on that would introduce a considerable impact on Ftrace fast path > since a tracepoint basically was a string which was too long to be > written to STM with some acceptable impact on fast path, so I stopped > upstreaming that feature. Did we ever consider writing a string pointer (or even on offset into the corresponding section, to avoid KASLR fun) instead of the actual string? This would require a kernel binary on the decoding end, though. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] stm class: Document the stm_ftrace
Chunyan Zhang writes: > Hi Alex, > > On 20 March 2017 at 16:49, Alexander Shishkin > wrote: >> Hi Chunyan, >> >> A couple of clarifications: iirc this applies to the function tracer >> of ftrace, right? Does it make sense to mention that? Also, are you > > Right, only applies to the function tracer currently (actually only > function address and parent function address of Function tracer is > recorded into STM, I mean it doesn't include like "pid" "task name" > "cpu-id" these information right now). It makes sense to mention > function tracer, I will address that. Thanks! >> planning to support other ftrace payloads like trace_printk()s? > > No plan so far, but I think I can consider to do that, it depends on > how many people think that are helpful. > What do you think? Well, I myself almost never use function tracer, but I do use tracepoints/trace_printk()s. I'm *guessing* that everybody who's subsystem implement tracepoints will be interested in those. I confess that I haven't yet looked at the code properly, so I'm a don't have a picture of what it will take to implement these. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] stm class: Document the stm_ftrace
Hi Chunyan, A couple of clarifications: iirc this applies to the function tracer of ftrace, right? Does it make sense to mention that? Also, are you planning to support other ftrace payloads like trace_printk()s? Thanks, -- Alex On 20 March 2017 at 08:47, Chunyan Zhang wrote: > This patch adds a description of the stm_ftrace device source, an > interface for collecting Function trace information via STM devices. > > Signed-off-by: Chunyan Zhang > --- > Documentation/trace/stm.txt | 10 +- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/Documentation/trace/stm.txt b/Documentation/trace/stm.txt > index 11cff47..7ec1c0e 100644 > --- a/Documentation/trace/stm.txt > +++ b/Documentation/trace/stm.txt > @@ -83,7 +83,7 @@ by writing the name of the desired stm device there, for > example: > $ echo dummy_stm.0 > /sys/class/stm_source/console/stm_source_link > > For examples on how to use stm_source interface in the kernel, refer > -to stm_console or stm_heartbeat drivers. > +to stm_console, stm_heartbeat or stm_ftrace drivers. > > Each stm_source device will need to assume a master and a range of > channels, depending on how many channels it requires. These are > @@ -107,5 +107,13 @@ console in the STP stream, create a "console" policy > entry (see the > beginning of this text on how to do that). When initialized, it will > consume one channel. > > +stm_ftrace > +== > + > +This is another "stm_source" device, once the stm_ftrace is linked with > +an stm device, function address and parent function address which > +Ftrace subsystem would store into ring buffer will be exported via the > +stm device at the same time. > + > [1] > https://software.intel.com/sites/default/files/managed/d3/3c/intel-th-developer-manual.pdf > [2] > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0444b/index.html > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] security,perf: Allow further restriction of perf_event_open
Ben Hutchings writes: > When kernel.perf_event_open is set to 3 (or greater), disallow all > access to performance events by users without CAP_SYS_ADMIN. > Add a Kconfig symbol CONFIG_SECURITY_PERF_EVENTS_RESTRICT that > makes this value the default. So this patch does two things, can it then be made into two patches? > > This is based on a similar feature in grsecurity > (CONFIG_GRKERNSEC_PERF_HARDEN). This version doesn't include making > the variable read-only. It also allows enabling further restriction > at run-time regardless of whether the default is changed. This paragraph doesn't seem to belong in the commit message. What this commit message is missing entirely is the rationale behind this change other than "grsecurity does the same". Can you please elaborate? > Signed-off-by: Ben Hutchings > --- > I made a similar change to Debian's kernel packages in August, > including the more restrictive default, and no-one has complained yet. As a debian user, is this a good place to complain? Because it does get it the way. Thanks, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 0/4] Introduce CoreSight STM support
Mathieu Poirier writes: > On 6 April 2016 at 20:51, Chunyan Zhang wrote: >> This patchset adds support for the CoreSight STM IP block. > > This has been out there long enough - I'm picking this up. > > Alex, I'll have 1/4 go through my tree. Get back to me if you want to > proceed differently. Yes, let's keep it that way. I was sure I had it in my tree too, but, alas, I didn't. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH V4 1/4] stm class: provision for statically assigned masterIDs
Mathieu Poirier writes:
> On 21 March 2016 at 01:47, Alexander Shishkin
> wrote:
>> Chunyan Zhang writes:
>>
>>> From: Mathieu Poirier
>>>
>>> Some architecture like ARM assign masterIDs at the HW design
>>> phase. Those are therefore unreachable to users, making masterID
>>> management in the generic STM core irrelevant.
>>>
>>> In this kind of configuration channels are shared between masters
>>> rather than being allocated on a per master basis.
>>>
>>> This patch adds a new 'mshared' flag to struct stm_data that tells the
>>> core that this specific STM device doesn't need explicit masterID
>>> management.
>>
>> There are two kinds of 'masterIDs' that we're talking about here: the
>> ones that turn up in the STP stream and the ones that are accessible to
>> trace-side software (sw_start/sw_end). So in this case we want to
>> reflect the fact that there is no correlation between the two, because
>> hardware assigns STP channels dynamically based on the states of the
>> trace/execution environment. And although the trace side software can do
>> very little with this information, it does make sense to provide it.
>>
>> The sw_start==sw_end situation, on the other hand, is a side effect of
>> the above and, as I said in one of the previous threads, may not even be
>> the case, or at least I don't see why it has to. And when it is the
>> case, I don't see the point in handling it differently from
>> sw_start Other than the above, is there anything you'd like to see modified in
> this patch, i.e how the mshared flag is used to modify the output in
> sysFS/configFS?
Yes, the two paragraphs quoted above that had gone unnoticed. I've
been thinking some more about this and came up with the following. No
need to do any extra handling in the policy code or anywhere else.
>From 8be22a8e391b94ec13d428976e7b1410aa59991e Mon Sep 17 00:00:00 2001
From: Alexander Shishkin
Date: Thu, 31 Mar 2016 15:51:29 +0300
Subject: [PATCH] stm class: Support devices that override software assigned
masters
Some STM devices adjust software assigned master numbers depending on
the trace source and its runtime state and whatnot. This patch adds
a sysfs attribute to inform the trace-side software that master numbers
assigned to software sources will not match those in the STP stream,
so that, for example, master/channel allocation policy can be adjusted
accordingly.
Signed-off-by: Alexander Shishkin
---
Documentation/ABI/testing/sysfs-class-stm | 10 ++
drivers/hwtracing/stm/core.c | 15 +++
include/linux/stm.h | 3 +++
3 files changed, 28 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-stm
b/Documentation/ABI/testing/sysfs-class-stm
index c9aa4f3fc9..77ed3da0f6 100644
--- a/Documentation/ABI/testing/sysfs-class-stm
+++ b/Documentation/ABI/testing/sysfs-class-stm
@@ -12,3 +12,13 @@ KernelVersion: 4.3
Contact: Alexander Shishkin
Description:
Shows the number of channels per master on this STM device.
+
+What: /sys/class/stm//hw_override
+Date: March 2016
+KernelVersion: 4.7
+Contact: Alexander Shishkin
+Description:
+ Reads as 0 if master numbers in the STP stream produced by
+ this stm device will match the master numbers assigned by
+ the software or 1 if the stm hardware overrides software
+ assigned masters.
diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 2591442e2c..ff31108b06 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -67,9 +67,24 @@ static ssize_t channels_show(struct device *dev,
static DEVICE_ATTR_RO(channels);
+static ssize_t hw_override_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct stm_device *stm = to_stm_device(dev);
+ int ret;
+
+ ret = sprintf(buf, "%u\n", stm->data->hw_override);
+
+ return ret;
+}
+
+static DEVICE_ATTR_RO(hw_override);
+
static struct attribute *stm_attrs[] = {
&dev_attr_masters.attr,
&dev_attr_channels.attr,
+ &dev_attr_hw_override.attr,
NULL,
};
diff --git a/include/linux/stm.h b/include/linux/stm.h
index 1a79ed8e43..8369d8a8ca 100644
--- a/include/linux/stm.h
+++ b/include/linux/stm.h
@@ -50,6 +50,8 @@ struct stm_device;
* @sw_end:last STP master available to software
* @sw_nchannels: number of STP channels per master
* @sw_mmiosz: size of one channel's IO space, for mmap, optional
+ * @hw_override: masters in the STP stream will not match the
Re: [RESEND PATCH V4 1/4] stm class: provision for statically assigned masterIDs
Chunyan Zhang writes:
> From: Mathieu Poirier
>
> Some architecture like ARM assign masterIDs at the HW design
> phase. Those are therefore unreachable to users, making masterID
> management in the generic STM core irrelevant.
>
> In this kind of configuration channels are shared between masters
> rather than being allocated on a per master basis.
>
> This patch adds a new 'mshared' flag to struct stm_data that tells the
> core that this specific STM device doesn't need explicit masterID
> management.
There are two kinds of 'masterIDs' that we're talking about here: the
ones that turn up in the STP stream and the ones that are accessible to
trace-side software (sw_start/sw_end). So in this case we want to
reflect the fact that there is no correlation between the two, because
hardware assigns STP channels dynamically based on the states of the
trace/execution environment. And although the trace side software can do
very little with this information, it does make sense to provide it.
The sw_start==sw_end situation, on the other hand, is a side effect of
the above and, as I said in one of the previous threads, may not even be
the case, or at least I don't see why it has to. And when it is the
case, I don't see the point in handling it differently from
sw_start In the core sw_start/end of masterID are set to '1',
> i.e there is only one masterID to deal with.
Why does this need to be done in the core and why '1'? IOW,
sw_{start,end} come straight from the driver, this new 'mshared' comes
straight from the driver, why do we need the core to modify the former
based on the latter?
> Also this patch depends on [1], so that the number of masterID
> is '1' too.
It's 7b3bb0e753 in Linus' tree, but I don't see the logical connection
in the statement above.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
Mathieu Poirier writes: > On 8 February 2016 at 06:26, Alexander Shishkin > wrote: >> This $end==$start situation itself may be ambiguous and can be >> interpreted either as having just one *static* master ID fixed for all >> SW writers (what I assumed from your commit message) or as having a >> floating master ID, which changes of its own accord and is not >> controllable by software. > > Some clarification here. > > On ARM from a SW point of view $end == $start and that doesn't change > (with regards to masterIDs) . The ambiguity comes from the fact that > on other platforms where masterID configuration does change and is > important, the condition $end == $start could also be valid. Yes, that's what I was saying. The thing is, on the system-under-tracing side these two situations are not very different from one another. Master IDs are really just numbers without any semantics attached to them in the sense that they are not covered by the mipi spec or any other standard (to my knowledge). The difference is in the way we map channels to masters. One way is to allocate a distinct set of channels for each master (the way Intel Trace Hub does it); another way is to share the same set of channels between multiple masters. So we can describe this as "hardware implements the same set of channels across multiple masters" or something along those lines. Actually, in the latter scheme of things you can also have multiple masters, at least theoretically. Say, you have masters [0..15], each with distinct set of channels, but depending on hardware state these masters actually end up as $state*16+$masterID in the STP stream. So we might also think about differentiating between the hardware masters (0 though 15 in the above example) and STP masters. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
Al Grant writes: >> Mike did write "master IDs are hardwired to individual cores and core >> security >> states", which make assignment for one platform very static. >> On the flip side those will change from one system to another. > > It depends on your perspective. From the perspective of a userspace > process not pinned to a core, the master id will appear to vary > dynamically and unpredictably as the thread migrates from one > core to another. (That's actually useful if the decoder wants to know > where the thread is running at any given point, as it can find that out > for free, without the need to track migration events.) > > On the other hand if you are pinned (e.g. you're the kernel on a > particular core, or you're a per-core worker thread in some thread > pooling system) then you have a fixed master id, and then you can > have one instance per core all using the same range of channel > numbers, with the master id indicating the core - this saves on > channel space compared to having to give each core its own > range of channel space. What I understood from Mike's explanation is that basically nothing is fixed from the software perspective (talking about Coresight STM). One doesn't know the master id with which one's data will be tagged, regardless of whether one is cpu-affine or not, because: * even per-core master IDs will be highly implementation dependent and therefore not knowable by the kernel and subsequently userspace, * master IDs may change based on other conditions (besides security states), so even if we knew per-core master IDs for sure, it wouldn't give us the complete picture. So even though from the HW perspective master IDs may be "hardwired" to hardware states, the hardware states themselves are dynamic, so from the SW perspective these master IDs are not static or hardwired at all. If the same mmio write done by software would always result in the exact same master ID tagged to it, that would be more like "static" to me. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 6/6] coresight-stm: adding driver for CoreSight STM component
Chunyan Zhang writes:
> +static long stm_generic_set_options(struct stm_data *stm_data,
> + unsigned int master,
> + unsigned int channel,
> + unsigned int nr_chans,
> + unsigned long options)
> +{
> + struct stm_drvdata *drvdata = container_of(stm_data,
> +struct stm_drvdata, stm);
> + if (!(drvdata && drvdata->enable))
> + return -EINVAL;
> +
> + if (channel >= drvdata->numsp)
> + return -EINVAL;
> +
> + switch (options) {
> + case STM_OPTION_GUARANTEED:
> + set_bit(channel, drvdata->chs.guaranteed);
> + break;
> +
> + case STM_OPTION_INVARIANT:
> + clear_bit(channel, drvdata->chs.guaranteed);
> + break;
This is a bad interface. Firstly, neither option is described
anywhere. Secondly, I'm pretty sure "invariant" does not mean "not
guaranteed" in english, although this function seems to imply this.
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static long stm_generic_get_options(struct stm_data *stm_data,
> + unsigned int master,
> + unsigned int channel,
> + unsigned int nr_chans,
> + u64 *options)
> +{
> + struct stm_drvdata *drvdata = container_of(stm_data,
> +struct stm_drvdata, stm);
> + if (!(drvdata && drvdata->enable))
> + return -EINVAL;
> +
> + if (channel >= drvdata->numsp)
> + return -EINVAL;
> +
> + switch (*options) {
> + case STM_OPTION_GUARANTEED:
> + *options = test_bit(channel, drvdata->chs.guaranteed);
> + break;
This just doesn't work. @options here is an on-stack variable in
stm_char_ioctl(), hitherto uninitialized. The get_options ioctl command
as you implemented it doesn't fetch @options from userspace either, it
just passes a pointer to it to this callback, expecting that the
callback will set it so that it can be copy_to_user()ed back to the
user.
Then, when we figure this out, there is again the question of what
should one make of STM_OPTION_{GUARANTEED,INVARIANT} and how do they fit
into *options.
The idea behind set_options ioctl is that the user specifies a bit mask
of options that he/she wants to set.
[snip]
> +#ifndef __UAPI_CORESIGHT_STM_H_
> +#define __UAPI_CORESIGHT_STM_H_
> +
> +#define STM_FLAG_TIMESTAMPED BIT(3)
> +#define STM_FLAG_GUARANTEEDBIT(7)
> +
> +enum {
> + STM_OPTION_GUARANTEED = 0,
> + STM_OPTION_INVARIANT,
> +};
Each of these guys could also use an explanation.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/6] stm class: Add ioctl get_options interface
Chunyan Zhang writes: > There is already an interface of set_options, but no get_options yet. > Before setting any options, one would may want to see the current > status of that option by means of get_options interface. This > interface has been used in CoreSight STM driver. I'm not sure I understand the reasoning behind this. If a userspace program opens a communication channel and wants to configure certain features on it, why does its choice depend on what has been configured for this channel previously? It can be anything at all. Most likely, it's either unconfigured or configured to its default values, but why does this matter for a new writer? Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
Mathieu Poirier writes: > On 5 February 2016 at 05:52, Alexander Shishkin > wrote: >> Chunyan Zhang writes: >> >>> From: Mathieu Poirier >>> >>> Some architecture like ARM assign masterIDs statically at the HW design >>> phase, making masterID manipulation in the generic STM core irrelevant. >>> >>> This patch adds a new 'mstatic' flag to struct stm_data that tells the >>> core that this specific STM device doesn't need explicit masterID >>> management. >> >> So why do we need this patch? If your STM only has master 42 allocated >> for software sources, simply set sw_start = 42, sw_end = 42 and you're >> good to go, software will have exactly one channel to choose from. See >> also the comment from : > > On ARM each source, i.e entity capable of accessing STM channels, has > a different master ID set in HW. We can't assume the IDs are > contiguous and from a SW point of view there is no way to probe the > values. Ok, it's the 'static' word that got me confused. From Mike's explanation it seems to me that it's the antithesis of static; the master ID assignment is so dynamic that it's not controllable by the software and may or may not reflect core id, power state, phase of the moon, etc. >>> In the core sw_start/end of masterID are set to '1', >>> i.e there is only one masterID to deal with. >> >> This is also a completely arbitrary and unnecessary requirement. Again, >> you can set both to 42 and it will still work. > > True - any value will do. The important thing to remember is that on > ARM, there is only one masterID channel (from an STM core point of > view). But we could also proceed differently, see below for more > details. Well, we have the masters attribute with two numbers in it that define the range of master IDs that the software can choose from. More specifically to this situation: * the number of channel ID spaces available to the SW is $end - $start + 1, that is, in your case, just 1; * the number of master IDs for the SW to choose from is $end - $start; * if $end==$start, their actual numeric value doesn't really matter, either for the policy definition or for the actual writers. This $end==$start situation itself may be ambiguous and can be interpreted either as having just one *static* master ID fixed for all SW writers (what I assumed from your commit message) or as having a floating master ID, which changes of its own accord and is not controllable by software. These two situations are really the same thing from the perspective of the system under tracing. Also, both of these situations should already work if the driver sets both sw_start and sw_end to the same value. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
Mike Leach writes: > Hi, > > I think a quick clarification of the ARM hardware STM architecture may be of > value here. > > The ARM hardware STM, when implemented as recommend in a hardware design, the > master IDs are not under driver control, but have a hardwire association with > source devices in the system. The master IDs are hardwired to individual > cores and core security states, and there could be other IDs associated with > hardware trace sources requiring output via the hardware STM. (an example of > this is the ARM Juno development board). > > Therefore in multi-core systems many masters may be associated with a single > software STM stream. A user-space application running on Core 1, may have a > master ID of 0x11 in the STPv2 trace stream, but if this application is > context switched and later continues running on Core 2, then master ID could > change to 0x21. Masters identify a hardware source in this scheme, the > precise values used will be implementation dependent. > > So the number of masters "available to the software" depends on your > interpretation of the phrase. > If you mean "master IDs on which software trace will appear" then that will > be many - it depends on which core is running the application. However as > described above, this is not predictable by any decoder, and the master set > used may not be contiguous. > If you mean "master IDs selectable/allocated by the driver" then that will be > 0 - the driver has no control, and possibly no knowledge of which master is > associated with the current execution context. (this is of course system > dependent - it may well be possible to have some configuration database > associating cores with hardware IDs, but this will be implementation and > board support dependant). > > Therefore, there is a requirement to tell both the user-space STM client and > decoder that not only is master ID management not needed - it is in fact not > possible and the key to identify the software source for a given STM packet > is the channel alone. In addition we have a nominal single Master ID > "available to the software", to satisfy the requirements of the generic ST > module API. > > So on Juno for example, while we do have 128 masters, each with 65536 > channels, the master allocation is not visible to system software - each core > sees a single master with 65536 channels. Therefore differentiation between > software sources in the STM trace is by channel number allocations only. Ok, thanks a lot for the detailed explanation. I was confused by the "static" bit in the patch description. It looks like something along the lines of this patch might indeed be in order. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 6/6] coresight-stm: adding driver for CoreSight STM component
Chunyan Zhang writes:
> +#ifndef CONFIG_64BIT
> +static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
> +{
> + asm volatile("strd %1, %0"
> + : "+Qo" (*(volatile u64 __force *)addr)
> + : "r" (val));
> +}
Is it really ok to do this for all !64bit arms, inside a driver, just
like that? I'm not an expert, but I'm pretty sure there's more to it.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 1/6] stm class: Add ioctl get_options interface
Chunyan Zhang writes: > There is already an interface of set_options, but no get_options yet. > Before setting any options, one would may want to see the current > status of that option by means of get_options interface. This > interface has been used in CoreSight STM driver. > > Signed-off-by: Chunyan Zhang > --- > drivers/hwtracing/stm/core.c | 11 +++ > include/linux/stm.h | 3 +++ > include/uapi/linux/stm.h | 1 + > 3 files changed, 15 insertions(+) > > diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c > index b6445d9..86bb4e3 100644 > --- a/drivers/hwtracing/stm/core.c > +++ b/drivers/hwtracing/stm/core.c > @@ -571,6 +571,17 @@ stm_char_ioctl(struct file *file, unsigned int cmd, > unsigned long arg) > options); > > break; > + > + case STP_GET_OPTIONS: > + if (stm_data->get_options) > + err = stm_data->get_options(stm_data, > + stmf->output.master, > + stmf->output.channel, > + stmf->output.nr_chans, > + &options); > + > + return copy_to_user((void __user *)arg, &options, sizeof(u64)); The return value of copy_to_user() is not what you assume it is. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
Chunyan Zhang writes: > From: Mathieu Poirier > > Some architecture like ARM assign masterIDs statically at the HW design > phase, making masterID manipulation in the generic STM core irrelevant. > > This patch adds a new 'mstatic' flag to struct stm_data that tells the > core that this specific STM device doesn't need explicit masterID > management. So why do we need this patch? If your STM only has master 42 allocated for software sources, simply set sw_start = 42, sw_end = 42 and you're good to go, software will have exactly one channel to choose from. See also the comment from : * @sw_start: first STP master available to software * @sw_end: last STP master available to software particularly the "available to software" part. Any other kinds of masters the STM class code doesn't care about (yet). > In the core sw_start/end of masterID are set to '1', > i.e there is only one masterID to deal with. This is also a completely arbitrary and unnecessary requirement. Again, you can set both to 42 and it will still work. > Also this patch depends on [1], so that the number of masterID > is '1' too. > > Finally the lower and upper bound for masterIDs as presented > in ($SYSFS)/class/stm/XYZ.stm/masters and > ($SYSFS)/../stp-policy/XYZ.stm.my_policy/some_device/masters > are set to '-1'. That way users can't confuse them with > architecture where masterID management is required (where any > other value would be valid). Why is this a good idea? Having the actual master there will allow software to know what it is and also tell the decoding side what it is (assuming you have more than one source in the STP stream, it might not be easy to figure out, unless you know it in advance). I don't see how one master statically assigned to software sources is different from two masters or 32 masters. And I don't see any benefit of hiding the master id from userspace. Am I missing something? Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name
Chunyan Zhang writes:
I few comments below:
> The node name of STM master management policy is a concatenation of an
> STM device name to which this policy applies and following an arbitrary
> string, these two strings are concatenated with a dot.
This is true.
> This patch adds a loop for extracting the STM device name when an
> arbitrary number of dot(s) are found in this STM device name.
It's not very easy to tell what's going on here from this
description. The reader be left curious as to why an arbitrary number of
dots is a reason to run a loop. When in doubt, try to imagine as if
you're seeing this patch for the first time and ask yourself, does the
message give a clear explanation of what's going on in it.
> Signed-off-by: Chunyan Zhang
> ---
> drivers/hwtracing/stm/policy.c | 27 ---
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
> index 11ab6d0..691686e 100644
> --- a/drivers/hwtracing/stm/policy.c
> +++ b/drivers/hwtracing/stm/policy.c
> @@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const
> char *name)
> /*
>* node must look like ., where
>* is the name of an existing stm device and
> - * is an arbitrary string
> + * is an arbitrary string, when an arbitrary
> + * number of dot(s) are found in the , the
> + * first matched STM device name would be extracted.
>*/
This leaves room for a number of suspicious situations. What if both
"xyz" and "xyz.0" are stm devices, how would you create a policy for the
latter, for example?
The rules should be such that you can tell exactly what the intended stm
device is from a policy directory name, otherwise it's just asking for
trouble.
> - p = strchr(devname, '.');
> - if (!p) {
> - kfree(devname);
> - return ERR_PTR(-EINVAL);
> - }
> + for (p = devname; ; p++) {
> + p = strchr(p, '.');
> + if (!p) {
> + kfree(devname);
> + return ERR_PTR(-EINVAL);
> + }
>
> - *p++ = '\0';
> + *p = '\0';
>
> - stm = stm_find_device(devname);
> - kfree(devname);
> + stm = stm_find_device(devname);
> + if (stm)
> + break;
> + *p = '.';
> + }
>
> - if (!stm)
> - return ERR_PTR(-ENODEV);
> + kfree(devname);
In the existing code there is a clear distinction between -ENODEV, which
is to say "we didn't find the device" and -EINVAL, "directory name
breaks rules/is badly formatted". After the change, it's all -EINVAL,
which also becomes "we tried everything, sorry".
So, having said all that, does the following patch solve your problem:
>From 870dc5fefa5623c39552511d31e0fa0da984d581 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin
Date: Thu, 4 Feb 2016 18:56:34 +0200
Subject: [PATCH] stm class: Support devices with multiple instances
By convention, the name of the stm policy directory in configfs consists of
the device name to which it applies and the actual policy name, separated
by a dot. Now, some devices already have dots in their names that separate
name of the actual device from its instance identifier. Such devices will
result in two (or more, who can tell) dots in the policy directory name.
Existing policy code, however, will treat the first dot as the one that
separates device name from policy name, therefore failing the above case.
This patch makes the last dot in the directory name be the separator, thus
prohibiting dots from being used in policy names.
Suggested-by: Chunyan Zhang
Signed-off-by: Alexander Shishkin
---
drivers/hwtracing/stm/policy.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 94d3abfb73..1db189657b 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -332,10 +332,11 @@ stp_policies_make(struct config_group *group, const char
*name)
/*
* node must look like ., where
-* is the name of an existing stm device and
-* is an arbitrary string
+* is the name of an existing stm device; may
+* contain dots;
+* is an arbitrary string; may not contain dots
*/
- p = strchr(devname, '.');
+ p = strrchr(devname, '.');
if (!p) {
kfree(devname);
return ERR_PTR(-EINVAL);
--
2.7.0
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V8 18/23] coresight: etm-perf: new PMU driver for ETM tracers
Mathieu Poirier writes: >> I'd like to understand all the potential failures here, because it's >> really a good idea to keep those to a minimum for the sake of >> consistency. That is, if the user succeeded in creating an event, about >> the only good reason for the event not starting is a filled up buffer. > > Enabling a path should fail when one or many components of that path > are already enabled by an ongoing trace session. This situation is > quite likely to happen since in a lot of design tracers share the link > and sinks. Yes, but provided that we don't get interference from sysfs users (which, I guess, could be blocked out while etm perf events exist), this part shouldn't fail, as nobody else should be using these links and sinks but etm events and those are safe from overlapping because of PERF_PMU_CAP_EXCLUSIVE. Or am I missing something? >> This is why it makes a lot of sense to keep all the >> coresight_build_path()/coresight_enable_path() to the .event_init() >> phase and let them fail early, if they should fail. > > If we do enable enable paths in .event_init() we can't support > multiple concurrent trace session (see explanation above). The > ultimate design is to have a source directly connected to a sink but > so far none of the coresight topologies I've seen have been wired like > that. So if we call dibs on those paths early (like event_init early), in such a way that nobody but other etm events can use them, we should be ok, I think. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V8 16/23] coresight: etb10: implementing AUX API
Mathieu Poirier writes:
> Adding an ETB10 specific AUX area operations to be used
> by the perf framework when events are initialised.
>
> Part of this operation involves modeling the mmap'ed area
> based on the specific ways a sink buffer gathers information.
I don't mind being CC'd on the rest of the patches too, btw. :)
> +static unsigned long etb_reset_buffer(struct coresight_device *csdev,
> + struct perf_output_handle *handle,
> + void *sink_config, bool *lost)
> +{
> + unsigned long size = 0;
> + struct cs_buffers *buf = sink_config;
> +
> + if (buf) {
> + /*
> + * In snapshot mode ->data_size holds the new address of the
> + * ring buffer's head. The size itself is the whole address
> + * range since we want the latest information.
> + */
> + if (buf->snapshot)
> + handle->head = local_xchg(&buf->data_size,
> + buf->nr_pages << PAGE_SHIFT);
> +
> + /*
> + * Tell the tracer PMU how much we got in this run and if
> + * something went wrong along the way. Nobody else can use
> + * this cs_buffers instance until we are done. As such
> + * resetting parameters here and squaring off with the ring
> + * buffer API in the tracer PMU is fine.
> + */
> + *lost = local_xchg(&buf->lost, 0);
This is a thin ice, you can't really make assumptions about bool's
storage size or even type, afaict.
> + size = local_xchg(&buf->data_size, 0);
> + }
> +
> + return size;
> +}
> +
> +static void etb_update_buffer(struct coresight_device *csdev,
> + struct perf_output_handle *handle,
> + void *sink_config)
> +{
> + int i, cur;
> + u8 *buf_ptr;
> + u32 read_ptr, write_ptr, capacity;
> + u32 status, read_data, to_read;
> + unsigned long flags, offset;
> + struct cs_buffers *buf = sink_config;
> + struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> + if (!buf)
> + return;
> +
> + capacity = drvdata->buffer_depth * ETB_FRAME_SIZE_WORDS;
> +
> + spin_lock_irqsave(&drvdata->spinlock, flags);
This spinlock seems to be held over the entire readout operation,
however, I can't find clear rules wrt what structures etc are serialized
on it. Instead, the comment says "only one at a time pls". Same for
etm's big drvdata spinlock.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V8 18/23] coresight: etm-perf: new PMU driver for ETM tracers
Mathieu Poirier writes:
> +static int etm_event_init(struct perf_event *event)
> +{
> + if (event->attr.type != etm_pmu.type)
> + return -ENOENT;
> +
> + if (event->cpu >= nr_cpu_ids)
> + return -EINVAL;
perf_event_alloc() already does this. Except for this one doesn't cover
the negative space.
[snip]
> +static void etm_free_aux(void *data)
> +{
> + struct etm_event_data *event_data = data;
> +
> + pr_err("Queing work\n");
Probably not pr_err().
> + schedule_work(&event_data->work);
> +}
[snip]
> +static void etm_event_start(struct perf_event *event, int flags)
> +{
> + int cpu = smp_processor_id();
> + struct etm_event_data *event_data;
> + struct perf_output_handle *handle = this_cpu_ptr(&ctx_handle);
> + struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu);
> +
> + if (!csdev)
> + goto fail;
> +
> + /*
> + * Deal with the ring buffer API and get a handle on the
> + * session's information.
> + */
> + event_data = perf_aux_output_begin(handle, event);
> + if (WARN_ON_ONCE(!event_data))
> + goto fail;
There really shouldn't be a warning here. I understand that the 'no
buffer' case is taped over by the !csdev check above, but there are
other ligitimate reasons for perf_aux_output_begin() to return NULL,
like no-space-left.
> +
> + /* We need a sink, no need to continue without one */
> + sink = coresight_get_sink(event_data->path[cpu]);
> + if (!sink || !sink_ops(sink)->set_buffer)
> + goto fail_end_stop;
Is this possible after the coresight_build_path() things in setup_aux?
Might be a better candidate for WARN_*ONCE().
> +
> + /* Configure the sink */
> + if (sink_ops(sink)->set_buffer(sink, handle,
> +event_data->snk_config))
> + goto fail_end_stop;
> +
> + /* Nothing will happen without a path */
> + if (coresight_enable_path(event_data->path[cpu], CS_MODE_PERF))
> + goto fail_end_stop;
I'd like to understand all the potential failures here, because it's
really a good idea to keep those to a minimum for the sake of
consistency. That is, if the user succeeded in creating an event, about
the only good reason for the event not starting is a filled up buffer.
This is why it makes a lot of sense to keep all the
coresight_build_path()/coresight_enable_path() to the .event_init()
phase and let them fail early, if they should fail.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
