Re: [PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()
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
Re: [PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()
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
Re: [PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()
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
Re: [PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()
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
Re: [PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()
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
> ---
> 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
[PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()
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
---
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(-)
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 )
--
2.34.1
