[PATCH] perf, p4: Add format attributes

[PATCH] perf, p4: Add format attributes

[Peter Zijlstra]

Subject: perf, p4: Add format attributes
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Tue Mar 27 16:19:25 CEST 2012

Steven reported his P4 not booting properly, the missing format
attributes cause a NULL ptr deref. Cure this by adding the missing
format specification.

I took the format description out of the comment near 
p4_config_pack*() and hope that comment is still relatively 
accurate.

Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Lin Ming <ming.m.lin@intel.com>
Reported-by: Steven Rostedt <rostedt@goodmis.org>
Tested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 arch/x86/kernel/cpu/perf_event_p4.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

--- a/arch/x86/kernel/cpu/perf_event_p4.c
+++ b/arch/x86/kernel/cpu/perf_event_p4.c
@@ -1271,6 +1271,17 @@ static int p4_pmu_schedule_events(struct
 	return num ? -EINVAL : 0;
 }
 
+PMU_FORMAT_ATTR(cccr, "config:0-31" );
+PMU_FORMAT_ATTR(escr, "config:32-62");
+PMU_FORMAT_ATTR(ht,   "config:63"   );
+
+static struct attribute *intel_p4_formats_attr[] = {
+	&format_attr_cccr.attr,
+	&format_attr_escr.attr,
+	&format_attr_ht.attr,
+	NULL,
+};
+
 static __initconst const struct x86_pmu p4_pmu = {
 	.name			= "Netburst P4/Xeon",
 	.handle_irq		= p4_pmu_handle_irq,
@@ -1305,6 +1316,8 @@ static __initconst const struct x86_pmu
 	 * the former idea is taken from OProfile code
 	 */
 	.perfctr_second_write	= 1,
+
+	.format_attrs		= intel_p4_formats_attr,
 };
 
 __init int p4_pmu_init(void)

Re: [PATCH] perf, p4: Add format attributes

[Cyrill Gorcunov]

On Tue, Mar 27, 2012 at 04:50:42PM +0200, Peter Zijlstra wrote:
> Subject: perf, p4: Add format attributes
> From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Date: Tue Mar 27 16:19:25 CEST 2012
> 
> Steven reported his P4 not booting properly, the missing format
> attributes cause a NULL ptr deref. Cure this by adding the missing
> format specification.
> 
> I took the format description out of the comment near 
> p4_config_pack*() and hope that comment is still relatively 
> accurate.
> 
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Lin Ming <ming.m.lin@intel.com>
> Reported-by: Steven Rostedt <rostedt@goodmis.org>
> Tested-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

Thanks a lot, Peter! This format description is somewhat
new to me (I think I've missed when it was introduced first
time).

Reviewed-by: Cyrill Gorcunov <gorcunov@openvz.org>

	Cyrill

Re: [PATCH] perf, p4: Add format attributes

[Cyrill Gorcunov]

On Tue, Mar 27, 2012 at 06:57:28PM +0400, Cyrill Gorcunov wrote:
> 
> Thanks a lot, Peter! This format description is somewhat
> new to me (I think I've missed when it was introduced first
> time).

By format description I mean of course PMU_FORMAT_ATTR :)

	Cyrill

Re: [PATCH] perf, p4: Add format attributes

[Peter Zijlstra]

On Tue, 2012-03-27 at 18:57 +0400, Cyrill Gorcunov wrote:
> On Tue, Mar 27, 2012 at 04:50:42PM +0200, Peter Zijlstra wrote:
> > Subject: perf, p4: Add format attributes
> > From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Date: Tue Mar 27 16:19:25 CEST 2012
> > 
> > Steven reported his P4 not booting properly, the missing format
> > attributes cause a NULL ptr deref. Cure this by adding the missing
> > format specification.
> > 
> > I took the format description out of the comment near 
> > p4_config_pack*() and hope that comment is still relatively 
> > accurate.
> > 
> > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> > Cc: Lin Ming <ming.m.lin@intel.com>
> > Reported-by: Steven Rostedt <rostedt@goodmis.org>
> > Tested-by: Steven Rostedt <rostedt@goodmis.org>
> > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> 
> Thanks a lot, Peter! This format description is somewhat
> new to me (I think I've missed when it was introduced first
> time).

Yeah, its all brand-spanking new stuff.. see
https://lkml.org/lkml/2012/1/16/146 , Jiri added a full fledged
bison/yacc parser to perf so we can now write events like:

perf stat -e cpu/event=0xb7,umask=0x01,offcore_rsp=0x500b/

and it uses /sys/bus/event_source/devices/cpu/format/* to figure out how
to convert that into perf_event_attr::config{,1,2} values.

So with this patch you could, on your P4, write:

perf stat -e cpu/cccr=0x1234,escr=0x4321,ht/

and it would construct the perf_event_attr::config value 0x92344321 for
you.

I've still not actually read the P4-PMU specs, so I don't know if
there's anything more convenient we can do on P4. If there's a
better/more useful format representation possible, don't hesitate to
send a patch! :-)

> Reviewed-by: Cyrill Gorcunov <gorcunov@openvz.org>

Thanks!

Re: [PATCH] perf, p4: Add format attributes

[Peter Zijlstra]

On Tue, 2012-03-27 at 17:05 +0200, Peter Zijlstra wrote:
> perf stat -e cpu/cccr=0x1234,escr=0x4321,ht/
> 
> and it would construct the perf_event_attr::config value 0x92344321
> for
> you. 

Or rather, 0xc3211234, I got cccr and escr reversed.

Re: [PATCH] perf, p4: Add format attributes

[Cyrill Gorcunov]

On Tue, Mar 27, 2012 at 05:05:59PM +0200, Peter Zijlstra wrote:
> > 
> > Thanks a lot, Peter! This format description is somewhat
> > new to me (I think I've missed when it was introduced first
> > time).
> 
> Yeah, its all brand-spanking new stuff.. see
> https://lkml.org/lkml/2012/1/16/146 , Jiri added a full fledged
> bison/yacc parser to perf so we can now write events like:
> 
> perf stat -e cpu/event=0xb7,umask=0x01,offcore_rsp=0x500b/
> 
> and it uses /sys/bus/event_source/devices/cpu/format/* to figure out how
> to convert that into perf_event_attr::config{,1,2} values.
> 
> So with this patch you could, on your P4, write:
> 
> perf stat -e cpu/cccr=0x1234,escr=0x4321,ht/
> 
> and it would construct the perf_event_attr::config value 0x92344321 for
> you.
> 

Ah, I see thanks for the reference, Peter!

> I've still not actually read the P4-PMU specs, so I don't know if
> there's anything more convenient we can do on P4. If there's a
> better/more useful format representation possible, don't hesitate to
> send a patch! :-)

Sure, I'll try to find some time tonight to read this brand
new stuff. And if I find something -- will send out. At moment
your patch should be more than enough ;)

	Cyrill

Re: [PATCH] perf, p4: Add format attributes

[Cyrill Gorcunov]

On Tue, Mar 27, 2012 at 07:12:50PM +0400, Cyrill Gorcunov wrote:
> 
> Ah, I see thanks for the reference, Peter!
> 
> > I've still not actually read the P4-PMU specs, so I don't know if
> > there's anything more convenient we can do on P4. If there's a
> > better/more useful format representation possible, don't hesitate to
> > send a patch! :-)
> 
> Sure, I'll try to find some time tonight to read this brand
> new stuff. And if I find something -- will send out. At moment
> your patch should be more than enough ;)

Hi, Peter, sorry for delay,

after looking some more I think ht bit might be dropped from
the format, it's used internally by the kernel in attmept to
eliminate redundant msr poking if new event runs on the same
thread as previous one, and it's always cleared before real
write to msr happens, which means user-space might set or
clear it, but it never be considered by the kernel.

(still my earlier reviewed-by is valid because this ht bit
 is harmless ;)

	Cyrill

Re: [PATCH] perf, p4: Add format attributes

[Peter Zijlstra]

On Wed, 2012-03-28 at 02:03 +0400, Cyrill Gorcunov wrote:
> On Tue, Mar 27, 2012 at 07:12:50PM +0400, Cyrill Gorcunov wrote:
> > 
> > Ah, I see thanks for the reference, Peter!
> > 
> > > I've still not actually read the P4-PMU specs, so I don't know if
> > > there's anything more convenient we can do on P4. If there's a
> > > better/more useful format representation possible, don't hesitate to
> > > send a patch! :-)
> > 
> > Sure, I'll try to find some time tonight to read this brand
> > new stuff. And if I find something -- will send out. At moment
> > your patch should be more than enough ;)
> 
> Hi, Peter, sorry for delay,
> 
> after looking some more I think ht bit might be dropped from
> the format, it's used internally by the kernel in attmept to
> eliminate redundant msr poking if new event runs on the same
> thread as previous one, and it's always cleared before real
> write to msr happens, which means user-space might set or
> clear it, but it never be considered by the kernel.
> 
> (still my earlier reviewed-by is valid because this ht bit
>  is harmless ;)

So does that make escr:32-62 or should that become 32-63 ?

Re: [PATCH] perf, p4: Add format attributes

[Cyrill Gorcunov]

On Wed, Mar 28, 2012 at 12:05:05AM +0200, Peter Zijlstra wrote:
> 
> So does that make escr:32-62 or should that become 32-63 ?

32-63 I think will be better, because it's raw format for
escr, simply 63 bit is never used in hw level so we borrowed
it for own needs.

	Cyrill

[tip:perf/urgent] perf/x86/p4: Add format attributes

[tip-bot for Peter Zijlstra]

Commit-ID:  7b8e6da46b921d30ac1553cac56d8fb74f0b431d
Gitweb:     http://git.kernel.org/tip/7b8e6da46b921d30ac1553cac56d8fb74f0b431d
Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
AuthorDate: Tue, 27 Mar 2012 16:50:42 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 3 Apr 2012 08:33:38 +0200

perf/x86/p4: Add format attributes

Steven reported his P4 not booting properly, the missing format
attributes cause a NULL ptr deref. Cure this by adding the
missing format specification.

I took the format description out of the comment near
p4_config_pack*() and hope that comment is still relatively
accurate.

Reported-by: Steven Rostedt <rostedt@goodmis.org>
Reported-by: Bruno Prémont <bonbons@linux-vserver.org>
Tested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Lin Ming <ming.m.lin@intel.com>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1332859842.16159.227.camel@twins
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/perf_event_p4.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_p4.c b/arch/x86/kernel/cpu/perf_event_p4.c
index ef484d9..a2dfacf 100644
--- a/arch/x86/kernel/cpu/perf_event_p4.c
+++ b/arch/x86/kernel/cpu/perf_event_p4.c
@@ -1271,6 +1271,17 @@ done:
 	return num ? -EINVAL : 0;
 }
 
+PMU_FORMAT_ATTR(cccr, "config:0-31" );
+PMU_FORMAT_ATTR(escr, "config:32-62");
+PMU_FORMAT_ATTR(ht,   "config:63"   );
+
+static struct attribute *intel_p4_formats_attr[] = {
+	&format_attr_cccr.attr,
+	&format_attr_escr.attr,
+	&format_attr_ht.attr,
+	NULL,
+};
+
 static __initconst const struct x86_pmu p4_pmu = {
 	.name			= "Netburst P4/Xeon",
 	.handle_irq		= p4_pmu_handle_irq,
@@ -1305,6 +1316,8 @@ static __initconst const struct x86_pmu p4_pmu = {
 	 * the former idea is taken from OProfile code
 	 */
 	.perfctr_second_write	= 1,
+
+	.format_attrs		= intel_p4_formats_attr,
 };
 
 __init int p4_pmu_init(void)