Re: [PATCH 1/3] x86/ucode: Fix error handling during parallel ucode load

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.11.2025 23:21, Andrew Cooper wrote:
> wait_for_state() returns false on encountering LOADING_EXIT.
> control_thread_fn() can move directly to this state in the case of an early
> error.  It is not an error condition for APs, but right now the latest write
> into stopmachine_data.fn_result wins, causing the real error, -EIO, to get
> clobbered with -EBUSY.  e.g.:
> 
>   # xen-ucode /lib/firmware/amd-ucode/microcode_amd_fam17h.bin --force
>   Failed to update microcode. (err: Device or resource busy)
> 
>   (XEN) 256 cores are to update their microcode
>   (XEN) microcode: CPU0 update rev 0x830107d to 0x830107c failed, result 
> 0x830107d
>   (XEN) Late loading aborted: CPU0 failed to update ucode: -5
> 
> Drop all the -EBUSY's, and treat hitting LOADING_EXIT as a success case.  This
> causes only a single error to be returned through stop_machine_run().  e.g.:

Why "single"? stop_machine_run() can't return multiple ones, having only a
scalar return type? Or do you mean "a single, consistent" or some such?

>   # xen-ucode /lib/firmware/amd-ucode/microcode_amd_fam17h.bin --force
>   Failed to update microcode. (err: Input/output error)
> 
>   (XEN) 256 cores are to update their microcode
>   (XEN) microcode: CPU0 update rev 0x830107d to 0x830107c failed, result 
> 0x830107d
>   (XEN) Late loading aborted: CPU0 failed to update ucode: -5

The sole difference being which specific error is observed, which looks to
support the above interpretation. What I don't quite understand is ...

> Fixes: 5ed12565aa32 ("microcode: rendezvous CPUs in NMI handler and load 
> ucode")

... this and the specific indication that this needs backporting: Why is
the particular error code this important here?

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -260,7 +260,9 @@ static int secondary_nmi_work(void)
>  {
>      cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
>  
> -    return wait_for_state(LOADING_EXIT) ? 0 : -EBUSY;
> +    wait_for_state(LOADING_EXIT);
> +
> +    return 0;
>  }

At which point the function could as well return void? Preferably with this
adjustment (and the knock-on one at the call site) and with the slight
clarification to the description
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> @@ -271,7 +273,7 @@ static int primary_thread_work(const struct 
> microcode_patch *patch,
>      cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
>  
>      if ( !wait_for_state(LOADING_ENTER) )
> -        return -EBUSY;
> +        return 0;
>  
>      ret = alternative_call(ucode_ops.apply_microcode, patch, flags);
>      if ( !ret )
> @@ -313,7 +315,7 @@ static int cf_check microcode_nmi_callback(
>  static int secondary_thread_fn(void)
>  {
>      if ( !wait_for_state(LOADING_CALLIN) )
> -        return -EBUSY;
> +        return 0;
>  
>      self_nmi();
>  
> @@ -336,7 +338,7 @@ static int primary_thread_fn(const struct microcode_patch 
> *patch,
>                               unsigned int flags)
>  {
>      if ( !wait_for_state(LOADING_CALLIN) )
> -        return -EBUSY;
> +        return 0;
>  
>      if ( ucode_in_nmi )
>      {

Vaguely recalling the original intentions, these changes looked wrong to me at
the first glance. But yes, an exit indication from the control thread isn't
really a separate error condition.

Jan