[PATCH 0/7] plugins: Use unwind info for special gdb registers

[PATCH 0/7] plugins: Use unwind info for special gdb registers

[Richard Henderson]

Based-on: 20240404230611.21231-1-richard.henderson@linaro.org
("[PATCH v2 00/21] Rewrite plugin code generation")

This is an attempt to fix
https://gitlab.com/qemu-project/qemu/-/issues/2208
("PC is not updated for each instruction in TCG plugins")

I have only updated target/i386 so far, but basically all targets
need updating for the new callbacks.  Extra points to anyone who
sees how to avoid the extra code duplication.  :-)


r~


Richard Henderson (7):
  tcg: Introduce INDEX_op_plugin_pc
  accel/tcg: Set CPUState.plugin_ra before all plugin callbacks
  accel/tcg: Return the TranslationBlock from cpu_unwind_state_data
  plugins: Introduce TCGCPUOps callbacks for mid-tb register reads
  target/i386: Split out gdb-internal.h
  target/i386: Introduce cpu_compute_eflags_ccop
  target/i386: Implement TCGCPUOps for plugin register reads

 include/exec/cpu-common.h     |  9 +++--
 include/hw/core/cpu.h         |  1 +
 include/hw/core/tcg-cpu-ops.h | 13 +++++++
 include/tcg/tcg-op-common.h   |  1 +
 include/tcg/tcg-opc.h         |  1 +
 target/i386/cpu.h             |  2 +
 target/i386/gdb-internal.h    | 65 +++++++++++++++++++++++++++++++
 accel/tcg/plugin-gen.c        | 50 +++++++++++++++++++++---
 accel/tcg/translate-all.c     |  9 +++--
 plugins/api.c                 | 36 +++++++++++++++++-
 target/i386/gdbstub.c         |  1 +
 target/i386/helper.c          |  6 ++-
 target/i386/tcg/cc_helper.c   | 10 +++++
 target/i386/tcg/tcg-cpu.c     | 72 +++++++++++++++++++++++++++--------
 tcg/tcg-op.c                  |  5 +++
 tcg/tcg.c                     | 10 +++++
 16 files changed, 258 insertions(+), 33 deletions(-)
 create mode 100644 target/i386/gdb-internal.h

-- 
2.34.1

[PATCH 1/7] tcg: Introduce INDEX_op_plugin_pc

[Richard Henderson]

Add an opcode to find a code address within the current insn,
for later use with unwinding.  Generate the code generically
using tcg_reg_alloc_do_movi.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/tcg/tcg-op-common.h |  1 +
 include/tcg/tcg-opc.h       |  1 +
 tcg/tcg-op.c                |  5 +++++
 tcg/tcg.c                   | 10 ++++++++++
 4 files changed, 17 insertions(+)

diff --git a/include/tcg/tcg-op-common.h b/include/tcg/tcg-op-common.h
index 009e2778c5..a32c88a182 100644
--- a/include/tcg/tcg-op-common.h
+++ b/include/tcg/tcg-op-common.h
@@ -76,6 +76,7 @@ void tcg_gen_lookup_and_goto_ptr(void);
 
 void tcg_gen_plugin_cb(unsigned from);
 void tcg_gen_plugin_mem_cb(TCGv_i64 addr, unsigned meminfo);
+void tcg_gen_plugin_pc(TCGv_ptr);
 
 /* 32 bit ops */
 
diff --git a/include/tcg/tcg-opc.h b/include/tcg/tcg-opc.h
index 546eb49c11..087d1b82da 100644
--- a/include/tcg/tcg-opc.h
+++ b/include/tcg/tcg-opc.h
@@ -199,6 +199,7 @@ DEF(goto_ptr, 0, 1, 0, TCG_OPF_BB_EXIT | TCG_OPF_BB_END)
 
 DEF(plugin_cb, 0, 0, 1, TCG_OPF_NOT_PRESENT)
 DEF(plugin_mem_cb, 0, 1, 1, TCG_OPF_NOT_PRESENT)
+DEF(plugin_pc, 1, 0, 0, TCG_OPF_NOT_PRESENT)
 
 /* Replicate ld/st ops for 32 and 64-bit guest addresses. */
 DEF(qemu_ld_a32_i32, 1, 1, 1,
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index eff3728622..b8ca78cbe4 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -322,6 +322,11 @@ void tcg_gen_plugin_mem_cb(TCGv_i64 addr, unsigned meminfo)
     tcg_gen_op2(INDEX_op_plugin_mem_cb, tcgv_i64_arg(addr), meminfo);
 }
 
+void tcg_gen_plugin_pc(TCGv_ptr arg)
+{
+    tcg_gen_op1(INDEX_op_plugin_pc, tcgv_ptr_arg(arg));
+}
+
 /* 32 bit ops */
 
 void tcg_gen_discard_i32(TCGv_i32 arg)
diff --git a/tcg/tcg.c b/tcg/tcg.c
index d248c52e96..42e2b53729 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -4701,6 +4701,13 @@ static void tcg_reg_alloc_mov(TCGContext *s, const TCGOp *op)
     }
 }
 
+static void tcg_reg_alloc_plugin_pc(TCGContext *s, const TCGOp *op)
+{
+    tcg_reg_alloc_do_movi(s, arg_temp(op->args[0]),
+                          (uintptr_t)tcg_splitwx_to_rx(s->code_ptr),
+                          op->life, output_pref(op, 0));
+}
+
 /*
  * Specialized code generation for INDEX_op_dup_vec.
  */
@@ -6208,6 +6215,9 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb, uint64_t pc_start)
         case INDEX_op_mov_vec:
             tcg_reg_alloc_mov(s, op);
             break;
+        case INDEX_op_plugin_pc:
+            tcg_reg_alloc_plugin_pc(s, op);
+            break;
         case INDEX_op_dup_vec:
             tcg_reg_alloc_dup(s, op);
             break;
-- 
2.34.1

[PATCH 2/7] accel/tcg: Set CPUState.plugin_ra before all plugin callbacks

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/hw/core/cpu.h  |  1 +
 accel/tcg/plugin-gen.c | 50 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 10cd492aff..f4af37c13d 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -350,6 +350,7 @@ typedef union IcountDecr {
 typedef struct CPUNegativeOffsetState {
     CPUTLB tlb;
 #ifdef CONFIG_PLUGIN
+    uintptr_t plugin_ra;
     GArray *plugin_mem_cbs;
 #endif
     IcountDecr icount_decr;
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 36e9134a5d..f96b49cce6 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -37,6 +37,12 @@ enum plugin_gen_from {
     PLUGIN_GEN_AFTER_TB,
 };
 
+enum plugin_gen_ra {
+    GEN_RA_DONE,
+    GEN_RA_FROM_TB,
+    GEN_RA_FROM_INSN,
+};
+
 /* called before finishing a TB with exit_tb, goto_tb or goto_ptr */
 void plugin_gen_disable_mem_helpers(void)
 {
@@ -151,11 +157,38 @@ static void gen_mem_cb(struct qemu_plugin_dyn_cb *cb,
     tcg_temp_free_i32(cpu_index);
 }
 
-static void inject_cb(struct qemu_plugin_dyn_cb *cb)
+static void inject_ra(enum plugin_gen_ra *gen_ra)
+{
+    TCGv_ptr ra;
+
+    switch (*gen_ra) {
+    case GEN_RA_DONE:
+        return;
+    case GEN_RA_FROM_TB:
+        ra = tcg_constant_ptr(NULL);
+        break;
+    case GEN_RA_FROM_INSN:
+        ra = tcg_temp_ebb_new_ptr();
+        tcg_gen_plugin_pc(ra);
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    tcg_gen_st_ptr(ra, tcg_env,
+                   offsetof(CPUState, neg.plugin_ra) -
+                   offsetof(ArchCPU, env));
+    tcg_temp_free_ptr(ra);
+    *gen_ra = GEN_RA_DONE;
+}
+
+static void inject_cb(struct qemu_plugin_dyn_cb *cb,
+                      enum plugin_gen_ra *gen_ra)
 
 {
     switch (cb->type) {
     case PLUGIN_CB_REGULAR:
+        inject_ra(gen_ra);
         gen_udata_cb(cb);
         break;
     case PLUGIN_CB_INLINE:
@@ -167,16 +200,18 @@ static void inject_cb(struct qemu_plugin_dyn_cb *cb)
 }
 
 static void inject_mem_cb(struct qemu_plugin_dyn_cb *cb,
+                          enum plugin_gen_ra *gen_ra,
                           enum qemu_plugin_mem_rw rw,
                           qemu_plugin_meminfo_t meminfo, TCGv_i64 addr)
 {
     if (cb->rw & rw) {
         switch (cb->type) {
         case PLUGIN_CB_MEM_REGULAR:
+            inject_ra(gen_ra);
             gen_mem_cb(cb, meminfo, addr);
             break;
         default:
-            inject_cb(cb);
+            inject_cb(cb, gen_ra);
             break;
         }
     }
@@ -186,6 +221,7 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
 {
     TCGOp *op, *next;
     int insn_idx = -1;
+    enum plugin_gen_ra gen_ra;
 
     if (unlikely(qemu_loglevel_mask(LOG_TB_OP_PLUGIN)
                  && qemu_log_in_addr_range(plugin_tb->vaddr))) {
@@ -205,10 +241,12 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
      */
     memset(tcg_ctx->free_temps, 0, sizeof(tcg_ctx->free_temps));
 
+    gen_ra = GEN_RA_FROM_TB;
     QTAILQ_FOREACH_SAFE(op, &tcg_ctx->ops, link, next) {
         switch (op->opc) {
         case INDEX_op_insn_start:
             insn_idx++;
+            gen_ra = GEN_RA_FROM_INSN;
             break;
 
         case INDEX_op_plugin_cb:
@@ -244,7 +282,8 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
                 cbs = plugin_tb->cbs;
                 for (i = 0, n = (cbs ? cbs->len : 0); i < n; i++) {
                     inject_cb(
-                        &g_array_index(cbs, struct qemu_plugin_dyn_cb, i));
+                        &g_array_index(cbs, struct qemu_plugin_dyn_cb, i),
+                        &gen_ra);
                 }
                 break;
 
@@ -256,7 +295,8 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
                 cbs = insn->insn_cbs;
                 for (i = 0, n = (cbs ? cbs->len : 0); i < n; i++) {
                     inject_cb(
-                        &g_array_index(cbs, struct qemu_plugin_dyn_cb, i));
+                        &g_array_index(cbs, struct qemu_plugin_dyn_cb, i),
+                        &gen_ra);
                 }
                 break;
 
@@ -288,7 +328,7 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
             cbs = insn->mem_cbs;
             for (i = 0, n = (cbs ? cbs->len : 0); i < n; i++) {
                 inject_mem_cb(&g_array_index(cbs, struct qemu_plugin_dyn_cb, i),
-                              rw, meminfo, addr);
+                              &gen_ra, rw, meminfo, addr);
             }
 
             tcg_ctx->emit_before_op = NULL;
-- 
2.34.1

[PATCH 3/7] accel/tcg: Return the TranslationBlock from cpu_unwind_state_data

[Richard Henderson]

Fix the i386 get_memio_eip function to use tb->cflags
instead of cs->tcg_cflags.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu-common.h | 9 +++++----
 accel/tcg/translate-all.c | 9 +++++----
 target/i386/helper.c      | 6 ++++--
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 6346df17ce..f056132cab 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -176,12 +176,13 @@ void list_cpus(void);
  * @host_pc: the host pc within the translation
  * @data: output data
  *
- * Attempt to load the the unwind state for a host pc occurring in
- * translated code.  If @host_pc is not in translated code, the
- * function returns false; otherwise @data is loaded.
+ * Attempt to load the the unwind state for a host pc occurring in translated
+ * code.  If @host_pc is not in translated code, the function returns NULL;
+ * otherwise @data is loaded and the TranslationBlock is returned.
  * This is the same unwind info as given to restore_state_to_opc.
  */
-bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data);
+const TranslationBlock *cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc,
+                                              uint64_t *data);
 
 /**
  * cpu_restore_state:
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 83cc14fbde..c745bc5b6c 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -243,15 +243,16 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc)
     return false;
 }
 
-bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data)
+const TranslationBlock *
+cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data)
 {
     if (in_code_gen_buffer((const void *)(host_pc - tcg_splitwx_diff))) {
         TranslationBlock *tb = tcg_tb_lookup(host_pc);
-        if (tb) {
-            return cpu_unwind_data_from_tb(tb, host_pc, data) >= 0;
+        if (tb && cpu_unwind_data_from_tb(tb, host_pc, data) >= 0) {
+            return tb;
         }
     }
-    return false;
+    return NULL;
 }
 
 void page_init(void)
diff --git a/target/i386/helper.c b/target/i386/helper.c
index 23ccb23a5b..eaa691a851 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -517,13 +517,15 @@ static inline target_ulong get_memio_eip(CPUX86State *env)
 #ifdef CONFIG_TCG
     uint64_t data[TARGET_INSN_START_WORDS];
     CPUState *cs = env_cpu(env);
+    const TranslationBlock *tb;
 
-    if (!cpu_unwind_state_data(cs, cs->mem_io_pc, data)) {
+    tb = cpu_unwind_state_data(cs, cs->mem_io_pc, data);
+    if (!tb) {
         return env->eip;
     }
 
     /* Per x86_restore_state_to_opc. */
-    if (cs->tcg_cflags & CF_PCREL) {
+    if (tb->cflags & CF_PCREL) {
         return (env->eip & TARGET_PAGE_MASK) | data[0];
     } else {
         return data[0] - env->segs[R_CS].base;
-- 
2.34.1

[PATCH 4/7] plugins: Introduce TCGCPUOps callbacks for mid-tb register reads

[Richard Henderson]

Certain target registers are not updated continuously within
the translation block.  For normal exception handling we use
unwind info to re-generate the correct value when required.
Leverage that same info for reading those registers for plugins.

All targets will need updating for these new callbacks.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/hw/core/tcg-cpu-ops.h | 13 +++++++++++++
 plugins/api.c                 | 36 +++++++++++++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
index bf8ff8e3ee..e954d83edf 100644
--- a/include/hw/core/tcg-cpu-ops.h
+++ b/include/hw/core/tcg-cpu-ops.h
@@ -49,6 +49,19 @@ struct TCGCPUOps {
     /** @debug_excp_handler: Callback for handling debug exceptions */
     void (*debug_excp_handler)(CPUState *cpu);
 
+    /**
+     * @plugin_need_unwind_for_reg:
+     * True if unwind info needed for reading reg.
+     */
+    bool (*plugin_need_unwind_for_reg)(CPUState *cpu, int reg);
+    /**
+     * @plugin_unwind_read_reg:
+     * Like CPUClass.gdb_read_register, but for registers that require
+     * regeneration using unwind info, like in @restore_state_to_opc.
+     */
+    int (*plugin_unwind_read_reg)(CPUState *cpu, GByteArray *buf, int reg,
+                                  const TranslationBlock *tb,
+                                  const uint64_t *data);
 #ifdef NEED_CPU_H
 #ifdef CONFIG_USER_ONLY
     /**
diff --git a/plugins/api.c b/plugins/api.c
index 3912c9cc8f..3543647a89 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -40,10 +40,12 @@
 #include "qemu/plugin.h"
 #include "qemu/log.h"
 #include "tcg/tcg.h"
+#include "tcg/insn-start-words.h"
 #include "exec/exec-all.h"
 #include "exec/gdbstub.h"
 #include "exec/ram_addr.h"
 #include "disas/disas.h"
+#include "hw/core/tcg-cpu-ops.h"
 #include "plugin.h"
 #ifndef CONFIG_USER_ONLY
 #include "qemu/plugin-memory.h"
@@ -454,9 +456,39 @@ GArray *qemu_plugin_get_registers(void)
 
 int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
 {
-    g_assert(current_cpu);
+    CPUState *cs;
+    uintptr_t ra;
+    int regno;
 
-    return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg));
+    assert(current_cpu);
+    cs = current_cpu;
+    ra = cs->neg.plugin_ra;
+    regno = GPOINTER_TO_INT(reg);
+
+    /*
+     * When plugin_ra is 0, we have no unwind info.  This will be true for
+     * TB callbacks that happen before any insns of the TB have started.
+     */
+    if (ra) {
+        const TCGCPUOps *tcg_ops = cs->cc->tcg_ops;
+
+        /*
+         * For plugins in the middle of the TB, we may need to locate
+         * and use unwind data to reconstruct a register value.
+         * Usually this required for the PC, but there may be others.
+         */
+        if (tcg_ops->plugin_need_unwind_for_reg &&
+            tcg_ops->plugin_need_unwind_for_reg(cs, regno)) {
+            uint64_t data[TARGET_INSN_START_WORDS];
+            const TranslationBlock *tb;
+
+            tb = cpu_unwind_state_data(cs, ra, data);
+            assert(tb);
+            return tcg_ops->plugin_unwind_read_reg(cs, buf, regno, tb, data);
+        }
+    }
+
+    return gdb_read_register(cs, buf, regno);
 }
 
 struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size)
-- 
2.34.1

[PATCH 5/7] target/i386: Split out gdb-internal.h

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/gdb-internal.h | 65 ++++++++++++++++++++++++++++++++++++++
 target/i386/gdbstub.c      |  1 +
 2 files changed, 66 insertions(+)
 create mode 100644 target/i386/gdb-internal.h

diff --git a/target/i386/gdb-internal.h b/target/i386/gdb-internal.h
new file mode 100644
index 0000000000..7cf4c1a656
--- /dev/null
+++ b/target/i386/gdb-internal.h
@@ -0,0 +1,65 @@
+/*
+ * x86 gdb server stub
+ *
+ * Copyright (c) 2003-2005 Fabrice Bellard
+ * Copyright (c) 2013 SUSE LINUX Products GmbH
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef I386_GDB_INTERNAL_H
+#define I386_GDB_INTERNAL_H
+
+/*
+ * Keep these in sync with assignment to
+ * gdb_num_core_regs in target/i386/cpu.c
+ * and with the machine description
+ */
+
+/*
+ * SEG: 6 segments, plus fs_base, gs_base, kernel_gs_base
+ */
+
+/*
+ * general regs ----->  8 or 16
+ */
+#define IDX_NB_IP       1
+#define IDX_NB_FLAGS    1
+#define IDX_NB_SEG      (6 + 3)
+#define IDX_NB_CTL      6
+#define IDX_NB_FP       16
+/*
+ * fpu regs ----------> 8 or 16
+ */
+#define IDX_NB_MXCSR    1
+/*
+ *          total ----> 8+1+1+9+6+16+8+1=50 or 16+1+1+9+6+16+16+1=66
+ */
+
+#define IDX_IP_REG      CPU_NB_REGS
+#define IDX_FLAGS_REG   (IDX_IP_REG + IDX_NB_IP)
+#define IDX_SEG_REGS    (IDX_FLAGS_REG + IDX_NB_FLAGS)
+#define IDX_CTL_REGS    (IDX_SEG_REGS + IDX_NB_SEG)
+#define IDX_FP_REGS     (IDX_CTL_REGS + IDX_NB_CTL)
+#define IDX_XMM_REGS    (IDX_FP_REGS + IDX_NB_FP)
+#define IDX_MXCSR_REG   (IDX_XMM_REGS + CPU_NB_REGS)
+
+#define IDX_CTL_CR0_REG     (IDX_CTL_REGS + 0)
+#define IDX_CTL_CR2_REG     (IDX_CTL_REGS + 1)
+#define IDX_CTL_CR3_REG     (IDX_CTL_REGS + 2)
+#define IDX_CTL_CR4_REG     (IDX_CTL_REGS + 3)
+#define IDX_CTL_CR8_REG     (IDX_CTL_REGS + 4)
+#define IDX_CTL_EFER_REG    (IDX_CTL_REGS + 5)
+
+#endif
diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
index ebb000df6a..9662509b82 100644
--- a/target/i386/gdbstub.c
+++ b/target/i386/gdbstub.c
@@ -20,6 +20,7 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "include/gdbstub/helpers.h"
+#include "gdb-internal.h"
 
 #ifdef TARGET_X86_64
 static const int gpr_map[16] = {
-- 
2.34.1

[PATCH 6/7] target/i386: Introduce cpu_compute_eflags_ccop

[Richard Henderson]

This is a generalization of cpu_compute_eflags, with a dynamic
value of cc_op, and is thus tcg specific.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/cpu.h           |  2 ++
 target/i386/tcg/cc_helper.c | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 6b05738079..285f26d99d 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2379,6 +2379,8 @@ void cpu_x86_inject_mce(Monitor *mon, X86CPU *cpu, int bank,
 
 uint32_t cpu_cc_compute_all(CPUX86State *env1);
 
+uint32_t cpu_compute_eflags_ccop(CPUX86State *env, CCOp op);
+
 static inline uint32_t cpu_compute_eflags(CPUX86State *env)
 {
     uint32_t eflags = env->eflags;
diff --git a/target/i386/tcg/cc_helper.c b/target/i386/tcg/cc_helper.c
index f76e9cb8cf..8203682ca8 100644
--- a/target/i386/tcg/cc_helper.c
+++ b/target/i386/tcg/cc_helper.c
@@ -225,6 +225,16 @@ uint32_t cpu_cc_compute_all(CPUX86State *env)
     return helper_cc_compute_all(CC_DST, CC_SRC, CC_SRC2, CC_OP);
 }
 
+uint32_t cpu_compute_eflags_ccop(CPUX86State *env, CCOp op)
+{
+    uint32_t eflags;
+
+    eflags = helper_cc_compute_all(CC_DST, CC_SRC, CC_SRC2, op);
+    eflags |= env->df & DF_MASK;
+    eflags |= env->eflags & ~(VM_MASK | RF_MASK);
+    return eflags;
+}
+
 target_ulong helper_cc_compute_c(target_ulong dst, target_ulong src1,
                                  target_ulong src2, int op)
 {
-- 
2.34.1

[PATCH 7/7] target/i386: Implement TCGCPUOps for plugin register reads

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/tcg-cpu.c | 72 ++++++++++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 16 deletions(-)

diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index cca19cd40e..2370053df2 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -22,9 +22,11 @@
 #include "helper-tcg.h"
 #include "qemu/accel.h"
 #include "hw/core/accel-cpu.h"
-
+#include "gdbstub/helpers.h"
+#include "gdb-internal.h"
 #include "tcg-cpu.h"
 
+
 /* Frob eflags into and out of the CPU temporary format.  */
 
 static void x86_cpu_exec_enter(CPUState *cs)
@@ -61,38 +63,74 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs,
     }
 }
 
-static void x86_restore_state_to_opc(CPUState *cs,
-                                     const TranslationBlock *tb,
-                                     const uint64_t *data)
+static uint64_t eip_from_unwind(CPUX86State *env, const TranslationBlock *tb,
+                                uint64_t data0)
 {
-    X86CPU *cpu = X86_CPU(cs);
-    CPUX86State *env = &cpu->env;
-    int cc_op = data[1];
     uint64_t new_pc;
 
     if (tb_cflags(tb) & CF_PCREL) {
         /*
-         * data[0] in PC-relative TBs is also a linear address, i.e. an address with
-         * the CS base added, because it is not guaranteed that EIP bits 12 and higher
-         * stay the same across the translation block.  Add the CS base back before
-         * replacing the low bits, and subtract it below just like for !CF_PCREL.
+         * data[0] in PC-relative TBs is also a linear address,
+         * i.e. an address with the CS base added, because it is
+         * not guaranteed that EIP bits 12 and higher stay the
+         * same across the translation block.  Add the CS base
+         * back before replacing the low bits, and subtract it
+         * below just like for !CF_PCREL.
          */
         uint64_t pc = env->eip + tb->cs_base;
-        new_pc = (pc & TARGET_PAGE_MASK) | data[0];
+        new_pc = (pc & TARGET_PAGE_MASK) | data0;
     } else {
-        new_pc = data[0];
+        new_pc = data0;
     }
     if (tb->flags & HF_CS64_MASK) {
-        env->eip = new_pc;
-    } else {
-        env->eip = (uint32_t)(new_pc - tb->cs_base);
+        return new_pc;
     }
+    return (uint32_t)(new_pc - tb->cs_base);
+}
 
+static void x86_restore_state_to_opc(CPUState *cs,
+                                     const TranslationBlock *tb,
+                                     const uint64_t *data)
+{
+    CPUX86State *env = cpu_env(cs);
+    CCOp cc_op;
+
+    env->eip = eip_from_unwind(env, tb, data[0]);
+
+    cc_op = data[1];
     if (cc_op != CC_OP_DYNAMIC) {
         env->cc_op = cc_op;
     }
 }
 
+static bool x86_plugin_need_unwind_for_reg(CPUState *cs, int reg)
+{
+    return reg == IDX_IP_REG || reg == IDX_FLAGS_REG;
+}
+
+static int x86_plugin_unwind_read_reg(CPUState *cs, GByteArray *buf, int reg,
+                                      const TranslationBlock *tb,
+                                      const uint64_t *data)
+{
+    CPUX86State *env = cpu_env(cs);
+    CCOp cc_op;
+
+    switch (reg) {
+    case IDX_IP_REG:
+        return gdb_get_regl(buf, eip_from_unwind(env, tb, data[0]));
+
+    case IDX_FLAGS_REG:
+        cc_op = data[1];
+        if (cc_op == CC_OP_DYNAMIC) {
+            cc_op = env->cc_op;
+        }
+        return gdb_get_reg32(buf, cpu_compute_eflags_ccop(env, cc_op));
+
+    default:
+        g_assert_not_reached();
+    }
+}
+
 #ifndef CONFIG_USER_ONLY
 static bool x86_debug_check_breakpoint(CPUState *cs)
 {
@@ -110,6 +148,8 @@ static const TCGCPUOps x86_tcg_ops = {
     .initialize = tcg_x86_init,
     .synchronize_from_tb = x86_cpu_synchronize_from_tb,
     .restore_state_to_opc = x86_restore_state_to_opc,
+    .plugin_need_unwind_for_reg = x86_plugin_need_unwind_for_reg,
+    .plugin_unwind_read_reg = x86_plugin_unwind_read_reg,
     .cpu_exec_enter = x86_cpu_exec_enter,
     .cpu_exec_exit = x86_cpu_exec_exit,
 #ifdef CONFIG_USER_ONLY
-- 
2.34.1

Re: [PATCH 0/7] plugins: Use unwind info for special gdb registers

[Pierrick Bouvier]

On 4/15/24 21:06, Richard Henderson wrote:
> Based-on: 20240404230611.21231-1-richard.henderson@linaro.org
> ("[PATCH v2 00/21] Rewrite plugin code generation")
> 
> This is an attempt to fix
> https://gitlab.com/qemu-project/qemu/-/issues/2208
> ("PC is not updated for each instruction in TCG plugins")
> 
> I have only updated target/i386 so far, but basically all targets
> need updating for the new callbacks.  Extra points to anyone who
> sees how to avoid the extra code duplication.  :-)
>

Thanks for the series Richard. It looks good to me.

Besides reviewing individual commits, I have a more general question.
 From some discussions we had, it seems like that previously gdb stub 
was correctly updating all register values, and that it has been dropped 
at some point.

Was it for performance reasons, or an architectural change in QEMU?
Is gdb stub the right way to poke register values for plugins?

I don't know exactly why some registers are not updated correctly in 
this context, but it seems like we are trying to fix this afterward, 
instead of identifying root cause.

Sorry if my question is irrelevant, I'm trying to understand the full 
context here.

Thanks,
Pierrick

> 
> r~
> 
> 
> Richard Henderson (7):
>    tcg: Introduce INDEX_op_plugin_pc
>    accel/tcg: Set CPUState.plugin_ra before all plugin callbacks
>    accel/tcg: Return the TranslationBlock from cpu_unwind_state_data
>    plugins: Introduce TCGCPUOps callbacks for mid-tb register reads
>    target/i386: Split out gdb-internal.h
>    target/i386: Introduce cpu_compute_eflags_ccop
>    target/i386: Implement TCGCPUOps for plugin register reads
> 
>   include/exec/cpu-common.h     |  9 +++--
>   include/hw/core/cpu.h         |  1 +
>   include/hw/core/tcg-cpu-ops.h | 13 +++++++
>   include/tcg/tcg-op-common.h   |  1 +
>   include/tcg/tcg-opc.h         |  1 +
>   target/i386/cpu.h             |  2 +
>   target/i386/gdb-internal.h    | 65 +++++++++++++++++++++++++++++++
>   accel/tcg/plugin-gen.c        | 50 +++++++++++++++++++++---
>   accel/tcg/translate-all.c     |  9 +++--
>   plugins/api.c                 | 36 +++++++++++++++++-
>   target/i386/gdbstub.c         |  1 +
>   target/i386/helper.c          |  6 ++-
>   target/i386/tcg/cc_helper.c   | 10 +++++
>   target/i386/tcg/tcg-cpu.c     | 72 +++++++++++++++++++++++++++--------
>   tcg/tcg-op.c                  |  5 +++
>   tcg/tcg.c                     | 10 +++++
>   16 files changed, 258 insertions(+), 33 deletions(-)
>   create mode 100644 target/i386/gdb-internal.h
> 

Re: [PATCH 0/7] plugins: Use unwind info for special gdb registers

[Richard Henderson]

On 4/16/24 17:35, Pierrick Bouvier wrote:
> On 4/15/24 21:06, Richard Henderson wrote:
>> Based-on: 20240404230611.21231-1-richard.henderson@linaro.org
>> ("[PATCH v2 00/21] Rewrite plugin code generation")
>>
>> This is an attempt to fix
>> https://gitlab.com/qemu-project/qemu/-/issues/2208
>> ("PC is not updated for each instruction in TCG plugins")
>>
>> I have only updated target/i386 so far, but basically all targets
>> need updating for the new callbacks.  Extra points to anyone who
>> sees how to avoid the extra code duplication.  :-)
>>
> 
> Thanks for the series Richard. It looks good to me.
> 
> Besides reviewing individual commits, I have a more general question.
>  From some discussions we had, it seems like that previously gdb stub was correctly 
> updating all register values, and that it has been dropped at some point.

Normally gdbstub does not run in the middle of a TB -- we end normally (single-step, 
breakpoint) or raise an exception (watchpoint).  Only then, after TCG state has been made 
consistent, does gdbstub have access to the CPUState.


> Was it for performance reasons, or an architectural change in QEMU?
> Is gdb stub the right way to poke register values for plugins?
> 
> I don't know exactly why some registers are not updated correctly in this context, but it 
> seems like we are trying to fix this afterward, instead of identifying root cause.

The one or two registers are not updated continuously for performance reasons.  And 
because they are not updated during initial code generation, it's not easy to do so later 
with plugin injection.  But recovering that data is what the unwind info is for -- a bit 
expensive to access that way, but overall less, with the expectation that it is rare.


r~

Re: [PATCH 0/7] plugins: Use unwind info for special gdb registers

[Pierrick Bouvier]

On 4/16/24 19:40, Richard Henderson wrote:
> On 4/16/24 17:35, Pierrick Bouvier wrote:
>> On 4/15/24 21:06, Richard Henderson wrote:
>>> Based-on: 20240404230611.21231-1-richard.henderson@linaro.org
>>> ("[PATCH v2 00/21] Rewrite plugin code generation")
>>>
>>> This is an attempt to fix
>>> https://gitlab.com/qemu-project/qemu/-/issues/2208
>>> ("PC is not updated for each instruction in TCG plugins")
>>>
>>> I have only updated target/i386 so far, but basically all targets
>>> need updating for the new callbacks.  Extra points to anyone who
>>> sees how to avoid the extra code duplication.  :-)
>>>
>>
>> Thanks for the series Richard. It looks good to me.
>>
>> Besides reviewing individual commits, I have a more general question.
>>   From some discussions we had, it seems like that previously gdb stub was correctly
>> updating all register values, and that it has been dropped at some point.
> 
> Normally gdbstub does not run in the middle of a TB -- we end normally (single-step,
> breakpoint) or raise an exception (watchpoint).  Only then, after TCG state has been made
> consistent, does gdbstub have access to the CPUState.
>

That makes sense.
  >
>> Was it for performance reasons, or an architectural change in QEMU?
>> Is gdb stub the right way to poke register values for plugins?
>>
>> I don't know exactly why some registers are not updated correctly in this context, but it
>> seems like we are trying to fix this afterward, instead of identifying root cause.
> 
> The one or two registers are not updated continuously for performance reasons.  And
> because they are not updated during initial code generation, it's not easy to do so later
> with plugin injection.  But recovering that data is what the unwind info is for -- a bit
> expensive to access that way, but overall less, with the expectation that it is rare.
> 

Thanks for the description, I understand better the approach you picked 
for that issue.

> 
> r~

Re: [PATCH 1/7] tcg: Introduce INDEX_op_plugin_pc

[Pierrick Bouvier]

On 4/15/24 21:06, Richard Henderson wrote:
> Add an opcode to find a code address within the current insn,
> for later use with unwinding.  Generate the code generically
> using tcg_reg_alloc_do_movi.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/tcg/tcg-op-common.h |  1 +
>   include/tcg/tcg-opc.h       |  1 +
>   tcg/tcg-op.c                |  5 +++++
>   tcg/tcg.c                   | 10 ++++++++++
>   4 files changed, 17 insertions(+)
> 
> diff --git a/include/tcg/tcg-op-common.h b/include/tcg/tcg-op-common.h
> index 009e2778c5..a32c88a182 100644
> --- a/include/tcg/tcg-op-common.h
> +++ b/include/tcg/tcg-op-common.h
> @@ -76,6 +76,7 @@ void tcg_gen_lookup_and_goto_ptr(void);
>   
>   void tcg_gen_plugin_cb(unsigned from);
>   void tcg_gen_plugin_mem_cb(TCGv_i64 addr, unsigned meminfo);
> +void tcg_gen_plugin_pc(TCGv_ptr);
>   
>   /* 32 bit ops */
>   
> diff --git a/include/tcg/tcg-opc.h b/include/tcg/tcg-opc.h
> index 546eb49c11..087d1b82da 100644
> --- a/include/tcg/tcg-opc.h
> +++ b/include/tcg/tcg-opc.h
> @@ -199,6 +199,7 @@ DEF(goto_ptr, 0, 1, 0, TCG_OPF_BB_EXIT | TCG_OPF_BB_END)
>   
>   DEF(plugin_cb, 0, 0, 1, TCG_OPF_NOT_PRESENT)
>   DEF(plugin_mem_cb, 0, 1, 1, TCG_OPF_NOT_PRESENT)
> +DEF(plugin_pc, 1, 0, 0, TCG_OPF_NOT_PRESENT)
>   
>   /* Replicate ld/st ops for 32 and 64-bit guest addresses. */
>   DEF(qemu_ld_a32_i32, 1, 1, 1,
> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
> index eff3728622..b8ca78cbe4 100644
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c
> @@ -322,6 +322,11 @@ void tcg_gen_plugin_mem_cb(TCGv_i64 addr, unsigned meminfo)
>       tcg_gen_op2(INDEX_op_plugin_mem_cb, tcgv_i64_arg(addr), meminfo);
>   }
>   
> +void tcg_gen_plugin_pc(TCGv_ptr arg)
> +{
> +    tcg_gen_op1(INDEX_op_plugin_pc, tcgv_ptr_arg(arg));
> +}
> +
>   /* 32 bit ops */
>   
>   void tcg_gen_discard_i32(TCGv_i32 arg)
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index d248c52e96..42e2b53729 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -4701,6 +4701,13 @@ static void tcg_reg_alloc_mov(TCGContext *s, const TCGOp *op)
>       }
>   }
>   
> +static void tcg_reg_alloc_plugin_pc(TCGContext *s, const TCGOp *op)
> +{
> +    tcg_reg_alloc_do_movi(s, arg_temp(op->args[0]),
> +                          (uintptr_t)tcg_splitwx_to_rx(s->code_ptr),
> +                          op->life, output_pref(op, 0));
> +}
> +
>   /*
>    * Specialized code generation for INDEX_op_dup_vec.
>    */
> @@ -6208,6 +6215,9 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb, uint64_t pc_start)
>           case INDEX_op_mov_vec:
>               tcg_reg_alloc_mov(s, op);
>               break;
> +        case INDEX_op_plugin_pc:
> +            tcg_reg_alloc_plugin_pc(s, op);
> +            break;
>           case INDEX_op_dup_vec:
>               tcg_reg_alloc_dup(s, op);
>               break;

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 2/7] accel/tcg: Set CPUState.plugin_ra before all plugin callbacks

[Pierrick Bouvier]

On 4/15/24 21:06, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/hw/core/cpu.h  |  1 +
>   accel/tcg/plugin-gen.c | 50 +++++++++++++++++++++++++++++++++++++-----
>   2 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 10cd492aff..f4af37c13d 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -350,6 +350,7 @@ typedef union IcountDecr {
>   typedef struct CPUNegativeOffsetState {
>       CPUTLB tlb;
>   #ifdef CONFIG_PLUGIN
> +    uintptr_t plugin_ra;
>       GArray *plugin_mem_cbs;
>   #endif
>       IcountDecr icount_decr;
> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
> index 36e9134a5d..f96b49cce6 100644
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -37,6 +37,12 @@ enum plugin_gen_from {
>       PLUGIN_GEN_AFTER_TB,
>   };
>   
> +enum plugin_gen_ra {
> +    GEN_RA_DONE,
> +    GEN_RA_FROM_TB,
> +    GEN_RA_FROM_INSN,
> +};
> +
>   /* called before finishing a TB with exit_tb, goto_tb or goto_ptr */
>   void plugin_gen_disable_mem_helpers(void)
>   {
> @@ -151,11 +157,38 @@ static void gen_mem_cb(struct qemu_plugin_dyn_cb *cb,
>       tcg_temp_free_i32(cpu_index);
>   }
>   
> -static void inject_cb(struct qemu_plugin_dyn_cb *cb)
> +static void inject_ra(enum plugin_gen_ra *gen_ra)
> +{
> +    TCGv_ptr ra;
> +
> +    switch (*gen_ra) {
> +    case GEN_RA_DONE:
> +        return;
> +    case GEN_RA_FROM_TB:
> +        ra = tcg_constant_ptr(NULL);
> +        break;
> +    case GEN_RA_FROM_INSN:
> +        ra = tcg_temp_ebb_new_ptr();
> +        tcg_gen_plugin_pc(ra);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    tcg_gen_st_ptr(ra, tcg_env,
> +                   offsetof(CPUState, neg.plugin_ra) -
> +                   offsetof(ArchCPU, env));
> +    tcg_temp_free_ptr(ra);
> +    *gen_ra = GEN_RA_DONE;
> +}
> +
> +static void inject_cb(struct qemu_plugin_dyn_cb *cb,
> +                      enum plugin_gen_ra *gen_ra)
>   
>   {
>       switch (cb->type) {
>       case PLUGIN_CB_REGULAR:
> +        inject_ra(gen_ra);
>           gen_udata_cb(cb);
>           break;
>       case PLUGIN_CB_INLINE:
> @@ -167,16 +200,18 @@ static void inject_cb(struct qemu_plugin_dyn_cb *cb)
>   }
>   
>   static void inject_mem_cb(struct qemu_plugin_dyn_cb *cb,
> +                          enum plugin_gen_ra *gen_ra,
>                             enum qemu_plugin_mem_rw rw,
>                             qemu_plugin_meminfo_t meminfo, TCGv_i64 addr)
>   {
>       if (cb->rw & rw) {
>           switch (cb->type) {
>           case PLUGIN_CB_MEM_REGULAR:
> +            inject_ra(gen_ra);
>               gen_mem_cb(cb, meminfo, addr);
>               break;
>           default:
> -            inject_cb(cb);
> +            inject_cb(cb, gen_ra);
>               break;
>           }
>       }
> @@ -186,6 +221,7 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
>   {
>       TCGOp *op, *next;
>       int insn_idx = -1;
> +    enum plugin_gen_ra gen_ra;
>   
>       if (unlikely(qemu_loglevel_mask(LOG_TB_OP_PLUGIN)
>                    && qemu_log_in_addr_range(plugin_tb->vaddr))) {
> @@ -205,10 +241,12 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
>        */
>       memset(tcg_ctx->free_temps, 0, sizeof(tcg_ctx->free_temps));
>   
> +    gen_ra = GEN_RA_FROM_TB;
>       QTAILQ_FOREACH_SAFE(op, &tcg_ctx->ops, link, next) {
>           switch (op->opc) {
>           case INDEX_op_insn_start:
>               insn_idx++;
> +            gen_ra = GEN_RA_FROM_INSN;
>               break;
>   
>           case INDEX_op_plugin_cb:
> @@ -244,7 +282,8 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
>                   cbs = plugin_tb->cbs;
>                   for (i = 0, n = (cbs ? cbs->len : 0); i < n; i++) {
>                       inject_cb(
> -                        &g_array_index(cbs, struct qemu_plugin_dyn_cb, i));
> +                        &g_array_index(cbs, struct qemu_plugin_dyn_cb, i),
> +                        &gen_ra);
>                   }
>                   break;
>   
> @@ -256,7 +295,8 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
>                   cbs = insn->insn_cbs;
>                   for (i = 0, n = (cbs ? cbs->len : 0); i < n; i++) {
>                       inject_cb(
> -                        &g_array_index(cbs, struct qemu_plugin_dyn_cb, i));
> +                        &g_array_index(cbs, struct qemu_plugin_dyn_cb, i),
> +                        &gen_ra);
>                   }
>                   break;
>   
> @@ -288,7 +328,7 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
>               cbs = insn->mem_cbs;
>               for (i = 0, n = (cbs ? cbs->len : 0); i < n; i++) {
>                   inject_mem_cb(&g_array_index(cbs, struct qemu_plugin_dyn_cb, i),
> -                              rw, meminfo, addr);
> +                              &gen_ra, rw, meminfo, addr);
>               }
>   
>               tcg_ctx->emit_before_op = NULL;

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 3/7] accel/tcg: Return the TranslationBlock from cpu_unwind_state_data

[Pierrick Bouvier]

On 4/15/24 21:06, Richard Henderson wrote:
> Fix the i386 get_memio_eip function to use tb->cflags
> instead of cs->tcg_cflags.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/exec/cpu-common.h | 9 +++++----
>   accel/tcg/translate-all.c | 9 +++++----
>   target/i386/helper.c      | 6 ++++--
>   3 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 6346df17ce..f056132cab 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -176,12 +176,13 @@ void list_cpus(void);
>    * @host_pc: the host pc within the translation
>    * @data: output data
>    *
> - * Attempt to load the the unwind state for a host pc occurring in
> - * translated code.  If @host_pc is not in translated code, the
> - * function returns false; otherwise @data is loaded.
> + * Attempt to load the the unwind state for a host pc occurring in translated
> + * code.  If @host_pc is not in translated code, the function returns NULL;
> + * otherwise @data is loaded and the TranslationBlock is returned.
>    * This is the same unwind info as given to restore_state_to_opc.
>    */
> -bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data);
> +const TranslationBlock *cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc,
> +                                              uint64_t *data);
>   
>   /**
>    * cpu_restore_state:
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 83cc14fbde..c745bc5b6c 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -243,15 +243,16 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc)
>       return false;
>   }
>   
> -bool cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data)
> +const TranslationBlock *
> +cpu_unwind_state_data(CPUState *cpu, uintptr_t host_pc, uint64_t *data)
>   {
>       if (in_code_gen_buffer((const void *)(host_pc - tcg_splitwx_diff))) {
>           TranslationBlock *tb = tcg_tb_lookup(host_pc);
> -        if (tb) {
> -            return cpu_unwind_data_from_tb(tb, host_pc, data) >= 0;
> +        if (tb && cpu_unwind_data_from_tb(tb, host_pc, data) >= 0) {
> +            return tb;
>           }
>       }
> -    return false;
> +    return NULL;
>   }
>   
>   void page_init(void)
> diff --git a/target/i386/helper.c b/target/i386/helper.c
> index 23ccb23a5b..eaa691a851 100644
> --- a/target/i386/helper.c
> +++ b/target/i386/helper.c
> @@ -517,13 +517,15 @@ static inline target_ulong get_memio_eip(CPUX86State *env)
>   #ifdef CONFIG_TCG
>       uint64_t data[TARGET_INSN_START_WORDS];
>       CPUState *cs = env_cpu(env);
> +    const TranslationBlock *tb;
>   
> -    if (!cpu_unwind_state_data(cs, cs->mem_io_pc, data)) {
> +    tb = cpu_unwind_state_data(cs, cs->mem_io_pc, data);
> +    if (!tb) {
>           return env->eip;
>       }
>   
>       /* Per x86_restore_state_to_opc. */
> -    if (cs->tcg_cflags & CF_PCREL) {
> +    if (tb->cflags & CF_PCREL) {
>           return (env->eip & TARGET_PAGE_MASK) | data[0];
>       } else {
>           return data[0] - env->segs[R_CS].base;

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 4/7] plugins: Introduce TCGCPUOps callbacks for mid-tb register reads

[Pierrick Bouvier]

On 4/15/24 21:06, Richard Henderson wrote:
> Certain target registers are not updated continuously within
> the translation block.  For normal exception handling we use
> unwind info to re-generate the correct value when required.
> Leverage that same info for reading those registers for plugins.
> 
> All targets will need updating for these new callbacks.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/hw/core/tcg-cpu-ops.h | 13 +++++++++++++
>   plugins/api.c                 | 36 +++++++++++++++++++++++++++++++++--
>   2 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
> index bf8ff8e3ee..e954d83edf 100644
> --- a/include/hw/core/tcg-cpu-ops.h
> +++ b/include/hw/core/tcg-cpu-ops.h
> @@ -49,6 +49,19 @@ struct TCGCPUOps {
>       /** @debug_excp_handler: Callback for handling debug exceptions */
>       void (*debug_excp_handler)(CPUState *cpu);
>   
> +    /**
> +     * @plugin_need_unwind_for_reg:
> +     * True if unwind info needed for reading reg.
> +     */
> +    bool (*plugin_need_unwind_for_reg)(CPUState *cpu, int reg);
> +    /**
> +     * @plugin_unwind_read_reg:
> +     * Like CPUClass.gdb_read_register, but for registers that require
> +     * regeneration using unwind info, like in @restore_state_to_opc.
> +     */
> +    int (*plugin_unwind_read_reg)(CPUState *cpu, GByteArray *buf, int reg,
> +                                  const TranslationBlock *tb,
> +                                  const uint64_t *data);
>   #ifdef NEED_CPU_H
>   #ifdef CONFIG_USER_ONLY
>       /**
> diff --git a/plugins/api.c b/plugins/api.c
> index 3912c9cc8f..3543647a89 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -40,10 +40,12 @@
>   #include "qemu/plugin.h"
>   #include "qemu/log.h"
>   #include "tcg/tcg.h"
> +#include "tcg/insn-start-words.h"
>   #include "exec/exec-all.h"
>   #include "exec/gdbstub.h"
>   #include "exec/ram_addr.h"
>   #include "disas/disas.h"
> +#include "hw/core/tcg-cpu-ops.h"
>   #include "plugin.h"
>   #ifndef CONFIG_USER_ONLY
>   #include "qemu/plugin-memory.h"
> @@ -454,9 +456,39 @@ GArray *qemu_plugin_get_registers(void)
>   
>   int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
>   {
> -    g_assert(current_cpu);
> +    CPUState *cs;
> +    uintptr_t ra;
> +    int regno;
>   
> -    return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg));
> +    assert(current_cpu);
> +    cs = current_cpu;
> +    ra = cs->neg.plugin_ra;
> +    regno = GPOINTER_TO_INT(reg);
> +
> +    /*
> +     * When plugin_ra is 0, we have no unwind info.  This will be true for
> +     * TB callbacks that happen before any insns of the TB have started.
> +     */
> +    if (ra) {
> +        const TCGCPUOps *tcg_ops = cs->cc->tcg_ops;
> +
> +        /*
> +         * For plugins in the middle of the TB, we may need to locate
> +         * and use unwind data to reconstruct a register value.
> +         * Usually this required for the PC, but there may be others.
> +         */
> +        if (tcg_ops->plugin_need_unwind_for_reg &&
> +            tcg_ops->plugin_need_unwind_for_reg(cs, regno)) {
> +            uint64_t data[TARGET_INSN_START_WORDS];
> +            const TranslationBlock *tb;
> +
> +            tb = cpu_unwind_state_data(cs, ra, data);
> +            assert(tb);
> +            return tcg_ops->plugin_unwind_read_reg(cs, buf, regno, tb, data);
> +        }
> +    }
> +
> +    return gdb_read_register(cs, buf, regno);
>   }
>   
>   struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 5/7] target/i386: Split out gdb-internal.h

[Pierrick Bouvier]

On 4/15/24 21:06, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/i386/gdb-internal.h | 65 ++++++++++++++++++++++++++++++++++++++
>   target/i386/gdbstub.c      |  1 +
>   2 files changed, 66 insertions(+)
>   create mode 100644 target/i386/gdb-internal.h
> 
> diff --git a/target/i386/gdb-internal.h b/target/i386/gdb-internal.h
> new file mode 100644
> index 0000000000..7cf4c1a656
> --- /dev/null
> +++ b/target/i386/gdb-internal.h
> @@ -0,0 +1,65 @@
> +/*
> + * x86 gdb server stub
> + *
> + * Copyright (c) 2003-2005 Fabrice Bellard
> + * Copyright (c) 2013 SUSE LINUX Products GmbH
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef I386_GDB_INTERNAL_H
> +#define I386_GDB_INTERNAL_H
> +
> +/*
> + * Keep these in sync with assignment to
> + * gdb_num_core_regs in target/i386/cpu.c
> + * and with the machine description
> + */
> +
> +/*
> + * SEG: 6 segments, plus fs_base, gs_base, kernel_gs_base
> + */
> +
> +/*
> + * general regs ----->  8 or 16
> + */
> +#define IDX_NB_IP       1
> +#define IDX_NB_FLAGS    1
> +#define IDX_NB_SEG      (6 + 3)
> +#define IDX_NB_CTL      6
> +#define IDX_NB_FP       16
> +/*
> + * fpu regs ----------> 8 or 16
> + */
> +#define IDX_NB_MXCSR    1
> +/*
> + *          total ----> 8+1+1+9+6+16+8+1=50 or 16+1+1+9+6+16+16+1=66
> + */
> +
> +#define IDX_IP_REG      CPU_NB_REGS
> +#define IDX_FLAGS_REG   (IDX_IP_REG + IDX_NB_IP)
> +#define IDX_SEG_REGS    (IDX_FLAGS_REG + IDX_NB_FLAGS)
> +#define IDX_CTL_REGS    (IDX_SEG_REGS + IDX_NB_SEG)
> +#define IDX_FP_REGS     (IDX_CTL_REGS + IDX_NB_CTL)
> +#define IDX_XMM_REGS    (IDX_FP_REGS + IDX_NB_FP)
> +#define IDX_MXCSR_REG   (IDX_XMM_REGS + CPU_NB_REGS)
> +
> +#define IDX_CTL_CR0_REG     (IDX_CTL_REGS + 0)
> +#define IDX_CTL_CR2_REG     (IDX_CTL_REGS + 1)
> +#define IDX_CTL_CR3_REG     (IDX_CTL_REGS + 2)
> +#define IDX_CTL_CR4_REG     (IDX_CTL_REGS + 3)
> +#define IDX_CTL_CR8_REG     (IDX_CTL_REGS + 4)
> +#define IDX_CTL_EFER_REG    (IDX_CTL_REGS + 5)
> +
> +#endif
> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
> index ebb000df6a..9662509b82 100644
> --- a/target/i386/gdbstub.c
> +++ b/target/i386/gdbstub.c
> @@ -20,6 +20,7 @@
>   #include "qemu/osdep.h"
>   #include "cpu.h"
>   #include "include/gdbstub/helpers.h"
> +#include "gdb-internal.h"
>   
>   #ifdef TARGET_X86_64
>   static const int gpr_map[16] = {

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 6/7] target/i386: Introduce cpu_compute_eflags_ccop

[Pierrick Bouvier]

On 4/15/24 21:06, Richard Henderson wrote:
> This is a generalization of cpu_compute_eflags, with a dynamic
> value of cc_op, and is thus tcg specific.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/i386/cpu.h           |  2 ++
>   target/i386/tcg/cc_helper.c | 10 ++++++++++
>   2 files changed, 12 insertions(+)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 6b05738079..285f26d99d 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -2379,6 +2379,8 @@ void cpu_x86_inject_mce(Monitor *mon, X86CPU *cpu, int bank,
>   
>   uint32_t cpu_cc_compute_all(CPUX86State *env1);
>   
> +uint32_t cpu_compute_eflags_ccop(CPUX86State *env, CCOp op);
> +
>   static inline uint32_t cpu_compute_eflags(CPUX86State *env)
>   {
>       uint32_t eflags = env->eflags;
> diff --git a/target/i386/tcg/cc_helper.c b/target/i386/tcg/cc_helper.c
> index f76e9cb8cf..8203682ca8 100644
> --- a/target/i386/tcg/cc_helper.c
> +++ b/target/i386/tcg/cc_helper.c
> @@ -225,6 +225,16 @@ uint32_t cpu_cc_compute_all(CPUX86State *env)
>       return helper_cc_compute_all(CC_DST, CC_SRC, CC_SRC2, CC_OP);
>   }
>   
> +uint32_t cpu_compute_eflags_ccop(CPUX86State *env, CCOp op)
> +{
> +    uint32_t eflags;
> +
> +    eflags = helper_cc_compute_all(CC_DST, CC_SRC, CC_SRC2, op);
> +    eflags |= env->df & DF_MASK;
> +    eflags |= env->eflags & ~(VM_MASK | RF_MASK);
> +    return eflags;
> +}
> +
>   target_ulong helper_cc_compute_c(target_ulong dst, target_ulong src1,
>                                    target_ulong src2, int op)
>   {

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 7/7] target/i386: Implement TCGCPUOps for plugin register reads

[Pierrick Bouvier]

On 4/15/24 21:06, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/i386/tcg/tcg-cpu.c | 72 ++++++++++++++++++++++++++++++---------
>   1 file changed, 56 insertions(+), 16 deletions(-)
> 
> diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
> index cca19cd40e..2370053df2 100644
> --- a/target/i386/tcg/tcg-cpu.c
> +++ b/target/i386/tcg/tcg-cpu.c
> @@ -22,9 +22,11 @@
>   #include "helper-tcg.h"
>   #include "qemu/accel.h"
>   #include "hw/core/accel-cpu.h"
> -
> +#include "gdbstub/helpers.h"
> +#include "gdb-internal.h"
>   #include "tcg-cpu.h"
>   
> +
>   /* Frob eflags into and out of the CPU temporary format.  */
>   
>   static void x86_cpu_exec_enter(CPUState *cs)
> @@ -61,38 +63,74 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs,
>       }
>   }
>   
> -static void x86_restore_state_to_opc(CPUState *cs,
> -                                     const TranslationBlock *tb,
> -                                     const uint64_t *data)
> +static uint64_t eip_from_unwind(CPUX86State *env, const TranslationBlock *tb,
> +                                uint64_t data0)
>   {
> -    X86CPU *cpu = X86_CPU(cs);
> -    CPUX86State *env = &cpu->env;
> -    int cc_op = data[1];
>       uint64_t new_pc;
>   
>       if (tb_cflags(tb) & CF_PCREL) {
>           /*
> -         * data[0] in PC-relative TBs is also a linear address, i.e. an address with
> -         * the CS base added, because it is not guaranteed that EIP bits 12 and higher
> -         * stay the same across the translation block.  Add the CS base back before
> -         * replacing the low bits, and subtract it below just like for !CF_PCREL.
> +         * data[0] in PC-relative TBs is also a linear address,
> +         * i.e. an address with the CS base added, because it is
> +         * not guaranteed that EIP bits 12 and higher stay the
> +         * same across the translation block.  Add the CS base
> +         * back before replacing the low bits, and subtract it
> +         * below just like for !CF_PCREL.
>            */
>           uint64_t pc = env->eip + tb->cs_base;
> -        new_pc = (pc & TARGET_PAGE_MASK) | data[0];
> +        new_pc = (pc & TARGET_PAGE_MASK) | data0;
>       } else {
> -        new_pc = data[0];
> +        new_pc = data0;
>       }
>       if (tb->flags & HF_CS64_MASK) {
> -        env->eip = new_pc;
> -    } else {
> -        env->eip = (uint32_t)(new_pc - tb->cs_base);
> +        return new_pc;
>       }
> +    return (uint32_t)(new_pc - tb->cs_base);
> +}
>   
> +static void x86_restore_state_to_opc(CPUState *cs,
> +                                     const TranslationBlock *tb,
> +                                     const uint64_t *data)
> +{
> +    CPUX86State *env = cpu_env(cs);
> +    CCOp cc_op;
> +
> +    env->eip = eip_from_unwind(env, tb, data[0]);
> +
> +    cc_op = data[1];
>       if (cc_op != CC_OP_DYNAMIC) {
>           env->cc_op = cc_op;
>       }
>   }
>   
> +static bool x86_plugin_need_unwind_for_reg(CPUState *cs, int reg)
> +{
> +    return reg == IDX_IP_REG || reg == IDX_FLAGS_REG;
> +}
> +
> +static int x86_plugin_unwind_read_reg(CPUState *cs, GByteArray *buf, int reg,
> +                                      const TranslationBlock *tb,
> +                                      const uint64_t *data)
> +{
> +    CPUX86State *env = cpu_env(cs);
> +    CCOp cc_op;
> +
> +    switch (reg) {
> +    case IDX_IP_REG:
> +        return gdb_get_regl(buf, eip_from_unwind(env, tb, data[0]));
> +
> +    case IDX_FLAGS_REG:
> +        cc_op = data[1];
> +        if (cc_op == CC_OP_DYNAMIC) {
> +            cc_op = env->cc_op;
> +        }
> +        return gdb_get_reg32(buf, cpu_compute_eflags_ccop(env, cc_op));
> +
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
>   #ifndef CONFIG_USER_ONLY
>   static bool x86_debug_check_breakpoint(CPUState *cs)
>   {
> @@ -110,6 +148,8 @@ static const TCGCPUOps x86_tcg_ops = {
>       .initialize = tcg_x86_init,
>       .synchronize_from_tb = x86_cpu_synchronize_from_tb,
>       .restore_state_to_opc = x86_restore_state_to_opc,
> +    .plugin_need_unwind_for_reg = x86_plugin_need_unwind_for_reg,
> +    .plugin_unwind_read_reg = x86_plugin_unwind_read_reg,
>       .cpu_exec_enter = x86_cpu_exec_enter,
>       .cpu_exec_exit = x86_cpu_exec_exit,
>   #ifdef CONFIG_USER_ONLY

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 0/7] plugins: Use unwind info for special gdb registers

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Based-on: 20240404230611.21231-1-richard.henderson@linaro.org
> ("[PATCH v2 00/21] Rewrite plugin code generation")

I'm getting code conflicts w.r.t to the above (which is already merged?)
so it would be helpful to get a re-base.

>
> This is an attempt to fix
> https://gitlab.com/qemu-project/qemu/-/issues/2208
> ("PC is not updated for each instruction in TCG plugins")

The issue raises another question about PCREL support which makes me
wonder if we need to deprecate get_vaddr at translation time and make it
a run time only value?

>
> I have only updated target/i386 so far, but basically all targets
> need updating for the new callbacks.  Extra points to anyone who
> sees how to avoid the extra code duplication.  :-)
>
>
> r~
>
>
> Richard Henderson (7):
>   tcg: Introduce INDEX_op_plugin_pc
>   accel/tcg: Set CPUState.plugin_ra before all plugin callbacks
>   accel/tcg: Return the TranslationBlock from cpu_unwind_state_data
>   plugins: Introduce TCGCPUOps callbacks for mid-tb register reads
>   target/i386: Split out gdb-internal.h
>   target/i386: Introduce cpu_compute_eflags_ccop
>   target/i386: Implement TCGCPUOps for plugin register reads
>
>  include/exec/cpu-common.h     |  9 +++--
>  include/hw/core/cpu.h         |  1 +
>  include/hw/core/tcg-cpu-ops.h | 13 +++++++
>  include/tcg/tcg-op-common.h   |  1 +
>  include/tcg/tcg-opc.h         |  1 +
>  target/i386/cpu.h             |  2 +
>  target/i386/gdb-internal.h    | 65 +++++++++++++++++++++++++++++++
>  accel/tcg/plugin-gen.c        | 50 +++++++++++++++++++++---
>  accel/tcg/translate-all.c     |  9 +++--
>  plugins/api.c                 | 36 +++++++++++++++++-
>  target/i386/gdbstub.c         |  1 +
>  target/i386/helper.c          |  6 ++-
>  target/i386/tcg/cc_helper.c   | 10 +++++
>  target/i386/tcg/tcg-cpu.c     | 72 +++++++++++++++++++++++++++--------
>  tcg/tcg-op.c                  |  5 +++
>  tcg/tcg.c                     | 10 +++++
>  16 files changed, 258 insertions(+), 33 deletions(-)
>  create mode 100644 target/i386/gdb-internal.h

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro