[PATCH v3 2/2] x86/mce: Reset MCA_SYND1/2 and kflags between bank scans

[PATCH v3 2/2] x86/mce: Reset MCA_SYND1/2 and kflags between bank scans

[OptoCloud]

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, ADDR and
SYND. Three more fields are written conditionally inside the loop and
never cleared again, so a later bank inherits them.

mce_read_aux() writes err->vendor.amd.synd1/synd2 only on SMCA, and
then only when MCI_STATUS_SYNDV is set, so a bank without SYNDV is
printed with the supplemental syndromes of an earlier bank in the
same scan.

A stale m->kflags changes what happens to the next bank:

  - smca_should_log_poll_error() sets MCE_CHECK_DFR_REGS when an
    error was taken from MCA_DESTAT rather than MCA_STATUS. A later
    bank in the same poll then reads MCx_DEADDR instead of MCA_ADDR
    if it has ADDRV, and amd_clear_bank() returns before writing 0 to
    MCA_STATUS, so MCI_STATUS_VAL stays set and the bank is logged a
    second time on the next poll.

  - mce_default_notifier() prints a record only if m->kflags is empty
    or print_all is set, and the record reaches the gen pool as it
    stands. A later bank that inherits MCE_CHECK_DFR_REGS is logged
    with a non-zero m->kflags and is not printed by the notifier
    chain.

Factor the per-bank clearing into mce_clear_hw_err_fields() and call
it from both loops.

Found by code inspection; not reproduced on hardware.

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 v2:
    - Squashed the MCA_SYND1/2 reset and the ->kflags reset into one
      patch, with the ->kflags reset in mce_clear_hw_err_fields() (Yazen)
    - m->kflags is now cleared in __mc_scan_banks() as well, which it was
      not in v2
    - Subject prefix x86/mce/amd: -> x86/mce:
    
    Changes since v1:
    - Dropped Cc: stable (Yazen)
    - Added mce_clear_hw_err_fields() so the resets are not repeated in
      machine_check_poll() and __mc_scan_banks() (Yazen)
    - Reset the whole m->kflags field instead of just MCE_CHECK_DFR_REGS
      (Yazen)

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

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 16183fa4ddc7..e906c3f8e888 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -653,6 +653,22 @@ 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.amd.synd1 = 0;
+	err->vendor.amd.synd2 = 0;
+}
+
 /*
  * Read ADDR and MISC registers.
  */
@@ -806,9 +822,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;
-		m->synd = 0;
+		mce_clear_hw_err_fields(&err);
 		m->bank = i;
 
 		barrier();
@@ -1346,9 +1360,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;
-		m->synd = 0;
+		mce_clear_hw_err_fields(err);
 		m->bank = i;
 
 		m->status = mce_rdmsrq(mca_msr_reg(i, MCA_STATUS));
-- 
2.55.0

Cheers,
Eirik Bøe

Re: [PATCH v3 2/2] x86/mce: Reset MCA_SYND1/2 and kflags between bank scans

[Yazen Ghannam]

On Mon, Sep 21, 2026 at 01:38:12PM +0000, OptoCloud 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, ADDR and
> SYND. Three more fields are written conditionally inside the loop and
> never cleared again, so a later bank inherits them.
> 
> mce_read_aux() writes err->vendor.amd.synd1/synd2 only on SMCA, and
> then only when MCI_STATUS_SYNDV is set, so a bank without SYNDV is
> printed with the supplemental syndromes of an earlier bank in the
> same scan.
> 
> A stale m->kflags changes what happens to the next bank:
> 
>   - smca_should_log_poll_error() sets MCE_CHECK_DFR_REGS when an
>     error was taken from MCA_DESTAT rather than MCA_STATUS. A later
>     bank in the same poll then reads MCx_DEADDR instead of MCA_ADDR
>     if it has ADDRV, and amd_clear_bank() returns before writing 0 to
>     MCA_STATUS, so MCI_STATUS_VAL stays set and the bank is logged a
>     second time on the next poll.
> 
>   - mce_default_notifier() prints a record only if m->kflags is empty
>     or print_all is set, and the record reaches the gen pool as it
>     stands. A later bank that inherits MCE_CHECK_DFR_REGS is logged
>     with a non-zero m->kflags and is not printed by the notifier
>     chain.
> 
> Factor the per-bank clearing into mce_clear_hw_err_fields() and call
> it from both loops.
> 
> Found by code inspection; not reproduced on hardware.
> 
> 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 v2:
>     - Squashed the MCA_SYND1/2 reset and the ->kflags reset into one
>       patch, with the ->kflags reset in mce_clear_hw_err_fields() (Yazen)
>     - m->kflags is now cleared in __mc_scan_banks() as well, which it was
>       not in v2
>     - Subject prefix x86/mce/amd: -> x86/mce:
>     
>     Changes since v1:
>     - Dropped Cc: stable (Yazen)
>     - Added mce_clear_hw_err_fields() so the resets are not repeated in
>       machine_check_poll() and __mc_scan_banks() (Yazen)
>     - Reset the whole m->kflags field instead of just MCE_CHECK_DFR_REGS
>       (Yazen)
> 
>  arch/x86/kernel/cpu/mce/core.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 16183fa4ddc7..e906c3f8e888 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -653,6 +653,22 @@ 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.amd.synd1 = 0;
> +	err->vendor.amd.synd2 = 0;

It seems that the entire union can be reset in one step:
	err->vendor = (union vendor_info){ };

This would be more future-proof if/when new fields or structs are added
to the union.

Also, please align the lines on the '='.

Thanks,
Yazen