lkml.org 
[lkml]   [2022]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] x86/microcode/intel: Allow late loading only if a min rev is specified
On Mon, Aug 29, 2022 at 10:41:56PM +0000, Ashok Raj wrote:
> But I suppose, you mean what refresh_hw is supposed to mean from the
> existing code?

I've been meaning this for a while now.

> refresh_hw seems to imply when to update the copy of the microcode from
> the filesystem. Also seems to imply late loading.

After your patch:

$ git grep refresh_fw arch/x86/
arch/x86/kernel/cpu/microcode/core.c:601:static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
arch/x86/kernel/cpu/microcode/core.c:616: ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev, refresh_fw);

$ git grep late_loading
arch/x86/include/asm/microcode.h:36: bool late_loading);
arch/x86/kernel/cpu/microcode/amd.c:894: bool late_loading)
arch/x86/kernel/cpu/microcode/amd.c:903: if (!late_loading || !bsp)
arch/x86/kernel/cpu/microcode/intel.c:166:static int microcode_sanity_check(void *mc, int print_err, bool late_loading)
...

Now you have both. More mess.

> its used in the following places.
>
> 1. During reload_store() where this is exclicitly due to echo 1 > reload
>
> tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);
>
> Here passing true makes sense since you are going to do a full
> refresh on all CPUs via late loading.

Yes, and here it is perfectly clear that it is late loading.

> 2. microcode_update_cpu() -> microcode_init_cpu()->request_microcode_fw(false)
>
> Early loading from resume.

CPU hotplug rather.

> So we would use the microcode cache to load from.

So this happens when the CPU is coming online. I'm not sure why I set it
to "false" back then - whether it is because there's no filesystem yet
or there was another reason. I *think* this was some contrived used case
again.

In any case, this'll need to be experimented with to figure out what
happens when it is set to "true".

> 3. mc_device_add() -> microcode_init_cpu(true)->request_microcode_fw(true)
>
> This seems like normal CPU hot-add, I'm not sure if refresh_fw=true is
> valid. A new CPU should also use from the cache, but not a full reload
> from filesystem. This could end up with new cpu with an updated ucode
> and older with something that was loaded earlier.

Just check when mc_device_add() is actually called and then try to
figure out what that actually does instead of speculating.

> Sort of what was fixed in:
> commit 7189b3c11903667808029ec9766a6e96de5012a5 (tag: x86_microcode_for_v5.13)

That's a tag - not a git commit.

I think you mean

7189b3c11903 ("x86/microcode: Check for offline CPUs before requesting new microcode")

> intel.c doesn't seem to use this parameter at all today. But judging from
>
> amd.c: request_microcode_amd()
>
> /* reload ucode container only on the boot cpu */
> if (!refresh_fw || !bsp)
> return UCODE_OK;
>
> Seems like it does the right job, since you would refresh only if
> refresh_fw=true and its the bsp. Hence it seems immute to the
> mc_device_add() bug.

I don't see a bug there.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

\
 
 \ /
  Last update: 2022-09-01 04:54    [W:0.039 / U:3.452 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site