x86/ucode: add support for early loading of CPU ucode on AMD.

[chs]

Summary

Sponsored-by: Netflix

[markj]

Does this implementation require that the administrator preload the correct microcode file? The cpu-microcode-intel package concatenates all Intel microcode files into a single blob (installed as /boot/firmware/intel-ucode.bin), and on the kernel side we select the appropriate file from the blob. That way, there's no need to figure out exactly which file the CPU wants (modulo having to choose between AMD and Intel, now).

Reading the code, it looks a bit like this implementation doesn't support the design described above. Can this be addressed?

sys/x86/include/ucode.h

65

Is there any reason to have these structures in ucode.h rather than ucode.c? In retrospect, I don't know why I put the existing structures there. But, if the new ones stay here, their names should be prefixed by "ucode_amd_" to keep the namespace somewhat clean.

75

This doesn't appear to be used?

sys/x86/x86/ucode.c

301

How was this useful? This code runs before printf() works.

[chs]

updated patch with requested changes

[chs]

I added the handling you requested for the loader blob containing all of the amd ucode files concatenated together.

sys/x86/include/ucode.h

65

I just followed the example of the intel code. The structure names (and contents) are copied exactly from usr.sbin/cpucontrol/amd.h. I can put them in ucode.c instead if that's what you'd prefer.

75

This was just part of the stuff I copied from usr.sbin/cpucontrol/amd.h. I'll remove it from here.

sys/x86/x86/ucode.c

301

Warner recently added support for early printf on x86 (via the UART_NS8250_EARLY_PORT kernel option) after I asked him about that feature. This ucode code runs before normal printf works, but after early printf works, and early printf was very helpful in getting this code working.

[markj]

In D43318#989219, @chs wrote:

I added the handling you requested for the loader blob containing all of the amd ucode files concatenated together.

Thank you. So if we modify the cpu-microcode-amd port to concatenate them all together to provide /boot/firmware/amd-ucode.bin, that should just work? I can work on that, but don't have any platforms on which to test.

sys/x86/include/ucode.h

65

Presumably the copyright lines from cpucontrol/amd10h.c should also be carried over?

sys/x86/x86/ucode.c

287

Presumably it's not all that useful to prefix most of the log messages with "loader blob"?

301

Thanks, I didn't know about that.

Could you please remove the unused "level" parameter and gate the print on bootverbose? That way this code won't trigger a bunch of prints if someone has UART_NS8250_EARLY_PORT for a different purpose.

[markj]

sys/x86/x86/ucode.c

436

If you put the return (NULL) case first, then the goto dones in the preceding loop can be simplified to breaks, and there's no need for a goto in the fw_size != 0 case.

[imp]

So for the copyright: I'd create a header that has the shared code between the kernel and cpucontrol.c in it. The code would then be in sync both places. That header could then have the two copyrights in place. You'd need only one SPDX line since both headers look to be identical (thankfully).
That would avoid the ambiguity that's created when you blend two files with multiple copyright holders with verbatim copying.

sys/x86/include/ucode.h

65

Since we have two users of these structures, maybe they should be in a common header?

sys/x86/x86/ucode.c

92

Yea, I'd kinda like these to be in a common header that we can include from both the kernel and userland users of them.

That common header could likely provide both the code that's shared and the structures that's shared. That way we won't have to update things both here and in cpucontrol.c

[chs]

sys/x86/include/ucode.h

65

oh right, I didn't think about that. I applied what I think are the right copyright comments to the new file in the updated patch.

65

they are in a common file in the updated patch.

sys/x86/x86/ucode.c

92

I moved the definitions and code that I copied from usr.sbin/cpucontrol to a new kernel file sys/x86/x86/ucode_subr.c which is compiled separately for the kernel and cpucontrol.

287

well I have to put something there since the now-shared code wants to print something. if there's some other string that you'd prefer I have no problem changing it to something else.

301

in the updated patch, the "level" parameter is no longer unused (when the new file is compiled for cpucontrol). I did change the kernel version of WARNX to use bootverbose.

436

I rearranged this to have fewer gotos in the updated patch.

[chs]

move the code copied from cpucontrol to a new file so that we can share it between the kernel and cpucontrol.

[markj]

sys/x86/x86/ucode.c

78

The const additions should be landed as a separate patch.

usr.sbin/cpucontrol/Makefile

4–6

This addition needs to be conditional on ${MACHINE} == "amd64" or similar.

usr.sbin/cpucontrol/amd10h.c

36

The stdbool include should be grouped with the other standard library includes further below. If ucode.h depends on having a definition of bool, it should include whatever it needs (i.e., sys/types.h in the kernel, stdbool.h otherwise).

[chs]

sys/x86/x86/ucode.c

78

done.

usr.sbin/cpucontrol/Makefile

4–6

The entire cpucontrol program is already built only for amd64 and i386.

usr.sbin/cpucontrol/amd10h.c

36

I changed ucode.h as you suggested.

[chs]

Updated diff for latest review comments.

[markj]

I don't have any more comments. Thank you for working on this!