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.
Fixes: 1008c52c09dc ("x86/CPU: Add a microcode loader callback")
Signed-off-by: Ashok Raj <[email protected]>
---
arch/x86/kernel/cpu/microcode/core.c | 36 ++++++++++++++++++++--------
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index ef24e1d228d0..fab2010ff368 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -431,12 +431,28 @@ static int __reload_late(void *info)
return ret;
}
+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);
if (ret == 0)
- microcode_check();
+ microcode_check(&info);
pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
old, boot_cpu_data.microcode);
--
2.34.1
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.
> 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?
Thanks,
tglx
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Ashok Raj <[email protected]>
---
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;
}
On Tue, Nov 29, 2022 at 01:08:29PM -0800, 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.
Hmm, but if that has happened, the capabilities will be turned off in
your @orig argument below?
Or are you saying that this copy_cpu_caps() read before the update will
overwrite the cleared bits with the their actual values from CPUID so
that what you really wanna compare here is *hardware* CPUID bits before
and after and not our potentially modified copy?
I.e., some bit in CPUID is 1 and we have cleared it in our copy.
Close?
If so, then this should be specifically called out in the commit
message.
> +static void copy_cpu_caps(struct cpuinfo_x86 *info)
If anything, that function should be called
store_cpu_caps()
and store it into its parameter *info.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Dec 07, 2022 at 09:25:54PM +0100, Borislav Petkov wrote:
> On Tue, Nov 29, 2022 at 01:08:29PM -0800, 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.
>
> Hmm, but if that has happened, the capabilities will be turned off in
> your @orig argument below?
>
> Or are you saying that this copy_cpu_caps() read before the update will
> overwrite the cleared bits with the their actual values from CPUID so
> that what you really wanna compare here is *hardware* CPUID bits before
> and after and not our potentially modified copy?
>
> I.e., some bit in CPUID is 1 and we have cleared it in our copy.
>
> Close?
Correct.
>
> If so, then this should be specifically called out in the commit
> message.
Sounds good, I can certainly add it when I repost.
>
> > +static void copy_cpu_caps(struct cpuinfo_x86 *info)
>
> If anything, that function should be called
>
> store_cpu_caps()
>
The names sounds more fitting, I can change it as suggested.
Cheers,
Ashok