[Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[Julian Seward]

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


For ppc32 guests, computation of XER.CA and XER.OV in some obscure
cases is incorrect.  At least, it doesn't produce the same results
as a real MPC7447, and doesn't appear to be in accordance with the
instruction set documentation.

The attached patch fixes it:

* addme{o}{.}, subfme{o}{.}: compute XER.CA correctly

* mullwo{.}: sign extend arguments before doing 64-bit
  multiply, so as to make the XER.OV computation correct

I suspect the handling of the 64-bit equivalents is similarly borked,
but I haven't checked so far.

J

[-- Attachment #2: qemu-ppc32-fix-xer-ca-obscure-cases.diff --]
[-- Type: text/x-diff, Size: 2323 bytes --]

Index: target-ppc/op_helper.c
===================================================================
--- target-ppc/op_helper.c	(revision 4422)
+++ target-ppc/op_helper.c	(working copy)
@@ -147,11 +147,14 @@
 
 void do_addmeo (void)
 {
+    uint32_t argL = T0;
+    uint32_t res  = T0 + xer_ca + (-1);
+    uint32_t carried = res < argL || ((xer_ca & 1) == 1 && res == argL);
     T1 = T0;
-    T0 += xer_ca + (-1);
+    T0 = res;
     xer_ov = ((uint32_t)T1 & ((uint32_t)T1 ^ (uint32_t)T0)) >> 31;
     xer_so |= xer_ov;
-    if (likely(T1 != 0))
+    if (carried)
         xer_ca = 1;
     else
         xer_ca = 0;
@@ -227,9 +230,9 @@
 
 void do_mullwo (void)
 {
-    int64_t res = (int64_t)T0 * (int64_t)T1;
+    int64_t res = ((int64_t)(int32_t)T0) * ((int64_t)(int32_t)T1);
 
-    if (likely((int32_t)res == res)) {
+    if (likely( ((int64_t)(int32_t)res) == ((int64_t)res) )) {
         xer_ov = 0;
     } else {
         xer_ov = 1;
@@ -306,11 +309,14 @@
 
 void do_subfmeo (void)
 {
+    uint32_t argR = -1;
+    uint32_t res  = ~T0 + xer_ca - 1;
+    uint32_t carried = res < argR || ((xer_ca & 1) == 1 && res == argR);
     T1 = T0;
-    T0 = ~T0 + xer_ca - 1;
+    T0 = res;
     xer_ov = ((uint32_t)~T1 & ((uint32_t)~T1 ^ (uint32_t)T0)) >> 31;
     xer_so |= xer_ov;
-    if (likely((uint32_t)T1 != UINT32_MAX))
+    if (carried)
         xer_ca = 1;
     else
         xer_ca = 0;
Index: target-ppc/op.c
===================================================================
--- target-ppc/op.c	(revision 4422)
+++ target-ppc/op.c	(working copy)
@@ -883,8 +883,11 @@
 /* add to minus one extended */
 void OPPROTO op_add_me (void)
 {
-    T0 += xer_ca + (-1);
-    if (likely((uint32_t)T1 != 0))
+    uint32_t argL = T0;
+    uint32_t res  = T0 + xer_ca + (-1);
+    uint32_t carried = res < argL || ((xer_ca & 1) == 1 && res == argL);
+    T0 = res;
+    if (carried)
         xer_ca = 1;
     else
         xer_ca = 0;
@@ -1176,8 +1179,11 @@
 /* subtract from minus one extended */
 void OPPROTO op_subfme (void)
 {
-    T0 = ~T0 + xer_ca - 1;
-    if (likely((uint32_t)T0 != UINT32_MAX))
+    uint32_t argR = -1;
+    uint32_t res  = ~T0 + xer_ca - 1;
+    uint32_t carried = res < argR || ((xer_ca & 1) == 1 && res == argR);
+    T0 = res;
+    if (carried)
         xer_ca = 1;
     else
         xer_ca = 0;

Re: [Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[Aurelien Jarno]

On Sun, May 11, 2008 at 02:04:47AM +0200, Julian Seward wrote:
> 
> For ppc32 guests, computation of XER.CA and XER.OV in some obscure
> cases is incorrect.  At least, it doesn't produce the same results
> as a real MPC7447, and doesn't appear to be in accordance with the
> instruction set documentation.
> 
> The attached patch fixes it:
> 
> * addme{o}{.}, subfme{o}{.}: compute XER.CA correctly
> 
> * mullwo{.}: sign extend arguments before doing 64-bit
>   multiply, so as to make the XER.OV computation correct

Could you please give us a corner case for at least one of the 
instructions? It would help to clearly understand the problem.

Thanks,
Aurelien

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[Julian Seward]

On Tuesday 17 June 2008 14:27, Aurelien Jarno wrote:
> On Sun, May 11, 2008 at 02:04:47AM +0200, Julian Seward wrote:
> > For ppc32 guests, computation of XER.CA and XER.OV in some obscure
> > cases is incorrect.  At least, it doesn't produce the same results
> > as a real MPC7447, and doesn't appear to be in accordance with the
> > instruction set documentation.
> >
> > The attached patch fixes it:
> >
> > * addme{o}{.}, subfme{o}{.}: compute XER.CA correctly
> >
> > * mullwo{.}: sign extend arguments before doing 64-bit
> >   multiply, so as to make the XER.OV computation correct
>
> Could you please give us a corner case for at least one of the
> instructions? It would help to clearly understand the problem.

Below is a test case showing the problem with mullwo, addme and
subfme.  On a real 7447 it prints

mullwo. 000f423f ffffffff  = fff0bdc1 (cr 80000000 xer 00000000)
addme   00000000 XER.CA=1  = 00000000 (cr 00000000 xer 20000000)
subfme  00000000 XER.CA=1  = ffffffff (cr 00000000 xer 20000000)

On QEMU, unpatched, both the result cr and xer are wrong for
mullwo., and the xer values are wrong for addme and subfme.

mullwo. 000f423f ffffffff  = fff0bdc1 (cr 90000000 xer c0000000)
addme   00000000 XER.CA=1  = 00000000 (cr 00000000 xer 00000000)
subfme  00000000 XER.CA=1  = ffffffff (cr 00000000 xer 00000000)

On QEMU, with patch applied, result is same as on 7447.

mullwo. 000f423f ffffffff  = fff0bdc1 (cr 80000000 xer 00000000)
addme   00000000 XER.CA=1  = 00000000 (cr 00000000 xer 20000000)
subfme  00000000 XER.CA=1  = ffffffff (cr 00000000 xer 20000000)

J


/* Compiled with "gcc -Wall -g -O -o ppcbad ppcbad.c" */

#include <stdio.h>
#include <string.h>

typedef unsigned int UInt;

/* At entry, res[0] and res[1] contain 2 args to "mullwo.".  At exit,
   res[0] holds result, res[1] holds resulting %cr, and res[2] holds
   resulting %xer. */
void do_mullwo_dot ( UInt* res )
{
  __asm__ __volatile__(
"\tmr 29,%0\n"
"\tli 25,0\n"
"\tmtxer 25\n"
"\tmtcr  25\n"
"\tlwz 21,0(29)\n"
"\tlwz 22,4(29)\n"
"\tmullwo. 23,21,22\n"
"\tstw 23,0(29)\n"
"\tmfcr 25\n"
"\tstw 25,4(29)\n"
"\tmfxer 25\n"
"\tstw 25,8(29)\n"
: /*OUT*/ : /*IN*/"b"(res) : /*trash*/ "r29","r25","r21","r22","r23",
"memory","cc"
);
}

/* At entry, res[0] contain 1 arg to "addme".  Set XER.CA=1.  At exit,
   res[0] holds result, res[1] holds resulting %cr, and res[2] holds
   resulting %xer. */
void do_addme ( UInt* res )
{
  __asm__ __volatile__(
"\tmr 29,%0\n"
"\tli 25,0\n"
"\tmtcr  25\n"
"\tlis 25,0x2000\n"
"\tmtxer 25\n"     // set XER.CA=1
"\tlwz 21,0(29)\n"
"\taddme 23,21\n"
"\tstw 23,0(29)\n"
"\tmfcr 25\n"
"\tstw 25,4(29)\n"
"\tmfxer 25\n"
"\tstw 25,8(29)\n"
: /*OUT*/ : /*IN*/"b"(res) : /*trash*/ "r29","r25","r21","r22","r23",
"memory","cc"
);
}

/* At entry, res[0] contain 1 arg to "subfme".  Set XER.CA=1.  At exit,
   res[0] holds result, res[1] holds resulting %cr, and res[2] holds
   resulting %xer. */
void do_subfme ( UInt* res )
{
  __asm__ __volatile__(
"\tmr 29,%0\n"
"\tli 25,0\n"
"\tmtcr  25\n"
"\tlis 25,0x2000\n"
"\tmtxer 25\n"     // set XER.CA=1
"\tlwz 21,0(29)\n"
"\tsubfme 23,21\n"
"\tstw 23,0(29)\n"
"\tmfcr 25\n"
"\tstw 25,4(29)\n"
"\tmfxer 25\n"
"\tstw 25,8(29)\n"
: /*OUT*/ : /*IN*/"b"(res) : /*trash*/ "r29","r25","r21","r22","r23",
"memory","cc"
);
}

UInt zzz ( void ) { return 0x20000000; }


int main ( void )
{
  UInt arr[3];

  /* Try mullwo. */
  arr[0] = 0x000f423f;
  arr[1] = 0xffffffff;
  arr[2] = 0;
  do_mullwo_dot(&arr[0]);
  printf("mullwo. 000f423f ffffffff  = %08x (cr %08x xer %08x)\n",
         arr[0],arr[1],arr[2]);

  /* Try addme */
  arr[0] = 0;
  arr[1] = 0;
  arr[2] = 0;
  do_addme(&arr[0]);
  printf("addme   00000000 XER.CA=1  = %08x (cr %08x xer %08x)\n",
         arr[0],arr[1],arr[2]);

  /* Try subfme */
  arr[0] = 0;
  arr[1] = 0;
  arr[2] = 0;
  do_subfme(&arr[0]);
  printf("subfme  00000000 XER.CA=1  = %08x (cr %08x xer %08x)\n",
         arr[0],arr[1],arr[2]);

  return 0;
}

Re: [Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[J. Mayer]

On Wed, 2008-06-18 at 00:06 +0200, Julian Seward wrote:
> On Tuesday 17 June 2008 14:27, Aurelien Jarno wrote:
> > On Sun, May 11, 2008 at 02:04:47AM +0200, Julian Seward wrote:
> > > For ppc32 guests, computation of XER.CA and XER.OV in some obscure
> > > cases is incorrect.  At least, it doesn't produce the same results
> > > as a real MPC7447, and doesn't appear to be in accordance with the
> > > instruction set documentation.
> > >
> > > The attached patch fixes it:
> > >
> > > * addme{o}{.}, subfme{o}{.}: compute XER.CA correctly
> > >
> > > * mullwo{.}: sign extend arguments before doing 64-bit
> > >   multiply, so as to make the XER.OV computation correct

which seems to already been done... (and tested many times)

> > Could you please give us a corner case for at least one of the
> > instructions? It would help to clearly understand the problem.
> 
> Below is a test case showing the problem with mullwo, addme and
> subfme.  On a real 7447 it prints

This patch looks really ugly, does not respect the coding style and
addme/subme changes seem very suspicious to me... I won't merge this
as-is, but I could check more on real hardware I got (from 601 to G5).

Could you please exactly describe the test conditions (host, ...) ?

Thanks

[...]

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[Julian Seward]

On Wednesday 18 June 2008 00:53, J. Mayer wrote:

> This patch looks really ugly, does not respect the coding style and
> addme/subme changes seem very suspicious to me...

Thanks for the encouragement.

> Could you please exactly describe the test conditions (host, ...) ?

Core 2 Duo, openSUSE 10.2, 64-bit mode, gcc-3.4.6, qemu svn trunk from
2 days ago.

J

Re: [Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[J. Mayer]

On Wed, 2008-06-18 at 01:23 +0200, Julian Seward wrote:
> On Wednesday 18 June 2008 00:53, J. Mayer wrote:
> 
> > This patch looks really ugly, does not respect the coding style and
> > addme/subme changes seem very suspicious to me...
> 
> Thanks for the encouragement.

I guess I won't become a diplomat soon...

Looking at the (relativelly) recent changes in this code, I can see that
I did an optimization that seems correct at first sight but...  The goal
was to avoid tests for xer_ov & xer_so computation. The previous version
was OK, I still have tests results which validate the cases you
describe.

@@ -319,15 +149,12 @@ void do_addmeo (void)
 {
     T1 = T0;
     T0 += xer_ca + (-1);
-    if (likely(!((uint32_t)T1 &
-                 ((uint32_t)T1 ^ (uint32_t)T0) & (1UL << 31)))) {
-        xer_ov = 0;
-    } else {
-        xer_ov = 1;
-        xer_so = 1;
-    }
+    xer_ov = ((uint32_t)T1 & ((uint32_t)T1 ^ (uint32_t)T0)) >> 31;
+    xer_so |= xer_ov;
     if (likely(T1 != 0))
         xer_ca = 1;
 @@ -335,43 +162,40 @@ void do_addmeo_64 (void)
 {
     T1 = T0;
     T0 += xer_ca + (-1);
-    if (likely(!((uint64_t)T1 &
-                 ((uint64_t)T1 ^ (uint64_t)T0) & (1ULL << 63)))) {
-        xer_ov = 0;
-    } else {
-        xer_ov = 1;
-        xer_so = 1;
-    }
+    xer_ov = ((uint64_t)T1 & ((uint64_t)T1 ^ (uint64_t)T0)) >> 63;
+    xer_so |= xer_ov;
     if (likely(T1 != 0))
         xer_ca = 1;
@@ -483,15 +308,12 @@ void do_subfmeo (void)
 {
     T1 = T0;
     T0 = ~T0 + xer_ca - 1;
-    if (likely(!((uint32_t)~T1 & ((uint32_t)~T1 ^ (uint32_t)T0) &
-                 (1UL << 31)))) {
-        xer_ov = 0;
-    } else {
-        xer_ov = 1;
-        xer_so = 1;
-    }
+    xer_ov = ((uint32_t)~T1 & ((uint32_t)~T1 ^ (uint32_t)T0)) >> 31;
+    xer_so |= xer_ov;
     if (likely((uint32_t)T1 != UINT32_MAX))
         xer_ca = 1;
@@ -499,15 +321,12 @@ void do_subfmeo_64 (void)
 {
     T1 = T0;
     T0 = ~T0 + xer_ca - 1;
-    if (likely(!((uint64_t)~T1 & ((uint64_t)~T1 ^ (uint64_t)T0) &
-                 (1ULL << 63)))) {
-        xer_ov = 0;
-    } else {
-        xer_ov = 1;
-        xer_so = 1;
-    }
+    xer_ov = ((uint64_t)~T1 & ((uint64_t)~T1 ^ (uint64_t)T0)) >> 63;
+    xer_so |= xer_ov;
     if (likely((uint64_t)T1 != UINT64_MAX))
         xer_ca = 1;
@@ -515,13 +334,9 @@ void do_subfzeo (void)
 {
     T1 = T0;
     T0 = ~T0 + xer_ca;
-    if (likely(!(((uint32_t)~T1 ^ UINT32_MAX) &
-                 ((uint32_t)(~T1) ^ (uint32_t)T0) & (1UL << 31)))) {
-        xer_ov = 0;
-    } else {
-        xer_ov = 1;
-        xer_so = 1;
-    }
+    xer_ov = (((uint32_t)~T1 ^ UINT32_MAX) &
+              ((uint32_t)(~T1) ^ (uint32_t)T0)) >> 31;
+    xer_so |= xer_ov;
     if (likely((uint32_t)T0 >= (uint32_t)~T1)) {
         xer_ca = 0;
     } else {
@@ -534,13 +349,9 @@ void do_subfzeo_64 (void)
 {
     T1 = T0;
     T0 = ~T0 + xer_ca;
-    if (likely(!(((uint64_t)~T1 ^ UINT64_MAX) &
-                 ((uint64_t)(~T1) ^ (uint64_t)T0) & (1ULL << 63)))) {
-        xer_ov = 0;
-    } else {
-        xer_ov = 1;
-        xer_so = 1;
-    }
+    xer_ov = (((uint64_t)~T1 ^  UINT64_MAX) &
+              ((uint64_t)(~T1) ^ (uint64_t)T0)) >> 63;
+    xer_so |= xer_ov;
     if (likely((uint64_t)T0 >= (uint64_t)~T1)) {
         xer_ca = 0;
     } else {

Do you feel like this patch is buggy ?

> 
> > Could you please exactly describe the test conditions (host, ...) ?
> 
> Core 2 Duo, openSUSE 10.2, 64-bit mode, gcc-3.4.6, qemu svn trunk from
> 2 days ago.

64 bits only bug, for mullwo ? Well, I'm running amd64 for years,
but....

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[Julian Seward]

On Wednesday 18 June 2008 23:32, J. Mayer wrote:
> Looking at the (relativelly) recent changes in this code, I can see that
> I did an optimization that seems correct at first sight but... 
> [ .. big patch deleted .. ]
> Do you feel like this patch is buggy ?

Why don't you just commit the patch I sent?  It's shorter, it
fixes the problem I reported and it does not cause any regressions.

J

Re: [Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[J. Mayer]

On Wed, 2008-06-18 at 23:39 +0200, Julian Seward wrote:
> On Wednesday 18 June 2008 23:32, J. Mayer wrote:
> > Looking at the (relativelly) recent changes in this code, I can see that
> > I did an optimization that seems correct at first sight but... 
> > [ .. big patch deleted .. ]
> > Do you feel like this patch is buggy ?
> 
> Why don't you just commit the patch I sent?  It's shorter, it
> fixes the problem I reported and it does not cause any regressions.

it's ugly, obfuscated code, adding useless temporary variables and tests
that used not to be needed to achieve the same computation (where we
want the code to be fast and would like to remove all tests in flags
computations...).
Furthermore, it's inconsistent as more operations use similar code then
would need to be patched too if those one are false.
Then, I got numerous test results which gives the correct result with
the same arguments you use, running qemu on different hosts (x86, x86_64
and PPC32/64), then I must find time to redo all those tests to get
convinced it's now broken, find what did broke it, then rollback the
changes that broke this code.

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[Tristan Gingold]

On Jun 17, 2008, at 6:06 PM, Julian Seward wrote:
> Below is a test case showing the problem with mullwo, addme and
> subfme.  On a real 7447 it prints
>

> addme   00000000 XER.CA=1  = 00000000 (cr 00000000 xer 20000000)

Did I miss the obvious, but why does 0 + 1 - 1 generate a carry ?

Re: [Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[Julian Seward]

> > addme   00000000 XER.CA=1  = 00000000 (cr 00000000 xer 20000000)
>
> Did I miss the obvious, but why does 0 + 1 - 1 generate a carry ?

Because (as per the docs) it's computing rA + XER.CA + 0xFFFF_FFFF,
which is 0 + 1 + 0xFFFF_FFFF == 0x1_0000_0000.  Which does indeed
carry.  I tested now on both a 7447 and a G5 and they both do that.

On a real 7447:

mullwo. 000f423f ffffffff  = fff0bdc1 (cr 80000000 xer 00000000)
addme   00000000 XER.CA=1  = 00000000 (cr 00000000 xer 20000000)
subfme  00000000 XER.CA=1  = ffffffff (cr 00000000 xer 20000000)

On a real G5:

mullwo. 000f423f ffffffff  = fff0bdc1 (cr 80000000 xer 00000000)
addme   00000000 XER.CA=1  = 00000000 (cr 00000000 xer 20000000)
subfme  00000000 XER.CA=1  = ffffffff (cr 00000000 xer 20000000)

J

Re: [Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[Tristan Gingold]

On Jun 19, 2008, at 3:36 AM, Julian Seward wrote:

>
>>> addme   00000000 XER.CA=1  = 00000000 (cr 00000000 xer 20000000)
>>
>> Did I miss the obvious, but why does 0 + 1 - 1 generate a carry ?
>
> Because (as per the docs) it's computing rA + XER.CA + 0xFFFF_FFFF,
> which is 0 + 1 + 0xFFFF_FFFF == 0x1_0000_0000.  Which does indeed
> carry.  I tested now on both a 7447 and a G5 and they both do that.

Ok thanks.  So addme set the carry unless both xer.ca and rA are 0.

Your expression is a little bit complex, especially the second part.
If xer_ca = 1 then res == argL, right ?

For reference:

  void do_addmeo (void)
  {
+    uint32_t argL = T0;
+    uint32_t res  = T0 + xer_ca + (-1);
+    uint32_t carried = res < argL || ((xer_ca & 1) == 1 && res ==  
argL);

Tristan.

Re: [Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[J. Mayer]

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

On Thu, 2008-06-19 at 04:36 -0400, Tristan Gingold wrote:
> On Jun 19, 2008, at 3:36 AM, Julian Seward wrote:
> 
> >
> >>> addme   00000000 XER.CA=1  = 00000000 (cr 00000000 xer 20000000)
> >>
> >> Did I miss the obvious, but why does 0 + 1 - 1 generate a carry ?
> >
> > Because (as per the docs) it's computing rA + XER.CA + 0xFFFF_FFFF,
> > which is 0 + 1 + 0xFFFF_FFFF == 0x1_0000_0000.  Which does indeed
> > carry.  I tested now on both a 7447 and a G5 and they both do that.
> 
> Ok thanks.  So addme set the carry unless both xer.ca and rA are 0.
> 
> Your expression is a little bit complex, especially the second part.
> If xer_ca = 1 then res == argL, right ?

OK, then I found the faulty change (from me) and reverting it should do
the trick and fix addme and subfme in 32 and 64 bits cases. I also added
some missing casts for addmeo. The fix is quite simple: the carry should
never be reseted by those instructions as xer_ca == 1 will always lead
to set the carry with the result (T0 + 1 -1 is always equal to T0,
whatever T0 value...)

For the mullw(o) issue, I guess the mullw seems faulty too, as it should
produce a 64 bits signed result when running in 64 bits mode. And I
guess this part of your patch is useless as res is already an int64_t :
-    if (likely((int32_t)res == res)) {
+    if (likely( ((int64_t)(int32_t)res) == ((int64_t)res) )) {
.
Can you confirm ?

-- 
J. Mayer <l_indien@magic.fr>
Never organized

[-- Attachment #2: addme_subfme.diff --]
[-- Type: text/x-patch, Size: 2301 bytes --]

Index: op.c
===================================================================
RCS file: /sources/qemu/qemu/target-ppc/op.c,v
retrieving revision 1.68
diff -u -d -d -p -r1.68 op.c
--- op.c	16 Nov 2007 14:11:28 -0000	1.68
+++ op.c	19 Jun 2008 08:57:01 -0000
@@ -920,8 +920,6 @@ void OPPROTO op_add_me (void)
     T0 += xer_ca + (-1);
     if (likely((uint32_t)T1 != 0))
         xer_ca = 1;
-    else
-        xer_ca = 0;
     RETURN();
 }
 
@@ -931,8 +929,6 @@ void OPPROTO op_add_me_64 (void)
     T0 += xer_ca + (-1);
     if (likely((uint64_t)T1 != 0))
         xer_ca = 1;
-    else
-        xer_ca = 0;
     RETURN();
 }
 #endif
@@ -1213,8 +1209,6 @@ void OPPROTO op_subfme (void)
     T0 = ~T0 + xer_ca - 1;
     if (likely((uint32_t)T0 != UINT32_MAX))
         xer_ca = 1;
-    else
-        xer_ca = 0;
     RETURN();
 }
 
@@ -1224,8 +1218,6 @@ void OPPROTO op_subfme_64 (void)
     T0 = ~T0 + xer_ca - 1;
     if (likely((uint64_t)T0 != UINT64_MAX))
         xer_ca = 1;
-    else
-        xer_ca = 0;
     RETURN();
 }
 #endif
Index: op_helper.c
===================================================================
RCS file: /sources/qemu/qemu/target-ppc/op_helper.c,v
retrieving revision 1.73
diff -u -d -d -p -r1.73 op_helper.c
--- op_helper.c	24 Nov 2007 02:03:55 -0000	1.73
+++ op_helper.c	19 Jun 2008 08:57:01 -0000
@@ -151,10 +151,8 @@ void do_addmeo (void)
     T0 += xer_ca + (-1);
     xer_ov = ((uint32_t)T1 & ((uint32_t)T1 ^ (uint32_t)T0)) >> 31;
     xer_so |= xer_ov;
-    if (likely(T1 != 0))
+    if (likely((uint32_t)T1 != 0))
         xer_ca = 1;
-    else
-        xer_ca = 0;
 }
 
 #if defined(TARGET_PPC64)
@@ -164,10 +162,8 @@ void do_addmeo_64 (void)
     T0 += xer_ca + (-1);
     xer_ov = ((uint64_t)T1 & ((uint64_t)T1 ^ (uint64_t)T0)) >> 63;
     xer_so |= xer_ov;
-    if (likely(T1 != 0))
+    if (likely((uint64_t)T1 != 0))
         xer_ca = 1;
-    else
-        xer_ca = 0;
 }
 #endif
 
@@ -312,8 +308,6 @@ void do_subfmeo (void)
     xer_so |= xer_ov;
     if (likely((uint32_t)T1 != UINT32_MAX))
         xer_ca = 1;
-    else
-        xer_ca = 0;
 }
 
 #if defined(TARGET_PPC64)
@@ -325,8 +319,6 @@ void do_subfmeo_64 (void)
     xer_so |= xer_ov;
     if (likely((uint64_t)T1 != UINT64_MAX))
         xer_ca = 1;
-    else
-        xer_ca = 0;
 }
 #endif
 

Re: [Qemu-devel] [PATCH] ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

[Aurelien Jarno]

On Sun, May 11, 2008 at 02:04:47AM +0200, Julian Seward wrote:
> 
> For ppc32 guests, computation of XER.CA and XER.OV in some obscure
> cases is incorrect.  At least, it doesn't produce the same results
> as a real MPC7447, and doesn't appear to be in accordance with the
> instruction set documentation.
> 
> The attached patch fixes it:
> 
> * addme{o}{.}, subfme{o}{.}: compute XER.CA correctly
> 
> * mullwo{.}: sign extend arguments before doing 64-bit
>   multiply, so as to make the XER.OV computation correct
> 

I fixed both problems by applying a mix of your patch and of the patch
from Jocelyn Mayer.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net