[PATCH] Set the correct env->fpip for x86 float instructions [cleaned]

[PATCH] Set the correct env->fpip for x86 float instructions [cleaned]

[Ziqiao Kong]

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

Re: [PATCH] Set the correct env->fpip for x86 float instructions [cleaned]

[Ziqiao Kong]

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
>

Re: [PATCH] Set the correct env->fpip for x86 float instructions [cleaned]

[Richard Henderson]

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~

Re: [PATCH] Set the correct env->fpip for x86 float instructions [cleaned]

[Ziqiao Kong]

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~