[Xen-devel] [PATCH v2 1/4] x86/microcode: Improve documentation and parsing for ucode=

[Eslam Elnikety <elnikety@xxxxxxxxxx>]

Decouple the microcode referencing mechanism when using GRUB to that
when using EFI. This allows us to avoid the "unspecified effect" of
using `<integer> | scan` along xen.efi. With that, Xen can explicitly
ignore those named options when using EFI. As an added benefit,
we get a straightfoward parsing of the ucode parameter. While at it,
simplify the logic in microcode_grab_module().

Update the command line documentation for consistency. Also, drop the
leading comment for parse_ucode_param. (No practical use for it given
this commit).

Signed-off-by: Eslam Elnikety <elnikety@xxxxxxxxxx>
---
 docs/misc/xen-command-line.pandoc | 18 ++++++++---
 xen/arch/x86/microcode.c          | 51 ++++++++++++++-----------------
 2 files changed, 36 insertions(+), 33 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc 
b/docs/misc/xen-command-line.pandoc
index 7a1be84ca9..40faf3bc3a 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2128,7 +2128,13 @@ logic applies:
 ### ucode (x86)
 > `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
 
-Specify how and where to find CPU microcode update blob.
+    Applicability: x86
+    Default: `nmi`
+
+Controls for CPU microcode loading. For early loading, this parameter can
+specify how and where to find the microcode update blob. For late loading,
+this parameter specifies if the update happens within a NMI handler or in
+a stop_machine context.
 
 'integer' specifies the CPU microcode update blob module index. When positive,
 this specifies the n-th module (in the GrUB entry, zero based) to be used
@@ -2136,10 +2142,7 @@ for updating CPU micrcode. When negative, counting 
starts at the end of
 the modules in the GrUB entry (so with the blob commonly being last,
 one could specify `ucode=-1`). Note that the value of zero is not valid
 here (entry zero, i.e. the first module, is always the Dom0 kernel
-image). Note further that use of this option has an unspecified effect
-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)).
+image).
 
 'scan' instructs the hypervisor to scan the multiboot images for an cpio
 image that contains microcode. Depending on the platform the blob with the
@@ -2151,6 +2154,11 @@ microcode in the cpio name space must be:
 stop_machine context. In NMI handler, even NMIs are blocked, which is
 considered safer. The default value is `true`.
 
+Note: When booting via EFI, both options 'integer' and 'scan' are ignored.
+Here, the concept of modules does not exist. The microcode update blob for
+early loading gets specified via the `ucode=<filename>` config file/section
+entry; see [EFI configuration file description](efi.html)).
+
 ### unrestricted_guest (Intel)
 > `= <boolean>`
 
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 6ced293d88..8b4d87782c 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -60,7 +60,7 @@
 
 static module_t __initdata ucode_mod;
 static signed int __initdata ucode_mod_idx;
-static bool_t __initdata ucode_mod_forced;
+static signed int __initdata ucode_mod_efi_idx;
 static unsigned int nr_cores;
 
 /*
@@ -105,16 +105,10 @@ static struct microcode_patch *microcode_cache;
 
 void __init microcode_set_module(unsigned int idx)
 {
-    ucode_mod_idx = idx;
-    ucode_mod_forced = 1;
+    ucode_mod_efi_idx = 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.
- */
-static int __init parse_ucode(const char *s)
+static int __init parse_ucode_param(const char *s)
 {
     const char *ss;
     int val, rc = 0;
@@ -126,18 +120,15 @@ static int __init parse_ucode(const char *s)
 
         if ( (val = parse_boolean("nmi", s, ss)) >= 0 )
             ucode_in_nmi = val;
-        else if ( !ucode_mod_forced ) /* Not forced by EFI */
+        else if ( (val = parse_boolean("scan", s, ss)) >= 0 )
+            ucode_scan = val;
+        else
         {
-            if ( (val = parse_boolean("scan", s, ss)) >= 0 )
-                ucode_scan = val;
-            else
-            {
-                const char *q;
-
-                ucode_mod_idx = simple_strtol(s, &q, 0);
-                if ( q != ss )
-                    rc = -EINVAL;
-            }
+            const char *q;
+
+            ucode_mod_idx = simple_strtol(s, &q, 0);
+            if ( q != ss )
+                rc = -EINVAL;
         }
 
         s = ss + 1;
@@ -145,7 +136,7 @@ static int __init parse_ucode(const char *s)
 
     return rc;
 }
-custom_param("ucode", parse_ucode);
+custom_param("ucode", parse_ucode_param);
 
 /*
  * 8MB ought to be enough.
@@ -228,14 +219,18 @@ void __init microcode_grab_module(
 {
     module_t *mod = (module_t *)__va(mbi->mods_addr);
 
-    if ( ucode_mod_idx < 0 )
+    if ( ucode_mod_efi_idx ) /* Microcode specified by EFI */
+    {
+        ucode_mod = mod[ucode_mod_efi_idx];
+        return;
+    }
+
+    if ( ucode_mod_idx < 0 ) /* Count from the end? */
         ucode_mod_idx += mbi->mods_count;
-    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= mbi->mods_count ||
-         !__test_and_clear_bit(ucode_mod_idx, module_map) )
-        goto scan;
-    ucode_mod = mod[ucode_mod_idx];
-scan:
-    if ( ucode_scan )
+    if ( ucode_mod_idx > 0 && ucode_mod_idx < mbi->mods_count &&
+         __test_and_clear_bit(ucode_mod_idx, module_map) )
+        ucode_mod = mod[ucode_mod_idx];
+    else if ( ucode_scan )
         microcode_scan_module(module_map, mbi);
 }
 
-- 
2.17.1


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