Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 17 Aug 2022 11:40:35 +0000
From:   Ashok Raj <ashok.raj@intel.com>
To:     Ingo Molnar <mingo@kernel.org>
CC:     Borislav Petkov <bp@alien8.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        Tony Luck <tony.luck@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        "LKML Mailing List" <linux-kernel@vger.kernel.org>,
        X86-kernel <x86@kernel.org>,
        Andy Lutomirski <luto@amacapital.net>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Jacon Jun Pan <jacob.jun.pan@intel.com>,
        Ashok Raj <ashok.raj@intel.com>
Subject: Re: [PATCH v3 3/5] x86/microcode: Avoid any chance of MCE's during
 microcode update
Message-ID: <YvzTsxilM1kUGeFQ@araj-dh-work>
References: <20220817051127.3323755-1-ashok.raj@intel.com>
 <20220817051127.3323755-4-ashok.raj@intel.com>
 <Yvybq+hYT4tG/yAg@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Yvybq+hYT4tG/yAg@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?rvEljZ1GeY/lixDiuCqy8jN6JkM6J0oBPgMVEgvuaRIbRUdq96utcYmebC4D?=
 =?us-ascii?Q?RHz1+EQnSswfiAbXP5hcnzicxgGP6qxsqVVVSLysEtzQ0go6/sP8zHdvamqW?=
 =?us-ascii?Q?Am3sRRcR0EsG/NtzeovFBE/UHiZ9tDzgFBBWEg3qKGdoKNIrIx2iFyFNwn+Q?=
 =?us-ascii?Q?lhAoxCpqqQXScijZW0xPw6ZljKRo29eABJ9lpYvL06//fzlW2izbxmrmrfcd?=
 =?us-ascii?Q?2DaYHiSOy6/+e/PIt/qDzWizHbALgTIINSPAUw+Fud/RvpofNr2VxN95i4ax?=
 =?us-ascii?Q?gm2x8uxQT4WRKEVbBZmbQlF79jtR+/qbeJQQpaf5X89xr4qbdqTcpUwMWr2J?=
 =?us-ascii?Q?ChEJdhjY1DlEJJfBa9vC8kzRKtJQVuQGzGipo04Fen2B1Y5NvFbTt7l3lqSg?=
 =?us-ascii?Q?H9I6NR6oJTYh2P7ydqwwxTW3XLgLIpyDvrGZlDFbbCVuJ4RR0VgXUmJ+x3aj?=
 =?us-ascii?Q?aWBNxNlJB8oveoY2Wj84o8Mzj+bjxPRnGDPo8xvPACJqqSpRy8cCiQJNbAGF?=
 =?us-ascii?Q?SfKLoBhqMpgewSvTYpZiY6KQdteflVGtquH7RzM1gZkD/F1S5EGx28l/w242?=
 =?us-ascii?Q?a6TZlCzvWCnMeh808bx+RPwiNjS1550VRjcFuExzr70EQzlTudBGybAiIwkI?=
 =?us-ascii?Q?aHPXt9HiYvBeOOtv0gYmUst9PKzllSx2zi/tZvcG/m5dCOHhGX30AymEcIu/?=
 =?us-ascii?Q?Dll5OwfyYzyfz9SOvNzWmummz2FsgzeIYyXdQQIrMOXlWk+kjLwL4lphF+xa?=
 =?us-ascii?Q?Dc5O+csMI3xng4SEaRYolDUss4zWRwH36URjQQpVzdONcRBcj/DMT9HemeWS?=
 =?us-ascii?Q?IbhNw399wiR2We9JN/Nb0S0bE1nLJRGr5zynmc6M8AIp5F2JD4vY3DC/nXRZ?=
 =?us-ascii?Q?EoAc0oIgXbReOArTfHMLRau3EoAm8hzZoNhbfbsXpWSdJiDhoB5GeelEAcX2?=
 =?us-ascii?Q?/ZXlx7WTnBnHKOSgldVHvQnuI3bCuenT/x5fXmqdgmGOQ8xH4Oc+/Hg0lMDd?=
 =?us-ascii?Q?gnDWyAtP0BbSPYosvCmLOnCjKVgAnf9yeI3UZmuD4g0LgOUFud8M5d0LuGw7?=
 =?us-ascii?Q?2cC1d99gcdcJQoNee0HKQj+udwJ9xtqlvcDxaT4EUiGDGsEqXGDjmKFH/SSV?=
 =?us-ascii?Q?rMq9zpz7GGECilC8j26Ymr14blYPhvvrhOs06SCgE/fLDyx2qN3Ggb5Vqie6?=
 =?us-ascii?Q?gshUfYCNpX0NUUQV4lgcODFWk/4qcV7L6hhoAynjq38JsDzlFmh1iY/P8JuF?=
 =?us-ascii?Q?OiJ76vDy26ZcgYbPQYf+KIDExGhsFEoFyDH8d3Iw4wRYA5u4WiQX9etFDQ7/?=
 =?us-ascii?Q?11kzCnj4I+G3E59yh8BFdz2illfTBEXoPM0GTUUkY64ITjJi6NLg6pthWjH4?=
 =?us-ascii?Q?lXoUf0+v9t1yLqFv1u7L5Irv/0bOxPUAeiYNhhy6vvGeYzIuCvoeO1vFXmgD?=
 =?us-ascii?Q?4cb3BQB5j1lUuHkHyQySFqlR+KfsnhnswJ7wO/nu2uYstYOQBhnp5x/JSXFe?=
 =?us-ascii?Q?prad13+rvKmSk9rGp13TAg+/YBDEPPUInAp32nL1qXpXN29rkuJaUdZPuyYs?=
 =?us-ascii?Q?Td/OsQRIzyER+pW6D+AS1+pykWs0GrFJeHyHy72ZF3fZk7ZSm5BQ/aaQNUAl?=
 =?us-ascii?Q?Wg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c79fe2db-5598-41f4-eae9-08da80455838
X-MS-Exchange-CrossTenant-AuthSource: SJ1PR11MB6201.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Aug 2022 11:40:54.8663
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: +Z3oDIr2PcnoFtK8TBbKWIckRtIoEJ0TVQLK3jfAmX5xvT0hlwyS9GMDdwzcinvHaxtFCvIzXnK50Vjx2sycyA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5389
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Aug 17, 2022 at 09:41:31AM +0200, Ingo Molnar wrote:
> 
> > +void mce_set_mcip(void)
> > +{
> > +	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0x1);
> > +}
> > +
> > +void mce_clear_mcip(void)
> > +{
> > +	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0x0);
> > +}
> 
> Instead of naming new APIs after how they are doing stuff, please name them 
> after *what* they are doing at the highest level: they disable/enable MCEs.
> 
> Ie. I'd suggest something like:
> 
>      mce_disable()
>      mce_enable()

We actually aren't disabling MCE's we set things up to promote it to a more
severe shutdown if an MCE were to be signaled while in the ucode update
flow.

I'm struggling to find a suitable name. But I agree with what you are
saying.

promote_mce_to_fatal()? I'll take any names that seem fit.

> 
> I'd also suggest to at minimum add a WARN_ON_ONCE() if MSR_IA32_MCG_STATUS 
> is already 1 when we disable it - because whoever wanted it disabled will 
> now be surprised by us enabling them again.

Ok, will add.

> 
> > +	/*
> > +	 * Its dangerous to let MCE while microcode update is in progress.
> 
> s/let MCE while
>  /let MCEs execute while
> 
> > +	 * Its extremely rare and even if happens they are fatal errors.
> > +	 * But reading patched areas before the update is complete can be
> > +	 * leading to unpredictable results. Setting MCIP will guarantee
> 
> s/can be leading to
>  /can lead to
> 
> > +	 * the platform is taken to reset predictively.
> 
> What does 'the platform is taken to reset predictively' mean?

Since we are setting MCG_STATUS.MCIP=1, since MCE's aren't nestable, 
if there is a hardware event trying to signal a MCE, it will turn into a
platform reset. The MCE registers that logged the event will be sticky
and preserve in a warm reset case. BIOS or OS can pickup values after the
reboot is complete.

> 
> Did you mean 'predictibly'/'reliably'?

I don't know the difference, except both are a trustworthy topic :-)

I like predictable, the system is going down.. not reliable :-)
> 
> > +	 */
> > +	mce_set_mcip();
> >  	/*
> >  	 * On an SMT system, it suffices to load the microcode on one sibling of
> >  	 * the core because the microcode engine is shared between the threads.
> > @@ -457,6 +466,7 @@ static int __reload_late(void *info)
> >  	 * loading attempts happen on multiple threads of an SMT core. See
> >  	 * below.
> >  	 */
> > +
> >  	if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu)
> >  		apply_microcode_local(&err);
> >  	else
> 
> Spurious newline added?

It's a gonner !

Cheers,
Ashok