Patchwork Patch: fix FSTENV (and FSAVE) instructions in target-i386 (resend, cleaned up).

login
register
mail settings
Submitter ChALkeR
Date Nov. 26, 2010, 1:14 p.m.
Message ID <AANLkTi=xVSUPoDcdhiCUVSHh6n8xGs6t4Bvgt3FyKzuf@mail.gmail.com>
Download mbox | patch
Permalink /patch/73174/
State New
Headers show

Comments

ChALkeR - Nov. 26, 2010, 1:14 p.m.
Patch for the bug https://bugs.launchpad.net/qemu/+bug/661696

The testcase:

$ cat test.c
#include <stdio.h>

extern void *x;

int main() {
        int a;
        asm volatile ("x: fldz\n\
        push %%edx\n\
        fnstenv -0xc(%%esp)\n\
        pop %%edx\n" : "=d" (a) : : "memory");
        printf ("%x %x\n", a, &x);
        return 0;
}
$ gcc -m32 test.c -o test
$ ./test
80483ae 80483ae
$ ./qemu-build/i386-linux-user/qemu-i386 ./test
0 80483ae
$ ./qemu-fixed-build/i386-linux-user/qemu-i386 ./test
80483ae 80483ae

The patch:

Signed-off-by: Nikita A Skovoroda <chalkerx@gmail.com>

From e07b99b12a9dd4d933936d5376efde8a992472dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=D0=A1=D0=BA=D0=BE=D0=B2=D0=BE=D1=80=D0=BE=D0=B4=D0=B0=20=D0=9D=D0=B8=D0=BA=D0=B8=D1=82=D0=B0=20=D0=90=D0=BD=D0=B4=D1=80=D0=B5=D0=B5=D0=B2=D0=B8=D1=87?=
<chalkerx@gmail.com>
Date: Fri, 26 Nov 2010 15:35:05 +0300
Subject: [PATCH] Fix FSTENV (and FSAVE) instructions in target-i386.
Fixes bug #616696.

---
 target-i386/cpu.h       |    1 +
 target-i386/helper.h    |    2 ++
 target-i386/op_helper.c |    9 +++++++--
 target-i386/translate.c |    1 +
 4 files changed, 11 insertions(+), 2 deletions(-)

         /* string ops */
Blue Swirl - Nov. 26, 2010, 7:30 p.m.
On Fri, Nov 26, 2010 at 1:14 PM, ChALkeR <chalkerx@gmail.com> wrote:
> Patch for the bug https://bugs.launchpad.net/qemu/+bug/661696
>
> The testcase:
>
> $ cat test.c
> #include <stdio.h>
>
> extern void *x;
>
> int main() {
>        int a;
>        asm volatile ("x: fldz\n\
>        push %%edx\n\
>        fnstenv -0xc(%%esp)\n\
>        pop %%edx\n" : "=d" (a) : : "memory");
>        printf ("%x %x\n", a, &x);
>        return 0;
> }
> $ gcc -m32 test.c -o test
> $ ./test
> 80483ae 80483ae
> $ ./qemu-build/i386-linux-user/qemu-i386 ./test
> 0 80483ae
> $ ./qemu-fixed-build/i386-linux-user/qemu-i386 ./test
> 80483ae 80483ae
>
> The patch:
>
> Signed-off-by: Nikita A Skovoroda <chalkerx@gmail.com>
>
> From e07b99b12a9dd4d933936d5376efde8a992472dd Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?=D0=A1=D0=BA=D0=BE=D0=B2=D0=BE=D1=80=D0=BE=D0=B4=D0=B0=20=D0=9D=D0=B8=D0=BA=D0=B8=D1=82=D0=B0=20=D0=90=D0=BD=D0=B4=D1=80=D0=B5=D0=B5=D0=B2=D0=B8=D1=87?=
> <chalkerx@gmail.com>
> Date: Fri, 26 Nov 2010 15:35:05 +0300
> Subject: [PATCH] Fix FSTENV (and FSAVE) instructions in target-i386.
> Fixes bug #616696.

This is a bit terse, the description should preferably state the
problem (for extra coolness, in past tense) and then how the patch
fixes it. For example:

IP field stored by FSTENV was zero instead of IP at last FPU instruction.

Store the IP value to env and use that in FSTENV helper to store the correct IP.

>
> ---
>  target-i386/cpu.h       |    1 +
>  target-i386/helper.h    |    2 ++
>  target-i386/op_helper.c |    9 +++++++--
>  target-i386/translate.c |    1 +
>  4 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 06e40f3..aad0dcb 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -642,6 +642,7 @@ typedef struct CPUX86State {
>     uint16_t fpuc;
>     uint8_t fptags[8];   /* 0 = valid, 1 = empty */
>     FPReg fpregs[8];
> +    target_ulong fpip;

Also CS, the instruction and data pointer should be saved for completeness.

>
>     /* emulator internal variables */
>     float_status fp_status;
> diff --git a/target-i386/helper.h b/target-i386/helper.h
> index 6b518ad..26b47a3 100644
> --- a/target-i386/helper.h
> +++ b/target-i386/helper.h
> @@ -3,6 +3,8 @@
>  DEF_HELPER_FLAGS_1(cc_compute_all, TCG_CALL_PURE, i32, int)
>  DEF_HELPER_FLAGS_1(cc_compute_c, TCG_CALL_PURE, i32, int)
>
> +DEF_HELPER_1(save_fpip, void, tl)
> +
>  DEF_HELPER_0(lock, void)
>  DEF_HELPER_0(unlock, void)
>  DEF_HELPER_2(write_eflags, void, tl, i32)
> diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
> index 43fbd0c..ab65f0c 100644
> --- a/target-i386/op_helper.c
> +++ b/target-i386/op_helper.c
> @@ -109,6 +109,11 @@ static const CPU86_LDouble f15rk[7] =
>
>  static spinlock_t global_cpu_lock = SPIN_LOCK_UNLOCKED;
>
> +void helper_save_fpip(target_ulong fpip)
> +{
> +    env->fpip = fpip;
> +}
> +
>  void helper_lock(void)
>  {
>     spin_lock(&global_cpu_lock);
> @@ -4273,7 +4278,7 @@ void helper_fstenv(target_ulong ptr, int data32)
>         stl(ptr, env->fpuc);
>         stl(ptr + 4, fpus);
>         stl(ptr + 8, fptag);
> -        stl(ptr + 12, 0); /* fpip */
> +        stl(ptr + 12, env->fpip); /* fpip */
>         stl(ptr + 16, 0); /* fpcs */
>         stl(ptr + 20, 0); /* fpoo */
>         stl(ptr + 24, 0); /* fpos */
> @@ -4282,7 +4287,7 @@ void helper_fstenv(target_ulong ptr, int data32)
>         stw(ptr, env->fpuc);
>         stw(ptr + 2, fpus);
>         stw(ptr + 4, fptag);
> -        stw(ptr + 6, 0);
> +        stw(ptr + 6, env->fpip);

Please also consider fixing FSAVE and FXSAVE. Alternatively, a comment
with XXX to highlight the unimplemented features would be nice.

>         stw(ptr + 8, 0);
>         stw(ptr + 10, 0);
>         stw(ptr + 12, 0);
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 7b6e3c2..c7d1d33 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -5974,6 +5974,7 @@ static target_ulong disas_insn(DisasContext *s,
> target_ulong pc_start)
>                 goto illegal_op;
>             }
>         }
> +        gen_helper_save_fpip(tcg_const_tl(pc_start - s->cs_base));

You can simply use tcg_gen_movi_tl() and tcg_gen_st_tl(), no need for
a helper except for the data pointer.

In this place, the data would be saved for every instruction. But I
think only FPU instructions should update the fields.
ChALkeR - Nov. 26, 2010, 10:53 p.m.
> Please also consider fixing FSAVE and FXSAVE.

FSAVE also works, i checked twice (code and test).
FSAVE in QEMU calls FSTENV.

op_helper.c:
> void helper_fsave(target_ulong ptr, int data32)
> {
>    CPU86_LDouble tmp;
>    int i;
>
>    helper_fstenv(ptr, data32);

Not sure how FXSAVE operates, that has to be checked in the manuals.
But the patch does not break anything and fixes FSTENV and FSAVE instructions.

> In this place, the data would be saved for every instruction. But I
> think only FPU instructions should update the fields.

I do not understand. The patch stores eip only for FPU instructions.
This is correct.
FSTENV should put the eip of the last FPU instruction in the stack.
The helper is called at the end of every instruction on floating-point
case of the switch.
There should be some additional data, too, but the correct operation
of fpip is provided by the patch.

translate.c, line 2461:
>        /************************/
>        /* floats */
>    case 0xd8 ... 0xdf:

> You can simply use tcg_gen_movi_tl() and tcg_gen_st_tl(), no need for
> a helper except for the data pointer.

Can you fix it, please, if I did something wrong?

Sorry for my bad English.

2010/11/26 Blue Swirl <blauwirbel@gmail.com>:
> On Fri, Nov 26, 2010 at 1:14 PM, ChALkeR <chalkerx@gmail.com> wrote:
>> Patch for the bug https://bugs.launchpad.net/qemu/+bug/661696
>>
>> The testcase:
>>
>> $ cat test.c
>> #include <stdio.h>
>>
>> extern void *x;
>>
>> int main() {
>>        int a;
>>        asm volatile ("x: fldz\n\
>>        push %%edx\n\
>>        fnstenv -0xc(%%esp)\n\
>>        pop %%edx\n" : "=d" (a) : : "memory");
>>        printf ("%x %x\n", a, &x);
>>        return 0;
>> }
>> $ gcc -m32 test.c -o test
>> $ ./test
>> 80483ae 80483ae
>> $ ./qemu-build/i386-linux-user/qemu-i386 ./test
>> 0 80483ae
>> $ ./qemu-fixed-build/i386-linux-user/qemu-i386 ./test
>> 80483ae 80483ae
>>
>> The patch:
>>
>> Signed-off-by: Nikita A Skovoroda <chalkerx@gmail.com>
>>
>> From e07b99b12a9dd4d933936d5376efde8a992472dd Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?=D0=A1=D0=BA=D0=BE=D0=B2=D0=BE=D1=80=D0=BE=D0=B4=D0=B0=20=D0=9D=D0=B8=D0=BA=D0=B8=D1=82=D0=B0=20=D0=90=D0=BD=D0=B4=D1=80=D0=B5=D0=B5=D0=B2=D0=B8=D1=87?=
>> <chalkerx@gmail.com>
>> Date: Fri, 26 Nov 2010 15:35:05 +0300
>> Subject: [PATCH] Fix FSTENV (and FSAVE) instructions in target-i386.
>> Fixes bug #616696.
>
> This is a bit terse, the description should preferably state the
> problem (for extra coolness, in past tense) and then how the patch
> fixes it. For example:
>
> IP field stored by FSTENV was zero instead of IP at last FPU instruction.
>
> Store the IP value to env and use that in FSTENV helper to store the correct IP.
>
>>
>> ---
>>  target-i386/cpu.h       |    1 +
>>  target-i386/helper.h    |    2 ++
>>  target-i386/op_helper.c |    9 +++++++--
>>  target-i386/translate.c |    1 +
>>  4 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index 06e40f3..aad0dcb 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -642,6 +642,7 @@ typedef struct CPUX86State {
>>     uint16_t fpuc;
>>     uint8_t fptags[8];   /* 0 = valid, 1 = empty */
>>     FPReg fpregs[8];
>> +    target_ulong fpip;
>
> Also CS, the instruction and data pointer should be saved for completeness.
>
>>
>>     /* emulator internal variables */
>>     float_status fp_status;
>> diff --git a/target-i386/helper.h b/target-i386/helper.h
>> index 6b518ad..26b47a3 100644
>> --- a/target-i386/helper.h
>> +++ b/target-i386/helper.h
>> @@ -3,6 +3,8 @@
>>  DEF_HELPER_FLAGS_1(cc_compute_all, TCG_CALL_PURE, i32, int)
>>  DEF_HELPER_FLAGS_1(cc_compute_c, TCG_CALL_PURE, i32, int)
>>
>> +DEF_HELPER_1(save_fpip, void, tl)
>> +
>>  DEF_HELPER_0(lock, void)
>>  DEF_HELPER_0(unlock, void)
>>  DEF_HELPER_2(write_eflags, void, tl, i32)
>> diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
>> index 43fbd0c..ab65f0c 100644
>> --- a/target-i386/op_helper.c
>> +++ b/target-i386/op_helper.c
>> @@ -109,6 +109,11 @@ static const CPU86_LDouble f15rk[7] =
>>
>>  static spinlock_t global_cpu_lock = SPIN_LOCK_UNLOCKED;
>>
>> +void helper_save_fpip(target_ulong fpip)
>> +{
>> +    env->fpip = fpip;
>> +}
>> +
>>  void helper_lock(void)
>>  {
>>     spin_lock(&global_cpu_lock);
>> @@ -4273,7 +4278,7 @@ void helper_fstenv(target_ulong ptr, int data32)
>>         stl(ptr, env->fpuc);
>>         stl(ptr + 4, fpus);
>>         stl(ptr + 8, fptag);
>> -        stl(ptr + 12, 0); /* fpip */
>> +        stl(ptr + 12, env->fpip); /* fpip */
>>         stl(ptr + 16, 0); /* fpcs */
>>         stl(ptr + 20, 0); /* fpoo */
>>         stl(ptr + 24, 0); /* fpos */
>> @@ -4282,7 +4287,7 @@ void helper_fstenv(target_ulong ptr, int data32)
>>         stw(ptr, env->fpuc);
>>         stw(ptr + 2, fpus);
>>         stw(ptr + 4, fptag);
>> -        stw(ptr + 6, 0);
>> +        stw(ptr + 6, env->fpip);
>
> Please also consider fixing FSAVE and FXSAVE. Alternatively, a comment
> with XXX to highlight the unimplemented features would be nice.
>
>>         stw(ptr + 8, 0);
>>         stw(ptr + 10, 0);
>>         stw(ptr + 12, 0);
>> diff --git a/target-i386/translate.c b/target-i386/translate.c
>> index 7b6e3c2..c7d1d33 100644
>> --- a/target-i386/translate.c
>> +++ b/target-i386/translate.c
>> @@ -5974,6 +5974,7 @@ static target_ulong disas_insn(DisasContext *s,
>> target_ulong pc_start)
>>                 goto illegal_op;
>>             }
>>         }
>> +        gen_helper_save_fpip(tcg_const_tl(pc_start - s->cs_base));
>
> You can simply use tcg_gen_movi_tl() and tcg_gen_st_tl(), no need for
> a helper except for the data pointer.
>
> In this place, the data would be saved for every instruction. But I
> think only FPU instructions should update the fields.
>
Blue Swirl - Nov. 27, 2010, 10:01 a.m.
On Fri, Nov 26, 2010 at 10:53 PM, ChALkeR <chalkerx@gmail.com> wrote:
>> Please also consider fixing FSAVE and FXSAVE.
>
> FSAVE also works, i checked twice (code and test).
> FSAVE in QEMU calls FSTENV.
>
> op_helper.c:
>> void helper_fsave(target_ulong ptr, int data32)
>> {
>>    CPU86_LDouble tmp;
>>    int i;
>>
>>    helper_fstenv(ptr, data32);

Sorry, I missed that.

> Not sure how FXSAVE operates, that has to be checked in the manuals.
> But the patch does not break anything and fixes FSTENV and FSAVE instructions.

Yes, I just wish for more fixes. I think FRSTOR/FXRSTOR also need to
update env->fpip etc.

>> In this place, the data would be saved for every instruction. But I
>> think only FPU instructions should update the fields.
>
> I do not understand. The patch stores eip only for FPU instructions.
> This is correct.
> FSTENV should put the eip of the last FPU instruction in the stack.
> The helper is called at the end of every instruction on floating-point
> case of the switch.
> There should be some additional data, too, but the correct operation
> of fpip is provided by the patch.
>
> translate.c, line 2461:
>>        /************************/
>>        /* floats */
>>    case 0xd8 ... 0xdf:

I thought the store happened at the end of disas_insn(). Then this is OK.

>> You can simply use tcg_gen_movi_tl() and tcg_gen_st_tl(), no need for
>> a helper except for the data pointer.
>
> Can you fix it, please, if I did something wrong?

It's not wrong but just less than optimal, there's some overhead with
helper calls. Something like:
tcg_gen_movi_tl(cpu_tmp0, pc_start - s->cs_base);
tcg_gen_st_tl(cpu_tmp0, cpu_env, offsetof(CPUState, fpip));

Likewise for CS and instruction. Actually the data pointer also seems
to be in A0, so we can also store that:
tcg_gen_st_tl(cpu_A0, cpu_env, offsetof(CPUState, fpdp));

> Sorry for my bad English.

No problem, I suppose most people on this list are not native English
speakers anyway.

Patch

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 06e40f3..aad0dcb 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -642,6 +642,7 @@  typedef struct CPUX86State {
     uint16_t fpuc;
     uint8_t fptags[8];   /* 0 = valid, 1 = empty */
     FPReg fpregs[8];
+    target_ulong fpip;

     /* emulator internal variables */
     float_status fp_status;
diff --git a/target-i386/helper.h b/target-i386/helper.h
index 6b518ad..26b47a3 100644
--- a/target-i386/helper.h
+++ b/target-i386/helper.h
@@ -3,6 +3,8 @@ 
 DEF_HELPER_FLAGS_1(cc_compute_all, TCG_CALL_PURE, i32, int)
 DEF_HELPER_FLAGS_1(cc_compute_c, TCG_CALL_PURE, i32, int)

+DEF_HELPER_1(save_fpip, void, tl)
+
 DEF_HELPER_0(lock, void)
 DEF_HELPER_0(unlock, void)
 DEF_HELPER_2(write_eflags, void, tl, i32)
diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
index 43fbd0c..ab65f0c 100644
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -109,6 +109,11 @@  static const CPU86_LDouble f15rk[7] =

 static spinlock_t global_cpu_lock = SPIN_LOCK_UNLOCKED;

+void helper_save_fpip(target_ulong fpip)
+{
+    env->fpip = fpip;
+}
+
 void helper_lock(void)
 {
     spin_lock(&global_cpu_lock);
@@ -4273,7 +4278,7 @@  void helper_fstenv(target_ulong ptr, int data32)
         stl(ptr, env->fpuc);
         stl(ptr + 4, fpus);
         stl(ptr + 8, fptag);
-        stl(ptr + 12, 0); /* fpip */
+        stl(ptr + 12, env->fpip); /* fpip */
         stl(ptr + 16, 0); /* fpcs */
         stl(ptr + 20, 0); /* fpoo */
         stl(ptr + 24, 0); /* fpos */
@@ -4282,7 +4287,7 @@  void helper_fstenv(target_ulong ptr, int data32)
         stw(ptr, env->fpuc);
         stw(ptr + 2, fpus);
         stw(ptr + 4, fptag);
-        stw(ptr + 6, 0);
+        stw(ptr + 6, env->fpip);
         stw(ptr + 8, 0);
         stw(ptr + 10, 0);
         stw(ptr + 12, 0);
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 7b6e3c2..c7d1d33 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -5974,6 +5974,7 @@  static target_ulong disas_insn(DisasContext *s,
target_ulong pc_start)
                 goto illegal_op;
             }
         }
+        gen_helper_save_fpip(tcg_const_tl(pc_start - s->cs_base));
         break;
         /************************/