[PATCH 0/6] 8xx TLB fixes.

[PATCH 0/6] 8xx TLB fixes.

[Joakim Tjernlund]

OK, here is the next try att fixing the MPC8xx MMU
problems. Pleas add one(or two) patches at a time and
test.

Expect some trivial merge conflicts in 8xx header file, sorry
about that.

Joakim Tjernlund (6):
  8xx: DTLB Error must check for more errors.
  8xx: get rid of _PAGE_HWWRITE dependency in MMU.
  8xx: invalidate non present TLBs
  8xx: Tag DAR with 0x00f0 to catch buggy instructions.
  8xx: Fixup DAR from buggy dcbX instructions.
  8xx: start using dcbX instructions in various copy routines

 arch/powerpc/include/asm/pte-8xx.h |   13 +-
 arch/powerpc/kernel/head_8xx.S     |  243 +++++++++++++++++++++++++++++-------
 arch/powerpc/kernel/misc_32.S      |   18 ---
 arch/powerpc/lib/copy_32.S         |   24 ----
 arch/powerpc/mm/fault.c            |    8 +-
 5 files changed, 209 insertions(+), 97 deletions(-)

[PATCH 1/6] 8xx: DTLB Error must check for more errors.

[Joakim Tjernlund]

DataTLBError currently does:
 if ((err & 0x02000000) == 0)
    DSI();
This won't handle a store with no valid translation.
Change this to
 if ((err & 0x48000000) != 0)
    DSI();
that is, branch to DSI if either !permission or
!translation.
---
 arch/powerpc/kernel/head_8xx.S |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index 52ff8c5..118bb05 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -472,8 +472,8 @@ DataTLBError:
 	/* First, make sure this was a store operation.
 	*/
 	mfspr	r10, SPRN_DSISR
-	andis.	r11, r10, 0x0200	/* If set, indicates store op */
-	beq	2f
+	andis.	r11, r10, 0x4800 /* no translation, no permission. */
+	bne	2f	/* branch if either is set */
 
 	/* The EA of a data TLB miss is automatically stored in the MD_EPN
 	 * register.  The EA of a data TLB error is automatically stored in
-- 
1.6.4.4

[PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU.

[Joakim Tjernlund]

Update the TLB asm to make proper use of _PAGE_DIRY and _PAGE_ACCESSED.
Pros:
 - I/D TLB Miss never needs to write to the linux pte.
 - _PAGE_ACCESSED is only set on TLB Error fixing accounting
 - _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly
    when a page has been made dirty.
 - Proper RO/RW mapping of user space.
 - Free up 2 SW TLB bits in the linux pte(add back _PAGE_WRITETHRU ?)
Cons:
 - 1 more instructions in I/D TLB Miss, but the since the linux pte is
   not written anymore, it should still be a big win.
---
 arch/powerpc/include/asm/pte-8xx.h |   13 +++---
 arch/powerpc/kernel/head_8xx.S     |   82 ++++++++++++++++--------------------
 2 files changed, 43 insertions(+), 52 deletions(-)

diff --git a/arch/powerpc/include/asm/pte-8xx.h b/arch/powerpc/include/asm/pte-8xx.h
index 8c6e312..f23cd15 100644
--- a/arch/powerpc/include/asm/pte-8xx.h
+++ b/arch/powerpc/include/asm/pte-8xx.h
@@ -32,22 +32,21 @@
 #define _PAGE_FILE	0x0002	/* when !present: nonlinear file mapping */
 #define _PAGE_NO_CACHE	0x0002	/* I: cache inhibit */
 #define _PAGE_SHARED	0x0004	/* No ASID (context) compare */
+#define _PAGE_DIRTY	0x0100	/* C: page changed */
 
-/* These five software bits must be masked out when the entry is loaded
- * into the TLB.
+/* These 3 software bits must be masked out when the entry is loaded
+ * into the TLB, 2 SW bits left.
  */
 #define _PAGE_EXEC	0x0008	/* software: i-cache coherency required */
 #define _PAGE_GUARDED	0x0010	/* software: guarded access */
-#define _PAGE_DIRTY	0x0020	/* software: page changed */
-#define _PAGE_RW	0x0040	/* software: user write access allowed */
-#define _PAGE_ACCESSED	0x0080	/* software: page referenced */
+#define _PAGE_ACCESSED	0x0020	/* software: page referenced */
 
 /* Setting any bits in the nibble with the follow two controls will
  * require a TLB exception handler change.  It is assumed unused bits
  * are always zero.
  */
-#define _PAGE_HWWRITE	0x0100	/* h/w write enable: never set in Linux PTE */
-#define _PAGE_USER	0x0800	/* One of the PP bits, the other is USER&~RW */
+#define _PAGE_RW	0x0400	/* lsb PP bits, inverted in HW */
+#define _PAGE_USER	0x0800	/* msb PP bits */
 
 #define _PMD_PRESENT	0x0001
 #define _PMD_BAD	0x0ff0
diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index 118bb05..3cf1289 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -333,26 +333,18 @@ InstructionTLBMiss:
 	mfspr	r11, SPRN_MD_TWC	/* ....and get the pte address */
 	lwz	r10, 0(r11)	/* Get the pte */
 
-#ifdef CONFIG_SWAP
-	/* do not set the _PAGE_ACCESSED bit of a non-present page */
-	andi.	r11, r10, _PAGE_PRESENT
-	beq	4f
-	ori	r10, r10, _PAGE_ACCESSED
-	mfspr	r11, SPRN_MD_TWC	/* get the pte address again */
-	stw	r10, 0(r11)
-4:
-#else
-	ori	r10, r10, _PAGE_ACCESSED
-	stw	r10, 0(r11)
-#endif
+	andi.	r11, r10, _PAGE_USER | _PAGE_ACCESSED
+	cmpwi	cr0, r11, _PAGE_USER | _PAGE_ACCESSED
+	bne-	cr0, 2f
+	/* Dont' bother with PP lsb, bit 21 for now */
 
 	/* The Linux PTE won't go exactly into the MMU TLB.
-	 * Software indicator bits 21, 22 and 28 must be clear.
+	 * Software indicator bits 22 and 28 must be clear.
 	 * Software indicator bits 24, 25, 26, and 27 must be
 	 * set.  All other Linux PTE bits control the behavior
 	 * of the MMU.
 	 */
-2:	li	r11, 0x00f0
+	li	r11, 0x00f0
 	rlwimi	r10, r11, 0, 24, 28	/* Set 24-27, clear 28 */
 	DO_8xx_CPU6(0x2d80, r3)
 	mtspr	SPRN_MI_RPN, r10	/* Update TLB entry */
@@ -365,6 +357,19 @@ InstructionTLBMiss:
 	lwz	r3, 8(r0)
 #endif
 	rfi
+2:
+	mfspr	r11, SRR1
+	rlwinm	r11, r11, 0, 5, 3 /* clear guarded */
+	mtspr	SRR1, r11
+
+	mfspr	r10, SPRN_M_TW	/* Restore registers */
+	lwz	r11, 0(r0)
+	mtcr	r11
+	lwz	r11, 4(r0)
+#ifdef CONFIG_8xx_CPU6
+	lwz	r3, 8(r0)
+#endif
+	b	InstructionAccess
 
 	. = 0x1200
 DataStoreTLBMiss:
@@ -409,21 +414,14 @@ DataStoreTLBMiss:
 	DO_8xx_CPU6(0x3b80, r3)
 	mtspr	SPRN_MD_TWC, r11
 
-#ifdef CONFIG_SWAP
-	/* do not set the _PAGE_ACCESSED bit of a non-present page */
-	andi.	r11, r10, _PAGE_PRESENT
-	beq	4f
-	ori	r10, r10, _PAGE_ACCESSED
-4:
-	/* and update pte in table */
-#else
-	ori	r10, r10, _PAGE_ACCESSED
-#endif
-	mfspr	r11, SPRN_MD_TWC	/* get the pte address again */
-	stw	r10, 0(r11)
+	andi.	r11, r10, _PAGE_ACCESSED
+	bne+	cr0, 5f	/* branch if access allowed */
+	rlwinm	r10, r10, 0, 21, 19 /* Clear _PAGE_USER */
+	ori	r10, r10, _PAGE_RW  /* Set RW bit for xor below to clear it */
+5:	xori	r10, r10, _PAGE_RW  /* invert RW bit */
 
 	/* The Linux PTE won't go exactly into the MMU TLB.
-	 * Software indicator bits 21, 22 and 28 must be clear.
+	 * Software indicator bits 22 and 28 must be clear.
 	 * Software indicator bits 24, 25, 26, and 27 must be
 	 * set.  All other Linux PTE bits control the behavior
 	 * of the MMU.
@@ -472,8 +470,8 @@ DataTLBError:
 	/* First, make sure this was a store operation.
 	*/
 	mfspr	r10, SPRN_DSISR
-	andis.	r11, r10, 0x4800 /* no translation, no permission. */
-	bne	2f	/* branch if either is set */
+	andis.	r11, r10, 0x4000 /* no translation */
+	bne	2f	/* branch if set */
 
 	/* The EA of a data TLB miss is automatically stored in the MD_EPN
 	 * register.  The EA of a data TLB error is automatically stored in
@@ -522,26 +520,20 @@ DataTLBError:
 	mfspr	r11, SPRN_MD_TWC		/* ....and get the pte address */
 	lwz	r10, 0(r11)		/* Get the pte */
 
-	andi.	r11, r10, _PAGE_RW	/* Is it writeable? */
-	beq	2f			/* Bail out if not */
-
-	/* Update 'changed', among others.
-	*/
-#ifdef CONFIG_SWAP
+	mfspr	r11, DSISR
+	andis.	r11, r11, 0x0200	/* store */
+	beq	5f
+	andi.	r11, r10, _PAGE_RW	/* writeable? */
+	beq	2f /* nope */
 	ori	r10, r10, _PAGE_DIRTY|_PAGE_HWWRITE
-	/* do not set the _PAGE_ACCESSED bit of a non-present page */
-	andi.	r11, r10, _PAGE_PRESENT
-	beq	4f
-	ori	r10, r10, _PAGE_ACCESSED
-4:
-#else
-	ori	r10, r10, _PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_HWWRITE
-#endif
-	mfspr	r11, SPRN_MD_TWC		/* Get pte address again */
+5:	ori	r10, r10, _PAGE_ACCESSED
+	mfspr	r11, MD_TWC		/* Get pte address again */
 	stw	r10, 0(r11)		/* and update pte in table */
 
+	xori	r10, r10, _PAGE_RW	/* RW bit is inverted */
+
 	/* The Linux PTE won't go exactly into the MMU TLB.
-	 * Software indicator bits 21, 22 and 28 must be clear.
+	 * Software indicator bits 22 and 28 must be clear.
 	 * Software indicator bits 24, 25, 26, and 27 must be
 	 * set.  All other Linux PTE bits control the behavior
 	 * of the MMU.
-- 
1.6.4.4

[PATCH 3/6] 8xx: invalidate non present TLBs

[Joakim Tjernlund]

8xx sometimes need to load a invalid/non-present TLBs in
it DTLB asm handler.
These must be invalidated separaly as linux mm don't.
---
 arch/powerpc/mm/fault.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 7699394..72941c7 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -39,7 +39,7 @@
 #include <asm/uaccess.h>
 #include <asm/tlbflush.h>
 #include <asm/siginfo.h>
-
+#include <mm/mmu_decl.h>
 
 #ifdef CONFIG_KPROBES
 static inline int notify_page_fault(struct pt_regs *regs)
@@ -243,6 +243,12 @@ good_area:
 		goto bad_area;
 #endif /* CONFIG_6xx */
 #if defined(CONFIG_8xx)
+	/* 8xx sometimes need to load a invalid/non-present TLBs.
+	 * These must be invalidated separately as linux mm don't.
+	 */
+	if (error_code & 0x40000000) /* no translation? */
+		_tlbil_va(address);
+
         /* The MPC8xx seems to always set 0x80000000, which is
          * "undefined".  Of those that can be set, this is the only
          * one which seems bad.
-- 
1.6.4.4

[PATCH 4/6] 8xx: Tag DAR with 0x00f0 to catch buggy instructions.

[Joakim Tjernlund]

dcbz, dcbf, dcbi, dcbst and icbi do not set DAR when they
cause a DTLB Error. Dectect this by tagging DAR with 0x00f0
at every exception exit that modifies DAR.
Test for DAR=0x00f0 in DataTLBError and bail
to handle_page_fault().
---
 arch/powerpc/kernel/head_8xx.S |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index 3cf1289..37aa7d0 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -206,6 +206,8 @@ MachineCheck:
 	EXCEPTION_PROLOG
 	mfspr r4,SPRN_DAR
 	stw r4,_DAR(r11)
+	li r5,0x00f0
+	mtspr SPRN_DAR,r5	/* Tag DAR, to be used in DTLB Error */
 	mfspr r5,SPRN_DSISR
 	stw r5,_DSISR(r11)
 	addi r3,r1,STACK_FRAME_OVERHEAD
@@ -222,6 +224,8 @@ DataAccess:
 	stw	r10,_DSISR(r11)
 	mr	r5,r10
 	mfspr	r4,SPRN_DAR
+	li	r10,0x00f0
+	mtspr	SPRN_DAR,r10	/* Tag DAR, to be used in DTLB Error */
 	EXC_XFER_EE_LITE(0x300, handle_page_fault)
 
 /* Instruction access exception.
@@ -244,6 +248,8 @@ Alignment:
 	EXCEPTION_PROLOG
 	mfspr	r4,SPRN_DAR
 	stw	r4,_DAR(r11)
+	li	r5,0x00f0
+	mtspr	SPRN_DAR,r5	/* Tag DAR, to be used in DTLB Error */
 	mfspr	r5,SPRN_DSISR
 	stw	r5,_DSISR(r11)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
@@ -427,6 +433,7 @@ DataStoreTLBMiss:
 	 * of the MMU.
 	 */
 2:	li	r11, 0x00f0
+	mtspr	SPRN_DAR,r11	/* Tag DAR */
 	rlwimi	r10, r11, 0, 24, 28	/* Set 24-27, clear 28 */
 	DO_8xx_CPU6(0x3d80, r3)
 	mtspr	SPRN_MD_RPN, r10	/* Update TLB entry */
@@ -467,10 +474,14 @@ DataTLBError:
 	stw	r10, 0(r0)
 	stw	r11, 4(r0)
 
+	mfspr	r10, SPRN_DAR
+	cmpwi	cr0, r10, 0x00f0
+	beq-	2f	/* must be a buggy dcbX, icbi insn. */
+
 	/* First, make sure this was a store operation.
 	*/
-	mfspr	r10, SPRN_DSISR
-	andis.	r11, r10, 0x4000 /* no translation */
+	mfspr	r11, SPRN_DSISR
+	andis.	r11, r11, 0x4000 /* no translation */
 	bne	2f	/* branch if set */
 
 	/* The EA of a data TLB miss is automatically stored in the MD_EPN
@@ -489,7 +500,8 @@ DataTLBError:
 	 * are initialized in mapin_ram().  This will avoid the problem,
 	 * assuming we only use the dcbi instruction on kernel addresses.
 	 */
-	mfspr	r10, SPRN_DAR
+
+	/* DAR is in r10 already */
 	rlwinm	r11, r10, 0, 0, 19
 	ori	r11, r11, MD_EVALID
 	mfspr	r10, SPRN_M_CASID
@@ -539,6 +551,7 @@ DataTLBError:
 	 * of the MMU.
 	 */
 	li	r11, 0x00f0
+	mtspr	SPRN_DAR,r11	/* Tag DAR */
 	rlwimi	r10, r11, 0, 24, 28	/* Set 24-27, clear 28 */
 	DO_8xx_CPU6(0x3d80, r3)
 	mtspr	SPRN_MD_RPN, r10	/* Update TLB entry */
-- 
1.6.4.4

[PATCH 5/6] 8xx: Fixup DAR from buggy dcbX instructions.

[Joakim Tjernlund]

This is an assembler version to fixup DAR not being set
by dcbX, icbi instructions. There are two versions, one
uses selfmodifing code(default), the other uses
jump table but is much bigger.
---
 arch/powerpc/kernel/head_8xx.S |  146 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 145 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index 37aa7d0..8c4c416 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -476,7 +476,8 @@ DataTLBError:
 
 	mfspr	r10, SPRN_DAR
 	cmpwi	cr0, r10, 0x00f0
-	beq-	2f	/* must be a buggy dcbX, icbi insn. */
+	beq-	FixDAR	/* must be a buggy dcbX, icbi insn. */
+DARFix:	/* Return from dcbx instruction bug workaround, r10 holds value of DAR */
 
 	/* First, make sure this was a store operation.
 	*/
@@ -593,6 +594,149 @@ DataTLBError:
 
 	. = 0x2000
 
+/* This is the procedure to calculate the data EA for buggy dcbx,dcbi instructions
+ * by decoding the registers used by the dcbx instruction and adding them.
+ * DAR is set to the calculated address and r10 also holds the EA on exit.
+ */
+//#define NO_SELF_MODIFYING_CODE /* define if you don't want to use self modifying code */
+	nop	/* A few nops to make the modified_instr: space below cache line aligned */
+	nop
+139:	/* fetch instruction from userspace memory */
+	DO_8xx_CPU6(0x3780, r3)
+	mtspr	SPRN_MD_EPN, r10
+	mfspr	r11, SPRN_M_TWB	/* Get level 1 table entry address */
+	lwz	r11, 0(r11)	/* Get the level 1 entry */
+	tophys  (r11, r11)
+	DO_8xx_CPU6(0x3b80, r3)
+	mtspr	SPRN_MD_TWC, r11	/* Load pte table base address */
+	mfspr	r11, SPRN_MD_TWC	/* ....and get the pte address */
+	lwz	r11, 0(r11)	/* Get the pte */
+	/* concat physical page address(r11) and page offset(r10) */
+	rlwimi	r11, r10, 0, 20, 31
+	b	140f
+FixDAR:	/* Entry point for dcbx workaround. */
+	/* fetch instruction from memory. */
+	mfspr	r10, SPRN_SRR0
+	andis.	r11, r10, 0x8000
+	tophys  (r11, r10)
+	beq-	139b		/* Branch if user space address */
+140:	lwz	r11,0(r11)
+#ifdef CONFIG_8xx_CPU6
+	lwz	r3, 8(r0)	/* restore r3 from memory */
+#endif
+#ifndef NO_SELF_MODIFYING_CODE
+	andis.	r10,r11,0x1f	/* test if reg RA is r0 */
+	li	r10,modified_instr@l
+	dcbtst	r0,r10		/* touch for store */
+	rlwinm	r11,r11,0,0,20	/* Zero lower 10 bits */
+	oris	r11,r11,640	/* Transform instr. to a "add r10,RA,RB" */
+	ori	r11,r11,532
+	stw	r11,0(r10)	/* store add/and instruction */
+	dcbf	0,r10		/* flush new instr. to memory. */
+	icbi	0,r10		/* invalidate instr. cache line */
+	lwz	r11, 4(r0)	/* restore r11 from memory */
+	mfspr	r10, SPRN_M_TW	/* restore r10 from M_TW */
+	isync			/* Wait until new instr is loaded from memory */
+modified_instr:
+	.space	4		/* this is where the add/and instr. is stored */
+	bne+	143f
+	subf	r10,r0,r10	/* r10=r10-r0, only if reg RA is r0 */
+143:	mtdar	r10		/* store faulting EA in DAR */
+	b	DARFix		/* Go back to normal TLB handling */
+#else
+	mfctr	r10
+	mtdar	r10			/* save ctr reg in DAR */
+	rlwinm	r10, r11, 24, 24, 28	/* offset into jump table for reg RB */
+	addi	r10, r10, 150f@l	/* add start of table */
+	mtctr	r10			/* load ctr with jump address */
+	xor	r10, r10, r10		/* sum starts at zero */
+	bctr				/* jump into table */
+150:
+	add	r10, r10, r0
+	b	151f
+	add	r10, r10, r1
+	b	151f
+	add	r10, r10, r2
+	b	151f
+	add	r10, r10, r3
+	b	151f
+	add	r10, r10, r4
+	b	151f
+	add	r10, r10, r5
+	b	151f
+	add	r10, r10, r6
+	b	151f
+	add	r10, r10, r7
+	b	151f
+	add	r10, r10, r8
+	b	151f
+	add	r10, r10, r9
+	b	151f
+	add	r10, r10, r10
+	b	151f
+	add	r10, r10, r11
+	b	151f
+	add	r10, r10, r12
+	b	151f
+	add	r10, r10, r13
+	b	151f
+	add	r10, r10, r14
+	b	151f
+	add	r10, r10, r15
+	b	151f
+	add	r10, r10, r16
+	b	151f
+	add	r10, r10, r17
+	b	151f
+	add	r10, r10, r18
+	b	151f
+	add	r10, r10, r19
+	b	151f
+	mtctr	r11	/* r10 needs special handling */
+	b	154f
+	mtctr	r11	/* r11 needs special handling */
+	b	153f
+	add	r10, r10, r22
+	b	151f
+	add	r10, r10, r23
+	b	151f
+	add	r10, r10, r24
+	b	151f
+	add	r10, r10, r25
+	b	151f
+	add	r10, r10, r25
+	b	151f
+	add	r10, r10, r27
+	b	151f
+	add	r10, r10, r28
+	b	151f
+	add	r10, r10, r29
+	b	151f
+	add	r10, r10, r30
+	b	151f
+	add	r10, r10, r31
+151:
+	rlwinm. r11,r11,19,24,28	/* offset into jump table for reg RA */
+	beq	152f			/* if reg RA is zero, don't add it */ 
+	addi	r11, r11, 150b@l	/* add start of table */
+	mtctr	r11			/* load ctr with jump address */
+	rlwinm	r11,r11,0,16,10		/* make sure we don't execute this more than once */
+	bctr				/* jump into table */
+152:
+	mfdar	r11
+	mtctr	r11			/* restore ctr reg from DAR */
+	mtdar	r10			/* save fault EA to DAR */
+	b	DARFix			/* Go back to normal TLB handling */
+
+	/* special handling for r10,r11 since these are modified already */
+153:	lwz	r11, 4(r0)	/* load r11 from memory */
+	b	155f
+154:	mfspr	r11, SPRN_M_TW	/* load r10 from M_TW */
+155:	add	r10, r10, r11	/* add it */
+	mfctr	r11		/* restore r11 */
+	b	151b
+#endif
+
 	.globl	giveup_fpu
 giveup_fpu:
 	blr
-- 
1.6.4.4

[PATCH 6/6] 8xx: start using dcbX instructions in various copy routines

[Joakim Tjernlund]

Now that 8xx can fixup dcbX instructions, start using them
where possible like every other PowerPc arch do.
---
 arch/powerpc/kernel/misc_32.S |   18 ------------------
 arch/powerpc/lib/copy_32.S    |   24 ------------------------
 2 files changed, 0 insertions(+), 42 deletions(-)

diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
index 15f28e0..b92095e 100644
--- a/arch/powerpc/kernel/misc_32.S
+++ b/arch/powerpc/kernel/misc_32.S
@@ -495,15 +495,7 @@ _GLOBAL(clear_pages)
 	li	r0,PAGE_SIZE/L1_CACHE_BYTES
 	slw	r0,r0,r4
 	mtctr	r0
-#ifdef CONFIG_8xx
-	li	r4, 0
-1:	stw	r4, 0(r3)
-	stw	r4, 4(r3)
-	stw	r4, 8(r3)
-	stw	r4, 12(r3)
-#else
 1:	dcbz	0,r3
-#endif
 	addi	r3,r3,L1_CACHE_BYTES
 	bdnz	1b
 	blr
@@ -528,15 +520,6 @@ _GLOBAL(copy_page)
 	addi	r3,r3,-4
 	addi	r4,r4,-4
 
-#ifdef CONFIG_8xx
-	/* don't use prefetch on 8xx */
-    	li	r0,4096/L1_CACHE_BYTES
-	mtctr	r0
-1:	COPY_16_BYTES
-	bdnz	1b
-	blr
-
-#else	/* not 8xx, we can prefetch */
 	li	r5,4
 
 #if MAX_COPY_PREFETCH > 1
@@ -577,7 +560,6 @@ _GLOBAL(copy_page)
 	li	r0,MAX_COPY_PREFETCH
 	li	r11,4
 	b	2b
-#endif	/* CONFIG_8xx */
 
 /*
  * void atomic_clear_mask(atomic_t mask, atomic_t *addr)
diff --git a/arch/powerpc/lib/copy_32.S b/arch/powerpc/lib/copy_32.S
index c657de5..74a7f41 100644
--- a/arch/powerpc/lib/copy_32.S
+++ b/arch/powerpc/lib/copy_32.S
@@ -98,20 +98,7 @@ _GLOBAL(cacheable_memzero)
 	bdnz	4b
 3:	mtctr	r9
 	li	r7,4
-#if !defined(CONFIG_8xx)
 10:	dcbz	r7,r6
-#else
-10:	stw	r4, 4(r6)
-	stw	r4, 8(r6)
-	stw	r4, 12(r6)
-	stw	r4, 16(r6)
-#if CACHE_LINE_SIZE >= 32
-	stw	r4, 20(r6)
-	stw	r4, 24(r6)
-	stw	r4, 28(r6)
-	stw	r4, 32(r6)
-#endif /* CACHE_LINE_SIZE */
-#endif
 	addi	r6,r6,CACHELINE_BYTES
 	bdnz	10b
 	clrlwi	r5,r8,32-LG_CACHELINE_BYTES
@@ -200,9 +187,7 @@ _GLOBAL(cacheable_memcpy)
 	mtctr	r0
 	beq	63f
 53:
-#if !defined(CONFIG_8xx)
 	dcbz	r11,r6
-#endif
 	COPY_16_BYTES
 #if L1_CACHE_BYTES >= 32
 	COPY_16_BYTES
@@ -356,14 +341,6 @@ _GLOBAL(__copy_tofrom_user)
 	li	r11,4
 	beq	63f
 
-#ifdef CONFIG_8xx
-	/* Don't use prefetch on 8xx */
-	mtctr	r0
-	li	r0,0
-53:	COPY_16_BYTES_WITHEX(0)
-	bdnz	53b
-
-#else /* not CONFIG_8xx */
 	/* Here we decide how far ahead to prefetch the source */
 	li	r3,4
 	cmpwi	r0,1
@@ -416,7 +393,6 @@ _GLOBAL(__copy_tofrom_user)
 	li	r3,4
 	li	r7,0
 	bne	114b
-#endif /* CONFIG_8xx */
 
 63:	srwi.	r0,r5,2
 	mtctr	r0
-- 
1.6.4.4

Re: [PATCH 4/6] 8xx: Tag DAR with 0x00f0 to catch buggy instructions.

[Benjamin Herrenschmidt]

On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
> dcbz, dcbf, dcbi, dcbst and icbi do not set DAR when they
> cause a DTLB Error. Dectect this by tagging DAR with 0x00f0
> at every exception exit that modifies DAR.
> Test for DAR=0x00f0 in DataTLBError and bail
> to handle_page_fault().

Why not -1 ? :-)

Ben.

> ---
>  arch/powerpc/kernel/head_8xx.S |   19 ++++++++++++++++---
>  1 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
> index 3cf1289..37aa7d0 100644
> --- a/arch/powerpc/kernel/head_8xx.S
> +++ b/arch/powerpc/kernel/head_8xx.S
> @@ -206,6 +206,8 @@ MachineCheck:
>  	EXCEPTION_PROLOG
>  	mfspr r4,SPRN_DAR
>  	stw r4,_DAR(r11)
> +	li r5,0x00f0
> +	mtspr SPRN_DAR,r5	/* Tag DAR, to be used in DTLB Error */
>  	mfspr r5,SPRN_DSISR
>  	stw r5,_DSISR(r11)
>  	addi r3,r1,STACK_FRAME_OVERHEAD
> @@ -222,6 +224,8 @@ DataAccess:
>  	stw	r10,_DSISR(r11)
>  	mr	r5,r10
>  	mfspr	r4,SPRN_DAR
> +	li	r10,0x00f0
> +	mtspr	SPRN_DAR,r10	/* Tag DAR, to be used in DTLB Error */
>  	EXC_XFER_EE_LITE(0x300, handle_page_fault)
>  
>  /* Instruction access exception.
> @@ -244,6 +248,8 @@ Alignment:
>  	EXCEPTION_PROLOG
>  	mfspr	r4,SPRN_DAR
>  	stw	r4,_DAR(r11)
> +	li	r5,0x00f0
> +	mtspr	SPRN_DAR,r5	/* Tag DAR, to be used in DTLB Error */
>  	mfspr	r5,SPRN_DSISR
>  	stw	r5,_DSISR(r11)
>  	addi	r3,r1,STACK_FRAME_OVERHEAD
> @@ -427,6 +433,7 @@ DataStoreTLBMiss:
>  	 * of the MMU.
>  	 */
>  2:	li	r11, 0x00f0
> +	mtspr	SPRN_DAR,r11	/* Tag DAR */
>  	rlwimi	r10, r11, 0, 24, 28	/* Set 24-27, clear 28 */
>  	DO_8xx_CPU6(0x3d80, r3)
>  	mtspr	SPRN_MD_RPN, r10	/* Update TLB entry */
> @@ -467,10 +474,14 @@ DataTLBError:
>  	stw	r10, 0(r0)
>  	stw	r11, 4(r0)
>  
> +	mfspr	r10, SPRN_DAR
> +	cmpwi	cr0, r10, 0x00f0
> +	beq-	2f	/* must be a buggy dcbX, icbi insn. */
> +
>  	/* First, make sure this was a store operation.
>  	*/
> -	mfspr	r10, SPRN_DSISR
> -	andis.	r11, r10, 0x4000 /* no translation */
> +	mfspr	r11, SPRN_DSISR
> +	andis.	r11, r11, 0x4000 /* no translation */
>  	bne	2f	/* branch if set */
>  
>  	/* The EA of a data TLB miss is automatically stored in the MD_EPN
> @@ -489,7 +500,8 @@ DataTLBError:
>  	 * are initialized in mapin_ram().  This will avoid the problem,
>  	 * assuming we only use the dcbi instruction on kernel addresses.
>  	 */
> -	mfspr	r10, SPRN_DAR
> +
> +	/* DAR is in r10 already */
>  	rlwinm	r11, r10, 0, 0, 19
>  	ori	r11, r11, MD_EVALID
>  	mfspr	r10, SPRN_M_CASID
> @@ -539,6 +551,7 @@ DataTLBError:
>  	 * of the MMU.
>  	 */
>  	li	r11, 0x00f0
> +	mtspr	SPRN_DAR,r11	/* Tag DAR */
>  	rlwimi	r10, r11, 0, 24, 28	/* Set 24-27, clear 28 */
>  	DO_8xx_CPU6(0x3d80, r3)
>  	mtspr	SPRN_MD_RPN, r10	/* Update TLB entry */

Re: [PATCH 4/6] 8xx: Tag DAR with 0x00f0 to catch buggy instructions.

[Joakim Tjernlund]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 07/10/2009 23:18:21:
>
> On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
> > dcbz, dcbf, dcbi, dcbst and icbi do not set DAR when they
> > cause a DTLB Error. Dectect this by tagging DAR with 0x00f0
> > at every exception exit that modifies DAR.
> > Test for DAR=0x00f0 in DataTLBError and bail
> > to handle_page_fault().
>
> Why not -1 ? :-)

Because 0x00f0 is already in use in the TLB fast path, saves one insn.

Re: [PATCH 4/6] 8xx: Tag DAR with 0x00f0 to catch buggy instructions.

[Benjamin Herrenschmidt]

On Thu, 2009-10-08 at 00:13 +0200, Joakim Tjernlund wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 07/10/2009 23:18:21:
> >
> > On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
> > > dcbz, dcbf, dcbi, dcbst and icbi do not set DAR when they
> > > cause a DTLB Error. Dectect this by tagging DAR with 0x00f0
> > > at every exception exit that modifies DAR.
> > > Test for DAR=0x00f0 in DataTLBError and bail
> > > to handle_page_fault().
> >
> > Why not -1 ? :-)
> 
> Because 0x00f0 is already in use in the TLB fast path, saves one insn.

But will cause weird things if the user really uses 0xf0 (though granted
that's unlikely :-)

Cheers,
Ben.

Re: [PATCH 4/6] 8xx: Tag DAR with 0x00f0 to catch buggy instructions.

[Joakim Tjernlund]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 08/10/2009 00:21:24:
>
> On Thu, 2009-10-08 at 00:13 +0200, Joakim Tjernlund wrote:
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 07/10/2009 23:18:21:
> > >
> > > On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
> > > > dcbz, dcbf, dcbi, dcbst and icbi do not set DAR when they
> > > > cause a DTLB Error. Dectect this by tagging DAR with 0x00f0
> > > > at every exception exit that modifies DAR.
> > > > Test for DAR=0x00f0 in DataTLBError and bail
> > > > to handle_page_fault().
> > >
> > > Why not -1 ? :-)
> >
> > Because 0x00f0 is already in use in the TLB fast path, saves one insn.
>
> But will cause weird things if the user really uses 0xf0 (though granted
> that's unlikely :-)

Yeah, you will most likely end up with a segv anyway.

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Benjamin Herrenschmidt]

On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
> 8xx sometimes need to load a invalid/non-present TLBs in
> it DTLB asm handler.
> These must be invalidated separaly as linux mm don't.

not sure about the dsisr test here, what is the point ?

Cheers,
Ben.

> ---
>  arch/powerpc/mm/fault.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 7699394..72941c7 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -39,7 +39,7 @@
>  #include <asm/uaccess.h>
>  #include <asm/tlbflush.h>
>  #include <asm/siginfo.h>
> -
> +#include <mm/mmu_decl.h>
>  
>  #ifdef CONFIG_KPROBES
>  static inline int notify_page_fault(struct pt_regs *regs)
> @@ -243,6 +243,12 @@ good_area:
>  		goto bad_area;
>  #endif /* CONFIG_6xx */
>  #if defined(CONFIG_8xx)
> +	/* 8xx sometimes need to load a invalid/non-present TLBs.
> +	 * These must be invalidated separately as linux mm don't.
> +	 */
> +	if (error_code & 0x40000000) /* no translation? */
> +		_tlbil_va(address);
> +
>          /* The MPC8xx seems to always set 0x80000000, which is
>           * "undefined".  Of those that can be set, this is the only
>           * one which seems bad.

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Joakim Tjernlund]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 07/10/2009 23:18:05:
>
> On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
> > 8xx sometimes need to load a invalid/non-present TLBs in
> > it DTLB asm handler.
> > These must be invalidated separaly as linux mm don't.
>
> not sure about the dsisr test here, what is the point ?

To remove the need to do the same in generic pte code. Then
we only need to do it when it counts. Lets see how it works out.

>
> Cheers,
> Ben.
>
> > ---
> >  arch/powerpc/mm/fault.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index 7699394..72941c7 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -39,7 +39,7 @@
> >  #include <asm/uaccess.h>
> >  #include <asm/tlbflush.h>
> >  #include <asm/siginfo.h>
> > -
> > +#include <mm/mmu_decl.h>
> >
> >  #ifdef CONFIG_KPROBES
> >  static inline int notify_page_fault(struct pt_regs *regs)
> > @@ -243,6 +243,12 @@ good_area:
> >        goto bad_area;
> >  #endif /* CONFIG_6xx */
> >  #if defined(CONFIG_8xx)
> > +   /* 8xx sometimes need to load a invalid/non-present TLBs.
> > +    * These must be invalidated separately as linux mm don't.
> > +    */
> > +   if (error_code & 0x40000000) /* no translation? */
> > +      _tlbil_va(address);
> > +
> >          /* The MPC8xx seems to always set 0x80000000, which is
> >           * "undefined".  Of those that can be set, this is the only
> >           * one which seems bad.
>
>
>
>

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Benjamin Herrenschmidt]

On Thu, 2009-10-08 at 00:12 +0200, Joakim Tjernlund wrote:
> > not sure about the dsisr test here, what is the point ?
> 
> To remove the need to do the same in generic pte code. Then
> we only need to do it when it counts. Lets see how it works out.

But I'm not sure I trust that DSISR test. Just do it unconditionally...
How much does it cost anyway ?

Ben.

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Joakim Tjernlund]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 07/10/2009 23:18:05:
>
> On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
> > 8xx sometimes need to load a invalid/non-present TLBs in
> > it DTLB asm handler.
> > These must be invalidated separaly as linux mm don't.
>
> not sure about the dsisr test here, what is the point ?

Without this patch I get about twice as many DTLB errors( on 2.4)

I have also noted that all my dcbst DTLB has the store bit set:
  trap:300 address:10030b8c, dar:10030b8c,err:42000000 dcbst

Thare are comments in the kernel that dcbst wrongly
generates TLB Errors with store set on 8xx. Is this really so?
Should dcbst always trap as a load?

 Jocke

>
> Cheers,
> Ben.
>
> > ---
> >  arch/powerpc/mm/fault.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index 7699394..72941c7 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -39,7 +39,7 @@
> >  #include <asm/uaccess.h>
> >  #include <asm/tlbflush.h>
> >  #include <asm/siginfo.h>
> > -
> > +#include <mm/mmu_decl.h>
> >
> >  #ifdef CONFIG_KPROBES
> >  static inline int notify_page_fault(struct pt_regs *regs)
> > @@ -243,6 +243,12 @@ good_area:
> >        goto bad_area;
> >  #endif /* CONFIG_6xx */
> >  #if defined(CONFIG_8xx)
> > +   /* 8xx sometimes need to load a invalid/non-present TLBs.
> > +    * These must be invalidated separately as linux mm don't.
> > +    */
> > +   if (error_code & 0x40000000) /* no translation? */
> > +      _tlbil_va(address);
> > +
> >          /* The MPC8xx seems to always set 0x80000000, which is
> >           * "undefined".  Of those that can be set, this is the only
> >           * one which seems bad.
>
>
>
>

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Dan Malek]

On Oct 8, 2009, at 12:22 PM, Joakim Tjernlund wrote:

> hare are comments in the kernel that dcbst wrongly
> generates TLB Errors with store set on 8xx. Is this really so?
> Should dcbst always trap as a load?

There are many comments written about 8xx as various
behavior was discovered.  Worse, some of these details
would be different among the different processor versions.
You need to be careful and test as many different part
versions as possible to ensure you have everything
covered.....  then someone will find a part that doesn't
quite work, "fix" it, and break others :-)

In this particular case, the PEM does state dcbst is treated
as a load, but from experience we know 8xx doesn't work
that way.  Of course, since dcbst is a store operation,
you could argue that 8xx got it correct :-)

Have fun!

	-- Dan

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Benjamin Herrenschmidt]

On Thu, 2009-10-08 at 13:11 -0700, Dan Malek wrote:
> 
> There are many comments written about 8xx as various
> behavior was discovered.  Worse, some of these details
> would be different among the different processor versions.
> You need to be careful and test as many different part
> versions as possible to ensure you have everything
> covered.....  then someone will find a part that doesn't
> quite work, "fix" it, and break others :-)
> 
> In this particular case, the PEM does state dcbst is treated
> as a load, but from experience we know 8xx doesn't work
> that way.  Of course, since dcbst is a store operation,
> you could argue that 8xx got it correct :-) 

Hehe. Well, it's architecturally incorrect, as dcbst is not really a
store operation in the sense that it doesn't modify the target cache
line, and as such doesn't (mustn't) be covered by write access
protection, shouldn't set DIRTY, etc...

So I would argue that 8xx got it wrong either way :-)

Cheers,
Ben.

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Benjamin Herrenschmidt]

Hoy Dan !

While you are around ... I have a question :-)

Do you happen to remember what the story is with the invalidation of
"unpopulated" (aka invalid) entries ?

IE. We create those in the 8xx TLB miss when the PTE is !present (or the
PMD is absent). Those then cause a TLB error on the next access which
allows us to process the page fault. But when/how are those invalid
entries supposed to be invalidated ?

The doco seems to hint that at least in the case of an entry with the
wrong C bit (store to an entry with C=0), the HW automatically
invalidates it before taking the TLB Error but that's all I found.

Is there a general HW policy on 8xx to invalidate TLB entries that cause
TLB errors ? Or is the kernel expected to do it most of the time ?

Cheers,
Ben.

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Dan Malek]

Hi Ben.

On Oct 8, 2009, at 1:28 PM, Benjamin Herrenschmidt wrote:

> While you are around ... I have a question :-)

I'll try.  Many brain cells have been replaced or lost
over the years :-)

> Do you happen to remember what the story is with the invalidation of
> "unpopulated" (aka invalid) entries ?
>
> IE. We create those in the 8xx TLB miss when the PTE is !present  
> (or the
> PMD is absent). Those then cause a TLB error on the next access which
> allows us to process the page fault. But when/how are those invalid
> entries supposed to be invalidated ?

I thought we did a tlbie() (or whatever the equivalent is today)
when the PTE was updated in the table.  An optimization was to
load the TLB with the entry at that time to avoid a subsequent miss.
In any case, the TLB entry has to be modified by the software.

> The doco seems to hint that at least in the case of an entry with the
> wrong C bit (store to an entry with C=0), the HW automatically
> invalidates it before taking the TLB Error but that's all I found.

I don't remember how C was used in the past, but I suspect
it just mirrored the Linux VM state.  I'm quite certain the MMU
HW would never change a TLB entry.  Where did you read this?
For most of the 8xx "early days," I used to just mark all write
pages as dirty.   For some reason I just overloaded the write/changed
into one bit, it avoided TLB Error overhead and I think even some
silicon bugs.  Since they were tiny and fairly static embedded
systems, it didn't have any effect on the way VM was managed.

> Is there a general HW policy on 8xx to invalidate TLB entries that  
> cause
> TLB errors ? Or is the kernel expected to do it most of the time ?

The MMU HW on the 8xx is just a translator.  I'm now really
certain it won't ever change a TLB entry.  It's completely up to
software to make all TLB changes.

Just make it simple :-)

Thanks.

	-- Dan

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Benjamin Herrenschmidt]

On Thu, 2009-10-08 at 15:08 -0700, Dan Malek wrote:
> Hi Ben.
> 
> On Oct 8, 2009, at 1:28 PM, Benjamin Herrenschmidt wrote:
> 
> > While you are around ... I have a question :-)
> 
> I'll try.  Many brain cells have been replaced or lost
> over the years :-)

Replaced ? You lucky ! I only lose mines :-)

> I thought we did a tlbie() (or whatever the equivalent is today)
> when the PTE was updated in the table.  An optimization was to
> load the TLB with the entry at that time to avoid a subsequent miss.
> In any case, the TLB entry has to be modified by the software.

Ok, that's my understanding too and I think we had the tlbie in
update_mmu_cache to do the trick, though the comment is misleading
making it think that the only reason it's there is for the dcbst
problem. At least that's my understanding. That was lost recently in 2.6
so I'll have to put it back properly.

I don't think we do the pre-load to avoid the second fault, but we
certainly could and should.

> I don't remember how C was used in the past, but I suspect
> it just mirrored the Linux VM state.  I'm quite certain the MMU
> HW would never change a TLB entry.  Where did you read this?

MPC855UM, chapter 8.6 "Memory attributes":

<<
• Reference and change bit updates—The MPC850 does not generate an exception for
  an R (reference) bit update. In fact, there is no entry for an R bit in the TLB.
  The change bit (C) is bit 23 in the level-two descriptor, described in Table 8-4.
  Software updates C (changed) bits, but hardware treats the C bit (negated) as a
  write-protect attribute. Therefore, attempting to write to a page marked unmodified
  invalidates that entry and causes an implementation-specific DTLB error exception.
  ^^^^^^^^^^^^^^^^^^^^^^
  If change bits are not needed, set the C bit to one by default in the PTEs.
>>

And yes, it's weird and that's the only place I think where this is
mentioned which makes me think it could well be a doco bug.

> For most of the 8xx "early days," I used to just mark all write
> pages as dirty.   For some reason I just overloaded the write/changed
> into one bit, it avoided TLB Error overhead and I think even some
> silicon bugs.  Since they were tiny and fairly static embedded
> systems, it didn't have any effect on the way VM was managed.

Well, nowadays at least, most of the time, a writeable page is also
dirty unless it's a writeable shared mapping, and in that later case
you really want to do proper dirty tracking. So I suspect we can
simplify some of that and let the generic code handle dirty by mapping
_PAGE_DIRTY to C and removing _PAGE_HWWRITE. We can also remove all
of the asm munging from DataTLBError, and let the generic C code fixup
DIRTY and ACCESSED when needed, since those should only rarely need a
fixup.

> The MMU HW on the 8xx is just a translator.  I'm now really
> certain it won't ever change a TLB entry.  It's completely up to
> software to make all TLB changes.

That makes sense.

> Just make it simple :-)

Yeah. I think we can simplify the current code a lot, which will speed
up TLB misses (well, nothing much you can do about the infamous errata
#6 but that's another story). It won't give 2.6 back the 2.4 speed due
to sheer bloat of the generic code but it might at least offset some of
the loss by improving the overall TLB miss performance.

Now, I don't have any 8xx gear, so it will be up to Joakim, Scott etc...
to get that stuff right :-)

Thanks for your feedback.

Cheers,
Ben.

> Thanks.
> 
> 	-- Dan

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Joakim Tjernlund]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 09/10/2009 00:23:48:
>
> On Thu, 2009-10-08 at 15:08 -0700, Dan Malek wrote:
> > Hi Ben.
> >
> > On Oct 8, 2009, at 1:28 PM, Benjamin Herrenschmidt wrote:
> >
> > > While you are around ... I have a question :-)
> >
> > I'll try.  Many brain cells have been replaced or lost
> > over the years :-)
>
> Replaced ? You lucky ! I only lose mines :-)
>
> > I thought we did a tlbie() (or whatever the equivalent is today)
> > when the PTE was updated in the table.  An optimization was to
> > load the TLB with the entry at that time to avoid a subsequent miss.
> > In any case, the TLB entry has to be modified by the software.
>
> Ok, that's my understanding too and I think we had the tlbie in
> update_mmu_cache to do the trick, though the comment is misleading
> making it think that the only reason it's there is for the dcbst
> problem. At least that's my understanding. That was lost recently in 2.6
> so I'll have to put it back properly.

So you don't think my invalidate "only !present pages" patch in do_page_fault
is enough?

>
> I don't think we do the pre-load to avoid the second fault, but we
> certainly could and should.
>
> > I don't remember how C was used in the past, but I suspect
> > it just mirrored the Linux VM state.  I'm quite certain the MMU
> > HW would never change a TLB entry.  Where did you read this?
>
> MPC855UM, chapter 8.6 "Memory attributes":
>
> <<
> • Reference and change bit updates—The MPC850 does not generate an exception for
>   an R (reference) bit update. In fact, there is no entry for an R bit in the TLB.
>   The change bit (C) is bit 23 in the level-two descriptor, described in Table 8-4.
>   Software updates C (changed) bits, but hardware treats the C bit (negated) as a
>   write-protect attribute. Therefore, attempting to write to a page marked unmodified
>   invalidates that entry and causes an implementation-specific DTLB error exception.
>   ^^^^^^^^^^^^^^^^^^^^^^
>   If change bits are not needed, set the C bit to one by default in the PTEs.
> >>
>
> And yes, it's weird and that's the only place I think where this is
> mentioned which makes me think it could well be a doco bug.
>
> > For most of the 8xx "early days," I used to just mark all write
> > pages as dirty.   For some reason I just overloaded the write/changed
> > into one bit, it avoided TLB Error overhead and I think even some
> > silicon bugs.  Since they were tiny and fairly static embedded
> > systems, it didn't have any effect on the way VM was managed.
>
> Well, nowadays at least, most of the time, a writeable page is also
> dirty unless it's a writeable shared mapping, and in that later case
> you really want to do proper dirty tracking. So I suspect we can
> simplify some of that and let the generic code handle dirty by mapping
> _PAGE_DIRTY to C and removing _PAGE_HWWRITE. We can also remove all
> of the asm munging from DataTLBError, and let the generic C code fixup
> DIRTY and ACCESSED when needed, since those should only rarely need a
> fixup.
>
> > The MMU HW on the 8xx is just a translator.  I'm now really
> > certain it won't ever change a TLB entry.  It's completely up to
> > software to make all TLB changes.
>
> That makes sense.
>
> > Just make it simple :-)
>
> Yeah. I think we can simplify the current code a lot, which will speed
> up TLB misses (well, nothing much you can do about the infamous errata
> #6 but that's another story). It won't give 2.6 back the 2.4 speed due
> to sheer bloat of the generic code but it might at least offset some of
> the loss by improving the overall TLB miss performance.

It won't get much faster than my current patch. Trapping all DTLB
Errors to C won't make it faster, only more correct should there be
a bug in the asm version. Actually there is one that has been there
all the time, guarded flag is not set by DTLB Error.

 Jocke

>
> Now, I don't have any 8xx gear, so it will be up to Joakim, Scott etc...
> to get that stuff right :-)

Waiting for Rex and Scott to comment/test.

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Benjamin Herrenschmidt]

On Fri, 2009-10-09 at 01:01 +0200, Joakim Tjernlund wrote:
> Ok, that's my understanding too and I think we had the tlbie in
> > update_mmu_cache to do the trick, though the comment is misleading
> > making it think that the only reason it's there is for the dcbst
> > problem. At least that's my understanding. That was lost recently in
> 2.6
> > so I'll have to put it back properly.
> 
> So you don't think my invalidate "only !present pages" patch in
> do_page_fault is enough?

It might well be the right solution, I was talking about the code as we
have upstream today.

> I don't think we do the pre-load to avoid the second fault, but we
> It won't get much faster than my current patch. Trapping all DTLB
> Errors to C won't make it faster, only more correct should there be
> a bug in the asm version. Actually there is one that has been there
> all the time, guarded flag is not set by DTLB Error.

There's other areas of improvements I suggested that can make it faster
such as avoiding the whole kernel/user test in the TLB misses.

Removing the stuff in DataTLBError can potentially make normal page
faults faster too by avoiding going through a bunch of useless code
before going to do the real thing in C :-)

As I said, the case of ACCESSED or DIRTY updates are rare enough to not
warrant code in the main page fault hot path.

Cheers,
Ben.

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Dan Malek]

On Oct 8, 2009, at 3:23 PM, Benjamin Herrenschmidt wrote:

> <<
> =95 Reference and change bit updates=97The MPC850 does not generate an =
=20
> exception for
>   an R (reference) bit update. In fact, there is no entry for an R =20
> bit in the TLB.
>   The change bit (C) is bit 23 in the level-two descriptor, =20
> described in Table 8-4.
>   Software updates C (changed) bits, but hardware treats the C bit =20
> (negated) as a
>   write-protect attribute. Therefore, attempting to write to a page =20=

> marked unmodified
>   invalidates that entry and causes an implementation-specific DTLB =20=

> error exception.
>   ^^^^^^^^^^^^^^^^^^^^^^
>   If change bits are not needed, set the C bit to one by default in =20=

> the PTEs.

How interesting....

I've looked at many 8xx docs and they all have the same text
(probably cut/paste :-))  I'd place some debug code in the C functions
to print out a few of the TLB Entry for various errors to see if this =20=

really
happens, and for other errors, too.  I guess I never stumbled into
this because I always thought I had to do everything from software,
so just made sure I did.

	-- Dan

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Benjamin Herrenschmidt]

On Thu, 2009-10-08 at 17:36 -0700, Dan Malek wrote:
> On Oct 8, 2009, at 3:23 PM, Benjamin Herrenschmidt wrote:
> 
> > <<
> > • Reference and change bit updates—The MPC850 does not generate an  
> > exception for
> >   an R (reference) bit update. In fact, there is no entry for an R  
> > bit in the TLB.
> >   The change bit (C) is bit 23 in the level-two descriptor,  
> > described in Table 8-4.
> >   Software updates C (changed) bits, but hardware treats the C bit  
> > (negated) as a
> >   write-protect attribute. Therefore, attempting to write to a page  
> > marked unmodified
> >   invalidates that entry and causes an implementation-specific DTLB  
> > error exception.
> >   ^^^^^^^^^^^^^^^^^^^^^^
> >   If change bits are not needed, set the C bit to one by default in  
> > the PTEs.
> 
> How interesting....
> 
> I've looked at many 8xx docs and they all have the same text
> (probably cut/paste :-))  I'd place some debug code in the C functions
> to print out a few of the TLB Entry for various errors to see if this  
> really
> happens, and for other errors, too.  I guess I never stumbled into
> this because I always thought I had to do everything from software,
> so just made sure I did.

I'm not sure it's worth bothering :-) I'm happy to continue assuming we
need to tlbie and always make sure we do so.

Cheers,
Ben.

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Joakim Tjernlund]

Dan Malek <dan@embeddedalley.com> wrote on 08/10/2009 22:11:07:
>
>
> On Oct 8, 2009, at 12:22 PM, Joakim Tjernlund wrote:
>
> > hare are comments in the kernel that dcbst wrongly
> > generates TLB Errors with store set on 8xx. Is this really so?
> > Should dcbst always trap as a load?

Hi, been a long time since I heard from you :)

>
> There are many comments written about 8xx as various
> behavior was discovered.  Worse, some of these details
> would be different among the different processor versions.
> You need to be careful and test as many different part
> versions as possible to ensure you have everything
> covered.....  then someone will find a part that doesn't
> quite work, "fix" it, and break others :-)
>
> In this particular case, the PEM does state dcbst is treated
> as a load, but from experience we know 8xx doesn't work
> that way.  Of course, since dcbst is a store operation,
> you could argue that 8xx got it correct :-)

One could try clearing the store bit in the page fault handler, but then
that might cause a loop.
Not sure it has any practical meaning though.

Anyhow, you are welcome to have a look at the patches I have been tossing out.

 Jocke

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Benjamin Herrenschmidt]

> One could try clearing the store bit in the page fault handler, but then
> that might cause a loop.
> Not sure it has any practical meaning though.
> 
> Anyhow, you are welcome to have a look at the patches I have been tossing out.

The store bit in do_page_fault() is -very- important (and the only DSISR
bit that is as I wrote earlier). The generic code must know if a fault
is caused by a load or a store since the later is what will cause
copy_on_write to happen for example, or dirty to be set, etc..

Ben.

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Dan Malek]

On Oct 8, 2009, at 1:37 PM, Joakim Tjernlund wrote:

> Hi, been a long time since I heard from you :)

Yeah, hiding among other projects :-)

> Anyhow, you are welcome to have a look at the patches I have been  
> tossing out.

I've been looking, but since I'm not familiar with the current
VM implementation, there isn't much I can contribute.  If
I see something where I can be useful, I'll speak up.

Thanks.

	-- Dan

Re: [PATCH 3/6] 8xx: invalidate non present TLBs

[Benjamin Herrenschmidt]

On Thu, 2009-10-08 at 21:22 +0200, Joakim Tjernlund wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 07/10/2009 23:18:05:
> >
> > On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
> > > 8xx sometimes need to load a invalid/non-present TLBs in
> > > it DTLB asm handler.
> > > These must be invalidated separaly as linux mm don't.
> >
> > not sure about the dsisr test here, what is the point ?
> 
> Without this patch I get about twice as many DTLB errors( on 2.4)

Ok, so it is useful... I would have thought that invalidating a TLB
entry that just caused a fault mostly be a nop.. well, the tlbie on 8xx
ignores the ASID so maybe it's invalidating next door process entries
but even that doesn't sound right. The TLB is so tiny on these things...

Oh well, as I said, something else to look at more closely.

> I have also noted that all my dcbst DTLB has the store bit set:
>   trap:300 address:10030b8c, dar:10030b8c,err:42000000 dcbst
> 
> Thare are comments in the kernel that dcbst wrongly
> generates TLB Errors with store set on 8xx. Is this really so?
> Should dcbst always trap as a load?

Architecturally it should, that's a known 8xx core bug.

Cheers,
Ben.

>  Jocke
> 
> >
> > Cheers,
> > Ben.
> >
> > > ---
> > >  arch/powerpc/mm/fault.c |    8 +++++++-
> > >  1 files changed, 7 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > > index 7699394..72941c7 100644
> > > --- a/arch/powerpc/mm/fault.c
> > > +++ b/arch/powerpc/mm/fault.c
> > > @@ -39,7 +39,7 @@
> > >  #include <asm/uaccess.h>
> > >  #include <asm/tlbflush.h>
> > >  #include <asm/siginfo.h>
> > > -
> > > +#include <mm/mmu_decl.h>
> > >
> > >  #ifdef CONFIG_KPROBES
> > >  static inline int notify_page_fault(struct pt_regs *regs)
> > > @@ -243,6 +243,12 @@ good_area:
> > >        goto bad_area;
> > >  #endif /* CONFIG_6xx */
> > >  #if defined(CONFIG_8xx)
> > > +   /* 8xx sometimes need to load a invalid/non-present TLBs.
> > > +    * These must be invalidated separately as linux mm don't.
> > > +    */
> > > +   if (error_code & 0x40000000) /* no translation? */
> > > +      _tlbil_va(address);
> > > +
> > >          /* The MPC8xx seems to always set 0x80000000, which is
> > >           * "undefined".  Of those that can be set, this is the only
> > >           * one which seems bad.
> >
> >
> >
> >

Re: [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU.

[Benjamin Herrenschmidt]

On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:

> +	andi.	r11, r10, _PAGE_USER | _PAGE_ACCESSED
> +	cmpwi	cr0, r11, _PAGE_USER | _PAGE_ACCESSED
> +	bne-	cr0, 2f

Did you mean _PAGE_PRESENT | _PAGE_ACCESSED ?

> +2:
> +	mfspr	r11, SRR1
> +	rlwinm	r11, r11, 0, 5, 3 /* clear guarded */
> +	mtspr	SRR1, r11

What is the above for ?

> +	mfspr	r10, SPRN_M_TW	/* Restore registers */
> +	lwz	r11, 0(r0)
> +	mtcr	r11
> +	lwz	r11, 4(r0)
> +#ifdef CONFIG_8xx_CPU6
> +	lwz	r3, 8(r0)
> +#endif
> +	b	InstructionAccess
>  

 .../...

> +	andi.	r11, r10, _PAGE_ACCESSED
> +	bne+	cr0, 5f	/* branch if access allowed */
> +	rlwinm	r10, r10, 0, 21, 19 /* Clear _PAGE_USER */
> +	ori	r10, r10, _PAGE_RW  /* Set RW bit for xor below to clear it */
> +5:	xori	r10, r10, _PAGE_RW  /* invert RW bit */

Why are you clearing _PAGE_USER ? One needs to differenciate usr
from kernel pages or user will be access to write to kernel...

Why don't you do the trick I proposed in my email with loading a
constant that contains all the bit combinations and shifting it
by the amount defined by _PAGE_RW and _PAGE_USER used as a two
bits index ?
 
>  	/* The Linux PTE won't go exactly into the MMU TLB.
> -	 * Software indicator bits 21, 22 and 28 must be clear.
> +	 * Software indicator bits 22 and 28 must be clear.
>  	 * Software indicator bits 24, 25, 26, and 27 must be
>  	 * set.  All other Linux PTE bits control the behavior
>  	 * of the MMU.
> @@ -472,8 +470,8 @@ DataTLBError:
>  	/* First, make sure this was a store operation.
>  	*/
>  	mfspr	r10, SPRN_DSISR
> -	andis.	r11, r10, 0x4800 /* no translation, no permission. */
> -	bne	2f	/* branch if either is set */
> +	andis.	r11, r10, 0x4000 /* no translation */
> +	bne	2f	/* branch if set */
>  
>  	/* The EA of a data TLB miss is automatically stored in the MD_EPN
>  	 * register.  The EA of a data TLB error is automatically stored in
> @@ -522,26 +520,20 @@ DataTLBError:
>  	mfspr	r11, SPRN_MD_TWC		/* ....and get the pte address */
>  	lwz	r10, 0(r11)		/* Get the pte */
>  
> -	andi.	r11, r10, _PAGE_RW	/* Is it writeable? */
> -	beq	2f			/* Bail out if not */
> -
> -	/* Update 'changed', among others.
> -	*/
> -#ifdef CONFIG_SWAP
> +	mfspr	r11, DSISR
> +	andis.	r11, r11, 0x0200	/* store */
> +	beq	5f
> +	andi.	r11, r10, _PAGE_RW	/* writeable? */
> +	beq	2f /* nope */
>  	ori	r10, r10, _PAGE_DIRTY|_PAGE_HWWRITE
> -	/* do not set the _PAGE_ACCESSED bit of a non-present page */
> -	andi.	r11, r10, _PAGE_PRESENT
> -	beq	4f
> -	ori	r10, r10, _PAGE_ACCESSED
> -4:
> -#else
> -	ori	r10, r10, _PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_HWWRITE
> -#endif
> -	mfspr	r11, SPRN_MD_TWC		/* Get pte address again */
> +5:	ori	r10, r10, _PAGE_ACCESSED
> +	mfspr	r11, MD_TWC		/* Get pte address again */
>  	stw	r10, 0(r11)		/* and update pte in table */
>  
> +	xori	r10, r10, _PAGE_RW	/* RW bit is inverted */
> +
>  	/* The Linux PTE won't go exactly into the MMU TLB.
> -	 * Software indicator bits 21, 22 and 28 must be clear.
> +	 * Software indicator bits 22 and 28 must be clear.
>  	 * Software indicator bits 24, 25, 26, and 27 must be
>  	 * set.  All other Linux PTE bits control the behavior
>  	 * of the MMU.

I still don't find how any of the above is useful... why not just go
straight to C code ?

Cheers,
Ben.

Re: [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU.

[Joakim Tjernlund]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 07/10/2009 23:14:52:
>
> On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
>
> > +   andi.   r11, r10, _PAGE_USER | _PAGE_ACCESSED
> > +   cmpwi   cr0, r11, _PAGE_USER | _PAGE_ACCESSED
> > +   bne-   cr0, 2f
>
> Did you mean _PAGE_PRESENT | _PAGE_ACCESSED ?
>
> > +2:
> > +   mfspr   r11, SRR1
> > +   rlwinm   r11, r11, 0, 5, 3 /* clear guarded */
> > +   mtspr   SRR1, r11
>
> What is the above for ?

TLB Miss will set that bit unconditionally and that is
the same bit as protection error in TLB error.

>
> > +   mfspr   r10, SPRN_M_TW   /* Restore registers */
> > +   lwz   r11, 0(r0)
> > +   mtcr   r11
> > +   lwz   r11, 4(r0)
> > +#ifdef CONFIG_8xx_CPU6
> > +   lwz   r3, 8(r0)
> > +#endif
> > +   b   InstructionAccess
> >
>
>  .../...
>
> > +   andi.   r11, r10, _PAGE_ACCESSED
> > +   bne+   cr0, 5f   /* branch if access allowed */
> > +   rlwinm   r10, r10, 0, 21, 19 /* Clear _PAGE_USER */
> > +   ori   r10, r10, _PAGE_RW  /* Set RW bit for xor below to clear it */
> > +5:   xori   r10, r10, _PAGE_RW  /* invert RW bit */
>
> Why are you clearing _PAGE_USER ? One needs to differenciate usr
> from kernel pages or user will be access to write to kernel...

To force a TLB error so I can work out load or store. I just
came up with a better idea for that, need to test it though.

>
> Why don't you do the trick I proposed in my email with loading a
> constant that contains all the bit combinations and shifting it
> by the amount defined by _PAGE_RW and _PAGE_USER used as a two
> bits index ?

Lets start simple, shall we? :)
Anyhow, I looked some more at that and I don't the best thing is
to use shifts. All bits are correct if you invert RW and add an exception
for extended coding.

>
> >     /* The Linux PTE won't go exactly into the MMU TLB.
> > -    * Software indicator bits 21, 22 and 28 must be clear.
> > +    * Software indicator bits 22 and 28 must be clear.
> >      * Software indicator bits 24, 25, 26, and 27 must be
> >      * set.  All other Linux PTE bits control the behavior
> >      * of the MMU.
> > @@ -472,8 +470,8 @@ DataTLBError:
> >     /* First, make sure this was a store operation.
> >     */
> >     mfspr   r10, SPRN_DSISR
> > -   andis.   r11, r10, 0x4800 /* no translation, no permission. */
> > -   bne   2f   /* branch if either is set */
> > +   andis.   r11, r10, 0x4000 /* no translation */
> > +   bne   2f   /* branch if set */
> >
> >     /* The EA of a data TLB miss is automatically stored in the MD_EPN
> >      * register.  The EA of a data TLB error is automatically stored in
> > @@ -522,26 +520,20 @@ DataTLBError:
> >     mfspr   r11, SPRN_MD_TWC      /* ....and get the pte address */
> >     lwz   r10, 0(r11)      /* Get the pte */
> >
> > -   andi.   r11, r10, _PAGE_RW   /* Is it writeable? */
> > -   beq   2f         /* Bail out if not */
> > -
> > -   /* Update 'changed', among others.
> > -   */
> > -#ifdef CONFIG_SWAP
> > +   mfspr   r11, DSISR
> > +   andis.   r11, r11, 0x0200   /* store */
> > +   beq   5f
> > +   andi.   r11, r10, _PAGE_RW   /* writeable? */
> > +   beq   2f /* nope */
> >     ori   r10, r10, _PAGE_DIRTY|_PAGE_HWWRITE
> > -   /* do not set the _PAGE_ACCESSED bit of a non-present page */
> > -   andi.   r11, r10, _PAGE_PRESENT
> > -   beq   4f
> > -   ori   r10, r10, _PAGE_ACCESSED
> > -4:
> > -#else
> > -   ori   r10, r10, _PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_HWWRITE
> > -#endif
> > -   mfspr   r11, SPRN_MD_TWC      /* Get pte address again */
> > +5:   ori   r10, r10, _PAGE_ACCESSED
> > +   mfspr   r11, MD_TWC      /* Get pte address again */
> >     stw   r10, 0(r11)      /* and update pte in table */
> >
> > +   xori   r10, r10, _PAGE_RW   /* RW bit is inverted */
> > +
> >     /* The Linux PTE won't go exactly into the MMU TLB.
> > -    * Software indicator bits 21, 22 and 28 must be clear.
> > +    * Software indicator bits 22 and 28 must be clear.
> >      * Software indicator bits 24, 25, 26, and 27 must be
> >      * set.  All other Linux PTE bits control the behavior
> >      * of the MMU.
>
> I still don't find how any of the above is useful... why not just go
> straight to C code ?

Because if you go to C with a protection fault, you are in trouble.
So deal with it here. Now, I got another idea too that will make this go away
if it work out

Re: [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU.

[Benjamin Herrenschmidt]

On Thu, 2009-10-08 at 00:08 +0200, Joakim Tjernlund wrote:
> 
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 07/10/2009 23:14:52:
> >
> > On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
> >
> > > +   andi.   r11, r10, _PAGE_USER | _PAGE_ACCESSED
> > > +   cmpwi   cr0, r11, _PAGE_USER | _PAGE_ACCESSED
> > > +   bne-   cr0, 2f
> >
> > Did you mean _PAGE_PRESENT | _PAGE_ACCESSED ?
> >
> > > +2:
> > > +   mfspr   r11, SRR1
> > > +   rlwinm   r11, r11, 0, 5, 3 /* clear guarded */
> > > +   mtspr   SRR1, r11
> >
> > What is the above for ?
> 
> TLB Miss will set that bit unconditionally and that is
> the same bit as protection error in TLB error.

And ? Big deal :-) IE. Once you get to InstructionAccess, it doesn't
matter if that bit is set, does it ?

> Lets start simple, shall we? :)
> Anyhow, I looked some more at that and I don't the best thing is
> to use shifts. All bits are correct if you invert RW and add an exception
> for extended coding.

Right, as long as you avoid doing a conditional branch :-)

> Because if you go to C with a protection fault, you are in trouble.

Why ?

> So deal with it here. Now, I got another idea too that will make this go away
> if it work out

I don't understand your point about protection faults.

You should be able to go straight to C with -anything-, that's what I do
for all other platforms.

Ben.

Re: [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU.

[Joakim Tjernlund]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 08/10/2009 00:20:17:

>
> On Thu, 2009-10-08 at 00:08 +0200, Joakim Tjernlund wrote:
> >
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 07/10/2009 23:14:52:
> > >
> > > On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
> > >
> > > > +   andi.   r11, r10, _PAGE_USER | _PAGE_ACCESSED
> > > > +   cmpwi   cr0, r11, _PAGE_USER | _PAGE_ACCESSED
> > > > +   bne-   cr0, 2f
> > >
> > > Did you mean _PAGE_PRESENT | _PAGE_ACCESSED ?

YES! cut and paste error, will send a new much improved patch
with my new idea.

> > >
> > > > +2:
> > > > +   mfspr   r11, SRR1
> > > > +   rlwinm   r11, r11, 0, 5, 3 /* clear guarded */
> > > > +   mtspr   SRR1, r11
> > >
> > > What is the above for ?
> >
> > TLB Miss will set that bit unconditionally and that is
> > the same bit as protection error in TLB error.
>
> And ? Big deal :-) IE. Once you get to InstructionAccess, it doesn't
> matter if that bit is set, does it ?

Yes it does. If one adds HWEXEC it will fail, right?
Also this count as a read and you could easily end up
in the protection case(in 2.4 you do)

>
> > Lets start simple, shall we? :)
> > Anyhow, I looked some more at that and I don't the best thing is
> > to use shifts. All bits are correct if you invert RW and add an exception
> > for extended coding.
>
> Right, as long as you avoid doing a conditional branch :-)

hey, I think you have to show how then :) I am not
good at ppc shift, mask, rotate insn.

>
> > Because if you go to C with a protection fault, you are in trouble.
>
> Why ?

In 2.4 you end up in read protection fault an get a SEGV back :)

>
> > So deal with it here. Now, I got another idea too that will make this go away
> > if it work out
>
> I don't understand your point about protection faults.
>
> You should be able to go straight to C with -anything-, that's what I do
> for all other platforms.

Well, you don't force a tlb error like I do, however my new version
handles this better.

Now I only handle DIRTY and the rest in C. Figured it is
much faster and really simplie now, stay tuned.

Re: [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU.

[Benjamin Herrenschmidt]

> Yes it does. If one adds HWEXEC it will fail, right?

Why ? We can just filter out DSISR, we don't really care why it failed
as long as we know whether it was a store or not.

> Also this count as a read and you could easily end up
> in the protection case(in 2.4 you do)

I'm not sure what you mean by "the protection case" Again, the C code
shouldn't care.

> hey, I think you have to show how then :) I am not
> good at ppc shift, mask, rotate insn.

They are so fun ! :-0

> >
> > > Because if you go to C with a protection fault, you are in trouble.
> >
> > Why ?
> 
> In 2.4 you end up in read protection fault an get a SEGV back :)

We probably should ignore the DSISR bits then. So it just goes to
generic C code which then fixes up ACCESSED etc... and returns.

> Now I only handle DIRTY and the rest in C. Figured it is
> much faster and really simplie now, stay tuned.

You should not even have to handle DIRTY at all in asm. At least in 2.6.
I can't vouch for what 2.4 generic code does... You should really port
your board over :-)

Cheers,
Ben.

Re: [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU.

[Joakim Tjernlund]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 08/10/2009 02:04:56:
>
>
> > Yes it does. If one adds HWEXEC it will fail, right?
>
> Why ? We can just filter out DSISR, we don't really care why it failed
> as long as we know whether it was a store or not.
>
> > Also this count as a read and you could easily end up
> > in the protection case(in 2.4 you do)
>
> I'm not sure what you mean by "the protection case" Again, the C code
> shouldn't care.

it does, and it should. How else should you know if you try
to read a NA space?

See:
	/* Protection fault on exec go straight to failure on
		 * Hash based MMUs as they either don't support per-page
		 * execute permission, or if they do, it's handled already
		 * at the hash level. This test would probably have to
		 * be removed if we change the way this works to make hash
		 * processors use the same I/D cache coherency mechanism
		 * as embedded.
		 */
		if (error_code & DSISR_PROTFAULT)
			goto bad_area;

...
/* a read */
	} else {
		/* protection fault */
		if (error_code & 0x08000000)
			goto bad_area;

>
> > hey, I think you have to show how then :) I am not
> > good at ppc shift, mask, rotate insn.
>
> They are so fun ! :-0

Great that someone thinks so :)

>
> > >
> > > > Because if you go to C with a protection fault, you are in trouble.
> > >
> > > Why ?
> >
> > In 2.4 you end up in read protection fault an get a SEGV back :)
>
> We probably should ignore the DSISR bits then. So it just goes to
> generic C code which then fixes up ACCESSED etc... and returns.

Not really I think.

>
> > Now I only handle DIRTY and the rest in C. Figured it is
> > much faster and really simplie now, stay tuned.
>
> You should not even have to handle DIRTY at all in asm. At least in 2.6.
> I can't vouch for what 2.4 generic code does... You should really port
> your board over :-)

2.4 and 2.6 have the same handling in asm.
hmm, maybe I should just call C, but 8xx isn't a speed monster so every
cycle counts :)
It works if I trap to C for DIRTY too.
Before thinking on porting my old board, I want 2.4 to enjoy
the new TLB code too :)

  Jocke

Re: [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU.

[Benjamin Herrenschmidt]

On Thu, 2009-10-08 at 02:19 +0200, Joakim Tjernlund wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 08/10/2009 02:04:56:
> >
> >
> > > Yes it does. If one adds HWEXEC it will fail, right?
> >
> > Why ? We can just filter out DSISR, we don't really care why it failed
> > as long as we know whether it was a store or not.
> >
> > > Also this count as a read and you could easily end up
> > > in the protection case(in 2.4 you do)
> >
> > I'm not sure what you mean by "the protection case" Again, the C code
> > shouldn't care.
> 
> it does, and it should. How else should you know if you try
> to read a NA space?

Generic code should sort it out in handle_mm_fault() (or earlier if it
can't find a VMA at all).

The DSISR munging is really not necessary I believe.

> 2.4 and 2.6 have the same handling in asm.

Yeah but the C code, especially the generic part, is different.

> hmm, maybe I should just call C, but 8xx isn't a speed monster so every
> cycle counts :)

But that's a slow path anyways.

> It works if I trap to C for DIRTY too.
> Before thinking on porting my old board, I want 2.4 to enjoy
> the new TLB code too :)

Hehehe.

Cheers,
Ben.

Re: [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU.

[Joakim Tjernlund]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 08/10/2009 02:28:19:
>
> On Thu, 2009-10-08 at 02:19 +0200, Joakim Tjernlund wrote:
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 08/10/2009 02:04:56:
> > >
> > >
> > > > Yes it does. If one adds HWEXEC it will fail, right?
> > >
> > > Why ? We can just filter out DSISR, we don't really care why it failed
> > > as long as we know whether it was a store or not.
> > >
> > > > Also this count as a read and you could easily end up
> > > > in the protection case(in 2.4 you do)
> > >
> > > I'm not sure what you mean by "the protection case" Again, the C code
> > > shouldn't care.
> >
> > it does, and it should. How else should you know if you try
> > to read a NA space?
>
> Generic code should sort it out in handle_mm_fault() (or earlier if it
> can't find a VMA at all).

How can it? You need to know more that just read and write.

>
> The DSISR munging is really not necessary I believe.

Well, it is today and I don't see how you can remove it.

>
> > 2.4 and 2.6 have the same handling in asm.
>
> Yeah but the C code, especially the generic part, is different.

yes, but it seem to work the same.

>
> > hmm, maybe I should just call C, but 8xx isn't a speed monster so every
> > cycle counts :)
>
> But that's a slow path anyways.

How so? You take a TLB Error for the first write to
every page.

 Jocke

Re: [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU.

[Benjamin Herrenschmidt]

On Thu, 2009-10-08 at 08:45 +0200, Joakim Tjernlund wrote:

> > Generic code should sort it out in handle_mm_fault() (or earlier if it
> > can't find a VMA at all).
> 
> How can it? You need to know more that just read and write.

It does. It's going to look for the VMA, which will tell it what is
allowed or not. You'll notice 4xx/BookE doesn't use DSISR (except the
ESR bit we pass to separate loads from stores).

If the region has no access, the kernel will know it (no VMA for
example) and will trigger a SEGV.

Really, the DSISR stuff is not as necessary as you think it is :-) You
should be able to jump to C code straight from both TLB error
interrupts.

> >
> > But that's a slow path anyways.
> 
> How so? You take a TLB Error for the first write to
> every page.

Compared to the TLB miss that is :-) But my main point is that a TLB
error caused by a lack of DIRTY or ACCESSED will be rare.

Ben.

Re: [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU.

[Joakim Tjernlund]

Joakim Tjernlund/Transmode wrote on 08/10/2009 01:11:23:
>
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 08/10/2009 00:20:17:
> >
> > On Thu, 2009-10-08 at 00:08 +0200, Joakim Tjernlund wrote:
> > >
> > > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 07/10/2009 23:14:52:
> > > >
> > > > On Wed, 2009-10-07 at 22:46 +0200, Joakim Tjernlund wrote:
> > > >
> > > > > +   andi.   r11, r10, _PAGE_USER | _PAGE_ACCESSED
> > > > > +   cmpwi   cr0, r11, _PAGE_USER | _PAGE_ACCESSED
> > > > > +   bne-   cr0, 2f
> > > >
> > > > Did you mean _PAGE_PRESENT | _PAGE_ACCESSED ?
>
> YES! cut and paste error, will send a new much improved patch
> with my new idea.

So here it is(on top for now), what do you think?

diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index 8c4c416..fea9f5b 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -339,8 +339,8 @@ InstructionTLBMiss:
 	mfspr	r11, SPRN_MD_TWC	/* ....and get the pte address */
 	lwz	r10, 0(r11)	/* Get the pte */

-	andi.	r11, r10, _PAGE_USER | _PAGE_ACCESSED
-	cmpwi	cr0, r11, _PAGE_USER | _PAGE_ACCESSED
+	andi.	r21, r20, _PAGE_ACCESSED | _PAGE_PRESENT
+	cmpwi	cr0, r21, _PAGE_ACCESSED | _PAGE_PRESENT
 	bne-	cr0, 2f
 	/* Dont' bother with PP lsb, bit 21 for now */

@@ -365,7 +365,10 @@ InstructionTLBMiss:
 	rfi
 2:
 	mfspr	r11, SRR1
-	rlwinm	r11, r11, 0, 5, 3 /* clear guarded */
+	/* clear all error bits as TLB Miss
+	 * sets a few unconditionally
+	*/
+	rlwinm	r21, r21, 0, 0xffff
 	mtspr	SRR1, r11

 	mfspr	r10, SPRN_M_TW	/* Restore registers */
@@ -422,8 +425,8 @@ DataStoreTLBMiss:

 	andi.	r11, r10, _PAGE_ACCESSED
 	bne+	cr0, 5f	/* branch if access allowed */
-	rlwinm	r10, r10, 0, 21, 19 /* Clear _PAGE_USER */
-	ori	r10, r10, _PAGE_RW  /* Set RW bit for xor below to clear it */
+	/* Need to know if load/store -> force a TLB Error */
+	rlwinm	r20, r20, 0, 0, 30 /* Clear _PAGE_PRESENT */
 5:	xori	r10, r10, _PAGE_RW  /* invert RW bit */

 	/* The Linux PTE won't go exactly into the MMU TLB.
@@ -482,8 +485,11 @@ DARFix:	/* Return from dcbx instruction bug workaround, r10 holds value of DAR *
 	/* First, make sure this was a store operation.
 	*/
 	mfspr	r11, SPRN_DSISR
-	andis.	r11, r11, 0x4000 /* no translation */
-	bne	2f	/* branch if set */
+	andis.	r21, r21, 0x4800	/* !translation or protection */
+	bne	2f	/* branch if either is set */
+	/* Only Change bit left now, do it here as it is faster
+	 * than trapping to the C fault handler.
+	*/

 	/* The EA of a data TLB miss is automatically stored in the MD_EPN
 	 * register.  The EA of a data TLB error is automatically stored in
@@ -533,16 +539,8 @@ DARFix:	/* Return from dcbx instruction bug workaround, r10 holds value of DAR *
 	mfspr	r11, SPRN_MD_TWC		/* ....and get the pte address */
 	lwz	r10, 0(r11)		/* Get the pte */

-	mfspr	r11, DSISR
-	andis.	r11, r11, 0x0200	/* store */
-	beq	5f
-	andi.	r11, r10, _PAGE_RW	/* writeable? */
-	beq	2f /* nope */
-	ori	r10, r10, _PAGE_DIRTY|_PAGE_HWWRITE
-5:	ori	r10, r10, _PAGE_ACCESSED
-	mfspr	r11, MD_TWC		/* Get pte address again */
+	ori	r10, r10, _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_HWWRITE
 	stw	r10, 0(r11)		/* and update pte in table */
-
 	xori	r10, r10, _PAGE_RW	/* RW bit is inverted */

 	/* The Linux PTE won't go exactly into the MMU TLB.