qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] Patch: fix FSTENV (and FSAVE) instructions in target-i3


From: ChALkeR
Subject: Re: [Qemu-devel] Patch: fix FSTENV (and FSAVE) instructions in target-i386 (resend, cleaned up).
Date: Sat, 27 Nov 2010 01:53:04 +0300

> 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 <address@hidden>:
> On Fri, Nov 26, 2010 at 1:14 PM, ChALkeR <address@hidden> 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 <address@hidden>
>>
>> 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?=
>> <address@hidden>
>> 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.
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]