[Xen-devel] [PATCH v2 0/4] x86/microcode: Support builtin CPU microcode

[Xen-devel] [PATCH v2 0/4] x86/microcode: Support builtin CPU microcode

[Eslam Elnikety]

The main goal of this patch series is to add support for builtin microcode.
Towards that end, the series starts with a few improvements for the
documentation and parsing of the ucode= Xen command line parameter that
controls early loading of microcode (Patches 1--3), and follows with the
main builtin suppot (Patch 4).

Changes in v2:
- An earlier version of Patch 4 was submitted in isolation. Refer to the
  patch itself for details regarding the relevant changes.
- Patches 1--3 are additions.

Eslam Elnikety (4):
  x86/microcode: Improve documentation and parsing for ucode=
  x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data
  x86/microcode: use const qualifier for microcode buffer
  x86/microcode: Support builtin CPU microcode

 docs/admin-guide/microcode-loading.rst |  31 ++++++
 docs/misc/xen-command-line.pandoc      |  26 +++--
 xen/arch/x86/Kconfig                   |  30 ++++++
 xen/arch/x86/Makefile                  |   1 +
 xen/arch/x86/microcode.c               | 139 ++++++++++++++-----------
 xen/arch/x86/microcode/Makefile        |  46 ++++++++
 xen/arch/x86/xen.lds.S                 |  12 +++
 7 files changed, 221 insertions(+), 64 deletions(-)
 create mode 100644 xen/arch/x86/microcode/Makefile

-- 
2.17.1


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

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

[Eslam Elnikety]

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@amazon.com>
---
 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 2/4] x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data

[Eslam Elnikety]

When using `ucode=scan` and if a matching module is found, the microcode
payload is maintained in an xmalloc()'d region. This is unnecessary since
the bootmap would just do. Remove the xmalloc and xfree on the microcode
module scan path.

This commit also does away with the restriction on the microcode module
size limit. The concern that a large microcode module would consume too
much memory preventing guests launch is misplaced since this is all the
init path. While having such safeguards is valuable, this should apply
across the board for all early/late microcode loading. Having it just on
the `scan` path is confusing.

Looking forward, we are a bit closer (i.e., one xmalloc down) to pulling
the early microcode loading of the BSP a bit earlier in the early boot
process. This commit is the low hanging fruit. There is still a sizable
amount of work to get there as there are still a handful of xmalloc in
microcode_{amd,intel}.c.

First, there are xmallocs on the path of finding a matching microcode
update. Similar to the commit at hand, searching through the microcode
blob can be done on the already present buffer with no need to xmalloc
any further. Even better, do the filtering in microcode.c before
requesting the microcode update on all CPUs. The latter requires careful
restructuring and exposing the arch-specific logic for iterating over
patches and declaring a match.

Second, there are xmallocs for the microcode cache. Here, we would need
to ensure that the cache corresponding to the BSP gets xmalloc()'d and
populated after the fact.

Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
---
 xen/arch/x86/microcode.c | 32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 8b4d87782c..c878fc71ff 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -138,11 +138,6 @@ static int __init parse_ucode_param(const char *s)
 }
 custom_param("ucode", parse_ucode_param);
 
-/*
- * 8MB ought to be enough.
- */
-#define MAX_EARLY_CPIO_MICROCODE (8 << 20)
-
 void __init microcode_scan_module(
     unsigned long *module_map,
     const multiboot_info_t *mbi)
@@ -187,31 +182,12 @@ void __init microcode_scan_module(
         cd = find_cpio_data(p, _blob_start, _blob_size, &offset /* ignore */);
         if ( cd.data )
         {
-                /*
-                 * This is an arbitrary check - it would be sad if the blob
-                 * consumed most of the memory and did not allow guests
-                 * to launch.
-                 */
-                if ( cd.size > MAX_EARLY_CPIO_MICROCODE )
-                {
-                    printk("Multiboot %d microcode payload too big! (%ld, we can do %d)\n",
-                           i, cd.size, MAX_EARLY_CPIO_MICROCODE);
-                    goto err;
-                }
-                ucode_blob.size = cd.size;
-                ucode_blob.data = xmalloc_bytes(cd.size);
-                if ( !ucode_blob.data )
-                    cd.data = NULL;
-                else
-                    memcpy(ucode_blob.data, cd.data, cd.size);
+            ucode_blob.size = cd.size;
+            ucode_blob.data = cd.data;
+            break;
         }
         bootstrap_map(NULL);
-        if ( cd.data )
-            break;
     }
-    return;
-err:
-    bootstrap_map(NULL);
 }
 void __init microcode_grab_module(
     unsigned long *module_map,
@@ -725,7 +701,7 @@ static int __init microcode_init(void)
      */
     if ( ucode_blob.size )
     {
-        xfree(ucode_blob.data);
+        bootstrap_map(NULL);
         ucode_blob.size = 0;
         ucode_blob.data = NULL;
     }
-- 
2.17.1


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

[Xen-devel] [PATCH v2 3/4] x86/microcode: use const qualifier for microcode buffer

[Eslam Elnikety]

The buffer holding the microcode bits should be marked as const.

Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
---
 xen/arch/x86/microcode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index c878fc71ff..4616fa9d2e 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -86,7 +86,7 @@ static enum {
  * memory.
  */
 struct ucode_mod_blob {
-    void *data;
+    const void *data;
     size_t size;
 };
 
@@ -744,7 +744,7 @@ int microcode_update_one(bool start_update)
 int __init early_microcode_update_cpu(void)
 {
     int rc = 0;
-    void *data = NULL;
+    const void *data = NULL;
     size_t len;
     struct microcode_patch *patch;
 
-- 
2.17.1


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

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

[Eslam Elnikety]

Xen relies on boot modules to perform early microcode updates. This commit adds
another mode, namely "builtin" via the BUILTIN_UCODE config parameter. If set,
the Xen image itself will contain the microcode updates. Upon boot, Xen
inspects its image for microcode blobs and performs the update.

A Xen image with builtin microcode will, by default, attempt the microcode
update. Disabling the builtin microcode update can be done via the Xen command
line parameter 'ucode=no-builtin'. Moreover, the microcode provided via other
options (such as 'ucode=<integer>|scan' or 'ucode=<filename>' config when
booting via EFI) takes precedence over the builtin one.

Signed-off-by: Eslam Elnikety <elnikety@amazon.com>

---
Changes in v2:
- Allow for ucode=<integer>|scan,{no-}builtin and detail the model. Reflect
  those changes onto microcode.c and docs/misc/xen-command-line.pandoc
- Add documentation to the existing docs/admin-guide/microcode-loading.rst
- Build on Patches 1--3 to avoid xmalloc/memcpy for the builtin microcode
- Work configuration in order to specify the individual microcode blobs to use
  for the builtin microcode, and rework the microcode/Makefile accordingly
---
 docs/admin-guide/microcode-loading.rst | 31 +++++++++++++++
 docs/misc/xen-command-line.pandoc      | 10 ++++-
 xen/arch/x86/Kconfig                   | 30 +++++++++++++++
 xen/arch/x86/Makefile                  |  1 +
 xen/arch/x86/microcode.c               | 52 ++++++++++++++++++++++++++
 xen/arch/x86/microcode/Makefile        | 46 +++++++++++++++++++++++
 xen/arch/x86/xen.lds.S                 | 12 ++++++
 7 files changed, 180 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/x86/microcode/Makefile

diff --git a/docs/admin-guide/microcode-loading.rst b/docs/admin-guide/microcode-loading.rst
index e83cadd2c2..989e8d446b 100644
--- a/docs/admin-guide/microcode-loading.rst
+++ b/docs/admin-guide/microcode-loading.rst
@@ -104,6 +104,37 @@ The ``ucode=scan`` command line option will cause Xen to search through all
 modules to find any CPIO archives, and search the archive for the applicable
 file.  Xen will stop searching at the first match.
 
+Loading microcode built within the Xen image
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Xen can bundle microcode updates within its image. This support is conditional
+on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is
+useful to ensure that, by default, a minimum microcode patch level will be
+applied to the underlying CPU.
+
+To use microcode updates available on the build system as builtin,
+use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates
+and specify the individual microcode patches via either BUILTIN_UCODE_AMD or
+BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For
+instance, the configuration below is suitable for a build system which has a
+``/lib/firmware/`` directory which, in turn, includes the individual microcode
+patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and
+``intel-ucode/06-2f-02``.
+
+  CONFIG_BUILTIN_UCODE=y
+  CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/"
+  CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
+  CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02"
+
+Alternatively, CONFIG_BUILTIN_UCODE_{AMD,INTEL} can directly point to the
+concatenation of the individual microcode blobs. For instance, assuming that
+``amd-ucode/AuthenticAMD.bin`` and ``intel-ucode/GenuineIntel.bin`` hold
+multiple microcode updates for AMD and INTEL, respectively, you may use the
+configuration below.
+
+  CONFIG_BUILTIN_UCODE_AMD="amd-ucode/AuthenticAMD.bin"
+  CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/GenuineIntel.bin"
+
 
 Run time microcode loading
 --------------------------
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 40faf3bc3a..9cfc2df05a 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2126,10 +2126,10 @@ logic applies:
    active by default.
 
 ### ucode (x86)
-> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
+> `= List of [ <integer> | scan=<bool>, builtin=<bool>, nmi=<bool> ]`
 
     Applicability: x86
-    Default: `nmi`
+    Default: `nmi` if BUILTIN_UCODE is not enabled, `builtin,nmi` otherwise
 
 Controls for CPU microcode loading. For early loading, this parameter can
 specify how and where to find the microcode update blob. For late loading,
@@ -2150,6 +2150,12 @@ microcode in the cpio name space must be:
   - on Intel: kernel/x86/microcode/GenuineIntel.bin
   - on AMD  : kernel/x86/microcode/AuthenticAMD.bin
 
+'builtin' instructs the hypervisor to use the builtin microcode update. This
+option is available only if option BUILTIN_UCODE is enabled at build. The
+default value is `true`. If a microcode is provided via other options (such
+as 'integer', 'scan', or `ucode=<filename>` config when booting via EFI),
+the provided microcode takes precedence over the builtin one.
+
 'nmi' determines late loading is performed in NMI handler or just in
 stop_machine context. In NMI handler, even NMIs are blocked, which is
 considered safer. The default value is `true`.
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 02bb05f42e..9bc220925b 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -218,6 +218,36 @@ config MEM_SHARING
 	bool "Xen memory sharing support" if EXPERT = "y"
 	depends on HVM
 
+config BUILTIN_UCODE
+	bool "Support for Builtin Microcode"
+	---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.
+
+config BUILTIN_UCODE_DIR
+	string "Directory containing microcode updates"
+	default "/lib/firmware/"
+	depends on BUILTIN_UCODE
+	---help---
+	  The directory containing the microcode blobs.
+
+config BUILTIN_UCODE_AMD
+	string "AMD microcode updates"
+	default ""
+	depends on BUILTIN_UCODE
+	---help---
+	  AMD builtin microcode; space-sparated, relative to BUILTIN_UCODE_DIR.
+
+config BUILTIN_UCODE_INTEL
+	string "INTEL microcode updates"
+	default ""
+	depends on BUILTIN_UCODE
+	---help---
+	  INTEL builtin microcode; space-sparated, relative to BUILTIN_UCODE_DIR.
+
 endmenu
 
 source "common/Kconfig"
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 7da5a2631e..886691a377 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -3,6 +3,7 @@ subdir-y += cpu
 subdir-y += genapic
 subdir-$(CONFIG_GUEST) += guest
 subdir-$(CONFIG_HVM) += hvm
+subdir-$(CONFIG_BUILTIN_UCODE) += microcode
 subdir-y += mm
 subdir-$(CONFIG_XENOPROF) += oprofile
 subdir-$(CONFIG_PV) += pv
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 4616fa9d2e..bcfbd31041 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 __initdata ucode_builtin = 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;
 
@@ -122,6 +130,10 @@ static int __init parse_ucode_param(const char *s)
             ucode_in_nmi = val;
         else if ( (val = parse_boolean("scan", s, ss)) >= 0 )
             ucode_scan = val;
+#ifdef CONFIG_BUILTIN_UCODE
+        else if ( (val = parse_boolean("builtin", s, ss)) >= 0 )
+            ucode_builtin = val;
+#endif
         else
         {
             const char *q;
@@ -208,6 +220,40 @@ void __init microcode_grab_module(
         ucode_mod = mod[ucode_mod_idx];
     else 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 )
+    {
+        ucode_builtin = false;
+        return;
+    }
+
+    /* Set ucode_start/_end to the proper blob */
+    if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+    {
+        ucode_blob.size = __builtin_amd_ucode_end - __builtin_amd_ucode_start;
+        ucode_blob.data = __builtin_amd_ucode_start;
+    }
+    else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+    {
+        ucode_blob.size = __builtin_intel_ucode_end -
+                          __builtin_intel_ucode_start;
+        ucode_blob.data = __builtin_intel_ucode_start;
+    }
+    else
+        return;
+
+    if ( !ucode_blob.size )
+    {
+        printk("No builtin ucode for the CPU vendor.\n");
+        ucode_blob.data = NULL;
+    }
+#endif
 }
 
 const struct microcode_ops *microcode_ops;
@@ -701,7 +747,13 @@ static int __init microcode_init(void)
      */
     if ( ucode_blob.size )
     {
+#ifdef CONFIG_BUILTIN_UCODE
+        /* No need to destroy module mappings if builtin was used */
+        if ( !ucode_builtin )
+            bootstrap_map(NULL);
+#else
         bootstrap_map(NULL);
+#endif
         ucode_blob.size = 0;
         ucode_blob.data = NULL;
     }
diff --git a/xen/arch/x86/microcode/Makefile b/xen/arch/x86/microcode/Makefile
new file mode 100644
index 0000000000..c34d99903a
--- /dev/null
+++ b/xen/arch/x86/microcode/Makefile
@@ -0,0 +1,46 @@
+# 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.
+
+# Remove quotes and excess spaces from configuration strings
+UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
+UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
+UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
+
+# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs.
+amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
+intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
+
+ifneq ($(amd-blobs),)
+obj-y += ucode_amd.o
+endif
+
+ifneq ($(intel-blobs),)
+obj-y += ucode_intel.o
+endif
+
+ifeq ($(amd-blobs)$(intel-blobs),)
+obj-y += ucode_dummy.o
+endif
+
+ucode_amd.o: Makefile $(amd-blobs)
+	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 $@
+	rm -f $@.bin
+
+ucode_intel.o: Makefile $(intel-blobs)
+	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 $@
+	rm -f $@.bin
+
+ucode_dummy.o: Makefile
+	$(CC) $(CFLAGS) -c -x c /dev/null -o $@;
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 111edb5360..7a4c58c246 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -265,6 +265,18 @@ SECTIONS
        *(SORT(.data.vpci.*))
        __end_vpci_array = .;
 #endif
+
+#if defined(CONFIG_BUILTIN_UCODE)
+       . = ALIGN(POINTER_ALIGN);
+       __builtin_amd_ucode_start = .;
+       *(.builtin_amd_ucode)
+       __builtin_amd_ucode_end = .;
+
+       . = ALIGN(POINTER_ALIGN);
+       __builtin_intel_ucode_start = .;
+       *(.builtin_intel_ucode)
+       __builtin_intel_ucode_end = .;
+#endif
   } :text
 
   . = ALIGN(SECTION_ALIGN);
-- 
2.17.1


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

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

[Jan Beulich]

On 18.12.2019 02:32, Eslam Elnikety wrote:
> 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.

I guess "unspecified effect" in the doc was pretty pointless - such
options have been ignored before; in fact ...

> With that, Xen can explicitly
> ignore those named options when using EFI.

... I don't see things becoming any more explicit (not even parsing
the options was quite explicit to me).

> As an added benefit,
> we get a straightfoward parsing of the ucode parameter.

It's a single if() you eliminate - for me this doesn't make it
meaningfully more straightforward.

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

It's always stop_machine context, isn't it? I also don't think this
implementation detail belongs here.

> --- 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;

I don't see anything negative be put into here - should be unsigned
int then.

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

Is it guaranteed (now and forever) that the index passed in is
non-zero? You changes to microcode_grab_module() imply so, but
just looking at the call site of the function I can't convince
myself this is the case. _If_ it is (thought to be) guaranteed,
then I think you at least want to ASSERT() here, perhaps with
a comment.

>  }
>  
> -/*
> - * 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)

Any particular reason for the renaming? The function name was quite
fine imo.

Jan

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

Re: [Xen-devel] [PATCH v2 2/4] x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data

[Jan Beulich]

On 18.12.2019 02:32, Eslam Elnikety wrote:
> @@ -725,7 +701,7 @@ static int __init microcode_init(void)
>       */
>      if ( ucode_blob.size )
>      {
> -        xfree(ucode_blob.data);
> +        bootstrap_map(NULL);

As much as I like the change, I wholeheartedly disagree with this
aspect of it: You make it largely unpredictable when the boot
mappings will go away - it becomes entirely dependent upon link
order. And of course we really want these mappings to be gone,
the very latest (I think), by the time we start bringing up APs
(but generally the sooner the better). This is (one of?) the main
reason(s) why it hadn't been done this way to begin with. The
alternative is more complicated (set up a proper, long term
mapping), but it's going to be more clean (including the mapping
then also being suitable to post-boot CPU onlining).

Jan

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

Re: [Xen-devel] [PATCH v2 3/4] x86/microcode: use const qualifier for microcode buffer

[Jan Beulich]

On 18.12.2019 02:32, Eslam Elnikety wrote:
> The buffer holding the microcode bits should be marked as const.
> 
> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

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

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

[Jan Beulich]

On 18.12.2019 02:32, Eslam Elnikety wrote:
> Xen relies on boot modules to perform early microcode updates. This commit adds
> another mode, namely "builtin" via the BUILTIN_UCODE config parameter. If set,
> the Xen image itself will contain the microcode updates. Upon boot, Xen
> inspects its image for microcode blobs and performs the update.
> 
> A Xen image with builtin microcode will, by default, attempt the microcode
> update. Disabling the builtin microcode update can be done via the Xen command
> line parameter 'ucode=no-builtin'. Moreover, the microcode provided via other
> options (such as 'ucode=<integer>|scan' or 'ucode=<filename>' config when
> booting via EFI) takes precedence over the builtin one.
> 
> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
> 
> ---
> Changes in v2:
> - Allow for ucode=<integer>|scan,{no-}builtin and detail the model. Reflect
>   those changes onto microcode.c and docs/misc/xen-command-line.pandoc
> - Add documentation to the existing docs/admin-guide/microcode-loading.rst
> - Build on Patches 1--3 to avoid xmalloc/memcpy for the builtin microcode
> - Work configuration in order to specify the individual microcode blobs to use
>   for the builtin microcode, and rework the microcode/Makefile accordingly
> ---
>  docs/admin-guide/microcode-loading.rst | 31 +++++++++++++++
>  docs/misc/xen-command-line.pandoc      | 10 ++++-
>  xen/arch/x86/Kconfig                   | 30 +++++++++++++++
>  xen/arch/x86/Makefile                  |  1 +
>  xen/arch/x86/microcode.c               | 52 ++++++++++++++++++++++++++
>  xen/arch/x86/microcode/Makefile        | 46 +++++++++++++++++++++++
>  xen/arch/x86/xen.lds.S                 | 12 ++++++
>  7 files changed, 180 insertions(+), 2 deletions(-)
>  create mode 100644 xen/arch/x86/microcode/Makefile
> 
> diff --git a/docs/admin-guide/microcode-loading.rst b/docs/admin-guide/microcode-loading.rst
> index e83cadd2c2..989e8d446b 100644
> --- a/docs/admin-guide/microcode-loading.rst
> +++ b/docs/admin-guide/microcode-loading.rst
> @@ -104,6 +104,37 @@ The ``ucode=scan`` command line option will cause Xen to search through all
>  modules to find any CPIO archives, and search the archive for the applicable
>  file.  Xen will stop searching at the first match.
>  
> +Loading microcode built within the Xen image

I think either s/within/into/ or e.g. s/built/contained/.

> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Xen can bundle microcode updates within its image. This support is conditional
> +on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is
> +useful to ensure that, by default, a minimum microcode patch level will be
> +applied to the underlying CPU.
> +
> +To use microcode updates available on the build system as builtin,
> +use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates
> +and specify the individual microcode patches via either BUILTIN_UCODE_AMD or
> +BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For
> +instance, the configuration below is suitable for a build system which has a
> +``/lib/firmware/`` directory which, in turn, includes the individual microcode
> +patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and
> +``intel-ucode/06-2f-02``.
> +
> +  CONFIG_BUILTIN_UCODE=y
> +  CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/"
> +  CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
> +  CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02"

Rather than a blank as separator, the more conventional one on
Unix and alike would be : I think. Of course ideally there wouldn't
be any restriction at all on the characters usable here for file
names.

> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -218,6 +218,36 @@ config MEM_SHARING
>  	bool "Xen memory sharing support" if EXPERT = "y"
>  	depends on HVM
>  
> +config BUILTIN_UCODE
> +	bool "Support for Builtin Microcode"
> +	---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.

I continue to be unconvinced that this separate option is needed.
Albeit compared to the v1 approach I will agree that handling
would become more complicated without.

> +config BUILTIN_UCODE_DIR
> +	string "Directory containing microcode updates"
> +	default "/lib/firmware/"
> +	depends on BUILTIN_UCODE
> +	---help---
> +	  The directory containing the microcode blobs.
> +
> +config BUILTIN_UCODE_AMD
> +	string "AMD microcode updates"
> +	default ""
> +	depends on BUILTIN_UCODE
> +	---help---
> +	  AMD builtin microcode; space-sparated, relative to BUILTIN_UCODE_DIR.
> +
> +config BUILTIN_UCODE_INTEL
> +	string "INTEL microcode updates"
> +	default ""
> +	depends on BUILTIN_UCODE
> +	---help---
> +	  INTEL builtin microcode; space-sparated, relative to BUILTIN_UCODE_DIR.

I don't think Intel is commonly spelled all uppercase.

I further think you want to mention further constraints (beyond
the separator to use): DIR would probably better be an absolute
path, it's ambiguous (right here, i.e. without looking at the
implementation) whether a trailing slash needs including), the
individual blobs may include paths, and it's ambiguous again
what them having a leading slash would mean (I think it would be
better if it was "absolute or relative to BUILTIN_UCODE_DIR").

Also a spelling nit: "separated".

> --- 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 __initdata ucode_builtin = true;
> +
> +extern const char __builtin_intel_ucode_start[], __builtin_intel_ucode_end[];
> +extern const char __builtin_amd_ucode_start[], __builtin_amd_ucode_end[];

While we do use plain char elsewhere for similar purposes, I think
this is bad practice. It would better be unsigned char or uint8_t.

> @@ -208,6 +220,40 @@ void __init microcode_grab_module(
>          ucode_mod = mod[ucode_mod_idx];
>      else 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 )
> +    {
> +        ucode_builtin = false;
> +        return;
> +    }
> +
> +    /* Set ucode_start/_end to the proper blob */
> +    if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
> +    {
> +        ucode_blob.size = __builtin_amd_ucode_end - __builtin_amd_ucode_start;
> +        ucode_blob.data = __builtin_amd_ucode_start;
> +    }
> +    else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
> +    {
> +        ucode_blob.size = __builtin_intel_ucode_end -
> +                          __builtin_intel_ucode_start;
> +        ucode_blob.data = __builtin_intel_ucode_start;
> +    }
> +    else
> +        return;

"switch ( boot_cpu_data.x86_vendor )" please.

> +    if ( !ucode_blob.size )
> +    {
> +        printk("No builtin ucode for the CPU vendor.\n");

Please either omit "for the CPU vendor" or name the vendor. In any
event please omit the full stop.

> @@ -701,7 +747,13 @@ static int __init microcode_init(void)
>       */
>      if ( ucode_blob.size )
>      {
> +#ifdef CONFIG_BUILTIN_UCODE
> +        /* No need to destroy module mappings if builtin was used */
> +        if ( !ucode_builtin )
> +            bootstrap_map(NULL);
> +#else
>          bootstrap_map(NULL);
> +#endif

First of all - is there no ucode unrelated side effect of this
invocation? I.e. can it safely be skipped? If yes, then I think
you want to get away without #ifdef here, by having a suitably
placed

#define ucode_builtin false

somewhere up the file.

> --- /dev/null
> +++ b/xen/arch/x86/microcode/Makefile
> @@ -0,0 +1,46 @@
> +# 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.
> +
> +# Remove quotes and excess spaces from configuration strings
> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
> +
> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs.
> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
> +
> +ifneq ($(amd-blobs),)
> +obj-y += ucode_amd.o
> +endif
> +
> +ifneq ($(intel-blobs),)
> +obj-y += ucode_intel.o
> +endif
> +
> +ifeq ($(amd-blobs)$(intel-blobs),)
> +obj-y += ucode_dummy.o
> +endif
> +
> +ucode_amd.o: Makefile $(amd-blobs)
> +	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 $@
> +	rm -f $@.bin
> +
> +ucode_intel.o: Makefile $(intel-blobs)
> +	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 $@
> +	rm -f $@.bin

This can be had with a pattern rule (with the vendor being the stem)
and hence without duplication, I think.

Also - is simply concatenating the blobs reliable enough? There's no
build time diagnostic that the result would actually be understood
at runtime.

> +ucode_dummy.o: Makefile
> +	$(CC) $(CFLAGS) -c -x c /dev/null -o $@;

Since the commit message doesn't explain why this is needed, I
have to ask (I guess we somewhere have a dependency on $(obj-y)
not being empty). _If_ it is needed, I don't see why you need
ifeq() around its use. In fact you could have

obj-y := ucode-dummy.o

right at the top of the file.

Furthermore I don't really understand why you need this in the
first place. While cat won't do what you want with an empty
argument list, can't you simply prepend / append /dev/null?

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -265,6 +265,18 @@ SECTIONS
>         *(SORT(.data.vpci.*))
>         __end_vpci_array = .;
>  #endif
> +
> +#if defined(CONFIG_BUILTIN_UCODE)
> +       . = ALIGN(POINTER_ALIGN);

Why (same further down)? The alignment of both vendors' header
structures is just 4 afaict.

Jan

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

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

[Eslam Elnikety]

On 18.12.19 12:49, Jan Beulich wrote:
> On 18.12.2019 02:32, Eslam Elnikety wrote:
>> 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.
> 
> I guess "unspecified effect" in the doc was pretty pointless - such
> options have been ignored before; in fact ...
> 
>> With that, Xen can explicitly
>> ignore those named options when using EFI.
> 
> ... I don't see things becoming any more explicit (not even parsing
> the options was quite explicit to me).
> 

I agree that those options have been ignored so far in the case of EFI. 
The documentation contradicts that however. The documentation:
* says <integer> has unspecified effect.
* does not mention anything about scan being ignored.

With this patch, it is explicit in code and in documentation that both 
options are ignored in case of EFI.

>> As an added benefit,
>> we get a straightfoward parsing of the ucode parameter.
> 
> It's a single if() you eliminate - for me this doesn't make it
> meaningfully more straightforward.
> 

As we decouple ucode_mod_idx and ucode_mod_efi_idx, the parsing of the 
ucode= just follows the pattern "[ <integer> | scan=<bool>, nmi=<bool> 
]" and it does not have to cater for corner cases. In either case, if 
you strongly disagree with the wording, I can drop that sentence.

>> --- 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.
> 
> It's always stop_machine context, isn't it? I also don't think this
> implementation detail belongs here.
> 

Needs a better wording indeed. Let me know if you have particular 
suggestions, and I will incorporate in v3.

>> --- 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;
> 
> I don't see anything negative be put into here - should be unsigned
> int then.
> 

Correct! The type just carried over from the coupling with 
ucode_mod_idx. Will adjust in v3.

>> @@ -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;
> 
> Is it guaranteed (now and forever) that the index passed in is
> non-zero? You changes to microcode_grab_module() imply so, but
> just looking at the call site of the function I can't convince
> myself this is the case. _If_ it is (thought to be) guaranteed,
> then I think you at least want to ASSERT() here, perhaps with
> a comment.
> 

For x86, it seems we have that guarantee (given that we must have a 
dom0). Right?

>>   }
>>   
>> -/*
>> - * 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)
> 
> Any particular reason for the renaming? The function name was quite
> fine imo.
> 

To me, "parse_ucode" is a misnomer.

Thanks for your comments, Jan.

-- Eslam

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

Re: [Xen-devel] [PATCH v2 2/4] x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data

[Eslam Elnikety]

On 18.12.19 13:05, Jan Beulich wrote:
> On 18.12.2019 02:32, Eslam Elnikety wrote:
>> @@ -725,7 +701,7 @@ static int __init microcode_init(void)
>>        */
>>       if ( ucode_blob.size )
>>       {
>> -        xfree(ucode_blob.data);
>> +        bootstrap_map(NULL);
> 
> As much as I like the change, I wholeheartedly disagree with this
> aspect of it: You make it largely unpredictable when the boot
> mappings will go away - it becomes entirely dependent upon link
> order. And of course we really want these mappings to be gone,
> the very latest (I think), by the time we start bringing up APs
> (but generally the sooner the better). This is (one of?) the main
> reason(s) why it hadn't been done this way to begin with. The
> alternative is more complicated (set up a proper, long term
> mapping), but it's going to be more clean (including the mapping
> then also being suitable to post-boot CPU onlining).
> 

This change is an improvement on the current status. We get to avoid 
xmalloc/memcpy in the case of a successful ucode=scan. The problematic 
aspect you highlight is anyway there regardless of this patch (ref. to 
the "else if ( ucode_mod.mod_end )" in microcode_init). The proper, long 
term mapping would be a welcome change, but is otherwise independent of 
this patch imo.

Thanks,
Eslam

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

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

[Eslam Elnikety]

On 18.12.19 13:42, Jan Beulich wrote:
> On 18.12.2019 02:32, Eslam Elnikety wrote:
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +Xen can bundle microcode updates within its image. This support is conditional
>> +on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is
>> +useful to ensure that, by default, a minimum microcode patch level will be
>> +applied to the underlying CPU.
>> +
>> +To use microcode updates available on the build system as builtin,
>> +use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates
>> +and specify the individual microcode patches via either BUILTIN_UCODE_AMD or
>> +BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For
>> +instance, the configuration below is suitable for a build system which has a
>> +``/lib/firmware/`` directory which, in turn, includes the individual microcode
>> +patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and
>> +``intel-ucode/06-2f-02``.
>> +
>> +  CONFIG_BUILTIN_UCODE=y
>> +  CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/"
>> +  CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
>> +  CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02"
> 
> Rather than a blank as separator, the more conventional one on
> Unix and alike would be : I think. Of course ideally there wouldn't
> be any restriction at all on the characters usable here for file
> names.
> 

It would be great if there is a particular convention. The blank 
separator is aligned with Linux way of doing builtin microcode.

>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -218,6 +218,36 @@ config MEM_SHARING
>>   	bool "Xen memory sharing support" if EXPERT = "y"
>>   	depends on HVM
>>   
>> +config BUILTIN_UCODE
>> +	bool "Support for Builtin Microcode"
>> +	---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.
> 
> I continue to be unconvinced that this separate option is needed.
> Albeit compared to the v1 approach I will agree that handling
> would become more complicated without.
> 

Any particular preference between the v1 vs v2 approach?

>> @@ -701,7 +747,13 @@ static int __init microcode_init(void)
>>        */
>>       if ( ucode_blob.size )
>>       {
>> +#ifdef CONFIG_BUILTIN_UCODE
>> +        /* No need to destroy module mappings if builtin was used */
>> +        if ( !ucode_builtin )
>> +            bootstrap_map(NULL);
>> +#else
>>           bootstrap_map(NULL);
>> +#endif
> 
> First of all - is there no ucode unrelated side effect of this
> invocation? I.e. can it safely be skipped?

Maybe I am missing something. Are you asking if we can safely skip the 
bootstrap_map(NULL)? (Quoting your response on PATCH v2 2/4 "And of 
course we really want these mappings to be gone")

> If yes, then I think
> you want to get away without #ifdef here, by having a suitably
> placed
> 
> #define ucode_builtin false
> 
> somewhere up the file.
> 

Agreed. That will make the code snippet more readable indeed.

>> --- /dev/null
>> +++ b/xen/arch/x86/microcode/Makefile
>> @@ -0,0 +1,46 @@
>> +# 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.
>> +
>> +# Remove quotes and excess spaces from configuration strings
>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
>> +
>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs.
>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
>> +
>> +ifneq ($(amd-blobs),)
>> +obj-y += ucode_amd.o
>> +endif
>> +
>> +ifneq ($(intel-blobs),)
>> +obj-y += ucode_intel.o
>> +endif
>> +
>> +ifeq ($(amd-blobs)$(intel-blobs),)
>> +obj-y += ucode_dummy.o
>> +endif
>> +
>> +ucode_amd.o: Makefile $(amd-blobs)
>> +	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 $@
>> +	rm -f $@.bin
>> +
>> +ucode_intel.o: Makefile $(intel-blobs)
>> +	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 $@
>> +	rm -f $@.bin
> 
> This can be had with a pattern rule (with the vendor being the stem)
> and hence without duplication, I think.
> 
> Also - is simply concatenating the blobs reliable enough? There's no
> build time diagnostic that the result would actually be understood
> at runtime.
> 

Concatenation is reliable (as long as the individual microcode blobs are 
not malformed, and in that case the builtin is not making matters worse 
compared to presenting the malformed update via <integer> | scan).

>> +ucode_dummy.o: Makefile
>> +	$(CC) $(CFLAGS) -c -x c /dev/null -o $@;
> 
> Since the commit message doesn't explain why this is needed, I
> have to ask (I guess we somewhere have a dependency on $(obj-y)
> not being empty).

Your guess is correct. All sub-directories of xen/arch/x86 are expected 
to produce built_in.o. If there are not amd nor intel microcode blobs, 
there will be no build dependencies and the build fails preparing the 
built_in.o

> _If_ it is needed, I don't see why you need
> ifeq() around its use. In fact you could have
> 
> obj-y := ucode-dummy.o
> 
> right at the top of the file.
> 
> Furthermore I don't really understand why you need this in the
> first place. While cat won't do what you want with an empty
> argument list, can't you simply prepend / append /dev/null?
> 

To make sure we are on the same page. You are suggesting using 
"/dev/null" in case there are no amd/intel ucode to generate the 
ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as 
input (complains about empty binary).

(I agree with your other inline suggestions that I have omitted. Those I 
will address in v3).

Thanks,
Eslam

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

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

[Jan Beulich]

On 19.12.2019 22:08, Eslam Elnikety wrote:
> On 18.12.19 12:49, Jan Beulich wrote:
>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>> 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.
>>
>> I guess "unspecified effect" in the doc was pretty pointless - such
>> options have been ignored before; in fact ...
>>
>>> With that, Xen can explicitly
>>> ignore those named options when using EFI.
>>
>> ... I don't see things becoming any more explicit (not even parsing
>> the options was quite explicit to me).
>>
> 
> I agree that those options have been ignored so far in the case of EFI. 
> The documentation contradicts that however. The documentation:
> * says <integer> has unspecified effect.
> * does not mention anything about scan being ignored.
> 
> With this patch, it is explicit in code and in documentation that both 
> options are ignored in case of EFI.

But isn't it rather that ucode=scan could (and hence perhaps should)
also have its value on EFI?

>>> As an added benefit,
>>> we get a straightfoward parsing of the ucode parameter.
>>
>> It's a single if() you eliminate - for me this doesn't make it
>> meaningfully more straightforward.
>>
> 
> As we decouple ucode_mod_idx and ucode_mod_efi_idx, the parsing of the 
> ucode= just follows the pattern "[ <integer> | scan=<bool>, nmi=<bool> 
> ]" and it does not have to cater for corner cases. In either case, if 
> you strongly disagree with the wording, I can drop that sentence.

Well, what I don't like is giving the impression that things have been
worse than they actually are.

>>> --- 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.
>>
>> It's always stop_machine context, isn't it? I also don't think this
>> implementation detail belongs here.
>>
> 
> Needs a better wording indeed. Let me know if you have particular 
> suggestions, and I will incorporate in v3.

Just drop everything from "or" onwards?

>>> @@ -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;
>>
>> Is it guaranteed (now and forever) that the index passed in is
>> non-zero? You changes to microcode_grab_module() imply so, but
>> just looking at the call site of the function I can't convince
>> myself this is the case. _If_ it is (thought to be) guaranteed,
>> then I think you at least want to ASSERT() here, perhaps with
>> a comment.
>>
> 
> For x86, it seems we have that guarantee (given that we must have a 
> dom0). Right?

For fully bringing up the system - yes. But there's no reason to
have a Dom0 if all you're after is testing early Xen boot. There'll
be a panic() in the case, but there shouldn't be anything breaking
proper behavior prior to this point.

>>>   }
>>>   
>>> -/*
>>> - * 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)
>>
>> Any particular reason for the renaming? The function name was quite
>> fine imo.
>>
> 
> To me, "parse_ucode" is a misnomer.

Well, parse_"ucode= isn't a valid identifier. parse_ucode is what
results when stripping everything that makes it invalid. I can
see the ambiguity with parsing actual ucode, but the context here
makes it pretty clear what the function is about. Personally I'd
prefer such unnecessary renames to be avoided.

Jan

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

Re: [Xen-devel] [PATCH v2 2/4] x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data

[Jan Beulich]

On 19.12.2019 22:25, Eslam Elnikety wrote:
> On 18.12.19 13:05, Jan Beulich wrote:
>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>> @@ -725,7 +701,7 @@ static int __init microcode_init(void)
>>>        */
>>>       if ( ucode_blob.size )
>>>       {
>>> -        xfree(ucode_blob.data);
>>> +        bootstrap_map(NULL);
>>
>> As much as I like the change, I wholeheartedly disagree with this
>> aspect of it: You make it largely unpredictable when the boot
>> mappings will go away - it becomes entirely dependent upon link
>> order. And of course we really want these mappings to be gone,
>> the very latest (I think), by the time we start bringing up APs
>> (but generally the sooner the better). This is (one of?) the main
>> reason(s) why it hadn't been done this way to begin with. The
>> alternative is more complicated (set up a proper, long term
>> mapping), but it's going to be more clean (including the mapping
>> then also being suitable to post-boot CPU onlining).
>>
> 
> This change is an improvement on the current status. We get to avoid 
> xmalloc/memcpy in the case of a successful ucode=scan. The problematic 
> aspect you highlight is anyway there regardless of this patch (ref. to 
> the "else if ( ucode_mod.mod_end )" in microcode_init).

Hmm, fair point. I'm not a fan of making a bad situation worse,
but I think it's acceptable here:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

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

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

[Jan Beulich]

On 19.12.2019 23:11, Eslam Elnikety wrote:
> On 18.12.19 13:42, Jan Beulich wrote:
>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +Xen can bundle microcode updates within its image. This support is conditional
>>> +on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is
>>> +useful to ensure that, by default, a minimum microcode patch level will be
>>> +applied to the underlying CPU.
>>> +
>>> +To use microcode updates available on the build system as builtin,
>>> +use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates
>>> +and specify the individual microcode patches via either BUILTIN_UCODE_AMD or
>>> +BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For
>>> +instance, the configuration below is suitable for a build system which has a
>>> +``/lib/firmware/`` directory which, in turn, includes the individual microcode
>>> +patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and
>>> +``intel-ucode/06-2f-02``.
>>> +
>>> +  CONFIG_BUILTIN_UCODE=y
>>> +  CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/"
>>> +  CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
>>> +  CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02"
>>
>> Rather than a blank as separator, the more conventional one on
>> Unix and alike would be : I think. Of course ideally there wouldn't
>> be any restriction at all on the characters usable here for file
>> names.
>>
> 
> It would be great if there is a particular convention. The blank 
> separator is aligned with Linux way of doing builtin microcode.

Well, this is then another area where I would question whether we
really want to follow the Linux approach, but I'm not bothered
enough to make less non-conventional behavior here a requirement.

>>> --- a/xen/arch/x86/Kconfig
>>> +++ b/xen/arch/x86/Kconfig
>>> @@ -218,6 +218,36 @@ config MEM_SHARING
>>>   	bool "Xen memory sharing support" if EXPERT = "y"
>>>   	depends on HVM
>>>   
>>> +config BUILTIN_UCODE
>>> +	bool "Support for Builtin Microcode"
>>> +	---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.
>>
>> I continue to be unconvinced that this separate option is needed.
>> Albeit compared to the v1 approach I will agree that handling
>> would become more complicated without.
> 
> Any particular preference between the v1 vs v2 approach?

I definitely like the vendor separation.

>>> @@ -701,7 +747,13 @@ static int __init microcode_init(void)
>>>        */
>>>       if ( ucode_blob.size )
>>>       {
>>> +#ifdef CONFIG_BUILTIN_UCODE
>>> +        /* No need to destroy module mappings if builtin was used */
>>> +        if ( !ucode_builtin )
>>> +            bootstrap_map(NULL);
>>> +#else
>>>           bootstrap_map(NULL);
>>> +#endif
>>
>> First of all - is there no ucode unrelated side effect of this
>> invocation? I.e. can it safely be skipped?
> 
> Maybe I am missing something. Are you asking if we can safely skip the 
> bootstrap_map(NULL)? (Quoting your response on PATCH v2 2/4 "And of 
> course we really want these mappings to be gone")

Yes - my point is that invoking the function here may in
principle cover for other mappings. However - this is the
invocation you've added in an earlier patch, isn't it? In
which case omitting it should be fine. Nevertheless I don't
see and harm in invoking the function, i.e. I'd rather keep
the code here simple.

>>> --- /dev/null
>>> +++ b/xen/arch/x86/microcode/Makefile
>>> @@ -0,0 +1,46 @@
>>> +# 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.
>>> +
>>> +# Remove quotes and excess spaces from configuration strings
>>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
>>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
>>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
>>> +
>>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs.
>>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
>>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
>>> +
>>> +ifneq ($(amd-blobs),)
>>> +obj-y += ucode_amd.o
>>> +endif
>>> +
>>> +ifneq ($(intel-blobs),)
>>> +obj-y += ucode_intel.o
>>> +endif
>>> +
>>> +ifeq ($(amd-blobs)$(intel-blobs),)
>>> +obj-y += ucode_dummy.o
>>> +endif
>>> +
>>> +ucode_amd.o: Makefile $(amd-blobs)
>>> +	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 $@
>>> +	rm -f $@.bin
>>> +
>>> +ucode_intel.o: Makefile $(intel-blobs)
>>> +	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 $@
>>> +	rm -f $@.bin
>>
>> This can be had with a pattern rule (with the vendor being the stem)
>> and hence without duplication, I think.
>>
>> Also - is simply concatenating the blobs reliable enough? There's no
>> build time diagnostic that the result would actually be understood
>> at runtime.
>>
> 
> Concatenation is reliable (as long as the individual microcode blobs are 
> not malformed, and in that case the builtin is not making matters worse 
> compared to presenting the malformed update via <integer> | scan).

A malformed update found the other way is a bug in the tools
constructing the respective images. A malformed built-in
update is a bug in the Xen build system. The put the question
differently: Is it specified somewhere that the blobs all have
to have certain properties, which the straight concatenation
relies upon?

>>> +ucode_dummy.o: Makefile
>>> +	$(CC) $(CFLAGS) -c -x c /dev/null -o $@;
>>
>> Since the commit message doesn't explain why this is needed, I
>> have to ask (I guess we somewhere have a dependency on $(obj-y)
>> not being empty).
> 
> Your guess is correct. All sub-directories of xen/arch/x86 are expected 
> to produce built_in.o. If there are not amd nor intel microcode blobs, 
> there will be no build dependencies and the build fails preparing the 
> built_in.o

That's rather poor, but it's of course not your task to get this
fixed (it shouldn't be very difficult to create an empty
built_in.o for an empty $(obj-y)).

>> _If_ it is needed, I don't see why you need
>> ifeq() around its use. In fact you could have
>>
>> obj-y := ucode-dummy.o
>>
>> right at the top of the file.
>>
>> Furthermore I don't really understand why you need this in the
>> first place. While cat won't do what you want with an empty
>> argument list, can't you simply prepend / append /dev/null?
>>
> 
> To make sure we are on the same page. You are suggesting using 
> "/dev/null" in case there are no amd/intel ucode to generate the 
> ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as 
> input (complains about empty binary).

That's again rather poor, this time of the utility - it should be
easy enough to produce an object with an empty .data (or whatever
it is) section. As above - I'm fine with you keeping the logic
then as is, provided you say in the description why it can't be
simplified.

Jan

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

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

[Jürgen Groß]

[-- Attachment #1: Type: text/plain, Size: 8397 bytes --]

On 20.12.19 11:12, Jan Beulich wrote:
> On 19.12.2019 23:11, Eslam Elnikety wrote:
>> On 18.12.19 13:42, Jan Beulich wrote:
>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> +
>>>> +Xen can bundle microcode updates within its image. This support is conditional
>>>> +on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is
>>>> +useful to ensure that, by default, a minimum microcode patch level will be
>>>> +applied to the underlying CPU.
>>>> +
>>>> +To use microcode updates available on the build system as builtin,
>>>> +use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates
>>>> +and specify the individual microcode patches via either BUILTIN_UCODE_AMD or
>>>> +BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For
>>>> +instance, the configuration below is suitable for a build system which has a
>>>> +``/lib/firmware/`` directory which, in turn, includes the individual microcode
>>>> +patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and
>>>> +``intel-ucode/06-2f-02``.
>>>> +
>>>> +  CONFIG_BUILTIN_UCODE=y
>>>> +  CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/"
>>>> +  CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
>>>> +  CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02"
>>>
>>> Rather than a blank as separator, the more conventional one on
>>> Unix and alike would be : I think. Of course ideally there wouldn't
>>> be any restriction at all on the characters usable here for file
>>> names.
>>>
>>
>> It would be great if there is a particular convention. The blank
>> separator is aligned with Linux way of doing builtin microcode.
> 
> Well, this is then another area where I would question whether we
> really want to follow the Linux approach, but I'm not bothered
> enough to make less non-conventional behavior here a requirement.
> 
>>>> --- a/xen/arch/x86/Kconfig
>>>> +++ b/xen/arch/x86/Kconfig
>>>> @@ -218,6 +218,36 @@ config MEM_SHARING
>>>>    	bool "Xen memory sharing support" if EXPERT = "y"
>>>>    	depends on HVM
>>>>    
>>>> +config BUILTIN_UCODE
>>>> +	bool "Support for Builtin Microcode"
>>>> +	---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.
>>>
>>> I continue to be unconvinced that this separate option is needed.
>>> Albeit compared to the v1 approach I will agree that handling
>>> would become more complicated without.
>>
>> Any particular preference between the v1 vs v2 approach?
> 
> I definitely like the vendor separation.
> 
>>>> @@ -701,7 +747,13 @@ static int __init microcode_init(void)
>>>>         */
>>>>        if ( ucode_blob.size )
>>>>        {
>>>> +#ifdef CONFIG_BUILTIN_UCODE
>>>> +        /* No need to destroy module mappings if builtin was used */
>>>> +        if ( !ucode_builtin )
>>>> +            bootstrap_map(NULL);
>>>> +#else
>>>>            bootstrap_map(NULL);
>>>> +#endif
>>>
>>> First of all - is there no ucode unrelated side effect of this
>>> invocation? I.e. can it safely be skipped?
>>
>> Maybe I am missing something. Are you asking if we can safely skip the
>> bootstrap_map(NULL)? (Quoting your response on PATCH v2 2/4 "And of
>> course we really want these mappings to be gone")
> 
> Yes - my point is that invoking the function here may in
> principle cover for other mappings. However - this is the
> invocation you've added in an earlier patch, isn't it? In
> which case omitting it should be fine. Nevertheless I don't
> see and harm in invoking the function, i.e. I'd rather keep
> the code here simple.
> 
>>>> --- /dev/null
>>>> +++ b/xen/arch/x86/microcode/Makefile
>>>> @@ -0,0 +1,46 @@
>>>> +# 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.
>>>> +
>>>> +# Remove quotes and excess spaces from configuration strings
>>>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
>>>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
>>>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
>>>> +
>>>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs.
>>>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
>>>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
>>>> +
>>>> +ifneq ($(amd-blobs),)
>>>> +obj-y += ucode_amd.o
>>>> +endif
>>>> +
>>>> +ifneq ($(intel-blobs),)
>>>> +obj-y += ucode_intel.o
>>>> +endif
>>>> +
>>>> +ifeq ($(amd-blobs)$(intel-blobs),)
>>>> +obj-y += ucode_dummy.o
>>>> +endif
>>>> +
>>>> +ucode_amd.o: Makefile $(amd-blobs)
>>>> +	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 $@
>>>> +	rm -f $@.bin
>>>> +
>>>> +ucode_intel.o: Makefile $(intel-blobs)
>>>> +	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 $@
>>>> +	rm -f $@.bin
>>>
>>> This can be had with a pattern rule (with the vendor being the stem)
>>> and hence without duplication, I think.
>>>
>>> Also - is simply concatenating the blobs reliable enough? There's no
>>> build time diagnostic that the result would actually be understood
>>> at runtime.
>>>
>>
>> Concatenation is reliable (as long as the individual microcode blobs are
>> not malformed, and in that case the builtin is not making matters worse
>> compared to presenting the malformed update via <integer> | scan).
> 
> A malformed update found the other way is a bug in the tools
> constructing the respective images. A malformed built-in
> update is a bug in the Xen build system. The put the question
> differently: Is it specified somewhere that the blobs all have
> to have certain properties, which the straight concatenation
> relies upon?
> 
>>>> +ucode_dummy.o: Makefile
>>>> +	$(CC) $(CFLAGS) -c -x c /dev/null -o $@;
>>>
>>> Since the commit message doesn't explain why this is needed, I
>>> have to ask (I guess we somewhere have a dependency on $(obj-y)
>>> not being empty).
>>
>> Your guess is correct. All sub-directories of xen/arch/x86 are expected
>> to produce built_in.o. If there are not amd nor intel microcode blobs,
>> there will be no build dependencies and the build fails preparing the
>> built_in.o
> 
> That's rather poor, but it's of course not your task to get this
> fixed (it shouldn't be very difficult to create an empty
> built_in.o for an empty $(obj-y)).
> 
>>> _If_ it is needed, I don't see why you need
>>> ifeq() around its use. In fact you could have
>>>
>>> obj-y := ucode-dummy.o
>>>
>>> right at the top of the file.
>>>
>>> Furthermore I don't really understand why you need this in the
>>> first place. While cat won't do what you want with an empty
>>> argument list, can't you simply prepend / append /dev/null?
>>>
>>
>> To make sure we are on the same page. You are suggesting using
>> "/dev/null" in case there are no amd/intel ucode to generate the
>> ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as
>> input (complains about empty binary).
> 
> That's again rather poor, this time of the utility - it should be
> easy enough to produce an object with an empty .data (or whatever
> it is) section. As above - I'm fine with you keeping the logic
> then as is, provided you say in the description why it can't be
> simplified.

What about using the attached patch for including the binary files?

I wanted to post that for my hypervisor-fs series, but I think it would
fit here quite nice.


Juergen

[-- Attachment #2: 0001-xen-add-a-generic-way-to-include-binary-files-as-var.patch --]
[-- Type: text/x-patch, Size: 3793 bytes --]

From 1181c103c4d0ee77d518ac9b168ef91adcac4405 Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
To: xen-devel@lists.xenproject.org
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Date: Thu, 19 Dec 2019 10:00:56 +0100
Subject: [PATCH] xen: add a generic way to include binary files as variables

Add a new script xen/tools/binfile for including a binary file at build
time being usable via a pointer and a size variable in the hypervisor.

Make use of that generic tool in xsm.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 .gitignore                   |  1 +
 xen/tools/binfile            | 29 +++++++++++++++++++++++++++++
 xen/xsm/flask/Makefile       |  5 ++++-
 xen/xsm/flask/flask-policy.S | 16 ----------------
 4 files changed, 34 insertions(+), 17 deletions(-)
 create mode 100755 xen/tools/binfile
 delete mode 100644 xen/xsm/flask/flask-policy.S

diff --git a/.gitignore b/.gitignore
index 3ada0c4f0b..6a34db2507 100644
--- a/.gitignore
+++ b/.gitignore
@@ -315,6 +315,7 @@ xen/test/livepatch/xen_replace_world.livepatch
 xen/tools/kconfig/.tmp_gtkcheck
 xen/tools/kconfig/.tmp_qtcheck
 xen/tools/symbols
+xen/xsm/flask/flask-policy.S
 xen/xsm/flask/include/av_perm_to_string.h
 xen/xsm/flask/include/av_permissions.h
 xen/xsm/flask/include/class_to_string.h
diff --git a/xen/tools/binfile b/xen/tools/binfile
new file mode 100755
index 0000000000..122111ff6d
--- /dev/null
+++ b/xen/tools/binfile
@@ -0,0 +1,29 @@
+#!/bin/sh
+# usage: binfile [-i] <target-src.S> <binary-file> <varname>
+# -i     add to .init.rodata (default: .rodata) section
+
+[ "$1" = "-i" ] && {
+    shift
+    section=".init"
+}
+
+target=$1
+binsource=$2
+varname=$3
+
+cat <<EOF >$target
+#include <asm/asm_defns.h>
+
+        .section $section.rodata, "a", %progbits
+
+        .global $varname
+$varname:
+        .incbin "$binsource"
+.Lend:
+
+        .type $varname, %object
+        .size $varname, . - $varname
+
+        .global ${varname}_size
+        ASM_INT(${varname}_size, .Lend - $varname)
+EOF
diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
index 7c3f381287..a807521235 100644
--- a/xen/xsm/flask/Makefile
+++ b/xen/xsm/flask/Makefile
@@ -30,6 +30,9 @@ $(AV_H_FILES): $(AV_H_DEPEND)
 obj-bin-$(CONFIG_XSM_FLASK_POLICY) += flask-policy.o
 flask-policy.o: policy.bin
 
+flask-policy.S: $(XEN_ROOT)/xen/tools/binfile
+	$(XEN_ROOT)/xen/tools/binfile -i $@ policy.bin xsm_flask_init_policy
+
 FLASK_BUILD_DIR := $(CURDIR)
 POLICY_SRC := $(FLASK_BUILD_DIR)/xenpolicy-$(XEN_FULLVERSION)
 
@@ -39,4 +42,4 @@ policy.bin: FORCE
 
 .PHONY: clean
 clean::
-	rm -f $(ALL_H_FILES) *.o $(DEPS_RM) policy.* $(POLICY_SRC)
+	rm -f $(ALL_H_FILES) *.o $(DEPS_RM) policy.* $(POLICY_SRC) flask-policy.S
diff --git a/xen/xsm/flask/flask-policy.S b/xen/xsm/flask/flask-policy.S
deleted file mode 100644
index d38aa39964..0000000000
--- a/xen/xsm/flask/flask-policy.S
+++ /dev/null
@@ -1,16 +0,0 @@
-#include <asm/asm_defns.h>
-
-        .section .init.rodata, "a", %progbits
-
-/* const unsigned char xsm_flask_init_policy[] __initconst */
-        .global xsm_flask_init_policy
-xsm_flask_init_policy:
-        .incbin "policy.bin"
-.Lend:
-
-        .type xsm_flask_init_policy, %object
-        .size xsm_flask_init_policy, . - xsm_flask_init_policy
-
-/* const unsigned int __initconst xsm_flask_init_policy_size */
-        .global xsm_flask_init_policy_size
-        ASM_INT(xsm_flask_init_policy_size, .Lend - xsm_flask_init_policy)
-- 
2.16.4


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

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

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

[Jan Beulich]

On 20.12.2019 11:34, Jürgen Groß wrote:
> On 20.12.19 11:12, Jan Beulich wrote:
>> On 19.12.2019 23:11, Eslam Elnikety wrote:
>>> On 18.12.19 13:42, Jan Beulich wrote:
>>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> +
>>>>> +Xen can bundle microcode updates within its image. This support is conditional
>>>>> +on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is
>>>>> +useful to ensure that, by default, a minimum microcode patch level will be
>>>>> +applied to the underlying CPU.
>>>>> +
>>>>> +To use microcode updates available on the build system as builtin,
>>>>> +use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates
>>>>> +and specify the individual microcode patches via either BUILTIN_UCODE_AMD or
>>>>> +BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For
>>>>> +instance, the configuration below is suitable for a build system which has a
>>>>> +``/lib/firmware/`` directory which, in turn, includes the individual microcode
>>>>> +patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and
>>>>> +``intel-ucode/06-2f-02``.
>>>>> +
>>>>> +  CONFIG_BUILTIN_UCODE=y
>>>>> +  CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/"
>>>>> +  CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
>>>>> +  CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02"
>>>>
>>>> Rather than a blank as separator, the more conventional one on
>>>> Unix and alike would be : I think. Of course ideally there wouldn't
>>>> be any restriction at all on the characters usable here for file
>>>> names.
>>>>
>>>
>>> It would be great if there is a particular convention. The blank
>>> separator is aligned with Linux way of doing builtin microcode.
>>
>> Well, this is then another area where I would question whether we
>> really want to follow the Linux approach, but I'm not bothered
>> enough to make less non-conventional behavior here a requirement.
>>
>>>>> --- a/xen/arch/x86/Kconfig
>>>>> +++ b/xen/arch/x86/Kconfig
>>>>> @@ -218,6 +218,36 @@ config MEM_SHARING
>>>>>    	bool "Xen memory sharing support" if EXPERT = "y"
>>>>>    	depends on HVM
>>>>>    
>>>>> +config BUILTIN_UCODE
>>>>> +	bool "Support for Builtin Microcode"
>>>>> +	---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.
>>>>
>>>> I continue to be unconvinced that this separate option is needed.
>>>> Albeit compared to the v1 approach I will agree that handling
>>>> would become more complicated without.
>>>
>>> Any particular preference between the v1 vs v2 approach?
>>
>> I definitely like the vendor separation.
>>
>>>>> @@ -701,7 +747,13 @@ static int __init microcode_init(void)
>>>>>         */
>>>>>        if ( ucode_blob.size )
>>>>>        {
>>>>> +#ifdef CONFIG_BUILTIN_UCODE
>>>>> +        /* No need to destroy module mappings if builtin was used */
>>>>> +        if ( !ucode_builtin )
>>>>> +            bootstrap_map(NULL);
>>>>> +#else
>>>>>            bootstrap_map(NULL);
>>>>> +#endif
>>>>
>>>> First of all - is there no ucode unrelated side effect of this
>>>> invocation? I.e. can it safely be skipped?
>>>
>>> Maybe I am missing something. Are you asking if we can safely skip the
>>> bootstrap_map(NULL)? (Quoting your response on PATCH v2 2/4 "And of
>>> course we really want these mappings to be gone")
>>
>> Yes - my point is that invoking the function here may in
>> principle cover for other mappings. However - this is the
>> invocation you've added in an earlier patch, isn't it? In
>> which case omitting it should be fine. Nevertheless I don't
>> see and harm in invoking the function, i.e. I'd rather keep
>> the code here simple.
>>
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/x86/microcode/Makefile
>>>>> @@ -0,0 +1,46 @@
>>>>> +# 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.
>>>>> +
>>>>> +# Remove quotes and excess spaces from configuration strings
>>>>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
>>>>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
>>>>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
>>>>> +
>>>>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs.
>>>>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
>>>>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
>>>>> +
>>>>> +ifneq ($(amd-blobs),)
>>>>> +obj-y += ucode_amd.o
>>>>> +endif
>>>>> +
>>>>> +ifneq ($(intel-blobs),)
>>>>> +obj-y += ucode_intel.o
>>>>> +endif
>>>>> +
>>>>> +ifeq ($(amd-blobs)$(intel-blobs),)
>>>>> +obj-y += ucode_dummy.o
>>>>> +endif
>>>>> +
>>>>> +ucode_amd.o: Makefile $(amd-blobs)
>>>>> +	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 $@
>>>>> +	rm -f $@.bin
>>>>> +
>>>>> +ucode_intel.o: Makefile $(intel-blobs)
>>>>> +	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 $@
>>>>> +	rm -f $@.bin
>>>>
>>>> This can be had with a pattern rule (with the vendor being the stem)
>>>> and hence without duplication, I think.
>>>>
>>>> Also - is simply concatenating the blobs reliable enough? There's no
>>>> build time diagnostic that the result would actually be understood
>>>> at runtime.
>>>>
>>>
>>> Concatenation is reliable (as long as the individual microcode blobs are
>>> not malformed, and in that case the builtin is not making matters worse
>>> compared to presenting the malformed update via <integer> | scan).
>>
>> A malformed update found the other way is a bug in the tools
>> constructing the respective images. A malformed built-in
>> update is a bug in the Xen build system. The put the question
>> differently: Is it specified somewhere that the blobs all have
>> to have certain properties, which the straight concatenation
>> relies upon?
>>
>>>>> +ucode_dummy.o: Makefile
>>>>> +	$(CC) $(CFLAGS) -c -x c /dev/null -o $@;
>>>>
>>>> Since the commit message doesn't explain why this is needed, I
>>>> have to ask (I guess we somewhere have a dependency on $(obj-y)
>>>> not being empty).
>>>
>>> Your guess is correct. All sub-directories of xen/arch/x86 are expected
>>> to produce built_in.o. If there are not amd nor intel microcode blobs,
>>> there will be no build dependencies and the build fails preparing the
>>> built_in.o
>>
>> That's rather poor, but it's of course not your task to get this
>> fixed (it shouldn't be very difficult to create an empty
>> built_in.o for an empty $(obj-y)).
>>
>>>> _If_ it is needed, I don't see why you need
>>>> ifeq() around its use. In fact you could have
>>>>
>>>> obj-y := ucode-dummy.o
>>>>
>>>> right at the top of the file.
>>>>
>>>> Furthermore I don't really understand why you need this in the
>>>> first place. While cat won't do what you want with an empty
>>>> argument list, can't you simply prepend / append /dev/null?
>>>>
>>>
>>> To make sure we are on the same page. You are suggesting using
>>> "/dev/null" in case there are no amd/intel ucode to generate the
>>> ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as
>>> input (complains about empty binary).
>>
>> That's again rather poor, this time of the utility - it should be
>> easy enough to produce an object with an empty .data (or whatever
>> it is) section. As above - I'm fine with you keeping the logic
>> then as is, provided you say in the description why it can't be
>> simplified.
> 
> What about using the attached patch for including the binary files?
> 
> I wanted to post that for my hypervisor-fs series, but I think it would
> fit here quite nice.

Why not, if it can be made fit the ucode situation.

Jan

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

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

[Eslam Elnikety]

Picking this up again after the break. Apologies for the delay.

On 20.12.19 10:53, Jan Beulich wrote:
> On 19.12.2019 22:08, Eslam Elnikety wrote:
>> On 18.12.19 12:49, Jan Beulich wrote:
>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>> 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.
>>>
>>> I guess "unspecified effect" in the doc was pretty pointless - such
>>> options have been ignored before; in fact ...
>>>
>>>> With that, Xen can explicitly
>>>> ignore those named options when using EFI.
>>>
>>> ... I don't see things becoming any more explicit (not even parsing
>>> the options was quite explicit to me).
>>>
>>
>> I agree that those options have been ignored so far in the case of EFI.
>> The documentation contradicts that however. The documentation:
>> * says <integer> has unspecified effect.
>> * does not mention anything about scan being ignored.
>>
>> With this patch, it is explicit in code and in documentation that both
>> options are ignored in case of EFI.
> 
> But isn't it rather that ucode=scan could (and hence perhaps should)
> also have its value on EFI?
> 

I do not see "ucode=scan" applicable in anyway in the case of EFI. In 
EFI, there are not "modules" to scan through, but rather the efi config 
points exactly to the microcode blob.

>>>> --- 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.
>>>
>>> It's always stop_machine context, isn't it? I also don't think this
>>> implementation detail belongs here.
>>>
>>
>> Needs a better wording indeed. Let me know if you have particular
>> suggestions, and I will incorporate in v3.
> 
> Just drop everything from "or" onwards?
> 

Ack

>>>> @@ -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;
>>>
>>> Is it guaranteed (now and forever) that the index passed in is
>>> non-zero? You changes to microcode_grab_module() imply so, but
>>> just looking at the call site of the function I can't convince
>>> myself this is the case. _If_ it is (thought to be) guaranteed,
>>> then I think you at least want to ASSERT() here, perhaps with
>>> a comment.
>>>
>>
>> For x86, it seems we have that guarantee (given that we must have a
>> dom0). Right?
> 
> For fully bringing up the system - yes. But there's no reason to
> have a Dom0 if all you're after is testing early Xen boot. There'll
> be a panic() in the case, but there shouldn't be anything breaking
> proper behavior prior to this point.
> 

That's a valid point indeed. v3 will handle index being zero.

>>>>    }
>>>>    
>>>> -/*
>>>> - * 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)
>>>
>>> Any particular reason for the renaming? The function name was quite
>>> fine imo.
>>>
>>
>> To me, "parse_ucode" is a misnomer.
> 
> Well, parse_"ucode= isn't a valid identifier. parse_ucode is what
> results when stripping everything that makes it invalid. I can
> see the ambiguity with parsing actual ucode, but the context here
> makes it pretty clear what the function is about. Personally I'd
> prefer such unnecessary renames to be avoided.

Ack

-- Eslam

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

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

[Eslam Elnikety]

On 20.12.19 11:12, Jan Beulich wrote:
> On 19.12.2019 23:11, Eslam Elnikety wrote:
>> On 18.12.19 13:42, Jan Beulich wrote:
>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>> --- /dev/null
>>>> +++ b/xen/arch/x86/microcode/Makefile
>>>> @@ -0,0 +1,46 @@
>>>> +# 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.
>>>> +
>>>> +# Remove quotes and excess spaces from configuration strings
>>>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
>>>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
>>>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
>>>> +
>>>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs.
>>>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
>>>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
>>>> +
>>>> +ifneq ($(amd-blobs),)
>>>> +obj-y += ucode_amd.o
>>>> +endif
>>>> +
>>>> +ifneq ($(intel-blobs),)
>>>> +obj-y += ucode_intel.o
>>>> +endif
>>>> +
>>>> +ifeq ($(amd-blobs)$(intel-blobs),)
>>>> +obj-y += ucode_dummy.o
>>>> +endif
>>>> +
>>>> +ucode_amd.o: Makefile $(amd-blobs)
>>>> +	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 $@
>>>> +	rm -f $@.bin
>>>> +
>>>> +ucode_intel.o: Makefile $(intel-blobs)
>>>> +	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 $@
>>>> +	rm -f $@.bin
>>>
>>> This can be had with a pattern rule (with the vendor being the stem)
>>> and hence without duplication, I think.
>>>
>>> Also - is simply concatenating the blobs reliable enough? There's no
>>> build time diagnostic that the result would actually be understood
>>> at runtime.
>>>
>>
>> Concatenation is reliable (as long as the individual microcode blobs are
>> not malformed, and in that case the builtin is not making matters worse
>> compared to presenting the malformed update via <integer> | scan).
> 
> A malformed update found the other way is a bug in the tools
> constructing the respective images. A malformed built-in
> update is a bug in the Xen build system. The put the question
> differently: Is it specified somewhere that the blobs all have
> to have certain properties, which the straight concatenation
> relies upon?
> 

Refer to ( https://www.kernel.org/doc/Documentation/x86/microcode.txt ). 
AuthenticAMD.bin and GenuineIntel.bin are both concatenations of 
individual microcode blobs.

>>>> +ucode_dummy.o: Makefile
>>>> +	$(CC) $(CFLAGS) -c -x c /dev/null -o $@;
>>>
>>> Since the commit message doesn't explain why this is needed, I
>>> have to ask (I guess we somewhere have a dependency on $(obj-y)
>>> not being empty).
>>
>> Your guess is correct. All sub-directories of xen/arch/x86 are expected
>> to produce built_in.o. If there are not amd nor intel microcode blobs,
>> there will be no build dependencies and the build fails preparing the
>> built_in.o
> 
> That's rather poor, but it's of course not your task to get this
> fixed (it shouldn't be very difficult to create an empty
> built_in.o for an empty $(obj-y)).
> 
>>> _If_ it is needed, I don't see why you need
>>> ifeq() around its use. In fact you could have
>>>
>>> obj-y := ucode-dummy.o
>>>
>>> right at the top of the file.
>>>
>>> Furthermore I don't really understand why you need this in the
>>> first place. While cat won't do what you want with an empty
>>> argument list, can't you simply prepend / append /dev/null?
>>>
>>
>> To make sure we are on the same page. You are suggesting using
>> "/dev/null" in case there are no amd/intel ucode to generate the
>> ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as
>> input (complains about empty binary).
> 
> That's again rather poor, this time of the utility - it should be
> easy enough to produce an object with an empty .data (or whatever
> it is) section. As above - I'm fine with you keeping the logic
> then as is, provided you say in the description why it can't be
> simplified.

Ack. Will justify the logic in comments.

-- Eslam

> 
> Jan
> 


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

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

[Eslam Elnikety]

On 20.12.19 11:34, Jürgen Groß wrote:
> On 20.12.19 11:12, Jan Beulich wrote:
>> On 19.12.2019 23:11, Eslam Elnikety wrote:
>>> On 18.12.19 13:42, Jan Beulich wrote:
>>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/x86/microcode/Makefile
>>>>> @@ -0,0 +1,46 @@
>>>>> +# 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.
>>>>> +
>>>>> +# Remove quotes and excess spaces from configuration strings
>>>>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
>>>>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
>>>>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
>>>>> +
>>>>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for 
>>>>> existing blobs.
>>>>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
>>>>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
>>>>> +
>>>>> +ifneq ($(amd-blobs),)
>>>>> +obj-y += ucode_amd.o
>>>>> +endif
>>>>> +
>>>>> +ifneq ($(intel-blobs),)
>>>>> +obj-y += ucode_intel.o
>>>>> +endif
>>>>> +
>>>>> +ifeq ($(amd-blobs)$(intel-blobs),)
>>>>> +obj-y += ucode_dummy.o
>>>>> +endif
>>>>> +
>>>>> +ucode_amd.o: Makefile $(amd-blobs)
>>>>> +    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 $@
>>>>> +    rm -f $@.bin
>>>>> +
>>>>> +ucode_intel.o: Makefile $(intel-blobs)
>>>>> +    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 $@
>>>>> +    rm -f $@.bin
>>>>
>>>> This can be had with a pattern rule (with the vendor being the stem)
>>>> and hence without duplication, I think.
>>>>
>>>> Also - is simply concatenating the blobs reliable enough? There's no
>>>> build time diagnostic that the result would actually be understood
>>>> at runtime.
>>>>
>>>
>>> Concatenation is reliable (as long as the individual microcode blobs are
>>> not malformed, and in that case the builtin is not making matters worse
>>> compared to presenting the malformed update via <integer> | scan).
>>
>> A malformed update found the other way is a bug in the tools
>> constructing the respective images. A malformed built-in
>> update is a bug in the Xen build system. The put the question
>> differently: Is it specified somewhere that the blobs all have
>> to have certain properties, which the straight concatenation
>> relies upon?
>>
>>>>> +ucode_dummy.o: Makefile
>>>>> +    $(CC) $(CFLAGS) -c -x c /dev/null -o $@;
>>>>
>>>> Since the commit message doesn't explain why this is needed, I
>>>> have to ask (I guess we somewhere have a dependency on $(obj-y)
>>>> not being empty).
>>>
>>> Your guess is correct. All sub-directories of xen/arch/x86 are expected
>>> to produce built_in.o. If there are not amd nor intel microcode blobs,
>>> there will be no build dependencies and the build fails preparing the
>>> built_in.o
>>
>> That's rather poor, but it's of course not your task to get this
>> fixed (it shouldn't be very difficult to create an empty
>> built_in.o for an empty $(obj-y)).
>>
>>>> _If_ it is needed, I don't see why you need
>>>> ifeq() around its use. In fact you could have
>>>>
>>>> obj-y := ucode-dummy.o
>>>>
>>>> right at the top of the file.
>>>>
>>>> Furthermore I don't really understand why you need this in the
>>>> first place. While cat won't do what you want with an empty
>>>> argument list, can't you simply prepend / append /dev/null?
>>>>
>>>
>>> To make sure we are on the same page. You are suggesting using
>>> "/dev/null" in case there are no amd/intel ucode to generate the
>>> ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as
>>> input (complains about empty binary).
>>
>> That's again rather poor, this time of the utility - it should be
>> easy enough to produce an object with an empty .data (or whatever
>> it is) section. As above - I'm fine with you keeping the logic
>> then as is, provided you say in the description why it can't be
>> simplified.
> 
> What about using the attached patch for including the binary files?
> 
> I wanted to post that for my hypervisor-fs series, but I think it would
> fit here quite nice.

Thanks, Jürgen. That tool is indeed useful on its own right for 
flask/policies. However, it seems to me that creating a built_in.o right 
out of the microcode blobs is simpler and keeps the whole business 
contained within xen/arch/x86/microcode/.

-- Eslam

> 
> 
> Juergen


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

Re: [Xen-devel] [PATCH v2 2/4] x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data

[Andrew Cooper]

On 20/12/2019 09:57, Jan Beulich wrote:
> On 19.12.2019 22:25, Eslam Elnikety wrote:
>> On 18.12.19 13:05, Jan Beulich wrote:
>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>> @@ -725,7 +701,7 @@ static int __init microcode_init(void)
>>>>        */
>>>>       if ( ucode_blob.size )
>>>>       {
>>>> -        xfree(ucode_blob.data);
>>>> +        bootstrap_map(NULL);
>>> As much as I like the change, I wholeheartedly disagree with this
>>> aspect of it: You make it largely unpredictable when the boot
>>> mappings will go away - it becomes entirely dependent upon link
>>> order. And of course we really want these mappings to be gone,
>>> the very latest (I think), by the time we start bringing up APs
>>> (but generally the sooner the better). This is (one of?) the main
>>> reason(s) why it hadn't been done this way to begin with. The
>>> alternative is more complicated (set up a proper, long term
>>> mapping), but it's going to be more clean (including the mapping
>>> then also being suitable to post-boot CPU onlining).
>>>
>> This change is an improvement on the current status. We get to avoid 
>> xmalloc/memcpy in the case of a successful ucode=scan. The problematic 
>> aspect you highlight is anyway there regardless of this patch (ref. to 
>> the "else if ( ucode_mod.mod_end )" in microcode_init).
> Hmm, fair point. I'm not a fan of making a bad situation worse,
> but I think it's acceptable here:
> Acked-by: Jan Beulich <jbeulich@suse.com>

Specifically relevant to this conversation is point 2 of
https://lore.kernel.org/xen-devel/20200109193241.14542-1-andrew.cooper3@citrix.com/
where having dynamic bootmap mappings seems pointless when all we're
doing is mapping RAM below 4G.

~Andrew

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

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

[Jan Beulich]

On 17.01.2020 20:06, Eslam Elnikety wrote:
> On 20.12.19 10:53, Jan Beulich wrote:
>> On 19.12.2019 22:08, Eslam Elnikety wrote:
>>> On 18.12.19 12:49, Jan Beulich wrote:
>>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>>> 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.
>>>>
>>>> I guess "unspecified effect" in the doc was pretty pointless - such
>>>> options have been ignored before; in fact ...
>>>>
>>>>> With that, Xen can explicitly
>>>>> ignore those named options when using EFI.
>>>>
>>>> ... I don't see things becoming any more explicit (not even parsing
>>>> the options was quite explicit to me).
>>>>
>>>
>>> I agree that those options have been ignored so far in the case of EFI.
>>> The documentation contradicts that however. The documentation:
>>> * says <integer> has unspecified effect.
>>> * does not mention anything about scan being ignored.
>>>
>>> With this patch, it is explicit in code and in documentation that both
>>> options are ignored in case of EFI.
>>
>> But isn't it rather that ucode=scan could (and hence perhaps should)
>> also have its value on EFI?
>>
> 
> I do not see "ucode=scan" applicable in anyway in the case of EFI. In 
> EFI, there are not "modules" to scan through, but rather the efi config 
> points exactly to the microcode blob.

What would be wrong with the EFI code to also inspect whatever has been
specified with ramdisk= if there was no ucode= ?

Jan

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

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

[Jan Beulich]

On 17.01.2020 21:06, Eslam Elnikety wrote:
> On 20.12.19 11:12, Jan Beulich wrote:
>> On 19.12.2019 23:11, Eslam Elnikety wrote:
>>> On 18.12.19 13:42, Jan Beulich wrote:
>>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/x86/microcode/Makefile
>>>>> @@ -0,0 +1,46 @@
>>>>> +# 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.
>>>>> +
>>>>> +# Remove quotes and excess spaces from configuration strings
>>>>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
>>>>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
>>>>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
>>>>> +
>>>>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs.
>>>>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
>>>>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
>>>>> +
>>>>> +ifneq ($(amd-blobs),)
>>>>> +obj-y += ucode_amd.o
>>>>> +endif
>>>>> +
>>>>> +ifneq ($(intel-blobs),)
>>>>> +obj-y += ucode_intel.o
>>>>> +endif
>>>>> +
>>>>> +ifeq ($(amd-blobs)$(intel-blobs),)
>>>>> +obj-y += ucode_dummy.o
>>>>> +endif
>>>>> +
>>>>> +ucode_amd.o: Makefile $(amd-blobs)
>>>>> +	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 $@
>>>>> +	rm -f $@.bin
>>>>> +
>>>>> +ucode_intel.o: Makefile $(intel-blobs)
>>>>> +	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 $@
>>>>> +	rm -f $@.bin
>>>>
>>>> This can be had with a pattern rule (with the vendor being the stem)
>>>> and hence without duplication, I think.
>>>>
>>>> Also - is simply concatenating the blobs reliable enough? There's no
>>>> build time diagnostic that the result would actually be understood
>>>> at runtime.
>>>>
>>>
>>> Concatenation is reliable (as long as the individual microcode blobs are
>>> not malformed, and in that case the builtin is not making matters worse
>>> compared to presenting the malformed update via <integer> | scan).
>>
>> A malformed update found the other way is a bug in the tools
>> constructing the respective images. A malformed built-in
>> update is a bug in the Xen build system. The put the question
>> differently: Is it specified somewhere that the blobs all have
>> to have certain properties, which the straight concatenation
>> relies upon?
>>
> 
> Refer to ( https://www.kernel.org/doc/Documentation/x86/microcode.txt ). 
> AuthenticAMD.bin and GenuineIntel.bin are both concatenations of 
> individual microcode blobs.

Well, yes, and from practical observations this is good enough. But
observe e.g. how that paragraph starts with "Here's a crude example
...".

Jan

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

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

[Eslam Elnikety]

On 20.01.20 09:42, Jan Beulich wrote:
> On 17.01.2020 20:06, Eslam Elnikety wrote:
>> On 20.12.19 10:53, Jan Beulich wrote:
>>> On 19.12.2019 22:08, Eslam Elnikety wrote:
>>>> On 18.12.19 12:49, Jan Beulich wrote:
>>>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>>>> 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.
>>>>>
>>>>> I guess "unspecified effect" in the doc was pretty pointless - such
>>>>> options have been ignored before; in fact ...
>>>>>
>>>>>> With that, Xen can explicitly
>>>>>> ignore those named options when using EFI.
>>>>>
>>>>> ... I don't see things becoming any more explicit (not even parsing
>>>>> the options was quite explicit to me).
>>>>>
>>>>
>>>> I agree that those options have been ignored so far in the case of EFI.
>>>> The documentation contradicts that however. The documentation:
>>>> * says <integer> has unspecified effect.
>>>> * does not mention anything about scan being ignored.
>>>>
>>>> With this patch, it is explicit in code and in documentation that both
>>>> options are ignored in case of EFI.
>>>
>>> But isn't it rather that ucode=scan could (and hence perhaps should)
>>> also have its value on EFI?
>>>
>>
>> I do not see "ucode=scan" applicable in anyway in the case of EFI. In
>> EFI, there are not "modules" to scan through, but rather the efi config
>> points exactly to the microcode blob.
> 
> What would be wrong with the EFI code to also inspect whatever has been
> specified with ramdisk= if there was no ucode= ?
> 
> Jan
> 

I see, interesting. This sounds like a legitimate use case indeed. I 
wonder, would I be breaking anything if I simply allow the existing code 
that iterates over modules to kick in when ucode=scan irrespective of 
EFI or legacy boot? Also, it seems to me that the ucode= specified by 
efi.cfg would take precedence over the ucode=scan. Do not you think?

Eslam

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

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

[Jan Beulich]

On 21.01.2020 00:50, Eslam Elnikety wrote:
> On 20.01.20 09:42, Jan Beulich wrote:
>> On 17.01.2020 20:06, Eslam Elnikety wrote:
>>> On 20.12.19 10:53, Jan Beulich wrote:
>>>> On 19.12.2019 22:08, Eslam Elnikety wrote:
>>>>> On 18.12.19 12:49, Jan Beulich wrote:
>>>>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>>>>> 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.
>>>>>>
>>>>>> I guess "unspecified effect" in the doc was pretty pointless - such
>>>>>> options have been ignored before; in fact ...
>>>>>>
>>>>>>> With that, Xen can explicitly
>>>>>>> ignore those named options when using EFI.
>>>>>>
>>>>>> ... I don't see things becoming any more explicit (not even parsing
>>>>>> the options was quite explicit to me).
>>>>>>
>>>>>
>>>>> I agree that those options have been ignored so far in the case of EFI.
>>>>> The documentation contradicts that however. The documentation:
>>>>> * says <integer> has unspecified effect.
>>>>> * does not mention anything about scan being ignored.
>>>>>
>>>>> With this patch, it is explicit in code and in documentation that both
>>>>> options are ignored in case of EFI.
>>>>
>>>> But isn't it rather that ucode=scan could (and hence perhaps should)
>>>> also have its value on EFI?
>>>>
>>>
>>> I do not see "ucode=scan" applicable in anyway in the case of EFI. In
>>> EFI, there are not "modules" to scan through, but rather the efi config
>>> points exactly to the microcode blob.
>>
>> What would be wrong with the EFI code to also inspect whatever has been
>> specified with ramdisk= if there was no ucode= ?
> 
> I see, interesting. This sounds like a legitimate use case indeed. I 
> wonder, would I be breaking anything if I simply allow the existing code 
> that iterates over modules to kick in when ucode=scan irrespective of 
> EFI or legacy boot?

I don't think so, no, but it would need double checking (and
mentioning in the description and/or documentation).

> Also, it seems to me that the ucode= specified by 
> efi.cfg would take precedence over the ucode=scan. Do not you think?

I guess we need to settle on what we want to take precedence and
then make sure code also behaves this way. But yes, I think ucode=
from the .cfg should supersede ucode=scan on the command line. A
possibly useful adjustment to this might be to distinguish whether
the ucode=scan was in a specific .cfg section while the ucode= was
in [global] (i.e. sort of a default), in which case maybe the
ucode=scan should win. Thoughts?

Jan

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

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

[Eslam Elnikety]

On 21.01.20 10:27, Jan Beulich wrote:
> On 21.01.2020 00:50, Eslam Elnikety wrote:
>> On 20.01.20 09:42, Jan Beulich wrote:
>>> On 17.01.2020 20:06, Eslam Elnikety wrote:
>>>> On 20.12.19 10:53, Jan Beulich wrote:
>>>>> On 19.12.2019 22:08, Eslam Elnikety wrote:
>>>>>> On 18.12.19 12:49, Jan Beulich wrote:
>>>>>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>>>>>> 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.
>>>>>>>
>>>>>>> I guess "unspecified effect" in the doc was pretty pointless - such
>>>>>>> options have been ignored before; in fact ...
>>>>>>>
>>>>>>>> With that, Xen can explicitly
>>>>>>>> ignore those named options when using EFI.
>>>>>>>
>>>>>>> ... I don't see things becoming any more explicit (not even parsing
>>>>>>> the options was quite explicit to me).
>>>>>>>
>>>>>>
>>>>>> I agree that those options have been ignored so far in the case of EFI.
>>>>>> The documentation contradicts that however. The documentation:
>>>>>> * says <integer> has unspecified effect.
>>>>>> * does not mention anything about scan being ignored.
>>>>>>
>>>>>> With this patch, it is explicit in code and in documentation that both
>>>>>> options are ignored in case of EFI.
>>>>>
>>>>> But isn't it rather that ucode=scan could (and hence perhaps should)
>>>>> also have its value on EFI?
>>>>>
>>>>
>>>> I do not see "ucode=scan" applicable in anyway in the case of EFI. In
>>>> EFI, there are not "modules" to scan through, but rather the efi config
>>>> points exactly to the microcode blob.
>>>
>>> What would be wrong with the EFI code to also inspect whatever has been
>>> specified with ramdisk= if there was no ucode= ?
>>
>> I see, interesting. This sounds like a legitimate use case indeed. I
>> wonder, would I be breaking anything if I simply allow the existing code
>> that iterates over modules to kick in when ucode=scan irrespective of
>> EFI or legacy boot?
> 
> I don't think so, no, but it would need double checking (and
> mentioning in the description and/or documentation).
> 
>> Also, it seems to me that the ucode= specified by
>> efi.cfg would take precedence over the ucode=scan. Do not you think?
> 
> I guess we need to settle on what we want to take precedence and
> then make sure code also behaves this way. But yes, I think ucode=
> from the .cfg should supersede ucode=scan on the command line. A
> possibly useful adjustment to this might be to distinguish whether
> the ucode=scan was in a specific .cfg section while the ucode= was
> in [global] (i.e. sort of a default), in which case maybe the
> ucode=scan should win. Thoughts?
> 
> Jan
> 

I think any ucode= in the EFI .cfg ought to supersede the ucode=scan. 
The semantics are simpler in this case, rather than having to worry 
about where exactly the ucode= was specified in the EFI .cfg. With that, 
an administrator would default to using ucode=scan on the commandline to 
load the ramdisk microcode, and a ucode= in .cfg would be an explicit 
signal to use different microcode.

Eslam


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

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

[Konrad Rzeszutek Wilk]

On Tue, Jan 21, 2020 at 10:27:47AM +0100, Jan Beulich wrote:
> On 21.01.2020 00:50, Eslam Elnikety wrote:
> > On 20.01.20 09:42, Jan Beulich wrote:
> >> On 17.01.2020 20:06, Eslam Elnikety wrote:
> >>> On 20.12.19 10:53, Jan Beulich wrote:
> >>>> On 19.12.2019 22:08, Eslam Elnikety wrote:
> >>>>> On 18.12.19 12:49, Jan Beulich wrote:
> >>>>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
> >>>>>>> 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.
> >>>>>>
> >>>>>> I guess "unspecified effect" in the doc was pretty pointless - such
> >>>>>> options have been ignored before; in fact ...
> >>>>>>
> >>>>>>> With that, Xen can explicitly
> >>>>>>> ignore those named options when using EFI.
> >>>>>>
> >>>>>> ... I don't see things becoming any more explicit (not even parsing
> >>>>>> the options was quite explicit to me).
> >>>>>>
> >>>>>
> >>>>> I agree that those options have been ignored so far in the case of EFI.
> >>>>> The documentation contradicts that however. The documentation:
> >>>>> * says <integer> has unspecified effect.
> >>>>> * does not mention anything about scan being ignored.
> >>>>>
> >>>>> With this patch, it is explicit in code and in documentation that both
> >>>>> options are ignored in case of EFI.
> >>>>
> >>>> But isn't it rather that ucode=scan could (and hence perhaps should)
> >>>> also have its value on EFI?
> >>>>
> >>>
> >>> I do not see "ucode=scan" applicable in anyway in the case of EFI. In
> >>> EFI, there are not "modules" to scan through, but rather the efi config
> >>> points exactly to the microcode blob.
> >>
> >> What would be wrong with the EFI code to also inspect whatever has been
> >> specified with ramdisk= if there was no ucode= ?
> > 
> > I see, interesting. This sounds like a legitimate use case indeed. I 
> > wonder, would I be breaking anything if I simply allow the existing code 
> > that iterates over modules to kick in when ucode=scan irrespective of 
> > EFI or legacy boot?
> 
> I don't think so, no, but it would need double checking (and
> mentioning in the description and/or documentation).

Originally I wanted to have 'ucode=scan' be the default choice. That
would have been the easiest choice.
 

> 
> > Also, it seems to me that the ucode= specified by 
> > efi.cfg would take precedence over the ucode=scan. Do not you think?
> 
> I guess we need to settle on what we want to take precedence and
> then make sure code also behaves this way. But yes, I think ucode=
> from the .cfg should supersede ucode=scan on the command line. A
> possibly useful adjustment to this might be to distinguish whether
> the ucode=scan was in a specific .cfg section while the ucode= was
> in [global] (i.e. sort of a default), in which case maybe the
> ucode=scan should win. Thoughts?
> 
> Jan

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

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

[Eslam Elnikety]

On 21.01.20 21:51, Eslam Elnikety wrote:
> On 21.01.20 10:27, Jan Beulich wrote:
>> On 21.01.2020 00:50, Eslam Elnikety wrote:
>>> On 20.01.20 09:42, Jan Beulich wrote:
>>>> On 17.01.2020 20:06, Eslam Elnikety wrote:
>>>>> On 20.12.19 10:53, Jan Beulich wrote:
>>>>>> On 19.12.2019 22:08, Eslam Elnikety wrote:
>>>>>>> On 18.12.19 12:49, Jan Beulich wrote:
>>>>>>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> I guess "unspecified effect" in the doc was pretty pointless - such
>>>>>>>> options have been ignored before; in fact ...
>>>>>>>>
>>>>>>>>> With that, Xen can explicitly
>>>>>>>>> ignore those named options when using EFI.
>>>>>>>>
>>>>>>>> ... I don't see things becoming any more explicit (not even parsing
>>>>>>>> the options was quite explicit to me).
>>>>>>>>
>>>>>>>
>>>>>>> I agree that those options have been ignored so far in the case 
>>>>>>> of EFI.
>>>>>>> The documentation contradicts that however. The documentation:
>>>>>>> * says <integer> has unspecified effect.
>>>>>>> * does not mention anything about scan being ignored.
>>>>>>>
>>>>>>> With this patch, it is explicit in code and in documentation that 
>>>>>>> both
>>>>>>> options are ignored in case of EFI.
>>>>>>
>>>>>> But isn't it rather that ucode=scan could (and hence perhaps should)
>>>>>> also have its value on EFI?
>>>>>>
>>>>>
>>>>> I do not see "ucode=scan" applicable in anyway in the case of EFI. In
>>>>> EFI, there are not "modules" to scan through, but rather the efi 
>>>>> config
>>>>> points exactly to the microcode blob.
>>>>
>>>> What would be wrong with the EFI code to also inspect whatever has been
>>>> specified with ramdisk= if there was no ucode= ?
>>>
>>> I see, interesting. This sounds like a legitimate use case indeed. I
>>> wonder, would I be breaking anything if I simply allow the existing code
>>> that iterates over modules to kick in when ucode=scan irrespective of
>>> EFI or legacy boot?
>>
>> I don't think so, no, but it would need double checking (and
>> mentioning in the description and/or documentation).
>>
>>> Also, it seems to me that the ucode= specified by
>>> efi.cfg would take precedence over the ucode=scan. Do not you think?
>>
>> I guess we need to settle on what we want to take precedence and
>> then make sure code also behaves this way. But yes, I think ucode=
>> from the .cfg should supersede ucode=scan on the command line. A
>> possibly useful adjustment to this might be to distinguish whether
>> the ucode=scan was in a specific .cfg section while the ucode= was
>> in [global] (i.e. sort of a default), in which case maybe the
>> ucode=scan should win. Thoughts?
>>
>> Jan
>>
> 
> I think any ucode= in the EFI .cfg ought to supersede the ucode=scan. 
> The semantics are simpler in this case, rather than having to worry 
> about where exactly the ucode= was specified in the EFI .cfg. With that, 
> an administrator would default to using ucode=scan on the commandline to 
> load the ramdisk microcode, and a ucode= in .cfg would be an explicit 
> signal to use different microcode.
> 
> Eslam
> 

So that happens to be the existing behaviour already :)

I was under the impression that ucode=scan was simply ignored under EFI. 
That's not the case. It is only ignored if ucode=<filename> is specified 
in the EFI config. In other words, what we had just discussed above is 
already the case. This clearly needs spelling out in the documentation, 
which is the first patch in the "x86/microcode: Improve documentation 
and code" series I have sent just now.

Cheers,
Eslam






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