[2/5] x86/microcode: Simplify init path even more
Commit Message
From: Borislav Petkov <bp@suse.de>
Get rid of all the IPI-sending functions and their wrappers and use
those which are supposed to be called on each CPU.
Thus:
- microcode_init_cpu() gets called on each CPU on init, applying any new
microcode that the driver might've found on the filesystem.
- mc_cpu_starting() simply tries to apply cached microcode as this is
the cpuhp starting callback which gets called on CPU resume too.
And since the driver init function is a late initcall, there is
filesystem by then so a new firmware load attempt can simply be done. In
case a new one is there. Which is weird to begin with - how would the
initrd contain an older revision than what's on the fs since former gets
created by using the blobs from the filesystem.
Oh well, it is cheap to do so why not...
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/kernel/cpu/microcode/core.c | 127 +++++----------------------
1 file changed, 23 insertions(+), 104 deletions(-)
Comments
On Wed, Oct 19, 2022 at 07:54:23PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
>
> Get rid of all the IPI-sending functions and their wrappers and use
> those which are supposed to be called on each CPU.
Good cleanup overall!.. Trying to apply them and do a quick test.
[snip]
>
> + /*
> + * Try to load microcode once on the BSP in case the initrd has older revision.
> + * Frankly, I have no clue how that can happen but hey, loading here is cheap so
> + * why not.
> + */
> + microcode_ops->request_microcode_fw(boot_cpu_data.cpu_index, µcode_pdev->dev, true);
> +
You need to call a microcode_ops->apply_microcode() too if you want to
apply. But you also want to pay attention to the return code too and call
it appropriately.
> /* Do per-CPU setup */
> cpus_read_lock();
> on_each_cpu(setup_online_cpu, NULL, 0);
> --
> 2.35.1
>
On Wed, Oct 19, 2022 at 12:22:57PM -0700, Ashok Raj wrote:
> Good cleanup overall!.. Trying to apply them and do a quick test.
Thx. Use 6.1-rc1 as base.
> You need to call a microcode_ops->apply_microcode() too if you want to
> apply.
That's done ...
> But you also want to pay attention to the return code too and call it
> appropriately.
Ah ok, yes, I need to check ->request_microcode_fw's retval.
> > /* Do per-CPU setup */
> > cpus_read_lock();
> > on_each_cpu(setup_online_cpu, NULL, 0);
... in here.
On Wed, Oct 19, 2022 at 09:37:59PM +0200, Borislav Petkov wrote:
> Ah ok, yes, I need to check ->request_microcode_fw's retval.
On a second thought, no I don't have to: the request routine will try to
load any new microcode but if it fails, ->apply_microcode later simply
won't find it in the cache. And that's perfectly fine.
setup_online_cpu() still needs to run on each CPU unconditionally,
though, in order to setup the sysfs groups.
Thx.
On Thu, Oct 20, 2022 at 10:18:01AM +0200, Borislav Petkov wrote:
> On Wed, Oct 19, 2022 at 09:37:59PM +0200, Borislav Petkov wrote:
> > Ah ok, yes, I need to check ->request_microcode_fw's retval.
>
> On a second thought, no I don't have to: the request routine will try to
> load any new microcode but if it fails, ->apply_microcode later simply
> won't find it in the cache. And that's perfectly fine.
>
> setup_online_cpu() still needs to run on each CPU unconditionally,
> though, in order to setup the sysfs groups.
That's true, its benign for this step.
I tried rebase everything I have, (i need to drop 2 of my patches after
your 5 patch series is applied on top of 6.1.0-rc1.
Everything works fine, which is good news.
Except I can't get this new load you added in microcode_init() to work.
It always gets UCODE_NFOUND.
- BIOS had version X
- Early applied Y > X
- I copied a new version Z > Y.
But i only see the early loaded version. I haven't looked at it yet, but
will try to look today.
Cheers,
Ashok
On Thu, Oct 20, 2022 at 08:04:53AM -0700, Ashok Raj wrote:
> - BIOS had version X
> - Early applied Y > X
> - I copied a new version Z > Y.
When exactly do you copy a new version?
Please write down the exact steps you're doing.
On Fri, Oct 21, 2022 at 11:28:10AM +0200, Borislav Petkov wrote:
> On Thu, Oct 20, 2022 at 08:04:53AM -0700, Ashok Raj wrote:
> > - BIOS had version X
> > - Early applied Y > X
> > - I copied a new version Z > Y.
>
> When exactly do you copy a new version?
>
> Please write down the exact steps you're doing.
- Place a new version of microcode in /lib/firmware/intel-ucode/
- make install, it also ends up creating a new initrd
image with the added microcode.
- Now put another new version in the same default directory.
0000000 0001 0000 0081 2b00 2022 0906 06f8 0008
------------------^^^^^^^^^ New revision
0000020 220a 855c 0001 0000 0087 0000 7b80 0008
0000040 7c00 0008 0000 0000 0041 2b00 0000 0000
----------------------------^^^^^^^^^ minrev
- reboot
During boot, i see early update message
microcode: early update: 0x2b000041 -> 0x2b000070, date = 2022-08-22
0x41 is the version from BIOS, 0x70 is what got updated in early boot.
0x81 is sitting in the default directory.
After boot expect to see 0x81 as the revision. But its left with what was
loaded at initrd image. The filesystem still had 0x81, and can successfuly
load after boot is completed.
Cheers,
Ashok
On Fri, Oct 21, 2022 at 03:21:03AM -0700, Ashok Raj wrote:
> After boot expect to see 0x81 as the revision. But its left with what
> was loaded at initrd image.
Are you testing with all patches, including yours or just the 5 I sent
you?
> The filesystem still had 0x81, and can successfuly load after boot is
> completed.
By doing late load through sysfs or what does "can successfuly load"
mean exactly?
Hi Boris
On Fri, Oct 21, 2022 at 12:57:32PM +0200, Borislav Petkov wrote:
> On Fri, Oct 21, 2022 at 03:21:03AM -0700, Ashok Raj wrote:
> > After boot expect to see 0x81 as the revision. But its left with what
> > was loaded at initrd image.
>
> Are you testing with all patches, including yours or just the 5 I sent
> you?
I tested just with only your patches
I use Ubuntu
Did a unmkinitrd and verified that 0x70 is in it. After extract.
araj@araj-ucode:/tmp/temp/early2/kernel/x86/microcode$ od -x GenuineIntel.bin | grep "0070 2b00"
12102000 0001 0000 0070 2b00 2022 0822 06f8 0008
-------------------^^^^ ^^^^
12102060 0000 0000 00e0 0000 0001 0003 0070 2b00
Patches in my work tree.. I use stgit mostly
araj@araj-ucode:~/work/linux.git.trees$ qyet
1 + v2-bp1
2 + bp2
3 + bp3
4 + bp4
5 + bp5
Now just copy the 0x81 ucode in /lib/firmware/intel-ucode/
araj@araj-ucode:~/work/ucode$ od -x /lib/firmware/intel-ucode/06-8f-05 | head
0000000 0001 0000 0081 2b00 2022 0906 06f8 0008
------------------^^^^ ^^^^
After reboot that file is unchanged.
dmesg shows:
[ 0.000000] microcode: early update: 0x2b000041 -> 0x2b000070, date = 2022-08
-22
araj@araj-ucode:~$ cat /proc/cpuinfo | grep microcode | sort -u
microcode : 0x2b000070
root@araj-ucode:/home/araj# echo 1 > /sys/devices/system/cpu/microcode/reload
root@araj-ucode:/home/araj# cat /proc/cpuinfo | grep microcode | sort -u
microcode : 0x2b000081
dmesg:
[ 654.103784] microcode: Attempting late microcode loading - it is dangerous and taints the kernel.
[ 654.113807] microcode: You should switch to early loading, if possible.
[ 654.613154] microcode: update 0x2b000070 -> 0x2b000081, date = 2022-09-06
>
> > The filesystem still had 0x81, and can successfuly load after boot is
> > completed.
>
> By doing late load through sysfs or what does "can successfuly load"
> mean exactly?
I meant by echo 1 > reload
Cheers,
Ashok
On Fri, Oct 21, 2022 at 04:39:55AM -0700, Ashok Raj wrote:
> I tested just with only your patches
Yeah, it can't work:
[ 1.365806] microcode: microcode_init: will request
[ 1.365812] microcode: request_microcode_fw: name: intel-ucode/06-3a-09
[ 1.365999] platform microcode: loading /lib/firmware/updates/6.1.0-rc1+/intel-ucode/06-3a-09 failed for no such file or directory.
[ 1.366032] platform microcode: loading /lib/firmware/updates/intel-ucode/06-3a-09 failed for no such file or directory.
[ 1.366061] platform microcode: loading /lib/firmware/6.1.0-rc1+/intel-ucode/06-3a-09 failed for no such file or directory.
[ 1.366088] platform microcode: loading /lib/firmware/intel-ucode/06-3a-09 failed for no such file or directory.
[ 1.366092] firmware_class: _request_firmware: fw_get_filesystem_firmware: retval: -2
[ 1.366096] platform microcode: Direct firmware load for intel-ucode/06-3a-09 failed with error -2
[ 1.366174] firmware_class: request_firmware_direct: ret: -2
[ 1.366178] microcode: data file intel-ucode/06-3a-09 load failed
[ 1.366248] microcode: sig=0x306a9, pf=0x10, revision=0x21
[ 1.366380] microcode: Microcode Update Driver: v2.2.
because that happens before the hdd driver has even been loaded yet.
My assumption that late_initcall() is late enough was wrong.
But that's ok - it was a weird use case anyway. One is supposed to use
early loading anyway.
Ok, lemme remove that loading attempt and test them more.
Thx.
@@ -319,60 +319,6 @@ void reload_early_microcode(void)
}
}
-static void collect_cpu_info_local(void *arg)
-{
- struct cpu_info_ctx *ctx = arg;
-
- ctx->err = microcode_ops->collect_cpu_info(smp_processor_id(),
- ctx->cpu_sig);
-}
-
-static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig)
-{
- struct cpu_info_ctx ctx = { .cpu_sig = cpu_sig, .err = 0 };
- int ret;
-
- ret = smp_call_function_single(cpu, collect_cpu_info_local, &ctx, 1);
- if (!ret)
- ret = ctx.err;
-
- return ret;
-}
-
-static int collect_cpu_info(int cpu)
-{
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- int ret;
-
- memset(uci, 0, sizeof(*uci));
-
- ret = collect_cpu_info_on_target(cpu, &uci->cpu_sig);
- if (!ret)
- uci->valid = 1;
-
- return ret;
-}
-
-static void apply_microcode_local(void *arg)
-{
- enum ucode_state *err = arg;
-
- *err = microcode_ops->apply_microcode(smp_processor_id());
-}
-
-static int apply_microcode_on_target(int cpu)
-{
- enum ucode_state err;
- int ret;
-
- ret = smp_call_function_single(cpu, apply_microcode_local, &err, 1);
- if (!ret) {
- if (err == UCODE_ERROR)
- ret = 1;
- }
- return ret;
-}
-
/* fake device for request_firmware */
static struct platform_device *microcode_pdev;
@@ -458,7 +404,7 @@ static int __reload_late(void *info)
* below.
*/
if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu)
- apply_microcode_local(&err);
+ err = microcode_ops->apply_microcode(cpu);
else
goto wait_for_siblings;
@@ -480,7 +426,7 @@ static int __reload_late(void *info)
* revision.
*/
if (cpumask_first(topology_sibling_cpumask(cpu)) != cpu)
- apply_microcode_local(&err);
+ err = microcode_ops->apply_microcode(cpu);
return ret;
}
@@ -589,51 +535,15 @@ static void microcode_fini_cpu(int cpu)
microcode_ops->microcode_fini_cpu(cpu);
}
-static enum ucode_state microcode_resume_cpu(int cpu)
-{
- if (apply_microcode_on_target(cpu))
- return UCODE_ERROR;
-
- pr_debug("CPU%d updated upon resume\n", cpu);
-
- return UCODE_OK;
-}
-
-static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
-{
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- enum ucode_state ustate;
-
- if (uci->valid)
- return UCODE_OK;
-
- if (collect_cpu_info(cpu))
- return UCODE_ERROR;
-
- /* --dimm. Trigger a delayed update? */
- if (system_state != SYSTEM_RUNNING)
- return UCODE_NFOUND;
-
- ustate = microcode_ops->request_microcode_fw(cpu, µcode_pdev->dev, refresh_fw);
- if (ustate == UCODE_NEW) {
- pr_debug("CPU%d updated upon init\n", cpu);
- apply_microcode_on_target(cpu);
- }
-
- return ustate;
-}
-
-static enum ucode_state microcode_update_cpu(int cpu)
+static enum ucode_state microcode_init_cpu(int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- /* Refresh CPU microcode revision after resume. */
- collect_cpu_info(cpu);
+ memset(uci, 0, sizeof(*uci));
- if (uci->valid)
- return microcode_resume_cpu(cpu);
+ microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
- return microcode_init_cpu(cpu, false);
+ return microcode_ops->apply_microcode(cpu);
}
/**
@@ -651,14 +561,14 @@ void microcode_bsp_resume(void)
}
static struct syscore_ops mc_syscore_ops = {
- .resume = microcode_bsp_resume,
+ .resume = microcode_bsp_resume,
};
static int mc_cpu_starting(unsigned int cpu)
{
- microcode_update_cpu(cpu);
- pr_debug("CPU%d added\n", cpu);
- return 0;
+ enum ucode_state err = microcode_ops->apply_microcode(cpu);
+
+ return err == UCODE_ERROR;
}
static int mc_cpu_online(unsigned int cpu)
@@ -688,11 +598,13 @@ static int mc_cpu_down_prep(unsigned int cpu)
static void setup_online_cpu(void *info)
{
int cpu = smp_processor_id();
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
- memset(uci, 0, sizeof(*uci));
+ enum ucode_state err;
- microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+ err = microcode_init_cpu(cpu);
+ if (err == UCODE_ERROR) {
+ pr_err("Error applying microcode on CPU%d\n", cpu);
+ return;
+ }
mc_cpu_online(cpu);
}
@@ -740,6 +652,13 @@ static int __init microcode_init(void)
goto out_pdev;
}
+ /*
+ * Try to load microcode once on the BSP in case the initrd has older revision.
+ * Frankly, I have no clue how that can happen but hey, loading here is cheap so
+ * why not.
+ */
+ microcode_ops->request_microcode_fw(boot_cpu_data.cpu_index, µcode_pdev->dev, true);
+
/* Do per-CPU setup */
cpus_read_lock();
on_each_cpu(setup_online_cpu, NULL, 0);