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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.06.2023 19:08, Alejandro Vallejo wrote:
> tsx_init() has some ad-hoc code to read MSR_ARCH_CAPS if present. In order
> to suuport DIS_MCU_UPDATE we need access to it earlier, so this patch moves
> early read to the tail of early_microcode_init(), after the early microcode
> update.
> 
> The read of the 7d0 CPUID leaf is left in a helper because it's reused in a
> later patch.
> 
> No functional change.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
> I suspect there was an oversight in tsx_init() by which
> boot_cpu_data.cpuid_level was never read? The first read I can
> see is in identify_cpu(), which happens after tsx_init().

See early_cpu_init(). (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.)

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

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

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

Jan