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

Daniel Axtens wrote:

Sathvika Vasireddy [off-list ref] writes:

quoted

This adds emulation support for the following instruction:
   * Set Boolean (setb)

Signed-off-by: Sathvika Vasireddy <redacted>
---
 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...

quoted

+			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.

quoted

+			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.

Indeed. I wonder if it is better to follow the ISA itself. Per the ISA, 
the bit we are interested in is:
	4 x BFA + 32

So, if we use that along with the PPC_BIT() macro, we get:
	if (regs->ccr & PPC_BIT(ra + 32))

quoted

+			goto compute_done;
+

I can see why you thought this should be in the section with other 
logical instructions. However, since this instruction does not modify CR 
itself, this is probably better placed earlier -- somewhere near 'mfcr' 
instruction emulation.


- Naveen