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

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

On 10.12.19 10:37, Jan Beulich wrote:
On 09.12.2019 09:41, Eslam Elnikety wrote:
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2113,7 +2113,7 @@ logic applies:
     active by default.
### ucode (x86)
-> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
+> `= List of [ <integer> | scan=<bool> | builtin=<bool>, nmi=<bool> ]`

Despite my other question regarding the usefulness of this as a
whole a few comments.

Do "scan" and "builtin" really need to exclude each other? I.e.
don't you mean , instead of | ?
The useful case here would be specifying ucode=scan,builtin which would translate to fallback onto the builtin microcode if a scan fails. In fact, this is already the case given the implementation in v1. It will be better to clarify this semantic by allowing scan,builtin.

On that note, I really see the "<integer>" and "scan=<bool>" to be equal. Following the logic earlier we should probably also allow ucode=<integer>,builtin. This translates to fallback to builtin if there are no modules at index <integer>.

@@ -2128,6 +2128,9 @@ when used with xen.efi (there the concept of modules 
doesn't exist, and
  the blob gets specified via the `ucode=<filename>` config file/section
  entry; see [EFI configuration file description](efi.html)).
+'builtin' instructs the hypervisor to use the builtin microcode update. This
+option is available only if option BUILTIN_UCODE is enabled.

You also want to clarify its default - your reply to Andrew
suggested to me that only the negative form would really be

Indeed. This in any case will need a revamp to rework the ", instead of |". Will address in v2.

--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -218,6 +218,26 @@ config MEM_SHARING
        bool "Xen memory sharing support" if EXPERT = "y"
        depends on HVM
+       def_bool n
+       prompt "Support for Builtin Microcode"

These two lines should be folded into just

        bool "Support for Builtin Microcode"

irrespective of other bad examples you may find in the code base.
The again ...

Will address in v2.

+       ---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.
+       string
+       default "/lib/firmware"
+       depends on BUILTIN_UCODE

... are two separate options needed at all? Can't this latter one
being the empty string just imply the feature to be disabled?

I can go either way here. To me, two options is clearer.

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -7,6 +7,7 @@ subdir-y += mm
  subdir-$(CONFIG_XENOPROF) += oprofile
  subdir-$(CONFIG_PV) += pv
  subdir-y += x86_64
+subdir-$(CONFIG_BUILTIN_UCODE) += microcode

Please respect the (half way?) alphabetical sorting here, unless
adding last is a requirement (in which case a brief comment should
say so, and why).

@@ -130,6 +138,10 @@ static int __init parse_ucode(const char *s)
              if ( (val = parse_boolean("scan", s, ss)) >= 0 )
                  ucode_scan = val;
+           else if ( (val = parse_boolean("builtin", s, ss)) >= 0 )

Please watch out for hard tabs where they don't belong.

Good catch! Will fix in v2.

@@ -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);
+    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 )

With the "return" above please omit the "else" here. But why this
restriction, and ...

Will adjust in v2.

+    {
+        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;
+    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);

... why the copying? Can't you simply point ucode_blob.data at

I am all onboard. See my earlier response to Andrew. I used the same logic that already exists for scan (which assumes that ucode_blob.data is allocated and should be freed when all CPUs are updated).

Thanks for the comments, Jan. On the earlier discussion, please do let me know (and others too) if you are convinced that builtin support for microcode is warranted/useful.



Xen-devel mailing list



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