[PATCH] PPC: sstep.c: Add modsw, moduw instruction emulation

[PATCH] PPC: sstep.c: Add modsw, moduw instruction emulation

[PrasannaKumar Muralidharan]

Add modsw and moduw instruction emulation support to analyse_instr.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
---
 arch/powerpc/lib/sstep.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 9c78a9c..5acef72 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1148,6 +1148,15 @@ int __kprobes analyse_instr(struct instruction_op *op, struct pt_regs *regs,
 				(int) regs->gpr[rb];
 			goto arith_done;
 
+		case 779:	/* modsw */
+			regs->gpr[rd] = (int) regs->gpr[ra] %
+				(int) regs->gpr[rb];
+			goto arith_done;
+
+		case 267:	/* moduw */
+			regs->gpr[rd] = (unsigned int) regs->gpr[ra] %
+				(unsigned int) regs->gpr[rb];
+			goto arith_done;
 
 /*
  * Logical instructions
-- 
2.9.3

Re: [PATCH] PPC: sstep.c: Add modsw, moduw instruction emulation

[Naveen N. Rao]

On 2016/12/04 10:25PM, PrasannaKumar Muralidharan wrote:
> Add modsw and moduw instruction emulation support to analyse_instr.
> 
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>

Hi Prasanna,
Thanks for the patch! A few minor comments below...

> ---
>  arch/powerpc/lib/sstep.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index 9c78a9c..5acef72 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1148,6 +1148,15 @@ int __kprobes analyse_instr(struct instruction_op *op, struct pt_regs *regs,
>  				(int) regs->gpr[rb];
>  			goto arith_done;
> 
> +		case 779:	/* modsw */
> +			regs->gpr[rd] = (int) regs->gpr[ra] %
> +				(int) regs->gpr[rb];
> +			goto arith_done;

Since these instructions don't update CR, you can directly goto 
instr_done.

> +
> +		case 267:	/* moduw */

Please move this case further up so that the extended opcodes are in 
numerical order.


- Naveen

Re: [PATCH] PPC: sstep.c: Add modsw, moduw instruction emulation

[PrasannaKumar Muralidharan]

Hi Naveen,

Thanks for the review.

>> ---
>>  arch/powerpc/lib/sstep.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
>> index 9c78a9c..5acef72 100644
>> --- a/arch/powerpc/lib/sstep.c
>> +++ b/arch/powerpc/lib/sstep.c
>> @@ -1148,6 +1148,15 @@ int __kprobes analyse_instr(struct instruction_op *op, struct pt_regs *regs,
>>                               (int) regs->gpr[rb];
>>                       goto arith_done;
>>
>> +             case 779:       /* modsw */
>> +                     regs->gpr[rd] = (int) regs->gpr[ra] %
>> +                             (int) regs->gpr[rb];
>> +                     goto arith_done;
>
> Since these instructions don't update CR, you can directly goto
> instr_done.

Sure. Will use that.

>> +
>> +             case 267:       /* moduw */
>
> Please move this case further up so that the extended opcodes are in
> numerical order.

Placed it after divide instruction as it appeared logical. Also placed
267 below 779 as it is the order in which the instructions are
documented in the ISA book. This may help in finding related
instructions together. If this style is not preferred I can arrange it
in numerical order.

Regards,
PrasannaKumar

Re: [PATCH] PPC: sstep.c: Add modsw, moduw instruction emulation

[Naveen N. Rao]

On 2016/12/06 01:21AM, PrasannaKumar Muralidharan wrote:
> >> +
> >> +             case 267:       /* moduw */
> >
> > Please move this case further up so that the extended opcodes are in
> > numerical order.
> 
> Placed it after divide instruction as it appeared logical. Also placed
> 267 below 779 as it is the order in which the instructions are
> documented in the ISA book. This may help in finding related
> instructions together. If this style is not preferred I can arrange it
> in numerical order.

I guessed as much, but if you look at the existing function, you'll see 
that things have been arranged in numerical order. As such, it's best to 
stick to that convention.

- Naveen

Re: [PATCH] PPC: sstep.c: Add modsw, moduw instruction emulation

[Naveen N. Rao]

By the way, I missed mentioning previously: please use 'powerpc: ' 
prefix for the subject, rather than PPC.


On 2016/12/04 10:25PM, PrasannaKumar Muralidharan wrote:
> Add modsw and moduw instruction emulation support to analyse_instr.

And, it will be better if you can briefly describe what these functions 
do for the benefit of others.

- Naveen

Re: [PATCH] PPC: sstep.c: Add modsw, moduw instruction emulation

[PrasannaKumar Muralidharan]

> I guessed as much, but if you look at the existing function, you'll see
> that things have been arranged in numerical order. As such, it's best to
> stick to that convention.

Makes sense. Will do.

Re: [PATCH] PPC: sstep.c: Add modsw, moduw instruction emulation

[PrasannaKumar Muralidharan]

> By the way, I missed mentioning previously: please use 'powerpc: '
> prefix for the subject, rather than PPC.

I will change it. Wondering how they are different.

>> Add modsw and moduw instruction emulation support to analyse_instr.
>
> And, it will be better if you can briefly describe what these functions
> do for the benefit of others.

Sure. I will add description.

Re: [PATCH] PPC: sstep.c: Add modsw, moduw instruction emulation

[Naveen N. Rao]

On 2016/12/06 10:18PM, PrasannaKumar Muralidharan wrote:
> > By the way, I missed mentioning previously: please use 'powerpc: '
> > prefix for the subject, rather than PPC.
> 
> I will change it. Wondering how they are different.

It's by convention. Maintainers are picky ;)

- Naveen