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
This adds emulation support for the following instruction:
* Set Boolean (setb)
Signed-off-by: Sathvika Vasireddy <redacted>
---
arch/powerpc/lib/sstep.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
From: Daniel Axtens <hidden> Date: 2021-04-16 07:45:31
Sathvika Vasireddy [off-list ref] writes:
quoted hunk
This adds emulation support for the following instruction:
* Set Boolean (setb)
Signed-off-by: Sathvika Vasireddy <redacted>
---
arch/powerpc/lib/sstep.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
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.
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 <redacted>
Kind regards,
Daniel
From: Naveen N. Rao <hidden> Date: 2021-04-20 06:26:43
Daniel Axtens wrote:
Sathvika Vasireddy [off-list ref] writes:
quoted
This adds emulation support for the following instruction:
* Set Boolean (setb)
Signed-off-by: Sathvika Vasireddy <redacted>
---
arch/powerpc/lib/sstep.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
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.
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))
quoted
+ 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
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-04-21 07:31:10
"Naveen N. Rao" [off-list ref] writes:
Daniel Axtens wrote:
quoted
Sathvika Vasireddy [off-list ref] writes:
quoted
This adds emulation support for the following instruction:
* Set Boolean (setb)
Signed-off-by: Sathvika Vasireddy <redacted>
...
quoted
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
From: Naveen N. Rao <hidden> Date: 2021-04-22 10:02:02
Michael Ellerman wrote:
"Naveen N. Rao" [off-list ref] writes:
quoted
Daniel Axtens wrote:
quoted
Sathvika Vasireddy [off-list ref] writes:
quoted
This adds emulation support for the following instruction:
* Set Boolean (setb)
Signed-off-by: Sathvika Vasireddy <redacted>
...
quoted
quoted
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
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-04-23 13:29:56
"Naveen N. Rao" [off-list ref] writes:
Michael Ellerman wrote:
quoted
"Naveen N. Rao" [off-list ref] writes:
quoted
Daniel Axtens wrote:
quoted
Sathvika Vasireddy [off-list ref] writes:
quoted
This adds emulation support for the following instruction:
* Set Boolean (setb)
Signed-off-by: Sathvika Vasireddy <redacted>
...
quoted
quoted
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
From: Naveen N. Rao <hidden> Date: 2021-04-27 16:45:26
Michael Ellerman wrote:
"Naveen N. Rao" [off-list ref] writes:
quoted
Michael Ellerman wrote:
quoted
"Naveen N. Rao" [off-list ref] writes:
quoted
Daniel Axtens wrote:
quoted
Sathvika Vasireddy [off-list ref] writes:
quoted
This adds emulation support for the following instruction:
* Set Boolean (setb)
Signed-off-by: Sathvika Vasireddy <redacted>
...
quoted
quoted
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.
quoted
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
quoted
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
Hi!
On Fri, Apr 16, 2021 at 05:44:52PM +1000, Daniel Axtens wrote:
Sathvika Vasireddy [off-list ref] writes:
Ok, if I've understood correctly...
quoted
+ 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.
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
From: Gabriel Paubert <hidden> Date: 2021-04-22 22:27:38
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:
quoted
Sathvika Vasireddy [off-list ref] writes:
Ok, if I've understood correctly...
quoted
+ 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.
quoted
Let's then check to see if your calculations read the right fields.
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).
quoted
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
quoted
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 :-)
quoted
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
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.
quoted
quoted
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
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).
quoted
(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.
quoted
quoted
quoted
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.
quoted
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 ;-)
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:
quoted
quoted
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.
quoted
(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).
quoted
quoted
quoted
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?
From: Daniel Axtens <hidden> Date: 2021-04-24 16:14:34
Segher Boessenkool [off-list ref] writes:
Hi!
On Fri, Apr 16, 2021 at 05:44:52PM +1000, Daniel Axtens wrote:
quoted
Sathvika Vasireddy [off-list ref] writes:
Ok, if I've understood correctly...
quoted
+ 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.
quoted
Let's then check to see if your calculations read the right fields.
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 :-)
quoted
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
quoted
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 :-)
quoted
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