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

[David Wang]

> -----Original Mail-----
>  Sender: Borislav Petkov [mailto:bp@xxxxxxxxx]
>  Time: 2018å6æ26æ 22:30
>  Receiver: David Wang <davidwang@xxxxxxxxxxx>
> CC: tony.luck@xxxxxxxxx; mingo@xxxxxxxxxx; tglx@xxxxxxxxxxxxx;
> hpa@xxxxxxxxx; x86@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-edac@xxxxxxxxxxxxxxx; cooperyan@xxxxxxxxxxx;
> qiyuanwang@xxxxxxxxxxx; benjaminpan@xxxxxxxxxxx;
> lukelin@xxxxxxxxxx; timguo@xxxxxxxxxxx
> 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@xxxxxxxxxxx>
> 
> ...
> 
> > 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.