| Message ID | 1255008298-19949-3-git-send-email-Joakim.Tjernlund@transmode.se (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 2009-10-08 at 15:24 +0200, Joakim Tjernlund wrote: > +#define _PAGE_RW 0x0400 /* lsb PP bits, inverted in HW */ > +#define _PAGE_USER 0x0800 /* msb PP bits */ > > + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ > + rlwimi. r10, r10, 27, 31, 31 > + beq- cr0, 2f /* Can be removed, costs a ITLB Err */ Did you mean + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>4) */ + rlwimi. r10, r10, 28, 30, 31 + beq- cr0, 2f /* Can be removed, costs a ITLB Err */ Which would still be incorrect. You want -both- to be set, and your code will move on if -any- is set. Might want to add a ~ of the whole thing maybe and invert the condition ? I find it easier to just do li rX, requested_bits, and then, andc. rscratch, rX, rPTE > +#if 0 /* Dont' bother with PP lsb, bit 21 for now */ > + /* r10 = (r10 & ~0x0400) | ((r10 & _PAGE_EXEC) << 7) */ > + rlwimi r10, r10, 7, 21, 21 /* Set _PAGE_EXEC << 7 */ > +#endif I don't get that one. Don't bother with _PAGE_EXEC, there's no control of execute permission that works on 8xx. It's #if 0 anyway. You still need to massage the PP bits into place. I don't see that happening. As it is, your PTE contains for bit 20 and 21, which translates to: PTE: Translates to PP bits: RW: 0 USER: 0 00 supervisor RW (ok) RW: 0 USER: 1 01 supervisor RW user RO (WRONG) RW: 1 USER: 0 10 supervisor RW user RW (WRONG) RW: 1 USER: 1 11 supervisor RO user RO (WRONG) I would suggest you do the stuff I suggested initially with RW and USER being an "index" into a pre-cooked immediate value with all the encodings which also gives you the extended encoding for supervisor RO for free. > + /* Need to know if load/store -> force a TLB Error > + * by copying ACCESSED to PRESENT. > + */ > + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ > + rlwimi r10, r10, 27, 31, 31 That doesn't sound right, just like ITLB... you need to AND those two bits in a way or another, or basically test that they are both set (or neither is set) > +#if 0 /* Not yet */ > + /* Honour kernel RO, User NA */ > + andi. r11, r10, _PAGE_USER | _PAGE_RW > + bne- cr0, 5f > + ori r10,r10, 0x200 /* Extended encoding, bit 22 */ > #endif > - mfspr r11, SPRN_MD_TWC /* get the pte address again */ > - stw r10, 0(r11) > +5: xori r10, r10, _PAGE_RW /* invert RW bit */ Well, you are missing that xori from the ITLB miss I think. Also, that changes the table above to: PTE: Translates to PP bits: RW: 0 USER: 0 10 supervisor RW user RW (WRONG) RW: 0 USER: 1 11 supervisor RO user RO (ok) RW: 1 USER: 0 00 supervisor RW (ok) RW: 1 USER: 1 01 supervisor RW user RO (WRONG) So it's still not right :-) Cheers, Ben.
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 08/10/2009 23:04:03: > > On Thu, 2009-10-08 at 15:24 +0200, Joakim Tjernlund wrote: > > > +#define _PAGE_RW 0x0400 /* lsb PP bits, inverted in HW */ > > +#define _PAGE_USER 0x0800 /* msb PP bits */ > > > > + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ > > + rlwimi. r10, r10, 27, 31, 31 > > + beq- cr0, 2f /* Can be removed, costs a ITLB Err */ > > Did you mean (counting bits) ... No, it should be >> 5 > > + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>4) */ > + rlwimi. r10, r10, 28, 30, 31 > + beq- cr0, 2f /* Can be removed, costs a ITLB Err */ > > Which would still be incorrect. You want -both- to be set, > and your code will move on if -any- is set. Might want to add > a ~ of the whole thing maybe and invert the condition ? I want: if (!accessed) present = 0; accessed == 1 and present = 0 is impossible, right? So basically just copy over accessed to present and linux mm set both when trapping to C. > > I find it easier to just do li rX, requested_bits, and then, andc. > rscratch, rX, rPTE > > > +#if 0 /* Dont' bother with PP lsb, bit 21 for now */ > > + /* r10 = (r10 & ~0x0400) | ((r10 & _PAGE_EXEC) << 7) */ > > + rlwimi r10, r10, 7, 21, 21 /* Set _PAGE_EXEC << 7 */ > > +#endif > > I don't get that one. Don't bother with _PAGE_EXEC, there's no > control of execute permission that works on 8xx. It's #if 0 anyway. What about the execute perms in Level 2 descriptor, page 247? > > You still need to massage the PP bits into place. I don't see that > happening. Not at the moment, later. > > As it is, your PTE contains for bit 20 and 21, which translates to: > > PTE: Translates to PP bits: > RW: 0 USER: 0 00 supervisor RW (ok) > RW: 0 USER: 1 01 supervisor RW user RO (WRONG) > RW: 1 USER: 0 10 supervisor RW user RW (WRONG) > RW: 1 USER: 1 11 supervisor RO user RO (WRONG) You got USER and RW swapped and the table is different for exec. > > I would suggest you do the stuff I suggested initially with RW and USER > being an "index" into a pre-cooked immediate value with all the encodings > which also gives you the extended encoding for supervisor RO for free. > > > + /* Need to know if load/store -> force a TLB Error > > + * by copying ACCESSED to PRESENT. > > + */ > > + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ > > + rlwimi r10, r10, 27, 31, 31 > > That doesn't sound right, just like ITLB... you need to AND those two > bits in a way or another, or basically test that they are both set > (or neither is set) Same here as for ITLB. > > > +#if 0 /* Not yet */ > > + /* Honour kernel RO, User NA */ > > + andi. r11, r10, _PAGE_USER | _PAGE_RW > > + bne- cr0, 5f > > + ori r10,r10, 0x200 /* Extended encoding, bit 22 */ > > #endif > > - mfspr r11, SPRN_MD_TWC /* get the pte address again */ > > - stw r10, 0(r11) > > +5: xori r10, r10, _PAGE_RW /* invert RW bit */ > > Well, you are missing that xori from the ITLB miss I think. Also, that Nope, no xori needed for exec perms > changes the table above to: > > PTE: Translates to PP bits: > RW: 0 USER: 0 10 supervisor RW user RW (WRONG) > RW: 0 USER: 1 11 supervisor RO user RO (ok) > RW: 1 USER: 0 00 supervisor RW (ok) > RW: 1 USER: 1 01 supervisor RW user RO (WRONG) > > So it's still not right :-) User and RW swapped here too, as I read the table. I don't think user space would boot if I got it wrong.
On Fri, 2009-10-09 at 00:44 +0200, Joakim Tjernlund wrote: > accessed == 1 and present = 0 is impossible, right? > So basically just copy over accessed to present and > linux mm set both when trapping to C. No, when present = 0, then the rest of the PTE can contain unrelated things, you can't trust ACCESSED. > What about the execute perms in Level 2 descriptor, page 247? Not useful, not fine grained enough. > > You still need to massage the PP bits into place. I don't see that > > happening. > > Not at the moment, later. > > > > > As it is, your PTE contains for bit 20 and 21, which translates to: > > > > PTE: Translates to PP bits: > > RW: 0 USER: 0 00 supervisor RW (ok) > > RW: 0 USER: 1 01 supervisor RW user RO (WRONG) > > RW: 1 USER: 0 10 supervisor RW user RW (WRONG) > > RW: 1 USER: 1 11 supervisor RO user RO (WRONG) > > You got USER and RW swapped and the table is different > for exec. Hrm, let me see... yes. You are right, I mixed RW and USER. However, I don't think the PP bits change do they ? IE. Basically, Read == Exec at the page level. So the table isn't really different between I and D. However, indeed, since you don't have a unified TLB, the case can be made that we can ignore R vs. W in the iTLB case. In which case, you get for iTLB: PTE: Translates to PP bits: RW: 0 USER: 0 00 supervisor X only (ok) RW: 0 USER: 1 10 supervisor X user X (ok) RW: 1 USER: 0 01 supervisor X user X (WRONG) RW: 1 USER: 1 11 supervisor X user X (ok) So a page with _PAGE_RW and not _PAGE_USER would still be executable from user... oops :-) I think what you want for iTLB is just basically have a base of 00 and or-in _PAGE_USER only (ie, keep _PAGE_RW clear with a rlwinm) so that you basically get supervisor X only if _PAGE_USER is 0 and both X if _PAGE_USER is 1 For the dTLB, the table becomes (including your inversion of _PAGE_RW) PTE: Translates to PP bits: RW: 0 USER: 0 01 supervisor RW user RO (WRONG) RW: 0 USER: 1 11 supervisor RO user RO (ok) RW: 1 USER: 0 00 supervisor RW only (ok) RW: 1 USER: 1 10 supervisor RW user RW (ok) So it's -almost- right :-) You still got the RW:0 USER:0 case wrong, ie a read-only kernel page would be user readable. You can work around that by never setting kernel pages read-only (which we do mostly), but in the grand scheme of things, my trick I proposed initially would sort it out all including support for kernel RO :-) In any case, the above, while wrong, wouldn't cause crashes or issues for well behaved userspace so it's a step forward. > Same here as for ITLB. And still not right :-) ie. you cannot rely on the value of _PAGE_ACCESSED if _PAGE_PRESENT is not set. > Nope, no xori needed for exec perms Right, thanks to having a split TLB, but you still need to mask out one of the bits afaik. > I don't think user space would boot if I got it wrong. True. I think it's more correct than I initially thought but still subtely wrong :-) Cheers, Ben.
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 09/10/2009 02:53:31: > > Subject: > > Re: [PATCH 2/6] 8xx: Update TLB asm so it behaves as linux mm expects. > > On Fri, 2009-10-09 at 00:44 +0200, Joakim Tjernlund wrote: > > > accessed == 1 and present = 0 is impossible, right? > > So basically just copy over accessed to present and > > linux mm set both when trapping to C. > > No, when present = 0, then the rest of the PTE can contain unrelated > things, you can't trust ACCESSED. Ah, OK. > > > What about the execute perms in Level 2 descriptor, page 247? > > Not useful, not fine grained enough. > > > > You still need to massage the PP bits into place. I don't see that > > > happening. > > > > Not at the moment, later. > > > > > > > > As it is, your PTE contains for bit 20 and 21, which translates to: > > > > > > PTE: Translates to PP bits: > > > RW: 0 USER: 0 00 supervisor RW (ok) > > > RW: 0 USER: 1 01 supervisor RW user RO (WRONG) > > > RW: 1 USER: 0 10 supervisor RW user RW (WRONG) > > > RW: 1 USER: 1 11 supervisor RO user RO (WRONG) > > > > You got USER and RW swapped and the table is different > > for exec. > > Hrm, let me see... yes. You are right, I mixed RW and USER. However, > I don't think the PP bits change do they ? IE. Basically, Read == Exec > at the page level. So the table isn't really different between I and D. > > However, indeed, since you don't have a unified TLB, the case can be > made that we can ignore R vs. W in the iTLB case. In which case, you get > for iTLB: > > > PTE: Translates to PP bits: > RW: 0 USER: 0 00 supervisor X only (ok) > RW: 0 USER: 1 10 supervisor X user X (ok) > RW: 1 USER: 0 01 supervisor X user X (WRONG) > RW: 1 USER: 1 11 supervisor X user X (ok) > > So a page with _PAGE_RW and not _PAGE_USER would still be executable > from user... oops :-) Yes, it is not perfect, but should work for now. > > I think what you want for iTLB is just basically have a base of 00 > and or-in _PAGE_USER only (ie, keep _PAGE_RW clear with a rlwinm) > so that you basically get supervisor X only if _PAGE_USER is 0 and > both X if _PAGE_USER is 1 > > For the dTLB, the table becomes (including your inversion of _PAGE_RW) > > PTE: Translates to PP bits: > RW: 0 USER: 0 01 supervisor RW user RO (WRONG) > RW: 0 USER: 1 11 supervisor RO user RO (ok) > RW: 1 USER: 0 00 supervisor RW only (ok) > RW: 1 USER: 1 10 supervisor RW user RW (ok) > > So it's -almost- right :-) You still got the RW:0 USER:0 case wrong, > ie a read-only kernel page would be user readable. Which will be fixed once I activate: #if 0 /* Not yet */ /* Honour kernel RO, User NA */ andi. r11, r10, _PAGE_USER | _PAGE_RW bne- cr0, 5f ori r10,r10, 0x200 /* Extended encoding, bit 22 */ #endif > > You can work around that by never setting kernel pages read-only (which > we do mostly), but in the grand scheme of things, my trick I proposed > initially would sort it out all including support for kernel RO :-) Not convinced that your trick will be a win. The other bits will need to move around too. Maybe I misunderstand something?
On Fri, 2009-10-09 at 08:16 +0200, Joakim Tjernlund wrote: > Which will be fixed once I activate: > #if 0 /* Not yet */ > /* Honour kernel RO, User NA */ > andi. r11, r10, _PAGE_USER | _PAGE_RW > bne- cr0, 5f > ori r10,r10, 0x200 /* Extended encoding, bit 22 */ > #endif Which will be more code including a conditional than my proposed trick :-) I'll try to write the asm for you later. > Not convinced that your trick will be a win. The other > bits will need to move around too. Maybe I misunderstand > something? I'll write some code to show you what I have in mind later. Cheers, Ben.
diff --git a/arch/powerpc/include/asm/pte-8xx.h b/arch/powerpc/include/asm/pte-8xx.h index 8c6e312..f23cd15 100644 --- a/arch/powerpc/include/asm/pte-8xx.h +++ b/arch/powerpc/include/asm/pte-8xx.h @@ -32,22 +32,21 @@ #define _PAGE_FILE 0x0002 /* when !present: nonlinear file mapping */ #define _PAGE_NO_CACHE 0x0002 /* I: cache inhibit */ #define _PAGE_SHARED 0x0004 /* No ASID (context) compare */ +#define _PAGE_DIRTY 0x0100 /* C: page changed */ -/* These five software bits must be masked out when the entry is loaded - * into the TLB. +/* These 3 software bits must be masked out when the entry is loaded + * into the TLB, 2 SW bits left. */ #define _PAGE_EXEC 0x0008 /* software: i-cache coherency required */ #define _PAGE_GUARDED 0x0010 /* software: guarded access */ -#define _PAGE_DIRTY 0x0020 /* software: page changed */ -#define _PAGE_RW 0x0040 /* software: user write access allowed */ -#define _PAGE_ACCESSED 0x0080 /* software: page referenced */ +#define _PAGE_ACCESSED 0x0020 /* software: page referenced */ /* Setting any bits in the nibble with the follow two controls will * require a TLB exception handler change. It is assumed unused bits * are always zero. */ -#define _PAGE_HWWRITE 0x0100 /* h/w write enable: never set in Linux PTE */ -#define _PAGE_USER 0x0800 /* One of the PP bits, the other is USER&~RW */ +#define _PAGE_RW 0x0400 /* lsb PP bits, inverted in HW */ +#define _PAGE_USER 0x0800 /* msb PP bits */ #define _PMD_PRESENT 0x0001 #define _PMD_BAD 0x0ff0 diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S index 118bb05..1639d16 100644 --- a/arch/powerpc/kernel/head_8xx.S +++ b/arch/powerpc/kernel/head_8xx.S @@ -333,26 +333,21 @@ InstructionTLBMiss: mfspr r11, SPRN_MD_TWC /* ....and get the pte address */ lwz r10, 0(r11) /* Get the pte */ -#ifdef CONFIG_SWAP - /* do not set the _PAGE_ACCESSED bit of a non-present page */ - andi. r11, r10, _PAGE_PRESENT - beq 4f - ori r10, r10, _PAGE_ACCESSED - mfspr r11, SPRN_MD_TWC /* get the pte address again */ - stw r10, 0(r11) -4: -#else - ori r10, r10, _PAGE_ACCESSED - stw r10, 0(r11) -#endif + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ + rlwimi. r10, r10, 27, 31, 31 + beq- cr0, 2f /* Can be removed, costs a ITLB Err */ +#if 0 /* Dont' bother with PP lsb, bit 21 for now */ + /* r10 = (r10 & ~0x0400) | ((r10 & _PAGE_EXEC) << 7) */ + rlwimi r10, r10, 7, 21, 21 /* Set _PAGE_EXEC << 7 */ +#endif /* The Linux PTE won't go exactly into the MMU TLB. - * Software indicator bits 21, 22 and 28 must be clear. + * Software indicator bits 22 and 28 must be clear. * Software indicator bits 24, 25, 26, and 27 must be * set. All other Linux PTE bits control the behavior * of the MMU. */ -2: li r11, 0x00f0 + li r11, 0x00f0 rlwimi r10, r11, 0, 24, 28 /* Set 24-27, clear 28 */ DO_8xx_CPU6(0x2d80, r3) mtspr SPRN_MI_RPN, r10 /* Update TLB entry */ @@ -365,6 +360,22 @@ InstructionTLBMiss: lwz r3, 8(r0) #endif rfi +2: + mfspr r11, SRR1 + /* clear all error bits as TLB Miss + * sets a few unconditionally + */ + rlwinm r11, r11, 0, 0xffff + mtspr SRR1, r11 + + mfspr r10, SPRN_M_TW /* Restore registers */ + lwz r11, 0(r0) + mtcr r11 + lwz r11, 4(r0) +#ifdef CONFIG_8xx_CPU6 + lwz r3, 8(r0) +#endif + b InstructionAccess . = 0x1200 DataStoreTLBMiss: @@ -409,21 +420,22 @@ DataStoreTLBMiss: DO_8xx_CPU6(0x3b80, r3) mtspr SPRN_MD_TWC, r11 -#ifdef CONFIG_SWAP - /* do not set the _PAGE_ACCESSED bit of a non-present page */ - andi. r11, r10, _PAGE_PRESENT - beq 4f - ori r10, r10, _PAGE_ACCESSED -4: - /* and update pte in table */ -#else - ori r10, r10, _PAGE_ACCESSED + /* Need to know if load/store -> force a TLB Error + * by copying ACCESSED to PRESENT. + */ + /* r10=(r10&~_PAGE_PRESENT)|((r10&_PAGE_ACCESSED)>>5) */ + rlwimi r10, r10, 27, 31, 31 + +#if 0 /* Not yet */ + /* Honour kernel RO, User NA */ + andi. r11, r10, _PAGE_USER | _PAGE_RW + bne- cr0, 5f + ori r10,r10, 0x200 /* Extended encoding, bit 22 */ #endif - mfspr r11, SPRN_MD_TWC /* get the pte address again */ - stw r10, 0(r11) +5: xori r10, r10, _PAGE_RW /* invert RW bit */ /* The Linux PTE won't go exactly into the MMU TLB. - * Software indicator bits 21, 22 and 28 must be clear. + * Software indicator bits 22 and 28 must be clear. * Software indicator bits 24, 25, 26, and 27 must be * set. All other Linux PTE bits control the behavior * of the MMU. @@ -469,11 +481,12 @@ DataTLBError: stw r10, 0(r0) stw r11, 4(r0) - /* First, make sure this was a store operation. - */ - mfspr r10, SPRN_DSISR - andis. r11, r10, 0x4800 /* no translation, no permission. */ + mfspr r11, SPRN_DSISR + andis. r11, r11, 0x4800 /* !translation or protection */ bne 2f /* branch if either is set */ + /* Only Change bit left now, do it here as it is faster + * than trapping to the C fault handler. + */ /* The EA of a data TLB miss is automatically stored in the MD_EPN * register. The EA of a data TLB error is automatically stored in @@ -522,26 +535,12 @@ DataTLBError: mfspr r11, SPRN_MD_TWC /* ....and get the pte address */ lwz r10, 0(r11) /* Get the pte */ - andi. r11, r10, _PAGE_RW /* Is it writeable? */ - beq 2f /* Bail out if not */ - - /* Update 'changed', among others. - */ -#ifdef CONFIG_SWAP - ori r10, r10, _PAGE_DIRTY|_PAGE_HWWRITE - /* do not set the _PAGE_ACCESSED bit of a non-present page */ - andi. r11, r10, _PAGE_PRESENT - beq 4f - ori r10, r10, _PAGE_ACCESSED -4: -#else - ori r10, r10, _PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_HWWRITE -#endif - mfspr r11, SPRN_MD_TWC /* Get pte address again */ + ori r10, r10, _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_HWWRITE stw r10, 0(r11) /* and update pte in table */ + xori r10, r10, _PAGE_RW /* RW bit is inverted */ /* The Linux PTE won't go exactly into the MMU TLB. - * Software indicator bits 21, 22 and 28 must be clear. + * Software indicator bits 22 and 28 must be clear. * Software indicator bits 24, 25, 26, and 27 must be * set. All other Linux PTE bits control the behavior * of the MMU.
Update the TLB asm to make proper use of _PAGE_DIRY and _PAGE_ACCESSED. Get rid of _PAGE_HWWRITE too. Pros: - I/D TLB Miss never needs to write to the linux pte. - _PAGE_ACCESSED is only set on TLB Error fixing accounting - _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly when a page has been made dirty. - Proper RO/RW mapping of user space. - Free up 2 SW TLB bits in the linux pte(add back _PAGE_WRITETHRU ?) - Less instructions in I/D TLB Miss. - Prepared for HWEXEC support. - Prepared kernel RO/user NA support. Cons: - None ? --- arch/powerpc/include/asm/pte-8xx.h | 13 ++--- arch/powerpc/kernel/head_8xx.S | 93 ++++++++++++++++++------------------ 2 files changed, 52 insertions(+), 54 deletions(-)