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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

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@xxxxxxxx>
> 
> Jan
Thanks. I'm happy with all 3 changes being done on commit.

Alejandro