[xen staging] x86/ucode: Fix cache handling in microcode_update_helper()

[patchbot@xxxxxxx]

commit 91afda4a34ae05dd65ff314bc84e0ebbd00032df
Author:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
AuthorDate: Thu Nov 7 22:33:53 2024 +0000
Commit:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CommitDate: Fri Nov 15 18:54:47 2024 +0000

    x86/ucode: Fix cache handling in microcode_update_helper()
    
    microcode_update_cache() now has a single caller, but inlining it shows how
    unnecessarily complicated the logic really is.
    
    Outside of error paths, there is always one microcode patch to free.  Its
    either result of parse_blob(), or it's the old cached value.
    
    In order to fix this, have a local patch pointer (mostly to avoid the
    unnecessary verbosity of patch_with_flags.patch), and always free it at the
    end.  The only error path needing care is the IS_ERR(patch) path, which is
    easy enough to handle.
    
    Also, widen the scope of result.  We only need to call compare_patch() once,
    and the answer is still good later when updating the cache.  In order to
    update the cache, simply SWAP() the patch and the cache pointers, allowing 
the
    singular xfree() at the end to cover both cases.
    
    This also removes all callers microcode_free_patch() which fixes the need to
    cast away const to allow it to compile.  This also removed several 
violations
    of MISRA Rule 11.8 which disallows casting away const.
    
    No functional change.
    
    Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
    Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
---
 xen/arch/x86/cpu/microcode/core.c | 66 ++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 42 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c 
b/xen/arch/x86/cpu/microcode/core.c
index 5897ec5403..0cc5daa251 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -86,7 +86,7 @@ struct patch_with_flags {
 static bool ucode_in_nmi = true;
 
 /* Protected by microcode_mutex */
-static const struct microcode_patch *microcode_cache;
+static struct microcode_patch *microcode_cache;
 
 /*
  * opt_mod_idx and opt_scan have subtle semantics.
@@ -192,33 +192,6 @@ static struct microcode_patch *parse_blob(const char *buf, 
size_t len)
     return alternative_call(ucode_ops.cpu_request_microcode, buf, len, true);
 }
 
-static void microcode_free_patch(const struct microcode_patch *patch)
-{
-    xfree((struct microcode_patch *)patch);
-}
-
-/* Return true if cache gets updated. Otherwise, return false */
-static bool microcode_update_cache(const struct microcode_patch *patch)
-{
-    ASSERT(spin_is_locked(&microcode_mutex));
-
-    if ( !microcode_cache )
-        microcode_cache = patch;
-    else if ( alternative_call(ucode_ops.compare_patch,
-                               patch, microcode_cache) == NEW_UCODE )
-    {
-        microcode_free_patch(microcode_cache);
-        microcode_cache = patch;
-    }
-    else
-    {
-        microcode_free_patch(patch);
-        return false;
-    }
-
-    return true;
-}
-
 /* Returns true if ucode should be loaded on a given cpu */
 static bool is_cpu_primary(unsigned int cpu)
 {
@@ -496,6 +469,8 @@ struct ucode_buf {
 
 static long cf_check microcode_update_helper(void *data)
 {
+    struct microcode_patch *patch = NULL;
+    enum microcode_match_result result;
     int ret;
     struct ucode_buf *buffer = data;
     unsigned int cpu, updated;
@@ -524,17 +499,20 @@ static long cf_check microcode_update_helper(void *data)
         goto put;
     }
 
-    patch_with_flags.patch = parse_blob(buffer->buffer, buffer->len);
+    patch = parse_blob(buffer->buffer, buffer->len);
     patch_with_flags.flags = buffer->flags;
+
     xfree(buffer);
-    if ( IS_ERR(patch_with_flags.patch) )
+
+    if ( IS_ERR(patch) )
     {
-        ret = PTR_ERR(patch_with_flags.patch);
+        ret = PTR_ERR(patch);
+        patch = NULL;
         printk(XENLOG_WARNING "Parsing microcode blob error %d\n", ret);
         goto put;
     }
 
-    if ( !patch_with_flags.patch )
+    if ( !patch )
     {
         printk(XENLOG_WARNING "microcode: couldn't find any matching ucode in "
                               "the provided blob!\n");
@@ -549,10 +527,7 @@ static long cf_check microcode_update_helper(void *data)
     spin_lock(&microcode_mutex);
     if ( microcode_cache )
     {
-        enum microcode_match_result result;
-
-        result = alternative_call(ucode_ops.compare_patch,
-                                  patch_with_flags.patch, microcode_cache);
+        result = alternative_call(ucode_ops.compare_patch, patch, 
microcode_cache);
 
         if ( result != NEW_UCODE &&
              !(ucode_force && (result == OLD_UCODE || result == SAME_UCODE)) )
@@ -561,12 +536,13 @@ static long cf_check microcode_update_helper(void *data)
             printk(XENLOG_WARNING
                    "microcode: couldn't find any newer%s revision in the 
provided blob!\n",
                    ucode_force ? " (or a valid)" : "");
-            microcode_free_patch(patch_with_flags.patch);
             ret = -EEXIST;
 
             goto put;
         }
     }
+    else
+        result = NEW_UCODE;
     spin_unlock(&microcode_mutex);
 
     cpumask_clear(&cpu_callin_map);
@@ -593,14 +569,18 @@ static long cf_check microcode_update_helper(void *data)
      *   this requirement can be relaxed in the future. Right now, this is
      *   conservative and good.
      */
+    patch_with_flags.patch = patch;
     ret = stop_machine_run(do_microcode_update, &patch_with_flags, NR_CPUS);
 
     updated = atomic_read(&cpu_updated);
     if ( updated > 0 )
     {
-        spin_lock(&microcode_mutex);
-        microcode_update_cache(patch_with_flags.patch);
-        spin_unlock(&microcode_mutex);
+        if ( result == NEW_UCODE )
+        {
+            spin_lock(&microcode_mutex);
+            SWAP(patch, microcode_cache);
+            spin_unlock(&microcode_mutex);
+        }
 
         /*
          * Refresh the raw CPU policy, in case the features have changed.
@@ -615,8 +595,6 @@ static long cf_check microcode_update_helper(void *data)
         if ( ctxt_switch_masking )
             alternative_vcall(ctxt_switch_masking, current);
     }
-    else
-        microcode_free_patch(patch_with_flags.patch);
 
     if ( updated && updated != nr_cores )
         printk(XENLOG_ERR "ERROR: Updating microcode succeeded on %u cores and 
failed\n"
@@ -627,6 +605,10 @@ static long cf_check microcode_update_helper(void *data)
 
  put:
     put_cpu_maps();
+
+    /* The parsed blob or old cached value, whichever we're not keeping. */
+    xfree(patch);
+
     return ret;
 }
 
--
generated by git-patchbot for /home/xen/git/xen.git#staging