Re: [PATCH 1/2] powerpc/sstep: Add emulation support for ‘setb’ instruction

[Daniel Axtens <dja@axtens.net>]

Sathvika Vasireddy <sathvika@linux.vnet.ibm.com> writes:

> This adds emulation support for the following instruction:
>    * Set Boolean (setb)
>
> Signed-off-by: Sathvika Vasireddy <sathvika@linux.vnet.ibm.com>
> ---
>  arch/powerpc/lib/sstep.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index c6aebc149d14..263c613d7490 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1964,6 +1964,18 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
>  			op->val = ~(regs->gpr[rd] | regs->gpr[rb]);
>  			goto logical_done;
>  
> +		case 128:	/* setb */
> +			if (!cpu_has_feature(CPU_FTR_ARCH_300))
> +				goto unknown_opcode;

Ok, if I've understood correctly...

> +			ra = ra & ~0x3;

This masks off the bits of RA that are not part of BTF:

ra is in [0, 31] which is [0b00000, 0b11111]
Then ~0x3 = ~0b00011
ra = ra & 0b11100

This gives us then,
ra = btf << 2; or
btf = ra >> 2;

Let's then check to see if your calculations read the right fields.

> +			if ((regs->ccr) & (1 << (31 - ra)))
> +				op->val = -1;
> +			else if ((regs->ccr) & (1 << (30 - ra)))
> +				op->val = 1;
> +			else
> +				op->val = 0;


CR field:      7    6    5    4    3    2    1    0
bit:          0123 0123 0123 0123 0123 0123 0123 0123
normal bit #: 0.....................................31
ibm bit #:   31.....................................0

If btf = 0, ra = 0, check normal bits 31 and 30, which are both in CR0.
CR field:      7    6    5    4    3    2    1    0
bit:          0123 0123 0123 0123 0123 0123 0123 0123
                                                   ^^

If btf = 7, ra = 0b11100 = 28, so check normal bits 31-28 and 30-28,
which are 3 and 2.

CR field:      7    6    5    4    3    2    1    0
bit:          0123 0123 0123 0123 0123 0123 0123 0123
                ^^

If btf = 3, ra = 0b01100 = 12, for normal bits 19 and 18:

CR field:      7    6    5    4    3    2    1    0
bit:          0123 0123 0123 0123 0123 0123 0123 0123
                                    ^^

So yes, your calculations, while I struggle to follow _how_ they work,
do in fact seem to work.

Checkpatch does have one complaint:

CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'regs->ccr'
#30: FILE: arch/powerpc/lib/sstep.c:1971:
+			if ((regs->ccr) & (1 << (31 - ra)))

I don't really mind the parenteses: I think you are safe to ignore
checkpatch here unless someone else complains :)

If you do end up respinning the patch, I think it would be good to make
the maths a bit clearer. I think it works because a left shift of 2 is
the same as multiplying by 4, but it would be easier to follow if you
used a temporary variable for btf.

However, I do think this is still worth adding to the kernel either way,
so:

Reviewed-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel

> +			goto compute_done;
> +
>  		case 154:	/* prtyw */
>  			do_prty(regs, op, regs->gpr[rd], 32);
>  			goto logical_done_nocc;
> -- 
> 2.16.4