[PATCH] powerpc: fix ioremap_flags() with book3e pte definition

[PATCH] powerpc: fix ioremap_flags() with book3e pte definition

[Li Yang]

We can't just clear the user read permission in book3e pte, because
that will also clear supervisor read permission.  This surely isn't
desired.  Fix the problem by adding the supervisor read back.

Signed-off-by: Li Yang <leoli@freescale.com>
---
 arch/powerpc/mm/pgtable_32.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index cb96cb2..aff7c04 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -144,6 +144,11 @@ ioremap_flags(phys_addr_t addr, unsigned long size, unsigned long flags)
 	/* we don't want to let _PAGE_USER and _PAGE_EXEC leak out */
 	flags &= ~(_PAGE_USER | _PAGE_EXEC);
 
+#if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_PTE_64BIT)
+	/* supervisor read permission has just been cleared, add back */
+	flags |= _PAGE_BAP_SR;
+#endif
+
 	return __ioremap_caller(addr, size, flags, __builtin_return_address(0));
 }
 EXPORT_SYMBOL(ioremap_flags);
-- 
1.6.6-rc1.GIT

Re: [PATCH] powerpc: fix ioremap_flags() with book3e pte definition

[Benjamin Herrenschmidt]

On Fri, 2010-02-05 at 00:53 +0800, Li Yang wrote:
> We can't just clear the user read permission in book3e pte, because
> that will also clear supervisor read permission.  This surely isn't
> desired.  Fix the problem by adding the supervisor read back.
> 
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
>  arch/powerpc/mm/pgtable_32.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
> index cb96cb2..aff7c04 100644
> --- a/arch/powerpc/mm/pgtable_32.c
> +++ b/arch/powerpc/mm/pgtable_32.c
> @@ -144,6 +144,11 @@ ioremap_flags(phys_addr_t addr, unsigned long size, unsigned long flags)
>  	/* we don't want to let _PAGE_USER and _PAGE_EXEC leak out */
>  	flags &= ~(_PAGE_USER | _PAGE_EXEC);
>  
> +#if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_PTE_64BIT)
> +	/* supervisor read permission has just been cleared, add back */
> +	flags |= _PAGE_BAP_SR;
> +#endif
> +

So this is a bit fishy indeed. pgtable_64.c seems to have the same
problem in fact.

It boils down to the "hack" I have in the new PTE format which consists
of having _SR be part of _PAGE_USER. I wonder if I should change that
instead. Kumar, what do you reckon ?

_Maybe_ an option is to have _PAGE_NO_USER that defaults to be
~_PAGE_USER in pte-common.h and can be set in pte-book3e.h to
only strip out UR and UW ?

Cheers,
Ben.

Re: [PATCH] powerpc: fix ioremap_flags() with book3e pte definition

[Kumar Gala]

On Feb 7, 2010, at 10:21 PM, Benjamin Herrenschmidt wrote:

> On Fri, 2010-02-05 at 00:53 +0800, Li Yang wrote:
>> We can't just clear the user read permission in book3e pte, because
>> that will also clear supervisor read permission.  This surely isn't
>> desired.  Fix the problem by adding the supervisor read back.
>>=20
>> Signed-off-by: Li Yang <leoli@freescale.com>
>> ---
>> arch/powerpc/mm/pgtable_32.c |    5 +++++
>> 1 files changed, 5 insertions(+), 0 deletions(-)
>>=20
>> diff --git a/arch/powerpc/mm/pgtable_32.c =
b/arch/powerpc/mm/pgtable_32.c
>> index cb96cb2..aff7c04 100644
>> --- a/arch/powerpc/mm/pgtable_32.c
>> +++ b/arch/powerpc/mm/pgtable_32.c
>> @@ -144,6 +144,11 @@ ioremap_flags(phys_addr_t addr, unsigned long =
size, unsigned long flags)
>> 	/* we don't want to let _PAGE_USER and _PAGE_EXEC leak out */
>> 	flags &=3D ~(_PAGE_USER | _PAGE_EXEC);
>>=20
>> +#if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_PTE_64BIT)
>> +	/* supervisor read permission has just been cleared, add back */
>> +	flags |=3D _PAGE_BAP_SR;
>> +#endif
>> +
>=20
> So this is a bit fishy indeed. pgtable_64.c seems to have the same
> problem in fact.
>=20
> It boils down to the "hack" I have in the new PTE format which =
consists
> of having _SR be part of _PAGE_USER. I wonder if I should change that
> instead. Kumar, what do you reckon ?
>=20
> _Maybe_ an option is to have _PAGE_NO_USER that defaults to be
> ~_PAGE_USER in pte-common.h and can be set in pte-book3e.h to
> only strip out UR and UW ?

We never closed on this.  I think lets add _PAGE_NO_USER

- k=

[PATCH] powerpc: Fix ioremap_flags() with book3e pte definition

[Kumar Gala]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

We can't just clear the user read permission in book3e pte, because
that will also clear supervisor read permission.  This surely isn't
desired.  Fix the problem by adding the supervisor read back.

BenH: Slightly simplified the ifdef and applied to ppc64 too

Signed-off-by: Li Yang <leoli@freescale.com>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
For 2.6.34 stable commit 55052eeca6d71d76f7c3f156c0501814d8e5e6d3

 arch/powerpc/mm/pgtable_32.c |    8 ++++++++
 arch/powerpc/mm/pgtable_64.c |    8 ++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index b9243e7..767b0cf 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -146,6 +146,14 @@ ioremap_flags(phys_addr_t addr, unsigned long size, unsigned long flags)
 	/* we don't want to let _PAGE_USER and _PAGE_EXEC leak out */
 	flags &= ~(_PAGE_USER | _PAGE_EXEC);
 
+#ifdef _PAGE_BAP_SR
+	/* _PAGE_USER contains _PAGE_BAP_SR on BookE using the new PTE format
+	 * which means that we just cleared supervisor access... oops ;-) This
+	 * restores it
+	 */
+	flags |= _PAGE_BAP_SR;
+#endif
+
 	return __ioremap_caller(addr, size, flags, __builtin_return_address(0));
 }
 EXPORT_SYMBOL(ioremap_flags);
diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
index d95679a..d050fc8 100644
--- a/arch/powerpc/mm/pgtable_64.c
+++ b/arch/powerpc/mm/pgtable_64.c
@@ -265,6 +265,14 @@ void __iomem * ioremap_flags(phys_addr_t addr, unsigned long size,
 	/* we don't want to let _PAGE_USER and _PAGE_EXEC leak out */
 	flags &= ~(_PAGE_USER | _PAGE_EXEC);
 
+#ifdef _PAGE_BAP_SR
+	/* _PAGE_USER contains _PAGE_BAP_SR on BookE using the new PTE format
+	 * which means that we just cleared supervisor access... oops ;-) This
+	 * restores it
+	 */
+	flags |= _PAGE_BAP_SR;
+#endif
+
 	if (ppc_md.ioremap)
 		return ppc_md.ioremap(addr, size, flags, caller);
 	return __ioremap_caller(addr, size, flags, caller);
-- 
1.6.0.6

[PATCH] powerpc/fsl-booke: Fix InstructionTLBError execute permission check

[Kumar Gala]

From: Li Yang <leoli@freescale.com>

In CONFIG_PTE_64BIT the PTE format has unique permission bits for user
and supervisor execute.  However on !CONFIG_PTE_64BIT we overload the
supervisor bit to imply user execute with _PAGE_USER set.  This allows
us to use the same permission check mask for user or supervisor code on
!CONFIG_PTE_64BIT.

However, on CONFIG_PTE_64BIT we map _PAGE_EXEC to _PAGE_BAP_UX so we
need a different permission mask based on the fault coming from a kernel
address or user space.

Without unique permission masks we see issues like the following with
modules:

Unable to handle kernel paging request for instruction fetch
Faulting instruction address: 0xf938d040
Oops: Kernel access of bad area, sig: 11 [#1]

Signed-off-by: Li Yang <leoli@freescale.com>
Signed-off-by: Jin Qing <b24347@freescale.com>
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
For 2.6.34 stable commit 78e2e68a2b79f394b7cd61e07987a8a89af907f7

 arch/powerpc/kernel/head_fsl_booke.S |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S
index 7255265..edd4a57 100644
--- a/arch/powerpc/kernel/head_fsl_booke.S
+++ b/arch/powerpc/kernel/head_fsl_booke.S
@@ -639,6 +639,13 @@ interrupt_base:
 	rlwinm	r12,r12,0,16,1
 	mtspr	SPRN_MAS1,r12
 
+	/* Make up the required permissions for kernel code */
+#ifdef CONFIG_PTE_64BIT
+	li	r13,_PAGE_PRESENT | _PAGE_BAP_SX
+	oris	r13,r13,_PAGE_ACCESSED@h
+#else
+	li	r13,_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_EXEC
+#endif
 	b	4f
 
 	/* Get the PGD for the current thread */
@@ -646,15 +653,15 @@ interrupt_base:
 	mfspr	r11,SPRN_SPRG_THREAD
 	lwz	r11,PGDIR(r11)
 
-4:
-	/* Make up the required permissions */
+	/* Make up the required permissions for user code */
 #ifdef CONFIG_PTE_64BIT
-	li	r13,_PAGE_PRESENT | _PAGE_EXEC
+	li	r13,_PAGE_PRESENT | _PAGE_BAP_UX
 	oris	r13,r13,_PAGE_ACCESSED@h
 #else
 	li	r13,_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_EXEC
 #endif
 
+4:
 	FIND_PTE
 	andc.	r13,r13,r11		/* Check permission */
 
-- 
1.6.0.6

[PATCH] powerpc/fsl-booke: Move loadcam_entry back to asm code to fix SMP ftrace

[Kumar Gala]

When we build with ftrace enabled its possible that loadcam_entry would
have used the stack pointer (even though the code doesn't need it).  We
call loadcam_entry in __secondary_start before the stack is setup.  To
ensure that loadcam_entry doesn't use the stack pointer the easiest
solution is to just have it in asm code.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
For 2.6.34 stable commit 78f622377f7d31d988db350a43c5689dd5f31876

 arch/powerpc/kernel/asm-offsets.c |    8 ++++++++
 arch/powerpc/mm/fsl_booke_mmu.c   |   25 +++----------------------
 arch/powerpc/mm/mmu_decl.h        |   10 +++++++++-
 arch/powerpc/mm/tlb_nohash_low.S  |   28 ++++++++++++++++++++++++++++
 4 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 957ceb7..0271b58 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -448,6 +448,14 @@ int main(void)
 	DEFINE(PGD_T_LOG2, PGD_T_LOG2);
 	DEFINE(PTE_T_LOG2, PTE_T_LOG2);
 #endif
+#ifdef CONFIG_FSL_BOOKE
+	DEFINE(TLBCAM_SIZE, sizeof(struct tlbcam));
+	DEFINE(TLBCAM_MAS0, offsetof(struct tlbcam, MAS0));
+	DEFINE(TLBCAM_MAS1, offsetof(struct tlbcam, MAS1));
+	DEFINE(TLBCAM_MAS2, offsetof(struct tlbcam, MAS2));
+	DEFINE(TLBCAM_MAS3, offsetof(struct tlbcam, MAS3));
+	DEFINE(TLBCAM_MAS7, offsetof(struct tlbcam, MAS7));
+#endif
 
 #ifdef CONFIG_KVM_EXIT_TIMING
 	DEFINE(VCPU_TIMING_EXIT_TBU, offsetof(struct kvm_vcpu,
diff --git a/arch/powerpc/mm/fsl_booke_mmu.c b/arch/powerpc/mm/fsl_booke_mmu.c
index 1ed6b52..cdc7526 100644
--- a/arch/powerpc/mm/fsl_booke_mmu.c
+++ b/arch/powerpc/mm/fsl_booke_mmu.c
@@ -2,7 +2,7 @@
  * Modifications by Kumar Gala (galak@kernel.crashing.org) to support
  * E500 Book E processors.
  *
- * Copyright 2004 Freescale Semiconductor, Inc
+ * Copyright 2004,2010 Freescale Semiconductor, Inc.
  *
  * This file contains the routines for initializing the MMU
  * on the 4xx series of chips.
@@ -56,19 +56,13 @@
 
 unsigned int tlbcam_index;
 
-#define NUM_TLBCAMS	(64)
 
 #if defined(CONFIG_LOWMEM_CAM_NUM_BOOL) && (CONFIG_LOWMEM_CAM_NUM >= NUM_TLBCAMS)
 #error "LOWMEM_CAM_NUM must be less than NUM_TLBCAMS"
 #endif
 
-struct tlbcam {
-	u32	MAS0;
-	u32	MAS1;
-	unsigned long	MAS2;
-	u32	MAS3;
-	u32	MAS7;
-} TLBCAM[NUM_TLBCAMS];
+#define NUM_TLBCAMS	(64)
+struct tlbcam TLBCAM[NUM_TLBCAMS];
 
 struct tlbcamrange {
 	unsigned long start;
@@ -109,19 +103,6 @@ unsigned long p_mapped_by_tlbcam(phys_addr_t pa)
 	return 0;
 }
 
-void loadcam_entry(int idx)
-{
-	mtspr(SPRN_MAS0, TLBCAM[idx].MAS0);
-	mtspr(SPRN_MAS1, TLBCAM[idx].MAS1);
-	mtspr(SPRN_MAS2, TLBCAM[idx].MAS2);
-	mtspr(SPRN_MAS3, TLBCAM[idx].MAS3);
-
-	if (mmu_has_feature(MMU_FTR_BIG_PHYS))
-		mtspr(SPRN_MAS7, TLBCAM[idx].MAS7);
-
-	asm volatile("isync;tlbwe;isync" : : : "memory");
-}
-
 /*
  * Set up one of the I/D BAT (block address translation) register pairs.
  * The parameters are not checked; in particular size must be a power
diff --git a/arch/powerpc/mm/mmu_decl.h b/arch/powerpc/mm/mmu_decl.h
index eb11d5d..63b84a0 100644
--- a/arch/powerpc/mm/mmu_decl.h
+++ b/arch/powerpc/mm/mmu_decl.h
@@ -144,7 +144,15 @@ extern unsigned long mmu_mapin_ram(unsigned long top);
 extern void MMU_init_hw(void);
 extern unsigned long mmu_mapin_ram(unsigned long top);
 extern void adjust_total_lowmem(void);
-
+extern void loadcam_entry(unsigned int index);
+
+struct tlbcam {
+	u32	MAS0;
+	u32	MAS1;
+	unsigned long	MAS2;
+	u32	MAS3;
+	u32	MAS7;
+};
 #elif defined(CONFIG_PPC32)
 /* anything 32-bit except 4xx or 8xx */
 extern void MMU_init_hw(void);
diff --git a/arch/powerpc/mm/tlb_nohash_low.S b/arch/powerpc/mm/tlb_nohash_low.S
index e925cb5..cfa7682 100644
--- a/arch/powerpc/mm/tlb_nohash_low.S
+++ b/arch/powerpc/mm/tlb_nohash_low.S
@@ -365,3 +365,31 @@ _GLOBAL(set_context)
 #else
 #error Unsupported processor type !
 #endif
+
+#if defined(CONFIG_FSL_BOOKE)
+/*
+ * extern void loadcam_entry(unsigned int index)
+ *
+ * Load TLBCAM[index] entry in to the L2 CAM MMU
+ */
+_GLOBAL(loadcam_entry)
+	LOAD_REG_ADDR(r4, TLBCAM)
+	mulli	r5,r3,TLBCAM_SIZE
+	add	r3,r5,r4
+	lwz	r4,TLBCAM_MAS0(r3)
+	mtspr	SPRN_MAS0,r4
+	lwz	r4,TLBCAM_MAS1(r3)
+	mtspr	SPRN_MAS1,r4
+	PPC_LL	r4,TLBCAM_MAS2(r3)
+	mtspr	SPRN_MAS2,r4
+	lwz	r4,TLBCAM_MAS3(r3)
+	mtspr	SPRN_MAS3,r4
+BEGIN_MMU_FTR_SECTION
+	lwz	r4,TLBCAM_MAS7(r3)
+	mtspr	SPRN_MAS7,r4
+END_MMU_FTR_SECTION_IFSET(MMU_FTR_BIG_PHYS)
+	isync
+	tlbwe
+	isync
+	blr
+#endif
-- 
1.6.0.6

Re: [PATCH] powerpc/fsl-booke: Move loadcam_entry back to asm code to fix SMP ftrace

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

On Mon, 2010-05-24 at 13:38 -0500, Kumar Gala wrote:
> When we build with ftrace enabled its possible that loadcam_entry would
> have used the stack pointer (even though the code doesn't need it).  We
> call loadcam_entry in __secondary_start before the stack is setup.  To
> ensure that loadcam_entry doesn't use the stack pointer the easiest
> solution is to just have it in asm code.

You could move it to a separate file and have that not built with the
ftrace options (-pg etc.) - but maybe that's not easier.

cheers


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] powerpc: Fix ioremap_flags() with book3e pte definition

[Kumar Gala]

On May 24, 2010, at 1:38 PM, Kumar Gala wrote:

> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>=20
> We can't just clear the user read permission in book3e pte, because
> that will also clear supervisor read permission.  This surely isn't
> desired.  Fix the problem by adding the supervisor read back.
>=20
> BenH: Slightly simplified the ifdef and applied to ppc64 too
>=20
> Signed-off-by: Li Yang <leoli@freescale.com>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> For 2.6.34 stable commit 55052eeca6d71d76f7c3f156c0501814d8e5e6d3
>=20
> arch/powerpc/mm/pgtable_32.c |    8 ++++++++
> arch/powerpc/mm/pgtable_64.c |    8 ++++++++
> 2 files changed, 16 insertions(+), 0 deletions(-)

Wondering what happened to this patch (and 2 other) getting into stable =
queue for .34.x

- k=

Re: [PATCH] powerpc: Fix ioremap_flags() with book3e pte definition

[Greg KH]

On Wed, Jun 02, 2010 at 04:59:16PM -0500, Kumar Gala wrote:
> 
> On May 24, 2010, at 1:38 PM, Kumar Gala wrote:
> 
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > 
> > We can't just clear the user read permission in book3e pte, because
> > that will also clear supervisor read permission.  This surely isn't
> > desired.  Fix the problem by adding the supervisor read back.
> > 
> > BenH: Slightly simplified the ifdef and applied to ppc64 too
> > 
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > For 2.6.34 stable commit 55052eeca6d71d76f7c3f156c0501814d8e5e6d3
> > 
> > arch/powerpc/mm/pgtable_32.c |    8 ++++++++
> > arch/powerpc/mm/pgtable_64.c |    8 ++++++++
> > 2 files changed, 16 insertions(+), 0 deletions(-)
> 
> Wondering what happened to this patch (and 2 other) getting into stable queue for .34.x

They are still in the "to-apply" queue.  Please be patient, I was busy
with the .35-rc1 merge window and now am catching up on the stable
trees.  There's over 130 patches that people seem to want in the first
.34 stable release, so it's taking some time to get them all in and
tested.

thanks,

greg k-h