| Message ID | 20210416153430.92187-1-ziqiaokong@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Set the correct env->fpip for x86 float instructions [cleaned] | expand |
Ping for this patch. Patchew: https://patchew.org/QEMU/20210416153430.92187-1-ziqiaokong@gmail.com/ Ziqiao. On Fri, Apr 16, 2021 at 11:38 PM Ziqiao Kong <ziqiaokong@gmail.com> wrote: > > Hello, everyone! > > Sorry that I forgot the Signed-off-by line and put the duplicate link just now. Please ignore my previous emails. > > This patch follows https://lists.gnu.org/archive/html/qemu-devel/2010-11/msg02497.html and https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg00307.html > > Sorry again for any inconvenience. > > Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com> > --- > target/i386/tcg/fpu_helper.c | 4 ++-- > target/i386/tcg/translate.c | 3 +++ > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c > index 60ed93520a..e8cbde4e1a 100644 > --- a/target/i386/tcg/fpu_helper.c > +++ b/target/i386/tcg/fpu_helper.c > @@ -2395,7 +2395,7 @@ static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32, > cpu_stl_data_ra(env, ptr, env->fpuc, retaddr); > cpu_stl_data_ra(env, ptr + 4, fpus, retaddr); > cpu_stl_data_ra(env, ptr + 8, fptag, retaddr); > - cpu_stl_data_ra(env, ptr + 12, 0, retaddr); /* fpip */ > + cpu_stl_data_ra(env, ptr + 12, env->fpip, retaddr); /* fpip */ > cpu_stl_data_ra(env, ptr + 16, 0, retaddr); /* fpcs */ > cpu_stl_data_ra(env, ptr + 20, 0, retaddr); /* fpoo */ > cpu_stl_data_ra(env, ptr + 24, 0, retaddr); /* fpos */ > @@ -2404,7 +2404,7 @@ static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32, > cpu_stw_data_ra(env, ptr, env->fpuc, retaddr); > cpu_stw_data_ra(env, ptr + 2, fpus, retaddr); > cpu_stw_data_ra(env, ptr + 4, fptag, retaddr); > - cpu_stw_data_ra(env, ptr + 6, 0, retaddr); > + cpu_stw_data_ra(env, ptr + 6, env->fpip, retaddr); > cpu_stw_data_ra(env, ptr + 8, 0, retaddr); > cpu_stw_data_ra(env, ptr + 10, 0, retaddr); > cpu_stw_data_ra(env, ptr + 12, 0, retaddr); > diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c > index 880bc45561..cc4398f03b 100644 > --- a/target/i386/tcg/translate.c > +++ b/target/i386/tcg/translate.c > @@ -6337,7 +6337,10 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > goto unknown_op; > } > } > + tcg_gen_movi_tl(s->tmp0, pc_start - s->cs_base); > + tcg_gen_st_tl(s->tmp0, cpu_env, offsetof(CPUX86State, fpip)); > break; > + > /************************/ > /* string ops */ > > -- > 2.25.1 >
On 4/16/21 8:34 AM, Ziqiao Kong wrote: > +++ b/target/i386/tcg/translate.c > @@ -6337,7 +6337,10 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > goto unknown_op; > } > } > + tcg_gen_movi_tl(s->tmp0, pc_start - s->cs_base); > + tcg_gen_st_tl(s->tmp0, cpu_env, offsetof(CPUX86State, fpip)); This placement is wrong because it catches instructions that should not modify FIP, like FINIT. It might be best to set a flag around this case like bool update_fip; case 0xd8 .. 0xdf: ... update_fip = true; if (mod != 3) { ... } else { ... } if (update_fip) { ... } break; and set update_fip to false for the set of insns that either do not update FIP or clear it (8.1.8 x87 fpu instruction and data (operand) pointers). I notice you're not saving FCS to go along with this, at least for CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 0. And if you're going to this trouble, you might want to think about FDP+FDS as well. It should be about the same amount of effort. r~
Thanks for your review! I did a full re-read of the Intel Manual about x87 programming just now and would send another patch to handle FCS:FIP and FDS:FDP. Ziqiao On Wed, Apr 28, 2021 at 1:49 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 4/16/21 8:34 AM, Ziqiao Kong wrote: > > +++ b/target/i386/tcg/translate.c > > @@ -6337,7 +6337,10 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > > goto unknown_op; > > } > > } > > + tcg_gen_movi_tl(s->tmp0, pc_start - s->cs_base); > > + tcg_gen_st_tl(s->tmp0, cpu_env, offsetof(CPUX86State, fpip)); > > This placement is wrong because it catches instructions that should not modify > FIP, like FINIT. > > It might be best to set a flag around this case like > > bool update_fip; > > case 0xd8 .. 0xdf: > ... > update_fip = true; > if (mod != 3) { > ... > } else { > ... > } > if (update_fip) { > ... > } > break; > > and set update_fip to false for the set of insns that either do not update FIP > or clear it (8.1.8 x87 fpu instruction and data (operand) pointers). > > I notice you're not saving FCS to go along with this, at least for > CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 0. > > And if you're going to this trouble, you might want to think about FDP+FDS as > well. It should be about the same amount of effort. > > > r~
diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c index 60ed93520a..e8cbde4e1a 100644 --- a/target/i386/tcg/fpu_helper.c +++ b/target/i386/tcg/fpu_helper.c @@ -2395,7 +2395,7 @@ static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32, cpu_stl_data_ra(env, ptr, env->fpuc, retaddr); cpu_stl_data_ra(env, ptr + 4, fpus, retaddr); cpu_stl_data_ra(env, ptr + 8, fptag, retaddr); - cpu_stl_data_ra(env, ptr + 12, 0, retaddr); /* fpip */ + cpu_stl_data_ra(env, ptr + 12, env->fpip, retaddr); /* fpip */ cpu_stl_data_ra(env, ptr + 16, 0, retaddr); /* fpcs */ cpu_stl_data_ra(env, ptr + 20, 0, retaddr); /* fpoo */ cpu_stl_data_ra(env, ptr + 24, 0, retaddr); /* fpos */ @@ -2404,7 +2404,7 @@ static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32, cpu_stw_data_ra(env, ptr, env->fpuc, retaddr); cpu_stw_data_ra(env, ptr + 2, fpus, retaddr); cpu_stw_data_ra(env, ptr + 4, fptag, retaddr); - cpu_stw_data_ra(env, ptr + 6, 0, retaddr); + cpu_stw_data_ra(env, ptr + 6, env->fpip, retaddr); cpu_stw_data_ra(env, ptr + 8, 0, retaddr); cpu_stw_data_ra(env, ptr + 10, 0, retaddr); cpu_stw_data_ra(env, ptr + 12, 0, retaddr); diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c index 880bc45561..cc4398f03b 100644 --- a/target/i386/tcg/translate.c +++ b/target/i386/tcg/translate.c @@ -6337,7 +6337,10 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) goto unknown_op; } } + tcg_gen_movi_tl(s->tmp0, pc_start - s->cs_base); + tcg_gen_st_tl(s->tmp0, cpu_env, offsetof(CPUX86State, fpip)); break; + /************************/ /* string ops */
Hello, everyone! Sorry that I forgot the Signed-off-by line and put the duplicate link just now. Please ignore my previous emails. This patch follows https://lists.gnu.org/archive/html/qemu-devel/2010-11/msg02497.html and https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg00307.html Sorry again for any inconvenience. Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com> --- target/i386/tcg/fpu_helper.c | 4 ++-- target/i386/tcg/translate.c | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-)