Date:   Wed, 1 Feb 2023 11:32:40 -0800
From:   Ashok Raj <ashok.raj@intel.com>
To:     Dave Hansen <dave.hansen@intel.com>
CC:     Borislav Petkov <bp@alien8.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        LKML <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Tony Luck <tony.luck@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Reinette Chatre <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>,
        Andy Lutomirski <luto@kernel.org>,
        Andrew Cooper <Andrew.Cooper3@citrix.com>,
        Boris Ostrovsky <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: <Y9q+WL+hnS5ZymDj@a4bf019067fa.jf.intel.com>
References: <20230130213955.6046-1-ashok.raj@intel.com>
 <20230130213955.6046-4-ashok.raj@intel.com>
 <388d0260-042c-209f-a986-c9ca47061b3c@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <388d0260-042c-209f-a986-c9ca47061b3c@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?tx3iBrUZKAaYH8lp3psWP8dqNsqwwpJaxRK9dz9i++o/gxHfuBa6g9OwPcG1?=
 =?us-ascii?Q?eZJUPS2V4BB6wBE+dnxvjWos2Xh03SGshTxA7zzlt3xYPP4TUS0k7N1KgQdM?=
 =?us-ascii?Q?eifLB65FtJhq+SIXFLDPEj3Y9B7gzh5ABvqE965HXnyMQhVTTq3R82ziRYq9?=
 =?us-ascii?Q?zO35sLKyrdT6hjyLdC5Qwx/5luEb8YOJoZv0q+5IPfHGSISqOlftDJiZRD35?=
 =?us-ascii?Q?83ZlnaMH8EtHLjDWiK/AlNkwEGo8gT+larYCXVo9OQ4xilc6SGnW9cC0wF07?=
 =?us-ascii?Q?UYfXtzX8DSHXzn2f2YW/AAhBzJvaosvsFmAADG8+tFcvprd3mi0wuQma90vQ?=
 =?us-ascii?Q?VPch0v5SR9nVtiyFqdL3G9EtLUqs/PirVxbOw9Xlj5y4fgIl47nPmNHWl0fs?=
 =?us-ascii?Q?yC4KdYMeHj2ayIz9MbCfZpuJPmfSlgB204JtDFRbNRkQnwKF14GijpwWELK5?=
 =?us-ascii?Q?qsjIQLpr/zE+41ni8EVlNigoAL0ZQOfL75wNLkVQkuILnnGcCYbVuSRIYw3E?=
 =?us-ascii?Q?3+3dZOzlIiOybe3+nYGeX/r4DqqM2KM7V+kNJ+IUIdWcSihZ55z1C053cN0Q?=
 =?us-ascii?Q?HoAycnJv3KiSPubgVfIKvxLJfme8uWkslImKz7SrySMjrUJdp14fvo3QdhiH?=
 =?us-ascii?Q?mf9ovty3CmUVjG2Yc3srIxIVnpaxfrbCRl2Hgw4qGcGSF+IoSLvTNAr7Izev?=
 =?us-ascii?Q?tZOLIS+ssk8iLzi8yyLsSNxvc2acRF40duzSrT3Tql0q6wkFPvPCp1cHf1aw?=
 =?us-ascii?Q?22HRe4jnpm72dnrNe0hejjXKULxBROo/as4Q0JWlfS6djctbjB8kFHRXlY/u?=
 =?us-ascii?Q?VShQrZt66cdRk5l+QKxVkKoewVFCyNzuhxpFgGrMmQm/4amaWcFMUCQlqAdL?=
 =?us-ascii?Q?kefTiTBQ6M0IjmkRypvMmSaGNWLrVn1GSUi4zaDlE1HizJsNI1fpjp2TLEgY?=
 =?us-ascii?Q?3F8lOQis69FhYGQnsq7VITWQF4Whjkmlp1S/gpdhP+LCAgYhqJ1Gl3OS3vKS?=
 =?us-ascii?Q?RCGTdJpfeaJNeOnFCbkEeN7D6Gxc2knA32V0XyziJ66Lz0R8D4Z/6kpN+OkT?=
 =?us-ascii?Q?j0eeC+CJ4uT6GzK7HJfXFu6qLoUHxvb1yrv3ahfyk4RAMS51sRZEYjMY+0yF?=
 =?us-ascii?Q?KWT2f6EEd1VfUlcN9hpg/IafyBck5CooS4I/ycASqv8ALqJigVdtundkZ39s?=
 =?us-ascii?Q?JCyT4rbBPwaikbV0knNrf1J60W7/gI0OvFK+EwTWRDVQEbKY5MsrHueDuZdC?=
 =?us-ascii?Q?YOaVf725SeQNgO3LKjieyCyk2uO9l+F0FvOn0Jvwh83t8TJ85I0L28xm0c4Q?=
 =?us-ascii?Q?M5wI42bsgtg4dhZjTkHjvfzk4s6ca7TDX6ZTNpOZ06yhKgGJss9VyDKnvC8d?=
 =?us-ascii?Q?g0h1JuFZQqKZcwpAGwjYoMtjKH9iUnvAlO2Wpg42k+EVLe4Z1gzKuGmTq8JI?=
 =?us-ascii?Q?UVPQNN6ej5XOx3dm1DmQU/4gwN8mGXnVEjTouANHPRUGbTtJOS7tz7CfGp56?=
 =?us-ascii?Q?0B23/qzHOtOadKzxWCpqYDPMehxoIx5Hwp9fS7InjwOr6UBjuE8CU7bBGeu1?=
 =?us-ascii?Q?Nh9Kue9ulHnCMb2rfLNOdjNJ/uZE+CSQbyzAS4ypDwVKOf4RDZnstpfuW6J0?=
 =?us-ascii?Q?Cw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e20b4d5e-d32d-4063-1d68-08db048b29b0
X-MS-Exchange-CrossTenant-AuthSource: SJ1PR11MB6201.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2023 19:33:15.4285
 (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: YU9b2u4DrHyncIBvBtDZccQ8RSlQV20xO0jtbSqntf3/XWSHvRuj7rmH3zqv/sjWVg2W0kh/4ZXiUBfCl8Xctw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB6831
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 11:13:58AM -0800, Dave Hansen wrote:
> On 1/30/23 13:39, Ashok Raj wrote:
> > Currently collect_cpu_info() is only returning what was cached earlier
> > instead of reading the current revision from the proper MSR.
> > 
> > Collect the current revision and report that value instead of reflecting
> > what was cached in the past.
> > 
> > [TBD:
> >     Need to change microcode/amd.c. I didn't quite follow the logic since
> >     it reports the revision from the patch file, instead of reporting the
> >     real PATCH_LEVEL MSR.
> > 
> >     Untested on AMD.
> > ]
> 
> This thread is meandering a bit.  I think it's because this changelog
> doesn't have a problem statement.  It's hard to agree on a patch being a
> solution to anything if we haven't first agreed on the problem.
> 
> What is the problem?

I alluded here.. But yes, clearly missed in the commit log.

https://lore.kernel.org/lkml/Y9mW7EiL%2FBpYFLWn@a4bf019067fa.jf.intel.com/

Thomas alluded here https://lore.kernel.org/lkml/87y1pygiyf.ffs@tglx/
that error handling in __reload_late()::wait_for_siblings() code patch is
completely broken. 

This is one that I "assumed" he was referring to, since all we need is to
update the current revision, but we end up depending on the behavior of
apply_microcode() and that might accidentally have some side effects. 

Instead only call the collect_cpu_info() and allow that to update the
per-cpu revision instead. And there is no risk in performing that vs
accidentally letting it fall through with an apply_microcode() that might
have risks.

> 
> What does this "fix"?

The code performs this delicate late-load dance to prevent sibling threads
to be quiet while performing the update.

At wait_for_siblings() when all threads arrive, then the sibling does the
apply_microcode() which seems wrong.