RE: [PATCH] x86, mce: Fix machine_check_poll() tests for which errors to log

[Ghannam, Yazen]

> -----Original Message-----
> From: linux-kernel-owner@xxxxxxxxxxxxxxx <linux-kernel-owner@xxxxxxxxxxxxxxx> On Behalf Of Tony Luck
> Sent: Monday, March 11, 2019 1:51 PM
> To: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Tony Luck <tony.luck@xxxxxxxxx>; x86@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Ashok Raj <ashok.raj@xxxxxxxxx>
> Subject: [PATCH] x86, mce: Fix machine_check_poll() tests for which errors to log
> 
> There has been a lurking "TBD" in the machine check poll routine ever
> since it was first split out from the machine check handler. The potential
> issue is that the poll routine may have just begun a read from the STATUS
> register in a machine check bank when the hardware logs an error in that
> bank and signals a machine check. That race used to be pretty small back
> when machine checks were broadcast, but the addition of local machine check
> means that the poll code could continue running and clear the error from the
> bank before the local machine check handler on another CPU gets around to
> reading it.
> 
> Fix the code to be sure to only process errors that need to be processed
> in the poll code, leaving other logged errors alone for the machine check
> handler to find and process.
> 
> Fixes: b79109c3bbcf ("x86, mce: separate correct machine check poller and fatal exception handler")
> Fixes: ed7290d0ee8f ("x86, mce: implement new status bits")
> Reported-by: Ashok Raj <ashok.raj@xxxxxxxxx>
> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/mce/core.c | 42 ++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 6ce290c506d9..806551b381ae 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -712,19 +712,47 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
> 
>  		barrier();
>  		m.status = mce_rdmsrl(msr_ops.status(i));
> +
> +		/* If this entry is not valid, ignore it */
>  		if (!(m.status & MCI_STATUS_VAL))
>  			continue;
> 
>  		/*
> -		 * Uncorrected or signalled events are handled by the exception
> -		 * handler when it is enabled, so don't process those here.
> -		 *
> -		 * TBD do the same check for MCI_STATUS_EN here?
> +		 * If we are logging everything (at CPU online) or this
> +		 * is a corrected error, then we must log it.
>  		 */
> -		if (!(flags & MCP_UC) &&
> -		    (m.status & (mca_cfg.ser ? MCI_STATUS_S : MCI_STATUS_UC)))
> -			continue;
> +		if ((flags & MCP_UC) || (m.status & MCI_STATUS_UC) == 0)
> +			goto log_it;
> +
> +		/*
> +		 * Older systems that do not support software error recovery
> +		 * should skip over uncorrected errors, but log everything else
> +		 */
> +		if (!mca_cfg.ser) {
> +			if (m.status & MCI_STATUS_UC)
> +				continue;
> +			goto log_it;
> +		}
> +
> +		/* Log "not enabled" (speculative) errors */
> +		if (!(m.status & MCI_STATUS_EN))
> +			goto log_it;
> +
> +		/*
> +		 * Log UCNA (SDM: 15.6.3 "UCR Error Classification")
> +		 * UC == 1 && PCC == 0 && S == 0
> +		 */
> +		if (!(m.status & MCI_STATUS_PCC) && !(m.status & MCI_STATUS_S))
> +			goto log_it;
> +

Can you please include a vendor check with this? MCi_STATUS[56] is not defined the same way on AMD systems.

Thanks,
Yazen