Date:   Wed, 1 Feb 2023 07:13:30 -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: <Y9qBmugSm+o5u4pq@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>
 <SJ1PR11MB608384F3B075E0D2F25683D3FCD09@SJ1PR11MB6083.namprd11.prod.outlook.com>
 <Y9pgzGr4MccwEJAl@zn.tnic>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y9pgzGr4MccwEJAl@zn.tnic>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?5PdxrGPvQc5ashy5WXc6AZ2q+LDePWbDNVW9qF341gH5v6WtO3J+ldZNnKmz?=
 =?us-ascii?Q?Cnq4xxOBJOe3sGUzUN4TlKYqGxSZhKBGUFmsLIyxaBKJLdeujPi8C/tYiXvs?=
 =?us-ascii?Q?1f5X9Uj45M9lmwFgGW6Dj6cI27hILw2wh6mbRWCtC7u5Qt9OU+r+62tMRVX0?=
 =?us-ascii?Q?mC+SzVLJtnWm1ttbkEghkPL+bhxswNI4vqZWcQH3CFbtnCksifz50nwSHsuJ?=
 =?us-ascii?Q?LeV3Dr8V7qTAfwF6PZ07EA3iyPAvpK6q+36jaQ7uPsn8xZx4t9h4GlzNar4W?=
 =?us-ascii?Q?nUmuV+2rRUCRFRH/5hUrkmU4CziKy1B+F2EPxRU7KKUmSblWM8Wnb8cTYYzp?=
 =?us-ascii?Q?3JG8PA36ppRkIESS1jZAbVeado1iO4kXLnwl0FamZ0YDZSuAhNyNVrKQk7Fy?=
 =?us-ascii?Q?AuuqT9Vnjv6qJXaw2dNu9W+C+GMb7xunFK/7qqwgjYYkAXSDJQ2NXXVcP03L?=
 =?us-ascii?Q?342NmkPRGoJbvVDFozMPK67qgw9Zmc009W6NboS9Tv0TxP+cpktN40ciYD75?=
 =?us-ascii?Q?0xNE1jmCZn7F75SWekr42tptpnwk9/BKKtptkfHEjUD/BYWpDPLU+DP8hfTd?=
 =?us-ascii?Q?YfHmZp3Orgwvwi1VXYsI9BmIF6iJaklYffB12abxsmQx6ssKisCvQFVn/Hs7?=
 =?us-ascii?Q?LAKmbt8QdDlVTbYS0wOq3BxsJiyKglbvDSC6FxXi00t82EKA31Y424n9DsKM?=
 =?us-ascii?Q?aZqWglX9pemg3Ig/R46QVNst5kmh7Lgaqf0RGqfycAOKWGbzr44zzJmcGiED?=
 =?us-ascii?Q?bBg3Sw0AIDDbObCNK0qzDlEpnFnEq3Zv1lsRYC2QE3Nzk1mN9UltZfn+UILG?=
 =?us-ascii?Q?xtNzv8Nk1y+2K6GnEturRV0adOSnTJYbvCNgiUrgAThhDzS1HxjpK9uyLY9v?=
 =?us-ascii?Q?0+NT1cLymyzUvPgITik70/9XhKYVNlP3V1V2Vb7BRKhybCOIwYTVKIw4iUms?=
 =?us-ascii?Q?5pvQowPsBzSlNKStSHPT6UaX5Kc5xT5uyNlPohiLbuxjUt9ze6E9Ipq4plKA?=
 =?us-ascii?Q?RgTEs0cUeGUtmURwSCAOn2nWOLK2c9inSIyqjJc1YJXmEny17kFjiDouvM5n?=
 =?us-ascii?Q?Ld3uxGDqtbl7nMdHN3bWPocf7zRze1OtIAYTr657+kFAmq4nzaHYxgRkkCDR?=
 =?us-ascii?Q?o639aMPbgsVZFWPzNQFQS7/TUUtNojuVTFWtgVzlh+wJWOJPZTB6JA2GCICn?=
 =?us-ascii?Q?H+P63w9rykj0I8z5Dx5dqsFXCDashI1gSWdoSscpPRpqqVpAs2UsfZsmZfD2?=
 =?us-ascii?Q?DDTi2s+ANqcsl0ym7tJKrpW7ZrXk5nYjQ859QAWJ1REESmQKYJqM8ypuB4xv?=
 =?us-ascii?Q?XTZ5wQEAiDOpfZpyStscLmtpGcGR09sSWXt1woTFvpj3SVyqwPjDXbDH1Z/m?=
 =?us-ascii?Q?27vH4Mcs4c7HXz6i627t8ERb0PO557tgNTUwUscYaq760Do4pVLhY5hf+nrZ?=
 =?us-ascii?Q?9XkFwr3u/3v0+PcnAwQyv+UVg4Ijgh8Sioh+5cRutgZIxzDGlBpCrS4mfdcN?=
 =?us-ascii?Q?4bGm4sQGl3jcWBcGW1LF783tfB3WmFzmoGhj40k/gxEnKCTBcZJbsUDZsGg6?=
 =?us-ascii?Q?aBIm/sgXDNXWWvqIjaEh96MJNKO39/yR8FHDsJ16k7YBwzmaK5c6VMUa9x1s?=
 =?us-ascii?Q?AQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3692d7de-57f3-432d-9fda-08db0466f399
X-MS-Exchange-CrossTenant-AuthSource: SJ1PR11MB6201.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2023 15:14:02.5507
 (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: cEVpTqbbKoui2pRFVWbLBMliChf4KEM/j+lBZT9ClEcnrWDSxHTFeNZsosZeM4O6OGDzF7AkcUWRwJ0mBDaVJA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7092
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Feb 01, 2023 at 01:53:32PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2023 at 10:43:23PM +0000, Luck, Tony wrote:
> > In an ideal world yes. But what if T1 arrives here and tries to do the
> > update while T0, which has returned out of the microcode update
> > code and could be doing anything, happen to be doing WRMSR(some MSR
> > that the ucode update is tinkering with).
> > 
> > Now T0 explodes (not literally, I hope!) but does something crazy because
> > it was in the middle of some microcode flow that got updated between two
> > operations.
> 
> So first of all, I'm wondering whether the scenario you're chasing is
> something completely hypothetical or you're actually thinking of
> something concrete which has actually happened or there's high potential
> for it.
> 
> In that case, that late patching sync algorithm would need to be made
> more robust to handle cases like that.

That's correct. But fundamentally we sent the sibling down the
apply_microcode() path just to make sure the per-thread info is updated.

It appears the code is using a side effect that the revision got updated
even though we don't actually intend to perform a wrmsr on the sibling
in the normal case that primary completes the update.

If the purpose is only to update the revision, using the collect_cpu_info()
which seems more appropriate for that purpose, and doesn't have any
implied issues with using a wrmsr flow. It's not broken today, but the code
isn't future proof. Calling the revision update only keeps those questions
at bay.

I think this is what Thomas implied to cleanup in his comments. 

> 
> Because from what I'm reading above, this doesn't sound like the
> reporting is wrong only but more like, if T0 fails the update and T1
> gets to do that update for a change, then crap can happen.
> 
> Which means, our update dance cannot handle that case properly.
> 

It doesn't need to if we don't do an apply_microcode() for the sibling.

Cheers,
Ashok