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

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

On 09.12.19 16:19, Andrew Cooper wrote:
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

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.


While the existing docs/admin-guide/microcode-loading.rst speaks a different tone than what I added, that documentation anyway needs to be updated with builtin if such support were to be added. I will adjust accordingly. If docs/hypervisor-guide/microcode-loading.rst makes it in time for v2 of this patch, I will reflect changes there too.

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;
+/* builtin is the default when BUILTIN_UCODE is set */
+static bool_t __initdata ucode_builtin = 1;

bool, true.

Will fix in v2.

+extern const char __builtin_intel_ucode_start[], __builtin_intel_ucode_end[];
+extern const char __builtin_amd_ucode_start[], __builtin_amd_ucode_end[];
  /* 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.)

Unless you are planning that along your on-going docs/hypervisor-guide/microcode-loading.rst effort, I can pick up this clean-up/prereq patch myself. What do you have in mind? (Or point me to a good example and I will figure things out).

@@ -237,6 +249,48 @@ void __init microcode_grab_module(
      if ( ucode_scan )
          microcode_scan_module(module_map, mbi);
+    /*
+     * 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.

Will fix in v2.

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

I think this would be a welcomed change. It seems to me that we have two ways to go about it.

1) We factor the code in the intel-/amd-specific cpu_request_microcode to extract logic for finding a match into its own new function, expose that through microcode_ops, and finally do xalloc only for the matching microcode when early loading is scan or builtin.

2) Cannot we just do away completely with xalloc? I see that each individual microcode update gets allocated anyway in microcode_intel.c/get_next_ucode_from_buffer() and in microcode_amd.c/cpu_request_microcode(). Unless I am missing something, the xmalloc_bytes for ucode_blob.data is redundant.


+    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);
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@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.
+obj-y += builtin_ucode.o
+# Directory holding the microcode updates.
+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.

We can limit the amd-blobs and intel-blob to binaries following the naming convention AuthenticAMD.bin and GenuineIntel.bin for amd and intel, respectively. Yet, this would be imposing an unnecessary restriction on administrators who may want to be innovative with naming (or want to use the naming microcode_amd_*.bin or FF-MM-SS instead).

Alternatively, we can introduce CONFIG_BUILTIN_UCODE_INTEL and CONFIG_BUILTIN_UCODE_AMD. Both default to empty strings. Then, an administrator can specify exactly the microcodes to include relative to the CONFIG_BUILTIN_UCODE_DIR. For example:


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

Just to make sure we are on the same page. You are after a dummy binary with weak symbols that eventually get overridden when I link the actual microcode binaries into builtin_ucode.o? If so, possible of course. Except that I do not particularly see the downside of the existing approach with dummy builtin_ucode.o.


Thanks a lot for those insightful comments, Andrew.


Xen-devel mailing list



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