linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V6 1/5] x86/cpufeature: Add facility to check for min microcode revisions
@ 2019-01-21 21:42 kan.liang
  2019-01-21 21:42 ` [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering kan.liang
                   ` (4 more replies)
  0 siblings, 5 replies; 27+ messages in thread
From: kan.liang @ 2019-01-21 21:42 UTC (permalink / raw)
  To: x86, linux-kernel, tglx, bp, peterz, mingo; +Cc: ak, eranian, Kan Liang

From: Kan Liang <kan.liang@linux.intel.com>

For bug workarounds or checks, it is useful to check for specific
microcode revisions.

Add a new generic function to match the CPU with stepping.
Add the other function to check the min microcode revisions for
the matched CPU.
A new table format is introduced to facilitate the quirk to
fill the related information.

This does not change the existing x86_cpu_id because it's an ABI
shared with modules, and also has quite different requirements,
as in no wildcards, but everything has to be matched exactly.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Originally-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---

Changes since V5:
- Use proper tag
- Rename x86_cpu_check to x86_cpu_desc, and rename its members
  the same way as the corresponding members in struct cpuinfo_x86
  are named.
- Rename INTEL_CHECK_UCODE to INTEL_CPU_DESC
- Use EXPORT_SYMBOL_GPL

 arch/x86/include/asm/cpu_device_id.h | 28 ++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/match.c          | 31 +++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/arch/x86/include/asm/cpu_device_id.h b/arch/x86/include/asm/cpu_device_id.h
index baeba05..f35b303 100644
--- a/arch/x86/include/asm/cpu_device_id.h
+++ b/arch/x86/include/asm/cpu_device_id.h
@@ -11,4 +11,32 @@
 
 extern const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match);
 
+/*
+ * Match specific microcode revisions.
+ *
+ * vendor/family/model/stepping must be all set.
+ *
+ * only checks against the boot CPU.  When mixed-stepping configs are
+ * valid for a CPU model, add a quirk for every valid stepping and
+ * do the fine-tuning in the quirk handler.
+ */
+
+struct x86_cpu_desc {
+	__u8	x86;		/* CPU family */
+	__u8	x86_vendor;	/* CPU vendor */
+	__u8	x86_model;
+	__u8	x86_stepping;
+	__u32	x86_microcode_rev;
+};
+
+#define INTEL_CPU_DESC(mod, step, rev) {			\
+	.x86 = 6,						\
+	.x86_vendor = X86_VENDOR_INTEL,				\
+	.x86_model = mod,					\
+	.x86_stepping = step,					\
+	.x86_microcode_rev = rev,				\
+}
+
+extern bool x86_cpu_has_min_microcode_rev(const struct x86_cpu_desc *table);
+
 #endif
diff --git a/arch/x86/kernel/cpu/match.c b/arch/x86/kernel/cpu/match.c
index 3fed388..4f4bf89 100644
--- a/arch/x86/kernel/cpu/match.c
+++ b/arch/x86/kernel/cpu/match.c
@@ -48,3 +48,34 @@ const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match)
 	return NULL;
 }
 EXPORT_SYMBOL(x86_match_cpu);
+
+static const struct x86_cpu_desc *
+x86_match_cpu_with_stepping(const struct x86_cpu_desc *match)
+{
+	struct cpuinfo_x86 *c = &boot_cpu_data;
+	const struct x86_cpu_desc *m;
+
+	for (m = match; m->x86 | m->x86_model; m++) {
+		if (c->x86_vendor != m->x86_vendor)
+			continue;
+		if (c->x86 != m->x86)
+			continue;
+		if (c->x86_model != m->x86_model)
+			continue;
+		if (c->x86_stepping != m->x86_stepping)
+			continue;
+		return m;
+	}
+	return NULL;
+}
+
+bool x86_cpu_has_min_microcode_rev(const struct x86_cpu_desc *table)
+{
+	const struct x86_cpu_desc *res = x86_match_cpu_with_stepping(table);
+
+	if (!res || res->x86_microcode_rev > boot_cpu_data.microcode)
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(x86_cpu_has_min_microcode_rev);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-01-21 21:42 [PATCH V6 1/5] x86/cpufeature: Add facility to check for min microcode revisions kan.liang
@ 2019-01-21 21:42 ` kan.liang
  2019-02-04 15:06   ` Peter Zijlstra
                     ` (2 more replies)
  2019-01-21 21:42 ` [PATCH V6 3/5] perf/x86/intel: Clean up SNB pebs quirk kan.liang
                   ` (3 subsequent siblings)
  4 siblings, 3 replies; 27+ messages in thread
From: kan.liang @ 2019-01-21 21:42 UTC (permalink / raw)
  To: x86, linux-kernel, tglx, bp, peterz, mingo; +Cc: ak, eranian, Kan Liang

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

KVM added a workaround for PEBS events leaking into guests with
commit 26a4f3c08de4 ("perf/x86: disable PEBS on a guest entry.")
This uses the VT entry/exit list to add an extra disable of the
PEBS_ENABLE MSR.

Intel also added a fix for this issue to microcode updates on
Haswell/Broadwell/Skylake.

It turns out using the MSR entry/exit list makes VM exits
significantly slower. The list is only needed for disabling
PEBS, because the GLOBAL_CTRL change gets optimized by
KVM into changing the VMCS.

Check for the microcode updates that have the microcode
fix for leaking PEBS, and disable the extra entry/exit list
entry for PEBS_ENABLE. In addition we always clear the
GLOBAL_CTRL for the PEBS counter while running in the guest,
which is enough to make them never fire at the wrong
side of the host/guest transition.

The overhead for VM exits with the filtering active with the patch is
reduced from 8% to 4%.

The microcode patch has already been merged into future platforms.
This patch is one-off thing. The quirks is used here.

For other old platforms which doesn't have microcode patch and quirks,
extra disable of the PEBS_ENABLE MSR is still required.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---

Changes since V5:
- The microcode patch has already been merged into future platforms.
  So this patch is actually one-off things. The quirks is still used in
  the patch.
  Rename pebs_isolated to pebs_no_isolation. The default value is 0. So
  we don't need to worry about the future platforms. However, we have to
  set pebs_no_isolation = 1 for other old platforms which doesn't have
  microcode patch and quirks.
  Also update the commit message accordingly.
- Remove "We" in commit messages
- Add my SOB
- Align isolation_ucodes vertically
- Remove intel_check_isolation() from SNB check.

 arch/x86/events/intel/core.c | 85 ++++++++++++++++++++++++++++++++++++++------
 arch/x86/events/perf_event.h | 15 ++++----
 2 files changed, 83 insertions(+), 17 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 40e12cf..13baff5 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -18,6 +18,7 @@
 #include <asm/hardirq.h>
 #include <asm/intel-family.h>
 #include <asm/apic.h>
+#include <asm/cpu_device_id.h>
 
 #include "../perf_event.h"
 
@@ -3206,16 +3207,27 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr)
 	arr[0].msr = MSR_CORE_PERF_GLOBAL_CTRL;
 	arr[0].host = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
 	arr[0].guest = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_host_mask;
-	/*
-	 * If PMU counter has PEBS enabled it is not enough to disable counter
-	 * on a guest entry since PEBS memory write can overshoot guest entry
-	 * and corrupt guest memory. Disabling PEBS solves the problem.
-	 */
-	arr[1].msr = MSR_IA32_PEBS_ENABLE;
-	arr[1].host = cpuc->pebs_enabled;
-	arr[1].guest = 0;
+	if (x86_pmu.flags & PMU_FL_PEBS_ALL)
+		arr[0].guest &= ~cpuc->pebs_enabled;
+	else
+		arr[0].guest &= ~(cpuc->pebs_enabled & PEBS_COUNTER_MASK);
+	*nr = 1;
+
+	if (x86_pmu.pebs_no_isolation) {
+		/*
+		 * If PMU counter has PEBS enabled it is not enough to
+		 * disable counter on a guest entry since PEBS memory
+		 * write can overshoot guest entry and corrupt guest
+		 * memory. Disabling PEBS solves the problem.
+		 *
+		 * Don't do this if the CPU already enforces it.
+		 */
+		arr[1].msr = MSR_IA32_PEBS_ENABLE;
+		arr[1].host = cpuc->pebs_enabled;
+		arr[1].guest = 0;
+		*nr = 2;
+	}
 
-	*nr = 2;
 	return arr;
 }
 
@@ -3733,6 +3745,41 @@ static __init void intel_clovertown_quirk(void)
 	x86_pmu.pebs_constraints = NULL;
 }
 
+static const struct x86_cpu_desc isolation_ucodes[] = {
+	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_CORE,		 3, 0x0000001f),
+	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_ULT,		 1, 0x0000001e),
+	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_GT3E,		 1, 0x00000015),
+	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_X,		 2, 0x00000037),
+	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_X,		 4, 0x0000000a),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_CORE,	 4, 0x00000023),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_GT3E,	 1, 0x00000014),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_XEON_D,	 2, 0x00000010),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_XEON_D,	 3, 0x07000009),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_XEON_D,	 4, 0x0f000009),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_XEON_D,	 5, 0x0e000002),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_X,		 2, 0x0b000014),
+	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_X,		 3, 0x00000021),
+	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_X,		 4, 0x00000000),
+	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_MOBILE,	 3, 0x0000007c),
+	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_DESKTOP,	 3, 0x0000007c),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	 9, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	 9, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	10, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	11, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	12, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	10, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	11, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	12, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	13, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_CANNONLAKE_MOBILE,	 3, 0x00000000),
+	{}
+};
+
+static void intel_check_isolation(void)
+{
+	x86_pmu.pebs_no_isolation = !x86_cpu_has_min_microcode_rev(isolation_ucodes);
+}
+
 static int intel_snb_pebs_broken(int cpu)
 {
 	u32 rev = UINT_MAX; /* default to broken for unknown models */
@@ -3838,6 +3885,12 @@ static __init void intel_sandybridge_quirk(void)
 	cpus_read_unlock();
 }
 
+static __init void intel_isolation_quirk(void)
+{
+	x86_pmu.check_microcode = intel_check_isolation;
+	intel_check_isolation();
+}
+
 static const struct { int id; char *name; } intel_arch_events_map[] __initconst = {
 	{ PERF_COUNT_HW_CPU_CYCLES, "cpu cycles" },
 	{ PERF_COUNT_HW_INSTRUCTIONS, "instructions" },
@@ -4178,6 +4231,7 @@ __init int intel_pmu_init(void)
 
 		x86_pmu.event_constraints = intel_core2_event_constraints;
 		x86_pmu.pebs_constraints = intel_core2_pebs_event_constraints;
+		x86_pmu.pebs_no_isolation = 1;
 		pr_cont("Core2 events, ");
 		name = "core2";
 		break;
@@ -4209,6 +4263,7 @@ __init int intel_pmu_init(void)
 		intel_pmu_pebs_data_source_nhm();
 		x86_add_quirk(intel_nehalem_quirk);
 		x86_pmu.pebs_no_tlb = 1;
+		x86_pmu.pebs_no_isolation = 1;
 		extra_attr = nhm_format_attr;
 
 		pr_cont("Nehalem events, ");
@@ -4228,6 +4283,7 @@ __init int intel_pmu_init(void)
 		x86_pmu.event_constraints = intel_gen_event_constraints;
 		x86_pmu.pebs_constraints = intel_atom_pebs_event_constraints;
 		x86_pmu.pebs_aliases = intel_pebs_aliases_core2;
+		x86_pmu.pebs_no_isolation = 1;
 		pr_cont("Atom events, ");
 		name = "bonnell";
 		break;
@@ -4249,6 +4305,7 @@ __init int intel_pmu_init(void)
 		x86_pmu.extra_regs = intel_slm_extra_regs;
 		x86_pmu.flags |= PMU_FL_HAS_RSP_1;
 		x86_pmu.cpu_events = slm_events_attrs;
+		x86_pmu.pebs_no_isolation = 1;
 		extra_attr = slm_format_attr;
 		pr_cont("Silvermont events, ");
 		name = "silvermont";
@@ -4276,6 +4333,7 @@ __init int intel_pmu_init(void)
 		x86_pmu.lbr_pt_coexist = true;
 		x86_pmu.flags |= PMU_FL_HAS_RSP_1;
 		x86_pmu.cpu_events = glm_events_attrs;
+		x86_pmu.pebs_no_isolation = 1;
 		extra_attr = slm_format_attr;
 		pr_cont("Goldmont events, ");
 		name = "goldmont";
@@ -4303,6 +4361,7 @@ __init int intel_pmu_init(void)
 		x86_pmu.flags |= PMU_FL_PEBS_ALL;
 		x86_pmu.get_event_constraints = glp_get_event_constraints;
 		x86_pmu.cpu_events = glm_events_attrs;
+		x86_pmu.pebs_no_isolation = 1;
 		/* Goldmont Plus has 4-wide pipeline */
 		event_attr_td_total_slots_scale_glm.event_str = "4";
 		extra_attr = slm_format_attr;
@@ -4336,6 +4395,7 @@ __init int intel_pmu_init(void)
 			X86_CONFIG(.event=0xb1, .umask=0x3f, .inv=1, .cmask=1);
 
 		intel_pmu_pebs_data_source_nhm();
+		x86_pmu.pebs_no_isolation = 1;
 		extra_attr = nhm_format_attr;
 		pr_cont("Westmere events, ");
 		name = "westmere";
@@ -4366,6 +4426,7 @@ __init int intel_pmu_init(void)
 		x86_pmu.flags |= PMU_FL_NO_HT_SHARING;
 
 		x86_pmu.cpu_events = snb_events_attrs;
+		x86_pmu.pebs_no_isolation = 1;
 		mem_attr = snb_mem_events_attrs;
 
 		/* UOPS_ISSUED.ANY,c=1,i=1 to count stall cycles */
@@ -4405,7 +4466,7 @@ __init int intel_pmu_init(void)
 		/* all extra regs are per-cpu when HT is on */
 		x86_pmu.flags |= PMU_FL_HAS_RSP_1;
 		x86_pmu.flags |= PMU_FL_NO_HT_SHARING;
-
+		x86_pmu.pebs_no_isolation = 1;
 		x86_pmu.cpu_events = snb_events_attrs;
 		mem_attr = snb_mem_events_attrs;
 
@@ -4424,6 +4485,7 @@ __init int intel_pmu_init(void)
 	case INTEL_FAM6_HASWELL_X:
 	case INTEL_FAM6_HASWELL_ULT:
 	case INTEL_FAM6_HASWELL_GT3E:
+		x86_add_quirk(intel_isolation_quirk);
 		x86_add_quirk(intel_ht_bug);
 		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, hsw_hw_cache_event_ids, sizeof(hw_cache_event_ids));
@@ -4456,6 +4518,7 @@ __init int intel_pmu_init(void)
 	case INTEL_FAM6_BROADWELL_XEON_D:
 	case INTEL_FAM6_BROADWELL_GT3E:
 	case INTEL_FAM6_BROADWELL_X:
+		x86_add_quirk(intel_isolation_quirk);
 		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, hsw_hw_cache_event_ids, sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, hsw_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
@@ -4508,6 +4571,7 @@ __init int intel_pmu_init(void)
 		/* all extra regs are per-cpu when HT is on */
 		x86_pmu.flags |= PMU_FL_HAS_RSP_1;
 		x86_pmu.flags |= PMU_FL_NO_HT_SHARING;
+		x86_pmu.pebs_no_isolation = 1;
 		extra_attr = slm_format_attr;
 		pr_cont("Knights Landing/Mill events, ");
 		name = "knights-landing";
@@ -4518,6 +4582,7 @@ __init int intel_pmu_init(void)
 	case INTEL_FAM6_SKYLAKE_X:
 	case INTEL_FAM6_KABYLAKE_MOBILE:
 	case INTEL_FAM6_KABYLAKE_DESKTOP:
+		x86_add_quirk(intel_isolation_quirk);
 		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, skl_hw_cache_event_ids, sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, skl_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 78d7b70..dea716e 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -601,13 +601,14 @@ struct x86_pmu {
 	/*
 	 * Intel DebugStore bits
 	 */
-	unsigned int	bts		:1,
-			bts_active	:1,
-			pebs		:1,
-			pebs_active	:1,
-			pebs_broken	:1,
-			pebs_prec_dist	:1,
-			pebs_no_tlb	:1;
+	unsigned int	bts			:1,
+			bts_active		:1,
+			pebs			:1,
+			pebs_active		:1,
+			pebs_broken		:1,
+			pebs_prec_dist		:1,
+			pebs_no_tlb		:1,
+			pebs_no_isolation	:1;
 	int		pebs_record_size;
 	int		pebs_buffer_size;
 	void		(*drain_pebs)(struct pt_regs *regs);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH V6 3/5] perf/x86/intel: Clean up SNB pebs quirk
  2019-01-21 21:42 [PATCH V6 1/5] x86/cpufeature: Add facility to check for min microcode revisions kan.liang
  2019-01-21 21:42 ` [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering kan.liang
@ 2019-01-21 21:42 ` kan.liang
  2019-01-21 21:42 ` [PATCH V6 4/5] perf/x86/intel: Clean up counter freezing quirk kan.liang
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 27+ messages in thread
From: kan.liang @ 2019-01-21 21:42 UTC (permalink / raw)
  To: x86, linux-kernel, tglx, bp, peterz, mingo; +Cc: ak, eranian, Kan Liang

From: Kan Liang <kan.liang@linux.intel.com>

Clean up SNB pebs quirk to use the new facility to check for min
microcode revisions.

Only check the boot CPU, assuming models and features are consistent
over all CPUs.

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---

Changes since V5:
- New patch to address Peter's comments.

 arch/x86/events/intel/core.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 13baff5..a3c7845 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3780,36 +3780,21 @@ static void intel_check_isolation(void)
 	x86_pmu.pebs_no_isolation = !x86_cpu_has_min_microcode_rev(isolation_ucodes);
 }
 
-static int intel_snb_pebs_broken(int cpu)
-{
-	u32 rev = UINT_MAX; /* default to broken for unknown models */
-
-	switch (cpu_data(cpu).x86_model) {
-	case INTEL_FAM6_SANDYBRIDGE:
-		rev = 0x28;
-		break;
-
-	case INTEL_FAM6_SANDYBRIDGE_X:
-		switch (cpu_data(cpu).x86_stepping) {
-		case 6: rev = 0x618; break;
-		case 7: rev = 0x70c; break;
-		}
-	}
+static const struct x86_cpu_desc pebs_ucodes[] = {
+	INTEL_CPU_DESC(INTEL_FAM6_SANDYBRIDGE,		7, 0x00000028),
+	INTEL_CPU_DESC(INTEL_FAM6_SANDYBRIDGE_X,	6, 0x00000618),
+	INTEL_CPU_DESC(INTEL_FAM6_SANDYBRIDGE_X,	7, 0x0000070c),
+	{}
+};
 
-	return (cpu_data(cpu).microcode < rev);
+static bool intel_snb_pebs_broken(void)
+{
+	return !x86_cpu_has_min_microcode_rev(pebs_ucodes);
 }
 
 static void intel_snb_check_microcode(void)
 {
-	int pebs_broken = 0;
-	int cpu;
-
-	for_each_online_cpu(cpu) {
-		if ((pebs_broken = intel_snb_pebs_broken(cpu)))
-			break;
-	}
-
-	if (pebs_broken == x86_pmu.pebs_broken)
+	if (intel_snb_pebs_broken() == x86_pmu.pebs_broken)
 		return;
 
 	/*
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH V6 4/5] perf/x86/intel: Clean up counter freezing quirk
  2019-01-21 21:42 [PATCH V6 1/5] x86/cpufeature: Add facility to check for min microcode revisions kan.liang
  2019-01-21 21:42 ` [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering kan.liang
  2019-01-21 21:42 ` [PATCH V6 3/5] perf/x86/intel: Clean up SNB pebs quirk kan.liang
@ 2019-01-21 21:42 ` kan.liang
  2019-01-21 21:42 ` [PATCH V6 5/5] perf/x86/intel: Add counter freezing quirk for Goldmont kan.liang
  2019-02-04 14:27 ` [PATCH V6 1/5] x86/cpufeature: Add facility to check for min microcode revisions Liang, Kan
  4 siblings, 0 replies; 27+ messages in thread
From: kan.liang @ 2019-01-21 21:42 UTC (permalink / raw)
  To: x86, linux-kernel, tglx, bp, peterz, mingo; +Cc: ak, eranian, Kan Liang

From: Kan Liang <kan.liang@linux.intel.com>

Clean up counter freezing quirk to use the new facility to check for
min microcode revisions.

Rename the counter freezing quirk related functions. Because other
platforms, e.g. Goldmont, also needs to call the quirk.

Only check the boot CPU, assuming models and features are consistent
over all CPUs.

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---

Changes since V5:
- Small changes in commit message
- Apply the new name

 arch/x86/events/intel/core.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index a3c7845..bad19a9 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3917,23 +3917,18 @@ static __init void intel_nehalem_quirk(void)
 	}
 }
 
-static bool intel_glp_counter_freezing_broken(int cpu)
-{
-	u32 rev = UINT_MAX; /* default to broken for unknown stepping */
-
-	switch (cpu_data(cpu).x86_stepping) {
-	case 1:
-		rev = 0x28;
-		break;
-	case 8:
-		rev = 0x6;
-		break;
-	}
+static const struct x86_cpu_desc counter_freezing_ucodes[] = {
+	INTEL_CPU_DESC(INTEL_FAM6_ATOM_GOLDMONT_PLUS,	1, 0x00000028),
+	INTEL_CPU_DESC(INTEL_FAM6_ATOM_GOLDMONT_PLUS,	8, 0x00000006),
+	{}
+};
 
-	return (cpu_data(cpu).microcode < rev);
+static bool intel_counter_freezing_broken(void)
+{
+	return !x86_cpu_has_min_microcode_rev(counter_freezing_ucodes);
 }
 
-static __init void intel_glp_counter_freezing_quirk(void)
+static __init void intel_counter_freezing_quirk(void)
 {
 	/* Check if it's already disabled */
 	if (disable_counter_freezing)
@@ -3943,7 +3938,7 @@ static __init void intel_glp_counter_freezing_quirk(void)
 	 * If the system starts with the wrong ucode, leave the
 	 * counter-freezing feature permanently disabled.
 	 */
-	if (intel_glp_counter_freezing_broken(raw_smp_processor_id())) {
+	if (intel_counter_freezing_broken()) {
 		pr_info("PMU counter freezing disabled due to CPU errata,"
 			"please upgrade microcode\n");
 		x86_pmu.counter_freezing = false;
@@ -4325,7 +4320,7 @@ __init int intel_pmu_init(void)
 		break;
 
 	case INTEL_FAM6_ATOM_GOLDMONT_PLUS:
-		x86_add_quirk(intel_glp_counter_freezing_quirk);
+		x86_add_quirk(intel_counter_freezing_quirk);
 		memcpy(hw_cache_event_ids, glp_hw_cache_event_ids,
 		       sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, glp_hw_cache_extra_regs,
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH V6 5/5] perf/x86/intel: Add counter freezing quirk for Goldmont
  2019-01-21 21:42 [PATCH V6 1/5] x86/cpufeature: Add facility to check for min microcode revisions kan.liang
                   ` (2 preceding siblings ...)
  2019-01-21 21:42 ` [PATCH V6 4/5] perf/x86/intel: Clean up counter freezing quirk kan.liang
@ 2019-01-21 21:42 ` kan.liang
  2019-02-04 14:27 ` [PATCH V6 1/5] x86/cpufeature: Add facility to check for min microcode revisions Liang, Kan
  4 siblings, 0 replies; 27+ messages in thread
From: kan.liang @ 2019-01-21 21:42 UTC (permalink / raw)
  To: x86, linux-kernel, tglx, bp, peterz, mingo; +Cc: ak, eranian, Kan Liang

From: Kan Liang <kan.liang@linux.intel.com>

A microcode patch is also needed for Goldmont while counter freezing
feature is enabled. Otherwise, there will be some issues, e.g. PMI lost.

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---

Changes since V5:
- Apply the new name

 arch/x86/events/intel/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index bad19a9..3eacdf6 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3918,8 +3918,12 @@ static __init void intel_nehalem_quirk(void)
 }
 
 static const struct x86_cpu_desc counter_freezing_ucodes[] = {
-	INTEL_CPU_DESC(INTEL_FAM6_ATOM_GOLDMONT_PLUS,	1, 0x00000028),
-	INTEL_CPU_DESC(INTEL_FAM6_ATOM_GOLDMONT_PLUS,	8, 0x00000006),
+	INTEL_CPU_DESC(INTEL_FAM6_ATOM_GOLDMONT,	 2, 0x0000000e),
+	INTEL_CPU_DESC(INTEL_FAM6_ATOM_GOLDMONT,	 9, 0x0000002e),
+	INTEL_CPU_DESC(INTEL_FAM6_ATOM_GOLDMONT,	10, 0x00000008),
+	INTEL_CPU_DESC(INTEL_FAM6_ATOM_GOLDMONT_X,	 1, 0x00000028),
+	INTEL_CPU_DESC(INTEL_FAM6_ATOM_GOLDMONT_PLUS,	 1, 0x00000028),
+	INTEL_CPU_DESC(INTEL_FAM6_ATOM_GOLDMONT_PLUS,	 8, 0x00000006),
 	{}
 };
 
@@ -4293,6 +4297,7 @@ __init int intel_pmu_init(void)
 
 	case INTEL_FAM6_ATOM_GOLDMONT:
 	case INTEL_FAM6_ATOM_GOLDMONT_X:
+		x86_add_quirk(intel_counter_freezing_quirk);
 		memcpy(hw_cache_event_ids, glm_hw_cache_event_ids,
 		       sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, glm_hw_cache_extra_regs,
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 1/5] x86/cpufeature: Add facility to check for min microcode revisions
  2019-01-21 21:42 [PATCH V6 1/5] x86/cpufeature: Add facility to check for min microcode revisions kan.liang
                   ` (3 preceding siblings ...)
  2019-01-21 21:42 ` [PATCH V6 5/5] perf/x86/intel: Add counter freezing quirk for Goldmont kan.liang
@ 2019-02-04 14:27 ` Liang, Kan
  4 siblings, 0 replies; 27+ messages in thread
From: Liang, Kan @ 2019-02-04 14:27 UTC (permalink / raw)
  To: x86, linux-kernel, tglx, bp, peterz, mingo; +Cc: ak, eranian

Hi all,

Ping. Any comments on this series?

Thanks,
Kan

On 1/21/2019 4:42 PM, kan.liang@linux.intel.com wrote:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> For bug workarounds or checks, it is useful to check for specific
> microcode revisions.
> 
> Add a new generic function to match the CPU with stepping.
> Add the other function to check the min microcode revisions for
> the matched CPU.
> A new table format is introduced to facilitate the quirk to
> fill the related information.
> 
> This does not change the existing x86_cpu_id because it's an ABI
> shared with modules, and also has quite different requirements,
> as in no wildcards, but everything has to be matched exactly.
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Originally-by: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> ---
> 
> Changes since V5:
> - Use proper tag
> - Rename x86_cpu_check to x86_cpu_desc, and rename its members
>    the same way as the corresponding members in struct cpuinfo_x86
>    are named.
> - Rename INTEL_CHECK_UCODE to INTEL_CPU_DESC
> - Use EXPORT_SYMBOL_GPL
> 
>   arch/x86/include/asm/cpu_device_id.h | 28 ++++++++++++++++++++++++++++
>   arch/x86/kernel/cpu/match.c          | 31 +++++++++++++++++++++++++++++++
>   2 files changed, 59 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpu_device_id.h b/arch/x86/include/asm/cpu_device_id.h
> index baeba05..f35b303 100644
> --- a/arch/x86/include/asm/cpu_device_id.h
> +++ b/arch/x86/include/asm/cpu_device_id.h
> @@ -11,4 +11,32 @@
>   
>   extern const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match);
>   
> +/*
> + * Match specific microcode revisions.
> + *
> + * vendor/family/model/stepping must be all set.
> + *
> + * only checks against the boot CPU.  When mixed-stepping configs are
> + * valid for a CPU model, add a quirk for every valid stepping and
> + * do the fine-tuning in the quirk handler.
> + */
> +
> +struct x86_cpu_desc {
> +	__u8	x86;		/* CPU family */
> +	__u8	x86_vendor;	/* CPU vendor */
> +	__u8	x86_model;
> +	__u8	x86_stepping;
> +	__u32	x86_microcode_rev;
> +};
> +
> +#define INTEL_CPU_DESC(mod, step, rev) {			\
> +	.x86 = 6,						\
> +	.x86_vendor = X86_VENDOR_INTEL,				\
> +	.x86_model = mod,					\
> +	.x86_stepping = step,					\
> +	.x86_microcode_rev = rev,				\
> +}
> +
> +extern bool x86_cpu_has_min_microcode_rev(const struct x86_cpu_desc *table);
> +
>   #endif
> diff --git a/arch/x86/kernel/cpu/match.c b/arch/x86/kernel/cpu/match.c
> index 3fed388..4f4bf89 100644
> --- a/arch/x86/kernel/cpu/match.c
> +++ b/arch/x86/kernel/cpu/match.c
> @@ -48,3 +48,34 @@ const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match)
>   	return NULL;
>   }
>   EXPORT_SYMBOL(x86_match_cpu);
> +
> +static const struct x86_cpu_desc *
> +x86_match_cpu_with_stepping(const struct x86_cpu_desc *match)
> +{
> +	struct cpuinfo_x86 *c = &boot_cpu_data;
> +	const struct x86_cpu_desc *m;
> +
> +	for (m = match; m->x86 | m->x86_model; m++) {
> +		if (c->x86_vendor != m->x86_vendor)
> +			continue;
> +		if (c->x86 != m->x86)
> +			continue;
> +		if (c->x86_model != m->x86_model)
> +			continue;
> +		if (c->x86_stepping != m->x86_stepping)
> +			continue;
> +		return m;
> +	}
> +	return NULL;
> +}
> +
> +bool x86_cpu_has_min_microcode_rev(const struct x86_cpu_desc *table)
> +{
> +	const struct x86_cpu_desc *res = x86_match_cpu_with_stepping(table);
> +
> +	if (!res || res->x86_microcode_rev > boot_cpu_data.microcode)
> +		return false;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(x86_cpu_has_min_microcode_rev);
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-01-21 21:42 ` [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering kan.liang
@ 2019-02-04 15:06   ` Peter Zijlstra
  2019-02-04 15:17     ` Peter Zijlstra
  2019-02-04 15:38   ` Peter Zijlstra
  2019-02-04 16:10   ` Peter Zijlstra
  2 siblings, 1 reply; 27+ messages in thread
From: Peter Zijlstra @ 2019-02-04 15:06 UTC (permalink / raw)
  To: kan.liang; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian

On Mon, Jan 21, 2019 at 01:42:28PM -0800, kan.liang@linux.intel.com wrote:

So what's wrong with the below?

Installing a quirk for 

--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3781,7 +3781,7 @@ static const struct x86_cpu_desc isolati
 	{}
 };
 
-static void intel_check_isolation(void)
+static void intel_check_pebs_isolation(void)
 {
 	x86_pmu.pebs_no_isolation = !x86_cpu_has_min_microcode_rev(isolation_ucodes);
 }
@@ -3891,12 +3891,6 @@ static __init void intel_sandybridge_qui
 	cpus_read_unlock();
 }
 
-static __init void intel_isolation_quirk(void)
-{
-	x86_pmu.check_microcode = intel_check_isolation;
-	intel_check_isolation();
-}
-
 static const struct { int id; char *name; } intel_arch_events_map[] __initconst = {
 	{ PERF_COUNT_HW_CPU_CYCLES, "cpu cycles" },
 	{ PERF_COUNT_HW_INSTRUCTIONS, "instructions" },
@@ -4213,6 +4207,7 @@ __init int intel_pmu_init(void)
 	}
 
 	intel_ds_init();
+	intel_check_pebs_isolation();
 
 	x86_add_quirk(intel_arch_events_quirk); /* Install first, so it runs last */
 
@@ -4239,7 +4234,6 @@ __init int intel_pmu_init(void)
 
 		x86_pmu.event_constraints = intel_core2_event_constraints;
 		x86_pmu.pebs_constraints = intel_core2_pebs_event_constraints;
-		x86_pmu.pebs_no_isolation = 1;
 		pr_cont("Core2 events, ");
 		name = "core2";
 		break;
@@ -4271,7 +4265,6 @@ __init int intel_pmu_init(void)
 		intel_pmu_pebs_data_source_nhm();
 		x86_add_quirk(intel_nehalem_quirk);
 		x86_pmu.pebs_no_tlb = 1;
-		x86_pmu.pebs_no_isolation = 1;
 		extra_attr = nhm_format_attr;
 
 		pr_cont("Nehalem events, ");
@@ -4291,7 +4284,6 @@ __init int intel_pmu_init(void)
 		x86_pmu.event_constraints = intel_gen_event_constraints;
 		x86_pmu.pebs_constraints = intel_atom_pebs_event_constraints;
 		x86_pmu.pebs_aliases = intel_pebs_aliases_core2;
-		x86_pmu.pebs_no_isolation = 1;
 		pr_cont("Atom events, ");
 		name = "bonnell";
 		break;
@@ -4313,7 +4305,6 @@ __init int intel_pmu_init(void)
 		x86_pmu.extra_regs = intel_slm_extra_regs;
 		x86_pmu.flags |= PMU_FL_HAS_RSP_1;
 		x86_pmu.cpu_events = slm_events_attrs;
-		x86_pmu.pebs_no_isolation = 1;
 		extra_attr = slm_format_attr;
 		pr_cont("Silvermont events, ");
 		name = "silvermont";
@@ -4341,7 +4332,6 @@ __init int intel_pmu_init(void)
 		x86_pmu.lbr_pt_coexist = true;
 		x86_pmu.flags |= PMU_FL_HAS_RSP_1;
 		x86_pmu.cpu_events = glm_events_attrs;
-		x86_pmu.pebs_no_isolation = 1;
 		extra_attr = slm_format_attr;
 		pr_cont("Goldmont events, ");
 		name = "goldmont";
@@ -4369,7 +4359,6 @@ __init int intel_pmu_init(void)
 		x86_pmu.flags |= PMU_FL_PEBS_ALL;
 		x86_pmu.get_event_constraints = glp_get_event_constraints;
 		x86_pmu.cpu_events = glm_events_attrs;
-		x86_pmu.pebs_no_isolation = 1;
 		/* Goldmont Plus has 4-wide pipeline */
 		event_attr_td_total_slots_scale_glm.event_str = "4";
 		extra_attr = slm_format_attr;
@@ -4403,7 +4392,6 @@ __init int intel_pmu_init(void)
 			X86_CONFIG(.event=0xb1, .umask=0x3f, .inv=1, .cmask=1);
 
 		intel_pmu_pebs_data_source_nhm();
-		x86_pmu.pebs_no_isolation = 1;
 		extra_attr = nhm_format_attr;
 		pr_cont("Westmere events, ");
 		name = "westmere";
@@ -4434,7 +4422,6 @@ __init int intel_pmu_init(void)
 		x86_pmu.flags |= PMU_FL_NO_HT_SHARING;
 
 		x86_pmu.cpu_events = snb_events_attrs;
-		x86_pmu.pebs_no_isolation = 1;
 		mem_attr = snb_mem_events_attrs;
 
 		/* UOPS_ISSUED.ANY,c=1,i=1 to count stall cycles */
@@ -4474,7 +4461,6 @@ __init int intel_pmu_init(void)
 		/* all extra regs are per-cpu when HT is on */
 		x86_pmu.flags |= PMU_FL_HAS_RSP_1;
 		x86_pmu.flags |= PMU_FL_NO_HT_SHARING;
-		x86_pmu.pebs_no_isolation = 1;
 		x86_pmu.cpu_events = snb_events_attrs;
 		mem_attr = snb_mem_events_attrs;
 
@@ -4493,7 +4479,6 @@ __init int intel_pmu_init(void)
 	case INTEL_FAM6_HASWELL_X:
 	case INTEL_FAM6_HASWELL_ULT:
 	case INTEL_FAM6_HASWELL_GT3E:
-		x86_add_quirk(intel_isolation_quirk);
 		x86_add_quirk(intel_ht_bug);
 		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, hsw_hw_cache_event_ids, sizeof(hw_cache_event_ids));
@@ -4526,7 +4511,6 @@ __init int intel_pmu_init(void)
 	case INTEL_FAM6_BROADWELL_XEON_D:
 	case INTEL_FAM6_BROADWELL_GT3E:
 	case INTEL_FAM6_BROADWELL_X:
-		x86_add_quirk(intel_isolation_quirk);
 		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, hsw_hw_cache_event_ids, sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, hsw_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
@@ -4579,7 +4563,6 @@ __init int intel_pmu_init(void)
 		/* all extra regs are per-cpu when HT is on */
 		x86_pmu.flags |= PMU_FL_HAS_RSP_1;
 		x86_pmu.flags |= PMU_FL_NO_HT_SHARING;
-		x86_pmu.pebs_no_isolation = 1;
 		extra_attr = slm_format_attr;
 		pr_cont("Knights Landing/Mill events, ");
 		name = "knights-landing";
@@ -4590,7 +4573,6 @@ __init int intel_pmu_init(void)
 	case INTEL_FAM6_SKYLAKE_X:
 	case INTEL_FAM6_KABYLAKE_MOBILE:
 	case INTEL_FAM6_KABYLAKE_DESKTOP:
-		x86_add_quirk(intel_isolation_quirk);
 		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, skl_hw_cache_event_ids, sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, skl_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 15:06   ` Peter Zijlstra
@ 2019-02-04 15:17     ` Peter Zijlstra
  2019-02-04 15:39       ` Liang, Kan
  0 siblings, 1 reply; 27+ messages in thread
From: Peter Zijlstra @ 2019-02-04 15:17 UTC (permalink / raw)
  To: kan.liang; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian

On Mon, Feb 04, 2019 at 04:06:23PM +0100, Peter Zijlstra wrote:
> On Mon, Jan 21, 2019 at 01:42:28PM -0800, kan.liang@linux.intel.com wrote:
> 
> So what's wrong with the below?
> 
> Installing a quirk for 

(typing hard..)

but what I was going to say is that it seems overkill to sprinkle all
that stuff around.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-01-21 21:42 ` [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering kan.liang
  2019-02-04 15:06   ` Peter Zijlstra
@ 2019-02-04 15:38   ` Peter Zijlstra
  2019-02-04 15:43     ` Liang, Kan
  2019-02-04 15:44     ` Peter Zijlstra
  2019-02-04 16:10   ` Peter Zijlstra
  2 siblings, 2 replies; 27+ messages in thread
From: Peter Zijlstra @ 2019-02-04 15:38 UTC (permalink / raw)
  To: kan.liang; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian



This then? That's nearly what you had; except a lot less noisy.

--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -18,6 +18,7 @@
 #include <asm/hardirq.h>
 #include <asm/intel-family.h>
 #include <asm/apic.h>
+#include <asm/cpu_device_id.h>
 
 #include "../perf_event.h"
 
@@ -3206,16 +3207,27 @@ static struct perf_guest_switch_msr *int
 	arr[0].msr = MSR_CORE_PERF_GLOBAL_CTRL;
 	arr[0].host = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
 	arr[0].guest = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_host_mask;
-	/*
-	 * If PMU counter has PEBS enabled it is not enough to disable counter
-	 * on a guest entry since PEBS memory write can overshoot guest entry
-	 * and corrupt guest memory. Disabling PEBS solves the problem.
-	 */
-	arr[1].msr = MSR_IA32_PEBS_ENABLE;
-	arr[1].host = cpuc->pebs_enabled;
-	arr[1].guest = 0;
+	if (x86_pmu.flags & PMU_FL_PEBS_ALL)
+		arr[0].guest &= ~cpuc->pebs_enabled;
+	else
+		arr[0].guest &= ~(cpuc->pebs_enabled & PEBS_COUNTER_MASK);
+	*nr = 1;
+
+	if (x86_pmu.pebs && x86_pmu.pebs_no_isolation) {
+		/*
+		 * If PMU counter has PEBS enabled it is not enough to
+		 * disable counter on a guest entry since PEBS memory
+		 * write can overshoot guest entry and corrupt guest
+		 * memory. Disabling PEBS solves the problem.
+		 *
+		 * Don't do this if the CPU already enforces it.
+		 */
+		arr[1].msr = MSR_IA32_PEBS_ENABLE;
+		arr[1].host = cpuc->pebs_enabled;
+		arr[1].guest = 0;
+		*nr = 2;
+	}
 
-	*nr = 2;
 	return arr;
 }
 
@@ -3739,6 +3751,48 @@ static __init void intel_clovertown_quir
 	x86_pmu.pebs_constraints = NULL;
 }
 
+static const struct x86_cpu_desc isolation_ucodes[] = {
+	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_CORE,		 3, 0x0000001f),
+	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_ULT,		 1, 0x0000001e),
+	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_GT3E,		 1, 0x00000015),
+	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_X,		 2, 0x00000037),
+	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_X,		 4, 0x0000000a),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_CORE,	 4, 0x00000023),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_GT3E,	 1, 0x00000014),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_XEON_D,	 2, 0x00000010),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_XEON_D,	 3, 0x07000009),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_XEON_D,	 4, 0x0f000009),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_XEON_D,	 5, 0x0e000002),
+	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_X,		 2, 0x0b000014),
+	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_X,		 3, 0x00000021),
+	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_X,		 4, 0x00000000),
+	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_MOBILE,	 3, 0x0000007c),
+	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_DESKTOP,	 3, 0x0000007c),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	 9, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	 9, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	10, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	11, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	12, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	10, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	11, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	12, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	13, 0x0000004e),
+	INTEL_CPU_DESC(INTEL_FAM6_CANNONLAKE_MOBILE,	 3, 0x00000000),
+	{}
+};
+
+static void intel_check_pebs_isolation(void)
+{
+	x86_pmu.pebs_no_isolation = !x86_cpu_has_min_microcode_rev(isolation_ucodes);
+}
+
+static __init void intel_pebs_isolation_quirk(void)
+{
+	WARN_ON_ONCE(x86_pmu.check_microcode);
+	x86_pmu.check_microcode = intel_check_pebs_isolation;
+	intel_check_pebs_isolation();
+}
+
 static int intel_snb_pebs_broken(int cpu)
 {
 	u32 rev = UINT_MAX; /* default to broken for unknown models */
@@ -4433,6 +4487,7 @@ __init int intel_pmu_init(void)
 	case INTEL_FAM6_HASWELL_ULT:
 	case INTEL_FAM6_HASWELL_GT3E:
 		x86_add_quirk(intel_ht_bug);
+		x86_add_quirk(intel_pebs_isolation_quirk);
 		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, hsw_hw_cache_event_ids, sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, hsw_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
@@ -4464,6 +4519,7 @@ __init int intel_pmu_init(void)
 	case INTEL_FAM6_BROADWELL_XEON_D:
 	case INTEL_FAM6_BROADWELL_GT3E:
 	case INTEL_FAM6_BROADWELL_X:
+		x86_add_quirk(intel_pebs_isolation_quirk);
 		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, hsw_hw_cache_event_ids, sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, hsw_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
@@ -4526,6 +4582,7 @@ __init int intel_pmu_init(void)
 	case INTEL_FAM6_SKYLAKE_X:
 	case INTEL_FAM6_KABYLAKE_MOBILE:
 	case INTEL_FAM6_KABYLAKE_DESKTOP:
+		x86_add_quirk(intel_pebs_isolation_quirk);
 		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, skl_hw_cache_event_ids, sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, skl_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -1628,6 +1628,7 @@ void __init intel_ds_init(void)
 	x86_pmu.bts  = boot_cpu_has(X86_FEATURE_BTS);
 	x86_pmu.pebs = boot_cpu_has(X86_FEATURE_PEBS);
 	x86_pmu.pebs_buffer_size = PEBS_BUFFER_SIZE;
+	x86_pmu.pebs_no_isolation = 1;
 	if (x86_pmu.pebs) {
 		char pebs_type = x86_pmu.intel_cap.pebs_trap ?  '+' : '-';
 		int format = x86_pmu.intel_cap.pebs_format;
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -601,13 +601,14 @@ struct x86_pmu {
 	/*
 	 * Intel DebugStore bits
 	 */
-	unsigned int	bts		:1,
-			bts_active	:1,
-			pebs		:1,
-			pebs_active	:1,
-			pebs_broken	:1,
-			pebs_prec_dist	:1,
-			pebs_no_tlb	:1;
+	unsigned int	bts			:1,
+			bts_active		:1,
+			pebs			:1,
+			pebs_active		:1,
+			pebs_broken		:1,
+			pebs_prec_dist		:1,
+			pebs_no_tlb		:1,
+			pebs_no_isolation	:1;
 	int		pebs_record_size;
 	int		pebs_buffer_size;
 	void		(*drain_pebs)(struct pt_regs *regs);

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 15:17     ` Peter Zijlstra
@ 2019-02-04 15:39       ` Liang, Kan
  0 siblings, 0 replies; 27+ messages in thread
From: Liang, Kan @ 2019-02-04 15:39 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian



On 2/4/2019 10:17 AM, Peter Zijlstra wrote:
> On Mon, Feb 04, 2019 at 04:06:23PM +0100, Peter Zijlstra wrote:
>> On Mon, Jan 21, 2019 at 01:42:28PM -0800, kan.liang@linux.intel.com wrote:
>>
>> So what's wrong with the below?
>>
>> Installing a quirk for
> 
> (typing hard..)
> 
> but what I was going to say is that it seems overkill to sprinkle all
> that stuff around.
> 

The microcode patch for PEBS has been applied for Icelake and future 
platforms.
   - For the platforms before Haswell, there is no microcode patch 
applied. The x86_pmu.pebs_no_isolation should always be 1.
   - Between Haswell and Kabylake, x86_pmu.pebs_no_isolation is decided 
by quirk table.
   - For Icelake and future platforms, the x86_pmu.pebs_no_isolation 
should always be 0.

The patch you proposed in previous Email can work well for the existing 
platforms. But it will be a problem when adding new support. We have to 
always add INTEL_CPU_DESC(INTEL_FAM6_XXX,	X, 0x00000000) for each CPU ID 
and each stepping. That will be a disaster for maintenance.

Thanks,
Kan

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 15:38   ` Peter Zijlstra
@ 2019-02-04 15:43     ` Liang, Kan
  2019-02-04 16:15       ` Peter Zijlstra
  2019-02-04 15:44     ` Peter Zijlstra
  1 sibling, 1 reply; 27+ messages in thread
From: Liang, Kan @ 2019-02-04 15:43 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian



On 2/4/2019 10:38 AM, Peter Zijlstra wrote:
> 
> 
> This then? That's nearly what you had; except a lot less noisy.
> 
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -18,6 +18,7 @@
>   #include <asm/hardirq.h>
>   #include <asm/intel-family.h>
>   #include <asm/apic.h>
> +#include <asm/cpu_device_id.h>
>   
>   #include "../perf_event.h"
>   
> @@ -3206,16 +3207,27 @@ static struct perf_guest_switch_msr *int
>   	arr[0].msr = MSR_CORE_PERF_GLOBAL_CTRL;
>   	arr[0].host = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
>   	arr[0].guest = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_host_mask;
> -	/*
> -	 * If PMU counter has PEBS enabled it is not enough to disable counter
> -	 * on a guest entry since PEBS memory write can overshoot guest entry
> -	 * and corrupt guest memory. Disabling PEBS solves the problem.
> -	 */
> -	arr[1].msr = MSR_IA32_PEBS_ENABLE;
> -	arr[1].host = cpuc->pebs_enabled;
> -	arr[1].guest = 0;
> +	if (x86_pmu.flags & PMU_FL_PEBS_ALL)
> +		arr[0].guest &= ~cpuc->pebs_enabled;
> +	else
> +		arr[0].guest &= ~(cpuc->pebs_enabled & PEBS_COUNTER_MASK);
> +	*nr = 1;
> +
> +	if (x86_pmu.pebs && x86_pmu.pebs_no_isolation) {
> +		/*
> +		 * If PMU counter has PEBS enabled it is not enough to
> +		 * disable counter on a guest entry since PEBS memory
> +		 * write can overshoot guest entry and corrupt guest
> +		 * memory. Disabling PEBS solves the problem.
> +		 *
> +		 * Don't do this if the CPU already enforces it.
> +		 */
> +		arr[1].msr = MSR_IA32_PEBS_ENABLE;
> +		arr[1].host = cpuc->pebs_enabled;
> +		arr[1].guest = 0;
> +		*nr = 2;
> +	}
>   
> -	*nr = 2;
>   	return arr;
>   }
>   
> @@ -3739,6 +3751,48 @@ static __init void intel_clovertown_quir
>   	x86_pmu.pebs_constraints = NULL;
>   }
>   
> +static const struct x86_cpu_desc isolation_ucodes[] = {
> +	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_CORE,		 3, 0x0000001f),
> +	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_ULT,		 1, 0x0000001e),
> +	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_GT3E,		 1, 0x00000015),
> +	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_X,		 2, 0x00000037),
> +	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_X,		 4, 0x0000000a),
> +	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_CORE,	 4, 0x00000023),
> +	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_GT3E,	 1, 0x00000014),
> +	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_XEON_D,	 2, 0x00000010),
> +	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_XEON_D,	 3, 0x07000009),
> +	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_XEON_D,	 4, 0x0f000009),
> +	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_XEON_D,	 5, 0x0e000002),
> +	INTEL_CPU_DESC(INTEL_FAM6_BROADWELL_X,		 2, 0x0b000014),
> +	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_X,		 3, 0x00000021),
> +	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_X,		 4, 0x00000000),
> +	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_MOBILE,	 3, 0x0000007c),
> +	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_DESKTOP,	 3, 0x0000007c),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	 9, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	 9, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	10, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	11, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	12, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	10, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	11, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	12, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	13, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_CANNONLAKE_MOBILE,	 3, 0x00000000),
> +	{}
> +};
> +
> +static void intel_check_pebs_isolation(void)
> +{
> +	x86_pmu.pebs_no_isolation = !x86_cpu_has_min_microcode_rev(isolation_ucodes);
> +}
> +
> +static __init void intel_pebs_isolation_quirk(void)
> +{
> +	WARN_ON_ONCE(x86_pmu.check_microcode);
> +	x86_pmu.check_microcode = intel_check_pebs_isolation;
> +	intel_check_pebs_isolation();
> +}
> +
>   static int intel_snb_pebs_broken(int cpu)
>   {
>   	u32 rev = UINT_MAX; /* default to broken for unknown models */
> @@ -4433,6 +4487,7 @@ __init int intel_pmu_init(void)
>   	case INTEL_FAM6_HASWELL_ULT:
>   	case INTEL_FAM6_HASWELL_GT3E:
>   		x86_add_quirk(intel_ht_bug);
> +		x86_add_quirk(intel_pebs_isolation_quirk);
>   		x86_pmu.late_ack = true;
>   		memcpy(hw_cache_event_ids, hsw_hw_cache_event_ids, sizeof(hw_cache_event_ids));
>   		memcpy(hw_cache_extra_regs, hsw_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
> @@ -4464,6 +4519,7 @@ __init int intel_pmu_init(void)
>   	case INTEL_FAM6_BROADWELL_XEON_D:
>   	case INTEL_FAM6_BROADWELL_GT3E:
>   	case INTEL_FAM6_BROADWELL_X:
> +		x86_add_quirk(intel_pebs_isolation_quirk);
>   		x86_pmu.late_ack = true;
>   		memcpy(hw_cache_event_ids, hsw_hw_cache_event_ids, sizeof(hw_cache_event_ids));
>   		memcpy(hw_cache_extra_regs, hsw_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
> @@ -4526,6 +4582,7 @@ __init int intel_pmu_init(void)
>   	case INTEL_FAM6_SKYLAKE_X:
>   	case INTEL_FAM6_KABYLAKE_MOBILE:
>   	case INTEL_FAM6_KABYLAKE_DESKTOP:
> +		x86_add_quirk(intel_pebs_isolation_quirk);
>   		x86_pmu.late_ack = true;
>   		memcpy(hw_cache_event_ids, skl_hw_cache_event_ids, sizeof(hw_cache_event_ids));
>   		memcpy(hw_cache_extra_regs, skl_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1628,6 +1628,7 @@ void __init intel_ds_init(void)
>   	x86_pmu.bts  = boot_cpu_has(X86_FEATURE_BTS);
>   	x86_pmu.pebs = boot_cpu_has(X86_FEATURE_PEBS);
>   	x86_pmu.pebs_buffer_size = PEBS_BUFFER_SIZE;
> +	x86_pmu.pebs_no_isolation = 1;

We will submit the Icelake support soon (probably next week).
That will be a problem for Icelake.

Thanks,
Kan

>   	if (x86_pmu.pebs) {
>   		char pebs_type = x86_pmu.intel_cap.pebs_trap ?  '+' : '-';
>   		int format = x86_pmu.intel_cap.pebs_format;
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -601,13 +601,14 @@ struct x86_pmu {
>   	/*
>   	 * Intel DebugStore bits
>   	 */
> -	unsigned int	bts		:1,
> -			bts_active	:1,
> -			pebs		:1,
> -			pebs_active	:1,
> -			pebs_broken	:1,
> -			pebs_prec_dist	:1,
> -			pebs_no_tlb	:1;
> +	unsigned int	bts			:1,
> +			bts_active		:1,
> +			pebs			:1,
> +			pebs_active		:1,
> +			pebs_broken		:1,
> +			pebs_prec_dist		:1,
> +			pebs_no_tlb		:1,
> +			pebs_no_isolation	:1;
>   	int		pebs_record_size;
>   	int		pebs_buffer_size;
>   	void		(*drain_pebs)(struct pt_regs *regs);
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 15:38   ` Peter Zijlstra
  2019-02-04 15:43     ` Liang, Kan
@ 2019-02-04 15:44     ` Peter Zijlstra
  2019-02-04 15:57       ` Liang, Kan
  1 sibling, 1 reply; 27+ messages in thread
From: Peter Zijlstra @ 2019-02-04 15:44 UTC (permalink / raw)
  To: kan.liang; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian

On Mon, Feb 04, 2019 at 04:38:27PM +0100, Peter Zijlstra wrote:
> +static const struct x86_cpu_desc isolation_ucodes[] = {
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	 9, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	10, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	11, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	12, 0x0000004e),

> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	10, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	11, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	12, 0x0000004e),
> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	13, 0x0000004e),

Do we want a special stepping (0 / -1) to be able to denote 'all' ?

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 15:44     ` Peter Zijlstra
@ 2019-02-04 15:57       ` Liang, Kan
  2019-02-04 16:04         ` Borislav Petkov
                           ` (2 more replies)
  0 siblings, 3 replies; 27+ messages in thread
From: Liang, Kan @ 2019-02-04 15:57 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian



On 2/4/2019 10:44 AM, Peter Zijlstra wrote:
> On Mon, Feb 04, 2019 at 04:38:27PM +0100, Peter Zijlstra wrote:
>> +static const struct x86_cpu_desc isolation_ucodes[] = {
>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	 9, 0x0000004e),
>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	10, 0x0000004e),
>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	11, 0x0000004e),
>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	12, 0x0000004e),
> 
>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	10, 0x0000004e),
>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	11, 0x0000004e),
>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	12, 0x0000004e),
>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	13, 0x0000004e),
> 
> Do we want a special stepping (0 / -1) to be able to denote 'all' ?
> 

Something like as below?
#define X86_STEPPING_ANY	0xff

As my understanding, the microcode version for each stepping is 
independent and irrelevant. The 0x0000004e should be just coincidence.
If so, I don't think X86_STEPPING_ANY is very useful.

Andi, if I'm wrong please correct me.

Thanks,
Kan

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 15:57       ` Liang, Kan
@ 2019-02-04 16:04         ` Borislav Petkov
  2019-02-04 16:23           ` Liang, Kan
  2019-02-04 16:13         ` Andi Kleen
  2019-02-04 16:23         ` Peter Zijlstra
  2 siblings, 1 reply; 27+ messages in thread
From: Borislav Petkov @ 2019-02-04 16:04 UTC (permalink / raw)
  To: Liang, Kan; +Cc: Peter Zijlstra, x86, linux-kernel, tglx, mingo, ak, eranian

On Mon, Feb 04, 2019 at 10:57:32AM -0500, Liang, Kan wrote:
> 
> 
> On 2/4/2019 10:44 AM, Peter Zijlstra wrote:
> > On Mon, Feb 04, 2019 at 04:38:27PM +0100, Peter Zijlstra wrote:
> > > +static const struct x86_cpu_desc isolation_ucodes[] = {
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	 9, 0x0000004e),
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	10, 0x0000004e),
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	11, 0x0000004e),
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	12, 0x0000004e),
> > 
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	10, 0x0000004e),
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	11, 0x0000004e),
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	12, 0x0000004e),
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	13, 0x0000004e),
> > 
> > Do we want a special stepping (0 / -1) to be able to denote 'all' ?
> > 
> 
> Something like as below?
> #define X86_STEPPING_ANY	0xff
> 
> As my understanding, the microcode version for each stepping is independent
> and irrelevant.

Huh?

Why are we looking at the stepping then?

x86_match_cpu_with_stepping() <---

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-01-21 21:42 ` [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering kan.liang
  2019-02-04 15:06   ` Peter Zijlstra
  2019-02-04 15:38   ` Peter Zijlstra
@ 2019-02-04 16:10   ` Peter Zijlstra
  2019-02-04 16:16     ` Liang, Kan
  2 siblings, 1 reply; 27+ messages in thread
From: Peter Zijlstra @ 2019-02-04 16:10 UTC (permalink / raw)
  To: kan.liang; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian

On Mon, Jan 21, 2019 at 01:42:28PM -0800, kan.liang@linux.intel.com wrote:
> +	INTEL_CPU_DESC(INTEL_FAM6_CANNONLAKE_MOBILE,	 3, 0x00000000),

Funny that, CNL doesn't have any perf support at all yet.. Maybe do that
patch first or delay this line to that patch?

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 15:57       ` Liang, Kan
  2019-02-04 16:04         ` Borislav Petkov
@ 2019-02-04 16:13         ` Andi Kleen
  2019-02-04 16:23         ` Peter Zijlstra
  2 siblings, 0 replies; 27+ messages in thread
From: Andi Kleen @ 2019-02-04 16:13 UTC (permalink / raw)
  To: Liang, Kan; +Cc: Peter Zijlstra, x86, linux-kernel, tglx, bp, mingo, eranian

> As my understanding, the microcode version for each stepping is independent
> and irrelevant. The 0x0000004e should be just coincidence.
> If so, I don't think X86_STEPPING_ANY is very useful.
> 
> Andi, if I'm wrong please correct me.

Yes that's right. You cannot match microcode without stepping.

-Andi

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 15:43     ` Liang, Kan
@ 2019-02-04 16:15       ` Peter Zijlstra
  2019-02-04 16:18         ` Liang, Kan
  0 siblings, 1 reply; 27+ messages in thread
From: Peter Zijlstra @ 2019-02-04 16:15 UTC (permalink / raw)
  To: Liang, Kan; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian

On Mon, Feb 04, 2019 at 10:43:41AM -0500, Liang, Kan wrote:
> > --- a/arch/x86/events/intel/ds.c
> > +++ b/arch/x86/events/intel/ds.c
> > @@ -1628,6 +1628,7 @@ void __init intel_ds_init(void)
> >   	x86_pmu.bts  = boot_cpu_has(X86_FEATURE_BTS);
> >   	x86_pmu.pebs = boot_cpu_has(X86_FEATURE_PEBS);
> >   	x86_pmu.pebs_buffer_size = PEBS_BUFFER_SIZE;
> > +	x86_pmu.pebs_no_isolation = 1;
> 
> We will submit the Icelake support soon (probably next week).
> That will be a problem for Icelake.

We can have ICL set it to 0 explicitly, but explicitly setting it to 1
_11_ times is just silly.

Also, what perfmon version will ICL have? If it were to be 5 we could
key off of that.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 16:10   ` Peter Zijlstra
@ 2019-02-04 16:16     ` Liang, Kan
  0 siblings, 0 replies; 27+ messages in thread
From: Liang, Kan @ 2019-02-04 16:16 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian



On 2/4/2019 11:10 AM, Peter Zijlstra wrote:
> On Mon, Jan 21, 2019 at 01:42:28PM -0800, kan.liang@linux.intel.com wrote:
>> +	INTEL_CPU_DESC(INTEL_FAM6_CANNONLAKE_MOBILE,	 3, 0x00000000),
> 
> Funny that, CNL doesn't have any perf support at all yet.. Maybe do that
> patch first or delay this line to that patch?
>

I will delete the line for now.

Thanks,
Kan


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 16:15       ` Peter Zijlstra
@ 2019-02-04 16:18         ` Liang, Kan
  0 siblings, 0 replies; 27+ messages in thread
From: Liang, Kan @ 2019-02-04 16:18 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian



On 2/4/2019 11:15 AM, Peter Zijlstra wrote:
> On Mon, Feb 04, 2019 at 10:43:41AM -0500, Liang, Kan wrote:
>>> --- a/arch/x86/events/intel/ds.c
>>> +++ b/arch/x86/events/intel/ds.c
>>> @@ -1628,6 +1628,7 @@ void __init intel_ds_init(void)
>>>    	x86_pmu.bts  = boot_cpu_has(X86_FEATURE_BTS);
>>>    	x86_pmu.pebs = boot_cpu_has(X86_FEATURE_PEBS);
>>>    	x86_pmu.pebs_buffer_size = PEBS_BUFFER_SIZE;
>>> +	x86_pmu.pebs_no_isolation = 1;
>>
>> We will submit the Icelake support soon (probably next week).
>> That will be a problem for Icelake.
> 
> We can have ICL set it to 0 explicitly, but explicitly setting it to 1
> _11_ times is just silly.
> 
> Also, what perfmon version will ICL have? If it were to be 5 we could
> key off of that.
>

Right, we can use perfmon version here. I will apply it in V7.

Thanks,
Kan


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 15:57       ` Liang, Kan
  2019-02-04 16:04         ` Borislav Petkov
  2019-02-04 16:13         ` Andi Kleen
@ 2019-02-04 16:23         ` Peter Zijlstra
  2019-02-04 16:55           ` Liang, Kan
  2 siblings, 1 reply; 27+ messages in thread
From: Peter Zijlstra @ 2019-02-04 16:23 UTC (permalink / raw)
  To: Liang, Kan; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian

On Mon, Feb 04, 2019 at 10:57:32AM -0500, Liang, Kan wrote:
> 
> 
> On 2/4/2019 10:44 AM, Peter Zijlstra wrote:
> > On Mon, Feb 04, 2019 at 04:38:27PM +0100, Peter Zijlstra wrote:
> > > +static const struct x86_cpu_desc isolation_ucodes[] = {
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	 9, 0x0000004e),
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	10, 0x0000004e),
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	11, 0x0000004e),
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	12, 0x0000004e),
> > 
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	10, 0x0000004e),
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	11, 0x0000004e),
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	12, 0x0000004e),
> > > +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	13, 0x0000004e),
> > 
> > Do we want a special stepping (0 / -1) to be able to denote 'all' ?
> > 
> 
> Something like as below?
> #define X86_STEPPING_ANY	0xff
> 
> As my understanding, the microcode version for each stepping is independent
> and irrelevant. The 0x0000004e should be just coincidence.
> If so, I don't think X86_STEPPING_ANY is very useful.

Sure; but since we have this happy accident, we can still use it for a
notational convenience, right?

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 16:04         ` Borislav Petkov
@ 2019-02-04 16:23           ` Liang, Kan
  2019-02-04 16:41             ` Borislav Petkov
  0 siblings, 1 reply; 27+ messages in thread
From: Liang, Kan @ 2019-02-04 16:23 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Peter Zijlstra, x86, linux-kernel, tglx, mingo, ak, eranian



On 2/4/2019 11:04 AM, Borislav Petkov wrote:
> On Mon, Feb 04, 2019 at 10:57:32AM -0500, Liang, Kan wrote:
>>
>>
>> On 2/4/2019 10:44 AM, Peter Zijlstra wrote:
>>> On Mon, Feb 04, 2019 at 04:38:27PM +0100, Peter Zijlstra wrote:
>>>> +static const struct x86_cpu_desc isolation_ucodes[] = {
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	 9, 0x0000004e),
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	10, 0x0000004e),
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	11, 0x0000004e),
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	12, 0x0000004e),
>>>
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	10, 0x0000004e),
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	11, 0x0000004e),
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	12, 0x0000004e),
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	13, 0x0000004e),
>>>
>>> Do we want a special stepping (0 / -1) to be able to denote 'all' ?
>>>
>>
>> Something like as below?
>> #define X86_STEPPING_ANY	0xff
>>
>> As my understanding, the microcode version for each stepping is independent
>> and irrelevant.
> 
> Huh?
> 
> Why are we looking at the stepping then?
> 
> x86_match_cpu_with_stepping() <---
> 

I mean that the microcode version number is irrelevant between stepping.
Let's use SKL server as an example.
+	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_X,		 3, 0x00000021),
+	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_X,		 4, 0x00000000),

We need the function to check the min microcode version number for each 
stepping.


Thanks,
Kan

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 16:23           ` Liang, Kan
@ 2019-02-04 16:41             ` Borislav Petkov
  0 siblings, 0 replies; 27+ messages in thread
From: Borislav Petkov @ 2019-02-04 16:41 UTC (permalink / raw)
  To: Liang, Kan; +Cc: Peter Zijlstra, x86, linux-kernel, tglx, mingo, ak, eranian

On Mon, Feb 04, 2019 at 11:23:13AM -0500, Liang, Kan wrote:
> I mean that the microcode version number is irrelevant between stepping.
> Let's use SKL server as an example.
> +	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_X,		 3, 0x00000021),
> +	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_X,		 4, 0x00000000),
> 
> We need the function to check the min microcode version number for each
> stepping.

So you need to say "ignore stepping". I.e., X86_STEPPING_ANY.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 16:23         ` Peter Zijlstra
@ 2019-02-04 16:55           ` Liang, Kan
  2019-02-04 18:15             ` Borislav Petkov
  0 siblings, 1 reply; 27+ messages in thread
From: Liang, Kan @ 2019-02-04 16:55 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: x86, linux-kernel, tglx, bp, mingo, ak, eranian



On 2/4/2019 11:23 AM, Peter Zijlstra wrote:
> On Mon, Feb 04, 2019 at 10:57:32AM -0500, Liang, Kan wrote:
>>
>>
>> On 2/4/2019 10:44 AM, Peter Zijlstra wrote:
>>> On Mon, Feb 04, 2019 at 04:38:27PM +0100, Peter Zijlstra wrote:
>>>> +static const struct x86_cpu_desc isolation_ucodes[] = {
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	 9, 0x0000004e),
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	10, 0x0000004e),
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	11, 0x0000004e),
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	12, 0x0000004e),
>>>
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	10, 0x0000004e),
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	11, 0x0000004e),
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	12, 0x0000004e),
>>>> +	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	13, 0x0000004e),
>>>
>>> Do we want a special stepping (0 / -1) to be able to denote 'all' ?
>>>
>>
>> Something like as below?
>> #define X86_STEPPING_ANY	0xff
>>
>> As my understanding, the microcode version for each stepping is independent
>> and irrelevant. The 0x0000004e should be just coincidence.
>> If so, I don't think X86_STEPPING_ANY is very useful.
> 
> Sure; but since we have this happy accident, we can still use it for a
> notational convenience, right?

We cannot apply X86_STEPPING_ANY to ignore the stepping. There will be 
problems for 0-8 stepping for KABYLAKE_MOBILE.
I think what we need is x86_match_cpu_with_stepping_range().
But I don't think it is worth enabling it just for this rare case.

Thanks,
Kan

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 16:55           ` Liang, Kan
@ 2019-02-04 18:15             ` Borislav Petkov
  2019-02-04 18:57               ` Liang, Kan
  0 siblings, 1 reply; 27+ messages in thread
From: Borislav Petkov @ 2019-02-04 18:15 UTC (permalink / raw)
  To: Liang, Kan; +Cc: Peter Zijlstra, x86, linux-kernel, tglx, mingo, ak, eranian

On Mon, Feb 04, 2019 at 11:55:27AM -0500, Liang, Kan wrote:

> We cannot apply X86_STEPPING_ANY to ignore the stepping. There will be
> problems for 0-8 stepping for KABYLAKE_MOBILE.

So why are we even doing this new "interface"
x86_cpu_has_min_microcode_rev() if even at the conversion stage it shows
that it is inadequate?

> I think what we need is x86_match_cpu_with_stepping_range().
> But I don't think it is worth enabling it just for this rare case.

Sounds to me like you wanna go back to the drawing board after having
evaluated all the use cases.

And yes, I can imagine:

+struct x86_cpu_desc {
+       __u8    x86;            /* CPU family */
+       __u8    x86_vendor;     /* CPU vendor */
+       __u8    x86_model;
+       __u8    x86_min_stepping;
+       __u8    x86_max_stepping;
+       __u32   x86_microcode_rev;
+};

along with the usage:

INTEL_CPU_DESC(mod, min_step, max_step, rev)

to make it more elegant.

Question is, can you have a given microcode revision X applying to
multiple revisions?

If yes, the above should work...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 18:15             ` Borislav Petkov
@ 2019-02-04 18:57               ` Liang, Kan
  2019-02-04 19:43                 ` Borislav Petkov
  0 siblings, 1 reply; 27+ messages in thread
From: Liang, Kan @ 2019-02-04 18:57 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Peter Zijlstra, x86, linux-kernel, tglx, mingo, ak, eranian



On 2/4/2019 1:15 PM, Borislav Petkov wrote:
> On Mon, Feb 04, 2019 at 11:55:27AM -0500, Liang, Kan wrote:
> 
>> We cannot apply X86_STEPPING_ANY to ignore the stepping. There will be
>> problems for 0-8 stepping for KABYLAKE_MOBILE.
> 
> So why are we even doing this new "interface"
> x86_cpu_has_min_microcode_rev() if even at the conversion stage it shows
> that it is inadequate?
> 
>> I think what we need is x86_match_cpu_with_stepping_range().
>> But I don't think it is worth enabling it just for this rare case.
> 
> Sounds to me like you wanna go back to the drawing board after having
> evaluated all the use cases.
> 
> And yes, I can imagine:
> 
> +struct x86_cpu_desc {
> +       __u8    x86;            /* CPU family */
> +       __u8    x86_vendor;     /* CPU vendor */
> +       __u8    x86_model;
> +       __u8    x86_min_stepping;
> +       __u8    x86_max_stepping;
> +       __u32   x86_microcode_rev;
> +};
> 
> along with the usage:
> 
> INTEL_CPU_DESC(mod, min_step, max_step, rev)
> 
> to make it more elegant.
> 
> Question is, can you have a given microcode revision X applying to
> multiple revisions?
> 
> If yes, the above should work...
> 

You mean a given microcode revision X applying to multiple stepping,
right?
I don't think so. I still think the KABYLAKE case is an uncommon case.


Can we do something as below just for this case?

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 3eacdf6..d5fba67 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3745,6 +3745,12 @@ static __init void intel_clovertown_quirk(void)
  	x86_pmu.pebs_constraints = NULL;
  }

+#define INTEL_CPU_DESC_FOUR(mod, start, rev)			\
+	INTEL_CPU_DESC(mod, start, rev),			\
+	INTEL_CPU_DESC(mod, start + 1, rev),			\
+	INTEL_CPU_DESC(mod, start + 2, rev),			\
+	INTEL_CPU_DESC(mod, start + 3, rev)
+
  static const struct x86_cpu_desc isolation_ucodes[] = {
  	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_CORE,		 3, 0x0000001f),
  	INTEL_CPU_DESC(INTEL_FAM6_HASWELL_ULT,		 1, 0x0000001e),
@@ -3763,15 +3769,8 @@ static const struct x86_cpu_desc 
isolation_ucodes[] = {
  	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_MOBILE,	 3, 0x0000007c),
  	INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_DESKTOP,	 3, 0x0000007c),
  	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	 9, 0x0000004e),
-	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	 9, 0x0000004e),
-	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	10, 0x0000004e),
-	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	11, 0x0000004e),
-	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE,	12, 0x0000004e),
-	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	10, 0x0000004e),
-	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	11, 0x0000004e),
-	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	12, 0x0000004e),
-	INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP,	13, 0x0000004e),
-	INTEL_CPU_DESC(INTEL_FAM6_CANNONLAKE_MOBILE,	 3, 0x00000000),
+	INTEL_CPU_DESC_FOUR(INTEL_FAM6_KABYLAKE_MOBILE,	 9, 0x0000004e),
+	INTEL_CPU_DESC_FOUR(INTEL_FAM6_KABYLAKE_DESKTOP,10, 0x0000004e),
  	{}
  };

Thanks,
Kan

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 18:57               ` Liang, Kan
@ 2019-02-04 19:43                 ` Borislav Petkov
  2019-02-04 20:37                   ` Liang, Kan
  0 siblings, 1 reply; 27+ messages in thread
From: Borislav Petkov @ 2019-02-04 19:43 UTC (permalink / raw)
  To: Liang, Kan; +Cc: Peter Zijlstra, x86, linux-kernel, tglx, mingo, ak, eranian

On Mon, Feb 04, 2019 at 01:57:49PM -0500, Liang, Kan wrote:
> You mean a given microcode revision X applying to multiple stepping,
> right?

Yes, the range thing. You specify a range of steppings:

kabylake mobile  with steppings 9-12
kabylake desktop with steppings 10-13

> I don't think so. I still think the KABYLAKE case is an uncommon case.

You mean it is uncommon because there are already *two* models which
need it or because this is only kabylake and it won't happen in the
future?

> Can we do something as below just for this case?

Of course not. _FOUR is just silly and if a *fifth* stepping appears,
you need to go fixup again.

If anything and if this kabylake thing is one off, I'd prefer we keep it
as is.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering
  2019-02-04 19:43                 ` Borislav Petkov
@ 2019-02-04 20:37                   ` Liang, Kan
  0 siblings, 0 replies; 27+ messages in thread
From: Liang, Kan @ 2019-02-04 20:37 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Peter Zijlstra, x86, linux-kernel, tglx, mingo, ak, eranian



On 2/4/2019 2:43 PM, Borislav Petkov wrote:
> On Mon, Feb 04, 2019 at 01:57:49PM -0500, Liang, Kan wrote:
>> You mean a given microcode revision X applying to multiple stepping,
>> right?
> 
> Yes, the range thing. You specify a range of steppings:
> 
> kabylake mobile  with steppings 9-12
> kabylake desktop with steppings 10-13
> 
>> I don't think so. I still think the KABYLAKE case is an uncommon case.
> 
> You mean it is uncommon because there are already *two* models which
> need it or because this is only kabylake and it won't happen in the
> future?

The latter.
As you can see in the patch series, for other platforms (from core to 
atom), the microcode revision is always different on different stepping 
of the same model.
I believe the kabylake thing is one off.

> 
>> Can we do something as below just for this case?
> 
> Of course not. _FOUR is just silly and if a *fifth* stepping appears,
> you need to go fixup again.
> 
> If anything and if this kabylake thing is one off, I'd prefer we keep it
> as is.
> 

OK. I will keep it as is on V7.

Thanks,
Kan

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2019-02-04 20:38 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-21 21:42 [PATCH V6 1/5] x86/cpufeature: Add facility to check for min microcode revisions kan.liang
2019-01-21 21:42 ` [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering kan.liang
2019-02-04 15:06   ` Peter Zijlstra
2019-02-04 15:17     ` Peter Zijlstra
2019-02-04 15:39       ` Liang, Kan
2019-02-04 15:38   ` Peter Zijlstra
2019-02-04 15:43     ` Liang, Kan
2019-02-04 16:15       ` Peter Zijlstra
2019-02-04 16:18         ` Liang, Kan
2019-02-04 15:44     ` Peter Zijlstra
2019-02-04 15:57       ` Liang, Kan
2019-02-04 16:04         ` Borislav Petkov
2019-02-04 16:23           ` Liang, Kan
2019-02-04 16:41             ` Borislav Petkov
2019-02-04 16:13         ` Andi Kleen
2019-02-04 16:23         ` Peter Zijlstra
2019-02-04 16:55           ` Liang, Kan
2019-02-04 18:15             ` Borislav Petkov
2019-02-04 18:57               ` Liang, Kan
2019-02-04 19:43                 ` Borislav Petkov
2019-02-04 20:37                   ` Liang, Kan
2019-02-04 16:10   ` Peter Zijlstra
2019-02-04 16:16     ` Liang, Kan
2019-01-21 21:42 ` [PATCH V6 3/5] perf/x86/intel: Clean up SNB pebs quirk kan.liang
2019-01-21 21:42 ` [PATCH V6 4/5] perf/x86/intel: Clean up counter freezing quirk kan.liang
2019-01-21 21:42 ` [PATCH V6 5/5] perf/x86/intel: Add counter freezing quirk for Goldmont kan.liang
2019-02-04 14:27 ` [PATCH V6 1/5] x86/cpufeature: Add facility to check for min microcode revisions Liang, Kan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).