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>
---
v5:
 * Re-run early_cpu_init() after early_microcode_init() rather than
   reloading specific fields
 * Amended early_cpu_init() so it takes a `verbose` argument in order
   to skip printing the same information before and after early microcode
   updates
---
 xen/arch/x86/cpu/common.c         | 23 +++++++++++++++--------
 xen/arch/x86/cpu/microcode/core.c |  6 ++++++
 xen/arch/x86/setup.c              |  2 +-
 xen/arch/x86/tsx.c                | 16 ++++------------
 4 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index cfcdaace12..a1be0aa4bd 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -303,7 +303,7 @@ static inline u32 phys_pkg_id(u32 cpuid_apic, int index_msb)
 
    WARNING: this function is only called on the BP.  Don't add code here
    that is supposed to run on all CPUs. */
-void __init early_cpu_init(void)
+void __init early_cpu_init(bool verbose)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
 	u32 eax, ebx, ecx, edx;
@@ -324,9 +324,10 @@ void __init early_cpu_init(void)
 	case X86_VENDOR_SHANGHAI: this_cpu = &shanghai_cpu_dev; break;
 	case X86_VENDOR_HYGON:    this_cpu = &hygon_cpu_dev;    break;
 	default:
-		printk(XENLOG_ERR
-		       "Unrecognised or unsupported CPU vendor '%.12s'\n",
-		       c->x86_vendor_id);
+		if (verbose)
+			printk(XENLOG_ERR
+			       "Unrecognised or unsupported CPU vendor '%.12s'\n",
+			       c->x86_vendor_id);
 	}
 
 	cpuid(0x00000001, &eax, &ebx, &ecx, &edx);
@@ -340,10 +341,11 @@ void __init early_cpu_init(void)
 	c->x86_capability[FEATURESET_1d] = edx;
 	c->x86_capability[FEATURESET_1c] = ecx;
 
-	printk(XENLOG_INFO
-	       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
-	       x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86,
-	       c->x86_model, c->x86_model, c->x86_mask, eax);
+	if (verbose)
+		printk(XENLOG_INFO
+		       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
+		       x86_cpuid_vendor_to_str(boot_cpu_data->x86_vendor), c->x86, c->x86,
+		       c->x86_model, c->x86_model, c->x86_mask, eax);
 
 	if (c->cpuid_level >= 7) {
 		uint32_t max_subleaf;
@@ -352,6 +354,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 b620e3bfa6..98a5aebfe3 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -886,5 +886,11 @@ 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
+     * boot_cpu_data if so because they are needed in tsx_init().
+     */
+    early_cpu_init(false);
+
     return rc;
 }
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 74e3915a4d..bdf66e80ac 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1211,7 +1211,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         panic("Bootloader provided no memory information\n");
 
     /* This must come before e820 code because it sets paddr_bits. */
-    early_cpu_init();
+    early_cpu_init(true);
 
     /* Choose shadow stack early, to set infrastructure up appropriately. */
     if ( !boot_cpu_has(X86_FEATURE_CET_SS) )
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 )
-- 
2.34.1

On 29.06.2023 17:26, Alejandro Vallejo wrote:
> @@ -324,9 +324,10 @@ void __init early_cpu_init(void)
>  	case X86_VENDOR_SHANGHAI: this_cpu = &shanghai_cpu_dev; break;
>  	case X86_VENDOR_HYGON:    this_cpu = &hygon_cpu_dev;    break;
>  	default:
> -		printk(XENLOG_ERR
> -		       "Unrecognised or unsupported CPU vendor '%.12s'\n",
> -		       c->x86_vendor_id);
> +		if (verbose)
> +			printk(XENLOG_ERR
> +			       "Unrecognised or unsupported CPU vendor '%.12s'\n",
> +			       c->x86_vendor_id);

Just as a remark:

	if (!verbose)
		break;

would have been less of a delta and keeping all lines within the 80
chars limit.

> @@ -340,10 +341,11 @@ void __init early_cpu_init(void)
>  	c->x86_capability[FEATURESET_1d] = edx;
>  	c->x86_capability[FEATURESET_1c] = ecx;
>  
> -	printk(XENLOG_INFO
> -	       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
> -	       x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86,
> -	       c->x86_model, c->x86_model, c->x86_mask, eax);
> +	if (verbose)
> +		printk(XENLOG_INFO
> +		       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
> +		       x86_cpuid_vendor_to_str(boot_cpu_data->x86_vendor), c->x86, c->x86,
> +		       c->x86_model, c->x86_model, c->x86_mask, eax);

Since rearrangement to limit line length isn't really possible here,
the last two lines need re-flowing to stay within limits.

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -886,5 +886,11 @@ 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
> +     * boot_cpu_data if so because they are needed in tsx_init().
> +     */
> +    early_cpu_init(false);

I think the comment would better talk of ARCH_CAPS as an example of what
may newly appear with a ucode update.

With at least the middle item taken care of (which I'd be happy to
do while committing)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

On Wed, Jul 05, 2023 at 12:43:27PM +0200, Jan Beulich wrote:
> On 29.06.2023 17:26, Alejandro Vallejo wrote:
> > @@ -324,9 +324,10 @@ void __init early_cpu_init(void)
> >  	case X86_VENDOR_SHANGHAI: this_cpu = &shanghai_cpu_dev; break;
> >  	case X86_VENDOR_HYGON:    this_cpu = &hygon_cpu_dev;    break;
> >  	default:
> > -		printk(XENLOG_ERR
> > -		       "Unrecognised or unsupported CPU vendor '%.12s'\n",
> > -		       c->x86_vendor_id);
> > +		if (verbose)
> > +			printk(XENLOG_ERR
> > +			       "Unrecognised or unsupported CPU vendor '%.12s'\n",
> > +			       c->x86_vendor_id);
> 
> Just as a remark:
> 
> 	if (!verbose)
> 		break;
> 
> would have been less of a delta and keeping all lines within the 80
> chars limit.
Very true, that looks nicer.

> > @@ -340,10 +341,11 @@ void __init early_cpu_init(void)
> >  	c->x86_capability[FEATURESET_1d] = edx;
> >  	c->x86_capability[FEATURESET_1c] = ecx;
> >  
> > -	printk(XENLOG_INFO
> > -	       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
> > -	       x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86,
> > -	       c->x86_model, c->x86_model, c->x86_mask, eax);
> > +	if (verbose)
> > +		printk(XENLOG_INFO
> > +		       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
> > +		       x86_cpuid_vendor_to_str(boot_cpu_data->x86_vendor), c->x86, c->x86,
> > +		       c->x86_model, c->x86_model, c->x86_mask, eax);
> 
> Since rearrangement to limit line length isn't really possible here,
> the last two lines need re-flowing to stay within limits.
I assumed they could could share the length of the printk string. I don't
mind either way.

> 
> > --- a/xen/arch/x86/cpu/microcode/core.c
> > +++ b/xen/arch/x86/cpu/microcode/core.c
> > @@ -886,5 +886,11 @@ 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
> > +     * boot_cpu_data if so because they are needed in tsx_init().
> > +     */
> > +    early_cpu_init(false);
> 
> I think the comment would better talk of ARCH_CAPS as an example of what
> may newly appear with a ucode update.
I just started writing a paragraph stating that it's unlikely anything else
will just appear, but thinking it through you're definitely right. A new
MSR_NEW_SPEC_MITIGATIONS might very well appear.

Something along this lines would be better?
```
          * Microcode updates may change CPUID or MSRs. We need to reload
          * the early subset boot_cpu_data before continuing. Notably tsx_init()
          * needs an up to date MSR_ARCH_CAPS.
```

> 
> With at least the middle item taken care of (which I'd be happy to
> do while committing)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Jan
Thanks. I'm happy with all 3 changes being done on commit.

Alejandro

On Wed, Jul 05, 2023 at 02:12:58PM +0100, Alejandro Vallejo wrote:
> Something along this lines would be better?
> ```
>           * Microcode updates may change CPUID or MSRs. We need to reload
>           * the early subset boot_cpu_data before continuing. Notably tsx_init()
>           * needs an up to date MSR_ARCH_CAPS.
> ```
That makes no sense. I sent the email before completing the sentence. I
meant:

```
          * Microcode updates may change CPUID or MSRs. We need to reload
          * the subset of boot_cpu_data we got on early_cpu_init() before
          * continuing. Notably tsx_init() needs an up to date MSR_ARCH_CAPS.
```

Alejandro