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

But, I feel it helps make it easy to follow the code when referring to 
the ISA. 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()?


- Naveen