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

[Daniel Axtens <dja@axtens.net>]

Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hi!
>
> On Fri, Apr 16, 2021 at 05:44:52PM +1000, Daniel Axtens wrote:
>> Sathvika Vasireddy <sathvika@linux.vnet.ibm.com> writes:
>> 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;
>
> Yes.  In effect, you want the offset in bits of the CR field, which is
> just fine like this.  But a comment would not hurt.
>
>> 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;
>
> It imo is clearer if written
>
> 			if ((regs->ccr << ra) & 0x80000000)
> 				op->val = -1;
> 			else if ((regs->ccr << ra) & 0x40000000)
> 				op->val = 1;
> 			else
> 				op->val = 0;
>
> but I guess not everyone agrees :-)
>
>> 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
>
> The bit numbers in CR fields are *always* numbered left-to-right.  I
> have never seen anyone use LE for it, anyway.
>
> Also, even people who write LE have the bigger end on the left normally
> (they just write some things right-to-left, and other things
> left-to-right).

Sorry, the numbers in the CR fields weren't meant to be especially
meaningful, I was just trying to convince myself that we referenced the
same bits doing the ISA way vs the way this code did it.

Kind regards,
Daniel
>
>> 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 :)
>
> I find them annoying.  If there are too many parentheses, it is hard to
> see at a glance what groups where.  Also, a suspicious reader might
> think there is something special going on (with macros for example).
>
> This is simple code of course, but :-)
>
>> 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.
>
> It is very simple.  The BFA instruction field is closely related to the
> BI instruction field, which is 5 bits, and selects one of the 32 bits in
> the CR.  If you have "BFA00 BFA01 BFA10 BFA11", that gives the bit
> numbers of all four bits in the selected CR field.  So the "& ~3" does
> all you need.  It is quite pretty :-)
>
>
> Segher