Re: [Qemu-devel] Patch: fix FSTENV (and FSAVE) instructions in target-i386 (resend, cleaned up).

[Blue Swirl]

On Fri, Nov 26, 2010 at 10:53 PM, ChALkeR <address@hidden> wrote:
>> Please also consider fixing FSAVE and FXSAVE.
>
> FSAVE also works, i checked twice (code and test).
> FSAVE in QEMU calls FSTENV.
>
> op_helper.c:
>> void helper_fsave(target_ulong ptr, int data32)
>> {
>>    CPU86_LDouble tmp;
>>    int i;
>>
>>    helper_fstenv(ptr, data32);

Sorry, I missed that.

> Not sure how FXSAVE operates, that has to be checked in the manuals.
> But the patch does not break anything and fixes FSTENV and FSAVE instructions.

Yes, I just wish for more fixes. I think FRSTOR/FXRSTOR also need to
update env->fpip etc.

>> In this place, the data would be saved for every instruction. But I
>> think only FPU instructions should update the fields.
>
> I do not understand. The patch stores eip only for FPU instructions.
> This is correct.
> FSTENV should put the eip of the last FPU instruction in the stack.
> The helper is called at the end of every instruction on floating-point
> case of the switch.
> There should be some additional data, too, but the correct operation
> of fpip is provided by the patch.
>
> translate.c, line 2461:
>>        /************************/
>>        /* floats */
>>    case 0xd8 ... 0xdf:

I thought the store happened at the end of disas_insn(). Then this is OK.

>> You can simply use tcg_gen_movi_tl() and tcg_gen_st_tl(), no need for
>> a helper except for the data pointer.
>
> Can you fix it, please, if I did something wrong?

It's not wrong but just less than optimal, there's some overhead with
helper calls. Something like:
tcg_gen_movi_tl(cpu_tmp0, pc_start - s->cs_base);
tcg_gen_st_tl(cpu_tmp0, cpu_env, offsetof(CPUState, fpip));

Likewise for CS and instruction. Actually the data pointer also seems
to be in A0, so we can also store that:
tcg_gen_st_tl(cpu_A0, cpu_env, offsetof(CPUState, fpdp));

> Sorry for my bad English.

No problem, I suppose most people on this list are not native English
speakers anyway.