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

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

Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.ibm.com> writes:
>> Michael Ellerman wrote:
>>> "Naveen N. Rao" <naveen.n.rao@linux.ibm.com> writes:
>>>> 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>
>>> ...
>>>>> 
>>>>> 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))
>>> 
>>> Use of PPC_BIT risks annoying your maintainer :)
>>
>> Uh oh... that isn't good :)
>>
>> I looked up previous discussions and I think I now understand why you 
>> don't prefer it.
> 
> Hah, I'd forgotten I'd written (ranted :D) about this in the past.
> 
>> But, I feel it helps make it easy to follow the code when referring to 
>> the ISA.
> 
> That's true. But I think that's much much less common than people
> reading the code in isolation.

I thought that isn't so for at least the instruction emulation 
infrastructure...

> 
> And ultimately it doesn't matter if the code (appears to) match the ISA,
> it matters that the code works. My worry is that too much use of those
> type of macros obscures what's actually happening.

... but, I agree on the above point. I can see why it is better to keep 
it simple.

I also see precedence for what both you and Segher are suggesting in the 
existing code in sstep.c

> 
>> I'm wondering if it is just the name you dislike and if so, 
>> does it make sense to rename PPC_BIT() to something else? We have 
>> BIT_ULL(), so perhaps BIT_MSB_ULL() or MSB_BIT_ULL()?
> 
> The name is part of it. But I don't really like BIT_ULL() either, it
> hides in a macro something that could just be there in front of you
> ie. (1ull << x).
> 
> 
> For this case of setb, I think I'd go with something like below. It
> doesn't exactly match the ISA, but I think there's minimal obfuscation
> of what's actually going on.
> 
>     	// ra is now bfa
> 	ra = (ra >> 2);
> 
> 	// Extract 4-bit CR field
> 	val = regs->ccr >> (CR0_SHIFT - 4 * ra);
> 
> 	if (val & 8)
> 		op->val = -1;
> 	else if (val & 4)
> 		op->val = 1;
> 	else
> 		op->val = 0;
> 
> 
> If anything could use a macro it would be the 8 and 4, eg. CR_LT, CR_GT.
> 
> Of course that's probably got a bug in it, because I just wrote it by
> eye and it's 11:28 pm :)

LGTM, thanks. I'll let Sathvika decide on which variant she wants to go 
with for v2 :)


- Naveen