Re: [Qemu-ppc] [Qemu-devel] Incorrect handling of more PPC64 insns (PATCH)

[Aurelien Jarno]

On Tue, May 07, 2013 at 09:30:24PM +0200, Torbjorn Granlund wrote:
> I realised a possible problem with my suggested patch.
> 
> What about a 32-bit processor?  Then NARROW_MODE macro is identical 0.
> 
> The pre-patch behaviour was then to ignore the L bit and decode both
> 32-bit and 64-bit instruction in the same way.
> 
> Apparently that is correct behaviour.  (The manual is slightly vague,
> but I let hardware decide.)
> 
> With my patch, the bit is not ignored, and invalid code will be
> generated for 32-bit targets, if they'd set the L bit.
> 
> Here is an uglier but hopefully completely correct patch.
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 1a84653..69d684c 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -675,49 +675,65 @@ 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 defined(TARGET_PPC64)
> +    if (!(ctx->opcode & 0x00200000)) {
> +#endif
>          gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
>                       1, crfD(ctx->opcode));
> +#if defined(TARGET_PPC64)
>      } else {
>          gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
>                     1, crfD(ctx->opcode));
>      }
> +#endif
>  }

I agree that there is a bug there, and that cmp32 should be used with
when L=0. That said your code is not going to generate and invalid code
on a 32-bit CPU with L=1, but instead just skip the instruction.
Moreover as Alexander pointed, TARGET_PPC64 doesn't mean it's a 64-bit
CPU, but that the resulting qemu binaries support 64-bit CPU.

What about the following patch (only lightly tested).


From: Aurelien Jarno <address@hidden>

target-ppc: fix cmp instructions on 64-bit CPUs

64-bit CPUs check for the L bit of comparison instruction to determine
if the instruction is 32-bit wide, and not to the MSR SF bit.

L=1 on a 32-bit CPU should generate an invalid instruction exception.

Reported-by: Torbjorn Granlund <address@hidden>
Signed-off-by: Aurelien Jarno <address@hidden>
---
 target-ppc/translate.c |   48 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 0886f4d..ab41dc1 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -675,48 +675,64 @@ 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) {
+        if (unlikely(!(ctx->insns_flags & PPC_64B))) {
+            gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
+        } else {
+            gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
+                       1, crfD(ctx->opcode));
+        }
+    } else {
         gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
                      1, crfD(ctx->opcode));
-    } else {
-        gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
-                   1, crfD(ctx->opcode));
     }
 }
 
 /* cmpi */
 static void gen_cmpi(DisasContext *ctx)
 {
-    if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+    if (ctx->opcode & 0x00200000) {
+        if (unlikely(!(ctx->insns_flags & PPC_64B))) {
+            gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
+        } else {
+            gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
+                        1, crfD(ctx->opcode));
+        }
+    } else {
         gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
                       1, crfD(ctx->opcode));
-    } else {
-        gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
-                    1, crfD(ctx->opcode));
     }
 }
 
 /* cmpl */
 static void gen_cmpl(DisasContext *ctx)
 {
-    if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+    if (ctx->opcode & 0x00200000) {
+        if (unlikely(!(ctx->insns_flags & PPC_64B))) {
+            gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
+        } else {
+            gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
+                       0, crfD(ctx->opcode));
+        }
+    } else {
         gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
                      0, crfD(ctx->opcode));
-    } else {
-        gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
-                   0, crfD(ctx->opcode));
     }
 }
 
 /* cmpli */
 static void gen_cmpli(DisasContext *ctx)
 {
-    if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+    if (ctx->opcode & 0x00200000) {
+        if (unlikely(!(ctx->insns_flags & PPC_64B))) {
+            gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
+        } else {
+            gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
+                        0, crfD(ctx->opcode));
+        }
+    } else {
         gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
                       0, crfD(ctx->opcode));
-    } else {
-        gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
-                    0, crfD(ctx->opcode));
     }
 }
 
-- 
1.7.10.4

 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net