Re: [PATCH v1 1/3] x86/mce: Add centaur vendor to support Zhaoxin MCA

[Tony W Wang-oc]

On 2024/9/13 23:47, Dave Hansen wrote:
> [这封邮件来自外部发件人 谨防风险]
>
> On 9/13/24 07:27, Borislav Petkov wrote:
> > > +    if (c->x86_vendor == X86_VENDOR_CENTAUR) {
> > > +            /*
> > > +             * All newer Centaur CPUs support MCE broadcasting. Enable
> > > +             * synchronization with a one second timeout.
> > > +             */
> > > +            if ((c->x86 == 6 && c->x86_model == 0xf && c->x86_stepping >= 0xe) ||
> > > +                 c->x86 > 6) {
> > > +                    if (cfg->monarch_timeout < 0)
> > > +                            cfg->monarch_timeout = USEC_PER_SEC;
> > > +            }
> > > +    }
> > So if centaur == zhaoxin, why aren't you moving this hunk to
> > mce_zhaoxin_feature_init() instead?
>
> The centaur and zhaoxin logic is also _really_ close here:
>
> >                  if (c->x86 > 6 || (c->x86_model == 0x19 || c->x86_model == 0x1f)) {
> >                          if (cfg->monarch_timeout < 0)
> >                                  cfg->monarch_timeout = USEC_PER_SEC;
> >                  }
>
> vs
>
> >          if ((c->x86 == 6 && c->x86_model == 0xf && c->x86_stepping >= 0xe) ||
> >               c->x86 > 6) {
> >                  if (cfg->monarch_timeout < 0)
> >                          cfg->monarch_timeout = USEC_PER_SEC;
> >          }
>
> I'd just randomly guess that the zhaoxin version is buggy because it
> doesn't do a c->x86 check before the "(c->x86_model == 0x19 ||
> c->x86_model == 0x1f)".

Yes, the check for c->x86 == 6 is omitted in the zhaoxin version.

> So instead of copying and pasting the same block over and over, can we
> consolidate it a bit?
>
> foo()
> {
>          /* Older CPUs do not do MCE broadcast: */
>          if (c->x86 < 6)
>                  return;
>          /* All newer ones do: */
>          if (c->x86 > 6)
>                  goto mce_broadcast;
>
>          /* Family 6 is mixed: */
>          if (c->x86_vendor == X86_VENDOR_CENTAUR) {
>                  if (c->x86_model == 0xf &&
>                      c->x86_stepping >= 0xe)
>                          goto mce_broadcast;
>          } else if (c->x86_vendor == X86_VENDOR_ZHAOXIN) {
>                  if (c->x86_model == 0x19 ||
>                      c->x86_model == 0x1f))
>                          goto mce_broadcast;
>          }
>
>          return;
>
> mce_broadcast:
>          if (cfg->monarch_timeout < 0)
>                  cfg->monarch_timeout = USEC_PER_SEC;
> }

Thank you! That makes more sense, and will adopt it into zhaoxin.c

Sincerely!
TonyWWang-oc