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(-)

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

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 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

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