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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Eslam Elnikety <elnikety@amazon.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"Paul Durrant" <pdurrant@amazon.co.uk>,
	"Jan Beulich" <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org,
	"David Woodhouse" <dwmw@amazon.co.uk>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH] x86/microcode: Support builtin CPU microcode
Date: Mon, 9 Dec 2019 15:19:27 +0000	[thread overview]
Message-ID: <180013ce-7b18-335a-f04b-1db0d4f2adf4@citrix.com> (raw)
In-Reply-To: <20191209084119.87563-1-elnikety@amazon.com>

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

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