These two early exits skipped re-enabling the watchdog, and restoring the NMI
callback.  Always execute the tail of the function on the way out.

Fixes: 8dd4dfa92d62 ("x86/microcode: Synchronize late microcode loading")
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>

Also, added in the same patch is:

   * Note that RDTSC (in wait_for_condition()) is safe for threads to
   * execute while waiting for completion of loading an update.

which is absolutely not true in the slightest.  RDTSC is all microcode, and
Intel's guidance on the matter right now is that LFENCE is about the only safe
instruction to execute in a wait loop.  Even PAUSE is explcitily prohibited...
---
 xen/arch/x86/cpu/microcode/core.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index cfa2d5053a52..61cd36d601d6 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -492,10 +492,7 @@ static int control_thread_fn(const struct microcode_patch *patch)
     ret = wait_for_condition(wait_cpu_callin, num_online_cpus(),
                              MICROCODE_CALLIN_TIMEOUT_US);
     if ( ret )
-    {
-        set_state(LOADING_EXIT);
-        return ret;
-    }
+        goto out;
 
     /* Control thread loads ucode first while others are in NMI handler. */
     ret = alternative_call(ucode_ops.apply_microcode, patch);
@@ -507,8 +504,7 @@ static int control_thread_fn(const struct microcode_patch *patch)
     {
         printk(XENLOG_ERR
                "Late loading aborted: CPU%u failed to update ucode\n", cpu);
-        set_state(LOADING_EXIT);
-        return ret;
+        goto out;
     }
 
     /* Let primary threads load the given ucode update */
@@ -539,6 +535,7 @@ static int control_thread_fn(const struct microcode_patch *patch)
         }
     }
 
+ out:
     /* Mark loading is done to unblock other threads */
     set_state(LOADING_EXIT);
 
-- 
2.30.2

On 24.03.2023 22:44, Andrew Cooper wrote:
> These two early exits skipped re-enabling the watchdog, and restoring the NMI
> callback.  Always execute the tail of the function on the way out.
> 
> Fixes: 8dd4dfa92d62 ("x86/microcode: Synchronize late microcode loading")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

As a bonus you also avoid nmi_patch to hold a dangling pointer. May be worth
adding to the description.

Jan

On Fri, Mar 24, 2023 at 9:44 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> These two early exits skipped re-enabling the watchdog, and restoring the NMI
> callback.  Always execute the tail of the function on the way out.
>
> Fixes: 8dd4dfa92d62 ("x86/microcode: Synchronize late microcode loading")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

Thanks,
Sergey