Re: [XEN PATCH v2 5/5] x86/MCE: optional build of AMD/Intel MCE code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2024 11:21, Sergiy Kibrik wrote:
> Separate Intel/AMD-specific MCE code using CONFIG_{INTEL,AMD} config options.
> Now we can avoid build of mcheck code if support for specific platform is
> intentionally disabled by configuration.
> 
> Add default return value to init_nonfatal_mce_checker() routine -- in case
> of a build with both AMD and INTEL options are off (e.g. randconfig).

I'm afraid that, as before, I can't accept this as a justification for the
addition. The addition likely is wanted, but perhaps in a separate up-front
patch and explaining what's wrong when that's missing.

> Also global Intel-specific variables lmce_support & cmci_support have to be
> redefined if !INTEL, as they get checked in common code.

Them being checked in common code may have different resolution strategies.
The justification here imo is that, right now, both variables are only ever
written by mce_intel.c. As mentioned for vmce_has_lmce(), there's nothing
fundamentally preventing MCG_CAP from having respective bits set on a non-
Intel CPU.

> --- a/xen/arch/x86/cpu/mcheck/mce.h
> +++ b/xen/arch/x86/cpu/mcheck/mce.h
> @@ -40,7 +40,11 @@ enum mcheck_type {
>  };
>  
>  extern uint8_t cmci_apic_vector;
> +#ifdef CONFIG_INTEL
>  extern bool lmce_support;
> +#else
> +#define lmce_support (false)

Nit: Neither here nor ...

> @@ -120,7 +124,11 @@ DECLARE_PER_CPU(struct mca_banks *, poll_bankmask);
>  DECLARE_PER_CPU(struct mca_banks *, no_cmci_banks);
>  DECLARE_PER_CPU(struct mca_banks *, mce_clear_banks);
>  
> +#ifdef CONFIG_INTEL
>  extern bool cmci_support;
> +#else
> +#define cmci_support (false)

... here parentheses are really needed.

> --- a/xen/arch/x86/cpu/mcheck/non-fatal.c
> +++ b/xen/arch/x86/cpu/mcheck/non-fatal.c
> @@ -24,14 +24,20 @@ static int __init cf_check init_nonfatal_mce_checker(void)
>        * Check for non-fatal errors every MCE_RATE s
>        */
>       switch (c->x86_vendor) {
> +#ifdef CONFIG_AMD
>       case X86_VENDOR_AMD:
>       case X86_VENDOR_HYGON:
>               /* Assume we are on K8 or newer AMD or Hygon CPU here */
>               amd_nonfatal_mcheck_init(c);
>               break;
> +#endif
> +#ifdef CONFIG_INTEL
>       case X86_VENDOR_INTEL:
>               intel_nonfatal_mcheck_init(c);
>               break;
> +#endif
> +     default:
> +             return -ENODEV;
>       }
>       printk(KERN_INFO "mcheck_poll: Machine check polling timer started.\n");
>       return 0;

Along the lines of remarks on earlier patches, it would be a good opportunity
here to add missing blank lines between non-fall-through case blocks.

Jan