[Xen-devel] [PATCH 0/2] x86: port self-snoop related patches from Linux

[Xen-devel] [PATCH 0/2] x86: port self-snoop related patches from Linux

[Jan Beulich]

Andrew had spotted these going in there, and we clearly want them
too.

1: x86/cpu/intel: Clear cache self-snoop capability in CPUs with known errata
2: x86/mtrr: Skip cache flushes on CPUs with cache self-snooping

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/2] x86/cpu/intel: Clear cache self-snoop capability in CPUs with known errata

[Jan Beulich]

From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Processors which have self-snooping capability can handle conflicting
memory type across CPUs by snooping its own cache. However, there exists
CPU models in which having conflicting memory types still leads to
unpredictable behavior, machine check errors, or hangs.

Clear this feature on affected CPUs to prevent its use.

Suggested-by: Alan Cox <alan.cox@intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
[Linux commit 1e03bff3600101bd9158d005e4313132e55bdec8]

Strip Yonah - as per ark.intel.com it doesn't look to be 64-bit capable.
Call check_memory_type_self_snoop_errata() only on the boot CPU.

Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -15,6 +15,32 @@
  #include "cpu.h"
  
  /*
+ * Processors which have self-snooping capability can handle conflicting
+ * memory type across CPUs by snooping its own cache. However, there exists
+ * CPU models in which having conflicting memory types still leads to
+ * unpredictable behavior, machine check errors, or hangs. Clear this
+ * feature to prevent its use on machines with known erratas.
+ */
+static void __init check_memory_type_self_snoop_errata(void)
+{
+	switch (boot_cpu_data.x86_model) {
+	case 0x0f: /* Merom */
+	case 0x16: /* Merom L */
+	case 0x17: /* Penryn */
+	case 0x1d: /* Dunnington */
+	case 0x1e: /* Nehalem */
+	case 0x1f: /* Auburndale / Havendale */
+	case 0x1a: /* Nehalem EP */
+	case 0x2e: /* Nehalem EX */
+	case 0x25: /* Westmere */
+	case 0x2c: /* Westmere EP */
+	case 0x2a: /* SandyBridge */
+		setup_clear_cpu_cap(X86_FEATURE_SS);
+		break;
+	}
+}
+
+/*
   * 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.
@@ -256,8 +282,11 @@ static void early_init_intel(struct cpui
  	    (boot_cpu_data.x86_mask == 3 || boot_cpu_data.x86_mask == 4))
  		paddr_bits = 36;
  
-	if (c == &boot_cpu_data)
+	if (c == &boot_cpu_data) {
+		check_memory_type_self_snoop_errata();
+
  		intel_init_levelling();
+	}
  
  	ctxt_switch_levelling(NULL);
  }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/2] x86/mtrr: Skip cache flushes on CPUs with cache self-snooping

[Jan Beulich]

From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Programming MTRR registers in multi-processor systems is a rather lengthy
process. Furthermore, all processors must program these registers in lock
step and with interrupts disabled; the process also involves flushing
caches and TLBs twice. As a result, the process may take a considerable
amount of time.

On some platforms, this can lead to a large skew of the refined-jiffies
clock source. Early when booting, if no other clock is available (e.g.,
booting with hpet=disabled), the refined-jiffies clock source is used to
monitor the TSC clock source. If the skew of refined-jiffies is too large,
Linux wrongly assumes that the TSC is unstable:

   clocksource: timekeeping watchdog on CPU1: Marking clocksource
                'tsc-early' as unstable because the skew is too large:
   clocksource: 'refined-jiffies' wd_now: fffedc10 wd_last:
                fffedb90 mask: ffffffff
   clocksource: 'tsc-early' cs_now: 5eccfddebc cs_last: 5e7e3303d4
                mask: ffffffffffffffff
   tsc: Marking TSC unstable due to clocksource watchdog

As per measurements, around 98% of the time needed by the procedure to
program MTRRs in multi-processor systems is spent flushing caches with
wbinvd(). As per the Section 11.11.8 of the Intel 64 and IA 32
Architectures Software Developer's Manual, it is not necessary to flush
caches if the CPU supports cache self-snooping. Thus, skipping the cache
flushes can reduce by several tens of milliseconds the time needed to
complete the programming of the MTRR registers:

Platform                      	Before	   After
104-core (208 Threads) Skylake  1437ms      28ms
   2-core (  4 Threads) Haswell   114ms       2ms

Reported-by: Mohammad Etemadi <mohammad.etemadi@intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
[Linux commit fd329f276ecaad7a371d6f91b9bbea031d0c3440]

Use alternatives patching instead of static_cpu_has() (which we don't
have [yet]).

Interestingly we've been lacking the 2nd wbinvd(), which I'm taking the
liberty here.

Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -450,7 +450,14 @@ static bool prepare_set(void)
  
  	/*  Enter the no-fill (CD=1, NW=0) cache mode and flush caches. */
  	write_cr0(read_cr0() | X86_CR0_CD);
-	wbinvd();
+
+	/*
+	 * Cache flushing is the most time-consuming step when programming
+	 * the MTRRs. Fortunately, as per the Intel Software Development
+	 * Manual, we can skip it if the processor supports cache self-
+	 * snooping.
+	 */
+	alternative("wbinvd", "", X86_FEATURE_SS);
  
  	cr4 = read_cr4();
  	if (cr4 & X86_CR4_PGE)
@@ -466,6 +473,9 @@ static bool prepare_set(void)
  	/*  Disable MTRRs, and set the default type to uncached  */
  	mtrr_wrmsr(MSR_MTRRdefType, deftype & ~0xcff);
  
+	/* Again, only flush caches if we have to. */
+	alternative("wbinvd", "", X86_FEATURE_SS);
+
  	return cr4 & X86_CR4_PGE;
  }
  

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] x86/cpu/intel: Clear cache self-snoop capability in CPUs with known errata

[Andrew Cooper]

On 18/07/2019 13:09, Jan Beulich wrote:
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -15,6 +15,32 @@
>   #include "cpu.h"
>   
>   /*
> + * Processors which have self-snooping capability can handle conflicting
> + * memory type across CPUs by snooping its own cache. However, there exists
> + * CPU models in which having conflicting memory types still leads to
> + * unpredictable behavior, machine check errors, or hangs. Clear this
> + * feature to prevent its use on machines with known erratas.
> + */
> +static void __init check_memory_type_self_snoop_errata(void)
> +{
> +	switch (boot_cpu_data.x86_model) {
> +	case 0x0f: /* Merom */
> +	case 0x16: /* Merom L */
> +	case 0x17: /* Penryn */
> +	case 0x1d: /* Dunnington */
> +	case 0x1e: /* Nehalem */
> +	case 0x1f: /* Auburndale / Havendale */
> +	case 0x1a: /* Nehalem EP */
> +	case 0x2e: /* Nehalem EX */
> +	case 0x25: /* Westmere */
> +	case 0x2c: /* Westmere EP */
> +	case 0x2a: /* SandyBridge */

It would have been nice if the errata had actually been identified...

> +		setup_clear_cpu_cap(X86_FEATURE_SS);

I'm regretting exposing SS to guests at this point.

As this stands, it will result in a migration compatibility issue,
because updating Xen will cause a feature to disappear.  If we had a
default vs full policy split, this would be easy enough to work around
in a compatible way.  I wonder if there is anything clever we can do in
the meantime as a stopgap workaround.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86/mtrr: Skip cache flushes on CPUs with cache self-snooping

[Andrew Cooper]

On 18/07/2019 13:10, Jan Beulich wrote:
> From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>
> Programming MTRR registers in multi-processor systems is a rather lengthy
> process. Furthermore, all processors must program these registers in lock
> step and with interrupts disabled; the process also involves flushing
> caches and TLBs twice. As a result, the process may take a considerable
> amount of time.
>
> On some platforms, this can lead to a large skew of the refined-jiffies
> clock source. Early when booting, if no other clock is available (e.g.,
> booting with hpet=disabled), the refined-jiffies clock source is used to
> monitor the TSC clock source. If the skew of refined-jiffies is too large,
> Linux wrongly assumes that the TSC is unstable:
>
>    clocksource: timekeeping watchdog on CPU1: Marking clocksource
>                 'tsc-early' as unstable because the skew is too large:
>    clocksource: 'refined-jiffies' wd_now: fffedc10 wd_last:
>                 fffedb90 mask: ffffffff
>    clocksource: 'tsc-early' cs_now: 5eccfddebc cs_last: 5e7e3303d4
>                 mask: ffffffffffffffff
>    tsc: Marking TSC unstable due to clocksource watchdog
>
> As per measurements, around 98% of the time needed by the procedure to
> program MTRRs in multi-processor systems is spent flushing caches with
> wbinvd(). As per the Section 11.11.8 of the Intel 64 and IA 32
> Architectures Software Developer's Manual, it is not necessary to flush
> caches if the CPU supports cache self-snooping. Thus, skipping the cache
> flushes can reduce by several tens of milliseconds the time needed to
> complete the programming of the MTRR registers:
>
> Platform                      	Before	   After
> 104-core (208 Threads) Skylake  1437ms      28ms
>    2-core (  4 Threads) Haswell   114ms       2ms
>
> Reported-by: Mohammad Etemadi <mohammad.etemadi@intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> [Linux commit fd329f276ecaad7a371d6f91b9bbea031d0c3440]
>
> Use alternatives patching instead of static_cpu_has() (which we don't
> have [yet]).
> Interestingly we've been lacking the 2nd wbinvd(), which I'm taking the
> liberty here.
>
> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] x86/cpu/intel: Clear cache self-snoop capability in CPUs with known errata

[Jan Beulich]

On 18.07.2019 14:23, Andrew Cooper wrote:
> On 18/07/2019 13:09, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpu/intel.c
>> +++ b/xen/arch/x86/cpu/intel.c
>> @@ -15,6 +15,32 @@
>>    #include "cpu.h"
>>    
>>    /*
>> + * Processors which have self-snooping capability can handle conflicting
>> + * memory type across CPUs by snooping its own cache. However, there exists
>> + * CPU models in which having conflicting memory types still leads to
>> + * unpredictable behavior, machine check errors, or hangs. Clear this
>> + * feature to prevent its use on machines with known erratas.
>> + */
>> +static void __init check_memory_type_self_snoop_errata(void)
>> +{
>> +	switch (boot_cpu_data.x86_model) {
>> +	case 0x0f: /* Merom */
>> +	case 0x16: /* Merom L */
>> +	case 0x17: /* Penryn */
>> +	case 0x1d: /* Dunnington */
>> +	case 0x1e: /* Nehalem */
>> +	case 0x1f: /* Auburndale / Havendale */
>> +	case 0x1a: /* Nehalem EP */
>> +	case 0x2e: /* Nehalem EX */
>> +	case 0x25: /* Westmere */
>> +	case 0x2c: /* Westmere EP */
>> +	case 0x2a: /* SandyBridge */
> 
> It would have been nice if the errata had actually been identified...

Indeed; I hope you don't expect me to go hunt them down. I'm
cloning a Linux commit here only, after all.

>> +		setup_clear_cpu_cap(X86_FEATURE_SS);
> 
> I'm regretting exposing SS to guests at this point.
> 
> As this stands, it will result in a migration compatibility issue,
> because updating Xen will cause a feature to disappear.  If we had a
> default vs full policy split, this would be easy enough to work around
> in a compatible way.  I wonder if there is anything clever we can do in
> the meantime as a stopgap workaround.

Should we perhaps introduce X86_FEATURE_XEN_SELFSNOOP, just like
we do for SMEP and SMAP, such that we can leave the real one alone?

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] x86/cpu/intel: Clear cache self-snoop capability in CPUs with known errata

[Andrew Cooper]

On 18/07/2019 14:07, Jan Beulich wrote:
> On 18.07.2019 14:23, Andrew Cooper wrote:
>> On 18/07/2019 13:09, Jan Beulich wrote:
>>> --- a/xen/arch/x86/cpu/intel.c
>>> +++ b/xen/arch/x86/cpu/intel.c
>>> @@ -15,6 +15,32 @@
>>>    #include "cpu.h"
>>>    
>>>    /*
>>> + * Processors which have self-snooping capability can handle conflicting
>>> + * memory type across CPUs by snooping its own cache. However, there exists
>>> + * CPU models in which having conflicting memory types still leads to
>>> + * unpredictable behavior, machine check errors, or hangs. Clear this
>>> + * feature to prevent its use on machines with known erratas.
>>> + */
>>> +static void __init check_memory_type_self_snoop_errata(void)
>>> +{
>>> +	switch (boot_cpu_data.x86_model) {
>>> +	case 0x0f: /* Merom */
>>> +	case 0x16: /* Merom L */
>>> +	case 0x17: /* Penryn */
>>> +	case 0x1d: /* Dunnington */
>>> +	case 0x1e: /* Nehalem */
>>> +	case 0x1f: /* Auburndale / Havendale */
>>> +	case 0x1a: /* Nehalem EP */
>>> +	case 0x2e: /* Nehalem EX */
>>> +	case 0x25: /* Westmere */
>>> +	case 0x2c: /* Westmere EP */
>>> +	case 0x2a: /* SandyBridge */
>> It would have been nice if the errata had actually been identified...
> Indeed; I hope you don't expect me to go hunt them down. I'm
> cloning a Linux commit here only, after all.

No - I don't expect you to hunt them down.

I'll try to keep an eye out next time I'm in the relevant documents.

>
>>> +		setup_clear_cpu_cap(X86_FEATURE_SS);
>> I'm regretting exposing SS to guests at this point.
>>
>> As this stands, it will result in a migration compatibility issue,
>> because updating Xen will cause a feature to disappear.  If we had a
>> default vs full policy split, this would be easy enough to work around
>> in a compatible way.  I wonder if there is anything clever we can do in
>> the meantime as a stopgap workaround.
> Should we perhaps introduce X86_FEATURE_XEN_SELFSNOOP, just like
> we do for SMEP and SMAP, such that we can leave the real one alone?

Ah yes - that will do just fine.

When we get a default/max split, I'll try to remember to take it back out.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel