Here are my latest code to fixup 8xx's TLB code.
This code needs some serious testing before it
can be commited.
The "8xx, fault: Add some debug code to do_page_fault()" is
purely a debug check and will be removed/disabled when
this series appear stable.
Scott and Rex, please disregard other patches from me and
try these out instead.
Joakim Tjernlund (6):
8xx: DTLB Error must check for more errors.
8xx, fault: Add some debug code to do_page_fault()
8xx: get rid of _PAGE_HWWRITE dependency in MMU.
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 | 9 +-
arch/powerpc/kernel/head_8xx.S | 324 ++++++++++++++++++++++++++++++-----
arch/powerpc/kernel/misc_32.S | 18 --
arch/powerpc/lib/copy_32.S | 24 ---
arch/powerpc/mm/fault.c | 82 +++++++++
5 files changed, 363 insertions(+), 94 deletions(-)
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(-)
This is an assembler version to fixup DAR not being set
by dcbX, icbi instructions. There are two versions, one
uses selfmodifing code(default), the other uses
jump table but is much bigger.
---
arch/powerpc/kernel/head_8xx.S | 146 +++++++++++++++++++++++++++++++++++++++-
1 files changed, 145 insertions(+), 1 deletions(-)
@@ -674,6 +675,149 @@ DataTLBError:.=0x2000+/*ThisistheproceduretocalculatethedataEAforbuggydcbx,dcbiinstructions+*bydecodingtheregistersusedbythedcbxinstructionandaddingthem.+*DARissettothecalculatedaddressandr10alsoholdstheEAonexit.+*/+//#define NO_SELF_MODIFYING_CODE /* define if you don't want to use self modifying code */+nop/*Afewnopstomakethemodified_instr:spacebelowcachelinealigned*/+nop+139:/*fetchinstructionfromuserspacememory*/+DO_8xx_CPU6(0x3780,r3)+mtsprSPRN_MD_EPN,r10+mfsprr11,SPRN_M_TWB/*Getlevel1tableentryaddress*/+lwzr11,0(r11)/*Getthelevel1entry*/+tophys (r11,r11)+DO_8xx_CPU6(0x3b80,r3)+mtsprSPRN_MD_TWC,r11/*Loadptetablebaseaddress*/+mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/+lwzr11,0(r11)/*Getthepte*/+/*concatphysicalpageaddress(r11)andpageoffset(r10)*/+rlwimir11,r10,0,20,31+b140f+FixDAR:/*Entrypointfordcbxworkaround.*/+/*fetchinstructionfrommemory.*/+mfsprr10,SPRN_SRR0+andis.r11,r10,0x8000+tophys (r11,r10)+beq-139b/*Branchifuserspaceaddress*/+140:lwzr11,0(r11)+#ifdef CONFIG_8xx_CPU6+lwzr3,8(r0)/*restorer3frommemory*/+#endif+#ifndef NO_SELF_MODIFYING_CODE+andis.r10,r11,0x1f/*testifregRAisr0*/+lir10,modified_instr@l+dcbtstr0,r10/*touchforstore*/+rlwinmr11,r11,0,0,20/*Zerolower10bits*/+orisr11,r11,640/*Transforminstr.toa"add r10,RA,RB"*/+orir11,r11,532+stwr11,0(r10)/*storeadd/andinstruction*/+dcbf0,r10/*flushnewinstr.tomemory.*/+icbi0,r10/*invalidateinstr.cacheline*/+lwzr11,4(r0)/*restorer11frommemory*/+mfsprr10,SPRN_M_TW/*restorer10fromM_TW*/+isync/*Waituntilnewinstrisloadedfrommemory*/+modified_instr:+.space4/*thisiswheretheadd/andinstr.isstored*/+bne+143f+subfr10,r0,r10/*r10=r10-r0,onlyifregRAisr0*/+143:mtdarr10/*storefaultingEAinDAR*/+bDARFix/*GobacktonormalTLBhandling*/+#else+mfctrr10+mtdarr10/*savectrreginDAR*/+rlwinmr10,r11,24,24,28/*offsetintojumptableforregRB*/+addir10,r10,150f@l/*addstartoftable*/+mtctrr10/*loadctrwithjumpaddress*/+xorr10,r10,r10/*sumstartsatzero*/+bctr/*jumpintotable*/+150:+addr10,r10,r0+b151f+addr10,r10,r1+b151f+addr10,r10,r2+b151f+addr10,r10,r3+b151f+addr10,r10,r4+b151f+addr10,r10,r5+b151f+addr10,r10,r6+b151f+addr10,r10,r7+b151f+addr10,r10,r8+b151f+addr10,r10,r9+b151f+addr10,r10,r10+b151f+addr10,r10,r11+b151f+addr10,r10,r12+b151f+addr10,r10,r13+b151f+addr10,r10,r14+b151f+addr10,r10,r15+b151f+addr10,r10,r16+b151f+addr10,r10,r17+b151f+addr10,r10,r18+b151f+addr10,r10,r19+b151f+mtctrr11/*r10needsspecialhandling*/+b154f+mtctrr11/*r11needsspecialhandling*/+b153f+addr10,r10,r22+b151f+addr10,r10,r23+b151f+addr10,r10,r24+b151f+addr10,r10,r25+b151f+addr10,r10,r25+b151f+addr10,r10,r27+b151f+addr10,r10,r28+b151f+addr10,r10,r29+b151f+addr10,r10,r30+b151f+addr10,r10,r31+151:+rlwinm.r11,r11,19,24,28/*offsetintojumptableforregRA*/+beq152f/*ifregRAiszero,don't add it */ +addir11,r11,150b@l/*addstartoftable*/+mtctrr11/*loadctrwithjumpaddress*/+rlwinmr11,r11,0,16,10/*makesurewedon't execute this more than once */+bctr/*jumpintotable*/+152:+mfdarr11+mtctrr11/*restorectrregfromDAR*/+mtdarr10/*savefaultEAtoDAR*/+bDARFix/*GobacktonormalTLBhandling*/++/*specialhandlingforr10,r11sincethesearemodifiedalready*/+153:lwzr11,4(r0)/*loadr11frommemory*/+b155f+154:mfsprr11,SPRN_M_TW/*loadr10fromM_TW*/+155:addr10,r10,r11/*addit*/+mfctrr11/*restorer11*/+b151b+#endif+.globlgiveup_fpugiveup_fpu:blr
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 I/D 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:
- 4 more instructions in I/D TLB Miss, but the since the linux pte is
not written anymore, it should still be a win.
---
arch/powerpc/include/asm/pte-8xx.h | 9 +-
arch/powerpc/kernel/head_8xx.S | 163 ++++++++++++++++++++++++++----------
2 files changed, 122 insertions(+), 50 deletions(-)
@@ -333,21 +333,15 @@ InstructionTLBMiss:mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/lwzr10,0(r11)/*Getthepte*/-#ifdef CONFIG_SWAP-/*donotsetthe_PAGE_ACCESSEDbitofanon-presentpage*/-andi.r11,r10,_PAGE_PRESENT-beq4f-orir10,r10,_PAGE_ACCESSED-mfsprr11,SPRN_MD_TWC/*getthepteaddressagain*/-stwr10,0(r11)-4:-#else-orir10,r10,_PAGE_ACCESSED-stwr10,0(r11)-#endif-+andi.r11,r10,_PAGE_USER|_PAGE_ACCESSED+cmpwicr0,r11,_PAGE_USER|_PAGE_ACCESSED+beq+cr0,5f/*branchifaccessallowed*/+rlwinmr10,r10,0,22,19/*r10&=~(_PAGE_ACCESSED|_PAGE_RW)*/+b6f+5:xorir10,r10,_PAGE_RW/*invertRWbit*/+6:/*TheLinuxPTEwon't go exactly into the MMU TLB.-*Softwareindicatorbits21,22and28mustbeclear.+*Softwareindicatorbit28mustbeclear.*Softwareindicatorbits24,25,26,and27mustbe*set.AllotherLinuxPTEbitscontrolthebehavior*oftheMMU.
@@ -409,21 +403,15 @@ DataStoreTLBMiss:DO_8xx_CPU6(0x3b80,r3)mtsprSPRN_MD_TWC,r11-#ifdef CONFIG_SWAP-/*donotsetthe_PAGE_ACCESSEDbitofanon-presentpage*/-andi.r11,r10,_PAGE_PRESENT-beq4f-orir10,r10,_PAGE_ACCESSED-4:-/*andupdatepteintable*/-#else-orir10,r10,_PAGE_ACCESSED-#endif-mfsprr11,SPRN_MD_TWC/*getthepteaddressagain*/-stwr10,0(r11)-+andi.r11,r10,_PAGE_USER|_PAGE_ACCESSED+cmpwicr0,r11,_PAGE_USER|_PAGE_ACCESSED+beq+cr0,5f/*branchifaccessallowed*/+rlwinmr10,r10,0,22,19/*r10&=~(_PAGE_ACCESSED|_PAGE_RW)*/+b6f+5:xorir10,r10,_PAGE_RW/*invertRWbit*/+6:/*TheLinuxPTEwon't go exactly into the MMU TLB.-*Softwareindicatorbits21,22and28mustbeclear.+*Softwareindicatorbit28mustbeclear.*Softwareindicatorbits24,25,26,and27mustbe*set.AllotherLinuxPTEbitscontrolthebehavior*oftheMMU.
@@ -448,6 +436,91 @@ DataStoreTLBMiss:*/.=0x1300InstructionTLBError:+#ifdef CONFIG_8xx_CPU6+stwr3,8(r0)+#endif+DO_8xx_CPU6(0x3f80,r3)+mtsprSPRN_M_TW,r10/*Saveacoupleofworkingregisters*/+mfcrr10+stwr10,0(r0)+stwr11,4(r0)++mfsprr11,SPRN_SRR1+andis.r11,r11,0x5000/*notranslation,guarded*/+bne2f++mfsprr10,SPRN_SRR0/*Geteffectiveaddressoffault*/+#ifdef CONFIG_8xx_CPU15+addir11,r10,0x1000+tlbier11+addir11,r10,-0x1000+tlbier11+#endif+DO_8xx_CPU6(0x3780,r3)+mtsprSPRN_MD_EPN,r10/*HavetouseMD_EPNforwalk,MI_EPNcan't */+mfsprr10,SPRN_M_TWB/*Getlevel1tableentryaddress*/++/*Ifwearefaultingakerneladdress,wehavetousethe+*kernelpagetables.+*/+andi.r11,r10,0x0800/*Address>=0x80000000*/+beq3f+lisr11,swapper_pg_dir@h+orir11,r11,swapper_pg_dir@l+rlwimir10,r11,0,2,19+3:+lwzr11,0(r10)/*Getthelevel1entry*/+rlwinm.r10,r11,0,0,19/*Extractpagedescriptorpageaddress*/+beq2f/*Ifzero,don't try to find a pte */++/*Wehaveaptetable,soloadtheMI_TWCwiththeattributes+*forthis"segment."+*/+orir11,r11,1/*Setvalidbit*/+DO_8xx_CPU6(0x2b80,r3)+mtsprSPRN_MI_TWC,r11/*Setsegmentattributes*/+DO_8xx_CPU6(0x3b80,r3)+mtsprSPRN_MD_TWC,r11/*Loadptetablebaseaddress*/++mfsprr11,SPRN_SRR1+andi.r11,r11,0x4000/*MSR[PR]*/+mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/+lwzr10,0(r11)/*Getthepte*/+beq5f/*KernelaccessalwaysOK*/+andi.r11,r10,_PAGE_USER+beq2f+5:orir10,r10,_PAGE_ACCESSED+mfsprr21,SPRN_MD_TWC/*....andgetthepteaddress*/+stwr10,0(r11)+xorir10,r10,_PAGE_RW/*invertRWbit*/++/*TheLinuxPTEwon't go exactly into the MMU TLB.+*Softwareindicatorbit28mustbeclear.+*Softwareindicatorbits24,25,26,and27mustbe+*set.AllotherLinuxPTEbitscontrolthebehavior+*oftheMMU.+*/+lir11,0x00f0+rlwimir10,r11,0,24,28/*Set24-27,clear28*/+DO_8xx_CPU6(0x2d80,r3)+mtsprSPRN_MI_RPN,r10/*UpdateTLBentry*/++mfsprr10,SPRN_M_TW/*Restoreregisters*/+lwzr11,0(r0)+mtcrr11+lwzr11,4(r0)+#ifdef CONFIG_8xx_CPU6+lwzr3,8(r0)+#endif+rfi++2:mfsprr10,SPRN_M_TW/*Restoreregisters*/+lwzr11,0(r0)+mtcrr11+lwzr11,4(r0)+#ifdef CONFIG_8xx_CPU6+lwzr3,8(r0)+#endifbInstructionAccess/*ThisisthedataTLBerrorontheMPC8xx.Thiscouldbedueto
@@ -522,26 +595,26 @@ DataTLBError:mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/lwzr10,0(r11)/*Getthepte*/-andi.r11,r10,_PAGE_RW/*Isitwriteable?*/-beq2f/*Bailoutifnot*/+mfsprr11,SPRN_SRR1+andi.r11,r11,0x4000/*MSR[PR]*/+beq5f/*KernelaccessalwaysOK*/+andi.r11,r10,_PAGE_USER+beq2f+5:mfsprr11,SPRN_DSISR+andis.r11,r11,0x0200/*store*/+beq6f+andi.r11,r10,_PAGE_RW/*writeable?*/+beq2f/*branchifnot*/+orir10,r10,_PAGE_DIRTY|_PAGE_HWWRITE+6:orir10,r10,_PAGE_ACCESSED-/*Update'changed',amongothers.-*/-#ifdef CONFIG_SWAP-orir10,r10,_PAGE_DIRTY|_PAGE_HWWRITE-/*donotsetthe_PAGE_ACCESSEDbitofanon-presentpage*/-andi.r11,r10,_PAGE_PRESENT-beq4f-orir10,r10,_PAGE_ACCESSED-4:-#else-orir10,r10,_PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_HWWRITE-#endifmfsprr11,SPRN_MD_TWC/*Getpteaddressagain*/stwr10,0(r11)/*andupdatepteintable*/+xorir10,r10,_PAGE_RW/*InvertRWbit*/+/*TheLinuxPTEwon't go exactly into the MMU TLB.-*Softwareindicatorbits21,22and28mustbeclear.+*Softwareindicatorbit28mustbeclear.*Softwareindicatorbits24,25,26,and27mustbe*set.AllotherLinuxPTEbitscontrolthebehavior*oftheMMU.
dcbz, dcbf, dcbi, dcbst and icbi do not set DAR when they
cause a DTLB Error. Dectect this by tagging DAR with 0x00f0
at every exception exit that modifies DAR.
Test for DAR=0x00f0 in DataTLBError and bail
to handle_page_fault().
---
arch/powerpc/kernel/head_8xx.S | 19 ++++++++++++++++---
1 files changed, 16 insertions(+), 3 deletions(-)
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(-)
@@ -528,15 +520,6 @@ _GLOBAL(copy_page)addir3,r3,-4addir4,r4,-4-#ifdef CONFIG_8xx-/*don't use prefetch on 8xx */-lir0,4096/L1_CACHE_BYTES-mtctrr0-1:COPY_16_BYTES-bdnz1b-blr--#else /* not 8xx, we can prefetch */lir5,4#if MAX_COPY_PREFETCH > 1
@@ -356,14 +341,6 @@ _GLOBAL(__copy_tofrom_user)lir11,4beq63f-#ifdef CONFIG_8xx-/*Don't use prefetch on 8xx */-mtctrr0-lir0,0-53:COPY_16_BYTES_WITHEX(0)-bdnz53b--#else /* not CONFIG_8xx *//*Herewedecidehowfaraheadtoprefetchthesource*/lir3,4cmpwir0,1
From: Scott Wood <hidden> Date: 2009-10-05 18:12:21
On Mon, Oct 05, 2009 at 02:16:33PM +0200, Joakim Tjernlund wrote:
Here are my latest code to fixup 8xx's TLB code.
This code needs some serious testing before it
can be commited.
The "8xx, fault: Add some debug code to do_page_fault()" is
purely a debug check and will be removed/disabled when
this series appear stable.
Scott and Rex, please disregard other patches from me and
try these out instead.
I'm not sure which tree you based this on, but I got some conflicts
in pte-8xx.h using Ben's merge branch.
After resolving the conflict, without adding tlbil_va in do_page_fault(), I
get the same stuck behavior as before. With tlbil_va, I get this (not sure
if it's related to using the wrong base tree):
INIT: version 2.85 booting
Mounting /proc and /sys
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 173 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 174 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 175 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 176 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 177 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 178 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 179 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 180 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 181 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 182 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 37: 183 Segmentation fault /etc/rc.d/rc.local $mode
INIT: Entering runlevel: 1
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
INIT: Id "l1" respawning too fast: disabled for 5 minutes
INIT: no more processes left in this runlevel
-Scott
Scott Wood [off-list ref] wrote on 05/10/2009 20:12:34:
On Mon, Oct 05, 2009 at 02:16:33PM +0200, Joakim Tjernlund wrote:
quoted
Here are my latest code to fixup 8xx's TLB code.
This code needs some serious testing before it
can be commited.
The "8xx, fault: Add some debug code to do_page_fault()" is
purely a debug check and will be removed/disabled when
this series appear stable.
Scott and Rex, please disregard other patches from me and
try these out instead.
I'm not sure which tree you based this on, but I got some conflicts
in pte-8xx.h using Ben's merge branch.
It is a somewhat older tree, didn't think it would clash, sorry
After resolving the conflict, without adding tlbil_va in do_page_fault(), I
get the same stuck behavior as before.
Expected, I havn't not tried to fix the missing tlbil_va(). That is
different problem that you and Ben needs to sort out.
With tlbil_va, I get this (not sure
if it's related to using the wrong base tree):
Could be the debug code doing something bad or the
"Fixup DAR from buggy dcbX instructions". Could you back that one out?
and if that does not help, backout:
"start using dcbX instructions in various copy routines" too.
INIT: version 2.85 booting
Mounting /proc and /sys
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 173 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 174 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 175 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 176 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 177 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 178 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 179 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 180 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 181 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 24: 182 Segmentation fault /etc/rc.d/init.d/$i $mode
dcbz: address:1, dar:30020000!
/etc/rc.d/rcS: line 37: 183 Segmentation fault /etc/rc.d/rc.local $mode
INIT: Entering runlevel: 1
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
dcbz: address:1, dar:30020000!
INIT: Id "l1" respawning too fast: disabled for 5 minutes
INIT: no more processes left in this runlevel
-Scott
From: Scott Wood <hidden> Date: 2009-10-05 20:09:29
On Mon, Oct 05, 2009 at 08:27:39PM +0200, Joakim Tjernlund wrote:
quoted
After resolving the conflict, without adding tlbil_va in do_page_fault(), I
get the same stuck behavior as before.
Expected, I havn't not tried to fix the missing tlbil_va(). That is
different problem that you and Ben needs to sort out.
Right, I just wanted to be clear about which results were from which
modifications.
quoted
With tlbil_va, I get this (not sure
if it's related to using the wrong base tree):
Could be the debug code doing something bad or the
"Fixup DAR from buggy dcbX instructions". Could you back that one out?
That gets rid of the segfaults. The bootup gets stuck running udev, though
-- the system is either idle, or it's stuck doing some syscall.
I see it sometimes (but less reliably) without this patchset, so it may just
be changing the circumstances to expose the issue more consistently. Or
maybe I'm seeing the dcbX bug? There do seem to be dcbX instructions in the
dynamic linker I'm using (including a disturbing section that appears to be
assuming 32 byte cache lines, even though this is supposed to be an 8xx
RFS).
I'll look into it.
and if that does not help, backout:
"start using dcbX instructions in various copy routines" too.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-05 20:17:19
On Mon, 2009-10-05 at 14:16 +0200, Joakim Tjernlund wrote:
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 I/D TLB Error fixing accounting
- _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly
when a page has been made dirty.
Not sure here. You seem to still set those from asm. Is that necessary ?
The approach I take on BookE is to simply not set these from asm, -and-
(this is important) not set write permission if dirty is not set in
the TLB miss and set no access permission at all when accessed is not
set. This is important or we'll miss dirty updates which can
be fatal.
The C code in handle_pte_fault() will fixup missing access and dirty
if necessary and flush.
Also look at the 440 code, I think you could simplify your
implementation using similar techniques, such as andc of PTE against
requested bits etc... and thus maybe save a couple of insns.
Cheers,
Ben.
quoted hunk
- Proper RO/RW mapping of user space.
- Free up 2 SW TLB bits in the linux pte(add back _PAGE_WRITETHRU ?)
Cons:
- 4 more instructions in I/D TLB Miss, but the since the linux pte is
not written anymore, it should still be a win.
---
arch/powerpc/include/asm/pte-8xx.h | 9 +-
arch/powerpc/kernel/head_8xx.S | 163 ++++++++++++++++++++++++++----------
2 files changed, 122 insertions(+), 50 deletions(-)
@@ -333,21 +333,15 @@ InstructionTLBMiss:mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/lwzr10,0(r11)/*Getthepte*/-#ifdef CONFIG_SWAP-/*donotsetthe_PAGE_ACCESSEDbitofanon-presentpage*/-andi.r11,r10,_PAGE_PRESENT-beq4f-orir10,r10,_PAGE_ACCESSED-mfsprr11,SPRN_MD_TWC/*getthepteaddressagain*/-stwr10,0(r11)-4:-#else-orir10,r10,_PAGE_ACCESSED-stwr10,0(r11)-#endif-+andi.r11,r10,_PAGE_USER|_PAGE_ACCESSED+cmpwicr0,r11,_PAGE_USER|_PAGE_ACCESSED+beq+cr0,5f/*branchifaccessallowed*/+rlwinmr10,r10,0,22,19/*r10&=~(_PAGE_ACCESSED|_PAGE_RW)*/+b6f+5:xorir10,r10,_PAGE_RW/*invertRWbit*/+6:/*TheLinuxPTEwon't go exactly into the MMU TLB.-*Softwareindicatorbits21,22and28mustbeclear.+*Softwareindicatorbit28mustbeclear.*Softwareindicatorbits24,25,26,and27mustbe*set.AllotherLinuxPTEbitscontrolthebehavior*oftheMMU.
@@ -409,21 +403,15 @@ DataStoreTLBMiss:DO_8xx_CPU6(0x3b80,r3)mtsprSPRN_MD_TWC,r11-#ifdef CONFIG_SWAP-/*donotsetthe_PAGE_ACCESSEDbitofanon-presentpage*/-andi.r11,r10,_PAGE_PRESENT-beq4f-orir10,r10,_PAGE_ACCESSED-4:-/*andupdatepteintable*/-#else-orir10,r10,_PAGE_ACCESSED-#endif-mfsprr11,SPRN_MD_TWC/*getthepteaddressagain*/-stwr10,0(r11)-+andi.r11,r10,_PAGE_USER|_PAGE_ACCESSED+cmpwicr0,r11,_PAGE_USER|_PAGE_ACCESSED+beq+cr0,5f/*branchifaccessallowed*/+rlwinmr10,r10,0,22,19/*r10&=~(_PAGE_ACCESSED|_PAGE_RW)*/+b6f+5:xorir10,r10,_PAGE_RW/*invertRWbit*/+6:/*TheLinuxPTEwon't go exactly into the MMU TLB.-*Softwareindicatorbits21,22and28mustbeclear.+*Softwareindicatorbit28mustbeclear.*Softwareindicatorbits24,25,26,and27mustbe*set.AllotherLinuxPTEbitscontrolthebehavior*oftheMMU.
@@ -448,6 +436,91 @@ DataStoreTLBMiss:*/.=0x1300InstructionTLBError:+#ifdef CONFIG_8xx_CPU6+stwr3,8(r0)+#endif+DO_8xx_CPU6(0x3f80,r3)+mtsprSPRN_M_TW,r10/*Saveacoupleofworkingregisters*/+mfcrr10+stwr10,0(r0)+stwr11,4(r0)++mfsprr11,SPRN_SRR1+andis.r11,r11,0x5000/*notranslation,guarded*/+bne2f++mfsprr10,SPRN_SRR0/*Geteffectiveaddressoffault*/+#ifdef CONFIG_8xx_CPU15+addir11,r10,0x1000+tlbier11+addir11,r10,-0x1000+tlbier11+#endif+DO_8xx_CPU6(0x3780,r3)+mtsprSPRN_MD_EPN,r10/*HavetouseMD_EPNforwalk,MI_EPNcan't */+mfsprr10,SPRN_M_TWB/*Getlevel1tableentryaddress*/++/*Ifwearefaultingakerneladdress,wehavetousethe+*kernelpagetables.+*/+andi.r11,r10,0x0800/*Address>=0x80000000*/+beq3f+lisr11,swapper_pg_dir@h+orir11,r11,swapper_pg_dir@l+rlwimir10,r11,0,2,19+3:+lwzr11,0(r10)/*Getthelevel1entry*/+rlwinm.r10,r11,0,0,19/*Extractpagedescriptorpageaddress*/+beq2f/*Ifzero,don't try to find a pte */++/*Wehaveaptetable,soloadtheMI_TWCwiththeattributes+*forthis"segment."+*/+orir11,r11,1/*Setvalidbit*/+DO_8xx_CPU6(0x2b80,r3)+mtsprSPRN_MI_TWC,r11/*Setsegmentattributes*/+DO_8xx_CPU6(0x3b80,r3)+mtsprSPRN_MD_TWC,r11/*Loadptetablebaseaddress*/++mfsprr11,SPRN_SRR1+andi.r11,r11,0x4000/*MSR[PR]*/+mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/+lwzr10,0(r11)/*Getthepte*/+beq5f/*KernelaccessalwaysOK*/+andi.r11,r10,_PAGE_USER+beq2f+5:orir10,r10,_PAGE_ACCESSED+mfsprr21,SPRN_MD_TWC/*....andgetthepteaddress*/+stwr10,0(r11)+xorir10,r10,_PAGE_RW/*invertRWbit*/++/*TheLinuxPTEwon't go exactly into the MMU TLB.+*Softwareindicatorbit28mustbeclear.+*Softwareindicatorbits24,25,26,and27mustbe+*set.AllotherLinuxPTEbitscontrolthebehavior+*oftheMMU.+*/+lir11,0x00f0+rlwimir10,r11,0,24,28/*Set24-27,clear28*/+DO_8xx_CPU6(0x2d80,r3)+mtsprSPRN_MI_RPN,r10/*UpdateTLBentry*/++mfsprr10,SPRN_M_TW/*Restoreregisters*/+lwzr11,0(r0)+mtcrr11+lwzr11,4(r0)+#ifdef CONFIG_8xx_CPU6+lwzr3,8(r0)+#endif+rfi++2:mfsprr10,SPRN_M_TW/*Restoreregisters*/+lwzr11,0(r0)+mtcrr11+lwzr11,4(r0)+#ifdef CONFIG_8xx_CPU6+lwzr3,8(r0)+#endifbInstructionAccess/*ThisisthedataTLBerrorontheMPC8xx.Thiscouldbedueto
@@ -522,26 +595,26 @@ DataTLBError:mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/lwzr10,0(r11)/*Getthepte*/-andi.r11,r10,_PAGE_RW/*Isitwriteable?*/-beq2f/*Bailoutifnot*/+mfsprr11,SPRN_SRR1+andi.r11,r11,0x4000/*MSR[PR]*/+beq5f/*KernelaccessalwaysOK*/+andi.r11,r10,_PAGE_USER+beq2f+5:mfsprr11,SPRN_DSISR+andis.r11,r11,0x0200/*store*/+beq6f+andi.r11,r10,_PAGE_RW/*writeable?*/+beq2f/*branchifnot*/+orir10,r10,_PAGE_DIRTY|_PAGE_HWWRITE+6:orir10,r10,_PAGE_ACCESSED-/*Update'changed',amongothers.-*/-#ifdef CONFIG_SWAP-orir10,r10,_PAGE_DIRTY|_PAGE_HWWRITE-/*donotsetthe_PAGE_ACCESSEDbitofanon-presentpage*/-andi.r11,r10,_PAGE_PRESENT-beq4f-orir10,r10,_PAGE_ACCESSED-4:-#else-orir10,r10,_PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_HWWRITE-#endifmfsprr11,SPRN_MD_TWC/*Getpteaddressagain*/stwr10,0(r11)/*andupdatepteintable*/+xorir10,r10,_PAGE_RW/*InvertRWbit*/+/*TheLinuxPTEwon't go exactly into the MMU TLB.-*Softwareindicatorbits21,22and28mustbeclear.+*Softwareindicatorbit28mustbeclear.*Softwareindicatorbits24,25,26,and27mustbe*set.AllotherLinuxPTEbitscontrolthebehavior*oftheMMU.
Scott Wood [off-list ref] wrote on 05/10/2009 22:09:41:
On Mon, Oct 05, 2009 at 08:27:39PM +0200, Joakim Tjernlund wrote:
quoted
quoted
After resolving the conflict, without adding tlbil_va in do_page_fault(), I
get the same stuck behavior as before.
Expected, I havn't not tried to fix the missing tlbil_va(). That is
different problem that you and Ben needs to sort out.
Right, I just wanted to be clear about which results were from which
modifications.
quoted
quoted
With tlbil_va, I get this (not sure
if it's related to using the wrong base tree):
Could be the debug code doing something bad or the
"Fixup DAR from buggy dcbX instructions". Could you back that one out?
That gets rid of the segfaults. The bootup gets stuck running udev, though
-- the system is either idle, or it's stuck doing some syscall.
Ok, that just means that my asm version of fixing up dcbX insn does not work.
I see it sometimes (but less reliably) without this patchset, so it may just
be changing the circumstances to expose the issue more consistently. Or
maybe I'm seeing the dcbX bug? There do seem to be dcbX instructions in the
dynamic linker I'm using (including a disturbing section that appears to be
assuming 32 byte cache lines, even though this is supposed to be an 8xx
RFS).
Yes, every ld.so uses dcbX and icbi insn when relocatin code.
Maybe you see some version of the dcbX bug, but my fault.c should
fix them up. My bet would be the 32 byte cache line, it will miss
out every second line and so the results are unreliable.
I'll look into it.
quoted
and if that does not help, backout:
"start using dcbX instructions in various copy routines" too.
No difference.
Good, it seems possible to uses the dcbX insn now. I will
have to fixup fault a bit more though.
Benjamin Herrenschmidt [off-list ref] wrote on 05/10/2009 22:17:04:
On Mon, 2009-10-05 at 14:16 +0200, Joakim Tjernlund wrote:
quoted
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 I/D TLB Error fixing accounting
- _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly
when a page has been made dirty.
Not sure here. You seem to still set those from asm. Is that necessary ?
Well, yes. head_32.S also sets ACCESSED and DIRTY from asm so why not me?
The basic method I use is:
TLB gets mapped with NoAccess, then the first access will trap
into TLB error, where ACCESSED will be set if permission to access
the page is OK (and RW too). On first store 8xx will trap
into DTLB error and permissions is OK, DIRTY will be set too.
Is this wrong?
Do I have to trap to C for first store?
The approach I take on BookE is to simply not set these from asm, -and-
(this is important) not set write permission if dirty is not set in
the TLB miss and set no access permission at all when accessed is not
set. This is important or we'll miss dirty updates which can
be fatal.
not sure, but this seems similar to what I do. DIRTY will be set,
in asm, on first store.
ACCESSED will only be set iff (USER && VALID)
The C code in handle_pte_fault() will fixup missing access and dirty
if necessary and flush.
Also look at the 440 code, I think you could simplify your
implementation using similar techniques, such as andc of PTE against
requested bits etc... and thus maybe save a couple of insns.
Great, but first I want to make sure I doing it right :)
So is there some golden rule I have to follow?
Jocke
Cheers,
Ben.
quoted
- Proper RO/RW mapping of user space.
- Free up 2 SW TLB bits in the linux pte(add back _PAGE_WRITETHRU ?)
Cons:
- 4 more instructions in I/D TLB Miss, but the since the linux pte is
not written anymore, it should still be a win.
---
arch/powerpc/include/asm/pte-8xx.h | 9 +-
arch/powerpc/kernel/head_8xx.S | 163 ++++++++++++++++++++++++++----------
2 files changed, 122 insertions(+), 50 deletions(-)
@@ -333,21 +333,15 @@ InstructionTLBMiss:mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/lwzr10,0(r11)/*Getthepte*/-#ifdef CONFIG_SWAP-/*donotsetthe_PAGE_ACCESSEDbitofanon-presentpage*/-andi.r11,r10,_PAGE_PRESENT-beq4f-orir10,r10,_PAGE_ACCESSED-mfsprr11,SPRN_MD_TWC/*getthepteaddressagain*/-stwr10,0(r11)-4:-#else-orir10,r10,_PAGE_ACCESSED-stwr10,0(r11)-#endif-+andi.r11,r10,_PAGE_USER|_PAGE_ACCESSED+cmpwicr0,r11,_PAGE_USER|_PAGE_ACCESSED+beq+cr0,5f/*branchifaccessallowed*/+rlwinmr10,r10,0,22,19/*r10&=~(_PAGE_ACCESSED|_PAGE_RW)*/+b6f+5:xorir10,r10,_PAGE_RW/*invertRWbit*/+6:/*TheLinuxPTEwon't go exactly into the MMU TLB.-*Softwareindicatorbits21,22and28mustbeclear.+*Softwareindicatorbit28mustbeclear.*Softwareindicatorbits24,25,26,and27mustbe*set.AllotherLinuxPTEbitscontrolthebehavior*oftheMMU.
@@ -409,21 +403,15 @@ DataStoreTLBMiss:DO_8xx_CPU6(0x3b80,r3)mtsprSPRN_MD_TWC,r11-#ifdef CONFIG_SWAP-/*donotsetthe_PAGE_ACCESSEDbitofanon-presentpage*/-andi.r11,r10,_PAGE_PRESENT-beq4f-orir10,r10,_PAGE_ACCESSED-4:-/*andupdatepteintable*/-#else-orir10,r10,_PAGE_ACCESSED-#endif-mfsprr11,SPRN_MD_TWC/*getthepteaddressagain*/-stwr10,0(r11)-+andi.r11,r10,_PAGE_USER|_PAGE_ACCESSED+cmpwicr0,r11,_PAGE_USER|_PAGE_ACCESSED+beq+cr0,5f/*branchifaccessallowed*/+rlwinmr10,r10,0,22,19/*r10&=~(_PAGE_ACCESSED|_PAGE_RW)*/+b6f+5:xorir10,r10,_PAGE_RW/*invertRWbit*/+6:/*TheLinuxPTEwon't go exactly into the MMU TLB.-*Softwareindicatorbits21,22and28mustbeclear.+*Softwareindicatorbit28mustbeclear.*Softwareindicatorbits24,25,26,and27mustbe*set.AllotherLinuxPTEbitscontrolthebehavior*oftheMMU.
@@ -448,6 +436,91 @@ DataStoreTLBMiss:*/.=0x1300InstructionTLBError:+#ifdef CONFIG_8xx_CPU6+stwr3,8(r0)+#endif+DO_8xx_CPU6(0x3f80,r3)+mtsprSPRN_M_TW,r10/*Saveacoupleofworkingregisters*/+mfcrr10+stwr10,0(r0)+stwr11,4(r0)++mfsprr11,SPRN_SRR1+andis.r11,r11,0x5000/*notranslation,guarded*/+bne2f++mfsprr10,SPRN_SRR0/*Geteffectiveaddressoffault*/+#ifdef CONFIG_8xx_CPU15+addir11,r10,0x1000+tlbier11+addir11,r10,-0x1000+tlbier11+#endif+DO_8xx_CPU6(0x3780,r3)+mtsprSPRN_MD_EPN,r10/*HavetouseMD_EPNforwalk,MI_EPNcan't */+mfsprr10,SPRN_M_TWB/*Getlevel1tableentryaddress*/++/*Ifwearefaultingakerneladdress,wehavetousethe+*kernelpagetables.+*/+andi.r11,r10,0x0800/*Address>=0x80000000*/+beq3f+lisr11,swapper_pg_dir@h+orir11,r11,swapper_pg_dir@l+rlwimir10,r11,0,2,19+3:+lwzr11,0(r10)/*Getthelevel1entry*/+rlwinm.r10,r11,0,0,19/*Extractpagedescriptorpageaddress*/+beq2f/*Ifzero,don't try to find a pte */++/*Wehaveaptetable,soloadtheMI_TWCwiththeattributes+*forthis"segment."+*/+orir11,r11,1/*Setvalidbit*/+DO_8xx_CPU6(0x2b80,r3)+mtsprSPRN_MI_TWC,r11/*Setsegmentattributes*/+DO_8xx_CPU6(0x3b80,r3)+mtsprSPRN_MD_TWC,r11/*Loadptetablebaseaddress*/++mfsprr11,SPRN_SRR1+andi.r11,r11,0x4000/*MSR[PR]*/+mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/+lwzr10,0(r11)/*Getthepte*/+beq5f/*KernelaccessalwaysOK*/+andi.r11,r10,_PAGE_USER+beq2f+5:orir10,r10,_PAGE_ACCESSED+mfsprr21,SPRN_MD_TWC/*....andgetthepteaddress*/+stwr10,0(r11)+xorir10,r10,_PAGE_RW/*invertRWbit*/++/*TheLinuxPTEwon't go exactly into the MMU TLB.+*Softwareindicatorbit28mustbeclear.+*Softwareindicatorbits24,25,26,and27mustbe+*set.AllotherLinuxPTEbitscontrolthebehavior+*oftheMMU.+*/+lir11,0x00f0+rlwimir10,r11,0,24,28/*Set24-27,clear28*/+DO_8xx_CPU6(0x2d80,r3)+mtsprSPRN_MI_RPN,r10/*UpdateTLBentry*/++mfsprr10,SPRN_M_TW/*Restoreregisters*/+lwzr11,0(r0)+mtcrr11+lwzr11,4(r0)+#ifdef CONFIG_8xx_CPU6+lwzr3,8(r0)+#endif+rfi++2:mfsprr10,SPRN_M_TW/*Restoreregisters*/+lwzr11,0(r0)+mtcrr11+lwzr11,4(r0)+#ifdef CONFIG_8xx_CPU6+lwzr3,8(r0)+#endifbInstructionAccess/*ThisisthedataTLBerrorontheMPC8xx.Thiscouldbedueto
@@ -522,26 +595,26 @@ DataTLBError:mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/lwzr10,0(r11)/*Getthepte*/-andi.r11,r10,_PAGE_RW/*Isitwriteable?*/-beq2f/*Bailoutifnot*/+mfsprr11,SPRN_SRR1+andi.r11,r11,0x4000/*MSR[PR]*/+beq5f/*KernelaccessalwaysOK*/+andi.r11,r10,_PAGE_USER+beq2f+5:mfsprr11,SPRN_DSISR+andis.r11,r11,0x0200/*store*/+beq6f+andi.r11,r10,_PAGE_RW/*writeable?*/+beq2f/*branchifnot*/+orir10,r10,_PAGE_DIRTY|_PAGE_HWWRITE+6:orir10,r10,_PAGE_ACCESSED-/*Update'changed',amongothers.-*/-#ifdef CONFIG_SWAP-orir10,r10,_PAGE_DIRTY|_PAGE_HWWRITE-/*donotsetthe_PAGE_ACCESSEDbitofanon-presentpage*/-andi.r11,r10,_PAGE_PRESENT-beq4f-orir10,r10,_PAGE_ACCESSED-4:-#else-orir10,r10,_PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_HWWRITE-#endifmfsprr11,SPRN_MD_TWC/*Getpteaddressagain*/stwr10,0(r11)/*andupdatepteintable*/+xorir10,r10,_PAGE_RW/*InvertRWbit*/+/*TheLinuxPTEwon't go exactly into the MMU TLB.-*Softwareindicatorbits21,22and28mustbeclear.+*Softwareindicatorbit28mustbeclear.*Softwareindicatorbits24,25,26,and27mustbe*set.AllotherLinuxPTEbitscontrolthebehavior*oftheMMU.
From: Scott Wood <hidden> Date: 2009-10-05 21:31:34
Joakim Tjernlund wrote:
Yes, every ld.so uses dcbX and icbi insn when relocatin code.
Maybe you see some version of the dcbX bug, but my fault.c should
fix them up. My bet would be the 32 byte cache line, it will miss
out every second line and so the results are unreliable.
I found the 32-byte stuff from an objdump; it looks like there's a
runtime check for the cache line size that avoids running that code if
it's different.
-Scott
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-05 21:31:51
On Mon, 2009-10-05 at 23:04 +0200, Joakim Tjernlund wrote:
Yes, every ld.so uses dcbX and icbi insn when relocatin code.
Even with -msecure-plt ?
Maybe you see some version of the dcbX bug, but my fault.c should
fix them up. My bet would be the 32 byte cache line, it will miss
out every second line and so the results are unreliable.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-05 21:37:34
On Mon, 2009-10-05 at 23:25 +0200, Joakim Tjernlund wrote:
Benjamin Herrenschmidt [off-list ref] wrote on 05/10/2009 22:17:04:
quoted
On Mon, 2009-10-05 at 14:16 +0200, Joakim Tjernlund wrote:
quoted
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 I/D TLB Error fixing accounting
- _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly
when a page has been made dirty.
Not sure here. You seem to still set those from asm. Is that necessary ?
Well, yes. head_32.S also sets ACCESSED and DIRTY from asm so why not me?
Don't look at the hash 32 code :-)
The basic method I use is:
TLB gets mapped with NoAccess, then the first access will trap
into TLB error, where ACCESSED will be set if permission to access
the page is OK (and RW too). On first store 8xx will trap
into DTLB error and permissions is OK, DIRTY will be set too.
Is this wrong?
From your explanation it looks ok. IE. as long as !DIRTY -> not
writeable (will fault on store) and !ACCESSED means not accessible (will
fault on any access) then you are ok.
Do I have to trap to C for first store?
Most of the time you do anyways since the PTE isn't populated at all. At
which point, Linux will populate a PTE with DIRTY and ACCESSED already
set. It should be reasonably rare to actually fault because DIRTY and/or
ACCESSED are missing.
quoted
The approach I take on BookE is to simply not set these from asm, -and-
(this is important) not set write permission if dirty is not set in
the TLB miss and set no access permission at all when accessed is not
set. This is important or we'll miss dirty updates which can
be fatal.
not sure, but this seems similar to what I do. DIRTY will be set,
in asm, on first store.
Don't set DIRTY if !VALID
ACCESSED will only be set iff (USER && VALID)
My point is that you should be able to simplify the code here, have only
the TLB miss look at the PTE, not the data access and instruction
access, and have the later be a boring exception going straight to C.
quoted
The C code in handle_pte_fault() will fixup missing access and dirty
if necessary and flush.
Also look at the 440 code, I think you could simplify your
implementation using similar techniques, such as andc of PTE against
requested bits etc... and thus maybe save a couple of insns.
Great, but first I want to make sure I doing it right :)
So is there some golden rule I have to follow?
Mostly only !ACCESSED -> no access permitted and !DIRTY -> no store
permitted and don't write anything back if !VALID.
Cheers,
Ben.
Jocke
quoted
Cheers,
Ben.
quoted
- Proper RO/RW mapping of user space.
- Free up 2 SW TLB bits in the linux pte(add back _PAGE_WRITETHRU ?)
Cons:
- 4 more instructions in I/D TLB Miss, but the since the linux pte is
not written anymore, it should still be a win.
---
arch/powerpc/include/asm/pte-8xx.h | 9 +-
arch/powerpc/kernel/head_8xx.S | 163 ++++++++++++++++++++++++++----------
2 files changed, 122 insertions(+), 50 deletions(-)
@@ -333,21 +333,15 @@ InstructionTLBMiss:mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/lwzr10,0(r11)/*Getthepte*/-#ifdef CONFIG_SWAP-/*donotsetthe_PAGE_ACCESSEDbitofanon-presentpage*/-andi.r11,r10,_PAGE_PRESENT-beq4f-orir10,r10,_PAGE_ACCESSED-mfsprr11,SPRN_MD_TWC/*getthepteaddressagain*/-stwr10,0(r11)-4:-#else-orir10,r10,_PAGE_ACCESSED-stwr10,0(r11)-#endif-+andi.r11,r10,_PAGE_USER|_PAGE_ACCESSED+cmpwicr0,r11,_PAGE_USER|_PAGE_ACCESSED+beq+cr0,5f/*branchifaccessallowed*/+rlwinmr10,r10,0,22,19/*r10&=~(_PAGE_ACCESSED|_PAGE_RW)*/+b6f+5:xorir10,r10,_PAGE_RW/*invertRWbit*/+6:/*TheLinuxPTEwon't go exactly into the MMU TLB.-*Softwareindicatorbits21,22and28mustbeclear.+*Softwareindicatorbit28mustbeclear.*Softwareindicatorbits24,25,26,and27mustbe*set.AllotherLinuxPTEbitscontrolthebehavior*oftheMMU.
@@ -409,21 +403,15 @@ DataStoreTLBMiss:DO_8xx_CPU6(0x3b80,r3)mtsprSPRN_MD_TWC,r11-#ifdef CONFIG_SWAP-/*donotsetthe_PAGE_ACCESSEDbitofanon-presentpage*/-andi.r11,r10,_PAGE_PRESENT-beq4f-orir10,r10,_PAGE_ACCESSED-4:-/*andupdatepteintable*/-#else-orir10,r10,_PAGE_ACCESSED-#endif-mfsprr11,SPRN_MD_TWC/*getthepteaddressagain*/-stwr10,0(r11)-+andi.r11,r10,_PAGE_USER|_PAGE_ACCESSED+cmpwicr0,r11,_PAGE_USER|_PAGE_ACCESSED+beq+cr0,5f/*branchifaccessallowed*/+rlwinmr10,r10,0,22,19/*r10&=~(_PAGE_ACCESSED|_PAGE_RW)*/+b6f+5:xorir10,r10,_PAGE_RW/*invertRWbit*/+6:/*TheLinuxPTEwon't go exactly into the MMU TLB.-*Softwareindicatorbits21,22and28mustbeclear.+*Softwareindicatorbit28mustbeclear.*Softwareindicatorbits24,25,26,and27mustbe*set.AllotherLinuxPTEbitscontrolthebehavior*oftheMMU.
@@ -448,6 +436,91 @@ DataStoreTLBMiss:*/.=0x1300InstructionTLBError:+#ifdef CONFIG_8xx_CPU6+stwr3,8(r0)+#endif+DO_8xx_CPU6(0x3f80,r3)+mtsprSPRN_M_TW,r10/*Saveacoupleofworkingregisters*/+mfcrr10+stwr10,0(r0)+stwr11,4(r0)++mfsprr11,SPRN_SRR1+andis.r11,r11,0x5000/*notranslation,guarded*/+bne2f++mfsprr10,SPRN_SRR0/*Geteffectiveaddressoffault*/+#ifdef CONFIG_8xx_CPU15+addir11,r10,0x1000+tlbier11+addir11,r10,-0x1000+tlbier11+#endif+DO_8xx_CPU6(0x3780,r3)+mtsprSPRN_MD_EPN,r10/*HavetouseMD_EPNforwalk,MI_EPNcan't */+mfsprr10,SPRN_M_TWB/*Getlevel1tableentryaddress*/++/*Ifwearefaultingakerneladdress,wehavetousethe+*kernelpagetables.+*/+andi.r11,r10,0x0800/*Address>=0x80000000*/+beq3f+lisr11,swapper_pg_dir@h+orir11,r11,swapper_pg_dir@l+rlwimir10,r11,0,2,19+3:+lwzr11,0(r10)/*Getthelevel1entry*/+rlwinm.r10,r11,0,0,19/*Extractpagedescriptorpageaddress*/+beq2f/*Ifzero,don't try to find a pte */++/*Wehaveaptetable,soloadtheMI_TWCwiththeattributes+*forthis"segment."+*/+orir11,r11,1/*Setvalidbit*/+DO_8xx_CPU6(0x2b80,r3)+mtsprSPRN_MI_TWC,r11/*Setsegmentattributes*/+DO_8xx_CPU6(0x3b80,r3)+mtsprSPRN_MD_TWC,r11/*Loadptetablebaseaddress*/++mfsprr11,SPRN_SRR1+andi.r11,r11,0x4000/*MSR[PR]*/+mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/+lwzr10,0(r11)/*Getthepte*/+beq5f/*KernelaccessalwaysOK*/+andi.r11,r10,_PAGE_USER+beq2f+5:orir10,r10,_PAGE_ACCESSED+mfsprr21,SPRN_MD_TWC/*....andgetthepteaddress*/+stwr10,0(r11)+xorir10,r10,_PAGE_RW/*invertRWbit*/++/*TheLinuxPTEwon't go exactly into the MMU TLB.+*Softwareindicatorbit28mustbeclear.+*Softwareindicatorbits24,25,26,and27mustbe+*set.AllotherLinuxPTEbitscontrolthebehavior+*oftheMMU.+*/+lir11,0x00f0+rlwimir10,r11,0,24,28/*Set24-27,clear28*/+DO_8xx_CPU6(0x2d80,r3)+mtsprSPRN_MI_RPN,r10/*UpdateTLBentry*/++mfsprr10,SPRN_M_TW/*Restoreregisters*/+lwzr11,0(r0)+mtcrr11+lwzr11,4(r0)+#ifdef CONFIG_8xx_CPU6+lwzr3,8(r0)+#endif+rfi++2:mfsprr10,SPRN_M_TW/*Restoreregisters*/+lwzr11,0(r0)+mtcrr11+lwzr11,4(r0)+#ifdef CONFIG_8xx_CPU6+lwzr3,8(r0)+#endifbInstructionAccess/*ThisisthedataTLBerrorontheMPC8xx.Thiscouldbedueto
@@ -522,26 +595,26 @@ DataTLBError:mfsprr11,SPRN_MD_TWC/*....andgetthepteaddress*/lwzr10,0(r11)/*Getthepte*/-andi.r11,r10,_PAGE_RW/*Isitwriteable?*/-beq2f/*Bailoutifnot*/+mfsprr11,SPRN_SRR1+andi.r11,r11,0x4000/*MSR[PR]*/+beq5f/*KernelaccessalwaysOK*/+andi.r11,r10,_PAGE_USER+beq2f+5:mfsprr11,SPRN_DSISR+andis.r11,r11,0x0200/*store*/+beq6f+andi.r11,r10,_PAGE_RW/*writeable?*/+beq2f/*branchifnot*/+orir10,r10,_PAGE_DIRTY|_PAGE_HWWRITE+6:orir10,r10,_PAGE_ACCESSED-/*Update'changed',amongothers.-*/-#ifdef CONFIG_SWAP-orir10,r10,_PAGE_DIRTY|_PAGE_HWWRITE-/*donotsetthe_PAGE_ACCESSEDbitofanon-presentpage*/-andi.r11,r10,_PAGE_PRESENT-beq4f-orir10,r10,_PAGE_ACCESSED-4:-#else-orir10,r10,_PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_HWWRITE-#endifmfsprr11,SPRN_MD_TWC/*Getpteaddressagain*/stwr10,0(r11)/*andupdatepteintable*/+xorir10,r10,_PAGE_RW/*InvertRWbit*/+/*TheLinuxPTEwon't go exactly into the MMU TLB.-*Softwareindicatorbits21,22and28mustbeclear.+*Softwareindicatorbit28mustbeclear.*Softwareindicatorbits24,25,26,and27mustbe*set.AllotherLinuxPTEbitscontrolthebehavior*oftheMMU.
Benjamin Herrenschmidt [off-list ref] wrote on 05/10/2009 23:31:39:
quoted
Yes, every ld.so uses dcbX and icbi insn when relocatin code.
Even with -msecure-plt ?
hmm, maybe not. Can't remember now. But perhaps LAZY relocs still
need dcbX? Easiest is to check in uClibc. I impl. it, but was to long
time ago for me to remember. Scott are you using uClibc? I don't
think so because of the dynamic check of the cache line size
quoted
Maybe you see some version of the dcbX bug, but my fault.c should
fix them up. My bet would be the 32 byte cache line, it will miss
out every second line and so the results are unreliable.
From: Scott Wood <hidden> Date: 2009-10-05 21:46:19
Joakim Tjernlund wrote:
Benjamin Herrenschmidt [off-list ref] wrote on 05/10/2009 23:31:39:
quoted
quoted
Yes, every ld.so uses dcbX and icbi insn when relocatin code.
Even with -msecure-plt ?
hmm, maybe not. Can't remember now. But perhaps LAZY relocs still
need dcbX? Easiest is to check in uClibc. I impl. it, but was to long
time ago for me to remember. Scott are you using uClibc? I don't
think so because of the dynamic check of the cache line size
Benjamin Herrenschmidt [off-list ref] wrote on 05/10/2009 23:37:23:
On Mon, 2009-10-05 at 23:25 +0200, Joakim Tjernlund wrote:
quoted
Benjamin Herrenschmidt [off-list ref] wrote on 05/10/2009 22:17:04:
quoted
On Mon, 2009-10-05 at 14:16 +0200, Joakim Tjernlund wrote:
quoted
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 I/D TLB Error fixing accounting
- _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly
when a page has been made dirty.
Not sure here. You seem to still set those from asm. Is that necessary ?
Well, yes. head_32.S also sets ACCESSED and DIRTY from asm so why not me?
Don't look at the hash 32 code :-)
So what to look at then? :)
quoted
The basic method I use is:
TLB gets mapped with NoAccess, then the first access will trap
into TLB error, where ACCESSED will be set if permission to access
the page is OK (and RW too). On first store 8xx will trap
into DTLB error and permissions is OK, DIRTY will be set too.
Is this wrong?
quoted
From your explanation it looks ok. IE. as long as !DIRTY -> not
writeable (will fault on store) and !ACCESSED means not accessible (will
fault on any access) then you are ok.
Yes, that is what I have (tried) to do.
quoted
Do I have to trap to C for first store?
Most of the time you do anyways since the PTE isn't populated at all. At
which point, Linux will populate a PTE with DIRTY and ACCESSED already
set. It should be reasonably rare to actually fault because DIRTY and/or
ACCESSED are missing.
I tried to unconditionally trap to C in DTLB error but it just hung if I did
that so the asm has to do something.
quoted
quoted
The approach I take on BookE is to simply not set these from asm, -and-
(this is important) not set write permission if dirty is not set in
Did you really mean "if dirty is not" ? I test RW for write permission.
Dirty is just set when the first write happens after the permission check.
quoted
quoted
the TLB miss and set no access permission at all when accessed is not
set. This is important or we'll miss dirty updates which can
be fatal.
not sure, but this seems similar to what I do. DIRTY will be set,
in asm, on first store.
Don't set DIRTY if !VALID
I don't. !VALID trap to C
quoted
ACCESSED will only be set iff (USER && VALID)
My point is that you should be able to simplify the code here, have only
the TLB miss look at the PTE, not the data access and instruction
access, and have the later be a boring exception going straight to C.
I do this, TLB Miss only looks at TLB and then transforms it into a HW pte.
The HW pte do not map 1:1 so I need to some magic to fit the linux pte
into a HW pte.
quoted
quoted
The C code in handle_pte_fault() will fixup missing access and dirty
if necessary and flush.
Also look at the 440 code, I think you could simplify your
implementation using similar techniques, such as andc of PTE against
requested bits etc... and thus maybe save a couple of insns.
Great, but first I want to make sure I doing it right :)
So is there some golden rule I have to follow?
Mostly only !ACCESSED -> no access permitted and !DIRTY -> no store
permitted and don't write anything back if !VALID.
That should be !RW and not !DIRTY I hope? Then trap
first store and set DIRTY (without trapping to C)
Cheers,
Ben.
quoted
Jocke
quoted
Cheers,
Ben.
quoted
- Proper RO/RW mapping of user space.
- Free up 2 SW TLB bits in the linux pte(add back _PAGE_WRITETHRU ?)
Cons:
- 4 more instructions in I/D TLB Miss, but the since the linux pte is
not written anymore, it should still be a win.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-05 22:09:26
On Tue, 2009-10-06 at 00:00 +0200, Joakim Tjernlund wrote:
Benjamin Herrenschmidt [off-list ref] wrote on 05/10/2009 23:37:23:
quoted
On Mon, 2009-10-05 at 23:25 +0200, Joakim Tjernlund wrote:
quoted
Benjamin Herrenschmidt [off-list ref] wrote on 05/10/2009 22:17:04:
quoted
On Mon, 2009-10-05 at 14:16 +0200, Joakim Tjernlund wrote:
quoted
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 I/D TLB Error fixing accounting
- _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly
when a page has been made dirty.
Not sure here. You seem to still set those from asm. Is that necessary ?
Well, yes. head_32.S also sets ACCESSED and DIRTY from asm so why not me?
Don't look at the hash 32 code :-)
So what to look at then? :)
head_440.S on a recent 2.6
quoted
Most of the time you do anyways since the PTE isn't populated at all. At
which point, Linux will populate a PTE with DIRTY and ACCESSED already
set. It should be reasonably rare to actually fault because DIRTY and/or
ACCESSED are missing.
I tried to unconditionally trap to C in DTLB error but it just hung if I did
that so the asm has to do something.
Sure, the question is what and how can we get away without it ? :-)
quoted
quoted
quoted
The approach I take on BookE is to simply not set these from asm, -and-
(this is important) not set write permission if dirty is not set in
Did you really mean "if dirty is not" ? I test RW for write permission.
Dirty is just set when the first write happens after the permission check.
Sure but if dirty is cleared by the kernel, then you also need to remove
write permission in the TLB or it will miss setting dirty on the next
store to the page.
So dirty basically acts as a filter on RW, and accessed as a filter on
valid if you want.
quoted
Mostly only !ACCESSED -> no access permitted and !DIRTY -> no store
permitted and don't write anything back if !VALID.
That should be !RW and not !DIRTY I hope? Then trap
first store and set DIRTY (without trapping to C)
No it's really !(RW _AND_ DIRTY) -> no store permitted, and
!(PRESENT _AND_ ACCESSED) -> no access permitted.
Cheers,
Ben.
quoted
Cheers,
Ben.
quoted
Jocke
quoted
Cheers,
Ben.
quoted
- Proper RO/RW mapping of user space.
- Free up 2 SW TLB bits in the linux pte(add back _PAGE_WRITETHRU ?)
Cons:
- 4 more instructions in I/D TLB Miss, but the since the linux pte is
not written anymore, it should still be a win.
Scott and Rex, please disregard other patches from me and
try these out instead.
I have results similar to Scott's. I tried both with and without patch
5 & 6, and I also need the tlbia_va in ptep_set_access_flags().
I got this oops:
Unable to handle kernel paging request for data at address 0x00000000
Faulting instruction address: 0xc000e110
Oops: Kernel access of bad area, sig: 11 [#1]
MRV NM2
NIP: c000e110 LR: c000d520 CTR: 1006bf40
REGS: c2203dd0 TRAP: 0300 Not tainted (2.6.32-rc3-00014-gcea49b0-dirty)
MSR: 00009032 <EE,ME,IR,DR> CR: 44022422 XER: 20000000
DAR: 00000000, DSISR: c0000000
TASK = c21c18c0[69] 'rc.sysinit' THREAD: c2202000
GPR00: 00000401 c2203e80 c21c18c0 c2203f50 00000000 4000d032 00000000 00000000
GPR08: 10093eb4 00000000 00009032 c000d514 021c1ad0 10095c50 03ffb000 10006500
GPR16: 10006508 00000000 00000000 00000000 100a2fc8 021c4238 00000000 00000400
GPR24: c21a2700 00000002 40000000 00000000 00000000 c2203f50 00000000 00000011
NIP [c000e110] do_page_fault+0x44/0x5fc
LR [c000d520] handle_page_fault+0xc/0x80
Call Trace:
[c2203e80] [c000e58c] do_page_fault+0x4c0/0x5fc (unreliable)
[c2203f40] [c000d520] handle_page_fault+0xc/0x80
Instruction dump:
800300a0 7cba2b78 54170036 2f970400 7c9c2378 830200e0 54b6018c 40be000c
74ba4820 3ac00000 813d0080 3bc00000 <80a90000> 54a036be 2f80001f 40be0100
---[ end trace 99a4d88f7e2f1b60 ]---
this happens in do_page_fault becaose regs->nip is null, so
regs or regs->nip is NULL? Either one does not make sense
In any case it might be a secondary problem as DAR is NULL already when you
enter the page fault.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-05 22:37:37
On Tue, 2009-10-06 at 00:31 +0200, Joakim Tjernlund wrote:
regs or regs->nip is NULL? Either one does not make sense
In any case it might be a secondary problem as DAR is NULL already
when you
enter the page fault.
hmm, I wonder if you managed to invalidate the a kernel TLB?
Are you using pinned kernel TLBs?
You should not dereference a user address like that. Use get_user !
Obviously you got 0 in SRR0 for some reason (somebody tried to jump
to 0, either intentionally or as a result of some other problem) and
the above will crash the kernel when it happens.
Cheers,
Ben.
I got this oops:
Unable to handle kernel paging request for data at address 0x00000000
Faulting instruction address: 0xc000e110
Oops: Kernel access of bad area, sig: 11 [#1]
MRV NM2
NIP: c000e110 LR: c000d520 CTR: 1006bf40
REGS: c2203dd0 TRAP: 0300 Not tainted (2.6.32-rc3-00014-gcea49b0-dirty)
MSR: 00009032 <EE,ME,IR,DR> CR: 44022422 XER: 20000000
DAR: 00000000, DSISR: c0000000
TASK = c21c18c0[69] 'rc.sysinit' THREAD: c2202000
GPR00: 00000401 c2203e80 c21c18c0 c2203f50 00000000 4000d032 00000000 00000000
GPR08: 10093eb4 00000000 00009032 c000d514 021c1ad0 10095c50 03ffb000 10006500
GPR16: 10006508 00000000 00000000 00000000 100a2fc8 021c4238 00000000 00000400
GPR24: c21a2700 00000002 40000000 00000000 00000000 c2203f50 00000000 00000011
NIP [c000e110] do_page_fault+0x44/0x5fc
LR [c000d520] handle_page_fault+0xc/0x80
Call Trace:
[c2203e80] [c000e58c] do_page_fault+0x4c0/0x5fc (unreliable)
[c2203f40] [c000d520] handle_page_fault+0xc/0x80
Instruction dump:
800300a0 7cba2b78 54170036 2f970400 7c9c2378 830200e0 54b6018c 40be000c
74ba4820 3ac00000 813d0080 3bc00000 <80a90000> 54a036be 2f80001f 40be0100
---[ end trace 99a4d88f7e2f1b60 ]---
this happens in do_page_fault becaose regs->nip is null, so
regs or regs->nip is NULL? Either one does not make sense
In any case it might be a secondary problem as DAR is NULL already when you
enter the page fault.
I assumed it was NIP because ... I'm not sure why. TRAP() above
dereferences regs, and that didn't fail, but I didn't see that until
now.
Benjamin Herrenschmidt [off-list ref] wrote on 06/10/2009 00:09:16:
On Tue, 2009-10-06 at 00:00 +0200, Joakim Tjernlund wrote:
quoted
Benjamin Herrenschmidt [off-list ref] wrote on 05/10/2009 23:37:23:
quoted
On Mon, 2009-10-05 at 23:25 +0200, Joakim Tjernlund wrote:
quoted
Benjamin Herrenschmidt [off-list ref] wrote on 05/10/2009 22:17:04:
quoted
On Mon, 2009-10-05 at 14:16 +0200, Joakim Tjernlund wrote:
quoted
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 I/D TLB Error fixing accounting
- _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly
when a page has been made dirty.
Not sure here. You seem to still set those from asm. Is that necessary ?
Well, yes. head_32.S also sets ACCESSED and DIRTY from asm so why not me?
Don't look at the hash 32 code :-)
So what to look at then? :)
head_440.S on a recent 2.6
OK, will look
quoted
quoted
Most of the time you do anyways since the PTE isn't populated at all. At
which point, Linux will populate a PTE with DIRTY and ACCESSED already
set. It should be reasonably rare to actually fault because DIRTY and/or
ACCESSED are missing.
I tried to unconditionally trap to C in DTLB error but it just hung if I did
that so the asm has to do something.
Sure, the question is what and how can we get away without it ? :-)
You tell me :)
quoted
quoted
quoted
quoted
The approach I take on BookE is to simply not set these from asm, -and-
(this is important) not set write permission if dirty is not set in
Did you really mean "if dirty is not" ? I test RW for write permission.
Dirty is just set when the first write happens after the permission check.
Sure but if dirty is cleared by the kernel, then you also need to remove
write permission in the TLB or it will miss setting dirty on the next
store to the page.
Dirty map to HW changed bit so if kernel clears it, the next
store will trap to DTLB error. There I will check RW and clear it again
without trapping to C. Is that OK? Not if I read you correcly below ..
So dirty basically acts as a filter on RW, and accessed as a filter on
valid if you want.
quoted
quoted
Mostly only !ACCESSED -> no access permitted and !DIRTY -> no store
permitted and don't write anything back if !VALID.
That should be !RW and not !DIRTY I hope? Then trap
first store and set DIRTY (without trapping to C)
No it's really !(RW _AND_ DIRTY) -> no store permitted, and
.. hmm, I don't do that. I should do a
if ( store && !(pte & (RW | DIRTY))
goto DSI
in DTLB error?
!(PRESENT _AND_ ACCESSED) -> no access permitted.
Yes, how about those pinned kernel ptes. Do they have ACCESSED from start?
Cheers,
Ben.
quoted
quoted
Cheers,
Ben.
quoted
Jocke
quoted
Cheers,
Ben.
quoted
- Proper RO/RW mapping of user space.
- Free up 2 SW TLB bits in the linux pte(add back _PAGE_WRITETHRU ?)
Cons:
- 4 more instructions in I/D TLB Miss, but the since the linux pte is
not written anymore, it should still be a win.
I got this oops:
Unable to handle kernel paging request for data at address 0x00000000
Faulting instruction address: 0xc000e110
Oops: Kernel access of bad area, sig: 11 [#1]
MRV NM2
NIP: c000e110 LR: c000d520 CTR: 1006bf40
REGS: c2203dd0 TRAP: 0300 Not tainted (2.6.32-rc3-00014-gcea49b0-dirty)
MSR: 00009032 <EE,ME,IR,DR> CR: 44022422 XER: 20000000
DAR: 00000000, DSISR: c0000000
TASK = c21c18c0[69] 'rc.sysinit' THREAD: c2202000
GPR00: 00000401 c2203e80 c21c18c0 c2203f50 00000000 4000d032 00000000 00000000
GPR08: 10093eb4 00000000 00009032 c000d514 021c1ad0 10095c50 03ffb000 10006500
GPR16: 10006508 00000000 00000000 00000000 100a2fc8 021c4238 00000000 00000400
GPR24: c21a2700 00000002 40000000 00000000 00000000 c2203f50 00000000 00000011
NIP [c000e110] do_page_fault+0x44/0x5fc
LR [c000d520] handle_page_fault+0xc/0x80
Call Trace:
[c2203e80] [c000e58c] do_page_fault+0x4c0/0x5fc (unreliable)
[c2203f40] [c000d520] handle_page_fault+0xc/0x80
Instruction dump:
800300a0 7cba2b78 54170036 2f970400 7c9c2378 830200e0 54b6018c 40be000c
74ba4820 3ac00000 813d0080 3bc00000 <80a90000> 54a036be 2f80001f 40be0100
---[ end trace 99a4d88f7e2f1b60 ]---
this happens in do_page_fault becaose regs->nip is null, so
regs or regs->nip is NULL? Either one does not make sense
In any case it might be a secondary problem as DAR is NULL already when you
enter the page fault.
I assumed it was NIP because ... I'm not sure why. TRAP() above
dereferences regs, and that didn't fail, but I didn't see that until
now.
It probably was and I need to use get_user() instead.
Benjamin Herrenschmidt [off-list ref] wrote on 06/10/2009 00:37:28:
On Tue, 2009-10-06 at 00:31 +0200, Joakim Tjernlund wrote:
quoted
regs or regs->nip is NULL? Either one does not make sense
In any case it might be a secondary problem as DAR is NULL already
when you
enter the page fault.
hmm, I wonder if you managed to invalidate the a kernel TLB?
Are you using pinned kernel TLBs?
You should not dereference a user address like that. Use get_user !
Ah, forgot about that. Will change
Obviously you got 0 in SRR0 for some reason (somebody tried to jump
to 0, either intentionally or as a result of some other problem) and
the above will crash the kernel when it happens.
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-05 23:15:49
On Tue, 2009-10-06 at 00:55 +0200, Joakim Tjernlund wrote:
quoted
Sure but if dirty is cleared by the kernel, then you also need to
remove
quoted
write permission in the TLB or it will miss setting dirty on the
next
quoted
store to the page.
Dirty map to HW changed bit so if kernel clears it, the next
store will trap to DTLB error. There I will check RW and clear it
again
without trapping to C. Is that OK? Not if I read you correcly below ..
Well, if the HW has the ability to enforce trap when store with !DIRTY,
then that's fine, just map it that way, but you shouldn't have to handle
it in the DTLB error neither, the kernel will fix it up for you in
handle_pte_fault().
I think I need to get myself an 8xx manual and figure out what that damn
MMU actually does :-)
No it's really !(RW _AND_ DIRTY) -> no store permitted, and
.. hmm, I don't do that. I should do a
if ( store && !(pte & (RW | DIRTY))
goto DSI
in DTLB error?
Well, if the HW does the test of DIRTY, then it's fine, as you seem to
suggest above. _something_ needs to do it, either SW or HW, ie, we need
to catch any attempt to store to a non-dirty page to set dirty.
quoted
!(PRESENT _AND_ ACCESSED) -> no access permitted.
Yes, how about those pinned kernel ptes. Do they have ACCESSED from
start?
Benjamin Herrenschmidt [off-list ref] wrote on 06/10/2009 01:15:39:
On Tue, 2009-10-06 at 00:55 +0200, Joakim Tjernlund wrote:
quoted
quoted
Sure but if dirty is cleared by the kernel, then you also need to
remove
quoted
write permission in the TLB or it will miss setting dirty on the
next
quoted
store to the page.
Dirty map to HW changed bit so if kernel clears it, the next
store will trap to DTLB error. There I will check RW and clear it
again
without trapping to C. Is that OK? Not if I read you correcly below ..
Well, if the HW has the ability to enforce trap when store with !DIRTY,
Yes, provided that the kernel invalidates the TLB too so the next access
will provoke a TLB Miss, which will then provoke a TLB error. The TLB
error routine checks VALID, RW and USER(if not a kernel access), then sets
ACCESSED & DIRTY and writes the TLB(RPN reg).
Perhaps the missing invalidate is haunting us here?
then that's fine, just map it that way, but you shouldn't have to handle
it in the DTLB error neither, the kernel will fix it up for you in
handle_pte_fault().
Does not all ppc have the Changed bit?
I think I need to get myself an 8xx manual and figure out what that damn
MMU actually does :-)
Please do, get the mpc862 users manual :)
quoted
No it's really !(RW _AND_ DIRTY) -> no store permitted, and
.. hmm, I don't do that. I should do a
if ( store && !(pte & (RW | DIRTY))
goto DSI
in DTLB error?
Well, if the HW does the test of DIRTY, then it's fine, as you seem to
suggest above. _something_ needs to do it, either SW or HW, ie, we need
to catch any attempt to store to a non-dirty page to set dirty.
Yes, I think I do this.
quoted
quoted
!(PRESENT _AND_ ACCESSED) -> no access permitted.
Yes, how about those pinned kernel ptes. Do they have ACCESSED from
start?
Benjamin Herrenschmidt [off-list ref] wrote on 06/10/2009 00:37:28:
On Tue, 2009-10-06 at 00:31 +0200, Joakim Tjernlund wrote:
quoted
regs or regs->nip is NULL? Either one does not make sense
In any case it might be a secondary problem as DAR is NULL already
when you
enter the page fault.
@@ -153,7 +153,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,#ifdef DEBUG_DCBXconstchar*istr=NULL;-insn=*((unsignedlong*)regs->nip);+__get_user(insn,(unsignedlong__user*)regs->nip);if(((insn>>(31-5))&0x3f)==31){if(((insn>>1)&0x3ff)==1014)/* dcbz ? 0x3f6 */istr="dcbz";
@@ -178,11 +178,12 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,ra,rb,dar);is_write=0;}-+#if 0if(trap==0x300&&address!=dar){__asm__("mtdar %0"::"r"(dar));return0;}+#endif}}#endif
@@ -191,7 +192,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,/* This is from a dcbX or icbi insn gone bad, these*insndonotsetDARsowehavetodoithereinstead*/-insn=*((unsignedlong*)regs->nip);+__get_user(insn,(unsignedlong__user*)regs->nip);ra=(insn>>(31-15))&0x1f;/* Reg RA */rb=(insn>>(31-20))&0x1f;/* Reg RB */
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-06 00:34:24
On Tue, 2009-10-06 at 01:35 +0200, Joakim Tjernlund wrote:
quoted
Well, if the HW has the ability to enforce trap when store with !
DIRTY,
Yes, provided that the kernel invalidates the TLB too so the next
access
will provoke a TLB Miss, which will then provoke a TLB error. The TLB
error routine checks VALID, RW and USER(if not a kernel access), then
sets
ACCESSED & DIRTY and writes the TLB(RPN reg).
Perhaps the missing invalidate is haunting us here?
No, the kernel will invalidate when clearing dirty or accessed, I don't
think that's our problem.
This is still all inefficient, we end up basically with two traps.
8xx provides backup GPRs when doing TLB misses ? What does it cost to
jump out of a TLB miss back into "normal" context ?
IE. What I do on 440 is I set a mask of required bits, basically
_PAGE_PRESENT | _PAGE_ACCESSED is the base. The DTLB miss also sticks
in _PAGE_RW | _PAGE_DIRTY when it's a store fault.
Then, I andc. the PTE value off that mask, and if the result is non-0
(which means one of the required bits is missing), I get out of the TLB
miss immediately and go to the data (or instruction) access interrupt.
Once you've done that, you should be able to have data and instruction
access go straight to C. Missing _PAGE_ACCESSED and _PAGE_DIRTY are
going to be fixed up by generic code.
quoted
then that's fine, just map it that way, but you shouldn't have to
handle
quoted
it in the DTLB error neither, the kernel will fix it up for you in
handle_pte_fault().
@@ -153,7 +153,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,#ifdef DEBUG_DCBXconstchar*istr=NULL;-insn=*((unsignedlong*)regs->nip);+__get_user(insn,(unsignedlong__user*)regs->nip);
No, use get_user() not __get_user() or if you use the later, also use
access_ok(), and test the result in case it errors (if it does, you
probably want to just goto bad access and SEGV).
Cheers,
Ben.
@@ -191,7 +192,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address, /* This is from a dcbX or icbi insn gone bad, these * insn do not set DAR so we have to do it here instead */- insn = *((unsigned long *)regs->nip);+ __get_user(insn, (unsigned long __user *)regs->nip); ra = (insn >> (31-15)) & 0x1f; /* Reg RA */ rb = (insn >> (31-20)) & 0x1f; /* Reg RB */
Benjamin Herrenschmidt [off-list ref] wrote on 06/10/2009 02:34:15:
On Tue, 2009-10-06 at 01:35 +0200, Joakim Tjernlund wrote:
quoted
quoted
Well, if the HW has the ability to enforce trap when store with !
DIRTY,
Yes, provided that the kernel invalidates the TLB too so the next
access
will provoke a TLB Miss, which will then provoke a TLB error. The TLB
error routine checks VALID, RW and USER(if not a kernel access), then
sets
ACCESSED & DIRTY and writes the TLB(RPN reg).
Perhaps the missing invalidate is haunting us here?
No, the kernel will invalidate when clearing dirty or accessed, I don't
think that's our problem.
This is still all inefficient, we end up basically with two traps.
Yes, but once the 2 traps is over, it gets much cheaper plus I don't
get a choice, see below.
8xx provides backup GPRs when doing TLB misses ? What does it cost to
jump out of a TLB miss back into "normal" context ?
Nope, there is just one TLB scratch register. I have been meaning to
ask you about SPRG2, it seems unused?
There is a leftover from 2.4 that inits G2 to something but the
it appears unused otherwise.
IE. What I do on 440 is I set a mask of required bits, basically
_PAGE_PRESENT | _PAGE_ACCESSED is the base. The DTLB miss also sticks
in _PAGE_RW | _PAGE_DIRTY when it's a store fault.
Yes, I would too but TLB Miss knows nothing about load/store, protection etc.
because DSISR isn't set. So I cannot see any other way than the TLB Error way.
Then, I andc. the PTE value off that mask, and if the result is non-0
(which means one of the required bits is missing), I get out of the TLB
miss immediately and go to the data (or instruction) access interrupt.
Once you've done that, you should be able to have data and instruction
access go straight to C. Missing _PAGE_ACCESSED and _PAGE_DIRTY are
going to be fixed up by generic code.
quoted
quoted
then that's fine, just map it that way, but you shouldn't have to
handle
quoted
it in the DTLB error neither, the kernel will fix it up for you in
handle_pte_fault().
Does not all ppc have the Changed bit?
No. BookE doesn't.
But I guess BookE knows if it is a load or store in TLB Miss?
Then it can emulate changed bit I guess.
I got this oops:
Unable to handle kernel paging request for data at address 0x00000000
Faulting instruction address: 0xc000e110
Oops: Kernel access of bad area, sig: 11 [#1]
MRV NM2
NIP: c000e110 LR: c000d520 CTR: 1006bf40
REGS: c2203dd0 TRAP: 0300 Not tainted (2.6.32-rc3-00014-gcea49b0-dirty)
MSR: 00009032 <EE,ME,IR,DR> CR: 44022422 XER: 20000000
DAR: 00000000, DSISR: c0000000
TASK = c21c18c0[69] 'rc.sysinit' THREAD: c2202000
GPR00: 00000401 c2203e80 c21c18c0 c2203f50 00000000 4000d032 00000000 00000000
GPR08: 10093eb4 00000000 00009032 c000d514 021c1ad0 10095c50 03ffb000 10006500
GPR16: 10006508 00000000 00000000 00000000 100a2fc8 021c4238 00000000 00000400
GPR24: c21a2700 00000002 40000000 00000000 00000000 c2203f50 00000000 00000011
NIP [c000e110] do_page_fault+0x44/0x5fc
LR [c000d520] handle_page_fault+0xc/0x80
Call Trace:
[c2203e80] [c000e58c] do_page_fault+0x4c0/0x5fc (unreliable)
[c2203f40] [c000d520] handle_page_fault+0xc/0x80
Instruction dump:
800300a0 7cba2b78 54170036 2f970400 7c9c2378 830200e0 54b6018c 40be000c
74ba4820 3ac00000 813d0080 3bc00000 <80a90000> 54a036be 2f80001f 40be0100
---[ end trace 99a4d88f7e2f1b60 ]---
this happens in do_page_fault becaose regs->nip is null, so
regs or regs->nip is NULL? Either one does not make sense
In any case it might be a secondary problem as DAR is NULL already when you
enter the page fault.
I assumed it was NIP because ... I'm not sure why. TRAP() above
dereferences regs, and that didn't fail, but I didn't see that until
now.
Yes, it is a bit strange though that the kernel isn't allowed to read from
NULL. Is that expected?
Jocke
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-06 06:44:55
quoted
I assumed it was NIP because ... I'm not sure why. TRAP() above
dereferences regs, and that didn't fail, but I didn't see that until
now.
Yes, it is a bit strange though that the kernel isn't allowed to read from
NULL. Is that expected?
Yes, that's absolutely expected :-) You really want any NULL deref to
blow up asap.
0 is actually part of the address space assigned to user processes. By
default they don't have anything there neither though, but if the
current process do have something mapped there, then the kernel would
read that when doing a NULL deref.
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-06 06:45:56
On Tue, 2009-10-06 at 08:15 +0200, Joakim Tjernlund wrote:
Yes, I would too but TLB Miss knows nothing about load/store, protection etc.
because DSISR isn't set. So I cannot see any other way than the TLB Error way.
Hrm... that MMU really sucks more than I thought :-(
I'll go read the manual and think about that a bit more.
But I guess BookE knows if it is a load or store in TLB Miss?
Then it can emulate changed bit I guess.
Benjamin Herrenschmidt [off-list ref] wrote on 06/10/2009 08:45:47:
On Tue, 2009-10-06 at 08:15 +0200, Joakim Tjernlund wrote:
quoted
Yes, I would too but TLB Miss knows nothing about load/store, protection etc.
because DSISR isn't set. So I cannot see any other way than the TLB Error way.
Hrm... that MMU really sucks more than I thought :-(
Yeah, but at least it got a Changed bit as opposed to some other
arch mention in this mail thread :)
I'll go read the manual and think about that a bit more.
Thanks
I wonder if not much of the problems Scott and Rex are seeing are from
dcbX insn and that my fixup isn't quite working yet. I am tempted
to get the asm version working again as then I don't have to worry about
permission and get_user() et. all :)
@@ -153,7 +153,7 @@ int __kprobes do_page_fault(struct pt_regs *regs,
unsigned long address,
quoted
#ifdef DEBUG_DCBX
const char *istr = NULL;
- insn = *((unsigned long *)regs->nip);
+ __get_user(insn, (unsigned long __user *)regs->nip);
No, use get_user() not __get_user() or if you use the later, also use
access_ok(), and test the result in case it errors (if it does, you
probably want to just goto bad access and SEGV).
@@ -153,7 +153,8 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,#ifdef DEBUG_DCBXconstchar*istr=NULL;-insn=*((unsignedlong*)regs->nip);+insn=0;+__get_user(insn,(unsignedlong__user*)regs->nip);if(((insn>>(31-5))&0x3f)==31){if(((insn>>1)&0x3ff)==1014)/* dcbz ? 0x3f6 */istr="dcbz";
@@ -171,27 +172,32 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,dar=regs->gpr[rb];if(ra)dar+=regs->gpr[ra];-if(dar!=address&&address!=0x00f0&&trap==0x300)+if(dar!=address&&trap==0x300)printk(KERN_CRIT"%s: address:%lx, dar:%lx!\n",istr,address,dar);if(!strcmp(istr,"dcbst")&&is_write){printk(KERN_CRIT"dcbst R%ld,R%ld = %lx as a store, fixing!\n",ra,rb,dar);is_write=0;}-+#if 0if(trap==0x300&&address!=dar){__asm__("mtdar %0"::"r"(dar));return0;}+#endif}}#endifif(address==0x00f0&&trap==0x300){-pte_t*ptep;+//pte_t *ptep;/* This is from a dcbX or icbi insn gone bad, these*insndonotsetDARsowehavetodoithereinstead*/-insn=*((unsignedlong*)regs->nip);+if(get_user(insn,(unsignedlong__user*)regs->nip)){+printk(KERN_CRIT"get_user failed, NIP:%lx\n",+regs->nip);+gotobad_area_nosemaphore;+}ra=(insn>>(31-15))&0x1f;/* Reg RA */rb=(insn>>(31-20))&0x1f;/* Reg RB */
@@ -206,7 +212,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,trap,address,dar,error_code,istr);#endifaddress=dar;-#if 1+#if 0if(is_write&&get_pteptr(mm,dar,&ptep,NULL)){pte_tmy_pte=*ptep;
@@ -216,7 +222,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,}}#else-return0;+//return 0;#endif}}
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-06 08:32:58
quoted
No, use get_user() not __get_user() or if you use the later, also use
access_ok(), and test the result in case it errors (if it does, you
probably want to just goto bad access and SEGV).
@@ -153,7 +153,8 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,#ifdef DEBUG_DCBXconstchar*istr=NULL;-insn=*((unsignedlong*)regs->nip);+insn=0;+__get_user(insn,(unsignedlong__user*)regs->nip);if(((insn>>(31-5))&0x3f)==31){if(((insn>>1)&0x3ff)==1014)/* dcbz ? 0x3f6 */istr="dcbz";
@@ -171,27 +172,32 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,dar=regs->gpr[rb];if(ra)dar+=regs->gpr[ra];-if(dar!=address&&address!=0x00f0&&trap==0x300)+if(dar!=address&&trap==0x300)printk(KERN_CRIT"%s: address:%lx, dar:%lx!\n",istr,address,dar);if(!strcmp(istr,"dcbst")&&is_write){printk(KERN_CRIT"dcbst R%ld,R%ld = %lx as a store, fixing!\n",ra,rb,dar);is_write=0;}-+#if 0if(trap==0x300&&address!=dar){__asm__("mtdar %0"::"r"(dar));return0;}+#endif}}#endifif(address==0x00f0&&trap==0x300){-pte_t*ptep;+//pte_t *ptep;/* This is from a dcbX or icbi insn gone bad, these*insndonotsetDARsowehavetodoithereinstead*/-insn=*((unsignedlong*)regs->nip);+if(get_user(insn,(unsignedlong__user*)regs->nip)){+printk(KERN_CRIT"get_user failed, NIP:%lx\n",+regs->nip);+gotobad_area_nosemaphore;+}ra=(insn>>(31-15))&0x1f;/* Reg RA */rb=(insn>>(31-20))&0x1f;/* Reg RB */
@@ -206,7 +212,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,trap,address,dar,error_code,istr);#endifaddress=dar;-#if 1+#if 0if(is_write&&get_pteptr(mm,dar,&ptep,NULL)){pte_tmy_pte=*ptep;
@@ -216,7 +222,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,}}#else-return0;+//return 0;#endif}}
No, use get_user() not __get_user() or if you use the later, also use
access_ok(), and test the result in case it errors (if it does, you
probably want to just goto bad access and SEGV).
@@ -171,27 +172,32 @@ int __kprobes do_page_fault(struct pt_regs *regs,
unsigned long address,
quoted
dar = regs->gpr[rb];
if (ra)
dar += regs->gpr[ra];
- if (dar != address && address != 0x00f0 && trap == 0x300)
+ if (dar != address && trap == 0x300)
printk(KERN_CRIT "%s: address:%lx, dar:%lx!\n", istr, address, dar);
if (!strcmp(istr, "dcbst") && is_write) {
printk(KERN_CRIT "dcbst R%ld,R%ld = %lx as a store, fixing!\n",
ra, rb, dar);
is_write = 0;
}
-
+#if 0
if (trap == 0x300 && address != dar) {
__asm__ ("mtdar %0" : : "r" (dar));
return 0;
}
+#endif
}
}
#endif
if (address == 0x00f0 && trap == 0x300) {
- pte_t *ptep;
+ //pte_t *ptep;
/* This is from a dcbX or icbi insn gone bad, these
* insn do not set DAR so we have to do it here instead */
- insn = *((unsigned long *)regs->nip);
+ if (get_user(insn, (unsigned long __user *)regs->nip)) {
+ printk(KERN_CRIT "get_user failed, NIP:%lx\n",
+ regs->nip);
+ goto bad_area_nosemaphore;
+ }
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-06 11:06:35
On Tue, 2009-10-06 at 12:58 +0200, Joakim Tjernlund wrote:
Here I don't care if err. insn will be 0 if it fails and the following
if will be false
I'd rather you use get_user() so it does access_ok().
Else, you can probably manufacture some code that will make the kernel
access some MMIO register for example, which could be nasty.
At this point, you may as well also check the result even if indeed a
fault isn't going to matter. Just makes the code cleaner and avoids some
random janitor coming up with a patch later on :-)
Cheers,
Ben.
@@ -171,27 +172,32 @@ int __kprobes do_page_fault(struct pt_regs *regs,
unsigned long address,
quoted
dar = regs->gpr[rb];
if (ra)
dar += regs->gpr[ra];
- if (dar != address && address != 0x00f0 && trap == 0x300)
+ if (dar != address && trap == 0x300)
printk(KERN_CRIT "%s: address:%lx, dar:%lx!\n", istr, address, dar);
if (!strcmp(istr, "dcbst") && is_write) {
printk(KERN_CRIT "dcbst R%ld,R%ld = %lx as a store, fixing!\n",
ra, rb, dar);
is_write = 0;
}
-
+#if 0
if (trap == 0x300 && address != dar) {
__asm__ ("mtdar %0" : : "r" (dar));
return 0;
}
+#endif
}
}
#endif
if (address == 0x00f0 && trap == 0x300) {
- pte_t *ptep;
+ //pte_t *ptep;
/* This is from a dcbX or icbi insn gone bad, these
* insn do not set DAR so we have to do it here instead */
- insn = *((unsigned long *)regs->nip);
+ if (get_user(insn, (unsigned long __user *)regs->nip)) {
+ printk(KERN_CRIT "get_user failed, NIP:%lx\n",
+ regs->nip);
+ goto bad_area_nosemaphore;
+ }
Benjamin Herrenschmidt [off-list ref] wrote on 06/10/2009 13:06:26:
From:
Benjamin Herrenschmidt [off-list ref]
To:
Joakim Tjernlund [off-list ref]
Cc:
"linuxppc-dev@ozlabs.org" [off-list ref], Rex Feany
[off-list ref], Scott Wood [off-list ref]
Date:
06/10/2009 13:06
Subject:
Re: [PATCH 0/6] PowerPc 8xx TLB/MMU fixes
On Tue, 2009-10-06 at 12:58 +0200, Joakim Tjernlund wrote:
quoted
Here I don't care if err. insn will be 0 if it fails and the following
if will be false
I'd rather you use get_user() so it does access_ok().
It is only debug code so it will go away
The real access is later.
I need do kernel space separately, is there a better way than:
if (regs->nip & 0xc0000000) /* kernel space ? */
insn = *((unsigned long *)regs->nip);
else if (get_user(insn, (unsigned long *)regs->nip)) {
printk(KERN_CRIT "get_user! NIP:%lx\n", regs->nip); /* DEBUG */
goto bad_area_nosemaphore;
}
Benjamin Herrenschmidt [off-list ref] wrote on 06/10/2009 13:06:26:
On Tue, 2009-10-06 at 12:58 +0200, Joakim Tjernlund wrote:
quoted
Here I don't care if err. insn will be 0 if it fails and the following
if will be false
I'd rather you use get_user() so it does access_ok().
Else, you can probably manufacture some code that will make the kernel
access some MMIO register for example, which could be nasty.
At this point, you may as well also check the result even if indeed a
fault isn't going to matter. Just makes the code cleaner and avoids some
random janitor coming up with a patch later on :-)
Cheers,
Ben.
So my user space access was bust. Try slapping this on top
It might be that my crappy user space handling also tripped the
asm version, would be great if you could try that one again too.
I suspect that you both, Rex and Scott, have dcbX/icbi insn's in user
space that trip DTLB errors, that would explain everything I think.
Jocke
@@ -152,8 +152,16 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,unsignedlongra,rb,dar,insn;#ifdef DEBUG_DCBXconstchar*istr=NULL;+intret;++insn=0;+if(user_mode(regs)){+if((ret=get_user(insn,(unsignedlong__user*)regs->nip)))+printk(KERN_CRIT"get_user:NIP:0x%08lx, err:%d\n",+regs->nip,ret);+}else+insn=*((unsignedlong*)regs->nip);-insn=*((unsignedlong*)regs->nip);if(((insn>>(31-5))&0x3f)==31){if(((insn>>1)&0x3ff)==1014)/* dcbz ? 0x3f6 */istr="dcbz";
@@ -178,20 +186,27 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,ra,rb,dar);is_write=0;}-+#if 0if(trap==0x300&&address!=dar){__asm__("mtdar %0"::"r"(dar));return0;}+#endif}}#endifif(address==0x00f0&&trap==0x300){-pte_t*ptep;-+intret;/* This is from a dcbX or icbi insn gone bad, these*insndonotsetDARsowehavetodoithereinstead*/-insn=*((unsignedlong*)regs->nip);+if(user_mode(regs)){+if((ret=get_user(insn,(unsignedlong__user*)regs->nip))){+printk(KERN_CRIT"get_user:NIP:%lx, err:%d\n",+regs->nip,ret);+gotobad_area_nosemaphore;+}+}else+insn=*((unsignedlong*)regs->nip);ra=(insn>>(31-15))&0x1f;/* Reg RA */rb=(insn>>(31-20))&0x1f;/* Reg RB */
@@ -206,18 +221,6 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,trap,address,dar,error_code,istr);#endifaddress=dar;-#if 1-if(is_write&&get_pteptr(mm,dar,&ptep,NULL)){-pte_tmy_pte=*ptep;--if(pte_present(my_pte)&&pte_write(my_pte)){-pte_val(my_pte)|=_PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_HWWRITE;-set_pte_at(mm,dar,ptep,my_pte);-}-}-#else-return0;-#endif}}#endif
Benjamin Herrenschmidt [off-list ref] wrote on 06/10/2009 08:45:47:
On Tue, 2009-10-06 at 08:15 +0200, Joakim Tjernlund wrote:
quoted
Yes, I would too but TLB Miss knows nothing about load/store, protection etc.
because DSISR isn't set. So I cannot see any other way than the TLB Error way.
Hrm... that MMU really sucks more than I thought :-(
I'll go read the manual and think about that a bit more.
I realise now that for an ITLB Miss I can work out everything I need.
an ITLB must be a read en everything else is in the linux pte.
Just check present and user, add accessed if not already there.
I blame that on too little sleep :)
I don't think I will dwell on that ATM.
Jocke
Benjamin Herrenschmidt [off-list ref] wrote on 06/10/2009 08:45:47:
quoted
On Tue, 2009-10-06 at 08:15 +0200, Joakim Tjernlund wrote:
quoted
Yes, I would too but TLB Miss knows nothing about load/store, protection etc.
because DSISR isn't set. So I cannot see any other way than the TLB Error way.
Hrm... that MMU really sucks more than I thought :-(
I'll go read the manual and think about that a bit more.
I realise now that for an ITLB Miss I can work out everything I need.
an ITLB must be a read en everything else is in the linux pte.
Just check present and user, add accessed if not already there.
I blame that on too little sleep :)
bugger, more bugs I think. Need to fix the TLB code some more
Benjamin Herrenschmidt [off-list ref] wrote on 06/10/2009 02:34:15:
On Tue, 2009-10-06 at 01:35 +0200, Joakim Tjernlund wrote:
quoted
quoted
Well, if the HW has the ability to enforce trap when store with !
DIRTY,
Yes, provided that the kernel invalidates the TLB too so the next
access
will provoke a TLB Miss, which will then provoke a TLB error. The TLB
error routine checks VALID, RW and USER(if not a kernel access), then
sets
ACCESSED & DIRTY and writes the TLB(RPN reg).
Perhaps the missing invalidate is haunting us here?
No, the kernel will invalidate when clearing dirty or accessed, I don't
think that's our problem.
This is still all inefficient, we end up basically with two traps.
8xx provides backup GPRs when doing TLB misses ? What does it cost to
jump out of a TLB miss back into "normal" context ?
IE. What I do on 440 is I set a mask of required bits, basically
_PAGE_PRESENT | _PAGE_ACCESSED is the base. The DTLB miss also sticks
in _PAGE_RW | _PAGE_DIRTY when it's a store fault.
After some more thinking I don't think I do TLB Miss/Error correctly yet.
The problem is ACCESSED. Since I don't know if load or store in TLB Miss
I must choose:
- Assume load and do what you do above. That will incorrectly
set ACCESSED on store ops when mapped as RO(plus whatever more I haven't thought about yet)
- Trap to TLB Error and do the above. That will set ACCESSED correctly
but won't trap kernel space so these remain what they are.
Is anything depending on ACCESSED for kernel pages?
if so, what if we set ACCESSED on all kernel pages when mapping them at boot?
Will SWAP or some other service(accounting?) not like that?
Finally, why do you need to include DIRTY when a store OP?
Do you need to do COW before dirtying the page?
Seems to work for me to just set DIRTY in TLB Error if RW is set too
and not trap to C.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-06 23:26:11
After some more thinking I don't think I do TLB Miss/Error correctly yet.
The problem is ACCESSED. Since I don't know if load or store in TLB Miss
I must choose:
- Assume load and do what you do above. That will incorrectly
set ACCESSED on store ops when mapped as RO(plus whatever more I haven't thought about yet)
- Trap to TLB Error and do the above. That will set ACCESSED correctly
but won't trap kernel space so these remain what they are.
Is anything depending on ACCESSED for kernel pages?
if so, what if we set ACCESSED on all kernel pages when mapping them at boot?
Will SWAP or some other service(accounting?) not like that?
Kernel pages always have ACCESSED set and never clear it.
I think the only solution here is that if -anything- doesn't look right,
the TLB miss should create one of those "unpopulated" entries and we
need to make sure they are properly invalidated in the subsequent fault
path. I'll read the 8xx doco asap to make sure I get it right.
Finally, why do you need to include DIRTY when a store OP?
Do you need to do COW before dirtying the page?
Seems to work for me to just set DIRTY in TLB Error if RW is set too
and not trap to C.
You can, but what about a load ? That shouldn't set dirty. So if you set
dirty only on stores, then a load will bring in a page without dirty,
you need to make sure you get another TLB error when writing to it so
you get a chance to set dirty.
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-07 01:07:50
Allright, did a bit of reading of doco and code..
Doco isn't totally clear though. At some stage, it -hints- that in case
of a TLB "error" (match on EA/ASID but incorrect
protection/valid/changed/...) the offending TLB entry is automatically
invalidated. Do you know if that is correct ?
I would hope so since we never do anything to remove the "invalid"
entries we write to the TLB when hitting non-present PTEs but then, that
may also explain some of our problems...
Now, a few comments from what I read in the code:
- The whole writeback could be avoided in Instruction/Data TLB miss,
but for that, you need to make sure that the TLB entry we create has
valid set only if -both- present and accessed are set. That would save
in the case of CONFIG_SWAP, a store and a read back of TWC I suppose,
but you do need to find a way to do that ANDing of ACCESSED and PRESENT.
- I think we can get rid of HWWRITE. We can make CHANGED be the current
HWWRITE value, I agree with you, which matches the HW changed bit. We
need to be a bit careful of how we setup the PP bits tho. At this stage,
I see several approaches:
* One is to basically "generate" the right PP bits based on a
combination of _PAGE_USER and _PAGE_RW. That's the simpler approach but
probably uses more code in the TLB miss handler. It would look like
that, with MxCTR:PPCS=0
_PAGE_USER _PAGE_RW PP (bits 20..27)
0 0 011C1111 (C is _PAGE_DIRTY)
0 1 000C1111
1 0 110C1111
1 1 100C1111
One easy way to do that is to have _PAGE_USER and _PAGE_RW sit next to
each other in bit position 28 and 29 (0xc). Load a GPR with something
like 0110000011001000 and rotate it left by PTE & 0xc, then move the
resulting 3 bits into position, or something along those lines. You can
also give up on kernel read-only support and go down to 2 PP bits and
never use the extended encoding.
* Another one is to use MxCTR:PPCS=1 a mask of 100C1U00 (U is
_PAGE_USER) and or in ^_PAGE_RW (it could actually be made
reverse-polarity in the PTE but that would mean some changes to non-8xx
specific headers, so let's avoid it for now at least).
At least that's the best options I can see from my reading of the doco,
though it's not totally clear to me what really happens when doing the
PPCS trick, will it generate a TLB error on a non-match or will it try
to TLB miss, which could be bad.
* Last but not least, it wouldn't be hard to use either of the above
encodings, and have the PTE actually contain the right bit combination
already. You don't need to have a _PAGE_RW, you don't need to have a
_PAGE_USER :-) Look at how I do things for book3e, where I layout the
6 BookE protection bit directly in the PTE. That's a bit harder and maybe
will need subtle changes to pte-common.h and such to accomodate it, and
so probably something to experiment with as a second step, but it's the
most efficient approach in the long run for obvious reasons.
- I think we could have more bits pre-set to the right values in the
PTE, look how we play with defining some of the constants in
pte-common.h, might be worth having a look, but -after- we have
something that works :-)
- Possible bug: I'm very disturbed by the fact that DataTLBError sets
HWWRITE and DIRTY on a non-present PTE. It should not. Just like
ACCESSED. That's going to cause trouble and swap corruption, even more
as we move DIRTY around.
- Maybe we can completely remove that mucking around with dirty,
setting of accessed etc... from DataTLBError. Just make it go to C code
just like InstructionAccess, as we discussed earlier, the generic code
will fix it up and we'll speed up page faults. It should be fairly rare
to take a fault due to a missing _PAGE_ACCESSED or _PAGE_DIRTY in any
case, so all that fixup in those exceptions is just overhead.
- Later on, if it rocks your boat, you may want to look into removing
the mucking around with swapper_pg_dir in the TLB miss. Instead, what
you can do is lazily copy the kernel PMDs into the user PMDs. IE. On the
first kernel access from a new user context, you fault, and from the
do_page_fault() code, we can detect that and fixup the user PMD to point
to the kernel page tables, thus avoiding that whole bunch of code in the
TLB miss. When creating new user page tables, we can pre-fill the kernel
PMDs too. I think x86 does that. We could do that for BookE too, though
it's less of a win since we have to fixup the TID in MMUCR/MAS but for
8xx it would save a few instructions & conditionals in the TLB miss fast
path.
- I still have problems with the comment next to the "workaround"
tlbil_va() we have in the icache/dcache flush. It doesn't make much
sense to me since dcbst is going to be done by the kernel on a kernel
address, not a user address, so I don't see how the thing we invalidate
relates to the flush we do just below.... So that raises a few
questions:
* If this is to avoid write faults on kernel pages, then we may just
want to have a fixup in do_page_fault() to ignore them when coming from
flush_dcache_icache_* using the exception table mechanism instead ?
* I don't see how it works around user faults but maybe you have a
clear scenario in mind
* It appears still reasonably obvious that we need that tlbil_va
somewhere in that fault path, but I don't know what for, I'm not sure
it's really what the comment says it's about. That raises back the whole
question of when are those "invalid" TLB entries that we create when
_PAGE_PRESENT is not set are going to be removed. The doc hints that
they go a way on TLB error, but the doc really only says so for missing
changed bit... so I'm tempted to say we need to basically stick a
tlbil_va on 8xx on both set_pte_at() and ptep_set_access_flags() for any
non-kernel page. What do you think ? That would be the safest.
- The "fake" DataAccess and InstructionAccess are useless... just a
spurious jump for nothing. We could implement those straight in
DataTLBError and InstructionTLBError, we just need to make sure that
the trap numbers we put in the exception frame are 0x300 and 0x400, but
that's just a matter of passing the right bits to EXC_XFER_EE_LITE().
- Finally, another boat-rocking optimisation to do is preload. You
could hook in update_mmu_cache() for 8xx to go whack an entry in the TLB
as well, which would avoid a TLB miss right after a page fault. But
again, only when things work.
None of the above of course address your DAR-not-updated concern. I
think your approach of "clobbering" the DAR on every storage exception
after it's been snapshotted and then testing for the clobber and
emulating the instruction is the way to go, though your patch could use
some cleaning there.
I'll post comments on it separately.
At the end of the day, I can't help that much without HW access and/or a
simulator, as you can guess, and no, I'm not asking for people to send
me an 8xx board :-) (especially useless without a BDI imho). So I rely
on Scott and yourself to sort this out. But thanks for showing up, and
please, do port your board to 2.6 so you can test your stuff :-)
Cheers,
Ben.
Benjamin Herrenschmidt [off-list ref] wrote on 07/10/2009 03:07:35:
Allright, did a bit of reading of doco and code..
hey, this is a super, thanks!
Doco isn't totally clear though. At some stage, it -hints- that in case
of a TLB "error" (match on EA/ASID but incorrect
protection/valid/changed/...) the offending TLB entry is automatically
invalidated. Do you know if that is correct ?
Nope, I don't know.
I would hope so since we never do anything to remove the "invalid"
entries we write to the TLB when hitting non-present PTEs but then, that
may also explain some of our problems...
hmm, then a tlbil_va() in do_page_fault for "no translation" errors
would help I guess.
Now, a few comments from what I read in the code:
- The whole writeback could be avoided in Instruction/Data TLB miss,
but for that, you need to make sure that the TLB entry we create has
valid set only if -both- present and accessed are set. That would save
in the case of CONFIG_SWAP, a store and a read back of TWC I suppose,
but you do need to find a way to do that ANDing of ACCESSED and PRESENT.
So far I have mapped !ACCESSED pages as No Access instead, maybe that
was wrong? I thought it was easier but I will have a look at this too.
- I think we can get rid of HWWRITE. We can make CHANGED be the current
HWWRITE value, I agree with you, which matches the HW changed bit. We
need to be a bit careful of how we setup the PP bits tho. At this stage,
I see several approaches:
* One is to basically "generate" the right PP bits based on a
combination of _PAGE_USER and _PAGE_RW. That's the simpler approach but
probably uses more code in the TLB miss handler. It would look like
that, with MxCTR:PPCS=0
_PAGE_USER _PAGE_RW PP (bits 20..27)
0 0 011C1111 (C is _PAGE_DIRTY)
0 1 000C1111
1 0 110C1111
1 1 100C1111
One easy way to do that is to have _PAGE_USER and _PAGE_RW sit next to
each other in bit position 28 and 29 (0xc). Load a GPR with something
like 0110000011001000 and rotate it left by PTE & 0xc, then move the
resulting 3 bits into position, or something along those lines. You can
also give up on kernel read-only support and go down to 2 PP bits and
never use the extended encoding.
yes, I do think the extended encoding is too much work and not worth it.
One concern I have is if a user RO mapping also can be a kernel RO
mapping? That is, do kernel require RW to a user page mapped RO?
* Another one is to use MxCTR:PPCS=1 a mask of 100C1U00 (U is
_PAGE_USER) and or in ^_PAGE_RW (it could actually be made
reverse-polarity in the PTE but that would mean some changes to non-8xx
specific headers, so let's avoid it for now at least).
hmm, interesting.
Then I will also have set ACCESSED manually in TLB Miss I think.
At least that's the best options I can see from my reading of the doco,
though it's not totally clear to me what really happens when doing the
PPCS trick, will it generate a TLB error on a non-match or will it try
to TLB miss, which could be bad.
Yes, I don't think anybody has tested this.
* Last but not least, it wouldn't be hard to use either of the above
encodings, and have the PTE actually contain the right bit combination
already. You don't need to have a _PAGE_RW, you don't need to have a
_PAGE_USER :-) Look at how I do things for book3e, where I layout the
6 BookE protection bit directly in the PTE. That's a bit harder and maybe
will need subtle changes to pte-common.h and such to accomodate it, and
so probably something to experiment with as a second step, but it's the
most efficient approach in the long run for obvious reasons.
- I think we could have more bits pre-set to the right values in the
PTE, look how we play with defining some of the constants in
pte-common.h, might be worth having a look, but -after- we have
something that works :-)
- Possible bug: I'm very disturbed by the fact that DataTLBError sets
HWWRITE and DIRTY on a non-present PTE. It should not. Just like
ACCESSED. That's going to cause trouble and swap corruption, even more
as we move DIRTY around.
Yes, this is what I fix with the first patch in my series:
8xx: DTLB Error must check for more errors.
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.
- Maybe we can completely remove that mucking around with dirty,
setting of accessed etc... from DataTLBError. Just make it go to C code
just like InstructionAccess, as we discussed earlier, the generic code
will fix it up and we'll speed up page faults. It should be fairly rare
to take a fault due to a missing _PAGE_ACCESSED or _PAGE_DIRTY in any
case, so all that fixup in those exceptions is just overhead.
That depends on if you set ACCESSED in TLB Miss or not I think.
if TLB Miss maps pages as NoAccess du to missing ACCESSED then the
first access is always going to trap to TLB Error and then to
C just to fix DIRTY and/or ACCESSED, feels a bit more expensive
fixing it in TLB error, no?
BTW, does 2.4 update ACCESSED and DIRTY in generic code the same way
as 2.6? It is still much easier for me to test thing in 2.4 and then
move it over to 2.6
- Later on, if it rocks your boat, you may want to look into removing
the mucking around with swapper_pg_dir in the TLB miss. Instead, what
you can do is lazily copy the kernel PMDs into the user PMDs. IE. On the
first kernel access from a new user context, you fault, and from the
do_page_fault() code, we can detect that and fixup the user PMD to point
to the kernel page tables, thus avoiding that whole bunch of code in the
TLB miss. When creating new user page tables, we can pre-fill the kernel
PMDs too. I think x86 does that. We could do that for BookE too, though
it's less of a win since we have to fixup the TID in MMUCR/MAS but for
8xx it would save a few instructions & conditionals in the TLB miss fast
path.
That would be nice, I have had that idea but no clue as how to do it.
- I still have problems with the comment next to the "workaround"
tlbil_va() we have in the icache/dcache flush. It doesn't make much
sense to me since dcbst is going to be done by the kernel on a kernel
address, not a user address, so I don't see how the thing we invalidate
relates to the flush we do just below.... So that raises a few
questions:
This workaround was added when we started to force TLB Errors in
TLB Miss for missing pmd entries so it may be some other bug in the
TLB code that makes this happen.
Here is a link:
http://www.mail-archive.com/linuxppc-embedded@ozlabs.org/msg07885.html
Seems to boil down to a missing tlbie()
* If this is to avoid write faults on kernel pages, then we may just
want to have a fixup in do_page_fault() to ignore them when coming from
flush_dcache_icache_* using the exception table mechanism instead ?
* I don't see how it works around user faults but maybe you have a
clear scenario in mind
* It appears still reasonably obvious that we need that tlbil_va
somewhere in that fault path, but I don't know what for, I'm not sure
it's really what the comment says it's about. That raises back the whole
question of when are those "invalid" TLB entries that we create when
_PAGE_PRESENT is not set are going to be removed. The doc hints that
they go a way on TLB error, but the doc really only says so for missing
changed bit... so I'm tempted to say we need to basically stick a
tlbil_va on 8xx on both set_pte_at() and ptep_set_access_flags() for any
non-kernel page. What do you think ? That would be the safest.
Possibly just doing a tlbil_va in do_page_fault() for no translation
errors comes to mind.
- The "fake" DataAccess and InstructionAccess are useless... just a
spurious jump for nothing. We could implement those straight in
DataTLBError and InstructionTLBError, we just need to make sure that
the trap numbers we put in the exception frame are 0x300 and 0x400, but
that's just a matter of passing the right bits to EXC_XFER_EE_LITE().
- Finally, another boat-rocking optimisation to do is preload. You
could hook in update_mmu_cache() for 8xx to go whack an entry in the TLB
as well, which would avoid a TLB miss right after a page fault. But
again, only when things work.
None of the above of course address your DAR-not-updated concern. I
think your approach of "clobbering" the DAR on every storage exception
after it's been snapshotted and then testing for the clobber and
emulating the instruction is the way to go, though your patch could use
some cleaning there.
Yes, it is full of debug cruft now.
I'll post comments on it separately.
Thanks, I still like the asm version for two reasons:
- It contains the problem to 8xx files
- fixing it up in do_page_fault() may lead to a never ending loop.
This is mainly due to the current TLB code not using DIRTY correctly.
In any case it would be nice to keep the asm version in the file, should
we need it some day. It was rather painful to get it working so saving it
somewhere would be nice.
At the end of the day, I can't help that much without HW access and/or a
simulator, as you can guess, and no, I'm not asking for people to send
me an 8xx board :-) (especially useless without a BDI imho). So I rely
on Scott and yourself to sort this out. But thanks for showing up, and
please, do port your board to 2.6 so you can test your stuff :-)