Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup

2016-01-25 Thread Jan Beulich 
>>> On 22.01.16 at 18:03,  wrote:
> On 22/01/16 14:12, Jan Beulich wrote:
>>
>> And then, how is this supposed to work? You only restore defaults,
>> but never write non-default values. Namely, nextd is an unused
>> function parameter ...
>>
>> Also I guess my comment about adding unused code needs
>> repeating here.
> Future patches build on this, both using the parameter, and not using
> the defaults.
>
> I am also certain that if I did two patches, the first putting in a void
> function, and the second changing it to a pointer, your review would ask
> me to turn it into this.
 Well, I realize things will all make sense by the end of the series, but
 honestly in some of the cases I'm not sure the split between patches
 was well done.
>>> If you can suggest a better ordering then I am all ears.
>> For example, move all the context switch logic into the patch
>> actually invoking the new hook. That still leaves more than
>> enough in the AMD and Intel rework patches.
> 
> But the context switch logic is used by this patch, which is why it is
> introduced here.
> 
> It takes the BSP/AP from the boot state into the default levelled state,
> by passing NULL as the pointer.  See the final hunk, which modifies
> early_init_amd().

Ah, right. Goes back to me not recognizing the dual purpose of that
function (as noted elsewhere in reply to some of your explanations).

Jan


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

Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup

2016-01-22 Thread Jan Beulich 
>>> On 16.12.15 at 22:24,  wrote:
> This patch is best reviewed as its end result rather than as a diff, as it
> rewrites almost all of the setup.

This, I think, doesn't belong in the commit message itself.

> @@ -126,126 +133,172 @@ static const struct cpuidmask *__init noinline 
> get_cpuidmask(const char *opt)
>  }
>  
>  /*
> - * Mask the features and extended features returned by CPUID.  Parameters are
> - * set from the boot line via two methods:
> - *
> - *   1) Specific processor revision string
> - *   2) User-defined masks
> - *
> - * The processor revision string parameter has precedene.
> + * Sets caps in expected_levelling_cap, probes for the specified mask MSR, 
> and
> + * set caps in levelling_caps if it is found.  Processors prior to Fam 10h
> + * required a 32-bit password for masking MSRs.  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 feat_ecx, feat_edx;
> - static unsigned int extfeat_ecx, extfeat_edx;
> - static unsigned int l7s0_eax, l7s0_ebx;
> - static unsigned int thermal_ecx;
> - static bool_t skip_feat, skip_extfeat;
> - static bool_t skip_l7s0_eax_ebx, skip_thermal_ecx;
> - static enum { not_parsed, no_mask, set_mask } status;
> - unsigned int eax, ebx, ecx, edx;
> -
> - if (status == no_mask)
> - return;
> + unsigned int hi, lo;
> +
> +expected_levelling_cap |= caps;

Mixing indentation styles.

> +
> + if ((rdmsr_amd_safe(msr, , ) == 0) &&
> + (wrmsr_amd_safe(msr, lo, hi) == 0))
> + levelling_caps |= caps;

This logic assumes that faults are possible only because the MSR is
not there at all, not because of the actual value used. Is this a safe
assumption, the more that you don't use the "safe" variants
anymore at runtime?

> +/*
> + * Probe for the existance of the expected masking MSRs.  They might easily
> + * not be available if Xen is running virtualised.
> + */
> +static void __init noinline probe_masking_msrs(void)
> +{
> + const struct cpuinfo_x86 *c = _cpu_data;
>  
> - ASSERT((status == not_parsed) && (c == _cpu_data));
> - status = no_mask;
> + /*
> +  * First, work out which masking MSRs we should have, based on
> +  * revision and cpuid.
> +  */
>  
>   /* Fam11 doesn't support masking at all. */
>   if (c->x86 == 0x11)
>   return;
>  
> - if (~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx &
> -   opt_cpuid_mask_ext_ecx & opt_cpuid_mask_ext_edx &
> -   opt_cpuid_mask_l7s0_eax & opt_cpuid_mask_l7s0_ebx &
> -   opt_cpuid_mask_thermal_ecx)) {
> - feat_ecx = opt_cpuid_mask_ecx;
> - feat_edx = opt_cpuid_mask_edx;
> - extfeat_ecx = opt_cpuid_mask_ext_ecx;
> - extfeat_edx = opt_cpuid_mask_ext_edx;
> - l7s0_eax = opt_cpuid_mask_l7s0_eax;
> - l7s0_ebx = opt_cpuid_mask_l7s0_ebx;
> - thermal_ecx = opt_cpuid_mask_thermal_ecx;
> - } else if (*opt_famrev == '\0') {
> + __probe_mask_msr(MSR_K8_FEATURE_MASK, LCAP_1cd,
> +  _defaults._1cd);
> + __probe_mask_msr(MSR_K8_EXT_FEATURE_MASK, LCAP_e1cd,
> +  _defaults.e1cd);
> +
> + if (c->cpuid_level >= 7)
> + __probe_mask_msr(MSR_AMD_L7S0_FEATURE_MASK, LCAP_7ab0,
> +  _defaults._7ab0);
> +
> + if (c->x86 == 0x15 && c->cpuid_level >= 6 && cpuid_ecx(6))
> + __probe_mask_msr(MSR_AMD_THRM_FEATURE_MASK, LCAP_6c,
> +  _defaults._6c);
> +
> + /*
> +  * 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;
> - } else {
> - const struct cpuidmask *m = get_cpuidmask(opt_famrev);
> +
> + printk(XENLOG_WARNING "Mismatch between expected (%#x"
> +") and real (%#x) levelling caps: missing %#x\n",

Breaking a format string inside parentheses is certainly a little odd.
Also I don't think the "missing" part is really useful, considering that
you already print both values used in its calculation.

> +expected_levelling_cap, levelling_caps,
> +(expected_levelling_cap ^ levelling_caps) & levelling_caps);
> + printk(XENLOG_WARNING "Fam %#x, model %#x level %#x\n",
> +c->x86, c->x86_model, c->cpuid_level);
> + printk(XENLOG_WARNING
> +"If not running virtualised, please report a bug\n");

Well - you checked for running virtualized, so making it here when
running virtualized is already a bug (albeit not in the code here)?

> +void

Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup

2016-01-22 Thread Andrew Cooper 
On 22/01/16 09:27, Jan Beulich wrote:
 On 16.12.15 at 22:24,  wrote:
>> This patch is best reviewed as its end result rather than as a diff, as it
>> rewrites almost all of the setup.
> This, I think, doesn't belong in the commit message itself.

Why not? It applies equally to anyone reading the commit in tree.

>
>> @@ -126,126 +133,172 @@ static const struct cpuidmask *__init noinline 
>> get_cpuidmask(const char *opt)
>>  }
>>  
>>  /*
>> - * Mask the features and extended features returned by CPUID.  Parameters 
>> are
>> - * set from the boot line via two methods:
>> - *
>> - *   1) Specific processor revision string
>> - *   2) User-defined masks
>> - *
>> - * The processor revision string parameter has precedene.
>> + * Sets caps in expected_levelling_cap, probes for the specified mask MSR, 
>> and
>> + * set caps in levelling_caps if it is found.  Processors prior to Fam 10h
>> + * required a 32-bit password for masking MSRs.  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 feat_ecx, feat_edx;
>> -static unsigned int extfeat_ecx, extfeat_edx;
>> -static unsigned int l7s0_eax, l7s0_ebx;
>> -static unsigned int thermal_ecx;
>> -static bool_t skip_feat, skip_extfeat;
>> -static bool_t skip_l7s0_eax_ebx, skip_thermal_ecx;
>> -static enum { not_parsed, no_mask, set_mask } status;
>> -unsigned int eax, ebx, ecx, edx;
>> -
>> -if (status == no_mask)
>> -return;
>> +unsigned int hi, lo;
>> +
>> +expected_levelling_cap |= caps;
> Mixing indentation styles.

Yes - sadly some crept in, despite my best efforts.  I have already
fixed all I can find in the series.

>
>> +
>> +if ((rdmsr_amd_safe(msr, , ) == 0) &&
>> +(wrmsr_amd_safe(msr, lo, hi) == 0))
>> +levelling_caps |= caps;
> This logic assumes that faults are possible only because the MSR is
> not there at all, not because of the actual value used. Is this a safe
> assumption, the more that you don't use the "safe" variants
> anymore at runtime?

Yes - it is a safe assumption.

The MSRs on AMD are at fixed indices and all specified to cover the full
32bit of a cpuid feature leaf.  All we really care about is whether the
MSR is present (and to read its defaults, if it is).

If the MSR isn't present, levelling_caps won't be updated, and Xen will
never touch the MSR again.

>
>> +/*
>> + * Probe for the existance of the expected masking MSRs.  They might easily
>> + * not be available if Xen is running virtualised.
>> + */
>> +static void __init noinline probe_masking_msrs(void)
>> +{
>> +const struct cpuinfo_x86 *c = _cpu_data;
>>  
>> -ASSERT((status == not_parsed) && (c == _cpu_data));
>> -status = no_mask;
>> +/*
>> + * First, work out which masking MSRs we should have, based on
>> + * revision and cpuid.
>> + */
>>  
>>  /* Fam11 doesn't support masking at all. */
>>  if (c->x86 == 0x11)
>>  return;
>>  
>> -if (~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx &
>> -  opt_cpuid_mask_ext_ecx & opt_cpuid_mask_ext_edx &
>> -  opt_cpuid_mask_l7s0_eax & opt_cpuid_mask_l7s0_ebx &
>> -  opt_cpuid_mask_thermal_ecx)) {
>> -feat_ecx = opt_cpuid_mask_ecx;
>> -feat_edx = opt_cpuid_mask_edx;
>> -extfeat_ecx = opt_cpuid_mask_ext_ecx;
>> -extfeat_edx = opt_cpuid_mask_ext_edx;
>> -l7s0_eax = opt_cpuid_mask_l7s0_eax;
>> -l7s0_ebx = opt_cpuid_mask_l7s0_ebx;
>> -thermal_ecx = opt_cpuid_mask_thermal_ecx;
>> -} else if (*opt_famrev == '\0') {
>> +__probe_mask_msr(MSR_K8_FEATURE_MASK, LCAP_1cd,
>> + _defaults._1cd);
>> +__probe_mask_msr(MSR_K8_EXT_FEATURE_MASK, LCAP_e1cd,
>> + _defaults.e1cd);
>> +
>> +if (c->cpuid_level >= 7)
>> +__probe_mask_msr(MSR_AMD_L7S0_FEATURE_MASK, LCAP_7ab0,
>> + _defaults._7ab0);
>> +
>> +if (c->x86 == 0x15 && c->cpuid_level >= 6 && cpuid_ecx(6))
>> +__probe_mask_msr(MSR_AMD_THRM_FEATURE_MASK, LCAP_6c,
>> + _defaults._6c);
>> +
>> +/*
>> + * 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;
>> -} else {
>> -const struct cpuidmask *m = get_cpuidmask(opt_famrev);
>> +
>> +printk(XENLOG_WARNING "Mismatch between expected (%#x"
>> +   ") and real (%#x) levelling caps: missing %#x\n",
> Breaking a format string inside parentheses is certainly a little odd.
> Also I don't think

Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup

2016-01-22 Thread Jan Beulich 
>>> On 22.01.16 at 12:01,  wrote:
> On 22/01/16 09:27, Jan Beulich wrote:
> On 16.12.15 at 22:24,  wrote:
>>> +  expected_levelling_cap, levelling_caps,
>>> +  (expected_levelling_cap ^ levelling_caps) & levelling_caps);
>>> +   printk(XENLOG_WARNING "Fam %#x, model %#x level %#x\n",
>>> +  c->x86, c->x86_model, c->cpuid_level);
>>> +   printk(XENLOG_WARNING
>>> +  "If not running virtualised, please report a bug\n");
>> Well - you checked for running virtualized, so making it here when
>> running virtualized is already a bug (albeit not in the code here)?
> 
> Why would it be a bug?  The virtualising environment might provide these
> MSRs, in which case we should use them.

Because you won't make it here when cpu_has_hypervisor.

>>> +void amd_ctxt_switch_levelling(const struct domain *nextd)
>>> +{
>>> +   struct cpumasks *these_masks = _cpu(cpumasks);
>>> +   const struct cpumasks *masks = _defaults;
>>> +
>>> +#define LAZY(cap, msr, field)  
>>> \
>>> +   ({  \
>>> +   if ( ((levelling_caps & cap) == cap) && \
>>> +(these_masks->field != masks->field) ) \
>>> +   {   \
>>> +   wrmsr_amd(msr, masks->field);   \
>>> +   these_masks->field = masks->field;  \
>>> +   }   \
>>> +   })
>>> +
>>> +   LAZY(LCAP_1cd,  MSR_K8_FEATURE_MASK,   _1cd);
>>> +   LAZY(LCAP_e1cd, MSR_K8_EXT_FEATURE_MASK,   e1cd);
>>> +   LAZY(LCAP_7ab0, MSR_AMD_L7S0_FEATURE_MASK, _7ab0);
>>> +   LAZY(LCAP_6c,   MSR_AMD_THRM_FEATURE_MASK, _6c);
>> So here we already have the first example where fully consistent
>> naming would allow elimination of a macro parameter.
> 
> Token concatenation deliberately obscures code from tool like grep and
> cscope.  There is already too much of the Xen source code obscured like
> this; I'd prefer not to add to it.

I'm not sure grep-abilty is more important than code readability.
My personal opinion surely is that the latter is more important.

>> And then, how is this supposed to work? You only restore defaults,
>> but never write non-default values. Namely, nextd is an unused
>> function parameter ...
>>
>> Also I guess my comment about adding unused code needs
>> repeating here.
> 
> Future patches build on this, both using the parameter, and not using
> the defaults.
> 
> I am also certain that if I did two patches, the first putting in a void
> function, and the second changing it to a pointer, your review would ask
> me to turn it into this.

Well, I realize things will all make sense by the end of the series, but
honestly in some of the cases I'm not sure the split between patches
was well done. But just to be clear, none of the related comments
mean I would outright reject any of the patches just for that reason.

Jan


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

Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup

2016-01-22 Thread Andrew Cooper 
On 22/01/16 14:12, Jan Beulich wrote:
>
> And then, how is this supposed to work? You only restore defaults,
> but never write non-default values. Namely, nextd is an unused
> function parameter ...
>
> Also I guess my comment about adding unused code needs
> repeating here.
 Future patches build on this, both using the parameter, and not using
 the defaults.

 I am also certain that if I did two patches, the first putting in a void
 function, and the second changing it to a pointer, your review would ask
 me to turn it into this.
>>> Well, I realize things will all make sense by the end of the series, but
>>> honestly in some of the cases I'm not sure the split between patches
>>> was well done.
>> If you can suggest a better ordering then I am all ears.
> For example, move all the context switch logic into the patch
> actually invoking the new hook. That still leaves more than
> enough in the AMD and Intel rework patches.

But the context switch logic is used by this patch, which is why it is
introduced here.

It takes the BSP/AP from the boot state into the default levelled state,
by passing NULL as the pointer.  See the final hunk, which modifies
early_init_amd().

~Andrew

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

Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup

2016-01-22 Thread Andrew Cooper 
On 22/01/16 11:13, Jan Beulich wrote:
 On 22.01.16 at 12:01,  wrote:
>> On 22/01/16 09:27, Jan Beulich wrote:
>> On 16.12.15 at 22:24,  wrote:
 + expected_levelling_cap, levelling_caps,
 + (expected_levelling_cap ^ levelling_caps) & levelling_caps);
 +  printk(XENLOG_WARNING "Fam %#x, model %#x level %#x\n",
 + c->x86, c->x86_model, c->cpuid_level);
 +  printk(XENLOG_WARNING
 + "If not running virtualised, please report a bug\n");
>>> Well - you checked for running virtualized, so making it here when
>>> running virtualized is already a bug (albeit not in the code here)?
>> Why would it be a bug?  The virtualising environment might provide these
>> MSRs, in which case we should use them.
> Because you won't make it here when cpu_has_hypervisor.

Not all hypervisors advertise themselves with the hypervisor bit.  Some
versions of HyperV and ESXi will panic if they see a hypervisor bit, so
virtualisation software needs to hide the hypervisor bit to successfully
visualise some software.

The user reading Xen's error message is in a better position to judge
when Xen is actually virtualised, because Xen is not able to say with
certainty.

>
 +void amd_ctxt_switch_levelling(const struct domain *nextd)
 +{
 +  struct cpumasks *these_masks = _cpu(cpumasks);
 +  const struct cpumasks *masks = _defaults;
 +
 +#define LAZY(cap, msr, field) 
 \
 +  ({  \
 +  if ( ((levelling_caps & cap) == cap) && \
 +   (these_masks->field != masks->field) ) \
 +  {   \
 +  wrmsr_amd(msr, masks->field);   \
 +  these_masks->field = masks->field;  \
 +  }   \
 +  })
 +
 +  LAZY(LCAP_1cd,  MSR_K8_FEATURE_MASK,   _1cd);
 +  LAZY(LCAP_e1cd, MSR_K8_EXT_FEATURE_MASK,   e1cd);
 +  LAZY(LCAP_7ab0, MSR_AMD_L7S0_FEATURE_MASK, _7ab0);
 +  LAZY(LCAP_6c,   MSR_AMD_THRM_FEATURE_MASK, _6c);
>>> So here we already have the first example where fully consistent
>>> naming would allow elimination of a macro parameter.
>> Token concatenation deliberately obscures code from tool like grep and
>> cscope.  There is already too much of the Xen source code obscured like
>> this; I'd prefer not to add to it.
> I'm not sure grep-abilty is more important than code readability.
> My personal opinion surely is that the latter is more important.

It doesn't matter how readable the code is if you can't find it again later.

And in this case, the difference between 2 and 3 macro parameters
doesn't affect the readability of the code.  All context is available in
the preceding 10 lines.

>
>>> And then, how is this supposed to work? You only restore defaults,
>>> but never write non-default values. Namely, nextd is an unused
>>> function parameter ...
>>>
>>> Also I guess my comment about adding unused code needs
>>> repeating here.
>> Future patches build on this, both using the parameter, and not using
>> the defaults.
>>
>> I am also certain that if I did two patches, the first putting in a void
>> function, and the second changing it to a pointer, your review would ask
>> me to turn it into this.
> Well, I realize things will all make sense by the end of the series, but
> honestly in some of the cases I'm not sure the split between patches
> was well done.

If you can suggest a better ordering then I am all ears.

~Andrew

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

Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup

2016-01-22 Thread Jan Beulich 
>>> On 22.01.16 at 14:59,  wrote:
> On 22/01/16 11:13, Jan Beulich wrote:
> On 22.01.16 at 12:01,  wrote:
>>> On 22/01/16 09:27, Jan Beulich wrote:
>>> On 16.12.15 at 22:24,  wrote:
> +expected_levelling_cap, levelling_caps,
> +(expected_levelling_cap ^ levelling_caps) & levelling_caps);
> + printk(XENLOG_WARNING "Fam %#x, model %#x level %#x\n",
> +c->x86, c->x86_model, c->cpuid_level);
> + printk(XENLOG_WARNING
> +"If not running virtualised, please report a bug\n");
 Well - you checked for running virtualized, so making it here when
 running virtualized is already a bug (albeit not in the code here)?
>>> Why would it be a bug?  The virtualising environment might provide these
>>> MSRs, in which case we should use them.
>> Because you won't make it here when cpu_has_hypervisor.
> 
> Not all hypervisors advertise themselves with the hypervisor bit.  Some
> versions of HyperV and ESXi will panic if they see a hypervisor bit, so
> virtualisation software needs to hide the hypervisor bit to successfully
> visualise some software.

Well, but that's the bug I talk of: Not setting that CPUID bit is
not following the spec, and forcing it off to make some code work
in a virtualized environment isn't much better.

> The user reading Xen's error message is in a better position to judge
> when Xen is actually virtualised, because Xen is not able to say with
> certainty.

Okay, okay, bit that way then.

 And then, how is this supposed to work? You only restore defaults,
 but never write non-default values. Namely, nextd is an unused
 function parameter ...

 Also I guess my comment about adding unused code needs
 repeating here.
>>> Future patches build on this, both using the parameter, and not using
>>> the defaults.
>>>
>>> I am also certain that if I did two patches, the first putting in a void
>>> function, and the second changing it to a pointer, your review would ask
>>> me to turn it into this.
>> Well, I realize things will all make sense by the end of the series, but
>> honestly in some of the cases I'm not sure the split between patches
>> was well done.
> 
> If you can suggest a better ordering then I am all ears.

For example, move all the context switch logic into the patch
actually invoking the new hook. That still leaves more than
enough in the AMD and Intel rework patches.

Jan


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

[Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR 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.

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/amd.c | 250 ++---
 1 file changed, 153 insertions(+), 97 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 265fbc0..6b95ab6 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -80,6 +80,13 @@ static inline int wrmsr_amd_safe(unsigned int msr, unsigned 
int lo,
return err;
 }
 
+static void wrmsr_amd(unsigned int msr, uint64_t val)
+{
+   asm volatile("wrmsr" ::
+"c" (msr), "a" ((uint32_t)val),
+"d" (val >> 32), "D" (0x9c5a203a));
+}
+
 static const struct cpuidmask {
uint16_t fam;
char rev[2];
@@ -126,126 +133,172 @@ static const struct cpuidmask *__init noinline 
get_cpuidmask(const char *opt)
 }
 
 /*
- * Mask the features and extended features returned by CPUID.  Parameters are
- * set from the boot line via two methods:
- *
- *   1) Specific processor revision string
- *   2) User-defined masks
- *
- * The processor revision string parameter has precedene.
+ * Sets caps in expected_levelling_cap, probes for the specified mask MSR, and
+ * set caps in levelling_caps if it is found.  Processors prior to Fam 10h
+ * required a 32-bit password for masking MSRs.  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 feat_ecx, feat_edx;
-   static unsigned int extfeat_ecx, extfeat_edx;
-   static unsigned int l7s0_eax, l7s0_ebx;
-   static unsigned int thermal_ecx;
-   static bool_t skip_feat, skip_extfeat;
-   static bool_t skip_l7s0_eax_ebx, skip_thermal_ecx;
-   static enum { not_parsed, no_mask, set_mask } status;
-   unsigned int eax, ebx, ecx, edx;
-
-   if (status == no_mask)
-   return;
+   unsigned int hi, lo;
+
+expected_levelling_cap |= caps;
+
+   if ((rdmsr_amd_safe(msr, , ) == 0) &&
+   (wrmsr_amd_safe(msr, lo, hi) == 0))
+   levelling_caps |= caps;
+
+   *msr_val = ((uint64_t)hi << 32) | lo;
+}
 
-   if (status == set_mask)
-   goto setmask;
+/*
+ * Probe for the existance of the expected masking MSRs.  They might easily
+ * not be available if Xen is running virtualised.
+ */
+static void __init noinline probe_masking_msrs(void)
+{
+   const struct cpuinfo_x86 *c = _cpu_data;
 
-   ASSERT((status == not_parsed) && (c == _cpu_data));
-   status = no_mask;
+   /*
+* First, work out which masking MSRs we should have, based on
+* revision and cpuid.
+*/
 
/* Fam11 doesn't support masking at all. */
if (c->x86 == 0x11)
return;
 
-   if (~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx &
- opt_cpuid_mask_ext_ecx & opt_cpuid_mask_ext_edx &
- opt_cpuid_mask_l7s0_eax & opt_cpuid_mask_l7s0_ebx &
- opt_cpuid_mask_thermal_ecx)) {
-   feat_ecx = opt_cpuid_mask_ecx;
-   feat_edx = opt_cpuid_mask_edx;
-   extfeat_ecx = opt_cpuid_mask_ext_ecx;
-   extfeat_edx = opt_cpuid_mask_ext_edx;
-   l7s0_eax = opt_cpuid_mask_l7s0_eax;
-   l7s0_ebx = opt_cpuid_mask_l7s0_ebx;
-   thermal_ecx = opt_cpuid_mask_thermal_ecx;
-   } else if (*opt_famrev == '\0') {
+   __probe_mask_msr(MSR_K8_FEATURE_MASK, LCAP_1cd,
+_defaults._1cd);
+   __probe_mask_msr(MSR_K8_EXT_FEATURE_MASK, LCAP_e1cd,
+_defaults.e1cd);
+
+   if (c->cpuid_level >= 7)
+   __probe_mask_msr(MSR_AMD_L7S0_FEATURE_MASK, LCAP_7ab0,
+_defaults._7ab0);
+
+   if (c->x86 == 0x15 && c->cpuid_level >= 6 && cpuid_ecx(6))
+   __probe_mask_msr(MSR_AMD_THRM_FEATURE_MASK, LCAP_6c,
+_defaults._6c);
+
+   /*
+* 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;
-   } else {
-   const struct cpuidmask *m = get_cpuidmask(opt_famrev);
+
+