Re: [PATCH v5 3/4] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()

[Alejandro Vallejo <alejandro.vallejo@cloud.com>]

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