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

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

[kan.liang]

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

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 <kan.liang@linux.intel.com>
---
 arch/x86/events/intel/core.c | 44 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index ab01ef9..56401bc 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -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,
-- 
2.7.4

Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont

[Thomas Gleixner]

On Tue, 2 Oct 2018, kan.liang@linux.intel.com wrote:
> +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);

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

Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont

[Liang, Kan]

On 10/3/2018 2:10 AM, Thomas Gleixner wrote:
> On Tue, 2 Oct 2018, kan.liang@linux.intel.com wrote:
>> +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);
> 
> 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

Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont

[Thomas Gleixner]

On Wed, 3 Oct 2018, Liang, Kan wrote:
> On 10/3/2018 2:10 AM, Thomas Gleixner 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.
> > 
> 
> 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.

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

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

Thanks,

	tglx

Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont

[Liang, Kan]

On 10/3/2018 9:55 AM, Thomas Gleixner wrote:
> On Wed, 3 Oct 2018, Liang, Kan wrote:
>> On 10/3/2018 2:10 AM, Thomas Gleixner 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.
>>>
>>
>> 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

Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont

[Thomas Gleixner]

On Wed, 3 Oct 2018, Liang, Kan wrote:
> On 10/3/2018 9:55 AM, Thomas Gleixner wrote:
> > On Wed, 3 Oct 2018, Liang, Kan wrote:
> > > On 10/3/2018 2:10 AM, Thomas Gleixner 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.
> > > > 
> > > 
> > > 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;
> };

Makes sense.

Thanks,

	tglx

Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont

[Liang, Kan]

On 10/3/2018 10:15 AM, linux-kernel-owner@vger.kernel.org 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

Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont

[Borislav Petkov]

On Wed, Oct 03, 2018 at 10:25:03AM -0400, Liang, Kan wrote:
> 
> 
> On 10/3/2018 10:15 AM, linux-kernel-owner@vger.kernel.org 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 {

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

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont

[Andi Kleen]

> 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

Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont

[Peter Zijlstra]

On Wed, Oct 03, 2018 at 08:10:31AM +0200, Thomas Gleixner wrote:
> On Tue, 2 Oct 2018, kan.liang@linux.intel.com 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...

Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont

[Borislav Petkov]

On Wed, Oct 03, 2018 at 05:41:32PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 03, 2018 at 08:10:31AM +0200, Thomas Gleixner wrote:
> > On Tue, 2 Oct 2018, kan.liang@linux.intel.com 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...

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

-- 
Regards/Gruss,
    Boris.

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