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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/06/2023 4:46 pm, Jan Beulich wrote:
> On 05.06.2023 19:08, Alejandro Vallejo wrote:
>> --- 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).

Hmm, yes.  I suspect that is due to the CET series (which needed to know
7d0 much earlier than previously), and me forgetting to clean up tsx_init().

>  At which point ...
>
>> @@ -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.

>
>> --- 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.

The first sentence needs to stay as-is.  That's still relevant even with
the feature handling moved out.

The second sentence wants to say something like "However,
microcode_init() has already prepared the feature bits we need." because
it's the justification of why we don't do it here.

~Andrew