Re: [PATCH v2] x86/mce: Do away with unnecessary context quirks

["Luck, Tony" <tony.luck@intel.com>]

On Thu, Aug 14, 2025 at 03:30:56PM -0400, Yazen Ghannam wrote:
> On Thu, Aug 14, 2025 at 09:54:54AM -0700, Luck, Tony wrote:
> > On Thu, Aug 14, 2025 at 11:48:09AM -0400, Yazen Ghannam wrote:
> > > -/*
> > > - * During IFU recovery Sandy Bridge -EP4S processors set the RIPV and
> > > - * EIPV bits in MCG_STATUS to zero on the affected logical processor (SDM
> > > - * Vol 3B Table 15-20). But this confuses both the code that determines
> > > - * whether the machine check occurred in kernel or user mode, and also
> > > - * the severity assessment code. Pretend that EIPV was set, and take the
> > > - * ip/cs values from the pt_regs that mce_gather_info() ignored earlier.
> > > - */
> > > -static __always_inline void
> > > -quirk_sandybridge_ifu(int bank, struct mce *m, struct pt_regs *regs)
> > > -{
> > > -	if (bank != 0)
> > > -		return;
> > > -	if ((m->mcgstatus & (MCG_STATUS_EIPV|MCG_STATUS_RIPV)) != 0)
> > > -		return;
> > > -	if ((m->status & (MCI_STATUS_OVER|MCI_STATUS_UC|
> > > -		          MCI_STATUS_EN|MCI_STATUS_MISCV|MCI_STATUS_ADDRV|
> > > -			  MCI_STATUS_PCC|MCI_STATUS_S|MCI_STATUS_AR|
> > > -			  MCACOD)) !=
> > > -			 (MCI_STATUS_UC|MCI_STATUS_EN|
> > > -			  MCI_STATUS_MISCV|MCI_STATUS_ADDRV|MCI_STATUS_S|
> > > -			  MCI_STATUS_AR|MCACOD_INSTR))
> > > -		return;
> > > -
> > > -	m->mcgstatus |= MCG_STATUS_EIPV;
> > 
> > I don't think this part of the Sandybridge quirk is covered in your
> > new code. Without EIPV set, the Intel severity table driven code will
> > fail to note this as recoverable.
> > 
> 
> Which severity do you mean? EIPV is not needed to be set in any of them.
> 
> Unless you mean this check:
> 
> 	if (!mc_recoverable(m->mcgstatus))
> 		return IN_KERNEL;
> 
> This would never give the "IN_KERNEL_RECOV" context.
> 
> And this is the only case affected:
> "Action required: data load in error recoverable area of kernel"
> 
> But that is for "Data": MCACOD_DATA
> 
> And the quirk is for "Instructions": MCACOD_INSTR
> 
> So I *think* we're covered.
> 
> I got the impression that setting EIPV in the quirk was to fake our way
> through getting the CS register. It seemed to me that it wasn't directly
> needed for severity grading in the quirky case.
> 
> If we unconditionally get the CS register, then we no longer need to
> fake EIPV. At least, that is my understanding.
 
Yazen,

It's horribly subtle.  On Sandybridge machine check bank 0 is shared by
the two hyperthreads on a core, and machine checks are always broadcast.

For instruction poison consumption both threads on the core see the
machine check and the same IA32_MC0_STATUS value.

IA32_MCG_STATUS is per-thread.

The thread that tried to consume the poison sees: RIPV=0, EIPV=0, MCIP=1

The innocent bystander thread sees: RIPV=1, EIPV=0, MCIP=1

The innocent bystander matches this entry in the severity table:

        MCESEV(
                KEEP, "Action required but unaffected thread is continuable",
                SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR, MCI_UC_SAR|MCI_ADDR),
                MCGMASK(MCG_STATUS_RIPV|MCG_STATUS_EIPV, MCG_STATUS_RIPV)
                ),

I need the consuming thread to match this one:

        MCESEV(
                AR, "Action required: instruction fetch error in a user process",
                SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|MCACOD_INSTR),
                USER
                ),


But the first match nature of the table means that this rule hits
(becauase neither or RIPV or EIPV is set):

        /* Neither return not error IP -- no chance to recover -> PANIC */
        MCESEV(
                PANIC, "Neither restart nor error IP",
                EXCP, MCGMASK(MCG_STATUS_RIPV|MCG_STATUS_EIPV, 0)
                ),

-Tony