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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <64b1becd-ebf8-40ae-af5a-59d32b652362@zhaoxin.com>
Date: Sat, 14 Sep 2024 17:36:39 +0800
From: Tony W Wang-oc <TonyWWang-oc@...oxin.com>
To: Dave Hansen <dave.hansen@...el.com>, Borislav Petkov <bp@...en8.de>
CC: <tglx@...utronix.de>, <mingo@...hat.com>, <dave.hansen@...ux.intel.com>,
	<x86@...nel.org>, <hpa@...or.com>, <tony.luck@...el.com>,
	<linux-kernel@...r.kernel.org>, <linux-edac@...r.kernel.org>,
	<CobeChen@...oxin.com>, <TimGuo@...oxin.com>, <LeoLiu-oc@...oxin.com>, "Lyle
 Li" <LyleLi@...oxin.com>
Subject: Re: [PATCH v1 1/3] x86/mce: Add centaur vendor to support Zhaoxin MCA



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