diff mbox series

[v2,23/23] target/i386: Enable TARGET_TB_PCREL

Message ID 20220906100932.343523-24-richard.henderson@linaro.org
State New
Headers show
Series target/i386: pc-relative translation blocks | expand

Commit Message

Richard Henderson Sept. 6, 2022, 10:09 a.m. UTC 
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/cpu-param.h     |  1 +
 target/i386/tcg/tcg-cpu.c   |  8 ++--
 target/i386/tcg/translate.c | 86 ++++++++++++++++++++++++++++++-------
 3 files changed, 77 insertions(+), 18 deletions(-)

Comments

Paolo Bonzini Sept. 21, 2022, 1:31 p.m. UTC | #1 
On Tue, Sep 6, 2022 at 12:10 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>  static void gen_update_eip_cur(DisasContext *s)
>  {
>      gen_jmp_im(s, s->base.pc_next - s->cs_base);
> +    s->pc_save = s->base.pc_next;

s->pc_save is not valid after all gen_jmp_im() calls. Is it worth
noting after each call to gen_jmp_im() why this is not a problem?

>  }
>
>  static void gen_update_eip_next(DisasContext *s)
>  {
>      gen_jmp_im(s, s->pc - s->cs_base);
> +    s->pc_save = s->pc;
> +}
> +
> +static TCGv gen_eip_cur(DisasContext *s)
> +{
> +    if (TARGET_TB_PCREL) {
> +        gen_update_eip_cur(s);
> +        return cpu_eip;
> +    } else {
> +        return tcg_constant_tl(s->base.pc_next - s->cs_base);
> +    }

Ok, now I see why you called it gen_eip_cur(), but it's still a bit
disconcerting to see the difference in behavior between the
TARGET_TB_PCREL and !TARGET_TB_PCREL cases, one of them updating
cpu_eip and other not.

Perhaps gen_jmp_im() and gen_update_eip_cur() could be rewritten to
return the destination instead:

static TCGv gen_jmp_im(DisasContext *s, target_ulong eip)
{
    if (TARGET_TB_PCREL) {
        target_ulong eip_save = s->pc_save - s->cs_base;
        tcg_gen_addi_tl(cpu_eip, cpu_eip, eip - eip_save);
        return cpu_eip;
    } else {
        TCGv dest = tcg_constant_tl(eip);
        tcg_gen_mov_tl(cpu_eip, dest);
        return dest;
    }
}

static TCGv gen_update_eip_cur(DisasContext *s)
{
    TCGv dest = gen_jmp_im(s, s->base.pc_next - s->cs_base);
    s->pc_save = s->base.pc_next;
    return dest;
}

and the "if (update_ip)" case would use the return value?

This change would basically replace the previous patch, with just the
"if (TARGET_TB_PCREL)" added here.

Paolo
Richard Henderson Oct. 1, 2022, 1:51 a.m. UTC | #2 
On 9/21/22 06:31, Paolo Bonzini wrote:
> On Tue, Sep 6, 2022 at 12:10 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>   static void gen_update_eip_cur(DisasContext *s)
>>   {
>>       gen_jmp_im(s, s->base.pc_next - s->cs_base);
>> +    s->pc_save = s->base.pc_next;
> 
> s->pc_save is not valid after all gen_jmp_im() calls. Is it worth
> noting after each call to gen_jmp_im() why this is not a problem?
> 
>>   }
>>
>>   static void gen_update_eip_next(DisasContext *s)
>>   {
>>       gen_jmp_im(s, s->pc - s->cs_base);
>> +    s->pc_save = s->pc;
>> +}
>> +
>> +static TCGv gen_eip_cur(DisasContext *s)
>> +{
>> +    if (TARGET_TB_PCREL) {
>> +        gen_update_eip_cur(s);
>> +        return cpu_eip;
>> +    } else {
>> +        return tcg_constant_tl(s->base.pc_next - s->cs_base);
>> +    }
> 
> Ok, now I see why you called it gen_eip_cur(), but it's still a bit
> disconcerting to see the difference in behavior between the
> TARGET_TB_PCREL and !TARGET_TB_PCREL cases, one of them updating
> cpu_eip and other not.
> 
> Perhaps gen_jmp_im() and gen_update_eip_cur() could be rewritten to
> return the destination instead:
> 
> static TCGv gen_jmp_im(DisasContext *s, target_ulong eip)
> {
>      if (TARGET_TB_PCREL) {
>          target_ulong eip_save = s->pc_save - s->cs_base;
>          tcg_gen_addi_tl(cpu_eip, cpu_eip, eip - eip_save);
>          return cpu_eip;
>      } else {
>          TCGv dest = tcg_constant_tl(eip);
>          tcg_gen_mov_tl(cpu_eip, dest);
>          return dest;
>      }
> }
> 
> static TCGv gen_update_eip_cur(DisasContext *s)
> {
>      TCGv dest = gen_jmp_im(s, s->base.pc_next - s->cs_base);
>      s->pc_save = s->base.pc_next;
>      return dest;
> }

I don't see what I'd do with the return values.  But I see your point about gen_eip_cur 
only updating eip sometimes.  I have changed the name to eip_cur_tl, as suggested, and it 
writes to a temporary, like eip_next_tl.


r~
diff mbox series

Patch

diff --git a/target/i386/cpu-param.h b/target/i386/cpu-param.h
index 9740bd7abd..51a3f153bf 100644
--- a/target/i386/cpu-param.h
+++ b/target/i386/cpu-param.h
@@ -24,5 +24,6 @@ 
 #endif
 #define TARGET_PAGE_BITS 12
 #define NB_MMU_MODES 3
+#define TARGET_TB_PCREL 1
 
 #endif
diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index 76989a5a9d..74333247c5 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -49,9 +49,11 @@  static void x86_cpu_exec_exit(CPUState *cs)
 static void x86_cpu_synchronize_from_tb(CPUState *cs,
                                         const TranslationBlock *tb)
 {
-    X86CPU *cpu = X86_CPU(cs);
-
-    cpu->env.eip = tb_pc(tb) - tb->cs_base;
+    /* The instruction pointer is always up to date with TARGET_TB_PCREL. */
+    if (!TARGET_TB_PCREL) {
+        CPUX86State *env = cs->env_ptr;
+        env->eip = tb_pc(tb) - tb->cs_base;
+    }
 }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 39bcb7263b..249309ddbc 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -64,6 +64,7 @@ 
 
 /* global register indexes */
 static TCGv cpu_cc_dst, cpu_cc_src, cpu_cc_src2;
+static TCGv cpu_eip;
 static TCGv_i32 cpu_cc_op;
 static TCGv cpu_regs[CPU_NB_REGS];
 static TCGv cpu_seg_base[6];
@@ -77,6 +78,7 @@  typedef struct DisasContext {
 
     target_ulong pc;       /* pc = eip + cs_base */
     target_ulong cs_base;  /* base of CS segment */
+    target_ulong pc_save;
 
     MemOp aflag;
     MemOp dflag;
@@ -481,7 +483,7 @@  static void gen_add_A0_im(DisasContext *s, int val)
 
 static inline void gen_op_jmp_v(TCGv dest)
 {
-    tcg_gen_st_tl(dest, cpu_env, offsetof(CPUX86State, eip));
+    tcg_gen_mov_tl(cpu_eip, dest);
 }
 
 static inline
@@ -516,24 +518,36 @@  static inline void gen_op_st_rm_T0_A0(DisasContext *s, int idx, int d)
     }
 }
 
-static TCGv gen_eip_cur(DisasContext *s)
+static void gen_jmp_im(DisasContext *s, target_ulong eip)
 {
-    return tcg_constant_tl(s->base.pc_next - s->cs_base);
-}
-
-static void gen_jmp_im(DisasContext *s, target_ulong pc)
-{
-    gen_op_jmp_v(tcg_constant_tl(pc));
+    if (TARGET_TB_PCREL) {
+        target_ulong eip_save = s->pc_save - s->cs_base;
+        tcg_gen_addi_tl(cpu_eip, cpu_eip, eip - eip_save);
+    } else {
+        tcg_gen_movi_tl(cpu_eip, eip);
+    }
 }
 
 static void gen_update_eip_cur(DisasContext *s)
 {
     gen_jmp_im(s, s->base.pc_next - s->cs_base);
+    s->pc_save = s->base.pc_next;
 }
 
 static void gen_update_eip_next(DisasContext *s)
 {
     gen_jmp_im(s, s->pc - s->cs_base);
+    s->pc_save = s->pc;
+}
+
+static TCGv gen_eip_cur(DisasContext *s)
+{
+    if (TARGET_TB_PCREL) {
+        gen_update_eip_cur(s);
+        return cpu_eip;
+    } else {
+        return tcg_constant_tl(s->base.pc_next - s->cs_base);
+    }
 }
 
 static int cur_insn_len(DisasContext *s)
@@ -548,12 +562,25 @@  static TCGv_i32 cur_insn_len_i32(DisasContext *s)
 
 static TCGv_i32 eip_next_i32(DisasContext *s)
 {
-    return tcg_constant_i32(s->pc - s->cs_base);
+    if (TARGET_TB_PCREL) {
+        TCGv_i32 ret = tcg_temp_new_i32();
+        tcg_gen_trunc_tl_i32(ret, cpu_eip);
+        tcg_gen_addi_i32(ret, ret, s->pc - s->pc_save);
+        return ret;
+    } else {
+        return tcg_constant_i32(s->pc - s->cs_base);
+    }
 }
 
 static TCGv eip_next_tl(DisasContext *s)
 {
-    return tcg_constant_tl(s->pc - s->cs_base);
+    if (TARGET_TB_PCREL) {
+        TCGv ret = tcg_temp_new();
+        tcg_gen_addi_tl(ret, cpu_eip, s->pc - s->pc_save);
+        return ret;
+    } else {
+        return tcg_constant_tl(s->pc - s->cs_base);
+    }
 }
 
 /* Compute SEG:REG into A0.  SEG is selected from the override segment
@@ -2252,7 +2279,12 @@  static TCGv gen_lea_modrm_1(DisasContext *s, AddressParts a)
         ea = cpu_regs[a.base];
     }
     if (!ea) {
-        tcg_gen_movi_tl(s->A0, a.disp);
+        if (TARGET_TB_PCREL && a.base == -2) {
+            /* With cpu_eip ~= pc_save, the expression is pc-relative. */
+            tcg_gen_addi_tl(s->A0, cpu_eip, a.disp - s->pc_save);
+        } else {
+            tcg_gen_movi_tl(s->A0, a.disp);
+        }
         ea = s->A0;
     } else if (a.disp != 0) {
         tcg_gen_addi_tl(s->A0, ea, a.disp);
@@ -2366,8 +2398,13 @@  static void gen_goto_tb(DisasContext *s, int tb_num, target_ulong eip)
 
     if (translator_use_goto_tb(&s->base, pc))  {
         /* jump to same page: we can use a direct jump */
-        tcg_gen_goto_tb(tb_num);
-        gen_jmp_im(s, eip);
+        if (TARGET_TB_PCREL) {
+            gen_jmp_im(s, eip);
+            tcg_gen_goto_tb(tb_num);
+        } else {
+            tcg_gen_goto_tb(tb_num);
+            gen_jmp_im(s, eip);
+        }
         tcg_gen_exit_tb(s->base.tb, tb_num);
         s->base.is_jmp = DISAS_NORETURN;
     } else {
@@ -8571,6 +8608,13 @@  void tcg_x86_init(void)
         [R_EDI] = "edi",
         [R_EBP] = "ebp",
         [R_ESP] = "esp",
+#endif
+    };
+    static const char eip_name[] = {
+#ifdef TARGET_X86_64
+        "rip"
+#else
+        "eip"
 #endif
     };
     static const char seg_base_names[6][8] = {
@@ -8597,6 +8641,7 @@  void tcg_x86_init(void)
                                     "cc_src");
     cpu_cc_src2 = tcg_global_mem_new(cpu_env, offsetof(CPUX86State, cc_src2),
                                      "cc_src2");
+    cpu_eip = tcg_global_mem_new(cpu_env, offsetof(CPUX86State, eip), eip_name);
 
     for (i = 0; i < CPU_NB_REGS; ++i) {
         cpu_regs[i] = tcg_global_mem_new(cpu_env,
@@ -8633,6 +8678,7 @@  static void i386_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cpu)
     int iopl = (flags >> IOPL_SHIFT) & 3;
 
     dc->cs_base = dc->base.tb->cs_base;
+    dc->pc_save = dc->base.pc_next;
     dc->flags = flags;
 #ifndef CONFIG_USER_ONLY
     dc->cpl = cpl;
@@ -8696,9 +8742,14 @@  static void i386_tr_tb_start(DisasContextBase *db, CPUState *cpu)
 static void i386_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
+    target_ulong pc_arg = dc->base.pc_next;
 
     dc->prev_insn_end = tcg_last_op();
-    tcg_gen_insn_start(dc->base.pc_next, dc->cc_op);
+    if (TARGET_TB_PCREL) {
+        pc_arg -= dc->cs_base;
+        pc_arg &= ~TARGET_PAGE_MASK;
+    }
+    tcg_gen_insn_start(pc_arg, dc->cc_op);
 }
 
 static void i386_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
@@ -8799,7 +8850,12 @@  void restore_state_to_opc(CPUX86State *env, TranslationBlock *tb,
                           target_ulong *data)
 {
     int cc_op = data[1];
-    env->eip = data[0] - tb->cs_base;
+
+    if (TARGET_TB_PCREL) {
+        env->eip = (env->eip & TARGET_PAGE_MASK) | data[0];
+    } else {
+        env->eip = data[0] - tb->cs_base;
+    }
     if (cc_op != CC_OP_DYNAMIC) {
         env->cc_op = cc_op;
     }