| Message ID | 20230605170817.9913-3-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Prevent attempting updates known to fail | expand |
On 05.06.2023 19:08, Alejandro Vallejo wrote: > tsx_init() has some ad-hoc code to read MSR_ARCH_CAPS if present. In order > to suuport DIS_MCU_UPDATE we need access to it earlier, so this patch moves > early read to the tail of early_microcode_init(), after the early microcode > update. > > The read of the 7d0 CPUID leaf is left in a helper because it's reused in a > later patch. > > No functional change. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > --- > I suspect there was an oversight in tsx_init() by which > boot_cpu_data.cpuid_level was never read? The first read I can > see is in identify_cpu(), which happens after tsx_init(). See early_cpu_init(). (I have to admit that I was also struggling with your use of "read": Aiui you mean the field was never written / set, and "read" really refers to the reading of the corresponding CPUID leaf.) > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -840,6 +840,15 @@ static int __init early_microcode_update_cpu(void) > return microcode_update_cpu(patch); > } > > +static void __init early_read_cpuid_7d0(void) > +{ > + boot_cpu_data.cpuid_level = cpuid_eax(0); As per above I don't think this is needed. > + if ( boot_cpu_data.cpuid_level >= 7 ) > + boot_cpu_data.x86_capability[FEATURESET_7d0] > + = cpuid_count_edx(7, 0); This is actually filled in early_cpu_init() as well, so doesn't need re-doing here unless because of a suspected change to the value (but then other CPUID output may have changed, too). At which point ... > @@ -878,5 +887,17 @@ int __init early_microcode_init(unsigned long *module_map, > if ( ucode_mod.mod_end || ucode_blob.size ) > rc = early_microcode_update_cpu(); > > + early_read_cpuid_7d0(); > + > + /* > + * tsx_init() needs MSR_ARCH_CAPS, but it runs before identify_cpu() > + * populates boot_cpu_data, so we read it here to centralize early > + * CPUID/MSR reads in the same place. > + */ > + if ( cpu_has_arch_caps ) > + rdmsr(MSR_ARCH_CAPABILITIES, > + boot_cpu_data.x86_capability[FEATURESET_m10Al], > + boot_cpu_data.x86_capability[FEATURESET_m10Ah]); ... "centralize" aspect goes away, and hence the comment needs adjusting. > --- 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. > + * While MSRs/CPUID haven't yet been scanned, MSR_ARCH_CAPABILITIES > + * and leaf 7d0 have already been read if present after early microcode > + * loading time. So we can assume _those_ are present. > */ > if ( unlikely(!once) ) > { I think I'd like to see at least the initial part of the original comment retained here. Jan
On 12/06/2023 4:46 pm, Jan Beulich wrote: > On 05.06.2023 19:08, Alejandro Vallejo wrote: >> --- a/xen/arch/x86/cpu/microcode/core.c >> +++ b/xen/arch/x86/cpu/microcode/core.c >> @@ -840,6 +840,15 @@ static int __init early_microcode_update_cpu(void) >> return microcode_update_cpu(patch); >> } >> >> +static void __init early_read_cpuid_7d0(void) >> +{ >> + boot_cpu_data.cpuid_level = cpuid_eax(0); > As per above I don't think this is needed. > >> + if ( boot_cpu_data.cpuid_level >= 7 ) >> + boot_cpu_data.x86_capability[FEATURESET_7d0] >> + = cpuid_count_edx(7, 0); > This is actually filled in early_cpu_init() as well, so doesn't need > re-doing here unless because of a suspected change to the value (but > then other CPUID output may have changed, too). Hmm, yes. I suspect that is due to the CET series (which needed to know 7d0 much earlier than previously), and me forgetting to clean up tsx_init(). > At which point ... > >> @@ -878,5 +887,17 @@ int __init early_microcode_init(unsigned long *module_map, >> if ( ucode_mod.mod_end || ucode_blob.size ) >> rc = early_microcode_update_cpu(); >> >> + early_read_cpuid_7d0(); >> + >> + /* >> + * tsx_init() needs MSR_ARCH_CAPS, but it runs before identify_cpu() >> + * populates boot_cpu_data, so we read it here to centralize early >> + * CPUID/MSR reads in the same place. >> + */ >> + if ( cpu_has_arch_caps ) >> + rdmsr(MSR_ARCH_CAPABILITIES, >> + boot_cpu_data.x86_capability[FEATURESET_m10Al], >> + boot_cpu_data.x86_capability[FEATURESET_m10Ah]); > ... "centralize" aspect goes away, and hence the comment needs adjusting. I find it weird splitting apart the various reads into x86_capability[], but in light of the feedback, only the rdmsr() needs to stay. > >> --- 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. >> + * While MSRs/CPUID haven't yet been scanned, MSR_ARCH_CAPABILITIES >> + * and leaf 7d0 have already been read if present after early microcode >> + * loading time. So we can assume _those_ are present. >> */ >> if ( unlikely(!once) ) >> { > I think I'd like to see at least the initial part of the original comment > retained here. The first sentence needs to stay as-is. That's still relevant even with the feature handling moved out. The second sentence wants to say something like "However, microcode_init() has already prepared the feature bits we need." because it's the justification of why we don't do it here. ~Andrew
On 12.06.2023 20:25, Andrew Cooper wrote: > On 12/06/2023 4:46 pm, Jan Beulich wrote: >> On 05.06.2023 19:08, Alejandro Vallejo wrote: >>> @@ -878,5 +887,17 @@ int __init early_microcode_init(unsigned long *module_map, >>> if ( ucode_mod.mod_end || ucode_blob.size ) >>> rc = early_microcode_update_cpu(); >>> >>> + early_read_cpuid_7d0(); >>> + >>> + /* >>> + * tsx_init() needs MSR_ARCH_CAPS, but it runs before identify_cpu() >>> + * populates boot_cpu_data, so we read it here to centralize early >>> + * CPUID/MSR reads in the same place. >>> + */ >>> + if ( cpu_has_arch_caps ) >>> + rdmsr(MSR_ARCH_CAPABILITIES, >>> + boot_cpu_data.x86_capability[FEATURESET_m10Al], >>> + boot_cpu_data.x86_capability[FEATURESET_m10Ah]); >> ... "centralize" aspect goes away, and hence the comment needs adjusting. > > I find it weird splitting apart the various reads into x86_capability[], > but in light of the feedback, only the rdmsr() needs to stay. Hmm, wait: When updating a CPU from a pre-arch-caps ucode level on one that supports arch-caps, don't we need to re-read 7d0 here? (I.e. the call to early_read_cpuid_7d0() needs to stay, but for a reason different from the one presently stated in the description, and possibly even worth a brief comment.) Jan
On 13/06/2023 7:40 am, Jan Beulich wrote: > On 12.06.2023 20:25, Andrew Cooper wrote: >> On 12/06/2023 4:46 pm, Jan Beulich wrote: >>> On 05.06.2023 19:08, Alejandro Vallejo wrote: >>>> @@ -878,5 +887,17 @@ int __init early_microcode_init(unsigned long *module_map, >>>> if ( ucode_mod.mod_end || ucode_blob.size ) >>>> rc = early_microcode_update_cpu(); >>>> >>>> + early_read_cpuid_7d0(); >>>> + >>>> + /* >>>> + * tsx_init() needs MSR_ARCH_CAPS, but it runs before identify_cpu() >>>> + * populates boot_cpu_data, so we read it here to centralize early >>>> + * CPUID/MSR reads in the same place. >>>> + */ >>>> + if ( cpu_has_arch_caps ) >>>> + rdmsr(MSR_ARCH_CAPABILITIES, >>>> + boot_cpu_data.x86_capability[FEATURESET_m10Al], >>>> + boot_cpu_data.x86_capability[FEATURESET_m10Ah]); >>> ... "centralize" aspect goes away, and hence the comment needs adjusting. >> I find it weird splitting apart the various reads into x86_capability[], >> but in light of the feedback, only the rdmsr() needs to stay. > Hmm, wait: When updating a CPU from a pre-arch-caps ucode level on one > that supports arch-caps, don't we need to re-read 7d0 here? (I.e. the > call to early_read_cpuid_7d0() needs to stay, but for a reason > different from the one presently stated in the description, and > possibly even worth a brief comment.) Urgh yes. We do have situations where this ucode load will cause MSR_ARCH_CAPS (and features there-within) to appear. I'll rethink the safety here when I've got some breathing room from other tasks. ~Andrew
On Mon, Jun 12, 2023 at 05:46:07PM +0200, Jan Beulich wrote: > See early_cpu_init(). Ah, I missed that. I didn't expect several leafs to be read at once. I see now that that function takes care of > (I have to admit that I was also struggling with > your use of "read": Aiui you mean the field was never written / set, > and "read" really refers to the reading of the corresponding CPUID > leaf.) Correct, though in retrospect that does sound misleading. I meant read from the HW CPUID leaf. > > --- a/xen/arch/x86/cpu/microcode/core.c > > +++ b/xen/arch/x86/cpu/microcode/core.c > > @@ -840,6 +840,15 @@ static int __init early_microcode_update_cpu(void) > > return microcode_update_cpu(patch); > > } > > > > +static void __init early_read_cpuid_7d0(void) > > +{ > > + boot_cpu_data.cpuid_level = cpuid_eax(0); > > As per above I don't think this is needed. > > > + if ( boot_cpu_data.cpuid_level >= 7 ) > > + boot_cpu_data.x86_capability[FEATURESET_7d0] > > + = cpuid_count_edx(7, 0); > > This is actually filled in early_cpu_init() as well, so doesn't need > re-doing here unless because of a suspected change to the value (but > then other CPUID output may have changed, too). At which point ... MSR_ARCH_CAPS may genuinely appear only after a microcode update, so re-reading 7d0 and the MSR itself is probably the sane thing to do. > > > @@ -878,5 +887,17 @@ int __init early_microcode_init(unsigned long *module_map, > > if ( ucode_mod.mod_end || ucode_blob.size ) > > rc = early_microcode_update_cpu(); > > > > + early_read_cpuid_7d0(); > > + > > + /* > > + * tsx_init() needs MSR_ARCH_CAPS, but it runs before identify_cpu() > > + * populates boot_cpu_data, so we read it here to centralize early > > + * CPUID/MSR reads in the same place. > > + */ > > + if ( cpu_has_arch_caps ) > > + rdmsr(MSR_ARCH_CAPABILITIES, > > + boot_cpu_data.x86_capability[FEATURESET_m10Al], > > + boot_cpu_data.x86_capability[FEATURESET_m10Ah]); > > ... "centralize" aspect goes away, and hence the comment needs adjusting. Indeed. I'll rewrite that. > > --- 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. > > + * While MSRs/CPUID haven't yet been scanned, MSR_ARCH_CAPABILITIES > > + * and leaf 7d0 have already been read if present after early microcode > > + * loading time. So we can assume _those_ are present. > > */ > > if ( unlikely(!once) ) > > { > > I think I'd like to see at least the initial part of the original comment > retained here. Ack. Sure. Alejandro
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 29ff38f35c..892bcec901 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -840,6 +840,15 @@ static int __init early_microcode_update_cpu(void) return microcode_update_cpu(patch); } +static void __init early_read_cpuid_7d0(void) +{ + boot_cpu_data.cpuid_level = cpuid_eax(0); + + if ( boot_cpu_data.cpuid_level >= 7 ) + boot_cpu_data.x86_capability[FEATURESET_7d0] + = cpuid_count_edx(7, 0); +} + int __init early_microcode_init(unsigned long *module_map, const struct multiboot_info *mbi) { @@ -878,5 +887,17 @@ int __init early_microcode_init(unsigned long *module_map, if ( ucode_mod.mod_end || ucode_blob.size ) rc = early_microcode_update_cpu(); + early_read_cpuid_7d0(); + + /* + * tsx_init() needs MSR_ARCH_CAPS, but it runs before identify_cpu() + * populates boot_cpu_data, so we read it here to centralize early + * CPUID/MSR reads in the same place. + */ + 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..0501e181bf 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. + * While MSRs/CPUID haven't yet been scanned, MSR_ARCH_CAPABILITIES + * and leaf 7d0 have already been read if present after early microcode + * loading time. So we can assume _those_ are present. */ 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 )
tsx_init() has some ad-hoc code to read MSR_ARCH_CAPS if present. In order to suuport DIS_MCU_UPDATE we need access to it earlier, so this patch moves early read to the tail of early_microcode_init(), after the early microcode update. The read of the 7d0 CPUID leaf is left in a helper because it's reused in a later patch. No functional change. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- I suspect there was an oversight in tsx_init() by which boot_cpu_data.cpuid_level was never read? The first read I can see is in identify_cpu(), which happens after tsx_init(). v2: * New addition --- xen/arch/x86/cpu/microcode/core.c | 21 +++++++++++++++++++++ xen/arch/x86/tsx.c | 15 +++------------ 2 files changed, 24 insertions(+), 12 deletions(-)