Re: [Qemu-devel] Patch: fix FSTENV (and FSAVE) instructions in target-i386 (resend, cleaned up).

[Blue Swirl]

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.