ppc32 guests: fix computation of XER.{CA, OV} in addme, subfme, mullwo

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

Index: target-ppc/op_helper.c
===================================================================
@@ -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
===================================================================
@@ -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;

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
 `. `'   [email protected]         | [email protected]
   `-    people.debian.org/~aurel32 | www.aurel32.net

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;
}

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

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 <[email protected]>
Never organized

Thanks for the encouragement.

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

J

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 ?

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

J. Mayer <[email protected]>
Never organized

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

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 <[email protected]>
Never organized

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

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.

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 <[email protected]>
Never organized

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
@@ -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
@@ -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
 

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
 `. `'   [email protected]         | [email protected]
   `-    people.debian.org/~aurel32 | www.aurel32.net