Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1513748 > unrolled thread

[PATCH 1/3] powerpc: Emulation support for load/store instructions on LE

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/3] powerpc: Emulation support for load/store instructions on LE Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> - 2016-11-02 10:00 +0100
    Re: [PATCH 1/3] powerpc: Emulation support for load/store  instructions on LE Anton Blanchard <anton@samba.org> - 2016-11-02 22:10 +0100
      Re: [PATCH 1/3] powerpc: Emulation support for load/store  instructions on LE Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> - 2016-11-03 06:50 +0100
        Re: [PATCH 1/3] powerpc: Emulation support for load/store instructions on LE Michael Ellerman <mpe@ellerman.id.au> - 2016-11-03 10:50 +0100
          Re: [PATCH 1/3] powerpc: Emulation support for load/store  instructions on LE Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> - 2016-11-03 11:30 +0100
            Re: [PATCH 1/3] powerpc: Emulation support for load/store  instructions on LE Andrew Donnellan <andrew.donnellan@au1.ibm.com> - 2016-11-04 03:10 +0100
              Re: [PATCH 1/3] powerpc: Emulation support for load/store  instructions on LE Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> - 2016-11-04 06:40 +0100
                Re: [PATCH 1/3] powerpc: Emulation support for load/store  instructions on LE Anton Blanchard <anton@samba.org> - 2016-11-05 20:40 +0100
                  Re: [PATCH 1/3] powerpc: Emulation support for load/store  instructions on LE Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> - 2016-11-06 20:10 +0100

#1513748 — [PATCH 1/3] powerpc: Emulation support for load/store instructions on LE

emulate_step() uses a number of underlying kernel functions that were
initially not enabled for LE. This has been rectified since. So, fix
emulate_step() for LE for the corresponding instructions.

Reported-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 arch/powerpc/lib/sstep.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 3362299..6ca3b90 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1807,8 +1807,6 @@ int __kprobes emulate_step(struct pt_regs *regs, unsigned int instr)
 		goto instr_done;
 
 	case LARX:
-		if (regs->msr & MSR_LE)
-			return 0;
 		if (op.ea & (size - 1))
 			break;		/* can't handle misaligned */
 		err = -EFAULT;
@@ -1832,8 +1830,6 @@ int __kprobes emulate_step(struct pt_regs *regs, unsigned int instr)
 		goto ldst_done;
 
 	case STCX:
-		if (regs->msr & MSR_LE)
-			return 0;
 		if (op.ea & (size - 1))
 			break;		/* can't handle misaligned */
 		err = -EFAULT;
@@ -1859,8 +1855,6 @@ int __kprobes emulate_step(struct pt_regs *regs, unsigned int instr)
 		goto ldst_done;
 
 	case LOAD:
-		if (regs->msr & MSR_LE)
-			return 0;
 		err = read_mem(&regs->gpr[op.reg], op.ea, size, regs);
 		if (!err) {
 			if (op.type & SIGNEXT)
@@ -1872,8 +1866,6 @@ int __kprobes emulate_step(struct pt_regs *regs, unsigned int instr)
 
 #ifdef CONFIG_PPC_FPU
 	case LOAD_FP:
-		if (regs->msr & MSR_LE)
-			return 0;
 		if (size == 4)
 			err = do_fp_load(op.reg, do_lfs, op.ea, size, regs);
 		else
@@ -1882,15 +1874,11 @@ int __kprobes emulate_step(struct pt_regs *regs, unsigned int instr)
 #endif
 #ifdef CONFIG_ALTIVEC
 	case LOAD_VMX:
-		if (regs->msr & MSR_LE)
-			return 0;
 		err = do_vec_load(op.reg, do_lvx, op.ea & ~0xfUL, regs);
 		goto ldst_done;
 #endif
 #ifdef CONFIG_VSX
 	case LOAD_VSX:
-		if (regs->msr & MSR_LE)
-			return 0;
 		err = do_vsx_load(op.reg, do_lxvd2x, op.ea, regs);
 		goto ldst_done;
 #endif
@@ -1913,8 +1901,6 @@ int __kprobes emulate_step(struct pt_regs *regs, unsigned int instr)
 		goto instr_done;
 
 	case STORE:
-		if (regs->msr & MSR_LE)
-			return 0;
 		if ((op.type & UPDATE) && size == sizeof(long) &&
 		    op.reg == 1 && op.update_reg == 1 &&
 		    !(regs->msr & MSR_PR) &&
@@ -1927,8 +1913,6 @@ int __kprobes emulate_step(struct pt_regs *regs, unsigned int instr)
 
 #ifdef CONFIG_PPC_FPU
 	case STORE_FP:
-		if (regs->msr & MSR_LE)
-			return 0;
 		if (size == 4)
 			err = do_fp_store(op.reg, do_stfs, op.ea, size, regs);
 		else
@@ -1937,15 +1921,11 @@ int __kprobes emulate_step(struct pt_regs *regs, unsigned int instr)
 #endif
 #ifdef CONFIG_ALTIVEC
 	case STORE_VMX:
-		if (regs->msr & MSR_LE)
-			return 0;
 		err = do_vec_store(op.reg, do_stvx, op.ea & ~0xfUL, regs);
 		goto ldst_done;
 #endif
 #ifdef CONFIG_VSX
 	case STORE_VSX:
-		if (regs->msr & MSR_LE)
-			return 0;
 		err = do_vsx_store(op.reg, do_stxvd2x, op.ea, regs);
 		goto ldst_done;
 #endif
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1514124 — Re: [PATCH 1/3] powerpc: Emulation support for load/store instructions on LE

Hi Ravi,

> emulate_step() uses a number of underlying kernel functions that were
> initially not enabled for LE. This has been rectified since. So, fix
> emulate_step() for LE for the corresponding instructions.

Thanks. Should this be queued up for stable?

Anton

> Reported-by: Anton Blanchard <anton@samba.org>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
>  arch/powerpc/lib/sstep.c | 20 --------------------
>  1 file changed, 20 deletions(-)
> 
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index 3362299..6ca3b90 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1807,8 +1807,6 @@ int __kprobes emulate_step(struct pt_regs
> *regs, unsigned int instr) goto instr_done;
>  
>  	case LARX:
> -		if (regs->msr & MSR_LE)
> -			return 0;
>  		if (op.ea & (size - 1))
>  			break;		/* can't handle
> misaligned */ err = -EFAULT;
> @@ -1832,8 +1830,6 @@ int __kprobes emulate_step(struct pt_regs
> *regs, unsigned int instr) goto ldst_done;
>  
>  	case STCX:
> -		if (regs->msr & MSR_LE)
> -			return 0;
>  		if (op.ea & (size - 1))
>  			break;		/* can't handle
> misaligned */ err = -EFAULT;
> @@ -1859,8 +1855,6 @@ int __kprobes emulate_step(struct pt_regs
> *regs, unsigned int instr) goto ldst_done;
>  
>  	case LOAD:
> -		if (regs->msr & MSR_LE)
> -			return 0;
>  		err = read_mem(&regs->gpr[op.reg], op.ea, size,
> regs); if (!err) {
>  			if (op.type & SIGNEXT)
> @@ -1872,8 +1866,6 @@ int __kprobes emulate_step(struct pt_regs
> *regs, unsigned int instr) 
>  #ifdef CONFIG_PPC_FPU
>  	case LOAD_FP:
> -		if (regs->msr & MSR_LE)
> -			return 0;
>  		if (size == 4)
>  			err = do_fp_load(op.reg, do_lfs, op.ea,
> size, regs); else
> @@ -1882,15 +1874,11 @@ int __kprobes emulate_step(struct pt_regs
> *regs, unsigned int instr) #endif
>  #ifdef CONFIG_ALTIVEC
>  	case LOAD_VMX:
> -		if (regs->msr & MSR_LE)
> -			return 0;
>  		err = do_vec_load(op.reg, do_lvx, op.ea & ~0xfUL,
> regs); goto ldst_done;
>  #endif
>  #ifdef CONFIG_VSX
>  	case LOAD_VSX:
> -		if (regs->msr & MSR_LE)
> -			return 0;
>  		err = do_vsx_load(op.reg, do_lxvd2x, op.ea, regs);
>  		goto ldst_done;
>  #endif
> @@ -1913,8 +1901,6 @@ int __kprobes emulate_step(struct pt_regs
> *regs, unsigned int instr) goto instr_done;
>  
>  	case STORE:
> -		if (regs->msr & MSR_LE)
> -			return 0;
>  		if ((op.type & UPDATE) && size == sizeof(long) &&
>  		    op.reg == 1 && op.update_reg == 1 &&
>  		    !(regs->msr & MSR_PR) &&
> @@ -1927,8 +1913,6 @@ int __kprobes emulate_step(struct pt_regs
> *regs, unsigned int instr) 
>  #ifdef CONFIG_PPC_FPU
>  	case STORE_FP:
> -		if (regs->msr & MSR_LE)
> -			return 0;
>  		if (size == 4)
>  			err = do_fp_store(op.reg, do_stfs, op.ea,
> size, regs); else
> @@ -1937,15 +1921,11 @@ int __kprobes emulate_step(struct pt_regs
> *regs, unsigned int instr) #endif
>  #ifdef CONFIG_ALTIVEC
>  	case STORE_VMX:
> -		if (regs->msr & MSR_LE)
> -			return 0;
>  		err = do_vec_store(op.reg, do_stvx, op.ea & ~0xfUL,
> regs); goto ldst_done;
>  #endif
>  #ifdef CONFIG_VSX
>  	case STORE_VSX:
> -		if (regs->msr & MSR_LE)
> -			return 0;
>  		err = do_vsx_store(op.reg, do_stxvd2x, op.ea, regs);
>  		goto ldst_done;
>  #endif

[toc] | [prev] | [next] | [standalone]

#1514301 — Re: [PATCH 1/3] powerpc: Emulation support for load/store instructions on LE

On Thursday 03 November 2016 02:34 AM, Anton Blanchard wrote:
> Hi Ravi,
>
>> emulate_step() uses a number of underlying kernel functions that were
>> initially not enabled for LE. This has been rectified since. So, fix
>> emulate_step() for LE for the corresponding instructions.
> Thanks. Should this be queued up for stable?

Thanks Anton. Yes, this should go in stable.

-Ravi

[toc] | [prev] | [next] | [standalone]

#1514417

Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> writes:

> On Thursday 03 November 2016 02:34 AM, Anton Blanchard wrote:
>> Hi Ravi,
>>
>>> emulate_step() uses a number of underlying kernel functions that were
>>> initially not enabled for LE. This has been rectified since. So, fix
>>> emulate_step() for LE for the corresponding instructions.
>> Thanks. Should this be queued up for stable?
>
> Thanks Anton. Yes, this should go in stable.

It's fairly big for stable. Does it fix an actual bug? If so what, and
how bad is it, what's the user impact.

Can you also pinpoint which commit it "fixes"?

cheers

[toc] | [prev] | [next] | [standalone]

#1514437 — Re: [PATCH 1/3] powerpc: Emulation support for load/store instructions on LE

On Thursday 03 November 2016 03:18 PM, Michael Ellerman wrote:
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> writes:
>
>> On Thursday 03 November 2016 02:34 AM, Anton Blanchard wrote:
>>> Hi Ravi,
>>>
>>>> emulate_step() uses a number of underlying kernel functions that were
>>>> initially not enabled for LE. This has been rectified since. So, fix
>>>> emulate_step() for LE for the corresponding instructions.
>>> Thanks. Should this be queued up for stable?
>> Thanks Anton. Yes, this should go in stable.
> It's fairly big for stable. Does it fix an actual bug? If so what, and
> how bad is it, what's the user impact.

Hi Michael,

Yes, kernel-space hw-breakpoint feature is broken on LE without this.

Actually, there is no specific commit that introduced this. Back
in 2010, Paul Mackerras has added emulation support for load/store
instructions for BE. hw-breakpoint was also develpoed by K.Prasad
in the same timeframe.

Kernel-space hw-breakpoint emulates causative instruction before
notifying to user. As emulate_step is never enabled for LE, kernel-
space hw-breakpoint is always broken on LE.

-Ravi

> Can you also pinpoint which commit it "fixes"?
>
> cheers
>

[toc] | [prev] | [next] | [standalone]

#1514965 — Re: [PATCH 1/3] powerpc: Emulation support for load/store instructions on LE

On 03/11/16 21:27, Ravi Bangoria wrote:
> Yes, kernel-space hw-breakpoint feature is broken on LE without this.

Is there any actual user-visible feature that depends on this, or is 
this solely for debugging and development purposes?

It would of course be *nice* to have it in stable trees (particularly so 
we pick it up in distros) but I'm not convinced that enabling HW 
breakpoints on a platform where it has *never* worked qualifies as an 
"actual bug".

(BTW many thanks for fixing this - I had a shot at it late last year but 
never quite got there!)

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

[toc] | [prev] | [next] | [standalone]

#1515018 — Re: [PATCH 1/3] powerpc: Emulation support for load/store instructions on LE

On Friday 04 November 2016 07:37 AM, Andrew Donnellan wrote:
> On 03/11/16 21:27, Ravi Bangoria wrote:
>> Yes, kernel-space hw-breakpoint feature is broken on LE without this.
>
> Is there any actual user-visible feature that depends on this, or is this solely for debugging and development purposes?
>
> It would of course be *nice* to have it in stable trees (particularly so we pick it up in distros) but I'm not convinced that enabling HW breakpoints on a platform where it has *never* worked qualifies as an "actual bug".
>
> (BTW many thanks for fixing this - I had a shot at it late last year but never quite got there!)

Thanks Andrew,

kprobe, uprobe, hw-breakpoint and xmon are the only user of emulate_step.

Kprobe / uprobe single-steps instruction if they can't emulate it, so there
is no problem with them. As I mention, hw-breakpoint is broken. However
I'm not sure about xmon, I need to check that.

So yes, there is no user-visible feature that depends on this.

-Ravi

[toc] | [prev] | [next] | [standalone]

#1515617 — Re: [PATCH 1/3] powerpc: Emulation support for load/store instructions on LE

Hi,

> kprobe, uprobe, hw-breakpoint and xmon are the only user of
> emulate_step.
> 
> Kprobe / uprobe single-steps instruction if they can't emulate it, so
> there is no problem with them. As I mention, hw-breakpoint is broken.
> However I'm not sure about xmon, I need to check that.

I was mostly concerned that it would impact kprobes. Sounds like we are
ok there.

> So yes, there is no user-visible feature that depends on this.

Aren't hardware breakpoints exposed via perf? I'd call perf
user-visible.

Anton

[toc] | [prev] | [next] | [standalone]

#1515737 — Re: [PATCH 1/3] powerpc: Emulation support for load/store instructions on LE

On Sunday 06 November 2016 01:01 AM, Anton Blanchard wrote:
> Hi,
>
>> kprobe, uprobe, hw-breakpoint and xmon are the only user of
>> emulate_step.
>>
>> Kprobe / uprobe single-steps instruction if they can't emulate it, so
>> there is no problem with them. As I mention, hw-breakpoint is broken.
>> However I'm not sure about xmon, I need to check that.
> I was mostly concerned that it would impact kprobes. Sounds like we are
> ok there.
>
>> So yes, there is no user-visible feature that depends on this.
> Aren't hardware breakpoints exposed via perf? I'd call perf
> user-visible.


Thanks Anton, That's a good catch. I tried this on ppc64le:

  $ sudo cat /proc/kallsyms  | grep pid_max
    c00000000116998c D pid_max

  $ sudo ./perf record -a --event=mem:0xc00000000116998c sleep 10


Before patch:
  It does not record any data and throws below warning.

  $ dmesg
    [  817.895573] Unable to handle hardware breakpoint. Breakpoint at 0xc00000000116998c will be disabled.
    [  817.895581] ------------[ cut here ]------------
    [  817.895588] WARNING: CPU: 24 PID: 2032 at arch/powerpc/kernel/hw_breakpoint.c:277 hw_breakpoint_handler+0x124/0x230
    ...

After patch:
  It records data properly.

  $ sudo ./perf report --stdio
    ...
    # Samples: 36  of event 'mem:0xc00000000116998c'
    # Event count (approx.): 36
    #
    # Overhead  Command        Shared Object     Symbol      
    # ........  .............  ................  .............
    #
        63.89%  kdumpctl       [kernel.vmlinux]  [k] alloc_pid
        27.78%  opal_errd      [kernel.vmlinux]  [k] alloc_pid
         5.56%  kworker/u97:4  [kernel.vmlinux]  [k] alloc_pid
         2.78%  systemd        [kernel.vmlinux]  [k] alloc_pid


-Ravi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web