[PATCH] x86: Quark: Flush TLB via CR3 not CR4.PGE in setup_arch()

[PATCH] x86: Quark: Flush TLB via CR3 not CR4.PGE in setup_arch()

[Bryan O'Donoghue]

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

This patch flushes the TLB in the required way for Quark in setup_arch()
See Quark Core_DevMan_001.pdf section 6.4.11

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

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 41ead8d..1d2396a 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -879,7 +879,10 @@ void __init setup_arch(char **cmdline_p)
 			KERNEL_PGD_PTRS);
 
 	load_cr3(swapper_pg_dir);
-	__flush_tlb_all();
+	if (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] x86: Quark: Flush TLB via CR3 not CR4.PGE in setup_arch()

[Ingo Molnar]

* Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote:

> Quark X1000 requires CR3 to be rewritten to flush TLB entries
> irrespective of the PGE bits in CR4 or PTE.PGE
> 
> This patch flushes the TLB in the required way for Quark in setup_arch()
> See Quark Core_DevMan_001.pdf section 6.4.11
> 
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
> ---
>  arch/x86/kernel/setup.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 41ead8d..1d2396a 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -879,7 +879,10 @@ void __init setup_arch(char **cmdline_p)
>  			KERNEL_PGD_PTRS);
>  
>  	load_cr3(swapper_pg_dir);
> -	__flush_tlb_all();
> +	if (boot_cpu_data.x86 == 5 && boot_cpu_data.x86_model == 9)
> +		__flush_tlb();
> +	else
> +		__flush_tlb_all();

So why not make __flush_tlb_all() Quark-quirk-aware and be done 
with it, instead of having to validate every single 
__flush_tlb_all() user?

Quark breaks the x86 'flush all TLBs' semantics - the way to fix 
it is to restore those semantics, not to sprinkle the breakage 
all around the code ...

Thanks,

	Ingo

Re: [PATCH] x86: Quark: Flush TLB via CR3 not CR4.PGE in setup_arch()

[Bryan O'Donoghue]

>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index 41ead8d..1d2396a 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -879,7 +879,10 @@ void __init setup_arch(char **cmdline_p)
>>   			KERNEL_PGD_PTRS);
>>
>>   	load_cr3(swapper_pg_dir);
>> -	__flush_tlb_all();
>> +	if (boot_cpu_data.x86 == 5 && boot_cpu_data.x86_model == 9)
>> +		__flush_tlb();
>> +	else
>> +		__flush_tlb_all();
>
> So why not make __flush_tlb_all() Quark-quirk-aware and be done
> with it, instead of having to validate every single
> __flush_tlb_all() user?
>
> Quark breaks the x86 'flush all TLBs' semantics - the way to fix
> it is to restore those semantics, not to sprinkle the breakage
> all around the code ...

Hi Ingo.

We have made __flush_tlb_all() Quark aware - because the previous patch 
we applied to arch/x86/kernel/cpu/intel.c

ee1b5b165c0a2f04d2107e634e51f05d0eb107de

+	if (c->x86 == 5 && c->x86_model == 9) {
+		pr_info("Disabling PGE capability bit\n");
+		setup_clear_cpu_cap(X86_FEATURE_PGE);
+	}

will cause cpu_has_pge() to be false and then the flush_tlb_all code 
will take the path we want __flush_tlb not __flush_tlb_global

static inline void __flush_tlb_all(void)
{
         if (cpu_has_pge)
                 __flush_tlb_global();
         else
                 __flush_tlb();
}

The code in setup_arch runs before the cpu_has_pge bit been clobbered.

The commit you did 02276a3a677d681f0cd227d7111c71fdbce23832 just adds a 
comment to setup_arch to indicate the behaviour we are relying on

setup_arch()
{

load_cr3(swapper_pg_dir);	/* this will flush the TLB on Quark */
__flush_tlb_all();		/*cpu_has_pge() is true at this point*/

......

/* this is where we latch the cpu cabability bits */
early_cpu_init();
}

--
BOD

Re: [PATCH] x86: Quark: Flush TLB via CR3 not CR4.PGE in setup_arch()

[Ingo Molnar]

* Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote:

> >>diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> >>index 41ead8d..1d2396a 100644
> >>--- a/arch/x86/kernel/setup.c
> >>+++ b/arch/x86/kernel/setup.c
> >>@@ -879,7 +879,10 @@ void __init setup_arch(char **cmdline_p)
> >>  			KERNEL_PGD_PTRS);
> >>
> >>  	load_cr3(swapper_pg_dir);
> >>-	__flush_tlb_all();
> >>+	if (boot_cpu_data.x86 == 5 && boot_cpu_data.x86_model == 9)
> >>+		__flush_tlb();
> >>+	else
> >>+		__flush_tlb_all();
> >
> >So why not make __flush_tlb_all() Quark-quirk-aware and be done
> >with it, instead of having to validate every single
> >__flush_tlb_all() user?
> >
> >Quark breaks the x86 'flush all TLBs' semantics - the way to fix
> >it is to restore those semantics, not to sprinkle the breakage
> >all around the code ...
> 
> Hi Ingo.
> 
> We have made __flush_tlb_all() Quark aware - because the previous patch we
> applied to arch/x86/kernel/cpu/intel.c
> 
> ee1b5b165c0a2f04d2107e634e51f05d0eb107de
> 
> +	if (c->x86 == 5 && c->x86_model == 9) {
> +		pr_info("Disabling PGE capability bit\n");
> +		setup_clear_cpu_cap(X86_FEATURE_PGE);
> +	}
> 
> will cause cpu_has_pge() to be false and then the flush_tlb_all code will
> take the path we want __flush_tlb not __flush_tlb_global
> 
> static inline void __flush_tlb_all(void)
> {
>         if (cpu_has_pge)
>                 __flush_tlb_global();
>         else
>                 __flush_tlb();
> }
> 
> The code in setup_arch runs before the cpu_has_pge bit been clobbered.

So why is the comment in setup_arch() not talking about that?

> The commit you did 02276a3a677d681f0cd227d7111c71fdbce23832 just adds a
> comment to setup_arch to indicate the behaviour we are relying on
> 
> setup_arch()
> {
> 
> load_cr3(swapper_pg_dir);	/* this will flush the TLB on Quark */
> __flush_tlb_all();		/*cpu_has_pge() is true at this point*/
> 
> ......
> 
> /* this is where we latch the cpu cabability bits */
> early_cpu_init();
> }

I've zapped that commit for the time being, because I think that 
at minimum the comment is misleading - it says nothing about this 
being an early quirk and that normally __flush_tlb_all() will 
DTRT.

It talks about:

+       /*
+        * Locate the page directory and flush the TLB.
+        * On Quark X1000 rewriting CR3 flushes the TLB no if/else is required
+        * to choose between __flush_tlb() and __flush_tlb_all()
+        */
        load_cr3(swapper_pg_dir);
        __flush_tlb_all();

But it is completely silent on the real reason for why we don't 
need a Quark quirk here, which would be something like:

	/*
	 * On Quark CPUs we still have the PGE bit set so 
	 * __flush_tlb_all() is not yet doing what it says - but 
	 * accidentally we have a cr3 flush here which is what is 
	 * needed - so there's no need to add a Quark quirk here.
	 */

Right?

Thanks,

	Ingo

Re: [PATCH] x86: Quark: Flush TLB via CR3 not CR4.PGE in setup_arch()

[Bryan O'Donoghue]

> It talks about:
>
> +       /*
> +        * Locate the page directory and flush the TLB.
> +        * On Quark X1000 rewriting CR3 flushes the TLB no if/else is required
> +        * to choose between __flush_tlb() and __flush_tlb_all()
> +        */
>          load_cr3(swapper_pg_dir);
>          __flush_tlb_all();
>
> But it is completely silent on the real reason for why we don't
> need a Quark quirk here, which would be something like:
>
> 	/*
> 	 * On Quark CPUs we still have the PGE bit set so
> 	 * __flush_tlb_all() is not yet doing what it says - but
> 	 * accidentally we have a cr3 flush here which is what is
> 	 * needed - so there's no need to add a Quark quirk here.
> 	 */
>
> Right?

OK.

IMO.

If we're adding a comment though the first thing the comment ought to 
say is what the code does for everybody else - stuff CR3 and flush the 
TLB, then it should comment on the exception for Quark.

/*
  * Locate the page directory and flush the TLB.
  *
  * On Quark CPUs we still have the PGE bit set so
  * __flush_tlb_all() is not yet doing what it says - but
  * accidentally we have a cr3 flush here which is what is
  * needed - so there's no need to add a Quark quirk here.
  */

?

Re: [PATCH] x86: Quark: Flush TLB via CR3 not CR4.PGE in setup_arch()

[Ingo Molnar]

* Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote:

> >It talks about:
> >
> >+       /*
> >+        * Locate the page directory and flush the TLB.
> >+        * On Quark X1000 rewriting CR3 flushes the TLB no if/else is required
> >+        * to choose between __flush_tlb() and __flush_tlb_all()
> >+        */
> >         load_cr3(swapper_pg_dir);
> >         __flush_tlb_all();
> >
> >But it is completely silent on the real reason for why we don't
> >need a Quark quirk here, which would be something like:
> >
> >	/*
> >	 * On Quark CPUs we still have the PGE bit set so
> >	 * __flush_tlb_all() is not yet doing what it says - but
> >	 * accidentally we have a cr3 flush here which is what is
> >	 * needed - so there's no need to add a Quark quirk here.
> >	 */
> >
> >Right?
> 
> OK.
> 
> IMO.
> 
> If we're adding a comment though the first thing the comment ought to say is
> what the code does for everybody else - stuff CR3 and flush the TLB, then it
> should comment on the exception for Quark.
> 
> /*
>  * Locate the page directory and flush the TLB.
>  *
>  * On Quark CPUs we still have the PGE bit set so
>  * __flush_tlb_all() is not yet doing what it says - but
>  * accidentally we have a cr3 flush here which is what is
>  * needed - so there's no need to add a Quark quirk here.
>  */
> 
> ?

Yeah, fair enough. You can even put the latter in parentheses, to 
signal that it's all a rare case.

Thanks,

	Ingo

Re: [PATCH] x86: Quark: Flush TLB via CR3 not CR4.PGE in setup_arch()

[Henrique de Moraes Holschuh]

On Thu, 25 Sep 2014, Ingo Molnar wrote:
> > If we're adding a comment though the first thing the comment ought to say is
> > what the code does for everybody else - stuff CR3 and flush the TLB, then it
> > should comment on the exception for Quark.
> > 
> > /*
> >  * Locate the page directory and flush the TLB.
> >  *
> >  * On Quark CPUs we still have the PGE bit set so
> >  * __flush_tlb_all() is not yet doing what it says - but
> >  * accidentally we have a cr3 flush here which is what is
> >  * needed - so there's no need to add a Quark quirk here.
> >  */
> > 
> > ?
> 
> Yeah, fair enough. You can even put the latter in parentheses, to 
> signal that it's all a rare case.

I'd have mentioned "erratum" there, otherwise people won't understand why
the PGE bit being set would be a problem in the first place.

Something like:

/*
 * 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.
 */

-- 
  "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] x86: Quark: Flush TLB via CR3 not CR4.PGE in setup_arch()

[Ingo Molnar]

* Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:

> On Thu, 25 Sep 2014, Ingo Molnar wrote:
> > > If we're adding a comment though the first thing the comment ought to say is
> > > what the code does for everybody else - stuff CR3 and flush the TLB, then it
> > > should comment on the exception for Quark.
> > > 
> > > /*
> > >  * Locate the page directory and flush the TLB.
> > >  *
> > >  * On Quark CPUs we still have the PGE bit set so
> > >  * __flush_tlb_all() is not yet doing what it says - but
> > >  * accidentally we have a cr3 flush here which is what is
> > >  * needed - so there's no need to add a Quark quirk here.
> > >  */
> > > 
> > > ?
> > 
> > Yeah, fair enough. You can even put the latter in parentheses, to 
> > signal that it's all a rare case.
> 
> I'd have mentioned "erratum" there, otherwise people won't understand why
> the PGE bit being set would be a problem in the first place.
> 
> Something like:
> 
> /*
>  * 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.
>  */

Yeah.

I'd also add the fact that it's an unintended erratum to the 
Quark quirk section of early_init_intel() as well.

Thanks,

	Ingo

Re: [PATCH] x86: Quark: Flush TLB via CR3 not CR4.PGE in setup_arch()

[Bryan O'Donoghue]

>> I'd have mentioned "erratum" there, otherwise people won't understand why
>> the PGE bit being set would be a problem in the first place.
>>
>> Something like:
>>
>> /*
>>   * 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.
>>   */
>
> Yeah.
>
> I'd also add the fact that it's an unintended erratum to the
> Quark quirk section of early_init_intel() as well.

OK.

How about.

/*
   * 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
   */

Re: [PATCH] x86: Quark: Flush TLB via CR3 not CR4.PGE in setup_arch()

[Ingo Molnar]

* Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote:

> >>I'd have mentioned "erratum" there, otherwise people won't understand why
> >>the PGE bit being set would be a problem in the first place.
> >>
> >>Something like:
> >>
> >>/*
> >>  * 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.
> >>  */
> >
> >Yeah.
> >
> >I'd also add the fact that it's an unintended erratum to the
> >Quark quirk section of early_init_intel() as well.
> 
> OK.
> 
> How about.
> 
> /*
>   * 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
>   */

Sounds good.

Thanks,

	Ingo