Re: [PATCH]Fix early microcode loading on AMD
On Wed, Jul 24, 2013 at 05:01:39PM +0200, Torsten Kaiser wrote: > 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. Ah yes, I missed that. Thanks. -- 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/
Re: [PATCH]Fix early microcode loading on AMD
On Wed, Jul 24, 2013 at 04:44:45PM +0200, Torsten Kaiser wrote:
> Then it would probably be the best to kill free_cache() completely.
> Which would mean cleanup() should also go.
> Which will make unloading microcode_amd.ko impossible.
> But that is probably a good idea anyway: If you unload the module
> there is no way to keep pcache.
>
> But I still have another way to kill you: free_equiv_cpu_table()
> Without that table find_patch() can't work and will not return the
> correct information.
>
> And that can be triggered by:
> * start of load_microcode_amd(): If you reach that function (Only
> UCODE_MAGIC needs to be in the file) that table is dead.
> * __load_microcode_amd(): If the file only contains the table but no
> patches ("invalid type field in container file section header\n")
If you have root, there are a gazillion ways to kill the system. Those
are just a couple more, and a bit complicated even :)
> But it already called free_equiv_cpu_table() and so pcache is inaccessible.
That's the old table. __load_microcode_amd() installs the current one.
Ok, I remember now, the situation is a bit different: the container file
has all patches. If we load a new container file, it contains newer
replacements + old ones which remained unchanged:
http://marc.info/?l=linux-kernel=137427483928693
So actually, when we encounter an error when loading a new blob,
we have to *keep* the old pcache and equiv_table. Which means
load_microcode_amd() shouldn't free the table *before* doing
__load_microcode_amd() but *after* verifying it succeeded.
> And I don't think just preserving equiv_cpu_table for restoring in
> the error case will be the right solution: If the new firmware file
> contains a new table with fewer entries (or different entries!) some
> of the patches in pcache might become inaccessible.
Yes, see above.
> >> That copying already in load_microcode also is suspicious if someone
> >> would only load the microcode but not apply it. But I did not find
> >> a codepath in arch/x86/kernel/microcode_core.c to load it without a
> >> followup apply.
> >
> > Yeah, we always load and apply.
> >
> > So now back to the original problem - load_microcode_amd() shouldn't
> > clear the pcache and, in that case, a subsequent find_patch() would
> > always give the right patch.
>
> Not if equiv_cpu_table got mangled.
> So should install_equiv_cpu_table() be turned into
> add_to_equiv_cpu_table() or should pcache save all cpu_sig with each
> patch, so that find_patch() no longer needs equiv_cpu_table?
> I suspect saving that in struct ucode_patch might be better, to
> prevent changes in equiv_id <-> cpu_sig mapping to make a patch
> inaccessible.
Yeah. I remembered :) See above.
Thanks.
--
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/
Re: [PATCH]Fix early microcode loading on AMD
On Wed, Jul 24, 2013 at 04:20:58PM +0200, Torsten Kaiser wrote: > First moving that hunk, then switching from cpu to x86family did work. > See patch 4/5 and 5/5. :-) I will get to those eventually. > > 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*. No I didn't - I meant what I said, I simply pointed at the wrong function. find_cpu_family_by_equiv_cpu() at the beginning of verify_and_add_patch() does look at the family when *loading*. > 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()) No reason to deal with it if it is only a hypothetical issue. > 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. Yes, I think 4 should be more than enough. And I think this should be prepared in save_microcode_in_initrd_amd() like this: look at all APs - if they have mixed steppings, save the following mapping: CPUID(1).EAX -> patch blob. Then, at load_ucode_amd_ap() during resume from suspend, we get do CPUID(1) on the current AP, get the patch blob and apply it. Something like that... > 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. On 64-bit we create the pcache with the first call to load_microcode_amd() on the first AP. For all the subsequent APs, we do find_patch(cpu) so no need to cache a BSP patch. > 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. See above. > 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. Remember: we either *must* apply a microcode patch on *all* cores or not at all. But with the suggestion above I think that should be not that hard. Thanks. -- 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/
Re: [PATCH]Fix early microcode loading on AMD
On Wed, Jul 24, 2013 at 4:19 PM, Borislav Petkov 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. > -- -- 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/
Re: [PATCH]Fix early microcode loading on AMD
On Wed, Jul 24, 2013 at 3:56 PM, Borislav Petkov wrote:
> On Tue, Jul 23, 2013 at 06:57:12PM +0200, Torsten Kaiser wrote:
>> >> * Save the amd_bsp_mpb on apply, not on load. Otherwise someone could
>> >> later load an older microcode file that would overwrite amd_bsp_mpb,
>> >> but would not be applied to the CPUs
>> >
>> > See the patch id check in apply_ucode_in_initrd()?
>> >
>> > if (eq_id == mc->hdr.processor_rev_id && rev < mc->hdr.patch_id)
>>
>> I meant with "load" load_microcode_amd() not the loading of the
>> microcode into the CPU.
>>
>> 1.: load microcode rev X into CPU (early or normal is not important)
>> 2.: get older microcode file that only contains rev Y with Y> 3.: trigger load_microcode_amd() with a corrupt file: This will call
>> cleanup() and empty pcache.
>
> Ok, that's actually a good catch. So I wonder: why in hell would we
> flush the pcache if some of the blobs we're loading are corrupted. So
> what?! Jacob, what were you thinking - I'd be very interested to know
> what the idea behind this was.
>
> So, just to refresh everybody: the idea of the pcache is to keep all
> patches for the current family in memory so that we can support all
> sorts of hotplug and cpu mixed stepping diddling.
Then it would probably be the best to kill free_cache() completely.
Which would mean cleanup() should also go.
Which will make unloading microcode_amd.ko impossible.
But that is probably a good idea anyway: If you unload the module
there is no way to keep pcache.
But I still have another way to kill you: free_equiv_cpu_table()
Without that table find_patch() can't work and will not return the
correct information.
And that can be triggered by:
* start of load_microcode_amd(): If you reach that function (Only
UCODE_MAGIC needs to be in the file) that table is dead.
* __load_microcode_amd(): If the file only contains the table but no
patches ("invalid type field in container file section header\n")
>> 4.: trigger load_microcode_amd() with the older file:
>> * this will now load rev Y into pcache
>> * rev Y will be returned by find_patch and copied into amd_bsp_mpb
>> * any try to apply rev Y will be skipped in apply_microcode_amd()
>>
>> So now the CPU still correctly has rev X, but amd_bsp_mpb will contain
>> the wrong rev Y.
>
> Right, so this shouldn't happen - what should happen is, pcache would
> hold both X and Y and find_patch would automatically give you the right
> one.
>
> And this is guaranteed since we keep the patches in a sorted linked list
> by ->patch_id which is guaranteed to be increasing.
>
> So actually load_microcode_amd() shouldn't be doing cleanup() but simply
> return ret upwards.
But it already called free_equiv_cpu_table() and so pcache is inaccessible.
And I don't think just preserving equiv_cpu_table for restoring in the
error case will be the right solution: If the new firmware file
contains a new table with fewer entries (or different entries!) some
of the patches in pcache might become inaccessible.
>> That copying already in load_microcode also is suspicious if someone
>> would only load the microcode but not apply it. But I did not find
>> a codepath in arch/x86/kernel/microcode_core.c to load it without a
>> followup apply.
>
> Yeah, we always load and apply.
>
> So now back to the original problem - load_microcode_amd() shouldn't
> clear the pcache and, in that case, a subsequent find_patch() would
> always give the right patch.
Not if equiv_cpu_table got mangled.
So should install_equiv_cpu_table() be turned into
add_to_equiv_cpu_table() or should pcache save all cpu_sig with each
patch, so that find_patch() no longer needs equiv_cpu_table?
I suspect saving that in struct ucode_patch might be better, to
prevent changes in equiv_id <-> cpu_sig mapping to make a patch
inaccessible.
Torsten
--
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/
Re: [PATCH]Fix early microcode loading on AMD
On Wed, Jul 24, 2013 at 3:41 PM, Borislav Petkov 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 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)
Re: [PATCH]Fix early microcode loading on AMD
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! -- 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/
Re: [PATCH]Fix early microcode loading on AMD
On Tue, Jul 23, 2013 at 06:57:12PM +0200, Torsten Kaiser wrote:
> >> * Save the amd_bsp_mpb on every update. Otherwise someone could offline
> >> the BSP, update the microcode and this would be lost on resume
> >
> > Huh, is amd_bsp_mpb going to disappear all of a sudden?
> >
> > And that doesn't matter because when we online the BSP later, it goes
> > through the CPU hotplug notifier mc_cpu_callback. Or am I missing
> > something?
>
> Yeah, me correctly describing what I was meaning. ;-)
>
> 1.: boot system, BIOS give microcode rev. X
> 2.: offline the BSP
> 3.: update microcode to rev. Y with Y > X
Right, with cleanup() removed, when you do that step, you go through
load_microcode_amd() which adds the patch to the pcache with
__load_microcode_amd() and a subsequent find_patch will give you Y which
you memcpy to amd_bsp_mpb.
> Because the BSP is not online rev. Y will not be copied into amd_bsp_mpb
> 4.: supend
> 5.: resume, BIOS gives rev. X again
> 6.: amd_bsp_mpb is empty -> rev. Y will not be reapplied.
>
> >> * apply_ucode_in_initrd() now also needs to save amd_bsp_mbp, because
> >> load_microcode_amd() its no longer doing this and its not using
> >> apply_microcode_amd().
> >> * extract common checks and initialisations from load_ucode_ap() and
> >> load_microcode_amd() to load_microcode_amd_early(). The change from
> >> cpu to x86family in load_microcode_amd() allows to drop the code messing
> >> with cpu_data(cpu), with is wrong anyway because at that point the
> >> per-cpu cpu_info is not yet setup. And these values would later be
> >> overwritten by smp_store_boot_cpu_info() / smp_store_cpu_info().
> >
> > Right, so I was thinking about this. And the code is pretty nasty: we do a
> > load_ucode_amd_ap() but we do add the ucode for the BSP:
> >
> > if (load_microcode_amd(0, ucode, ucode_size) != UCODE_OK)
>
> No, that code will not be reached for the BSP, because it is behind:
That's correct - load_ucode_amd_ap() is not supposed to load ucode on
the BSP.
> if (cpu && !ucode_loaded) {
> The BSP has cpu == 0. Thats why I adding the following in my patch:
> + /* BSP via load_ucode_amd_bsp() */
> + if (!cpu)
> + return;
>
> I don't understand if that is really correct, but that was the
> original behavior, and I didn't feel competent enough to decree that
> calling load_microcode_amd() for the BSP would be save.
> (The code there is strange: There is a load_ucode_amd_bsp() but
> load_ucode_amd_ap() will also be called for the BSP.
Yes, this is strange and this is the confusing issue.
Here's how it should work: we want the BSP to load the microcode, put it
in the pcache and also write it into amd_bsp_mpb. The 32-bit version of
load_ucode_amd_ap() takes it and applies it because we run with paging
off at the time.
> And it seems the call to load_ucode_ap() for the BSP will come from a
> very different place (via trap_init()) that the other CPUs.
> And I did not even try to understand what X86_32 is doing...)
Yep. That question needs sorting too.
--
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/
Re: [PATCH]Fix early microcode loading on AMD
On Tue, Jul 23, 2013 at 06:57:12PM +0200, Torsten Kaiser wrote: > >> * Save the amd_bsp_mpb on apply, not on load. Otherwise someone could > >> later load an older microcode file that would overwrite amd_bsp_mpb, > >> but would not be applied to the CPUs > > > > See the patch id check in apply_ucode_in_initrd()? > > > > if (eq_id == mc->hdr.processor_rev_id && rev < mc->hdr.patch_id) > > I meant with "load" load_microcode_amd() not the loading of the > microcode into the CPU. > > 1.: load microcode rev X into CPU (early or normal is not important) > 2.: get older microcode file that only contains rev Y with Y 3.: trigger load_microcode_amd() with a corrupt file: This will call > cleanup() and empty pcache. Ok, that's actually a good catch. So I wonder: why in hell would we flush the pcache if some of the blobs we're loading are corrupted. So what?! Jacob, what were you thinking - I'd be very interested to know what the idea behind this was. So, just to refresh everybody: the idea of the pcache is to keep all patches for the current family in memory so that we can support all sorts of hotplug and cpu mixed stepping diddling. > 4.: trigger load_microcode_amd() with the older file: > * this will now load rev Y into pcache > * rev Y will be returned by find_patch and copied into amd_bsp_mpb > * any try to apply rev Y will be skipped in apply_microcode_amd() > > So now the CPU still correctly has rev X, but amd_bsp_mpb will contain > the wrong rev Y. Right, so this shouldn't happen - what should happen is, pcache would hold both X and Y and find_patch would automatically give you the right one. And this is guaranteed since we keep the patches in a sorted linked list by ->patch_id which is guaranteed to be increasing. So actually load_microcode_amd() shouldn't be doing cleanup() but simply return ret upwards. > That copying already in load_microcode also is suspicious if someone > would only load the microcode but not apply it. But I did not find > a codepath in arch/x86/kernel/microcode_core.c to load it without a > followup apply. Yeah, we always load and apply. So now back to the original problem - load_microcode_amd() shouldn't clear the pcache and, in that case, a subsequent find_patch() would always give the right patch. 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/
Re: [PATCH]Fix early microcode loading on AMD
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 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. > >> 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. > 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. > > 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/
Re: [PATCH]Fix early microcode loading on AMD
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. 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. 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. 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/
Re: [PATCH]Fix early microcode loading on AMD
On Tue, Jul 23, 2013 at 06:57:12PM +0200, Torsten Kaiser wrote: * Save the amd_bsp_mpb on apply, not on load. Otherwise someone could later load an older microcode file that would overwrite amd_bsp_mpb, but would not be applied to the CPUs See the patch id check in apply_ucode_in_initrd()? if (eq_id == mc-hdr.processor_rev_id rev mc-hdr.patch_id) I meant with load load_microcode_amd() not the loading of the microcode into the CPU. 1.: load microcode rev X into CPU (early or normal is not important) 2.: get older microcode file that only contains rev Y with YX 3.: trigger load_microcode_amd() with a corrupt file: This will call cleanup() and empty pcache. Ok, that's actually a good catch. So I wonder: why in hell would we flush the pcache if some of the blobs we're loading are corrupted. So what?! Jacob, what were you thinking - I'd be very interested to know what the idea behind this was. So, just to refresh everybody: the idea of the pcache is to keep all patches for the current family in memory so that we can support all sorts of hotplug and cpu mixed stepping diddling. 4.: trigger load_microcode_amd() with the older file: * this will now load rev Y into pcache * rev Y will be returned by find_patch and copied into amd_bsp_mpb * any try to apply rev Y will be skipped in apply_microcode_amd() So now the CPU still correctly has rev X, but amd_bsp_mpb will contain the wrong rev Y. Right, so this shouldn't happen - what should happen is, pcache would hold both X and Y and find_patch would automatically give you the right one. And this is guaranteed since we keep the patches in a sorted linked list by -patch_id which is guaranteed to be increasing. So actually load_microcode_amd() shouldn't be doing cleanup() but simply return ret upwards. That copying already in load_microcode also is suspicious if someone would only load the microcode but not apply it. But I did not find a codepath in arch/x86/kernel/microcode_core.c to load it without a followup apply. Yeah, we always load and apply. So now back to the original problem - load_microcode_amd() shouldn't clear the pcache and, in that case, a subsequent find_patch() would always give the right patch. 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/
Re: [PATCH]Fix early microcode loading on AMD
On Tue, Jul 23, 2013 at 06:57:12PM +0200, Torsten Kaiser wrote:
* Save the amd_bsp_mpb on every update. Otherwise someone could offline
the BSP, update the microcode and this would be lost on resume
Huh, is amd_bsp_mpb going to disappear all of a sudden?
And that doesn't matter because when we online the BSP later, it goes
through the CPU hotplug notifier mc_cpu_callback. Or am I missing
something?
Yeah, me correctly describing what I was meaning. ;-)
1.: boot system, BIOS give microcode rev. X
2.: offline the BSP
3.: update microcode to rev. Y with Y X
Right, with cleanup() removed, when you do that step, you go through
load_microcode_amd() which adds the patch to the pcache with
__load_microcode_amd() and a subsequent find_patch will give you Y which
you memcpy to amd_bsp_mpb.
Because the BSP is not online rev. Y will not be copied into amd_bsp_mpb
4.: supend
5.: resume, BIOS gives rev. X again
6.: amd_bsp_mpb is empty - rev. Y will not be reapplied.
* apply_ucode_in_initrd() now also needs to save amd_bsp_mbp, because
load_microcode_amd() its no longer doing this and its not using
apply_microcode_amd().
* extract common checks and initialisations from load_ucode_ap() and
load_microcode_amd() to load_microcode_amd_early(). The change from
cpu to x86family in load_microcode_amd() allows to drop the code messing
with cpu_data(cpu), with is wrong anyway because at that point the
per-cpu cpu_info is not yet setup. And these values would later be
overwritten by smp_store_boot_cpu_info() / smp_store_cpu_info().
Right, so I was thinking about this. And the code is pretty nasty: we do a
load_ucode_amd_ap() but we do add the ucode for the BSP:
if (load_microcode_amd(0, ucode, ucode_size) != UCODE_OK)
No, that code will not be reached for the BSP, because it is behind:
That's correct - load_ucode_amd_ap() is not supposed to load ucode on
the BSP.
if (cpu !ucode_loaded) {
The BSP has cpu == 0. Thats why I adding the following in my patch:
+ /* BSP via load_ucode_amd_bsp() */
+ if (!cpu)
+ return;
I don't understand if that is really correct, but that was the
original behavior, and I didn't feel competent enough to decree that
calling load_microcode_amd() for the BSP would be save.
(The code there is strange: There is a load_ucode_amd_bsp() but
load_ucode_amd_ap() will also be called for the BSP.
Yes, this is strange and this is the confusing issue.
Here's how it should work: we want the BSP to load the microcode, put it
in the pcache and also write it into amd_bsp_mpb. The 32-bit version of
load_ucode_amd_ap() takes it and applies it because we run with paging
off at the time.
And it seems the call to load_ucode_ap() for the BSP will come from a
very different place (via trap_init()) that the other CPUs.
And I did not even try to understand what X86_32 is doing...)
Yep. That question needs sorting too.
--
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/
Re: [PATCH]Fix early microcode loading on AMD
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! -- 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/
Re: [PATCH]Fix early microcode loading on AMD
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
Re: [PATCH]Fix early microcode loading on AMD
On Wed, Jul 24, 2013 at 3:56 PM, Borislav Petkov b...@alien8.de wrote: On Tue, Jul 23, 2013 at 06:57:12PM +0200, Torsten Kaiser wrote: * Save the amd_bsp_mpb on apply, not on load. Otherwise someone could later load an older microcode file that would overwrite amd_bsp_mpb, but would not be applied to the CPUs See the patch id check in apply_ucode_in_initrd()? if (eq_id == mc-hdr.processor_rev_id rev mc-hdr.patch_id) I meant with load load_microcode_amd() not the loading of the microcode into the CPU. 1.: load microcode rev X into CPU (early or normal is not important) 2.: get older microcode file that only contains rev Y with YX 3.: trigger load_microcode_amd() with a corrupt file: This will call cleanup() and empty pcache. Ok, that's actually a good catch. So I wonder: why in hell would we flush the pcache if some of the blobs we're loading are corrupted. So what?! Jacob, what were you thinking - I'd be very interested to know what the idea behind this was. So, just to refresh everybody: the idea of the pcache is to keep all patches for the current family in memory so that we can support all sorts of hotplug and cpu mixed stepping diddling. Then it would probably be the best to kill free_cache() completely. Which would mean cleanup() should also go. Which will make unloading microcode_amd.ko impossible. But that is probably a good idea anyway: If you unload the module there is no way to keep pcache. But I still have another way to kill you: free_equiv_cpu_table() Without that table find_patch() can't work and will not return the correct information. And that can be triggered by: * start of load_microcode_amd(): If you reach that function (Only UCODE_MAGIC needs to be in the file) that table is dead. * __load_microcode_amd(): If the file only contains the table but no patches (invalid type field in container file section header\n) 4.: trigger load_microcode_amd() with the older file: * this will now load rev Y into pcache * rev Y will be returned by find_patch and copied into amd_bsp_mpb * any try to apply rev Y will be skipped in apply_microcode_amd() So now the CPU still correctly has rev X, but amd_bsp_mpb will contain the wrong rev Y. Right, so this shouldn't happen - what should happen is, pcache would hold both X and Y and find_patch would automatically give you the right one. And this is guaranteed since we keep the patches in a sorted linked list by -patch_id which is guaranteed to be increasing. So actually load_microcode_amd() shouldn't be doing cleanup() but simply return ret upwards. But it already called free_equiv_cpu_table() and so pcache is inaccessible. And I don't think just preserving equiv_cpu_table for restoring in the error case will be the right solution: If the new firmware file contains a new table with fewer entries (or different entries!) some of the patches in pcache might become inaccessible. That copying already in load_microcode also is suspicious if someone would only load the microcode but not apply it. But I did not find a codepath in arch/x86/kernel/microcode_core.c to load it without a followup apply. Yeah, we always load and apply. So now back to the original problem - load_microcode_amd() shouldn't clear the pcache and, in that case, a subsequent find_patch() would always give the right patch. Not if equiv_cpu_table got mangled. So should install_equiv_cpu_table() be turned into add_to_equiv_cpu_table() or should pcache save all cpu_sig with each patch, so that find_patch() no longer needs equiv_cpu_table? I suspect saving that in struct ucode_patch might be better, to prevent changes in equiv_id - cpu_sig mapping to make a patch inaccessible. Torsten -- 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/
Re: [PATCH]Fix early microcode loading on AMD
On Wed, Jul 24, 2013 at 4:19 PM, Borislav Petkov b...@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. -- -- 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/
Re: [PATCH]Fix early microcode loading on AMD
On Wed, Jul 24, 2013 at 04:20:58PM +0200, Torsten Kaiser wrote: First moving that hunk, then switching from cpu to x86family did work. See patch 4/5 and 5/5. :-) I will get to those eventually. 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*. No I didn't - I meant what I said, I simply pointed at the wrong function. find_cpu_family_by_equiv_cpu() at the beginning of verify_and_add_patch() does look at the family when *loading*. 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()) No reason to deal with it if it is only a hypothetical issue. 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. Yes, I think 4 should be more than enough. And I think this should be prepared in save_microcode_in_initrd_amd() like this: look at all APs - if they have mixed steppings, save the following mapping: CPUID(1).EAX - patch blob. Then, at load_ucode_amd_ap() during resume from suspend, we get do CPUID(1) on the current AP, get the patch blob and apply it. Something like that... 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. On 64-bit we create the pcache with the first call to load_microcode_amd() on the first AP. For all the subsequent APs, we do find_patch(cpu) so no need to cache a BSP patch. 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. See above. 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. Remember: we either *must* apply a microcode patch on *all* cores or not at all. But with the suggestion above I think that should be not that hard. Thanks. -- 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/
Re: [PATCH]Fix early microcode loading on AMD
On Wed, Jul 24, 2013 at 04:44:45PM +0200, Torsten Kaiser wrote: Then it would probably be the best to kill free_cache() completely. Which would mean cleanup() should also go. Which will make unloading microcode_amd.ko impossible. But that is probably a good idea anyway: If you unload the module there is no way to keep pcache. But I still have another way to kill you: free_equiv_cpu_table() Without that table find_patch() can't work and will not return the correct information. And that can be triggered by: * start of load_microcode_amd(): If you reach that function (Only UCODE_MAGIC needs to be in the file) that table is dead. * __load_microcode_amd(): If the file only contains the table but no patches (invalid type field in container file section header\n) If you have root, there are a gazillion ways to kill the system. Those are just a couple more, and a bit complicated even :) But it already called free_equiv_cpu_table() and so pcache is inaccessible. That's the old table. __load_microcode_amd() installs the current one. Ok, I remember now, the situation is a bit different: the container file has all patches. If we load a new container file, it contains newer replacements + old ones which remained unchanged: http://marc.info/?l=linux-kernelm=137427483928693 So actually, when we encounter an error when loading a new blob, we have to *keep* the old pcache and equiv_table. Which means load_microcode_amd() shouldn't free the table *before* doing __load_microcode_amd() but *after* verifying it succeeded. And I don't think just preserving equiv_cpu_table for restoring in the error case will be the right solution: If the new firmware file contains a new table with fewer entries (or different entries!) some of the patches in pcache might become inaccessible. Yes, see above. That copying already in load_microcode also is suspicious if someone would only load the microcode but not apply it. But I did not find a codepath in arch/x86/kernel/microcode_core.c to load it without a followup apply. Yeah, we always load and apply. So now back to the original problem - load_microcode_amd() shouldn't clear the pcache and, in that case, a subsequent find_patch() would always give the right patch. Not if equiv_cpu_table got mangled. So should install_equiv_cpu_table() be turned into add_to_equiv_cpu_table() or should pcache save all cpu_sig with each patch, so that find_patch() no longer needs equiv_cpu_table? I suspect saving that in struct ucode_patch might be better, to prevent changes in equiv_id - cpu_sig mapping to make a patch inaccessible. Yeah. I remembered :) See above. Thanks. -- 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/
Re: [PATCH]Fix early microcode loading on AMD
On Wed, Jul 24, 2013 at 05:01:39PM +0200, Torsten Kaiser wrote: 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. Ah yes, I missed that. Thanks. -- 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/
Re: [PATCH]Fix early microcode loading on AMD
On Tue, Jul 23, 2013 at 5:15 PM, Borislav Petkov 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(0x0001) to find_patch() be a better interface? Then the early microcode loading code would not need to access ucode_cpu_info at all. Torsten -- 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/
Re: [PATCH]Fix early microcode loading on AMD
On Tue, Jul 23, 2013 at 5:15 PM, Borislav Petkov 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.
>> 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.
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?
> 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).
>> 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().
>> * Save the amd_bsp_mpb on apply, not on load. Otherwise someone could
>> later load an older microcode file that would overwrite amd_bsp_mpb,
>> but would not be applied to the CPUs
>
> See the patch id check in apply_ucode_in_initrd()?
>
> if (eq_id == mc->hdr.processor_rev_id && rev < mc->hdr.patch_id)
I meant with "load" load_microcode_amd() not the loading of the
microcode into the CPU.
1.: load microcode rev X into CPU (early or normal is not important)
2.: get older microcode file that only contains rev Y with Y> * Save the amd_bsp_mpb on every update. Otherwise someone could offline
>> the BSP, update the microcode and this would be lost on resume
>
> Huh, is amd_bsp_mpb going to disappear all of a sudden?
>
> And that doesn't matter because when we online the BSP later, it goes
> through the CPU hotplug notifier mc_cpu_callback. Or am I missing
> something?
Yeah, me correctly describing what I was meaning. ;-)
1.: boot system, BIOS give microcode rev. X
2.: offline the BSP
3.: update microcode to rev. Y with Y > X
Because the BSP is not online rev. Y will not be copied into amd_bsp_mpb
4.: supend
5.: resume, BIOS gives rev. X again
6.: amd_bsp_mpb is empty -> rev. Y will not be reapplied.
>> * apply_ucode_in_initrd() now also needs to save amd_bsp_mbp, because
>> load_microcode_amd() its no longer doing this and its not using
>> apply_microcode_amd().
>> * extract common checks and initialisations from load_ucode_ap() and
>> load_microcode_amd() to load_microcode_amd_early(). The change from
>> cpu to x86family in load_microcode_amd() allows to drop the code messing
>> with cpu_data(cpu), with is wrong anyway because at that point the
>> per-cpu cpu_info is not yet setup. And these values would later be
>> overwritten by smp_store_boot_cpu_info() / smp_store_cpu_info().
>
> Right, so I was thinking about this. And the code is pretty nasty: we do a
> load_ucode_amd_ap() but we do add the ucode for the BSP:
>
> if (load_microcode_amd(0, ucode, ucode_size) != UCODE_OK)
No, that code will not be reached for the BSP, because it is behind:
if (cpu && !ucode_loaded) {
The BSP has cpu == 0. Thats why I adding the following in my patch:
+ /* BSP via load_ucode_amd_bsp() */
+ if (!cpu)
+ return;
I don't understand if that is really correct, but that was the
original behavior, and I didn't feel competent enough to decree that
calling load_microcode_amd() for the BSP would be save.
(The code there is strange: There is a load_ucode_amd_bsp() but
load_ucode_amd_ap() will also be called for the BSP.
And it seems the call to load_ucode_ap() for the BSP will come from a
very different place (via trap_init()) that the other CPUs.
And I did not even try to understand what X86_32 is doing...)
> so the actual problem is
Re: [PATCH]Fix early microcode loading on AMD
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. 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. > * Save the amd_bsp_mpb on apply, not on load. Otherwise someone could > later load an older microcode file that would overwrite amd_bsp_mpb, > but would not be applied to the CPUs See the patch id check in apply_ucode_in_initrd()? if (eq_id == mc->hdr.processor_rev_id && rev < mc->hdr.patch_id) > * Save the amd_bsp_mpb on every update. Otherwise someone could offline > the BSP, update the microcode and this would be lost on resume Huh, is amd_bsp_mpb going to disappear all of a sudden? And that doesn't matter because when we online the BSP later, it goes through the CPU hotplug notifier mc_cpu_callback. Or am I missing something? > * apply_ucode_in_initrd() now also needs to save amd_bsp_mbp, because > load_microcode_amd() its no longer doing this and its not using > apply_microcode_amd(). > * extract common checks and initialisations from load_ucode_ap() and > load_microcode_amd() to load_microcode_amd_early(). The change from > cpu to x86family in load_microcode_amd() allows to drop the code messing > with cpu_data(cpu), with is wrong anyway because at that point the > per-cpu cpu_info is not yet setup. And these values would later be > overwritten by smp_store_boot_cpu_info() / smp_store_cpu_info(). Right, so I was thinking about this. And the code is pretty nasty: we do a load_ucode_amd_ap() but we do add the ucode for the BSP: if (load_microcode_amd(0, ucode, ucode_size) != UCODE_OK) so the actual problem is apply_microcode_amd() using as an arg cpu != 0. And we can take care of that by using, say __apply_microcode_amd() and hand it the BSP's patch. 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. 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. > * fold collect_cpu_info_amd_early() into load_ucode_amd_ap(), because its > only used at one place. Yes, that should be done especially since we are not going to touch cpuinfo_x86 fields anymore. > * 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... And so on. 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 :)). Thanks. -- 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/
Re: [PATCH]Fix early microcode loading on AMD
On Tue, Jul 23, 2013 at 05:02:28AM -0700, H. Peter Anvin wrote: > Borislav, do you agree with these patches? (This one and the errata > check?) If so I will queue them up in x86/urgent. I was just about to have another look at the whole deal. Give me a couple of days - I'll let you know. Thanks. -- 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/
Re: [PATCH]Fix early microcode loading on AMD
Borislav, do you agree with these patches? (This one and the errata check?) If so I will queue them up in x86/urgent. -hpa -- 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/
[PATCH]Fix early microcode loading on AMD
Fixup the early AMD microcode loading.
* load_microcode_amd() (and the helper its using) should not have an
cpu parameter. The microcode loading is not depending on the CPU it is
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.
* Save the amd_bsp_mpb on apply, not on load. Otherwise someone could later
load an older microcode file that would overwrite amd_bsp_mpb, but would
not be applied to the CPUs
* Save the amd_bsp_mpb on every update. Otherwise someone could offline
the BSP, update the microcode and this would be lost on resume
* apply_ucode_in_initrd() now also needs to save amd_bsp_mbp, because
load_microcode_amd() its no longer doing this and its not using
apply_microcode_amd().
* extract common checks and initialisations from load_ucode_ap() and
load_microcode_amd() to load_microcode_amd_early(). The change from
cpu to x86family in load_microcode_amd() allows to drop the code messing
with cpu_data(cpu), with is wrong anyway because at that point the
per-cpu cpu_info is not yet setup. And these values would later be
overwritten by smp_store_boot_cpu_info() / smp_store_cpu_info().
* fold collect_cpu_info_amd_early() into load_ucode_amd_ap(), because its
only used at one place.
* 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.
* reduce save_microcode_in_initrd_amd() by reusing load_microcode_amd_early()
Main benefit is, that the early microcode loading no longer plays games with
the
not-yet-initialised per-cpu cpu_info. apply_microcode_amd() will still write
into cpu_data(cpu)->microcode, but I see no good way to remove that there,
because
for not-early microcode updates that is exactly the right place for that update.
Signed-off-by: Torsten Kaiser
---
This alone also fixes the hang-on-boot I experienced with 3.11-rc1 even
if the fix for cpu_has_amd_erratum() is not applied, because now the
trigger (filling ->x86 but not ->x86_vendor) is no longer there. But I
think both patches should be applied.
Boot tested on 64 and 32bit, but as my BIOS already provides up-to-date
microcode I could not test, if that gets applied correctly.
--- a/arch/x86/include/asm/microcode_amd.h 2013-07-22 17:54:25.166193431
+0200
+++ b/arch/x86/include/asm/microcode_amd.h 2013-07-22 17:56:31.066192463
+0200
@@ -59,7 +59,7 @@ static inline u16 find_equiv_id(struct e
extern int __apply_microcode_amd(struct microcode_amd *mc_amd);
extern int apply_microcode_amd(int cpu);
-extern enum ucode_state load_microcode_amd(int cpu, const u8 *data, size_t
size);
+extern enum ucode_state load_microcode_amd(u8 x86family, const u8 *data,
size_t size);
#ifdef CONFIG_MICROCODE_AMD_EARLY
#ifdef CONFIG_X86_32
--- a/arch/x86/kernel/microcode_amd.c 2013-07-22 17:33:55.856202878 +0200
+++ b/arch/x86/kernel/microcode_amd.c 2013-07-22 21:45:28.186086900 +0200
@@ -145,10 +145,9 @@ static int collect_cpu_info_amd(int cpu,
return 0;
}
-static unsigned int verify_patch_size(int cpu, u32 patch_size,
+static unsigned int verify_patch_size(u8 x86family, u32 patch_size,
unsigned int size)
{
- struct cpuinfo_x86 *c = _data(cpu);
u32 max_size;
#define F1XH_MPB_MAX_SIZE 2048
@@ -156,7 +155,7 @@ static unsigned int verify_patch_size(in
#define F15H_MPB_MAX_SIZE 4096
#define F16H_MPB_MAX_SIZE 3458
- switch (c->x86) {
+ switch (x86family) {
case 0x14:
max_size = F14H_MPB_MAX_SIZE;
break;
@@ -220,12 +219,20 @@ int apply_microcode_amd(int cpu)
return 0;
}
- if (__apply_microcode_amd(mc_amd))
+ if (__apply_microcode_amd(mc_amd)) {
pr_err("CPU%d: update failed for patch_level=0x%08x\n",
cpu, mc_amd->hdr.patch_id);
- else
- pr_info("CPU%d: new patch_level=0x%08x\n", cpu,
- mc_amd->hdr.patch_id);
+ return -1;
+ }
+
+#if defined(CONFIG_MICROCODE_AMD_EARLY) && defined(CONFIG_X86_32)
+ /* save applied patch for early load */
+ memset(amd_bsp_mpb, 0, MPB_MAX_SIZE);
+ memcpy(amd_bsp_mpb, p->data, min_t(u32, ksize(p->data), MPB_MAX_SIZE));
+#endif
+
+ pr_info("CPU%d: new patch_level=0x%08x\n", cpu,
+ mc_amd->hdr.patch_id);
uci->cpu_sig.rev = mc_amd->hdr.patch_id;
c->microcode = mc_amd->hdr.patch_id;
@@ -276,9 +283,8 @@ static void cleanup(void)
* driver cannot continue functioning normally. In such cases, we tear
* down everything we've used up so far and exit.
*/
-static int verify_and_add_patch(unsigned int cpu, u8 *fw, unsigned int
leftover)
+static int verify_and_add_patch(u8 x86family, u8 *fw, unsigned int leftover)
{
- struct
[PATCH]Fix early microcode loading on AMD
Fixup the early AMD microcode loading.
* load_microcode_amd() (and the helper its using) should not have an
cpu parameter. The microcode loading is not depending on the CPU it is
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.
* Save the amd_bsp_mpb on apply, not on load. Otherwise someone could later
load an older microcode file that would overwrite amd_bsp_mpb, but would
not be applied to the CPUs
* Save the amd_bsp_mpb on every update. Otherwise someone could offline
the BSP, update the microcode and this would be lost on resume
* apply_ucode_in_initrd() now also needs to save amd_bsp_mbp, because
load_microcode_amd() its no longer doing this and its not using
apply_microcode_amd().
* extract common checks and initialisations from load_ucode_ap() and
load_microcode_amd() to load_microcode_amd_early(). The change from
cpu to x86family in load_microcode_amd() allows to drop the code messing
with cpu_data(cpu), with is wrong anyway because at that point the
per-cpu cpu_info is not yet setup. And these values would later be
overwritten by smp_store_boot_cpu_info() / smp_store_cpu_info().
* fold collect_cpu_info_amd_early() into load_ucode_amd_ap(), because its
only used at one place.
* 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.
* reduce save_microcode_in_initrd_amd() by reusing load_microcode_amd_early()
Main benefit is, that the early microcode loading no longer plays games with
the
not-yet-initialised per-cpu cpu_info. apply_microcode_amd() will still write
into cpu_data(cpu)-microcode, but I see no good way to remove that there,
because
for not-early microcode updates that is exactly the right place for that update.
Signed-off-by: Torsten Kaiser just.for.l...@googlemail.com
---
This alone also fixes the hang-on-boot I experienced with 3.11-rc1 even
if the fix for cpu_has_amd_erratum() is not applied, because now the
trigger (filling -x86 but not -x86_vendor) is no longer there. But I
think both patches should be applied.
Boot tested on 64 and 32bit, but as my BIOS already provides up-to-date
microcode I could not test, if that gets applied correctly.
--- a/arch/x86/include/asm/microcode_amd.h 2013-07-22 17:54:25.166193431
+0200
+++ b/arch/x86/include/asm/microcode_amd.h 2013-07-22 17:56:31.066192463
+0200
@@ -59,7 +59,7 @@ static inline u16 find_equiv_id(struct e
extern int __apply_microcode_amd(struct microcode_amd *mc_amd);
extern int apply_microcode_amd(int cpu);
-extern enum ucode_state load_microcode_amd(int cpu, const u8 *data, size_t
size);
+extern enum ucode_state load_microcode_amd(u8 x86family, const u8 *data,
size_t size);
#ifdef CONFIG_MICROCODE_AMD_EARLY
#ifdef CONFIG_X86_32
--- a/arch/x86/kernel/microcode_amd.c 2013-07-22 17:33:55.856202878 +0200
+++ b/arch/x86/kernel/microcode_amd.c 2013-07-22 21:45:28.186086900 +0200
@@ -145,10 +145,9 @@ static int collect_cpu_info_amd(int cpu,
return 0;
}
-static unsigned int verify_patch_size(int cpu, u32 patch_size,
+static unsigned int verify_patch_size(u8 x86family, u32 patch_size,
unsigned int size)
{
- struct cpuinfo_x86 *c = cpu_data(cpu);
u32 max_size;
#define F1XH_MPB_MAX_SIZE 2048
@@ -156,7 +155,7 @@ static unsigned int verify_patch_size(in
#define F15H_MPB_MAX_SIZE 4096
#define F16H_MPB_MAX_SIZE 3458
- switch (c-x86) {
+ switch (x86family) {
case 0x14:
max_size = F14H_MPB_MAX_SIZE;
break;
@@ -220,12 +219,20 @@ int apply_microcode_amd(int cpu)
return 0;
}
- if (__apply_microcode_amd(mc_amd))
+ if (__apply_microcode_amd(mc_amd)) {
pr_err(CPU%d: update failed for patch_level=0x%08x\n,
cpu, mc_amd-hdr.patch_id);
- else
- pr_info(CPU%d: new patch_level=0x%08x\n, cpu,
- mc_amd-hdr.patch_id);
+ return -1;
+ }
+
+#if defined(CONFIG_MICROCODE_AMD_EARLY) defined(CONFIG_X86_32)
+ /* save applied patch for early load */
+ memset(amd_bsp_mpb, 0, MPB_MAX_SIZE);
+ memcpy(amd_bsp_mpb, p-data, min_t(u32, ksize(p-data), MPB_MAX_SIZE));
+#endif
+
+ pr_info(CPU%d: new patch_level=0x%08x\n, cpu,
+ mc_amd-hdr.patch_id);
uci-cpu_sig.rev = mc_amd-hdr.patch_id;
c-microcode = mc_amd-hdr.patch_id;
@@ -276,9 +283,8 @@ static void cleanup(void)
* driver cannot continue functioning normally. In such cases, we tear
* down everything we've used up so far and exit.
*/
-static int verify_and_add_patch(unsigned int cpu, u8 *fw, unsigned int
leftover)
+static int verify_and_add_patch(u8 x86family, u8 *fw, unsigned int leftover)
{
-
Re: [PATCH]Fix early microcode loading on AMD
Borislav, do you agree with these patches? (This one and the errata check?) If so I will queue them up in x86/urgent. -hpa -- 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/
Re: [PATCH]Fix early microcode loading on AMD
On Tue, Jul 23, 2013 at 05:02:28AM -0700, H. Peter Anvin wrote: Borislav, do you agree with these patches? (This one and the errata check?) If so I will queue them up in x86/urgent. I was just about to have another look at the whole deal. Give me a couple of days - I'll let you know. Thanks. -- 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/
Re: [PATCH]Fix early microcode loading on AMD
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. 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. * Save the amd_bsp_mpb on apply, not on load. Otherwise someone could later load an older microcode file that would overwrite amd_bsp_mpb, but would not be applied to the CPUs See the patch id check in apply_ucode_in_initrd()? if (eq_id == mc-hdr.processor_rev_id rev mc-hdr.patch_id) * Save the amd_bsp_mpb on every update. Otherwise someone could offline the BSP, update the microcode and this would be lost on resume Huh, is amd_bsp_mpb going to disappear all of a sudden? And that doesn't matter because when we online the BSP later, it goes through the CPU hotplug notifier mc_cpu_callback. Or am I missing something? * apply_ucode_in_initrd() now also needs to save amd_bsp_mbp, because load_microcode_amd() its no longer doing this and its not using apply_microcode_amd(). * extract common checks and initialisations from load_ucode_ap() and load_microcode_amd() to load_microcode_amd_early(). The change from cpu to x86family in load_microcode_amd() allows to drop the code messing with cpu_data(cpu), with is wrong anyway because at that point the per-cpu cpu_info is not yet setup. And these values would later be overwritten by smp_store_boot_cpu_info() / smp_store_cpu_info(). Right, so I was thinking about this. And the code is pretty nasty: we do a load_ucode_amd_ap() but we do add the ucode for the BSP: if (load_microcode_amd(0, ucode, ucode_size) != UCODE_OK) so the actual problem is apply_microcode_amd() using as an arg cpu != 0. And we can take care of that by using, say __apply_microcode_amd() and hand it the BSP's patch. 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. 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. * fold collect_cpu_info_amd_early() into load_ucode_amd_ap(), because its only used at one place. Yes, that should be done especially since we are not going to touch cpuinfo_x86 fields anymore. * 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... And so on. 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 :)). Thanks. -- 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/
Re: [PATCH]Fix early microcode loading on AMD
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.
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.
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?
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).
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().
* Save the amd_bsp_mpb on apply, not on load. Otherwise someone could
later load an older microcode file that would overwrite amd_bsp_mpb,
but would not be applied to the CPUs
See the patch id check in apply_ucode_in_initrd()?
if (eq_id == mc-hdr.processor_rev_id rev mc-hdr.patch_id)
I meant with load load_microcode_amd() not the loading of the
microcode into the CPU.
1.: load microcode rev X into CPU (early or normal is not important)
2.: get older microcode file that only contains rev Y with YX
3.: trigger load_microcode_amd() with a corrupt file: This will call
cleanup() and empty pcache.
4.: trigger load_microcode_amd() with the older file:
* this will now load rev Y into pcache
* rev Y will be returned by find_patch and copied into amd_bsp_mpb
* any try to apply rev Y will be skipped in apply_microcode_amd()
So now the CPU still correctly has rev X, but amd_bsp_mpb will contain
the wrong rev Y.
That copying already in load_microcode also is suspicious if someone
would only load the microcode but not apply it. But I did not find a
codepath in arch/x86/kernel/microcode_core.c to load it without a
followup apply.
* Save the amd_bsp_mpb on every update. Otherwise someone could offline
the BSP, update the microcode and this would be lost on resume
Huh, is amd_bsp_mpb going to disappear all of a sudden?
And that doesn't matter because when we online the BSP later, it goes
through the CPU hotplug notifier mc_cpu_callback. Or am I missing
something?
Yeah, me correctly describing what I was meaning. ;-)
1.: boot system, BIOS give microcode rev. X
2.: offline the BSP
3.: update microcode to rev. Y with Y X
Because the BSP is not online rev. Y will not be copied into amd_bsp_mpb
4.: supend
5.: resume, BIOS gives rev. X again
6.: amd_bsp_mpb is empty - rev. Y will not be reapplied.
* apply_ucode_in_initrd() now also needs to save amd_bsp_mbp, because
load_microcode_amd() its no longer doing this and its not using
apply_microcode_amd().
* extract common checks and initialisations from load_ucode_ap() and
load_microcode_amd() to load_microcode_amd_early(). The change from
cpu to x86family in load_microcode_amd() allows to drop the code messing
with cpu_data(cpu), with is wrong anyway because at that point the
per-cpu cpu_info is not yet setup. And these values would later be
overwritten by smp_store_boot_cpu_info() / smp_store_cpu_info().
Right, so I was thinking about this. And the code is pretty nasty: we do a
load_ucode_amd_ap() but we do add the ucode for the BSP:
if (load_microcode_amd(0, ucode, ucode_size) != UCODE_OK)
No, that code will not be reached for the BSP, because it is behind:
if (cpu !ucode_loaded) {
The BSP has cpu == 0. Thats why I adding the following in my patch:
+ /* BSP via load_ucode_amd_bsp() */
+
Re: [PATCH]Fix early microcode loading on AMD
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). 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(0x0001) to find_patch() be a better interface? Then the early microcode loading code would not need to access ucode_cpu_info at all. Torsten -- 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/
