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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/core.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 4811b5ffb11c..2bf462bf0c2e 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -860,6 +860,10 @@ static int __init early_microcode_load(struct boot_info *bi)
 
     rc = ucode_ops.apply_microcode(patch, 0);
 
+    if ( rc == 0 )
+        /* Rescan CPUID/MSR features, which may have changed after a load. */
+        early_cpu_init(false);
+
  unmap:
     bootstrap_unmap();
 
@@ -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;
 }
-- 
2.39.5

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@citrix.com>

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

> @@ -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. 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). But
yes, one thing at a time ...

Jan

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@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

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