Re: [Qemu-devel] Bug in s390 instruction emulation

[tg@gmplib.org (Torbjörn Granlund)]

Paolo Bonzini <pbonzini@redhat.com> writes:
  
  Something like this?
  
  diff --git a/target-s390x/mem_helper.c b/target-s390x/mem_helper.c
  index 5a55de8..4de3fc2 100644
  --- a/target-s390x/mem_helper.c
  +++ b/target-s390x/mem_helper.c
  @@ -490,10 +490,18 @@ uint32_t HELPER(ex)(CPUS390XState *env, uint32_t cc, uint64_t v1,
               helper_mvc(env, l, get_address(env, 0, b1, d1),
                          get_address(env, 0, b2, d2));
               break;
  +        case 0x400:
  +            cc = helper_nc(env, l, get_address(env, 0, b1, d1),
  +                            get_address(env, 0, b2, d2));
  +            break;
           case 0x500:
               cc = helper_clc(env, l, get_address(env, 0, b1, d1),
                               get_address(env, 0, b2, d2));
               break;
  +        case 0x600:
  +            cc = helper_oc(env, l, get_address(env, 0, b1, d1),
  +                            get_address(env, 0, b2, d2));
  +            break;
           case 0x700:
               cc = helper_xc(env, l, get_address(env, 0, b1, d1),
                              get_address(env, 0, b2, d2));
  
That seems to work as per the needs of GMP.  I had expected a bigger
change to be needed.  Thanks!

Below is a more complete patch for the SLB* and SLBG* bugs.

This patch is to be attributed to torbjorng@google.com.

This patch fixes the bug with borrow_in being set incorrectly, but it
also simplifies the logic to be much more plain, improving speed.  It
fixes both the 32-bit SLB* and 64-bit SLBG*.

The SLBG* change has been well-tested.  I haven't tested the SLB* change
explicitly, but the code was copy-pasted from the tested code.

The error of these functions' current implementations would not likely
be triggered by compiler-generated code, since the only error was in the
state of the carry/borrow flag.  Compilers rarely generate an
instruction sequence such as carry-set -> carry-set-and-use ->
carry-use.

(With Paolo's fix and mine, there are still a couple of failures from
GMP's testsuite, but they are almost surely due to incorrect code
generation from gcc 4.9.  But since this gcc is running under qemu, it
might be qemu bugs.  I intend to investigate this.)

--- target-s390x/.~/cc_helper.c.~1~	2014-12-09 15:45:44.000000000 +0100
+++ target-s390x/cc_helper.c	2014-12-14 23:02:31.605725763 +0100
@@ -179,16 +179,11 @@
 
 static uint32_t cc_calc_subb_64(uint64_t a1, uint64_t a2, uint64_t ar)
 {
-    /* We had borrow-in if normal subtraction isn't equal.  */
-    int borrow_in = ar - (a1 - a2);
     int borrow_out;
 
-    /* If a2 was ULONG_MAX, and borrow_in, then a2 is logically 65 bits,
-       and we must have had borrow out.  */
-    if (borrow_in && a2 == (uint64_t)-1) {
-        borrow_out = 1;
+    if (ar != a1 - a2) {	/* difference means borrow-in */
+        borrow_out = (a2 >= a1);
     } else {
-        a2 += borrow_in;
         borrow_out = (a2 > a1);
     }
 
@@ -285,16 +280,11 @@
 
 static uint32_t cc_calc_subb_32(uint32_t a1, uint32_t a2, uint32_t ar)
 {
-    /* We had borrow-in if normal subtraction isn't equal.  */
-    int borrow_in = ar - (a1 - a2);
     int borrow_out;
 
-    /* If a2 was UINT_MAX, and borrow_in, then a2 is logically 65 bits,
-       and we must have had borrow out.  */
-    if (borrow_in && a2 == (uint32_t)-1) {
-        borrow_out = 1;
+    if (ar != a1 - a2) {	/* difference means borrow-in */
+        borrow_out = (a2 >= a1);
     } else {
-        a2 += borrow_in;
         borrow_out = (a2 > a1);
     }
 



-- 
Torbjörn
Please encrypt, key id 0xC8601622