[PATCH 1/3] x86, perf: Make merge_attr global to use from perf_event_intel

[PATCH 1/3] x86, perf: Make merge_attr global to use from perf_event_intel

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

merge_attr allows to merge two sysfs attribute tables.
Export it to be usable by other files too.

Next patch is going to use that to extend the sysfs format
attributes for a CPU.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event.c | 2 +-
 arch/x86/kernel/cpu/perf_event.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 5801a14..81e2a74 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1561,7 +1561,7 @@ static void __init filter_events(struct attribute **attrs)
 }
 
 /* Merge two pointer arrays */
-static __init struct attribute **merge_attr(struct attribute **a, struct attribute **b)
+__init struct attribute **merge_attr(struct attribute **a, struct attribute **b)
 {
 	struct attribute **new;
 	int j, i;
diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
index 0db9de4..3598f67 100644
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -795,6 +795,8 @@ static inline void set_linear_ip(struct pt_regs *regs, unsigned long ip)
 ssize_t x86_event_sysfs_show(char *page, u64 config, u64 event);
 ssize_t intel_event_sysfs_show(char *page, u64 config);
 
+struct attribute **merge_attr(struct attribute **a, struct attribute **b);
+
 #ifdef CONFIG_CPU_SUP_AMD
 
 int amd_pmu_init(void);
@@ -940,6 +942,7 @@ static inline int is_ht_workaround_enabled(void)
 {
 	return !!(x86_pmu.flags & PMU_FL_EXCL_ENABLED);
 }
+
 #else /* CONFIG_CPU_SUP_INTEL */
 
 static inline void reserve_ds_buffers(void)
-- 
2.4.2

[PATCH 2/3] x86, perf: Support custom test values for extra_regs

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Current kernels always test extra registers at boot with RMW
cycle, to catch lying virtual machines.

For a new register the standard 0x1ff test value does not work,
as 0x1ff is not a valid value and causes an #GP.

Add the ability to add custom test values to an extra_reg
into the description tables.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event.h                    | 15 +++++++++++----
 arch/x86/kernel/cpu/perf_event_intel.c              |  3 ++-
 arch/x86/kernel/cpu/perf_event_intel_uncore_nhmex.c |  5 +++--
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
index 3598f67..2a4701c 100644
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -422,23 +422,30 @@ struct extra_reg {
 	u64			valid_mask;
 	int			idx;  /* per_xxx->regs[] reg index */
 	bool			extra_msr_access;
+	u64			test_value;
 };
 
-#define EVENT_EXTRA_REG(e, ms, m, vm, i) {	\
+#define EVENT_EXTRA_REG(e, ms, m, vm, i, t) {	\
 	.event = (e),			\
 	.msr = (ms),			\
 	.config_mask = (m),		\
 	.valid_mask = (vm),		\
 	.idx = EXTRA_REG_##i,		\
 	.extra_msr_access = true,	\
+	.test_value = t,		\
 	}
 
 #define INTEL_EVENT_EXTRA_REG(event, msr, vm, idx)	\
-	EVENT_EXTRA_REG(event, msr, ARCH_PERFMON_EVENTSEL_EVENT, vm, idx)
+	EVENT_EXTRA_REG(event, msr, ARCH_PERFMON_EVENTSEL_EVENT, vm, idx, \
+			0x1ffUL)
 
 #define INTEL_UEVENT_EXTRA_REG(event, msr, vm, idx) \
 	EVENT_EXTRA_REG(event, msr, ARCH_PERFMON_EVENTSEL_EVENT | \
-			ARCH_PERFMON_EVENTSEL_UMASK, vm, idx)
+			ARCH_PERFMON_EVENTSEL_UMASK, vm, idx, 0x1ffUL)
+
+#define INTEL_UEVENT_EXTRA_REG_TVAL(event, msr, vm, idx, tval) \
+	EVENT_EXTRA_REG(event, msr, ARCH_PERFMON_EVENTSEL_EVENT | \
+			ARCH_PERFMON_EVENTSEL_UMASK, vm, idx, tval)
 
 #define INTEL_UEVENT_PEBS_LDLAT_EXTRA_REG(c) \
 	INTEL_UEVENT_EXTRA_REG(c, \
@@ -446,7 +453,7 @@ struct extra_reg {
 			       0xffff, \
 			       LDLAT)
 
-#define EVENT_EXTRA_END EVENT_EXTRA_REG(0, 0, 0, 0, RSP_0)
+#define EVENT_EXTRA_END EVENT_EXTRA_REG(0, 0, 0, 0, RSP_0, 0)
 
 union perf_capabilities {
 	struct {
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index 7c397e8..4df6783 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -3575,7 +3575,8 @@ __init int intel_pmu_init(void)
 	 */
 	if (x86_pmu.extra_regs) {
 		for (er = x86_pmu.extra_regs; er->msr; er++) {
-			er->extra_msr_access = check_msr(er->msr, 0x1ffUL);
+			er->extra_msr_access = check_msr(er->msr,
+							 er->test_value);
 			/* Disable LBR select mapping */
 			if ((er->idx == EXTRA_REG_LBR) && !er->extra_msr_access)
 				x86_pmu.lbr_sel_map = NULL;
diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore_nhmex.c b/arch/x86/kernel/cpu/perf_event_intel_uncore_nhmex.c
index 2749965..5bfa493 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore_nhmex.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore_nhmex.c
@@ -136,11 +136,12 @@
 				NHMEX_M_PMON_CTL_FLAG_MODE)
 #define MBOX_INC_SEL_EXTAR_REG(c, r) \
 		EVENT_EXTRA_REG(MBOX_INC_SEL(c), NHMEX_M0_MSR_PMU_##r, \
-				MBOX_INC_SEL_MASK, (u64)-1, NHMEX_M_##r)
+				MBOX_INC_SEL_MASK, (u64)-1, NHMEX_M_##r, \
+				0x1ffUL)
 #define MBOX_SET_FLAG_SEL_EXTRA_REG(c, r) \
 		EVENT_EXTRA_REG(MBOX_SET_FLAG_SEL(c), NHMEX_M0_MSR_PMU_##r, \
 				MBOX_SET_FLAG_SEL_MASK, \
-				(u64)-1, NHMEX_M_##r)
+				(u64)-1, NHMEX_M_##r, 0x1ffUL)
 
 /* NHM-EX Rbox */
 #define NHMEX_R_MSR_GLOBAL_CTL			0xe00
-- 
2.4.2

[PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Skylake has a new FRONTEND_LATENCY PEBS event to accurate profile
frontend problems (like ITLB or decoding issues)

The new event is configured through a separate MSR, which selects
a range of sub events.

Define the extra MSR as a extra reg and export support for it
through sysfs.  To avoid duplicating the existing
tables use a new function to add new entries to existing tables.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/include/asm/msr-index.h       |  2 ++
 arch/x86/kernel/cpu/perf_event.h       |  1 +
 arch/x86/kernel/cpu/perf_event_intel.c | 11 ++++++++++-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 96a00de..a5371dc 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -133,6 +133,8 @@
 #define DEBUGCTLMSR_BTS_OFF_USR		(1UL << 10)
 #define DEBUGCTLMSR_FREEZE_LBRS_ON_PMI	(1UL << 11)
 
+#define MSR_PEBS_FRONTEND		0x000003f7
+
 #define MSR_IA32_POWER_CTL		0x000001fc
 
 #define MSR_IA32_MC0_CTL		0x00000400
diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
index 2a4701c..6bd7390 100644
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -47,6 +47,7 @@ enum extra_reg_type {
 	EXTRA_REG_RSP_1 = 1,	/* offcore_response_1 */
 	EXTRA_REG_LBR   = 2,	/* lbr_select */
 	EXTRA_REG_LDLAT = 3,	/* ld_lat_threshold */
+	EXTRA_REG_FE    = 4,    /* fe_* */
 
 	EXTRA_REG_MAX		/* number of entries needed */
 };
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index 4df6783..c6cac61 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -205,6 +205,7 @@ static struct extra_reg intel_skl_extra_regs[] __read_mostly = {
 	INTEL_UEVENT_EXTRA_REG(0x01b7, MSR_OFFCORE_RSP_0, 0x3fffff8fffull, RSP_0),
 	INTEL_UEVENT_EXTRA_REG(0x01bb, MSR_OFFCORE_RSP_1, 0x3fffff8fffull, RSP_1),
 	INTEL_UEVENT_PEBS_LDLAT_EXTRA_REG(0x01cd),
+	INTEL_UEVENT_EXTRA_REG_TVAL(0x01c6, MSR_PEBS_FRONTEND, 0x3fff17, FE, 0x11UL),
 	EVENT_EXTRA_END
 };
 
@@ -2875,6 +2876,8 @@ PMU_FORMAT_ATTR(offcore_rsp, "config1:0-63");
 
 PMU_FORMAT_ATTR(ldlat, "config1:0-15");
 
+PMU_FORMAT_ATTR(frontend, "config1:0-23");
+
 static struct attribute *intel_arch3_formats_attr[] = {
 	&format_attr_event.attr,
 	&format_attr_umask.attr,
@@ -2891,6 +2894,11 @@ static struct attribute *intel_arch3_formats_attr[] = {
 	NULL,
 };
 
+static struct attribute *skl_format_attr[] = {
+	&format_attr_frontend.attr,
+	NULL,
+};
+
 static __initconst const struct x86_pmu core_pmu = {
 	.name			= "core",
 	.handle_irq		= x86_pmu_handle_irq,
@@ -3500,7 +3508,8 @@ __init int intel_pmu_init(void)
 
 		x86_pmu.hw_config = hsw_hw_config;
 		x86_pmu.get_event_constraints = hsw_get_event_constraints;
-		x86_pmu.cpu_events = hsw_events_attrs;
+		x86_pmu.format_attrs = merge_attr(intel_arch3_formats_attr,
+						  skl_format_attr);
 		WARN_ON(!x86_pmu.format_attrs);
 		x86_pmu.cpu_events = hsw_events_attrs;
 		pr_cont("Skylake events, ");
-- 
2.4.2

Re: [PATCH 2/3] x86, perf: Support custom test values for extra_regs

[Peter Zijlstra]

On Mon, Jun 29, 2015 at 02:22:14PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Current kernels always test extra registers at boot with RMW
> cycle, to catch lying virtual machines.
> 
> For a new register the standard 0x1ff test value does not work,
> as 0x1ff is not a valid value and causes an #GP.
> 
> Add the ability to add custom test values to an extra_reg
> into the description tables.

Just wondering; is there not a test value that works for all registers?

Re: [PATCH 2/3] x86, perf: Support custom test values for extra_regs

[Andi Kleen]

On Tue, Jun 30, 2015 at 01:19:07PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 29, 2015 at 02:22:14PM -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > Current kernels always test extra registers at boot with RMW
> > cycle, to catch lying virtual machines.
> > 
> > For a new register the standard 0x1ff test value does not work,
> > as 0x1ff is not a valid value and causes an #GP.
> > 
> > Add the ability to add custom test values to an extra_reg
> > into the description tables.
> 
> Just wondering; is there not a test value that works for all registers?

0x11 will likely work everywhere.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Stephane Eranian]

On Mon, Jun 29, 2015 at 2:22 PM, Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> Skylake has a new FRONTEND_LATENCY PEBS event to accurate profile
> frontend problems (like ITLB or decoding issues)
>
> The new event is configured through a separate MSR, which selects
> a range of sub events.
>
> Define the extra MSR as a extra reg and export support for it
> through sysfs.  To avoid duplicating the existing
> tables use a new function to add new entries to existing tables.
>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  arch/x86/include/asm/msr-index.h       |  2 ++
>  arch/x86/kernel/cpu/perf_event.h       |  1 +
>  arch/x86/kernel/cpu/perf_event_intel.c | 11 ++++++++++-
>  3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 96a00de..a5371dc 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -133,6 +133,8 @@
>  #define DEBUGCTLMSR_BTS_OFF_USR                (1UL << 10)
>  #define DEBUGCTLMSR_FREEZE_LBRS_ON_PMI (1UL << 11)
>
> +#define MSR_PEBS_FRONTEND              0x000003f7
> +
>  #define MSR_IA32_POWER_CTL             0x000001fc
>
>  #define MSR_IA32_MC0_CTL               0x00000400
> diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
> index 2a4701c..6bd7390 100644
> --- a/arch/x86/kernel/cpu/perf_event.h
> +++ b/arch/x86/kernel/cpu/perf_event.h
> @@ -47,6 +47,7 @@ enum extra_reg_type {
>         EXTRA_REG_RSP_1 = 1,    /* offcore_response_1 */
>         EXTRA_REG_LBR   = 2,    /* lbr_select */
>         EXTRA_REG_LDLAT = 3,    /* ld_lat_threshold */
> +       EXTRA_REG_FE    = 4,    /* fe_* */
>
>         EXTRA_REG_MAX           /* number of entries needed */
>  };
> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> index 4df6783..c6cac61 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -205,6 +205,7 @@ static struct extra_reg intel_skl_extra_regs[] __read_mostly = {
>         INTEL_UEVENT_EXTRA_REG(0x01b7, MSR_OFFCORE_RSP_0, 0x3fffff8fffull, RSP_0),
>         INTEL_UEVENT_EXTRA_REG(0x01bb, MSR_OFFCORE_RSP_1, 0x3fffff8fffull, RSP_1),
>         INTEL_UEVENT_PEBS_LDLAT_EXTRA_REG(0x01cd),
> +       INTEL_UEVENT_EXTRA_REG_TVAL(0x01c6, MSR_PEBS_FRONTEND, 0x3fff17, FE, 0x11UL),

I believe this mask of 0x3fff17 is wrong and should instead be
0x7fffff based on the description of the FRONTEND
MSR I see in the SDM Table 18-54 (bit 0-22 are valid). Otherwise, some
valid latency values may be rejected.


>         EVENT_EXTRA_END
>  };
>
> @@ -2875,6 +2876,8 @@ PMU_FORMAT_ATTR(offcore_rsp, "config1:0-63");
>
>  PMU_FORMAT_ATTR(ldlat, "config1:0-15");
>
> +PMU_FORMAT_ATTR(frontend, "config1:0-23");
> +
>  static struct attribute *intel_arch3_formats_attr[] = {
>         &format_attr_event.attr,
>         &format_attr_umask.attr,
> @@ -2891,6 +2894,11 @@ static struct attribute *intel_arch3_formats_attr[] = {
>         NULL,
>  };
>
> +static struct attribute *skl_format_attr[] = {
> +       &format_attr_frontend.attr,
> +       NULL,
> +};
> +
>  static __initconst const struct x86_pmu core_pmu = {
>         .name                   = "core",
>         .handle_irq             = x86_pmu_handle_irq,
> @@ -3500,7 +3508,8 @@ __init int intel_pmu_init(void)
>
>                 x86_pmu.hw_config = hsw_hw_config;
>                 x86_pmu.get_event_constraints = hsw_get_event_constraints;
> -               x86_pmu.cpu_events = hsw_events_attrs;
> +               x86_pmu.format_attrs = merge_attr(intel_arch3_formats_attr,
> +                                                 skl_format_attr);
>                 WARN_ON(!x86_pmu.format_attrs);
>                 x86_pmu.cpu_events = hsw_events_attrs;
>                 pr_cont("Skylake events, ");
> --
> 2.4.2
>

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Andi Kleen]

> I believe this mask of 0x3fff17 is wrong and should instead be
> 0x7fffff based on the description of the FRONTEND
> MSR I see in the SDM Table 18-54 (bit 0-22 are valid). Otherwise, some
> valid latency values may be rejected.

No, my mask is correct.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Thomas Gleixner]

On Fri, 17 Jul 2015, Andi Kleen wrote:

> > I believe this mask of 0x3fff17 is wrong and should instead be
> > 0x7fffff based on the description of the FRONTEND
> > MSR I see in the SDM Table 18-54 (bit 0-22 are valid). Otherwise, some
> > valid latency values may be rejected.
> 
> No, my mask is correct.

Provide a proper argument for that. Just claiming 'my mask is correct'
definitely falls not into that category.

Thanks,

	tglx

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Andi Kleen]

On Fri, Jul 17, 2015 at 10:11:28PM +0200, Thomas Gleixner wrote:
> On Fri, 17 Jul 2015, Andi Kleen wrote:
> 
> > > I believe this mask of 0x3fff17 is wrong and should instead be
> > > 0x7fffff based on the description of the FRONTEND
> > > MSR I see in the SDM Table 18-54 (bit 0-22 are valid). Otherwise, some
> > > valid latency values may be rejected.
> > 
> > No, my mask is correct.
> 
> Provide a proper argument for that. Just claiming 'my mask is correct'
> definitely falls not into that category.

Because I actually tested the code unlike you or Stephane.

# wrmsr 0x3f7 0x3fff17
# wrmsr 0x3f7 0x7fffff
wrmsr: CPU 0 cannot set MSR 0x000003f7 to 0x00000000007fffff
# 

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Stephane Eranian]

Andi,

On Fri, Jul 17, 2015 at 1:09 PM, Andi Kleen <ak@linux.intel.com> wrote:
>> I believe this mask of 0x3fff17 is wrong and should instead be
>> 0x7fffff based on the description of the FRONTEND
>> MSR I see in the SDM Table 18-54 (bit 0-22 are valid). Otherwise, some
>> valid latency values may be rejected.
>
> No, my mask is correct.
>
Ok, so your event mask (0x17) really only allows what's defined
instead the full width of the field.

As for the IDQ_BUBBLE_WIDTH, you only allow 2 bits out of 3, so
maximum bubble threshold is 3
instead of 7. I assume this is because you know that it cannot have
more than 3 simultaneously then.

Would be good to explain this a bit more in the code.

> --
> ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Andi Kleen]

On Fri, Jul 17, 2015 at 01:41:04PM -0700, Stephane Eranian wrote:
> Andi,
> 
> On Fri, Jul 17, 2015 at 1:09 PM, Andi Kleen <ak@linux.intel.com> wrote:
> >> I believe this mask of 0x3fff17 is wrong and should instead be
> >> 0x7fffff based on the description of the FRONTEND
> >> MSR I see in the SDM Table 18-54 (bit 0-22 are valid). Otherwise, some
> >> valid latency values may be rejected.
> >
> > No, my mask is correct.
> >
> Ok, so your event mask (0x17) really only allows what's defined
> instead the full width of the field.
> 
> As for the IDQ_BUBBLE_WIDTH, you only allow 2 bits out of 3, so
> maximum bubble threshold is 3
> instead of 7. I assume this is because you know that it cannot have
> more than 3 simultaneously then.
> 
> Would be good to explain this a bit more in the code.

I don't think the code is the right place to document such registers.
I can ask for the SDM to be clarified. But if you follow the documented
events you will never set any undefined bits, so you won't need to care
about this. We just have to exclude it at the kernel level to avoid #GP crashes
if someone is writing random values.

-Andi

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Stephane Eranian]

On Fri, Jul 17, 2015 at 1:33 PM, Andi Kleen <ak@linux.intel.com> wrote:
> On Fri, Jul 17, 2015 at 10:11:28PM +0200, Thomas Gleixner wrote:
>> On Fri, 17 Jul 2015, Andi Kleen wrote:
>>
>> > > I believe this mask of 0x3fff17 is wrong and should instead be
>> > > 0x7fffff based on the description of the FRONTEND
>> > > MSR I see in the SDM Table 18-54 (bit 0-22 are valid). Otherwise, some
>> > > valid latency values may be rejected.
>> >
>> > No, my mask is correct.
>>
>> Provide a proper argument for that. Just claiming 'my mask is correct'
>> definitely falls not into that category.
>
> Because I actually tested the code unlike you or Stephane.
>
> # wrmsr 0x3f7 0x3fff17
> # wrmsr 0x3f7 0x7fffff
> wrmsr: CPU 0 cannot set MSR 0x000003f7 to 0x00000000007fffff
> #
Fair enough!
But then, the SDM is misleading. It is not describing what's
implemented for SKL.

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Peter Zijlstra]

On Fri, Jul 17, 2015 at 10:52:33PM +0200, Andi Kleen wrote:
> 
> I don't think the code is the right place to document such registers.

If the code deviates from the SDM, it _is_ the right place to explain,
seeing how its the only place.

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Andi Kleen]

On Fri, Jul 17, 2015 at 11:05:37PM +0200, Peter Zijlstra wrote:
> On Fri, Jul 17, 2015 at 10:52:33PM +0200, Andi Kleen wrote:
> > 
> > I don't think the code is the right place to document such registers.
> 
> If the code deviates from the SDM, it _is_ the right place to explain,
> seeing how its the only place.

It won't anymore in the next revision.
-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Andi Kleen]

> But then, the SDM is misleading. It is not describing what's
> implemented for SKL.

Actually it has a list of valid values you can put into the various fields.
None of them have the bits set you're trying to set.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Stephane Eranian]

Andi,

On Fri, Jul 17, 2015 at 2:19 PM, Andi Kleen <ak@linux.intel.com> wrote:
>> But then, the SDM is misleading. It is not describing what's
>> implemented for SKL.
>
> Actually it has a list of valid values you can put into the various fields.
> None of them have the bits set you're trying to set.
>
You are talking about the events (bit 0-7). I am talking about the bubble
thresholds. I am okay with the event list for bits 0-7.

> -Andi
>
> --
> ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Thomas Gleixner]

On Fri, 17 Jul 2015, Andi Kleen wrote:
> On Fri, Jul 17, 2015 at 10:11:28PM +0200, Thomas Gleixner wrote:
> > On Fri, 17 Jul 2015, Andi Kleen wrote:
> > 
> > > > I believe this mask of 0x3fff17 is wrong and should instead be
> > > > 0x7fffff based on the description of the FRONTEND
> > > > MSR I see in the SDM Table 18-54 (bit 0-22 are valid). Otherwise, some
> > > > valid latency values may be rejected.
> > > 
> > > No, my mask is correct.
> > 
> > Provide a proper argument for that. Just claiming 'my mask is correct'
> > definitely falls not into that category.
> 
> Because I actually tested the code unlike you or Stephane.

You know fcking well that I cannot test on skylake and probably
Stephane can't either.
 
> # wrmsr 0x3f7 0x3fff17
> # wrmsr 0x3f7 0x7fffff
> wrmsr: CPU 0 cannot set MSR 0x000003f7 to 0x00000000007fffff
> # 

So you think that's an explanation? No, it is NOT. It's merily an
observation on some particular piece of HW to which you have access
to exclusively.

Nobody asks you to reveal NDA information, but you should at least
have the courtesy to provide an explanation which is understandable to
others. The minimal explanation you could have provided would have been:

  "On my test machine the valid mask is 0x3fff17 contrary to the SDM
   which defines the valid mask as 0x7fffff"

'My mask is correct' is definitely not an explanation at all.

Thanks,

	tglx

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Thomas Gleixner]

On Fri, 17 Jul 2015, Andi Kleen wrote:
> On Fri, Jul 17, 2015 at 11:05:37PM +0200, Peter Zijlstra wrote:
> > On Fri, Jul 17, 2015 at 10:52:33PM +0200, Andi Kleen wrote:
> > > 
> > > I don't think the code is the right place to document such registers.
> > 
> > If the code deviates from the SDM, it _is_ the right place to explain,
> > seeing how its the only place.
> 
> It won't anymore in the next revision.

And that justifies to leave completely undocumented crap in the code
for the current revision?

> ak@linux.intel.com -- Speaking for myself only

I'm sure Intel appreciates that disclaimer.

Thanks,

	tglx

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Andi Kleen]

On Fri, Jul 17, 2015 at 03:00:18PM -0700, Stephane Eranian wrote:
> Andi,
> 
> On Fri, Jul 17, 2015 at 2:19 PM, Andi Kleen <ak@linux.intel.com> wrote:
> >> But then, the SDM is misleading. It is not describing what's
> >> implemented for SKL.
> >
> > Actually it has a list of valid values you can put into the various fields.
> > None of them have the bits set you're trying to set.
> >
> You are talking about the events (bit 0-7). I am talking about the bubble
> thresholds. I am okay with the event list for bits 0-7.

Fair enough. There's a one-off in the MSR table and table 18-54. The IDQ
bubble width is only 21:20. I'll ask for that to be fixed in both places
that document them.

Thanks.

-Andi

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Stephane Eranian]

On Fri, Jul 17, 2015 at 4:31 PM, Andi Kleen <ak@linux.intel.com> wrote:
> On Fri, Jul 17, 2015 at 03:00:18PM -0700, Stephane Eranian wrote:
>> Andi,
>>
>> On Fri, Jul 17, 2015 at 2:19 PM, Andi Kleen <ak@linux.intel.com> wrote:
>> >> But then, the SDM is misleading. It is not describing what's
>> >> implemented for SKL.
>> >
>> > Actually it has a list of valid values you can put into the various fields.
>> > None of them have the bits set you're trying to set.
>> >
>> You are talking about the events (bit 0-7). I am talking about the bubble
>> thresholds. I am okay with the event list for bits 0-7.
>
> Fair enough. There's a one-off in the MSR table and table 18-54. The IDQ
> bubble width is only 21:20. I'll ask for that to be fixed in both places
> that document them.
>
There is still something broken here, if bit 22 is not implemented,
then you have
a bunch of frontend events in the SKL event table (download.01.org) which are
bogus:

{
    "EventCode": "0xC6",
    "UMask": "0x01",
    "EventName": "FRONTEND_RETIRED.LATENCY_GE_8",
    "BriefDescription": "Retired instructions that are fetched after
an interval where the front-end delivered no uops for a period of 8
cycles which was not interrupted by a back-end stall.",
    "PublicDescription": "Retired instructions that are fetched after
an interval where the front-end delivered no uops for a period of 8
cycles which was not interrupted by a back-end stall.",
    "Counter": "0,1,2,3",
    "CounterHTOff": "0,1,2,3",
    "SampleAfterValue": "100007",
    "MSRIndex": "0x3F7",
    "MSRValue": "0x400806",   <=========
    ....
  },
  {
    "EventCode": "0xC6",
    "UMask": "0x01",
    "EventName": "FRONTEND_RETIRED.LATENCY_GE_16",
    "BriefDescription": "Retired instructions that are fetched after
an interval where the front-end delivered no uops for a period of 16
cycles which was not interrupted by a back-end stall.",
    "PublicDescription": "Retired instructions that are fetched after
an interval where the front-end delivered no uops for a period of 16
cycles which was not interrupted by a back-end stall.",
    "Counter": "0,1,2,3",
    "CounterHTOff": "0,1,2,3",
    "SampleAfterValue": "100007",
    "MSRIndex": "0x3F7",
    "MSRValue": "0x401006", <=====
    ...
     },
  {
    "EventCode": "0xC6",
    "UMask": "0x01",
    "EventName": "FRONTEND_RETIRED.LATENCY_GE_32",
    "BriefDescription": "Retired instructions that are fetched after
an interval where the front-end delivered no uops for a period of 32
cycles which was not interrupted by a back-end stall.",
    "PublicDescription": "Retired instructions that are fetched after
an interval where the front-end delivered no uops for a period of 32
cycles which was not interrupted by a back-end stall.",
    "Counter": "0,1,2,3",
    "CounterHTOff": "0,1,2,3",
    "SampleAfterValue": "100007",
    "MSRIndex": "0x3F7",
    "MSRValue": "0x402006", <=========
    ...
  },

Re: [PATCH 3/3] x86, perf: Add PEBS frontend profiling for Skylake

[Andi Kleen]

On Fri, Jul 17, 2015 at 04:52:56PM -0700, Stephane Eranian wrote:
> On Fri, Jul 17, 2015 at 4:31 PM, Andi Kleen <ak@linux.intel.com> wrote:
> > On Fri, Jul 17, 2015 at 03:00:18PM -0700, Stephane Eranian wrote:
> >> Andi,
> >>
> >> On Fri, Jul 17, 2015 at 2:19 PM, Andi Kleen <ak@linux.intel.com> wrote:
> >> >> But then, the SDM is misleading. It is not describing what's
> >> >> implemented for SKL.
> >> >
> >> > Actually it has a list of valid values you can put into the various fields.
> >> > None of them have the bits set you're trying to set.
> >> >
> >> You are talking about the events (bit 0-7). I am talking about the bubble
> >> thresholds. I am okay with the event list for bits 0-7.
> >
> > Fair enough. There's a one-off in the MSR table and table 18-54. The IDQ
> > bubble width is only 21:20. I'll ask for that to be fixed in both places
> > that document them.
> >
> There is still something broken here, if bit 22 is not implemented,
> then you have
> a bunch of frontend events in the SKL event table (download.01.org) which are
> bogus:

I double checked and bit 22 is actually implemented. I'll send a patch to 
update the mask in the driver. Thanks for the report.

-Andi

[tip:perf/core] perf/x86: Make merge_attr() global to use from perf_event_intel

[tip-bot for Andi Kleen]

Commit-ID:  47732d886385af769449022a02c7cf0ce45d8a5c
Gitweb:     http://git.kernel.org/tip/47732d886385af769449022a02c7cf0ce45d8a5c
Author:     Andi Kleen <ak@linux.intel.com>
AuthorDate: Mon, 29 Jun 2015 14:22:13 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 4 Aug 2015 10:16:59 +0200

perf/x86: Make merge_attr() global to use from perf_event_intel

merge_attr() allows to merge two sysfs attribute tables.
Export it to be usable by other files too.

Next patch is going to use that to extend the sysfs format
attributes for a CPU.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: eranian@google.com
Link: http://lkml.kernel.org/r/1435612935-24425-1-git-send-email-andi@firstfloor.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/perf_event.c | 2 +-
 arch/x86/kernel/cpu/perf_event.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 3658de4..8bac4bb 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1551,7 +1551,7 @@ static void __init filter_events(struct attribute **attrs)
 }
 
 /* Merge two pointer arrays */
-static __init struct attribute **merge_attr(struct attribute **a, struct attribute **b)
+__init struct attribute **merge_attr(struct attribute **a, struct attribute **b)
 {
 	struct attribute **new;
 	int j, i;
diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
index 8ad9241..5edf6d8 100644
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -795,6 +795,8 @@ static inline void set_linear_ip(struct pt_regs *regs, unsigned long ip)
 ssize_t x86_event_sysfs_show(char *page, u64 config, u64 event);
 ssize_t intel_event_sysfs_show(char *page, u64 config);
 
+struct attribute **merge_attr(struct attribute **a, struct attribute **b);
+
 #ifdef CONFIG_CPU_SUP_AMD
 
 int amd_pmu_init(void);
@@ -926,6 +928,7 @@ static inline int is_ht_workaround_enabled(void)
 {
 	return !!(x86_pmu.flags & PMU_FL_EXCL_ENABLED);
 }
+
 #else /* CONFIG_CPU_SUP_INTEL */
 
 static inline void reserve_ds_buffers(void)

[PATCH 1/3] x86, perf: Make merge_attr global to use from perf_event_intel

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

merge_attr allows to merge two sysfs attribute tables.
Export it to be usable by other files too.

Next patch is going to use that to extend the sysfs format
attributes for a CPU.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event.c | 2 +-
 arch/x86/kernel/cpu/perf_event.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 5801a14..81e2a74 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1561,7 +1561,7 @@ static void __init filter_events(struct attribute **attrs)
 }
 
 /* Merge two pointer arrays */
-static __init struct attribute **merge_attr(struct attribute **a, struct attribute **b)
+__init struct attribute **merge_attr(struct attribute **a, struct attribute **b)
 {
 	struct attribute **new;
 	int j, i;
diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
index 0db9de4..3598f67 100644
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -795,6 +795,8 @@ static inline void set_linear_ip(struct pt_regs *regs, unsigned long ip)
 ssize_t x86_event_sysfs_show(char *page, u64 config, u64 event);
 ssize_t intel_event_sysfs_show(char *page, u64 config);
 
+struct attribute **merge_attr(struct attribute **a, struct attribute **b);
+
 #ifdef CONFIG_CPU_SUP_AMD
 
 int amd_pmu_init(void);
@@ -940,6 +942,7 @@ static inline int is_ht_workaround_enabled(void)
 {
 	return !!(x86_pmu.flags & PMU_FL_EXCL_ENABLED);
 }
+
 #else /* CONFIG_CPU_SUP_INTEL */
 
 static inline void reserve_ds_buffers(void)
-- 
2.4.2