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 <[email protected]> >> Reviewed-by: Jan Beulich <[email protected]> > > 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
