[PATCH 0/2] powerpc/sstep: Add emulation support and tests for 'setb' instruction

[PATCH 0/2] powerpc/sstep: Add emulation support and tests for 'setb' instruction

[Sathvika Vasireddy]

This patchset adds emulation support and tests for setb instruction. Test cases are written to test different CR fields with different bits set in each field.

Sathvika Vasireddy (2):
  powerpc/sstep: Add emulation support for ‘setb’ instruction
  powerpc/sstep: Add tests for setb instruction

 arch/powerpc/include/asm/ppc-opcode.h |  1 +
 arch/powerpc/lib/sstep.c              | 12 +++++++
 arch/powerpc/lib/test_emulate_step.c  | 28 +++++++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

-- 
2.16.4

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

[Sathvika Vasireddy]

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;
+			ra = ra & ~0x3;
+			if ((regs->ccr) & (1 << (31 - ra)))
+				op->val = -1;
+			else if ((regs->ccr) & (1 << (30 - ra)))
+				op->val = 1;
+			else
+				op->val = 0;
+			goto compute_done;
+
 		case 154:	/* prtyw */
 			do_prty(regs, op, regs->gpr[rd], 32);
 			goto logical_done_nocc;
-- 
2.16.4

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

[Daniel Axtens]

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.

However, I do think this is still worth adding to the kernel either way,
so:

Reviewed-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel

> +			goto compute_done;
> +
>  		case 154:	/* prtyw */
>  			do_prty(regs, op, regs->gpr[rd], 32);
>  			goto logical_done_nocc;
> -- 
> 2.16.4

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

[Naveen N. Rao]

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

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

[Michael Ellerman]

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

cheers

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

[Naveen N. Rao]

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

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

[Michael Ellerman]

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

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

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

[Naveen N. Rao]

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

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

[Segher Boessenkool]

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

> 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

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

[Gabriel Paubert]

	Hi,

On Thu, Apr 22, 2021 at 02:13:34PM -0500, Segher Boessenkool wrote:
> 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 :-)
> 

But this can be made jump free :-):

	int tmp = regs->ccr << ra;
	op->val = (tmp >> 31) | ((tmp >> 30) & 1);

(IIRC the srawi instruction sign-extends its result to 64 bits).



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

Around 1985, I had a documentation for the the National's 32032
(little-endian) processor family, and all the instruction encodings were
presented with the LSB on the left and MSB on the right.

BTW on these processors, the immediate operands and the offsets (1, 2 or
4 bytes) for the addressing modes were encoded in big-endian byte order,
but I digress. Consistency is overrated ;-)

	Gabriel


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

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

[Segher Boessenkool]

Hi!

On Fri, Apr 23, 2021 at 12:16:18AM +0200, Gabriel Paubert wrote:
> On Thu, Apr 22, 2021 at 02:13:34PM -0500, Segher Boessenkool wrote:
> > On Fri, Apr 16, 2021 at 05:44:52PM +1000, Daniel Axtens wrote:
> > > Sathvika Vasireddy <sathvika@linux.vnet.ibm.com> writes:
> > > > +			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 :-)
> 
> But this can be made jump free :-):
> 
> 	int tmp = regs->ccr << ra;
> 	op->val = (tmp >> 31) | ((tmp >> 30) & 1);

The compiler will do so automatically (or think of some better way to
get the same result); in source code, what matters most is readability,
or clarity in general (also clarity to the compiler).

(Right shifts of negative numbers are implementation-defined in C,
fwiw -- but work like you expect in GCC).

> (IIRC the srawi instruction sign-extends its result to 64 bits).

If you consider it to work on 32-bit inputs, yeah, that is a simple way
to express it.

> > > 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).
> 
> Around 1985, I had a documentation for the the National's 32032
> (little-endian) processor family, and all the instruction encodings were
> presented with the LSB on the left and MSB on the right.

Ouch!  Did they write "regular" numbers with the least significant digit
on the left as well?

> BTW on these processors, the immediate operands and the offsets (1, 2 or
> 4 bytes) for the addressing modes were encoded in big-endian byte order,
> but I digress. Consistency is overrated ;-)

Inconsistency is the spice of life, yeah :-)


Segher

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

[Gabriel Paubert]

[-- Attachment #1: Type: text/plain, Size: 3503 bytes --]

On Thu, Apr 22, 2021 at 06:26:16PM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Apr 23, 2021 at 12:16:18AM +0200, Gabriel Paubert wrote:
> > On Thu, Apr 22, 2021 at 02:13:34PM -0500, Segher Boessenkool wrote:
> > > On Fri, Apr 16, 2021 at 05:44:52PM +1000, Daniel Axtens wrote:
> > > > Sathvika Vasireddy <sathvika@linux.vnet.ibm.com> writes:
> > > > > +			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 :-)
> > 
> > But this can be made jump free :-):
> > 
> > 	int tmp = regs->ccr << ra;
> > 	op->val = (tmp >> 31) | ((tmp >> 30) & 1);
> 
> The compiler will do so automatically (or think of some better way to
> get the same result); in source code, what matters most is readability,
> or clarity in general (also clarity to the compiler).

I just did a test (trivial code attached) and the original code always
produces one conditional branch at -O2, at least with the cross-compiler
I have on Debian (gcc 8.3). I have tested both -m32 and -m64. The 64 bit
version produces an unnecessary "extsw", so I wrote the second version
splitting the setting of the return value which gets rid of it.

The second "if" is fairly simple to optimize and the compiler does it
properly.

Of course with my suggestion the compiler does not produce any branch. 
But it needs a really good comment.


> 
> (Right shifts of negative numbers are implementation-defined in C,
> fwiw -- but work like you expect in GCC).

Well, I'm not worried about it, since I'd expect a compiler that does
logical right shifts on signed valued to break so much code that it
would be easily noticed (also in the kernel).


> 
> > (IIRC the srawi instruction sign-extends its result to 64 bits).
> 
> If you consider it to work on 32-bit inputs, yeah, that is a simple way
> to express it.
> 
> > > > 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).
> > 
> > Around 1985, I had a documentation for the the National's 32032
> > (little-endian) processor family, and all the instruction encodings were
> > presented with the LSB on the left and MSB on the right.
> 
> Ouch!  Did they write "regular" numbers with the least significant digit
> on the left as well?

No, they were not that sadistic!

At least instructions were a whole number of bytes, unlike the iAPX432
where jumps needed to encode target addresses down to the bit level.

> 
> > BTW on these processors, the immediate operands and the offsets (1, 2 or
> > 4 bytes) for the addressing modes were encoded in big-endian byte order,
> > but I digress. Consistency is overrated ;-)
> 
> Inconsistency is the spice of life, yeah :-)

;-)

	Gabriel

 

[-- Attachment #2: setb.c --]
[-- Type: text/x-csrc, Size: 439 bytes --]

long setb_cond(int cr, int shift)
{
	long ret;
	if ((cr << shift) & 0x80000000)
		ret = -1;
	else if ((cr << shift) & 0x40000000)
		ret = 1;
	else
		ret = 0;
	return ret;
}

long setb_uncond(int cr, int shift)
{
	int tmp = cr << shift;
	long ret;
	ret = (tmp >> 31) | ((tmp >> 30) & 1);
	return ret;
}

long setb_uncond2(int cr, int shift)
{
	int tmp = cr << shift;
	long ret;
	ret = (tmp >> 31);
	ret |= ((tmp >> 30) & 1);
	return ret;
}

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

[Segher Boessenkool]

On Fri, Apr 23, 2021 at 12:26:57PM +0200, Gabriel Paubert wrote:
> On Thu, Apr 22, 2021 at 06:26:16PM -0500, Segher Boessenkool wrote:
> > > But this can be made jump free :-):
> > > 
> > > 	int tmp = regs->ccr << ra;
> > > 	op->val = (tmp >> 31) | ((tmp >> 30) & 1);
> > 
> > The compiler will do so automatically (or think of some better way to
> > get the same result); in source code, what matters most is readability,
> > or clarity in general (also clarity to the compiler).
> 
> I just did a test (trivial code attached) and the original code always
> produces one conditional branch at -O2, at least with the cross-compiler
> I have on Debian (gcc 8.3). I have tested both -m32 and -m64. The 64 bit
> version produces an unnecessary "extsw", so I wrote the second version
> splitting the setting of the return value which gets rid of it.

That is an older compiler, and it will be out-of-service any day now.

It depends on what compiler flags you use, and what version of the ISA
you are targetting.

> The second "if" is fairly simple to optimize and the compiler does it
> properly.

Yeah.

> Of course with my suggestion the compiler does not produce any branch. 
> But it needs a really good comment.

Or you could try and help improve the compiler ;-)  You can do this
without writing compiler code yourself, by writing up some good
enhancement request in bugzilla.

The wider and more OoO the processors become, the more important it
becomes to have branch-free code, in situations where the branches would
not be well-predictable.

> > (Right shifts of negative numbers are implementation-defined in C,
> > fwiw -- but work like you expect in GCC).
> 
> Well, I'm not worried about it, since I'd expect a compiler that does
> logical right shifts on signed valued to break so much code that it
> would be easily noticed (also in the kernel).

Yup.  And it *is* defined for signed values, as long as they are
non-negative (the common case).

> > > > 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).
> > > 
> > > Around 1985, I had a documentation for the the National's 32032
> > > (little-endian) processor family, and all the instruction encodings were
> > > presented with the LSB on the left and MSB on the right.
> > 
> > Ouch!  Did they write "regular" numbers with the least significant digit
> > on the left as well?
> 
> No, they were not that sadistic!

But more inconsistent :-)


Segher

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

[Daniel Axtens]

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

[PATCH 2/2] powerpc/sstep: Add tests for setb instruction

[Sathvika Vasireddy]

This adds selftests for setb instruction.

Signed-off-by: Sathvika Vasireddy <sathvika@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/ppc-opcode.h |  1 +
 arch/powerpc/lib/test_emulate_step.c  | 28 +++++++++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index ed161ef2b3ca..32bf53260737 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -245,6 +245,7 @@
 #define PPC_INST_STRING			0x7c00042a
 #define PPC_INST_STRING_MASK		0xfc0007fe
 #define PPC_INST_STRING_GEN_MASK	0xfc00067e
+#define PPC_INST_SETB			0x7c000100
 #define PPC_INST_STSWI			0x7c0005aa
 #define PPC_INST_STSWX			0x7c00052a
 #define PPC_INST_TRECHKPT		0x7c0007dd
diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
index 783d1b85ecfe..c338e35b627c 100644
--- a/arch/powerpc/lib/test_emulate_step.c
+++ b/arch/powerpc/lib/test_emulate_step.c
@@ -53,6 +53,8 @@
 	ppc_inst_prefix(PPC_PREFIX_MLS | __PPC_PRFX_R(pr) | IMM_H(i), \
 			PPC_RAW_ADDI(t, a, i))
 
+#define TEST_SETB(t, bfa)       ppc_inst(PPC_INST_SETB | ___PPC_RT(t) | ___PPC_RA((bfa & 0x7) << 2))
+
 
 static void __init init_pt_regs(struct pt_regs *regs)
 {
@@ -929,6 +931,67 @@ static struct compute_test compute_tests[] = {
 			}
 		}
 	},
+	{
+		.mnemonic = "setb",
+		.subtests = {
+			{
+				.descr = "BFA = 1, CR = GT",
+				.instr = TEST_SETB(20, 1),
+				.regs = {
+					.ccr = 0x4000000,
+				}
+			},
+			{
+				.descr = "BFA = 4, CR = LT",
+				.instr = TEST_SETB(20, 4),
+				.regs = {
+					.ccr = 0x8000,
+				}
+			},
+			{
+				.descr = "BFA = 5, CR = EQ",
+				.instr = TEST_SETB(20, 5),
+				.regs = {
+					.ccr = 0x200,
+				}
+			}
+		}
+	},
 	{
 		.mnemonic = "add",
 		.subtests = {
-- 
2.16.4

Re: [PATCH 2/2] powerpc/sstep: Add tests for setb instruction

[Naveen N. Rao]

Sathvika Vasireddy wrote:
> This adds selftests for setb instruction.
> 
> Signed-off-by: Sathvika Vasireddy <sathvika@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/ppc-opcode.h |  1 +
>  arch/powerpc/lib/test_emulate_step.c  | 28 +++++++++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> index ed161ef2b3ca..32bf53260737 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -245,6 +245,7 @@
>  #define PPC_INST_STRING			0x7c00042a
>  #define PPC_INST_STRING_MASK		0xfc0007fe
>  #define PPC_INST_STRING_GEN_MASK	0xfc00067e
> +#define PPC_INST_SETB			0x7c000100
>  #define PPC_INST_STSWI			0x7c0005aa
>  #define PPC_INST_STSWX			0x7c00052a
>  #define PPC_INST_TRECHKPT		0x7c0007dd
> diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
> index 783d1b85ecfe..c338e35b627c 100644
> --- a/arch/powerpc/lib/test_emulate_step.c
> +++ b/arch/powerpc/lib/test_emulate_step.c
> @@ -53,6 +53,8 @@
>  	ppc_inst_prefix(PPC_PREFIX_MLS | __PPC_PRFX_R(pr) | IMM_H(i), \
>  			PPC_RAW_ADDI(t, a, i))
>  
> +#define TEST_SETB(t, bfa)       ppc_inst(PPC_INST_SETB | ___PPC_RT(t) | ___PPC_RA((bfa & 0x7) << 2))
> +
>  
>  static void __init init_pt_regs(struct pt_regs *regs)
>  {
> @@ -929,6 +931,67 @@ static struct compute_test compute_tests[] = {
>  			}
>  		}
>  	},
> +	{
> +		.mnemonic = "setb",
> +		.subtests = {

Since this is ISA v3.0, you also need to restrict these tests. You can 
do that by setting cpu_feature.

- Naveen