[PATCH v6 02/15] x86/mce: Define BSP-only init

Yazen Ghannam posted 15 patches 5 months ago
There is a newer version of this series
[PATCH v6 02/15] x86/mce: Define BSP-only init
Posted by Yazen Ghannam 5 months ago
Currently, MCA initialization is executed identically on each CPU as
they are brought online. However, a number of MCA initialization tasks
only need to be done once.

Define a function to collect all 'global' init tasks and call this from
the BSP only. Start with CPU features.

Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Tested-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    Link:
    https://lore.kernel.org/r/20250825-wip-mca-updates-v5-8-865768a2eef8@amd.com
    
    v5->v6:
    * No change.
    
    v4->v5:
    * No change.
    
    v3->v4:
    * Change cpu_mca_init() to mca_bsp_init().
    * Drop code comment.
    
    v2->v3:
    * Add tags from Qiuxu and Tony.
    
    v1->v2:
    * New in v2.

 arch/x86/include/asm/mce.h     |  2 ++
 arch/x86/kernel/cpu/common.c   |  1 +
 arch/x86/kernel/cpu/mce/amd.c  |  3 ---
 arch/x86/kernel/cpu/mce/core.c | 28 +++++++++++++++++++++-------
 4 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 3224f3862dc8..31e3cb550fb3 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -241,12 +241,14 @@ struct cper_ia_proc_ctx;
 
 #ifdef CONFIG_X86_MCE
 int mcheck_init(void);
+void mca_bsp_init(struct cpuinfo_x86 *c);
 void mcheck_cpu_init(struct cpuinfo_x86 *c);
 void mcheck_cpu_clear(struct cpuinfo_x86 *c);
 int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
 			       u64 lapic_id);
 #else
 static inline int mcheck_init(void) { return 0; }
+static inline void mca_bsp_init(struct cpuinfo_x86 *c) {}
 static inline void mcheck_cpu_init(struct cpuinfo_x86 *c) {}
 static inline void mcheck_cpu_clear(struct cpuinfo_x86 *c) {}
 static inline int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 34a054181c4d..8bbfde05f04f 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1784,6 +1784,7 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 		setup_clear_cpu_cap(X86_FEATURE_LA57);
 
 	detect_nopl();
+	mca_bsp_init(c);
 }
 
 void __init init_cpu_devs(void)
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index aa13c9304ad8..3c6c19eb0a18 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -653,9 +653,6 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 	u32 low = 0, high = 0, address = 0;
 	int offset = -1;
 
-	mce_flags.overflow_recov = cpu_feature_enabled(X86_FEATURE_OVERFLOW_RECOV);
-	mce_flags.succor	 = cpu_feature_enabled(X86_FEATURE_SUCCOR);
-	mce_flags.smca		 = cpu_feature_enabled(X86_FEATURE_SMCA);
 	mce_flags.amd_threshold	 = 1;
 
 	for (bank = 0; bank < this_cpu_read(mce_num_banks); ++bank) {
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 9e31834b3542..79f3dd7f7851 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1837,13 +1837,6 @@ static void __mcheck_cpu_cap_init(void)
 	this_cpu_write(mce_num_banks, b);
 
 	__mcheck_cpu_mce_banks_init();
-
-	/* Use accurate RIP reporting if available. */
-	if ((cap & MCG_EXT_P) && MCG_EXT_CNT(cap) >= 9)
-		mca_cfg.rip_msr = MSR_IA32_MCG_EIP;
-
-	if (cap & MCG_SER_P)
-		mca_cfg.ser = 1;
 }
 
 static void __mcheck_cpu_init_generic(void)
@@ -2240,6 +2233,27 @@ DEFINE_IDTENTRY_RAW(exc_machine_check)
 }
 #endif
 
+void mca_bsp_init(struct cpuinfo_x86 *c)
+{
+	u64 cap;
+
+	if (!mce_available(c))
+		return;
+
+	mce_flags.overflow_recov = cpu_feature_enabled(X86_FEATURE_OVERFLOW_RECOV);
+	mce_flags.succor	 = cpu_feature_enabled(X86_FEATURE_SUCCOR);
+	mce_flags.smca		 = cpu_feature_enabled(X86_FEATURE_SMCA);
+
+	rdmsrq(MSR_IA32_MCG_CAP, cap);
+
+	/* Use accurate RIP reporting if available. */
+	if ((cap & MCG_EXT_P) && MCG_EXT_CNT(cap) >= 9)
+		mca_cfg.rip_msr = MSR_IA32_MCG_EIP;
+
+	if (cap & MCG_SER_P)
+		mca_cfg.ser = 1;
+}
+
 /*
  * Called for each booted CPU to set up machine checks.
  * Must be called with preempt off:

-- 
2.51.0
Re: [PATCH v6 02/15] x86/mce: Define BSP-only init
Posted by Nikolay Borisov 5 months ago

On 9/8/25 18:40, Yazen Ghannam wrote:
> Currently, MCA initialization is executed identically on each CPU as
> they are brought online. However, a number of MCA initialization tasks
> only need to be done once.
> 
> Define a function to collect all 'global' init tasks and call this from
> the BSP only. Start with CPU features.
> 
> Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Tested-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>

<snip>

> @@ -2240,6 +2233,27 @@ DEFINE_IDTENTRY_RAW(exc_machine_check)
>   }
>   #endif
>   
> +void mca_bsp_init(struct cpuinfo_x86 *c)
> +{
> +	u64 cap;
> +
> +	if (!mce_available(c))
> +		return;
> +
> +	mce_flags.overflow_recov = cpu_feature_enabled(X86_FEATURE_OVERFLOW_RECOV);
> +	mce_flags.succor	 = cpu_feature_enabled(X86_FEATURE_SUCCOR);
> +	mce_flags.smca		 = cpu_feature_enabled(X86_FEATURE_SMCA);
> +
> +	rdmsrq(MSR_IA32_MCG_CAP, cap);
> +
> +	/* Use accurate RIP reporting if available. */
> +	if ((cap & MCG_EXT_P) && MCG_EXT_CNT(cap) >= 9)
> +		mca_cfg.rip_msr = MSR_IA32_MCG_EIP;
> +
> +	if (cap & MCG_SER_P)
> +		mca_cfg.ser = 1;
> +}
> +


LGTM

Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>

nit: One question though for those CPUs which consist of P+E cores, is 
it mandated that both types of cores will have identical MCE 
architecture, I assume the x86 world will be a lot more unified than 
Arm's big.LITTLE ?

>   /*
>    * Called for each booted CPU to set up machine checks.
>    * Must be called with preempt off:
>
RE: [PATCH v6 02/15] x86/mce: Define BSP-only init
Posted by Luck, Tony 5 months ago
> nit: One question though for those CPUs which consist of P+E cores, is 
> it mandated that both types of cores will have identical MCE 
> architecture, I assume the x86 world will be a lot more unified than 
> Arm's big.LITTLE ?

Intel P and E cores have the same architectural MCE behavior (though the
model specific bits like IA32_MCi_MISC and IA32_STATUS.MSCOD could
be different on cores in the same hybrid part).

-Tony
Re: [PATCH v6 02/15] x86/mce: Define BSP-only init
Posted by Yazen Ghannam 5 months ago
On Wed, Sep 10, 2025 at 02:47:16PM +0300, Nikolay Borisov wrote:
> 
> 
> On 9/8/25 18:40, Yazen Ghannam wrote:
> > Currently, MCA initialization is executed identically on each CPU as
> > they are brought online. However, a number of MCA initialization tasks
> > only need to be done once.
> > 
> > Define a function to collect all 'global' init tasks and call this from
> > the BSP only. Start with CPU features.
> > 
> > Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > Tested-by: Tony Luck <tony.luck@intel.com>
> > Reviewed-by: Tony Luck <tony.luck@intel.com>
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> <snip>
> 
> > @@ -2240,6 +2233,27 @@ DEFINE_IDTENTRY_RAW(exc_machine_check)
> >   }
> >   #endif
> > +void mca_bsp_init(struct cpuinfo_x86 *c)
> > +{
> > +	u64 cap;
> > +
> > +	if (!mce_available(c))
> > +		return;
> > +
> > +	mce_flags.overflow_recov = cpu_feature_enabled(X86_FEATURE_OVERFLOW_RECOV);
> > +	mce_flags.succor	 = cpu_feature_enabled(X86_FEATURE_SUCCOR);
> > +	mce_flags.smca		 = cpu_feature_enabled(X86_FEATURE_SMCA);
> > +
> > +	rdmsrq(MSR_IA32_MCG_CAP, cap);
> > +
> > +	/* Use accurate RIP reporting if available. */
> > +	if ((cap & MCG_EXT_P) && MCG_EXT_CNT(cap) >= 9)
> > +		mca_cfg.rip_msr = MSR_IA32_MCG_EIP;
> > +
> > +	if (cap & MCG_SER_P)
> > +		mca_cfg.ser = 1;
> > +}
> > +
> 
> 
> LGTM
> 
> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>

Thanks Nikokay for the reviews.

> 
> nit: One question though for those CPUs which consist of P+E cores, is it
> mandated that both types of cores will have identical MCE architecture, I
> assume the x86 world will be a lot more unified than Arm's big.LITTLE ?
> 

I think technically no, they won't be mandated to be identical. We already
have per-CPU feature bits, registers, etc. And in Scalable MCA there are
also per-bank config bits.

However, this doesn't mean we will have different MCE features between
CPUs in a system just yet. The architects do try to make things flexible
and scalable just in case there is a need in the future.

We can code to the architectures and be *mostly* future-proof. But it's
not guaranteed to be without bugs. And it's not guaranteed that every
possible case will be used. 

Do you have any concerns or see any issues with the code today in this
area? Maybe you're thinking we should have per-CPU config for feature
bits? If so, I agree but we don't have a real need today AFAIK.

Maybe others can comment too.

Thanks,
Yazen
Re: [PATCH v6 02/15] x86/mce: Define BSP-only init
Posted by Nikolay Borisov 5 months ago

On 10.09.25 г. 16:53 ч., Yazen Ghannam wrote:
> On Wed, Sep 10, 2025 at 02:47:16PM +0300, Nikolay Borisov wrote:
>>
>>
>> On 9/8/25 18:40, Yazen Ghannam wrote:
>>> Currently, MCA initialization is executed identically on each CPU as
>>> they are brought online. However, a number of MCA initialization tasks
>>> only need to be done once.
>>>
>>> Define a function to collect all 'global' init tasks and call this from
>>> the BSP only. Start with CPU features.
>>>
>>> Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>> Tested-by: Tony Luck <tony.luck@intel.com>
>>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>>> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
>>
>> <snip>
>>
>>> @@ -2240,6 +2233,27 @@ DEFINE_IDTENTRY_RAW(exc_machine_check)
>>>    }
>>>    #endif
>>> +void mca_bsp_init(struct cpuinfo_x86 *c)
>>> +{
>>> +	u64 cap;
>>> +
>>> +	if (!mce_available(c))
>>> +		return;
>>> +
>>> +	mce_flags.overflow_recov = cpu_feature_enabled(X86_FEATURE_OVERFLOW_RECOV);
>>> +	mce_flags.succor	 = cpu_feature_enabled(X86_FEATURE_SUCCOR);
>>> +	mce_flags.smca		 = cpu_feature_enabled(X86_FEATURE_SMCA);
>>> +
>>> +	rdmsrq(MSR_IA32_MCG_CAP, cap);
>>> +
>>> +	/* Use accurate RIP reporting if available. */
>>> +	if ((cap & MCG_EXT_P) && MCG_EXT_CNT(cap) >= 9)
>>> +		mca_cfg.rip_msr = MSR_IA32_MCG_EIP;
>>> +
>>> +	if (cap & MCG_SER_P)
>>> +		mca_cfg.ser = 1;
>>> +}
>>> +
>>
>>
>> LGTM
>>
>> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
> 
> Thanks Nikokay for the reviews.
> 
>>
>> nit: One question though for those CPUs which consist of P+E cores, is it
>> mandated that both types of cores will have identical MCE architecture, I
>> assume the x86 world will be a lot more unified than Arm's big.LITTLE ?
>>
> 
> I think technically no, they won't be mandated to be identical. We already
> have per-CPU feature bits, registers, etc. And in Scalable MCA there are
> also per-bank config bits.
> 
> However, this doesn't mean we will have different MCE features between
> CPUs in a system just yet. The architects do try to make things flexible
> and scalable just in case there is a need in the future.
> 
> We can code to the architectures and be *mostly* future-proof. But it's
> not guaranteed to be without bugs. And it's not guaranteed that every
> possible case will be used.
> 
> Do you have any concerns or see any issues with the code today in this
> area? Maybe you're thinking we should have per-CPU config for feature
> bits? If so, I agree but we don't have a real need today AFAIK.

Yes, for example in your current code you assume that whatever the value 
of MSR_IA32_MCG_CAP w.r.t MCG_EXT_P or MCG_SER_P it will be identical 
for all CPUs. That might be fine, but what if, in the future, we end up 
with machines where MCG_CAP will have different values for different 
CPU's/cores.

Or that in a system we can end up with cpu's which have different 
X86_FEATURE_SMCA value.

OTOH, you are right that when the time comes we can worry about this 
since there can possibly be infinite number of possible scenarios.

> 
> Maybe others can comment too.
> 
> Thanks,
> Yazen