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

2016-01-22 Thread Jan Beulich 
>>> On 16.12.15 at 22:24,  wrote:
> + if (msr_basic)
> + __probe_mask_msr(_basic, LCAP_1cd, _defaults._1cd);
> +
> + if (msr_ext)
> + __probe_mask_msr(_ext, LCAP_e1cd, _defaults.e1cd);
> +
> + if (msr_xsave)
> + __probe_mask_msr(_xsave, LCAP_Da1, _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?

> +/*
> + * 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 ..."?

> @@ -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 == _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 == _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? Also - indentation.

Jan


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

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

2016-01-22 Thread Andrew Cooper 
On 22/01/16 09:40, Jan Beulich wrote:
 On 16.12.15 at 22:24,  wrote:
>> +if (msr_basic)
>> +__probe_mask_msr(_basic, LCAP_1cd, _defaults._1cd);
>> +
>> +if (msr_ext)
>> +__probe_mask_msr(_ext, LCAP_e1cd, _defaults.e1cd);
>> +
>> +if (msr_xsave)
>> +__probe_mask_msr(_xsave, LCAP_Da1, _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 == _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 == _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

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

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

2016-01-22 Thread Jan Beulich 
>>> On 22.01.16 at 15:09,  wrote:
> On 22/01/16 09:40, Jan Beulich wrote:
> On 16.12.15 at 22:24,  wrote:
>>> @@ -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 == _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 == _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.

So where did that Xen crash point move to (since I didn't spot it)?

Jan


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

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

2016-01-22 Thread Jan Beulich 
>>> On 22.01.16 at 15:46,  wrote:
> On 22/01/16 14:29, Jan Beulich wrote:
> On 22.01.16 at 15:09,  wrote:
>>> On 22/01/16 09:40, Jan Beulich wrote:
>>> On 16.12.15 at 22:24,  wrote:
> @@ -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 == _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 == _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.
>> So where did that Xen crash point move to (since I didn't spot it)?
> 
> set_cpuid_faulting() doesn't use safe MSR accesses, so would crash on
> first use in a mixed setup.

Oh, that's pretty implicit.

> I could extend it to ASSERT(cpu_has_cpuid_faulting) if you would prefer.

If you don't mind, I'd indeed prefer this to be explicit (perhaps
even using BUG_ON() instead of ASSERT()).

Jan


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

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

2016-01-22 Thread Andrew Cooper 
On 22/01/16 14:29, Jan Beulich wrote:
 On 22.01.16 at 15:09,  wrote:
>> On 22/01/16 09:40, Jan Beulich wrote:
>> On 16.12.15 at 22:24,  wrote:
 @@ -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 == _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 == _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.
> So where did that Xen crash point move to (since I didn't spot it)?

set_cpuid_faulting() doesn't use safe MSR accesses, so would crash on
first use in a mixed setup.

I could extend it to ASSERT(cpu_has_cpuid_faulting) if you would prefer.

~Andrew

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

[Xen-devel] [PATCH RFC 27/31] xen/x86: Rework Intel masking/faulting setup

2015-12-16 Thread Andrew Cooper 
This patch is best reviewed as its end result rather than as a diff, as it
rewrites almost all of the setup.

On the BSP, cpuid information is used to evaluate the potential available set
of masking MSRs, and they are unconditionally probed, filling in the
availability information and hardware defaults.  A side effect of this is that
probe_intel_cpuid_faulting() can move to being __init.

The command line parameters are then combined with the hardware defaults to
further restrict the Xen default masking level.  Each cpu is then context
switched into the default levelling state.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
---
 xen/arch/x86/cpu/intel.c | 219 ---
 1 file changed, 132 insertions(+), 87 deletions(-)

diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index d251b53..f1bb222 100644
--- 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;
+
+   expected_levelling_cap |= LCAP_faulting;
+   levelling_caps |=  LCAP_faulting;
+   __set_bit(X86_FEATURE_CPUID_FAULTING, boot_cpu_data.x86_capability);
+   return 1;
 }
 
 static DEFINE_PER_CPU(bool_t, cpuid_faulting_enabled);
@@ -44,41 +51,45 @@ 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 __devinit 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;
+   }
+}
 
-   ASSERT((status == not_parsed) && (c == _cpu_data));
-   status = no_mask;
+static unsigned int __read_mostly msr_basic, msr_ext, msr_xsave;
 
-   if (!~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx &
-  opt_cpuid_mask_ext_ecx & opt_cpuid_mask_ext_edx &
-  opt_cpuid_mask_xsave_eax))
-   return;
+/*
+ * Probe for the existance of the expected masking MSRs.  They might easily
+ * not be available if Xen is running virtualised.
+ */
+static void __init probe_masking_msrs(void)
+{
+   const struct cpuinfo_x86 *c = _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,72 +99,115 @@ static void __devinit 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_CPUID8001;
+   exp_msr_basic = msr_basic = MSR_INTEL_MASK_V2_CPUID1;
+   exp_msr_ext   = msr_ext   = MSR_INTEL_MASK_V2_CPUID8001;
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_CPUID8001;
-   msr_xsave = MSR_INTEL_MASK_V3_CPUIDD_01;
+   exp_msr_basic =