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

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.