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

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao at linux.ibm.com> writes:
> Michael Ellerman wrote:
>> "Naveen N. Rao" <naveen.n.rao at linux.ibm.com> writes:
>>> Daniel Axtens wrote:
>>>> Sathvika Vasireddy <sathvika at linux.vnet.ibm.com> writes:
>>>> 
>>>>> This adds emulation support for the following instruction:
>>>>>    * Set Boolean (setb)
>>>>>
>>>>> Signed-off-by: Sathvika Vasireddy <sathvika at 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.

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.

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

cheers