diff mbox series

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

Message ID 82df6ef350a2b4f42ec7adf12a90ebeae1d133f6.1713860310.git.Sergiy_Kibrik@epam.com (mailing list archive)
State Superseded
Headers show
Series x86: make Intel/AMD vPMU & MCE support configurable | expand

Commit Message

Sergiy Kibrik April 23, 2024, 9 a.m. UTC 
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.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
 xen/arch/x86/cpu/mcheck/Makefile    | 6 ++----
 xen/arch/x86/cpu/mcheck/non-fatal.c | 6 ++++++
 xen/arch/x86/cpu/mcheck/vmce.h      | 1 +
 3 files changed, 9 insertions(+), 4 deletions(-)

Comments

Stefano Stabellini April 26, 2024, 11:16 p.m. UTC | #1 
On Tue, 23 Apr 2024, 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.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> ---
>  xen/arch/x86/cpu/mcheck/Makefile    | 6 ++----
>  xen/arch/x86/cpu/mcheck/non-fatal.c | 6 ++++++
>  xen/arch/x86/cpu/mcheck/vmce.h      | 1 +
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/Makefile b/xen/arch/x86/cpu/mcheck/Makefile
> index f927f10b4d..5b3f6d875c 100644
> --- 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!

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> diff --git a/xen/arch/x86/cpu/mcheck/non-fatal.c b/xen/arch/x86/cpu/mcheck/non-fatal.c
> index 33cacd15c2..2d91a3b1e0 100644
> --- 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;

For consistency in all other cases this patch series uses IS_ENABLED
checks. They could be used here as well.
Jan Beulich April 29, 2024, 3:54 p.m. UTC | #2 
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
Sergiy Kibrik May 2, 2024, 8:59 a.m. UTC | #3 
29.04.24 18:54, Jan Beulich:
> 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.

sure, I'll leave ordering as before

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

I see, will try to describe it a bit better then.

   -Sergiy
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu/mcheck/Makefile b/xen/arch/x86/cpu/mcheck/Makefile
index f927f10b4d..5b3f6d875c 100644
--- 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
diff --git a/xen/arch/x86/cpu/mcheck/non-fatal.c b/xen/arch/x86/cpu/mcheck/non-fatal.c
index 33cacd15c2..2d91a3b1e0 100644
--- 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;