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

[Richard Henderson <richard.henderson@linaro.org>]

On 4/29/21 7:19 PM, Ziqiao Kong wrote:
> @@ -1440,7 +1442,9 @@ typedef struct CPUX86State {
>       FPReg fpregs[8];
>       /* KVM-only so far */
>       uint16_t fpop;
> +    uint16_t fpcs;
>       uint64_t fpip;
> +    uint16_t fpds;
>       uint64_t fpdp;

Let's put all uint16_t together, just after fpop, to avoid extra padding 
between the uint64_t.

> +        cpu_stw_data_ra(env, ptr + 16, fpcs, retaddr); /* fpcs */
> +        cpu_stw_data_ra(env, ptr + 18, 0, retaddr);
...
> +        cpu_stw_data_ra(env, ptr + 24, fpds, retaddr); /* fpds */
> +        cpu_stw_data_ra(env, ptr + 26, 0, retaddr);

Better to continue to use stl here, to zero-extend the field.

> @@ -5850,7 +5858,11 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
>           op = ((b & 7) << 3) | ((modrm >> 3) & 7);
>           if (mod != 3) {
>               /* memory op */
> -            gen_lea_modrm(env, s, modrm);
> +            update_fdp = true;

You should just move the declaration of update_fdp and last_addr into this 
block.  Then the store to fdp+fds goes at the end of this block.

> +            last_addr = gen_lea_modrm_0(env, s, modrm);
> +            ea = gen_lea_modrm_1(s, last_addr);
> +            gen_lea_v_seg(s, s->aflag, ea, last_addr.def_seg, s->override);

I think you should copy ea into a temporary here.

> +            tcg_gen_mov_tl(s->T0, cpu_seg_base[R_CS]);
> +            tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State, fpcs));

cpu_seg_base is segment base, not the segment selector.

I believe that you want to copy offsetof(CPUX86State, segs[seg_reg].selector) 
into this field.

> +            ea = gen_lea_modrm_1(s, last_addr);
> +            tcg_gen_st_tl(ea, cpu_env, offsetof(CPUX86State, fpdp));

Use the temporary saved above instead of re-computing.


r~