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

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

[Andrew Cooper]

collect_cpu_info() is fairly expensive and called excessively.  Rationalise
this substantially.

Except I found yet another bug when testing this.  Notes in patch 3.

Andrew Cooper (3):
  x86/ucode: Don't use microcode_update_cpu() in early_microcode_load()
  x86/ucode: Fold microcode_update_cpu() and fix error handling
  x86/ucode: Remove the collect_cpu_info() call from parse_blob()

 xen/arch/x86/cpu/microcode/core.c | 53 +++++++++----------------------
 1 file changed, 15 insertions(+), 38 deletions(-)

-- 
2.39.5

[PATCH 1/3] x86/ucode: Don't use microcode_update_cpu() in early_microcode_load()

[Andrew Cooper]

There are two callers of microcode_update_cpu(), and because one passes NULL
and one doesn't, there are effectively two disjoint pieces of logic wrapped in
a single function.

early_microcode_load()'s use skips all the microcode_cache handling, and is
just a simple patch application.

This skips a redundant collect_cpu_info() call (performed in
early_microcode_init(), marginally earlier), and avoids holding
microcode_mutex when we're not interacting with microcode_cache at all.

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, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 11cd878d1f2e..d9406ec3fd34 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -900,7 +900,7 @@ static int __init early_microcode_load(struct boot_info *bi)
      */
     early_mod_idx = idx;
 
-    rc = microcode_update_cpu(patch, 0);
+    rc = ucode_ops.apply_microcode(patch, 0);
 
  unmap:
     bootstrap_unmap();
-- 
2.39.5

[PATCH 2/3] x86/ucode: Fold microcode_update_cpu() and fix error handling

[Andrew Cooper]

Fold microcode_update_cpu() into its single remaining caller and simplify the
logic by removing the patch != NULL path with microcode_mutex held.

Explain why we bother grabbing the microcode revision even if we can't load
microcode.

Furthermore, delete the -EIO path.  An error updating microcode on AP boot or
S3 resume is certainly bad, but freeing the cache is about the worst possible
action we can take in response; it prevents subsequent APs from taking an
update they might have accepted.

This avoids a double collect_cpu_info() call on each AP.

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 | 49 +++++++++----------------------
 1 file changed, 14 insertions(+), 35 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index d9406ec3fd34..fd4b08b45388 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -263,40 +263,6 @@ static bool cf_check wait_cpu_callout(unsigned int nr)
     return atomic_read(&cpu_out) >= nr;
 }
 
-/*
- * Load a microcode update to current CPU.
- *
- * If no patch is provided, the cached patch will be loaded. Microcode update
- * during APs bringup and CPU resuming falls into this case.
- */
-static int microcode_update_cpu(const struct microcode_patch *patch,
-                                unsigned int flags)
-{
-    int err;
-
-    alternative_vcall(ucode_ops.collect_cpu_info);
-
-    spin_lock(&microcode_mutex);
-    if ( patch )
-        err = alternative_call(ucode_ops.apply_microcode, patch, flags);
-    else if ( microcode_cache )
-    {
-        err = alternative_call(ucode_ops.apply_microcode, microcode_cache,
-                               flags);
-        if ( err == -EIO )
-        {
-            microcode_free_patch(microcode_cache);
-            microcode_cache = NULL;
-        }
-    }
-    else
-        /* No patch to update */
-        err = -ENOENT;
-    spin_unlock(&microcode_mutex);
-
-    return err;
-}
-
 static bool wait_for_state(typeof(loading_state) state)
 {
     typeof(loading_state) cur_state;
@@ -702,13 +668,26 @@ int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
 /* Load a cached update to current cpu */
 int microcode_update_one(void)
 {
+    int rc;
+
+    /*
+     * This path is used for APs and S3 resume.  Read the microcode revision
+     * if possible, even if we can't load microcode.
+     */
     if ( ucode_ops.collect_cpu_info )
         alternative_vcall(ucode_ops.collect_cpu_info);
 
     if ( !ucode_ops.apply_microcode )
         return -EOPNOTSUPP;
 
-    return microcode_update_cpu(NULL, 0);
+    spin_lock(&microcode_mutex);
+    if ( microcode_cache )
+        rc = alternative_call(ucode_ops.apply_microcode, microcode_cache, 0);
+    else
+        rc = -ENOENT;
+    spin_unlock(&microcode_mutex);
+
+    return rc;
 }
 
 /*
-- 
2.39.5

[PATCH 3/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>

Slightly RFC.

Just before posting, I've realised that cpu_request_microcode() does actually
use the current CPU revision, and it's buggy, and it's the cause of `xen-ucode
--force` not working as expected.

I'm tempted to do another series cleaning that up in isolation, such that this
patch becomes true in this form.

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

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

[Andrew Cooper]

On 07/11/2024 12:21 pm, 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().  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>
>
> Slightly RFC.
>
> Just before posting, I've realised that cpu_request_microcode() does actually
> use the current CPU revision, and it's buggy, and it's the cause of `xen-ucode
> --force` not working as expected.
>
> I'm tempted to do another series cleaning that up in isolation, such that this
> patch becomes true in this form.

Actually no.  Having tried a bit, I think it's easier to do with patch 2
already in place.

So instead I'm tempted to edit the middle paragraph to note that it
currently uses the revision but that's going to be fixed shortly.  The
rest of the paragraph explains why it's still safe anyway.

~Andrew

[PATCH 4/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.

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>

This is in prepartion to totally overhaul compare_patch().  There's now only a
single caller.
---
 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

Re: [PATCH 1/3] x86/ucode: Don't use microcode_update_cpu() in early_microcode_load()

[Jan Beulich]

On 07.11.2024 13:21, Andrew Cooper wrote:
> There are two callers of microcode_update_cpu(), and because one passes NULL
> and one doesn't, there are effectively two disjoint pieces of logic wrapped in
> a single function.
> 
> early_microcode_load()'s use skips all the microcode_cache handling, and is
> just a simple patch application.
> 
> This skips a redundant collect_cpu_info() call (performed in
> early_microcode_init(), marginally earlier), and avoids holding
> microcode_mutex when we're not interacting with microcode_cache at all.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Personally I would have deemed it more logical (and Misra-compliant) to
remove the "if ( patch )" case from microcode_update_cpu() right here.
Surely (by its title) the next patch is going to have the same effect.

Jan

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

[Andrew Cooper]

On 07/11/2024 9:58 pm, Andrew Cooper wrote:
> On 07/11/2024 12:21 pm, 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().  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>
>>
>> Slightly RFC.
>>
>> Just before posting, I've realised that cpu_request_microcode() does actually
>> use the current CPU revision, and it's buggy, and it's the cause of `xen-ucode
>> --force` not working as expected.
>>
>> I'm tempted to do another series cleaning that up in isolation, such that this
>> patch becomes true in this form.
> Actually no.  Having tried a bit, I think it's easier to do with patch 2
> already in place.
>
> So instead I'm tempted to edit the middle paragraph to note that it
> currently uses the revision but that's going to be fixed shortly.  The
> rest of the paragraph explains why it's still safe anyway.

So, after the latter series, this patch happens to be accurate.

cpu_request_microcode() does read the revision, but discards the result
of the calculation which used it.

~Andrew

Re: [PATCH 2/3] x86/ucode: Fold microcode_update_cpu() and fix error handling

[Jan Beulich]

On 07.11.2024 13:21, Andrew Cooper wrote:
> Fold microcode_update_cpu() into its single remaining caller and simplify the
> logic by removing the patch != NULL path with microcode_mutex held.
> 
> Explain why we bother grabbing the microcode revision even if we can't load
> microcode.
> 
> Furthermore, delete the -EIO path.  An error updating microcode on AP boot or
> S3 resume is certainly bad, but freeing the cache is about the worst possible
> action we can take in response; it prevents subsequent APs from taking an
> update they might have accepted.

I'm afraid I disagree here, but I also disagree with the present error handling.
-EIO indicates the patch didn't apply. Why would there be any hope that any
other CPU would accept it? We're assuming fully symmetric hardware, after all.
However, imo it's not -EIO that ought to be special cased, but success and
-EEXIST. In all other cases the same error will re-surface for other CPUs. Plus
by not cleaning the cache we prevent an older revision to be installed (without
forcing its installation).

Keeping what's cached might be an option, but then followed by cleaning the
cache unless at least one CPU actually accepted the ucode.

Jan

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

[Jan Beulich]

On 12.11.2024 11:36, Andrew Cooper wrote:
> On 07/11/2024 9:58 pm, Andrew Cooper wrote:
>> On 07/11/2024 12:21 pm, 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().  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>
>>>
>>> Slightly RFC.
>>>
>>> Just before posting, I've realised that cpu_request_microcode() does actually
>>> use the current CPU revision, and it's buggy, and it's the cause of `xen-ucode
>>> --force` not working as expected.
>>>
>>> I'm tempted to do another series cleaning that up in isolation, such that this
>>> patch becomes true in this form.
>> Actually no.  Having tried a bit, I think it's easier to do with patch 2
>> already in place.
>>
>> So instead I'm tempted to edit the middle paragraph to note that it
>> currently uses the revision but that's going to be fixed shortly.  The
>> rest of the paragraph explains why it's still safe anyway.
> 
> So, after the latter series, this patch happens to be accurate.
> 
> cpu_request_microcode() does read the revision, but discards the result
> of the calculation which used it.

What's the intended overall sequence of patches then? With two series that
(aiui) now have grown some sort of dependency, and with this series have
gained a 4/3 patch, having a clear picture would certainly help. Might it
be best if you merge both series and re-submit as a single one?

Jan

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

[Andrew Cooper]

On 12/11/2024 10:49 am, Jan Beulich wrote:
> On 12.11.2024 11:36, Andrew Cooper wrote:
>> On 07/11/2024 9:58 pm, Andrew Cooper wrote:
>>> On 07/11/2024 12:21 pm, 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().  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>
>>>>
>>>> Slightly RFC.
>>>>
>>>> Just before posting, I've realised that cpu_request_microcode() does actually
>>>> use the current CPU revision, and it's buggy, and it's the cause of `xen-ucode
>>>> --force` not working as expected.
>>>>
>>>> I'm tempted to do another series cleaning that up in isolation, such that this
>>>> patch becomes true in this form.
>>> Actually no.  Having tried a bit, I think it's easier to do with patch 2
>>> already in place.
>>>
>>> So instead I'm tempted to edit the middle paragraph to note that it
>>> currently uses the revision but that's going to be fixed shortly.  The
>>> rest of the paragraph explains why it's still safe anyway.
>> So, after the latter series, this patch happens to be accurate.
>>
>> cpu_request_microcode() does read the revision, but discards the result
>> of the calculation which used it.
> What's the intended overall sequence of patches then? With two series that
> (aiui) now have grown some sort of dependency, and with this series have
> gained a 4/3 patch, having a clear picture would certainly help. Might it
> be best if you merge both series and re-submit as a single one?

The order turns out to be as emailed out and threaded.

~Andrew

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

[Jan Beulich]

On 12.11.2024 11:57, Andrew Cooper wrote:
> On 12/11/2024 10:49 am, Jan Beulich wrote:
>> On 12.11.2024 11:36, Andrew Cooper wrote:
>>> On 07/11/2024 9:58 pm, Andrew Cooper wrote:
>>>> On 07/11/2024 12:21 pm, 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().  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>
>>>>>
>>>>> Slightly RFC.
>>>>>
>>>>> Just before posting, I've realised that cpu_request_microcode() does actually
>>>>> use the current CPU revision, and it's buggy, and it's the cause of `xen-ucode
>>>>> --force` not working as expected.
>>>>>
>>>>> I'm tempted to do another series cleaning that up in isolation, such that this
>>>>> patch becomes true in this form.
>>>> Actually no.  Having tried a bit, I think it's easier to do with patch 2
>>>> already in place.
>>>>
>>>> So instead I'm tempted to edit the middle paragraph to note that it
>>>> currently uses the revision but that's going to be fixed shortly.  The
>>>> rest of the paragraph explains why it's still safe anyway.
>>> So, after the latter series, this patch happens to be accurate.
>>>
>>> cpu_request_microcode() does read the revision, but discards the result
>>> of the calculation which used it.
>> What's the intended overall sequence of patches then? With two series that
>> (aiui) now have grown some sort of dependency, and with this series have
>> gained a 4/3 patch, having a clear picture would certainly help. Might it
>> be best if you merge both series and re-submit as a single one?
> 
> The order turns out to be as emailed out and threaded.

Yet above you said "after the latter series, this patch happens to be accurate."
Which suggest to me that at least part of the latter series needs to be in place
for the change here to be correct. IOW - I'm confused now.

Jan

Re: [PATCH 2/3] x86/ucode: Fold microcode_update_cpu() and fix error handling

[Andrew Cooper]

On 12/11/2024 10:45 am, Jan Beulich wrote:
> On 07.11.2024 13:21, Andrew Cooper wrote:
>> Fold microcode_update_cpu() into its single remaining caller and simplify the
>> logic by removing the patch != NULL path with microcode_mutex held.
>>
>> Explain why we bother grabbing the microcode revision even if we can't load
>> microcode.
>>
>> Furthermore, delete the -EIO path.  An error updating microcode on AP boot or
>> S3 resume is certainly bad, but freeing the cache is about the worst possible
>> action we can take in response; it prevents subsequent APs from taking an
>> update they might have accepted.
> I'm afraid I disagree here, but I also disagree with the present error handling.
> -EIO indicates the patch didn't apply. Why would there be any hope that any
> other CPU would accept it?

-EIO is "something went wrong".

On modern systems this can include "checksum didn't match because
there's a bad SRAM cell".  This is literally one of the failures leading
to the introduction of In-Field-Scan.

Individual cores really can fail in a way which won't be the same
elsewhere in the system.

> Keeping what's cached might be an option, but then followed by cleaning the
> cache unless at least one CPU actually accepted the ucode.

We already have that behaviour.


We cache speculatively on boot, even if the BSP doesn't need to load,
because APs might need to.  This really is the best we can do.

The only other time the cache gets modified is after a late-load attempt
which reported success.  There are still a lot of partial-failure error
cases to handle less badly, but that needs yet more untangling before it
can be addressed adequately.

~Andrew

Re: [PATCH 2/3] x86/ucode: Fold microcode_update_cpu() and fix error handling

[Jan Beulich]

On 12.11.2024 13:55, Andrew Cooper wrote:
> On 12/11/2024 10:45 am, Jan Beulich wrote:
>> On 07.11.2024 13:21, Andrew Cooper wrote:
>>> Fold microcode_update_cpu() into its single remaining caller and simplify the
>>> logic by removing the patch != NULL path with microcode_mutex held.
>>>
>>> Explain why we bother grabbing the microcode revision even if we can't load
>>> microcode.
>>>
>>> Furthermore, delete the -EIO path.  An error updating microcode on AP boot or
>>> S3 resume is certainly bad, but freeing the cache is about the worst possible
>>> action we can take in response; it prevents subsequent APs from taking an
>>> update they might have accepted.
>> I'm afraid I disagree here, but I also disagree with the present error handling.
>> -EIO indicates the patch didn't apply. Why would there be any hope that any
>> other CPU would accept it?
> 
> -EIO is "something went wrong".
> 
> On modern systems this can include "checksum didn't match because
> there's a bad SRAM cell".  This is literally one of the failures leading
> to the introduction of In-Field-Scan.
> 
> Individual cores really can fail in a way which won't be the same
> elsewhere in the system.

Hmm, well, slightly hesitantly
Acked-by: Jan Beulich <jbeulich@suse.com>

Ideally with a remark added to the description that there is known room
for further improvement.

>> Keeping what's cached might be an option, but then followed by cleaning the
>> cache unless at least one CPU actually accepted the ucode.
> 
> We already have that behaviour.
> 
> 
> We cache speculatively on boot, even if the BSP doesn't need to load,
> because APs might need to.  This really is the best we can do.

That's a different scenario. If we ended up with ucode which no single
CPU accepts, there's hardly much point in caching that ucode. This
specifically is meant not to include the case where simply all CPUs are
already up-to-date. The one largely theoretical case where caching may
still make sense is for CPU hotplug, where the hot-plugged CPU(s) may
accept what all boot-time CPUs refused.

Jan