[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Patch: fix FSTENV (and FSAVE) instructions in target-i3
From: |
Blue Swirl |
Subject: |
Re: [Qemu-devel] Patch: fix FSTENV (and FSAVE) instructions in target-i386 (resend, cleaned up). |
Date: |
Sat, 27 Nov 2010 10:01:11 +0000 |
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.