Date:   Tue, 31 Jan 2023 14:32:12 -0800
From:   Ashok Raj <ashok.raj@intel.com>
To:     Borislav Petkov <bp@alien8.de>
CC:     "Luck, Tony" <tony.luck@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        LKML <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        "Schofield, Alison" <alison.schofield@intel.com>,
        "Chatre, Reinette" <reinette.chatre@intel.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        "Stefan Talpalaru" <stefantalpalaru@yahoo.com>,
        David Woodhouse <dwmw2@infradead.org>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Jonathan Corbet <corbet@lwn.net>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Peter Zilstra <peterz@infradead.org>,
        "Lutomirski, Andy" <luto@kernel.org>,
        "andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
        "Ostrovsky, Boris" <boris.ostrovsky@oracle.com>,
        Martin Pohlack <mpohlack@amazon.de>,
        Ashok Raj <ashok.raj@intel.com>
Subject: Re: [Patch v3 Part2 3/9] x86/microcode/intel: Fix collect_cpu_info()
 to reflect current microcode
Message-ID: <Y9mW7EiL/BpYFLWn@a4bf019067fa.jf.intel.com>
References: <20230130213955.6046-1-ashok.raj@intel.com>
 <20230130213955.6046-4-ashok.raj@intel.com>
 <Y9lGdh+0faIrIIiQ@zn.tnic>
 <SJ1PR11MB6083580526A7FFA11F110B77FCD09@SJ1PR11MB6083.namprd11.prod.outlook.com>
 <Y9l8yGVvVHBLKAoh@zn.tnic>
 <SJ1PR11MB60837E6E6AE7C82511DC039EFCD09@SJ1PR11MB6083.namprd11.prod.outlook.com>
 <Y9mDYMASXCFaFkNU@zn.tnic>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y9mDYMASXCFaFkNU@zn.tnic>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?OdMPL8yxp7jA0c1brhWKMkQa429CKW65Bd3J2yM3snCN8Yb66CU2EL2MY1Ci?=
 =?us-ascii?Q?DeJwEvLuDXM6V4wUkiehF4sSCwhlkscULu7QdCnN5bYY5YS7HMO0cY7DXUOp?=
 =?us-ascii?Q?IMBb5hs4dmawT28XdtgYMFz9oRW26+dXvfbaqoBLGwvdUVyfPx9pyg3lipzk?=
 =?us-ascii?Q?5wki4uESIi7PCVjpdqCSsRaYuwfWQ0KgHM3wnCrGijgFHAfd5sSZIC6VuP0r?=
 =?us-ascii?Q?wSL42xo+xBFJ1GB7hnPcRRwlGxWfgbc870TaS3S3hHyaW/5s9gARyfnOLvtr?=
 =?us-ascii?Q?ND2G3aB5rGmdQz9EVhcgd0BBBI0mQs34b9U8/eljWbpD+edUlrFMei7qadFR?=
 =?us-ascii?Q?Z4qco0A+rE10czqeoKJRx1Oq8/fGiRFmqT7/R9MwgPKMIM/62qDiEz7tzwim?=
 =?us-ascii?Q?2p7DknR5nYXroiIZRVyIE6S/49sMzmY/j0CZsh4ASG9ja5dFVg9a4ppP19Fc?=
 =?us-ascii?Q?ekwmSEaur32UdvyvoypOmBDe4lG/0FRCfoRMo0EYa03KQAVSADhBEFKE9C8X?=
 =?us-ascii?Q?/yeZvW9Mn1+F2Ldv9amacKpiINFEwjoBObYggM173O5ut9NIzSixCrmkoeFP?=
 =?us-ascii?Q?uriZMaxsyoZxwZW3d9kyEdON9zADnO2gs+UkW6Am8VXP1eJzVUVkpE8xk67C?=
 =?us-ascii?Q?bY7HML58G5/vNbdg2sFUwE0jCegUyoYSQzmoAsP3sBVTnKb5MGcKV269Gcu5?=
 =?us-ascii?Q?mWDZdgNZKrNqAwbi+l/Z6l/8aIY8gcNvELc9ps9qelUPybrGKUZr9jDc1LZE?=
 =?us-ascii?Q?mEpePRycuUpC6H9ml70nMpCj8MM8wfS9b3kWlbzaOnnfqUBprEvl+A+JfmVE?=
 =?us-ascii?Q?lpLuExL7DpGoxfDVIsxlLaTNCxrJwbuvRuZH7+GILmAwtvZ0LsbrtxMaoIlt?=
 =?us-ascii?Q?GeiIcZLNpapGVXt/NvxiZ2hZ58Fkq6AlqD0GZHEBqnCmrmjp9/d0QEL33KC8?=
 =?us-ascii?Q?n6QZ7TbxFkZqM2R4iphkbLLSyeuMZT0/A70p4J0RPvXLnmfZSi0AN6fDzs28?=
 =?us-ascii?Q?KMnbTyUORgZZoyFMJF3+ILb1+kJV8UV58PZF8b63jU+9Sf95BEe5C6F/MI2p?=
 =?us-ascii?Q?ReabQMulPhUlQqB9iCKOMsRSUjzoS1qm7af5H/AuvdoLjVgutdJ7dMcbbgqt?=
 =?us-ascii?Q?y9JcZ94Jpeom+DUOw95snP7DPLYyxTFegN1gU3tuxYaPSRd/NDjPgCNyp0ES?=
 =?us-ascii?Q?hVhQqzAQDlnuX6BLHYoK2GnmPqjxTXs7FSg75t8RlVvAJsyJ3BdxX1MiYpJH?=
 =?us-ascii?Q?vIyboBpjAYaqx1cybzLEWj2bxh1xqd27P1khQyei3fR/EADN8Sd4evJhSXVW?=
 =?us-ascii?Q?grDJGtZni9pwRrvAf/QPfkee3uTb9RvSG6fqslS8dKXF95jrJifTMkSOZ22w?=
 =?us-ascii?Q?SN/PRWVH89JTj+8ZH9rHN6a2SbIcV825vAvslHelE77VOyPetssYyuycDY9y?=
 =?us-ascii?Q?dX1/Zw2sH/fl3X3Wzmc+yeIfsZpsLie2OcKj/edj5CnJJ7CIDPIx7vi9ei0U?=
 =?us-ascii?Q?zF1AYGJtauxniUXq9ZTVI4bqdNvI9Gx8Tb1csxys6r02o7WWS8z8vGohfz6v?=
 =?us-ascii?Q?ta2D38UngA2FkYMuXdI7rOM7z6PItRhmCgylg7F3SJO4L+HrsmZZ07/KfHTB?=
 =?us-ascii?Q?nQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a5048ff7-f3d4-44d2-00d0-08db03db12aa
X-MS-Exchange-CrossTenant-AuthSource: SJ1PR11MB6201.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2023 22:32:45.1096
 (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: 2hn/8+hfGNscZNs9OlYG5iVW2ozY2d5OSRD48sFQzqpX1jGpZf6LWqwIVmeEsXbIDG+jmhdFAGz/ZjxEjHULQQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5308
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Jan 31, 2023 at 10:08:48PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2023 at 08:49:52PM +0000, Luck, Tony wrote:
> > What happens here if the update on the first hyperthread failed (sure, it shouldn't,
> > but stuff happens at large scale).  In this case the current rev is still older that the
> > the cache version ... so there is no "goto out", and this hyperthread will now write
> > the MSR to initiate microcode update here, while the first thread is off executing
> > arbitrary code (the situation that we want to avoid).
> 
> Lemme see if I can follow: we sync all threads in __reload_late() and
> once they all arrive, we send them down into ->apply_microcode.

I was interpreting Thomas's response here

https://lore.kernel.org/lkml/87y1pygiyf.ffs@tglx/

---------
#3

Not to talk about the completely broken error handling in the actual
microcode loading case in __reload_late()::wait_for_siblings code path.
---------

I thought sending sibling down the apply_microcode() just to update
revision might have bad interaction, so revise this to be more explicit and
update only the revision number and not have any other intended side
effect.

Patch3 was a prep-patch for patch4, to eliminate the call to
apply_microcode(). Maybe that wasn't the issue?

It's likely Thomas hinted at somthing else and I took the wrong hint.

> 
> T0 arrives, and fails the update. That is this piece:
> 
>         /* write microcode via MSR 0x79 */
>         wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
> 
>         rev = intel_get_microcode_revision();
> 
>         if (rev != mc->hdr.rev) {
>                 pr_err("CPU%d update to revision 0x%x failed\n",
>                        cpu, mc->hdr.rev);
>                 return UCODE_ERROR;
>         }
> 
> We return here without updating cpu_sig.rev, as we should.
> 
> T1 arrives, updates successfully and updates its cpu_sig.rev.
> 
> T0's patch level has been updated too with that because the microcode
> engine is shared between the threads. T0's cpu_sig.rev isn't, however,
> as that has happened "behind its back", so to speak.
> 
> Is that the scenario you're talking about?

The fix in patch4 was just attempting to not call apply_microcode(), since
at this time the other CPUs that arrived in wait_for_siblings: have left,
doing an update when the others are strictly not in renedzvous is the
condition we try to avoid.

Again, maybe this is me reading Thomas's request incorrectly.

Cheers,
Ashok