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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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