Re: [PATCH]Fix early microcode loading on AMD

Torsten Kaiser
2013-07-24T15:01:41Z

On Wed, Jul 24, 2013 at 4:19 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, Jul 23, 2013 at 06:57:12PM +0200, Torsten Kaiser wrote:
>> > The other problem I see is not updating c->microcode since it is going
>> > to be overwritten by smp_store_cpu_info, which is unfortunate.
>> >
>> > And I don't see where Intel are updating that cpuinfo_x86.microcode
>> > field on early load too.
>> >
>> > So, AFAICT, c->microcode would remain unset when we only do early
>> > microcode load. But that is something we should fix as a later patch.
>>
>> I don't see a problem with that staying unset.
>> apply_microcode_amd() directly reads the rev from
>> MSR_AMD64_PATCH_LEVEL so it does not depend on that being correct.
>> And smp_store_(boot)_cpu_info will also read the current rev directly
>> from the CPU to fill ->microcode.
>
> We need to store the actual microcode revision to c->microcode for
> /proc/cpuinfo and MCE.

init_amd() will fill that field. (You could alway compile with
CONFIG_MICROCODE_AMD=n and that field would still need filling)
And as that will get called before smp_store_(boo)_cpu_info()
everything should be fine.

>> > So I think you should switch load_ucode_amd_ap to __apply_microcode_amd:
>> >
>> > p = find_patch()
>> >
>> > __apply_microcode_amd(p->mc_data);
>> >
>> > which should take care of the issue you're seeing, IMHO.
>>
>> The issue I'm seeing is that collect_cpu_info_amd_early() fills c->x86
>> but not c->x86_vendor.
>> Which breaks cpu_has_amd_erratum() and then Erratum 400 breaks the boot.
>>
>> I did not really want to switch from apply_microcode_amd() to
>> __apply_microcode_amd() because then I would lose the check if the new
>> microcode is really an upgrade.
>
> Well, if the BSP has already loaded the pcache, there's no need for
> the AP to parse and load the same microcode blobs file for the initrd,
> right?

loading != applying.

load_ucode_amd_ap() should probably called apply_ucode_amd_ap()
because that is primarily for applying the microcode.
That it also loads it (but really only once thanks to ucode_loaded) is
only because nobody else has run yet.

That whole place is hairy: Because on 32bit that seems to run much
earlier the 64 and 32 cases are very different.
64bit can and will use pcache/apply_microcode_amd() for the non BSP
CPUs, but on 32 bit everything directly applys the patches from initrd
memory into the CPUs be directly calling __apply_microcode_amd(). And
so bypassing pcache.

See comment above the 32bit version of load_ucode_amd_ap():
/*
* On 32-bit, since AP's early load occurs before paging is turned on, we
* cannot traverse cpu_equiv_table and pcache in kernel heap memory. So during
* cold boot, AP will apply_ucode_in_initrd() just like the BSP. During
* save_microcode_in_initrd_amd() BSP's patch is copied to amd_bsp_mpb, which
* is used upon resume from suspend.
*/

As written in the other email: I'm currently trying to see if I can
kill amd_bsp_mpb...

>> >> * load_ucode_ap(): Quick exit for !cpu, because without load_microcode_amd()
>> >> getting called apply_microcode_amd() can't do anything. Exit, if no microcode
>> >> could be loaded.
>> >
>> > This could probably be a WARN_ON(!cpu) to catch errors...
>>
>> No, load_ucode_ap() will be called for cpu == 0.
>
> This needs fixing IMO...

Can't answer that. I have only seen that it is called for cpu == 0 and
that there is no special case für CPU#0 in all the places that call
load_ucode_ap()...

> Btw, thanks for looking at this and asking critical questions!
>
> --
> Regards/Gruss,
> Boris.
>
> Sent from a fat crate under my desk. Formatting is fine.
> --