[3/5] powerpc/lib/sstep: Add bpermd instruction emulation

Message ID 20170713032548.451-3-matthew.brown.dev@gmail.com
State Superseded
Headers show

Commit Message

Matt Brown July 13, 2017, 3:25 a.m.
This adds emulation for the bpermd instruction.

Signed-off-by: Matt Brown <matthew.brown.dev@gmail.com>
---
 arch/powerpc/lib/sstep.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Gabriel Paubert July 13, 2017, 7:16 a.m. | #1
On Thu, Jul 13, 2017 at 01:25:46PM +1000, Matt Brown wrote:
> This adds emulation for the bpermd instruction.
> 
> Signed-off-by: Matt Brown <matthew.brown.dev@gmail.com>
> ---
>  arch/powerpc/lib/sstep.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index cf69987..603654d 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -637,6 +637,21 @@ static nokprobe_inline void do_popcnt(struct pt_regs *regs, unsigned long v1,
>  	regs->gpr[ra] = out_val;
>  }
>  
> +static nokprobe_inline void do_bpermd(struct pt_regs *regs, unsigned long v1,
> +				unsigned long v2, int ra)
> +{
> +	unsigned int idx, i;
> +	unsigned char perm;
> +
> +	perm = 0x0;
> +	for (i = 0; i < 8; i++) {
> +		idx = (v1 >> (i * 8)) & 0xff;
> +		if (idx < 64)
> +			perm |= (v2 & (1 << idx)) >> (idx - i);
> +	}
> +	regs->gpr[ra] = 0 | perm;

Huh? What's the point of doing an or with 0?

The compiler will eliminate it, but it just confuses the reader.

	Gabriel
> +}
> +
>  static nokprobe_inline int trap_compare(long v1, long v2)
>  {
>  	int ret = 0;
> @@ -1274,6 +1289,14 @@ int analyse_instr(struct instruction_op *op, struct pt_regs *regs,
>  			goto logical_done;
>  #endif
>  
> +#ifdef __powerpc64__
> +		case 2396736:	/* bpermd */
> +			val = regs->gpr[rd];
> +			val2 = regs->gpr[rb];
> +			do_bpermd(regs, val, val2, ra);
> +			goto logical_done;
> +#endif
> +
>  /*
>   * Shift instructions
>   */
> -- 
> 2.9.3
Segher Boessenkool July 13, 2017, 7:28 a.m. | #2
On Thu, Jul 13, 2017 at 01:25:46PM +1000, Matt Brown wrote:
> +static nokprobe_inline void do_bpermd(struct pt_regs *regs, unsigned long v1,
> +				unsigned long v2, int ra)
> +{
> +	unsigned int idx, i;
> +	unsigned char perm;
> +
> +	perm = 0x0;
> +	for (i = 0; i < 8; i++) {
> +		idx = (v1 >> (i * 8)) & 0xff;
> +		if (idx < 64)
> +			perm |= (v2 & (1 << idx)) >> (idx - i);

That doesn't work I think, the bit numbers ("idx") are big-endian?

> +	}
> +	regs->gpr[ra] = 0 | perm;

And that is just silly :-)

> +}


Segher
Matt Brown July 14, 2017, 4:19 a.m. | #3
On Thu, Jul 13, 2017 at 5:28 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Thu, Jul 13, 2017 at 01:25:46PM +1000, Matt Brown wrote:
>> +static nokprobe_inline void do_bpermd(struct pt_regs *regs, unsigned long v1,
>> +                             unsigned long v2, int ra)
>> +{
>> +     unsigned int idx, i;
>> +     unsigned char perm;
>> +
>> +     perm = 0x0;
>> +     for (i = 0; i < 8; i++) {
>> +             idx = (v1 >> (i * 8)) & 0xff;
>> +             if (idx < 64)
>> +                     perm |= (v2 & (1 << idx)) >> (idx - i);
>
> That doesn't work I think, the bit numbers ("idx") are big-endian?

Why would it be big-endian? Wouldn't it be in the same endian form as the arch?
>
>> +     }
>> +     regs->gpr[ra] = 0 | perm;
>
> And that is just silly :-)
>

Yep that is silly.

Thanks,
Matt

>> +}
>
>
> Segher
Segher Boessenkool July 14, 2017, 4:17 p.m. | #4
On Fri, Jul 14, 2017 at 02:19:34PM +1000, Matt Brown wrote:
> >> +static nokprobe_inline void do_bpermd(struct pt_regs *regs, unsigned long v1,
> >> +                             unsigned long v2, int ra)
> >> +{
> >> +     unsigned int idx, i;
> >> +     unsigned char perm;
> >> +
> >> +     perm = 0x0;
> >> +     for (i = 0; i < 8; i++) {
> >> +             idx = (v1 >> (i * 8)) & 0xff;
> >> +             if (idx < 64)
> >> +                     perm |= (v2 & (1 << idx)) >> (idx - i);
> >
> > That doesn't work I think, the bit numbers ("idx") are big-endian?
> 
> Why would it be big-endian? Wouldn't it be in the same endian form as the arch?

Because that is what the ISA says.  Bit ordering is always BE.  If any
instruction behaves differently in LE mode that is explicitly described.

Please somehow test that the emulation works correctly, and describe
how you tested it, to give people the warm fuzzies.


Segher

Patch

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index cf69987..603654d 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -637,6 +637,21 @@  static nokprobe_inline void do_popcnt(struct pt_regs *regs, unsigned long v1,
 	regs->gpr[ra] = out_val;
 }
 
+static nokprobe_inline void do_bpermd(struct pt_regs *regs, unsigned long v1,
+				unsigned long v2, int ra)
+{
+	unsigned int idx, i;
+	unsigned char perm;
+
+	perm = 0x0;
+	for (i = 0; i < 8; i++) {
+		idx = (v1 >> (i * 8)) & 0xff;
+		if (idx < 64)
+			perm |= (v2 & (1 << idx)) >> (idx - i);
+	}
+	regs->gpr[ra] = 0 | perm;
+}
+
 static nokprobe_inline int trap_compare(long v1, long v2)
 {
 	int ret = 0;
@@ -1274,6 +1289,14 @@  int analyse_instr(struct instruction_op *op, struct pt_regs *regs,
 			goto logical_done;
 #endif
 
+#ifdef __powerpc64__
+		case 2396736:	/* bpermd */
+			val = regs->gpr[rd];
+			val2 = regs->gpr[rb];
+			do_bpermd(regs, val, val2, ra);
+			goto logical_done;
+#endif
+
 /*
  * Shift instructions
  */