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

[Yazen Ghannam <yazen.ghannam@amd.com>]

On Thu, Aug 14, 2025 at 12:52:19PM -0700, Luck, Tony wrote:
> 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)
>                 ),
> 

Thanks Tony. I see what you mean.

Do we really need this rule? It is essentially the same as the following
rule:

	        MCESEV(
			PANIC, "In kernel and no restart IP",
		        EXCP, KERNEL, MCGMASK(MCG_STATUS_RIPV, 0)
			),

...since we assume "KERNEL" context if RIPV|EIPV are clear after
checking the CS register.

The message is not as explicit though. 

I did have an earlier idea that we introduce an "UNKNOWN" context for
the !pt_regs case.

We could add the "UNKNOWN" context to the "Neither restart nor error IP"
rule. That way it'll be skipped if we have a "USER" context and then it
should match the one you want.

Also, I just saw this in the Intel SDM:

"For the P6 family processors, if the EIPV flag in the MCG_STATUS MSR is
set, the saved contents of CS and EIP registers are directly associated
with the error that caused the machine-check exception to be generated;
if the flag is clear, the saved instruction pointer may not be associated
with the error (see Section 17.3.1.2, “IA32_MCG_STATUS MSR”)."

But I can't tell if this is true just for P6 or all, because the CS
register isn't referenced again with EIPV.

Thanks,
Yazen