[Top][All Lists]
[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: |
Blue Swirl |
Subject: |
Re: [Qemu-devel] Patch: fix FSTENV (and FSAVE) instructions in target-i386 (resend, cleaned up). |
Date: |
Fri, 26 Nov 2010 19:30:12 +0000 |
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.