Re: [XEN PATCH v1 7/7] x86/MCE: optional build of AMD/Intel MCE code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.04.2024 01:16, Stefano Stabellini wrote:
> On Tue, 23 Apr 2024, Sergiy Kibrik wrote:
>> --- a/xen/arch/x86/cpu/mcheck/Makefile
>> +++ b/xen/arch/x86/cpu/mcheck/Makefile
>> @@ -1,12 +1,10 @@
>> -obj-y += amd_nonfatal.o
>> -obj-y += mce_amd.o
>>  obj-y += mcaction.o
>>  obj-y += barrier.o
>> -obj-y += intel-nonfatal.o
>>  obj-y += mctelem.o
>>  obj-y += mce.o
>>  obj-y += mce-apei.o
>> -obj-y += mce_intel.o
>> +obj-$(CONFIG_AMD) += mce_amd.o amd_nonfatal.o
>> +obj-$(CONFIG_INTEL) += mce_intel.o intel-nonfatal.o
>>  obj-y += non-fatal.o
>>  obj-y += util.o
>>  obj-y += vmce.o
> 
> Awesome!

Almost. I'd appreciate if the ordering of files would be retained. It's
not quite alphabetic, but still. Moving mce_amd.o and mcaction.o to their
designated slots may or may not be done right here.

>> --- 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;

This, while perhaps desirable, doesn't fit ...

>>      }
>>      printk(KERN_INFO "mcheck_poll: Machine check polling timer started.\n");
>>      return 0;

... earlier behavior, and hence is somewhat unexpected in a change which, by
its description, looks like a "no functional change" one.

> For consistency in all other cases this patch series uses IS_ENABLED
> checks. They could be used here as well.

Hmm, I think for switch() statements like this (see also comments elsewhere
on this series) using #ifdef is overall better.

Jan