Skip to contentSkip to conversation

perf/x86/intel: Add counter freezing quirk for Goldmont

PATCH In review [email protected] 5 participants 11 messages Oct 2, 2018 – Oct 3, 2018

Subsystems (2)
x86 perf events
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 perf/x86/intel: Add counter freezing quirk for Goldmont

From: Kan Liang <[email protected]>

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

Add a quirk to check microcode version. If the system starts with the wrong ucode, leave the counter-freezing feature permanently disabled.

The quirk function for Goldmont is similar as Goldmont Plus. Reuse the quirk function and rename it to atom_v4.

Signed-off-by Kan Liang <[email protected]>
---
 arch/x86/events/intel/core.c | 44 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)
arch/x86/events/intel/core.c +35 -9
index ab01ef9..56401bc 100644
@@ -3839,23 +3839,48 @@ static __init void intel_nehalem_quirk(void)
 	}
 }
 
-static bool intel_glp_counter_freezing_broken(int cpu)
+static bool intel_atom_v4_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;
+	switch (cpu_data(cpu).x86_model) {
+	case INTEL_FAM6_ATOM_GOLDMONT:
+		switch (cpu_data(cpu).x86_stepping) {
+		case 2:
+			rev = 0xe;
+			break;
+		case 9:
+			rev = 0x2e;
+			break;
+		case 10:
+			rev = 0x8;
+			break;
+		}
 		break;
-	case 8:
-		rev = 0x6;
+
+	case INTEL_FAM6_ATOM_GOLDMONT_X:
+		switch (cpu_data(cpu).x86_stepping) {
+		case 1:
+			rev = 0x1a;
+			break;
+		}
 		break;
+
+	case INTEL_FAM6_ATOM_GOLDMONT_PLUS:
+		switch (cpu_data(cpu).x86_stepping) {
+		case 1:
+			rev = 0x28;
+			break;
+		case 8:
+			rev = 0x6;
+			break;
+		}
 	}
 
 	return (cpu_data(cpu).microcode < rev);
 }
 
-static __init void intel_glp_counter_freezing_quirk(void)
+static __init void intel_atom_v4_counter_freezing_quirk(void)
 {
 	/* Check if it's already disabled */
 	if (disable_counter_freezing)

@@ -3865,7 +3890,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_atom_v4_counter_freezing_broken(raw_smp_processor_id())) {
 		pr_info("PMU counter freezing disabled due to CPU errata,"
 			"please upgrade microcode\n");
 		x86_pmu.counter_freezing = false;

@@ -4196,6 +4221,7 @@ __init int intel_pmu_init(void)
 
 	case INTEL_FAM6_ATOM_GOLDMONT:
 	case INTEL_FAM6_ATOM_GOLDMONT_X:
+		x86_add_quirk(intel_atom_v4_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,

@@ -4222,7 +4248,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_atom_v4_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,
Signature · 1 line
2.7.4
#1 lore raw
Quoted 34 lines · On Tue, 2 Oct 2018, [email protected] wrote:
arch/x86/events/intel/core.c +24 -5
+static bool intel_atom_v4_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;
+	switch (cpu_data(cpu).x86_model) {
+	case INTEL_FAM6_ATOM_GOLDMONT:
+		switch (cpu_data(cpu).x86_stepping) {
+		case 2:
+			rev = 0xe;
+			break;
+		case 9:
+			rev = 0x2e;
+			break;
+		case 10:
+			rev = 0x8;
+			break;
+		}
 		break;
-	case 8:
-		rev = 0x6;
+
+	case INTEL_FAM6_ATOM_GOLDMONT_X:
+		switch (cpu_data(cpu).x86_stepping) {
+		case 1:
+			rev = 0x1a;
+			break;
+		}
 		break;
+
+	case INTEL_FAM6_ATOM_GOLDMONT_PLUS:
+		switch (cpu_data(cpu).x86_stepping) {

arch/x86/events/intel/core.c static __init void intel_nehalem_quirk(void) · line 3880

Quoted 10 lines
arch/x86/events/intel/core.c +7 -0
+		case 1:
+			rev = 0x28;
+			break;
+		case 8:
+			rev = 0x6;
+			break;
+		}
 	}
 
 	return (cpu_data(cpu).microcode < rev);

There is another variant of model/stepping micro code verification code in intel_snb_pebs_broken(). Can we please make this table based and use a common function? That's certainly not the last quirk we're going to have.

We already have a table based variant of ucode checking in bad_spectre_microcode(). It's trivial enough to generalize that.

Thanks,

tglx

author
#2 lore raw
Quoted 44 lines · Thomas Gleixner
Quoted 43 lines · On Tue, 2 Oct 2018, [email protected] wrote:
arch/x86/events/intel/core.c +31 -5
+static bool intel_atom_v4_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;
+	switch (cpu_data(cpu).x86_model) {
+	case INTEL_FAM6_ATOM_GOLDMONT:
+		switch (cpu_data(cpu).x86_stepping) {
+		case 2:
+			rev = 0xe;
+			break;
+		case 9:
+			rev = 0x2e;
+			break;
+		case 10:
+			rev = 0x8;
+			break;
+		}
  		break;
-	case 8:
-		rev = 0x6;
+
+	case INTEL_FAM6_ATOM_GOLDMONT_X:
+		switch (cpu_data(cpu).x86_stepping) {
+		case 1:
+			rev = 0x1a;
+			break;
+		}
  		break;
+
+	case INTEL_FAM6_ATOM_GOLDMONT_PLUS:
+		switch (cpu_data(cpu).x86_stepping) {
+		case 1:
+			rev = 0x28;
+			break;
+		case 8:
+			rev = 0x6;
+			break;
+		}
  	}
Quoted 9 lines
Quoted 1 line
  	return (cpu_data(cpu).microcode < rev);

There is another variant of model/stepping micro code verification code in intel_snb_pebs_broken(). Can we please make this table based and use a common function? That's certainly not the last quirk we're going to have.

We already have a table based variant of ucode checking in bad_spectre_microcode(). It's trivial enough to generalize that.

Sure, I will generalize the bad_spectre_microcode(), rename it to is_bad_intel_microcode(), and move it to arch\x86\kernel\cpu\microcode\intel.c. The spectre code and perf code will share the generalized function.

Thanks,
Kan

#3 lore raw
Quoted 5 lines · Kan
Quoted 4 lines · Thomas Gleixner

There is another variant of model/stepping micro code verification code in intel_snb_pebs_broken(). Can we please make this table based and use a common function? That's certainly not the last quirk we're going to have.

Quoted 7 lines
Quoted 3 lines

We already have a table based variant of ucode checking in bad_spectre_microcode(). It's trivial enough to generalize that.

Sure, I will generalize the bad_spectre_microcode(), rename it to is_bad_intel_microcode(), and move it to arch\x86\kernel\cpu\microcode\intel.c.

I suggest: is_bad_microcode() and have it in cpu/microcode/core.c unless you are claiming that bad microcode() is an intel only feature.

Quoted 1 line

The spectre code and perf code will share the generalized function.

Right. The tables stay in the calling code of course.

Thanks,

tglx

author
#4 lore raw
Quoted 10 lines · Thomas Gleixner
Quoted 9 lines · Kan
Quoted 7 lines · Thomas Gleixner

There is another variant of model/stepping micro code verification code in intel_snb_pebs_broken(). Can we please make this table based and use a common function? That's certainly not the last quirk we're going to have.

We already have a table based variant of ucode checking in bad_spectre_microcode(). It's trivial enough to generalize that.

Quoted 7 lines
Quoted 3 lines

Sure, I will generalize the bad_spectre_microcode(), rename it to is_bad_intel_microcode(), and move it to arch\x86\kernel\cpu\microcode\intel.c.

I suggest: is_bad_microcode() and have it in cpu/microcode/core.c unless you are claiming that bad microcode() is an intel only feature.

Yes, other platforms also have microcode issues.
To make it more generic, I think we also need to extend the struct sku_microcode to check vendor and family. The "model" in struct x86_cpu_id is u16. I will also change "model" and "stepping" to u16.

struct sku_microcode {
	u16 vendor;
	u16 family;
	u16 model;
	u16 stepping;
	u32 microcode;
};

Thanks,
Kan

#5 lore raw
Quoted 27 lines · Kan
Quoted 12 lines · Thomas Gleixner
Quoted 11 lines · Kan
Quoted 9 lines · Thomas Gleixner

There is another variant of model/stepping micro code verification code in
intel_snb_pebs_broken(). Can we please make this table based and use a common function? That's certainly not the last quirk we're going to have.

We already have a table based variant of ucode checking in bad_spectre_microcode(). It's trivial enough to generalize that.

Quoted 7 lines
Quoted 3 lines

Sure, I will generalize the bad_spectre_microcode(), rename it to is_bad_intel_microcode(), and move it to arch\x86\kernel\cpu\microcode\intel.c.

I suggest: is_bad_microcode() and have it in cpu/microcode/core.c unless you are claiming that bad microcode() is an intel only feature.

Yes, other platforms also have microcode issues.
To make it more generic, I think we also need to extend the struct sku_microcode to check vendor and family.
The "model" in struct x86_cpu_id is u16. I will also change "model" and "stepping" to u16.

Quoted 7 lines
struct sku_microcode {
	u16 vendor;
	u16 family;
	u16 model;
	u16 stepping;
	u32 microcode;
};

Makes sense.

Thanks,

tglx

author
#6 lore raw
In reply to Liang, Kan
Quoted 10 lines · On 10/3/2018 10:15 AM, [email protected] wrote:

To make it more generic, I think we also need to extend the struct sku_microcode to check vendor and family. The "model" in struct x86_cpu_id is u16. I will also change "model" and "stepping" to u16.

struct sku_microcode {
    u16 vendor;
    u16 family;
    u16 model;
    u16 stepping;
    u32 microcode;
};

No, should be consistent as struct cpuinfo_x86.
The struct sku_microcode should be

struct sku_microcode {
      u8 vendor;
      u8 family;
      u8 model;
      u8 stepping;
      u32 microcode;
};

Thanks,
Kan

maintainer
#7 lore raw
Quoted 16 lines · Kan
Quoted 5 lines · On 10/3/2018 10:15 AM, [email protected] wrote:

To make it more generic, I think we also need to extend the struct sku_microcode to check vendor and family.
The "model" in struct x86_cpu_id is u16. I will also change "model" and "stepping" to u16.

Quoted 7 lines

struct sku_microcode {
    u16 vendor;
    u16 family;
    u16 model;
    u16 stepping;
    u32 microcode;
};

Quoted 4 lines

No, should be consistent as struct cpuinfo_x86.
The struct sku_microcode should be

struct sku_microcode {

And drop that "sku_" prefix. Call this a struct microcode_bl_entry or so, to be clear what it is.

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
#8 lore raw
In reply to Thomas Gleixner
Quoted 3 lines

There is another variant of model/stepping micro code verification code in intel_snb_pebs_broken(). Can we please make this table based and use a common function? That's certainly not the last quirk we're going to have.

I have a patch to add a table driven microcode matcher for another fix. Will post shortly.

-Andi

maintainer
#9 lore raw
In reply to Thomas Gleixner
Quoted 8 lines · Thomas Gleixner

On Tue, 2 Oct 2018, [email protected] wrote:

There is another variant of model/stepping micro code verification code in intel_snb_pebs_broken(). Can we please make this table based and use a common function? That's certainly not the last quirk we're going to have.

We already have a table based variant of ucode checking in bad_spectre_microcode(). It's trivial enough to generalize that.

apic_check_deadline_errata() is another one. That one already uses the x86_cpu_id thing, but still plays silly games for steppings. So if we're going to build a new microcode table matcher...

maintainer
#10 lore raw
Quoted 7 lines · Peter Zijlstra
Quoted 6 lines · Thomas Gleixner

On Tue, 2 Oct 2018, [email protected] wrote:

There is another variant of model/stepping micro code verification code in intel_snb_pebs_broken(). Can we please make this table based and use a common function? That's certainly not the last quirk we're going to have.

Quoted 6 lines
Quoted 2 lines

We already have a table based variant of ucode checking in bad_spectre_microcode(). It's trivial enough to generalize that.

apic_check_deadline_errata() is another one. That one already uses the x86_cpu_id thing, but still plays silly games for steppings. So if we're going to build a new microcode table matcher...

intel_snb_pebs_broken() looks like a potential candidate too...

Signature · 4 lines
Regards/Gruss,
    Boris.

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