Re: [PATCH for 4.19?] x86/Intel: unlock CPUID earlier for the BSP

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Jun 13, 2024 at 10:19:30AM +0200, Jan Beulich wrote:
> Intel CPUs have a MSR bit to limit CPUID enumeration to leaf two. If
> this bit is set by the BIOS then CPUID evaluation does not work when
> data from any leaf greater than two is needed; early_cpu_init() in
> particular wants to collect leaf 7 data.
> 
> Cure this by unlocking CPUID right before evaluating anything which
> depends on the maximum CPUID leaf being greater than two.
> 
> Inspired by (and description cloned from) Linux commit 0c2f6d04619e
> ("x86/topology/intel: Unlock CPUID before evaluating anything").
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> While I couldn't spot anything, it kind of feels as if I'm overlooking
> further places where we might be inspecting in particular leaf 7 yet
> earlier.
> 
> No Fixes: tag(s), as imo it would be too many that would want
> enumerating.
> 
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -336,7 +336,8 @@ void __init early_cpu_init(bool verbose)
>  
>  	c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx);
>  	switch (c->x86_vendor) {
> -	case X86_VENDOR_INTEL:    actual_cpu = intel_cpu_dev;    break;
> +	case X86_VENDOR_INTEL:    intel_unlock_cpuid_leaves(c);
> +				  actual_cpu = intel_cpu_dev;    break;
>  	case X86_VENDOR_AMD:      actual_cpu = amd_cpu_dev;      break;
>  	case X86_VENDOR_CENTAUR:  actual_cpu = centaur_cpu_dev;  break;
>  	case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break;
> --- a/xen/arch/x86/cpu/cpu.h
> +++ b/xen/arch/x86/cpu/cpu.h
> @@ -24,3 +24,5 @@ void amd_init_lfence(struct cpuinfo_x86
>  void amd_init_ssbd(const struct cpuinfo_x86 *c);
>  void amd_init_spectral_chicken(void);
>  void detect_zen2_null_seg_behaviour(void);
> +
> +void intel_unlock_cpuid_leaves(struct cpuinfo_x86 *c);
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -303,10 +303,24 @@ static void __init noinline intel_init_l
>  		ctxt_switch_masking = intel_ctxt_switch_masking;
>  }
>  
> -static void cf_check early_init_intel(struct cpuinfo_x86 *c)
> +/* Unmask CPUID levels if masked. */
> +void intel_unlock_cpuid_leaves(struct cpuinfo_x86 *c)
>  {
> -	u64 misc_enable, disable;
> +	uint64_t misc_enable, disable;
> +
> +	rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> +
> +	disable = misc_enable & MSR_IA32_MISC_ENABLE_LIMIT_CPUID;
> +	if (disable) {
> +		wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable & ~disable);
> +		bootsym(trampoline_misc_enable_off) |= disable;
> +		c->cpuid_level = cpuid_eax(0);
> +		printk(KERN_INFO "revised cpuid level: %u\n", c->cpuid_level);
> +	}
> +}
>  
> +static void cf_check early_init_intel(struct cpuinfo_x86 *c)
> +{
>  	/* Netburst reports 64 bytes clflush size, but does IO in 128 bytes */
>  	if (c->x86 == 15 && c->x86_cache_alignment == 64)
>  		c->x86_cache_alignment = 128;
> @@ -315,16 +329,7 @@ static void cf_check early_init_intel(st
>  	    bootsym(trampoline_misc_enable_off) & MSR_IA32_MISC_ENABLE_XD_DISABLE)
>  		printk(KERN_INFO "re-enabled NX (Execute Disable) protection\n");
>  
> -	/* Unmask CPUID levels and NX if masked: */
> -	rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> -
> -	disable = misc_enable & MSR_IA32_MISC_ENABLE_LIMIT_CPUID;
> -	if (disable) {
> -		wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable & ~disable);
> -		bootsym(trampoline_misc_enable_off) |= disable;
> -		printk(KERN_INFO "revised cpuid level: %d\n",
> -		       cpuid_eax(0));
> -	}
> +	intel_unlock_cpuid_leaves(c);

Do you really need to call intel_unlock_cpuid_leaves() here?

For the BSP it will be taken care in early_cpu_init(), and for the APs
MSR_IA32_MISC_ENABLE it will be set as part of the trampoline with the
disables from trampoline_misc_enable_off.

Thanks, Roger.