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.

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.

Quoted 4 lines · Torsten Kaiser

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

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 3 lines

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

Quoted 2 lines

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

Quoted 9 lines

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

Quoted 2 lines

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

Quoted 3 lines

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

Quoted 8 lines · Borislav Petkov

Quoted 4 lines · Torsten Kaiser

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.

Quoted 4 lines

Quoted 1 line

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?

Quoted 2 lines

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

Quoted 6 lines

Quoted 4 lines

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().

Let me try to answer this as well as I can, peacemeal-wise.

Quoted 17 lines · Torsten Kaiser

Quoted 8 lines · Borislav Petkov

Quoted 4 lines · Torsten Kaiser

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)

Quoted 4 lines

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.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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 :)

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

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

First moving that hunk, then switching from cpu to x86family did work. See patch 4/5 and 5/5. :-)

Quoted 5 lines

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

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.

Quoted 7 lines

Quoted 3 lines

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

Quoted 8 lines · Torsten Kaiser

Quoted 7 lines

Quoted 3 lines

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

Quoted 7 lines

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

Quoted 7 lines

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.

Quoted 4 lines

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.

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")

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.

Quoted 10 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 2 lines

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

Quoted 10 lines

Quoted 9 lines

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

Quoted 4 lines

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:

arch/x86/kernel/microcode_amd_early.c +3 -0

+       /* 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...)

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 5 lines

Quoted 3 lines

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

Quoted 9 lines · Torsten Kaiser

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 12 lines

Quoted 7 lines

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.

Quoted 3 lines

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?

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

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

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.

Quoted 4 lines · Torsten Kaiser

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

Quoted 1 line

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.

Quoted 4 lines

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.

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 :)).