Re: [Qemu-devel] PATCH for bugs 661696 and 1248376: target-i386: x87 exception pointers using TCG.

[Jaume Martí]

Thanks Richard for your feedback. I am going to correct the patch and
resubmit it.

Best regards,
Jaume

On Sun, Jun 22, 2014 at 8:55 PM, Richard Henderson <address@hidden> wrote:
> On 06/22/2014 07:55 AM, Jaume Martí wrote:
>> -        cpu_x86_fsave(env, fpstate_addr, 1);
>> -        fpstate->status = fpstate->sw;
>> -        magic = 0xffff;
>> +    cpu_x86_fsave(env, fpstate_addr);
>> +    fpstate->status = fpstate->sw;
>> +    magic = 0xffff;
>
> This patch needs to be split into format fixes and the actual change to be
> reviewed.
>
>> -    /* KVM-only so far */
>> -    uint16_t fpop;
>> +    union {
>> +        uint32_t tcg;
>> +        uint16_t kvm;
>> +    } fpop;
>
> This is highly questionable.
>
>>      .fields = (VMStateField[]) {
>> -        VMSTATE_UINT16(env.fpop, X86CPU),
>> +        VMSTATE_UINT16(env.fpop.kvm, X86CPU),
>
> You're breaking save/restore in tcg.  KVM is not required for migration.
>
>> +        if (non_control_x87_instr(modrm, b)) {
>> +            tcg_gen_movi_i32(cpu_fpop, ((b & 0x7) << 8) | (modrm & 0xff));
>> +            tcg_gen_movi_tl(cpu_fpip, pc_start - s->cs_base);
>> +            tcg_gen_movi_i32(cpu_fpcs, env->segs[R_CS].selector);
>> +        }
>
> I strongly suspect you can implement this feature without having to add 3
> (largely redundant) register writes to every x87 instruction executed.
>
> See how restore_state_to_opc works to compute the value of CC_OP during
> translation.  You can do the same thing to recover these three values.
>
> You do have to sync these values before normal exits from the TB, but you only
> have to do that once, not once for every insn executed.  See gen_update_cc_op.
>
>
> r~