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