* [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
@ 2018-10-02 21:30 kan.liang
2018-10-03 6:10 ` Thomas Gleixner
0 siblings, 1 reply; 11+ messages in thread
From: kan.liang @ 2018-10-02 21:30 UTC (permalink / raw)
To: peterz, tglx, mingo, acme, linux-kernel
Cc: eranian, ak, alexander.shishkin, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
2018-10-02 21:30 [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont kan.liang
@ 2018-10-03 6:10 ` Thomas Gleixner
2018-10-03 13:32 ` Liang, Kan
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Thomas Gleixner @ 2018-10-03 6:10 UTC (permalink / raw)
To: Kan Liang
Cc: peterz, mingo, acme, linux-kernel, eranian, ak, alexander.shishkin
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
2018-10-03 6:10 ` Thomas Gleixner
@ 2018-10-03 13:32 ` Liang, Kan
2018-10-03 13:55 ` Thomas Gleixner
2018-10-03 14:33 ` Andi Kleen
2018-10-03 15:41 ` Peter Zijlstra
2 siblings, 1 reply; 11+ messages in thread
From: Liang, Kan @ 2018-10-03 13:32 UTC (permalink / raw)
To: Thomas Gleixner
Cc: peterz, mingo, acme, linux-kernel, eranian, ak, alexander.shishkin
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
2018-10-03 13:32 ` Liang, Kan
@ 2018-10-03 13:55 ` Thomas Gleixner
2018-10-03 14:15 ` Liang, Kan
0 siblings, 1 reply; 11+ messages in thread
From: Thomas Gleixner @ 2018-10-03 13:55 UTC (permalink / raw)
To: Liang, Kan
Cc: Peter Zijlstra, mingo, acme, LKML, eranian, ak, alexander.shishkin, x86
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
2018-10-03 13:55 ` Thomas Gleixner
@ 2018-10-03 14:15 ` Liang, Kan
2018-10-03 14:24 ` Thomas Gleixner
2018-10-03 14:25 ` Liang, Kan
0 siblings, 2 replies; 11+ messages in thread
From: Liang, Kan @ 2018-10-03 14:15 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Peter Zijlstra, mingo, acme, LKML, eranian, ak, alexander.shishkin, x86
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
2018-10-03 14:15 ` Liang, Kan
@ 2018-10-03 14:24 ` Thomas Gleixner
2018-10-03 14:25 ` Liang, Kan
1 sibling, 0 replies; 11+ messages in thread
From: Thomas Gleixner @ 2018-10-03 14:24 UTC (permalink / raw)
To: Liang, Kan
Cc: Peter Zijlstra, mingo, acme, LKML, eranian, ak, alexander.shishkin, x86
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
2018-10-03 14:15 ` Liang, Kan
2018-10-03 14:24 ` Thomas Gleixner
@ 2018-10-03 14:25 ` Liang, Kan
2018-10-03 14:27 ` Borislav Petkov
1 sibling, 1 reply; 11+ messages in thread
From: Liang, Kan @ 2018-10-03 14:25 UTC (permalink / raw)
To: linux-kernel-owner, Thomas Gleixner
Cc: Peter Zijlstra, mingo, acme, LKML, eranian, ak, alexander.shishkin, x86
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
2018-10-03 14:25 ` Liang, Kan
@ 2018-10-03 14:27 ` Borislav Petkov
0 siblings, 0 replies; 11+ messages in thread
From: Borislav Petkov @ 2018-10-03 14:27 UTC (permalink / raw)
To: Liang, Kan
Cc: linux-kernel-owner, Thomas Gleixner, Peter Zijlstra, mingo, acme,
LKML, eranian, ak, alexander.shishkin, x86
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
2018-10-03 6:10 ` Thomas Gleixner
2018-10-03 13:32 ` Liang, Kan
@ 2018-10-03 14:33 ` Andi Kleen
2018-10-03 15:41 ` Peter Zijlstra
2 siblings, 0 replies; 11+ messages in thread
From: Andi Kleen @ 2018-10-03 14:33 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Kan Liang, peterz, mingo, acme, linux-kernel, eranian,
alexander.shishkin
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
2018-10-03 6:10 ` Thomas Gleixner
2018-10-03 13:32 ` Liang, Kan
2018-10-03 14:33 ` Andi Kleen
@ 2018-10-03 15:41 ` Peter Zijlstra
2018-10-03 15:49 ` Borislav Petkov
2 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2018-10-03 15:41 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Kan Liang, mingo, acme, linux-kernel, eranian, ak, alexander.shishkin
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...
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
2018-10-03 15:41 ` Peter Zijlstra
@ 2018-10-03 15:49 ` Borislav Petkov
0 siblings, 0 replies; 11+ messages in thread
From: Borislav Petkov @ 2018-10-03 15:49 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Thomas Gleixner, Kan Liang, mingo, acme, linux-kernel, eranian,
ak, alexander.shishkin
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-10-03 15:49 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-02 21:30 [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont kan.liang
2018-10-03 6:10 ` Thomas Gleixner
2018-10-03 13:32 ` Liang, Kan
2018-10-03 13:55 ` Thomas Gleixner
2018-10-03 14:15 ` Liang, Kan
2018-10-03 14:24 ` Thomas Gleixner
2018-10-03 14:25 ` Liang, Kan
2018-10-03 14:27 ` Borislav Petkov
2018-10-03 14:33 ` Andi Kleen
2018-10-03 15:41 ` Peter Zijlstra
2018-10-03 15:49 ` Borislav Petkov
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).