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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/02/16 07:57, Jan Beulich wrote:
>
>> +/* 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".

"Indices" is what I meant.

>
>> +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().

That would certainly be neater.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel