On Wed, Jul 24, 2013 at 3:41 PM, Borislav Petkov <b...@alien8.de> wrote: > Let me try to answer this as well as I can, peacemeal-wise. > > On Tue, Jul 23, 2013 at 06:57:12PM +0200, Torsten Kaiser wrote: >> On Tue, Jul 23, 2013 at 5:15 PM, Borislav Petkov <b...@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). >> >> Thats why I moved that part into apply_microcode_amd(). See later on >> more, why I think that move is the right thing. >> And without that the current cpu parameter will only be used to get >> the (in the early load case not even correctly set up!) per-cpu data. >> But the only member of cpuinfo_x86 that gets uses is ->x86, the family. >> Line 159: switch(c->x86) and Line 301: if (proc_fam!)c->x86) >> >> I really wanted to make that switch from cpu to x86family a separate >> patch, that it would be more obvious correct, but because of that >> amd_bsp_mpb hunk I can't find a good cut and thats why this patch is >> larger that I would have preferred. > > Ok.
First moving that hunk, then switching from cpu to x86family did work. See patch 4/5 and 5/5. :-) >> >> 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. >> >> Nothing looks at ->x86_model or ->x86_mask during load. >> It will always load all patches from the current family. > > Yes, that's the idea. We want to have all patches for the current family > loaded. And thats why switching from cpu to x86family is OK: during *load* we only care for the family. >> If loading would really depend on the current cpu in a mixed >> system that would be horrible: Depending on which CPU gets execute >> load_microcode_amd() it there would be different patches loaded into >> RAM? > > No, we load the microcode based on CPUID(1).EAX which is in the > equivalence table. Look at find_equiv_id(). > > But for that we need all patches belonging to the current family to be > in the cache. I think you confused *load* and *apply*. load_microcode_amd() *loads* the microcode from a firmwarefile into the pcache list. This wants all patches for the family and thats why my switch here is OK. apply_microcode_amd() *applies* the microcode to the CPU / "loads" it into the CPU. That function (or better its helper find_patch()) need the full stepping/masking. I did not change that function, because in that case 'cpu' makes sense as a parameter, because the microcode needs to be applied for each CPU. (You could argue that that parameter is also stupid: If you ever pass something else as raw_smp_processor_id() then it will BUG(). But removing that parameter would need to change the whole microcode_core.c and also microcode_intel.c. And there that parameter might make sense, so it's better to keep 'cpu' for apply_microcode_amd()) But wrt. you concern about mixed stepping systems: There early microcode loading is definitly broken for 32bit. The current mainline code will save the patch for the BSP in amd_bsp_mpb and then apply that to all CPUs irregardless of its stepping. With my change in 4/5 to move the amd_bsp_mpb setup to apply time it will now wrongly patch all CPUs with the microcode that was loaded last. But u8 amd_bsp_mpb[NR_CPUS][MPB_MAX_SIZE] doesn't look like a good idea. Maybe the best way here is to fail apply_microcode_amd() if amd_bsp_mpb already contains an incompatible patch and in load_ucode_amd_ap() only apply it when the cpu_sig matches. Or u8 amd_bsp_mpb[4][MPB_MAX_SIZE] which would support up to 4 different steppings per system. No patch yet, because I do not understand why that is not a problem on 64bit. load_ucode_amd_bsp() is shared between 32 and 64 so if that code works then I can't really find a need for amd_bsp_mpb at all. So my current plan is to look into who calls load_ucode_amd_bsp() and load_ucode_amd_ap() and in what sequence (..hopefully in the same sequence on 32 and 64bit...) and if I can find a rational why amd_bsp_mpb can be killed, I will send you a patch. Otherwise I will try to create something that will fail apply_microcode_amd() in a safe way, if CONFIG_MICROCODE_AMD_EARLY gets uses on a mixed system. >> > Btw, your config boots on my F14h box with "nomodeset" on the command >> > line because it is missing radeon firmware for my gpu. >> >> I suspect a F14h box will never see that hang. It trips over the the >> C1E erratum and amd_erratum_400[] looks like it only affects 0xfh and >> 0x10h (like my Phenom II X6). > > I could fire up my F10h if needed :) > >> >> 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. >> >> OK, will send that together with the resend for cpu_has_amd_erratum(). > > Ok. > > -- > Regards/Gruss, > Boris. > > Sent from a fat crate under my desk. Formatting is fine. > -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
