| Message ID | 1368019560-25218-1-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
On 2013-05-08 08:26, Alexander Graf wrote: > However, the situation is more complex than that. On 32bit CPUs, L=1 > instructions are either treated identical to L=0 instructions (G4) or > treated as illegal instructions (e500mc). Differenciating these cases > is out of scope for the 1.5 release and will follow afterwards. For now > just treat the 32bit CPU, 64bit cmp case as undefined. Ah ha. Interesting g4/e500 difference. Reviewed-by: Richard Henderson <rth@twiddle.net> r~
Alexander Graf <agraf@suse.de> writes:
Reported-by: Torbjorn Granlund <tg@gmplib.org>
Signed-off-by: Alexander Graf <agraf@suse.de>
Ah, so my original patch was correct after all.
Only the name of the author needed changing, apparently. :-)
---
target-ppc/translate.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index a018616..89a4445 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -675,7 +675,7 @@ static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg)
/* cmp */
static void gen_cmp(DisasContext *ctx)
{
- if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+ if (!(ctx->opcode & 0x00200000)) {
gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
1, crfD(ctx->opcode));
} else {
@@ -687,7 +687,7 @@ static void gen_cmp(DisasContext *ctx)
/* cmpi */
static void gen_cmpi(DisasContext *ctx)
{
- if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+ if (!(ctx->opcode & 0x00200000)) {
gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
1, crfD(ctx->opcode));
} else {
@@ -699,7 +699,7 @@ static void gen_cmpi(DisasContext *ctx)
/* cmpl */
static void gen_cmpl(DisasContext *ctx)
{
- if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+ if (!(ctx->opcode & 0x00200000)) {
gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
0, crfD(ctx->opcode));
} else {
@@ -711,7 +711,7 @@ static void gen_cmpl(DisasContext *ctx)
/* cmpli */
static void gen_cmpli(DisasContext *ctx)
{
- if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+ if (!(ctx->opcode & 0x00200000)) {
gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
0, crfD(ctx->opcode));
} else {
Hopefully my cmp speedup patch will be reposted under a new name and
then included.
On 08.05.2013, at 15:49, Torbjorn Granlund wrote: > Alexander Graf <agraf@suse.de> writes: > > Reported-by: Torbjorn Granlund <tg@gmplib.org> > Signed-off-by: Alexander Graf <agraf@suse.de> > > Ah, so my original patch was correct after all. > Only the name of the author needed changing, apparently. :-) No, I need a patch header and a Signed-off-by as well as understanding of the full problem space. If you can repost your patch with a proper header and SOB line I'll happily pull that into the queue. Alex
On Wed, May 08, 2013 at 03:26:00PM +0200, Alexander Graf wrote: > When running an L=1 cmp instruction on a 64bit PPC CPU with SF off, it > still behaves identical to what it does when SF is on. Remove the implicit > difference in the code. > > However, the situation is more complex than that. On 32bit CPUs, L=1 > instructions are either treated identical to L=0 instructions (G4) or > treated as illegal instructions (e500mc). Differenciating these cases > is out of scope for the 1.5 release and will follow afterwards. For now > just treat the 32bit CPU, 64bit cmp case as undefined. > > Reported-by: Torbjorn Granlund <tg@gmplib.org> > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > target-ppc/translate.c | 8 ++++---- > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target-ppc/translate.c b/target-ppc/translate.c > index a018616..89a4445 100644 > --- a/target-ppc/translate.c > +++ b/target-ppc/translate.c > @@ -675,7 +675,7 @@ static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg) > /* cmp */ > static void gen_cmp(DisasContext *ctx) > { > - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { > + if (!(ctx->opcode & 0x00200000)) { > gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], > 1, crfD(ctx->opcode)); > } else { > @@ -687,7 +687,7 @@ static void gen_cmp(DisasContext *ctx) > /* cmpi */ > static void gen_cmpi(DisasContext *ctx) > { > - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { > + if (!(ctx->opcode & 0x00200000)) { > gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode), > 1, crfD(ctx->opcode)); > } else { > @@ -699,7 +699,7 @@ static void gen_cmpi(DisasContext *ctx) > /* cmpl */ > static void gen_cmpl(DisasContext *ctx) > { > - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { > + if (!(ctx->opcode & 0x00200000)) { > gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], > 0, crfD(ctx->opcode)); > } else { > @@ -711,7 +711,7 @@ static void gen_cmpl(DisasContext *ctx) > /* cmpli */ > static void gen_cmpli(DisasContext *ctx) > { > - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { > + if (!(ctx->opcode & 0x00200000)) { > gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode), > 0, crfD(ctx->opcode)); > } else { Reviewed-by: Aurelien Jarno <aurelien@aurel32.net> That said this does implement neither the specification nor the silicon behaviour. This is fine for 1.5 as we are in freeze period, but this should be fixed for the 1.6 release.
Aurelien Jarno <aurelien@aurel32.net> writes: That said this does implement neither the specification nor the silicon behaviour. This is fine for 1.5 as we are in freeze period, but this should be fixed for the 1.6 release. I talked to IBM now. Reserved fields should be ignored by hardware. The architecture owner is IBM, not Freescale. That Freescale deviates from the architecture, is something that you may decide to ignore, unless it is vital for qemu's behaviour in practice. I very much doubt that L = 1 often, for code targeting a 32-bit processor. Trying to mimic decoding flaws on a per-processor basis, is going to take a lot of research, and will be prone to errors. So as far as I can tell, the patch is correct as per the architecture specification. One caveat though: Does 32-bit implementations define the SF bit, or else, does qemu define it and make sure it is 0 for 32-bit emulation? If not, the patch might cause trouble. Congrats, you read a "user message" until the last line. :-)
On Wed, May 08, 2013 at 04:48:22PM +0200, Torbjorn Granlund wrote: > Aurelien Jarno <aurelien@aurel32.net> writes: > > That said this does implement neither the specification nor the silicon > behaviour. This is fine for 1.5 as we are in freeze period, but this > should be fixed for the 1.6 release. > > I talked to IBM now. Reserved fields should be ignored by hardware. As it seems you have good contact with IBM, could you please ask them to fix their manuals? > The architecture owner is IBM, not Freescale. That Freescale deviates > from the architecture, is something that you may decide to ignore, > unless it is vital for qemu's behaviour in practice. At least Freescale CPUs matches what IBM documentation says. IBM CPUs doesn't. > I very much doubt that L = 1 often, for code targeting a 32-bit > processor. > > Trying to mimic decoding flaws on a per-processor basis, is going to > take a lot of research, and will be prone to errors. > > So as far as I can tell, the patch is correct as per the architecture > specification. No it's not correct, it doesn't match neither Freescale nor IBM behaviour. It also means the same code executed on a 32-bit emulated CPU run with qemu-system-ppc will behave differently than when run with qemu-system-ppc64. This is fine for now as we are in freeze period, but should be fixed afterwards. > One caveat though: Does 32-bit implementations define the SF bit, or > else, does qemu define it and make sure it is 0 for 32-bit emulation? > If not, the patch might cause trouble. QEMU makes sure it is 0 for 32-bit CPU. > Congrats, you read a "user message" until the last line. :-) > Like I did for the previous one. Would be nice if you can do the same.
Aurelien Jarno <aurelien@aurel32.net> writes: As it seems you have good contact with IBM, could you please ask them to fix their manuals? What flaw have your found? At least Freescale CPUs match what IBM documentation says. Which ones? Freescale 7447 and Freescale e500 disagree. (Or at least some versions of these chips, perhaps newer e500 steppings ignore the L bit.) IBM CPUs don't. Which ones? No it's not correct, it doesn't match neither Freescale nor IBM behaviour. It also means the same code executed on a 32-bit emulated CPU run with qemu-system-ppc will behave differently than when run with qemu-system-ppc64. This is fine for now as we are in freeze period, but should be fixed afterwards. I think one should check if it is a 64-bit CPU vs 32-bit CPU, as your patch did. (If I read it correctly; while I am an expert in the area, I am very little familiar with qemu's innards.) Except that it should probably not cast an exception (but I think either way there is no calamity).
On Wed, May 08, 2013 at 05:54:27PM +0200, Torbjorn Granlund wrote: > Aurelien Jarno <aurelien@aurel32.net> writes: > > As it seems you have good contact with IBM, could you please ask them > to fix their manuals? > > What flaw have your found? Don't people read what I write? From one of my previous email: Quoting the "IBM PowerPC Microprocessor Family: The Programming Environments Manual for 32 and 64-bit Microprocessors": | Note: In 32-bit implementations, if L = 1 the instruction form is invalid. This doesn't match what your contact says. > At least Freescale CPUs match what IBM documentation says. > > Which ones? Freescale 7447 and Freescale e500 disagree. (Or at least > some versions of these chips, perhaps newer e500 steppings ignore the L > bit.) The e500 CPU doesn't ignore the L bit, like the IBM manual says. > IBM CPUs don't. > > Which ones? The one from your contact saying that reserved fields should be ignored by hardware. > No it's not correct, it doesn't match neither Freescale nor IBM > behaviour. It also means the same code executed on a 32-bit emulated CPU > run with qemu-system-ppc will behave differently than when run with > qemu-system-ppc64. This is fine for now as we are in freeze period, but > should be fixed afterwards. > > I think one should check if it is a 64-bit CPU vs 32-bit CPU, as your > patch did. (If I read it correctly; while I am an expert in the area, I > am very little familiar with qemu's innards.) Except that it should > probably not cast an exception (but I think either way there is no > calamity). > Looking more into details about the issue. Old *PowerPC* manuals (the one from the 7447 era) clearly states that the L bit must trigger an invalid instruction exception. *POWER* manuals states that reserved fields in instructions are ignored by on Server environment, but not on Embedded environment, though it is now phased-in on the latter. In short everybody is correct, it only depends on the CPU.
Aurelien Jarno <aurelien@aurel32.net> writes: Don't people read what I write? From one of my previous email: I do...and even scrutinise it for grammar errors. ;-) Quoting the "IBM PowerPC Microprocessor Family: The Programming Environments Manual for 32 and 64-bit Microprocessors": | Note: In 32-bit implementations, if L = 1 the instruction form is invalid. This doesn't match what your contact says. I think you're reading too much into that wording. It is perhaps intended to mean that L = 1 makes no sense, that it will not have the desired effect. (I don't much like the way IBM's powerpc docs are written. They ought to be much more unambiguous, and could be less wordy.)
diff --git a/target-ppc/translate.c b/target-ppc/translate.c index a018616..89a4445 100644 --- a/target-ppc/translate.c +++ b/target-ppc/translate.c @@ -675,7 +675,7 @@ static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg) /* cmp */ static void gen_cmp(DisasContext *ctx) { - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { + if (!(ctx->opcode & 0x00200000)) { gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], 1, crfD(ctx->opcode)); } else { @@ -687,7 +687,7 @@ static void gen_cmp(DisasContext *ctx) /* cmpi */ static void gen_cmpi(DisasContext *ctx) { - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { + if (!(ctx->opcode & 0x00200000)) { gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode), 1, crfD(ctx->opcode)); } else { @@ -699,7 +699,7 @@ static void gen_cmpi(DisasContext *ctx) /* cmpl */ static void gen_cmpl(DisasContext *ctx) { - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { + if (!(ctx->opcode & 0x00200000)) { gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], 0, crfD(ctx->opcode)); } else { @@ -711,7 +711,7 @@ static void gen_cmpl(DisasContext *ctx) /* cmpli */ static void gen_cmpli(DisasContext *ctx) { - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { + if (!(ctx->opcode & 0x00200000)) { gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode), 0, crfD(ctx->opcode)); } else {
When running an L=1 cmp instruction on a 64bit PPC CPU with SF off, it still behaves identical to what it does when SF is on. Remove the implicit difference in the code. However, the situation is more complex than that. On 32bit CPUs, L=1 instructions are either treated identical to L=0 instructions (G4) or treated as illegal instructions (e500mc). Differenciating these cases is out of scope for the 1.5 release and will follow afterwards. For now just treat the 32bit CPU, 64bit cmp case as undefined. Reported-by: Torbjorn Granlund <tg@gmplib.org> Signed-off-by: Alexander Graf <agraf@suse.de> --- target-ppc/translate.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)