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 <elnik...@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
