[Qemu-devel] Incorrect handling of PPC64 rldcl insn

[Qemu-devel] Incorrect handling of PPC64 rldcl insn

[Torbjorn Granlund]

I could finally make Debian GNU/Linux install and run under
qemu-system-ppc64.  I used Debian 7.0.0 and qemu from the main git repo,
updated a few days ago.

While Debian runs well and not too slowly, GMP fails badly under all
ABIs, and in many different ways.  I have isolated the first problem.

Test case:

#include <stdio.h>
int
main ()
{
  unsigned long r;
  asm ("rldcl\t%0, %1, %2, 0" : "=r" (r) : "r" (0xcafebabedeadbeeful), "r" (16));
  printf ("%lx\n", r);
  return 0;
}

Expected output:
babedeadbeefcafe

Output under qemu:
0

I have single stepped in gdb to determine that it is indeed rldcl that
misbehaves.

-- 
Torbjörn

Re: [Qemu-devel] Incorrect handling of PPC64 rldcl insn

[Alexander Graf]

On 05/06/2013 07:00 PM, Torbjorn Granlund wrote:
> I could finally make Debian GNU/Linux install and run under
> qemu-system-ppc64.  I used Debian 7.0.0 and qemu from the main git repo,
> updated a few days ago.
>
> While Debian runs well and not too slowly, GMP fails badly under all
> ABIs, and in many different ways.  I have isolated the first problem.
>
> Test case:
>
> #include<stdio.h>
> int
> main ()
> {
>    unsigned long r;
>    asm ("rldcl\t%0, %1, %2, 0" : "=r" (r) : "r" (0xcafebabedeadbeeful), "r" (16));
>    printf ("%lx\n", r);
>    return 0;
> }
>
> Expected output:
> babedeadbeefcafe
>
> Output under qemu:
> 0
>
> I have single stepped in gdb to determine that it is indeed rldcl that
> misbehaves.

Thanks a lot for the bug report and test case! Please CC qemu-ppc 
whenever you find issues or have patches for PPC. That makes filtering 
for important mails a lot easier.

Does the patch below fix the issue for you?


Alex

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 0886f4d..a018616 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -1733,8 +1733,6 @@ static inline void gen_rldnm(DisasContext *ctx, 
uint32_t mb, uint32_t me)
  {
      TCGv t0;

-    mb = MB(ctx->opcode);
-    me = ME(ctx->opcode);
      t0 = tcg_temp_new();
      tcg_gen_andi_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x3f);
      tcg_gen_rotl_tl(t0, cpu_gpr[rS(ctx->opcode)], t0);

Re: [Qemu-devel] Incorrect handling of PPC64 rldcl insn

[Torbjorn Granlund]

Alexander Graf <agraf@suse.de> writes:

  Thanks a lot for the bug report and test case! Please CC qemu-ppc
  whenever you find issues or have patches for PPC. That makes filtering
  for important mails a lot easier.
  
Would that make my complaints be considered more or less important?  :-)

  Does the patch below fix the issue for you?
  
It indeed does.  (I actually tried that already, but I cannot follow the
data flow into these functions, so cannot tell if that patch is
sufficient.  This bug indicates complete non-testing status of these
insns, which are mainstream enough to be generated by gcc.  I suppose
there will likely be more such fundamental errors if more instructions
are also completely untested.)

-- 
Torbjörn

Re: [Qemu-devel] Incorrect handling of PPC64 rldcl insn

[Alexander Graf]

On 06.05.2013, at 20:13, Torbjorn Granlund wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>  Thanks a lot for the bug report and test case! Please CC qemu-ppc
>  whenever you find issues or have patches for PPC. That makes filtering
>  for important mails a lot easier.
> 
> Would that make my complaints be considered more or less important?  :-)
> 
>  Does the patch below fix the issue for you?
> 
> It indeed does.  (I actually tried that already, but I cannot follow the
> data flow into these functions, so cannot tell if that patch is
> sufficient.  

Yes, it is. It's a leftover bug from converting the code to TCG I assume.

> This bug indicates complete non-testing status of these
> insns, which are mainstream enough to be generated by gcc.  I suppose
> there will likely be more such fundamental errors if more instructions
> are also completely untested.)

There's a certain chance that happens, yes. We don't have instruction test suites for the PPC target.


Alex

Re: [Qemu-devel] Incorrect handling of PPC64 rldcl insn

[Aurelien Jarno]

On Tue, May 07, 2013 at 12:14:47AM +0200, Alexander Graf wrote:
> 
> On 06.05.2013, at 20:13, Torbjorn Granlund wrote:
> 
> > Alexander Graf <agraf@suse.de> writes:
> > 
> >  Thanks a lot for the bug report and test case! Please CC qemu-ppc
> >  whenever you find issues or have patches for PPC. That makes filtering
> >  for important mails a lot easier.
> > 
> > Would that make my complaints be considered more or less important?  :-)
> > 
> >  Does the patch below fix the issue for you?
> > 
> > It indeed does.  (I actually tried that already, but I cannot follow the
> > data flow into these functions, so cannot tell if that patch is
> > sufficient.  
> 
> Yes, it is. It's a leftover bug from converting the code to TCG I assume.

Yes, looks like I am the culprit here.

> > This bug indicates complete non-testing status of these
> > insns, which are mainstream enough to be generated by gcc.  I suppose
> > there will likely be more such fundamental errors if more instructions
> > are also completely untested.)
> 
> There's a certain chance that happens, yes. We don't have instruction test suites for the PPC target.
> 

We have the Gwenole Beauschene testsuite for the PPC32 target, even if
it doesn't work when compiled on a recent distribution, one has to use
the old binary. It currently passes, so the PPC32 and Altivec
instructions should be fine.

On the contrary, the PPC64 instructions are untested, and there are
likely a few bugs like this one left, especially on complex
instructions.

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

[Qemu-devel] Incorrect handling of more PPC64 insns

[Torbjorn Granlund]

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

Alexander Graf <agraf@suse.de> writes:

  There's a certain chance that happens, yes. We don't have instruction
  test suites for the PPC target.
  
There certainly are more bugs.  GMP still crashes all over the place.
I have semi-isolated one more.

Extracted stand-alone sources:


[-- Attachment #2: bug-qemu-ppc-again.c --]
[-- Type: application/octet-stream, Size: 1141 bytes --]

#include <stdio.h>
#include <stdlib.h>

typedef unsigned long long mp_limb_t;

#define GMP_LIMB_BITS 64
#define GMP_NAIL_BITS 0

#define GMP_NUMB_MAX (~(mp_limb_t) 0)

#define CNST_LIMB(C) ((mp_limb_t) C##LL)

#define GMP_NUMB_CEIL_MAX_DIV3   (GMP_NUMB_MAX / 3 + 1)

#define SHIFTHIGH(x) ((x) << GMP_LIMB_BITS/2)
#define SHIFTLOW(x)  ((x) >> GMP_LIMB_BITS/2)

#define LOWMASK   (((mp_limb_t) 1 << GMP_LIMB_BITS/2)-1)
#define HIGHMASK  SHIFTHIGH(LOWMASK)

#define LOWPART(x)   ((x) & LOWMASK)
#define HIGHPART(x)  SHIFTLOW((x) & HIGHMASK)

mp_limb_t
foo (mp_limb_t *lo, mp_limb_t x, mp_limb_t y)
{
  mp_limb_t  hi, s;

  *lo = LOWPART(x) * LOWPART(y);
  hi = HIGHPART(x) * HIGHPART(y);

  s = LOWPART(x) * HIGHPART(y);
  hi += HIGHPART(s);
  s = SHIFTHIGH(LOWPART(s));
  *lo += s;
  hi += (*lo < s);

  s = HIGHPART(x) * LOWPART(y);
  hi += HIGHPART(s);
  s = SHIFTHIGH(LOWPART(s));
  *lo += s;
  hi += (*lo < s);

  return hi;
}

int
main (int argc, char *argv[])
{
  mp_limb_t  hi, lo;

  hi = foo (&lo, GMP_NUMB_CEIL_MAX_DIV3, CNST_LIMB(3) << GMP_NAIL_BITS);
  if (hi < 1)
    printf ("GMP_NUMB_CEIL_MAX_DIV3 too small\n");

  return 0;
}

[-- Attachment #3: Type: text/plain, Size: 53 bytes --]


Asm code generated on gcc110 from the source file:


[-- Attachment #4: bug-qemu-ppc-again.s --]
[-- Type: application/octet-stream, Size: 1039 bytes --]

	.file	"bug-qemu-ppc-again.c"
	.section	".text"
	.align 2
	.globl foo
	.type	foo, @function
foo:
	rldimi 6,5,32,0
	rldimi 8,7,32,0
	li 9,-1
	rldicl 9,9,0,32
	and 10,6,9
	and 9,8,9
	srdi 6,6,32
	srdi 8,8,32
	mulld 5,8,6
	mulld 8,8,10
	sldi 4,8,32
	mulld 10,9,10
	add 10,4,10
	mulld 9,9,6
	srdi 8,8,32
	srdi 7,9,32
	add 7,8,7
	add 7,7,5
	cmpld 7,4,10
	mfcr 4
	rlwinm 4,4,30,1
	add 8,7,4
	sldi 4,9,32
	add 10,10,4
	std 10,0(3)
	cmpld 7,4,10
	mfcr 4
	rlwinm 4,4,30,1
	add 4,8,4
	srdi 3,4,32
	blr
	.size	foo,.-foo
	.align 2
	.globl main
	.type	main, @function
main:
	stwu 1,-32(1)
	mflr 0
	stw 0,36(1)
	addi 3,1,8
	lis 5,0x5555
	ori 5,5,21845
	lis 6,0x5555
	ori 6,6,21846
	li 7,0
	li 8,3
	bl foo
	rldimi 4,3,32,0
	cmpdi 7,4,0
	bne 7,.L3
	lis 3,.LC0@ha
	la 3,.LC0@l(3)
	bl puts
.L3:
	li 3,0
	lwz 0,36(1)
	mtlr 0
	addi 1,1,32
	blr
	.size	main,.-main
	.section	.rodata.str1.8,"aMS",@progbits,1
	.align 3
.LC0:
	.string	"GMP_NUMB_CEIL_MAX_DIV3 too small"
	.ident	"GCC: (GNU) 4.7.2 20121109 (Red Hat 4.7.2-8)"
	.section	.note.GNU-stack,"",@progbits

[-- Attachment #5: Type: text/plain, Size: 541 bytes --]


Generate executable and execute:

gcc -m32 -mpowerpc64 bug-qemu-ppc-again.s && ./a.out

This runs silently as it should on real hardware.  Under qemu (from
2013-05-02 plus the rldcl patch) I incorrectly get the error message:

GMP_NUMB_CEIL_MAX_DIV3 too small

This seems reproducible every time, unlike most qemu bugs that hit GMP.
I haven't isolated this bug to a single instruction, but if rldcl was
untested, expecting all of there here used rldicl rldimi rlwinm to be
tested is perhaps over-optimistic?

-- 
Torbjörn

Re: [Qemu-devel] Incorrect handling of more PPC64 insns

[Peter Maydell]

On 7 May 2013 11:27, Torbjorn Granlund <tg@gmplib.org> wrote:
> Alexander Graf <agraf@suse.de> writes:
>
>   There's a certain chance that happens, yes. We don't have instruction
>   test suites for the PPC target.
>
> There certainly are more bugs.  GMP still crashes all over the place.

If you want to more seriously test the PPC instructions
it might be worth extending risu to cope with more than
just the ARM architecture...
 https://wiki.linaro.org/PeterMaydell/Risu
That would be a moderate chunk of work -- about 300 lines
of C code in the client, plus refactoring the generator
perl script to separate out the architecture-dependent
bits and add ppc support. The advantage is that once
you've done it it's very easy to add support for testing
a single instruction, provided you have a known-good
hardware implementation to be the reference.

(one day soon I will have to make it cope with aarch64)

-- PMM

Re: [Qemu-devel] Incorrect handling of more PPC64 insns

[Torbjorn Granlund]

Peter Maydell <peter.maydell@linaro.org> writes:

  On 7 May 2013 11:27, Torbjorn Granlund <tg@gmplib.org> wrote:
  > Alexander Graf <agraf@suse.de> writes:
  >
  >   There's a certain chance that happens, yes. We don't have instruction
  >   test suites for the PPC target.
  >
  > There certainly are more bugs.  GMP still crashes all over the place.
  
  If you want to more seriously test the PPC instructions
  it might be worth extending risu to cope with more than
  just the ARM architecture...

I am an involuntary tester of lots of software, and a voluntary tester
of my own software.

I think we should do testing of the software we write ourselves, since we
else hurt the productivity of the community.

I waste half my hacking time on other hackers' plain bugs.

But no, I will not become a voluntary tester of the qemu hackers'
untested code...

-- 
Torbjörn

Re: [Qemu-devel] Incorrect handling of more PPC64 insns

[Peter Maydell]

On 7 May 2013 12:48, Torbjorn Granlund <tg@gmplib.org> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>   On 7 May 2013 11:27, Torbjorn Granlund <tg@gmplib.org> wrote:
>   > Alexander Graf <agraf@suse.de> writes:
>   >   There's a certain chance that happens, yes. We don't have instruction
>   >   test suites for the PPC target.
>   >
>   > There certainly are more bugs.  GMP still crashes all over the place.
>
>   If you want to more seriously test the PPC instructions
>   it might be worth extending risu to cope with more than
>   just the ARM architecture...
>
> I am an involuntary tester of lots of software, and a voluntary tester
> of my own software.
>
> I think we should do testing of the software we write ourselves, since we
> else hurt the productivity of the community.
>
> I waste half my hacking time on other hackers' plain bugs.
>
> But no, I will not become a voluntary tester of the qemu hackers'
> untested code...

Sorry, I should have phrased myself more clearly. It was more
meant as a suggestion for anybody reading qemu-ppc/qemu-devel
who was interested.

-- PMM

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

[Torbjorn Granlund]

OK, so took to reading some of translate to see how well it agrees with
the PPC architecture definition.

I spotted a bug with cmp, which was repeated 4 times.  Somebody decided
that NARROW_MODE should affect the handling of cmp instructions, which
is contrary to the ISA documentation.

The first hunk is just a comment about suspicious code.  I don't suggest
to apply that.

Incidentally, this patch makes GMP testing go a bit further, and the
testcase bug-qemu-ppc-again.s works correctly.

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 1a84653..c44b96d 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -665,6 +665,7 @@ static inline void gen_op_cmpi32(TCGv arg0, target_ulong arg1, int s, int crf)
 
 static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg)
 {
+// suspicious code -- tege
     if (NARROW_MODE(ctx)) {
         gen_op_cmpi32(reg, 0, 1, 0);
     } else {
@@ -675,7 +676,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 +688,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 +700,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 +712,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 {

-- 
Torbjörn

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

[Alexander Graf]

On 05/07/2013 05:58 PM, Torbjorn Granlund wrote:
> OK, so took to reading some of translate to see how well it agrees with
> the PPC architecture definition.
>
> I spotted a bug with cmp, which was repeated 4 times.  Somebody decided
> that NARROW_MODE should affect the handling of cmp instructions, which
> is contrary to the ISA documentation.
>
> The first hunk is just a comment about suspicious code.  I don't suggest
> to apply that.
>
> Incidentally, this patch makes GMP testing go a bit further, and the
> testcase bug-qemu-ppc-again.s works correctly.

Richard, could you please proof read this?


Thanks!

Alex

>
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 1a84653..c44b96d 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -665,6 +665,7 @@ static inline void gen_op_cmpi32(TCGv arg0, target_ulong arg1, int s, int crf)
>
>   static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg)
>   {
> +// suspicious code -- tege
>       if (NARROW_MODE(ctx)) {
>           gen_op_cmpi32(reg, 0, 1, 0);
>       } else {
> @@ -675,7 +676,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 +688,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 +700,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 +712,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 {
>

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

[Torbjorn Granlund]

Alexander Graf <agraf@suse.de> writes:

  > The first hunk is just a comment about suspicious code.  I don't suggest
  > to apply that.

The "suspicious" code is correct.  The Rc update should indeed be
SF-mode dependent.

With the other 4 hunks, qemu-ppc64 is now able to execute GMP's
testsuite to completion, using a shared lib build.  (The static build
has yet to complete.)

-- 
Torbjörn

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

[Torbjorn Granlund]

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
 }
 
 /* cmpi */
 static void gen_cmpi(DisasContext *ctx)
 {
-    if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+#if defined(TARGET_PPC64)
+    if (!(ctx->opcode & 0x00200000)) {
+#endif
         gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
                       1, crfD(ctx->opcode));
+#if defined(TARGET_PPC64)
     } else {
         gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
                     1, crfD(ctx->opcode));
     }
+#endif
 }
 
 /* cmpl */
 static void gen_cmpl(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)],
                      0, crfD(ctx->opcode));
+#if defined(TARGET_PPC64)
     } else {
         gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
                    0, crfD(ctx->opcode));
     }
+#endif
 }
 
 /* cmpli */
 static void gen_cmpli(DisasContext *ctx)
 {
-    if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+#if defined(TARGET_PPC64)
+    if (!(ctx->opcode & 0x00200000)) {
+#endif
         gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
                       0, crfD(ctx->opcode));
+#if defined(TARGET_PPC64)
     } else {
         gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
                     0, crfD(ctx->opcode));
     }
+#endif
 }
 
 /* isel (PowerPC 2.03 specification) */

-- 
Torbjörn

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

[Alexander Graf]

Am 07.05.2013 um 21:30 schrieb Torbjorn Granlund <tg@gmplib.org>:

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

The ppc64 target can also execute as ppc32 CPU if you pass in the correct -cpu value. So this one looks slightly bogus...

Alex

> +#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
> }
> 
> /* cmpi */
> static void gen_cmpi(DisasContext *ctx)
> {
> -    if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
> +#if defined(TARGET_PPC64)
> +    if (!(ctx->opcode & 0x00200000)) {
> +#endif
>         gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
>                       1, crfD(ctx->opcode));
> +#if defined(TARGET_PPC64)
>     } else {
>         gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
>                     1, crfD(ctx->opcode));
>     }
> +#endif
> }
> 
> /* cmpl */
> static void gen_cmpl(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)],
>                      0, crfD(ctx->opcode));
> +#if defined(TARGET_PPC64)
>     } else {
>         gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
>                    0, crfD(ctx->opcode));
>     }
> +#endif
> }
> 
> /* cmpli */
> static void gen_cmpli(DisasContext *ctx)
> {
> -    if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
> +#if defined(TARGET_PPC64)
> +    if (!(ctx->opcode & 0x00200000)) {
> +#endif
>         gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
>                       0, crfD(ctx->opcode));
> +#if defined(TARGET_PPC64)
>     } else {
>         gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
>                     0, crfD(ctx->opcode));
>     }
> +#endif
> }
> 
> /* isel (PowerPC 2.03 specification) */
> 
> -- 
> Torbjörn

Re: [Qemu-devel] [Qemu-ppc] 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 <aurelien@aurel32.net>

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 <tg@gmplib.org>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 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
aurelien@aurel32.net                 http://www.aurel32.net

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

[Alexander Graf]

On 08.05.2013, at 08:50, Aurelien Jarno wrote:

> 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 <aurelien@aurel32.net>
> 
> 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 <tg@gmplib.org>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
> 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);

Can't we handle this through the reserved bits in the instruction map?


Alex

> +        } 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
> aurelien@aurel32.net                 http://www.aurel32.net

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

[Torbjorn Granlund]

Aurelien Jarno <aurelien@aurel32.net> writes:

  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.
  
No.  See my previous post.

The L bit is to be ignored for 32-bit CPUs, just like the original code
did.

-- 
Torbjörn

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

[Alexander Graf]

On 08.05.2013, at 11:20, Torbjorn Granlund wrote:

> Aurelien Jarno <aurelien@aurel32.net> writes:
> 
>  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.
> 
> No.  See my previous post.
> 
> The L bit is to be ignored for 32-bit CPUs, just like the original code
> did.

I see. So if the target is 64bit capable, then we distinguish by the instruction bit, but for 32bit targets we always call the 32bit variant regardless of the bit?


Alex

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

[Alexander Graf]

On 08.05.2013, at 11:32, Alexander Graf wrote:

> 
> On 08.05.2013, at 11:20, Torbjorn Granlund wrote:
> 
>> Aurelien Jarno <aurelien@aurel32.net> writes:
>> 
>> 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.
>> 
>> No.  See my previous post.
>> 
>> The L bit is to be ignored for 32-bit CPUs, just like the original code
>> did.
> 
> I see. So if the target is 64bit capable, then we distinguish by the instruction bit, but for 32bit targets we always call the 32bit variant regardless of the bit?

Ok, so the real problem here is that NARROW_MODE is not set, but is used to differentiate whether to use the 32bit cmp only or not.

Richard, there are 2 ways out of this:

  1) get rid of NARROW_MODE and always check ctx->sf
  2) add a new 32bit only insns flag and create separate functions for 32bit cmp calls

I have a patch set ready for 2, but I think 1 would be the better alternative. The only cases I could spot where things could break is in the add/sub corner case. Let me try to cook up something.


Alex

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

[Torbjorn Granlund]

Alexander Graf <agraf@suse.de> writes:

  Ok, so the real problem here is that NARROW_MODE is not set, but is
  used to differentiate whether to use the 32bit cmp only or not.
  
Eh?

  Richard, there are 2 ways out of this:
  
    1) get rid of NARROW_MODE and always check ctx->sf

No!

The cmp insn with L set should NOT be affected by SF.  That's the entire
point of my change.

I reviewed the other uses of NARROW_MODE and didn't spot any errors.
(But I must confess that I would need to red the PPC manuals better inn
order to tell for sure.)

    2) add a new 32bit only insns flag and create separate functions for 32bit cmp calls
  
Aurelien's patch looked promising, if one removes the exception casting.

-- 
Torbjörn

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

[Alexander Graf]

On 08.05.2013, at 12:07, Torbjorn Granlund wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>  Ok, so the real problem here is that NARROW_MODE is not set, but is
>  used to differentiate whether to use the 32bit cmp only or not.
> 
> Eh?
> 
>  Richard, there are 2 ways out of this:
> 
>    1) get rid of NARROW_MODE and always check ctx->sf
> 
> No!
> 
> The cmp insn with L set should NOT be affected by SF.  That's the entire
> point of my change.

You're right. I got confused there :).

> 
> I reviewed the other uses of NARROW_MODE and didn't spot any errors.
> (But I must confess that I would need to red the PPC manuals better inn
> order to tell for sure.)
> 
>    2) add a new 32bit only insns flag and create separate functions for 32bit cmp calls
> 
> Aurelien's patch looked promising, if one removes the exception casting.

His exception casting is actually correct. You can use qemu-(system-)ppc64, but run it with a CPU definition that is 32bit only, like a G3. These old CPUs did not know the instruction with L yet, so they do throw an illegal instruction exception, which we have to model.


Alex