Re: [PATCH] x86: Quark: Switch of CR4.PGE TLB flush use CR3 instead

Re: [PATCH] x86: Quark: Switch of CR4.PGE TLB flush use CR3 instead

[Bryan O'Donoghue]

On 24/09/14 08:12, Ingo Molnar wrote:
>
> * Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote:
>
>> Quark x1000 advertises PGE via the standard CPUID method
>> PGE bits exist in Quark X1000's PTEs. In order to flush
>> an individual PTE it is necessary to reload CR3 irrespective
>> of the PTE.PGE bit.
>>
>> See Quark Core_DevMan_001.pdf section 6.4.11
>>
> How was this bug found?
>
> I'm wondering, how was Linux ever booted on an Intel Quark CPU
> successfully if PGE does not work as advertised? Getting TLB
> flushes wrong is a surefire way to crash the kernel. Was any
> related instability observed?
>
> Thanks,
>
> 	Ingo

Hi Ingo.

Resending this email...

The received wisdom from the silicon people prior to launch meant it was 
never a problem for the Quark/Galileo BSP.

In the reference BSP code for Galileo, which I was involved in making, 
this fix is already in place. So pretty much everybody running Linux on 
Quark is doing it with the modified 3.8.7 kernel shipping with the 
Galileo BSP.

So far there's no related instability - since we've never really run any 
torture tests with the wrong TLB flush method in-place.

To get properly Quark X1000 support out-of-the-box in Vanilla Linux 
though the TLB flush will need to change.


Best,
Bryan

[tip:x86/platform] x86/intel/quark: Switch off CR4.PGE so TLB flush uses CR3 instead

[tip-bot for Bryan O'Donoghue]

Commit-ID:  ee1b5b165c0a2f04d2107e634e51f05d0eb107de
Gitweb:     http://git.kernel.org/tip/ee1b5b165c0a2f04d2107e634e51f05d0eb107de
Author:     Bryan O'Donoghue <pure.logic@nexus-software.ie>
AuthorDate: Wed, 24 Sep 2014 00:26:24 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 24 Sep 2014 15:06:15 +0200

x86/intel/quark: Switch off CR4.PGE so TLB flush uses CR3 instead

Quark x1000 advertises PGE via the standard CPUID method
PGE bits exist in Quark X1000's PTEs. In order to flush
an individual PTE it is necessary to reload CR3 irrespective
of the PTE.PGE bit.

See Quark Core_DevMan_001.pdf section 6.4.11

This bug was fixed in Galileo kernels, unfixed vanilla kernels are expected to
crash and burn on this platform.

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Cc: Borislav Petkov <bp@alien8.de>
Cc: <stable@vger.kernel.org>
Link: http://lkml.kernel.org/r/1411514784-14885-1-git-send-email-pure.logic@nexus-software.ie
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/intel.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 74e804d..50ce751 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -144,6 +144,21 @@ static void early_init_intel(struct cpuinfo_x86 *c)
 			setup_clear_cpu_cap(X86_FEATURE_ERMS);
 		}
 	}
+
+	/*
+	 * Intel Quark Core DevMan_001.pdf section 6.4.11
+	 * "The operating system also is required to invalidate (i.e., flush)
+	 *  the TLB when any changes are made to any of the page table entries.
+	 *  The operating system must reload CR3 to cause the TLB to be flushed"
+	 *
+	 * As a result cpu_has_pge() in arch/x86/include/asm/tlbflush.h should
+	 * be false so that __flush_tlb_all() causes CR3 insted of CR4.PGE
+	 * to be modified
+	 */
+	if (c->x86 == 5 && c->x86_model == 9) {
+		pr_info("Disabling PGE capability bit\n");
+		setup_clear_cpu_cap(X86_FEATURE_PGE);
+	}
 }
 
 #ifdef CONFIG_X86_32

Re: [tip:x86/platform] x86/intel/quark: Switch off CR4.PGE so TLB flush uses CR3 instead

[H. Peter Anvin]

On 09/24/2014 08:01 AM, tip-bot for Bryan O'Donoghue wrote:
> Commit-ID:  ee1b5b165c0a2f04d2107e634e51f05d0eb107de
> Gitweb:     http://git.kernel.org/tip/ee1b5b165c0a2f04d2107e634e51f05d0eb107de
> Author:     Bryan O'Donoghue <pure.logic@nexus-software.ie>
> AuthorDate: Wed, 24 Sep 2014 00:26:24 +0100
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Wed, 24 Sep 2014 15:06:15 +0200
>
> x86/intel/quark: Switch off CR4.PGE so TLB flush uses CR3 instead
>
> Quark x1000 advertises PGE via the standard CPUID method
> PGE bits exist in Quark X1000's PTEs. In order to flush
> an individual PTE it is necessary to reload CR3 irrespective
> of the PTE.PGE bit.
>
> See Quark Core_DevMan_001.pdf section 6.4.11
>
> This bug was fixed in Galileo kernels, unfixed vanilla kernels are expected to
> crash and burn on this platform.
>
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: <stable@vger.kernel.org>
> Link: http://lkml.kernel.org/r/1411514784-14885-1-git-send-email-pure.logic@nexus-software.ie
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>   arch/x86/kernel/cpu/intel.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 74e804d..50ce751 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -144,6 +144,21 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>   			setup_clear_cpu_cap(X86_FEATURE_ERMS);
>   		}
>   	}
> +
> +	/*
> +	 * Intel Quark Core DevMan_001.pdf section 6.4.11
> +	 * "The operating system also is required to invalidate (i.e., flush)
> +	 *  the TLB when any changes are made to any of the page table entries.
> +	 *  The operating system must reload CR3 to cause the TLB to be flushed"
> +	 *
> +	 * As a result cpu_has_pge() in arch/x86/include/asm/tlbflush.h should
> +	 * be false so that __flush_tlb_all() causes CR3 insted of CR4.PGE
> +	 * to be modified
> +	 */
> +	if (c->x86 == 5 && c->x86_model == 9) {
> +		pr_info("Disabling PGE capability bit\n");
> +		setup_clear_cpu_cap(X86_FEATURE_PGE);
> +	}
>   }
>
>   #ifdef CONFIG_X86_32
>

I believe there is one more change needed: there is a __flush_tlb_all() 
in the early code which gets executed before the above code runs; the 
easiest fix is to just add a __flush_tlb() immediately after it.

This should have been pushed upstream, and not stayed in the BSP kernel.

	-hpa

Re: [tip:x86/platform] x86/intel/quark: Switch off CR4.PGE so TLB flush uses CR3 instead

[Bryan O'Donoghue]

>
> I believe there is one more change needed: there is a __flush_tlb_all()
> in the early code which gets executed before the above code runs; the
> easiest fix is to just add a __flush_tlb() immediately after it.
>
> This should have been pushed upstream, and not stayed in the BSP kernel.

Peter.

You're talking about void __init setup_arch() right ?

The code looks like this

load_cr3(swapper_pg_dir);
__flush_tlb_all();

Note that on Quark the method to invalidate the TLB is by reloading CR3 
- which is immediately prior to __flush_tlb_all();

So __flush_tlb_all(); will do nothing but that's OK since load_cr3() 
already flushed the TLB - including any PTE with PGE set


Best,
Bryan

Re: [tip:x86/platform] x86/intel/quark: Switch off CR4.PGE so TLB flush uses CR3 instead

[Bryan O'Donoghue]

On 24/09/14 16:58, Bryan O'Donoghue wrote:
>
>>
>> I believe there is one more change needed: there is a __flush_tlb_all()
>> in the early code which gets executed before the above code runs; the
>> easiest fix is to just add a __flush_tlb() immediately after it.
>>
>> This should have been pushed upstream, and not stayed in the BSP kernel.
>
> Peter.
>
> You're talking about void __init setup_arch() right ?
>
> The code looks like this
>
> load_cr3(swapper_pg_dir);
> __flush_tlb_all();
>
> Note that on Quark the method to invalidate the TLB is by reloading CR3
> - which is immediately prior to __flush_tlb_all();
>
> So __flush_tlb_all(); will do nothing but that's OK since load_cr3()
> already flushed the TLB - including any PTE with PGE set

I take your point though.

It's probably better to be explicit and do the whole

if (quark)
	__flush_tlb();
else
	__flush_tlb_all();

So that we aren't relying on the side effects of the previous statement.

Re: [tip:x86/platform] x86/intel/quark: Switch off CR4.PGE so TLB flush uses CR3 instead

[H. Peter Anvin]

On 09/24/2014 09:07 AM, Bryan O'Donoghue wrote:
>>
>> So __flush_tlb_all(); will do nothing but that's OK since load_cr3()
>> already flushed the TLB - including any PTE with PGE set
> 
> I take your point though.
> 
> It's probably better to be explicit and do the whole
> 
> if (quark)
>     __flush_tlb();
> else
>     __flush_tlb_all();
> 
> So that we aren't relying on the side effects of the previous statement.
> 

Well, we need a comment at least.

	-hpa

Re: [tip:x86/platform] x86/intel/quark: Switch off CR4.PGE so TLB flush uses CR3 instead

[H. Peter Anvin]

On 09/24/2014 11:42 AM, H. Peter Anvin wrote:
> On 09/24/2014 09:07 AM, Bryan O'Donoghue wrote:
>>>
>>> So __flush_tlb_all(); will do nothing but that's OK since load_cr3()
>>> already flushed the TLB - including any PTE with PGE set
>>
>> I take your point though.
>>
>> It's probably better to be explicit and do the whole
>>
>> if (quark)
>>     __flush_tlb();
>> else
>>     __flush_tlb_all();
>>
>> So that we aren't relying on the side effects of the previous statement.
>>
> 
> Well, we need a comment at least.
> 

And certainly we don't need an if/else as we can just do it.

	-hpa

Re: [tip:x86/platform] x86/intel/quark: Switch off CR4.PGE so TLB flush uses CR3 instead

[Bryan O'Donoghue]

>> Well, we need a comment at least.
>>
>
> And certainly we don't need an if/else as we can just do it.
>
> 	-hpa

OK.

No problem.

BOD