| Message ID | cf29db3bde903a5788322381ef6eac1a6ed9b2b9.1576630344.git.elnikety@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/microcode: Support builtin CPU microcode | expand |
On 18.12.2019 02:32, Eslam Elnikety wrote: > 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 will, by default, attempt the microcode > update. Disabling the builtin microcode update can be done via the Xen command > line parameter 'ucode=no-builtin'. Moreover, the microcode provided via other > options (such as 'ucode=<integer>|scan' or 'ucode=<filename>' config when > booting via EFI) takes precedence over the builtin one. > > Signed-off-by: Eslam Elnikety <elnikety@amazon.com> > > --- > Changes in v2: > - Allow for ucode=<integer>|scan,{no-}builtin and detail the model. Reflect > those changes onto microcode.c and docs/misc/xen-command-line.pandoc > - Add documentation to the existing docs/admin-guide/microcode-loading.rst > - Build on Patches 1--3 to avoid xmalloc/memcpy for the builtin microcode > - Work configuration in order to specify the individual microcode blobs to use > for the builtin microcode, and rework the microcode/Makefile accordingly > --- > docs/admin-guide/microcode-loading.rst | 31 +++++++++++++++ > docs/misc/xen-command-line.pandoc | 10 ++++- > xen/arch/x86/Kconfig | 30 +++++++++++++++ > xen/arch/x86/Makefile | 1 + > xen/arch/x86/microcode.c | 52 ++++++++++++++++++++++++++ > xen/arch/x86/microcode/Makefile | 46 +++++++++++++++++++++++ > xen/arch/x86/xen.lds.S | 12 ++++++ > 7 files changed, 180 insertions(+), 2 deletions(-) > create mode 100644 xen/arch/x86/microcode/Makefile > > diff --git a/docs/admin-guide/microcode-loading.rst b/docs/admin-guide/microcode-loading.rst > index e83cadd2c2..989e8d446b 100644 > --- a/docs/admin-guide/microcode-loading.rst > +++ b/docs/admin-guide/microcode-loading.rst > @@ -104,6 +104,37 @@ The ``ucode=scan`` command line option will cause Xen to search through all > modules to find any CPIO archives, and search the archive for the applicable > file. Xen will stop searching at the first match. > > +Loading microcode built within the Xen image I think either s/within/into/ or e.g. s/built/contained/. > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +Xen can bundle microcode updates within its image. This support is conditional > +on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is > +useful to ensure that, by default, a minimum microcode patch level will be > +applied to the underlying CPU. > + > +To use microcode updates available on the build system as builtin, > +use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates > +and specify the individual microcode patches via either BUILTIN_UCODE_AMD or > +BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For > +instance, the configuration below is suitable for a build system which has a > +``/lib/firmware/`` directory which, in turn, includes the individual microcode > +patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and > +``intel-ucode/06-2f-02``. > + > + CONFIG_BUILTIN_UCODE=y > + CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/" > + CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin" > + CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02" Rather than a blank as separator, the more conventional one on Unix and alike would be : I think. Of course ideally there wouldn't be any restriction at all on the characters usable here for file names. > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -218,6 +218,36 @@ config MEM_SHARING > bool "Xen memory sharing support" if EXPERT = "y" > depends on HVM > > +config BUILTIN_UCODE > + bool "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. I continue to be unconvinced that this separate option is needed. Albeit compared to the v1 approach I will agree that handling would become more complicated without. > +config BUILTIN_UCODE_DIR > + string "Directory containing microcode updates" > + default "/lib/firmware/" > + depends on BUILTIN_UCODE > + ---help--- > + The directory containing the microcode blobs. > + > +config BUILTIN_UCODE_AMD > + string "AMD microcode updates" > + default "" > + depends on BUILTIN_UCODE > + ---help--- > + AMD builtin microcode; space-sparated, relative to BUILTIN_UCODE_DIR. > + > +config BUILTIN_UCODE_INTEL > + string "INTEL microcode updates" > + default "" > + depends on BUILTIN_UCODE > + ---help--- > + INTEL builtin microcode; space-sparated, relative to BUILTIN_UCODE_DIR. I don't think Intel is commonly spelled all uppercase. I further think you want to mention further constraints (beyond the separator to use): DIR would probably better be an absolute path, it's ambiguous (right here, i.e. without looking at the implementation) whether a trailing slash needs including), the individual blobs may include paths, and it's ambiguous again what them having a leading slash would mean (I think it would be better if it was "absolute or relative to BUILTIN_UCODE_DIR"). Also a spelling nit: "separated". > --- 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 __initdata ucode_builtin = true; > + > +extern const char __builtin_intel_ucode_start[], __builtin_intel_ucode_end[]; > +extern const char __builtin_amd_ucode_start[], __builtin_amd_ucode_end[]; While we do use plain char elsewhere for similar purposes, I think this is bad practice. It would better be unsigned char or uint8_t. > @@ -208,6 +220,40 @@ void __init microcode_grab_module( > ucode_mod = mod[ucode_mod_idx]; > else 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 ) > + { > + ucode_builtin = false; > + return; > + } > + > + /* Set ucode_start/_end to the proper blob */ > + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) > + { > + ucode_blob.size = __builtin_amd_ucode_end - __builtin_amd_ucode_start; > + ucode_blob.data = __builtin_amd_ucode_start; > + } > + else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) > + { > + ucode_blob.size = __builtin_intel_ucode_end - > + __builtin_intel_ucode_start; > + ucode_blob.data = __builtin_intel_ucode_start; > + } > + else > + return; "switch ( boot_cpu_data.x86_vendor )" please. > + if ( !ucode_blob.size ) > + { > + printk("No builtin ucode for the CPU vendor.\n"); Please either omit "for the CPU vendor" or name the vendor. In any event please omit the full stop. > @@ -701,7 +747,13 @@ static int __init microcode_init(void) > */ > if ( ucode_blob.size ) > { > +#ifdef CONFIG_BUILTIN_UCODE > + /* No need to destroy module mappings if builtin was used */ > + if ( !ucode_builtin ) > + bootstrap_map(NULL); > +#else > bootstrap_map(NULL); > +#endif First of all - is there no ucode unrelated side effect of this invocation? I.e. can it safely be skipped? If yes, then I think you want to get away without #ifdef here, by having a suitably placed #define ucode_builtin false somewhere up the file. > --- /dev/null > +++ b/xen/arch/x86/microcode/Makefile > @@ -0,0 +1,46 @@ > +# 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. > + > +# Remove quotes and excess spaces from configuration strings > +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR))) > +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD))) > +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL))) > + > +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs. > +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD))) > +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL))) > + > +ifneq ($(amd-blobs),) > +obj-y += ucode_amd.o > +endif > + > +ifneq ($(intel-blobs),) > +obj-y += ucode_intel.o > +endif > + > +ifeq ($(amd-blobs)$(intel-blobs),) > +obj-y += ucode_dummy.o > +endif > + > +ucode_amd.o: Makefile $(amd-blobs) > + 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 $@ > + rm -f $@.bin > + > +ucode_intel.o: Makefile $(intel-blobs) > + 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 $@ > + rm -f $@.bin This can be had with a pattern rule (with the vendor being the stem) and hence without duplication, I think. Also - is simply concatenating the blobs reliable enough? There's no build time diagnostic that the result would actually be understood at runtime. > +ucode_dummy.o: Makefile > + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; Since the commit message doesn't explain why this is needed, I have to ask (I guess we somewhere have a dependency on $(obj-y) not being empty). _If_ it is needed, I don't see why you need ifeq() around its use. In fact you could have obj-y := ucode-dummy.o right at the top of the file. Furthermore I don't really understand why you need this in the first place. While cat won't do what you want with an empty argument list, can't you simply prepend / append /dev/null? > --- 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); Why (same further down)? The alignment of both vendors' header structures is just 4 afaict. Jan
On 18.12.19 13:42, Jan Beulich wrote: > On 18.12.2019 02:32, Eslam Elnikety wrote: >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> + >> +Xen can bundle microcode updates within its image. This support is conditional >> +on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is >> +useful to ensure that, by default, a minimum microcode patch level will be >> +applied to the underlying CPU. >> + >> +To use microcode updates available on the build system as builtin, >> +use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates >> +and specify the individual microcode patches via either BUILTIN_UCODE_AMD or >> +BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For >> +instance, the configuration below is suitable for a build system which has a >> +``/lib/firmware/`` directory which, in turn, includes the individual microcode >> +patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and >> +``intel-ucode/06-2f-02``. >> + >> + CONFIG_BUILTIN_UCODE=y >> + CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/" >> + CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin" >> + CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02" > > Rather than a blank as separator, the more conventional one on > Unix and alike would be : I think. Of course ideally there wouldn't > be any restriction at all on the characters usable here for file > names. > It would be great if there is a particular convention. The blank separator is aligned with Linux way of doing builtin microcode. >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -218,6 +218,36 @@ config MEM_SHARING >> bool "Xen memory sharing support" if EXPERT = "y" >> depends on HVM >> >> +config BUILTIN_UCODE >> + bool "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. > > I continue to be unconvinced that this separate option is needed. > Albeit compared to the v1 approach I will agree that handling > would become more complicated without. > Any particular preference between the v1 vs v2 approach? >> @@ -701,7 +747,13 @@ static int __init microcode_init(void) >> */ >> if ( ucode_blob.size ) >> { >> +#ifdef CONFIG_BUILTIN_UCODE >> + /* No need to destroy module mappings if builtin was used */ >> + if ( !ucode_builtin ) >> + bootstrap_map(NULL); >> +#else >> bootstrap_map(NULL); >> +#endif > > First of all - is there no ucode unrelated side effect of this > invocation? I.e. can it safely be skipped? Maybe I am missing something. Are you asking if we can safely skip the bootstrap_map(NULL)? (Quoting your response on PATCH v2 2/4 "And of course we really want these mappings to be gone") > If yes, then I think > you want to get away without #ifdef here, by having a suitably > placed > > #define ucode_builtin false > > somewhere up the file. > Agreed. That will make the code snippet more readable indeed. >> --- /dev/null >> +++ b/xen/arch/x86/microcode/Makefile >> @@ -0,0 +1,46 @@ >> +# 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. >> + >> +# Remove quotes and excess spaces from configuration strings >> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR))) >> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD))) >> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL))) >> + >> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs. >> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD))) >> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL))) >> + >> +ifneq ($(amd-blobs),) >> +obj-y += ucode_amd.o >> +endif >> + >> +ifneq ($(intel-blobs),) >> +obj-y += ucode_intel.o >> +endif >> + >> +ifeq ($(amd-blobs)$(intel-blobs),) >> +obj-y += ucode_dummy.o >> +endif >> + >> +ucode_amd.o: Makefile $(amd-blobs) >> + 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 $@ >> + rm -f $@.bin >> + >> +ucode_intel.o: Makefile $(intel-blobs) >> + 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 $@ >> + rm -f $@.bin > > This can be had with a pattern rule (with the vendor being the stem) > and hence without duplication, I think. > > Also - is simply concatenating the blobs reliable enough? There's no > build time diagnostic that the result would actually be understood > at runtime. > Concatenation is reliable (as long as the individual microcode blobs are not malformed, and in that case the builtin is not making matters worse compared to presenting the malformed update via <integer> | scan). >> +ucode_dummy.o: Makefile >> + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; > > Since the commit message doesn't explain why this is needed, I > have to ask (I guess we somewhere have a dependency on $(obj-y) > not being empty). Your guess is correct. All sub-directories of xen/arch/x86 are expected to produce built_in.o. If there are not amd nor intel microcode blobs, there will be no build dependencies and the build fails preparing the built_in.o > _If_ it is needed, I don't see why you need > ifeq() around its use. In fact you could have > > obj-y := ucode-dummy.o > > right at the top of the file. > > Furthermore I don't really understand why you need this in the > first place. While cat won't do what you want with an empty > argument list, can't you simply prepend / append /dev/null? > To make sure we are on the same page. You are suggesting using "/dev/null" in case there are no amd/intel ucode to generate the ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as input (complains about empty binary). (I agree with your other inline suggestions that I have omitted. Those I will address in v3). Thanks, Eslam
On 19.12.2019 23:11, Eslam Elnikety wrote: > On 18.12.19 13:42, Jan Beulich wrote: >> On 18.12.2019 02:32, Eslam Elnikety wrote: >>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> + >>> +Xen can bundle microcode updates within its image. This support is conditional >>> +on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is >>> +useful to ensure that, by default, a minimum microcode patch level will be >>> +applied to the underlying CPU. >>> + >>> +To use microcode updates available on the build system as builtin, >>> +use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates >>> +and specify the individual microcode patches via either BUILTIN_UCODE_AMD or >>> +BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For >>> +instance, the configuration below is suitable for a build system which has a >>> +``/lib/firmware/`` directory which, in turn, includes the individual microcode >>> +patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and >>> +``intel-ucode/06-2f-02``. >>> + >>> + CONFIG_BUILTIN_UCODE=y >>> + CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/" >>> + CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin" >>> + CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02" >> >> Rather than a blank as separator, the more conventional one on >> Unix and alike would be : I think. Of course ideally there wouldn't >> be any restriction at all on the characters usable here for file >> names. >> > > It would be great if there is a particular convention. The blank > separator is aligned with Linux way of doing builtin microcode. Well, this is then another area where I would question whether we really want to follow the Linux approach, but I'm not bothered enough to make less non-conventional behavior here a requirement. >>> --- a/xen/arch/x86/Kconfig >>> +++ b/xen/arch/x86/Kconfig >>> @@ -218,6 +218,36 @@ config MEM_SHARING >>> bool "Xen memory sharing support" if EXPERT = "y" >>> depends on HVM >>> >>> +config BUILTIN_UCODE >>> + bool "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. >> >> I continue to be unconvinced that this separate option is needed. >> Albeit compared to the v1 approach I will agree that handling >> would become more complicated without. > > Any particular preference between the v1 vs v2 approach? I definitely like the vendor separation. >>> @@ -701,7 +747,13 @@ static int __init microcode_init(void) >>> */ >>> if ( ucode_blob.size ) >>> { >>> +#ifdef CONFIG_BUILTIN_UCODE >>> + /* No need to destroy module mappings if builtin was used */ >>> + if ( !ucode_builtin ) >>> + bootstrap_map(NULL); >>> +#else >>> bootstrap_map(NULL); >>> +#endif >> >> First of all - is there no ucode unrelated side effect of this >> invocation? I.e. can it safely be skipped? > > Maybe I am missing something. Are you asking if we can safely skip the > bootstrap_map(NULL)? (Quoting your response on PATCH v2 2/4 "And of > course we really want these mappings to be gone") Yes - my point is that invoking the function here may in principle cover for other mappings. However - this is the invocation you've added in an earlier patch, isn't it? In which case omitting it should be fine. Nevertheless I don't see and harm in invoking the function, i.e. I'd rather keep the code here simple. >>> --- /dev/null >>> +++ b/xen/arch/x86/microcode/Makefile >>> @@ -0,0 +1,46 @@ >>> +# 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. >>> + >>> +# Remove quotes and excess spaces from configuration strings >>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR))) >>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD))) >>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL))) >>> + >>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs. >>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD))) >>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL))) >>> + >>> +ifneq ($(amd-blobs),) >>> +obj-y += ucode_amd.o >>> +endif >>> + >>> +ifneq ($(intel-blobs),) >>> +obj-y += ucode_intel.o >>> +endif >>> + >>> +ifeq ($(amd-blobs)$(intel-blobs),) >>> +obj-y += ucode_dummy.o >>> +endif >>> + >>> +ucode_amd.o: Makefile $(amd-blobs) >>> + 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 $@ >>> + rm -f $@.bin >>> + >>> +ucode_intel.o: Makefile $(intel-blobs) >>> + 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 $@ >>> + rm -f $@.bin >> >> This can be had with a pattern rule (with the vendor being the stem) >> and hence without duplication, I think. >> >> Also - is simply concatenating the blobs reliable enough? There's no >> build time diagnostic that the result would actually be understood >> at runtime. >> > > Concatenation is reliable (as long as the individual microcode blobs are > not malformed, and in that case the builtin is not making matters worse > compared to presenting the malformed update via <integer> | scan). A malformed update found the other way is a bug in the tools constructing the respective images. A malformed built-in update is a bug in the Xen build system. The put the question differently: Is it specified somewhere that the blobs all have to have certain properties, which the straight concatenation relies upon? >>> +ucode_dummy.o: Makefile >>> + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; >> >> Since the commit message doesn't explain why this is needed, I >> have to ask (I guess we somewhere have a dependency on $(obj-y) >> not being empty). > > Your guess is correct. All sub-directories of xen/arch/x86 are expected > to produce built_in.o. If there are not amd nor intel microcode blobs, > there will be no build dependencies and the build fails preparing the > built_in.o That's rather poor, but it's of course not your task to get this fixed (it shouldn't be very difficult to create an empty built_in.o for an empty $(obj-y)). >> _If_ it is needed, I don't see why you need >> ifeq() around its use. In fact you could have >> >> obj-y := ucode-dummy.o >> >> right at the top of the file. >> >> Furthermore I don't really understand why you need this in the >> first place. While cat won't do what you want with an empty >> argument list, can't you simply prepend / append /dev/null? >> > > To make sure we are on the same page. You are suggesting using > "/dev/null" in case there are no amd/intel ucode to generate the > ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as > input (complains about empty binary). That's again rather poor, this time of the utility - it should be easy enough to produce an object with an empty .data (or whatever it is) section. As above - I'm fine with you keeping the logic then as is, provided you say in the description why it can't be simplified. Jan
On 20.12.19 11:12, Jan Beulich wrote: > On 19.12.2019 23:11, Eslam Elnikety wrote: >> On 18.12.19 13:42, Jan Beulich wrote: >>> On 18.12.2019 02:32, Eslam Elnikety wrote: >>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>> + >>>> +Xen can bundle microcode updates within its image. This support is conditional >>>> +on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is >>>> +useful to ensure that, by default, a minimum microcode patch level will be >>>> +applied to the underlying CPU. >>>> + >>>> +To use microcode updates available on the build system as builtin, >>>> +use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates >>>> +and specify the individual microcode patches via either BUILTIN_UCODE_AMD or >>>> +BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For >>>> +instance, the configuration below is suitable for a build system which has a >>>> +``/lib/firmware/`` directory which, in turn, includes the individual microcode >>>> +patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and >>>> +``intel-ucode/06-2f-02``. >>>> + >>>> + CONFIG_BUILTIN_UCODE=y >>>> + CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/" >>>> + CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin" >>>> + CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02" >>> >>> Rather than a blank as separator, the more conventional one on >>> Unix and alike would be : I think. Of course ideally there wouldn't >>> be any restriction at all on the characters usable here for file >>> names. >>> >> >> It would be great if there is a particular convention. The blank >> separator is aligned with Linux way of doing builtin microcode. > > Well, this is then another area where I would question whether we > really want to follow the Linux approach, but I'm not bothered > enough to make less non-conventional behavior here a requirement. > >>>> --- a/xen/arch/x86/Kconfig >>>> +++ b/xen/arch/x86/Kconfig >>>> @@ -218,6 +218,36 @@ config MEM_SHARING >>>> bool "Xen memory sharing support" if EXPERT = "y" >>>> depends on HVM >>>> >>>> +config BUILTIN_UCODE >>>> + bool "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. >>> >>> I continue to be unconvinced that this separate option is needed. >>> Albeit compared to the v1 approach I will agree that handling >>> would become more complicated without. >> >> Any particular preference between the v1 vs v2 approach? > > I definitely like the vendor separation. > >>>> @@ -701,7 +747,13 @@ static int __init microcode_init(void) >>>> */ >>>> if ( ucode_blob.size ) >>>> { >>>> +#ifdef CONFIG_BUILTIN_UCODE >>>> + /* No need to destroy module mappings if builtin was used */ >>>> + if ( !ucode_builtin ) >>>> + bootstrap_map(NULL); >>>> +#else >>>> bootstrap_map(NULL); >>>> +#endif >>> >>> First of all - is there no ucode unrelated side effect of this >>> invocation? I.e. can it safely be skipped? >> >> Maybe I am missing something. Are you asking if we can safely skip the >> bootstrap_map(NULL)? (Quoting your response on PATCH v2 2/4 "And of >> course we really want these mappings to be gone") > > Yes - my point is that invoking the function here may in > principle cover for other mappings. However - this is the > invocation you've added in an earlier patch, isn't it? In > which case omitting it should be fine. Nevertheless I don't > see and harm in invoking the function, i.e. I'd rather keep > the code here simple. > >>>> --- /dev/null >>>> +++ b/xen/arch/x86/microcode/Makefile >>>> @@ -0,0 +1,46 @@ >>>> +# 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. >>>> + >>>> +# Remove quotes and excess spaces from configuration strings >>>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR))) >>>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD))) >>>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL))) >>>> + >>>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs. >>>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD))) >>>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL))) >>>> + >>>> +ifneq ($(amd-blobs),) >>>> +obj-y += ucode_amd.o >>>> +endif >>>> + >>>> +ifneq ($(intel-blobs),) >>>> +obj-y += ucode_intel.o >>>> +endif >>>> + >>>> +ifeq ($(amd-blobs)$(intel-blobs),) >>>> +obj-y += ucode_dummy.o >>>> +endif >>>> + >>>> +ucode_amd.o: Makefile $(amd-blobs) >>>> + 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 $@ >>>> + rm -f $@.bin >>>> + >>>> +ucode_intel.o: Makefile $(intel-blobs) >>>> + 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 $@ >>>> + rm -f $@.bin >>> >>> This can be had with a pattern rule (with the vendor being the stem) >>> and hence without duplication, I think. >>> >>> Also - is simply concatenating the blobs reliable enough? There's no >>> build time diagnostic that the result would actually be understood >>> at runtime. >>> >> >> Concatenation is reliable (as long as the individual microcode blobs are >> not malformed, and in that case the builtin is not making matters worse >> compared to presenting the malformed update via <integer> | scan). > > A malformed update found the other way is a bug in the tools > constructing the respective images. A malformed built-in > update is a bug in the Xen build system. The put the question > differently: Is it specified somewhere that the blobs all have > to have certain properties, which the straight concatenation > relies upon? > >>>> +ucode_dummy.o: Makefile >>>> + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; >>> >>> Since the commit message doesn't explain why this is needed, I >>> have to ask (I guess we somewhere have a dependency on $(obj-y) >>> not being empty). >> >> Your guess is correct. All sub-directories of xen/arch/x86 are expected >> to produce built_in.o. If there are not amd nor intel microcode blobs, >> there will be no build dependencies and the build fails preparing the >> built_in.o > > That's rather poor, but it's of course not your task to get this > fixed (it shouldn't be very difficult to create an empty > built_in.o for an empty $(obj-y)). > >>> _If_ it is needed, I don't see why you need >>> ifeq() around its use. In fact you could have >>> >>> obj-y := ucode-dummy.o >>> >>> right at the top of the file. >>> >>> Furthermore I don't really understand why you need this in the >>> first place. While cat won't do what you want with an empty >>> argument list, can't you simply prepend / append /dev/null? >>> >> >> To make sure we are on the same page. You are suggesting using >> "/dev/null" in case there are no amd/intel ucode to generate the >> ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as >> input (complains about empty binary). > > That's again rather poor, this time of the utility - it should be > easy enough to produce an object with an empty .data (or whatever > it is) section. As above - I'm fine with you keeping the logic > then as is, provided you say in the description why it can't be > simplified. What about using the attached patch for including the binary files? I wanted to post that for my hypervisor-fs series, but I think it would fit here quite nice. Juergen
On 20.12.2019 11:34, Jürgen Groß wrote: > On 20.12.19 11:12, Jan Beulich wrote: >> On 19.12.2019 23:11, Eslam Elnikety wrote: >>> On 18.12.19 13:42, Jan Beulich wrote: >>>> On 18.12.2019 02:32, Eslam Elnikety wrote: >>>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> + >>>>> +Xen can bundle microcode updates within its image. This support is conditional >>>>> +on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is >>>>> +useful to ensure that, by default, a minimum microcode patch level will be >>>>> +applied to the underlying CPU. >>>>> + >>>>> +To use microcode updates available on the build system as builtin, >>>>> +use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates >>>>> +and specify the individual microcode patches via either BUILTIN_UCODE_AMD or >>>>> +BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For >>>>> +instance, the configuration below is suitable for a build system which has a >>>>> +``/lib/firmware/`` directory which, in turn, includes the individual microcode >>>>> +patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and >>>>> +``intel-ucode/06-2f-02``. >>>>> + >>>>> + CONFIG_BUILTIN_UCODE=y >>>>> + CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/" >>>>> + CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin" >>>>> + CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02" >>>> >>>> Rather than a blank as separator, the more conventional one on >>>> Unix and alike would be : I think. Of course ideally there wouldn't >>>> be any restriction at all on the characters usable here for file >>>> names. >>>> >>> >>> It would be great if there is a particular convention. The blank >>> separator is aligned with Linux way of doing builtin microcode. >> >> Well, this is then another area where I would question whether we >> really want to follow the Linux approach, but I'm not bothered >> enough to make less non-conventional behavior here a requirement. >> >>>>> --- a/xen/arch/x86/Kconfig >>>>> +++ b/xen/arch/x86/Kconfig >>>>> @@ -218,6 +218,36 @@ config MEM_SHARING >>>>> bool "Xen memory sharing support" if EXPERT = "y" >>>>> depends on HVM >>>>> >>>>> +config BUILTIN_UCODE >>>>> + bool "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. >>>> >>>> I continue to be unconvinced that this separate option is needed. >>>> Albeit compared to the v1 approach I will agree that handling >>>> would become more complicated without. >>> >>> Any particular preference between the v1 vs v2 approach? >> >> I definitely like the vendor separation. >> >>>>> @@ -701,7 +747,13 @@ static int __init microcode_init(void) >>>>> */ >>>>> if ( ucode_blob.size ) >>>>> { >>>>> +#ifdef CONFIG_BUILTIN_UCODE >>>>> + /* No need to destroy module mappings if builtin was used */ >>>>> + if ( !ucode_builtin ) >>>>> + bootstrap_map(NULL); >>>>> +#else >>>>> bootstrap_map(NULL); >>>>> +#endif >>>> >>>> First of all - is there no ucode unrelated side effect of this >>>> invocation? I.e. can it safely be skipped? >>> >>> Maybe I am missing something. Are you asking if we can safely skip the >>> bootstrap_map(NULL)? (Quoting your response on PATCH v2 2/4 "And of >>> course we really want these mappings to be gone") >> >> Yes - my point is that invoking the function here may in >> principle cover for other mappings. However - this is the >> invocation you've added in an earlier patch, isn't it? In >> which case omitting it should be fine. Nevertheless I don't >> see and harm in invoking the function, i.e. I'd rather keep >> the code here simple. >> >>>>> --- /dev/null >>>>> +++ b/xen/arch/x86/microcode/Makefile >>>>> @@ -0,0 +1,46 @@ >>>>> +# 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. >>>>> + >>>>> +# Remove quotes and excess spaces from configuration strings >>>>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR))) >>>>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD))) >>>>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL))) >>>>> + >>>>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs. >>>>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD))) >>>>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL))) >>>>> + >>>>> +ifneq ($(amd-blobs),) >>>>> +obj-y += ucode_amd.o >>>>> +endif >>>>> + >>>>> +ifneq ($(intel-blobs),) >>>>> +obj-y += ucode_intel.o >>>>> +endif >>>>> + >>>>> +ifeq ($(amd-blobs)$(intel-blobs),) >>>>> +obj-y += ucode_dummy.o >>>>> +endif >>>>> + >>>>> +ucode_amd.o: Makefile $(amd-blobs) >>>>> + 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 $@ >>>>> + rm -f $@.bin >>>>> + >>>>> +ucode_intel.o: Makefile $(intel-blobs) >>>>> + 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 $@ >>>>> + rm -f $@.bin >>>> >>>> This can be had with a pattern rule (with the vendor being the stem) >>>> and hence without duplication, I think. >>>> >>>> Also - is simply concatenating the blobs reliable enough? There's no >>>> build time diagnostic that the result would actually be understood >>>> at runtime. >>>> >>> >>> Concatenation is reliable (as long as the individual microcode blobs are >>> not malformed, and in that case the builtin is not making matters worse >>> compared to presenting the malformed update via <integer> | scan). >> >> A malformed update found the other way is a bug in the tools >> constructing the respective images. A malformed built-in >> update is a bug in the Xen build system. The put the question >> differently: Is it specified somewhere that the blobs all have >> to have certain properties, which the straight concatenation >> relies upon? >> >>>>> +ucode_dummy.o: Makefile >>>>> + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; >>>> >>>> Since the commit message doesn't explain why this is needed, I >>>> have to ask (I guess we somewhere have a dependency on $(obj-y) >>>> not being empty). >>> >>> Your guess is correct. All sub-directories of xen/arch/x86 are expected >>> to produce built_in.o. If there are not amd nor intel microcode blobs, >>> there will be no build dependencies and the build fails preparing the >>> built_in.o >> >> That's rather poor, but it's of course not your task to get this >> fixed (it shouldn't be very difficult to create an empty >> built_in.o for an empty $(obj-y)). >> >>>> _If_ it is needed, I don't see why you need >>>> ifeq() around its use. In fact you could have >>>> >>>> obj-y := ucode-dummy.o >>>> >>>> right at the top of the file. >>>> >>>> Furthermore I don't really understand why you need this in the >>>> first place. While cat won't do what you want with an empty >>>> argument list, can't you simply prepend / append /dev/null? >>>> >>> >>> To make sure we are on the same page. You are suggesting using >>> "/dev/null" in case there are no amd/intel ucode to generate the >>> ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as >>> input (complains about empty binary). >> >> That's again rather poor, this time of the utility - it should be >> easy enough to produce an object with an empty .data (or whatever >> it is) section. As above - I'm fine with you keeping the logic >> then as is, provided you say in the description why it can't be >> simplified. > > What about using the attached patch for including the binary files? > > I wanted to post that for my hypervisor-fs series, but I think it would > fit here quite nice. Why not, if it can be made fit the ucode situation. Jan
On 20.12.19 11:12, Jan Beulich wrote: > On 19.12.2019 23:11, Eslam Elnikety wrote: >> On 18.12.19 13:42, Jan Beulich wrote: >>> On 18.12.2019 02:32, Eslam Elnikety wrote: >>>> --- /dev/null >>>> +++ b/xen/arch/x86/microcode/Makefile >>>> @@ -0,0 +1,46 @@ >>>> +# 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. >>>> + >>>> +# Remove quotes and excess spaces from configuration strings >>>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR))) >>>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD))) >>>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL))) >>>> + >>>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs. >>>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD))) >>>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL))) >>>> + >>>> +ifneq ($(amd-blobs),) >>>> +obj-y += ucode_amd.o >>>> +endif >>>> + >>>> +ifneq ($(intel-blobs),) >>>> +obj-y += ucode_intel.o >>>> +endif >>>> + >>>> +ifeq ($(amd-blobs)$(intel-blobs),) >>>> +obj-y += ucode_dummy.o >>>> +endif >>>> + >>>> +ucode_amd.o: Makefile $(amd-blobs) >>>> + 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 $@ >>>> + rm -f $@.bin >>>> + >>>> +ucode_intel.o: Makefile $(intel-blobs) >>>> + 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 $@ >>>> + rm -f $@.bin >>> >>> This can be had with a pattern rule (with the vendor being the stem) >>> and hence without duplication, I think. >>> >>> Also - is simply concatenating the blobs reliable enough? There's no >>> build time diagnostic that the result would actually be understood >>> at runtime. >>> >> >> Concatenation is reliable (as long as the individual microcode blobs are >> not malformed, and in that case the builtin is not making matters worse >> compared to presenting the malformed update via <integer> | scan). > > A malformed update found the other way is a bug in the tools > constructing the respective images. A malformed built-in > update is a bug in the Xen build system. The put the question > differently: Is it specified somewhere that the blobs all have > to have certain properties, which the straight concatenation > relies upon? > Refer to ( https://www.kernel.org/doc/Documentation/x86/microcode.txt ). AuthenticAMD.bin and GenuineIntel.bin are both concatenations of individual microcode blobs. >>>> +ucode_dummy.o: Makefile >>>> + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; >>> >>> Since the commit message doesn't explain why this is needed, I >>> have to ask (I guess we somewhere have a dependency on $(obj-y) >>> not being empty). >> >> Your guess is correct. All sub-directories of xen/arch/x86 are expected >> to produce built_in.o. If there are not amd nor intel microcode blobs, >> there will be no build dependencies and the build fails preparing the >> built_in.o > > That's rather poor, but it's of course not your task to get this > fixed (it shouldn't be very difficult to create an empty > built_in.o for an empty $(obj-y)). > >>> _If_ it is needed, I don't see why you need >>> ifeq() around its use. In fact you could have >>> >>> obj-y := ucode-dummy.o >>> >>> right at the top of the file. >>> >>> Furthermore I don't really understand why you need this in the >>> first place. While cat won't do what you want with an empty >>> argument list, can't you simply prepend / append /dev/null? >>> >> >> To make sure we are on the same page. You are suggesting using >> "/dev/null" in case there are no amd/intel ucode to generate the >> ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as >> input (complains about empty binary). > > That's again rather poor, this time of the utility - it should be > easy enough to produce an object with an empty .data (or whatever > it is) section. As above - I'm fine with you keeping the logic > then as is, provided you say in the description why it can't be > simplified. Ack. Will justify the logic in comments. -- Eslam > > Jan >
On 20.12.19 11:34, Jürgen Groß wrote: > On 20.12.19 11:12, Jan Beulich wrote: >> On 19.12.2019 23:11, Eslam Elnikety wrote: >>> On 18.12.19 13:42, Jan Beulich wrote: >>>> On 18.12.2019 02:32, Eslam Elnikety wrote: >>>>> --- /dev/null >>>>> +++ b/xen/arch/x86/microcode/Makefile >>>>> @@ -0,0 +1,46 @@ >>>>> +# 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. >>>>> + >>>>> +# Remove quotes and excess spaces from configuration strings >>>>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR))) >>>>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD))) >>>>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL))) >>>>> + >>>>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for >>>>> existing blobs. >>>>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD))) >>>>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL))) >>>>> + >>>>> +ifneq ($(amd-blobs),) >>>>> +obj-y += ucode_amd.o >>>>> +endif >>>>> + >>>>> +ifneq ($(intel-blobs),) >>>>> +obj-y += ucode_intel.o >>>>> +endif >>>>> + >>>>> +ifeq ($(amd-blobs)$(intel-blobs),) >>>>> +obj-y += ucode_dummy.o >>>>> +endif >>>>> + >>>>> +ucode_amd.o: Makefile $(amd-blobs) >>>>> + 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 $@ >>>>> + rm -f $@.bin >>>>> + >>>>> +ucode_intel.o: Makefile $(intel-blobs) >>>>> + 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 $@ >>>>> + rm -f $@.bin >>>> >>>> This can be had with a pattern rule (with the vendor being the stem) >>>> and hence without duplication, I think. >>>> >>>> Also - is simply concatenating the blobs reliable enough? There's no >>>> build time diagnostic that the result would actually be understood >>>> at runtime. >>>> >>> >>> Concatenation is reliable (as long as the individual microcode blobs are >>> not malformed, and in that case the builtin is not making matters worse >>> compared to presenting the malformed update via <integer> | scan). >> >> A malformed update found the other way is a bug in the tools >> constructing the respective images. A malformed built-in >> update is a bug in the Xen build system. The put the question >> differently: Is it specified somewhere that the blobs all have >> to have certain properties, which the straight concatenation >> relies upon? >> >>>>> +ucode_dummy.o: Makefile >>>>> + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; >>>> >>>> Since the commit message doesn't explain why this is needed, I >>>> have to ask (I guess we somewhere have a dependency on $(obj-y) >>>> not being empty). >>> >>> Your guess is correct. All sub-directories of xen/arch/x86 are expected >>> to produce built_in.o. If there are not amd nor intel microcode blobs, >>> there will be no build dependencies and the build fails preparing the >>> built_in.o >> >> That's rather poor, but it's of course not your task to get this >> fixed (it shouldn't be very difficult to create an empty >> built_in.o for an empty $(obj-y)). >> >>>> _If_ it is needed, I don't see why you need >>>> ifeq() around its use. In fact you could have >>>> >>>> obj-y := ucode-dummy.o >>>> >>>> right at the top of the file. >>>> >>>> Furthermore I don't really understand why you need this in the >>>> first place. While cat won't do what you want with an empty >>>> argument list, can't you simply prepend / append /dev/null? >>>> >>> >>> To make sure we are on the same page. You are suggesting using >>> "/dev/null" in case there are no amd/intel ucode to generate the >>> ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as >>> input (complains about empty binary). >> >> That's again rather poor, this time of the utility - it should be >> easy enough to produce an object with an empty .data (or whatever >> it is) section. As above - I'm fine with you keeping the logic >> then as is, provided you say in the description why it can't be >> simplified. > > What about using the attached patch for including the binary files? > > I wanted to post that for my hypervisor-fs series, but I think it would > fit here quite nice. Thanks, Jürgen. That tool is indeed useful on its own right for flask/policies. However, it seems to me that creating a built_in.o right out of the microcode blobs is simpler and keeps the whole business contained within xen/arch/x86/microcode/. -- Eslam > > > Juergen
On 17.01.2020 21:06, Eslam Elnikety wrote: > On 20.12.19 11:12, Jan Beulich wrote: >> On 19.12.2019 23:11, Eslam Elnikety wrote: >>> On 18.12.19 13:42, Jan Beulich wrote: >>>> On 18.12.2019 02:32, Eslam Elnikety wrote: >>>>> --- /dev/null >>>>> +++ b/xen/arch/x86/microcode/Makefile >>>>> @@ -0,0 +1,46 @@ >>>>> +# 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. >>>>> + >>>>> +# Remove quotes and excess spaces from configuration strings >>>>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR))) >>>>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD))) >>>>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL))) >>>>> + >>>>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs. >>>>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD))) >>>>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL))) >>>>> + >>>>> +ifneq ($(amd-blobs),) >>>>> +obj-y += ucode_amd.o >>>>> +endif >>>>> + >>>>> +ifneq ($(intel-blobs),) >>>>> +obj-y += ucode_intel.o >>>>> +endif >>>>> + >>>>> +ifeq ($(amd-blobs)$(intel-blobs),) >>>>> +obj-y += ucode_dummy.o >>>>> +endif >>>>> + >>>>> +ucode_amd.o: Makefile $(amd-blobs) >>>>> + 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 $@ >>>>> + rm -f $@.bin >>>>> + >>>>> +ucode_intel.o: Makefile $(intel-blobs) >>>>> + 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 $@ >>>>> + rm -f $@.bin >>>> >>>> This can be had with a pattern rule (with the vendor being the stem) >>>> and hence without duplication, I think. >>>> >>>> Also - is simply concatenating the blobs reliable enough? There's no >>>> build time diagnostic that the result would actually be understood >>>> at runtime. >>>> >>> >>> Concatenation is reliable (as long as the individual microcode blobs are >>> not malformed, and in that case the builtin is not making matters worse >>> compared to presenting the malformed update via <integer> | scan). >> >> A malformed update found the other way is a bug in the tools >> constructing the respective images. A malformed built-in >> update is a bug in the Xen build system. The put the question >> differently: Is it specified somewhere that the blobs all have >> to have certain properties, which the straight concatenation >> relies upon? >> > > Refer to ( https://www.kernel.org/doc/Documentation/x86/microcode.txt ). > AuthenticAMD.bin and GenuineIntel.bin are both concatenations of > individual microcode blobs. Well, yes, and from practical observations this is good enough. But observe e.g. how that paragraph starts with "Here's a crude example ...". Jan
diff --git a/docs/admin-guide/microcode-loading.rst b/docs/admin-guide/microcode-loading.rst index e83cadd2c2..989e8d446b 100644 --- a/docs/admin-guide/microcode-loading.rst +++ b/docs/admin-guide/microcode-loading.rst @@ -104,6 +104,37 @@ The ``ucode=scan`` command line option will cause Xen to search through all modules to find any CPIO archives, and search the archive for the applicable file. Xen will stop searching at the first match. +Loading microcode built within the Xen image +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Xen can bundle microcode updates within its image. This support is conditional +on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is +useful to ensure that, by default, a minimum microcode patch level will be +applied to the underlying CPU. + +To use microcode updates available on the build system as builtin, +use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates +and specify the individual microcode patches via either BUILTIN_UCODE_AMD or +BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For +instance, the configuration below is suitable for a build system which has a +``/lib/firmware/`` directory which, in turn, includes the individual microcode +patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and +``intel-ucode/06-2f-02``. + + CONFIG_BUILTIN_UCODE=y + CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/" + CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin" + CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02" + +Alternatively, CONFIG_BUILTIN_UCODE_{AMD,INTEL} can directly point to the +concatenation of the individual microcode blobs. For instance, assuming that +``amd-ucode/AuthenticAMD.bin`` and ``intel-ucode/GenuineIntel.bin`` hold +multiple microcode updates for AMD and INTEL, respectively, you may use the +configuration below. + + CONFIG_BUILTIN_UCODE_AMD="amd-ucode/AuthenticAMD.bin" + CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/GenuineIntel.bin" + Run time microcode loading -------------------------- diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 40faf3bc3a..9cfc2df05a 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2126,10 +2126,10 @@ logic applies: active by default. ### ucode (x86) -> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]` +> `= List of [ <integer> | scan=<bool>, builtin=<bool>, nmi=<bool> ]` Applicability: x86 - Default: `nmi` + Default: `nmi` if BUILTIN_UCODE is not enabled, `builtin,nmi` otherwise Controls for CPU microcode loading. For early loading, this parameter can specify how and where to find the microcode update blob. For late loading, @@ -2150,6 +2150,12 @@ microcode in the cpio name space must be: - on Intel: kernel/x86/microcode/GenuineIntel.bin - on AMD : kernel/x86/microcode/AuthenticAMD.bin +'builtin' instructs the hypervisor to use the builtin microcode update. This +option is available only if option BUILTIN_UCODE is enabled at build. The +default value is `true`. If a microcode is provided via other options (such +as 'integer', 'scan', or `ucode=<filename>` config when booting via EFI), +the provided microcode takes precedence over the builtin one. + 'nmi' determines late loading is performed in NMI handler or just in stop_machine context. In NMI handler, even NMIs are blocked, which is considered safer. The default value is `true`. diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 02bb05f42e..9bc220925b 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -218,6 +218,36 @@ config MEM_SHARING bool "Xen memory sharing support" if EXPERT = "y" depends on HVM +config BUILTIN_UCODE + bool "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 "Directory containing microcode updates" + default "/lib/firmware/" + depends on BUILTIN_UCODE + ---help--- + The directory containing the microcode blobs. + +config BUILTIN_UCODE_AMD + string "AMD microcode updates" + default "" + depends on BUILTIN_UCODE + ---help--- + AMD builtin microcode; space-sparated, relative to BUILTIN_UCODE_DIR. + +config BUILTIN_UCODE_INTEL + string "INTEL microcode updates" + default "" + depends on BUILTIN_UCODE + ---help--- + INTEL builtin microcode; space-sparated, relative to BUILTIN_UCODE_DIR. + endmenu source "common/Kconfig" diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 7da5a2631e..886691a377 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -3,6 +3,7 @@ subdir-y += cpu subdir-y += genapic subdir-$(CONFIG_GUEST) += guest subdir-$(CONFIG_HVM) += hvm +subdir-$(CONFIG_BUILTIN_UCODE) += microcode subdir-y += mm subdir-$(CONFIG_XENOPROF) += oprofile subdir-$(CONFIG_PV) += pv diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c index 4616fa9d2e..bcfbd31041 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 __initdata ucode_builtin = 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; @@ -122,6 +130,10 @@ static int __init parse_ucode_param(const char *s) ucode_in_nmi = val; else 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; @@ -208,6 +220,40 @@ void __init microcode_grab_module( ucode_mod = mod[ucode_mod_idx]; else 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 ) + { + ucode_builtin = false; + return; + } + + /* Set ucode_start/_end to the proper blob */ + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) + { + ucode_blob.size = __builtin_amd_ucode_end - __builtin_amd_ucode_start; + ucode_blob.data = __builtin_amd_ucode_start; + } + else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) + { + ucode_blob.size = __builtin_intel_ucode_end - + __builtin_intel_ucode_start; + ucode_blob.data = __builtin_intel_ucode_start; + } + else + return; + + if ( !ucode_blob.size ) + { + printk("No builtin ucode for the CPU vendor.\n"); + ucode_blob.data = NULL; + } +#endif } const struct microcode_ops *microcode_ops; @@ -701,7 +747,13 @@ static int __init microcode_init(void) */ if ( ucode_blob.size ) { +#ifdef CONFIG_BUILTIN_UCODE + /* No need to destroy module mappings if builtin was used */ + if ( !ucode_builtin ) + bootstrap_map(NULL); +#else bootstrap_map(NULL); +#endif ucode_blob.size = 0; ucode_blob.data = NULL; } diff --git a/xen/arch/x86/microcode/Makefile b/xen/arch/x86/microcode/Makefile new file mode 100644 index 0000000000..c34d99903a --- /dev/null +++ b/xen/arch/x86/microcode/Makefile @@ -0,0 +1,46 @@ +# 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. + +# Remove quotes and excess spaces from configuration strings +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR))) +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD))) +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL))) + +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs. +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD))) +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL))) + +ifneq ($(amd-blobs),) +obj-y += ucode_amd.o +endif + +ifneq ($(intel-blobs),) +obj-y += ucode_intel.o +endif + +ifeq ($(amd-blobs)$(intel-blobs),) +obj-y += ucode_dummy.o +endif + +ucode_amd.o: Makefile $(amd-blobs) + 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 $@ + rm -f $@.bin + +ucode_intel.o: Makefile $(intel-blobs) + 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 $@ + rm -f $@.bin + +ucode_dummy.o: Makefile + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; 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 will, by default, attempt the microcode update. Disabling the builtin microcode update can be done via the Xen command line parameter 'ucode=no-builtin'. Moreover, the microcode provided via other options (such as 'ucode=<integer>|scan' or 'ucode=<filename>' config when booting via EFI) takes precedence over the builtin one. Signed-off-by: Eslam Elnikety <elnikety@amazon.com> --- Changes in v2: - Allow for ucode=<integer>|scan,{no-}builtin and detail the model. Reflect those changes onto microcode.c and docs/misc/xen-command-line.pandoc - Add documentation to the existing docs/admin-guide/microcode-loading.rst - Build on Patches 1--3 to avoid xmalloc/memcpy for the builtin microcode - Work configuration in order to specify the individual microcode blobs to use for the builtin microcode, and rework the microcode/Makefile accordingly --- docs/admin-guide/microcode-loading.rst | 31 +++++++++++++++ docs/misc/xen-command-line.pandoc | 10 ++++- xen/arch/x86/Kconfig | 30 +++++++++++++++ xen/arch/x86/Makefile | 1 + xen/arch/x86/microcode.c | 52 ++++++++++++++++++++++++++ xen/arch/x86/microcode/Makefile | 46 +++++++++++++++++++++++ xen/arch/x86/xen.lds.S | 12 ++++++ 7 files changed, 180 insertions(+), 2 deletions(-) create mode 100644 xen/arch/x86/microcode/Makefile