Re: [Patch V1 4/7] x86/microcode/core: Take a snapshot before and after applying microcode

[Ashok Raj]

On Fri, Dec 02, 2022 at 08:09:30PM +0100, Thomas Gleixner wrote:
> On Tue, Nov 29 2022 at 13:08, Ashok Raj wrote:
> 
> > The kernel caches features about each CPU's features at boot in an
> > x86_capability[] structure. The microcode update takes one snapshot and
> > compares it with the saved copy at boot.
> >
> > However, the capabilities in the boot copy can be turned off as a result of
> > certain command line parameters or configuration restrictions. This can
> > cause a mismatch when comparing the values before and after the microcode
> > update.
> >
> > microcode_check() is called after an update to report any previously
> > cached CPUID bits might have changed due to the update.
> >
> > Ignore the capabilities recorded at boot. Take a new snapshot before the
> > update and compare with a snapshot after the update to eliminate the false
> > warning.
> 
> Makes sense.
> 
> > +static void copy_cpu_caps(struct cpuinfo_x86 *info)
> > +{
> > +	/* Reload CPUID max function as it might've changed. */
> > +	info->cpuid_level = cpuid_eax(0);
> > +
> > +	/*
> > +	 * Copy all capability leafs to pick up the synthetic ones so that
> > +	 * memcmp() below doesn't fail on that. The ones coming from CPUID will
> > +	 * get overwritten in get_cpu_cap().
> > +	 */
> > +	memcpy(info->x86_capability, &boot_cpu_data.x86_capability,
> > +	       sizeof(info->x86_capability));
> > +
> > +	get_cpu_cap(info);
> > +}
> > +
> >  /*
> >   * The microcode loader calls this upon late microcode load to recheck features,
> >   * only when microcode has been updated. Caller holds microcode_mutex and CPU
> >   * hotplug lock.
> >   */
> > -static void microcode_check(void)
> > +static void microcode_check(struct cpuinfo_x86 *orig)
> >  {
> >  	struct cpuinfo_x86 info;
> >  
> > @@ -446,15 +462,13 @@ static void microcode_check(void)
> >  	info.cpuid_level = cpuid_eax(0);
> >  
> >  	/*
> > -	 * Copy all capability leafs to pick up the synthetic ones so that
> > -	 * memcmp() below doesn't fail on that. The ones coming from CPUID will
> > -	 * get overwritten in get_cpu_cap().
> > -	 */
> > -	memcpy(&info.x86_capability, &boot_cpu_data.x86_capability, sizeof(info.x86_capability));
> > +	* Copy all capability leafs to pick up the synthetic ones so that
> > +	* memcmp() below doesn't fail on that. The ones coming from CPUID will
> > +	* get overwritten in get_cpu_cap().
> > +	*/
> > +	copy_cpu_caps(&info);
> >  
> > -	get_cpu_cap(&info);
> > -
> > -	if (!memcmp(&info.x86_capability, &boot_cpu_data.x86_capability,
> > +	if (!memcmp(&info.x86_capability, &orig->x86_capability,
> >  		    sizeof(info.x86_capability)))
> >  		return;
> >  
> > @@ -469,6 +483,7 @@ static void microcode_check(void)
> >  static int microcode_reload_late(void)
> >  {
> >  	int old = boot_cpu_data.microcode, ret;
> > +	struct cpuinfo_x86 info;
> >  
> >  	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
> >  	pr_err("You should switch to early loading, if possible.\n");
> > @@ -476,9 +491,10 @@ static int microcode_reload_late(void)
> >  	atomic_set(&late_cpus_in,  0);
> >  	atomic_set(&late_cpus_out, 0);
> >  
> > +	copy_cpu_caps(&info);
> >  	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
> 
> You clearly ran out of newlines and comments here.

:-).. I'll add comments and some separation in my next post.

> 
> >  	if (ret == 0)
> > -		microcode_check();
> > +		microcode_check(&info);
> >  
> >  	pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
> >  		old, boot_cpu_data.microcode);
> 
> Unrelated to that patch, but it just caught my attention. Why are we
> printing this is case of failure?

You are correct. I do have one, and for some reason was stuck behind couple
other unrelated patches. I moved it up and will include in my next repost.

From: Ashok Raj <ashok.raj@xxxxxxxxx>
Date: 2022-11-18 19:49:09 -0800

x86/microcode: Display revisions only when update is successful

Right now, both microcode loading failures and successes print the same
message that Reloading is completed. This is misleading to users who would
need to deduce the reload failure by checking that the revision fields
didn't actually change.

Display the updated revision number only if an update is successful.

Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
Signed-off-by: Ashok Raj <ashok.raj@xxxxxxxxx>

---

 arch/x86/kernel/cpu/microcode/core.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index fab2010ff368..58ee81ea09ed 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -493,11 +493,12 @@ static int microcode_reload_late(void)
 
 	copy_cpu_caps(&info);
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
-	if (ret == 0)
-		microcode_check(&info);
 
-	pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
-		old, boot_cpu_data.microcode);
+	if (ret == 0) {
+		pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
+			old, boot_cpu_data.microcode);
+		microcode_check(&info);
+	}
 
 	return ret;
 }