Re: [PATCH] x86/ucode: Only rescan features on successful microcode load

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.11.2024 14:50, Andrew Cooper wrote:
> On 20/11/2024 10:50 am, Jan Beulich wrote:
>> On 19.11.2024 22:58, Andrew Cooper wrote:
>>> There's no point rescanning if we didn't load something new.  Take the
>>> opportunity to make the comment a bit more concise.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Thanks.
> 
>>
>>> @@ -911,14 +915,5 @@ int __init early_microcode_init(struct boot_info *bi)
>>>  
>>>      rc = early_microcode_load(bi);
>>>  
>>> -    /*
>>> -     * Some CPUID leaves and MSRs are only present after microcode updates
>>> -     * on some processors. We take the chance here to make sure what little
>>> -     * state we have already probed is re-probed in order to ensure we do
>>> -     * not use stale values. tsx_init() in particular needs to have up to
>>> -     * date MSR_ARCH_CAPS.
>>> -     */
>>> -    early_cpu_init(false);
>>> -
>>>      return rc;
>>>  }
>> In principle with this rc could be dropped from the function.
> 
> Oh, so it can.  I think I did so in an earlier attempt, prior to
> deciding to go down the route that is partially committed.
> 
> I'm happy to fold in the removal.  The incremental diff is:
> 
> @@ -873,7 +873,6 @@ static int __init early_microcode_load(struct
> boot_info *bi)
>  int __init early_microcode_init(struct boot_info *bi)
>  {
>      const struct cpuinfo_x86 *c = &boot_cpu_data;
> -    int rc = 0;
>  
>      switch ( c->x86_vendor )
>      {
> @@ -913,7 +912,5 @@ int __init early_microcode_init(struct boot_info *bi)
>          return -ENODEV;
>      }
>  
> -    rc = early_microcode_load(bi);
> -
> -    return rc;
> +    return early_microcode_load(bi);
>  }

Please do.

>> It's then further
>> unclear why early_microcode_load() needs to be a separate function, rather 
>> than
>> simply being inlined here (as I expect the compiler is going to do anyway).
> 
> Both cognitive and code complexity.
> 
> "Probe and install hooks" is separate from "try to load new ucode if we
> can".
> 
> They've now got entirely disjoint local variables, and the latter has
> some non-trivial control flow in it.  It's liable to get even more
> complex if we try to allow CPIO in an explicitly nominated module.
> 
> More generally, a separate function and internal return statements can
> express control flow which can only be done with gotos at the outer
> level, even if we fully intend the compiler to fold the two together.

Fair enough.

Jan