| Message ID | 20241111060428.44258-6-qiuxu.zhuo@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/mce: Clean up some x86/mce code | expand |
On 11/10/2024 10:04 PM, Qiuxu Zhuo wrote: > Convert family/model mixed checks to VFM-based checks to make > the code more compact. > > Suggested-by: Sohil Mehta <sohil.mehta@intel.com> > Suggested-by: Dave Hansen <dave.hansen@intel.com> > Reviewed-by: Tony Luck <tony.luck@intel.com> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> > --- Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
On Mon, Nov 11, 2024 at 02:04:25PM +0800, Qiuxu Zhuo wrote: > Convert family/model mixed checks to VFM-based checks to make > the code more compact. > > Suggested-by: Sohil Mehta <sohil.mehta@intel.com> > Suggested-by: Dave Hansen <dave.hansen@intel.com> > Reviewed-by: Tony Luck <tony.luck@intel.com> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> > --- > Changes in v4: > - No changes but rebased. > > Changes in v3: > - Newly added. > > arch/x86/kernel/cpu/mce/core.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c > index 3855ec2ed0e0..d288cc7390f6 100644 > --- a/arch/x86/kernel/cpu/mce/core.c > +++ b/arch/x86/kernel/cpu/mce/core.c > @@ -1954,6 +1954,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c) > { > struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array); > > + /* Older CPUs (prior to family 6) don't need quirks. */ > + if (c->x86_vfm < INTEL_PENTIUM_PRO) > + return; > + Is it possible for pre-"family 6" to get here? Family 5 is "ancient" which has its own MCE init path. And I assume anything older doesn't support MCE/MCA. Is this correct? > /* > * SDM documents that on family 6 bank 0 should not be written > * because it aliases to another special BIOS controlled > @@ -1962,22 +1966,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c) > * Don't ignore bank 0 completely because there could be a > * valid event later, merely don't write CTL0. > */ > - if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0) > + if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks) > 0) The "> 0" is not needed, since mce_num_banks is unsigned int. Otherwise, looks good. Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com> Thanks, Yazen
Hi Yazen, > From: Yazen Ghannam <yazen.ghannam@amd.com> > [...] > > @@ -1954,6 +1954,10 @@ static void apply_quirks_intel(struct > > cpuinfo_x86 *c) { > > struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array); > > > > + /* Older CPUs (prior to family 6) don't need quirks. */ > > + if (c->x86_vfm < INTEL_PENTIUM_PRO) > > + return; > > + > > Is it possible for pre-"family 6" to get here? > > Family 5 is "ancient" which has its own MCE init path. And I assume anything > older doesn't support MCE/MCA. Is this correct? Yes, there is an early return in __mcheck_cpu_ancient_init() for Family 5. However, this code explicitly indicates that "prior to families 6 don't need quirks" and addresses concerns like: https://lore.kernel.org/all/dcfdba92-7004-413d-8011-12771636d11f@intel.com/ > > /* > > * SDM documents that on family 6 bank 0 should not be written > > * because it aliases to another special BIOS controlled @@ -1962,22 > > +1966,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c) > > * Don't ignore bank 0 completely because there could be a > > * valid event later, merely don't write CTL0. > > */ > > - if (c->x86 == 6 && c->x86_model < 0x1A && > this_cpu_read(mce_num_banks) > 0) > > + if (c->x86_vfm < INTEL_NEHALEM_EP && > this_cpu_read(mce_num_banks) > > > +0) > > The "> 0" is not needed, since mce_num_banks is unsigned int. I don't get your point here. But it needs to check for the case where mce_num_banks == 0. > Otherwise, looks good. > > Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com> Thanks! -Qiuxu
On Wed, Nov 13, 2024 at 12:10:31PM +0000, Zhuo, Qiuxu wrote: > Hi Yazen, > > > From: Yazen Ghannam <yazen.ghannam@amd.com> > > [...] > > > @@ -1954,6 +1954,10 @@ static void apply_quirks_intel(struct > > > cpuinfo_x86 *c) { > > > struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array); > > > > > > + /* Older CPUs (prior to family 6) don't need quirks. */ > > > + if (c->x86_vfm < INTEL_PENTIUM_PRO) > > > + return; > > > + > > > > Is it possible for pre-"family 6" to get here? > > > > Family 5 is "ancient" which has its own MCE init path. And I assume anything > > older doesn't support MCE/MCA. Is this correct? > > Yes, there is an early return in __mcheck_cpu_ancient_init() for Family 5. > However, this code explicitly indicates that "prior to families 6 don't need quirks" > and addresses concerns like: > > https://lore.kernel.org/all/dcfdba92-7004-413d-8011-12771636d11f@intel.com/ > Right, but my point is that this check would never be executed, since the older systems would not get here during init. So this seems like dead code. > > > /* > > > * SDM documents that on family 6 bank 0 should not be written > > > * because it aliases to another special BIOS controlled @@ -1962,22 > > > +1966,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c) > > > * Don't ignore bank 0 completely because there could be a > > > * valid event later, merely don't write CTL0. > > > */ > > > - if (c->x86 == 6 && c->x86_model < 0x1A && > > this_cpu_read(mce_num_banks) > 0) > > > + if (c->x86_vfm < INTEL_NEHALEM_EP && > > this_cpu_read(mce_num_banks) > > > > +0) > > > > The "> 0" is not needed, since mce_num_banks is unsigned int. > > I don't get your point here. > But it needs to check for the case where mce_num_banks == 0. > The check is "mce_num_banks > 0", and mce_num_banks is an unsigned int. Therefore, the check is reduced to "mce_num_banks != 0". In this case, you can just do "if (mce_num_banks)" to the same effect. Thanks, Yazen
Hi Yazen, > From: Yazen Ghannam <yazen.ghannam@amd.com> > [...] > > > > + if (c->x86_vfm < INTEL_NEHALEM_EP && > > > this_cpu_read(mce_num_banks) > > > > > +0) > > > > > > The "> 0" is not needed, since mce_num_banks is unsigned int. > > > > I don't get your point here. > > But it needs to check for the case where mce_num_banks == 0. > > > > The check is "mce_num_banks > 0", and mce_num_banks is an unsigned int. > Therefore, the check is reduced to "mce_num_banks != 0". In this case, you > can just do "if (mce_num_banks)" to the same effect. I got you. OK, if nobody else objects, I'll update it in the next version. [ Hope others won't blame this as over-optimization. ] -Qiuxu
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index 3855ec2ed0e0..d288cc7390f6 100644 --- a/arch/x86/kernel/cpu/mce/core.c +++ b/arch/x86/kernel/cpu/mce/core.c @@ -1954,6 +1954,10 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c) { struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array); + /* Older CPUs (prior to family 6) don't need quirks. */ + if (c->x86_vfm < INTEL_PENTIUM_PRO) + return; + /* * SDM documents that on family 6 bank 0 should not be written * because it aliases to another special BIOS controlled @@ -1962,22 +1966,21 @@ static void apply_quirks_intel(struct cpuinfo_x86 *c) * Don't ignore bank 0 completely because there could be a * valid event later, merely don't write CTL0. */ - if (c->x86 == 6 && c->x86_model < 0x1A && this_cpu_read(mce_num_banks) > 0) + if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks) > 0) mce_banks[0].init = false; /* * All newer Intel systems support MCE broadcasting. Enable * synchronization with a one second timeout. */ - if ((c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xe)) && - mca_cfg.monarch_timeout < 0) + if (c->x86_vfm >= INTEL_CORE_YONAH && mca_cfg.monarch_timeout < 0) mca_cfg.monarch_timeout = USEC_PER_SEC; /* * There are also broken BIOSes on some Pentium M and * earlier systems: */ - if (c->x86 == 6 && c->x86_model <= 13 && mca_cfg.bootlog < 0) + if (c->x86_vfm < INTEL_CORE_YONAH && mca_cfg.bootlog < 0) mca_cfg.bootlog = 0; if (c->x86_vfm == INTEL_SANDYBRIDGE_X)