diff mbox

[0/8] Fix 8xx MMU/TLB

Message ID OFA43B9FDF.F43B1705-ONC1257652.0041B12F-C1257652.004211A3@transmode.se (mailing list archive)
State Not Applicable
Headers show

Commit Message

Joakim Tjernlund Oct. 17, 2009, 12:01 p.m. UTC
Joakim Tjernlund/Transmode wrote on 17/10/2009 13:24:18:
>
> Rex Feany <RFeany@mrv.com> wrote on 16/10/2009 22:25:41:
> >
> > Thus spake Joakim Tjernlund (joakim.tjernlund@transmode.se):
> >
> > > Right, it is the pte table walk that is blowing up.
> > > I just noted that 2.6 lacks a tophys() call in its table walk
> > > so I removed that one(there is one more tophys call but I don't think
> > > it should be removed).
> > > Try this addon patch:
> >
> > no difference

> OK, thinking a bit more, this part should not be executed as
> copy_tofrom_user executes in kernel space.
>
> Any chance you can stick a HW breakpoint on FixupDAR?
> Perhaps there is something different with kernel
> virtual address to phys address?
> A simple topys() works in 2.4, but perhaps not in 2.6?
> this is the part of interest:
> FixupDAR: /* Entry point for dcbx workaround. */
>  /* fetch instruction from memory. */
>  mfspr r10, SPRN_SRR0
>  andis. r11, r10, 0x8000
>  tophys  (r11, r10)
>  beq- 139b  /* Branch if user space address */
> 140: lwz r11,0(r11)

Probably better to walk the kernel page table too. Does this
make a difference(needs the tophys() patch I posted earlier):


From 862dda30c3d3d3bedcc605e8520626408a26891c Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Sat, 17 Oct 2009 13:54:03 +0200
Subject: [PATCH] 8xx: Walk the page table for kernel addresses too.

---
 arch/powerpc/kernel/head_8xx.S |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

--
1.6.4.4

Comments

Benjamin Herrenschmidt Oct. 26, 2009, 10:47 p.m. UTC | #1
> Probably better to walk the kernel page table too. Does this
> make a difference(needs the tophys() patch I posted earlier):

This whole thing would be a -lot- easier to do from C code. Why ? Simply
because you could just use get_user() to load the instruction rather
than doing this page table walking in asm, which is simpler, faster, and
more fool proof (ok, you do pay the price of a kernel entry/exit
instead, but I still believe that code simplicity and maintainability
wins here).

Ben.

> >From 862dda30c3d3d3bedcc605e8520626408a26891c Mon Sep 17 00:00:00 2001
> From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> Date: Sat, 17 Oct 2009 13:54:03 +0200
> Subject: [PATCH] 8xx: Walk the page table for kernel addresses too.
> 
> ---
>  arch/powerpc/kernel/head_8xx.S |   25 ++++++++++++-------------
>  1 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
> index 0e91da4..edc9e9b 100644
> --- a/arch/powerpc/kernel/head_8xx.S
> +++ b/arch/powerpc/kernel/head_8xx.S
> @@ -532,28 +532,27 @@ DARFixed:/* Return from dcbx instruction bug workaround, r10 holds value of DAR
>   * by decoding the registers used by the dcbx instruction and adding them.
>   * DAR is set to the calculated address and r10 also holds the EA on exit.
>   */
> -#define NO_SELF_MODIFYING_CODE /* define if you don't want to use self modifying code */
> -     nop	/* A few nops to make the modified_instr: space below cache line aligned */
> -     nop
> -139:	/* fetch instruction from userspace memory */
> + /* define if you don't want to use self modifying code */
> +#define NO_SELF_MODIFYING_CODE
> +FixupDAR:/* Entry point for dcbx workaround. */
> +	/* fetch instruction from memory. */
> +     mfspr r10, SPRN_SRR0
>       DO_8xx_CPU6(0x3780, r3)
>       mtspr SPRN_MD_EPN, r10
>       mfspr r11, SPRN_M_TWB	/* Get level 1 table entry address */
> -     lwz   r11, 0(r11)	/* Get the level 1 entry */
> +     cmplwi      cr0, r11, 0x0800
> +     blt-  3f		/* Branch if user space */
> +     lis   r11, swapper_pg_dir@h
> +     ori   r11, r11, swapper_pg_dir@l
> +     rlwimi      r11, r11, 0, 2, 19
> +3:   lwz   r11, 0(r11)	/* Get the level 1 entry */
>       DO_8xx_CPU6(0x3b80, r3)
>       mtspr SPRN_MD_TWC, r11	/* Load pte table base address */
>       mfspr r11, SPRN_MD_TWC	/* ....and get the pte address */
>       lwz   r11, 0(r11)	/* Get the pte */
>       /* concat physical page address(r11) and page offset(r10) */
>       rlwimi      r11, r10, 0, 20, 31
> -     b     140f
> -FixupDAR:	/* Entry point for dcbx workaround. */
> -	/* fetch instruction from memory. */
> -     mfspr r10, SPRN_SRR0
> -     andis.      r11, r10, 0x8000
> -     tophys  (r11, r10)
> -     beq-  139b		/* Branch if user space address */
> -140: lwz   r11,0(r11)
> +     lwz   r11,0(r11)
>  /* Check if it really is a dcbx instruction. */
>  /* dcbt and dcbtst does not generate DTLB Misses/Errors,
>   * no need to include them here */
> --
> 1.6.4.4
Dan Malek Oct. 26, 2009, 11:26 p.m. UTC | #2
On Oct 26, 2009, at 3:47 PM, Benjamin Herrenschmidt wrote:

> This whole thing would be a -lot- easier to do from C code. Why ?  
> Simply
> because you could just use get_user() to load the instruction rather
> than doing this page table walking in asm,

Just be careful the get_user() doesn't regenerate the same
translation error you are trying to fix by being here......
It is nice doing things in C code, but you have to be aware
of the environment and the side effects when in this kind
of exception state.

Thanks.

	-- Dan
Benjamin Herrenschmidt Oct. 27, 2009, midnight UTC | #3
On Mon, 2009-10-26 at 16:26 -0700, Dan Malek wrote:
> Just be careful the get_user() doesn't regenerate the same
> translation error you are trying to fix by being here......

It shouldn't since it will always come up with a proper DAR but
you may want to double check before hand that your instruction
address you are loading from is -not- your marker value for bad DAR.

> It is nice doing things in C code, but you have to be aware
> of the environment and the side effects when in this kind 

Yup.

Cheers,
Ben.
Joakim Tjernlund Oct. 27, 2009, 9:16 a.m. UTC | #4
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 27/10/2009 01:00:53:
>
> On Mon, 2009-10-26 at 16:26 -0700, Dan Malek wrote:
> > Just be careful the get_user() doesn't regenerate the same
> > translation error you are trying to fix by being here......

yes, I had some problems with this initially but managed to work around that.
I noticed another problem though, I got multiple TLB errors for the same
address when I did it in C. Noticed by just printk:ing every hit
for a dcbX insn in do_page_fault. I can't explain it, but it seems
like when moving to C you have to execute a rfi insn and that might somehow
restart the dcbX insn before moving on to the page fault routine(or something
totally different)

>
> It shouldn't since it will always come up with a proper DAR but
> you may want to double check before hand that your instruction
> address you are loading from is -not- your marker value for bad DAR.

hmm, I check that the insn really is a dcbX insn, but not that the address is
!= 0x00f0. Don't see how it could be as if something is wrong with
the insn address you get ITLB error instead of a DTLB error.

Anyhow, things seems stalled as I haven't heard from Scott or Rex for a while.
If this isn't working now, I really don't know what is wrong and need
some debugging help.

>
> > It is nice doing things in C code, but you have to be aware
> > of the environment and the side effects when in this kind
>
> Yup.
>
> Cheers,
> Ben.
>
>
>
Scott Wood Oct. 27, 2009, 3:58 p.m. UTC | #5
On Tue, Oct 27, 2009 at 10:16:17AM +0100, Joakim Tjernlund wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 27/10/2009 01:00:53:
> >
> > On Mon, 2009-10-26 at 16:26 -0700, Dan Malek wrote:
> > > Just be careful the get_user() doesn't regenerate the same
> > > translation error you are trying to fix by being here......
> 
> yes, I had some problems with this initially but managed to work around
> that. I noticed another problem though, I got multiple TLB errors for the
> same address when I did it in C. Noticed by just printk:ing every hit for
> a dcbX insn in do_page_fault. I can't explain it, but it seems like when
> moving to C you have to execute a rfi insn and that might somehow restart
> the dcbX insn before moving on to the page fault routine(or something
> totally different)

The rfi should be to other kernel code -- there is no way that it should be
restarting the dcbX (other than when trying to turn a TLB miss into a TLB
error).  Can you post the C version, maybe we can see what's going wrong? 
Is the empty TLB entry from the miss getting invalidated in the dcbX fixup
case?

> > It shouldn't since it will always come up with a proper DAR but
> > you may want to double check before hand that your instruction
> > address you are loading from is -not- your marker value for bad DAR.
> 
> hmm, I check that the insn really is a dcbX insn, but not that the address is
> != 0x00f0. Don't see how it could be as if something is wrong with
> the insn address you get ITLB error instead of a DTLB error.

I'm guessing he meant the data address you're loading.

> Anyhow, things seems stalled as I haven't heard from Scott or Rex for a
> while. If this isn't working now, I really don't know what is wrong and
> need some debugging help.

I'll test the latest version, but I have some scheduling latency. :-)

-Scott
Joakim Tjernlund Oct. 27, 2009, 4:38 p.m. UTC | #6
Scott Wood <scottwood@freescale.com> wrote on 27/10/2009 16:58:41:
>
> On Tue, Oct 27, 2009 at 10:16:17AM +0100, Joakim Tjernlund wrote:
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 27/10/2009 01:00:53:
> > >
> > > On Mon, 2009-10-26 at 16:26 -0700, Dan Malek wrote:
> > > > Just be careful the get_user() doesn't regenerate the same
> > > > translation error you are trying to fix by being here......
> >
> > yes, I had some problems with this initially but managed to work around
> > that. I noticed another problem though, I got multiple TLB errors for the
> > same address when I did it in C. Noticed by just printk:ing every hit for
> > a dcbX insn in do_page_fault. I can't explain it, but it seems like when
> > moving to C you have to execute a rfi insn and that might somehow restart
> > the dcbX insn before moving on to the page fault routine(or something
> > totally different)
>
> The rfi should be to other kernel code -- there is no way that it should be
> restarting the dcbX (other than when trying to turn a TLB miss into a TLB
> error).  Can you post the C version, maybe we can see what's going wrong?

I don't have it for 2.6 and I never did cleanup up my 2.4 version.
Your best bet is to look at one of the earlier patches such
as:
  Add some debug code to do_page_fault
and fix the remaining bits.

> Is the empty TLB entry from the miss getting invalidated in the dcbX fixup
> case?

Yes, in all cases it was invalidated.

>
> > > It shouldn't since it will always come up with a proper DAR but
> > > you may want to double check before hand that your instruction
> > > address you are loading from is -not- your marker value for bad DAR.
> >
> > hmm, I check that the insn really is a dcbX insn, but not that the address is
> > != 0x00f0. Don't see how it could be as if something is wrong with
> > the insn address you get ITLB error instead of a DTLB error.
>
> I'm guessing he meant the data address you're loading.

Hopefully and I am already looking at the OP code to make sure it is
a dcbX insn.

>
> > Anyhow, things seems stalled as I haven't heard from Scott or Rex for a
> > while. If this isn't working now, I really don't know what is wrong and
> > need some debugging help.
>
> I'll test the latest version, but I have some scheduling latency. :-)

Get yourself a new scheduler :)

   Jocke
Scott Wood Oct. 30, 2009, 12:12 a.m. UTC | #7
On Sat, Oct 17, 2009 at 02:01:38PM +0200, Joakim Tjernlund wrote:
> Joakim Tjernlund/Transmode wrote on 17/10/2009 13:24:18:
> >
> > Rex Feany <RFeany@mrv.com> wrote on 16/10/2009 22:25:41:
> > >
> > > Thus spake Joakim Tjernlund (joakim.tjernlund@transmode.se):
> > >
> > > > Right, it is the pte table walk that is blowing up.
> > > > I just noted that 2.6 lacks a tophys() call in its table walk
> > > > so I removed that one(there is one more tophys call but I don't think
> > > > it should be removed).
> > > > Try this addon patch:
> > >
> > > no difference
> 
> > OK, thinking a bit more, this part should not be executed as
> > copy_tofrom_user executes in kernel space.
> >
> > Any chance you can stick a HW breakpoint on FixupDAR?
> > Perhaps there is something different with kernel
> > virtual address to phys address?
> > A simple topys() works in 2.4, but perhaps not in 2.6?
> > this is the part of interest:
> > FixupDAR: /* Entry point for dcbx workaround. */
> >  /* fetch instruction from memory. */
> >  mfspr r10, SPRN_SRR0
> >  andis. r11, r10, 0x8000
> >  tophys  (r11, r10)
> >  beq- 139b  /* Branch if user space address */
> > 140: lwz r11,0(r11)
> 
> Probably better to walk the kernel page table too. Does this
> make a difference(needs the tophys() patch I posted earlier):

After applying by hand (whitespace damage), I get this and a bunch more:

VFS: Mounted root (nfs filesystem) readonly on device 0:12.                     
Freeing unused kernel memory: 96k init                                          
INIT: version 2.85 booting                                                      
Mounting /proc and /sys                                                         
Oops: Machine check, sig: 7 [#1]                                                
Embedded Planet EP88xC                                                          
NIP: 00002020 LR: c0089c58 CTR: 00000038                                        
REGS: c38d7de0 TRAP: 0200   Not tainted  (2.6.32-rc4-00971-g2edbf13-dirty)      
MSR: 00001000 <ME>  CR: 44002428  XER: 00000000                                 
TASK = c383b7a0[173] 'udev' THREAD: c38d6000                                    
GPR00: 00000001 c38d7e90 c383b7a0 00000014 c380bffc 0000000c 3001fffc 00000001  
GPR08: 00000038 0000039b c001137c c021c000 00000000 100c7368 c01f59f4 c01f59d0  
GPR16: c0240000 100982dc 100c0aac 10095ccc 00000047 c38a5868 c38d7f20 00000000  
GPR24: c38dd880 00000400 30020000 00000000 c38d7ea0 00000000 0000039c c38a5840  
NIP [00002020] 0x2020                                                           
LR [c0089c58] seq_read+0x488/0x558                                              
Call Trace:                                                                     
[c38d7e90] [c0089a74] seq_read+0x2a4/0x558 (unreliable)                         
[c38d7ee0] [c00ac264] proc_reg_read+0x4c/0x70                                   
[c38d7ef0] [c006f7f4] vfs_read+0xb4/0x158                                       
[c38d7f10] [c006fb04] sys_read+0x4c/0x90                                        
[c38d7f40] [c000dfb8] ret_from_syscall+0x0/0x38                                 
Instruction dump:                                                               
00000000 XXXXXXXX XXXXXXXX XXXXXXXX 7d5a02a6 XXXXXXXX XXXXXXXX XXXXXXXX         
41800010 XXXXXXXX XXXXXXXX XXXXXXXX 816b0000 XXXXXXXX XXXXXXXX XXXXXXXX         
---[ end trace fab819d28e265675 ]---                                            
/etc/rc.d/rcS: line 24:   173 Bus error               /etc/rc.d/init.d/$i $mode 

-Scott
Joakim Tjernlund Oct. 30, 2009, 12:51 a.m. UTC | #8
Scott Wood <scottwood@freescale.com> wrote on 30/10/2009 01:12:28:
>
> On Sat, Oct 17, 2009 at 02:01:38PM +0200, Joakim Tjernlund wrote:
> > Joakim Tjernlund/Transmode wrote on 17/10/2009 13:24:18:
> > >
> > > Rex Feany <RFeany@mrv.com> wrote on 16/10/2009 22:25:41:
> > > >
> > > > Thus spake Joakim Tjernlund (joakim.tjernlund@transmode.se):
> > > >
> > > > > Right, it is the pte table walk that is blowing up.
> > > > > I just noted that 2.6 lacks a tophys() call in its table walk
> > > > > so I removed that one(there is one more tophys call but I don't think
> > > > > it should be removed).
> > > > > Try this addon patch:
> > > >
> > > > no difference
> >
> > > OK, thinking a bit more, this part should not be executed as
> > > copy_tofrom_user executes in kernel space.
> > >
> > > Any chance you can stick a HW breakpoint on FixupDAR?
> > > Perhaps there is something different with kernel
> > > virtual address to phys address?
> > > A simple topys() works in 2.4, but perhaps not in 2.6?
> > > this is the part of interest:
> > > FixupDAR: /* Entry point for dcbx workaround. */
> > >  /* fetch instruction from memory. */
> > >  mfspr r10, SPRN_SRR0
> > >  andis. r11, r10, 0x8000
> > >  tophys  (r11, r10)
> > >  beq- 139b  /* Branch if user space address */
> > > 140: lwz r11,0(r11)
> >
> > Probably better to walk the kernel page table too. Does this
> > make a difference(needs the tophys() patch I posted earlier):
>
> After applying by hand (whitespace damage), I get this and a bunch more:

OK, please send your diff to head_8xx.S. Maybe I can spot an
error, otherwise you will have to set a hw BP on fixDAR and step
through it.

 Jocke
Scott Wood Oct. 30, 2009, 5:16 p.m. UTC | #9
On Sat, Oct 17, 2009 at 02:01:38PM +0200, Joakim Tjernlund wrote:
> +     mfspr r10, SPRN_SRR0
>       DO_8xx_CPU6(0x3780, r3)
>       mtspr SPRN_MD_EPN, r10
>       mfspr r11, SPRN_M_TWB	/* Get level 1 table entry address */
> -     lwz   r11, 0(r11)	/* Get the level 1 entry */
> +     cmplwi      cr0, r11, 0x0800
> +     blt-  3f		/* Branch if user space */
> +     lis   r11, swapper_pg_dir@h
> +     ori   r11, r11, swapper_pg_dir@l
> +     rlwimi      r11, r11, 0, 2, 19

That rlwimi is a no-op -- I think you meant to use a different register
here?

> +3:   lwz   r11, 0(r11)	/* Get the level 1 entry */
>       DO_8xx_CPU6(0x3b80, r3)
>       mtspr SPRN_MD_TWC, r11	/* Load pte table base address */
>       mfspr r11, SPRN_MD_TWC	/* ....and get the pte address */
>       lwz   r11, 0(r11)	/* Get the pte */
>       /* concat physical page address(r11) and page offset(r10) */
>       rlwimi      r11, r10, 0, 20, 31

But r10 here contains SRR0 from above, and this is a data TLB error.

-Scott
Scott Wood Oct. 30, 2009, 5:37 p.m. UTC | #10
On Fri, Oct 30, 2009 at 12:16:07PM -0500, Scott Wood wrote:
> On Sat, Oct 17, 2009 at 02:01:38PM +0200, Joakim Tjernlund wrote:
> > +     mfspr r10, SPRN_SRR0
> >       DO_8xx_CPU6(0x3780, r3)
> >       mtspr SPRN_MD_EPN, r10
> >       mfspr r11, SPRN_M_TWB	/* Get level 1 table entry address */
> > -     lwz   r11, 0(r11)	/* Get the level 1 entry */
> > +     cmplwi      cr0, r11, 0x0800
> > +     blt-  3f		/* Branch if user space */
> > +     lis   r11, swapper_pg_dir@h
> > +     ori   r11, r11, swapper_pg_dir@l
> > +     rlwimi      r11, r11, 0, 2, 19
> 
> That rlwimi is a no-op -- I think you meant to use a different register
> here?
> 
> > +3:   lwz   r11, 0(r11)	/* Get the level 1 entry */
> >       DO_8xx_CPU6(0x3b80, r3)
> >       mtspr SPRN_MD_TWC, r11	/* Load pte table base address */
> >       mfspr r11, SPRN_MD_TWC	/* ....and get the pte address */
> >       lwz   r11, 0(r11)	/* Get the pte */
> >       /* concat physical page address(r11) and page offset(r10) */
> >       rlwimi      r11, r10, 0, 20, 31
> 
> But r10 here contains SRR0 from above, and this is a data TLB error.

Never mind that last one, forgot that you'd be wanting to load the
instruction. :-P

But the rlwimi is what's causing the machine checks.  I replaced it with:
rlwinm	r11, r11, 0, 0x3ffff000
rlwimi	r11, r10, 22, 0xffc

and things seem to work.  You could probably replace the rlwinm by
subtracting PAGE_OFFSET from swapper_pg_dir instead.

-Scott
Joakim Tjernlund Oct. 31, 2009, 10:31 a.m. UTC | #11
Scott Wood <scottwood@freescale.com> wrote on 30/10/2009 18:37:49:
>
> On Fri, Oct 30, 2009 at 12:16:07PM -0500, Scott Wood wrote:
> > On Sat, Oct 17, 2009 at 02:01:38PM +0200, Joakim Tjernlund wrote:
> > > +     mfspr r10, SPRN_SRR0
> > >       DO_8xx_CPU6(0x3780, r3)
> > >       mtspr SPRN_MD_EPN, r10
> > >       mfspr r11, SPRN_M_TWB   /* Get level 1 table entry address */
> > > -     lwz   r11, 0(r11)   /* Get the level 1 entry */
> > > +     cmplwi      cr0, r11, 0x0800
> > > +     blt-  3f      /* Branch if user space */
> > > +     lis   r11, swapper_pg_dir@h
> > > +     ori   r11, r11, swapper_pg_dir@l
> > > +     rlwimi      r11, r11, 0, 2, 19
> >
> > That rlwimi is a no-op -- I think you meant to use a different register
> > here?
> >
> > > +3:   lwz   r11, 0(r11)   /* Get the level 1 entry */
> > >       DO_8xx_CPU6(0x3b80, r3)
> > >       mtspr SPRN_MD_TWC, r11   /* Load pte table base address */
> > >       mfspr r11, SPRN_MD_TWC   /* ....and get the pte address */
> > >       lwz   r11, 0(r11)   /* Get the pte */
> > >       /* concat physical page address(r11) and page offset(r10) */
> > >       rlwimi      r11, r10, 0, 20, 31
> >
> > But r10 here contains SRR0 from above, and this is a data TLB error.
>
> Never mind that last one, forgot that you'd be wanting to load the
> instruction. :-P
>
> But the rlwimi is what's causing the machine checks.  I replaced it with:

Yes, I see now that it is wrong.

> rlwinm   r11, r11, 0, 0x3ffff000
> rlwimi   r11, r10, 22, 0xffc
>
> and things seem to work.  You could probably replace the rlwinm by
> subtracting PAGE_OFFSET from swapper_pg_dir instead.

Would you mind produce a proper path on top of my series? I am blind
here so I can only guess what will work or not.
Then Rex can give it some beating and we can push this to Ben
Joakim Tjernlund Nov. 3, 2009, 1:32 p.m. UTC | #12
Scott Wood <scottwood@freescale.com> wrote on 30/10/2009 18:37:49:
>
> On Fri, Oct 30, 2009 at 12:16:07PM -0500, Scott Wood wrote:
> > On Sat, Oct 17, 2009 at 02:01:38PM +0200, Joakim Tjernlund wrote:
> > > +     mfspr r10, SPRN_SRR0
> > >       DO_8xx_CPU6(0x3780, r3)
> > >       mtspr SPRN_MD_EPN, r10
> > >       mfspr r11, SPRN_M_TWB   /* Get level 1 table entry address */
> > > -     lwz   r11, 0(r11)   /* Get the level 1 entry */
> > > +     cmplwi      cr0, r11, 0x0800
> > > +     blt-  3f      /* Branch if user space */
> > > +     lis   r11, swapper_pg_dir@h
> > > +     ori   r11, r11, swapper_pg_dir@l
> > > +     rlwimi      r11, r11, 0, 2, 19
> >
> > That rlwimi is a no-op -- I think you meant to use a different register
> > here?
> >
> > > +3:   lwz   r11, 0(r11)   /* Get the level 1 entry */
> > >       DO_8xx_CPU6(0x3b80, r3)
> > >       mtspr SPRN_MD_TWC, r11   /* Load pte table base address */
> > >       mfspr r11, SPRN_MD_TWC   /* ....and get the pte address */
> > >       lwz   r11, 0(r11)   /* Get the pte */
> > >       /* concat physical page address(r11) and page offset(r10) */
> > >       rlwimi      r11, r10, 0, 20, 31
> >
> > But r10 here contains SRR0 from above, and this is a data TLB error.
>
> Never mind that last one, forgot that you'd be wanting to load the
> instruction. :-P
>
> But the rlwimi is what's causing the machine checks.  I replaced it with:
> rlwinm   r11, r11, 0, 0x3ffff000
> rlwimi   r11, r10, 22, 0xffc
>
> and things seem to work.  You could probably replace the rlwinm by
> subtracting PAGE_OFFSET from swapper_pg_dir instead.

Just guessing here, do you mean:
	lis	r11, (swapper_pg_dir-PAGE_OFFSET)@h
	ori	r11, r11, (swapper_pg_dir-PAGE_OFFSET)@l
	rlwimi	r11, r10, 22, 0xffc
or
	lis	r11, swapper_pg_dir@h
	ori	r11, r11, swapper_pg_dir@l
	subis	r11, r11 PAGE_OFFSET
	rlwimi	r11, r10, 22, 0xffc

 Jocke
Scott Wood Nov. 3, 2009, 4:59 p.m. UTC | #13
Joakim Tjernlund wrote:
>> and things seem to work.  You could probably replace the rlwinm by
>> subtracting PAGE_OFFSET from swapper_pg_dir instead.
> 
> Just guessing here, do you mean:
> 	lis	r11, (swapper_pg_dir-PAGE_OFFSET)@h
> 	ori	r11, r11, (swapper_pg_dir-PAGE_OFFSET)@l
> 	rlwimi	r11, r10, 22, 0xffc
> or
> 	lis	r11, swapper_pg_dir@h
> 	ori	r11, r11, swapper_pg_dir@l
> 	subis	r11, r11 PAGE_OFFSET
> 	rlwimi	r11, r10, 22, 0xffc

The former.

-Scott
Joakim Tjernlund Nov. 3, 2009, 5:16 p.m. UTC | #14
Scott Wood <scottwood@freescale.com> wrote on 03/11/2009 17:59:30:
>
> Joakim Tjernlund wrote:
> >> and things seem to work.  You could probably replace the rlwinm by
> >> subtracting PAGE_OFFSET from swapper_pg_dir instead.
> >
> > Just guessing here, do you mean:
> >    lis   r11, (swapper_pg_dir-PAGE_OFFSET)@h
> >    ori   r11, r11, (swapper_pg_dir-PAGE_OFFSET)@l
> >    rlwimi   r11, r10, 22, 0xffc
> > or
> >    lis   r11, swapper_pg_dir@h
> >    ori   r11, r11, swapper_pg_dir@l
> >    subis   r11, r11 PAGE_OFFSET
> >    rlwimi   r11, r10, 22, 0xffc
>
> The former.

OK, I will regenerate the patch series with the
 lis   r11, (swapper_pg_dir-PAGE_OFFSET)@h
 ori   r11, r11, (swapper_pg_dir-PAGE_OFFSET)@l
 rlwimi   r11, r10, 22, 0xffc
fix.
Have you already confirmed that this works too?

 Jocke
diff mbox

Patch

diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index 0e91da4..edc9e9b 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -532,28 +532,27 @@  DARFixed:/* Return from dcbx instruction bug workaround, r10 holds value of DAR
  * by decoding the registers used by the dcbx instruction and adding them.
  * DAR is set to the calculated address and r10 also holds the EA on exit.
  */
-#define NO_SELF_MODIFYING_CODE /* define if you don't want to use self modifying code */
-     nop	/* A few nops to make the modified_instr: space below cache line aligned */
-     nop
-139:	/* fetch instruction from userspace memory */
+ /* define if you don't want to use self modifying code */
+#define NO_SELF_MODIFYING_CODE
+FixupDAR:/* Entry point for dcbx workaround. */
+	/* fetch instruction from memory. */
+     mfspr r10, SPRN_SRR0
      DO_8xx_CPU6(0x3780, r3)
      mtspr SPRN_MD_EPN, r10
      mfspr r11, SPRN_M_TWB	/* Get level 1 table entry address */
-     lwz   r11, 0(r11)	/* Get the level 1 entry */
+     cmplwi      cr0, r11, 0x0800
+     blt-  3f		/* Branch if user space */
+     lis   r11, swapper_pg_dir@h
+     ori   r11, r11, swapper_pg_dir@l
+     rlwimi      r11, r11, 0, 2, 19
+3:   lwz   r11, 0(r11)	/* Get the level 1 entry */
      DO_8xx_CPU6(0x3b80, r3)
      mtspr SPRN_MD_TWC, r11	/* Load pte table base address */
      mfspr r11, SPRN_MD_TWC	/* ....and get the pte address */
      lwz   r11, 0(r11)	/* Get the pte */
      /* concat physical page address(r11) and page offset(r10) */
      rlwimi      r11, r10, 0, 20, 31
-     b     140f
-FixupDAR:	/* Entry point for dcbx workaround. */
-	/* fetch instruction from memory. */
-     mfspr r10, SPRN_SRR0
-     andis.      r11, r10, 0x8000
-     tophys  (r11, r10)
-     beq-  139b		/* Branch if user space address */
-140: lwz   r11,0(r11)
+     lwz   r11,0(r11)
 /* Check if it really is a dcbx instruction. */
 /* dcbt and dcbtst does not generate DTLB Misses/Errors,
  * no need to include them here */