| Message ID | 20140915111740.2776.38029.stgit@PASHA-ISP |
|---|---|
| State | New |
| Headers | show |
Il 15/09/2014 13:17, Pavel Dovgalyuk ha scritto: > This patch adds calls to update_fp_status() function from several > places where FPU state is changed. These calls update status of > softfloat library and prevent bugs caused by non-coherent rounding > settings of the FPU and softfloat. > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > --- > target-i386/cpu.c | 1 + > target-i386/cpu.h | 1 + > target-i386/fpu_helper.c | 5 ++++- > target-i386/machine.c | 2 +- > 4 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 217500c..0ebedb0 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2573,6 +2573,7 @@ static void x86_cpu_reset(CPUState *s) > env->fptags[i] = 1; > } > env->fpuc = 0x37f; > + update_fp_status(env); > > env->mxcsr = 0x1f80; > env->xstate_bv = XSTATE_FP | XSTATE_SSE; > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index e634d83..42bda46 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -1249,6 +1249,7 @@ void QEMU_NORETURN raise_interrupt(CPUX86State *nenv, int intno, int is_int, > /* cc_helper.c */ > extern const uint8_t parity_table[256]; > uint32_t cpu_cc_compute_all(CPUX86State *env1, int op); > +void update_fp_status(CPUX86State *env); > > static inline uint32_t cpu_compute_eflags(CPUX86State *env) > { > diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c > index 1b2900d..a8ffba9 100644 > --- a/target-i386/fpu_helper.c > +++ b/target-i386/fpu_helper.c > @@ -537,7 +537,7 @@ uint32_t helper_fnstcw(CPUX86State *env) > return env->fpuc; > } > > -static void update_fp_status(CPUX86State *env) > +void update_fp_status(CPUX86State *env) > { > int rnd_type; > > @@ -1006,6 +1006,7 @@ void helper_fstenv(CPUX86State *env, target_ulong ptr, int data32) > cpu_stw_data(env, ptr + 10, 0); > cpu_stw_data(env, ptr + 12, 0); > } > + update_fp_status(env); > } > > void helper_fldenv(CPUX86State *env, target_ulong ptr, int data32) > @@ -1055,6 +1056,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr, int data32) > env->fptags[5] = 1; > env->fptags[6] = 1; > env->fptags[7] = 1; > + update_fp_status(env); > } > > void helper_frstor(CPUX86State *env, target_ulong ptr, int data32) > @@ -1158,6 +1160,7 @@ void helper_fxrstor(CPUX86State *env, target_ulong ptr, int data64) > } > > env->fpuc = cpu_lduw_data(env, ptr); > + update_fp_status(env); > fpus = cpu_lduw_data(env, ptr + 2); > fptag = cpu_lduw_data(env, ptr + 4); > env->fpstt = (fpus >> 11) & 7; > diff --git a/target-i386/machine.c b/target-i386/machine.c > index 16d2f6a..2922c97 100644 > --- a/target-i386/machine.c > +++ b/target-i386/machine.c > @@ -315,13 +315,13 @@ static int cpu_post_load(void *opaque, int version_id) > env->hflags &= ~HF_CPL_MASK; > env->hflags |= (env->segs[R_SS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK; > > - /* XXX: restore FPU round state */ > env->fpstt = (env->fpus_vmstate >> 11) & 7; > env->fpus = env->fpus_vmstate & ~0x3800; > env->fptag_vmstate ^= 0xff; > for(i = 0; i < 8; i++) { > env->fptags[i] = (env->fptag_vmstate >> i) & 1; > } > + update_fp_status(env); > > cpu_breakpoint_remove_all(cs, BP_CPU); > cpu_watchpoint_remove_all(cs, BP_CPU); > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
On 09/15/2014 04:17 AM, Pavel Dovgalyuk wrote: > @@ -1006,6 +1006,7 @@ void helper_fstenv(CPUX86State *env, target_ulong ptr, int data32) > cpu_stw_data(env, ptr + 10, 0); > cpu_stw_data(env, ptr + 12, 0); > } > + update_fp_status(env); > } Why does this need to update status? All the reset looks good. r~
update_fp_status() should be used after all of env->fpuc changes : helper_fldcw(), helper_fninit(), helper_fldenv() ,helper_fxrstor(), helper_fsave(), x86_cpu_reset(), cpu_post_load(). On Mon, Sep 15, 2014 at 7:17 PM, Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> wrote: > This patch adds calls to update_fp_status() function from several > places where FPU state is changed. These calls update status of > softfloat library and prevent bugs caused by non-coherent rounding > settings of the FPU and softfloat. > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > --- > target-i386/cpu.c | 1 + > target-i386/cpu.h | 1 + > target-i386/fpu_helper.c | 5 ++++- > target-i386/machine.c | 2 +- > 4 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 217500c..0ebedb0 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2573,6 +2573,7 @@ static void x86_cpu_reset(CPUState *s) > env->fptags[i] = 1; > } > env->fpuc = 0x37f; > + update_fp_status(env); > > env->mxcsr = 0x1f80; > env->xstate_bv = XSTATE_FP | XSTATE_SSE; > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index e634d83..42bda46 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -1249,6 +1249,7 @@ void QEMU_NORETURN raise_interrupt(CPUX86State *nenv, int intno, int is_int, > /* cc_helper.c */ > extern const uint8_t parity_table[256]; > uint32_t cpu_cc_compute_all(CPUX86State *env1, int op); > +void update_fp_status(CPUX86State *env); > > static inline uint32_t cpu_compute_eflags(CPUX86State *env) > { > diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c > index 1b2900d..a8ffba9 100644 > --- a/target-i386/fpu_helper.c > +++ b/target-i386/fpu_helper.c > @@ -537,7 +537,7 @@ uint32_t helper_fnstcw(CPUX86State *env) > return env->fpuc; > } > > -static void update_fp_status(CPUX86State *env) > +void update_fp_status(CPUX86State *env) > { > int rnd_type; > > @@ -1006,6 +1006,7 @@ void helper_fstenv(CPUX86State *env, target_ulong ptr, int data32) > cpu_stw_data(env, ptr + 10, 0); > cpu_stw_data(env, ptr + 12, 0); > } > + update_fp_status(env); > } > > void helper_fldenv(CPUX86State *env, target_ulong ptr, int data32) > @@ -1055,6 +1056,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr, int data32) > env->fptags[5] = 1; > env->fptags[6] = 1; > env->fptags[7] = 1; > + update_fp_status(env); > } > > void helper_frstor(CPUX86State *env, target_ulong ptr, int data32) > @@ -1158,6 +1160,7 @@ void helper_fxrstor(CPUX86State *env, target_ulong ptr, int data64) > } > > env->fpuc = cpu_lduw_data(env, ptr); > + update_fp_status(env); > fpus = cpu_lduw_data(env, ptr + 2); > fptag = cpu_lduw_data(env, ptr + 4); > env->fpstt = (fpus >> 11) & 7; > diff --git a/target-i386/machine.c b/target-i386/machine.c > index 16d2f6a..2922c97 100644 > --- a/target-i386/machine.c > +++ b/target-i386/machine.c > @@ -315,13 +315,13 @@ static int cpu_post_load(void *opaque, int version_id) > env->hflags &= ~HF_CPL_MASK; > env->hflags |= (env->segs[R_SS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK; > > - /* XXX: restore FPU round state */ > env->fpstt = (env->fpus_vmstate >> 11) & 7; > env->fpus = env->fpus_vmstate & ~0x3800; > env->fptag_vmstate ^= 0xff; > for(i = 0; i < 8; i++) { > env->fptags[i] = (env->fptag_vmstate >> i) & 1; > } > + update_fp_status(env); > > cpu_breakpoint_remove_all(cs, BP_CPU); > cpu_watchpoint_remove_all(cs, BP_CPU); > >
On Tue, Sep 16, 2014 at 1:42 PM, TeLeMan <geleman@gmail.com> wrote: > update_fp_status() should be used after all of env->fpuc changes : > helper_fldcw(), helper_fninit(), helper_fldenv() > ,helper_fxrstor(), helper_fsave(), x86_cpu_reset(), cpu_post_load(). missing one: x86_cpu_gdb_write_register(). > > On Mon, Sep 15, 2014 at 7:17 PM, Pavel Dovgalyuk > <Pavel.Dovgaluk@ispras.ru> wrote: >> This patch adds calls to update_fp_status() function from several >> places where FPU state is changed. These calls update status of >> softfloat library and prevent bugs caused by non-coherent rounding >> settings of the FPU and softfloat. >> >> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> >> --- >> target-i386/cpu.c | 1 + >> target-i386/cpu.h | 1 + >> target-i386/fpu_helper.c | 5 ++++- >> target-i386/machine.c | 2 +- >> 4 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> index 217500c..0ebedb0 100644 >> --- a/target-i386/cpu.c >> +++ b/target-i386/cpu.c >> @@ -2573,6 +2573,7 @@ static void x86_cpu_reset(CPUState *s) >> env->fptags[i] = 1; >> } >> env->fpuc = 0x37f; >> + update_fp_status(env); >> >> env->mxcsr = 0x1f80; >> env->xstate_bv = XSTATE_FP | XSTATE_SSE; >> diff --git a/target-i386/cpu.h b/target-i386/cpu.h >> index e634d83..42bda46 100644 >> --- a/target-i386/cpu.h >> +++ b/target-i386/cpu.h >> @@ -1249,6 +1249,7 @@ void QEMU_NORETURN raise_interrupt(CPUX86State *nenv, int intno, int is_int, >> /* cc_helper.c */ >> extern const uint8_t parity_table[256]; >> uint32_t cpu_cc_compute_all(CPUX86State *env1, int op); >> +void update_fp_status(CPUX86State *env); >> >> static inline uint32_t cpu_compute_eflags(CPUX86State *env) >> { >> diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c >> index 1b2900d..a8ffba9 100644 >> --- a/target-i386/fpu_helper.c >> +++ b/target-i386/fpu_helper.c >> @@ -537,7 +537,7 @@ uint32_t helper_fnstcw(CPUX86State *env) >> return env->fpuc; >> } >> >> -static void update_fp_status(CPUX86State *env) >> +void update_fp_status(CPUX86State *env) >> { >> int rnd_type; >> >> @@ -1006,6 +1006,7 @@ void helper_fstenv(CPUX86State *env, target_ulong ptr, int data32) >> cpu_stw_data(env, ptr + 10, 0); >> cpu_stw_data(env, ptr + 12, 0); >> } >> + update_fp_status(env); >> } >> >> void helper_fldenv(CPUX86State *env, target_ulong ptr, int data32) >> @@ -1055,6 +1056,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr, int data32) >> env->fptags[5] = 1; >> env->fptags[6] = 1; >> env->fptags[7] = 1; >> + update_fp_status(env); >> } >> >> void helper_frstor(CPUX86State *env, target_ulong ptr, int data32) >> @@ -1158,6 +1160,7 @@ void helper_fxrstor(CPUX86State *env, target_ulong ptr, int data64) >> } >> >> env->fpuc = cpu_lduw_data(env, ptr); >> + update_fp_status(env); >> fpus = cpu_lduw_data(env, ptr + 2); >> fptag = cpu_lduw_data(env, ptr + 4); >> env->fpstt = (fpus >> 11) & 7; >> diff --git a/target-i386/machine.c b/target-i386/machine.c >> index 16d2f6a..2922c97 100644 >> --- a/target-i386/machine.c >> +++ b/target-i386/machine.c >> @@ -315,13 +315,13 @@ static int cpu_post_load(void *opaque, int version_id) >> env->hflags &= ~HF_CPL_MASK; >> env->hflags |= (env->segs[R_SS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK; >> >> - /* XXX: restore FPU round state */ >> env->fpstt = (env->fpus_vmstate >> 11) & 7; >> env->fpus = env->fpus_vmstate & ~0x3800; >> env->fptag_vmstate ^= 0xff; >> for(i = 0; i < 8; i++) { >> env->fptags[i] = (env->fptag_vmstate >> i) & 1; >> } >> + update_fp_status(env); >> >> cpu_breakpoint_remove_all(cs, BP_CPU); >> cpu_watchpoint_remove_all(cs, BP_CPU); >> >>
I suggest using cpu_set_fpuc() like cpu_set_mxcsr(). On Tue, Sep 16, 2014 at 1:51 PM, TeLeMan <geleman@gmail.com> wrote: > On Tue, Sep 16, 2014 at 1:42 PM, TeLeMan <geleman@gmail.com> wrote: >> update_fp_status() should be used after all of env->fpuc changes : >> helper_fldcw(), helper_fninit(), helper_fldenv() >> ,helper_fxrstor(), helper_fsave(), x86_cpu_reset(), cpu_post_load(). > > missing one: x86_cpu_gdb_write_register(). > >> >> On Mon, Sep 15, 2014 at 7:17 PM, Pavel Dovgalyuk >> <Pavel.Dovgaluk@ispras.ru> wrote: >>> This patch adds calls to update_fp_status() function from several >>> places where FPU state is changed. These calls update status of >>> softfloat library and prevent bugs caused by non-coherent rounding >>> settings of the FPU and softfloat. >>> >>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> >>> --- >>> target-i386/cpu.c | 1 + >>> target-i386/cpu.h | 1 + >>> target-i386/fpu_helper.c | 5 ++++- >>> target-i386/machine.c | 2 +- >>> 4 files changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >>> index 217500c..0ebedb0 100644 >>> --- a/target-i386/cpu.c >>> +++ b/target-i386/cpu.c >>> @@ -2573,6 +2573,7 @@ static void x86_cpu_reset(CPUState *s) >>> env->fptags[i] = 1; >>> } >>> env->fpuc = 0x37f; >>> + update_fp_status(env); >>> >>> env->mxcsr = 0x1f80; >>> env->xstate_bv = XSTATE_FP | XSTATE_SSE; >>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h >>> index e634d83..42bda46 100644 >>> --- a/target-i386/cpu.h >>> +++ b/target-i386/cpu.h >>> @@ -1249,6 +1249,7 @@ void QEMU_NORETURN raise_interrupt(CPUX86State *nenv, int intno, int is_int, >>> /* cc_helper.c */ >>> extern const uint8_t parity_table[256]; >>> uint32_t cpu_cc_compute_all(CPUX86State *env1, int op); >>> +void update_fp_status(CPUX86State *env); >>> >>> static inline uint32_t cpu_compute_eflags(CPUX86State *env) >>> { >>> diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c >>> index 1b2900d..a8ffba9 100644 >>> --- a/target-i386/fpu_helper.c >>> +++ b/target-i386/fpu_helper.c >>> @@ -537,7 +537,7 @@ uint32_t helper_fnstcw(CPUX86State *env) >>> return env->fpuc; >>> } >>> >>> -static void update_fp_status(CPUX86State *env) >>> +void update_fp_status(CPUX86State *env) >>> { >>> int rnd_type; >>> >>> @@ -1006,6 +1006,7 @@ void helper_fstenv(CPUX86State *env, target_ulong ptr, int data32) >>> cpu_stw_data(env, ptr + 10, 0); >>> cpu_stw_data(env, ptr + 12, 0); >>> } >>> + update_fp_status(env); >>> } >>> >>> void helper_fldenv(CPUX86State *env, target_ulong ptr, int data32) >>> @@ -1055,6 +1056,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr, int data32) >>> env->fptags[5] = 1; >>> env->fptags[6] = 1; >>> env->fptags[7] = 1; >>> + update_fp_status(env); >>> } >>> >>> void helper_frstor(CPUX86State *env, target_ulong ptr, int data32) >>> @@ -1158,6 +1160,7 @@ void helper_fxrstor(CPUX86State *env, target_ulong ptr, int data64) >>> } >>> >>> env->fpuc = cpu_lduw_data(env, ptr); >>> + update_fp_status(env); >>> fpus = cpu_lduw_data(env, ptr + 2); >>> fptag = cpu_lduw_data(env, ptr + 4); >>> env->fpstt = (fpus >> 11) & 7; >>> diff --git a/target-i386/machine.c b/target-i386/machine.c >>> index 16d2f6a..2922c97 100644 >>> --- a/target-i386/machine.c >>> +++ b/target-i386/machine.c >>> @@ -315,13 +315,13 @@ static int cpu_post_load(void *opaque, int version_id) >>> env->hflags &= ~HF_CPL_MASK; >>> env->hflags |= (env->segs[R_SS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK; >>> >>> - /* XXX: restore FPU round state */ >>> env->fpstt = (env->fpus_vmstate >> 11) & 7; >>> env->fpus = env->fpus_vmstate & ~0x3800; >>> env->fptag_vmstate ^= 0xff; >>> for(i = 0; i < 8; i++) { >>> env->fptags[i] = (env->fptag_vmstate >> i) & 1; >>> } >>> + update_fp_status(env); >>> >>> cpu_breakpoint_remove_all(cs, BP_CPU); >>> cpu_watchpoint_remove_all(cs, BP_CPU); >>> >>>
Il 15/09/2014 22:01, Richard Henderson ha scritto: >> > @@ -1006,6 +1006,7 @@ void helper_fstenv(CPUX86State *env, target_ulong ptr, int data32) >> > cpu_stw_data(env, ptr + 10, 0); >> > cpu_stw_data(env, ptr + 12, 0); >> > } >> > + update_fp_status(env); >> > } > Why does this need to update status? Ah, this is fstenv (I learnt about it today and misread it as frstenv...). The update_fd_status is needed in fldenv, and is missing from this patch. TeLeMan's suggestion is also good. Paolo
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 217500c..0ebedb0 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2573,6 +2573,7 @@ static void x86_cpu_reset(CPUState *s) env->fptags[i] = 1; } env->fpuc = 0x37f; + update_fp_status(env); env->mxcsr = 0x1f80; env->xstate_bv = XSTATE_FP | XSTATE_SSE; diff --git a/target-i386/cpu.h b/target-i386/cpu.h index e634d83..42bda46 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -1249,6 +1249,7 @@ void QEMU_NORETURN raise_interrupt(CPUX86State *nenv, int intno, int is_int, /* cc_helper.c */ extern const uint8_t parity_table[256]; uint32_t cpu_cc_compute_all(CPUX86State *env1, int op); +void update_fp_status(CPUX86State *env); static inline uint32_t cpu_compute_eflags(CPUX86State *env) { diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c index 1b2900d..a8ffba9 100644 --- a/target-i386/fpu_helper.c +++ b/target-i386/fpu_helper.c @@ -537,7 +537,7 @@ uint32_t helper_fnstcw(CPUX86State *env) return env->fpuc; } -static void update_fp_status(CPUX86State *env) +void update_fp_status(CPUX86State *env) { int rnd_type; @@ -1006,6 +1006,7 @@ void helper_fstenv(CPUX86State *env, target_ulong ptr, int data32) cpu_stw_data(env, ptr + 10, 0); cpu_stw_data(env, ptr + 12, 0); } + update_fp_status(env); } void helper_fldenv(CPUX86State *env, target_ulong ptr, int data32) @@ -1055,6 +1056,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr, int data32) env->fptags[5] = 1; env->fptags[6] = 1; env->fptags[7] = 1; + update_fp_status(env); } void helper_frstor(CPUX86State *env, target_ulong ptr, int data32) @@ -1158,6 +1160,7 @@ void helper_fxrstor(CPUX86State *env, target_ulong ptr, int data64) } env->fpuc = cpu_lduw_data(env, ptr); + update_fp_status(env); fpus = cpu_lduw_data(env, ptr + 2); fptag = cpu_lduw_data(env, ptr + 4); env->fpstt = (fpus >> 11) & 7; diff --git a/target-i386/machine.c b/target-i386/machine.c index 16d2f6a..2922c97 100644 --- a/target-i386/machine.c +++ b/target-i386/machine.c @@ -315,13 +315,13 @@ static int cpu_post_load(void *opaque, int version_id) env->hflags &= ~HF_CPL_MASK; env->hflags |= (env->segs[R_SS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK; - /* XXX: restore FPU round state */ env->fpstt = (env->fpus_vmstate >> 11) & 7; env->fpus = env->fpus_vmstate & ~0x3800; env->fptag_vmstate ^= 0xff; for(i = 0; i < 8; i++) { env->fptags[i] = (env->fptag_vmstate >> i) & 1; } + update_fp_status(env); cpu_breakpoint_remove_all(cs, BP_CPU); cpu_watchpoint_remove_all(cs, BP_CPU);
This patch adds calls to update_fp_status() function from several places where FPU state is changed. These calls update status of softfloat library and prevent bugs caused by non-coherent rounding settings of the FPU and softfloat. Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> --- target-i386/cpu.c | 1 + target-i386/cpu.h | 1 + target-i386/fpu_helper.c | 5 ++++- target-i386/machine.c | 2 +- 4 files changed, 7 insertions(+), 2 deletions(-)