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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
> 
> ---
> 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@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
> +# 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel