Messages in this thread | | | Date | Tue, 23 Jul 2013 23:44:45 +0200 | Subject | Re: [PATCH]Fix early microcode loading on AMD | From | Torsten Kaiser <> |
| |
On Tue, Jul 23, 2013 at 5:15 PM, Borislav Petkov <bp@alien8.de> wrote: > On Tue, Jul 23, 2013 at 01:58:53PM +0200, Torsten Kaiser wrote: >> Fixup the early AMD microcode loading. >> >> * load_microcode_amd() (and the helper its using) should not have an >> cpu parameter. > > Hmm, I don't think so - we get the cpu handed down from microcode_core > and besides the early load on 32bit needs to do find_patch(cpu). > >> The microcode loading is not depending on the CPU it is > > Mostly. There are mixed-stepping boxes which need to differentiate > between which cpu we're applying the patch for.
I redid the patch in 5 parts, hopefully now better to understand. Without the other changes the microcode_amd.c-part of patch 5/5 should make it much more obvious that my change did not result in a different behavior about which patches get loaded into the microcode cache 'pcache'.
> Btw, your config boots on my F14h box with "nomodeset" on the command > line because it is missing radeon firmware for my gpu. > >> executed and all the loaded patches will end up in a global list for all >> CPUs anyway. >> * Return -1 (like Intels apply_microcode) when the loading fails, >> also do not set the active microcode level on failure. > > Yep, this part I want. Please send it as a separate patch.
That is now patch 1/5. Patch 2/5 is new, I skipped that part originally because I did not want to make it even bigger...
> So I see a couple of issues in this patch and they should be separated > into single patches - one patch taking care of one issue and explaining > what the problem is in the commit message (I know you can do that good > :)).
I'm still seeing some things in the microcode code that look suspicious:
Why is the X86_64 code updating uci->cpu_sig.rev, but the 32bit version does not? And I can't see anything that reads that value.
Should apply_microcode_amd() really update uci->mc even before checking if the microcode is newer?
The X86_32 hunk in save_microcode_in_initrd_amd() now seems obsolete. load_microcode_amd() is no longer using find_patch() so it doesn't use ucode_cpu_info anymore. But why is that code using boot_cpu_data.cpu_index to find the BSP but always then passing 0 as cpu parameter to load_microcode_amd()? If boot_cpu_data.cpu_index is ever !=0 that code would fail.
... and collect_cpu_info_amd() also looks very weird. If csig would not point to uci->cpu_sig then find_patch() will not be happy. Wouldn't directly passing cpuid_eax(0x00000001) to find_patch() be a better interface? Then the early microcode loading code would not need to access ucode_cpu_info at all.
Torsten
| |