Re: [PATCH]Fix early microcode loading on AMD

Borislav Petkov
2013-07-24T14:19:37Z

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.

>
> > 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?

> >> * 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...

Btw, thanks for looking at this and asking critical questions!