* [PATCH 0/6] 8xx MMU fixes @ 2009-10-08 13:24 Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 1/6] 8xx: DTLB Error must check for more errors Joakim Tjernlund 2009-10-09 0:15 ` [PATCH 0/6] 8xx MMU fixes Rex Feany 0 siblings, 2 replies; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-08 13:24 UTC (permalink / raw) To: Benjamin Herrenschmidt, linuxppc-dev, Rex Feany, Scott Wood So here we go again. This time I am fairly confindent I got most things correct :) Also manged to use even less instructions in the TLB Miss handlers. Scott and Rex, forget previous versions and try this one out. Once this works we can discuss further enchantments. Joakim Tjernlund (6): 8xx: DTLB Error must check for more errors. 8xx: Update TLB asm so it behaves as linux mm expects. 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 | 252 +++++++++++++++++++++++++++++------- arch/powerpc/kernel/misc_32.S | 18 --- arch/powerpc/lib/copy_32.S | 24 ---- arch/powerpc/mm/fault.c | 8 +- 5 files changed, 217 insertions(+), 98 deletions(-) ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/6] 8xx: DTLB Error must check for more errors. 2009-10-08 13:24 [PATCH 0/6] 8xx MMU fixes Joakim Tjernlund @ 2009-10-08 13:24 ` Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects Joakim Tjernlund 2009-10-08 20:48 ` [PATCH 1/6] 8xx: DTLB Error must check for more errors Benjamin Herrenschmidt 2009-10-09 0:15 ` [PATCH 0/6] 8xx MMU fixes Rex Feany 1 sibling, 2 replies; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-08 13:24 UTC (permalink / raw) To: Benjamin Herrenschmidt, linuxppc-dev, Rex Feany, Scott Wood 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects. 2009-10-08 13:24 ` [PATCH 1/6] 8xx: DTLB Error must check for more errors Joakim Tjernlund @ 2009-10-08 13:24 ` Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 3/6] 8xx: invalidate non present TLBs Joakim Tjernlund 2009-10-08 21:04 ` [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects Benjamin Herrenschmidt 2009-10-08 20:48 ` [PATCH 1/6] 8xx: DTLB Error must check for more errors Benjamin Herrenschmidt 1 sibling, 2 replies; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-08 13:24 UTC (permalink / raw) To: Benjamin Herrenschmidt, linuxppc-dev, Rex Feany, Scott Wood Update the TLB asm to make proper use of _PAGE_DIRY and _PAGE_ACCESSED. Get rid of _PAGE_HWWRITE too. 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 ?) - Less instructions in I/D TLB Miss. - Prepared for HWEXEC support. - Prepared kernel RO/user NA support. Cons: - None ? --- arch/powerpc/include/asm/pte-8xx.h | 13 ++--- arch/powerpc/kernel/head_8xx.S | 93 ++++++++++++++++++------------------ 2 files changed, 52 insertions(+), 54 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..1639d16 100644 --- a/arch/powerpc/kernel/head_8xx.S +++ b/arch/powerpc/kernel/head_8xx.S @@ -333,26 +333,21 @@ 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 + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ + rlwimi. r10, r10, 27, 31, 31 + beq- cr0, 2f /* Can be removed, costs a ITLB Err */ +#if 0 /* Dont' bother with PP lsb, bit 21 for now */ + /* r10 = (r10 & ~0x0400) | ((r10 & _PAGE_EXEC) << 7) */ + rlwimi r10, r10, 7, 21, 21 /* Set _PAGE_EXEC << 7 */ +#endif /* 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 +360,22 @@ InstructionTLBMiss: lwz r3, 8(r0) #endif rfi +2: + mfspr r11, SRR1 + /* clear all error bits as TLB Miss + * sets a few unconditionally + */ + rlwinm r11, r11, 0, 0xffff + 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 +420,22 @@ 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 + /* Need to know if load/store -> force a TLB Error + * by copying ACCESSED to PRESENT. + */ + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ + rlwimi r10, r10, 27, 31, 31 + +#if 0 /* Not yet */ + /* Honour kernel RO, User NA */ + andi. r11, r10, _PAGE_USER | _PAGE_RW + bne- cr0, 5f + ori r10,r10, 0x200 /* Extended encoding, bit 22 */ #endif - mfspr r11, SPRN_MD_TWC /* get the pte address again */ - stw r10, 0(r11) +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. @@ -469,11 +481,12 @@ DataTLBError: stw r10, 0(r0) stw r11, 4(r0) - /* First, make sure this was a store operation. - */ - mfspr r10, SPRN_DSISR - andis. r11, r10, 0x4800 /* no translation, no permission. */ + mfspr r11, SPRN_DSISR + andis. r11, r11, 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 @@ -522,26 +535,12 @@ 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 - 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 */ + 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. - * 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 13:24 ` [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects Joakim Tjernlund @ 2009-10-08 13:24 ` Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 4/6] 8xx: Tag DAR with 0x00f0 to catch buggy instructions Joakim Tjernlund 2009-10-08 21:04 ` [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects Benjamin Herrenschmidt 1 sibling, 1 reply; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-08 13:24 UTC (permalink / raw) To: Benjamin Herrenschmidt, linuxppc-dev, Rex Feany, Scott Wood 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 4/6] 8xx: Tag DAR with 0x00f0 to catch buggy instructions. 2009-10-08 13:24 ` [PATCH 3/6] 8xx: invalidate non present TLBs Joakim Tjernlund @ 2009-10-08 13:24 ` Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 5/6] 8xx: Fixup DAR from buggy dcbX instructions Joakim Tjernlund 0 siblings, 1 reply; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-08 13:24 UTC (permalink / raw) To: Benjamin Herrenschmidt, linuxppc-dev, Rex Feany, Scott Wood 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 | 15 ++++++++++++++- 1 files changed, 14 insertions(+), 1 deletions(-) diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S index 1639d16..9707dc4 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 @@ -441,6 +447,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 */ @@ -481,6 +488,10 @@ 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. */ + mfspr r11, SPRN_DSISR andis. r11, r11, 0x4800 /* !translation or protection */ bne 2f /* branch if either is set */ @@ -504,7 +515,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 @@ -546,6 +558,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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 5/6] 8xx: Fixup DAR from buggy dcbX instructions. 2009-10-08 13:24 ` [PATCH 4/6] 8xx: Tag DAR with 0x00f0 to catch buggy instructions Joakim Tjernlund @ 2009-10-08 13:24 ` Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 6/6] 8xx: start using dcbX instructions in various copy routines Joakim Tjernlund 0 siblings, 1 reply; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-08 13:24 UTC (permalink / raw) To: Benjamin Herrenschmidt, linuxppc-dev, Rex Feany, Scott Wood This is an assembler version to fixup DAR not being set by dcbX, icbi instructions. There are two versions, one uses selfmodifing code, the other uses a jump table but is much bigger(default). --- 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 9707dc4..6541855 100644 --- a/arch/powerpc/kernel/head_8xx.S +++ b/arch/powerpc/kernel/head_8xx.S @@ -490,7 +490,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 */ mfspr r11, SPRN_DSISR andis. r11, r11, 0x4800 /* !translation or protection */ @@ -600,6 +601,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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 6/6] 8xx: start using dcbX instructions in various copy routines 2009-10-08 13:24 ` [PATCH 5/6] 8xx: Fixup DAR from buggy dcbX instructions Joakim Tjernlund @ 2009-10-08 13:24 ` Joakim Tjernlund 0 siblings, 0 replies; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-08 13:24 UTC (permalink / raw) To: Benjamin Herrenschmidt, linuxppc-dev, Rex Feany, Scott Wood 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects. 2009-10-08 13:24 ` [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 3/6] 8xx: invalidate non present TLBs Joakim Tjernlund @ 2009-10-08 21:04 ` Benjamin Herrenschmidt 2009-10-08 22:44 ` Joakim Tjernlund 1 sibling, 1 reply; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-08 21:04 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev, Rex Feany On Thu, 2009-10-08 at 15:24 +0200, Joakim Tjernlund wrote: > +#define _PAGE_RW 0x0400 /* lsb PP bits, inverted in HW */ > +#define _PAGE_USER 0x0800 /* msb PP bits */ > > + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ > + rlwimi. r10, r10, 27, 31, 31 > + beq- cr0, 2f /* Can be removed, costs a ITLB Err */ Did you mean + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>4) */ + rlwimi. r10, r10, 28, 30, 31 + beq- cr0, 2f /* Can be removed, costs a ITLB Err */ Which would still be incorrect. You want -both- to be set, and your code will move on if -any- is set. Might want to add a ~ of the whole thing maybe and invert the condition ? I find it easier to just do li rX, requested_bits, and then, andc. rscratch, rX, rPTE > +#if 0 /* Dont' bother with PP lsb, bit 21 for now */ > + /* r10 = (r10 & ~0x0400) | ((r10 & _PAGE_EXEC) << 7) */ > + rlwimi r10, r10, 7, 21, 21 /* Set _PAGE_EXEC << 7 */ > +#endif I don't get that one. Don't bother with _PAGE_EXEC, there's no control of execute permission that works on 8xx. It's #if 0 anyway. You still need to massage the PP bits into place. I don't see that happening. As it is, your PTE contains for bit 20 and 21, which translates to: PTE: Translates to PP bits: RW: 0 USER: 0 00 supervisor RW (ok) RW: 0 USER: 1 01 supervisor RW user RO (WRONG) RW: 1 USER: 0 10 supervisor RW user RW (WRONG) RW: 1 USER: 1 11 supervisor RO user RO (WRONG) I would suggest you do the stuff I suggested initially with RW and USER being an "index" into a pre-cooked immediate value with all the encodings which also gives you the extended encoding for supervisor RO for free. > + /* Need to know if load/store -> force a TLB Error > + * by copying ACCESSED to PRESENT. > + */ > + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ > + rlwimi r10, r10, 27, 31, 31 That doesn't sound right, just like ITLB... you need to AND those two bits in a way or another, or basically test that they are both set (or neither is set) > +#if 0 /* Not yet */ > + /* Honour kernel RO, User NA */ > + andi. r11, r10, _PAGE_USER | _PAGE_RW > + bne- cr0, 5f > + ori r10,r10, 0x200 /* Extended encoding, bit 22 */ > #endif > - mfspr r11, SPRN_MD_TWC /* get the pte address again */ > - stw r10, 0(r11) > +5: xori r10, r10, _PAGE_RW /* invert RW bit */ Well, you are missing that xori from the ITLB miss I think. Also, that changes the table above to: PTE: Translates to PP bits: RW: 0 USER: 0 10 supervisor RW user RW (WRONG) RW: 0 USER: 1 11 supervisor RO user RO (ok) RW: 1 USER: 0 00 supervisor RW (ok) RW: 1 USER: 1 01 supervisor RW user RO (WRONG) So it's still not right :-) Cheers, Ben. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects. 2009-10-08 21:04 ` [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects Benjamin Herrenschmidt @ 2009-10-08 22:44 ` Joakim Tjernlund 2009-10-09 0:53 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-08 22:44 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Scott Wood, linuxppc-dev, Rex Feany Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 08/10/2009 23:04:03: > > On Thu, 2009-10-08 at 15:24 +0200, Joakim Tjernlund wrote: > > > +#define _PAGE_RW 0x0400 /* lsb PP bits, inverted in HW */ > > +#define _PAGE_USER 0x0800 /* msb PP bits */ > > > > + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ > > + rlwimi. r10, r10, 27, 31, 31 > > + beq- cr0, 2f /* Can be removed, costs a ITLB Err */ > > Did you mean (counting bits) ... No, it should be >> 5 > > + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>4) */ > + rlwimi. r10, r10, 28, 30, 31 > + beq- cr0, 2f /* Can be removed, costs a ITLB Err */ > > Which would still be incorrect. You want -both- to be set, > and your code will move on if -any- is set. Might want to add > a ~ of the whole thing maybe and invert the condition ? I want: if (!accessed) present = 0; accessed == 1 and present = 0 is impossible, right? So basically just copy over accessed to present and linux mm set both when trapping to C. > > I find it easier to just do li rX, requested_bits, and then, andc. > rscratch, rX, rPTE > > > +#if 0 /* Dont' bother with PP lsb, bit 21 for now */ > > + /* r10 = (r10 & ~0x0400) | ((r10 & _PAGE_EXEC) << 7) */ > > + rlwimi r10, r10, 7, 21, 21 /* Set _PAGE_EXEC << 7 */ > > +#endif > > I don't get that one. Don't bother with _PAGE_EXEC, there's no > control of execute permission that works on 8xx. It's #if 0 anyway. What about the execute perms in Level 2 descriptor, page 247? > > You still need to massage the PP bits into place. I don't see that > happening. Not at the moment, later. > > As it is, your PTE contains for bit 20 and 21, which translates to: > > PTE: Translates to PP bits: > RW: 0 USER: 0 00 supervisor RW (ok) > RW: 0 USER: 1 01 supervisor RW user RO (WRONG) > RW: 1 USER: 0 10 supervisor RW user RW (WRONG) > RW: 1 USER: 1 11 supervisor RO user RO (WRONG) You got USER and RW swapped and the table is different for exec. > > I would suggest you do the stuff I suggested initially with RW and USER > being an "index" into a pre-cooked immediate value with all the encodings > which also gives you the extended encoding for supervisor RO for free. > > > + /* Need to know if load/store -> force a TLB Error > > + * by copying ACCESSED to PRESENT. > > + */ > > + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ > > + rlwimi r10, r10, 27, 31, 31 > > That doesn't sound right, just like ITLB... you need to AND those two > bits in a way or another, or basically test that they are both set > (or neither is set) Same here as for ITLB. > > > +#if 0 /* Not yet */ > > + /* Honour kernel RO, User NA */ > > + andi. r11, r10, _PAGE_USER | _PAGE_RW > > + bne- cr0, 5f > > + ori r10,r10, 0x200 /* Extended encoding, bit 22 */ > > #endif > > - mfspr r11, SPRN_MD_TWC /* get the pte address again */ > > - stw r10, 0(r11) > > +5: xori r10, r10, _PAGE_RW /* invert RW bit */ > > Well, you are missing that xori from the ITLB miss I think. Also, that Nope, no xori needed for exec perms > changes the table above to: > > PTE: Translates to PP bits: > RW: 0 USER: 0 10 supervisor RW user RW (WRONG) > RW: 0 USER: 1 11 supervisor RO user RO (ok) > RW: 1 USER: 0 00 supervisor RW (ok) > RW: 1 USER: 1 01 supervisor RW user RO (WRONG) > > So it's still not right :-) User and RW swapped here too, as I read the table. I don't think user space would boot if I got it wrong. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects. 2009-10-08 22:44 ` Joakim Tjernlund @ 2009-10-09 0:53 ` Benjamin Herrenschmidt 2009-10-09 6:16 ` Joakim Tjernlund 0 siblings, 1 reply; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-09 0:53 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev, Rex Feany On Fri, 2009-10-09 at 00:44 +0200, Joakim Tjernlund wrote: > accessed == 1 and present = 0 is impossible, right? > So basically just copy over accessed to present and > linux mm set both when trapping to C. No, when present = 0, then the rest of the PTE can contain unrelated things, you can't trust ACCESSED. > What about the execute perms in Level 2 descriptor, page 247? Not useful, not fine grained enough. > > You still need to massage the PP bits into place. I don't see that > > happening. > > Not at the moment, later. > > > > > As it is, your PTE contains for bit 20 and 21, which translates to: > > > > PTE: Translates to PP bits: > > RW: 0 USER: 0 00 supervisor RW (ok) > > RW: 0 USER: 1 01 supervisor RW user RO (WRONG) > > RW: 1 USER: 0 10 supervisor RW user RW (WRONG) > > RW: 1 USER: 1 11 supervisor RO user RO (WRONG) > > You got USER and RW swapped and the table is different > for exec. Hrm, let me see... yes. You are right, I mixed RW and USER. However, I don't think the PP bits change do they ? IE. Basically, Read == Exec at the page level. So the table isn't really different between I and D. However, indeed, since you don't have a unified TLB, the case can be made that we can ignore R vs. W in the iTLB case. In which case, you get for iTLB: PTE: Translates to PP bits: RW: 0 USER: 0 00 supervisor X only (ok) RW: 0 USER: 1 10 supervisor X user X (ok) RW: 1 USER: 0 01 supervisor X user X (WRONG) RW: 1 USER: 1 11 supervisor X user X (ok) So a page with _PAGE_RW and not _PAGE_USER would still be executable from user... oops :-) I think what you want for iTLB is just basically have a base of 00 and or-in _PAGE_USER only (ie, keep _PAGE_RW clear with a rlwinm) so that you basically get supervisor X only if _PAGE_USER is 0 and both X if _PAGE_USER is 1 For the dTLB, the table becomes (including your inversion of _PAGE_RW) PTE: Translates to PP bits: RW: 0 USER: 0 01 supervisor RW user RO (WRONG) RW: 0 USER: 1 11 supervisor RO user RO (ok) RW: 1 USER: 0 00 supervisor RW only (ok) RW: 1 USER: 1 10 supervisor RW user RW (ok) So it's -almost- right :-) You still got the RW:0 USER:0 case wrong, ie a read-only kernel page would be user readable. You can work around that by never setting kernel pages read-only (which we do mostly), but in the grand scheme of things, my trick I proposed initially would sort it out all including support for kernel RO :-) In any case, the above, while wrong, wouldn't cause crashes or issues for well behaved userspace so it's a step forward. > Same here as for ITLB. And still not right :-) ie. you cannot rely on the value of _PAGE_ACCESSED if _PAGE_PRESENT is not set. > Nope, no xori needed for exec perms Right, thanks to having a split TLB, but you still need to mask out one of the bits afaik. > I don't think user space would boot if I got it wrong. True. I think it's more correct than I initially thought but still subtely wrong :-) Cheers, Ben. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects. 2009-10-09 0:53 ` Benjamin Herrenschmidt @ 2009-10-09 6:16 ` Joakim Tjernlund 2009-10-09 6:30 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-09 6:16 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Scott Wood, linuxppc-dev, Rex Feany Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 09/10/2009 02:53:31: > > Subject: > > Re: [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects. > > On Fri, 2009-10-09 at 00:44 +0200, Joakim Tjernlund wrote: > > > accessed == 1 and present = 0 is impossible, right? > > So basically just copy over accessed to present and > > linux mm set both when trapping to C. > > No, when present = 0, then the rest of the PTE can contain unrelated > things, you can't trust ACCESSED. Ah, OK. > > > What about the execute perms in Level 2 descriptor, page 247? > > Not useful, not fine grained enough. > > > > You still need to massage the PP bits into place. I don't see that > > > happening. > > > > Not at the moment, later. > > > > > > > > As it is, your PTE contains for bit 20 and 21, which translates to: > > > > > > PTE: Translates to PP bits: > > > RW: 0 USER: 0 00 supervisor RW (ok) > > > RW: 0 USER: 1 01 supervisor RW user RO (WRONG) > > > RW: 1 USER: 0 10 supervisor RW user RW (WRONG) > > > RW: 1 USER: 1 11 supervisor RO user RO (WRONG) > > > > You got USER and RW swapped and the table is different > > for exec. > > Hrm, let me see... yes. You are right, I mixed RW and USER. However, > I don't think the PP bits change do they ? IE. Basically, Read == Exec > at the page level. So the table isn't really different between I and D. > > However, indeed, since you don't have a unified TLB, the case can be > made that we can ignore R vs. W in the iTLB case. In which case, you get > for iTLB: > > > PTE: Translates to PP bits: > RW: 0 USER: 0 00 supervisor X only (ok) > RW: 0 USER: 1 10 supervisor X user X (ok) > RW: 1 USER: 0 01 supervisor X user X (WRONG) > RW: 1 USER: 1 11 supervisor X user X (ok) > > So a page with _PAGE_RW and not _PAGE_USER would still be executable > from user... oops :-) Yes, it is not perfect, but should work for now. > > I think what you want for iTLB is just basically have a base of 00 > and or-in _PAGE_USER only (ie, keep _PAGE_RW clear with a rlwinm) > so that you basically get supervisor X only if _PAGE_USER is 0 and > both X if _PAGE_USER is 1 > > For the dTLB, the table becomes (including your inversion of _PAGE_RW) > > PTE: Translates to PP bits: > RW: 0 USER: 0 01 supervisor RW user RO (WRONG) > RW: 0 USER: 1 11 supervisor RO user RO (ok) > RW: 1 USER: 0 00 supervisor RW only (ok) > RW: 1 USER: 1 10 supervisor RW user RW (ok) > > So it's -almost- right :-) You still got the RW:0 USER:0 case wrong, > ie a read-only kernel page would be user readable. Which will be fixed once I activate: #if 0 /* Not yet */ /* Honour kernel RO, User NA */ andi. r11, r10, _PAGE_USER | _PAGE_RW bne- cr0, 5f ori r10,r10, 0x200 /* Extended encoding, bit 22 */ #endif > > You can work around that by never setting kernel pages read-only (which > we do mostly), but in the grand scheme of things, my trick I proposed > initially would sort it out all including support for kernel RO :-) Not convinced that your trick will be a win. The other bits will need to move around too. Maybe I misunderstand something? ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects. 2009-10-09 6:16 ` Joakim Tjernlund @ 2009-10-09 6:30 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-09 6:30 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev, Rex Feany On Fri, 2009-10-09 at 08:16 +0200, Joakim Tjernlund wrote: > Which will be fixed once I activate: > #if 0 /* Not yet */ > /* Honour kernel RO, User NA */ > andi. r11, r10, _PAGE_USER | _PAGE_RW > bne- cr0, 5f > ori r10,r10, 0x200 /* Extended encoding, bit 22 */ > #endif Which will be more code including a conditional than my proposed trick :-) I'll try to write the asm for you later. > Not convinced that your trick will be a win. The other > bits will need to move around too. Maybe I misunderstand > something? I'll write some code to show you what I have in mind later. Cheers, Ben. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/6] 8xx: DTLB Error must check for more errors. 2009-10-08 13:24 ` [PATCH 1/6] 8xx: DTLB Error must check for more errors Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects Joakim Tjernlund @ 2009-10-08 20:48 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-08 20:48 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev, Rex Feany On Thu, 2009-10-08 at 15:24 +0200, Joakim Tjernlund wrote: > 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. > --- As I said earlier, I don't think this is necessary, just get rid of the whole bunch of code in DataTLBError :-) Ben. > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/6] 8xx MMU fixes 2009-10-08 13:24 [PATCH 0/6] 8xx MMU fixes Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 1/6] 8xx: DTLB Error must check for more errors Joakim Tjernlund @ 2009-10-09 0:15 ` Rex Feany 2009-10-09 6:00 ` Joakim Tjernlund 1 sibling, 1 reply; 36+ messages in thread From: Rex Feany @ 2009-10-09 0:15 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev Thus spake Joakim Tjernlund (Joakim.Tjernlund@transmode.se): > So here we go again. This time I am > fairly confindent I got most things correct :) > Also manged to use even less instructions in the > TLB Miss handlers. > > Scott and Rex, forget previous versions and > try this one out. This patch set is better, userspace seems more stable but I am still seeing some odd problems. This is from straceing mount, which displays nothing even though /proc/mounts shows everything mounted properly: open("/proc/mounts", O_RDONLY) = 3 fstat64(0x3, 0x7fe7e2a8) = 0 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x3001f000 read(3, 0x3001f000, 1024) = -1 EFAULT (Bad address) exit_group(0) = ? it looks like the memory allocated using mmap is bad? take care! /rex. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/6] 8xx MMU fixes 2009-10-09 0:15 ` [PATCH 0/6] 8xx MMU fixes Rex Feany @ 2009-10-09 6:00 ` Joakim Tjernlund 2009-10-09 6:46 ` Rex Feany 0 siblings, 1 reply; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-09 6:00 UTC (permalink / raw) To: Rex Feany; +Cc: Scott Wood, linuxppc-dev Rex Feany <RFeany@mrv.com> wrote on 09/10/2009 02:15:27: > > Thus spake Joakim Tjernlund (Joakim.Tjernlund@transmode.se): > > > So here we go again. This time I am > > fairly confindent I got most things correct :) > > Also manged to use even less instructions in the > > TLB Miss handlers. > > > > Scott and Rex, forget previous versions and > > try this one out. > > This patch set is better, userspace seems more stable but I am still > seeing some odd problems. This is from straceing mount, which displays > nothing even though /proc/mounts shows everything mounted properly: > > open("/proc/mounts", O_RDONLY) = 3 > fstat64(0x3, 0x7fe7e2a8) = 0 > mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x3001f000 > read(3, 0x3001f000, 1024) = -1 EFAULT (Bad address) > exit_group(0) = ? > > it looks like the memory allocated using mmap is bad? Try making the tlbil_va in fault.c unconditional, just to make sure there isn't any old TLBs around. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/6] 8xx MMU fixes 2009-10-09 6:00 ` Joakim Tjernlund @ 2009-10-09 6:46 ` Rex Feany 2009-10-09 11:04 ` Joakim Tjernlund 2009-10-09 12:30 ` Joakim Tjernlund 0 siblings, 2 replies; 36+ messages in thread From: Rex Feany @ 2009-10-09 6:46 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev Thus spake Joakim Tjernlund (joakim.tjernlund@transmode.se): > Rex Feany <RFeany@mrv.com> wrote on 09/10/2009 02:15:27: > > open("/proc/mounts", O_RDONLY) = 3 > > fstat64(0x3, 0x7fe7e2a8) = 0 > > mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x3001f000 > > read(3, 0x3001f000, 1024) = -1 EFAULT (Bad address) > > exit_group(0) = ? > > Try making the tlbil_va in fault.c unconditional, just to make sure > there isn't any old TLBs around. didn't make a difference ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/6] 8xx MMU fixes 2009-10-09 6:46 ` Rex Feany @ 2009-10-09 11:04 ` Joakim Tjernlund 2009-10-09 12:30 ` Joakim Tjernlund 1 sibling, 0 replies; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-09 11:04 UTC (permalink / raw) To: Rex Feany; +Cc: Scott Wood, linuxppc-dev Rex Feany <RFeany@mrv.com> wrote on 09/10/2009 08:46:49: > > Thus spake Joakim Tjernlund (joakim.tjernlund@transmode.se): > > > Rex Feany <RFeany@mrv.com> wrote on 09/10/2009 02:15:27: > > > > open("/proc/mounts", O_RDONLY) = 3 > > > fstat64(0x3, 0x7fe7e2a8) = 0 > > > mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =0x3001f000 > > > read(3, 0x3001f000, 1024) = -1 EFAULT (Bad address) > > > exit_group(0) = ? > > > > Try making the tlbil_va in fault.c unconditional, just to make sure > > there isn't any old TLBs around. > > didn't make a difference OK, so how about: diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S index 6541855..f4b5dca 100644 --- a/arch/powerpc/kernel/head_8xx.S +++ b/arch/powerpc/kernel/head_8xx.S @@ -339,9 +339,9 @@ InstructionTLBMiss: mfspr r11, SPRN_MD_TWC /* ....and get the pte address */ lwz r10, 0(r11) /* Get the pte */ - /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ - rlwimi. r10, r10, 27, 31, 31 - beq- cr0, 2f /* Can be removed, costs a ITLB Err */ + andi. r11, r10, _PAGE_ACCESSED | _PAGE_PRESENT + cmpwi cr0, r11, _PAGE_ACCESSED | _PAGE_PRESENT + bne- cr0, 2f #if 0 /* Dont' bother with PP lsb, bit 21 for now */ /* r10 = (r10 & ~0x0400) | ((r10 & _PAGE_EXEC) << 7) */ @@ -429,9 +429,11 @@ DataStoreTLBMiss: /* Need to know if load/store -> force a TLB Error * by copying ACCESSED to PRESENT. */ - /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ - rlwimi r10, r10, 27, 31, 31 - + andi. r11, r10, _PAGE_ACCESSED | _PAGE_PRESENT + cmpwi cr0, r11, _PAGE_ACCESSED | _PAGE_PRESENT + beq+ cr0, 6f + rlwinm r10, r10, 0, 0, 30 /* Clear _PAGE_PRESENT */ +6: #if 0 /* Not yet */ /* Honour kernel RO, User NA */ andi. r11, r10, _PAGE_USER | _PAGE_RW @@ -492,7 +494,7 @@ DataTLBError: cmpwi cr0, r10, 0x00f0 beq- FixDAR /* must be a buggy dcbX, icbi insn. */ DARFix: /* Return from dcbx instruction bug workaround, r10 holds value of DAR */ - + b 2f /* Do DIRTY in C */ mfspr r11, SPRN_DSISR andis. r11, r11, 0x4800 /* !translation or protection */ bne 2f /* branch if either is set */ ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 0/6] 8xx MMU fixes 2009-10-09 6:46 ` Rex Feany 2009-10-09 11:04 ` Joakim Tjernlund @ 2009-10-09 12:30 ` Joakim Tjernlund 1 sibling, 0 replies; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-09 12:30 UTC (permalink / raw) To: Rex Feany; +Cc: Scott Wood, linuxppc-dev Rex Feany <RFeany@mrv.com> wrote on 09/10/2009 08:46:49: > > Thus spake Joakim Tjernlund (joakim.tjernlund@transmode.se): > > > Rex Feany <RFeany@mrv.com> wrote on 09/10/2009 02:15:27: > > > > open("/proc/mounts", O_RDONLY) = 3 > > > fstat64(0x3, 0x7fe7e2a8) = 0 > > > mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =0x3001f000 > > > read(3, 0x3001f000, 1024) = -1 EFAULT (Bad address) > > > exit_group(0) = ? > > > > Try making the tlbil_va in fault.c unconditional, just to make sure > > there isn't any old TLBs around. > > didn't make a difference Perhaps you are suffering from a buggy dcbst insn? I tested it on a RO mapping and it SEGVs. Clearing the store bit manually at least fixes the SEGVs. Here is a patch for that. Jocke >From 07dbca0cf9dc13cf0fbccf54d577e3bc1c5dfdf1 Mon Sep 17 00:00:00 2001 From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> Date: Fri, 9 Oct 2009 14:18:21 +0200 Subject: [PATCH] 8xx: dcbst sets store bit in DTLB error, workaround. dcbst should not set the store bit(bit 6, DSISR) when trapping into a DTLB Error. Clear this bit while doing the dcbX missing DAR workaround. --- arch/powerpc/kernel/head_8xx.S | 24 ++++++++++++++++++++++++ 1 files changed, 24 insertions(+), 0 deletions(-) diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S index 292bd87..7b31feb 100644 --- a/arch/powerpc/kernel/head_8xx.S +++ b/arch/powerpc/kernel/head_8xx.S @@ -630,6 +630,30 @@ FixDAR: /* Entry point for dcbx workaround. */ tophys (r11, r10) beq- 139b /* Branch if user space address */ 140: lwz r11,0(r11) +/* Check if it really is a dcbx instruction. */ +/* dcbt and dcbtst does not generate DTLB Misses/Errors, + * no need to include them here */ + srwi r10, r11, 26 /* check if major OP code is 31 */ + cmpwi cr0, r10, 31 + bne- 141f + rlwinm r10, r11, 0, 21, 30 + cmpwi cr0, r10, 2028 /* Is dcbz? */ + beq+ 142f + cmpwi cr0, r10, 940 /* Is dcbi? */ + beq+ 142f + cmpwi cr0, r10, 108 /* Is dcbst? */ + beq+ 144f /* Fix up store bit! */ + cmpwi cr0, r10, 172 /* Is dcbf? */ + beq+ 142f + cmpwi cr0, r10, 1964 /* Is icbi? */ + beq+ 142f +141: mfspr r10, SPRN_DAR /* r10 must hold DAR at exit */ + b DARfix /* Nope, go back to normal TLB processing */ + +144: mfspr r10, SPRN_DSISR + rlwinm r10, r10,0,7,5 /* Clear store bit for buggy dcbst insn */ + mtspr SPRN_DSISR, r10 +142: /* continue, it was a dcbx, dcbi instruction. */ #ifdef CONFIG_8xx_CPU6 lwz r3, 8(r0) /* restore r3 from memory */ #endif -- 1.6.4.4 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 0/6] 8xx TLB fixes. @ 2009-10-07 20:45 Joakim Tjernlund 2009-10-07 20:45 ` [PATCH 1/6] 8xx: DTLB Error must check for more errors Joakim Tjernlund 0 siblings, 1 reply; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-07 20:45 UTC (permalink / raw) To: linuxppc-dev, Rex Feany, Scott Wood, Benjamin Herrenschmidt 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(-) ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/6] 8xx: DTLB Error must check for more errors. 2009-10-07 20:45 [PATCH 0/6] 8xx TLB fixes Joakim Tjernlund @ 2009-10-07 20:45 ` Joakim Tjernlund 2009-10-07 20:46 ` [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU Joakim Tjernlund 0 siblings, 1 reply; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-07 20:45 UTC (permalink / raw) To: linuxppc-dev, Rex Feany, Scott Wood, Benjamin Herrenschmidt 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU. 2009-10-07 20:45 ` [PATCH 1/6] 8xx: DTLB Error must check for more errors Joakim Tjernlund @ 2009-10-07 20:46 ` Joakim Tjernlund 2009-10-07 20:46 ` [PATCH 3/6] 8xx: invalidate non present TLBs Joakim Tjernlund 0 siblings, 1 reply; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-07 20:46 UTC (permalink / raw) To: linuxppc-dev, Rex Feany, Scott Wood, Benjamin Herrenschmidt 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-07 20:46 ` [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU Joakim Tjernlund @ 2009-10-07 20:46 ` Joakim Tjernlund 2009-10-07 21:18 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-07 20:46 UTC (permalink / raw) To: linuxppc-dev, Rex Feany, Scott Wood, Benjamin Herrenschmidt 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-07 20:46 ` [PATCH 3/6] 8xx: invalidate non present TLBs Joakim Tjernlund @ 2009-10-07 21:18 ` Benjamin Herrenschmidt 2009-10-07 22:12 ` Joakim Tjernlund 2009-10-08 19:22 ` Joakim Tjernlund 0 siblings, 2 replies; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-07 21:18 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-07 21:18 ` Benjamin Herrenschmidt @ 2009-10-07 22:12 ` Joakim Tjernlund 2009-10-07 22:20 ` Benjamin Herrenschmidt 2009-10-08 19:22 ` Joakim Tjernlund 1 sibling, 1 reply; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-07 22:12 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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. > > > > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-07 22:12 ` Joakim Tjernlund @ 2009-10-07 22:20 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-07 22:20 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-07 21:18 ` Benjamin Herrenschmidt 2009-10-07 22:12 ` Joakim Tjernlund @ 2009-10-08 19:22 ` Joakim Tjernlund 2009-10-08 20:11 ` Dan Malek 2009-10-08 20:42 ` Benjamin Herrenschmidt 1 sibling, 2 replies; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-08 19:22 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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. > > > > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 19:22 ` Joakim Tjernlund @ 2009-10-08 20:11 ` Dan Malek 2009-10-08 20:18 ` Benjamin Herrenschmidt ` (2 more replies) 2009-10-08 20:42 ` Benjamin Herrenschmidt 1 sibling, 3 replies; 36+ messages in thread From: Dan Malek @ 2009-10-08 20:11 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, Rex Feany, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 20:11 ` Dan Malek @ 2009-10-08 20:18 ` Benjamin Herrenschmidt 2009-10-08 20:28 ` Benjamin Herrenschmidt 2009-10-08 20:37 ` Joakim Tjernlund 2 siblings, 0 replies; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-08 20:18 UTC (permalink / raw) To: Dan Malek; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 20:11 ` Dan Malek 2009-10-08 20:18 ` Benjamin Herrenschmidt @ 2009-10-08 20:28 ` Benjamin Herrenschmidt 2009-10-08 22:08 ` Dan Malek 2009-10-08 20:37 ` Joakim Tjernlund 2 siblings, 1 reply; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-08 20:28 UTC (permalink / raw) To: Dan Malek; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 20:28 ` Benjamin Herrenschmidt @ 2009-10-08 22:08 ` Dan Malek 2009-10-08 22:23 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 36+ messages in thread From: Dan Malek @ 2009-10-08 22:08 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 22:08 ` Dan Malek @ 2009-10-08 22:23 ` Benjamin Herrenschmidt 2009-10-08 23:01 ` Joakim Tjernlund 2009-10-09 0:36 ` Dan Malek 0 siblings, 2 replies; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-08 22:23 UTC (permalink / raw) To: Dan Malek; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 22:23 ` Benjamin Herrenschmidt @ 2009-10-08 23:01 ` Joakim Tjernlund 2009-10-09 0:56 ` Benjamin Herrenschmidt 2009-10-09 0:36 ` Dan Malek 1 sibling, 1 reply; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-08 23:01 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 23:01 ` Joakim Tjernlund @ 2009-10-09 0:56 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-09 0:56 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 22:23 ` Benjamin Herrenschmidt 2009-10-08 23:01 ` Joakim Tjernlund @ 2009-10-09 0:36 ` Dan Malek 2009-10-09 0:57 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 36+ messages in thread From: Dan Malek @ 2009-10-09 0:36 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-09 0:36 ` Dan Malek @ 2009-10-09 0:57 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-09 0:57 UTC (permalink / raw) To: Dan Malek; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 20:11 ` Dan Malek 2009-10-08 20:18 ` Benjamin Herrenschmidt 2009-10-08 20:28 ` Benjamin Herrenschmidt @ 2009-10-08 20:37 ` Joakim Tjernlund 2009-10-08 20:44 ` Benjamin Herrenschmidt 2009-10-09 0:05 ` Dan Malek 2 siblings, 2 replies; 36+ messages in thread From: Joakim Tjernlund @ 2009-10-08 20:37 UTC (permalink / raw) To: Dan Malek; +Cc: Scott Wood, Rex Feany, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 20:37 ` Joakim Tjernlund @ 2009-10-08 20:44 ` Benjamin Herrenschmidt 2009-10-09 0:05 ` Dan Malek 1 sibling, 0 replies; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-08 20:44 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev, Rex Feany > 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 20:37 ` Joakim Tjernlund 2009-10-08 20:44 ` Benjamin Herrenschmidt @ 2009-10-09 0:05 ` Dan Malek 1 sibling, 0 replies; 36+ messages in thread From: Dan Malek @ 2009-10-09 0:05 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, Rex Feany, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] 8xx: invalidate non present TLBs 2009-10-08 19:22 ` Joakim Tjernlund 2009-10-08 20:11 ` Dan Malek @ 2009-10-08 20:42 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 36+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-08 20:42 UTC (permalink / raw) To: Joakim Tjernlund; +Cc: Scott Wood, linuxppc-dev, Rex Feany 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. > > > > > > > > ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2009-10-09 12:31 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-10-08 13:24 [PATCH 0/6] 8xx MMU fixes Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 1/6] 8xx: DTLB Error must check for more errors Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 3/6] 8xx: invalidate non present TLBs Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 4/6] 8xx: Tag DAR with 0x00f0 to catch buggy instructions Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 5/6] 8xx: Fixup DAR from buggy dcbX instructions Joakim Tjernlund 2009-10-08 13:24 ` [PATCH 6/6] 8xx: start using dcbX instructions in various copy routines Joakim Tjernlund 2009-10-08 21:04 ` [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects Benjamin Herrenschmidt 2009-10-08 22:44 ` Joakim Tjernlund 2009-10-09 0:53 ` Benjamin Herrenschmidt 2009-10-09 6:16 ` Joakim Tjernlund 2009-10-09 6:30 ` Benjamin Herrenschmidt 2009-10-08 20:48 ` [PATCH 1/6] 8xx: DTLB Error must check for more errors Benjamin Herrenschmidt 2009-10-09 0:15 ` [PATCH 0/6] 8xx MMU fixes Rex Feany 2009-10-09 6:00 ` Joakim Tjernlund 2009-10-09 6:46 ` Rex Feany 2009-10-09 11:04 ` Joakim Tjernlund 2009-10-09 12:30 ` Joakim Tjernlund -- strict thread matches above, loose matches on Subject: below -- 2009-10-07 20:45 [PATCH 0/6] 8xx TLB fixes Joakim Tjernlund 2009-10-07 20:45 ` [PATCH 1/6] 8xx: DTLB Error must check for more errors Joakim Tjernlund 2009-10-07 20:46 ` [PATCH 2/6] 8xx: get rid of _PAGE_HWWRITE dependency in MMU Joakim Tjernlund 2009-10-07 20:46 ` [PATCH 3/6] 8xx: invalidate non present TLBs Joakim Tjernlund 2009-10-07 21:18 ` Benjamin Herrenschmidt 2009-10-07 22:12 ` Joakim Tjernlund 2009-10-07 22:20 ` Benjamin Herrenschmidt 2009-10-08 19:22 ` Joakim Tjernlund 2009-10-08 20:11 ` Dan Malek 2009-10-08 20:18 ` Benjamin Herrenschmidt 2009-10-08 20:28 ` Benjamin Herrenschmidt 2009-10-08 22:08 ` Dan Malek 2009-10-08 22:23 ` Benjamin Herrenschmidt 2009-10-08 23:01 ` Joakim Tjernlund 2009-10-09 0:56 ` Benjamin Herrenschmidt 2009-10-09 0:36 ` Dan Malek 2009-10-09 0:57 ` Benjamin Herrenschmidt 2009-10-08 20:37 ` Joakim Tjernlund 2009-10-08 20:44 ` Benjamin Herrenschmidt 2009-10-09 0:05 ` Dan Malek 2009-10-08 20:42 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).