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

[Tony W Wang-oc <TonyWWang-oc@zhaoxin.com>]

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