[PATCH v2] x86: Quark: Add if/else to setup_arch for Quark TLB bug

[PATCH v2] x86: Quark: Add if/else to setup_arch for Quark TLB bug

[Bryan O'Donoghue]

Quark X1000 incorrectly advertises PGE. In later stages of boot
specifically in early_init_intel we setup_clear_cpu_cap for PGE.
At this point in time cpu_has_pge() will still be true.

Use the boot_cpu_data to decide if __flush_tlb_all() or __flush_tlb()
should be called subsequent to loading CR3

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

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 41ead8d..980c3c3 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -879,7 +879,21 @@ void __init setup_arch(char **cmdline_p)
 			KERNEL_PGD_PTRS);
 
 	load_cr3(swapper_pg_dir);
-	__flush_tlb_all();
+
+	/*
+	 * Flush the TLB after loading CR3
+	 *
+	 * Quark X1000 wrongly advertises PGE. Use boot_cpu_data to
+	 * to make sure ithe TLB is flushed correctly in the early
+	 * stage of setup_arch() for Quark X1000.
+	 * X86_FEATURE_PGE flag is only setup later stage at
+	 * early_cpu_init();
+	 */
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
+	    boot_cpu_data.x86 == 5 && boot_cpu_data.x86_model == 9)
+		__flush_tlb();
+	else
+		__flush_tlb_all();
 #else
 	printk(KERN_INFO "Command line: %s\n", boot_command_line);
 #endif
-- 
1.9.1

Re: [PATCH v2] x86: Quark: Add if/else to setup_arch for Quark TLB bug

[Henrique de Moraes Holschuh]

On Fri, 26 Sep 2014, Bryan O'Donoghue wrote:
> Quark X1000 incorrectly advertises PGE. In later stages of boot
> specifically in early_init_intel we setup_clear_cpu_cap for PGE.
> At this point in time cpu_has_pge() will still be true.
> 
> Use the boot_cpu_data to decide if __flush_tlb_all() or __flush_tlb()
> should be called subsequent to loading CR3

...

>  	load_cr3(swapper_pg_dir);
> -	__flush_tlb_all();
> +
> +	/*
> +	 * Flush the TLB after loading CR3
> +	 *
> +	 * Quark X1000 wrongly advertises PGE. Use boot_cpu_data to
> +	 * to make sure ithe TLB is flushed correctly in the early
> +	 * stage of setup_arch() for Quark X1000.
> +	 * X86_FEATURE_PGE flag is only setup later stage at
> +	 * early_cpu_init();
> +	 */
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> +	    boot_cpu_data.x86 == 5 && boot_cpu_data.x86_model == 9)
> +		__flush_tlb();
> +	else
> +		__flush_tlb_all();
>  #else
>  	printk(KERN_INFO "Command line: %s\n", boot_command_line);
>  #endif

I'm confused, now.

Wasn't the other patch -- which just added a comment -- the one selected as
a better fix, because there is absolutely no point in calling __flush_tlb()
on Quark X1000 *right after* you just flushed the TLB [on these processors]
by doing a load_cr3() ?

Should this one ([PATCH v2] x86: Quark: Add if/else to setup_arch for Quark
TLB bug) be ignored?  Or should the other one which just adds a comment be
ignored?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH v2] x86: Quark: Add if/else to setup_arch for Quark TLB bug

[Bryan O'Donoghue]

On 26/09/14 21:59, Henrique de Moraes Holschuh wrote:

> I'm confused, now.
>
> Wasn't the other patch -- which just added a comment -- the one selected as
> a better fix, because there is absolutely no point in calling __flush_tlb()
> on Quark X1000 *right after* you just flushed the TLB [on these processors]
> by doing a load_cr3() ?
>
> Should this one ([PATCH v2] x86: Quark: Add if/else to setup_arch for Quark
> TLB bug) be ignored?  Or should the other one which just adds a comment be
> ignored?

Hi Henrique.

My view is that the CR3 load should have flushed the TLB in it's entirety.

Ong Boong Leong said that a discussion he had which included HPA 
concluded with a flush of the TLB being required after the CR3 reload.

The current code

a) Writes to CR3.
On Quark that will flush the entire TLB
Ref: Quark SoC X1000 Core Developer's Manual section 6.4.11

On other processors this won't invalidate the entire TLB under every 
scenario. For example a PTE with PGE on may not be flushed.

Ref: Intel 64 and IA32 Architectures .. vol (3a, 3b, 3c) 325384.pdf 
section 4.10.4.1

b) __flush_tlb_all
Depending on the value of cpu_has_pge() - which will be true for ~all 
x86 processors will call

native_write_cr4(cr4 & ~X86_CR4_PGE);

According to the same section "4.10.4.1" this will nuke the rest of the 
TLB entries even with PGE bits set in the PTEs

So the steps a & b are the appropriate sets to take to entirely flush 
the TLB for most processors i.e. one's that support PGE

The suggestion is that on Quark we should flush the TLB again.

The documentation doesn't really support that statement but, my view is 
OK if we are going to accommodate that position let's not force a third 
TLB flush on everybody who isn't running a Quark.

 From the technical/documentation standpoint I don't think Quark is 
forcing a step c - if we do decide to add a __flush_tlb() call for the 
sake of Quark anyway - then let's not foist the extra cycles on every 
other x86 processor out there.

My preference is

1. Just comment the code as is to explain why it works for Quark.

If that's not good enough for people then

2. if/else the flow so that Quark does __flush_tlb() and the rest of the 
world does a __flush_tlb_all()


--
BOD

RE: [PATCH v2] x86: Quark: Add if/else to setup_arch for Quark TLB bug

[Ong, Boon Leong]

> 
> My view is that the CR3 load should have flushed the TLB in it's entirety.
> 
> Ong Boong Leong said that a discussion he had which included HPA concluded
> with a flush of the TLB being required after the CR3 reload.

The proposed patch was discussed in April and after much thought into this,
I will suggest that as long as the commentary properly captured down why
__flush_tlb() is **NOT** needed because load_cr3() will have the same effect.

> 
> The current code
> 
> My preference is
> 
> 1. Just comment the code as is to explain why it works for Quark.
> 
> If that's not good enough for people then
> 
> 2. if/else the flow so that Quark does __flush_tlb() and the rest of the world
> does a __flush_tlb_all()
Bryan, just drop this proposal from my submission even though __flush_tlb() is
more obvious in what is supposed to do and does not consume any significant cpu-time.

Re: [PATCH v2] x86: Quark: Add if/else to setup_arch for Quark TLB bug

[Bryan O'Donoghue]

On 30/09/14 02:46, Ong, Boon Leong wrote:
>>
>> My view is that the CR3 load should have flushed the TLB in it's entirety.
>>
>> Ong Boong Leong said that a discussion he had which included HPA concluded
>> with a flush of the TLB being required after the CR3 reload.
>
> The proposed patch was discussed in April and after much thought into this,
> I will suggest that as long as the commentary properly captured down why
> __flush_tlb() is **NOT** needed because load_cr3() will have the same effect.

Agree.

>>
>> The current code
>>
>> My preference is
>>
>> 1. Just comment the code as is to explain why it works for Quark.
>>
>> If that's not good enough for people then
>>
>> 2. if/else the flow so that Quark does __flush_tlb() and the rest of the world
>> does a __flush_tlb_all()
> Bryan, just drop this proposal from my submission even though __flush_tlb() is
> more obvious in what is supposed to do and does not consume any significant cpu-time.

Very good. I've a new patchset ready to report cache size based on the 
callbacks that Ingo and Dave suggested - found a bug too - so I'll 
submit those changes as one.

1. Comment
2. Reporting of cache size
3. Bugfix to the code path for legacy_cache_size => currently broken for 
{PIII Tualatin, a bunch of AMDs, and some VIAs too by the looks of it}