Re: [PATCH 13/13] x86/microcode/intel: Add ability to update microcode even if rev is unchanged

From: Ashok Raj
Date: Wed Oct 19 2022 - 11:56:59 EST


Hi Boris,

There is a minor bug in this force reload option I added for testing.

See below:

On Fri, Oct 14, 2022 at 01:09:13PM -0700, Ashok Raj wrote:

[snip]

> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index eb2caa74de01..632c7a1fcffb 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -23,6 +23,7 @@
> #include <linux/miscdevice.h>
> #include <linux/capability.h>
> #include <linux/firmware.h>
> +#include <linux/debugfs.h>
> #include <linux/kernel.h>
> #include <linux/delay.h>
> #include <linux/mutex.h>
> @@ -46,6 +47,7 @@
>
> static struct microcode_ops *microcode_ops;
> static bool dis_ucode_ldr = true;
> +bool ucode_load_same;
>
> bool initrd_gone;
>
> @@ -542,11 +544,12 @@ static int __reload_late(void *info)
> goto wait_for_siblings;
> }
>
> - if (err >= UCODE_NFOUND) {
> - if (err == UCODE_ERROR)
> + if (ret || err >= UCODE_NFOUND) {
> + if (err == UCODE_ERROR ||
> + (err == UCODE_NFOUND && !ucode_load_same)) {
> pr_warn("Error reloading microcode on CPU %d\n", cpu);
> -
> - ret = -1;
> + ret = -1;
> + }
> }
>
> wait_for_siblings:
> @@ -636,9 +639,12 @@ static ssize_t reload_store(struct device *dev,
> }
>
> tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);
> - if (tmp_ret != UCODE_NEW)
> + if (tmp_ret != UCODE_NEW && !ucode_load_same)
> goto put;
>
> + if (tmp_ret != UCODE_NEW)
> + pr_info("Force loading ucode\n");
> +

The above needs additional check for UCODE_ERROR. If you are testing with
old microcode with minrev=0, noticed this went and tried to perform an
update without the UCODE_ERROR check.

I've queued for the next update.


- if (tmp_ret != UCODE_NEW)
+ if (tmp_ret == UCODE_ERROR ||
+ (tmp_ret != UCODE_NEW && !ucode_load_same))
goto put;

+ if (tmp_ret != UCODE_NEW)
+ pr_info("Force loading ucode\n");
+

> mutex_lock(&microcode_mutex);
> ret = microcode_reload_late();
> mutex_unlock(&microcode_mutex);
> @@ -841,6 +847,7 @@ static const struct attribute_group cpu_root_microcode_group = {
> static int __init microcode_init(void)
> {
> struct cpuinfo_x86 *c = &boot_cpu_data;
> + static struct dentry *dentry_ucode;
> int error;
>
> if (dis_ucode_ldr)
> @@ -884,7 +891,12 @@ static int __init microcode_init(void)
> cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "x86/microcode:online",
> mc_cpu_online, mc_cpu_down_prep);
>
> + dentry_ucode = debugfs_create_dir("microcode", NULL);
> + debugfs_create_bool("load_same", 0644, dentry_ucode, &ucode_load_same);
> +
> pr_info("Microcode Update Driver: v%s.", DRIVER_VERSION);
> + pr_info("ucode_load_same is %s\n",
> + ucode_load_same ? "enabled" : "disabled");
>
> return 0;
>
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index c61aa661ac2f..c9f1e6f5e53b 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -763,7 +763,7 @@ static enum ucode_state apply_microcode_intel(int cpu)
> * already.
> */
> rev = intel_get_microcode_revision();
> - if (rev >= mc->hdr.rev) {
> + if (rev >= mc->hdr.rev && !ucode_load_same) {
> ret = UCODE_OK;
> goto out;
> }
> @@ -779,7 +779,7 @@ static enum ucode_state apply_microcode_intel(int cpu)
> return UCODE_ERROR;
> }
>
> - if (bsp && rev != prev_rev) {
> + if (bsp && (rev != prev_rev || ucode_load_same)) {
> pr_info("updated to revision 0x%x, date = %04x-%02x-%02x\n",
> rev,
> mc->hdr.date & 0xffff,
> --
> 2.34.1
>