Re: [PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

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