Re: [PATCH v3] vmcoreinfo: Track and log recoverable hardware errors

[Breno Leitao]

Hello Shuai,

Thanks for the review,

On Wed, Jul 30, 2025 at 09:50:39PM +0800, Shuai Xue wrote:
> 在 2025/7/30 21:11, Breno Leitao 写道:
> >
> > @@ -1690,6 +1691,9 @@ noinstr void do_machine_check(struct pt_regs *regs)
> >     }
> > 
> >   out:
> > +   /* Given it didn't panic, mark it as recoverable */
> > +   hwerr_log_error_type(HWERR_RECOV_MCE);
> > +
> 
> Indentation: needs tab alignment.

No sure I got what it the alignment process. The code seems to be
properly aligned, and using tabs. Could you please clarify what is the
current problem?

> The current placement only logs errors that reach the out: label. Errors
> that go to `clear` lable won't be recorded. Would it be better to log at
> the beginning of do_machine_check() to capture all recoverable MCEs?

This is a good point, and I've thought about it. I understand we don't
want to track the code flow that goes to the clear: label, since it is
wrongly triggered by some CPUs, and it is not a real MCE.
That is described in commit 8ca97812c3c830 ("x86/mce: Work around an
erratum on fast string copy instructions").

At the same time, the current block of MCEs are not being properly
tracked, since they return earlier in do_machine_check(). Here is
a quick 

   void do_machine_check(struct pt_regs *regs)
   ...
          if (unlikely(mce_flags.p5))
                  return pentium_machine_check(regs);
          else if (unlikely(mce_flags.winchip))
                  return winchip_machine_check(regs);
          else if (unlikely(!mca_cfg.initialized))
                  return unexpected_machine_check(regs);

         if (mce_flags.skx_repmov_quirk && quirk_skylake_repmov())
                  goto clear;

          /* Code doesn't exit anymore unless through out: */

    }

Given that instrumentation is not enabled when those return are called,
we cannot easily call hwerr_log_error_type() before the returns.

An option is just to ignore those, given they are unlikely. Another
option is to call hwerr_log_error_type() inside those functions above,
so, we do not miss these counters in case do_machine_check() returns
earlier. 


--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1481,6 +1481,7 @@ static void queue_task_work(struct mce_hw_err *err, char 
*msg, void (*func)(stru
 static noinstr void unexpected_machine_check(struct pt_regs *regs)
 {
        instrumentation_begin();
+       hwerr_log_error_type(HWERR_RECOV_MCE);
        pr_err("CPU#%d: Unexpected int18 (Machine Check)\n",
               smp_processor_id());
        instrumentation_end();
diff --git a/arch/x86/kernel/cpu/mce/p5.c b/arch/x86/kernel/cpu/mce/p5.c
index 2272ad53fc339..a627ed10b752d 100644
--- a/arch/x86/kernel/cpu/mce/p5.c
+++ b/arch/x86/kernel/cpu/mce/p5.c
@@ -26,6 +26,7 @@ noinstr void pentium_machine_check(struct pt_regs *regs)
        u32 loaddr, hi, lotype;

        instrumentation_begin();
+       hwerr_log_error_type(HWERR_RECOV_MCE);
        rdmsr(MSR_IA32_P5_MC_ADDR, loaddr, hi);
        rdmsr(MSR_IA32_P5_MC_TYPE, lotype, hi);

diff --git a/arch/x86/kernel/cpu/mce/winchip.c 
b/arch/x86/kernel/cpu/mce/winchip.c
index 6c99f29419090..b7862bf5ba870 100644
--- a/arch/x86/kernel/cpu/mce/winchip.c
+++ b/arch/x86/kernel/cpu/mce/winchip.c
@@ -20,6 +20,7 @@
 noinstr void winchip_machine_check(struct pt_regs *regs)
 {
        instrumentation_begin();
+       hwerr_log_error_type(HWERR_RECOV_MCE);
        pr_emerg("CPU0: Machine Check Exception.\n");
        add_taint(TAINT_MACHINE_CHECK, LOCKDEP_NOW_UNRELIABLE);
        instrumentation_end();