Re: [PATCH] x86/microcode/intel: replace sync_core() with native_cpuid_reg(eax)

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 2496 bytes --]

On Mon, 2020-04-20 at 11:31 +0200, Borislav Petkov wrote:
> On Mon, Apr 20, 2020 at 03:00:37PM +0300, Evalds Iodzevics wrote:
> > sync_core() always jums past cpuid instruction on 32 bit machines
> > because data structure boot_cpu_data are not populated so early in boot.
> 
> I'm guessing because boot_cpu_data.cpuid_level is not properly set and
> very early code in head_32.S sets it to -1 temporarily until the highest
> CPUID level has been detected (or not).
> 
> But the microcode loading happens *before* that.
[...]
> Hrm, the original patch of mine did use native_cpuid_eax():
> 
> 4167709bbf82 ("x86/microcode/intel: Add a helper which gives the microcode revision")
> 
> but the backport:
> 
> commit 98cc1464cfd6edf9dc7fa96aaaf596aae952029b
> Author: Borislav Petkov <bp@suse.de>
> Date:   Mon Jan 9 12:41:45 2017 +0100
> 
>     x86/microcode/intel: Add a helper which gives the microcode revision
>     
>     commit 4167709bbf826512a52ebd6aafda2be104adaec9 upstream.
>     
>     Since on Intel we're required to do CPUID(1) first, before reading
>     the microcode revision MSR, let's add a special helper which does the
>     required steps so that we don't forget to do them next time, when we
>     want to read the microcode revision.
>     
>     Signed-off-by: Borislav Petkov <bp@suse.de>
>     Link: http://lkml.kernel.org/r/20170109114147.5082-4-bp@alien8.de
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>     [bwh: Backported to 4.4:
>      - Don't touch prev_rev variable in apply_microcode()
>      - Keep using sync_core(), which will alway includes the necessary CPUID
>      	^^^^^^^^^^^^^^^^^^^
> 
> decided to use sync_core() for whatever reason. Perhaps because the
> native_cpuid* things weren't there. Adding Ben to Cc.

This commit didn't introduce the call to native_cpuid_eax(), but only
moved it.  So I didn't think it made sense for the backport to change
from sync_core().

If it's important to use native_cpuid_eax() then these older branches
should presumably get backports of:

484d0e5c7943 x86/microcode/intel: Replace sync_core() with native_cpuid()
f3e2a51f568d x86/microcode: Use native CPUID to tickle out microcode revision

Ben.

> I believe this is the background info Greg needed to figure out *why*
> you're doing this.
> 
-- 
Ben Hutchings
Klipstein's 4th Law of Prototyping and Production:
                               A fail-safe circuit will destroy others.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]