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~