Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
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.li...@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.
Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
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.li...@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.
Re: [PATCH] perf/x86/intel: Add counter freezing quirk for Goldmont
On Wed, Oct 03, 2018 at 08:10:31AM +0200, Thomas Gleixner wrote: > On Tue, 2 Oct 2018, kan.li...@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
On Wed, Oct 03, 2018 at 08:10:31AM +0200, Thomas Gleixner wrote: > On Tue, 2 Oct 2018, kan.li...@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
> 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
> 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
On Wed, Oct 03, 2018 at 10:25:03AM -0400, Liang, Kan wrote: > > > On 10/3/2018 10:15 AM, linux-kernel-ow...@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
On Wed, Oct 03, 2018 at 10:25:03AM -0400, Liang, Kan wrote: > > > On 10/3/2018 10:15 AM, linux-kernel-ow...@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
On 10/3/2018 10:15 AM, linux-kernel-ow...@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
On 10/3/2018 10:15 AM, linux-kernel-ow...@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
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
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
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
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
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
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
On 10/3/2018 2:10 AM, Thomas Gleixner wrote: On Tue, 2 Oct 2018, kan.li...@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
On 10/3/2018 2:10 AM, Thomas Gleixner wrote: On Tue, 2 Oct 2018, kan.li...@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
On Tue, 2 Oct 2018, kan.li...@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
On Tue, 2 Oct 2018, kan.li...@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