| Message ID | 1323946810-4868-3-git-send-email-tiejun.chen@windriver.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
On Thu, 2011-12-15 at 19:00 +0800, Tiejun Chen wrote: > We can't emulate stwu since that may corrupt current exception stack. > So we will have to do real store operation in the exception return code. > > Firstly we'll allocate a trampoline exception frame below the kprobed > function stack and copy the current exception frame to the trampoline. > Then we can do this real store operation to implement 'stwu', and reroute > the trampoline frame to r1 to complete this exception migration. > > Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> > --- > arch/powerpc/kernel/entry_32.S | 35 +++++++++++++++++++++++++++++++++++ > 1 files changed, 35 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S > index 56212bc..0cdd27d 100644 > --- a/arch/powerpc/kernel/entry_32.S > +++ b/arch/powerpc/kernel/entry_32.S > @@ -850,6 +850,41 @@ resume_kernel: > > /* interrupts are hard-disabled at this point */ > restore: > + lwz r3,_MSR(r1) /* Returning to user mode? */ > + andi. r0,r3,MSR_PR > + bne 1f .../... Wouldn't it be better to use resume_kernel here ? IE. We already have restore_user vs. resume_kernel labels, including the PR test. In the !PREEMPT case, resume_kernel is empty but it's there, so you can just "populate" it, ie, something like: restore_user: ... existing dbcr0 stuff ... b restore resume_kernel: <-- removed the ifdef CONFIG_PREEMPT ... Do the stack store business here... #ifdef CONFIG_PREEMPT ... move the preempt code here... #endif restore: ... existing stuff ... Also, the added advantage is that the code to calc the thread info pointer and load the TI_FLAG can be shared now between your stuff and preempt, ie: resume_kernel: rlwinm r9,r1,0,0,(31-THREAD_SHIFT) lwz r0,TI_FLAGS(r9) andis. r0,r0,_TIF_EMULATE_STACK_STORE@h bne- emulate_stack_store #ifdef CONFIG_PREEMPT lwz r8,TI_PREEMPT(r9) <-- note use of r8 instead of r0, I -think- r8 can be clobbered here but pls dbl check cmpwi 0,r8,0 bne restore andi. r0,r0,_TIF_NEED_RESCHED etc... > + /* check current_thread_info, _TIF_EMULATE_STACK_STORE */ > + rlwinm r9,r1,0,0,(31-THREAD_SHIFT) > + lwz r0,TI_FLAGS(r9) > + andis. r0,r0,_TIF_EMULATE_STACK_STORE@h > + beq+ 1f > + > + addi r9,r1,INT_FRAME_SIZE /* Get the kprobed function entry */ > + > + lwz r3,GPR1(r1) > + subi r3,r3,INT_FRAME_SIZE /* dst: Allocate a trampoline exception frame */ > + mr r4,r1 /* src: current exception frame */ > + li r5,INT_FRAME_SIZE /* size: INT_FRAME_SIZE */ > + mr r1,r3 /* Reroute the trampoline frame to r1 */ > + bl memcpy /* Copy from the original to the trampoline */ > + > + /* Do real store operation to complete stwu */ > + lwz r5,GPR1(r1) > + stw r9,0(r5) Ok, I think I -finally- understand your trick of using r1 + INT_FRAME_SIZE as the value to store :-) It makes sense and it's actually a nice hack :-) I would recommend that in the C code part of the emulation, you add some sanity checking to make sure we don't overflow the kernel stack etc... it should come in generally handy especially if what's your trying to debug with kprobes is a kernel stack overflow :-) > + /* Clear _TIF_EMULATE_STACK_STORE flag */ > + rlwinm r9,r1,0,0,(31-THREAD_SHIFT) > + lis r11,_TIF_EMULATE_STACK_STORE@h > + addi r9,r9,TI_FLAGS > +0: lwarx r8,0,r9 > + andc r8,r8,r11 > +#ifdef CONFIG_IBM405_ERR77 > + dcbt 0,r9 > +#endif > + stwcx. r8,0,r9 > + bne- 0b > +1: > + > #ifdef CONFIG_44x > BEGIN_MMU_FTR_SECTION > b 1f BTW. Are you going to do a ppc64 variant of that patch ? Cheers, Ben.
On 05/10/2012 11:50 AM, Benjamin Herrenschmidt wrote: > On Thu, 2011-12-15 at 19:00 +0800, Tiejun Chen wrote: >> We can't emulate stwu since that may corrupt current exception stack. >> So we will have to do real store operation in the exception return code. >> >> Firstly we'll allocate a trampoline exception frame below the kprobed >> function stack and copy the current exception frame to the trampoline. >> Then we can do this real store operation to implement 'stwu', and reroute >> the trampoline frame to r1 to complete this exception migration. >> >> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> >> --- >> arch/powerpc/kernel/entry_32.S | 35 +++++++++++++++++++++++++++++++++++ >> 1 files changed, 35 insertions(+), 0 deletions(-) >> >> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S >> index 56212bc..0cdd27d 100644 >> --- a/arch/powerpc/kernel/entry_32.S >> +++ b/arch/powerpc/kernel/entry_32.S >> @@ -850,6 +850,41 @@ resume_kernel: >> >> /* interrupts are hard-disabled at this point */ >> restore: >> + lwz r3,_MSR(r1) /* Returning to user mode? */ >> + andi. r0,r3,MSR_PR >> + bne 1f > > .../... > Sorry for this delay response since I can't take time to do this last week :( > Wouldn't it be better to use resume_kernel here ? Agreed :) > > IE. We already have restore_user vs. resume_kernel labels, including > the PR test. In the !PREEMPT case, resume_kernel is empty but it's > there, so you can just "populate" it, ie, something like: > > restore_user: > ... existing dbcr0 stuff ... > b restore > > resume_kernel: <-- removed the ifdef CONFIG_PREEMPT > > ... Do the stack store business here... > > #ifdef CONFIG_PREEMPT > ... move the preempt code here... > #endif > > restore: > ... existing stuff ... > > Also, the added advantage is that the code to calc > the thread info pointer and load the TI_FLAG can be > shared now between your stuff and preempt, ie: > > resume_kernel: > rlwinm r9,r1,0,0,(31-THREAD_SHIFT) > lwz r0,TI_FLAGS(r9) > andis. r0,r0,_TIF_EMULATE_STACK_STORE@h > bne- emulate_stack_store > #ifdef CONFIG_PREEMPT > lwz r8,TI_PREEMPT(r9) <-- note use of r8 instead of r0, > I -think- r8 can be clobbered > here but pls dbl check Its should be safe. > cmpwi 0,r8,0 > bne restore > andi. r0,r0,_TIF_NEED_RESCHED > etc... Please check if next, v3, is fine. > >> + /* check current_thread_info, _TIF_EMULATE_STACK_STORE */ >> + rlwinm r9,r1,0,0,(31-THREAD_SHIFT) >> + lwz r0,TI_FLAGS(r9) >> + andis. r0,r0,_TIF_EMULATE_STACK_STORE@h >> + beq+ 1f >> + >> + addi r9,r1,INT_FRAME_SIZE /* Get the kprobed function entry */ >> + >> + lwz r3,GPR1(r1) >> + subi r3,r3,INT_FRAME_SIZE /* dst: Allocate a trampoline exception frame */ >> + mr r4,r1 /* src: current exception frame */ >> + li r5,INT_FRAME_SIZE /* size: INT_FRAME_SIZE */ >> + mr r1,r3 /* Reroute the trampoline frame to r1 */ >> + bl memcpy /* Copy from the original to the trampoline */ >> + >> + /* Do real store operation to complete stwu */ >> + lwz r5,GPR1(r1) >> + stw r9,0(r5) > > Ok, I think I -finally- understand your trick of using r1 + > INT_FRAME_SIZE as the value to store :-) It makes sense and it's > actually a nice hack :-) > > I would recommend that in the C code part of the emulation, you > add some sanity checking to make sure we don't overflow the > kernel stack etc... it should come in generally handy especially > if what's your trying to debug with kprobes is a kernel stack > overflow :-) Added. > >> + /* Clear _TIF_EMULATE_STACK_STORE flag */ >> + rlwinm r9,r1,0,0,(31-THREAD_SHIFT) >> + lis r11,_TIF_EMULATE_STACK_STORE@h >> + addi r9,r9,TI_FLAGS >> +0: lwarx r8,0,r9 >> + andc r8,r8,r11 >> +#ifdef CONFIG_IBM405_ERR77 >> + dcbt 0,r9 >> +#endif >> + stwcx. r8,0,r9 >> + bne- 0b >> +1: >> + >> #ifdef CONFIG_44x >> BEGIN_MMU_FTR_SECTION >> b 1f > > BTW. Are you going to do a ppc64 variant of that patch ? I'd like to go ppc64 ASAP once we did on ppc32 is good enough :) Tiejun > > Cheers, > Ben. > > > >
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S index 56212bc..0cdd27d 100644 --- a/arch/powerpc/kernel/entry_32.S +++ b/arch/powerpc/kernel/entry_32.S @@ -850,6 +850,41 @@ resume_kernel: /* interrupts are hard-disabled at this point */ restore: + lwz r3,_MSR(r1) /* Returning to user mode? */ + andi. r0,r3,MSR_PR + bne 1f + /* check current_thread_info, _TIF_EMULATE_STACK_STORE */ + rlwinm r9,r1,0,0,(31-THREAD_SHIFT) + lwz r0,TI_FLAGS(r9) + andis. r0,r0,_TIF_EMULATE_STACK_STORE@h + beq+ 1f + + addi r9,r1,INT_FRAME_SIZE /* Get the kprobed function entry */ + + lwz r3,GPR1(r1) + subi r3,r3,INT_FRAME_SIZE /* dst: Allocate a trampoline exception frame */ + mr r4,r1 /* src: current exception frame */ + li r5,INT_FRAME_SIZE /* size: INT_FRAME_SIZE */ + mr r1,r3 /* Reroute the trampoline frame to r1 */ + bl memcpy /* Copy from the original to the trampoline */ + + /* Do real store operation to complete stwu */ + lwz r5,GPR1(r1) + stw r9,0(r5) + + /* Clear _TIF_EMULATE_STACK_STORE flag */ + rlwinm r9,r1,0,0,(31-THREAD_SHIFT) + lis r11,_TIF_EMULATE_STACK_STORE@h + addi r9,r9,TI_FLAGS +0: lwarx r8,0,r9 + andc r8,r8,r11 +#ifdef CONFIG_IBM405_ERR77 + dcbt 0,r9 +#endif + stwcx. r8,0,r9 + bne- 0b +1: + #ifdef CONFIG_44x BEGIN_MMU_FTR_SECTION b 1f
We can't emulate stwu since that may corrupt current exception stack. So we will have to do real store operation in the exception return code. Firstly we'll allocate a trampoline exception frame below the kprobed function stack and copy the current exception frame to the trampoline. Then we can do this real store operation to implement 'stwu', and reroute the trampoline frame to r1 to complete this exception migration. Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> --- arch/powerpc/kernel/entry_32.S | 35 +++++++++++++++++++++++++++++++++++ 1 files changed, 35 insertions(+), 0 deletions(-)