[PATCH v2 0/3] x86/ucode: Simplify/fix loading paths further

[PATCH v2 0/3] x86/ucode: Simplify/fix loading paths further

[Andrew Cooper]

Remains of the previous patches, posted as a single series.

Andrew Cooper (3):
  x86/ucode: Remove the collect_cpu_info() call from parse_blob()
  x86/ucode: Fix cache handling in microcode_update_helper()
  x86/ucode: Drop MIS_UCODE and microcode_match_result

 xen/arch/x86/cpu/microcode/amd.c     | 10 ++--
 xen/arch/x86/cpu/microcode/core.c    | 69 ++++++++++------------------
 xen/arch/x86/cpu/microcode/intel.c   |  9 ++--
 xen/arch/x86/cpu/microcode/private.h | 21 ++++-----
 4 files changed, 42 insertions(+), 67 deletions(-)

-- 
2.39.5

[PATCH v2 1/3] x86/ucode: Remove the collect_cpu_info() call from parse_blob()

[Andrew Cooper]

With the tangle of logic starting to come under control, it is now plain to
see that parse_blob()'s side effect of re-gathering the signature/revision is
pointless.

The cpu_request_microcode() hooks need the signature only.  The BSP gathers
this in early_microcode_init(), the APs and S3 in microcode_update_cpu().  For
good measure, the apply_microcode() hooks also keep the revision correct as
load attempts are made.

This finally gets us down to a single call per CPU on boot / S3 resume, and no
calls during late-load hypercalls.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index fd4b08b45388..5897ec54032a 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -189,8 +189,6 @@ static struct patch_with_flags nmi_patch =
  */
 static struct microcode_patch *parse_blob(const char *buf, size_t len)
 {
-    alternative_vcall(ucode_ops.collect_cpu_info);
-
     return alternative_call(ucode_ops.cpu_request_microcode, buf, len, true);
 }
 
-- 
2.39.5

[PATCH v2 2/3] x86/ucode: Fix cache handling in microcode_update_helper()

[Andrew Cooper]

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
---
 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 5897ec54032a..0cc5daa251e2 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;
 }
 
-- 
2.39.5

[PATCH v2 3/3] x86/ucode: Drop MIS_UCODE and microcode_match_result

[Andrew Cooper]

All uses of MIS_UCODE, have been removed, leaving only a simple ordering
relation, and microcode_match_result being a stale name.

Drop the enum entirely, and use a simple int -1/0/1 scheme like other standard
ordering primitives in C.

Swap the order or parameters to compare_patch(), to reduce cognitive
complexity; all other logic operates the other way around.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

I don't particular like keeping "result" as a variable name, but nothing
better comes to mind.
---
 xen/arch/x86/cpu/microcode/amd.c     | 10 ++++------
 xen/arch/x86/cpu/microcode/core.c    |  5 ++---
 xen/arch/x86/cpu/microcode/intel.c   |  9 ++++-----
 xen/arch/x86/cpu/microcode/private.h | 21 ++++++++++-----------
 4 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 3861fec6565a..366c8c59e93a 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -170,8 +170,7 @@ static bool check_final_patch_levels(const struct cpu_signature *sig)
     return false;
 }
 
-static enum microcode_match_result compare_revisions(
-    uint32_t old_rev, uint32_t new_rev)
+static int compare_revisions(uint32_t old_rev, uint32_t new_rev)
 {
     if ( new_rev > old_rev )
         return NEW_UCODE;
@@ -199,8 +198,8 @@ static bool microcode_fits_cpu(const struct microcode_patch *patch)
     return equiv.id == patch->processor_rev_id;
 }
 
-static enum microcode_match_result cf_check compare_patch(
-    const struct microcode_patch *new, const struct microcode_patch *old)
+static int cf_check compare_patch(
+    const struct microcode_patch *old, const struct microcode_patch *new)
 {
     /* Both patches to compare are supposed to be applicable to local CPU. */
     ASSERT(microcode_fits_cpu(new));
@@ -212,11 +211,10 @@ static enum microcode_match_result cf_check compare_patch(
 static int cf_check apply_microcode(const struct microcode_patch *patch,
                                     unsigned int flags)
 {
-    int hw_err;
+    int hw_err, result;
     unsigned int cpu = smp_processor_id();
     struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
     uint32_t rev, old_rev = sig->rev;
-    enum microcode_match_result result;
     bool ucode_force = flags & XENPF_UCODE_FORCE;
 
     if ( !microcode_fits_cpu(patch) )
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 0cc5daa251e2..05d0d68d8158 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -470,8 +470,7 @@ struct ucode_buf {
 static long cf_check microcode_update_helper(void *data)
 {
     struct microcode_patch *patch = NULL;
-    enum microcode_match_result result;
-    int ret;
+    int ret, result;
     struct ucode_buf *buffer = data;
     unsigned int cpu, updated;
     struct patch_with_flags patch_with_flags;
@@ -527,7 +526,7 @@ static long cf_check microcode_update_helper(void *data)
     spin_lock(&microcode_mutex);
     if ( microcode_cache )
     {
-        result = alternative_call(ucode_ops.compare_patch, patch, microcode_cache);
+        result = alternative_call(ucode_ops.compare_patch, microcode_cache, patch);
 
         if ( result != NEW_UCODE &&
              !(ucode_force && (result == OLD_UCODE || result == SAME_UCODE)) )
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 3f37792ab4b5..9616a5e9db4b 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -229,8 +229,7 @@ static int microcode_sanity_check(const struct microcode_patch *patch)
  * Production microcode has a positive revision.  Pre-production microcode has
  * a negative revision.
  */
-static enum microcode_match_result compare_revisions(
-    int32_t old_rev, int32_t new_rev)
+static int compare_revisions(int32_t old_rev, int32_t new_rev)
 {
     if ( new_rev > old_rev )
         return NEW_UCODE;
@@ -270,8 +269,8 @@ static bool microcode_fits_cpu(const struct microcode_patch *mc)
     return false;
 }
 
-static enum microcode_match_result cf_check compare_patch(
-    const struct microcode_patch *new, const struct microcode_patch *old)
+static int cf_check compare_patch(
+    const struct microcode_patch *old, const struct microcode_patch *new)
 {
     /*
      * Both patches to compare are supposed to be applicable to local CPU.
@@ -290,7 +289,7 @@ static int cf_check apply_microcode(const struct microcode_patch *patch,
     unsigned int cpu = smp_processor_id();
     struct cpu_signature *sig = &this_cpu(cpu_sig);
     uint32_t rev, old_rev = sig->rev;
-    enum microcode_match_result result;
+    int result;
     bool ucode_force = flags & XENPF_UCODE_FORCE;
 
     if ( !microcode_fits_cpu(patch) )
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index c9dd8ba066f9..957d4d4293d0 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -5,13 +5,6 @@
 
 #include <asm/microcode.h>
 
-enum microcode_match_result {
-    OLD_UCODE, /* signature matched, but revision id is older */
-    SAME_UCODE, /* signature matched, but revision id is the same */
-    NEW_UCODE, /* signature matched, but revision id is newer */
-    MIS_UCODE, /* signature mismatched */
-};
-
 /* Opaque.  Internals are vendor-specific. */
 struct microcode_patch;
 
@@ -54,11 +47,17 @@ struct microcode_ops {
                            unsigned int flags);
 
     /*
-     * Given two patches, are they both applicable to the current CPU, and is
-     * new a higher revision than old?
+     * Given a current patch, and a proposed new patch, order them based on revision.
+     *
+     * This operation is not necessarily symmetrical.  In some cases, a debug
+     * "new" patch will always considered to be newer, on the expectation that
+     * whomever is using debug patches knows exactly what they're doing.
      */
-    enum microcode_match_result (*compare_patch)(
-        const struct microcode_patch *new, const struct microcode_patch *old);
+#define OLD_UCODE  -1
+#define SAME_UCODE  0
+#define NEW_UCODE   1
+    int (*compare_patch)(const struct microcode_patch *old,
+                         const struct microcode_patch *new);
 
     /*
      * For Linux inird microcode compatibliity.
-- 
2.39.5

Re: [PATCH v2 1/3] x86/ucode: Remove the collect_cpu_info() call from parse_blob()

[Jan Beulich]

On 12.11.2024 22:19, Andrew Cooper wrote:
> With the tangle of logic starting to come under control, it is now plain to
> see that parse_blob()'s side effect of re-gathering the signature/revision is
> pointless.
> 
> The cpu_request_microcode() hooks need the signature only.  The BSP gathers
> this in early_microcode_init(), the APs and S3 in microcode_update_cpu().

That's microcode_update_one() after 502478bc1d9d if I'm not mistaken. In the
course of determining that I'm afraid I also found the first sentence of this
paragraph rather misleading than helpful: While it is true what is being said,
in both cases it is collect_cpu_info() that is being invoked, retrieving both
signature and revision. IOW logic needing the signature only doesn't really
matter here (and the sentence made me hunt for cases where we would read just
the signature, aiming at verifying that leaving the revision field unset
would indeed not be a problem).

>  For
> good measure, the apply_microcode() hooks also keep the revision correct as
> load attempts are made.
> 
> This finally gets us down to a single call per CPU on boot / S3 resume, and no
> calls during late-load hypercalls.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Preferably with the problematic sentence dropped or clarified:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 2/3] x86/ucode: Fix cache handling in microcode_update_helper()

[Jan Beulich]

On 12.11.2024 22:19, Andrew Cooper wrote:
> 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.

I'm sure you're well aware that this in turn is just because of your opposition
to xfree() and alike taking const void *. Pointers needing to be to non-const
just because of eventual freeing is precisely the scenario why freeing (and
unmapping) functions better wouldn't take mutable pointers. Then ...

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

... this imo pretty undesirable change also wouldn't be needed.

Nevertheless, in the interest of not blocking this change over a long-standing
disagreement we have,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 3/3] x86/ucode: Drop MIS_UCODE and microcode_match_result

[Jan Beulich]

On 12.11.2024 22:19, Andrew Cooper wrote:
> @@ -199,8 +198,8 @@ static bool microcode_fits_cpu(const struct microcode_patch *patch)
>      return equiv.id == patch->processor_rev_id;
>  }
>  
> -static enum microcode_match_result cf_check compare_patch(
> -    const struct microcode_patch *new, const struct microcode_patch *old)
> +static int cf_check compare_patch(
> +    const struct microcode_patch *old, const struct microcode_patch *new)
>  {

Let's hope we won't screw up a backport because of this swapping. I'd
like to ask to at least consider renaming at least the functions,
perhaps also the hook pointer, perhaps simply by switching from singular
to plural. This would then also avoid reviewers like me to go hunt for
all uses of the function/hook, in an attempt to make sure none was left
out when converting.

> @@ -54,11 +47,17 @@ struct microcode_ops {
>                             unsigned int flags);
>  
>      /*
> -     * Given two patches, are they both applicable to the current CPU, and is
> -     * new a higher revision than old?
> +     * Given a current patch, and a proposed new patch, order them based on revision.
> +     *
> +     * This operation is not necessarily symmetrical.  In some cases, a debug
> +     * "new" patch will always considered to be newer, on the expectation that
> +     * whomever is using debug patches knows exactly what they're doing.
>       */
> -    enum microcode_match_result (*compare_patch)(
> -        const struct microcode_patch *new, const struct microcode_patch *old);
> +#define OLD_UCODE  -1

Nit: I'm pretty sure Misra wants parentheses here.

Preferably with both (mechanical) adjustments:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 1/3] x86/ucode: Remove the collect_cpu_info() call from parse_blob()

[Andrew Cooper]

On 14/11/2024 11:11 am, Jan Beulich wrote:
> On 12.11.2024 22:19, Andrew Cooper wrote:
>> With the tangle of logic starting to come under control, it is now plain to
>> see that parse_blob()'s side effect of re-gathering the signature/revision is
>> pointless.
>>
>> The cpu_request_microcode() hooks need the signature only.  The BSP gathers
>> this in early_microcode_init(), the APs and S3 in microcode_update_cpu().
> That's microcode_update_one() after 502478bc1d9d if I'm not mistaken.

I wouldn't necessarily say "after".

microcode_update_cpu() has been the way the APs and S3 get this
information for ages, whether its in the function directly, or in an
immediate callee.

>  In the
> course of determining that I'm afraid I also found the first sentence of this
> paragraph rather misleading than helpful:

Do you mean "The cpu_request_microcode() hooks need the signature only" ?


> While it is true what is being said,
> in both cases it is collect_cpu_info() that is being invoked, retrieving both
> signature and revision. IOW logic needing the signature only doesn't really
> matter here (and the sentence made me hunt for cases where we would read just
> the signature, aiming at verifying that leaving the revision field unset
> would indeed not be a problem).

It probably doesn't come as a surprise that I'm intending to rework
collect_cpu_info() entirely.  It's a mess.

The signature and platform flags are invariants for a CPU.  (In fact,
Platform Flags had better be the same for an entire system).  The
revision does change with type, but apply_microcode() keeps it up to date.

Yet we had logic which was throwing the details away and re-gathering
(which is quite expensive) for basically every microcode operation.


What I'm trying to express is "this information is collected once at the
start of day, and kept up to date, so collect_cpu_info() should not be
called under any other circumstance".

Perhaps I should just say that directly?



>>  For
>> good measure, the apply_microcode() hooks also keep the revision correct as
>> load attempts are made.
>>
>> This finally gets us down to a single call per CPU on boot / S3 resume, and no
>> calls during late-load hypercalls.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Preferably with the problematic sentence dropped or clarified:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

Re: [PATCH v2 1/3] x86/ucode: Remove the collect_cpu_info() call from parse_blob()

[Jan Beulich]

On 14.11.2024 16:59, Andrew Cooper wrote:
> On 14/11/2024 11:11 am, Jan Beulich wrote:
>> On 12.11.2024 22:19, Andrew Cooper wrote:
>>> With the tangle of logic starting to come under control, it is now plain to
>>> see that parse_blob()'s side effect of re-gathering the signature/revision is
>>> pointless.
>>>
>>> The cpu_request_microcode() hooks need the signature only.  The BSP gathers
>>> this in early_microcode_init(), the APs and S3 in microcode_update_cpu().
>> That's microcode_update_one() after 502478bc1d9d if I'm not mistaken.
> 
> I wouldn't necessarily say "after".

My point was merely that there's no microcode_update_cpu() anymore, as of
that commit.

> microcode_update_cpu() has been the way the APs and S3 get this
> information for ages, whether its in the function directly, or in an
> immediate callee.
> 
>>  In the
>> course of determining that I'm afraid I also found the first sentence of this
>> paragraph rather misleading than helpful:
> 
> Do you mean "The cpu_request_microcode() hooks need the signature only" ?

Yes.

>> While it is true what is being said,
>> in both cases it is collect_cpu_info() that is being invoked, retrieving both
>> signature and revision. IOW logic needing the signature only doesn't really
>> matter here (and the sentence made me hunt for cases where we would read just
>> the signature, aiming at verifying that leaving the revision field unset
>> would indeed not be a problem).
> 
> It probably doesn't come as a surprise that I'm intending to rework
> collect_cpu_info() entirely.  It's a mess.
> 
> The signature and platform flags are invariants for a CPU.  (In fact,
> Platform Flags had better be the same for an entire system).  The
> revision does change with type, but apply_microcode() keeps it up to date.
> 
> Yet we had logic which was throwing the details away and re-gathering
> (which is quite expensive) for basically every microcode operation.
> 
> 
> What I'm trying to express is "this information is collected once at the
> start of day, and kept up to date, so collect_cpu_info() should not be
> called under any other circumstance".
> 
> Perhaps I should just say that directly?

That may be a good thing, yes. The main point still being though that the
way that 1st sentence in the paragraph was written, it ended up confusing.

Jan

Re: [PATCH v2 3/3] x86/ucode: Drop MIS_UCODE and microcode_match_result

[Andrew Cooper]

On 14/11/2024 11:41 am, Jan Beulich wrote:
> On 12.11.2024 22:19, Andrew Cooper wrote:
>> @@ -199,8 +198,8 @@ static bool microcode_fits_cpu(const struct microcode_patch *patch)
>>      return equiv.id == patch->processor_rev_id;
>>  }
>>  
>> -static enum microcode_match_result cf_check compare_patch(
>> -    const struct microcode_patch *new, const struct microcode_patch *old)
>> +static int cf_check compare_patch(
>> +    const struct microcode_patch *old, const struct microcode_patch *new)
>>  {
> Let's hope we won't screw up a backport because of this swapping.

I wasn't going to start thinking about backports until the code gets
into a better state.

But if backports do happen, it will be all-or-nothing.  This code is far
too tangled.


That said, in this specific case, the only thing that would go wrong is
with Intel debug patches.  Even I've only had a handful of those in the
past 8 years.

> I'd like to ask to at least consider renaming at least the functions,
> perhaps also the hook pointer, perhaps simply by switching from singular
> to plural. This would then also avoid reviewers like me to go hunt for
> all uses of the function/hook, in an attempt to make sure none was left
> out when converting.

In the other series I've paused for a while, I have renamed some hooks
(along with related cleanup), but I'm undecided on this one.

One option is cmp(), or perhaps compare().

But, it occurs to me, another option would be is_newer().  We always
care about the operation one way around.

>
>> @@ -54,11 +47,17 @@ struct microcode_ops {
>>                             unsigned int flags);
>>  
>>      /*
>> -     * Given two patches, are they both applicable to the current CPU, and is
>> -     * new a higher revision than old?
>> +     * Given a current patch, and a proposed new patch, order them based on revision.
>> +     *
>> +     * This operation is not necessarily symmetrical.  In some cases, a debug
>> +     * "new" patch will always considered to be newer, on the expectation that
>> +     * whomever is using debug patches knows exactly what they're doing.
>>       */
>> -    enum microcode_match_result (*compare_patch)(
>> -        const struct microcode_patch *new, const struct microcode_patch *old);
>> +#define OLD_UCODE  -1
> Nit: I'm pretty sure Misra wants parentheses here.

Oh yes, so it does.  Rule 20.7 apparently.  Fine.

> Preferably with both (mechanical) adjustments:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew

Re: [PATCH v2 1/3] x86/ucode: Remove the collect_cpu_info() call from parse_blob()

[Andrew Cooper]

On 14/11/2024 4:15 pm, Jan Beulich wrote:
> On 14.11.2024 16:59, Andrew Cooper wrote:
>> On 14/11/2024 11:11 am, Jan Beulich wrote:
>>> On 12.11.2024 22:19, Andrew Cooper wrote:
>>>> With the tangle of logic starting to come under control, it is now plain to
>>>> see that parse_blob()'s side effect of re-gathering the signature/revision is
>>>> pointless.
>>>>
>>>> The cpu_request_microcode() hooks need the signature only.  The BSP gathers
>>>> this in early_microcode_init(), the APs and S3 in microcode_update_cpu().
>>> That's microcode_update_one() after 502478bc1d9d if I'm not mistaken.
>> I wouldn't necessarily say "after".
> My point was merely that there's no microcode_update_cpu() anymore, as of
> that commit.

Oh.  I totally missed that.

I'll fix it, and make the rest of the paragraph a little bit more direct.

~Andrew

Re: [PATCH v2 3/3] x86/ucode: Drop MIS_UCODE and microcode_match_result

[Jan Beulich]

On 14.11.2024 18:18, Andrew Cooper wrote:
> On 14/11/2024 11:41 am, Jan Beulich wrote:
>> On 12.11.2024 22:19, Andrew Cooper wrote:
>>> @@ -199,8 +198,8 @@ static bool microcode_fits_cpu(const struct microcode_patch *patch)
>>>      return equiv.id == patch->processor_rev_id;
>>>  }
>>>  
>>> -static enum microcode_match_result cf_check compare_patch(
>>> -    const struct microcode_patch *new, const struct microcode_patch *old)
>>> +static int cf_check compare_patch(
>>> +    const struct microcode_patch *old, const struct microcode_patch *new)
>>>  {
>> Let's hope we won't screw up a backport because of this swapping.
> 
> I wasn't going to start thinking about backports until the code gets
> into a better state.
> 
> But if backports do happen, it will be all-or-nothing.  This code is far
> too tangled.

I wasn't so much worrying about backporting of this work (as of now I don't
think it's a candidate), but anything that's yet to come.

> That said, in this specific case, the only thing that would go wrong is
> with Intel debug patches.  Even I've only had a handful of those in the
> past 8 years.

Why would that be? Doing the check the wrong way round would lead to
possible downgrading of ucode, wouldn't it?

>> I'd like to ask to at least consider renaming at least the functions,
>> perhaps also the hook pointer, perhaps simply by switching from singular
>> to plural. This would then also avoid reviewers like me to go hunt for
>> all uses of the function/hook, in an attempt to make sure none was left
>> out when converting.
> 
> In the other series I've paused for a while, I have renamed some hooks
> (along with related cleanup), but I'm undecided on this one.
> 
> One option is cmp(), or perhaps compare().

Either would be fine with me as a hook name. As a function name I'm less
certain this will (remain to) be unambiguous.

> But, it occurs to me, another option would be is_newer().  We always
> care about the operation one way around.

is_newer() doesn't very well lend itself to a tristate return value.

Jan

Re: [PATCH v2 3/3] x86/ucode: Drop MIS_UCODE and microcode_match_result

[Andrew Cooper]

On 15/11/2024 8:02 am, Jan Beulich wrote:
> On 14.11.2024 18:18, Andrew Cooper wrote:
>> On 14/11/2024 11:41 am, Jan Beulich wrote:
>>> On 12.11.2024 22:19, Andrew Cooper wrote:
>>>> @@ -199,8 +198,8 @@ static bool microcode_fits_cpu(const struct microcode_patch *patch)
>>>>      return equiv.id == patch->processor_rev_id;
>>>>  }
>>>>  
>>>> -static enum microcode_match_result cf_check compare_patch(
>>>> -    const struct microcode_patch *new, const struct microcode_patch *old)
>>>> +static int cf_check compare_patch(
>>>> +    const struct microcode_patch *old, const struct microcode_patch *new)
>>>>  {
>>> Let's hope we won't screw up a backport because of this swapping.
>> I wasn't going to start thinking about backports until the code gets
>> into a better state.
>>
>> But if backports do happen, it will be all-or-nothing.  This code is far
>> too tangled.
> I wasn't so much worrying about backporting of this work (as of now I don't
> think it's a candidate), but anything that's yet to come.

This work is towards supporting the Intel Min-Rev header, because it's
already deployed into the world for several releases, and is also what
is likely to drive a wish for backports.

Then there's the Intel uniform loading extensions, which are needed for
GNR/SRF.  If nothing else we need to be able to parse the loading-scope
and not get surprised when cross-core loading happens.  (This already
happens since Sky Lake if SGX is active, and Intel were surprised when I
noticed and asked them about it.)


But mostly, the pre-existing logic is just irrationally complex for
something so simple.  Most of the complexity appears to be because it
was too complex to start with.

>
>> That said, in this specific case, the only thing that would go wrong is
>> with Intel debug patches.  Even I've only had a handful of those in the
>> past 8 years.
> Why would that be? Doing the check the wrong way round would lead to
> possible downgrading of ucode, wouldn't it?

After this patch, there is a singular use of the hook.

It is comparing the hypercall-provided blob to the cached blob, yielding
OLD/SAME/NEW.

Deciding to initiate patching (entering stop_machine() context) is based
on !cached || NEW || --force.

There is another check in apply_microcode() (this is why we needed to
plumb --force down), which will catch an accidental swapping of the two
arguments.

Something that we don't handle properly is that we use "I have a cached
blob" as if it means "the system is at a consistent level", but this is
not true in both directions.  We might have not had anything to cache on
boot (AMD Client platforms in particular), and what we had on boot may
not have levelled a system which was left asymmetric by the BIOS.
>>> I'd like to ask to at least consider renaming at least the functions,
>>> perhaps also the hook pointer, perhaps simply by switching from singular
>>> to plural. This would then also avoid reviewers like me to go hunt for
>>> all uses of the function/hook, in an attempt to make sure none was left
>>> out when converting.
>> In the other series I've paused for a while, I have renamed some hooks
>> (along with related cleanup), but I'm undecided on this one.
>>
>> One option is cmp(), or perhaps compare().
> Either would be fine with me as a hook name. As a function name I'm less
> certain this will (remain to) be unambiguous.
>
>> But, it occurs to me, another option would be is_newer().  We always
>> care about the operation one way around.
> is_newer() doesn't very well lend itself to a tristate return value.

Fine.  I'll just go with compare().  I don't expect this will be the
last time it's edited.

~Andrew

Re: [PATCH v2 2/3] x86/ucode: Fix cache handling in microcode_update_helper()

[Andrew Cooper]

On 14/11/2024 11:26 am, Jan Beulich wrote:
> On 12.11.2024 22:19, Andrew Cooper wrote:
>> 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.
> I'm sure you're well aware that this in turn is just because of your opposition
> to xfree() and alike taking const void *.

My opposition, and the C standards committee, and MISRA to name but a few.

>  Pointers needing to be to non-const
> just because of eventual freeing is precisely the scenario why freeing (and
> unmapping) functions better wouldn't take mutable pointers. Then ...
>
>> --- 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;
> ... this imo pretty undesirable change also wouldn't be needed.
>
> Nevertheless, in the interest of not blocking this change over a long-standing
> disagreement we have,
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thankyou.

~Andrew

Re: [PATCH v2 2/3] x86/ucode: Fix cache handling in microcode_update_helper()

[Jan Beulich]

On 15.11.2024 19:03, Andrew Cooper wrote:
> On 14/11/2024 11:26 am, Jan Beulich wrote:
>> On 12.11.2024 22:19, Andrew Cooper wrote:
>>> 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.
>> I'm sure you're well aware that this in turn is just because of your opposition
>> to xfree() and alike taking const void *.
> 
> My opposition, and the C standards committee, and MISRA to name but a few.

The C standard's committee - yes, if we mean to stick to free()'s properties
to the letter (I don't think they have any idea about unmap-like things). For
Misra though, would you mind educating me where they would have a concern?

Jan