| Message ID | 20191209084119.87563-1-elnikety@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/microcode: Support builtin CPU microcode | expand |
On 09/12/2019 08:41, Eslam Elnikety wrote: > diff --git a/docs/misc/builtin-ucode.txt b/docs/misc/builtin-ucode.txt > new file mode 100644 > index 0000000000..43bb60d3eb Instead of introducing a new file, please extend docs/admin-guide/microcode-loading.rst I have an in-prep docs/hypervisor-guide/microcode-loading.rst as well, which I'll see about posting. It would be a more appropriate place for the technical details. > diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c > index 6ced293d88..7afbe44286 100644 > --- a/xen/arch/x86/microcode.c > +++ b/xen/arch/x86/microcode.c > @@ -97,6 +97,14 @@ static struct ucode_mod_blob __initdata ucode_blob; > */ > static bool_t __initdata ucode_scan; > > +#ifdef CONFIG_BUILTIN_UCODE > +/* builtin is the default when BUILTIN_UCODE is set */ > +static bool_t __initdata ucode_builtin = 1; bool, true. > + > +extern const char __builtin_intel_ucode_start[], __builtin_intel_ucode_end[]; > +extern const char __builtin_amd_ucode_start[], __builtin_amd_ucode_end[]; > +#endif > + > /* By default, ucode loading is done in NMI handler */ > static bool ucode_in_nmi = true; > > @@ -110,9 +118,9 @@ void __init microcode_set_module(unsigned int idx) > } > > /* > - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are > - * optional. If the EFI has forced which of the multiboot payloads is to be > - * used, only nmi=<bool> is parsed. > + * The format is '[<integer>|scan=<bool>|builtin=<bool>, nmi=<bool>]'. All > + * options are optional. If the EFI has forced which of the multiboot payloads > + * is to be used, only nmi=<bool> is parsed. > */ Please delete this, or I'll do a prereq patch to fix it and the command line docs. (Both are in a poor state.) > @@ -237,6 +249,48 @@ void __init microcode_grab_module( > scan: > if ( ucode_scan ) > microcode_scan_module(module_map, mbi); > + > +#ifdef CONFIG_BUILTIN_UCODE > + /* > + * Do not use the builtin microcode if: > + * (a) builtin has been explicitly turned off (e.g., ucode=no-builtin) > + * (b) a microcode module has been specified or a scan is successful > + */ > + if ( !ucode_builtin || ucode_mod.mod_end || ucode_blob.size ) > + return; > + > + /* Set ucode_start/_end to the proper blob */ > + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) > + ucode_blob.size = (size_t)(__builtin_amd_ucode_end > + - __builtin_amd_ucode_start); No need to cast the result. Also, preferred Xen style would be to have - on the preceding line. > + else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) > + ucode_blob.size = (size_t)(__builtin_intel_ucode_end > + - __builtin_intel_ucode_start); > + else > + return; > + > + if ( !ucode_blob.size ) > + { > + printk("No builtin ucode! 'ucode=builtin' is nullified.\n"); > + return; > + } > + else if ( ucode_blob.size > MAX_EARLY_CPIO_MICROCODE ) > + { > + printk("Builtin microcode payload too big! (%ld, we can do %d)\n", > + ucode_blob.size, MAX_EARLY_CPIO_MICROCODE); > + ucode_blob.size = 0; > + return; > + } > + > + ucode_blob.data = xmalloc_bytes(ucode_blob.size); > + if ( !ucode_blob.data ) > + return; Any chance we can reuse the "fits" logic to avoid holding every inapplicable blob in memory as well? > + > + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) > + memcpy(ucode_blob.data, __builtin_amd_ucode_start, ucode_blob.size); > + else > + memcpy(ucode_blob.data, __builtin_intel_ucode_start, ucode_blob.size); > +#endif > } > > const struct microcode_ops *microcode_ops; > diff --git a/xen/arch/x86/microcode/Makefile b/xen/arch/x86/microcode/Makefile > new file mode 100644 > index 0000000000..6d585c5482 > --- /dev/null > +++ b/xen/arch/x86/microcode/Makefile > @@ -0,0 +1,40 @@ > +# Copyright (C) 2019 Amazon.com, Inc. or its affiliates. > +# Author: Eslam Elnikety <elnikety@amazon.com> > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 2 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > + > +obj-y += builtin_ucode.o > + > +# Directory holding the microcode updates. > +UCODE_DIR=$(patsubst "%",%,$(CONFIG_BUILTIN_UCODE_DIR)) > +amd-blobs := $(wildcard $(UCODE_DIR)/amd-ucode/*) > +intel-blobs := $(wildcard $(UCODE_DIR)/intel-ucode/*) This is a little dangerous. I can see why you want to do it like this, and I can't provide any obvious suggestions, but if this glob picks up anything which isn't a microcode file, it will break the logic to search for the right blob. > + > +builtin_ucode.o: Makefile $(amd-blobs) $(intel-blobs) > + # Create AMD microcode blob if there are AMD updates on the build system > + if [ ! -z "$(amd-blobs)" ]; then \ > + cat $(amd-blobs) > $@.bin ; \ > + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section .data=.builtin_amd_ucode,alloc,load,readonly,data,contents $@.bin $@.amd; \ > + rm -f $@.bin; \ > + fi > + # Create INTEL microcode blob if there are INTEL updates on the build system > + if [ ! -z "$(intel-blobs)" ]; then \ > + cat $(intel-blobs) > $@.bin; \ > + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section .data=.builtin_intel_ucode,alloc,load,readonly,data,contents $@.bin $@.intel; \ > + rm -f $@.bin; \ > + fi > + # Create fake builtin_ucode.o if no updates were present. Otherwise, builtin_ucode.o carries the available updates > + if [ -z "$(amd-blobs)" -a -z "$(intel-blobs)" ]; then \ > + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; \ > + else \ > + $(LD) $(LDFLAGS) -r -o $@ $@.*; \ > + rm -f $@.*; \ > + fi How about using weak symbols, rather than playing games like this? ~Andrew
On 09.12.19 16:19, Andrew Cooper wrote: > On 09/12/2019 08:41, Eslam Elnikety wrote: >> diff --git a/docs/misc/builtin-ucode.txt b/docs/misc/builtin-ucode.txt >> new file mode 100644 >> index 0000000000..43bb60d3eb > > Instead of introducing a new file, please extend > docs/admin-guide/microcode-loading.rst > > I have an in-prep docs/hypervisor-guide/microcode-loading.rst as well, > which I'll see about posting. It would be a more appropriate place for > the technical details. > Agreed! While the existing docs/admin-guide/microcode-loading.rst speaks a different tone than what I added, that documentation anyway needs to be updated with builtin if such support were to be added. I will adjust accordingly. If docs/hypervisor-guide/microcode-loading.rst makes it in time for v2 of this patch, I will reflect changes there too. >> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c >> index 6ced293d88..7afbe44286 100644 >> --- a/xen/arch/x86/microcode.c >> +++ b/xen/arch/x86/microcode.c >> @@ -97,6 +97,14 @@ static struct ucode_mod_blob __initdata ucode_blob; >> */ >> static bool_t __initdata ucode_scan; >> >> +#ifdef CONFIG_BUILTIN_UCODE >> +/* builtin is the default when BUILTIN_UCODE is set */ >> +static bool_t __initdata ucode_builtin = 1; > > bool, true. > Will fix in v2. >> + >> +extern const char __builtin_intel_ucode_start[], __builtin_intel_ucode_end[]; >> +extern const char __builtin_amd_ucode_start[], __builtin_amd_ucode_end[]; >> +#endif >> + >> /* By default, ucode loading is done in NMI handler */ >> static bool ucode_in_nmi = true; >> >> @@ -110,9 +118,9 @@ void __init microcode_set_module(unsigned int idx) >> } >> >> /* >> - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are >> - * optional. If the EFI has forced which of the multiboot payloads is to be >> - * used, only nmi=<bool> is parsed. >> + * The format is '[<integer>|scan=<bool>|builtin=<bool>, nmi=<bool>]'. All >> + * options are optional. If the EFI has forced which of the multiboot payloads >> + * is to be used, only nmi=<bool> is parsed. >> */ > > Please delete this, or I'll do a prereq patch to fix it and the command > line docs. (Both are in a poor state.) > Unless you are planning that along your on-going docs/hypervisor-guide/microcode-loading.rst effort, I can pick up this clean-up/prereq patch myself. What do you have in mind? (Or point me to a good example and I will figure things out). >> @@ -237,6 +249,48 @@ void __init microcode_grab_module( >> scan: >> if ( ucode_scan ) >> microcode_scan_module(module_map, mbi); >> + >> +#ifdef CONFIG_BUILTIN_UCODE >> + /* >> + * Do not use the builtin microcode if: >> + * (a) builtin has been explicitly turned off (e.g., ucode=no-builtin) >> + * (b) a microcode module has been specified or a scan is successful >> + */ >> + if ( !ucode_builtin || ucode_mod.mod_end || ucode_blob.size ) >> + return; >> + >> + /* Set ucode_start/_end to the proper blob */ >> + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) >> + ucode_blob.size = (size_t)(__builtin_amd_ucode_end >> + - __builtin_amd_ucode_start); > > No need to cast the result. Also, preferred Xen style would be to have > - on the preceding line. > Will fix in v2. >> + else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) >> + ucode_blob.size = (size_t)(__builtin_intel_ucode_end >> + - __builtin_intel_ucode_start); >> + else >> + return; >> + >> + if ( !ucode_blob.size ) >> + { >> + printk("No builtin ucode! 'ucode=builtin' is nullified.\n"); >> + return; >> + } >> + else if ( ucode_blob.size > MAX_EARLY_CPIO_MICROCODE ) >> + { >> + printk("Builtin microcode payload too big! (%ld, we can do %d)\n", >> + ucode_blob.size, MAX_EARLY_CPIO_MICROCODE); >> + ucode_blob.size = 0; >> + return; >> + } >> + >> + ucode_blob.data = xmalloc_bytes(ucode_blob.size); >> + if ( !ucode_blob.data ) >> + return; > > Any chance we can reuse the "fits" logic to avoid holding every > inapplicable blob in memory as well? > I think this would be a welcomed change. It seems to me that we have two ways to go about it. 1) We factor the code in the intel-/amd-specific cpu_request_microcode to extract logic for finding a match into its own new function, expose that through microcode_ops, and finally do xalloc only for the matching microcode when early loading is scan or builtin. 2) Cannot we just do away completely with xalloc? I see that each individual microcode update gets allocated anyway in microcode_intel.c/get_next_ucode_from_buffer() and in microcode_amd.c/cpu_request_microcode(). Unless I am missing something, the xmalloc_bytes for ucode_blob.data is redundant. Thoughts? >> + >> + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) >> + memcpy(ucode_blob.data, __builtin_amd_ucode_start, ucode_blob.size); >> + else >> + memcpy(ucode_blob.data, __builtin_intel_ucode_start, ucode_blob.size); >> +#endif >> } >> >> const struct microcode_ops *microcode_ops; >> diff --git a/xen/arch/x86/microcode/Makefile b/xen/arch/x86/microcode/Makefile >> new file mode 100644 >> index 0000000000..6d585c5482 >> --- /dev/null >> +++ b/xen/arch/x86/microcode/Makefile >> @@ -0,0 +1,40 @@ >> +# Copyright (C) 2019 Amazon.com, Inc. or its affiliates. >> +# Author: Eslam Elnikety <elnikety@amazon.com> >> +# >> +# This program is free software; you can redistribute it and/or modify >> +# it under the terms of the GNU General Public License as published by >> +# the Free Software Foundation; either version 2 of the License, or >> +# (at your option) any later version. >> +# >> +# This program is distributed in the hope that it will be useful, >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> +# GNU General Public License for more details. >> + >> +obj-y += builtin_ucode.o >> + >> +# Directory holding the microcode updates. >> +UCODE_DIR=$(patsubst "%",%,$(CONFIG_BUILTIN_UCODE_DIR)) >> +amd-blobs := $(wildcard $(UCODE_DIR)/amd-ucode/*) >> +intel-blobs := $(wildcard $(UCODE_DIR)/intel-ucode/*) > > This is a little dangerous. I can see why you want to do it like this, > and I can't provide any obvious suggestions, but if this glob picks up > anything which isn't a microcode file, it will break the logic to search > for the right blob. > We can limit the amd-blobs and intel-blob to binaries following the naming convention AuthenticAMD.bin and GenuineIntel.bin for amd and intel, respectively. Yet, this would be imposing an unnecessary restriction on administrators who may want to be innovative with naming (or want to use the naming microcode_amd_*.bin or FF-MM-SS instead). Alternatively, we can introduce CONFIG_BUILTIN_UCODE_INTEL and CONFIG_BUILTIN_UCODE_AMD. Both default to empty strings. Then, an administrator can specify exactly the microcodes to include relative to the CONFIG_BUILTIN_UCODE_DIR. For example: CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09" CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin" Thoughts? >> + >> +builtin_ucode.o: Makefile $(amd-blobs) $(intel-blobs) >> + # Create AMD microcode blob if there are AMD updates on the build system >> + if [ ! -z "$(amd-blobs)" ]; then \ >> + cat $(amd-blobs) > $@.bin ; \ >> + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section .data=.builtin_amd_ucode,alloc,load,readonly,data,contents $@.bin $@.amd; \ >> + rm -f $@.bin; \ >> + fi >> + # Create INTEL microcode blob if there are INTEL updates on the build system >> + if [ ! -z "$(intel-blobs)" ]; then \ >> + cat $(intel-blobs) > $@.bin; \ >> + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section .data=.builtin_intel_ucode,alloc,load,readonly,data,contents $@.bin $@.intel; \ >> + rm -f $@.bin; \ >> + fi >> + # Create fake builtin_ucode.o if no updates were present. Otherwise, builtin_ucode.o carries the available updates >> + if [ -z "$(amd-blobs)" -a -z "$(intel-blobs)" ]; then \ >> + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; \ >> + else \ >> + $(LD) $(LDFLAGS) -r -o $@ $@.*; \ >> + rm -f $@.*; \ >> + fi > > How about using weak symbols, rather than playing games like this? Just to make sure we are on the same page. You are after a dummy binary with weak symbols that eventually get overridden when I link the actual microcode binaries into builtin_ucode.o? If so, possible of course. Except that I do not particularly see the downside of the existing approach with dummy builtin_ucode.o. > > ~Andrew > Thanks a lot for those insightful comments, Andrew. Cheers, Eslam
On 09.12.2019 22:49, Eslam Elnikety wrote: > On 09.12.19 16:19, Andrew Cooper wrote: >> On 09/12/2019 08:41, Eslam Elnikety wrote: >>> --- /dev/null >>> +++ b/xen/arch/x86/microcode/Makefile >>> @@ -0,0 +1,40 @@ >>> +# Copyright (C) 2019 Amazon.com, Inc. or its affiliates. >>> +# Author: Eslam Elnikety <elnikety@amazon.com> >>> +# >>> +# This program is free software; you can redistribute it and/or modify >>> +# it under the terms of the GNU General Public License as published by >>> +# the Free Software Foundation; either version 2 of the License, or >>> +# (at your option) any later version. >>> +# >>> +# This program is distributed in the hope that it will be useful, >>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> +# GNU General Public License for more details. >>> + >>> +obj-y += builtin_ucode.o >>> + >>> +# Directory holding the microcode updates. >>> +UCODE_DIR=$(patsubst "%",%,$(CONFIG_BUILTIN_UCODE_DIR)) >>> +amd-blobs := $(wildcard $(UCODE_DIR)/amd-ucode/*) >>> +intel-blobs := $(wildcard $(UCODE_DIR)/intel-ucode/*) >> >> This is a little dangerous. I can see why you want to do it like this, >> and I can't provide any obvious suggestions, but if this glob picks up >> anything which isn't a microcode file, it will break the logic to search >> for the right blob. >> > > We can limit the amd-blobs and intel-blob to binaries following the > naming convention AuthenticAMD.bin and GenuineIntel.bin for amd and > intel, respectively. Yet, this would be imposing an unnecessary > restriction on administrators who may want to be innovative with naming > (or want to use the naming microcode_amd_*.bin or FF-MM-SS instead). > > Alternatively, we can introduce CONFIG_BUILTIN_UCODE_INTEL and > CONFIG_BUILTIN_UCODE_AMD. Both default to empty strings. Then, an > administrator can specify exactly the microcodes to include relative to > the CONFIG_BUILTIN_UCODE_DIR. For example: > CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09" > CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin" This would make the feature even less generic - I already meant to ask whether building ucode into binaries is really a useful thing when we already have more flexible ways. I could see this being useful if there was no other way to make ucode available at boot time. Jan
On 09.12.2019 09:41, Eslam Elnikety wrote: > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -2113,7 +2113,7 @@ logic applies: > active by default. > > ### ucode (x86) > -> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]` > +> `= List of [ <integer> | scan=<bool> | builtin=<bool>, nmi=<bool> ]` Despite my other question regarding the usefulness of this as a whole a few comments. Do "scan" and "builtin" really need to exclude each other? I.e. don't you mean , instead of | ? > @@ -2128,6 +2128,9 @@ when used with xen.efi (there the concept of modules doesn't exist, and > the blob gets specified via the `ucode=<filename>` config file/section > entry; see [EFI configuration file description](efi.html)). > > +'builtin' instructs the hypervisor to use the builtin microcode update. This > +option is available only if option BUILTIN_UCODE is enabled. You also want to clarify its default - your reply to Andrew suggested to me that only the negative form would really be useful. > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -218,6 +218,26 @@ config MEM_SHARING > bool "Xen memory sharing support" if EXPERT = "y" > depends on HVM > > +config BUILTIN_UCODE > + def_bool n > + prompt "Support for Builtin Microcode" These two lines should be folded into just bool "Support for Builtin Microcode" irrespective of other bad examples you may find in the code base. The again ... > + ---help--- > + Include the CPU microcode update in the Xen image itself. With this > + support, Xen can update the CPU microcode upon boot using the builtin > + microcode, with no need for an additional microcode boot modules. > + > + If unsure, say N. > + > +config BUILTIN_UCODE_DIR > + string > + default "/lib/firmware" > + depends on BUILTIN_UCODE ... are two separate options needed at all? Can't this latter one being the empty string just imply the feature to be disabled? > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -7,6 +7,7 @@ subdir-y += mm > subdir-$(CONFIG_XENOPROF) += oprofile > subdir-$(CONFIG_PV) += pv > subdir-y += x86_64 > +subdir-$(CONFIG_BUILTIN_UCODE) += microcode Please respect the (half way?) alphabetical sorting here, unless adding last is a requirement (in which case a brief comment should say so, and why). > @@ -130,6 +138,10 @@ static int __init parse_ucode(const char *s) > { > if ( (val = parse_boolean("scan", s, ss)) >= 0 ) > ucode_scan = val; > +#ifdef CONFIG_BUILTIN_UCODE > + else if ( (val = parse_boolean("builtin", s, ss)) >= 0 ) Please watch out for hard tabs where they don't belong. > @@ -237,6 +249,48 @@ void __init microcode_grab_module( > scan: > if ( ucode_scan ) > microcode_scan_module(module_map, mbi); > + > +#ifdef CONFIG_BUILTIN_UCODE > + /* > + * Do not use the builtin microcode if: > + * (a) builtin has been explicitly turned off (e.g., ucode=no-builtin) > + * (b) a microcode module has been specified or a scan is successful > + */ > + if ( !ucode_builtin || ucode_mod.mod_end || ucode_blob.size ) > + return; > + > + /* Set ucode_start/_end to the proper blob */ > + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) > + ucode_blob.size = (size_t)(__builtin_amd_ucode_end > + - __builtin_amd_ucode_start); > + else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) > + ucode_blob.size = (size_t)(__builtin_intel_ucode_end > + - __builtin_intel_ucode_start); > + else > + return; > + > + if ( !ucode_blob.size ) > + { > + printk("No builtin ucode! 'ucode=builtin' is nullified.\n"); > + return; > + } > + else if ( ucode_blob.size > MAX_EARLY_CPIO_MICROCODE ) With the "return" above please omit the "else" here. But why this restriction, and ... > + { > + printk("Builtin microcode payload too big! (%ld, we can do %d)\n", > + ucode_blob.size, MAX_EARLY_CPIO_MICROCODE); > + ucode_blob.size = 0; > + return; > + } > + > + ucode_blob.data = xmalloc_bytes(ucode_blob.size); > + if ( !ucode_blob.data ) > + return; > + > + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) > + memcpy(ucode_blob.data, __builtin_amd_ucode_start, ucode_blob.size); > + else > + memcpy(ucode_blob.data, __builtin_intel_ucode_start, ucode_blob.size); ... why the copying? Can't you simply point ucode_blob.data at __builtin_{amd,intel}_ucode_start? Jan
On 10.12.19 10:21, Jan Beulich wrote: > On 09.12.2019 22:49, Eslam Elnikety wrote: >> On 09.12.19 16:19, Andrew Cooper wrote: >>> On 09/12/2019 08:41, Eslam Elnikety wrote: >>>> --- /dev/null >>>> +++ b/xen/arch/x86/microcode/Makefile >>>> @@ -0,0 +1,40 @@ >>>> +# Copyright (C) 2019 Amazon.com, Inc. or its affiliates. >>>> +# Author: Eslam Elnikety <elnikety@amazon.com> >>>> +# >>>> +# This program is free software; you can redistribute it and/or modify >>>> +# it under the terms of the GNU General Public License as published by >>>> +# the Free Software Foundation; either version 2 of the License, or >>>> +# (at your option) any later version. >>>> +# >>>> +# This program is distributed in the hope that it will be useful, >>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>> +# GNU General Public License for more details. >>>> + >>>> +obj-y += builtin_ucode.o >>>> + >>>> +# Directory holding the microcode updates. >>>> +UCODE_DIR=$(patsubst "%",%,$(CONFIG_BUILTIN_UCODE_DIR)) >>>> +amd-blobs := $(wildcard $(UCODE_DIR)/amd-ucode/*) >>>> +intel-blobs := $(wildcard $(UCODE_DIR)/intel-ucode/*) >>> >>> This is a little dangerous. I can see why you want to do it like this, >>> and I can't provide any obvious suggestions, but if this glob picks up >>> anything which isn't a microcode file, it will break the logic to search >>> for the right blob. >>> >> >> We can limit the amd-blobs and intel-blob to binaries following the >> naming convention AuthenticAMD.bin and GenuineIntel.bin for amd and >> intel, respectively. Yet, this would be imposing an unnecessary >> restriction on administrators who may want to be innovative with naming >> (or want to use the naming microcode_amd_*.bin or FF-MM-SS instead). >> >> Alternatively, we can introduce CONFIG_BUILTIN_UCODE_INTEL and >> CONFIG_BUILTIN_UCODE_AMD. Both default to empty strings. Then, an >> administrator can specify exactly the microcodes to include relative to >> the CONFIG_BUILTIN_UCODE_DIR. For example: >> CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09" >> CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin" > > This would make the feature even less generic - I already meant to I do not follow the point about being less generic. (I hope my example did not give the false impression that CONFIG_BUILTIN_UCODE_{AMD,INTEL} allow for only a single microcode blob for a single signature). > ask whether building ucode into binaries is really a useful thing > when we already have more flexible ways. I could see this being > useful if there was no other way to make ucode available at boot > time. It is useful in addition to the existing ways to do early microcode updates. First, when operating many hosts, using boot modules (either a distinct microcode module or within an initrd) becomes involved. For instance, tools to update boot entries (e.g., https://linux.die.net/man/8/grubby) do not support adding arbitrary (microcode) modules. Second, there is often need to couple a Xen build with a minimum microcode patch level. Having the microcode built within the Xen image itself is a streamlined, natural way of achieving that. Thanks, Eslam > > Jan >
On 10.12.19 10:37, Jan Beulich wrote: > On 09.12.2019 09:41, Eslam Elnikety wrote: >> --- a/docs/misc/xen-command-line.pandoc >> +++ b/docs/misc/xen-command-line.pandoc >> @@ -2113,7 +2113,7 @@ logic applies: >> active by default. >> >> ### ucode (x86) >> -> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]` >> +> `= List of [ <integer> | scan=<bool> | builtin=<bool>, nmi=<bool> ]` > > Despite my other question regarding the usefulness of this as a > whole a few comments. > > Do "scan" and "builtin" really need to exclude each other? I.e. > don't you mean , instead of | ? The useful case here would be specifying ucode=scan,builtin which would translate to fallback onto the builtin microcode if a scan fails. In fact, this is already the case given the implementation in v1. It will be better to clarify this semantic by allowing scan,builtin. On that note, I really see the "<integer>" and "scan=<bool>" to be equal. Following the logic earlier we should probably also allow ucode=<integer>,builtin. This translates to fallback to builtin if there are no modules at index <integer>. > >> @@ -2128,6 +2128,9 @@ when used with xen.efi (there the concept of modules doesn't exist, and >> the blob gets specified via the `ucode=<filename>` config file/section >> entry; see [EFI configuration file description](efi.html)). >> >> +'builtin' instructs the hypervisor to use the builtin microcode update. This >> +option is available only if option BUILTIN_UCODE is enabled. > > You also want to clarify its default - your reply to Andrew > suggested to me that only the negative form would really be > useful. > Indeed. This in any case will need a revamp to rework the ", instead of |". Will address in v2. >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -218,6 +218,26 @@ config MEM_SHARING >> bool "Xen memory sharing support" if EXPERT = "y" >> depends on HVM >> >> +config BUILTIN_UCODE >> + def_bool n >> + prompt "Support for Builtin Microcode" > > These two lines should be folded into just > > bool "Support for Builtin Microcode" > > irrespective of other bad examples you may find in the code base. > The again ... > Will address in v2. >> + ---help--- >> + Include the CPU microcode update in the Xen image itself. With this >> + support, Xen can update the CPU microcode upon boot using the builtin >> + microcode, with no need for an additional microcode boot modules. >> + >> + If unsure, say N. >> + >> +config BUILTIN_UCODE_DIR >> + string >> + default "/lib/firmware" >> + depends on BUILTIN_UCODE > > ... are two separate options needed at all? Can't this latter one > being the empty string just imply the feature to be disabled? > I can go either way here. To me, two options is clearer. >> --- a/xen/arch/x86/Makefile >> +++ b/xen/arch/x86/Makefile >> @@ -7,6 +7,7 @@ subdir-y += mm >> subdir-$(CONFIG_XENOPROF) += oprofile >> subdir-$(CONFIG_PV) += pv >> subdir-y += x86_64 >> +subdir-$(CONFIG_BUILTIN_UCODE) += microcode > > Please respect the (half way?) alphabetical sorting here, unless > adding last is a requirement (in which case a brief comment should > say so, and why). > >> @@ -130,6 +138,10 @@ static int __init parse_ucode(const char *s) >> { >> if ( (val = parse_boolean("scan", s, ss)) >= 0 ) >> ucode_scan = val; >> +#ifdef CONFIG_BUILTIN_UCODE >> + else if ( (val = parse_boolean("builtin", s, ss)) >= 0 ) > > Please watch out for hard tabs where they don't belong. > Good catch! Will fix in v2. >> @@ -237,6 +249,48 @@ void __init microcode_grab_module( >> scan: >> if ( ucode_scan ) >> microcode_scan_module(module_map, mbi); >> + >> +#ifdef CONFIG_BUILTIN_UCODE >> + /* >> + * Do not use the builtin microcode if: >> + * (a) builtin has been explicitly turned off (e.g., ucode=no-builtin) >> + * (b) a microcode module has been specified or a scan is successful >> + */ >> + if ( !ucode_builtin || ucode_mod.mod_end || ucode_blob.size ) >> + return; >> + >> + /* Set ucode_start/_end to the proper blob */ >> + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) >> + ucode_blob.size = (size_t)(__builtin_amd_ucode_end >> + - __builtin_amd_ucode_start); >> + else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) >> + ucode_blob.size = (size_t)(__builtin_intel_ucode_end >> + - __builtin_intel_ucode_start); >> + else >> + return; >> + >> + if ( !ucode_blob.size ) >> + { >> + printk("No builtin ucode! 'ucode=builtin' is nullified.\n"); >> + return; >> + } >> + else if ( ucode_blob.size > MAX_EARLY_CPIO_MICROCODE ) > > With the "return" above please omit the "else" here. But why this > restriction, and ... > Will adjust in v2. >> + { >> + printk("Builtin microcode payload too big! (%ld, we can do %d)\n", >> + ucode_blob.size, MAX_EARLY_CPIO_MICROCODE); >> + ucode_blob.size = 0; >> + return; >> + } >> + >> + ucode_blob.data = xmalloc_bytes(ucode_blob.size); >> + if ( !ucode_blob.data ) >> + return; >> + >> + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) >> + memcpy(ucode_blob.data, __builtin_amd_ucode_start, ucode_blob.size); >> + else >> + memcpy(ucode_blob.data, __builtin_intel_ucode_start, ucode_blob.size); > > ... why the copying? Can't you simply point ucode_blob.data at > __builtin_{amd,intel}_ucode_start? I am all onboard. See my earlier response to Andrew. I used the same logic that already exists for scan (which assumes that ucode_blob.data is allocated and should be freed when all CPUs are updated). Thanks for the comments, Jan. On the earlier discussion, please do let me know (and others too) if you are convinced that builtin support for microcode is warranted/useful. Cheers, Eslam > > Jan >
On 10.12.2019 23:40, Eslam Elnikety wrote: > On 10.12.19 10:21, Jan Beulich wrote: >> On 09.12.2019 22:49, Eslam Elnikety wrote: >>> On 09.12.19 16:19, Andrew Cooper wrote: >>>> On 09/12/2019 08:41, Eslam Elnikety wrote: >>>>> --- /dev/null >>>>> +++ b/xen/arch/x86/microcode/Makefile >>>>> @@ -0,0 +1,40 @@ >>>>> +# Copyright (C) 2019 Amazon.com, Inc. or its affiliates. >>>>> +# Author: Eslam Elnikety <elnikety@amazon.com> >>>>> +# >>>>> +# This program is free software; you can redistribute it and/or modify >>>>> +# it under the terms of the GNU General Public License as published by >>>>> +# the Free Software Foundation; either version 2 of the License, or >>>>> +# (at your option) any later version. >>>>> +# >>>>> +# This program is distributed in the hope that it will be useful, >>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>> +# GNU General Public License for more details. >>>>> + >>>>> +obj-y += builtin_ucode.o >>>>> + >>>>> +# Directory holding the microcode updates. >>>>> +UCODE_DIR=$(patsubst "%",%,$(CONFIG_BUILTIN_UCODE_DIR)) >>>>> +amd-blobs := $(wildcard $(UCODE_DIR)/amd-ucode/*) >>>>> +intel-blobs := $(wildcard $(UCODE_DIR)/intel-ucode/*) >>>> >>>> This is a little dangerous. I can see why you want to do it like this, >>>> and I can't provide any obvious suggestions, but if this glob picks up >>>> anything which isn't a microcode file, it will break the logic to search >>>> for the right blob. >>>> >>> >>> We can limit the amd-blobs and intel-blob to binaries following the >>> naming convention AuthenticAMD.bin and GenuineIntel.bin for amd and >>> intel, respectively. Yet, this would be imposing an unnecessary >>> restriction on administrators who may want to be innovative with naming >>> (or want to use the naming microcode_amd_*.bin or FF-MM-SS instead). >>> >>> Alternatively, we can introduce CONFIG_BUILTIN_UCODE_INTEL and >>> CONFIG_BUILTIN_UCODE_AMD. Both default to empty strings. Then, an >>> administrator can specify exactly the microcodes to include relative to >>> the CONFIG_BUILTIN_UCODE_DIR. For example: >>> CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09" >>> CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin" >> >> This would make the feature even less generic - I already meant to > > I do not follow the point about being less generic. (I hope my example > did not give the false impression that CONFIG_BUILTIN_UCODE_{AMD,INTEL} > allow for only a single microcode blob for a single signature). Well, the example indeed has given this impression to me. I'm having a hard time seeing how, beyond very narrow special cases, either of the examples could be useful to anyone. Yet I think examples should be generally useful. >> ask whether building ucode into binaries is really a useful thing >> when we already have more flexible ways. I could see this being >> useful if there was no other way to make ucode available at boot >> time. > > It is useful in addition to the existing ways to do early microcode > updates. First, when operating many hosts, using boot modules (either a > distinct microcode module or within an initrd) becomes involved. For > instance, tools to update boot entries (e.g., > https://linux.die.net/man/8/grubby) do not support adding arbitrary > (microcode) modules. I.e. you suggest to work around tools shortcomings by extending Xen? Wouldn't the more appropriate way to deal with this be to make the tools more capable? > Second, there is often need to couple a Xen build with a minimum > microcode patch level. Having the microcode built within the Xen image > itself is a streamlined, natural way of achieving that. Okay, I can accept this as a reason, to some degree at least. Yet as said elsewhere, I don't think you want then to override a possible "external" ucode module with the builtin blobs. Instead the newest of everything that's available should then be loaded. Jan
On 11.12.2019 00:18, Eslam Elnikety wrote: > On 10.12.19 10:37, Jan Beulich wrote: >> On 09.12.2019 09:41, Eslam Elnikety wrote: >>> --- a/docs/misc/xen-command-line.pandoc >>> +++ b/docs/misc/xen-command-line.pandoc >>> @@ -2113,7 +2113,7 @@ logic applies: >>> active by default. >>> >>> ### ucode (x86) >>> -> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]` >>> +> `= List of [ <integer> | scan=<bool> | builtin=<bool>, nmi=<bool> ]` >> >> Despite my other question regarding the usefulness of this as a >> whole a few comments. >> >> Do "scan" and "builtin" really need to exclude each other? I.e. >> don't you mean , instead of | ? > The useful case here would be specifying ucode=scan,builtin which would > translate to fallback onto the builtin microcode if a scan fails. In > fact, this is already the case given the implementation in v1. It will > be better to clarify this semantic by allowing scan,builtin. > > On that note, I really see the "<integer>" and "scan=<bool>" to be > equal. Following the logic earlier we should probably also allow > ucode=<integer>,builtin. This translates to fallback to builtin if there > are no modules at index <integer>. Almost - if the builtin one is newer than the separate blob, then either of the cmdline options you name should still cause the builtin one to be loaded. IOW you want to honor both options, not prefer the earlier over a later one. >>> + ---help--- >>> + Include the CPU microcode update in the Xen image itself. With this >>> + support, Xen can update the CPU microcode upon boot using the builtin >>> + microcode, with no need for an additional microcode boot modules. >>> + >>> + If unsure, say N. >>> + >>> +config BUILTIN_UCODE_DIR >>> + string >>> + default "/lib/firmware" >>> + depends on BUILTIN_UCODE >> >> ... are two separate options needed at all? Can't this latter one >> being the empty string just imply the feature to be disabled? > > I can go either way here. To me, two options is clearer. It's the other way around here, but I'd accept being outvoted. >>> + ucode_blob.data = xmalloc_bytes(ucode_blob.size); >>> + if ( !ucode_blob.data ) >>> + return; >>> + >>> + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) >>> + memcpy(ucode_blob.data, __builtin_amd_ucode_start, ucode_blob.size); >>> + else >>> + memcpy(ucode_blob.data, __builtin_intel_ucode_start, ucode_blob.size); >> >> ... why the copying? Can't you simply point ucode_blob.data at >> __builtin_{amd,intel}_ucode_start? > > I am all onboard. See my earlier response to Andrew. I used the same > logic that already exists for scan (which assumes that ucode_blob.data > is allocated and should be freed when all CPUs are updated). The scan case may be different in that it may not lend itself to re-using the blob in its original location. If that's not the reason for the present behavior, then I think we would want to do away with the unnecessary copying there as well. Jan
On 11.12.19 10:47, Jan Beulich wrote: > On 10.12.2019 23:40, Eslam Elnikety wrote: >> On 10.12.19 10:21, Jan Beulich wrote: >>> On 09.12.2019 22:49, Eslam Elnikety wrote: >>>> On 09.12.19 16:19, Andrew Cooper wrote: >>>>> On 09/12/2019 08:41, Eslam Elnikety wrote: >>>>>> --- /dev/null >>>>>> +++ b/xen/arch/x86/microcode/Makefile >>>>>> @@ -0,0 +1,40 @@ >>>>>> +# Copyright (C) 2019 Amazon.com, Inc. or its affiliates. >>>>>> +# Author: Eslam Elnikety <elnikety@amazon.com> >>>>>> +# >>>>>> +# This program is free software; you can redistribute it and/or modify >>>>>> +# it under the terms of the GNU General Public License as published by >>>>>> +# the Free Software Foundation; either version 2 of the License, or >>>>>> +# (at your option) any later version. >>>>>> +# >>>>>> +# This program is distributed in the hope that it will be useful, >>>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>>> +# GNU General Public License for more details. >>>>>> + >>>>>> +obj-y += builtin_ucode.o >>>>>> + >>>>>> +# Directory holding the microcode updates. >>>>>> +UCODE_DIR=$(patsubst "%",%,$(CONFIG_BUILTIN_UCODE_DIR)) >>>>>> +amd-blobs := $(wildcard $(UCODE_DIR)/amd-ucode/*) >>>>>> +intel-blobs := $(wildcard $(UCODE_DIR)/intel-ucode/*) >>>>> >>>>> This is a little dangerous. I can see why you want to do it like this, >>>>> and I can't provide any obvious suggestions, but if this glob picks up >>>>> anything which isn't a microcode file, it will break the logic to search >>>>> for the right blob. >>>>> >>>> >>>> We can limit the amd-blobs and intel-blob to binaries following the >>>> naming convention AuthenticAMD.bin and GenuineIntel.bin for amd and >>>> intel, respectively. Yet, this would be imposing an unnecessary >>>> restriction on administrators who may want to be innovative with naming >>>> (or want to use the naming microcode_amd_*.bin or FF-MM-SS instead). >>>> >>>> Alternatively, we can introduce CONFIG_BUILTIN_UCODE_INTEL and >>>> CONFIG_BUILTIN_UCODE_AMD. Both default to empty strings. Then, an >>>> administrator can specify exactly the microcodes to include relative to >>>> the CONFIG_BUILTIN_UCODE_DIR. For example: >>>> CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09" >>>> CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin" >>> >>> This would make the feature even less generic - I already meant to >> >> I do not follow the point about being less generic. (I hope my example >> did not give the false impression that CONFIG_BUILTIN_UCODE_{AMD,INTEL} >> allow for only a single microcode blob for a single signature). > > Well, the example indeed has given this impression to me. I'm > having a hard time seeing how, beyond very narrow special cases, > either of the examples could be useful to anyone. Yet I think > examples should be generally useful. > Andrew's earlier response hinted at "what can we do to avoid picking random bits in the builtin microcode?" My response was outlining alternatives, and the examples there were not meant to be useful beyond explaining those alternatives. >>> ask whether building ucode into binaries is really a useful thing >>> when we already have more flexible ways. I could see this being >>> useful if there was no other way to make ucode available at boot >>> time. >> >> It is useful in addition to the existing ways to do early microcode >> updates. First, when operating many hosts, using boot modules (either a >> distinct microcode module or within an initrd) becomes involved. For >> instance, tools to update boot entries (e.g., >> https://linux.die.net/man/8/grubby) do not support adding arbitrary >> (microcode) modules. > > I.e. you suggest to work around tools shortcomings by extending > Xen? Wouldn't the more appropriate way to deal with this be to > make the tools more capable? > >> Second, there is often need to couple a Xen build with a minimum >> microcode patch level. Having the microcode built within the Xen image >> itself is a streamlined, natural way of achieving that. > > Okay, I can accept this as a reason, to some degree at least. Yet > as said elsewhere, I don't think you want then to override a > possible "external" ucode module with the builtin blobs. Instead > the newest of everything that's available should then be loaded. Extending Xen to work around tools shortcomings is absolutely not what I have in mind. I should have started with the second reason. Read this as: Xen relies on a minimum microcode feature set, and it makes sense to couple both in one binary. This coupling just happens to provide an added benefit in the face of tools shortcoming. Thanks, Eslam
On 11.12.19 10:54, Jan Beulich wrote: > On 11.12.2019 00:18, Eslam Elnikety wrote: >> On 10.12.19 10:37, Jan Beulich wrote: >>> On 09.12.2019 09:41, Eslam Elnikety wrote: >>>> --- a/docs/misc/xen-command-line.pandoc >>>> +++ b/docs/misc/xen-command-line.pandoc >>>> @@ -2113,7 +2113,7 @@ logic applies: >>>> active by default. >>>> >>>> ### ucode (x86) >>>> -> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]` >>>> +> `= List of [ <integer> | scan=<bool> | builtin=<bool>, nmi=<bool> ]` >>> >>> Despite my other question regarding the usefulness of this as a >>> whole a few comments. >>> >>> Do "scan" and "builtin" really need to exclude each other? I.e. >>> don't you mean , instead of | ? >> The useful case here would be specifying ucode=scan,builtin which would >> translate to fallback onto the builtin microcode if a scan fails. In >> fact, this is already the case given the implementation in v1. It will >> be better to clarify this semantic by allowing scan,builtin. >> >> On that note, I really see the "<integer>" and "scan=<bool>" to be >> equal. Following the logic earlier we should probably also allow >> ucode=<integer>,builtin. This translates to fallback to builtin if there >> are no modules at index <integer>. > > Almost - if the builtin one is newer than the separate blob, then > either of the cmdline options you name should still cause the > builtin one to be loaded. IOW you want to honor both options, not > prefer the earlier over a later one. > On the "newest of everything": That's not what I intend to propose. The microcode provided via a scan (or <integer> for that matter) will always override the builtin microcode. The common case would be that the microcode provided via a scan (or <integer>) is newer than the builtin. Yet, an administrator will have the option, if needed, to use an older microcode via a scan (or <integer>) to override a newer builtin microcode. -- Eslam
On 12.12.2019 23:17, Eslam Elnikety wrote: > On the "newest of everything": That's not what I intend to propose. The > microcode provided via a scan (or <integer> for that matter) will always > override the builtin microcode. The common case would be that the > microcode provided via a scan (or <integer>) is newer than the builtin. > Yet, an administrator will have the option, if needed, to use an older > microcode via a scan (or <integer>) to override a newer builtin microcode. That's a fair enough model, but it wants spelling out in the patch description. Jan
On 09/12/2019 21:49, Eslam Elnikety wrote: >>> + >>> +extern const char __builtin_intel_ucode_start[], >>> __builtin_intel_ucode_end[]; >>> +extern const char __builtin_amd_ucode_start[], >>> __builtin_amd_ucode_end[]; >>> +#endif >>> + >>> /* By default, ucode loading is done in NMI handler */ >>> static bool ucode_in_nmi = true; >>> @@ -110,9 +118,9 @@ void __init microcode_set_module(unsigned int >>> idx) >>> } >>> /* >>> - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both >>> options are >>> - * optional. If the EFI has forced which of the multiboot payloads >>> is to be >>> - * used, only nmi=<bool> is parsed. >>> + * The format is '[<integer>|scan=<bool>|builtin=<bool>, >>> nmi=<bool>]'. All >>> + * options are optional. If the EFI has forced which of the >>> multiboot payloads >>> + * is to be used, only nmi=<bool> is parsed. >>> */ >> >> Please delete this, or I'll do a prereq patch to fix it and the command >> line docs. (Both are in a poor state.) >> > > Unless you are planning that along your on-going > docs/hypervisor-guide/microcode-loading.rst effort, I can pick up this > clean-up/prereq patch myself. What do you have in mind? (Or point me > to a good example and I will figure things out). c/s 3c5552954, 53a84f672, 633a40947 or 3136dee9c are good examples. ucode= is definitely more complicated to explain because of its implicit EFI behaviour. > >>> + else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) >>> + ucode_blob.size = (size_t)(__builtin_intel_ucode_end >>> + - __builtin_intel_ucode_start); >>> + else >>> + return; >>> + >>> + if ( !ucode_blob.size ) >>> + { >>> + printk("No builtin ucode! 'ucode=builtin' is nullified.\n"); >>> + return; >>> + } >>> + else if ( ucode_blob.size > MAX_EARLY_CPIO_MICROCODE ) >>> + { >>> + printk("Builtin microcode payload too big! (%ld, we can do >>> %d)\n", >>> + ucode_blob.size, MAX_EARLY_CPIO_MICROCODE); >>> + ucode_blob.size = 0; >>> + return; >>> + } >>> + >>> + ucode_blob.data = xmalloc_bytes(ucode_blob.size); >>> + if ( !ucode_blob.data ) >>> + return; >> >> Any chance we can reuse the "fits" logic to avoid holding every >> inapplicable blob in memory as well? >> > > I think this would be a welcomed change. It seems to me that we have > two ways to go about it. > > 1) We factor the code in the intel-/amd-specific cpu_request_microcode > to extract logic for finding a match into its own new function, expose > that through microcode_ops, and finally do xalloc only for the > matching microcode when early loading is scan or builtin. > > 2) Cannot we just do away completely with xalloc? I see that each > individual microcode update gets allocated anyway in > microcode_intel.c/get_next_ucode_from_buffer() and in > microcode_amd.c/cpu_request_microcode(). Unless I am missing > something, the xmalloc_bytes for ucode_blob.data is redundant. > > Thoughts? I'm certain the code is more complicated than it needs to be. Cleanup/simplification would be very welcome. And if you're up for that, there is a related area which would be a great improvement. At the moment, BSP microcode loading is very late because it depends on this xmalloc() to begin with. However, no memory allocation is needed to load microcode from a multiboot module or from the initrd, or from this future builtin location - all loading can be done from a directmap/bootmap pointer if needs be. This would allow moving the BSP microcode to much earlier on boot, probably somewhere between console setup and E820 handling. One way or another, the microcode cache which persists past boot has to be xmalloc()'d, because we will free the module/initrd/builtin. It would however be more friendly to AP's to only give them the single correct piece of ucode, rather than everything to scan through. (These behaviours and expectations are going to be a chunk of my intended second microcode.rst doc, including a "be aware that machines exist which do $X" section to cover some of the weirder corner cases we have encountered.) > >>> + >>> +builtin_ucode.o: Makefile $(amd-blobs) $(intel-blobs) >>> + # Create AMD microcode blob if there are AMD updates on the >>> build system >>> + if [ ! -z "$(amd-blobs)" ]; then \ >>> + cat $(amd-blobs) > $@.bin ; \ >>> + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 >>> --rename-section >>> .data=.builtin_amd_ucode,alloc,load,readonly,data,contents $@.bin >>> $@.amd; \ >>> + rm -f $@.bin; \ >>> + fi >>> + # Create INTEL microcode blob if there are INTEL updates on the >>> build system >>> + if [ ! -z "$(intel-blobs)" ]; then \ >>> + cat $(intel-blobs) > $@.bin; \ >>> + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 >>> --rename-section >>> .data=.builtin_intel_ucode,alloc,load,readonly,data,contents $@.bin >>> $@.intel; \ >>> + rm -f $@.bin; \ >>> + fi >>> + # Create fake builtin_ucode.o if no updates were present. >>> Otherwise, builtin_ucode.o carries the available updates >>> + if [ -z "$(amd-blobs)" -a -z "$(intel-blobs)" ]; then \ >>> + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; \ >>> + else \ >>> + $(LD) $(LDFLAGS) -r -o $@ $@.*; \ >>> + rm -f $@.*; \ >>> + fi >> >> How about using weak symbols, rather than playing games like this? > > Just to make sure we are on the same page. You are after a dummy > binary with weak symbols that eventually get overridden when I link > the actual microcode binaries into builtin_ucode.o? If so, possible of > course. Except that I do not particularly see the downside of the > existing approach with dummy builtin_ucode.o. Actually, you don't even need week symbols. Size being 0 means that no blob was inserted. There doesn't appear to be a need to organise a dummy builtin_ucode.o, or to manually merge Intel/AMD together. Simply make obj-y += ucode-$VENDOR.o dependent on there being some blob to insert. ~Andrew
On 12/12/2019 22:13, Eslam Elnikety wrote: >>> Second, there is often need to couple a Xen build with a minimum >>> microcode patch level. Having the microcode built within the Xen image >>> itself is a streamlined, natural way of achieving that. >> >> Okay, I can accept this as a reason, to some degree at least. Yet >> as said elsewhere, I don't think you want then to override a >> possible "external" ucode module with the builtin blobs. Instead >> the newest of everything that's available should then be loaded. > > Extending Xen to work around tools shortcomings is absolutely not what > I have in mind. I should have started with the second reason. Read > this as: Xen relies on a minimum microcode feature set, and it makes > sense to couple both in one binary. This coupling just happens to > provide an added benefit in the face of tools shortcoming. Do we have anything which strictly relies on a minimum version? I can definitely see the value of bundling the ucode and saying "this is the minimum we will tolerate" from a supportability point of view. There is also value when it comes to easier SRTM/DRTM measurements of the system in question, including cases where Xen sits on a boot ROM rather than on disk. ~Andrew
> There is also value when it comes to easier SRTM/DRTM measurements of > the system in question, including cases where Xen sits on a boot ROM > rather than on disk. We've explored that in the past - building things into Xen and Linux statically - and ultimately it only works if the command line passed to Xen also gets measured, otherwise you can always override any built-in component. So for example with OpenXT on UEFI the entire Xen config file gets measured. For DRTM I don't think it makes much difference, I believe the active microcode info is already part of the measurement, so having it measured as part of the Xen blob doesn't add anything. Tamas
On 13/12/2019 20:15, Tamas K Lengyel wrote: >> There is also value when it comes to easier SRTM/DRTM measurements of >> the system in question, including cases where Xen sits on a boot ROM >> rather than on disk. > We've explored that in the past - building things into Xen and Linux > statically - and ultimately it only works if the command line passed > to Xen also gets measured, otherwise you can always override any > built-in component. So for example with OpenXT on UEFI the entire Xen > config file gets measured. What I meant was "its one fewer random knobble which needs handling specially". > For DRTM I don't think it makes much > difference, I believe the active microcode info is already part of the > measurement, so having it measured as part of the Xen blob doesn't add > anything. I couldn't possibly comment on timelines, but if I could, the answer might be "not for a little while yet". For now, microcode is very definitely software's problem to include in measurements. ~Andrew
> > For DRTM I don't think it makes much > > difference, I believe the active microcode info is already part of the > > measurement, so having it measured as part of the Xen blob doesn't add > > anything. > > I couldn't possibly comment on timelines, but if I could, the answer > might be "not for a little while yet". > > For now, microcode is very definitely software's problem to include in > measurements. Ah right, I was mixing it up with the ACM blob that gets measured there. Tamas
On 13.12.19 14:57, Andrew Cooper wrote: > On 12/12/2019 22:13, Eslam Elnikety wrote: >>>> Second, there is often need to couple a Xen build with a minimum >>>> microcode patch level. Having the microcode built within the Xen image >>>> itself is a streamlined, natural way of achieving that. >>> >>> Okay, I can accept this as a reason, to some degree at least. Yet >>> as said elsewhere, I don't think you want then to override a >>> possible "external" ucode module with the builtin blobs. Instead >>> the newest of everything that's available should then be loaded. >> >> Extending Xen to work around tools shortcomings is absolutely not what >> I have in mind. I should have started with the second reason. Read >> this as: Xen relies on a minimum microcode feature set, and it makes >> sense to couple both in one binary. This coupling just happens to >> provide an added benefit in the face of tools shortcoming. > > Do we have anything which strictly relies on a minimum version? I had in mind microcode speculation mitigation features when reasoning with the minimum patch level argument. -- Eslam
On 17/12/2019 22:41, Eslam Elnikety wrote: > On 13.12.19 14:57, Andrew Cooper wrote: >> On 12/12/2019 22:13, Eslam Elnikety wrote: >>>>> Second, there is often need to couple a Xen build with a minimum >>>>> microcode patch level. Having the microcode built within the Xen >>>>> image >>>>> itself is a streamlined, natural way of achieving that. >>>> >>>> Okay, I can accept this as a reason, to some degree at least. Yet >>>> as said elsewhere, I don't think you want then to override a >>>> possible "external" ucode module with the builtin blobs. Instead >>>> the newest of everything that's available should then be loaded. >>> >>> Extending Xen to work around tools shortcomings is absolutely not what >>> I have in mind. I should have started with the second reason. Read >>> this as: Xen relies on a minimum microcode feature set, and it makes >>> sense to couple both in one binary. This coupling just happens to >>> provide an added benefit in the face of tools shortcoming. >> >> Do we have anything which strictly relies on a minimum version? > > I had in mind microcode speculation mitigation features when reasoning > with the minimum patch level argument. Considering how well the first round of speculative microcode went, mandating it would have been a rather bad thing... But yes - as a usecase of "I wish to bundle the minimum microcode I'd like to work with", this seems entirely reasonable. ~Andrew
On 13.12.19 14:40, Andrew Cooper wrote: > On 09/12/2019 21:49, Eslam Elnikety wrote: >>>> + >>>> +extern const char __builtin_intel_ucode_start[], >>>> __builtin_intel_ucode_end[]; >>>> +extern const char __builtin_amd_ucode_start[], >>>> __builtin_amd_ucode_end[]; >>>> +#endif >>>> + >>>> /* By default, ucode loading is done in NMI handler */ >>>> static bool ucode_in_nmi = true; >>>> @@ -110,9 +118,9 @@ void __init microcode_set_module(unsigned int >>>> idx) >>>> } >>>> /* >>>> - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both >>>> options are >>>> - * optional. If the EFI has forced which of the multiboot payloads >>>> is to be >>>> - * used, only nmi=<bool> is parsed. >>>> + * The format is '[<integer>|scan=<bool>|builtin=<bool>, >>>> nmi=<bool>]'. All >>>> + * options are optional. If the EFI has forced which of the >>>> multiboot payloads >>>> + * is to be used, only nmi=<bool> is parsed. >>>> */ >>> >>> Please delete this, or I'll do a prereq patch to fix it and the command >>> line docs. (Both are in a poor state.) >>> >> >> Unless you are planning that along your on-going >> docs/hypervisor-guide/microcode-loading.rst effort, I can pick up this >> clean-up/prereq patch myself. What do you have in mind? (Or point me >> to a good example and I will figure things out). > > c/s 3c5552954, 53a84f672, 633a40947 or 3136dee9c are good examples. > ucode= is definitely more complicated to explain because of its implicit > EFI behaviour. > Currently massaging a patch to that effect. >>>> + else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) >>>> + ucode_blob.size = (size_t)(__builtin_intel_ucode_end >>>> + - __builtin_intel_ucode_start); >>>> + else >>>> + return; >>>> + >>>> + if ( !ucode_blob.size ) >>>> + { >>>> + printk("No builtin ucode! 'ucode=builtin' is nullified.\n"); >>>> + return; >>>> + } >>>> + else if ( ucode_blob.size > MAX_EARLY_CPIO_MICROCODE ) >>>> + { >>>> + printk("Builtin microcode payload too big! (%ld, we can do >>>> %d)\n", >>>> + ucode_blob.size, MAX_EARLY_CPIO_MICROCODE); >>>> + ucode_blob.size = 0; >>>> + return; >>>> + } >>>> + >>>> + ucode_blob.data = xmalloc_bytes(ucode_blob.size); >>>> + if ( !ucode_blob.data ) >>>> + return; >>> >>> Any chance we can reuse the "fits" logic to avoid holding every >>> inapplicable blob in memory as well? >>> >> >> I think this would be a welcomed change. It seems to me that we have >> two ways to go about it. >> >> 1) We factor the code in the intel-/amd-specific cpu_request_microcode >> to extract logic for finding a match into its own new function, expose >> that through microcode_ops, and finally do xalloc only for the >> matching microcode when early loading is scan or builtin. >> >> 2) Cannot we just do away completely with xalloc? I see that each >> individual microcode update gets allocated anyway in >> microcode_intel.c/get_next_ucode_from_buffer() and in >> microcode_amd.c/cpu_request_microcode(). Unless I am missing >> something, the xmalloc_bytes for ucode_blob.data is redundant. >> >> Thoughts? > > I'm certain the code is more complicated than it needs to be. > Cleanup/simplification would be very welcome. And if you're up for > that, there is a related area which would be a great improvement. > > At the moment, BSP microcode loading is very late because it depends on > this xmalloc() to begin with. However, no memory allocation is needed > to load microcode from a multiboot module or from the initrd, or from > this future builtin location - all loading can be done from a > directmap/bootmap pointer if needs be. > > This would allow moving the BSP microcode to much earlier on boot, > probably somewhere between console setup and E820 handling. > > One way or another, the microcode cache which persists past boot has to > be xmalloc()'d, because we will free the module/initrd/builtin. It > would however be more friendly to AP's to only give them the single > correct piece of ucode, rather than everything to scan through. > > (These behaviours and expectations are going to be a chunk of my > intended second microcode.rst doc, including a "be aware that machines > exist which do $X" section to cover some of the weirder corner cases we > have encountered.) > Avoiding the xmalloc/memcpy on the scan for microcode is one of the patches that I will share shortly. In particular, the ucode_blob.data would directly point to the buffer matching the canonical name within the cpio name space. We are still a bit away from pushing the BSP microcode update earlier though. We will need to surgically remove all the unnecessary xmalloc/memcpy from within microcode_{amd,intel}.c. Also, as you hinted, the challenging bit is the per-cpu microcode cache. >>>> + >>>> +builtin_ucode.o: Makefile $(amd-blobs) $(intel-blobs) >>>> + # Create AMD microcode blob if there are AMD updates on the >>>> build system >>>> + if [ ! -z "$(amd-blobs)" ]; then \ >>>> + cat $(amd-blobs) > $@.bin ; \ >>>> + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 >>>> --rename-section >>>> .data=.builtin_amd_ucode,alloc,load,readonly,data,contents $@.bin >>>> $@.amd; \ >>>> + rm -f $@.bin; \ >>>> + fi >>>> + # Create INTEL microcode blob if there are INTEL updates on the >>>> build system >>>> + if [ ! -z "$(intel-blobs)" ]; then \ >>>> + cat $(intel-blobs) > $@.bin; \ >>>> + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 >>>> --rename-section >>>> .data=.builtin_intel_ucode,alloc,load,readonly,data,contents $@.bin >>>> $@.intel; \ >>>> + rm -f $@.bin; \ >>>> + fi >>>> + # Create fake builtin_ucode.o if no updates were present. >>>> Otherwise, builtin_ucode.o carries the available updates >>>> + if [ -z "$(amd-blobs)" -a -z "$(intel-blobs)" ]; then \ >>>> + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; \ >>>> + else \ >>>> + $(LD) $(LDFLAGS) -r -o $@ $@.*; \ >>>> + rm -f $@.*; \ >>>> + fi >>> >>> How about using weak symbols, rather than playing games like this? >> >> Just to make sure we are on the same page. You are after a dummy >> binary with weak symbols that eventually get overridden when I link >> the actual microcode binaries into builtin_ucode.o? If so, possible of >> course. Except that I do not particularly see the downside of the >> existing approach with dummy builtin_ucode.o. > > Actually, you don't even need week symbols. Size being 0 means that no > blob was inserted. > > There doesn't appear to be a need to organise a dummy builtin_ucode.o, > or to manually merge Intel/AMD together. Simply make obj-y += > ucode-$VENDOR.o dependent on there being some blob to insert. I have reworked this part in v2 such that the configurations specify explicitly the individual microcode blobs to include. I have also adopted the "obj-y += ucode-$VENDOR.o" and made it dependent on the corresponding blobs being available. That said, I was not able to get rid of the dummy object. The dummy is still needed in case no amd nor intel ucode blobs were specified. In case of no microcode blobs, obj-y will not refer to any dependency within xen/arch/x86/microcode/ and there will be no rule to generate microcode/built_in.o (which is required for all subdir in xen/arch/x86/). Of course, we can do logic in xen/arch/x86/Makefile to mark microcode as a subdir iff there are microcode blobs available, but it seems to me that this logic does not belong there. Also, my initial attempt at this quickly proved that the dummy approach is way simpler. -- Eslam > > ~Andrew >
diff --git a/docs/misc/builtin-ucode.txt b/docs/misc/builtin-ucode.txt new file mode 100644 index 0000000000..43bb60d3eb --- /dev/null +++ b/docs/misc/builtin-ucode.txt @@ -0,0 +1,60 @@ +------------------------------------------------- +Builtin Microcode Support for x86 (AMD and INTEL) +------------------------------------------------- +Author: + Eslam Elnikety <elnikety@amazon.com> +Initial version: + Dec 2019 +------------------------------------------------- + +About: +------ +* This documentation describes preparing the builtin microcode blobs to use as + builtin microcode update within the Xen image itself. + +* Support for builtin microcode is limited to x86. + +* Builtin support is available via the configurations BUILTIN_UCODE and + BUILTIN_UCODE_DIR. The first enables the support (default is off), and the + latter directs the build system to where it can find the microcode directory + (default is /lib/firmware). + +Microcode Directory: +-------------------- +This directory holds the microcode blobs to be built in the Xen image. There +are two subdirectories: amd-ucode and intel-ucode for AMD and INTEL, +respectively. + +INTEL microcode blobs typically follow the naming format FF-MM-SS for +{F}amily-{M}odel-{S}tepping. Alternatively, GenuineIntel.bin bundles a bunch +of FF-MM-SS blobs into a single binary and the one matching the host CPU gets +picked when performing the microcode update. For AMD, the canonical name is +AuthenticAMD.bin. Similarly, such binary can bundle a bunch of microcode blobs +for different families. + +The builtin microcode is generated by concatenating the microcode blobs under +intel-ucode into GenuineIntel.bin, and those under amd-ucode into +AuthenticAMD.bin. Those are then copied into the Xen image itself. + +Here is an example microcode directory structure, following the convention [1]: + +/lib/firmware +|-- amd-ucode +... +| |-- microcode_amd_fam15h.bin +... +|-- intel-ucode +... +| |-- 06-3a-09 +... + +Alternatively, the subdirectories can directly contain GenuineIntel.bin and +AuthenticAMD.bin (since both are concatenation of the individual microcode +blobs and the end result is the same). + +An empty or non-existant subdirectory (amd-ucode and/or intel-ucode) excludes +the respective AMD or INTEL microcode from being built in. + +Reference(s): +------------- +[1] https://www.kernel.org/doc/Documentation/x86/microcode.txt diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 891d2d439f..ba25db95da 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2113,7 +2113,7 @@ logic applies: active by default. ### ucode (x86) -> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]` +> `= List of [ <integer> | scan=<bool> | builtin=<bool>, nmi=<bool> ]` Specify how and where to find CPU microcode update blob. @@ -2128,6 +2128,9 @@ when used with xen.efi (there the concept of modules doesn't exist, and the blob gets specified via the `ucode=<filename>` config file/section entry; see [EFI configuration file description](efi.html)). +'builtin' instructs the hypervisor to use the builtin microcode update. This +option is available only if option BUILTIN_UCODE is enabled. + 'scan' instructs the hypervisor to scan the multiboot images for an cpio image that contains microcode. Depending on the platform the blob with the microcode in the cpio name space must be: diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 02bb05f42e..14c5992d86 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -218,6 +218,26 @@ config MEM_SHARING bool "Xen memory sharing support" if EXPERT = "y" depends on HVM +config BUILTIN_UCODE + def_bool n + prompt "Support for Builtin Microcode" + ---help--- + Include the CPU microcode update in the Xen image itself. With this + support, Xen can update the CPU microcode upon boot using the builtin + microcode, with no need for an additional microcode boot modules. + + If unsure, say N. + +config BUILTIN_UCODE_DIR + string + default "/lib/firmware" + depends on BUILTIN_UCODE + ---help--- + The directory containing the microcode blobs. + + See docs/misc/builtin-ucode.txt for how such directory should be + structured to hold AMD and INTEL microcode. + endmenu source "common/Kconfig" diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 7da5a2631e..8ac93a15a7 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -7,6 +7,7 @@ subdir-y += mm subdir-$(CONFIG_XENOPROF) += oprofile subdir-$(CONFIG_PV) += pv subdir-y += x86_64 +subdir-$(CONFIG_BUILTIN_UCODE) += microcode alternative-y := alternative.init.o alternative-$(CONFIG_LIVEPATCH) := diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c index 6ced293d88..7afbe44286 100644 --- a/xen/arch/x86/microcode.c +++ b/xen/arch/x86/microcode.c @@ -97,6 +97,14 @@ static struct ucode_mod_blob __initdata ucode_blob; */ static bool_t __initdata ucode_scan; +#ifdef CONFIG_BUILTIN_UCODE +/* builtin is the default when BUILTIN_UCODE is set */ +static bool_t __initdata ucode_builtin = 1; + +extern const char __builtin_intel_ucode_start[], __builtin_intel_ucode_end[]; +extern const char __builtin_amd_ucode_start[], __builtin_amd_ucode_end[]; +#endif + /* By default, ucode loading is done in NMI handler */ static bool ucode_in_nmi = true; @@ -110,9 +118,9 @@ void __init microcode_set_module(unsigned int idx) } /* - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are - * optional. If the EFI has forced which of the multiboot payloads is to be - * used, only nmi=<bool> is parsed. + * The format is '[<integer>|scan=<bool>|builtin=<bool>, nmi=<bool>]'. All + * options are optional. If the EFI has forced which of the multiboot payloads + * is to be used, only nmi=<bool> is parsed. */ static int __init parse_ucode(const char *s) { @@ -130,6 +138,10 @@ static int __init parse_ucode(const char *s) { if ( (val = parse_boolean("scan", s, ss)) >= 0 ) ucode_scan = val; +#ifdef CONFIG_BUILTIN_UCODE + else if ( (val = parse_boolean("builtin", s, ss)) >= 0 ) + ucode_builtin = val; +#endif else { const char *q; @@ -237,6 +249,48 @@ void __init microcode_grab_module( scan: if ( ucode_scan ) microcode_scan_module(module_map, mbi); + +#ifdef CONFIG_BUILTIN_UCODE + /* + * Do not use the builtin microcode if: + * (a) builtin has been explicitly turned off (e.g., ucode=no-builtin) + * (b) a microcode module has been specified or a scan is successful + */ + if ( !ucode_builtin || ucode_mod.mod_end || ucode_blob.size ) + return; + + /* Set ucode_start/_end to the proper blob */ + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) + ucode_blob.size = (size_t)(__builtin_amd_ucode_end + - __builtin_amd_ucode_start); + else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) + ucode_blob.size = (size_t)(__builtin_intel_ucode_end + - __builtin_intel_ucode_start); + else + return; + + if ( !ucode_blob.size ) + { + printk("No builtin ucode! 'ucode=builtin' is nullified.\n"); + return; + } + else if ( ucode_blob.size > MAX_EARLY_CPIO_MICROCODE ) + { + printk("Builtin microcode payload too big! (%ld, we can do %d)\n", + ucode_blob.size, MAX_EARLY_CPIO_MICROCODE); + ucode_blob.size = 0; + return; + } + + ucode_blob.data = xmalloc_bytes(ucode_blob.size); + if ( !ucode_blob.data ) + return; + + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) + memcpy(ucode_blob.data, __builtin_amd_ucode_start, ucode_blob.size); + else + memcpy(ucode_blob.data, __builtin_intel_ucode_start, ucode_blob.size); +#endif } const struct microcode_ops *microcode_ops; diff --git a/xen/arch/x86/microcode/Makefile b/xen/arch/x86/microcode/Makefile new file mode 100644 index 0000000000..6d585c5482 --- /dev/null +++ b/xen/arch/x86/microcode/Makefile @@ -0,0 +1,40 @@ +# Copyright (C) 2019 Amazon.com, Inc. or its affiliates. +# Author: Eslam Elnikety <elnikety@amazon.com> +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +obj-y += builtin_ucode.o + +# Directory holding the microcode updates. +UCODE_DIR=$(patsubst "%",%,$(CONFIG_BUILTIN_UCODE_DIR)) +amd-blobs := $(wildcard $(UCODE_DIR)/amd-ucode/*) +intel-blobs := $(wildcard $(UCODE_DIR)/intel-ucode/*) + +builtin_ucode.o: Makefile $(amd-blobs) $(intel-blobs) + # Create AMD microcode blob if there are AMD updates on the build system + if [ ! -z "$(amd-blobs)" ]; then \ + cat $(amd-blobs) > $@.bin ; \ + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section .data=.builtin_amd_ucode,alloc,load,readonly,data,contents $@.bin $@.amd; \ + rm -f $@.bin; \ + fi + # Create INTEL microcode blob if there are INTEL updates on the build system + if [ ! -z "$(intel-blobs)" ]; then \ + cat $(intel-blobs) > $@.bin; \ + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section .data=.builtin_intel_ucode,alloc,load,readonly,data,contents $@.bin $@.intel; \ + rm -f $@.bin; \ + fi + # Create fake builtin_ucode.o if no updates were present. Otherwise, builtin_ucode.o carries the available updates + if [ -z "$(amd-blobs)" -a -z "$(intel-blobs)" ]; then \ + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; \ + else \ + $(LD) $(LDFLAGS) -r -o $@ $@.*; \ + rm -f $@.*; \ + fi diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 111edb5360..7a4c58c246 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -265,6 +265,18 @@ SECTIONS *(SORT(.data.vpci.*)) __end_vpci_array = .; #endif + +#if defined(CONFIG_BUILTIN_UCODE) + . = ALIGN(POINTER_ALIGN); + __builtin_amd_ucode_start = .; + *(.builtin_amd_ucode) + __builtin_amd_ucode_end = .; + + . = ALIGN(POINTER_ALIGN); + __builtin_intel_ucode_start = .; + *(.builtin_intel_ucode) + __builtin_intel_ucode_end = .; +#endif } :text . = ALIGN(SECTION_ALIGN);
Xen relies on boot modules to perform early microcode updates. This commit adds another mode, namely "builtin" via the BUILTIN_UCODE config parameter. If set, the Xen image itself will contain the microcode updates. Upon boot, Xen inspects its image for microcode blobs and performs the update. A Xen image with builtin microcode can be explicitly instructed to: (a) look for microcode elsewhere (e.g., a boot module that contains more recent microcodes via ucode=scan), or (b) skip the builtin microcode update (e.g., ucode=no-builtin). Signed-off-by: Eslam Elnikety <elnikety@amazon.com> --- docs/misc/builtin-ucode.txt | 60 +++++++++++++++++++++++++++++++ docs/misc/xen-command-line.pandoc | 5 ++- xen/arch/x86/Kconfig | 20 +++++++++++ xen/arch/x86/Makefile | 1 + xen/arch/x86/microcode.c | 60 +++++++++++++++++++++++++++++-- xen/arch/x86/microcode/Makefile | 40 +++++++++++++++++++++ xen/arch/x86/xen.lds.S | 12 +++++++ 7 files changed, 194 insertions(+), 4 deletions(-) create mode 100644 docs/misc/builtin-ucode.txt create mode 100644 xen/arch/x86/microcode/Makefile