| Message ID | 20230615154834.959-5-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Prevent attempting updates known to fail | expand |
On 15.06.2023 17:48, Alejandro Vallejo wrote: > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -879,5 +879,18 @@ int __init early_microcode_init(unsigned long *module_map, > if ( ucode_mod.mod_end || ucode_blob.size ) > rc = early_microcode_update_cpu(); > > + /* > + * We might have exposed MSR_ARCH_CAPS after the microcode update. I'm struggling a little with this sentence, but not being a native speaker it may be me, not the sentence. I would perhaps have said "MSR_ARCH_CAPS may have appeared with the microcode update." > + * Reload relevant fields in boot_cpu_data if so because they are > + * needed in tsx_init() Nit: Missing full stop. I also wonder whether you wouldn't want to insert "e.g.", since iirc with the next patch tsx_init() isn't going to be the only user anymore. > + */ > + if ( boot_cpu_data.cpuid_level >= 7 ) > + boot_cpu_data.x86_capability[FEATURESET_7d0] > + = cpuid_count_edx(7, 0); I take it we assume the maximum CPUID level won't go from below 7 to 7 or higher with the ucode update? > --- a/xen/arch/x86/tsx.c > +++ b/xen/arch/x86/tsx.c > @@ -39,9 +39,9 @@ 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_microcode_init() has already > + * prepared the feature bits needed here after the microcode update. Is this true in all cases? early_microcode_init() may have bailed early, so I think you need to further transform early_microcode_init() (and as a personal request of mine preferably without goto). Jan
On Mon, Jun 19, 2023 at 05:57:14PM +0200, Jan Beulich wrote: > > + * We might have exposed MSR_ARCH_CAPS after the microcode update. > > I'm struggling a little with this sentence, but not being a native speaker > it may be me, not the sentence. I would perhaps have said "MSR_ARCH_CAPS > may have appeared with the microcode update." Sure, works for me. > I also wonder whether you wouldn't want to insert "e.g.", since iirc with > the next patch tsx_init() isn't going to be the only user anymore. tsx_init() is the only user, as far as I have seen. DIS_MCU_LOAD is checked before the update, using the cached data read in early_cpu_init() > > > + */ > > + if ( boot_cpu_data.cpuid_level >= 7 ) > > + boot_cpu_data.x86_capability[FEATURESET_7d0] > > + = cpuid_count_edx(7, 0); > > I take it we assume the maximum CPUID level won't go from below 7 to 7 > or higher with the ucode update? Do you mean from >=7 to <7 instead? Otherwise it just works and I don't undertand the concern. If so, that's an impossibly unlikely case and the current code does not try to clean up in that case. > > > --- a/xen/arch/x86/tsx.c > > +++ b/xen/arch/x86/tsx.c > > @@ -39,9 +39,9 @@ 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_microcode_init() has already > > + * prepared the feature bits needed here after the microcode update. > > Is this true in all cases? early_microcode_init() may have bailed > early, so I think you need to further transform early_microcode_init() > (and as a personal request of mine preferably without goto). > > Jan The series is eventually correct because MSR_ARCH_CAPS are also collected in early_cpu_init(). Alas, that's not the case here. You're right. I'll move the early MSR_ARCH_CAPS read to this patch as well. Alejandro
On 22.06.2023 16:55, Alejandro Vallejo wrote: > On Mon, Jun 19, 2023 at 05:57:14PM +0200, Jan Beulich wrote: >>> + if ( boot_cpu_data.cpuid_level >= 7 ) >>> + boot_cpu_data.x86_capability[FEATURESET_7d0] >>> + = cpuid_count_edx(7, 0); >> >> I take it we assume the maximum CPUID level won't go from below 7 to 7 >> or higher with the ucode update? > Do you mean from >=7 to <7 instead? Otherwise it just works and I don't > undertand the concern. No, I mean going from <7 to >=7. In such a case the earlier recorded .cpuid_level will prevent the leaf 7 read, and hence also the possible discovery of ARCH_CAPS having appeared. I actually wonder whether it wouldn't be better to re-run the whole of early_cpu_init() from here again. Jan
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 1554fa38eb..ef3c94018c 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -879,5 +879,18 @@ int __init early_microcode_init(unsigned long *module_map, if ( ucode_mod.mod_end || ucode_blob.size ) rc = early_microcode_update_cpu(); + /* + * We might have exposed MSR_ARCH_CAPS 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..11e9471180 100644 --- a/xen/arch/x86/tsx.c +++ b/xen/arch/x86/tsx.c @@ -39,9 +39,9 @@ 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_microcode_init() has already + * prepared the feature bits needed here after the microcode update. */ if ( unlikely(!once) ) { @@ -49,15 +49,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 immediately after the early microcode update. This helps keep the reload closer to its cause and is the earliest point we can read it, as it might be exposed only after a microcode update. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- v3: * Replaces v2/patch2. Moved after the "rev == ~0" check (Andrew) --- xen/arch/x86/cpu/microcode/core.c | 13 +++++++++++++ xen/arch/x86/tsx.c | 15 +++------------ 2 files changed, 16 insertions(+), 12 deletions(-)