linux-kernel - Re: [PATCH v2] x86/mce: add CMCI support for Centaur CPUs

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 4 Jul 2018 10:45:38 +0800
From:   David Wang <davidwang@...oxin.com>
To:     'Borislav Petkov' <bp@...en8.de>
CC:     <tony.luck@...el.com>, <mingo@...hat.com>, <tglx@...utronix.de>,
        <hpa@...or.com>, <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
        <linux-edac@...r.kernel.org>, <cooperyan@...oxin.com>,
        <qiyuanwang@...oxin.com>, <benjaminpan@...tech.com>,
        <lukelin@...cpu.com>, <timguo@...oxin.com>
Subject: Re: [PATCH v2] x86/mce: add CMCI support for Centaur CPUs



> -----Original Mail-----
>  Sender: Borislav Petkov [mailto:bp@...en8.de]
>  Time: 2018年6月26日 22:30
>  Receiver: David Wang <davidwang@...oxin.com>
> CC: tony.luck@...el.com; mingo@...hat.com; tglx@...utronix.de;
> hpa@...or.com; x86@...nel.org; linux-kernel@...r.kernel.org;
> linux-edac@...r.kernel.org; cooperyan@...oxin.com;
> qiyuanwang@...oxin.com; benjaminpan@...tech.com;
> lukelin@...cpu.com; timguo@...oxin.com
> Topic : Re: [PATCH v2] x86/mce: add CMCI support for Centaur CPUs
> 
> On Mon, Jun 04, 2018 at 10:37:33AM +0800, David Wang wrote:
> > New Centaur CPU support CMCI mechanism, which is compatible with
> INTEL CMCI.
> >
> > Signed-off-by: David Wang <davidwang@...oxin.com>
> 
> ...
> 
> > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c
> > b/arch/x86/kernel/cpu/mcheck/mce.c
> > index cd76380..2ebafc7 100644
> > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > @@ -1727,6 +1727,7 @@ static void __mcheck_cpu_init_early(struct
> cpuinfo_x86 *c)
> >  	}
> >  }
> >
> > +#ifdef CONFIG_X86_MCE_CENTAUR
> >  static void mce_centaur_feature_init(struct cpuinfo_x86 *c)  {
> >  	struct mca_config *cfg = &mca_cfg;
> > @@ -1740,7 +1741,12 @@ static void mce_centaur_feature_init(struct
> cpuinfo_x86 *c)
> >  		if (cfg->monarch_timeout < 0)
> >  			cfg->monarch_timeout = USEC_PER_SEC;
> >  	}
> > +	mce_intel_feature_init(c);
> > +	mce_adjust_timer = cmci_intel_adjust_timer;
> 
> This ...
> 
> >  }
> > +#else
> > +static inline void mce_centaur_feature_init(struct cpuinfo_x86 *c) {
> > +} #endif
> >
> >  static void __mcheck_cpu_init_vendor(struct cpuinfo_x86 *c)  { diff
> > --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c
> > b/arch/x86/kernel/cpu/mcheck/mce_intel.c
> > index d05be30..5b1b68f 100644
> > --- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
> > +++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
> > @@ -85,7 +85,8 @@ static int cmci_supported(int *banks)
> >  	 * initialization is vendor keyed and this
> >  	 * makes sure none of the backdoors are entered otherwise.
> >  	 */
> > -	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> > +	if ((boot_cpu_data.x86_vendor != X86_VENDOR_INTEL &&
> > +		boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR))
> >  		return 0;
> >  	if (!boot_cpu_has(X86_FEATURE_APIC) || lapic_get_maxlvt() < 6)
> >  		return 0;
> > @@ -506,10 +507,20 @@ static void intel_ppin_init(struct cpuinfo_x86
> > *c)
> >
> >  void mce_intel_feature_init(struct cpuinfo_x86 *c)  {
> > -	intel_init_thermal(c);
> > -	intel_init_cmci();
> > -	intel_init_lmce();
> > -	intel_ppin_init(c);
> > +
> > +	switch (c->x86_vendor) {
> > +	case X86_VENDOR_INTEL:
> > +		intel_init_thermal(c);
> > +		intel_init_cmci();
> > +		intel_init_lmce();
> > +		intel_ppin_init(c);
> > +		break;
> > +	case X86_VENDOR_CENTAUR:
> > +		intel_init_cmci();
> 
> ... and this I really don't like for the simple reason that if the Intel side gets
> changed, it could potentially break Centaur. And we don't want that. And
> the vendor should be free to change their code without asking another
> vendor for permission even if the other vendor is almost copying the
> code...
> 
> Long story short, I think you should extract the facilities you're going to
> need into generic, library-like ones and call them from centaur-specific
> compilation units which get enabled when CPU_SUP_CENTAUR is enabled.
> 
> So that the code can still be shared but there's no dependency on other
> vendors and so that one vendor doesn't break the other one and
> vice-versa.
> 
> IMO.
> 
> Thx.
> 
	OK. I will adjust code.
	Thank you.
> --
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.