[Qemu-devel] [PATCH] PPC: Depend behavior of cmp instructions only on instruction encoding

[Qemu-devel] [PATCH] PPC: Depend behavior of cmp instructions only on instruction encoding

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH] PPC: Depend behavior of cmp instructions only on instruction encoding

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH] PPC: Depend behavior of cmp instructions only on instruction encoding

[Torbjorn Granlund]

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.

-- 
Torbjörn

Re: [Qemu-devel] [PATCH] PPC: Depend behavior of cmp instructions only on instruction encoding

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH] PPC: Depend behavior of cmp instructions only on instruction encoding

[Aurelien Jarno]

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                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] PPC: Depend behavior of cmp instructions only on instruction encoding

[Torbjorn Granlund]

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

-- 
Torbjörn

Re: [Qemu-devel] [PATCH] PPC: Depend behavior of cmp instructions only on instruction encoding

[Aurelien Jarno]

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	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] PPC: Depend behavior of cmp instructions only on instruction encoding

[Torbjorn Granlund]

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

-- 
Torbjörn

Re: [Qemu-devel] [PATCH] PPC: Depend behavior of cmp instructions only on instruction encoding

[Aurelien Jarno]

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                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] PPC: Depend behavior of cmp instructions only on instruction encoding

[Torbjorn Granlund]

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

-- 
Torbjörn