[PATCH 0/5] x86/ucode: Fixes to bootstrap_map() handling

[PATCH 0/5] x86/ucode: Fixes to bootstrap_map() handling

[Andrew Cooper]

Stumbled on to while trying to fix bugs elsewhere with bootstrap_map().  I
don't know if I'd conciously reaslised how bogus microcode_init() was
previously, but it absolutely can't stay...

Andrew Cooper (5):
  xen/earlycpio: Drop nextoff parameter
  x86/ucode: Fold early_update_cache() into microcode_init_cache()
  x86/ucode: Fold early_microcode_update_cpu() into early_microcode_init()
  x86/ucode: Cache results in microcode_scan_module()
  x86/ucode: Drop ucode_mod and ucode_blob

 xen/arch/x86/cpu/microcode/core.c | 247 +++++++++++++++---------------
 xen/common/earlycpio.c            |   8 +-
 xen/include/xen/earlycpio.h       |   3 +-
 3 files changed, 128 insertions(+), 130 deletions(-)

-- 
2.30.2

[PATCH 1/5] xen/earlycpio: Drop nextoff parameter

[Andrew Cooper]

This is imported from Linux, but the parameter being signed is dubious in the
first place and we're not plausibly going to gain a use for the functionality.
Linux has subsequently made it an optional parameter to avoid forcing callers
to pass a stack variable they don't care about using.

In the unlikely case that we gain a usecase, we can reintroduce it, but in the
meantime simplify the single caller.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/cpu/microcode/core.c | 3 +--
 xen/common/earlycpio.c            | 8 +-------
 xen/include/xen/earlycpio.h       | 3 +--
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index cfa2d5053a52..11c471cc3f83 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -156,7 +156,6 @@ void __init microcode_scan_module(
     uint64_t *_blob_start;
     unsigned long _blob_size;
     struct cpio_data cd;
-    long offset;
     const char *p = NULL;
     int i;
 
@@ -189,7 +188,7 @@ void __init microcode_scan_module(
         }
         cd.data = NULL;
         cd.size = 0;
-        cd = find_cpio_data(p, _blob_start, _blob_size, &offset /* ignore */);
+        cd = find_cpio_data(p, _blob_start, _blob_size);
         if ( cd.data )
         {
             ucode_blob.size = cd.size;
diff --git a/xen/common/earlycpio.c b/xen/common/earlycpio.c
index 4bcf32a51ceb..6c76307c2541 100644
--- a/xen/common/earlycpio.c
+++ b/xen/common/earlycpio.c
@@ -56,10 +56,6 @@ enum cpio_fields {
  * @path:       The directory to search for, including a slash at the end
  * @data:       Pointer to the the cpio archive or a header inside
  * @len:        Remaining length of the cpio based on data pointer
- * @nextoff:    When a matching file is found, this is the offset from the
- *              beginning of the cpio to the beginning of the next file, not the
- *              matching file itself. It can be used to iterate through the cpio
- *              to find all files inside of a directory path.
  *
  * @return:     struct cpio_data containing the address, length and
  *              filename (with the directory path cut off) of the found file.
@@ -68,8 +64,7 @@ enum cpio_fields {
  *              the match returned an empty filename string.
  */
 
-struct cpio_data __init find_cpio_data(const char *path, void *data,
-				       size_t len,  long *nextoff)
+struct cpio_data __init find_cpio_data(const char *path, void *data, size_t len)
 {
 	const size_t cpio_header_len = 8*C_NFIELDS - 2;
 	struct cpio_data cd = { NULL, 0, "" };
@@ -129,7 +124,6 @@ struct cpio_data __init find_cpio_data(const char *path, void *data,
 		if ((ch[C_MODE] & 0170000) == 0100000 &&
 		    ch[C_NAMESIZE] >= mypathsize &&
 		    !memcmp(p, path, mypathsize)) {
-			*nextoff = (long)nptr - (long)data;
 			if (ch[C_NAMESIZE] - mypathsize >= MAX_CPIO_FILE_NAME) {
 				printk(
 				"File %s exceeding MAX_CPIO_FILE_NAME [%d]\n",
diff --git a/xen/include/xen/earlycpio.h b/xen/include/xen/earlycpio.h
index 16d9404d739e..d4992035982d 100644
--- a/xen/include/xen/earlycpio.h
+++ b/xen/include/xen/earlycpio.h
@@ -9,7 +9,6 @@ struct cpio_data {
 	char name[MAX_CPIO_FILE_NAME];
 };
 
-struct cpio_data find_cpio_data(const char *path, void *data, size_t len,
-				long *offset);
+struct cpio_data find_cpio_data(const char *path, void *data, size_t len);
 
 #endif /* _EARLYCPIO_H */
-- 
2.30.2

[PATCH 2/5] x86/ucode: Fold early_update_cache() into microcode_init_cache()

[Andrew Cooper]

It is not valid to retain a bootstrap_map() across returning back to
__start_xen(), but various pointers get stashed across calls.

Begin to address this by folding early_update_cache() into it's single caller,
rearranging the exit path to always invalidate the mapping.

No practical change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/cpu/microcode/core.c | 54 +++++++++++++++++--------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 11c471cc3f83..3d23e3ed7ee4 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -755,47 +755,51 @@ int microcode_update_one(void)
     return microcode_update_cpu(NULL);
 }
 
-static int __init early_update_cache(const void *data, size_t len)
+int __init microcode_init_cache(unsigned long *module_map,
+                                const struct multiboot_info *mbi)
 {
     int rc = 0;
     struct microcode_patch *patch;
+    struct ucode_mod_blob blob = {};
 
-    if ( !data )
-        return -ENOMEM;
+    if ( ucode_scan )
+        /* Need to rescan the modules because they might have been relocated */
+        microcode_scan_module(module_map, mbi);
+
+    if ( ucode_mod.mod_end )
+    {
+        blob.data = bootstrap_map(&ucode_mod);
+        blob.size = ucode_mod.mod_end;
+    }
+    else if ( ucode_blob.size )
+    {
+        blob = ucode_blob;
+    }
 
-    patch = parse_blob(data, len);
+    if ( !blob.data )
+        return 0;
+
+    patch = parse_blob(blob.data, blob.size);
     if ( IS_ERR(patch) )
     {
-        printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
-               PTR_ERR(patch));
-        return PTR_ERR(patch);
+        rc = PTR_ERR(patch);
+        printk(XENLOG_WARNING "Parsing microcode blob error %d\n", rc);
+        goto out;
     }
 
     if ( !patch )
-        return -ENOENT;
+    {
+        rc = -ENOENT;
+        goto out;
+    }
 
     spin_lock(&microcode_mutex);
     rc = microcode_update_cache(patch);
     spin_unlock(&microcode_mutex);
     ASSERT(rc);
 
-    return rc;
-}
-
-int __init microcode_init_cache(unsigned long *module_map,
-                                const struct multiboot_info *mbi)
-{
-    int rc = 0;
-
-    if ( ucode_scan )
-        /* Need to rescan the modules because they might have been relocated */
-        microcode_scan_module(module_map, mbi);
-
-    if ( ucode_mod.mod_end )
-        rc = early_update_cache(bootstrap_map(&ucode_mod),
-                                ucode_mod.mod_end);
-    else if ( ucode_blob.size )
-        rc = early_update_cache(ucode_blob.data, ucode_blob.size);
+ out:
+    bootstrap_map(NULL);
 
     return rc;
 }
-- 
2.30.2

[PATCH 3/5] x86/ucode: Fold early_microcode_update_cpu() into early_microcode_init()

[Andrew Cooper]

It is not valid to retain a bootstrap_map() across returning back to
__start_xen(), but various pointers get stashed across calls.

Begin to address this by folding early_update_cache() into it's single caller,
rearranging the exit path to always invalidate the mapping.

No practical change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/cpu/microcode/core.c | 70 +++++++++++++++----------------
 1 file changed, 33 insertions(+), 37 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 3d23e3ed7ee4..4d2a896fe78d 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -804,45 +804,12 @@ int __init microcode_init_cache(unsigned long *module_map,
     return rc;
 }
 
-/* BSP calls this function to parse ucode blob and then apply an update. */
-static int __init early_microcode_update_cpu(void)
-{
-    const void *data = NULL;
-    size_t len;
-    struct microcode_patch *patch;
-
-    if ( ucode_blob.size )
-    {
-        len = ucode_blob.size;
-        data = ucode_blob.data;
-    }
-    else if ( ucode_mod.mod_end )
-    {
-        len = ucode_mod.mod_end;
-        data = bootstrap_map(&ucode_mod);
-    }
-
-    if ( !data )
-        return -ENOMEM;
-
-    patch = ucode_ops.cpu_request_microcode(data, len, false);
-    if ( IS_ERR(patch) )
-    {
-        printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
-               PTR_ERR(patch));
-        return PTR_ERR(patch);
-    }
-
-    if ( !patch )
-        return -ENOENT;
-
-    return microcode_update_cpu(patch);
-}
-
 int __init early_microcode_init(unsigned long *module_map,
                                 const struct multiboot_info *mbi)
 {
     const struct cpuinfo_x86 *c = &boot_cpu_data;
+    struct microcode_patch *patch;
+    struct ucode_mod_blob blob = {};
     int rc = 0;
 
     switch ( c->x86_vendor )
@@ -868,8 +835,37 @@ int __init early_microcode_init(unsigned long *module_map,
 
     ucode_ops.collect_cpu_info();
 
-    if ( ucode_mod.mod_end || ucode_blob.size )
-        rc = early_microcode_update_cpu();
+    if ( ucode_blob.data )
+    {
+        blob = ucode_blob;
+    }
+    else if ( ucode_mod.mod_end )
+    {
+        blob.data = bootstrap_map(&ucode_mod);
+        blob.size = ucode_mod.mod_end;
+    }
+
+    if ( !blob.data )
+        return 0;
+
+    patch = ucode_ops.cpu_request_microcode(blob.data, blob.size, false);
+    if ( IS_ERR(patch) )
+    {
+        rc = PTR_ERR(patch);
+        printk(XENLOG_WARNING "Parsing microcode blob error %d\n", rc);
+        goto out;
+    }
+
+    if ( !patch )
+    {
+        rc = -ENOENT;
+        goto out;
+    }
+
+    rc = microcode_update_cpu(patch);
+
+ out:
+    bootstrap_map(NULL);
 
     return rc;
 }
-- 
2.30.2

[PATCH 4/5] x86/ucode: Cache results in microcode_scan_module()

[Andrew Cooper]

When microcode_scan_module() is used, it's used twice.

The caching of the bootstrap_map() pointer in ucode_blob.data is buggy for
multiple reasons and is going to be removed.

Right now, the boot flow depends on the second pass over
bootstrap_map()/find_cpio_data() altering ucode_blob.data to use the directmap
alias of the CPIO module, where previously it caches the early boostrap
mapping.

If the scan is successful, it will be successful the second time too, but
there's no point repeating the work.  Cache the module index, offset and size
to short circuit things the second time around.

While rearranging this, reduce the scope of the internals of the loop,
changing the type of _blob_start to void and droping the dead stores into cd.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/cpu/microcode/core.c | 34 ++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 4d2a896fe78d..7d32bc13db6f 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -152,14 +152,30 @@ void __init microcode_scan_module(
     unsigned long *module_map,
     const multiboot_info_t *mbi)
 {
+    static __initdata struct { /* Cached details of a previous successful scan. */
+        unsigned long offset;  /* Offset from the start of the CPIO archive. */
+        unsigned long size;    /* Size of the CPIO file. */
+        unsigned int mod_idx;  /* Which multiboot module the CPIO archive is. */
+    } scan;
+
     module_t *mod = (module_t *)__va(mbi->mods_addr);
-    uint64_t *_blob_start;
-    unsigned long _blob_size;
-    struct cpio_data cd;
     const char *p = NULL;
     int i;
 
     ucode_blob.size = 0;
+
+    if ( scan.mod_idx ) /* Previous scan was successful. */
+    {
+        void *map = bootstrap_map(&mod[scan.mod_idx]);
+
+        if ( !map )
+            return;
+
+        ucode_blob.size = scan.size;
+        ucode_blob.data = map + scan.offset;
+        return;
+    }
+
     if ( !ucode_scan )
         return;
 
@@ -175,6 +191,10 @@ void __init microcode_scan_module(
      */
     for ( i = 1 /* Ignore dom0 kernel */; i < mbi->mods_count; i++ )
     {
+        void *_blob_start;
+        unsigned long _blob_size;
+        struct cpio_data cd;
+
         if ( !test_bit(i, module_map) )
             continue;
 
@@ -186,15 +206,19 @@ void __init microcode_scan_module(
                    i, _blob_size);
             continue;
         }
-        cd.data = NULL;
-        cd.size = 0;
+
         cd = find_cpio_data(p, _blob_start, _blob_size);
         if ( cd.data )
         {
+            scan.mod_idx = i;
+            scan.offset  = cd.data - _blob_start;
+            scan.size    = cd.size;
+
             ucode_blob.size = cd.size;
             ucode_blob.data = cd.data;
             break;
         }
+
         bootstrap_map(NULL);
     }
 }
-- 
2.30.2

[PATCH 5/5] x86/ucode: Drop ucode_mod and ucode_blob

[Andrew Cooper]

These both incorrectly cache bootstrap_map()'d pointers across returns back to
__start_xen().  This is never valid, and such pointers may fault, or point to
something unrelated.

With the refactoring work in the previous patches, they're clearly now just
non-standard function return parameters.

Rename struct ucode_mod_blob to just struct blob for breviy and because
there's nothing really ucode-specific about it.

Introduce find_microcode_blob(), to replace microcode_grab_module(), and
rework microcode_scan_module() so they both return a pointer/size tuple, and
don't cache their return values in global state.

This allows us to remove the microcode_init() initcall, the comments of which
gives the false impression that either of the cached pointers are safe to use.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/cpu/microcode/core.c | 154 +++++++++++++-----------------
 1 file changed, 68 insertions(+), 86 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 7d32bc13db6f..0ae628ba99d4 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -55,7 +55,6 @@
  */
 #define MICROCODE_UPDATE_TIMEOUT_US 1000000
 
-static module_t __initdata ucode_mod;
 static signed int __initdata ucode_mod_idx;
 static bool_t __initdata ucode_mod_forced;
 static unsigned int nr_cores;
@@ -76,18 +75,11 @@ static enum {
     LOADING_EXIT,
 } loading_state;
 
-/*
- * If we scan the initramfs.cpio for the early microcode code
- * and find it, then 'ucode_blob' will contain the pointer
- * and the size of said blob. It is allocated from Xen's heap
- * memory.
- */
-struct ucode_mod_blob {
+struct blob {
     const void *data;
     size_t size;
 };
 
-static struct ucode_mod_blob __initdata ucode_blob;
 /*
  * By default we will NOT parse the multiboot modules to see if there is
  * cpio image with the microcode images.
@@ -148,7 +140,15 @@ static int __init cf_check parse_ucode(const char *s)
 }
 custom_param("ucode", parse_ucode);
 
-void __init microcode_scan_module(
+/*
+ * Scan all unclaimed modules, looking for a decompressed CPIO archive
+ * containing a microcode file according to the Linux early microcode API.
+ *
+ * Always caches the results, positive or negative.
+ *
+ * Returns a ptr/size tupe.  Either NULL/0, or a bootstrap_map()'d region.
+ */
+static struct blob __init microcode_scan_module(
     unsigned long *module_map,
     const multiboot_info_t *mbi)
 {
@@ -159,32 +159,35 @@ void __init microcode_scan_module(
     } scan;
 
     module_t *mod = (module_t *)__va(mbi->mods_addr);
+    struct blob blob = {};
     const char *p = NULL;
     int i;
 
-    ucode_blob.size = 0;
-
     if ( scan.mod_idx ) /* Previous scan was successful. */
     {
         void *map = bootstrap_map(&mod[scan.mod_idx]);
 
         if ( !map )
-            return;
+            return blob;
+
+        blob.size = scan.size;
+        blob.data = map + scan.offset;
 
-        ucode_blob.size = scan.size;
-        ucode_blob.data = map + scan.offset;
-        return;
+        return blob;
     }
 
     if ( !ucode_scan )
-        return;
+        return blob;
+
+    /* Only scan once, whatever the outcome. */
+    ucode_scan = false;
 
     if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
         p = "kernel/x86/microcode/AuthenticAMD.bin";
     else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
         p = "kernel/x86/microcode/GenuineIntel.bin";
     else
-        return;
+        return blob;
 
     /*
      * Try all modules and see whichever could be the microcode blob.
@@ -214,30 +217,15 @@ void __init microcode_scan_module(
             scan.offset  = cd.data - _blob_start;
             scan.size    = cd.size;
 
-            ucode_blob.size = cd.size;
-            ucode_blob.data = cd.data;
+            blob.size = cd.size;
+            blob.data = cd.data;
             break;
         }
 
         bootstrap_map(NULL);
     }
-}
-
-static void __init microcode_grab_module(
-    unsigned long *module_map,
-    const multiboot_info_t *mbi)
-{
-    module_t *mod = (module_t *)__va(mbi->mods_addr);
 
-    if ( ucode_mod_idx < 0 )
-        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 )
-        microcode_scan_module(module_map, mbi);
+    return blob;
 }
 
 static struct microcode_ops __ro_after_init ucode_ops;
@@ -746,28 +734,6 @@ int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
                                      microcode_update_helper, buffer);
 }
 
-static int __init cf_check microcode_init(void)
-{
-    /*
-     * At this point, all CPUs should have updated their microcode
-     * via the early_microcode_* paths so free the microcode blob.
-     */
-    if ( ucode_blob.size )
-    {
-        bootstrap_map(NULL);
-        ucode_blob.size = 0;
-        ucode_blob.data = NULL;
-    }
-    else if ( ucode_mod.mod_end )
-    {
-        bootstrap_map(NULL);
-        ucode_mod.mod_end = 0;
-    }
-
-    return 0;
-}
-__initcall(microcode_init);
-
 /* Load a cached update to current cpu */
 int microcode_update_one(void)
 {
@@ -779,26 +745,53 @@ int microcode_update_one(void)
     return microcode_update_cpu(NULL);
 }
 
+/*
+ * Find microcode within the boot modules provided.
+ *
+ * This is called twice on boot, once very early to find the BSP microcode,
+ * and a second time in order to initalise the cache once we've set up the
+ * heap to use.
+ *
+ * Returns a ptr/size tuple, either NULL/0 or a bootstrap_map()'d region.
+ */
+static struct blob __init find_microcode_blob(
+    unsigned long *module_map, const multiboot_info_t *mbi)
+{
+    struct blob blob = {};
+
+    if ( ucode_mod_idx != 0 )
+    {
+        module_t *mod = __va(mbi->mods_addr);
+
+        /* Adjust to support negative backreferences. */
+        if ( ucode_mod_idx < 0 )
+            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) )
+        {
+            mod += ucode_mod_idx;
+
+            blob.data = bootstrap_map(mod);
+            blob.size = mod->mod_end;
+
+            return blob;
+        }
+
+        /* Still not valid.  Ignore it next time around. */
+        ucode_mod_idx = 0;
+    }
+
+    return microcode_scan_module(module_map, mbi);
+}
+
 int __init microcode_init_cache(unsigned long *module_map,
                                 const struct multiboot_info *mbi)
 {
     int rc = 0;
     struct microcode_patch *patch;
-    struct ucode_mod_blob blob = {};
-
-    if ( ucode_scan )
-        /* Need to rescan the modules because they might have been relocated */
-        microcode_scan_module(module_map, mbi);
-
-    if ( ucode_mod.mod_end )
-    {
-        blob.data = bootstrap_map(&ucode_mod);
-        blob.size = ucode_mod.mod_end;
-    }
-    else if ( ucode_blob.size )
-    {
-        blob = ucode_blob;
-    }
+    struct blob blob = find_microcode_blob(module_map, mbi);
 
     if ( !blob.data )
         return 0;
@@ -833,7 +826,7 @@ int __init early_microcode_init(unsigned long *module_map,
 {
     const struct cpuinfo_x86 *c = &boot_cpu_data;
     struct microcode_patch *patch;
-    struct ucode_mod_blob blob = {};
+    struct blob blob = {};
     int rc = 0;
 
     switch ( c->x86_vendor )
@@ -855,20 +848,9 @@ int __init early_microcode_init(unsigned long *module_map,
         return -ENODEV;
     }
 
-    microcode_grab_module(module_map, mbi);
-
     ucode_ops.collect_cpu_info();
 
-    if ( ucode_blob.data )
-    {
-        blob = ucode_blob;
-    }
-    else if ( ucode_mod.mod_end )
-    {
-        blob.data = bootstrap_map(&ucode_mod);
-        blob.size = ucode_mod.mod_end;
-    }
-
+    blob = find_microcode_blob(module_map, mbi);
     if ( !blob.data )
         return 0;
 
-- 
2.30.2

Re: [PATCH 1/5] xen/earlycpio: Drop nextoff parameter

[Jan Beulich]

On 27.03.2023 21:41, Andrew Cooper wrote:
> This is imported from Linux, but the parameter being signed is dubious in the
> first place and we're not plausibly going to gain a use for the functionality.
> Linux has subsequently made it an optional parameter to avoid forcing callers
> to pass a stack variable they don't care about using.
> 
> In the unlikely case that we gain a usecase, we can reintroduce it, but in the
> meantime simplify the single caller.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 2/5] x86/ucode: Fold early_update_cache() into microcode_init_cache()

[Jan Beulich]

On 27.03.2023 21:41, Andrew Cooper wrote:
> It is not valid to retain a bootstrap_map() across returning back to
> __start_xen(), but various pointers get stashed across calls.

It's error prone, yes, but "not valid" isn't really true imo: As long as
nothing calls bootstrap_map(NULL) all mappings will remain as they are.

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -755,47 +755,51 @@ int microcode_update_one(void)
>      return microcode_update_cpu(NULL);
>  }
>  
> -static int __init early_update_cache(const void *data, size_t len)
> +int __init microcode_init_cache(unsigned long *module_map,
> +                                const struct multiboot_info *mbi)
>  {
>      int rc = 0;
>      struct microcode_patch *patch;
> +    struct ucode_mod_blob blob = {};
>  
> -    if ( !data )
> -        return -ENOMEM;

This is lost afaict. To be in sync with earlier code ) think you want to ...

> +    if ( ucode_scan )
> +        /* Need to rescan the modules because they might have been relocated */
> +        microcode_scan_module(module_map, mbi);
> +
> +    if ( ucode_mod.mod_end )
> +    {
> +        blob.data = bootstrap_map(&ucode_mod);

... check here instead of ...

> +        blob.size = ucode_mod.mod_end;
> +    }
> +    else if ( ucode_blob.size )
> +    {
> +        blob = ucode_blob;
> +    }

(nit: unnecessary braces)

> -    patch = parse_blob(data, len);
> +    if ( !blob.data )
> +        return 0;

... here, making the "return 0" the "else" to the earlier if/else-if.

Alternatively, if you think the -ENOMEM isn't sensible, I'm happy to
consider respective justification for its removal.

Jan

Re: [PATCH 3/5] x86/ucode: Fold early_microcode_update_cpu() into early_microcode_init()

[Jan Beulich]

On 27.03.2023 21:41, Andrew Cooper wrote:
> It is not valid to retain a bootstrap_map() across returning back to
> __start_xen(), but various pointers get stashed across calls.

Same comment here, and ...

> Begin to address this by folding early_update_cache() into it's single caller,
> rearranging the exit path to always invalidate the mapping.

... this looks to lack editing after copy-and-paste from the earlier patch.

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -804,45 +804,12 @@ int __init microcode_init_cache(unsigned long *module_map,
>      return rc;
>  }
>  
> -/* BSP calls this function to parse ucode blob and then apply an update. */
> -static int __init early_microcode_update_cpu(void)
> -{
> -    const void *data = NULL;
> -    size_t len;
> -    struct microcode_patch *patch;
> -
> -    if ( ucode_blob.size )
> -    {
> -        len = ucode_blob.size;
> -        data = ucode_blob.data;
> -    }
> -    else if ( ucode_mod.mod_end )
> -    {
> -        len = ucode_mod.mod_end;
> -        data = bootstrap_map(&ucode_mod);
> -    }
> -
> -    if ( !data )
> -        return -ENOMEM;

Like in the earlier patch this looks to be lost.

> -    patch = ucode_ops.cpu_request_microcode(data, len, false);
> -    if ( IS_ERR(patch) )
> -    {
> -        printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
> -               PTR_ERR(patch));
> -        return PTR_ERR(patch);
> -    }
> -
> -    if ( !patch )
> -        return -ENOENT;
> -
> -    return microcode_update_cpu(patch);
> -}
> -
>  int __init early_microcode_init(unsigned long *module_map,
>                                  const struct multiboot_info *mbi)
>  {
>      const struct cpuinfo_x86 *c = &boot_cpu_data;
> +    struct microcode_patch *patch;
> +    struct ucode_mod_blob blob = {};
>      int rc = 0;
>  
>      switch ( c->x86_vendor )
> @@ -868,8 +835,37 @@ int __init early_microcode_init(unsigned long *module_map,
>  
>      ucode_ops.collect_cpu_info();
>  
> -    if ( ucode_mod.mod_end || ucode_blob.size )
> -        rc = early_microcode_update_cpu();
> +    if ( ucode_blob.data )
> +    {
> +        blob = ucode_blob;
> +    }
> +    else if ( ucode_mod.mod_end )
> +    {
> +        blob.data = bootstrap_map(&ucode_mod);
> +        blob.size = ucode_mod.mod_end;
> +    }

I wonder whether the order of if/else-if being different between
microcode_init_cache() and here (also before your change) is meaningful
in any way. I would prefer if the checking was always done in the same
order, if I can talk you into re-arranging here and/or in the earlier
patch.

Jan

Re: [PATCH 4/5] x86/ucode: Cache results in microcode_scan_module()

[Jan Beulich]

On 27.03.2023 21:41, Andrew Cooper wrote:
> When microcode_scan_module() is used, it's used twice.
> 
> The caching of the bootstrap_map() pointer in ucode_blob.data is buggy for
> multiple reasons and is going to be removed.

As before I'm not convinced of "buggy".

> Right now, the boot flow depends on the second pass over
> bootstrap_map()/find_cpio_data() altering ucode_blob.data to use the directmap
> alias of the CPIO module, where previously it caches the early boostrap
> mapping.
> 
> If the scan is successful, it will be successful the second time too, but
> there's no point repeating the work.  Cache the module index, offset and size
> to short circuit things the second time around.

If the scan failed, it will fail the 2nd time too. Maybe deal with
this case as well, e.g. by clearing ucode_scan at the end of
microcode_scan_module() when nothing was found?

Jan

Re: [PATCH 5/5] x86/ucode: Drop ucode_mod and ucode_blob

[Jan Beulich]

On 27.03.2023 21:41, Andrew Cooper wrote:
> These both incorrectly cache bootstrap_map()'d pointers across returns back to
> __start_xen().  This is never valid, and such pointers may fault, or point to
> something unrelated.

As before - unless bootstrap_map(NULL) was (carefully) not called in
between. Same ...

> With the refactoring work in the previous patches, they're clearly now just
> non-standard function return parameters.
> 
> Rename struct ucode_mod_blob to just struct blob for breviy and because
> there's nothing really ucode-specific about it.
> 
> Introduce find_microcode_blob(), to replace microcode_grab_module(), and
> rework microcode_scan_module() so they both return a pointer/size tuple, and
> don't cache their return values in global state.
> 
> This allows us to remove the microcode_init() initcall, the comments of which
> gives the false impression that either of the cached pointers are safe to use.

... here.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
but I'd appreciate if the description was adjusted to only call the
original code as bad as it really is, not any worse.

Jan

Re: [PATCH 2/5] x86/ucode: Fold early_update_cache() into microcode_init_cache()

[Andrew Cooper]

On 28/03/2023 2:51 pm, Jan Beulich wrote:
> On 27.03.2023 21:41, Andrew Cooper wrote:
>> It is not valid to retain a bootstrap_map() across returning back to
>> __start_xen(), but various pointers get stashed across calls.
> It's error prone, yes, but "not valid" isn't really true imo: As long as
> nothing calls bootstrap_map(NULL) all mappings will remain as they are.

And how is this code supposed to know whether it's stashed pointer is
any good or not?

This is precisely "not valid" means.  It doesn't mean "it definitely
doesn't work", but very much does mean "can't rely on it working".

c/s dc380df12ac which introduced microcode_init_cache() demonstrates the
problem by having to call scan module a second time to refresh the
cached pointer in ucode_blob.data.

The code we have today really is buggy, and is having to go out of its
way to not trip over.

>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -755,47 +755,51 @@ int microcode_update_one(void)
>>      return microcode_update_cpu(NULL);
>>  }
>>  
>> -static int __init early_update_cache(const void *data, size_t len)
>> +int __init microcode_init_cache(unsigned long *module_map,
>> +                                const struct multiboot_info *mbi)
>>  {
>>      int rc = 0;
>>      struct microcode_patch *patch;
>> +    struct ucode_mod_blob blob = {};
>>  
>> -    if ( !data )
>> -        return -ENOMEM;
> This is lost afaict. To be in sync with earlier code ) think you want to ...
>
>> +    if ( ucode_scan )
>> +        /* Need to rescan the modules because they might have been relocated */
>> +        microcode_scan_module(module_map, mbi);
>> +
>> +    if ( ucode_mod.mod_end )
>> +    {
>> +        blob.data = bootstrap_map(&ucode_mod);
> ... check here instead of ...
>
>> +        blob.size = ucode_mod.mod_end;
>> +    }
>> +    else if ( ucode_blob.size )
>> +    {
>> +        blob = ucode_blob;
>> +    }
> (nit: unnecessary braces)
>
>> -    patch = parse_blob(data, len);
>> +    if ( !blob.data )
>> +        return 0;
> ... here, making the "return 0" the "else" to the earlier if/else-if.
>
> Alternatively, if you think the -ENOMEM isn't sensible, I'm happy to
> consider respective justification for its removal.

I'm a little on the fence here.  Nothing checks the return value at all,
and nothing printed a failure previously either.

Right now, the early boostrap map limit is 1G-16M, so this really does
fail with a large enough initrd to scan.  There is a corner case where
the second pass can succeed where the first one failed, but this is also
fixed in the general case when thread 1 loads microcode (and updates the
BSP too.)

I'm also not convinced that early bootstrap mapping is safe if the
bootloader places Xen in [16M, 1G) but if that goes wrong, it will go
wrong before we get to microcode loading.

Overall, I think that bootstrap_map() itself should warn (and ideally
provide a backtrace) if it fails to map, because one way or another it's
an issue wanting fixing.  Individual callers can't really do anything
useful.

~Andrew

Re: [PATCH 3/5] x86/ucode: Fold early_microcode_update_cpu() into early_microcode_init()

[Andrew Cooper]

On 28/03/2023 3:06 pm, Jan Beulich wrote:
> On 27.03.2023 21:41, Andrew Cooper wrote:
>> Begin to address this by folding early_update_cache() into it's single caller,
>> rearranging the exit path to always invalidate the mapping.
> ... this looks to lack editing after copy-and-paste from the earlier patch.

Will fix.

>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -868,8 +835,37 @@ int __init early_microcode_init(unsigned long *module_map,
>>  
>>      ucode_ops.collect_cpu_info();
>>  
>> -    if ( ucode_mod.mod_end || ucode_blob.size )
>> -        rc = early_microcode_update_cpu();
>> +    if ( ucode_blob.data )
>> +    {
>> +        blob = ucode_blob;
>> +    }
>> +    else if ( ucode_mod.mod_end )
>> +    {
>> +        blob.data = bootstrap_map(&ucode_mod);
>> +        blob.size = ucode_mod.mod_end;
>> +    }
> I wonder whether the order of if/else-if being different between
> microcode_init_cache() and here (also before your change) is meaningful
> in any way. I would prefer if the checking was always done in the same
> order, if I can talk you into re-arranging here and/or in the earlier
> patch.

It does matter, yes (well - certainly in patch 2).  (Although I see a
.size -> .data typo in the moved code, which I need to fix.)

However, both these chains are deleted in patch 5, so I'm going to keep
patches 2 and 3 as close to pure code movement as I can.

~Andrew

Re: [PATCH 4/5] x86/ucode: Cache results in microcode_scan_module()

[Andrew Cooper]

On 28/03/2023 3:19 pm, Jan Beulich wrote:
> On 27.03.2023 21:41, Andrew Cooper wrote:
>> Right now, the boot flow depends on the second pass over
>> bootstrap_map()/find_cpio_data() altering ucode_blob.data to use the directmap
>> alias of the CPIO module, where previously it caches the early boostrap
>> mapping.
>>
>> If the scan is successful, it will be successful the second time too, but
>> there's no point repeating the work.  Cache the module index, offset and size
>> to short circuit things the second time around.
> If the scan failed, it will fail the 2nd time too. Maybe deal with
> this case as well, e.g. by clearing ucode_scan at the end of
> microcode_scan_module() when nothing was found?

See patch 5.  It can only become true then because of how the callers
are arranged.

~Andrew

Re: [PATCH 2/5] x86/ucode: Fold early_update_cache() into microcode_init_cache()

[Jan Beulich]

On 28.03.2023 17:07, Andrew Cooper wrote:
> On 28/03/2023 2:51 pm, Jan Beulich wrote:
>> On 27.03.2023 21:41, Andrew Cooper wrote:
>>> It is not valid to retain a bootstrap_map() across returning back to
>>> __start_xen(), but various pointers get stashed across calls.
>> It's error prone, yes, but "not valid" isn't really true imo: As long as
>> nothing calls bootstrap_map(NULL) all mappings will remain as they are.
> 
> And how is this code supposed to know whether it's stashed pointer is
> any good or not?
> 
> This is precisely "not valid" means.  It doesn't mean "it definitely
> doesn't work", but very much does mean "can't rely on it working".

Hmm, not my understanding of "not valid".

> c/s dc380df12ac which introduced microcode_init_cache() demonstrates the
> problem by having to call scan module a second time to refresh the
> cached pointer in ucode_blob.data.
> 
> The code we have today really is buggy, and is having to go out of its
> way to not trip over.

Right - the code is fragile. And it's okay calling it so.

>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>> @@ -755,47 +755,51 @@ int microcode_update_one(void)
>>>      return microcode_update_cpu(NULL);
>>>  }
>>>  
>>> -static int __init early_update_cache(const void *data, size_t len)
>>> +int __init microcode_init_cache(unsigned long *module_map,
>>> +                                const struct multiboot_info *mbi)
>>>  {
>>>      int rc = 0;
>>>      struct microcode_patch *patch;
>>> +    struct ucode_mod_blob blob = {};
>>>  
>>> -    if ( !data )
>>> -        return -ENOMEM;
>> This is lost afaict. To be in sync with earlier code ) think you want to ...
>>
>>> +    if ( ucode_scan )
>>> +        /* Need to rescan the modules because they might have been relocated */
>>> +        microcode_scan_module(module_map, mbi);
>>> +
>>> +    if ( ucode_mod.mod_end )
>>> +    {
>>> +        blob.data = bootstrap_map(&ucode_mod);
>> ... check here instead of ...
>>
>>> +        blob.size = ucode_mod.mod_end;
>>> +    }
>>> +    else if ( ucode_blob.size )
>>> +    {
>>> +        blob = ucode_blob;
>>> +    }
>> (nit: unnecessary braces)
>>
>>> -    patch = parse_blob(data, len);
>>> +    if ( !blob.data )
>>> +        return 0;
>> ... here, making the "return 0" the "else" to the earlier if/else-if.
>>
>> Alternatively, if you think the -ENOMEM isn't sensible, I'm happy to
>> consider respective justification for its removal.
> 
> I'm a little on the fence here.  Nothing checks the return value at all,
> and nothing printed a failure previously either.
> 
> Right now, the early boostrap map limit is 1G-16M, so this really does
> fail with a large enough initrd to scan.  There is a corner case where
> the second pass can succeed where the first one failed, but this is also
> fixed in the general case when thread 1 loads microcode (and updates the
> BSP too.)

As said - with the removal justified in the description (e.g. by an
abridged version of the above), I'm okay.

> I'm also not convinced that early bootstrap mapping is safe if the
> bootloader places Xen in [16M, 1G) but if that goes wrong, it will go
> wrong before we get to microcode loading.

Hmm, yes, this could be a concern on systems with very little memory
below 4G.

Jan

> Overall, I think that bootstrap_map() itself should warn (and ideally
> provide a backtrace) if it fails to map, because one way or another it's
> an issue wanting fixing.  Individual callers can't really do anything
> useful.
> 
> ~Andrew

Re: [PATCH 3/5] x86/ucode: Fold early_microcode_update_cpu() into early_microcode_init()

[Jan Beulich]

On 28.03.2023 17:11, Andrew Cooper wrote:
> On 28/03/2023 3:06 pm, Jan Beulich wrote:
>> On 27.03.2023 21:41, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>> @@ -868,8 +835,37 @@ int __init early_microcode_init(unsigned long *module_map,
>>>  
>>>      ucode_ops.collect_cpu_info();
>>>  
>>> -    if ( ucode_mod.mod_end || ucode_blob.size )
>>> -        rc = early_microcode_update_cpu();
>>> +    if ( ucode_blob.data )
>>> +    {
>>> +        blob = ucode_blob;
>>> +    }
>>> +    else if ( ucode_mod.mod_end )
>>> +    {
>>> +        blob.data = bootstrap_map(&ucode_mod);
>>> +        blob.size = ucode_mod.mod_end;
>>> +    }
>> I wonder whether the order of if/else-if being different between
>> microcode_init_cache() and here (also before your change) is meaningful
>> in any way. I would prefer if the checking was always done in the same
>> order, if I can talk you into re-arranging here and/or in the earlier
>> patch.
> 
> It does matter, yes (well - certainly in patch 2).  (Although I see a
> .size -> .data typo in the moved code, which I need to fix.)
> 
> However, both these chains are deleted in patch 5, so I'm going to keep
> patches 2 and 3 as close to pure code movement as I can.

Right - having seen the last patch, I'm certainly okay with this.

Jan

Re: [PATCH 4/5] x86/ucode: Cache results in microcode_scan_module()

[Jan Beulich]

On 28.03.2023 17:12, Andrew Cooper wrote:
> On 28/03/2023 3:19 pm, Jan Beulich wrote:
>> On 27.03.2023 21:41, Andrew Cooper wrote:
>>> Right now, the boot flow depends on the second pass over
>>> bootstrap_map()/find_cpio_data() altering ucode_blob.data to use the directmap
>>> alias of the CPIO module, where previously it caches the early boostrap
>>> mapping.
>>>
>>> If the scan is successful, it will be successful the second time too, but
>>> there's no point repeating the work.  Cache the module index, offset and size
>>> to short circuit things the second time around.
>> If the scan failed, it will fail the 2nd time too. Maybe deal with
>> this case as well, e.g. by clearing ucode_scan at the end of
>> microcode_scan_module() when nothing was found?
> 
> See patch 5.  It can only become true then because of how the callers
> are arranged.

Right, I've meanwhile seen you do it there. That's fine. Yet I think it
could also be done earlier (and if I'm not mistaken also ahead of all
of the rearrangements you do).

Jan

Re: [PATCH 4/5] x86/ucode: Cache results in microcode_scan_module()

[Andrew Cooper]

On 28/03/2023 5:01 pm, Jan Beulich wrote:
> On 28.03.2023 17:12, Andrew Cooper wrote:
>> On 28/03/2023 3:19 pm, Jan Beulich wrote:
>>> On 27.03.2023 21:41, Andrew Cooper wrote:
>>>> Right now, the boot flow depends on the second pass over
>>>> bootstrap_map()/find_cpio_data() altering ucode_blob.data to use the directmap
>>>> alias of the CPIO module, where previously it caches the early boostrap
>>>> mapping.
>>>>
>>>> If the scan is successful, it will be successful the second time too, but
>>>> there's no point repeating the work.  Cache the module index, offset and size
>>>> to short circuit things the second time around.
>>> If the scan failed, it will fail the 2nd time too. Maybe deal with
>>> this case as well, e.g. by clearing ucode_scan at the end of
>>> microcode_scan_module() when nothing was found?
>> See patch 5.  It can only become true then because of how the callers
>> are arranged.
> Right, I've meanwhile seen you do it there. That's fine. Yet I think it
> could also be done earlier (and if I'm not mistaken also ahead of all
> of the rearrangements you do).

Prior to patch 5, calls into scan are guarded with "if ( ucode_scan )"
as well as there being an "if ( !ucode_scan )" check.

Clobbering ucode_scan prior to patch 5 breaks the second pass to cache
the ucode we loaded on the BSP.

~Andrew

Re: [PATCH 4/5] x86/ucode: Cache results in microcode_scan_module()

[Jan Beulich]

On 28.03.2023 18:07, Andrew Cooper wrote:
> On 28/03/2023 5:01 pm, Jan Beulich wrote:
>> On 28.03.2023 17:12, Andrew Cooper wrote:
>>> On 28/03/2023 3:19 pm, Jan Beulich wrote:
>>>> On 27.03.2023 21:41, Andrew Cooper wrote:
>>>>> Right now, the boot flow depends on the second pass over
>>>>> bootstrap_map()/find_cpio_data() altering ucode_blob.data to use the directmap
>>>>> alias of the CPIO module, where previously it caches the early boostrap
>>>>> mapping.
>>>>>
>>>>> If the scan is successful, it will be successful the second time too, but
>>>>> there's no point repeating the work.  Cache the module index, offset and size
>>>>> to short circuit things the second time around.
>>>> If the scan failed, it will fail the 2nd time too. Maybe deal with
>>>> this case as well, e.g. by clearing ucode_scan at the end of
>>>> microcode_scan_module() when nothing was found?
>>> See patch 5.  It can only become true then because of how the callers
>>> are arranged.
>> Right, I've meanwhile seen you do it there. That's fine. Yet I think it
>> could also be done earlier (and if I'm not mistaken also ahead of all
>> of the rearrangements you do).
> 
> Prior to patch 5, calls into scan are guarded with "if ( ucode_scan )"
> as well as there being an "if ( !ucode_scan )" check.
> 
> Clobbering ucode_scan prior to patch 5 breaks the second pass to cache
> the ucode we loaded on the BSP.

How that? We didn't load anything if scan failed on the first pass, and
clearing ucode_scan if the first pass failed would merely suppress a
fruitless second pass. Of course we cannot clear ucode_scan in the case
that the first pass succeeded, but my earlier suggestion was limited to
the failure case.

The "!ucode_scan" check also is dead code, btw (and hence doesn't matter
here), since neither caller would invoke the function in that case in
the first place.

Jan

Re: [PATCH 2/5] x86/ucode: Fold early_update_cache() into microcode_init_cache()

[Jan Beulich]

On 28.03.2023 17:07, Andrew Cooper wrote:
> On 28/03/2023 2:51 pm, Jan Beulich wrote:
>> On 27.03.2023 21:41, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>> @@ -755,47 +755,51 @@ int microcode_update_one(void)
>>>      return microcode_update_cpu(NULL);
>>>  }
>>>  
>>> -static int __init early_update_cache(const void *data, size_t len)
>>> +int __init microcode_init_cache(unsigned long *module_map,
>>> +                                const struct multiboot_info *mbi)
>>>  {
>>>      int rc = 0;
>>>      struct microcode_patch *patch;
>>> +    struct ucode_mod_blob blob = {};
>>>  
>>> -    if ( !data )
>>> -        return -ENOMEM;
>> This is lost afaict. To be in sync with earlier code ) think you want to ...
>>
>>> +    if ( ucode_scan )
>>> +        /* Need to rescan the modules because they might have been relocated */
>>> +        microcode_scan_module(module_map, mbi);
>>> +
>>> +    if ( ucode_mod.mod_end )
>>> +    {
>>> +        blob.data = bootstrap_map(&ucode_mod);
>> ... check here instead of ...
>>
>>> +        blob.size = ucode_mod.mod_end;
>>> +    }
>>> +    else if ( ucode_blob.size )
>>> +    {
>>> +        blob = ucode_blob;
>>> +    }
>> (nit: unnecessary braces)
>>
>>> -    patch = parse_blob(data, len);
>>> +    if ( !blob.data )
>>> +        return 0;
>> ... here, making the "return 0" the "else" to the earlier if/else-if.
>>
>> Alternatively, if you think the -ENOMEM isn't sensible, I'm happy to
>> consider respective justification for its removal.
> 
> I'm a little on the fence here.  Nothing checks the return value at all,
> and nothing printed a failure previously either.

So how about switching both microcode_init_cache() and, considering
the similar aspect in patch 3, early_microcode_init() to return void?
There's hardly any use the caller can make of the return value; if an
error message was to be logged, it likely would better be logged
closer to the source of the error.

Jan

Re: [PATCH 2/5] x86/ucode: Fold early_update_cache() into microcode_init_cache()

[George Dunlap]

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

On Tue, Mar 28, 2023 at 4:58 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 28.03.2023 17:07, Andrew Cooper wrote:
> > On 28/03/2023 2:51 pm, Jan Beulich wrote:
> >> On 27.03.2023 21:41, Andrew Cooper wrote:
> >>> It is not valid to retain a bootstrap_map() across returning back to
> >>> __start_xen(), but various pointers get stashed across calls.
> >> It's error prone, yes, but "not valid" isn't really true imo: As long as
> >> nothing calls bootstrap_map(NULL) all mappings will remain as they are.
> >
> > And how is this code supposed to know whether it's stashed pointer is
> > any good or not?
> >
> > This is precisely "not valid" means.  It doesn't mean "it definitely
> > doesn't work", but very much does mean "can't rely on it working".
>
> Hmm, not my understanding of "not valid".
>

A "valid" approach or algorithm is one which can be relied on.  If an
approach or algorithm may sometimes work or may sometimes not work, it's
not valid.

That said:

* "Not valid" is kind of vague: it tells you think it's "bad", but not
how.  (Even "racy" or "risky" or "error-prone" are more descriptive,
because it prompts you for the types of problems to think about.) It's
usually better to state exactly what problems might happen if you do X,
rather than simply saying X is "not valid".

* It's usually better to propose specific alternate wording, rather than
arguing about whether a given wording is... er, valid or not.

 -George

[-- Attachment #2: Type: text/html, Size: 2136 bytes --]

Re: [PATCH 2/5] x86/ucode: Fold early_update_cache() into microcode_init_cache()

[Jan Beulich]

On 29.03.2023 11:13, George Dunlap wrote:
> On Tue, Mar 28, 2023 at 4:58 PM Jan Beulich <jbeulich@suse.com> wrote:
> 
>> On 28.03.2023 17:07, Andrew Cooper wrote:
>>> On 28/03/2023 2:51 pm, Jan Beulich wrote:
>>>> On 27.03.2023 21:41, Andrew Cooper wrote:
>>>>> It is not valid to retain a bootstrap_map() across returning back to
>>>>> __start_xen(), but various pointers get stashed across calls.
>>>> It's error prone, yes, but "not valid" isn't really true imo: As long as
>>>> nothing calls bootstrap_map(NULL) all mappings will remain as they are.
>>>
>>> And how is this code supposed to know whether it's stashed pointer is
>>> any good or not?
>>>
>>> This is precisely "not valid" means.  It doesn't mean "it definitely
>>> doesn't work", but very much does mean "can't rely on it working".
>>
>> Hmm, not my understanding of "not valid".
>>
> 
> A "valid" approach or algorithm is one which can be relied on.  If an
> approach or algorithm may sometimes work or may sometimes not work, it's
> not valid.

Interesting. Still not my understanding of "not valid", not the least
because "may work" again depends on further aspects. In the case here,
carefully placed (or avoided) bootstrap_map(NULL) means the logic, which
has served us well for years, does always work - so long as you apply
sufficient care. Over time that "sufficient care" has arguably turned
into "overly much care", and hence Andrew's rework here is definitely an
improvement. All I dislike is to call things worse than they really are.
(Another thing would be if "sufficient care" wasn't applied at some
point, and if as a result we actually had active breakage somewhere.)

> That said:
> 
> * "Not valid" is kind of vague: it tells you think it's "bad", but not
> how.  (Even "racy" or "risky" or "error-prone" are more descriptive,
> because it prompts you for the types of problems to think about.) It's
> usually better to state exactly what problems might happen if you do X,
> rather than simply saying X is "not valid".
> 
> * It's usually better to propose specific alternate wording, rather than
> arguing about whether a given wording is... er, valid or not.

Which I did (without explicitly saying that this is an alternative wording
proposal) by starting with "It's error prone, ...". Similarly (iirc) in
replies elsewhere on this series' thread.

Jan