| Message ID | 20230622174219.8871-4-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Prevent attempting updates known to fail | expand |
On 22.06.2023 19:42, Alejandro Vallejo wrote: > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -885,5 +885,18 @@ int __init early_microcode_init(unsigned long *module_map, > if ( ucode_mod.mod_end || ucode_blob.size ) > rc = early_microcode_update_cpu(); > > + /* > + * MSR_ARCH_CAPS may have appeared after the microcode update. > + * Reload relevant fields in boot_cpu_data if so because they are > + * needed in tsx_init(). > + */ > + if ( boot_cpu_data.cpuid_level >= 7 ) > + boot_cpu_data.x86_capability[FEATURESET_7d0] > + = cpuid_count_edx(7, 0); > + if ( cpu_has_arch_caps ) > + rdmsr(MSR_ARCH_CAPABILITIES, > + boot_cpu_data.x86_capability[FEATURESET_m10Al], > + boot_cpu_data.x86_capability[FEATURESET_m10Ah]); > + > return rc; > } Did you consider simply calling early_cpu_init() a 2nd time, and then perhaps from setup.c and only if ucode load didn't report an error? There's a printk() in there which will want avoiding on the 2nd pass, but otherwise this would look more future-proof to me. Jan
On Fri, Jun 23, 2023 at 09:33:56AM +0200, Jan Beulich wrote: > On 22.06.2023 19:42, Alejandro Vallejo wrote: > > --- a/xen/arch/x86/cpu/microcode/core.c > > +++ b/xen/arch/x86/cpu/microcode/core.c > > @@ -885,5 +885,18 @@ int __init early_microcode_init(unsigned long *module_map, > > if ( ucode_mod.mod_end || ucode_blob.size ) > > rc = early_microcode_update_cpu(); > > > > + /* > > + * MSR_ARCH_CAPS may have appeared after the microcode update. > > + * Reload relevant fields in boot_cpu_data if so because they are > > + * needed in tsx_init(). > > + */ > > + if ( boot_cpu_data.cpuid_level >= 7 ) > > + boot_cpu_data.x86_capability[FEATURESET_7d0] > > + = cpuid_count_edx(7, 0); > > + if ( cpu_has_arch_caps ) > > + rdmsr(MSR_ARCH_CAPABILITIES, > > + boot_cpu_data.x86_capability[FEATURESET_m10Al], > > + boot_cpu_data.x86_capability[FEATURESET_m10Ah]); > > + > > return rc; > > } > > Did you consider simply calling early_cpu_init() a 2nd time, and then > perhaps from setup.c and only if ucode load didn't report an error? > There's a printk() in there which will want avoiding on the 2nd pass, > but otherwise this would look more future-proof to me. > > Jan I had, but avoiding the internal printk() was annoying. I've simply created a boolean "verbosity" flag on the new version for early_cpu_init() and called it at the end of early_microcode_init(). Alejandro
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index cfcdaace12..2f895e7c7c 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -352,6 +352,11 @@ void __init early_cpu_init(void) &c->x86_capability[FEATURESET_7c0], &c->x86_capability[FEATURESET_7d0]); + if (test_bit(X86_FEATURE_ARCH_CAPS, c->x86_capability)) + rdmsr(MSR_ARCH_CAPABILITIES, + c->x86_capability[FEATURESET_m10Al], + c->x86_capability[FEATURESET_m10Ah]); + if (max_subleaf >= 1) cpuid_count(7, 1, &eax, &ebx, &ecx, &c->x86_capability[FEATURESET_7d1]); diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index e67d143c97..dda6f03f7d 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -885,5 +885,18 @@ int __init early_microcode_init(unsigned long *module_map, if ( ucode_mod.mod_end || ucode_blob.size ) rc = early_microcode_update_cpu(); + /* + * MSR_ARCH_CAPS may have appeared after the microcode update. + * Reload relevant fields in boot_cpu_data if so because they are + * needed in tsx_init(). + */ + if ( boot_cpu_data.cpuid_level >= 7 ) + boot_cpu_data.x86_capability[FEATURESET_7d0] + = cpuid_count_edx(7, 0); + if ( cpu_has_arch_caps ) + rdmsr(MSR_ARCH_CAPABILITIES, + boot_cpu_data.x86_capability[FEATURESET_m10Al], + boot_cpu_data.x86_capability[FEATURESET_m10Ah]); + return rc; } diff --git a/xen/arch/x86/tsx.c b/xen/arch/x86/tsx.c index 80c6f4cedd..50d8059f23 100644 --- a/xen/arch/x86/tsx.c +++ b/xen/arch/x86/tsx.c @@ -39,9 +39,10 @@ void tsx_init(void) static bool __read_mostly once; /* - * This function is first called between microcode being loaded, and CPUID - * being scanned generally. Read into boot_cpu_data.x86_capability[] for - * the cpu_has_* bits we care about using here. + * This function is first called between microcode being loaded, and + * CPUID being scanned generally. early_cpu_init() has already prepared + * the feature bits needed here. And early_microcode_init() has ensured + * they are not stale after the microcode update. */ if ( unlikely(!once) ) { @@ -49,15 +50,6 @@ void tsx_init(void) once = true; - if ( boot_cpu_data.cpuid_level >= 7 ) - boot_cpu_data.x86_capability[FEATURESET_7d0] - = cpuid_count_edx(7, 0); - - if ( cpu_has_arch_caps ) - rdmsr(MSR_ARCH_CAPABILITIES, - boot_cpu_data.x86_capability[FEATURESET_m10Al], - boot_cpu_data.x86_capability[FEATURESET_m10Ah]); - has_rtm_always_abort = cpu_has_rtm_always_abort; if ( cpu_has_tsx_ctrl && cpu_has_srbds_ctrl )
Move MSR_ARCH_CAPS read code from tsx_init() to early_cpu_init(). Because microcode updates might make them that MSR to appear/have different values we also must reload it after a microcode update in early_microcode_init(). Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- v4: * Read MSR_ARCH_CAPS in early_cpu_init(). Otherwise tsx_init() doesn't have current values in the case where microcode wasn't updated (Jan) --- xen/arch/x86/cpu/common.c | 5 +++++ xen/arch/x86/cpu/microcode/core.c | 13 +++++++++++++ xen/arch/x86/tsx.c | 16 ++++------------ 3 files changed, 22 insertions(+), 12 deletions(-)