Re: [PATCH v7 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP

[Richard Henderson]

On 5/30/21 8:01 AM, Ziqiao Kong wrote:
>       /* fninit */
> -    env->fpus = 0;
> -    env->fpstt = 0;
> -    cpu_set_fpuc(env, 0x37f);
> -    env->fptags[0] = 1;
> -    env->fptags[1] = 1;
> -    env->fptags[2] = 1;
> -    env->fptags[3] = 1;
> -    env->fptags[4] = 1;
> -    env->fptags[5] = 1;
> -    env->fptags[6] = 1;
> -    env->fptags[7] = 1;
> +    helper_fninit(env);

Directly calling a function named "helper" is bad practice.  Those should only be called 
from TCG generated code.  In this case it happens to be ok, since helper_fninit does not 
use GETPC(), but really we should break out a "do_fninit" function to share.


>       case 0xd8 ... 0xdf:
>           {
> +            TCGv last_addr = tcg_temp_new();
> +            int last_seg;
> +            bool update_fdp = false;

These belong...

> @@ -5942,7 +5947,14 @@ 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);
> +                AddressParts a = gen_lea_modrm_0(env, s, modrm);

... here, within the section that deals with memory operands.

> +            if (update_fip) {
> +                tcg_gen_ld32u_tl(s->T0, cpu_env,
> +                                 offsetof(CPUX86State, segs[R_CS].selector));
> +                tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State, fpcs));
> +
> +                tcg_gen_movi_tl(s->T0, pc_start - s->cs_base);
> +                tcg_gen_st_tl(s->T0, cpu_env, offsetof(CPUX86State, fpip));
> +            }
> +
> +            if (update_fdp) {
> +                if (s->override >= 0) {
> +                    last_seg = s->override;
> +                }
> +                tcg_gen_ld32u_tl(s->T0, cpu_env,
> +                                 offsetof(CPUX86State,
> +                                 segs[last_seg].selector));
> +                tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State, fpds));
> +
> +                tcg_gen_st_tl(last_addr, cpu_env, offsetof(CPUX86State, fpdp));
> +            }

Similarly the update_fdp test belongs at the end of the memory op block.


r~