Re: [Qemu-devel] [PATCH] x86: Fix eflags tracking for syscall insn

[Doug Evans]

On Tue, Dec 6, 2016 at 11:43 AM, Richard Henderson <address@hidden> wrote:
> On 12/06/2016 09:13 AM, Doug Evans wrote:
>> @@ -7104,6 +7104,10 @@ static target_ulong disas_insn(CPUX86State *env,
>> DisasContext *s,
>>          gen_update_cc_op(s);
>>          gen_jmp_im(pc_start - s->cs_base);
>>          gen_helper_syscall(cpu_env, tcg_const_i32(s->pc - pc_start));
>> +        /* condition codes are modified only in long mode */
>> +        if (s->lma) {
>> +            set_cc_op(s, CC_OP_EFLAGS);
>> +        }
>
> Since we will definitely end the TranslationBlock here, I'm a bit confused as
> to how we get things wrong here.  Is this simply a visual inspection of the
> code, or do you have a test case?

Visual inspection and looking at what other code does.
[eflags handling is very clever, but a bit obscure - I'm sure just a
bit more documentation
would help but I dunno if it's "just me" ...]

> In gen_update_cc_op, we store the current version of CC_OP back to ENV.  This
> lets helper_syscall (via cpu_compute_eflags) compute the proper value.  We 
> then
> store the updated version back via cpu_load_eflags, which sets ENV->CC_OP to
> CC_OP_EFLAGS.
>
> So far so good.
>
> The only way I could see things going wrong is if we were to use DC->CC_OP
> after the call to the helper.  But since we've (1) synced the value first and
> (2) end the TB via gen_eob, I don't see how we would.
>
> If any change is required, I think better would be
>
> +  /* The syscall helper may read and modify EFLAGS.  */
>    gen_update_cc_op(s);
> +  set_cc_op(s, CC_OP_DYNAMIC);
>    gen_jmp_im(pc_start - s->cs_base);
>    gen_helper_syscall(cpu_env, tcg_const_i32(s->pc - pc_start));
>
> This will tell subsequent code within the translator that it must re-read
> ENV->CC_OP in order to compute the flags.

Ah.