[PATCH] x86, setup: Additional __flush_tlb for Quak X1000

[PATCH] x86, setup: Additional __flush_tlb for Quak X1000

[Ong Boon Leong]

Hi,

With respect to submission sent below:
  "x86: Quark: Comment setup_arch for TLB/PGE bugfix"

__flush_tlb_all() does not take __flush_tlb() because cpu_has_pge
flag that is setup in head_32.S by CPUID and not changed much later.
So, the comment added are incorrect. Explaination below:

For change on:

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

The X86_FEATURE_PGE cap is cleared in early_cpu_init() -->
 early_identify_cpu() --> early_init_intel(). And, early_cpu_init()
is performed after the __flush_tlb_all() step.

Anyway, thanks to Bryan for submitting patch for Quark SoC.

FYI, the suggestion to add __flush_tlb() has been discussed with some
of the kernel developers CC'ed below inclusing HPA himself.

Thank you very much,
Ong Boon Leong

Ong Boon Leong (1):
  x86, setup: add __flush_tlb() for Intel Quark X1000

 arch/x86/kernel/setup.c |    7 +++++++
 1 file changed, 7 insertions(+)

-- 
1.7.9.5

[PATCH] x86, setup: add __flush_tlb() for Intel Quark X1000

[Ong Boon Leong]

Intel Quark X1000 advertises PGE (bit 13 of EDX) in CPUID.
In reality, it does not implement PGE in current silicon.

This is an issue because __flush_tlb_all() depends on cpu_has_pge
which has following dependency:
 - cpu_has_pge --> boot_cpu_data --> new_cpu_data obtained from CPUID
   in head_32.S.

On another note in reference to the below commit:

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

The value of cpu_has_pge is updated to correctly reflect the
capability for Quark later within early_init_intel() through
setup_clear_cpu_cap(X86_FEATURE_PGE). So, future reference to
__flush_tlb_all() is mapped to __flush_tlb().

As this is early stage of kernel boot-up and  adding
__flush_tlb() is not going to hurt much in CPU cycles, we add it
here and together with the explanation for future reference.

Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com>
---
 arch/x86/kernel/setup.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 41ead8d..90e0b85 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -880,6 +880,13 @@ void __init setup_arch(char **cmdline_p)
 
 	load_cr3(swapper_pg_dir);
 	__flush_tlb_all();
+	/*
+	 * Quark X1000 wrongly advertises PGE, add __flush_tlb()
+	 * below to make sure 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();
+	 */
+	__flush_tlb();
 #else
 	printk(KERN_INFO "Command line: %s\n", boot_command_line);
 #endif
-- 
1.7.9.5

Re: [PATCH] x86, setup: add __flush_tlb() for Intel Quark X1000

[Dave Hansen]

On 09/25/2014 09:30 PM, Ong Boon Leong wrote:
> Intel Quark X1000 advertises PGE (bit 13 of EDX) in CPUID.
> In reality, it does not implement PGE in current silicon.
> 
> This is an issue because __flush_tlb_all() depends on cpu_has_pge
> which has following dependency:
>  - cpu_has_pge --> boot_cpu_data --> new_cpu_data obtained from CPUID
>    in head_32.S.

Can I suggest a better description?

__flush_tlb_all() looks at the CPUID PGE bit.  If it finds it, the TLB
is flushed by clearing and then setting the PGE bit in CR4.  However,
since the Intel Quark X1000 does not _actually_ have PGE, this bit in
CR4 is ignored and the  attempt to flush the TLB does nothing.

Normally, we can fix up these kinds of CPUID mishaps in software.
However, this is earlier than that fixup code actually runs.

To work around this, we simply use an unconditional __flush_tlb() which
uses a cr3 write instead of the PGE bit twiddling in CR4.

We considered attempting to reorder the CPUID fixup code, but decided
that we were more likely to cause collateral damage if we tried this.
The cost of the extra TLB flush is minimal and is unlikely to break
anything else.

> As this is early stage of kernel boot-up and  adding
> __flush_tlb() is not going to hurt much in CPU cycles, we add it
> here and together with the explanation for future reference.
> 
> Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com>
> ---
>  arch/x86/kernel/setup.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 41ead8d..90e0b85 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -880,6 +880,13 @@ void __init setup_arch(char **cmdline_p)
>  
>  	load_cr3(swapper_pg_dir);
>  	__flush_tlb_all();
> +	/*
> +	 * Quark X1000 wrongly advertises PGE, add __flush_tlb()
> +	 * below to make sure 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();
> +	 */
> +	__flush_tlb();

I'd just say:

Quark X1000 wrongly advertises PGE.  Work around this with an
unconditional full flush using a CR3 write instead of CR4.PGE:

Re: [PATCH] x86, setup: add __flush_tlb() for Intel Quark X1000

[Bryan O'Donoghue]

On 26/09/14 06:32, Dave Hansen wrote:
> I'd just say:
>
> Quark X1000 wrongly advertises PGE.  Work around this with an
> unconditional full flush using a CR3 write instead of CR4.PGE:

Hi Dave.

In another thread with Ingo Molnar and Henrique de Moraes Holschuh the 
following text was proposed.

+	/*
+	 * 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 flush here,
+	 * which is what is needed in 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
+	 */

Path submitted as "[PATCH] x86: Quark: Comment setup_arch for TLB/PGE 
bugfix"

Does that work for you ?

--
BOD

Re: [PATCH] x86, setup: add __flush_tlb() for Intel Quark X1000

[Bryan O'Donoghue]

> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 41ead8d..90e0b85 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -880,6 +880,13 @@ void __init setup_arch(char **cmdline_p)
>
>   	load_cr3(swapper_pg_dir);
>   	__flush_tlb_all();
> +	/*
> +	 * Quark X1000 wrongly advertises PGE, add __flush_tlb()
> +	 * below to make sure 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();
> +	 */
> +	__flush_tlb();
>   #else
>   	printk(KERN_INFO "Command line: %s\n", boot_command_line);
>   #endif

Sorry guys.

Just actually *read* the patch now.

If the above text works for Ingo and Henrique then it works for me too.


--
BOD

Re: [PATCH] x86, setup: add __flush_tlb() for Intel Quark X1000

[Dave Hansen]

On 09/26/2014 01:44 AM, Bryan O'Donoghue wrote:
> 
> +    /*
> +     * 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 flush here,
> +     * which is what is needed in 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
> +     */

That looks fine to me.

Re: [PATCH] x86, setup: add __flush_tlb() for Intel Quark X1000

[Bryan O'Donoghue]

On 26/09/14 15:20, Dave Hansen wrote:
> On 09/26/2014 01:44 AM, Bryan O'Donoghue wrote:
>>
>> +    /*
>> +     * 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 flush here,
>> +     * which is what is needed in 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
>> +     */
>
> That looks fine to me.

OK.

I think to keep everybody aligned/on-the-same-page no pun intended.

We want

+    /*
+     * 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 flush here,
+     * which is what is needed in 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
+     */
+    __flush_tlb();

With the extra __flush_tlb(); on the end.

Re: [PATCH] x86, setup: add __flush_tlb() for Intel Quark X1000

[Bryan O'Donoghue]

> +    /*
> +     * 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 flush here,
> +     * which is what is needed in 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
> +     */
> +    __flush_tlb();
>
> With the extra __flush_tlb(); on the end.

Scatch that.

ACK Ong Boon Leong's patch

Re: [PATCH] x86, setup: add __flush_tlb() for Intel Quark X1000

[Bryan O'Donoghue]

On 26/09/14 16:00, Bryan O'Donoghue wrote:
>> +    /*
>> +     * 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 flush here,
>> +     * which is what is needed in 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
>> +     */
>> +    __flush_tlb();
>>
>> With the extra __flush_tlb(); on the end.
>
> Scatch that.
>
> ACK Ong Boon Leong's patch

Hmm.

At the risk of contradicting myself in public I had a think about this 
on the drive home and ...

As I see it - the reload @ CR3 should have flushed the TLB. That's what 
the documentation says.

Right now with the proposed patch from Ong Boon Leong the code will be

load_cr3(swapper_pg_dir);
   __flush_tlb_all();
__flush_tlb();

While there may be no *harm* in adding an extra __flush_tlb(); - there's 
not much *sense* in it either.

If however there's a strong feeling that an explicit __flush_tlb() is 
required in Quark's case then - I believe the code should revert to the 
original logic from the Quark BSP namely.

+       if (boot_cpu_data.x86 == 5 && boot_cpu_data.x86_model == 9)
+               __flush_tlb();
+       else
+               __flush_tlb_all();


This was Peter's initial suggestion in any case and as I see it the 
right-thing-to-do rather than have another reduntant __flush_tlb() for 
everybody who's not Quark.

How does that make sense ?

I can't imagine it's preferable to do a __flush_tlb(); on *x86 for the 
sake of skipping an if/else

I'll resubmit that now with Ong Boon's commentary.


Best,
BOD