diff mbox

powerpc: Fix invalid page flags in create TLB CAM path for PTE_64BIT

Message ID 1285272615-22758-1-git-send-email-paul.gortmaker@windriver.com (mailing list archive)
State Superseded
Delegated to: Kumar Gala
Headers show

Commit Message

Paul Gortmaker Sept. 23, 2010, 8:10 p.m. UTC 
From: Tiejun Chen <tiejun.chen@windriver.com>

There exists a four line chunk of code, which when configured for
64 bit address space, can incorrectly set certain page flags during
the TLB creation.  It turns out that this is legacy code that is no
longer required, but since it isn't obvious why this is legacy code
or why it causes problems, the below description covers both in detail.

For powerpc bootstrap, the physical memory (at most 768M), is mapped
into the kernel space via the following path:

MMU_init()
    |
    + adjust_total_lowmem()
            |
            + map_mem_in_cams()
                    |
                    + settlbcam(i, virt, phys, cam_sz, PAGE_KERNEL_X, 0);

On settlbcam(), the kernel will create TLB entries according to the flag,
PAGE_KERNEL_X.

settlbcam()
{
        ...
        TLBCAM[index].MAS1 = MAS1_VALID
                        | MAS1_IPROT | MAS1_TSIZE(tsize) | MAS1_TID(pid);
                                ^
			These entries cannot be invalidated by the
			kernel since MAS1_IPROT is set on TLB property.
        ...
        if (flags & _PAGE_USER) {
           TLBCAM[index].MAS3 |= MAS3_UX | MAS3_UR;
           TLBCAM[index].MAS3 |= ((flags & _PAGE_RW) ? MAS3_UW : 0);
        }

For classic BookE (flags & _PAGE_USER) is 'zero' so it's fine.
But on boards like the the Freescale P4080, we want to support 36-bit
physical address on it. So the following options may be set:

CONFIG_FSL_BOOKE=y
CONFIG_PTE_64BIT=y
CONFIG_PHYS_64BIT=y

As a result, boards like the P4080 will introduce PTE format as Book3E.
As per the file: arch/powerpc/include/asm/pgtable-ppc32.h

  * #elif defined(CONFIG_FSL_BOOKE) && defined(CONFIG_PTE_64BIT)
  * #include <asm/pte-book3e.h>

So PAGE_KERNEL_X is __pgprot(_PAGE_BASE | _PAGE_KERNEL_RWX) and the
book3E version of _PAGE_KERNEL_RWX is defined with:

  (_PAGE_BAP_SW | _PAGE_BAP_SR | _PAGE_DIRTY | _PAGE_BAP_SX)

Note the _PAGE_BAP_SR, which is also defined in the book3E _PAGE_USER:

  #define _PAGE_USER        (_PAGE_BAP_UR | _PAGE_BAP_SR) /* Can be read */

So the possibility exists to wrongly assign the user MAS3_U<RWX> bits
to kernel (PAGE_KERNEL_X) address space via the following code fragment:

        if (flags & _PAGE_USER) {
           TLBCAM[index].MAS3 |= MAS3_UX | MAS3_UR;
           TLBCAM[index].MAS3 |= ((flags & _PAGE_RW) ? MAS3_UW : 0);
        }

Here is a dump of the TLB info from Simics with the above code present:
------
L2 TLB1
                                            GT                   SSS UUU V I
 Row  Logical           Physical            SS TLPID  TID  WIMGE XWR XWR F P   V
----- ----------------- ------------------- -- ----- ----- ----- --- --- - -   -
  0   c0000000-cfffffff 000000000-00fffffff 00     0     0   M   XWR XWR 0 1   1
  1   d0000000-dfffffff 010000000-01fffffff 00     0     0   M   XWR XWR 0 1   1
  2   e0000000-efffffff 020000000-02fffffff 00     0     0   M   XWR XWR 0 1   1

Actually this conditional code was only used for two legacy functions:

  1: support KGDB to set break point.
     KGDB already dropped this; now uses its core write to set break point.

  2: io_block_mapping() to create TLB in segmentation size (not PAGE_SIZE)
     for device IO space.
     This use case is also removed from the latest PowerPC kernel.

So it looks like the deletion of these 4 lines of code was simply
overlooked when the above two cases went away.

With the code deleted, the TLB appears without U having XWR as below:

-------
L2 TLB1
                                            GT                   SSS UUU V I
 Row  Logical           Physical            SS TLPID  TID  WIMGE XWR XWR F P   V
----- ----------------- ------------------- -- ----- ----- ----- --- --- - -   -
  0   c0000000-cfffffff 000000000-00fffffff 00     0     0   M   XWR     0 1   1
  1   d0000000-dfffffff 010000000-01fffffff 00     0     0   M   XWR     0 1   1
  2   e0000000-efffffff 020000000-02fffffff 00     0     0   M   XWR     0 1   1

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 arch/powerpc/mm/fsl_booke_mmu.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

Comments

Scott Wood Sept. 23, 2010, 8:33 p.m. UTC | #1 
On Thu, 23 Sep 2010 16:10:15 -0400
Paul Gortmaker <paul.gortmaker@windriver.com> wrote:

> So the possibility exists to wrongly assign the user MAS3_U<RWX> bits
> to kernel (PAGE_KERNEL_X) address space via the following code fragment:
> 
>         if (flags & _PAGE_USER) {
>            TLBCAM[index].MAS3 |= MAS3_UX | MAS3_UR;
>            TLBCAM[index].MAS3 |= ((flags & _PAGE_RW) ? MAS3_UW : 0);
>         }
> 
> Here is a dump of the TLB info from Simics with the above code present:
> ------
> L2 TLB1
>                                             GT                   SSS UUU V I
>  Row  Logical           Physical            SS TLPID  TID  WIMGE XWR XWR F P   V
> ----- ----------------- ------------------- -- ----- ----- ----- --- --- - -   -
>   0   c0000000-cfffffff 000000000-00fffffff 00     0     0   M   XWR XWR 0 1   1
>   1   d0000000-dfffffff 010000000-01fffffff 00     0     0   M   XWR XWR 0 1   1
>   2   e0000000-efffffff 020000000-02fffffff 00     0     0   M   XWR XWR 0 1   1
> 
> Actually this conditional code was only used for two legacy functions:
> 
>   1: support KGDB to set break point.
>      KGDB already dropped this; now uses its core write to set break point.
> 
>   2: io_block_mapping() to create TLB in segmentation size (not PAGE_SIZE)
>      for device IO space.
>      This use case is also removed from the latest PowerPC kernel.

io_block_mapping() went away, but the feature itself is still useful
and might come back with something like this:

http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg33851.html

...though I'm not sure why such mappings would ever have user access.

This could end up being used for large user pages by something like
hugetlbfs or KVM, though.  I don't think we want to make large user
pages fail, especailly if it just happens with the 32-bit page table
format (which i may not what the person adding such a feature tests
with).

I don't see a generic accessor that can test PTE flags for user
access -- in the absence of one, I guess we need an ifdef here.  Or at
least put in a comment so anyone who adds a userspace use knows they
need to fix it.

-Scott
Benjamin Herrenschmidt Sept. 23, 2010, 9:59 p.m. UTC | #2 
On Thu, 2010-09-23 at 15:33 -0500, Scott Wood wrote:
> I don't see a generic accessor that can test PTE flags for user
> access -- in the absence of one, I guess we need an ifdef here.  Or at
> least put in a comment so anyone who adds a userspace use knows they
> need to fix it. 

We could make up one in powerpc arch at least

#define pte_user(val) ((val & _PAGE_USER) == _PAGE_USER)

would do

Cheers,
Ben.
Tiejun Chen Sept. 24, 2010, 4:54 a.m. UTC | #3 
> -----Original Message-----
> From: 
> linuxppc-dev-bounces+tiejun.chen=windriver.com@lists.ozlabs.or
> g 
> [mailto:linuxppc-dev-bounces+tiejun.chen=windriver.com@lists.o
zlabs.org] On Behalf Of Scott Wood
> Sent: Friday, September 24, 2010 4:34 AM
> To: Gortmaker, Paul
> Cc: linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH] powerpc: Fix invalid page flags in 
> create TLB CAM pathfor PTE_64BIT
> 
> On Thu, 23 Sep 2010 16:10:15 -0400
> Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> 
> > So the possibility exists to wrongly assign the user 
> MAS3_U<RWX> bits 
> > to kernel (PAGE_KERNEL_X) address space via the following 
> code fragment:
> > 
> >         if (flags & _PAGE_USER) {
> >            TLBCAM[index].MAS3 |= MAS3_UX | MAS3_UR;
> >            TLBCAM[index].MAS3 |= ((flags & _PAGE_RW) ? MAS3_UW : 0);
> >         }
> > 
> > Here is a dump of the TLB info from Simics with the above 
> code present:
> > ------
> > L2 TLB1
> >                                             GT              
>      SSS UUU V I
> >  Row  Logical           Physical            SS TLPID  TID  
> WIMGE XWR XWR F P   V
> > ----- ----------------- ------------------- -- ----- ----- 
> ----- --- --- - -   -
> >   0   c0000000-cfffffff 000000000-00fffffff 00     0     0  
>  M   XWR XWR 0 1   1
> >   1   d0000000-dfffffff 010000000-01fffffff 00     0     0  
>  M   XWR XWR 0 1   1
> >   2   e0000000-efffffff 020000000-02fffffff 00     0     0  
>  M   XWR XWR 0 1   1
> > 
> > Actually this conditional code was only used for two legacy 
> functions:
> > 
> >   1: support KGDB to set break point.
> >      KGDB already dropped this; now uses its core write to 
> set break point.
> > 
> >   2: io_block_mapping() to create TLB in segmentation size 
> (not PAGE_SIZE)
> >      for device IO space.
> >      This use case is also removed from the latest PowerPC kernel.
> 
> io_block_mapping() went away, but the feature itself is still 
> useful and might come back with something like this:
> 
> http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg3
3851.html
> 
> ...though I'm not sure why such mappings would ever have user access.
> 
> This could end up being used for large user pages by 
> something like hugetlbfs or KVM, though.  I don't think we 
> want to make large user pages fail, especailly if it just 

Understand. 

Actually the following is my original modification. 
======
+#if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_PTE_64BIT)
+	/* On there _PAGE_BAP_UR is always integrated into flag,
_PAGE_KERNEL_RWX 
+	 * and _PAGE_USER here. So we have to only check _PAGE_BAP_UR as
the condition.
+ 	 */
+	if (flags & _PAGE_BAP_UR) {
+#else
	if (flags & _PAGE_USER) {
+#endif

But I find there is no any usage for this, except for the above #1> KGDB
and #2 io_block_mapping(). So I think it's possible to remove this
completely :)

> happens with the 32-bit page table format (which i may not 
> what the person adding such a feature tests with).
> 
> I don't see a generic accessor that can test PTE flags for 
> user access -- in the absence of one, I guess we need an 
> ifdef here.  Or at least put in a comment so anyone who adds 
> a userspace use knows they need to fix it.

I already notice Ben's advice and looks fine to us.

Tiejun

> 
> -Scott
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
Tiejun Chen Sept. 24, 2010, 5:04 a.m. UTC | #4 
> -----Original Message-----
> From: 
> linuxppc-dev-bounces+tiejun.chen=windriver.com@lists.ozlabs.or
> g 
> [mailto:linuxppc-dev-bounces+tiejun.chen=windriver.com@lists.o
> zlabs.org] On Behalf Of Benjamin Herrenschmidt
> Sent: Friday, September 24, 2010 5:59 AM
> To: Scott Wood
> Cc: Gortmaker, Paul; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH] powerpc: Fix invalid page flags in 
> create TLB CAM pathfor PTE_64BIT
> 
> On Thu, 2010-09-23 at 15:33 -0500, Scott Wood wrote:
> > I don't see a generic accessor that can test PTE flags for 
> user access 
> > -- in the absence of one, I guess we need an ifdef here.  
> Or at least 
> > put in a comment so anyone who adds a userspace use knows 
> they need to 
> > fix it.
> 
> We could make up one in powerpc arch at least
> 
> #define pte_user(val) ((val & _PAGE_USER) == _PAGE_USER)
> 

Looks good. 

Ben and Scott,

But for the patched issue we're discussing we have to do #ifdef that as
my original modification. Right? Or do you have other suggestion? Then I
can improve that as v2.

Thanks
Tiejun 

> would do
> 
> Cheers,
> Ben.
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
Scott Wood Sept. 24, 2010, 3:02 p.m. UTC | #5 
On Fri, 24 Sep 2010 07:04:28 +0200
"Chen, Tiejun" <Tiejun.Chen@windriver.com> wrote:

> > -----Original Message-----
> > From: 
> > linuxppc-dev-bounces+tiejun.chen=windriver.com@lists.ozlabs.or
> > g 
> > [mailto:linuxppc-dev-bounces+tiejun.chen=windriver.com@lists.o
> > zlabs.org] On Behalf Of Benjamin Herrenschmidt
> > Sent: Friday, September 24, 2010 5:59 AM
> > To: Scott Wood
> > Cc: Gortmaker, Paul; linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH] powerpc: Fix invalid page flags in 
> > create TLB CAM pathfor PTE_64BIT
> > 
> > On Thu, 2010-09-23 at 15:33 -0500, Scott Wood wrote:
> > > I don't see a generic accessor that can test PTE flags for 
> > user access 
> > > -- in the absence of one, I guess we need an ifdef here.  
> > Or at least 
> > > put in a comment so anyone who adds a userspace use knows 
> > they need to 
> > > fix it.
> > 
> > We could make up one in powerpc arch at least
> > 
> > #define pte_user(val) ((val & _PAGE_USER) == _PAGE_USER)
> > 
> 
> Looks good. 
> 
> Ben and Scott,
> 
> But for the patched issue we're discussing we have to do #ifdef that as
> my original modification. Right? Or do you have other suggestion? Then I
> can improve that as v2.

Ben's version should work without any ifdef, since it makes sure
all bits of _PAGE_USER are set.

-Scott
diff mbox

Patch

diff --git a/arch/powerpc/mm/fsl_booke_mmu.c b/arch/powerpc/mm/fsl_booke_mmu.c
index d5fa5f2..9de7e1b 100644
--- a/arch/powerpc/mm/fsl_booke_mmu.c
+++ b/arch/powerpc/mm/fsl_booke_mmu.c
@@ -136,11 +136,6 @@  static void settlbcam(int index, unsigned long virt, phys_addr_t phys,
 	if (mmu_has_feature(MMU_FTR_BIG_PHYS))
 		TLBCAM[index].MAS7 = (u64)phys >> 32;
 
-	if (flags & _PAGE_USER) {
-	   TLBCAM[index].MAS3 |= MAS3_UX | MAS3_UR;
-	   TLBCAM[index].MAS3 |= ((flags & _PAGE_RW) ? MAS3_UW : 0);
-	}
-
 	tlbcam_addrs[index].start = virt;
 	tlbcam_addrs[index].limit = virt + size - 1;
 	tlbcam_addrs[index].phys = phys;