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

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

[Doug Evans]

Hi.

While researching an issue related to the syscall insn it seemed
like eflags status tracking was missing this step.

I think(!) this is correct, it follows what similar code does elsewhere,
and what the doc says. If it's not correct IWBN to clarify the situation.

commit 393243eda30d4429a07a0f7c29b0f6297616a355
Author: Doug Evans <dje@google.com>
Date:   Tue Dec 6 09:00:42 2016 -0800

     syscall insn: update eflags to CC_OP_EFLAGS

     Signed-off-by: Doug Evans <dje@google.com>

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 324103c..9fd1a04 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -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);
+        }
          gen_eob(s);
          break;
      case 0x107: /* sysret */

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~

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

[Doug Evans]

On Tue, Dec 6, 2016 at 11:43 AM, Richard Henderson <rth@twiddle.net> 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.