| Message ID | 20241119215827.2891332-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/ucode: Only rescan features on successful microcode load | expand |
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
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@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); > } 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
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; }
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(-)