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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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


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

~Andrew