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

[Evalds Iodzevics]

On Mon, Apr 20, 2020 at 4:33 PM Ben Hutchings <ben@xxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxx>
> > 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@xxxxxxx>
> >     Link: http://lkml.kernel.org/r/20170109114147.5082-4-bp@xxxxxxxxx
> >     Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> >     [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.
>
As far as i can tell, these two:

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

are already made obsolete by:

98cc1464cfd6  x86/microcode/intel: Add a helper which gives the
microcode revision

except that it still relied on sync_core() witch did not behave
correctly on 32 bit machines