[PATCH 0/3] Quark X1000 Cache/TLB reporting/fixes

[PATCH 0/3] Quark X1000 Cache/TLB reporting/fixes

[Bryan O'Donoghue]

First patch:
legacy_cache_size is currently not reachable code from kernels compiled
CONFIG_X86_32 despite most/all legacy_cache_size code being ifdef'd
CONFIG_X86_32. Added to which for Intel, AMD and VIA processors that return
a valid vendor string via cpuid and hook code into c_init and friends the
legacy_cache_size code won't run. So the first patch adds in a call to
default_init when the x86_vendor is valid i.e. when
x86_vendor != X86_VENDOR_UNKNOWN. This ensures for processors that report
a vendor string like "GenuineIntel" that the legacy_cache_size code will
still run.

Second patch:
Quark X1000 contains a 16k 4-way set associative unified L1 cache
with 256 sets. The second patch gets Quark X1000 reporting 16k of
cache in-line with other legacy reporting processors like PIII Tualatin

Third patch:
Final patch adds a comment to arch/x86/kernel/setup.c. Quark SoC X1000
advertises PGE via cpuid but doesn't infact implement the functionality
to support global pages in the TLB.
Linux will by default toggle CR4.PGE for processors that advertise PGE
A fix is already in place to ensure __flush_tlb() as opposed to
__flush_tlb_all() is called during normal operation.

Since __flush_tlb() just rewrites CR3 there's no need to take any further
action on Quark after writing CR3 in setup.c to flush the TLB. We comment
that behaviour. Note cpu_has_pge() will be nuked later in the boot but,
changing the value at this phase of the boot is considered harmful and so
instead we have agreed to comment the existing code

Bryan O'Donoghue (3):
  x86: Bugfix bit-rot in the calling of legacy_cache_size
  x86: Quark: Update cache reporting, add Quark SoC X1000 string
  x86: Quark: Comment setup_arch for TLB/PGE bugfix

 arch/x86/kernel/cpu/common.c | 13 ++++++++++---
 arch/x86/kernel/cpu/intel.c  | 20 ++++++++++++++++++--
 arch/x86/kernel/setup.c      | 12 ++++++++++++
 3 files changed, 40 insertions(+), 5 deletions(-)

-- 
1.9.1

[PATCH 1/3] x86: Bugfix bit-rot in the calling of legacy_cache_size

[Bryan O'Donoghue]

legacy_cache_size is used by certain processors to report the
size of cache. Currently only X86_VENDOR_UNKNOWN could call
default_init() => cpu_detect_cache_sizes() => legacy_cache_size()

Make an update to identify_cpu to make an explicit call to default_init()
We want to do this since some processors that report vendor strings via
cpuid also want to run legacy_cache_size callbacks - which won't happen
since init_intel() init_amd() and friends take the place of default_init()
and don't themselves make explicit calls to legacy_cache_size.

Also current code has an
#ifdef CONFIG_X86_64
        cpu_detect_cache_sizes(c);
#endif

where cpu_detect_cache_sizes calls legacy_cache_size(); is typically
defined inside of a CONFIG_X86_32
#ifdef CONFIG_X86_32
        .legacy_cache_size = centaur_size_cache,
#endif
#ifdef CONFIG_X86_32
	.legacy_cache_size = intel_size_cache,
#endif
#ifdef CONFIG_X86_32
        .legacy_cache_size = amd_size_cache,
#endif

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 arch/x86/kernel/cpu/common.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index e4ab2b4..53a33fa 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -67,9 +67,8 @@ void __init setup_cpu_local_masks(void)
 
 static void default_init(struct cpuinfo_x86 *c)
 {
-#ifdef CONFIG_X86_64
 	cpu_detect_cache_sizes(c);
-#else
+
 	/* Not much we can do here... */
 	/* Check if at least it has cpuid */
 	if (c->cpuid_level == -1) {
@@ -79,7 +78,6 @@ static void default_init(struct cpuinfo_x86 *c)
 		else if (c->x86 == 3)
 			strcpy(c->x86_model_id, "386");
 	}
-#endif
 }
 
 static const struct cpu_dev default_cpu = {
@@ -874,6 +872,15 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 #endif
 
 	/*
+	 * If c_x86_vendor != X86_VENDOR_UNKNOWN i.e. a known vendor then
+	 * there's a vendor specific c_init()
+	 *
+	 * Even still Intel, AMD and VIA make use of legacy_cache_size which
+	 * is reachable only through default_init right now
+	 */
+	if (this_cpu->c_x86_vendor != X86_VENDOR_UNKNOWN)
+		default_init(c);
+	/*
 	 * Vendor-specific initialization.  In this section we
 	 * canonicalize the feature flags, meaning if there are
 	 * features a certain CPU supports which CPUID doesn't
-- 
1.9.1

Re: [PATCH 1/3] x86: Bugfix bit-rot in the calling of legacy_cache_size

[Dave Jones]

On Tue, Sep 30, 2014 at 04:11:48AM +0100, Bryan O'Donoghue wrote:

 > Make an update to identify_cpu to make an explicit call to default_init()
 > We want to do this since some processors that report vendor strings via
 > cpuid also want to run legacy_cache_size callbacks - which won't happen
 > since init_intel() init_amd() and friends take the place of default_init()
 > and don't themselves make explicit calls to legacy_cache_size.
 > 
 > Also current code has an
 > #ifdef CONFIG_X86_64
 >         cpu_detect_cache_sizes(c);
 > #endif

...

 >  static void default_init(struct cpuinfo_x86 *c)
 >  {
 > -#ifdef CONFIG_X86_64
 >  	cpu_detect_cache_sizes(c);
 > -#else
 > +
 >  	/* Not much we can do here... */
 >  	/* Check if at least it has cpuid */
 >  	if (c->cpuid_level == -1) {
 > @@ -79,7 +78,6 @@ static void default_init(struct cpuinfo_x86 *c)
 >  		else if (c->x86 == 3)
 >  			strcpy(c->x86_model_id, "386");
 >  	}
 > -#endif

Shouldn't this patch be more like..

-#ifdef CONFIG_X86_64
  	cpu_detect_cache_sizes(c);
-#else
+
+#ifdef CONFIG_X86_32

 ?

The 386/486 stuff in this codepath isn't huge, but it
seems pointless to be compiling it on 64-bit.

	Dave

Re: [PATCH 1/3] x86: Bugfix bit-rot in the calling of legacy_cache_size

[Bryan O'Donoghue]

On 30/09/14 04:41, Dave Jones wrote:
> Shouldn't this patch be more like..
>
> -#ifdef CONFIG_X86_64
>    	cpu_detect_cache_sizes(c);
> -#else
> +
> +#ifdef CONFIG_X86_32
>
>   ?

Hmm, maybe but you have code in cpu_detect_cache_sizes that's X86_64 only..

void cpu_detect_cache_sizes(struct cpuinfo_x86 *c)
{
         unsigned int n, dummy, ebx, ecx, edx, l2size;

         n = c->extended_cpuid_level;

         if (n >= 0x80000005) {
                 cpuid(0x80000005, &dummy, &ebx, &ecx, &edx);
                 c->x86_cache_size = (ecx>>24) + (edx>>24);
#ifdef CONFIG_X86_64
                 /* On K8 L1 TLB is inclusive, so don't count it */
                 c->x86_tlbsize = 0;
#endif

Re: [PATCH 1/3] x86: Bugfix bit-rot in the calling of legacy_cache_size

[Thomas Gleixner]

On Tue, 30 Sep 2014, Bryan O'Donoghue wrote:
>  static void default_init(struct cpuinfo_x86 *c)
>  {
> -#ifdef CONFIG_X86_64
>  	cpu_detect_cache_sizes(c);
> -#else
> +
>  	/* Not much we can do here... */
>  	/* Check if at least it has cpuid */
>  	if (c->cpuid_level == -1) {
> @@ -79,7 +78,6 @@ static void default_init(struct cpuinfo_x86 *c)
>  		else if (c->x86 == 3)
>  			strcpy(c->x86_model_id, "386");
>  	}
> -#endif

What's the exact point of this? This is the default init function
which is for X86_VENDOR_UNKNOWN, right?

>  }
>  
>  static const struct cpu_dev default_cpu = {
> @@ -874,6 +872,15 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>  #endif
>  
>  	/*
> +	 * If c_x86_vendor != X86_VENDOR_UNKNOWN i.e. a known vendor then
> +	 * there's a vendor specific c_init()
> +	 *
> +	 * Even still Intel, AMD and VIA make use of legacy_cache_size which
> +	 * is reachable only through default_init right now

That's nonsense. It's called from cpu_detect_cache_sizes() and that is
called from:

arch/x86/kernel/cpu/amd.c:      cpu_detect_cache_sizes(c);
arch/x86/kernel/cpu/centaur.c:  cpu_detect_cache_sizes(c);
arch/x86/kernel/cpu/common.c:   cpu_detect_cache_sizes(c);
arch/x86/kernel/cpu/cyrix.c:            cpu_detect_cache_sizes(c);
arch/x86/kernel/cpu/transmeta.c:        cpu_detect_cache_sizes(c);

So we have 4 callsites outside of default_init() already. Now you're
adding another one into identify_cpu()

> +	 */
> +	if (this_cpu->c_x86_vendor != X86_VENDOR_UNKNOWN)
> +		default_init(c);

With the consequence that for amd/centaur/cyrix/transmeta
cpu_detect_cache_sizes() is invoked twice for nothing.

So the only CPU vendor c_init() callback which does not call
cpu_detect_cache_sizes() is the Intel one. And therefor we inflict
that call on all others twice. Brilliant solution!

So the proper solution is to add the call to identify_cpu()
unconditionally and remove it from _all_ other call sites, make it
static and be done with it.

Sigh,

	tglx

Re: [PATCH 1/3] x86: Bugfix bit-rot in the calling of legacy_cache_size

[Bryan O'Donoghue]

On 30/09/14 21:28, Thomas Gleixner wrote:
> On Tue, 30 Sep 2014, Bryan O'Donoghue wrote:
>>   static void default_init(struct cpuinfo_x86 *c)
>>   {
>> -#ifdef CONFIG_X86_64
>>   	cpu_detect_cache_sizes(c);
>> -#else
>> +
>>   	/* Not much we can do here... */
>>   	/* Check if at least it has cpuid */
>>   	if (c->cpuid_level == -1) {
>> @@ -79,7 +78,6 @@ static void default_init(struct cpuinfo_x86 *c)
>>   		else if (c->x86 == 3)
>>   			strcpy(c->x86_model_id, "386");
>>   	}
>> -#endif
>
> What's the exact point of this? This is the default init function
> which is for X86_VENDOR_UNKNOWN, right?

Correct

>>   }
>>
>>   static const struct cpu_dev default_cpu = {
>> @@ -874,6 +872,15 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>>   #endif
>>
>>   	/*
>> +	 * If c_x86_vendor != X86_VENDOR_UNKNOWN i.e. a known vendor then
>> +	 * there's a vendor specific c_init()
>> +	 *
>> +	 * Even still Intel, AMD and VIA make use of legacy_cache_size which
>> +	 * is reachable only through default_init right now
>
> That's nonsense. It's called from cpu_detect_cache_sizes() and that is
> called from:
>
> arch/x86/kernel/cpu/amd.c:      cpu_detect_cache_sizes(c);
> arch/x86/kernel/cpu/centaur.c:  cpu_detect_cache_sizes(c);
> arch/x86/kernel/cpu/common.c:   cpu_detect_cache_sizes(c);
> arch/x86/kernel/cpu/cyrix.c:            cpu_detect_cache_sizes(c);
> arch/x86/kernel/cpu/transmeta.c:        cpu_detect_cache_sizes(c);

/uses grep

Yes I see for everybody except Intel that'll get called. I mised that.

> So we have 4 callsites outside of default_init() already. Now you're
> adding another one into identify_cpu()
>
>> +	 */
>> +	if (this_cpu->c_x86_vendor != X86_VENDOR_UNKNOWN)
>> +		default_init(c);
>
> With the consequence that for amd/centaur/cyrix/transmeta
> cpu_detect_cache_sizes() is invoked twice for nothing.

Yes - I see what's needed here is for cpu_detect_cache_sizes to get 
called for Intel only - so the bit rot is really only limited to

1. PIII Tualatin
2. New Intel additions on this path like Quark


> So the only CPU vendor c_init() callback which does not call
> cpu_detect_cache_sizes() is the Intel one. And therefor we inflict
> that call on all others twice. Brilliant solution!

Haha. When you put it like that you make it sound mad and unreasonable.

> So the proper solution is to add the call to identify_cpu()
> unconditionally and remove it from _all_ other call sites, make it
> static and be done with it.

Agree I'll make that update.

Thanks for your review.

--
BOD

[PATCH 2/3] x86: Quark: Update cache reporting, add Quark SoC X1000 string

[Bryan O'Donoghue]

Adds a path for legacy_cache_size to get a Quark SoC X1000 cache size
Update init_intel to take account of PIII Tualatin and Quark X1000
reporting cache size via legacy_cache_size
Add string to family/model structure for completeness and better
output of /proc/cpuinfo

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 arch/x86/kernel/cpu/intel.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 50ce751..686eae7 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -396,7 +396,13 @@ static void init_intel(struct cpuinfo_x86 *c)
 #endif
 	}
 
-	l2 = init_intel_cacheinfo(c);
+
+	/* legacy_cache may have provided and cache_size already if not probe */
+	if (c->x86_cache_size == 0)
+		l2 = init_intel_cacheinfo(c);
+	else
+		l2 = c->x86_cache_size;
+
 	if (c->cpuid_level > 9) {
 		unsigned eax = cpuid_eax(10);
 		/* Check for version and the number of counters */
@@ -500,6 +506,15 @@ static unsigned int intel_size_cache(struct cpuinfo_x86 *c, unsigned int size)
 	 */
 	if ((c->x86 == 6) && (c->x86_model == 11) && (size == 0))
 		size = 256;
+
+
+	/*
+	 * Intel Quark SoC X1000 contains a 4-way set associative
+	 * 16K cache with a 16 byte cache line and 256 lines per tag
+	 */
+	if ((c->x86 == 5) && (c->x86_model == 9))
+		size = 16;
+
 	return size;
 }
 #endif
@@ -701,7 +716,8 @@ static const struct cpu_dev intel_cpu_dev = {
 			  [3] = "OverDrive PODP5V83",
 			  [4] = "Pentium MMX",
 			  [7] = "Mobile Pentium 75 - 200",
-			  [8] = "Mobile Pentium MMX"
+			  [8] = "Mobile Pentium MMX",
+			  [9] = "Quark SoC X1000",
 		  }
 		},
 		{ .family = 6, .model_names =
-- 
1.9.1

Re: [PATCH 2/3] x86: Quark: Update cache reporting, add Quark SoC X1000 string

[Thomas Gleixner]

On Tue, 30 Sep 2014, Bryan O'Donoghue wrote:

> Adds a path for legacy_cache_size to get a Quark SoC X1000 cache size
> Update init_intel to take account of PIII Tualatin and Quark X1000
> reporting cache size via legacy_cache_size

I don't see any reference to PIII Tualatin in the patch itself.

> Add string to family/model structure for completeness and better
> output of /proc/cpuinfo
> 
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
> ---
>  arch/x86/kernel/cpu/intel.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 50ce751..686eae7 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -396,7 +396,13 @@ static void init_intel(struct cpuinfo_x86 *c)
>  #endif
>  	}
>  
> -	l2 = init_intel_cacheinfo(c);
> +
> +	/* legacy_cache may have provided and cache_size already if not probe */

That comment took me some time to decode.

	/* 
	 * If cache_size has not been initialized via legacy_cache()
	 * probe it via init_intel_cacheinfo().
	 */

Perhaps?

> +	if (c->x86_cache_size == 0)
> +		l2 = init_intel_cacheinfo(c);
> +	else
> +		l2 = c->x86_cache_size;
> +
>  	if (c->cpuid_level > 9) {
>  		unsigned eax = cpuid_eax(10);
>  		/* Check for version and the number of counters */
> @@ -500,6 +506,15 @@ static unsigned int intel_size_cache(struct cpuinfo_x86 *c, unsigned int size)
>  	 */
>  	if ((c->x86 == 6) && (c->x86_model == 11) && (size == 0))
>  		size = 256;
> +

Stray extra newline

> +
> +	/*
> +	 * Intel Quark SoC X1000 contains a 4-way set associative
> +	 * 16K cache with a 16 byte cache line and 256 lines per tag
> +	 */
> +	if ((c->x86 == 5) && (c->x86_model == 9))
> +		size = 16;
> +
>  	return size;
>  }

Thanks,

	tglx

[PATCH 3/3] x86: Quark: Comment setup_arch for TLB/PGE bugfix

[Bryan O'Donoghue]

Quark X1000 requires CR3 to be rewritten to flush TLB entries
irrespective of the PGE bits in CR4 or PTE.PGE

Add a comment to setup_arch to indicate that the code

load_cr3(swapper_pg_dir);
__flush_tlb_all();

Will already have flushed the TLB @ the CR3 reload allowing us
to skip over a potential if/else for Quark.

This comment clearly states the bug, the behaviour we rely on
and the reason why we only do it this way - one time.

Later on cpu_has_pge() will be false due to a fixup in
intel_init_early() and __flush_tlb_all() will work as expected
from that point onwards

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 arch/x86/kernel/setup.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 41ead8d..98b6660 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -878,6 +878,18 @@ void __init setup_arch(char **cmdline_p)
 			initial_page_table + KERNEL_PGD_BOUNDARY,
 			KERNEL_PGD_PTRS);
 
+	/*
+	 * Locate the page directory and flush the TLB.
+	 *
+	 * On Quark X1000 CPUs we still have the PGE bit incorrectly set
+	 * due to a processor erratum, so __flush_tlb_all() is not yet
+	 * doing what it says.  Fortunately we have a cr3 load here,
+	 * which is what is needed on this processor to flush TLBs, so
+	 * there's no need to add a Quark X1000 quirk here.
+	 *
+	 * early_init_intel will unset the X86_FEATURE_PGE flag later
+	 * and __flush_tlb_all() will flush via cr3
+	 */
 	load_cr3(swapper_pg_dir);
 	__flush_tlb_all();
 #else
-- 
1.9.1

Re: [PATCH 3/3] x86: Quark: Comment setup_arch for TLB/PGE bugfix

[Thomas Gleixner]

On Tue, 30 Sep 2014, Bryan O'Donoghue wrote:
>  
> +	/*
> +	 * Locate the page directory and flush the TLB.

s/Locate/Load/ ?

Thats what we do here. CR3 is used by the CPU to locate the page
directory.

> +	 *
> +	 * On Quark X1000 CPUs we still have the PGE bit incorrectly set
> +	 * due to a processor erratum, so __flush_tlb_all() is not yet
> +	 * doing what it says.  Fortunately we have a cr3 load here,
> +	 * which is what is needed on this processor to flush TLBs, so
> +	 * there's no need to add a Quark X1000 quirk here.
> +	 *
> +	 * early_init_intel will unset the X86_FEATURE_PGE flag later
> +	 * and __flush_tlb_all() will flush via cr3

Can we please write that a bit less convoluted?

> +	 */
>  	load_cr3(swapper_pg_dir);

	/*
	 * Note: Quark X1000 CPUs advertise PGE incorrectly and require
	 * a cr3 based tlb flush, so the following __flush_tlb_all()
	 * will not flush anything because the cpu quirk which clears
	 * X86_FEATURE_PGE has not been invoked yet. Though due to the
	 * load_cr3() above the TLB has been flushed already. The
	 * quirk is invoked before subsequent calls to __flush_tlb_all()
	 * so proper operation is guaranteed.
	 */
>  	__flush_tlb_all();

Thanks,

	tglx