[PATCH] target: i386: Check float overflow about register stack

chengang@emindsoft.com.cn posted 1 patch 4 years, 1 month ago
Test docker-quick@centos7 failed
Test FreeBSD passed
Test docker-mingw@fedora passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200221034547.5215-1-chengang@emindsoft.com.cn
Maintainers: Richard Henderson <rth@twiddle.net>, Eduardo Habkost <ehabkost@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
target/i386/cpu.h        |  1 +
target/i386/fpu_helper.c | 70 ++++++++++++++++++++++++----------------
2 files changed, 44 insertions(+), 27 deletions(-)
[PATCH] target: i386: Check float overflow about register stack
Posted by chengang@emindsoft.com.cn 4 years, 1 month ago
From: Chen Gang <gang.chen.5i5j@gmail.com>

The fxam instruction also checks the register stack overflow, which can
be get by the following fstsw instruction. The related code is below, it
works well under real x86_64 hardware, but can not work under qemu-i386.

0006b63c <_CIsqrt>:
   6b63c:       55                      push   %ebp
   6b63d:       89 e5                   mov    %esp,%ebp
   6b63f:       83 ec 44                sub    $0x44,%esp
   6b642:       dd 1c 24                fstpl  (%esp)
   6b645:       9b                      fwait
   6b646:       e8 d5 04 00 00          call   6bb20 <wine_backtrace>
   6b64b:       b9 01 00 00 00          mov    $0x1,%ecx
   6b650:       d9 e5                   fxam
   6b652:       9b df e0                fstsw  %ax
   6b655:       66 25 00 45             and    $0x4500,%ax
   6b659:       66 3d 00 41             cmp    $0x4100,%ax
   6b65d:       74 07                   je     6b666 <_CIsqrt+0x2a>
   6b65f:       dd 1c cc                fstpl  (%esp,%ecx,8)
   6b662:       9b                      fwait
   6b663:       41                      inc    %ecx
   6b664:       eb ea                   jmp    6b650 <_CIsqrt+0x14>
   6b666:       89 4d fc                mov    %ecx,-0x4(%ebp)
   6b669:       e8 b2 0f 00 00          call   6c620 <MSVCRT_sqrt>
   6b66e:       8b 4d fc                mov    -0x4(%ebp),%ecx
   6b671:       dd 1c 24                fstpl  (%esp)
   6b674:       49                      dec    %ecx
   6b675:       dd 04 cc                fldl   (%esp,%ecx,8)
   6b678:       83 f9 00                cmp    $0x0,%ecx
   6b67b:       75 f7                   jne    6b674 <_CIsqrt+0x38>
   6b67d:       c9                      leave
   6b67e:       c3                      ret
   6b67f:       90                      nop

Signed-off-by: Chen Gang <chengang@emindsoft.com.cn>
---
 target/i386/cpu.h        |  1 +
 target/i386/fpu_helper.c | 70 ++++++++++++++++++++++++----------------
 2 files changed, 44 insertions(+), 27 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 576f309bbf..3e2b719ab7 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1394,6 +1394,7 @@ typedef struct CPUX86State {
     struct {} start_init_save;
 
     /* FPU state */
+    bool foverflow;
     unsigned int fpstt; /* top of stack index */
     uint16_t fpus;
     uint16_t fpuc;
diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c
index 99f28f267f..81f3cefe8b 100644
--- a/target/i386/fpu_helper.c
+++ b/target/i386/fpu_helper.c
@@ -91,17 +91,31 @@ void cpu_set_ignne(void)
 }
 #endif
 
+static inline void set_fpstt(CPUX86State *env, unsigned int fpstt,
+                             bool pop, bool full)
+{
+    env->foverflow = (fpstt > 7) && full; /* clear the original flag */
+    if (pop) {
+        if (full) {
+            env->fptags[env->fpstt] = 1; /* invalidate stack entry */
+        }
+        env->fpstt = fpstt & 7;
+    } else {
+        env->fpstt = fpstt & 7;
+        if (full) {
+            env->fptags[env->fpstt] = 0; /* validate stack entry */
+        }
+    }
+}
 
 static inline void fpush(CPUX86State *env)
 {
-    env->fpstt = (env->fpstt - 1) & 7;
-    env->fptags[env->fpstt] = 0; /* validate stack entry */
+    set_fpstt(env, env->fpstt - 1, false, true);
 }
 
 static inline void fpop(CPUX86State *env)
 {
-    env->fptags[env->fpstt] = 1; /* invalidate stack entry */
-    env->fpstt = (env->fpstt + 1) & 7;
+    set_fpstt(env, env->fpstt + 1, true, true);
 }
 
 static inline floatx80 helper_fldt(CPUX86State *env, target_ulong ptr,
@@ -211,11 +225,10 @@ void helper_flds_ST0(CPUX86State *env, uint32_t val)
         uint32_t i;
     } u;
 
-    new_fpstt = (env->fpstt - 1) & 7;
+    new_fpstt = env->fpstt - 1;
     u.i = val;
-    env->fpregs[new_fpstt].d = float32_to_floatx80(u.f, &env->fp_status);
-    env->fpstt = new_fpstt;
-    env->fptags[new_fpstt] = 0; /* validate stack entry */
+    env->fpregs[new_fpstt & 7].d = float32_to_floatx80(u.f, &env->fp_status);
+    set_fpstt(env, new_fpstt, false, true);
 }
 
 void helper_fldl_ST0(CPUX86State *env, uint64_t val)
@@ -226,31 +239,28 @@ void helper_fldl_ST0(CPUX86State *env, uint64_t val)
         uint64_t i;
     } u;
 
-    new_fpstt = (env->fpstt - 1) & 7;
+    new_fpstt = env->fpstt - 1;
     u.i = val;
-    env->fpregs[new_fpstt].d = float64_to_floatx80(u.f, &env->fp_status);
-    env->fpstt = new_fpstt;
-    env->fptags[new_fpstt] = 0; /* validate stack entry */
+    env->fpregs[new_fpstt & 7].d = float64_to_floatx80(u.f, &env->fp_status);
+    set_fpstt(env, new_fpstt, false, true);
 }
 
 void helper_fildl_ST0(CPUX86State *env, int32_t val)
 {
     int new_fpstt;
 
-    new_fpstt = (env->fpstt - 1) & 7;
-    env->fpregs[new_fpstt].d = int32_to_floatx80(val, &env->fp_status);
-    env->fpstt = new_fpstt;
-    env->fptags[new_fpstt] = 0; /* validate stack entry */
+    new_fpstt = env->fpstt - 1;
+    env->fpregs[new_fpstt & 7].d = int32_to_floatx80(val, &env->fp_status);
+    set_fpstt(env, new_fpstt, false, true);
 }
 
 void helper_fildll_ST0(CPUX86State *env, int64_t val)
 {
     int new_fpstt;
 
-    new_fpstt = (env->fpstt - 1) & 7;
-    env->fpregs[new_fpstt].d = int64_to_floatx80(val, &env->fp_status);
-    env->fpstt = new_fpstt;
-    env->fptags[new_fpstt] = 0; /* validate stack entry */
+    new_fpstt = env->fpstt - 1;
+    env->fpregs[new_fpstt & 7].d = int64_to_floatx80(val, &env->fp_status);
+    set_fpstt(env, new_fpstt, false, true);
 }
 
 uint32_t helper_fsts_ST0(CPUX86State *env)
@@ -345,10 +355,9 @@ void helper_fldt_ST0(CPUX86State *env, target_ulong ptr)
 {
     int new_fpstt;
 
-    new_fpstt = (env->fpstt - 1) & 7;
-    env->fpregs[new_fpstt].d = helper_fldt(env, ptr, GETPC());
-    env->fpstt = new_fpstt;
-    env->fptags[new_fpstt] = 0; /* validate stack entry */
+    new_fpstt = env->fpstt - 1;
+    env->fpregs[new_fpstt & 7].d = helper_fldt(env, ptr, GETPC());
+    set_fpstt(env, new_fpstt, false, true);
 }
 
 void helper_fstt_ST0(CPUX86State *env, target_ulong ptr)
@@ -368,13 +377,13 @@ void helper_fpop(CPUX86State *env)
 
 void helper_fdecstp(CPUX86State *env)
 {
-    env->fpstt = (env->fpstt - 1) & 7;
+    set_fpstt(env, env->fpstt - 1, false, false);
     env->fpus &= ~0x4700;
 }
 
 void helper_fincstp(CPUX86State *env)
 {
-    env->fpstt = (env->fpstt + 1) & 7;
+    set_fpstt(env, env->fpstt + 1, true, false);
     env->fpus &= ~0x4700;
 }
 
@@ -382,6 +391,7 @@ void helper_fincstp(CPUX86State *env)
 
 void helper_ffree_STN(CPUX86State *env, int st_index)
 {
+    set_fpstt(env, env->fpstt + st_index, true, false);
     env->fptags[(env->fpstt + st_index) & 7] = 1;
 }
 
@@ -644,6 +654,7 @@ void helper_fninit(CPUX86State *env)
 {
     env->fpus = 0;
     env->fpstt = 0;
+    env->foverflow = false;
     cpu_set_fpuc(env, 0x37f);
     env->fptags[0] = 1;
     env->fptags[1] = 1;
@@ -1008,6 +1019,11 @@ void helper_fxam_ST0(CPUX86State *env)
     } else {
         env->fpus |= 0x400;
     }
+
+    if (env->foverflow) {
+        env->fpus |= 0x4100;
+        env->fpus &= ~0x400;
+    }
 }
 
 static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32,
@@ -1636,7 +1652,7 @@ void helper_ldmxcsr(CPUX86State *env, uint32_t val)
 
 void helper_enter_mmx(CPUX86State *env)
 {
-    env->fpstt = 0;
+    set_fpstt(env, 0, true, false);
     *(uint32_t *)(env->fptags) = 0;
     *(uint32_t *)(env->fptags + 4) = 0;
 }
-- 
2.24.0.308.g228f53135a




Re: [PATCH] target: i386: Check float overflow about register stack
Posted by Paolo Bonzini 4 years, 1 month ago
On 21/02/20 04:45, chengang@emindsoft.com.cn wrote:
>  static inline void fpush(CPUX86State *env)
>  {
> -    env->fpstt = (env->fpstt - 1) & 7;
> -    env->fptags[env->fpstt] = 0; /* validate stack entry */
> +    set_fpstt(env, env->fpstt - 1, false, true);

On overflow fpstt is ~0, so this does:

    env->foverflow = true;
    env->fpstt = 7;
    env->fptags[7] = 0;      /* validate stack entry */

Is this correct?  You are going to set ST0 so the register should not be
marked empty.

>  static inline void fpop(CPUX86State *env)
>  {
> -    env->fptags[env->fpstt] = 1; /* invalidate stack entry */
> -    env->fpstt = (env->fpstt + 1) & 7;
> +    set_fpstt(env, env->fpstt + 1, true, true);

While here:

    env->foverflow = true;
    env->fptags[7] = 1;
    env->fpstt = 0;

>  void helper_fdecstp(CPUX86State *env)
>  {
> -    env->fpstt = (env->fpstt - 1) & 7;
> +    set_fpstt(env, env->fpstt - 1, false, false);

This is clearing env->foverflow.  But after 8 consecutive fdecstp or
fincstp the result of FXAM should not change.

>      env->fpus &= ~0x4700;
>  }
>  
>  void helper_fincstp(CPUX86State *env)
>  {
> -    env->fpstt = (env->fpstt + 1) & 7;
> +    set_fpstt(env, env->fpstt + 1, true, false);

Same here.

The actual bug is hinted in helper_fxam_ST0:

    /* XXX: test fptags too */

I think the correct fix should be something like

diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c
index 99f28f267f..792a128a6d 100644
--- a/target/i386/fpu_helper.c
+++ b/target/i386/fpu_helper.c
@@ -991,7 +991,11 @@ void helper_fxam_ST0(CPUX86State *env)
         env->fpus |= 0x200; /* C1 <-- 1 */
     }

-    /* XXX: test fptags too */
+    if (env->fptags[env->fpstt]) {
+        env->fpus |= 0x4100; /* Empty */
+        return;
+    }
+
     expdif = EXPD(temp);
     if (expdif == MAXEXPD) {
         if (MANTD(temp) == 0x8000000000000000ULL) {

Paolo


Re: [PATCH] target: i386: Check float overflow about register stack
Posted by Chen Gang 4 years, 1 month ago
On 2020/2/21 下午4:58, Paolo Bonzini wrote:
> On 21/02/20 04:45, chengang@emindsoft.com.cn wrote:
>>  static inline void fpush(CPUX86State *env)
>>  {
>> -    env->fpstt = (env->fpstt - 1) & 7;
>> -    env->fptags[env->fpstt] = 0; /* validate stack entry */
>> +    set_fpstt(env, env->fpstt - 1, false, true);
> 
> On overflow fpstt is ~0, so this does:
> 
>     env->foverflow = true;
>     env->fpstt = 7;
>     env->fptags[7] = 0;      /* validate stack entry */
> 
> Is this correct?  You are going to set ST0 so the register should not be
> marked empty.
> 

Originally, I wanted to add foverflow to mark the stack overflow only,
and kept another things no touch.

But I think what you said above is correct, for me, if fpush/f[i]ld*_STO
are overflow, the env->fpstt, env->fpregs and env->fptags should be kept
no touch, and foverflow is set true, so there is no negative effect.

Welcome your idea.

>>  void helper_fdecstp(CPUX86State *env)
>>  {
>> -    env->fpstt = (env->fpstt - 1) & 7;
>> +    set_fpstt(env, env->fpstt - 1, false, false);
> 
> This is clearing env->foverflow.  But after 8 consecutive fdecstp or
> fincstp the result of FXAM should not change.
> 
>>      env->fpus &= ~0x4700;
>>  }
>>  
>>  void helper_fincstp(CPUX86State *env)
>>  {
>> -    env->fpstt = (env->fpstt + 1) & 7;
>> +    set_fpstt(env, env->fpstt + 1, true, false);
> 
> Same here.
> 

OK. thanks.

Now if foverflow is only for fpush/f[i]ld*_ST0, I guess fincstp/fdecstp
can clear foverflow. The env->fptags are only for fpop, which keep no
touch in fincstp/fdecstp.

> The actual bug is hinted in helper_fxam_ST0:
> 
>     /* XXX: test fptags too */
> 
> I think the correct fix should be something like
> 
> diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c
> index 99f28f267f..792a128a6d 100644
> --- a/target/i386/fpu_helper.c
> +++ b/target/i386/fpu_helper.c
> @@ -991,7 +991,11 @@ void helper_fxam_ST0(CPUX86State *env)
>          env->fpus |= 0x200; /* C1 <-- 1 */
>      }
> 
> -    /* XXX: test fptags too */
> +    if (env->fptags[env->fpstt]) {
> +        env->fpus |= 0x4100; /* Empty */
> +        return;
> +    }
> +

For fpop overflow, this fix is enough, but for me, we still need
foverflow to check fpush/fld*_ST0 overflow.

Don't you think we need check fpush/f[i]ld*_ST0 overflow?

Thanks



Re: [PATCH] target: i386: Check float overflow about register stack
Posted by Paolo Bonzini 4 years, 1 month ago
On 21/02/20 15:09, Chen Gang wrote:
>> -    /* XXX: test fptags too */
>> +    if (env->fptags[env->fpstt]) {
>> +        env->fpus |= 0x4100; /* Empty */
>> +        return;
>> +    }
>> +
> For fpop overflow, this fix is enough, but for me, we still need
> foverflow to check fpush/fld*_ST0 overflow.
> 
> Don't you think we need check fpush/f[i]ld*_ST0 overflow?

After fld/fild or any other push, FXAM ST0 should not return empty in my
opinion.

Paolo


Re: [PATCH] target: i386: Check float overflow about register stack
Posted by Chen Gang 4 years, 1 month ago
On 2020/2/22 上午12:18, Paolo Bonzini wrote:
> On 21/02/20 15:09, Chen Gang wrote:
>>> -    /* XXX: test fptags too */
>>> +    if (env->fptags[env->fpstt]) {
>>> +        env->fpus |= 0x4100; /* Empty */
>>> +        return;
>>> +    }
>>> +
>> For fpop overflow, this fix is enough, but for me, we still need
>> foverflow to check fpush/fld*_ST0 overflow.
>>
>> Don't you think we need check fpush/f[i]ld*_ST0 overflow?
> 
> After fld/fild or any other push, FXAM ST0 should not return empty in my
> opinion.
> 

OK, it sounds reasonable.

After check the intel document for f[i]ld* instructions, it says:

  "Set C1 to 1 if stack overflow occurred; set to 0 otherwise".

In helper_fxam_ST0, I guess, we need "env->fpus |= 0x200" (but I don't
know wheter it will be conflict with SIGND(temp)). And we have to still
need foverflow, because all env->fptags being 0 doesn't mean overflow.

Thanks.



Re: [PATCH] target: i386: Check float overflow about register stack
Posted by Paolo Bonzini 4 years, 1 month ago
On 22/02/20 03:10, Chen Gang wrote:
> Set C1 to 1 if stack overflow occurred; set to 0 otherwise".
> 
> In helper_fxam_ST0, I guess, we need "env->fpus |= 0x200" (but I don't
> know wheter it will be conflict with SIGND(temp)). And we have to still
> need foverflow, because all env->fptags being 0 doesn't mean overflow.

No, you need to add "env->fpus |= 0x200" and "env->fpus &= ~0x200"
directly to fpush, fpop, etc.

Polo


Re: [PATCH] target: i386: Check float overflow about register stack
Posted by Chen Gang 4 years, 1 month ago
On 2020/2/22 下午3:37, Paolo Bonzini wrote:
> On 22/02/20 03:10, Chen Gang wrote:
>> Set C1 to 1 if stack overflow occurred; set to 0 otherwise".
>>
>> In helper_fxam_ST0, I guess, we need "env->fpus |= 0x200" (but I don't
>> know wheter it will be conflict with SIGND(temp)). And we have to still
>> need foverflow, because all env->fptags being 0 doesn't mean overflow.
> 
> No, you need to add "env->fpus |= 0x200" and "env->fpus &= ~0x200"
> directly to fpush, fpop, etc.
> 

OK. The content below is my next TODO, welcome your opinions.

When overflow occurs, for me, we need keep everything no touch except
set C1 flag. In fxam, we don't clear C1, but keep no touch for clearning
C1 in another places.

When underflow occurs, for me, we need keep everything no touch except
set env->fpstt 8, so the next consecutive fpop/f[i]stp* can be checked
easier, and the next fpush/f[i]ld* can work well in normal.

For fxam, we check env->fpstt == 8 and env->fptags for empty. And when
env->fpstt is 8, it need be set 7 before used in fincstp and ffree_STN.

Thanks.



Re: [PATCH] target: i386: Check float overflow about register stack
Posted by Paolo Bonzini 4 years, 1 month ago
On 22/02/20 13:25, Chen Gang wrote:
> On 2020/2/22 下午3:37, Paolo Bonzini wrote:
>> On 22/02/20 03:10, Chen Gang wrote:
>>> Set C1 to 1 if stack overflow occurred; set to 0 otherwise".
>>>
>>> In helper_fxam_ST0, I guess, we need "env->fpus |= 0x200" (but I don't
>>> know wheter it will be conflict with SIGND(temp)). And we have to still
>>> need foverflow, because all env->fptags being 0 doesn't mean overflow.
>>
>> No, you need to add "env->fpus |= 0x200" and "env->fpus &= ~0x200"
>> directly to fpush, fpop, etc.
>>
> 
> OK. The content below is my next TODO, welcome your opinions.
> 
> When overflow occurs, for me, we need keep everything no touch except
> set C1 flag.

No, push will overwrite the top entry if there is overflow.

> In fxam, we don't clear C1, but keep no touch for clearning
> C1 in another places.

FXAM is neither push nor pop, it just detects an empty slot via fptags.
 FXAM should be okay with my patch.

> When underflow occurs, for me, we need keep everything no touch except
> set env->fpstt 8, so the next consecutive fpop/f[i]stp* can be checked
> easier, and the next fpush/f[i]ld* can work well in normal.
> For fxam, we check env->fpstt == 8 and env->fptags for empty. And when
> env->fpstt is 8, it need be set 7 before used in fincstp and ffree_STN.

I don't think you need env->fpstt to be set to 8 in any case.  Also, pop
must mark ST(0) as empty always, even if underflow occurs, and also
clear C1 if underflow occurs.

Paolo


Re: [PATCH] target: i386: Check float overflow about register stack
Posted by Chen Gang 4 years, 1 month ago
On 2020/2/24 下午8:43, Paolo Bonzini wrote:
> On 22/02/20 13:25, Chen Gang wrote:
>> On 2020/2/22 下午3:37, Paolo Bonzini wrote:
>>> On 22/02/20 03:10, Chen Gang wrote:
>>>> Set C1 to 1 if stack overflow occurred; set to 0 otherwise".
>>>>
>>>> In helper_fxam_ST0, I guess, we need "env->fpus |= 0x200" (but I don't
>>>> know wheter it will be conflict with SIGND(temp)). And we have to still
>>>> need foverflow, because all env->fptags being 0 doesn't mean overflow.
>>>
>>> No, you need to add "env->fpus |= 0x200" and "env->fpus &= ~0x200"
>>> directly to fpush, fpop, etc.
>>>
>>
>> OK. The content below is my next TODO, welcome your opinions.
>>
>> When overflow occurs, for me, we need keep everything no touch except
>> set C1 flag.
> 
> No, push will overwrite the top entry if there is overflow.
> 
>> In fxam, we don't clear C1, but keep no touch for clearning
>> C1 in another places.
> 
> FXAM is neither push nor pop, it just detects an empty slot via fptags.
>  FXAM should be okay with my patch.
> 

OK. I am not quite clear about it, but it fixes the current issues at
least. Please apply your patch.

Thanks.



Re: [PATCH] target: i386: Check float overflow about register stack
Posted by Chen Gang 4 years, 1 month ago
On 2020/2/22 上午10:10, Chen Gang wrote:
> On 2020/2/22 上午12:18, Paolo Bonzini wrote:
>> On 21/02/20 15:09, Chen Gang wrote:
>>>> -    /* XXX: test fptags too */
>>>> +    if (env->fptags[env->fpstt]) {
>>>> +        env->fpus |= 0x4100; /* Empty */
>>>> +        return;
>>>> +    }
>>>> +
>>> For fpop overflow, this fix is enough, but for me, we still need
>>> foverflow to check fpush/fld*_ST0 overflow.
>>>
>>> Don't you think we need check fpush/f[i]ld*_ST0 overflow?
>>
>> After fld/fild or any other push, FXAM ST0 should not return empty in my
>> opinion.
>>
> 
> OK, it sounds reasonable.
> 
> After check the intel document for f[i]ld* instructions, it says:
> 
>   "Set C1 to 1 if stack overflow occurred; set to 0 otherwise".
> 
> In helper_fxam_ST0, I guess, we need "env->fpus |= 0x200" (but I don't
> know wheter it will be conflict with SIGND(temp)). And we have to still
> need foverflow, because all env->fptags being 0 doesn't mean overflow.
> 

After read the document more about fstp* and fld* instructions, I find
that fstp* are also related with C1, and fld* will generate IS exception
when stack underflow or overflow occurred.

It seems more modifications should be done (but they will be several
patches).

Thanks.