Re: [RFC PATCH] x86/mce: Make mce_rdmsrl() do a plain RDMSR only

From: "Luck, Tony" <tony.luck@intel.com>
To: Borislav Petkov <bp@alien8.de>
Cc: x86-ml <x86@kernel.org>, lkml <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH] x86/mce: Make mce_rdmsrl() do a plain RDMSR only
Date: Mon, 7 Sep 2020 13:06:22 -0700	[thread overview]
Message-ID: <20200907200622.GA28517@agluck-desk2.amr.corp.intel.com> (raw)
In-Reply-To: <20200906212130.GA28456@zn.tnic>

On Sun, Sep 06, 2020 at 11:21:30PM +0200, Borislav Petkov wrote:
> Hi,
> 
> Ingo and I talked about this thing this morning and tglx has had it on
> his to-fix list too so here's a first attempt at it.
> 
> Below is just a brain dump of what we talked about so let's start with
> it and see where it would take us.

This very much looks to be the right thing to do. Returning "0" from
a failed RDMSR in MCE handling might be a much worse thing to do than
crashing. It papers over a serious error and the system might keep
running using corrupted data.

Digging into the history it seems that this rdmsrl_safe() was added for
a possible bug on a pentiumIII back in 2009 that was eventually closed
as "unreproducible".

Do we have three distinct classes of calls to RDMSR now?

1) This case. Architecture checks have been made. This call can't
fail. We are calling from a tricky state (on IST) so no tracing
allowed.

2) Normal case ... architecture checks have been done so shouldn't
fail. Safe state for tracing etc. Use rdmsrl().

3) Probe case. Architecture didn't provide definitive enumeration,
so we might take a #GP. Use the rdmsrl_safe().

> +	/*
> +	 * RDMSR on MCA MSRs should not fault. If they do, this is very much an
> +	 * architectural violation and needs to be reported to hw vendor.
> +	 */
> +	asm volatile("rdmsr" : EAX_EDX_RET(val, low, high) : "c" (msr));

If mce subsystem is the only instance of case "1", then this
inline is OK.  If there are others then rather than inlining
the asm here, we should have some new rdmsrl_notrace() inline
function declared for all to use.

Needs a:

Fixes: 11868a2dc4f5 ("x86: mce: Use safer ways to access MCE registers")

since this is undoing an earlier change.

-Tony