Re: [PATCH v2 19/30] x86/cpu: Rework Intel masking/faulting setup

["Jan Beulich" <JBeulich@suse.com>]

>>> On 05.02.16 at 14:42, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -18,11 +18,18 @@
>  
>  #define select_idle_routine(x) ((void)0)
>  
> -static unsigned int probe_intel_cpuid_faulting(void)
> +static bool_t __init probe_intel_cpuid_faulting(void)
>  {
>  	uint64_t x;
> -	return !rdmsr_safe(MSR_INTEL_PLATFORM_INFO, x) &&
> -		(x & MSR_PLATFORM_INFO_CPUID_FAULTING);
> +
> +	if ( rdmsr_safe(MSR_INTEL_PLATFORM_INFO, x) ||
> +	     !(x & MSR_PLATFORM_INFO_CPUID_FAULTING) )
> +		return 0;

Partial Xen coding style again.

> @@ -44,41 +51,46 @@ void set_cpuid_faulting(bool_t enable)
>  }
>  
>  /*
> - * opt_cpuid_mask_ecx/edx: cpuid.1[ecx, edx] feature mask.
> - * For example, E8400[Intel Core 2 Duo Processor series] ecx = 0x0008E3FD,
> - * edx = 0xBFEBFBFF when executing CPUID.EAX = 1 normally. If you want to
> - * 'rev down' to E8400, you can set these values in these Xen boot parameters.
> + * Set caps in expected_levelling_cap, probe a specific masking MSR, and set
> + * caps in levelling_caps if it is found, or clobber the MSR index if missing.
> + * If preset, reads the default value into msr_val.
>   */
> -static void set_cpuidmask(const struct cpuinfo_x86 *c)
> +static void __init __probe_mask_msr(unsigned int *msr, uint64_t caps,
> +				    uint64_t *msr_val)
>  {
> -	static unsigned int msr_basic, msr_ext, msr_xsave;
> -	static enum { not_parsed, no_mask, set_mask } status;
> -	u64 msr_val;
> +	uint64_t val;
>  
> -	if (status == no_mask)
> -		return;
> +	expected_levelling_cap |= caps;
>  
> -	if (status == set_mask)
> -		goto setmask;
> +	if (rdmsr_safe(*msr, val) || wrmsr_safe(*msr, val))
> +		*msr = 0;
> +	else
> +	{
> +		levelling_caps |= caps;
> +		*msr_val = val;
> +	}
> +}

Same as for the AMD side: Perhaps neater if the function returned
the MSR value? (Also again partial Xen coding style here.)

> +/* Indicies of the masking MSRs, or 0 if unavailable. */
> +static unsigned int __read_mostly msr_basic, msr_ext, msr_xsave;

I think this way __read_mostly applies only to msr_basic, which I
don't think is what you want. Also I think you mean "indices" or
"indexes".

> +static void __init probe_masking_msrs(void)
> +{
> +	const struct cpuinfo_x86 *c = &boot_cpu_data;
> +	unsigned int exp_msr_basic = 0, exp_msr_ext = 0, exp_msr_xsave = 0;
>  
>  	/* Only family 6 supports this feature. */
> -	if (c->x86 != 6) {
> -		printk("No CPUID feature masking support available\n");
> +	if (c->x86 != 6)
>  		return;
> -	}
>  
>  	switch (c->x86_model) {
>  	case 0x17: /* Yorkfield, Wolfdale, Penryn, Harpertown(DP) */
>  	case 0x1d: /* Dunnington(MP) */
> -		msr_basic = MSR_INTEL_MASK_V1_CPUID1;
> +		exp_msr_basic = msr_basic = MSR_INTEL_MASK_V1_CPUID1;
>  		break;
>  
>  	case 0x1a: /* Bloomfield, Nehalem-EP(Gainestown) */
> @@ -88,71 +100,126 @@ static void set_cpuidmask(const struct cpuinfo_x86 *c)
>  	case 0x2c: /* Gulftown, Westmere-EP */
>  	case 0x2e: /* Nehalem-EX(Beckton) */
>  	case 0x2f: /* Westmere-EX */
> -		msr_basic = MSR_INTEL_MASK_V2_CPUID1;
> -		msr_ext   = MSR_INTEL_MASK_V2_CPUID80000001;
> +		exp_msr_basic = msr_basic = MSR_INTEL_MASK_V2_CPUID1;
> +		exp_msr_ext   = msr_ext   = MSR_INTEL_MASK_V2_CPUID80000001;
>  		break;
>  
>  	case 0x2a: /* SandyBridge */
>  	case 0x2d: /* SandyBridge-E, SandyBridge-EN, SandyBridge-EP */
> -		msr_basic = MSR_INTEL_MASK_V3_CPUID1;
> -		msr_ext   = MSR_INTEL_MASK_V3_CPUID80000001;
> -		msr_xsave = MSR_INTEL_MASK_V3_CPUIDD_01;
> +		exp_msr_basic = msr_basic = MSR_INTEL_MASK_V3_CPUID1;
> +		exp_msr_ext   = msr_ext   = MSR_INTEL_MASK_V3_CPUID80000001;
> +		exp_msr_xsave = msr_xsave = MSR_INTEL_MASK_V3_CPUIDD_01;
>  		break;
>  	}

Instead of all these changes, and instead of the variable needing
initializers, you could simply initialize all three ext_msr_* right after
the switch().

> +static void intel_ctxt_switch_levelling(const struct domain *nextd)
> +{
> +	struct cpuidmasks *these_masks = &this_cpu(cpuidmasks);
> +	const struct cpuidmasks *masks = &cpuidmask_defaults;
> +
> +#define LAZY(msr, field)						\
> +	({								\
> +		if (msr && (these_masks->field != masks->field))	\
> +		{							\
> +			wrmsrl(msr, masks->field);			\
> +			these_masks->field = masks->field;		\
> +		}							\
> +	})
> +
> +	LAZY(msr_basic, _1cd);
> +	LAZY(msr_ext,   e1cd);
> +	LAZY(msr_xsave, Da1);

Please either use token concatenation inside the macro body to
eliminate the redundant msr_ prefixes here, or properly
parenthesize the uses of "msr" inside the macro body.

> +	if (opt_cpu_info) {
> +		printk(XENLOG_INFO "Levelling caps: %#x\n", levelling_caps);
> +		printk(XENLOG_INFO
> +		       "MSR defaults: 1d 0x%08x, 1c 0x%08x, e1d 0x%08x, "
> +		       "e1c 0x%08x, Da1 0x%08x\n",
> +		       (uint32_t)(cpuidmask_defaults._1cd >> 32),
> +		       (uint32_t)cpuidmask_defaults._1cd,
> +		       (uint32_t)(cpuidmask_defaults.e1cd >> 32),
> +		       (uint32_t)cpuidmask_defaults.e1cd,
> +		       (uint32_t)cpuidmask_defaults.Da1);

Could I convince you to make this second printk() dependent
upon there not being CPUID faulting support?

> @@ -190,22 +257,13 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>  	    (boot_cpu_data.x86_mask == 3 || boot_cpu_data.x86_mask == 4))
>  		paddr_bits = 36;
>  
> -	if (c == &boot_cpu_data && c->x86 == 6) {
> -		if (probe_intel_cpuid_faulting())
> -			__set_bit(X86_FEATURE_CPUID_FAULTING,
> -				  c->x86_capability);
> -	} else if (boot_cpu_has(X86_FEATURE_CPUID_FAULTING)) {
> -		BUG_ON(!probe_intel_cpuid_faulting());
> -		__set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability);
> -	}
> +	if (c == &boot_cpu_data)
> +		intel_init_levelling();
> +
> +	if (test_bit(X86_FEATURE_CPUID_FAULTING, boot_cpu_data.x86_capability))
> +            __set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability);

Mixing tabs and spaces for indentation.

Jan