Re: [patch 1/2] x86/microcode/32: Move early loading after paging enable

[Ingo Molnar <mingo@kernel.org>]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> 32-bit loads microcode before paging is enabled. The commit which
> introduced that has zero justification in the changelog. The cover letter
> has slightly more content, but it does not give any technical justification
> either:
> 
>   "The problem in current microcode loading method is that we load a
>    microcode way, way too late; ideally we should load it before turning
>    paging on.  This may only be practical on 32 bits since we can't get to
>    64-bit mode without paging on, but we should still do it as early as at
>    all possible."
> 
> Handwaving word salad with zero technical content.
> 
> Someone claimed in an offlist conversation that this is required for curing
> the ATOM erratum AAE44/AAF40/AAG38/AAH41. That erratum requires an
> microcode update in order to make the usage of PSE safe. But during early
> boot PSE is completely irrelevant and it is evaluated way later.
> 
> Neither is it relevant for the AP on single core HT enabled CPUs as the
> microcode loading on the AP is not doing anything.
> 
> On dual core CPUs there is a theoretical problem if a split of an
> executable large page between enabling paging including PSE and loading the
> microcode happens. But that's only theoretical, it's practically irrelevant
> because the affected dual core CPUs are 64bit enabled and therefore have
> paging and PSE enabled before loading the microcode on the second core. So
> why would it work on 64-bit but not on 32-bit?
> 
> The erratum:
> 
>   "AAG38 Code Fetch May Occur to Incorrect Address After a Large Page is
>    Split Into 4-Kbyte Pages
> 
>    Problem: If software clears the PS (page size) bit in a present PDE
>    (page directory entry), that will cause linear addresses mapped through
>    this PDE to use 4-KByte pages instead of using a large page after old
>    TLB entries are invalidated. Due to this erratum, if a code fetch uses
>    this PDE before the TLB entry for the large page is invalidated then it
>    may fetch from a different physical address than specified by either the
>    old large page translation or the new 4-KByte page translation. This
>    erratum may also cause speculative code fetches from incorrect addresses."
> 
> The practical relevance for this is exactly zero because there is no
> splitting of large text pages during early boot-time, i.e. between paging
> enable and microcode loading, and neither during CPU hotplug.
> 
> IOW, this load microcode before paging enable is yet another voodoo
> programming solution in search of a problem. What's worse is that it causes
> at least two serious problems:
> 
>  1) When stackprotector is enabled then the microcode loader code has the
>     stackprotector mechanics enabled. The read from the per CPU variable
>     __stack_chk_guard is always accessing the virtual address either
>     directly on UP or via FS on SMP. In physical address mode this results
>     in an access to memory above 3GB. So this works by chance as the
>     hardware returns the same value when there is no RAM at this physical
>     address. When there is RAM populated above 3G then the read is by
>     chance the same as nothing changes that memory during the very early
>     boot stage. That's not necessarily true during runtime CPU hotplug.
> 
>  2) When function tracing is enabled, then the relevant microcode loader
>     functions and the functions invoked from there will call into the
>     tracing code and evaluate global and per CPU variables in physical
>     address mode. What could potentially go wrong?
> 
> Cure this and move the microcode loading after the early paging enable and
> remove the gunk in the microcode loader which is required to handle
> physical address mode.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Peter Anvin <hpa@zytor.com>
> Link: https://lore.kernel.org/lkml/1356075872-3054-1-git-send-email-fenghua.yu@intel.com
> ---
>  arch/x86/kernel/cpu/microcode/amd.c      |   31 +-------
>  arch/x86/kernel/cpu/microcode/core.c     |   40 ++---------
>  arch/x86/kernel/cpu/microcode/intel.c    |  108 +++----------------------------
>  arch/x86/kernel/cpu/microcode/internal.h |    2 
>  arch/x86/kernel/head32.c                 |    3 
>  arch/x86/kernel/head_32.S                |   10 --
>  arch/x86/kernel/smpboot.c                |   12 +--
>  7 files changed, 35 insertions(+), 171 deletions(-)

Reviewed-by: Ingo Molnar <mingo@kernel.org>

Frankly, the general principle is that we should run as little Linux kernel 
code before paging is enabled as possible.

If a system is so broken that it requires a microcode update before it can 
even enable paging is IMO so terminally broken that it should be firmware 
patched at the factory (to have a proper microcode), or recalled from the 
market. It's not something we should make the kernel more fragile for.

Thanks,

	Ingo