Message ID | 20200221034547.5215-1-chengang@emindsoft.com.cn |
---|---|
State | New |
Headers | show |
Series | target: i386: Check float overflow about register stack | expand |
On 21/02/20 04:45, chengang@emindsoft.com.cn wrote: > static inline void fpush(CPUX86State *env) > { > - env->fpstt = (env->fpstt - 1) & 7; > - env->fptags[env->fpstt] = 0; /* validate stack entry */ > + set_fpstt(env, env->fpstt - 1, false, true); On overflow fpstt is ~0, so this does: env->foverflow = true; env->fpstt = 7; env->fptags[7] = 0; /* validate stack entry */ Is this correct? You are going to set ST0 so the register should not be marked empty. > static inline void fpop(CPUX86State *env) > { > - env->fptags[env->fpstt] = 1; /* invalidate stack entry */ > - env->fpstt = (env->fpstt + 1) & 7; > + set_fpstt(env, env->fpstt + 1, true, true); While here: env->foverflow = true; env->fptags[7] = 1; env->fpstt = 0; > void helper_fdecstp(CPUX86State *env) > { > - env->fpstt = (env->fpstt - 1) & 7; > + set_fpstt(env, env->fpstt - 1, false, false); This is clearing env->foverflow. But after 8 consecutive fdecstp or fincstp the result of FXAM should not change. > env->fpus &= ~0x4700; > } > > void helper_fincstp(CPUX86State *env) > { > - env->fpstt = (env->fpstt + 1) & 7; > + set_fpstt(env, env->fpstt + 1, true, false); Same here. The actual bug is hinted in helper_fxam_ST0: /* XXX: test fptags too */ I think the correct fix should be something like diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c index 99f28f267f..792a128a6d 100644 --- a/target/i386/fpu_helper.c +++ b/target/i386/fpu_helper.c @@ -991,7 +991,11 @@ void helper_fxam_ST0(CPUX86State *env) env->fpus |= 0x200; /* C1 <-- 1 */ } - /* XXX: test fptags too */ + if (env->fptags[env->fpstt]) { + env->fpus |= 0x4100; /* Empty */ + return; + } + expdif = EXPD(temp); if (expdif == MAXEXPD) { if (MANTD(temp) == 0x8000000000000000ULL) { Paolo
On 2020/2/21 下午4:58, Paolo Bonzini wrote: > On 21/02/20 04:45, chengang@emindsoft.com.cn wrote: >> static inline void fpush(CPUX86State *env) >> { >> - env->fpstt = (env->fpstt - 1) & 7; >> - env->fptags[env->fpstt] = 0; /* validate stack entry */ >> + set_fpstt(env, env->fpstt - 1, false, true); > > On overflow fpstt is ~0, so this does: > > env->foverflow = true; > env->fpstt = 7; > env->fptags[7] = 0; /* validate stack entry */ > > Is this correct? You are going to set ST0 so the register should not be > marked empty. > Originally, I wanted to add foverflow to mark the stack overflow only, and kept another things no touch. But I think what you said above is correct, for me, if fpush/f[i]ld*_STO are overflow, the env->fpstt, env->fpregs and env->fptags should be kept no touch, and foverflow is set true, so there is no negative effect. Welcome your idea. >> void helper_fdecstp(CPUX86State *env) >> { >> - env->fpstt = (env->fpstt - 1) & 7; >> + set_fpstt(env, env->fpstt - 1, false, false); > > This is clearing env->foverflow. But after 8 consecutive fdecstp or > fincstp the result of FXAM should not change. > >> env->fpus &= ~0x4700; >> } >> >> void helper_fincstp(CPUX86State *env) >> { >> - env->fpstt = (env->fpstt + 1) & 7; >> + set_fpstt(env, env->fpstt + 1, true, false); > > Same here. > OK. thanks. Now if foverflow is only for fpush/f[i]ld*_ST0, I guess fincstp/fdecstp can clear foverflow. The env->fptags are only for fpop, which keep no touch in fincstp/fdecstp. > The actual bug is hinted in helper_fxam_ST0: > > /* XXX: test fptags too */ > > I think the correct fix should be something like > > diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c > index 99f28f267f..792a128a6d 100644 > --- a/target/i386/fpu_helper.c > +++ b/target/i386/fpu_helper.c > @@ -991,7 +991,11 @@ void helper_fxam_ST0(CPUX86State *env) > env->fpus |= 0x200; /* C1 <-- 1 */ > } > > - /* XXX: test fptags too */ > + if (env->fptags[env->fpstt]) { > + env->fpus |= 0x4100; /* Empty */ > + return; > + } > + For fpop overflow, this fix is enough, but for me, we still need foverflow to check fpush/fld*_ST0 overflow. Don't you think we need check fpush/f[i]ld*_ST0 overflow? Thanks
On 21/02/20 15:09, Chen Gang wrote: >> - /* XXX: test fptags too */ >> + if (env->fptags[env->fpstt]) { >> + env->fpus |= 0x4100; /* Empty */ >> + return; >> + } >> + > For fpop overflow, this fix is enough, but for me, we still need > foverflow to check fpush/fld*_ST0 overflow. > > Don't you think we need check fpush/f[i]ld*_ST0 overflow? After fld/fild or any other push, FXAM ST0 should not return empty in my opinion. Paolo
On 2020/2/22 上午12:18, Paolo Bonzini wrote: > On 21/02/20 15:09, Chen Gang wrote: >>> - /* XXX: test fptags too */ >>> + if (env->fptags[env->fpstt]) { >>> + env->fpus |= 0x4100; /* Empty */ >>> + return; >>> + } >>> + >> For fpop overflow, this fix is enough, but for me, we still need >> foverflow to check fpush/fld*_ST0 overflow. >> >> Don't you think we need check fpush/f[i]ld*_ST0 overflow? > > After fld/fild or any other push, FXAM ST0 should not return empty in my > opinion. > OK, it sounds reasonable. After check the intel document for f[i]ld* instructions, it says: "Set C1 to 1 if stack overflow occurred; set to 0 otherwise". In helper_fxam_ST0, I guess, we need "env->fpus |= 0x200" (but I don't know wheter it will be conflict with SIGND(temp)). And we have to still need foverflow, because all env->fptags being 0 doesn't mean overflow. Thanks.
On 2020/2/22 上午10:10, Chen Gang wrote: > On 2020/2/22 上午12:18, Paolo Bonzini wrote: >> On 21/02/20 15:09, Chen Gang wrote: >>>> - /* XXX: test fptags too */ >>>> + if (env->fptags[env->fpstt]) { >>>> + env->fpus |= 0x4100; /* Empty */ >>>> + return; >>>> + } >>>> + >>> For fpop overflow, this fix is enough, but for me, we still need >>> foverflow to check fpush/fld*_ST0 overflow. >>> >>> Don't you think we need check fpush/f[i]ld*_ST0 overflow? >> >> After fld/fild or any other push, FXAM ST0 should not return empty in my >> opinion. >> > > OK, it sounds reasonable. > > After check the intel document for f[i]ld* instructions, it says: > > "Set C1 to 1 if stack overflow occurred; set to 0 otherwise". > > In helper_fxam_ST0, I guess, we need "env->fpus |= 0x200" (but I don't > know wheter it will be conflict with SIGND(temp)). And we have to still > need foverflow, because all env->fptags being 0 doesn't mean overflow. > After read the document more about fstp* and fld* instructions, I find that fstp* are also related with C1, and fld* will generate IS exception when stack underflow or overflow occurred. It seems more modifications should be done (but they will be several patches). Thanks.
On 22/02/20 03:10, Chen Gang wrote: > Set C1 to 1 if stack overflow occurred; set to 0 otherwise". > > In helper_fxam_ST0, I guess, we need "env->fpus |= 0x200" (but I don't > know wheter it will be conflict with SIGND(temp)). And we have to still > need foverflow, because all env->fptags being 0 doesn't mean overflow. No, you need to add "env->fpus |= 0x200" and "env->fpus &= ~0x200" directly to fpush, fpop, etc. Polo
On 2020/2/22 下午3:37, Paolo Bonzini wrote: > On 22/02/20 03:10, Chen Gang wrote: >> Set C1 to 1 if stack overflow occurred; set to 0 otherwise". >> >> In helper_fxam_ST0, I guess, we need "env->fpus |= 0x200" (but I don't >> know wheter it will be conflict with SIGND(temp)). And we have to still >> need foverflow, because all env->fptags being 0 doesn't mean overflow. > > No, you need to add "env->fpus |= 0x200" and "env->fpus &= ~0x200" > directly to fpush, fpop, etc. > OK. The content below is my next TODO, welcome your opinions. When overflow occurs, for me, we need keep everything no touch except set C1 flag. In fxam, we don't clear C1, but keep no touch for clearning C1 in another places. When underflow occurs, for me, we need keep everything no touch except set env->fpstt 8, so the next consecutive fpop/f[i]stp* can be checked easier, and the next fpush/f[i]ld* can work well in normal. For fxam, we check env->fpstt == 8 and env->fptags for empty. And when env->fpstt is 8, it need be set 7 before used in fincstp and ffree_STN. Thanks.
On 22/02/20 13:25, Chen Gang wrote: > On 2020/2/22 下午3:37, Paolo Bonzini wrote: >> On 22/02/20 03:10, Chen Gang wrote: >>> Set C1 to 1 if stack overflow occurred; set to 0 otherwise". >>> >>> In helper_fxam_ST0, I guess, we need "env->fpus |= 0x200" (but I don't >>> know wheter it will be conflict with SIGND(temp)). And we have to still >>> need foverflow, because all env->fptags being 0 doesn't mean overflow. >> >> No, you need to add "env->fpus |= 0x200" and "env->fpus &= ~0x200" >> directly to fpush, fpop, etc. >> > > OK. The content below is my next TODO, welcome your opinions. > > When overflow occurs, for me, we need keep everything no touch except > set C1 flag. No, push will overwrite the top entry if there is overflow. > In fxam, we don't clear C1, but keep no touch for clearning > C1 in another places. FXAM is neither push nor pop, it just detects an empty slot via fptags. FXAM should be okay with my patch. > When underflow occurs, for me, we need keep everything no touch except > set env->fpstt 8, so the next consecutive fpop/f[i]stp* can be checked > easier, and the next fpush/f[i]ld* can work well in normal. > For fxam, we check env->fpstt == 8 and env->fptags for empty. And when > env->fpstt is 8, it need be set 7 before used in fincstp and ffree_STN. I don't think you need env->fpstt to be set to 8 in any case. Also, pop must mark ST(0) as empty always, even if underflow occurs, and also clear C1 if underflow occurs. Paolo
On 2020/2/24 下午8:43, Paolo Bonzini wrote: > On 22/02/20 13:25, Chen Gang wrote: >> On 2020/2/22 下午3:37, Paolo Bonzini wrote: >>> On 22/02/20 03:10, Chen Gang wrote: >>>> Set C1 to 1 if stack overflow occurred; set to 0 otherwise". >>>> >>>> In helper_fxam_ST0, I guess, we need "env->fpus |= 0x200" (but I don't >>>> know wheter it will be conflict with SIGND(temp)). And we have to still >>>> need foverflow, because all env->fptags being 0 doesn't mean overflow. >>> >>> No, you need to add "env->fpus |= 0x200" and "env->fpus &= ~0x200" >>> directly to fpush, fpop, etc. >>> >> >> OK. The content below is my next TODO, welcome your opinions. >> >> When overflow occurs, for me, we need keep everything no touch except >> set C1 flag. > > No, push will overwrite the top entry if there is overflow. > >> In fxam, we don't clear C1, but keep no touch for clearning >> C1 in another places. > > FXAM is neither push nor pop, it just detects an empty slot via fptags. > FXAM should be okay with my patch. > OK. I am not quite clear about it, but it fixes the current issues at least. Please apply your patch. Thanks.
diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 576f309bbf..3e2b719ab7 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1394,6 +1394,7 @@ typedef struct CPUX86State { struct {} start_init_save; /* FPU state */ + bool foverflow; unsigned int fpstt; /* top of stack index */ uint16_t fpus; uint16_t fpuc; diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c index 99f28f267f..81f3cefe8b 100644 --- a/target/i386/fpu_helper.c +++ b/target/i386/fpu_helper.c @@ -91,17 +91,31 @@ void cpu_set_ignne(void) } #endif +static inline void set_fpstt(CPUX86State *env, unsigned int fpstt, + bool pop, bool full) +{ + env->foverflow = (fpstt > 7) && full; /* clear the original flag */ + if (pop) { + if (full) { + env->fptags[env->fpstt] = 1; /* invalidate stack entry */ + } + env->fpstt = fpstt & 7; + } else { + env->fpstt = fpstt & 7; + if (full) { + env->fptags[env->fpstt] = 0; /* validate stack entry */ + } + } +} static inline void fpush(CPUX86State *env) { - env->fpstt = (env->fpstt - 1) & 7; - env->fptags[env->fpstt] = 0; /* validate stack entry */ + set_fpstt(env, env->fpstt - 1, false, true); } static inline void fpop(CPUX86State *env) { - env->fptags[env->fpstt] = 1; /* invalidate stack entry */ - env->fpstt = (env->fpstt + 1) & 7; + set_fpstt(env, env->fpstt + 1, true, true); } static inline floatx80 helper_fldt(CPUX86State *env, target_ulong ptr, @@ -211,11 +225,10 @@ void helper_flds_ST0(CPUX86State *env, uint32_t val) uint32_t i; } u; - new_fpstt = (env->fpstt - 1) & 7; + new_fpstt = env->fpstt - 1; u.i = val; - env->fpregs[new_fpstt].d = float32_to_floatx80(u.f, &env->fp_status); - env->fpstt = new_fpstt; - env->fptags[new_fpstt] = 0; /* validate stack entry */ + env->fpregs[new_fpstt & 7].d = float32_to_floatx80(u.f, &env->fp_status); + set_fpstt(env, new_fpstt, false, true); } void helper_fldl_ST0(CPUX86State *env, uint64_t val) @@ -226,31 +239,28 @@ void helper_fldl_ST0(CPUX86State *env, uint64_t val) uint64_t i; } u; - new_fpstt = (env->fpstt - 1) & 7; + new_fpstt = env->fpstt - 1; u.i = val; - env->fpregs[new_fpstt].d = float64_to_floatx80(u.f, &env->fp_status); - env->fpstt = new_fpstt; - env->fptags[new_fpstt] = 0; /* validate stack entry */ + env->fpregs[new_fpstt & 7].d = float64_to_floatx80(u.f, &env->fp_status); + set_fpstt(env, new_fpstt, false, true); } void helper_fildl_ST0(CPUX86State *env, int32_t val) { int new_fpstt; - new_fpstt = (env->fpstt - 1) & 7; - env->fpregs[new_fpstt].d = int32_to_floatx80(val, &env->fp_status); - env->fpstt = new_fpstt; - env->fptags[new_fpstt] = 0; /* validate stack entry */ + new_fpstt = env->fpstt - 1; + env->fpregs[new_fpstt & 7].d = int32_to_floatx80(val, &env->fp_status); + set_fpstt(env, new_fpstt, false, true); } void helper_fildll_ST0(CPUX86State *env, int64_t val) { int new_fpstt; - new_fpstt = (env->fpstt - 1) & 7; - env->fpregs[new_fpstt].d = int64_to_floatx80(val, &env->fp_status); - env->fpstt = new_fpstt; - env->fptags[new_fpstt] = 0; /* validate stack entry */ + new_fpstt = env->fpstt - 1; + env->fpregs[new_fpstt & 7].d = int64_to_floatx80(val, &env->fp_status); + set_fpstt(env, new_fpstt, false, true); } uint32_t helper_fsts_ST0(CPUX86State *env) @@ -345,10 +355,9 @@ void helper_fldt_ST0(CPUX86State *env, target_ulong ptr) { int new_fpstt; - new_fpstt = (env->fpstt - 1) & 7; - env->fpregs[new_fpstt].d = helper_fldt(env, ptr, GETPC()); - env->fpstt = new_fpstt; - env->fptags[new_fpstt] = 0; /* validate stack entry */ + new_fpstt = env->fpstt - 1; + env->fpregs[new_fpstt & 7].d = helper_fldt(env, ptr, GETPC()); + set_fpstt(env, new_fpstt, false, true); } void helper_fstt_ST0(CPUX86State *env, target_ulong ptr) @@ -368,13 +377,13 @@ void helper_fpop(CPUX86State *env) void helper_fdecstp(CPUX86State *env) { - env->fpstt = (env->fpstt - 1) & 7; + set_fpstt(env, env->fpstt - 1, false, false); env->fpus &= ~0x4700; } void helper_fincstp(CPUX86State *env) { - env->fpstt = (env->fpstt + 1) & 7; + set_fpstt(env, env->fpstt + 1, true, false); env->fpus &= ~0x4700; } @@ -382,6 +391,7 @@ void helper_fincstp(CPUX86State *env) void helper_ffree_STN(CPUX86State *env, int st_index) { + set_fpstt(env, env->fpstt + st_index, true, false); env->fptags[(env->fpstt + st_index) & 7] = 1; } @@ -644,6 +654,7 @@ void helper_fninit(CPUX86State *env) { env->fpus = 0; env->fpstt = 0; + env->foverflow = false; cpu_set_fpuc(env, 0x37f); env->fptags[0] = 1; env->fptags[1] = 1; @@ -1008,6 +1019,11 @@ void helper_fxam_ST0(CPUX86State *env) } else { env->fpus |= 0x400; } + + if (env->foverflow) { + env->fpus |= 0x4100; + env->fpus &= ~0x400; + } } static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32, @@ -1636,7 +1652,7 @@ void helper_ldmxcsr(CPUX86State *env, uint32_t val) void helper_enter_mmx(CPUX86State *env) { - env->fpstt = 0; + set_fpstt(env, 0, true, false); *(uint32_t *)(env->fptags) = 0; *(uint32_t *)(env->fptags + 4) = 0; }