x86: Fix eflags tracking for syscall insn

PATCH In review Doug Evans 2 participants 3 messages Dec 6, 2016

lore

Share & tools

lore

Doug Evans

author

2016-12-06 17:15 UTC #0 lore raw

PATCH 1/1 x86: Fix eflags tracking for syscall insn

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 <[email protected]>
Date:   Tue Dec 6 09:00:42 2016 -0800

syscall insn: update eflags to CC_OP_EFLAGS

Signed-off-by Doug Evans <[email protected]>

target-i386/translate.c +4 -0

index 324103c..9fd1a04 100644
@@ -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 */

Richard Henderson

2016-12-06 19:44 UTC #1 lore raw

target-i386/translate.c static target_ulong disas_insn(CPUX86State *env, · line 7111

Quoted 9 lines · Doug Evans

target-i386/translate.c +4 -0

@@ -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.

Doug Evans

author

2016-12-06 22:38 UTC #2 lore raw

Quoted 14 lines · Richard Henderson

target-i386/translate.c static target_ulong disas_insn(CPUX86State *env, · line 7111
Quoted 9 lines · Doug Evans
target-i386/translate.c +4 -0
@@ -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" ...]

Quoted 19 lines

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
target-i386/translate.c +2 -0
+  /* 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));

Quoted 2 lines

This will tell subsequent code within the translator that it must re-read ENV->CC_OP in order to compute the flags.

Ah.