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

["Naveen N. Rao" <naveen.n.rao@linux.ibm.com>]

Daniel Axtens wrote:
> 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.

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))


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