[Qemu-devel] Bug in s390 instruction emulation

[Qemu-devel] Bug in s390 instruction emulation

[Torbjörn Granlund]

The s390 instruction emulation makes GMP fail most of its tests.
I have isolated one of the problems:

How to reproduce:

gcc m.c x.s
./a.out

Correct output on actual hardware:
ffffffff

Incorrect output using QEMU 2.2.0 rc4:
0

File m.c:
#include <stdio.h>
int foo();
int main() { printf("%x\n", foo()); return 0; }

File x.s:
	.text
	.align	8
	.globl	foo
	.type	foo,@function
foo:	lghi	%r2, 0
	lghi	%r3, 1
	slgr	%r2, %r3
	slbgr	%r3, %r3
	slbgr	%r2, %r2
	br	%r14

(This is using "user mode" emulation.  System mode emulation doesn't
work at all, and never did, as I am sure you know.  I suppose getting
basic instruction emulation correct is a good first step.)

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

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

[Torbjörn Granlund]

I wrote:

  The s390 instruction emulation makes GMP fail most of its tests.
  I have isolated one of the problems:
  
  How to reproduce:
  
  gcc m.c x.s
  ./a.out
  
  Correct output on actual hardware:
  ffffffff
  
  Incorrect output using QEMU 2.2.0 rc4:
  0
  
  File m.c:
  #include <stdio.h>
  int foo();
  int main() { printf("%x\n", foo()); return 0; }
  
  File x.s:
  	.text
  	.align	8
  	.globl	foo
  	.type	foo,@function
  foo:	lghi	%r2, 0
  	lghi	%r3, 1
  	slgr	%r2, %r3
  	slbgr	%r3, %r3
  	slbgr	%r2, %r2
  	br	%r14
  
Turns out that all failures except 3 are due to subb borrow handling
code which (almost) never works when there is borrow-in.  A minimal fix
is quite simple:

*** /home/tege/qemu/qemu-2.2.0/target-s390x/.~/cc_helper.c.~1~	Tue Dec  9 15:45:44 2014
--- /home/tege/qemu/qemu-2.2.0/target-s390x/cc_helper.c	Sat Dec 13 22:47:11 2014
***************
*** 182,184 ****
      /* We had borrow-in if normal subtraction isn't equal.  */
!     int borrow_in = ar - (a1 - a2);
      int borrow_out;
--- 182,184 ----
      /* We had borrow-in if normal subtraction isn't equal.  */
!     int borrow_in = (a1 - a2) - ar;
      int borrow_out;

There is at least one more instruction emulation error which I have not
yet isolated [two test failures].  And then EX is not implemented for
logical operations [one test failure].

This latter problem is adequately reported by qemu:
qemu: fatal: EXECUTE on instruction prefix 0xd400 not implemented
qemu: fatal: EXECUTE on instruction prefix 0xd600 not implemented


(I will not follow up in any way about this bug.  If you choose to
ignore the bug report, then qemu will remain slightly buggier.  If you
choose to deal with the bug in a reasonably timely manner, I'd be happy
to contribute.)


Torbjörn
Please encrypt, key id 0xC8601622

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

[Paolo Bonzini]

On 13/12/2014 23:10, Torbjörn Granlund wrote:
> I wrote:
> 
>   The s390 instruction emulation makes GMP fail most of its tests.
>   I have isolated one of the problems:
>   
>   How to reproduce:
>   
>   gcc m.c x.s
>   ./a.out
>   
>   Correct output on actual hardware:
>   ffffffff
>   
>   Incorrect output using QEMU 2.2.0 rc4:
>   0
>   
>   File m.c:
>   #include <stdio.h>
>   int foo();
>   int main() { printf("%x\n", foo()); return 0; }
>   
>   File x.s:
>   	.text
>   	.align	8
>   	.globl	foo
>   	.type	foo,@function
>   foo:	lghi	%r2, 0
>   	lghi	%r3, 1
>   	slgr	%r2, %r3
>   	slbgr	%r3, %r3
>   	slbgr	%r2, %r2
>   	br	%r14
>   
> Turns out that all failures except 3 are due to subb borrow handling
> code which (almost) never works when there is borrow-in.  A minimal fix
> is quite simple:
> 
> *** /home/tege/qemu/qemu-2.2.0/target-s390x/.~/cc_helper.c.~1~	Tue Dec  9 15:45:44 2014
> --- /home/tege/qemu/qemu-2.2.0/target-s390x/cc_helper.c	Sat Dec 13 22:47:11 2014
> ***************
> *** 182,184 ****
>       /* We had borrow-in if normal subtraction isn't equal.  */
> !     int borrow_in = ar - (a1 - a2);
>       int borrow_out;
> --- 182,184 ----
>       /* We had borrow-in if normal subtraction isn't equal.  */
> !     int borrow_in = (a1 - a2) - ar;
>       int borrow_out;
> 
> There is at least one more instruction emulation error which I have not
> yet isolated [two test failures].  And then EX is not implemented for
> logical operations [one test failure].
> 
> This latter problem is adequately reported by qemu:
> qemu: fatal: EXECUTE on instruction prefix 0xd400 not implemented
> qemu: fatal: EXECUTE on instruction prefix 0xd600 not implemented

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

Paolo

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

[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