[PATCH v4] x86/mce: Reset stale per-bank state between bank scans

[PATCH v4] x86/mce: Reset stale per-bank state between bank scans

[Eirik Bøe]

From: Eirik Bøe <git@optocloud.no>

machine_check_poll() and __mc_scan_banks() reuse a single
struct mce_hw_err across the whole bank scan. The record is zeroed
once before the loop; each iteration then resets only MISC and ADDR.
Four more fields are written conditionally inside the loop and never
cleared again, so a later bank inherits them.

On SMCA, mce_read_aux() writes SYND and synd1/synd2 only when
MCI_STATUS_SYNDV is set, so a bank without SYNDV is printed with the
syndromes of an earlier bank in the same scan. Decoders are expected
to ignore them when SYNDV is clear, so the stale values are cosmetic.

A stale m->kflags has two effects on a later bank:

  - smca_should_log_poll_error() sets MCE_CHECK_DFR_REGS when an
    error was taken from MCA_DESTAT. A later bank with ADDRV then
    reads MCx_DEADDR instead of MCA_ADDR, and amd_clear_bank()
    returns before clearing MCA_STATUS, so that bank is logged a
    second time.

  - mce_default_notifier() prints only when m->kflags is empty, so
    unless print_all is set, a bank that inherits the bit is not
    printed by the notifier chain.

Factor the per-bank clearing into mce_clear_hw_err_fields() and call
it from both loops. Reset the vendor union as a whole so later
additions are covered.

Found by code inspection; not reproduced on hardware.

Fixes: db819d60f672 ("x86/mce: Add support for new MCA_SYND register")
Fixes: d4fca1358ea9 ("x86/MCE/AMD: Add support for new MCA_SYND{1,2} registers")
Fixes: 7cb735d7c0cb ("x86/mce: Unify AMD DFR handler with MCA Polling")
Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Eirik Bøe <git@optocloud.no>
---

Notes (amlog):
    Changes since v3:
    - Dropped v3 1/2; mce_clear_hw_err_fields() covers it, so this is a
      single patch (Yazen)
    - Reset the vendor union in one step instead of synd1/synd2 by name
      (Yazen)
    - Aligned the assignments on '=' (Yazen)
    - Picked up db819d60f672 as a third Fixes: from the dropped patch
    - Dropped Yazen's Reviewed-by; it was given on the dropped patch
    - Rebased onto current master
    
    Changes since v2:
    - Squashed the synd1/2 and ->kflags resets into one patch, with the
      ->kflags reset in the helper (Yazen)
    - ->kflags now cleared in __mc_scan_banks() too
    - Subject prefix x86/mce/amd: -> x86/mce:
    
    Changes since v1:
    - Dropped Cc: stable (Yazen)
    - Added mce_clear_hw_err_fields() (Yazen)
    - Reset all of ->kflags instead of just MCE_CHECK_DFR_REGS (Yazen)

 arch/x86/kernel/cpu/mce/core.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 39f238952e14..6df932c53e8e 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -653,6 +653,21 @@ static struct notifier_block mce_default_nb = {
 	.priority	= MCE_PRIO_LOWEST,
 };
 
+/*
+ * These fields are only filled in conditionally, so clear them before each
+ * bank to stop a bank inheriting the previous bank's values.
+ */
+static noinstr void mce_clear_hw_err_fields(struct mce_hw_err *err)
+{
+	struct mce *m = &err->m;
+
+	m->misc     = 0;
+	m->addr     = 0;
+	m->synd     = 0;
+	m->kflags   = 0;
+	err->vendor = (union vendor_info){ };
+}
+
 /*
  * Read ADDR and MISC registers.
  */
@@ -806,8 +821,7 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		if (!mce_banks[i].ctl || !test_bit(i, *b))
 			continue;
 
-		m->misc = 0;
-		m->addr = 0;
+		mce_clear_hw_err_fields(&err);
 		m->bank = i;
 
 		barrier();
@@ -1345,8 +1359,7 @@ __mc_scan_banks(struct mce_hw_err *err, struct pt_regs *regs,
 		if (!mce_banks[i].ctl)
 			continue;
 
-		m->misc = 0;
-		m->addr = 0;
+		mce_clear_hw_err_fields(err);
 		m->bank = i;
 
 		m->status = mce_rdmsrq(mca_msr_reg(i, MCA_STATUS));

base-commit: 4056b3c9d3c92ea45955641dbafeb9b960e146ef
-- 
2.56.0

Cheers,
Eirik Bøe

RE: [PATCH v4] x86/mce: Reset stale per-bank state between bank scans

[Luck, Tony]

> From: Eirik Bøe <git@optocloud.no>
>
> machine_check_poll() and __mc_scan_banks() reuse a single
> struct mce_hw_err across the whole bank scan. The record is zeroed
> once before the loop; each iteration then resets only MISC and ADDR.
> Four more fields are written conditionally inside the loop and never
> cleared again, so a later bank inherits them.
>
> On SMCA, mce_read_aux() writes SYND and synd1/synd2 only when
> MCI_STATUS_SYNDV is set, so a bank without SYNDV is printed with the
> syndromes of an earlier bank in the same scan. Decoders are expected
> to ignore them when SYNDV is clear, so the stale values are cosmetic.
>
> A stale m->kflags has two effects on a later bank:
>
>   - smca_should_log_poll_error() sets MCE_CHECK_DFR_REGS when an
>     error was taken from MCA_DESTAT. A later bank with ADDRV then
>     reads MCx_DEADDR instead of MCA_ADDR, and amd_clear_bank()
>     returns before clearing MCA_STATUS, so that bank is logged a
>     second time.
>
>   - mce_default_notifier() prints only when m->kflags is empty, so
>     unless print_all is set, a bank that inherits the bit is not
>     printed by the notifier chain.
>
> Factor the per-bank clearing into mce_clear_hw_err_fields() and call
> it from both loops. Reset the vendor union as a whole so later
> additions are covered.
>
> Found by code inspection; not reproduced on hardware.
>
> Fixes: db819d60f672 ("x86/mce: Add support for new MCA_SYND register")
> Fixes: d4fca1358ea9 ("x86/MCE/AMD: Add support for new MCA_SYND{1,2} registers")
> Fixes: 7cb735d7c0cb ("x86/mce: Unify AMD DFR handler with MCA Polling")
> Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Signed-off-by: Eirik Bøe <git@optocloud.no>

LGTM

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

-Tony

Re: [PATCH v4] x86/mce: Reset stale per-bank state between bank scans

[Yazen Ghannam]

On Wed, Oct 14, 2026 at 01:01:39PM +0000, Eirik Bøe wrote:
> From: Eirik Bøe <git@optocloud.no>
> 
> machine_check_poll() and __mc_scan_banks() reuse a single
> struct mce_hw_err across the whole bank scan. The record is zeroed
> once before the loop; each iteration then resets only MISC and ADDR.
> Four more fields are written conditionally inside the loop and never
> cleared again, so a later bank inherits them.
> 
> On SMCA, mce_read_aux() writes SYND and synd1/synd2 only when
> MCI_STATUS_SYNDV is set, so a bank without SYNDV is printed with the
> syndromes of an earlier bank in the same scan. Decoders are expected
> to ignore them when SYNDV is clear, so the stale values are cosmetic.
> 
> A stale m->kflags has two effects on a later bank:
> 
>   - smca_should_log_poll_error() sets MCE_CHECK_DFR_REGS when an
>     error was taken from MCA_DESTAT. A later bank with ADDRV then
>     reads MCx_DEADDR instead of MCA_ADDR, and amd_clear_bank()
>     returns before clearing MCA_STATUS, so that bank is logged a
>     second time.
> 
>   - mce_default_notifier() prints only when m->kflags is empty, so
>     unless print_all is set, a bank that inherits the bit is not
>     printed by the notifier chain.
> 
> Factor the per-bank clearing into mce_clear_hw_err_fields() and call
> it from both loops. Reset the vendor union as a whole so later
> additions are covered.
> 
> Found by code inspection; not reproduced on hardware.
> 
> Fixes: db819d60f672 ("x86/mce: Add support for new MCA_SYND register")
> Fixes: d4fca1358ea9 ("x86/MCE/AMD: Add support for new MCA_SYND{1,2} registers")
> Fixes: 7cb735d7c0cb ("x86/mce: Unify AMD DFR handler with MCA Polling")
> Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Signed-off-by: Eirik Bøe <git@optocloud.no>
> ---
> 

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

Thanks,
Yazen