[PATCH 0/2] x86: Quark: Add legacy_cache_size and TLB comments

[PATCH 0/2] x86: Quark: Add legacy_cache_size and TLB comments

[Bryan O'Donoghue]

First 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

Second 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 (2):
  x86: Quark: Update cache reporting, add Quark SoC X1000 string
  x86: Quark: Comment setup_arch for TLB/PGE bugfix

 arch/x86/kernel/cpu/intel.c | 20 ++++++++++++++++++--
 arch/x86/kernel/setup.c     |  9 +++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH 1/2] 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 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..f37f589 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -396,7 +396,15 @@ static void init_intel(struct cpuinfo_x86 *c)
 #endif
 	}
 
-	l2 = init_intel_cacheinfo(c);
+	/*
+	 * If cache_size has not been initialized via legacy_cache()
+	 * probe it via init_intel_cacheinfo().
+	 */
+	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 +508,13 @@ 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

[PATCH 2/2] 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 41ead8d..235cfd3 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -879,6 +879,15 @@ void __init setup_arch(char **cmdline_p)
 			KERNEL_PGD_PTRS);
 
 	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();
 #else
 	printk(KERN_INFO "Command line: %s\n", boot_command_line);
-- 
1.9.1

Re: [PATCH 0/2] x86: Quark: Add legacy_cache_size and TLB comments

[Thomas Gleixner]

Bryan,

On Wed, 1 Oct 2014, Bryan O'Donoghue wrote:

what are you trying to achieve by sending out patches faster than
anyone can review?

Doing this has an obvious outcome:

  1) The patches you whip up 10 seconds after the review hits your
     inbox are likely not to be well thought out. Neither are the
     changelogs.

     Just by skimming the lot I noticed at least two issues which you
     just mechanically fixed up without applying any thought. I'm not
     going to tell you, simply because you are starting to abuse and
     waste my time.

  2) Keeping #1 as your modus operandi will make sure that you get on
     the backlog list of reviewers and maintainers and in the worst
     case on the shitlist.

Thanks,

	tglx

Re: [PATCH 0/2] x86: Quark: Add legacy_cache_size and TLB comments

[Bryan O'Donoghue]

On 01/10/14 01:11, Thomas Gleixner wrote:
> Bryan,
>
> On Wed, 1 Oct 2014, Bryan O'Donoghue wrote:
>
> what are you trying to achieve by sending out patches faster than
> anyone can review?

Thomas.

That's not the intention at all.

> Doing this has an obvious outcome:
>
>    1) The patches you whip up 10 seconds after the review hits your
>       inbox are likely not to be well thought out. Neither are the
>       changelogs.
>
>       Just by skimming the lot I noticed at least two issues which you
>       just mechanically fixed up without applying any thought. I'm not
>       going to tell you, simply because you are starting to abuse and
>       waste my time.
>
>    2) Keeping #1 as your modus operandi will make sure that you get on
>       the backlog list of reviewers and maintainers and in the worst
>       case on the shitlist.

1)
OK, I take your point on the frequency.

2)
On the substance.
I'm certainly not trying to antagonise you here - I assumed you were 
*suggesting* to apply those comments directly ?
Which is why I updated the sent patches with your comments - since they 
seemed more descriptive anyway - and sent back to the list.


Thanks,
Bryan

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

[Ong, Boon Leong]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Bryan O'Donoghue
> Sent: Wednesday, October 01, 2014 7:42 AM
> To: tglx@linutronix.de; mingo@redhat.com; hpa@zytor.com;
> davej@redhat.com; hmh@hmh.eng.br
> Cc: x86@kernel.org; linux-kernel@vger.kernel.org; Bryan O'Donoghue
> Subject: [PATCH 2/2] x86: Quark: Comment setup_arch for TLB/PGE bugfix
> 
> 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

s/intel_init_early/early_init_intel

Re: [PATCH 0/2] x86: Quark: Add legacy_cache_size and TLB comments

[Thomas Gleixner]

On Wed, 1 Oct 2014, Bryan O'Donoghue wrote:
> On 01/10/14 01:11, Thomas Gleixner wrote:
> On the substance.
> I'm certainly not trying to antagonise you here - I assumed you were
> *suggesting* to apply those comments directly ?
> Which is why I updated the sent patches with your comments - since they
> seemed more descriptive anyway - and sent back to the list.

That part is fine. What really annoyed me is the patch:

Subject: [PATCH] x86: Call identify_cpu() unconditionally once remove other
     callsites

which is a complete fail in all aspects. You should be able to figure
that out yourself easily:

Read the reviews of "[PATCH 1/3] x86: Bugfix bit-rot in the calling of
legacy_cache_size" again carefully. Then look at your patch, the
subject line and the changelog. It should be pretty obvious.

Thanks,

	tglx

Re: [PATCH 0/2] x86: Quark: Add legacy_cache_size and TLB comments

[Bryan O'Donoghue]

On 01/10/14 09:57, Thomas Gleixner wrote:
> On Wed, 1 Oct 2014, Bryan O'Donoghue wrote:
>> On 01/10/14 01:11, Thomas Gleixner wrote:
>> On the substance.
>> I'm certainly not trying to antagonise you here - I assumed you were
>> *suggesting* to apply those comments directly ?
>> Which is why I updated the sent patches with your comments - since they
>> seemed more descriptive anyway - and sent back to the list.
>
> That part is fine. What really annoyed me is the patch:
>
> Subject: [PATCH] x86: Call identify_cpu() unconditionally once remove other
>       callsites
>
> which is a complete fail in all aspects. You should be able to figure
> that out yourself easily:
>
> Read the reviews of "[PATCH 1/3] x86: Bugfix bit-rot in the calling of
> legacy_cache_size" again carefully. Then look at your patch, the
> subject line and the changelog. It should be pretty obvious.

OK - I'll read again.

Thanks for the reviews