Re: [PATCH RFC 27/31] xen/x86: Rework Intel masking/faulting setup

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/01/16 09:40, Jan Beulich wrote:
>>>> On 16.12.15 at 22:24, <andrew.cooper3@citrix.com> wrote:
>> +	if (msr_basic)
>> +		__probe_mask_msr(&msr_basic, LCAP_1cd, &cpumask_defaults._1cd);
>> +
>> +	if (msr_ext)
>> +		__probe_mask_msr(&msr_ext, LCAP_e1cd, &cpumask_defaults.e1cd);
>> +
>> +	if (msr_xsave)
>> +		__probe_mask_msr(&msr_xsave, LCAP_Da1, &cpumask_defaults.Da1);
>> +
>> +	/*
>> +	 * Don't bother warning about a mismatch if virtualised.  These MSRs
>> +	 * are not architectural and almost never virtualised.
>> +	 */
>> +	if ((expected_levelling_cap == levelling_caps) ||
>> +	    cpu_has_hypervisor)
>> +		return;
>> +
>> +	printk(XENLOG_WARNING "Mismatch between expected (%#x"
>> +	       ") and real (%#x) levelling caps: missing %#x\n",
>> +	       expected_levelling_cap, levelling_caps,
>> +	       (expected_levelling_cap ^ levelling_caps) & levelling_caps);
>> +	printk(XENLOG_WARNING "Fam %#x, model %#x expected (%#x/%#x/%#x), "
>> +	       "got (%#x/%#x/%#x)\n", c->x86, c->x86_model,
>> +	       exp_msr_basic, exp_msr_ext, exp_msr_xsave,
>> +	       msr_basic, msr_ext, msr_xsave);
> I may not have noticed the same on the AMD patch, but printing
> zeros as "missing" MSR indexes seems strange to me. Why not
> print the missing MSRs with their textual names, easing cross
> referencing with the FlexMigration document?

AMD and Intel are different in this regard.  AMD's masking MSRs are at
fixed addresses, while Intel's vary MSR address by generation, which is
why I stored the probed address in a variable.

There are not consistent names available.  In this case, I think the
numbers are actually clearer than the names.

>
>> +/*
>> + * 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.
>> + */
>> +static void __init noinline intel_init_levelling(void)
>> +{
>> +	if ( !probe_intel_cpuid_faulting() )
>> +		probe_masking_msrs();
>> +
>> +	if ( msr_basic )
>> +	{
>> +		cpumask_defaults._1cd =
>> +			((u64)opt_cpuid_mask_edx << 32) | opt_cpuid_mask_ecx;
>> +
>> +		if ( !~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx) )
>>  			printk("Writing CPUID feature mask ecx:edx -> %08x:%08x\n",
>>  			       opt_cpuid_mask_ecx, opt_cpuid_mask_edx);
> Are these messages, without adjustment to their wording, not
> going to be confusing? After all the intention is to not just write
> a single, never modified value. E.g. better "Defaulting ..."?

I will change the wording.  It would be better than confusing a new
different meaning with the old written message.

>
>> @@ -183,22 +237,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);
> So you intentionally delete the validation of CPUID faulting being
> available on APs?

Yes.  All this does is change where Xen crashes, in the case that AP's
have different capabilities to the BSP, and allows more startup code to
move into __init.

~Andrew