[PATCH 1/2] x86/cpu: Introduce cpu_dev.c_early_init()

[PATCH 1/2] x86/cpu: Introduce cpu_dev.c_early_init()

[Andrew Cooper]

The name is chosen to be consistent with Linux.  Doing this allows
early_intel_workaround() to be removed from common code.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/cpu/common.c | 17 +++++++++++------
 xen/arch/x86/cpu/cpu.h    |  3 +--
 xen/arch/x86/cpu/intel.c  |  5 ++---
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index e60929d..6b320a7 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -226,10 +226,8 @@ static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
 	/* Initialize the standard set of capabilities */
 	/* Note that the vendor-specific code below might override */
 
-	/* Intel-defined flags: level 0x00000001 */
+	/* Model and family information. */
 	cpuid(0x00000001, &eax, &ebx, &ecx, &edx);
-	c->x86_capability[cpufeat_word(X86_FEATURE_FPU)] = edx;
-	c->x86_capability[cpufeat_word(X86_FEATURE_XMM3)] = ecx;
 	c->x86 = (eax >> 8) & 15;
 	c->x86_model = (eax >> 4) & 15;
 	if (c->x86 == 0xf)
@@ -239,6 +237,16 @@ static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
 	c->x86_mask = eax & 15;
 	c->apicid = phys_pkg_id((ebx >> 24) & 0xFF, 0);
 	c->phys_proc_id = c->apicid;
+
+	if (this_cpu->c_early_init)
+		this_cpu->c_early_init(c);
+
+	/* c_early_init() may have adjusted cpuid levels/features.  Reread. */
+	c->cpuid_level = cpuid_eax(0);
+	cpuid(0x00000001, &eax, &ebx, &ecx, &edx);
+	c->x86_capability[cpufeat_word(X86_FEATURE_FPU)] = edx;
+	c->x86_capability[cpufeat_word(X86_FEATURE_XMM3)] = ecx;
+
 	if ( cpu_has(c, X86_FEATURE_CLFLSH) )
 		c->x86_clflush_size = ((ebx >> 8) & 0xff) * 8;
 
@@ -258,9 +266,6 @@ static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
 			get_model_name(c); /* Default name */
 	}
 
-	/* Might lift BIOS max_leaf=3 limit. */
-	early_intel_workaround(c);
-
 	/* Intel-defined flags: level 0x00000007 */
 	if ( c->cpuid_level >= 0x00000007 )
 		cpuid_count(0x00000007, 0, &tmp,
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index ed6cdf0..1877e7d 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -5,6 +5,7 @@ struct cpu_dev {
 	/* some have two possibilities for cpuid string */
 	char	* c_ident[2];	
 
+	void		(*c_early_init)(struct cpuinfo_x86 *c);
 	void		(*c_init)(struct cpuinfo_x86 * c);
 };
 
@@ -17,5 +18,3 @@ extern unsigned int opt_cpuid_mask_ext_ecx, opt_cpuid_mask_ext_edx;
 
 extern int get_model_name(struct cpuinfo_x86 *c);
 extern void display_cacheinfo(struct cpuinfo_x86 *c);
-
-extern void early_intel_workaround(struct cpuinfo_x86 *c);
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index c00657e..fda8d1b 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -156,10 +156,8 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
 	}
 }
 
-void __devinit early_intel_workaround(struct cpuinfo_x86 *c)
+static void __devinit early_init_intel(struct cpuinfo_x86 *c)
 {
-	if (c->x86_vendor != X86_VENDOR_INTEL)
-		return;
 	/* Netburst reports 64 bytes clflush size, but does IO in 128 bytes */
 	if (c->x86 == 15 && c->x86_cache_alignment == 64)
 		c->x86_cache_alignment = 128;
@@ -290,6 +288,7 @@ static void __devinit init_intel(struct cpuinfo_x86 *c)
 static const struct cpu_dev intel_cpu_dev = {
 	.c_vendor	= "Intel",
 	.c_ident 	= { "GenuineIntel" },
+	.c_early_init	= early_init_intel,
 	.c_init		= init_intel,
 };
 
-- 
2.1.4

[PATCH 2/2] x86/cpu: Move set_cpumask() calls into c_early_init()

[Andrew Cooper]

Before c/s 44e24f8567 "x86: don't call generic_identify() redundantly", the
commandline-provided masks would take effect in Xen's view of the features.

As the masks got applied after the query for features, the redundant call to
generic_identify() would clobber the wrong feature information with the new,
correct information.

Move the set_cpumask() calls into c_early_init() so their effects take place
before the main query for features in generic_identify().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/cpu/amd.c   |  8 ++++++--
 xen/arch/x86/cpu/intel.c | 34 +++++++++++++++++-----------------
 2 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 74a0152..335b72e 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -424,6 +424,11 @@ static void __devinit amd_get_topology(struct cpuinfo_x86 *c)
                                                          c->cpu_core_id);
 }
 
+static void __devinit early_init_amd(struct cpuinfo_x86 *c)
+{
+	set_cpuidmask(c);
+}
+
 static void __devinit init_amd(struct cpuinfo_x86 *c)
 {
 	u32 l, h;
@@ -610,14 +615,13 @@ static void __devinit init_amd(struct cpuinfo_x86 *c)
 	if ((smp_processor_id() == 1) && c1_ramping_may_cause_clock_drift(c))
 		disable_c1_ramping();
 
-	set_cpuidmask(c);
-
 	check_syscfg_dram_mod_en();
 }
 
 static const struct cpu_dev amd_cpu_dev = {
 	.c_vendor	= "AMD",
 	.c_ident 	= { "AuthenticAMD" },
+	.c_early_init	= early_init_amd,
 	.c_init		= init_amd,
 };
 
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index fda8d1b..b981762 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -182,6 +182,23 @@ static void __devinit early_init_intel(struct cpuinfo_x86 *c)
 	if (boot_cpu_data.x86 == 0xF && boot_cpu_data.x86_model == 3 &&
 	    (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 (!cpu_has_cpuid_faulting)
+		set_cpuidmask(c);
+	else if ((c == &boot_cpu_data) &&
+		 (~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx &
+		    opt_cpuid_mask_ext_ecx & opt_cpuid_mask_ext_edx &
+		    opt_cpuid_mask_xsave_eax)))
+		printk("No CPUID feature masking support available\n");
 }
 
 /*
@@ -251,23 +268,6 @@ static void __devinit init_intel(struct cpuinfo_x86 *c)
 		detect_ht(c);
 	}
 
-	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 (!cpu_has_cpuid_faulting)
-		set_cpuidmask(c);
-	else if ((c == &boot_cpu_data) &&
-		 (~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx &
-		    opt_cpuid_mask_ext_ecx & opt_cpuid_mask_ext_edx &
-		    opt_cpuid_mask_xsave_eax)))
-		printk("No CPUID feature masking support available\n");
-
 	/* Work around errata */
 	Intel_errata_workarounds(c);
 
-- 
2.1.4

Re: [PATCH 2/2] x86/cpu: Move set_cpumask() calls into c_early_init()

[Jan Beulich]

>>> On 26.11.15 at 17:59, <andrew.cooper3@citrix.com> wrote:
> Before c/s 44e24f8567 "x86: don't call generic_identify() redundantly", the
> commandline-provided masks would take effect in Xen's view of the features.
> 
> As the masks got applied after the query for features, the redundant call to
> generic_identify() would clobber the wrong feature information with the new,
> correct information.

I'm seriously wondering: Why would the un-adjusted feature
information be wrong for Xen's own use (and perhaps even Dom0's)?
I view it exactly the other way around: Said commit actually fixed
this misbehavior.

Jan

Re: [PATCH 2/2] x86/cpu: Move set_cpumask() calls into c_early_init()

[Andrew Cooper]

On 27/11/15 08:28, Jan Beulich wrote:
>>>> On 26.11.15 at 17:59, <andrew.cooper3@citrix.com> wrote:
>> Before c/s 44e24f8567 "x86: don't call generic_identify() redundantly", the
>> commandline-provided masks would take effect in Xen's view of the features.
>>
>> As the masks got applied after the query for features, the redundant call to
>> generic_identify() would clobber the wrong feature information with the new,
>> correct information.
> I'm seriously wondering: Why would the un-adjusted feature
> information be wrong for Xen's own use (and perhaps even Dom0's)?

There is at least one situation when levelling PV guests where you
absolutely do need to level Xen alongside.

Intel mask msrs cannot hide CPUID.1.ECX[OSXSAVE], as the copy from cr4
happens after the mask is applied, rather than before.

The only way to safely level systems with mixed xsave/non-xsave hardware
is to prevent Xen from turning it on in the first place.  This could be
special cased using the "xsave" command line option, but that is just
awkward.

> I view it exactly the other way around: Said commit actually fixed
> this misbehavior.

I am firmly of the opinion that Xen not respecting the mask options for
itself is misbehaviour.

With my feature levelling series (which is just about together now and
undergoing testing), I don't expect anyone to used the cpuid_mask_*
command line options in general, because the adjustments are made
dynamically on a per-guest basis.

This leaves the use of the command line options for debugging (which
used to work), and for actually making the hardware pretend to be older
hardware, which IMO is the expectation of using the options.

~Andrew

Re: [PATCH 2/2] x86/cpu: Move set_cpumask() calls into c_early_init()

[Jan Beulich]

>>> On 27.11.15 at 11:57, <andrew.cooper3@citrix.com> wrote:
> On 27/11/15 08:28, Jan Beulich wrote:
>>>>> On 26.11.15 at 17:59, <andrew.cooper3@citrix.com> wrote:
>>> Before c/s 44e24f8567 "x86: don't call generic_identify() redundantly", the
>>> commandline-provided masks would take effect in Xen's view of the features.
>>>
>>> As the masks got applied after the query for features, the redundant call to
>>> generic_identify() would clobber the wrong feature information with the new,
>>> correct information.
>> I'm seriously wondering: Why would the un-adjusted feature
>> information be wrong for Xen's own use (and perhaps even Dom0's)?
> 
> There is at least one situation when levelling PV guests where you
> absolutely do need to level Xen alongside.
> 
> Intel mask msrs cannot hide CPUID.1.ECX[OSXSAVE], as the copy from cr4
> happens after the mask is applied, rather than before.
> 
> The only way to safely level systems with mixed xsave/non-xsave hardware
> is to prevent Xen from turning it on in the first place.  This could be
> special cased using the "xsave" command line option, but that is just
> awkward.

I don't view this as awkward at all.

>> I view it exactly the other way around: Said commit actually fixed
>> this misbehavior.
> 
> I am firmly of the opinion that Xen not respecting the mask options for
> itself is misbehaviour.

Because of ??? I see no reason to restrict Xen's (or Dom0's)
capabilities just because of migration considerations.

> With my feature levelling series (which is just about together now and
> undergoing testing), I don't expect anyone to used the cpuid_mask_*
> command line options in general, because the adjustments are made
> dynamically on a per-guest basis.
> 
> This leaves the use of the command line options for debugging (which
> used to work), and for actually making the hardware pretend to be older
> hardware, which IMO is the expectation of using the options.

Which would make the patch acceptable once that series went in.

Jan

Re: [PATCH 2/2] x86/cpu: Move set_cpumask() calls into c_early_init()

[Andrew Cooper]

On 27/11/15 11:11, Jan Beulich wrote:
>>>> On 27.11.15 at 11:57, <andrew.cooper3@citrix.com> wrote:
>> On 27/11/15 08:28, Jan Beulich wrote:
>>>>>> On 26.11.15 at 17:59, <andrew.cooper3@citrix.com> wrote:
>>>> Before c/s 44e24f8567 "x86: don't call generic_identify() redundantly", the
>>>> commandline-provided masks would take effect in Xen's view of the features.
>>>>
>>>> As the masks got applied after the query for features, the redundant call to
>>>> generic_identify() would clobber the wrong feature information with the new,
>>>> correct information.
>>> I'm seriously wondering: Why would the un-adjusted feature
>>> information be wrong for Xen's own use (and perhaps even Dom0's)?
>> There is at least one situation when levelling PV guests where you
>> absolutely do need to level Xen alongside.
>>
>> Intel mask msrs cannot hide CPUID.1.ECX[OSXSAVE], as the copy from cr4
>> happens after the mask is applied, rather than before.
>>
>> The only way to safely level systems with mixed xsave/non-xsave hardware
>> is to prevent Xen from turning it on in the first place.  This could be
>> special cased using the "xsave" command line option, but that is just
>> awkward.
> I don't view this as awkward at all.

For a human levelling a couple of servers, maybe not.

For toolstack software running in a large environment, it very much is. 
Mapping each possibility to a unique Xen command line option is far more
awkward than just setting a mask.

>
>>> I view it exactly the other way around: Said commit actually fixed
>>> this misbehavior.
>> I am firmly of the opinion that Xen not respecting the mask options for
>> itself is misbehaviour.
> Because of ??? I see no reason to restrict Xen's (or Dom0's)
> capabilities just because of migration considerations.

Migration considerations absolutely take priority over Xen's self
considerations.

If you get migration considerations wrong, guests crash and Xen had
fundamentally failed at its job.

>
>> With my feature levelling series (which is just about together now and
>> undergoing testing), I don't expect anyone to used the cpuid_mask_*
>> command line options in general, because the adjustments are made
>> dynamically on a per-guest basis.
>>
>> This leaves the use of the command line options for debugging (which
>> used to work), and for actually making the hardware pretend to be older
>> hardware, which IMO is the expectation of using the options.
> Which would make the patch acceptable once that series went in.

I will see what I can do.

~Andrew

Re: [PATCH 1/2] x86/cpu: Introduce cpu_dev.c_early_init()

[Jan Beulich]

>>> On 26.11.15 at 17:58, <andrew.cooper3@citrix.com> wrote:
> The name is chosen to be consistent with Linux.  Doing this allows
> early_intel_workaround() to be removed from common code.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one adjustment suggestion (which I'll do on commit unless you
object):

> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -156,10 +156,8 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
>  	}
>  }
>  
> -void __devinit early_intel_workaround(struct cpuinfo_x86 *c)
> +static void __devinit early_init_intel(struct cpuinfo_x86 *c)

The __devinit should be dropped here considering that we mean
to drop them gradually anyway.

Jan

Re: [PATCH 1/2] x86/cpu: Introduce cpu_dev.c_early_init()

[Andrew Cooper]

On 27/11/15 08:24, Jan Beulich wrote:
>>>> On 26.11.15 at 17:58, <andrew.cooper3@citrix.com> wrote:
>> The name is chosen to be consistent with Linux.  Doing this allows
>> early_intel_workaround() to be removed from common code.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one adjustment suggestion (which I'll do on commit unless you
> object):

Absolutely fine.

>
>> --- a/xen/arch/x86/cpu/intel.c
>> +++ b/xen/arch/x86/cpu/intel.c
>> @@ -156,10 +156,8 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
>>  	}
>>  }
>>  
>> -void __devinit early_intel_workaround(struct cpuinfo_x86 *c)
>> +static void __devinit early_init_intel(struct cpuinfo_x86 *c)
> The __devinit should be dropped here considering that we mean
> to drop them gradually anyway.

I shall dust off my series which drops them all.

~Andrew