[PATCH v2 00/16] AMD MCA interrupts rework

[PATCH v2 00/16] AMD MCA interrupts rework

[Yazen Ghannam]

Hi all,

This set unifies the AMD MCA interrupt handlers with common MCA code.
The goal is to avoid duplicating functionality like reading and clearing
MCA banks.

Based on feedback, this revision also include changes to the MCA init
flow.

Patches 1-4:
General fixes and cleanups.

Patches 5-10:
Add BSP-only init flow and related changes.

Patches 11-15:
Updates from v1 set.

Patch 16:
Interrupt storm handling rebased on current set.

Thanks,
Yazen

---
Changes in v2:
- Add general cleanup pre-patches.
- Add changes for BSP-only init.
- Add interrupt storm handling for AMD.
- Link to v1: https://lore.kernel.org/r/20240523155641.2805411-1-yazen.ghannam@amd.com

---
Borislav Petkov (1):
      x86/mce: Cleanup bank processing on init

Smita Koralahalli (1):
      x86/mce: Handle AMD threshold interrupt storms

Yazen Ghannam (14):
      x86/mce: Don't remove sysfs if thresholding sysfs init fails
      x86/mce/amd: Remove return value for mce_threshold_create_device()
      x86/mce/amd: Remove smca_banks_map
      x86/mce/amd: Put list_head in threshold_bank
      x86/mce: Remove __mcheck_cpu_init_early()
      x86/mce: Define BSP-only init
      x86/mce: Define BSP-only SMCA init
      x86/mce: Do 'UNKNOWN' vendor check early
      x86/mce: Separate global and per-CPU quirks
      x86/mce: Move machine_check_poll() status checks to helper functions
      x86/mce: Unify AMD THR handler with MCA Polling
      x86/mce: Unify AMD DFR handler with MCA Polling
      x86/mce/amd: Enable interrupt vectors once per-CPU on SMCA systems
      x86/mce/amd: Support SMCA Corrected Error Interrupt

 arch/x86/include/asm/mce.h          |   7 +-
 arch/x86/kernel/cpu/common.c        |   1 +
 arch/x86/kernel/cpu/mce/amd.c       | 391 +++++++++++++-----------------------
 arch/x86/kernel/cpu/mce/core.c      | 322 ++++++++++++++---------------
 arch/x86/kernel/cpu/mce/intel.c     |  15 ++
 arch/x86/kernel/cpu/mce/internal.h  |   8 +
 arch/x86/kernel/cpu/mce/threshold.c |   3 +
 7 files changed, 332 insertions(+), 415 deletions(-)
---
base-commit: b36de8b904b8ff2095ece7af6b3cfff8c73c2fb1
change-id: 20250210-wip-mca-updates-bed2a67c9c57

[PATCH v2 01/16] x86/mce: Don't remove sysfs if thresholding sysfs init fails

[Yazen Ghannam]

Currently, the MCE subsystem sysfs interface will be removed if the
thresholding sysfs interface fails to be created. A common failure is
due to new MCA bank types that are not recognized and don't have a short
name set.

The MCA thresholding feature is optional and should not break the common
MCE sysfs interface. Also, new MCA bank types are occasionally
introduced, and updates will be needed to recognize them. But likewise,
this should not break the common sysfs interface.

Keep the MCE sysfs interface regardless of the status of the
thresholding sysfs interface.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: stable@vger.kernel.org
---

Notes:
    v1->v2:
    * New in v2.
    * Included stable tag but there's no specific commit for Fixes.

 arch/x86/kernel/cpu/mce/core.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 0dc00c9894c7..d39af20154c7 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -2801,15 +2801,9 @@ static int mce_cpu_dead(unsigned int cpu)
 static int mce_cpu_online(unsigned int cpu)
 {
 	struct timer_list *t = this_cpu_ptr(&mce_timer);
-	int ret;
 
 	mce_device_create(cpu);
-
-	ret = mce_threshold_create_device(cpu);
-	if (ret) {
-		mce_device_remove(cpu);
-		return ret;
-	}
+	mce_threshold_create_device(cpu);
 	mce_reenable_cpu();
 	mce_start_timer(t);
 	return 0;

-- 
2.43.0

RE: [PATCH v2 01/16] x86/mce: Don't remove sysfs if thresholding sysfs init fails

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Sent: Friday, February 14, 2025 12:46 AM
> To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> <yazen.ghannam@amd.com>; stable@vger.kernel.org
> Subject: [PATCH v2 01/16] x86/mce: Don't remove sysfs if thresholding sysfs
> init fails
> 
> Currently, the MCE subsystem sysfs interface will be removed if the
> thresholding sysfs interface fails to be created. A common failure is due to
> new MCA bank types that are not recognized and don't have a short name
> set.
> 
> The MCA thresholding feature is optional and should not break the common
> MCE sysfs interface. Also, new MCA bank types are occasionally introduced,
> and updates will be needed to recognize them. But likewise, this should not
> break the common sysfs interface.
> 
> Keep the MCE sysfs interface regardless of the status of the thresholding sysfs
> interface.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Cc: stable@vger.kernel.org

LGTM.
    Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

[PATCH v2 02/16] x86/mce/amd: Remove return value for mce_threshold_create_device()

[Yazen Ghannam]

The return value is no longer checked, so set return type to 'void'.

Also, move function declarations to internal.h, since this function is
only used within the MCE subsystem.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    v1->v2:
    * New in v2.

 arch/x86/include/asm/mce.h         |  2 --
 arch/x86/kernel/cpu/mce/amd.c      | 16 +++++++---------
 arch/x86/kernel/cpu/mce/internal.h |  2 ++
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index eb2db07ef39c..2701aca04aec 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -373,14 +373,12 @@ enum smca_bank_types {
 
 extern bool amd_mce_is_memory_error(struct mce *m);
 
-extern int mce_threshold_create_device(unsigned int cpu);
 extern int mce_threshold_remove_device(unsigned int cpu);
 
 void mce_amd_feature_init(struct cpuinfo_x86 *c);
 enum smca_bank_types smca_get_bank_type(unsigned int cpu, unsigned int bank);
 #else
 
-static inline int mce_threshold_create_device(unsigned int cpu)		{ return 0; };
 static inline int mce_threshold_remove_device(unsigned int cpu)		{ return 0; };
 static inline bool amd_mce_is_memory_error(struct mce *m)		{ return false; };
 static inline void mce_amd_feature_init(struct cpuinfo_x86 *c)		{ }
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 1075a90141da..4ea691006c3b 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1318,36 +1318,34 @@ int mce_threshold_remove_device(unsigned int cpu)
  * thread running on @cpu.  The callback is invoked on all CPUs which are
  * online when the callback is installed or during a real hotplug event.
  */
-int mce_threshold_create_device(unsigned int cpu)
+void mce_threshold_create_device(unsigned int cpu)
 {
 	unsigned int numbanks, bank;
 	struct threshold_bank **bp;
-	int err;
 
 	if (!mce_flags.amd_threshold)
-		return 0;
+		return;
 
 	bp = this_cpu_read(threshold_banks);
 	if (bp)
-		return 0;
+		return;
 
 	numbanks = this_cpu_read(mce_num_banks);
 	bp = kcalloc(numbanks, sizeof(*bp), GFP_KERNEL);
 	if (!bp)
-		return -ENOMEM;
+		return;
 
 	for (bank = 0; bank < numbanks; ++bank) {
 		if (!(this_cpu_read(bank_map) & BIT_ULL(bank)))
 			continue;
-		err = threshold_create_bank(bp, cpu, bank);
-		if (err) {
+		if (threshold_create_bank(bp, cpu, bank)) {
 			__threshold_remove_device(bp);
-			return err;
+			return;
 		}
 	}
 	this_cpu_write(threshold_banks, bp);
 
 	if (thresholding_irq_en)
 		mce_threshold_vector = amd_threshold_interrupt;
-	return 0;
+	return;
 }
diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
index 95a504ece43e..231ba8ca4a3e 100644
--- a/arch/x86/kernel/cpu/mce/internal.h
+++ b/arch/x86/kernel/cpu/mce/internal.h
@@ -265,6 +265,7 @@ void mce_prep_record_common(struct mce *m);
 void mce_prep_record_per_cpu(unsigned int cpu, struct mce *m);
 
 #ifdef CONFIG_X86_MCE_AMD
+void mce_threshold_create_device(unsigned int cpu);
 extern bool amd_filter_mce(struct mce *m);
 bool amd_mce_usable_address(struct mce *m);
 
@@ -293,6 +294,7 @@ static __always_inline void smca_extract_err_addr(struct mce *m)
 }
 
 #else
+static inline void mce_threshold_create_device(unsigned int cpu)	{ }
 static inline bool amd_filter_mce(struct mce *m) { return false; }
 static inline bool amd_mce_usable_address(struct mce *m) { return false; }
 static inline void smca_extract_err_addr(struct mce *m) { }

-- 
2.43.0

RE: [PATCH v2 02/16] x86/mce/amd: Remove return value for mce_threshold_create_device()

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Sent: Friday, February 14, 2025 12:46 AM
> To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> <yazen.ghannam@amd.com>
> Subject: [PATCH v2 02/16] x86/mce/amd: Remove return value for
> mce_threshold_create_device()
> 
> The return value is no longer checked, so set return type to 'void'.
>

This change can also be applied to mce_threshold_remove_device().

> Also, move function declarations to internal.h, since this function is only used
> within the MCE subsystem.

This change can also be applied to mce_threshold_remove_device().

So, can we also apply the two changes above for mce_threshold_remove_device() 
in this patch?

-Qiuxu

Re: [PATCH v2 02/16] x86/mce/amd: Remove return value for mce_threshold_create_device()

[Yazen Ghannam]

On Mon, Feb 17, 2025 at 07:11:52AM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > Sent: Friday, February 14, 2025 12:46 AM
> > To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> > Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> > Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> > <yazen.ghannam@amd.com>
> > Subject: [PATCH v2 02/16] x86/mce/amd: Remove return value for
> > mce_threshold_create_device()
> > 
> > The return value is no longer checked, so set return type to 'void'.
> >
> 
> This change can also be applied to mce_threshold_remove_device().
> 
> > Also, move function declarations to internal.h, since this function is only used
> > within the MCE subsystem.
> 
> This change can also be applied to mce_threshold_remove_device().
> 
> So, can we also apply the two changes above for mce_threshold_remove_device() 
> in this patch?
> 

Yes, good point. I can include these updates.

Thanks,
Yazen

[PATCH v2 03/16] x86/mce/amd: Remove smca_banks_map

[Yazen Ghannam]

The MCx_MISC0[BlkPtr] field was used on legacy systems to hold a
register offset for the next MCx_MISC* register. In this way, an
implementation-specific number of registers can be discovered at
runtime.

The MCAX/SMCA register space simplifies this by always including
the MCx_MISC[1-4] registers. The MCx_MISC0[BlkPtr] field is used to
indicate (true/false) whether any MCx_MISC[1-4] registers are present.
But it does not indicate which ones nor how many. Therefore, all the
registers are accessed and their bits are checked.

AMD systems generally enforce a Read-as-Zero/Writes-Ignored policy for
unused registers. Therefore, there is no harm to read an unused
register. This is already done in practice for most of the MCx_MISC
registers.

Remove the smca_banks_map variable as it is effectively redundant.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    v1->v2:
    * New in v2.

 arch/x86/kernel/cpu/mce/amd.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 4ea691006c3b..f8ad40c5c887 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -252,9 +252,6 @@ static DEFINE_PER_CPU(struct threshold_bank **, threshold_banks);
  */
 static DEFINE_PER_CPU(u64, bank_map);
 
-/* Map of banks that have more than MCA_MISC0 available. */
-static DEFINE_PER_CPU(u64, smca_misc_banks_map);
-
 static void amd_threshold_interrupt(void);
 static void amd_deferred_error_interrupt(void);
 
@@ -264,28 +261,6 @@ static void default_deferred_error_interrupt(void)
 }
 void (*deferred_error_int_vector)(void) = default_deferred_error_interrupt;
 
-static void smca_set_misc_banks_map(unsigned int bank, unsigned int cpu)
-{
-	u32 low, high;
-
-	/*
-	 * For SMCA enabled processors, BLKPTR field of the first MISC register
-	 * (MCx_MISC0) indicates presence of additional MISC regs set (MISC1-4).
-	 */
-	if (rdmsr_safe(MSR_AMD64_SMCA_MCx_CONFIG(bank), &low, &high))
-		return;
-
-	if (!(low & MCI_CONFIG_MCAX))
-		return;
-
-	if (rdmsr_safe(MSR_AMD64_SMCA_MCx_MISC(bank), &low, &high))
-		return;
-
-	if (low & MASK_BLKPTR_LO)
-		per_cpu(smca_misc_banks_map, cpu) |= BIT_ULL(bank);
-
-}
-
 static void smca_configure(unsigned int bank, unsigned int cpu)
 {
 	u8 *bank_counts = this_cpu_ptr(smca_bank_counts);
@@ -326,8 +301,6 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
 		wrmsr(smca_config, low, high);
 	}
 
-	smca_set_misc_banks_map(bank, cpu);
-
 	if (rdmsr_safe(MSR_AMD64_SMCA_MCx_IPID(bank), &low, &high)) {
 		pr_warn("Failed to read MCA_IPID for bank %d\n", bank);
 		return;
@@ -532,9 +505,6 @@ static u32 smca_get_block_address(unsigned int bank, unsigned int block,
 	if (!block)
 		return MSR_AMD64_SMCA_MCx_MISC(bank);
 
-	if (!(per_cpu(smca_misc_banks_map, cpu) & BIT_ULL(bank)))
-		return 0;
-
 	return MSR_AMD64_SMCA_MCx_MISCy(bank, block - 1);
 }
 

-- 
2.43.0

RE: [PATCH v2 03/16] x86/mce/amd: Remove smca_banks_map

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Sent: Friday, February 14, 2025 12:46 AM
> To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> <yazen.ghannam@amd.com>
> Subject: [PATCH v2 03/16] x86/mce/amd: Remove smca_banks_map
> 
> The MCx_MISC0[BlkPtr] field was used on legacy systems to hold a register
> offset for the next MCx_MISC* register. In this way, an implementation-
> specific number of registers can be discovered at runtime.
> 
> The MCAX/SMCA register space simplifies this by always including the
> MCx_MISC[1-4] registers. The MCx_MISC0[BlkPtr] field is used to indicate
> (true/false) whether any MCx_MISC[1-4] registers are present.
> But it does not indicate which ones nor how many. Therefore, all the registers

s/nor/or     (suggested by AI 😊)
OR
s/does not indicate/indicates neither

> are accessed and their bits are checked.
> 
> AMD systems generally enforce a Read-as-Zero/Writes-Ignored policy for
> unused registers. Therefore, there is no harm to read an unused register. This
> is already done in practice for most of the MCx_MISC registers.
> 
> Remove the smca_banks_map variable as it is effectively redundant.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>

Aside from the small nit above,

    Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> 

Re: [PATCH v2 03/16] x86/mce/amd: Remove smca_banks_map

[Yazen Ghannam]

On Mon, Feb 17, 2025 at 07:57:47AM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > Sent: Friday, February 14, 2025 12:46 AM
> > To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> > Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> > Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> > <yazen.ghannam@amd.com>
> > Subject: [PATCH v2 03/16] x86/mce/amd: Remove smca_banks_map
> > 
> > The MCx_MISC0[BlkPtr] field was used on legacy systems to hold a register
> > offset for the next MCx_MISC* register. In this way, an implementation-
> > specific number of registers can be discovered at runtime.
> > 
> > The MCAX/SMCA register space simplifies this by always including the
> > MCx_MISC[1-4] registers. The MCx_MISC0[BlkPtr] field is used to indicate
> > (true/false) whether any MCx_MISC[1-4] registers are present.
> > But it does not indicate which ones nor how many. Therefore, all the registers
> 
> s/nor/or     (suggested by AI 😊)
> OR
> s/does not indicate/indicates neither
> 

Hmm, okay, I think the second one.

  "But it indicates neither which ones nor how many".

I guess this falls under "Using 'nor' without a correlative pair".
https://www.learngrammar.net/a/how-to-use-nor-properly-in-a-sentence

:)

> > are accessed and their bits are checked.
> > 
> > AMD systems generally enforce a Read-as-Zero/Writes-Ignored policy for
> > unused registers. Therefore, there is no harm to read an unused register. This
> > is already done in practice for most of the MCx_MISC registers.
> > 
> > Remove the smca_banks_map variable as it is effectively redundant.
> > 
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> Aside from the small nit above,
> 
>     Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> 

Thanks,
Yazen

[PATCH v2 04/16] x86/mce/amd: Put list_head in threshold_bank

[Yazen Ghannam]

The threshold_bank structure is a container for one or more
threshold_block structures. Currently, the container has a single
pointer to the 'first' threshold_block structure which then has a linked
list of the remaining threshold_block structures.

This results in an extra level of indirection where the 'first' block is
checked before iterating over the remaining blocks.

Remove the indirection by including the head of the block list in the
threshold_bank structure which already acts as a container for all the
bank's thresholding blocks.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    v1->v2:
    * New in v2.

 arch/x86/kernel/cpu/mce/amd.c | 43 ++++++++++++-------------------------------
 1 file changed, 12 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index f8ad40c5c887..df8984aac1c6 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -241,7 +241,8 @@ struct threshold_block {
 
 struct threshold_bank {
 	struct kobject		*kobj;
-	struct threshold_block	*blocks;
+	/* List of threshold blocks within this MCA bank. */
+	struct list_head	miscj;
 };
 
 static DEFINE_PER_CPU(struct threshold_bank **, threshold_banks);
@@ -901,9 +902,9 @@ static void log_and_reset_block(struct threshold_block *block)
  */
 static void amd_threshold_interrupt(void)
 {
-	struct threshold_block *first_block = NULL, *block = NULL, *tmp = NULL;
-	struct threshold_bank **bp = this_cpu_read(threshold_banks);
+	struct threshold_bank **bp = this_cpu_read(threshold_banks), *thr_bank;
 	unsigned int bank, cpu = smp_processor_id();
+	struct threshold_block *block, *tmp;
 
 	/*
 	 * Validate that the threshold bank has been initialized already. The
@@ -917,16 +918,11 @@ static void amd_threshold_interrupt(void)
 		if (!(per_cpu(bank_map, cpu) & BIT_ULL(bank)))
 			continue;
 
-		first_block = bp[bank]->blocks;
-		if (!first_block)
+		thr_bank = bp[bank];
+		if (!thr_bank)
 			continue;
 
-		/*
-		 * The first block is also the head of the list. Check it first
-		 * before iterating over the rest.
-		 */
-		log_and_reset_block(first_block);
-		list_for_each_entry_safe(block, tmp, &first_block->miscj, miscj)
+		list_for_each_entry_safe(block, tmp, &thr_bank->miscj, miscj)
 			log_and_reset_block(block);
 	}
 }
@@ -1145,13 +1141,7 @@ static int allocate_threshold_blocks(unsigned int cpu, struct threshold_bank *tb
 		default_attrs[2] = NULL;
 	}
 
-	INIT_LIST_HEAD(&b->miscj);
-
-	/* This is safe as @tb is not visible yet */
-	if (tb->blocks)
-		list_add(&b->miscj, &tb->blocks->miscj);
-	else
-		tb->blocks = b;
+	list_add(&b->miscj, &tb->miscj);
 
 	err = kobject_init_and_add(&b->kobj, &threshold_ktype, tb->kobj, get_name(cpu, bank, b));
 	if (err)
@@ -1202,6 +1192,8 @@ static int threshold_create_bank(struct threshold_bank **bp, unsigned int cpu,
 		goto out_free;
 	}
 
+	INIT_LIST_HEAD(&b->miscj);
+
 	err = allocate_threshold_blocks(cpu, b, bank, 0, mca_msr_reg(bank, MCA_MISC));
 	if (err)
 		goto out_kobj;
@@ -1222,26 +1214,15 @@ static void threshold_block_release(struct kobject *kobj)
 	kfree(to_block(kobj));
 }
 
-static void deallocate_threshold_blocks(struct threshold_bank *bank)
+static void threshold_remove_bank(struct threshold_bank *bank)
 {
 	struct threshold_block *pos, *tmp;
 
-	list_for_each_entry_safe(pos, tmp, &bank->blocks->miscj, miscj) {
+	list_for_each_entry_safe(pos, tmp, &bank->miscj, miscj) {
 		list_del(&pos->miscj);
 		kobject_put(&pos->kobj);
 	}
 
-	kobject_put(&bank->blocks->kobj);
-}
-
-static void threshold_remove_bank(struct threshold_bank *bank)
-{
-	if (!bank->blocks)
-		goto out_free;
-
-	deallocate_threshold_blocks(bank);
-
-out_free:
 	kobject_put(bank->kobj);
 	kfree(bank);
 }

-- 
2.43.0

RE: [PATCH v2 04/16] x86/mce/amd: Put list_head in threshold_bank

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Sent: Friday, February 14, 2025 12:46 AM
> To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> <yazen.ghannam@amd.com>
> Subject: [PATCH v2 04/16] x86/mce/amd: Put list_head in threshold_bank
> 
> The threshold_bank structure is a container for one or more threshold_block
> structures. Currently, the container has a single pointer to the 'first'
> threshold_block structure which then has a linked list of the remaining
> threshold_block structures.
> 
> This results in an extra level of indirection where the 'first' block is checked
> before iterating over the remaining blocks.
> 
> Remove the indirection by including the head of the block list in the
> threshold_bank structure which already acts as a container for all the bank's
> thresholding blocks.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>

Nice cleanup.

    Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

[PATCH v2 05/16] x86/mce: Cleanup bank processing on init

[Yazen Ghannam]

From: Borislav Petkov <bp@suse.de>

Unify the bank preparation into __mcheck_cpu_init_clear_banks(), rename
that function to what it does now - prepares banks. Do this so that
generic and vendor banks init goes first so that settings done during
that init can take effect before the first bank polling takes place.

Move __mcheck_cpu_check_banks() into __mcheck_cpu_init_prepare_banks()
as it already loops over the banks.

Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    Link:
    https://lore.kernel.org/r/20221206173607.1185907-2-yazen.ghannam@amd.com
    
    v1->v2:
    * New in v2, but based on old patch (see link).
    * Kept old tags for reference.

 arch/x86/include/asm/mce.h     |  3 +-
 arch/x86/kernel/cpu/mce/core.c | 63 ++++++++++++------------------------------
 2 files changed, 19 insertions(+), 47 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 2701aca04aec..36ff81c1b3b1 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -290,8 +290,7 @@ DECLARE_PER_CPU(mce_banks_t, mce_poll_banks);
 enum mcp_flags {
 	MCP_TIMESTAMP	= BIT(0),	/* log time stamp */
 	MCP_UC		= BIT(1),	/* log uncorrected errors */
-	MCP_DONTLOG	= BIT(2),	/* only clear, don't log */
-	MCP_QUEUE_LOG	= BIT(3),	/* only queue to genpool */
+	MCP_QUEUE_LOG	= BIT(2),	/* only queue to genpool */
 };
 
 void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index d39af20154c7..d85bd861ecca 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -785,9 +785,6 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		continue;
 
 log_it:
-		if (flags & MCP_DONTLOG)
-			goto clear_it;
-
 		mce_read_aux(&err, i);
 		m->severity = mce_severity(m, NULL, NULL, false);
 		/*
@@ -1807,7 +1804,7 @@ static void __mcheck_cpu_mce_banks_init(void)
 		/*
 		 * Init them all, __mcheck_cpu_apply_quirks() is going to apply
 		 * the required vendor quirks before
-		 * __mcheck_cpu_init_clear_banks() does the final bank setup.
+		 * __mcheck_cpu_init_prepare_banks() does the final bank setup.
 		 */
 		b->ctl = -1ULL;
 		b->init = true;
@@ -1846,21 +1843,8 @@ static void __mcheck_cpu_cap_init(void)
 
 static void __mcheck_cpu_init_generic(void)
 {
-	enum mcp_flags m_fl = 0;
-	mce_banks_t all_banks;
 	u64 cap;
 
-	if (!mca_cfg.bootlog)
-		m_fl = MCP_DONTLOG;
-
-	/*
-	 * Log the machine checks left over from the previous reset. Log them
-	 * only, do not start processing them. That will happen in mcheck_late_init()
-	 * when all consumers have been registered on the notifier chain.
-	 */
-	bitmap_fill(all_banks, MAX_NR_BANKS);
-	machine_check_poll(MCP_UC | MCP_QUEUE_LOG | m_fl, &all_banks);
-
 	cr4_set_bits(X86_CR4_MCE);
 
 	rdmsrl(MSR_IA32_MCG_CAP, cap);
@@ -1868,36 +1852,23 @@ static void __mcheck_cpu_init_generic(void)
 		wrmsr(MSR_IA32_MCG_CTL, 0xffffffff, 0xffffffff);
 }
 
-static void __mcheck_cpu_init_clear_banks(void)
+static void __mcheck_cpu_init_prepare_banks(void)
 {
 	struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
+	u64 msrval;
 	int i;
 
-	for (i = 0; i < this_cpu_read(mce_num_banks); i++) {
-		struct mce_bank *b = &mce_banks[i];
+	/*
+	 * Log the machine checks left over from the previous reset. Log them
+	 * only, do not start processing them. That will happen in mcheck_late_init()
+	 * when all consumers have been registered on the notifier chain.
+	 */
+	if (mca_cfg.bootlog) {
+		mce_banks_t all_banks;
 
-		if (!b->init)
-			continue;
-		wrmsrl(mca_msr_reg(i, MCA_CTL), b->ctl);
-		wrmsrl(mca_msr_reg(i, MCA_STATUS), 0);
+		bitmap_fill(all_banks, MAX_NR_BANKS);
+		machine_check_poll(MCP_UC | MCP_QUEUE_LOG, &all_banks);
 	}
-}
-
-/*
- * Do a final check to see if there are any unused/RAZ banks.
- *
- * This must be done after the banks have been initialized and any quirks have
- * been applied.
- *
- * Do not call this from any user-initiated flows, e.g. CPU hotplug or sysfs.
- * Otherwise, a user who disables a bank will not be able to re-enable it
- * without a system reboot.
- */
-static void __mcheck_cpu_check_banks(void)
-{
-	struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
-	u64 msrval;
-	int i;
 
 	for (i = 0; i < this_cpu_read(mce_num_banks); i++) {
 		struct mce_bank *b = &mce_banks[i];
@@ -1905,6 +1876,9 @@ static void __mcheck_cpu_check_banks(void)
 		if (!b->init)
 			continue;
 
+		wrmsrl(mca_msr_reg(i, MCA_CTL), b->ctl);
+		wrmsrl(mca_msr_reg(i, MCA_STATUS), 0);
+
 		rdmsrl(mca_msr_reg(i, MCA_CTL), msrval);
 		b->init = !!msrval;
 	}
@@ -2310,8 +2284,7 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c)
 	__mcheck_cpu_init_early(c);
 	__mcheck_cpu_init_generic();
 	__mcheck_cpu_init_vendor(c);
-	__mcheck_cpu_init_clear_banks();
-	__mcheck_cpu_check_banks();
+	__mcheck_cpu_init_prepare_banks();
 	__mcheck_cpu_setup_timer();
 }
 
@@ -2479,7 +2452,7 @@ static void mce_syscore_resume(void)
 {
 	__mcheck_cpu_init_generic();
 	__mcheck_cpu_init_vendor(raw_cpu_ptr(&cpu_info));
-	__mcheck_cpu_init_clear_banks();
+	__mcheck_cpu_init_prepare_banks();
 }
 
 static struct syscore_ops mce_syscore_ops = {
@@ -2497,7 +2470,7 @@ static void mce_cpu_restart(void *data)
 	if (!mce_available(raw_cpu_ptr(&cpu_info)))
 		return;
 	__mcheck_cpu_init_generic();
-	__mcheck_cpu_init_clear_banks();
+	__mcheck_cpu_init_prepare_banks();
 	__mcheck_cpu_init_timer();
 }
 

-- 
2.43.0

Re: [PATCH v2 05/16] x86/mce: Cleanup bank processing on init

[Luck, Tony]

On Thu, Feb 13, 2025 at 04:45:54PM +0000, Yazen Ghannam wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> Unify the bank preparation into __mcheck_cpu_init_clear_banks(), rename
> that function to what it does now - prepares banks. Do this so that
> generic and vendor banks init goes first so that settings done during
> that init can take effect before the first bank polling takes place.
> 
> Move __mcheck_cpu_check_banks() into __mcheck_cpu_init_prepare_banks()
> as it already loops over the banks.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
> 
> Notes:
>     Link:
>     https://lore.kernel.org/r/20221206173607.1185907-2-yazen.ghannam@amd.com
>     
>     v1->v2:
>     * New in v2, but based on old patch (see link).
>     * Kept old tags for reference.
> 
>  arch/x86/include/asm/mce.h     |  3 +-
>  arch/x86/kernel/cpu/mce/core.c | 63 ++++++++++++------------------------------
>  2 files changed, 19 insertions(+), 47 deletions(-)
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 2701aca04aec..36ff81c1b3b1 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -290,8 +290,7 @@ DECLARE_PER_CPU(mce_banks_t, mce_poll_banks);
>  enum mcp_flags {
>  	MCP_TIMESTAMP	= BIT(0),	/* log time stamp */
>  	MCP_UC		= BIT(1),	/* log uncorrected errors */
> -	MCP_DONTLOG	= BIT(2),	/* only clear, don't log */

MCP_DONTLOG is removed in this patch. But no mention of this change in
the commit description.

-Tony

Re: [PATCH v2 05/16] x86/mce: Cleanup bank processing on init

[Yazen Ghannam]

On Thu, Feb 13, 2025 at 02:32:27PM -0800, Luck, Tony wrote:
> On Thu, Feb 13, 2025 at 04:45:54PM +0000, Yazen Ghannam wrote:
> > From: Borislav Petkov <bp@suse.de>
> > 
> > Unify the bank preparation into __mcheck_cpu_init_clear_banks(), rename
> > that function to what it does now - prepares banks. Do this so that
> > generic and vendor banks init goes first so that settings done during
> > that init can take effect before the first bank polling takes place.
> > 
> > Move __mcheck_cpu_check_banks() into __mcheck_cpu_init_prepare_banks()
> > as it already loops over the banks.
> > 
> > Signed-off-by: Borislav Petkov <bp@suse.de>
> > Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > ---
> > 
> > Notes:
> >     Link:
> >     https://lore.kernel.org/r/20221206173607.1185907-2-yazen.ghannam@amd.com
> >     
> >     v1->v2:
> >     * New in v2, but based on old patch (see link).
> >     * Kept old tags for reference.
> > 
> >  arch/x86/include/asm/mce.h     |  3 +-
> >  arch/x86/kernel/cpu/mce/core.c | 63 ++++++++++++------------------------------
> >  2 files changed, 19 insertions(+), 47 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> > index 2701aca04aec..36ff81c1b3b1 100644
> > --- a/arch/x86/include/asm/mce.h
> > +++ b/arch/x86/include/asm/mce.h
> > @@ -290,8 +290,7 @@ DECLARE_PER_CPU(mce_banks_t, mce_poll_banks);
> >  enum mcp_flags {
> >  	MCP_TIMESTAMP	= BIT(0),	/* log time stamp */
> >  	MCP_UC		= BIT(1),	/* log uncorrected errors */
> > -	MCP_DONTLOG	= BIT(2),	/* only clear, don't log */
> 
> MCP_DONTLOG is removed in this patch. But no mention of this change in
> the commit description.
> 

Yes, good point. How about this?

"The MCP_DONTLOG flag is no longer needed, since the MCA polling
function is now called only if boot-time logging should be done."

Thanks,
Yazen

RE: [PATCH v2 05/16] x86/mce: Cleanup bank processing on init

[Luck, Tony]

> > MCP_DONTLOG is removed in this patch. But no mention of this change in
> > the commit description.
> >
>
> Yes, good point. How about this?
>
> "The MCP_DONTLOG flag is no longer needed, since the MCA polling
> function is now called only if boot-time logging should be done."

Looks good.

-Tony

RE: [PATCH v2 05/16] x86/mce: Cleanup bank processing on init

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Sent: Friday, February 14, 2025 12:46 AM
> To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> <yazen.ghannam@amd.com>
> Subject: [PATCH v2 05/16] x86/mce: Cleanup bank processing on init
> 
> From: Borislav Petkov <bp@suse.de>
> 
> Unify the bank preparation into __mcheck_cpu_init_clear_banks(), rename
> that function to what it does now - prepares banks. Do this so that generic
> and vendor banks init goes first so that settings done during that init can take
> effect before the first bank polling takes place.
> 
> Move __mcheck_cpu_check_banks() into
> __mcheck_cpu_init_prepare_banks() as it already loops over the banks.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>

Other than Tony's comments about the missing comment for 
'MCP_DONTLOG',

    Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

 

[PATCH v2 06/16] x86/mce: Remove __mcheck_cpu_init_early()

[Yazen Ghannam]

The __mcheck_cpu_init_early() function was introduced so that some
vendor-specific features are detected before the first MCA polling event
done in __mcheck_cpu_init_generic().

Currently, __mcheck_cpu_init_early() is only used on AMD-based systems and
additional code will be needed to support various system configurations.

However, the current and future vendor-specific code should be done during
vendor init. This keeps all the vendor code in a common location and
simplifies the generic init flow.

Move all the __mcheck_cpu_init_early() code into mce_amd_feature_init().
Also, move __mcheck_cpu_init_generic() after
__mcheck_cpu_init_prepare_banks() so that MCA is enabled after the first
MCA polling event.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    Link:
    https://lore.kernel.org/r/20221206173607.1185907-3-yazen.ghannam@amd.com
    
    v1->v2:
    * New in v2, but based on old patch (see link).
    * Changed cpu_has() to cpu_feature_enabled().

 arch/x86/kernel/cpu/mce/amd.c  |  4 ++++
 arch/x86/kernel/cpu/mce/core.c | 20 +++-----------------
 2 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index df8984aac1c6..302a310d0630 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -656,6 +656,10 @@ 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) {
 		if (mce_flags.smca)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index d85bd861ecca..7fbf1c8291b8 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -2029,19 +2029,6 @@ static bool __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
 	return false;
 }
 
-/*
- * Init basic CPU features needed for early decoding of MCEs.
- */
-static void __mcheck_cpu_init_early(struct cpuinfo_x86 *c)
-{
-	if (c->x86_vendor == X86_VENDOR_AMD || c->x86_vendor == X86_VENDOR_HYGON) {
-		mce_flags.overflow_recov = !!cpu_has(c, X86_FEATURE_OVERFLOW_RECOV);
-		mce_flags.succor	 = !!cpu_has(c, X86_FEATURE_SUCCOR);
-		mce_flags.smca		 = !!cpu_has(c, X86_FEATURE_SMCA);
-		mce_flags.amd_threshold	 = 1;
-	}
-}
-
 static void mce_centaur_feature_init(struct cpuinfo_x86 *c)
 {
 	struct mca_config *cfg = &mca_cfg;
@@ -2281,10 +2268,9 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c)
 
 	mca_cfg.initialized = 1;
 
-	__mcheck_cpu_init_early(c);
-	__mcheck_cpu_init_generic();
 	__mcheck_cpu_init_vendor(c);
 	__mcheck_cpu_init_prepare_banks();
+	__mcheck_cpu_init_generic();
 	__mcheck_cpu_setup_timer();
 }
 
@@ -2450,9 +2436,9 @@ static void mce_syscore_shutdown(void)
  */
 static void mce_syscore_resume(void)
 {
-	__mcheck_cpu_init_generic();
 	__mcheck_cpu_init_vendor(raw_cpu_ptr(&cpu_info));
 	__mcheck_cpu_init_prepare_banks();
+	__mcheck_cpu_init_generic();
 }
 
 static struct syscore_ops mce_syscore_ops = {
@@ -2469,8 +2455,8 @@ static void mce_cpu_restart(void *data)
 {
 	if (!mce_available(raw_cpu_ptr(&cpu_info)))
 		return;
-	__mcheck_cpu_init_generic();
 	__mcheck_cpu_init_prepare_banks();
+	__mcheck_cpu_init_generic();
 	__mcheck_cpu_init_timer();
 }
 

-- 
2.43.0

RE: [PATCH v2 06/16] x86/mce: Remove __mcheck_cpu_init_early()

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Sent: Friday, February 14, 2025 12:46 AM
> To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> <yazen.ghannam@amd.com>
> Subject: [PATCH v2 06/16] x86/mce: Remove __mcheck_cpu_init_early()
> 
> The __mcheck_cpu_init_early() function was introduced so that some
> vendor-specific features are detected before the first MCA polling event done
> in __mcheck_cpu_init_generic().
> 
> Currently, __mcheck_cpu_init_early() is only used on AMD-based systems
> and additional code will be needed to support various system configurations.
> 
> However, the current and future vendor-specific code should be done during
> vendor init. This keeps all the vendor code in a common location and
> simplifies the generic init flow.
> 
> Move all the __mcheck_cpu_init_early() code into mce_amd_feature_init().
> Also, move __mcheck_cpu_init_generic() after
> __mcheck_cpu_init_prepare_banks() so that MCA is enabled after the first
> MCA polling event.

Maybe providing a bit more information about the first MCA polling event, 
as shown below, would be clearer:

   "... so that MCA is enabled after the first MCA polling event for the machine 
    checks left over from the previous reset."

> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>

Other than that,

    Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
  

Re: [PATCH v2 06/16] x86/mce: Remove __mcheck_cpu_init_early()

[Yazen Ghannam]

On Tue, Feb 18, 2025 at 03:00:12AM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > Sent: Friday, February 14, 2025 12:46 AM
> > To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> > Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> > Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> > <yazen.ghannam@amd.com>
> > Subject: [PATCH v2 06/16] x86/mce: Remove __mcheck_cpu_init_early()
> > 
> > The __mcheck_cpu_init_early() function was introduced so that some
> > vendor-specific features are detected before the first MCA polling event done
> > in __mcheck_cpu_init_generic().
> > 
> > Currently, __mcheck_cpu_init_early() is only used on AMD-based systems
> > and additional code will be needed to support various system configurations.
> > 
> > However, the current and future vendor-specific code should be done during
> > vendor init. This keeps all the vendor code in a common location and
> > simplifies the generic init flow.
> > 
> > Move all the __mcheck_cpu_init_early() code into mce_amd_feature_init().
> > Also, move __mcheck_cpu_init_generic() after
> > __mcheck_cpu_init_prepare_banks() so that MCA is enabled after the first
> > MCA polling event.
> 
> Maybe providing a bit more information about the first MCA polling event, 
> as shown below, would be clearer:
> 
>    "... so that MCA is enabled after the first MCA polling event for the machine 
>     checks left over from the previous reset."
> 

Sure thing.

> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> Other than that,
> 
>     Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>   

Thanks,
Yazen

Re: [PATCH v2 06/16] x86/mce: Remove __mcheck_cpu_init_early()

[Borislav Petkov]

On Thu, Feb 13, 2025 at 04:45:55PM +0000, Yazen Ghannam wrote:
> Also, move __mcheck_cpu_init_generic() after
> __mcheck_cpu_init_prepare_banks() so that MCA is enabled after the first
> MCA polling event.

The reason being?

Precaution?

It was this way since forever, why are you moving it now? Any particular
reason?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 06/16] x86/mce: Remove __mcheck_cpu_init_early()

[Yazen Ghannam]

On Thu, Feb 27, 2025 at 04:25:00PM +0100, Borislav Petkov wrote:
> On Thu, Feb 13, 2025 at 04:45:55PM +0000, Yazen Ghannam wrote:
> > Also, move __mcheck_cpu_init_generic() after
> > __mcheck_cpu_init_prepare_banks() so that MCA is enabled after the first
> > MCA polling event.
> 
> The reason being?
> 
> Precaution?
> 
> It was this way since forever, why are you moving it now? Any particular
> reason?
> 

1) To read/clear old errors before turning on MCA. The updated
__mcheck_cpu_init_prepare_banks() function does this for the MCi_CTL
registers. This patch does this for the MCG_CTL register too.

2) To ensure that vendor-specific setup is finished beforehand also.

Thanks,
Yazen

Re: [PATCH v2 06/16] x86/mce: Remove __mcheck_cpu_init_early()

[Borislav Petkov]

On February 27, 2025 5:31:48 PM GMT+01:00, Yazen Ghannam <yazen.ghannam@amd.com> wrote:
>On Thu, Feb 27, 2025 at 04:25:00PM +0100, Borislav Petkov wrote:
>> On Thu, Feb 13, 2025 at 04:45:55PM +0000, Yazen Ghannam wrote:
>> > Also, move __mcheck_cpu_init_generic() after
>> > __mcheck_cpu_init_prepare_banks() so that MCA is enabled after the first
>> > MCA polling event.
>> 
>> The reason being?
>> 
>> Precaution?
>> 
>> It was this way since forever, why are you moving it now? Any particular
>> reason?
>> 
>
>1) To read/clear old errors before turning on MCA. The updated
>__mcheck_cpu_init_prepare_banks() function does this for the MCi_CTL
>registers. This patch does this for the MCG_CTL register too.
>
>2) To ensure that vendor-specific setup is finished beforehand also.

That doesn't answer my question. All of the above gets done even without shuffling the order...


-- 
Sent from a small device: formatting sucks and brevity is inevitable.

Re: [PATCH v2 06/16] x86/mce: Remove __mcheck_cpu_init_early()

[Yazen Ghannam]

On Thu, Feb 27, 2025 at 08:33:19PM +0100, Borislav Petkov wrote:
> On February 27, 2025 5:31:48 PM GMT+01:00, Yazen Ghannam <yazen.ghannam@amd.com> wrote:
> >On Thu, Feb 27, 2025 at 04:25:00PM +0100, Borislav Petkov wrote:
> >> On Thu, Feb 13, 2025 at 04:45:55PM +0000, Yazen Ghannam wrote:
> >> > Also, move __mcheck_cpu_init_generic() after
> >> > __mcheck_cpu_init_prepare_banks() so that MCA is enabled after the first
> >> > MCA polling event.
> >> 
> >> The reason being?
> >> 
> >> Precaution?
> >> 
> >> It was this way since forever, why are you moving it now? Any particular
> >> reason?
> >> 
> >
> >1) To read/clear old errors before turning on MCA. The updated
> >__mcheck_cpu_init_prepare_banks() function does this for the MCi_CTL
> >registers. This patch does this for the MCG_CTL register too.
> >
> >2) To ensure that vendor-specific setup is finished beforehand also.
> 
> That doesn't answer my question. All of the above gets done even without shuffling the order...
> 
> 

MCA banks can start logging errors once MCG_CTL is set. The AMD docs say
"The operating system must initialize the MCA_CONFIG registers prior to
initialization of the MCA_CTL registers."

"The MCA_CTL registers must be initialized prior to enabling the error
reporting banks in MCG_CTL".

However, the Intel docs "Machine-Check Initialization Pseudocode" say
MCG_CTL first then MCi_CTL.

But both agree that CR4.MCE should be set last.

We have an old thread on the topic that led to this patch.
https://lore.kernel.org/all/YqJHwXkg3Ny9fI3s@yaz-fattaah/

And it seemed okay at the time.
https://lore.kernel.org/all/YrnTMmwl5TrHwT9J@zn.tnic/

I don't think anything much has changed since then, so I included the
old patch again in this set.

Thanks,
Yazen

Re: [PATCH v2 06/16] x86/mce: Remove __mcheck_cpu_init_early()

[Borislav Petkov]

On February 27, 2025 8:59:33 PM GMT+01:00, Yazen Ghannam <yazen.ghannam@amd.com> wrote:
>On Thu, Feb 27, 2025 at 08:33:19PM +0100, Borislav Petkov wrote:
>> On February 27, 2025 5:31:48 PM GMT+01:00, Yazen Ghannam <yazen.ghannam@amd.com> wrote:
>> >On Thu, Feb 27, 2025 at 04:25:00PM +0100, Borislav Petkov wrote:
>> >> On Thu, Feb 13, 2025 at 04:45:55PM +0000, Yazen Ghannam wrote:
>> >> > Also, move __mcheck_cpu_init_generic() after
>> >> > __mcheck_cpu_init_prepare_banks() so that MCA is enabled after the first
>> >> > MCA polling event.
>> >> 
>> >> The reason being?
>> >> 
>> >> Precaution?
>> >> 
>> >> It was this way since forever, why are you moving it now? Any particular
>> >> reason?
>> >> 
>> >
>> >1) To read/clear old errors before turning on MCA. The updated
>> >__mcheck_cpu_init_prepare_banks() function does this for the MCi_CTL
>> >registers. This patch does this for the MCG_CTL register too.
>> >
>> >2) To ensure that vendor-specific setup is finished beforehand also.
>> 
>> That doesn't answer my question. All of the above gets done even without shuffling the order...
>> 
>> 
>
>MCA banks can start logging errors once MCG_CTL is set. The AMD docs say
>"The operating system must initialize the MCA_CONFIG registers prior to
>initialization of the MCA_CTL registers."
>
>"The MCA_CTL registers must be initialized prior to enabling the error
>reporting banks in MCG_CTL".
>
>However, the Intel docs "Machine-Check Initialization Pseudocode" say
>MCG_CTL first then MCi_CTL.
>
>But both agree that CR4.MCE should be set last.
>
>We have an old thread on the topic that led to this patch.
>https://lore.kernel.org/all/YqJHwXkg3Ny9fI3s@yaz-fattaah/
>
>And it seemed okay at the time.
>https://lore.kernel.org/all/YrnTMmwl5TrHwT9J@zn.tnic/
>
>I don't think anything much has changed since then, so I included the
>old patch again in this set.
>
>Thanks,
>Yazen

This is exactly what needs to be in the commit message - why is the change being done. 
-- 
Sent from a small device: formatting sucks and brevity is inevitable.

Re: [PATCH v2 06/16] x86/mce: Remove __mcheck_cpu_init_early()

[Yazen Ghannam]

On Thu, Feb 27, 2025 at 09:48:01PM +0100, Borislav Petkov wrote:
> On February 27, 2025 8:59:33 PM GMT+01:00, Yazen Ghannam <yazen.ghannam@amd.com> wrote:
> >On Thu, Feb 27, 2025 at 08:33:19PM +0100, Borislav Petkov wrote:
> >> On February 27, 2025 5:31:48 PM GMT+01:00, Yazen Ghannam <yazen.ghannam@amd.com> wrote:
> >> >On Thu, Feb 27, 2025 at 04:25:00PM +0100, Borislav Petkov wrote:
> >> >> On Thu, Feb 13, 2025 at 04:45:55PM +0000, Yazen Ghannam wrote:
> >> >> > Also, move __mcheck_cpu_init_generic() after
> >> >> > __mcheck_cpu_init_prepare_banks() so that MCA is enabled after the first
> >> >> > MCA polling event.
> >> >> 
> >> >> The reason being?
> >> >> 
> >> >> Precaution?
> >> >> 
> >> >> It was this way since forever, why are you moving it now? Any particular
> >> >> reason?
> >> >> 
> >> >
> >> >1) To read/clear old errors before turning on MCA. The updated
> >> >__mcheck_cpu_init_prepare_banks() function does this for the MCi_CTL
> >> >registers. This patch does this for the MCG_CTL register too.
> >> >
> >> >2) To ensure that vendor-specific setup is finished beforehand also.
> >> 
> >> That doesn't answer my question. All of the above gets done even without shuffling the order...
> >> 
> >> 
> >
> >MCA banks can start logging errors once MCG_CTL is set. The AMD docs say
> >"The operating system must initialize the MCA_CONFIG registers prior to
> >initialization of the MCA_CTL registers."
> >
> >"The MCA_CTL registers must be initialized prior to enabling the error
> >reporting banks in MCG_CTL".
> >
> >However, the Intel docs "Machine-Check Initialization Pseudocode" say
> >MCG_CTL first then MCi_CTL.
> >
> >But both agree that CR4.MCE should be set last.
> >
> >We have an old thread on the topic that led to this patch.
> >https://lore.kernel.org/all/YqJHwXkg3Ny9fI3s@yaz-fattaah/
> >
> >And it seemed okay at the time.
> >https://lore.kernel.org/all/YrnTMmwl5TrHwT9J@zn.tnic/
> >
> >I don't think anything much has changed since then, so I included the
> >old patch again in this set.
> >
> >Thanks,
> >Yazen
> 
> This is exactly what needs to be in the commit message - why is the change being done. 
> -- 

Sure thing. I'll update the commit message.

Thanks,
Yazen

[PATCH v2 07/16] x86/mce: Define BSP-only init

[Yazen Ghannam]

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.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    Link:
    https://lore.kernel.org/r/Y6yQpWtlnFmL04h6@zn.tnic
    
    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 | 29 ++++++++++++++++++++++-------
 4 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 36ff81c1b3b1..c98387364d6c 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 cpu_mca_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 cpu_mca_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 76598a93a8fa..b14e2d98b45d 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1649,6 +1649,7 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 		setup_clear_cpu_cap(X86_FEATURE_LA57);
 
 	detect_nopl();
+	cpu_mca_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 302a310d0630..a4ef4ff1a7ff 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -656,9 +656,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 7fbf1c8291b8..f13d3f7ca56e 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1832,13 +1832,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)
@@ -2238,6 +2231,28 @@ DEFINE_IDTENTRY_RAW(exc_machine_check)
 }
 #endif
 
+/* Called only on the boot CPU. */
+void cpu_mca_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);
+
+	rdmsrl(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.43.0

RE: [PATCH v2 07/16] x86/mce: Define BSP-only init

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> diff --git a/arch/x86/kernel/cpu/mce/amd.c
> b/arch/x86/kernel/cpu/mce/amd.c index 302a310d0630..a4ef4ff1a7ff 100644
> --- a/arch/x86/kernel/cpu/mce/amd.c
> +++ b/arch/x86/kernel/cpu/mce/amd.c
> @@ -656,9 +656,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;
> 
> [...]
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> [...]
> +/* Called only on the boot CPU. */
> +void cpu_mca_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);

1. Before this patch set, the above code was executed only if the following 
    condition was true. Do we still need this check?

    if (c->x86_vendor == X86_VENDOR_AMD || c->x86_vendor == X86_VENDOR_HYGON) 
    {
          The above code.
     }

2. Can " mce_flags.amd_threshold  = 1;" also be moved here?

-Qiuxu

Re: [PATCH v2 07/16] x86/mce: Define BSP-only init

[Yazen Ghannam]

On Tue, Feb 18, 2025 at 03:16:49AM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > [...]
> > diff --git a/arch/x86/kernel/cpu/mce/amd.c
> > b/arch/x86/kernel/cpu/mce/amd.c index 302a310d0630..a4ef4ff1a7ff 100644
> > --- a/arch/x86/kernel/cpu/mce/amd.c
> > +++ b/arch/x86/kernel/cpu/mce/amd.c
> > @@ -656,9 +656,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;
> > 
> > [...]
> > --- a/arch/x86/kernel/cpu/mce/core.c
> > +++ b/arch/x86/kernel/cpu/mce/core.c
> > [...]
> > +/* Called only on the boot CPU. */
> > +void cpu_mca_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);
> 
> 1. Before this patch set, the above code was executed only if the following 
>     condition was true. Do we still need this check?
> 
>     if (c->x86_vendor == X86_VENDOR_AMD || c->x86_vendor == X86_VENDOR_HYGON) 
>     {
>           The above code.
>      }

I don't think so. Feature checks should be independent of vendor, so the
vendor checks are redundant.

> 
> 2. Can " mce_flags.amd_threshold  = 1;" also be moved here?
> 

No, because this flag is not based on a CPU feature. However, I think
more cleanup can be done for this, but that will come later.

Thanks,
Yazen

RE: [PATCH v2 07/16] x86/mce: Define BSP-only init

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> > > --- a/arch/x86/kernel/cpu/mce/core.c
> > > +++ b/arch/x86/kernel/cpu/mce/core.c
> > > [...]
> > > +/* Called only on the boot CPU. */
> > > +void cpu_mca_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);
> >
> > 1. Before this patch set, the above code was executed only if the following
> >     condition was true. Do we still need this check?
> >
> >     if (c->x86_vendor == X86_VENDOR_AMD || c->x86_vendor ==
> X86_VENDOR_HYGON)
> >     {
> >           The above code.
> >      }
> 
> I don't think so. Feature checks should be independent of vendor, so the
> vendor checks are redundant.

Without the vendor check, the Intel CPUs will also run this code (which they should
*NOT* ) and may mistakenly set those global AMD-specific flags, which may result in
Intel CPUs performing AMD-specific error handling during the machine check.

-Qiuxu

Re: [PATCH v2 07/16] x86/mce: Define BSP-only init

[Yazen Ghannam]

On Thu, Feb 20, 2025 at 01:37:37AM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > [...]
> > > > --- a/arch/x86/kernel/cpu/mce/core.c
> > > > +++ b/arch/x86/kernel/cpu/mce/core.c
> > > > [...]
> > > > +/* Called only on the boot CPU. */
> > > > +void cpu_mca_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);
> > >
> > > 1. Before this patch set, the above code was executed only if the following
> > >     condition was true. Do we still need this check?
> > >
> > >     if (c->x86_vendor == X86_VENDOR_AMD || c->x86_vendor ==
> > X86_VENDOR_HYGON)
> > >     {
> > >           The above code.
> > >      }
> > 
> > I don't think so. Feature checks should be independent of vendor, so the
> > vendor checks are redundant.
> 
> Without the vendor check, the Intel CPUs will also run this code (which they should
> *NOT* ) and may mistakenly set those global AMD-specific flags, which may result in
> Intel CPUs performing AMD-specific error handling during the machine check.
> 

It's okay if Intel CPUs run this code, because they don't support these
features. The feature flags are generally derived from CPUID bits, and
x86 vendors do try to make sure they are unique and not
overloaded/redefined between vendors.

The same is true in reverse. It's okay if AMD CPUs run code to check for
Intel-specific features. The feature checks will be false, and
feature-specific code will not be used.

Thanks,
Yazen

RE: [PATCH v2 07/16] x86/mce: Define BSP-only init

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...] 
> It's okay if Intel CPUs run this code, because they don't support these
> features. The feature flags are generally derived from CPUID bits, and
> x86 vendors do try to make sure they are unique and not
> overloaded/redefined between vendors.
>
> The same is true in reverse. It's okay if AMD CPUs run code to check for Intel-
> specific features. The feature checks will be false, and feature-specific code
> will not be used.

Thanks for addressing my concerns. 
This patch looks good to me.

    Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

[PATCH v2 08/16] x86/mce: Define BSP-only SMCA init

[Yazen Ghannam]

Currently on AMD systems, MCA interrupt handler functions are set during
CPU init. However, the functions only need to be set once for the whole
system.

Assign the handlers only during BSP init. Do so only for SMCA systems to
maintain the old behavior for legacy systems.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    Link:
    https://lore.kernel.org/r/20240604154635.GTZl8222q7WAEVSJKH@fat_crate.local
    
    v1->v2:
    * New in v2.

 arch/x86/kernel/cpu/mce/amd.c      | 6 ++++++
 arch/x86/kernel/cpu/mce/core.c     | 3 +++
 arch/x86/kernel/cpu/mce/internal.h | 2 ++
 3 files changed, 11 insertions(+)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index a4ef4ff1a7ff..bf2b1dc5aaa9 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -687,6 +687,12 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 		deferred_error_interrupt_enable(c);
 }
 
+void mce_smca_cpu_init(void)
+{
+	mce_threshold_vector	  = amd_threshold_interrupt;
+	deferred_error_int_vector = amd_deferred_error_interrupt;
+}
+
 /*
  * DRAM ECC errors are reported in the Northbridge (bank 4) with
  * Extended Error Code 8.
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index f13d3f7ca56e..402d7993eb96 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -2243,6 +2243,9 @@ void cpu_mca_init(struct cpuinfo_x86 *c)
 	mce_flags.succor	 = cpu_feature_enabled(X86_FEATURE_SUCCOR);
 	mce_flags.smca		 = cpu_feature_enabled(X86_FEATURE_SMCA);
 
+	if (mce_flags.smca)
+		mce_smca_cpu_init();
+
 	rdmsrl(MSR_IA32_MCG_CAP, cap);
 
 	/* Use accurate RIP reporting if available. */
diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
index 231ba8ca4a3e..a4bae8c0cf4c 100644
--- a/arch/x86/kernel/cpu/mce/internal.h
+++ b/arch/x86/kernel/cpu/mce/internal.h
@@ -293,11 +293,13 @@ static __always_inline void smca_extract_err_addr(struct mce *m)
 	m->addr &= GENMASK_ULL(55, lsb);
 }
 
+void mce_smca_cpu_init(void);
 #else
 static inline void mce_threshold_create_device(unsigned int cpu)	{ }
 static inline bool amd_filter_mce(struct mce *m) { return false; }
 static inline bool amd_mce_usable_address(struct mce *m) { return false; }
 static inline void smca_extract_err_addr(struct mce *m) { }
+static inline void mce_smca_cpu_init(void) {}
 #endif
 
 #ifdef CONFIG_X86_ANCIENT_MCE

-- 
2.43.0

RE: [PATCH v2 08/16] x86/mce: Define BSP-only SMCA init

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Sent: Friday, February 14, 2025 12:46 AM
> To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> <yazen.ghannam@amd.com>
> Subject: [PATCH v2 08/16] x86/mce: Define BSP-only SMCA init
> 
> Currently on AMD systems, MCA interrupt handler functions are set during
> CPU init. However, the functions only need to be set once for the whole
> system.
> 
> Assign the handlers only during BSP init. Do so only for SMCA systems to
> maintain the old behavior for legacy systems.

Looks like the interrupt handler is still set during each per-CPU online, right?
What's the benefit/purpose of this patch? Thanks!

    mce_cpu_online(cpu)
        mce_threshold_create_device(cpu) {
            ...
            mce_threshold_vector = amd_threshold_interrupt;
            ...
       }

> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>

[...]

Re: [PATCH v2 08/16] x86/mce: Define BSP-only SMCA init

[Yazen Ghannam]

On Tue, Feb 18, 2025 at 03:33:08AM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > Sent: Friday, February 14, 2025 12:46 AM
> > To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> > Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> > Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> > <yazen.ghannam@amd.com>
> > Subject: [PATCH v2 08/16] x86/mce: Define BSP-only SMCA init
> > 
> > Currently on AMD systems, MCA interrupt handler functions are set during
> > CPU init. However, the functions only need to be set once for the whole
> > system.
> > 
> > Assign the handlers only during BSP init. Do so only for SMCA systems to
> > maintain the old behavior for legacy systems.
> 
> Looks like the interrupt handler is still set during each per-CPU online, right?
> What's the benefit/purpose of this patch? Thanks!
> 

This patch is doing the "correct" thing.

>     mce_cpu_online(cpu)
>         mce_threshold_create_device(cpu) {
>             ...
>             mce_threshold_vector = amd_threshold_interrupt;
>             ...
>        }
> 

This part remains for legacy systems. However, I think this is another
place to do more cleanup later.

Thanks,
Yazen

[PATCH v2 09/16] x86/mce: Do 'UNKNOWN' vendor check early

[Yazen Ghannam]

The 'UNKNOWN' vendor check is handled as a quirk that is run on each
online CPU. However, all CPUs are expected to have the same vendor.

Move the 'UNKNOWN' vendor check to the BSP-only init so it is done early
and once. Remove the unnecessary return value from the quirks check.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    v1->v2:
    * New in v2.

 arch/x86/kernel/cpu/mce/core.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 402d7993eb96..38db802acde4 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1974,14 +1974,11 @@ static void apply_quirks_zhaoxin(struct cpuinfo_x86 *c)
 }
 
 /* Add per CPU specific workarounds here */
-static bool __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
+static void __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
 {
 	struct mca_config *cfg = &mca_cfg;
 
 	switch (c->x86_vendor) {
-	case X86_VENDOR_UNKNOWN:
-		pr_info("unknown CPU type - not enabling MCE support\n");
-		return false;
 	case X86_VENDOR_AMD:
 		apply_quirks_amd(c);
 		break;
@@ -1997,8 +1994,6 @@ static bool __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
 		cfg->monarch_timeout = 0;
 	if (cfg->bootlog != 0)
 		cfg->panic_timeout = 30;
-
-	return true;
 }
 
 static bool __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c)
@@ -2239,6 +2234,12 @@ void cpu_mca_init(struct cpuinfo_x86 *c)
 	if (!mce_available(c))
 		return;
 
+	if (c->x86_vendor == X86_VENDOR_UNKNOWN) {
+		mca_cfg.disabled = 1;
+		pr_info("unknown CPU type - not enabling MCE support\n");
+		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);
@@ -2273,10 +2274,7 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c)
 
 	__mcheck_cpu_cap_init();
 
-	if (!__mcheck_cpu_apply_quirks(c)) {
-		mca_cfg.disabled = 1;
-		return;
-	}
+	__mcheck_cpu_apply_quirks(c);
 
 	if (!mce_gen_pool_init()) {
 		mca_cfg.disabled = 1;

-- 
2.43.0

RE: [PATCH v2 09/16] x86/mce: Do 'UNKNOWN' vendor check early

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Sent: Friday, February 14, 2025 12:46 AM
> To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> <yazen.ghannam@amd.com>
> Subject: [PATCH v2 09/16] x86/mce: Do 'UNKNOWN' vendor check early
> 
> The 'UNKNOWN' vendor check is handled as a quirk that is run on each online
> CPU. However, all CPUs are expected to have the same vendor.
> 
> Move the 'UNKNOWN' vendor check to the BSP-only init so it is done early
> and once. Remove the unnecessary return value from the quirks check.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>

LGTM. Thanks.

    Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

[PATCH v2 10/16] x86/mce: Separate global and per-CPU quirks

[Yazen Ghannam]

Many quirks are global configuration settings and a handful apply to
each CPU.

Move the per-CPU quirks to vendor init to execute them on each online
CPU. Set the global quirks during BSP-only init so they're only executed
once and early.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    v1->v2:
    * New in v2.

 arch/x86/kernel/cpu/mce/amd.c   | 23 +++++++++++++++++++++++
 arch/x86/kernel/cpu/mce/core.c  | 36 ++----------------------------------
 arch/x86/kernel/cpu/mce/intel.c | 15 +++++++++++++++
 3 files changed, 40 insertions(+), 34 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index bf2b1dc5aaa9..c6510415159f 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -649,6 +649,28 @@ static void disable_err_thresholding(struct cpuinfo_x86 *c, unsigned int bank)
 		wrmsrl(MSR_K7_HWCR, hwcr);
 }
 
+static void amd_apply_quirks(struct cpuinfo_x86 *c)
+{
+	struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
+
+	/* This should be disabled by the BIOS, but isn't always */
+	if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
+		/*
+		 * disable GART TBL walk error reporting, which
+		 * trips off incorrectly with the IOMMU & 3ware
+		 * & Cerberus:
+		 */
+		clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
+	}
+
+	/*
+	 * Various K7s with broken bank 0 around. Always disable
+	 * by default.
+	 */
+	if (c->x86 == 6 && this_cpu_read(mce_num_banks))
+		mce_banks[0].ctl = 0;
+}
+
 /* cpu init entry point, called from mce.c with preempt off */
 void mce_amd_feature_init(struct cpuinfo_x86 *c)
 {
@@ -656,6 +678,7 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 	u32 low = 0, high = 0, address = 0;
 	int offset = -1;
 
+	amd_apply_quirks(c);
 	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 38db802acde4..1ea52f6259a4 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1879,18 +1879,6 @@ static void __mcheck_cpu_init_prepare_banks(void)
 
 static void apply_quirks_amd(struct cpuinfo_x86 *c)
 {
-	struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
-
-	/* This should be disabled by the BIOS, but isn't always */
-	if (c->x86 == 15 && this_cpu_read(mce_num_banks) > 4) {
-		/*
-		 * disable GART TBL walk error reporting, which
-		 * trips off incorrectly with the IOMMU & 3ware
-		 * & Cerberus:
-		 */
-		clear_bit(10, (unsigned long *)&mce_banks[4].ctl);
-	}
-
 	if (c->x86 < 0x11 && mca_cfg.bootlog < 0) {
 		/*
 		 * Lots of broken BIOS around that don't clear them
@@ -1899,13 +1887,6 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
 		mca_cfg.bootlog = 0;
 	}
 
-	/*
-	 * Various K7s with broken bank 0 around. Always disable
-	 * by default.
-	 */
-	if (c->x86 == 6 && this_cpu_read(mce_num_banks))
-		mce_banks[0].ctl = 0;
-
 	/*
 	 * overflow_recov is supported for F15h Models 00h-0fh
 	 * even though we don't have a CPUID bit for it.
@@ -1919,23 +1900,10 @@ static void apply_quirks_amd(struct cpuinfo_x86 *c)
 
 static void apply_quirks_intel(struct cpuinfo_x86 *c)
 {
-	struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
-
 	/* Older CPUs (prior to family 6) don't need quirks. */
 	if (c->x86_vfm < INTEL_PENTIUM_PRO)
 		return;
 
-	/*
-	 * SDM documents that on family 6 bank 0 should not be written
-	 * because it aliases to another special BIOS controlled
-	 * register.
-	 * But it's not aliased anymore on model 0x1a+
-	 * Don't ignore bank 0 completely because there could be a
-	 * valid event later, merely don't write CTL0.
-	 */
-	if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks))
-		mce_banks[0].init = false;
-
 	/*
 	 * All newer Intel systems support MCE broadcasting. Enable
 	 * synchronization with a one second timeout.
@@ -2255,6 +2223,8 @@ void cpu_mca_init(struct cpuinfo_x86 *c)
 
 	if (cap & MCG_SER_P)
 		mca_cfg.ser = 1;
+
+	__mcheck_cpu_apply_quirks(c);
 }
 
 /*
@@ -2274,8 +2244,6 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c)
 
 	__mcheck_cpu_cap_init();
 
-	__mcheck_cpu_apply_quirks(c);
-
 	if (!mce_gen_pool_init()) {
 		mca_cfg.disabled = 1;
 		pr_emerg("Couldn't allocate MCE records pool!\n");
diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
index f863df0ff42c..1a7aaee14991 100644
--- a/arch/x86/kernel/cpu/mce/intel.c
+++ b/arch/x86/kernel/cpu/mce/intel.c
@@ -468,8 +468,23 @@ static void intel_imc_init(struct cpuinfo_x86 *c)
 	}
 }
 
+static void intel_apply_quirks(struct cpuinfo_x86 *c)
+{
+	/*
+	 * SDM documents that on family 6 bank 0 should not be written
+	 * because it aliases to another special BIOS controlled
+	 * register.
+	 * But it's not aliased anymore on model 0x1a+
+	 * Don't ignore bank 0 completely because there could be a
+	 * valid event later, merely don't write CTL0.
+	 */
+	if (c->x86_vfm < INTEL_NEHALEM_EP && this_cpu_read(mce_num_banks))
+		this_cpu_ptr(mce_banks_array)[0].init = false;
+}
+
 void mce_intel_feature_init(struct cpuinfo_x86 *c)
 {
+	intel_apply_quirks(c);
 	intel_init_cmci();
 	intel_init_lmce();
 	intel_imc_init(c);

-- 
2.43.0

RE: [PATCH v2 10/16] x86/mce: Separate global and per-CPU quirks

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> --- a/arch/x86/kernel/cpu/mce/intel.c
> +++ b/arch/x86/kernel/cpu/mce/intel.c
> @@ -468,8 +468,23 @@ static void intel_imc_init(struct cpuinfo_x86 *c)
>  	}
>  }
> 
> +static void intel_apply_quirks(struct cpuinfo_x86 *c) {
> +	/*
> +	 * SDM documents that on family 6 bank 0 should not be written
> +	 * because it aliases to another special BIOS controlled
> +	 * register.
> +	 * But it's not aliased anymore on model 0x1a+
> +	 * Don't ignore bank 0 completely because there could be a
> +	 * valid event later, merely don't write CTL0.

Is it better to add the following description here? So that it's clear
we don't apply the quirks for older CPUs.

    Older CPUs (prior to family 6) can't reach this point and already return early 
    due to the check of __mcheck_cpu_ancient_init().

> +	 */
> +	if (c->x86_vfm < INTEL_NEHALEM_EP &&
> this_cpu_read(mce_num_banks))
> +		this_cpu_ptr(mce_banks_array)[0].init = false; }
> +
[...]

LGTM. Thanks.

    Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

Re: [PATCH v2 10/16] x86/mce: Separate global and per-CPU quirks

[Yazen Ghannam]

On Tue, Feb 18, 2025 at 06:03:42AM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > [...]
> > --- a/arch/x86/kernel/cpu/mce/intel.c
> > +++ b/arch/x86/kernel/cpu/mce/intel.c
> > @@ -468,8 +468,23 @@ static void intel_imc_init(struct cpuinfo_x86 *c)
> >  	}
> >  }
> > 
> > +static void intel_apply_quirks(struct cpuinfo_x86 *c) {
> > +	/*
> > +	 * SDM documents that on family 6 bank 0 should not be written
> > +	 * because it aliases to another special BIOS controlled
> > +	 * register.
> > +	 * But it's not aliased anymore on model 0x1a+
> > +	 * Don't ignore bank 0 completely because there could be a
> > +	 * valid event later, merely don't write CTL0.
> 
> Is it better to add the following description here? So that it's clear
> we don't apply the quirks for older CPUs.
> 
>     Older CPUs (prior to family 6) can't reach this point and already return early 
>     due to the check of __mcheck_cpu_ancient_init().
> 

I don't know. As you said, the older CPUs don't enter this code, so why
refer to them at all?

> > +	 */
> > +	if (c->x86_vfm < INTEL_NEHALEM_EP &&
> > this_cpu_read(mce_num_banks))
> > +		this_cpu_ptr(mce_banks_array)[0].init = false; }
> > +
> [...]
> 
> LGTM. Thanks.
> 
>     Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 

Thanks,
Yazen

RE: [PATCH v2 10/16] x86/mce: Separate global and per-CPU quirks

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> > Is it better to add the following description here? So that it's clear
> > we don't apply the quirks for older CPUs.
> >
> >     Older CPUs (prior to family 6) can't reach this point and already return
> early
> >     due to the check of __mcheck_cpu_ancient_init().
> >
> 
> I don't know. As you said, the older CPUs don't enter this code, so why refer
> to them at all?
> 

As the following check "if (c->x86_vfm < INTEL_NEHALEM_EP ..." includes older 
CPUs (e.g. family == 5). Stating that older CPUs don't reach this point makes it 
clear that the quirks are not applied to them.

> > > +	 */
> > > +	if (c->x86_vfm < INTEL_NEHALEM_EP &&
> > > this_cpu_read(mce_num_banks))
> > > +		this_cpu_ptr(mce_banks_array)[0].init = false; }
> > > +
> > [...]
> >
> > LGTM. Thanks.
> >
> >     Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> >
> 
> Thanks,
> Yazen

Re: [PATCH v2 10/16] x86/mce: Separate global and per-CPU quirks

[Yazen Ghannam]

On Thu, Feb 20, 2025 at 01:27:03AM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > [...]
> > > Is it better to add the following description here? So that it's clear
> > > we don't apply the quirks for older CPUs.
> > >
> > >     Older CPUs (prior to family 6) can't reach this point and already return
> > early
> > >     due to the check of __mcheck_cpu_ancient_init().
> > >
> > 
> > I don't know. As you said, the older CPUs don't enter this code, so why refer
> > to them at all?
> > 
> 
> As the following check "if (c->x86_vfm < INTEL_NEHALEM_EP ..." includes older 
> CPUs (e.g. family == 5). Stating that older CPUs don't reach this point makes it 
> clear that the quirks are not applied to them.
> 

Okay, that's fair.

Thanks,
Yazen

[PATCH v2 11/16] x86/mce: Move machine_check_poll() status checks to helper functions

[Yazen Ghannam]

There are a number of generic and vendor-specific status checks in
machine_check_poll(). These are used to determine if an error should be
skipped.

Move these into helper functions. Future vendor-specific checks will be
added to the helpers.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    Link:
    https://lore.kernel.org/r/20240523155641.2805411-5-yazen.ghannam@amd.com
    
    v1->v2:
    * Change log_poll_error() to should_log_poll_error().
    * Keep code comment.

 arch/x86/kernel/cpu/mce/core.c | 88 +++++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 40 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 1ea52f6259a4..372e8b078dd5 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -692,6 +692,52 @@ static noinstr void mce_read_aux(struct mce_hw_err *err, int i)
 
 DEFINE_PER_CPU(unsigned, mce_poll_count);
 
+/*
+ * Newer Intel systems that support software error
+ * recovery need to make additional checks. Other
+ * CPUs should skip over uncorrected errors, but log
+ * everything else.
+ */
+static bool ser_should_log_poll_error(struct mce *m)
+{
+	/* Log "not enabled" (speculative) errors */
+	if (!(m->status & MCI_STATUS_EN))
+		return true;
+
+	/*
+	 * Log UCNA (SDM: 15.6.3 "UCR Error Classification")
+	 * UC == 1 && PCC == 0 && S == 0
+	 */
+	if (!(m->status & MCI_STATUS_PCC) && !(m->status & MCI_STATUS_S))
+		return true;
+
+	return false;
+}
+
+static bool should_log_poll_error(enum mcp_flags flags, struct mce_hw_err *err)
+{
+	struct mce *m = &err->m;
+
+	/* If this entry is not valid, ignore it. */
+	if (!(m->status & MCI_STATUS_VAL))
+		return false;
+
+	/*
+	 * If we are logging everything (at CPU online) or this
+	 * is a corrected error, then we must log it.
+	 */
+	if ((flags & MCP_UC) || !(m->status & MCI_STATUS_UC))
+		return true;
+
+	if (mca_cfg.ser)
+		return ser_should_log_poll_error(m);
+
+	if (m->status & MCI_STATUS_UC)
+		return false;
+
+	return true;
+}
+
 /*
  * Poll for corrected events or events that happened before reset.
  * Those are just logged through /dev/mcelog.
@@ -743,48 +789,10 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		if (!mca_cfg.cmci_disabled)
 			mce_track_storm(m);
 
-		/* If this entry is not valid, ignore it */
-		if (!(m->status & MCI_STATUS_VAL))
+		/* Verify that the error should be logged based on hardware conditions. */
+		if (!should_log_poll_error(flags, &err))
 			continue;
 
-		/*
-		 * If we are logging everything (at CPU online) or this
-		 * is a corrected error, then we must log it.
-		 */
-		if ((flags & MCP_UC) || !(m->status & MCI_STATUS_UC))
-			goto log_it;
-
-		/*
-		 * Newer Intel systems that support software error
-		 * recovery need to make additional checks. Other
-		 * CPUs should skip over uncorrected errors, but log
-		 * everything else.
-		 */
-		if (!mca_cfg.ser) {
-			if (m->status & MCI_STATUS_UC)
-				continue;
-			goto log_it;
-		}
-
-		/* Log "not enabled" (speculative) errors */
-		if (!(m->status & MCI_STATUS_EN))
-			goto log_it;
-
-		/*
-		 * Log UCNA (SDM: 15.6.3 "UCR Error Classification")
-		 * UC == 1 && PCC == 0 && S == 0
-		 */
-		if (!(m->status & MCI_STATUS_PCC) && !(m->status & MCI_STATUS_S))
-			goto log_it;
-
-		/*
-		 * Skip anything else. Presumption is that our read of this
-		 * bank is racing with a machine check. Leave the log alone
-		 * for do_machine_check() to deal with it.
-		 */
-		continue;
-
-log_it:
 		mce_read_aux(&err, i);
 		m->severity = mce_severity(m, NULL, NULL, false);
 		/*

-- 
2.43.0

RE: [PATCH v2 11/16] x86/mce: Move machine_check_poll() status checks to helper functions

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Sent: Friday, February 14, 2025 12:46 AM
> To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> <yazen.ghannam@amd.com>
> Subject: [PATCH v2 11/16] x86/mce: Move machine_check_poll() status checks
> to helper functions
> 
> There are a number of generic and vendor-specific status checks in
> machine_check_poll(). These are used to determine if an error should be
> skipped.
> 
> Move these into helper functions. Future vendor-specific checks will be
> added to the helpers.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>

LGTM. Thanks.

    Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

[PATCH v2 12/16] x86/mce: Unify AMD THR handler with MCA Polling

[Yazen Ghannam]

AMD systems optionally support an MCA thresholding interrupt. The
interrupt should be used as another signal to trigger MCA polling. This
is similar to how the Intel Corrected Machine Check interrupt (CMCI) is
handled.

AMD MCA thresholding is managed using the MCA_MISC registers within an
MCA bank. The OS will need to modify the hardware error count field in
order to reset the threshold limit and rearm the interrupt. Management
of the MCA_MISC register should be done as a follow up to the basic MCA
polling flow. It should not be the main focus of the interrupt handler.

Furthermore, future systems will have the ability to send an MCA
thresholding interrupt to the OS even when the OS does not manage the
feature, i.e. MCA_MISC registers are Read-as-Zero/Locked.

Call the common MCA polling function when handling the MCA thresholding
interrupt. This will allow the OS to find any valid errors whether or
not the MCA thresholding feature is OS-managed. Also, this allows the
common MCA polling options and kernel parameters to apply to AMD
systems.

Add a callback to the MCA polling function to check and reset any
threshold blocks that have reached their threshold limit.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    Link:
    https://lore.kernel.org/r/20240523155641.2805411-7-yazen.ghannam@amd.com
    
    v1->v2:
    * Start collecting per-CPU items in a struct.
    * Keep and use mce_flags.amd_threshold.

 arch/x86/kernel/cpu/mce/amd.c      | 49 ++++++++++++++++----------------------
 arch/x86/kernel/cpu/mce/core.c     |  3 +++
 arch/x86/kernel/cpu/mce/internal.h |  2 ++
 3 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index c6510415159f..5e491dbdeecc 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -54,6 +54,12 @@
 
 static bool thresholding_irq_en;
 
+struct mce_amd_cpu_data {
+	mce_banks_t     thr_intr_banks;
+};
+
+static DEFINE_PER_CPU_READ_MOSTLY(struct mce_amd_cpu_data, mce_amd_data);
+
 static const char * const th_names[] = {
 	"load_store",
 	"insn_fetch",
@@ -559,6 +565,7 @@ prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
 	if (!b.interrupt_capable)
 		goto done;
 
+	__set_bit(bank, this_cpu_ptr(&mce_amd_data)->thr_intr_banks);
 	b.interrupt_enable = 1;
 
 	if (!mce_flags.smca) {
@@ -898,12 +905,7 @@ static void amd_deferred_error_interrupt(void)
 		log_error_deferred(bank);
 }
 
-static void log_error_thresholding(unsigned int bank, u64 misc)
-{
-	_log_error_deferred(bank, misc);
-}
-
-static void log_and_reset_block(struct threshold_block *block)
+static void reset_block(struct threshold_block *block)
 {
 	struct thresh_restart tr;
 	u32 low = 0, high = 0;
@@ -917,23 +919,14 @@ static void log_and_reset_block(struct threshold_block *block)
 	if (!(high & MASK_OVERFLOW_HI))
 		return;
 
-	/* Log the MCE which caused the threshold event. */
-	log_error_thresholding(block->bank, ((u64)high << 32) | low);
-
-	/* Reset threshold block after logging error. */
 	memset(&tr, 0, sizeof(tr));
 	tr.b = block;
 	threshold_restart_bank(&tr);
 }
 
-/*
- * Threshold interrupt handler will service THRESHOLD_APIC_VECTOR. The interrupt
- * goes off when error_count reaches threshold_limit.
- */
-static void amd_threshold_interrupt(void)
+void amd_reset_thr_limit(unsigned int bank)
 {
-	struct threshold_bank **bp = this_cpu_read(threshold_banks), *thr_bank;
-	unsigned int bank, cpu = smp_processor_id();
+	struct threshold_bank **bp = this_cpu_read(threshold_banks);
 	struct threshold_block *block, *tmp;
 
 	/*
@@ -941,20 +934,20 @@ static void amd_threshold_interrupt(void)
 	 * handler is installed at boot time, but on a hotplug event the
 	 * interrupt might fire before the data has been initialized.
 	 */
-	if (!bp)
+	if (!bp || !bp[bank])
 		return;
 
-	for (bank = 0; bank < this_cpu_read(mce_num_banks); ++bank) {
-		if (!(per_cpu(bank_map, cpu) & BIT_ULL(bank)))
-			continue;
-
-		thr_bank = bp[bank];
-		if (!thr_bank)
-			continue;
+	list_for_each_entry_safe(block, tmp, &bp[bank]->miscj, miscj)
+		reset_block(block);
+}
 
-		list_for_each_entry_safe(block, tmp, &thr_bank->miscj, miscj)
-			log_and_reset_block(block);
-	}
+/*
+ * Threshold interrupt handler will service THRESHOLD_APIC_VECTOR. The interrupt
+ * goes off when error_count reaches threshold_limit.
+ */
+static void amd_threshold_interrupt(void)
+{
+	machine_check_poll(MCP_TIMESTAMP, &this_cpu_ptr(&mce_amd_data)->thr_intr_banks);
 }
 
 /*
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 372e8b078dd5..b26eb576e413 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -809,6 +809,9 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 			mce_log(&err);
 
 clear_it:
+		if (mce_flags.amd_threshold)
+			amd_reset_thr_limit(i);
+
 		/*
 		 * Clear state for this bank.
 		 */
diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
index a4bae8c0cf4c..fe519acfafcf 100644
--- a/arch/x86/kernel/cpu/mce/internal.h
+++ b/arch/x86/kernel/cpu/mce/internal.h
@@ -268,6 +268,7 @@ void mce_prep_record_per_cpu(unsigned int cpu, struct mce *m);
 void mce_threshold_create_device(unsigned int cpu);
 extern bool amd_filter_mce(struct mce *m);
 bool amd_mce_usable_address(struct mce *m);
+void amd_reset_thr_limit(unsigned int bank);
 
 /*
  * If MCA_CONFIG[McaLsbInStatusSupported] is set, extract ErrAddr in bits
@@ -298,6 +299,7 @@ void mce_smca_cpu_init(void);
 static inline void mce_threshold_create_device(unsigned int cpu)	{ }
 static inline bool amd_filter_mce(struct mce *m) { return false; }
 static inline bool amd_mce_usable_address(struct mce *m) { return false; }
+static inline void amd_reset_thr_limit(unsigned int bank) { }
 static inline void smca_extract_err_addr(struct mce *m) { }
 static inline void mce_smca_cpu_init(void) {}
 #endif

-- 
2.43.0

RE: [PATCH v2 12/16] x86/mce: Unify AMD THR handler with MCA Polling

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Sent: Friday, February 14, 2025 12:46 AM
> To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> <yazen.ghannam@amd.com>
> Subject: [PATCH v2 12/16] x86/mce: Unify AMD THR handler with MCA Polling
> 
> AMD systems optionally support an MCA thresholding interrupt. The interrupt
> should be used as another signal to trigger MCA polling. This is similar to how
> the Intel Corrected Machine Check interrupt (CMCI) is handled.
> 
> AMD MCA thresholding is managed using the MCA_MISC registers within an
> MCA bank. The OS will need to modify the hardware error count field in order
> to reset the threshold limit and rearm the interrupt. Management of the
> MCA_MISC register should be done as a follow up to the basic MCA polling

s/follow up/follow-up

> flow. It should not be the main focus of the interrupt handler.
> 
> Furthermore, future systems will have the ability to send an MCA
> thresholding interrupt to the OS even when the OS does not manage the
> feature, i.e. MCA_MISC registers are Read-as-Zero/Locked.
> 
> Call the common MCA polling function when handling the MCA thresholding
> interrupt. This will allow the OS to find any valid errors whether or not the
> MCA thresholding feature is OS-managed. Also, this allows the common MCA
> polling options and kernel parameters to apply to AMD systems.
> 
> Add a callback to the MCA polling function to check and reset any threshold
> blocks that have reached their threshold limit.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>

Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

Re: [PATCH v2 12/16] x86/mce: Unify AMD THR handler with MCA Polling

[Yazen Ghannam]

On Tue, Feb 18, 2025 at 06:42:52AM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > Sent: Friday, February 14, 2025 12:46 AM
> > To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> > Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> > Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> > <yazen.ghannam@amd.com>
> > Subject: [PATCH v2 12/16] x86/mce: Unify AMD THR handler with MCA Polling
> > 
> > AMD systems optionally support an MCA thresholding interrupt. The interrupt
> > should be used as another signal to trigger MCA polling. This is similar to how
> > the Intel Corrected Machine Check interrupt (CMCI) is handled.
> > 
> > AMD MCA thresholding is managed using the MCA_MISC registers within an
> > MCA bank. The OS will need to modify the hardware error count field in order
> > to reset the threshold limit and rearm the interrupt. Management of the
> > MCA_MISC register should be done as a follow up to the basic MCA polling
> 
> s/follow up/follow-up
> 

Ack.

> > flow. It should not be the main focus of the interrupt handler.
> > 
> > Furthermore, future systems will have the ability to send an MCA
> > thresholding interrupt to the OS even when the OS does not manage the
> > feature, i.e. MCA_MISC registers are Read-as-Zero/Locked.
> > 
> > Call the common MCA polling function when handling the MCA thresholding
> > interrupt. This will allow the OS to find any valid errors whether or not the
> > MCA thresholding feature is OS-managed. Also, this allows the common MCA
> > polling options and kernel parameters to apply to AMD systems.
> > 
> > Add a callback to the MCA polling function to check and reset any threshold
> > blocks that have reached their threshold limit.
> > 
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 

Thanks,
Yazen

[PATCH v2 13/16] x86/mce: Unify AMD DFR handler with MCA Polling

[Yazen Ghannam]

AMD systems optionally support a deferred error interrupt. The interrupt
should be used as another signal to trigger MCA polling. This is similar
to how other MCA interrupts are handled.

Deferred errors do not require any special handling related to the
interrupt, e.g. resetting or rearming the interrupt, etc.

However, Scalable MCA systems include a pair of registers, MCA_DESTAT
and MCA_DEADDR, that should be checked for valid errors. This check
should be done whenever MCA registers are polled. Currently, the
deferred error interrupt does this check, but the MCA polling function
does not.

Call the MCA polling function when handling the deferred error
interrupt. This keeps all "polling" cases in a common function.

Call the polling function only for banks that have the deferred error
interrupt enabled.

Add an SMCA status check helper. This will do the same status check and
register clearing that the interrupt handler has done. And it extends
the common polling flow to find AMD deferred errors.

Remove old code whose functionality is already covered in the common MCA
code.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    Link:
    https://lore.kernel.org/r/20240523155641.2805411-8-yazen.ghannam@amd.com
    
    v1->v2:
    * Keep code comment.
    * Log directly from helper function rather than pass values.

 arch/x86/kernel/cpu/mce/amd.c  | 103 ++---------------------------------------
 arch/x86/kernel/cpu/mce/core.c |  58 +++++++++++++++++++++++
 2 files changed, 63 insertions(+), 98 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 5e491dbdeecc..5f547f5b3de0 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -56,6 +56,7 @@ static bool thresholding_irq_en;
 
 struct mce_amd_cpu_data {
 	mce_banks_t     thr_intr_banks;
+	mce_banks_t     dfr_intr_banks;
 };
 
 static DEFINE_PER_CPU_READ_MOSTLY(struct mce_amd_cpu_data, mce_amd_data);
@@ -300,8 +301,10 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
 		 * APIC based interrupt. First, check that no interrupt has been
 		 * set.
 		 */
-		if ((low & BIT(5)) && !((high >> 5) & 0x3))
+		if ((low & BIT(5)) && !((high >> 5) & 0x3)) {
+			__set_bit(bank, this_cpu_ptr(&mce_amd_data)->dfr_intr_banks);
 			high |= BIT(5);
+		}
 
 		this_cpu_ptr(mce_banks_array)[bank].lsb_in_status = !!(low & BIT(8));
 
@@ -794,37 +797,6 @@ bool amd_mce_usable_address(struct mce *m)
 	return false;
 }
 
-static void __log_error(unsigned int bank, u64 status, u64 addr, u64 misc)
-{
-	struct mce_hw_err err;
-	struct mce *m = &err.m;
-
-	mce_prep_record(&err);
-
-	m->status = status;
-	m->misc   = misc;
-	m->bank   = bank;
-	m->tsc	 = rdtsc();
-
-	if (m->status & MCI_STATUS_ADDRV) {
-		m->addr = addr;
-
-		smca_extract_err_addr(m);
-	}
-
-	if (mce_flags.smca) {
-		rdmsrl(MSR_AMD64_SMCA_MCx_IPID(bank), m->ipid);
-
-		if (m->status & MCI_STATUS_SYNDV) {
-			rdmsrl(MSR_AMD64_SMCA_MCx_SYND(bank), m->synd);
-			rdmsrl(MSR_AMD64_SMCA_MCx_SYND1(bank), err.vendor.amd.synd1);
-			rdmsrl(MSR_AMD64_SMCA_MCx_SYND2(bank), err.vendor.amd.synd2);
-		}
-	}
-
-	mce_log(&err);
-}
-
 DEFINE_IDTENTRY_SYSVEC(sysvec_deferred_error)
 {
 	trace_deferred_error_apic_entry(DEFERRED_ERROR_VECTOR);
@@ -834,75 +806,10 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_deferred_error)
 	apic_eoi();
 }
 
-/*
- * Returns true if the logged error is deferred. False, otherwise.
- */
-static inline bool
-_log_error_bank(unsigned int bank, u32 msr_stat, u32 msr_addr, u64 misc)
-{
-	u64 status, addr = 0;
-
-	rdmsrl(msr_stat, status);
-	if (!(status & MCI_STATUS_VAL))
-		return false;
-
-	if (status & MCI_STATUS_ADDRV)
-		rdmsrl(msr_addr, addr);
-
-	__log_error(bank, status, addr, misc);
-
-	wrmsrl(msr_stat, 0);
-
-	return status & MCI_STATUS_DEFERRED;
-}
-
-static bool _log_error_deferred(unsigned int bank, u32 misc)
-{
-	if (!_log_error_bank(bank, mca_msr_reg(bank, MCA_STATUS),
-			     mca_msr_reg(bank, MCA_ADDR), misc))
-		return false;
-
-	/*
-	 * Non-SMCA systems don't have MCA_DESTAT/MCA_DEADDR registers.
-	 * Return true here to avoid accessing these registers.
-	 */
-	if (!mce_flags.smca)
-		return true;
-
-	/* Clear MCA_DESTAT if the deferred error was logged from MCA_STATUS. */
-	wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(bank), 0);
-	return true;
-}
-
-/*
- * We have three scenarios for checking for Deferred errors:
- *
- * 1) Non-SMCA systems check MCA_STATUS and log error if found.
- * 2) SMCA systems check MCA_STATUS. If error is found then log it and also
- *    clear MCA_DESTAT.
- * 3) SMCA systems check MCA_DESTAT, if error was not found in MCA_STATUS, and
- *    log it.
- */
-static void log_error_deferred(unsigned int bank)
-{
-	if (_log_error_deferred(bank, 0))
-		return;
-
-	/*
-	 * Only deferred errors are logged in MCA_DE{STAT,ADDR} so just check
-	 * for a valid error.
-	 */
-	_log_error_bank(bank, MSR_AMD64_SMCA_MCx_DESTAT(bank),
-			      MSR_AMD64_SMCA_MCx_DEADDR(bank), 0);
-}
-
 /* APIC interrupt handler for deferred errors */
 static void amd_deferred_error_interrupt(void)
 {
-	unsigned int bank;
-
-	for (bank = 0; bank < this_cpu_read(mce_num_banks); ++bank)
-		log_error_deferred(bank);
+	machine_check_poll(MCP_TIMESTAMP, &this_cpu_ptr(&mce_amd_data)->dfr_intr_banks);
 }
 
 static void reset_block(struct threshold_block *block)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index b26eb576e413..9502e8d3fee7 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -692,6 +692,61 @@ static noinstr void mce_read_aux(struct mce_hw_err *err, int i)
 
 DEFINE_PER_CPU(unsigned, mce_poll_count);
 
+/*
+ * We have three scenarios for checking for Deferred errors:
+ *
+ * 1) Non-SMCA systems check MCA_STATUS and log error if found.
+ * 2) SMCA systems check MCA_STATUS. If error is found then log it and also
+ *    clear MCA_DESTAT.
+ * 3) SMCA systems check MCA_DESTAT, if error was not found in MCA_STATUS, and
+ *    log it.
+ */
+static bool smca_should_log_poll_error(enum mcp_flags flags, struct mce_hw_err *err)
+{
+	struct mce *m = &err->m;
+
+	/*
+	 * If this is a deferred error found in MCA_STATUS, then clear
+	 * the redundant data from the MCA_DESTAT register.
+	 */
+	if (m->status & MCI_STATUS_VAL) {
+		if (m->status & MCI_STATUS_DEFERRED)
+			mce_wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(m->bank), 0);
+
+		return true;
+	}
+
+	/*
+	 * If the MCA_DESTAT register has valid data, then use
+	 * it as the status register.
+	 */
+	m->status = mce_rdmsrl(MSR_AMD64_SMCA_MCx_DESTAT(m->bank));
+
+	if (!(m->status & MCI_STATUS_VAL))
+		return false;
+
+	/*
+	 * Gather all relevant data now and log the record before clearing
+	 * the deferred status register. This avoids needing to go back to
+	 * the polling function for these actions.
+	 */
+	mce_read_aux(err, m->bank);
+
+	if (m->status & MCI_STATUS_ADDRV)
+		m->addr = mce_rdmsrl(MSR_AMD64_SMCA_MCx_DEADDR(m->bank));
+
+	smca_extract_err_addr(m);
+	m->severity = mce_severity(m, NULL, NULL, false);
+
+	if (flags & MCP_QUEUE_LOG)
+		mce_gen_pool_add(err);
+	else
+		mce_log(err);
+
+	mce_wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(m->bank), 0);
+	return false;
+}
+
 /*
  * Newer Intel systems that support software error
  * recovery need to make additional checks. Other
@@ -718,6 +773,9 @@ static bool should_log_poll_error(enum mcp_flags flags, struct mce_hw_err *err)
 {
 	struct mce *m = &err->m;
 
+	if (mce_flags.smca)
+		return smca_should_log_poll_error(flags, err);
+
 	/* If this entry is not valid, ignore it. */
 	if (!(m->status & MCI_STATUS_VAL))
 		return false;

-- 
2.43.0

RE: [PATCH v2 13/16] x86/mce: Unify AMD DFR handler with MCA Polling

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> +static bool smca_should_log_poll_error(enum mcp_flags flags, struct
> +mce_hw_err *err) {
> +	struct mce *m = &err->m;
> +
> +	/*
> +	 * If this is a deferred error found in MCA_STATUS, then clear
> +	 * the redundant data from the MCA_DESTAT register.
> +	 */
> +	if (m->status & MCI_STATUS_VAL) {
> +		if (m->status & MCI_STATUS_DEFERRED)
> +			mce_wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(m-
> >bank), 0);
> +
> +		return true;
> +	}
> +
> +	/*
> +	 * If the MCA_DESTAT register has valid data, then use
> +	 * it as the status register.
> +	 */
> +	m->status = mce_rdmsrl(MSR_AMD64_SMCA_MCx_DESTAT(m-
> >bank));
> +
> +	if (!(m->status & MCI_STATUS_VAL))
> +		return false;
> +
> +	/*
> +	 * Gather all relevant data now and log the record before clearing
> +	 * the deferred status register. This avoids needing to go back to
> +	 * the polling function for these actions.
> +	 */
> +	mce_read_aux(err, m->bank);
> +
> +	if (m->status & MCI_STATUS_ADDRV)
> +		m->addr =
> mce_rdmsrl(MSR_AMD64_SMCA_MCx_DEADDR(m->bank));
> +
> +	smca_extract_err_addr(m);
> +	m->severity = mce_severity(m, NULL, NULL, false);
> +

Is the following check in machine_check_poll() needed before 
queuing/logging AMD's deferred error?

       if (mca_cfg.dont_log_ce && !mce_usable_address(m))
             //Just clear MCA_STATUS, but not queue/log errors.

> +	if (flags & MCP_QUEUE_LOG)
> +		mce_gen_pool_add(err);
> +	else
> +		mce_log(err);
> +
> +	mce_wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(m->bank), 0);
> +	return false;
> +}
[...]

Re: [PATCH v2 13/16] x86/mce: Unify AMD DFR handler with MCA Polling

[Yazen Ghannam]

On Tue, Feb 18, 2025 at 07:37:18AM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > [...]
> > +static bool smca_should_log_poll_error(enum mcp_flags flags, struct
> > +mce_hw_err *err) {
> > +	struct mce *m = &err->m;
> > +
> > +	/*
> > +	 * If this is a deferred error found in MCA_STATUS, then clear
> > +	 * the redundant data from the MCA_DESTAT register.
> > +	 */
> > +	if (m->status & MCI_STATUS_VAL) {
> > +		if (m->status & MCI_STATUS_DEFERRED)
> > +			mce_wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(m-
> > >bank), 0);
> > +
> > +		return true;
> > +	}
> > +
> > +	/*
> > +	 * If the MCA_DESTAT register has valid data, then use
> > +	 * it as the status register.
> > +	 */
> > +	m->status = mce_rdmsrl(MSR_AMD64_SMCA_MCx_DESTAT(m-
> > >bank));
> > +
> > +	if (!(m->status & MCI_STATUS_VAL))
> > +		return false;
> > +
> > +	/*
> > +	 * Gather all relevant data now and log the record before clearing
> > +	 * the deferred status register. This avoids needing to go back to
> > +	 * the polling function for these actions.
> > +	 */
> > +	mce_read_aux(err, m->bank);
> > +
> > +	if (m->status & MCI_STATUS_ADDRV)
> > +		m->addr =
> > mce_rdmsrl(MSR_AMD64_SMCA_MCx_DEADDR(m->bank));
> > +
> > +	smca_extract_err_addr(m);
> > +	m->severity = mce_severity(m, NULL, NULL, false);
> > +
> 
> Is the following check in machine_check_poll() needed before 
> queuing/logging AMD's deferred error?
> 
>        if (mca_cfg.dont_log_ce && !mce_usable_address(m))
>              //Just clear MCA_STATUS, but not queue/log errors.
> 

Good question. Deferred errors are uncorrectable errors that don't need
immediate action. They are not correctable errors, so the 'dont_log_ce'
flag shouldn't apply.

Thanks,
Yazen

RE: [PATCH v2 13/16] x86/mce: Unify AMD DFR handler with MCA Polling

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> > Is the following check in machine_check_poll() needed before
> > queuing/logging AMD's deferred error?
> >
> >        if (mca_cfg.dont_log_ce && !mce_usable_address(m))
> >              //Just clear MCA_STATUS, but not queue/log errors.
> >
> 
> Good question. Deferred errors are uncorrectable errors that don't need
> immediate action. They are not correctable errors, so the 'dont_log_ce'
> flag shouldn't apply.

Thanks for the explanation. 
How about the 2nd check "mce_usable_address(m)"?

-Qiuxu

Re: [PATCH v2 13/16] x86/mce: Unify AMD DFR handler with MCA Polling

[Yazen Ghannam]

On Thu, Feb 20, 2025 at 01:41:21AM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > [...]
> > > Is the following check in machine_check_poll() needed before
> > > queuing/logging AMD's deferred error?
> > >
> > >        if (mca_cfg.dont_log_ce && !mce_usable_address(m))
> > >              //Just clear MCA_STATUS, but not queue/log errors.
> > >
> > 
> > Good question. Deferred errors are uncorrectable errors that don't need
> > immediate action. They are not correctable errors, so the 'dont_log_ce'
> > flag shouldn't apply.
> 
> Thanks for the explanation. 
> How about the 2nd check "mce_usable_address(m)"?
> 

It's not relevant. We want to always log deferred errors since they are
uncorrectable.

Thanks,
Yazen

RE: [PATCH v2 13/16] x86/mce: Unify AMD DFR handler with MCA Polling

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> > Thanks for the explanation.
> > How about the 2nd check "mce_usable_address(m)"?
> >
> 
> It's not relevant. We want to always log deferred errors since they are
> uncorrectable.

Thanks for the explanation. The patch looks good to me.

   Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

[PATCH v2 14/16] x86/mce/amd: Enable interrupt vectors once per-CPU on SMCA systems

[Yazen Ghannam]

Scalable MCA systems have a per-CPU register that gives the APIC LVT
offset for the thresholding and deferred error interrupts.

Currently, this register is read once to set up the deferred error
interrupt and then read again for each thresholding block. Furthermore,
the APIC LVT registers are configured each time, but they only need to
be configured once per-CPU.

Move the APIC LVT setup to the early part of CPU init, so that the
registers are set up once. Also, this ensures that the kernel is ready
to service the interrupts before the individual error sources (each MCA
bank) are enabled.

Apply this change only to SMCA systems to avoid breaking any legacy
behavior. The deferred error interrupt is technically advertised by the
SUCCOR feature. However, this was first made available on SMCA systems.
Therefore, only set up the deferred error interrupt on SMCA systems and
simplify the code.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    Link:
    https://lore.kernel.org/r/20240523155641.2805411-9-yazen.ghannam@amd.com
    
    v1->v2:
    * Use new per-CPU struct.
    * Don't set up interrupt vectors.

 arch/x86/kernel/cpu/mce/amd.c | 113 ++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 65 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 5f547f5b3de0..75a48f484815 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -43,9 +43,6 @@
 /* Deferred error settings */
 #define MSR_CU_DEF_ERR		0xC0000410
 #define MASK_DEF_LVTOFF		0x000000F0
-#define MASK_DEF_INT_TYPE	0x00000006
-#define DEF_LVT_OFF		0x2
-#define DEF_INT_TYPE_APIC	0x2
 
 /* Scalable MCA: */
 
@@ -57,6 +54,8 @@ static bool thresholding_irq_en;
 struct mce_amd_cpu_data {
 	mce_banks_t     thr_intr_banks;
 	mce_banks_t     dfr_intr_banks;
+	bool		thr_intr_en;
+	bool		dfr_intr_en;
 };
 
 static DEFINE_PER_CPU_READ_MOSTLY(struct mce_amd_cpu_data, mce_amd_data);
@@ -271,6 +270,7 @@ void (*deferred_error_int_vector)(void) = default_deferred_error_interrupt;
 
 static void smca_configure(unsigned int bank, unsigned int cpu)
 {
+	struct mce_amd_cpu_data *data = this_cpu_ptr(&mce_amd_data);
 	u8 *bank_counts = this_cpu_ptr(smca_bank_counts);
 	const struct smca_hwid *s_hwid;
 	unsigned int i, hwid_mcatype;
@@ -301,8 +301,8 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
 		 * APIC based interrupt. First, check that no interrupt has been
 		 * set.
 		 */
-		if ((low & BIT(5)) && !((high >> 5) & 0x3)) {
-			__set_bit(bank, this_cpu_ptr(&mce_amd_data)->dfr_intr_banks);
+		if ((low & BIT(5)) && !((high >> 5) & 0x3) && data->dfr_intr_en) {
+			__set_bit(bank, data->dfr_intr_banks);
 			high |= BIT(5);
 		}
 
@@ -378,6 +378,14 @@ static bool lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
 {
 	int msr = (hi & MASK_LVTOFF_HI) >> 20;
 
+	/*
+	 * On SMCA CPUs, LVT offset is programmed at a different MSR, and
+	 * the BIOS provides the value. The original field where LVT offset
+	 * was set is reserved. Return early here:
+	 */
+	if (mce_flags.smca)
+		return false;
+
 	if (apic < 0) {
 		pr_err(FW_BUG "cpu %d, failed to setup threshold interrupt "
 		       "for bank %d, block %d (MSR%08X=0x%x%08x)\n", b->cpu,
@@ -386,14 +394,6 @@ static bool lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
 	}
 
 	if (apic != msr) {
-		/*
-		 * On SMCA CPUs, LVT offset is programmed at a different MSR, and
-		 * the BIOS provides the value. The original field where LVT offset
-		 * was set is reserved. Return early here:
-		 */
-		if (mce_flags.smca)
-			return false;
-
 		pr_err(FW_BUG "cpu %d, invalid threshold interrupt offset %d "
 		       "for bank %d, block %d (MSR%08X=0x%x%08x)\n",
 		       b->cpu, apic, b->bank, b->block, b->address, hi, lo);
@@ -474,41 +474,6 @@ static int setup_APIC_mce_threshold(int reserved, int new)
 	return reserved;
 }
 
-static int setup_APIC_deferred_error(int reserved, int new)
-{
-	if (reserved < 0 && !setup_APIC_eilvt(new, DEFERRED_ERROR_VECTOR,
-					      APIC_EILVT_MSG_FIX, 0))
-		return new;
-
-	return reserved;
-}
-
-static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c)
-{
-	u32 low = 0, high = 0;
-	int def_offset = -1, def_new;
-
-	if (rdmsr_safe(MSR_CU_DEF_ERR, &low, &high))
-		return;
-
-	def_new = (low & MASK_DEF_LVTOFF) >> 4;
-	if (!(low & MASK_DEF_LVTOFF)) {
-		pr_err(FW_BUG "Your BIOS is not setting up LVT offset 0x2 for deferred error IRQs correctly.\n");
-		def_new = DEF_LVT_OFF;
-		low = (low & ~MASK_DEF_LVTOFF) | (DEF_LVT_OFF << 4);
-	}
-
-	def_offset = setup_APIC_deferred_error(def_offset, def_new);
-	if ((def_offset == def_new) &&
-	    (deferred_error_int_vector != amd_deferred_error_interrupt))
-		deferred_error_int_vector = amd_deferred_error_interrupt;
-
-	if (!mce_flags.smca)
-		low = (low & ~MASK_DEF_INT_TYPE) | DEF_INT_TYPE_APIC;
-
-	wrmsr(MSR_CU_DEF_ERR, low, high);
-}
-
 static u32 smca_get_block_address(unsigned int bank, unsigned int block,
 				  unsigned int cpu)
 {
@@ -551,7 +516,6 @@ prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
 			int offset, u32 misc_high)
 {
 	unsigned int cpu = smp_processor_id();
-	u32 smca_low, smca_high;
 	struct threshold_block b;
 	int new;
 
@@ -571,18 +535,10 @@ prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
 	__set_bit(bank, this_cpu_ptr(&mce_amd_data)->thr_intr_banks);
 	b.interrupt_enable = 1;
 
-	if (!mce_flags.smca) {
-		new = (misc_high & MASK_LVTOFF_HI) >> 20;
-		goto set_offset;
-	}
-
-	/* Gather LVT offset for thresholding: */
-	if (rdmsr_safe(MSR_CU_DEF_ERR, &smca_low, &smca_high))
-		goto out;
-
-	new = (smca_low & SMCA_THR_LVT_OFF) >> 12;
+	if (mce_flags.smca)
+		goto done;
 
-set_offset:
+	new = (misc_high & MASK_LVTOFF_HI) >> 20;
 	offset = setup_APIC_mce_threshold(offset, new);
 	if (offset == new)
 		thresholding_irq_en = true;
@@ -590,7 +546,6 @@ prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
 done:
 	mce_threshold_block_init(&b, offset);
 
-out:
 	return offset;
 }
 
@@ -659,6 +614,32 @@ static void disable_err_thresholding(struct cpuinfo_x86 *c, unsigned int bank)
 		wrmsrl(MSR_K7_HWCR, hwcr);
 }
 
+/*
+ * Enable the APIC LVT interrupt vectors once per-CPU. This should be done before hardware is
+ * ready to send interrupts.
+ *
+ * Individual error sources are enabled later during per-bank init.
+ */
+static void smca_enable_interrupt_vectors(void)
+{
+	struct mce_amd_cpu_data *data = this_cpu_ptr(&mce_amd_data);
+	u64 mca_intr_cfg, offset;
+
+	if (!mce_flags.smca || !mce_flags.succor)
+		return;
+
+	if (rdmsrl_safe(MSR_CU_DEF_ERR, &mca_intr_cfg))
+		return;
+
+	offset = (mca_intr_cfg & SMCA_THR_LVT_OFF) >> 12;
+	if (!setup_APIC_eilvt(offset, THRESHOLD_APIC_VECTOR, APIC_EILVT_MSG_FIX, 0))
+		data->thr_intr_en = true;
+
+	offset = (mca_intr_cfg & MASK_DEF_LVTOFF) >> 4;
+	if (!setup_APIC_eilvt(offset, DEFERRED_ERROR_VECTOR, APIC_EILVT_MSG_FIX, 0))
+		data->dfr_intr_en = true;
+}
+
 static void amd_apply_quirks(struct cpuinfo_x86 *c)
 {
 	struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
@@ -690,11 +671,16 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 
 	amd_apply_quirks(c);
 	mce_flags.amd_threshold	 = 1;
+	smca_enable_interrupt_vectors();
 
 	for (bank = 0; bank < this_cpu_read(mce_num_banks); ++bank) {
-		if (mce_flags.smca)
+		if (mce_flags.smca) {
 			smca_configure(bank, cpu);
 
+			if (!this_cpu_ptr(&mce_amd_data)->thr_intr_en)
+				continue;
+		}
+
 		disable_err_thresholding(c, bank);
 
 		for (block = 0; block < NR_BLOCKS; ++block) {
@@ -715,9 +701,6 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 			offset = prepare_threshold_block(bank, block, address, offset, high);
 		}
 	}
-
-	if (mce_flags.succor)
-		deferred_error_interrupt_enable(c);
 }
 
 void mce_smca_cpu_init(void)

-- 
2.43.0

RE: [PATCH v2 14/16] x86/mce/amd: Enable interrupt vectors once per-CPU on SMCA systems

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> behavior. The deferred error interrupt is technically advertised by the SUCCOR
> feature. However, this was first made available on SMCA systems.
> Therefore, only set up the deferred error interrupt on SMCA systems and
> simplify the code.

Does this description imply that:

    if mce_flags.succor = true, then mce_flags.smca must also be true.

> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> -static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c) -{
> -	u32 low = 0, high = 0;
> -	int def_offset = -1, def_new;
> -
> -	if (rdmsr_safe(MSR_CU_DEF_ERR, &low, &high))
> -		return;
> -
> -	def_new = (low & MASK_DEF_LVTOFF) >> 4;
> -	if (!(low & MASK_DEF_LVTOFF)) {
> -		pr_err(FW_BUG "Your BIOS is not setting up LVT offset 0x2 for
> deferred error IRQs correctly.\n");
> -		def_new = DEF_LVT_OFF;
> -		low = (low & ~MASK_DEF_LVTOFF) | (DEF_LVT_OFF << 4);
> -	}

This code is missing from this patch.
Expected?

> -
> -	def_offset = setup_APIC_deferred_error(def_offset, def_new);
> -	if ((def_offset == def_new) &&
> -	    (deferred_error_int_vector != amd_deferred_error_interrupt))
> -		deferred_error_int_vector = amd_deferred_error_interrupt;
> -
> -	if (!mce_flags.smca)
> -		low = (low & ~MASK_DEF_INT_TYPE) | DEF_INT_TYPE_APIC;
> -
> -	wrmsr(MSR_CU_DEF_ERR, low, high);
> -}
> -

This code is missing from this patch.
Expected?

>  static u32 smca_get_block_address(unsigned int bank, unsigned int block,
>  				  unsigned int cpu)
>  {
> @@ -551,7 +516,6 @@ prepare_threshold_block(unsigned int bank, unsigned
> int block, u32 addr,
>  			int offset, u32 misc_high)
>  {
>  	unsigned int cpu = smp_processor_id();
> -	u32 smca_low, smca_high;
>  	struct threshold_block b;
>  	int new;
> 
> @@ -571,18 +535,10 @@ prepare_threshold_block(unsigned int bank,
> unsigned int block, u32 addr,
>  	__set_bit(bank, this_cpu_ptr(&mce_amd_data)->thr_intr_banks);
>  	b.interrupt_enable = 1;
> 
> -	if (!mce_flags.smca) {
> -		new = (misc_high & MASK_LVTOFF_HI) >> 20;
> -		goto set_offset;
> -	}
> -
> -	/* Gather LVT offset for thresholding: */
> -	if (rdmsr_safe(MSR_CU_DEF_ERR, &smca_low, &smca_high))
> -		goto out;
> -
> -	new = (smca_low & SMCA_THR_LVT_OFF) >> 12;
> +	if (mce_flags.smca)
> +		goto done;
> 
> -set_offset:
> +	new = (misc_high & MASK_LVTOFF_HI) >> 20;
>  	offset = setup_APIC_mce_threshold(offset, new);
>  	if (offset == new)
>  		thresholding_irq_en = true;
> @@ -590,7 +546,6 @@ prepare_threshold_block(unsigned int bank, unsigned
> int block, u32 addr,
>  done:
>  	mce_threshold_block_init(&b, offset);
> 
> -out:
>  	return offset;
>  }
> 
> @@ -659,6 +614,32 @@ static void disable_err_thresholding(struct
> cpuinfo_x86 *c, unsigned int bank)
>  		wrmsrl(MSR_K7_HWCR, hwcr);
>  }
> 
> +/*
> + * Enable the APIC LVT interrupt vectors once per-CPU. This should be
> +done before hardware is
> + * ready to send interrupts.
> + *
> + * Individual error sources are enabled later during per-bank init.
> + */
> +static void smca_enable_interrupt_vectors(void)
> +{
> +	struct mce_amd_cpu_data *data = this_cpu_ptr(&mce_amd_data);
> +	u64 mca_intr_cfg, offset;
> +
> +	if (!mce_flags.smca || !mce_flags.succor)
> +		return;
> +

In the old code, the deferred IRQ setup just depends on mce_flags.succor,
But now it depends on: mce_flags.smca && mce_flags.succor.
Is this expected?

> +	if (rdmsrl_safe(MSR_CU_DEF_ERR, &mca_intr_cfg))
> +		return;
> +
> +	offset = (mca_intr_cfg & SMCA_THR_LVT_OFF) >> 12;
> +	if (!setup_APIC_eilvt(offset, THRESHOLD_APIC_VECTOR,
> APIC_EILVT_MSG_FIX, 0))
> +		data->thr_intr_en = true;
> +
> +	offset = (mca_intr_cfg & MASK_DEF_LVTOFF) >> 4;
> +	if (!setup_APIC_eilvt(offset, DEFERRED_ERROR_VECTOR,
> APIC_EILVT_MSG_FIX, 0))
> +		data->dfr_intr_en = true;
> +}
> +
>  static void amd_apply_quirks(struct cpuinfo_x86 *c)  {
>  	struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array); @@
> -690,11 +671,16 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
> 
>  	amd_apply_quirks(c);
>  	mce_flags.amd_threshold	 = 1;
> +	smca_enable_interrupt_vectors();
> 
>  	for (bank = 0; bank < this_cpu_read(mce_num_banks); ++bank) {
> -		if (mce_flags.smca)
> +		if (mce_flags.smca) {
>  			smca_configure(bank, cpu);
> 
> +			if (!this_cpu_ptr(&mce_amd_data)->thr_intr_en)
> +				continue;
> +		}
> +
>  		disable_err_thresholding(c, bank);
> 
>  		for (block = 0; block < NR_BLOCKS; ++block) { @@ -715,9
> +701,6 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
>  			offset = prepare_threshold_block(bank, block,
> address, offset, high);
>  		}
>  	}
> -
> -	if (mce_flags.succor)
> -		deferred_error_interrupt_enable(c);

The old code to set up deferred error IRQ just depends on mce_flags.succor,
> [...]

Re: [PATCH v2 14/16] x86/mce/amd: Enable interrupt vectors once per-CPU on SMCA systems

[Yazen Ghannam]

On Tue, Feb 18, 2025 at 08:23:22AM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > [...]
> > behavior. The deferred error interrupt is technically advertised by the SUCCOR
> > feature. However, this was first made available on SMCA systems.
> > Therefore, only set up the deferred error interrupt on SMCA systems and
> > simplify the code.
> 
> Does this description imply that:
> 
>     if mce_flags.succor = true, then mce_flags.smca must also be true.
> 

No, they are independent features. However, in practice (on production
systems) SMCA systems also support SUCCOR.

> > 
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > [...]
> > -static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c) -{
> > -	u32 low = 0, high = 0;
> > -	int def_offset = -1, def_new;
> > -
> > -	if (rdmsr_safe(MSR_CU_DEF_ERR, &low, &high))
> > -		return;
> > -
> > -	def_new = (low & MASK_DEF_LVTOFF) >> 4;
> > -	if (!(low & MASK_DEF_LVTOFF)) {
> > -		pr_err(FW_BUG "Your BIOS is not setting up LVT offset 0x2 for
> > deferred error IRQs correctly.\n");
> > -		def_new = DEF_LVT_OFF;
> > -		low = (low & ~MASK_DEF_LVTOFF) | (DEF_LVT_OFF << 4);
> > -	}
> 
> This code is missing from this patch.
> Expected?
> 

Yes, this old code is replaced by smca_enable_interrupt_vectors().

> > -
> > -	def_offset = setup_APIC_deferred_error(def_offset, def_new);
> > -	if ((def_offset == def_new) &&
> > -	    (deferred_error_int_vector != amd_deferred_error_interrupt))
> > -		deferred_error_int_vector = amd_deferred_error_interrupt;
> > -
> > -	if (!mce_flags.smca)
> > -		low = (low & ~MASK_DEF_INT_TYPE) | DEF_INT_TYPE_APIC;
> > -
> > -	wrmsr(MSR_CU_DEF_ERR, low, high);
> > -}
> > -
> 
> This code is missing from this patch.
> Expected?
> 

Yes, same as above.

> >  static u32 smca_get_block_address(unsigned int bank, unsigned int block,
> >  				  unsigned int cpu)
> >  {
> > @@ -551,7 +516,6 @@ prepare_threshold_block(unsigned int bank, unsigned
> > int block, u32 addr,
> >  			int offset, u32 misc_high)
> >  {
> >  	unsigned int cpu = smp_processor_id();
> > -	u32 smca_low, smca_high;
> >  	struct threshold_block b;
> >  	int new;
> > 
> > @@ -571,18 +535,10 @@ prepare_threshold_block(unsigned int bank,
> > unsigned int block, u32 addr,
> >  	__set_bit(bank, this_cpu_ptr(&mce_amd_data)->thr_intr_banks);
> >  	b.interrupt_enable = 1;
> > 
> > -	if (!mce_flags.smca) {
> > -		new = (misc_high & MASK_LVTOFF_HI) >> 20;
> > -		goto set_offset;
> > -	}
> > -
> > -	/* Gather LVT offset for thresholding: */
> > -	if (rdmsr_safe(MSR_CU_DEF_ERR, &smca_low, &smca_high))
> > -		goto out;
> > -
> > -	new = (smca_low & SMCA_THR_LVT_OFF) >> 12;
> > +	if (mce_flags.smca)
> > +		goto done;
> > 
> > -set_offset:
> > +	new = (misc_high & MASK_LVTOFF_HI) >> 20;
> >  	offset = setup_APIC_mce_threshold(offset, new);
> >  	if (offset == new)
> >  		thresholding_irq_en = true;
> > @@ -590,7 +546,6 @@ prepare_threshold_block(unsigned int bank, unsigned
> > int block, u32 addr,
> >  done:
> >  	mce_threshold_block_init(&b, offset);
> > 
> > -out:
> >  	return offset;
> >  }
> > 
> > @@ -659,6 +614,32 @@ static void disable_err_thresholding(struct
> > cpuinfo_x86 *c, unsigned int bank)
> >  		wrmsrl(MSR_K7_HWCR, hwcr);
> >  }
> > 
> > +/*
> > + * Enable the APIC LVT interrupt vectors once per-CPU. This should be
> > +done before hardware is
> > + * ready to send interrupts.
> > + *
> > + * Individual error sources are enabled later during per-bank init.
> > + */
> > +static void smca_enable_interrupt_vectors(void)
> > +{
> > +	struct mce_amd_cpu_data *data = this_cpu_ptr(&mce_amd_data);
> > +	u64 mca_intr_cfg, offset;
> > +
> > +	if (!mce_flags.smca || !mce_flags.succor)
> > +		return;
> > +
> 
> In the old code, the deferred IRQ setup just depends on mce_flags.succor,
> But now it depends on: mce_flags.smca && mce_flags.succor.
> Is this expected?
> 

Yes, this is described in the quoted part of the commit message above.

> > +	if (rdmsrl_safe(MSR_CU_DEF_ERR, &mca_intr_cfg))
> > +		return;
> > +
> > +	offset = (mca_intr_cfg & SMCA_THR_LVT_OFF) >> 12;
> > +	if (!setup_APIC_eilvt(offset, THRESHOLD_APIC_VECTOR,
> > APIC_EILVT_MSG_FIX, 0))
> > +		data->thr_intr_en = true;
> > +
> > +	offset = (mca_intr_cfg & MASK_DEF_LVTOFF) >> 4;
> > +	if (!setup_APIC_eilvt(offset, DEFERRED_ERROR_VECTOR,
> > APIC_EILVT_MSG_FIX, 0))
> > +		data->dfr_intr_en = true;
> > +}
> > +
> >  static void amd_apply_quirks(struct cpuinfo_x86 *c)  {
> >  	struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array); @@
> > -690,11 +671,16 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
> > 
> >  	amd_apply_quirks(c);
> >  	mce_flags.amd_threshold	 = 1;
> > +	smca_enable_interrupt_vectors();
> > 
> >  	for (bank = 0; bank < this_cpu_read(mce_num_banks); ++bank) {
> > -		if (mce_flags.smca)
> > +		if (mce_flags.smca) {
> >  			smca_configure(bank, cpu);
> > 
> > +			if (!this_cpu_ptr(&mce_amd_data)->thr_intr_en)
> > +				continue;
> > +		}
> > +
> >  		disable_err_thresholding(c, bank);
> > 
> >  		for (block = 0; block < NR_BLOCKS; ++block) { @@ -715,9
> > +701,6 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
> >  			offset = prepare_threshold_block(bank, block,
> > address, offset, high);
> >  		}
> >  	}
> > -
> > -	if (mce_flags.succor)
> > -		deferred_error_interrupt_enable(c);
> 
> The old code to set up deferred error IRQ just depends on mce_flags.succor,
> > [...]

Correct, this is the same reasoning as above.

Thanks,
Yazen

[PATCH v2 15/16] x86/mce/amd: Support SMCA Corrected Error Interrupt

[Yazen Ghannam]

AMD systems optionally support MCA thresholding which provides the
ability for hardware to send an interrupt when a set error threshold is
reached. This feature counts errors of all severities, but it is
commonly used to report correctable errors with an interrupt rather than
polling.

Scalable MCA systems allow the Platform to take control of this feature.
In this case, the OS will not see the feature configuration and control
bits in the MCA_MISC* registers. The OS will not receive the MCA
thresholding interrupt, and it will need to poll for correctable errors.

A "corrected error interrupt" will be available on Scalable MCA systems.
This will be used in the same configuration where the Platform controls
MCA thresholding. However, the Platform will now be able to send the
MCA thresholding interrupt to the OS.

Check for the feature bit in the MCA_CONFIG register and confirm that
the MCA thresholding interrupt handler is already enabled. If successful,
set the feature enable bit in the MCA_CONFIG register to indicate to the
Platform that the OS is ready for the interrupt.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    Link:
    https://lore.kernel.org/r/20240523155641.2805411-10-yazen.ghannam@amd.com
    
    v1->v2:
    * Use new per-CPU struct.

 arch/x86/kernel/cpu/mce/amd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 75a48f484815..404e0c38f9d8 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -306,6 +306,11 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
 			high |= BIT(5);
 		}
 
+		if ((low & BIT(10)) && data->thr_intr_en) {
+			__set_bit(bank, data->thr_intr_banks);
+			high |= BIT(8);
+		}
+
 		this_cpu_ptr(mce_banks_array)[bank].lsb_in_status = !!(low & BIT(8));
 
 		wrmsr(smca_config, low, high);

-- 
2.43.0

Re: [PATCH v2 15/16] x86/mce/amd: Support SMCA Corrected Error Interrupt

[Luck, Tony]

On Thu, Feb 13, 2025 at 04:46:04PM +0000, Yazen Ghannam wrote:
> @@ -306,6 +306,11 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
>  			high |= BIT(5);
>  		}
>  
> +		if ((low & BIT(10)) && data->thr_intr_en) {
> +			__set_bit(bank, data->thr_intr_banks);
> +			high |= BIT(8);

Maybe someday add some #define names for these BITs?

> +		}
> +
>  		this_cpu_ptr(mce_banks_array)[bank].lsb_in_status = !!(low & BIT(8));
>  
>  		wrmsr(smca_config, low, high);

-Tony

Re: [PATCH v2 15/16] x86/mce/amd: Support SMCA Corrected Error Interrupt

[Yazen Ghannam]

On Thu, Feb 13, 2025 at 02:34:59PM -0800, Luck, Tony wrote:
> On Thu, Feb 13, 2025 at 04:46:04PM +0000, Yazen Ghannam wrote:
> > @@ -306,6 +306,11 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
> >  			high |= BIT(5);
> >  		}
> >  
> > +		if ((low & BIT(10)) && data->thr_intr_en) {
> > +			__set_bit(bank, data->thr_intr_banks);
> > +			high |= BIT(8);
> 
> Maybe someday add some #define names for these BITs?
> 

Yep, I have something like that:
https://lore.kernel.org/r/20240404151359.47970-6-yazen.ghannam@amd.com
https://lore.kernel.org/r/20240404151359.47970-13-yazen.ghannam@amd.com

These sets keep growing, so I'm trying to do better about breaking
things up and prioritizing. :)

Thanks,
Yazen

RE: [PATCH v2 15/16] x86/mce/amd: Support SMCA Corrected Error Interrupt

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> [...]
> --- a/arch/x86/kernel/cpu/mce/amd.c
> +++ b/arch/x86/kernel/cpu/mce/amd.c
> @@ -306,6 +306,11 @@ static void smca_configure(unsigned int bank,
> unsigned int cpu)
>  			high |= BIT(5);
>  		}
> 

Do you think it would be better to have some comments on the thresholding 
interrupt here (as the bits 10/8 look like magic numbers), similar to the 
comments above for the deferred interrupt?

> +		if ((low & BIT(10)) && data->thr_intr_en) {
> +			__set_bit(bank, data->thr_intr_banks);
> +			high |= BIT(8);
> +		}
> +
> [...]

Re: [PATCH v2 15/16] x86/mce/amd: Support SMCA Corrected Error Interrupt

[Yazen Ghannam]

On Tue, Feb 18, 2025 at 01:27:04PM +0000, Zhuo, Qiuxu wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > [...]
> > --- a/arch/x86/kernel/cpu/mce/amd.c
> > +++ b/arch/x86/kernel/cpu/mce/amd.c
> > @@ -306,6 +306,11 @@ static void smca_configure(unsigned int bank,
> > unsigned int cpu)
> >  			high |= BIT(5);
> >  		}
> > 
> 
> Do you think it would be better to have some comments on the thresholding 
> interrupt here (as the bits 10/8 look like magic numbers), similar to the 
> comments above for the deferred interrupt?
> 
> > +		if ((low & BIT(10)) && data->thr_intr_en) {
> > +			__set_bit(bank, data->thr_intr_banks);
> > +			high |= BIT(8);
> > +		}
> > +
> > [...]

Yes, I'd like to convert these to defines. I left that idea, for now, to
simplify the changes.

Thanks,
Yazen

[PATCH v2 16/16] x86/mce: Handle AMD threshold interrupt storms

[Yazen Ghannam]

From: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>

Extend the logic of handling CMCI storms to AMD threshold interrupts.

Rely on the similar approach as of Intel's CMCI to mitigate storms per
CPU and per bank. But, unlike CMCI, do not set thresholds and reduce
interrupt rate on a storm. Rather, disable the interrupt on the
corresponding CPU and bank. Re-enable back the interrupts if enough
consecutive polls of the bank show no corrected errors (30, as
programmed by Intel).

Turning off the threshold interrupts would be a better solution on AMD
systems as other error severities will still be handled even if the
threshold interrupts are disabled.

[Tony: Small tweak because mce_handle_storm() isn't a pointer now]
[Yazen: Rebase and simplify]

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---

Notes:
    Link:
    https://lore.kernel.org/r/20230616182744.17632-4-tony.luck@intel.com
    
    v1->v2:
    * New in v2, but based on older patch.
    * Rebased on current set and simplified.
    * Kept old tags.

 arch/x86/kernel/cpu/mce/amd.c       | 18 ++++++++++++++++++
 arch/x86/kernel/cpu/mce/internal.h  |  2 ++
 arch/x86/kernel/cpu/mce/threshold.c |  3 +++
 3 files changed, 23 insertions(+)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 404e0c38f9d8..a2d02f0c2153 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1218,3 +1218,21 @@ void mce_threshold_create_device(unsigned int cpu)
 		mce_threshold_vector = amd_threshold_interrupt;
 	return;
 }
+
+void mce_amd_handle_storm(unsigned int bank, bool on)
+{
+	struct threshold_bank **thr_banks = this_cpu_read(threshold_banks);
+	struct threshold_block *block, *tmp;
+	struct thresh_restart tr;
+
+	if (!thr_banks || !thr_banks[bank])
+		return;
+
+	memset(&tr, 0, sizeof(tr));
+
+	list_for_each_entry_safe(block, tmp, &thr_banks[bank]->miscj, miscj) {
+		tr.b = block;
+		tr.b->interrupt_enable = on;
+		threshold_restart_bank(&tr);
+	}
+}
diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
index fe519acfafcf..9d771db2bcae 100644
--- a/arch/x86/kernel/cpu/mce/internal.h
+++ b/arch/x86/kernel/cpu/mce/internal.h
@@ -266,6 +266,7 @@ void mce_prep_record_per_cpu(unsigned int cpu, struct mce *m);
 
 #ifdef CONFIG_X86_MCE_AMD
 void mce_threshold_create_device(unsigned int cpu);
+void mce_amd_handle_storm(unsigned int bank, bool on);
 extern bool amd_filter_mce(struct mce *m);
 bool amd_mce_usable_address(struct mce *m);
 void amd_reset_thr_limit(unsigned int bank);
@@ -297,6 +298,7 @@ static __always_inline void smca_extract_err_addr(struct mce *m)
 void mce_smca_cpu_init(void);
 #else
 static inline void mce_threshold_create_device(unsigned int cpu)	{ }
+static inline void mce_amd_handle_storm(unsigned int bank, bool on)	{ }
 static inline bool amd_filter_mce(struct mce *m) { return false; }
 static inline bool amd_mce_usable_address(struct mce *m) { return false; }
 static inline void amd_reset_thr_limit(unsigned int bank) { }
diff --git a/arch/x86/kernel/cpu/mce/threshold.c b/arch/x86/kernel/cpu/mce/threshold.c
index f4a007616468..45144598ec74 100644
--- a/arch/x86/kernel/cpu/mce/threshold.c
+++ b/arch/x86/kernel/cpu/mce/threshold.c
@@ -63,6 +63,9 @@ static void mce_handle_storm(unsigned int bank, bool on)
 	case X86_VENDOR_INTEL:
 		mce_intel_handle_storm(bank, on);
 		break;
+	case X86_VENDOR_AMD:
+		mce_amd_handle_storm(bank, on);
+		break;
 	}
 }
 

-- 
2.43.0

RE: [PATCH v2 16/16] x86/mce: Handle AMD threshold interrupt storms

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Sent: Friday, February 14, 2025 12:46 AM
> To: x86@kernel.org; Luck, Tony <tony.luck@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org;
> Smita.KoralahalliChannabasappa@amd.com; Yazen Ghannam
> <yazen.ghannam@amd.com>
> Subject: [PATCH v2 16/16] x86/mce: Handle AMD threshold interrupt storms
> 
> From: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> 
> Extend the logic of handling CMCI storms to AMD threshold interrupts.
> 
> Rely on the similar approach as of Intel's CMCI to mitigate storms per CPU
> and per bank. But, unlike CMCI, do not set thresholds and reduce interrupt
> rate on a storm. Rather, disable the interrupt on the corresponding CPU and
> bank. Re-enable back the interrupts if enough consecutive polls of the bank
> show no corrected errors (30, as programmed by Intel).
> 
> Turning off the threshold interrupts would be a better solution on AMD
> systems as other error severities will still be handled even if the threshold
> interrupts are disabled.
> 
> [Tony: Small tweak because mce_handle_storm() isn't a pointer now]
> [Yazen: Rebase and simplify]
> 
> Signed-off-by: Smita Koralahalli
> <Smita.KoralahalliChannabasappa@amd.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>

LGTM.

    Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

Re: [PATCH v2 00/16] AMD MCA interrupts rework

[Luck, Tony]

On Thu, Feb 13, 2025 at 04:45:49PM +0000, Yazen Ghannam wrote:
> Hi all,
> 
> This set unifies the AMD MCA interrupt handlers with common MCA code.
> The goal is to avoid duplicating functionality like reading and clearing
> MCA banks.
> 
> Based on feedback, this revision also include changes to the MCA init
> flow.

Apart from the nits I posed againt parts 5 & 15. LGTM.

Tested-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>

-Tony