[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 4/4] x86/microcode: Support builtin CPU microcode

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

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
+       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 )
+        /* No need to destroy module mappings if builtin was used */
+        if ( !ucode_builtin )
+            bootstrap_map(NULL);

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

#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@xxxxxxxxxx>
+# 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
+# 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)))
+# 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
+ifneq ($(intel-blobs),)
+obj-y += ucode_intel.o
+ifeq ($(amd-blobs)$(intel-blobs),)
+obj-y += ucode_dummy.o
+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).


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.