[PATCH V3] perf/x86/intel/uncore: remove nonexistent clockticks event for client uncore

[kan . liang]

From: Kan Liang <Kan.liang@xxxxxxxxx>

For client uncore, all CBOXes seems have their own clockticks event
uncore_cbox_*/clockticks/. However, only the clockticks event for CBOX_0
can work. Accessing other CBOX clockticks evnet will always return
EINVAL. Here is an example.
    $ perf stat -x, -e
      uncore_cbox_0/clockticks/,uncore_cbox_1/clockticks/ sleep 1
    10660314,,uncore_cbox_0/clockticks/,1001596935,100.00,,,,
    <not supported>,,uncore_cbox_1/clockticks/,0,100.00,,,,

There is only one clockticks fixed counter in Hardware.
Current kernel only allow the first CBOX to access the fixed counter.
However, it does not expose the limitation to user space.
User can still see the nonexistent clockticks event.

Simply removing the clockticks event for the rest CBOX does not work.
Because all boxes share the same attr_groups. If it is set for one box,
it will applied to other boxes.

The clockticks fixed counter is a standalone counter. It should be
removed from the CBOX PMUs. A new type of PMU is added which only
supports fixed counter events.

User observable change with the patch.
clockticks event is removed from CBOX. User may need to change their
script to use uncore_clock/clockticks/ instead.
    $ perf stat -x, -e uncore_clock/clockticks/ sleep 1
    10421811,,uncore_clock/clockticks/,1001581963,100.00,,,,

Reported-by: Jiri Olsa <jolsa@xxxxxxxxxx>
Tested-by: Jiri Olsa <jolsa@xxxxxxxxxx>
Signed-off-by: Kan Liang <Kan.liang@xxxxxxxxx>
---

The report and discussion can be found here.
https://lkml.org/lkml/2016/11/30/277

Changes since V2:
 - Refine changelog

Changes since V1:
 - Refine changelog
 - Add Tested-by

 arch/x86/events/intel/uncore.c     | 13 +++++++------
 arch/x86/events/intel/uncore_snb.c | 38 +++++++++++++++++++++++++++-----------
 2 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 8106df7db758..26a93f7a6003 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -658,6 +658,10 @@ static int uncore_pmu_event_init(struct perf_event *event)
 	if (hwc->sample_period)
 		return -EINVAL;
 
+	/* Single fixed PMU only has fixed event */
+	if (pmu->type->single_fixed && (event->attr.config != UNCORE_FIXED_EVENT))
+		return -EINVAL;
+
 	/*
 	 * Place all uncore events for a particular physical package
 	 * onto a single cpu
@@ -681,12 +685,9 @@ static int uncore_pmu_event_init(struct perf_event *event)
 		/* no fixed counter */
 		if (!pmu->type->fixed_ctl)
 			return -EINVAL;
-		/*
-		 * if there is only one fixed counter, only the first pmu
-		 * can access the fixed counter
-		 */
-		if (pmu->type->single_fixed && pmu->pmu_idx > 0)
-			return -EINVAL;
+
+		if (pmu->type->single_fixed)
+			event->hw.idx = UNCORE_PMC_IDX_FIXED;
 
 		/* fixed counters have event field hardcoded to zero */
 		hwc->config = 0ULL;
diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c
index dda44302207a..4090d872883c 100644
--- a/arch/x86/events/intel/uncore_snb.c
+++ b/arch/x86/events/intel/uncore_snb.c
@@ -122,7 +122,7 @@ static void snb_uncore_msr_exit_box(struct intel_uncore_box *box)
 }
 
 static struct uncore_event_desc snb_uncore_events[] = {
-	INTEL_UNCORE_EVENT_DESC(clockticks, "event=0xff,umask=0x00"),
+	INTEL_UNCORE_EVENT_DESC(clockticks, "event=0xff"),
 	{ /* end: all zeroes */ },
 };
 
@@ -160,17 +160,12 @@ static struct intel_uncore_type snb_uncore_cbox = {
 	.num_counters   = 2,
 	.num_boxes	= 4,
 	.perf_ctr_bits	= 44,
-	.fixed_ctr_bits	= 48,
 	.perf_ctr	= SNB_UNC_CBO_0_PER_CTR0,
 	.event_ctl	= SNB_UNC_CBO_0_PERFEVTSEL0,
-	.fixed_ctr	= SNB_UNC_FIXED_CTR,
-	.fixed_ctl	= SNB_UNC_FIXED_CTR_CTRL,
-	.single_fixed	= 1,
 	.event_mask	= SNB_UNC_RAW_EVENT_MASK,
 	.msr_offset	= SNB_UNC_CBO_MSR_OFFSET,
 	.ops		= &snb_uncore_msr_ops,
 	.format_group	= &snb_uncore_format_group,
-	.event_descs	= snb_uncore_events,
 };
 
 static struct intel_uncore_type snb_uncore_arb = {
@@ -187,9 +182,34 @@ static struct intel_uncore_type snb_uncore_arb = {
 	.format_group	= &snb_uncore_format_group,
 };
 
+static struct attribute *snb_uncore_clock_formats_attr[] = {
+	&format_attr_event.attr,
+	NULL,
+};
+
+static struct attribute_group snb_uncore_clock_format_group = {
+	.name = "format",
+	.attrs = snb_uncore_clock_formats_attr,
+};
+
+static struct intel_uncore_type snb_uncore_clockbox = {
+	.name		= "clock",
+	.num_counters   = 1,
+	.num_boxes	= 1,
+	.fixed_ctr_bits	= 48,
+	.fixed_ctr	= SNB_UNC_FIXED_CTR,
+	.fixed_ctl	= SNB_UNC_FIXED_CTR_CTRL,
+	.single_fixed	= 1,
+	.event_mask	= SNB_UNC_CTL_EV_SEL_MASK,
+	.format_group	= &snb_uncore_clock_format_group,
+	.ops		= &snb_uncore_msr_ops,
+	.event_descs	= snb_uncore_events,
+};
+
 static struct intel_uncore_type *snb_msr_uncores[] = {
 	&snb_uncore_cbox,
 	&snb_uncore_arb,
+	&snb_uncore_clockbox,
 	NULL,
 };
 
@@ -234,22 +254,18 @@ static struct intel_uncore_type skl_uncore_cbox = {
 	.num_counters   = 4,
 	.num_boxes	= 5,
 	.perf_ctr_bits	= 44,
-	.fixed_ctr_bits	= 48,
 	.perf_ctr	= SNB_UNC_CBO_0_PER_CTR0,
 	.event_ctl	= SNB_UNC_CBO_0_PERFEVTSEL0,
-	.fixed_ctr	= SNB_UNC_FIXED_CTR,
-	.fixed_ctl	= SNB_UNC_FIXED_CTR_CTRL,
-	.single_fixed	= 1,
 	.event_mask	= SNB_UNC_RAW_EVENT_MASK,
 	.msr_offset	= SNB_UNC_CBO_MSR_OFFSET,
 	.ops		= &skl_uncore_msr_ops,
 	.format_group	= &snb_uncore_format_group,
-	.event_descs	= snb_uncore_events,
 };
 
 static struct intel_uncore_type *skl_msr_uncores[] = {
 	&skl_uncore_cbox,
 	&snb_uncore_arb,
+	&snb_uncore_clockbox,
 	NULL,
 };
 
-- 
2.11.0