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

[Richard Henderson]

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?

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.


r~