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

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

[Richard Henderson]

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,arm} 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 (9):
  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
  target/arm: Add aarch64_tcg_ops
  target/arm: Implement TCGCPUOps for plugin register reads

 include/exec/cpu-common.h     |  9 +++--
 include/hw/core/cpu.h         |  4 +-
 include/hw/core/tcg-cpu-ops.h | 14 +++++++
 include/tcg/tcg-op-common.h   |  1 +
 include/tcg/tcg-opc.h         |  1 +
 target/arm/internals.h        |  8 +++-
 target/i386/cpu.h             |  2 +
 target/i386/gdb-internal.h    | 65 +++++++++++++++++++++++++++++++
 accel/tcg/plugin-gen.c        | 49 +++++++++++++++++++++---
 accel/tcg/translate-all.c     |  9 +++--
 plugins/api.c                 | 36 +++++++++++++++++-
 target/arm/cpu.c              | 40 ++++++++++++++++++-
 target/arm/cpu64.c            | 55 ++++++++++++++++++++++++++
 target/arm/tcg/cpu-v7m.c      |  2 +
 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 +++++
 20 files changed, 360 insertions(+), 39 deletions(-)
 create mode 100644 target/i386/gdb-internal.h

-- 
2.34.1

[PATCH v2 1/9] 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.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
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 34e3056380..b7c28d92a6 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -4689,6 +4689,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.
  */
@@ -6196,6 +6203,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

Re: [PATCH v2 1/9] tcg: Introduce INDEX_op_plugin_pc

[Alex Bennée]

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

> 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.
>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Richard Henderson]

Store a host code address to use with the tcg unwinder when called
from a plugin.  Generate one such store per guest insn that uses
a plugin callback.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/hw/core/cpu.h  |  4 +---
 accel/tcg/plugin-gen.c | 49 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index a2c8536943..19b7fcc9f3 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -354,9 +354,7 @@ typedef union IcountDecr {
 typedef struct CPUNegativeOffsetState {
     CPUTLB tlb;
 #ifdef CONFIG_PLUGIN
-    /*
-     * The callback pointer are accessed via TCG (see gen_empty_mem_helper).
-     */
+    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 cc1634e7a6..650e3810e6 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)
 {
@@ -213,11 +219,37 @@ static void gen_mem_cb(struct qemu_plugin_regular_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->regular);
         break;
     case PLUGIN_CB_COND:
@@ -235,19 +267,21 @@ 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)
 {
     switch (cb->type) {
     case PLUGIN_CB_MEM_REGULAR:
         if (rw && cb->regular.rw) {
+            inject_ra(gen_ra);
             gen_mem_cb(&cb->regular, meminfo, addr);
         }
         break;
     case PLUGIN_CB_INLINE_ADD_U64:
     case PLUGIN_CB_INLINE_STORE_U64:
         if (rw && cb->inline_insn.rw) {
-            inject_cb(cb);
+            inject_cb(cb, gen_ra);
         }
         break;
     default:
@@ -260,6 +294,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(tcg_ctx->plugin_db->pc_first))) {
@@ -279,10 +314,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:
@@ -318,7 +355,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;
 
@@ -330,7 +368,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;
 
@@ -362,7 +401,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

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

[Alex Bennée]

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

> Store a host code address to use with the tcg unwinder when called
> from a plugin.  Generate one such store per guest insn that uses
> a plugin callback.
>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Richard Henderson]

Adjust the i386 get_memio_eip function to use tb->cflags instead
of tcg_cflags_has, which is technically more correct.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
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 815342d043..c1887462e6 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -189,12 +189,13 @@ uint32_t curr_cflags(CPUState *cpu);
  * @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 fdf6d8ac19..45a1cf57bc 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 f9d1381f90..565e01a3a9 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -521,13 +521,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 (tcg_cflags_has(cs, 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

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

[Alex Bennée]

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

> Adjust the i386 get_memio_eip function to use tb->cflags instead
> of tcg_cflags_has, which is technically more correct.
>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 4/9] 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.

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

diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
index 099de3375e..b34f999e78 100644
--- a/include/hw/core/tcg-cpu-ops.h
+++ b/include/hw/core/tcg-cpu-ops.h
@@ -53,6 +53,20 @@ 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 CONFIG_USER_ONLY
     /**
      * @fake_user_interrupt: Callback for 'fake exception' handling.
diff --git a/plugins/api.c b/plugins/api.c
index 5a0a7f8c71..53127ed9ee 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/translator.h"
 #include "disas/disas.h"
+#include "hw/core/tcg-cpu-ops.h"
 #include "plugin.h"
 #ifndef CONFIG_USER_ONLY
 #include "exec/ram_addr.h"
@@ -526,9 +528,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

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

[Alex Bennée]

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

> 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.
>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> 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.
>>
>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

I'll note there is a minor merge conflict coming against:

  plugins: Ensure register handles are not NULL

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Richard Henderson]

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
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 4acf485879..96b4382a5d 100644
--- a/target/i386/gdbstub.c
+++ b/target/i386/gdbstub.c
@@ -20,6 +20,7 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "gdbstub/helpers.h"
+#include "gdb-internal.h"
 
 #ifdef TARGET_X86_64
 static const int gpr_map[16] = {
-- 
2.34.1

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

[Philippe Mathieu-Daudé]

On 6/6/24 05:29, Richard Henderson wrote:
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> 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 4acf485879..96b4382a5d 100644
> --- a/target/i386/gdbstub.c
> +++ b/target/i386/gdbstub.c
> @@ -20,6 +20,7 @@
>   #include "qemu/osdep.h"
>   #include "cpu.h"
>   #include "gdbstub/helpers.h"
> +#include "gdb-internal.h"
>   
>   #ifdef TARGET_X86_64
>   static const int gpr_map[16] = {

Shouldn't we remove the definitions from the source to
complete the "split"?

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

[Richard Henderson]

On 6/5/24 23:51, Philippe Mathieu-Daudé wrote:
> Shouldn't we remove the definitions from the source to
> complete the "split"?

Gah, I thought I had done that.


r~

[PATCH v2 6/9] 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.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
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 c64ef0c1a2..48ad6f495b 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2431,6 +2431,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 v2 7/9] target/i386: Implement TCGCPUOps for plugin register reads

[Richard Henderson]

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
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 v2 7/9] target/i386: Implement TCGCPUOps for plugin register reads

[Alex Bennée]

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

> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 8/9] target/arm: Add aarch64_tcg_ops

[Richard Henderson]

For the moment, this is an exact copy of arm_tcg_ops.
Export arm_cpu_exec_interrupt for the cross-file reference.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h |  1 +
 target/arm/cpu.c       |  2 +-
 target/arm/cpu64.c     | 30 ++++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 11b5da2562..dc53d86249 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -364,6 +364,7 @@ void arm_restore_state_to_opc(CPUState *cs,
 
 #ifdef CONFIG_TCG
 void arm_cpu_synchronize_from_tb(CPUState *cs, const TranslationBlock *tb);
+bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request);
 #endif /* CONFIG_TCG */
 
 typedef enum ARMFPRounding {
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 35fa281f1b..3cd4711064 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -824,7 +824,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
     return unmasked || pstate_unmasked;
 }
 
-static bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
+bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     CPUClass *cc = CPU_GET_CLASS(cs);
     CPUARMState *env = cpu_env(cs);
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 262a1d6c0b..7ba80099af 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -31,6 +31,9 @@
 #include "hvf_arm.h"
 #include "qapi/visitor.h"
 #include "hw/qdev-properties.h"
+#ifdef CONFIG_TCG
+#include "hw/core/tcg-cpu-ops.h"
+#endif
 #include "internals.h"
 #include "cpu-features.h"
 #include "cpregs.h"
@@ -793,6 +796,29 @@ static const gchar *aarch64_gdb_arch_name(CPUState *cs)
     return "aarch64";
 }
 
+#ifdef CONFIG_TCG
+static const TCGCPUOps aarch64_tcg_ops = {
+    .initialize = arm_translate_init,
+    .synchronize_from_tb = arm_cpu_synchronize_from_tb,
+    .debug_excp_handler = arm_debug_excp_handler,
+    .restore_state_to_opc = arm_restore_state_to_opc,
+
+#ifdef CONFIG_USER_ONLY
+    .record_sigsegv = arm_cpu_record_sigsegv,
+    .record_sigbus = arm_cpu_record_sigbus,
+#else
+    .tlb_fill = arm_cpu_tlb_fill,
+    .cpu_exec_interrupt = arm_cpu_exec_interrupt,
+    .do_interrupt = arm_cpu_do_interrupt,
+    .do_transaction_failed = arm_cpu_do_transaction_failed,
+    .do_unaligned_access = arm_cpu_do_unaligned_access,
+    .adjust_watchpoint_address = arm_adjust_watchpoint_address,
+    .debug_check_watchpoint = arm_debug_check_watchpoint,
+    .debug_check_breakpoint = arm_debug_check_breakpoint,
+#endif /* !CONFIG_USER_ONLY */
+};
+#endif /* CONFIG_TCG */
+
 static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
 {
     CPUClass *cc = CPU_CLASS(oc);
@@ -802,6 +828,10 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_core_xml_file = "aarch64-core.xml";
     cc->gdb_arch_name = aarch64_gdb_arch_name;
 
+#ifdef CONFIG_TCG
+    cc->tcg_ops = &aarch64_tcg_ops;
+#endif
+
     object_class_property_add_bool(oc, "aarch64", aarch64_cpu_get_aarch64,
                                    aarch64_cpu_set_aarch64);
     object_class_property_set_description(oc, "aarch64",
-- 
2.34.1

Re: [PATCH v2 8/9] target/arm: Add aarch64_tcg_ops

[Alex Bennée]

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

> For the moment, this is an exact copy of arm_tcg_ops.
> Export arm_cpu_exec_interrupt for the cross-file reference.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/internals.h |  1 +
>  target/arm/cpu.c       |  2 +-
>  target/arm/cpu64.c     | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 11b5da2562..dc53d86249 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -364,6 +364,7 @@ void arm_restore_state_to_opc(CPUState *cs,
>  
>  #ifdef CONFIG_TCG
>  void arm_cpu_synchronize_from_tb(CPUState *cs, const TranslationBlock *tb);
> +bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request);
>  #endif /* CONFIG_TCG */
>  
>  typedef enum ARMFPRounding {
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 35fa281f1b..3cd4711064 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -824,7 +824,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
>      return unmasked || pstate_unmasked;
>  }
>  
> -static bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> +bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cs);
>      CPUARMState *env = cpu_env(cs);
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 262a1d6c0b..7ba80099af 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -31,6 +31,9 @@
>  #include "hvf_arm.h"
>  #include "qapi/visitor.h"
>  #include "hw/qdev-properties.h"
> +#ifdef CONFIG_TCG
> +#include "hw/core/tcg-cpu-ops.h"
> +#endif
>  #include "internals.h"
>  #include "cpu-features.h"
>  #include "cpregs.h"
> @@ -793,6 +796,29 @@ static const gchar *aarch64_gdb_arch_name(CPUState *cs)
>      return "aarch64";
>  }
>  
> +#ifdef CONFIG_TCG
> +static const TCGCPUOps aarch64_tcg_ops = {
> +    .initialize = arm_translate_init,
> +    .synchronize_from_tb = arm_cpu_synchronize_from_tb,
> +    .debug_excp_handler = arm_debug_excp_handler,
> +    .restore_state_to_opc = arm_restore_state_to_opc,
> +
> +#ifdef CONFIG_USER_ONLY
> +    .record_sigsegv = arm_cpu_record_sigsegv,
> +    .record_sigbus = arm_cpu_record_sigbus,
> +#else
> +    .tlb_fill = arm_cpu_tlb_fill,
> +    .cpu_exec_interrupt = arm_cpu_exec_interrupt,
> +    .do_interrupt = arm_cpu_do_interrupt,
> +    .do_transaction_failed = arm_cpu_do_transaction_failed,
> +    .do_unaligned_access = arm_cpu_do_unaligned_access,
> +    .adjust_watchpoint_address = arm_adjust_watchpoint_address,
> +    .debug_check_watchpoint = arm_debug_check_watchpoint,
> +    .debug_check_breakpoint = arm_debug_check_breakpoint,
> +#endif /* !CONFIG_USER_ONLY */
> +};
> +#endif /* CONFIG_TCG */
> +

My principle concern is duplicating an otherwise identical structure
just gives us more opportunity to miss a change. 

>  static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      CPUClass *cc = CPU_CLASS(oc);
> @@ -802,6 +828,10 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_core_xml_file = "aarch64-core.xml";
>      cc->gdb_arch_name = aarch64_gdb_arch_name;
>  
> +#ifdef CONFIG_TCG
> +    cc->tcg_ops = &aarch64_tcg_ops;
> +#endif
> +

What happens when the CPU is running mixed mode code and jumping between
64 and 32 bit? Wouldn't it be easier to have a helper that routes to the
correct unwinder, c.f. gen_intermediate_code

  #ifdef TARGET_AARCH64
      if (EX_TBFLAG_ANY(tb_flags, AARCH64_STATE)) {
          ops = &aarch64_translator_ops;
      }
  #endif

which I guess for a runtime helper would be:

   if (is_a64(cpu_env(cs))) {
      aarch64_plugin_need_unwind_for_reg(...);
   } else {
      arm_plugin_need_unwind_for_reg(...);
   }


etc...


>      object_class_property_add_bool(oc, "aarch64", aarch64_cpu_get_aarch64,
>                                     aarch64_cpu_set_aarch64);
>      object_class_property_set_description(oc, "aarch64",

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 8/9] target/arm: Add aarch64_tcg_ops

[Richard Henderson]

On 6/12/24 07:36, Alex Bennée wrote:
> What happens when the CPU is running mixed mode code and jumping between
> 64 and 32 bit? Wouldn't it be easier to have a helper that routes to the
> correct unwinder, c.f. gen_intermediate_code

GDB can't switch modes, so there is *never* any mode switching.


r~

Re: [PATCH v2 8/9] target/arm: Add aarch64_tcg_ops

[Alex Bennée]

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

> On 6/12/24 07:36, Alex Bennée wrote:
>> What happens when the CPU is running mixed mode code and jumping between
>> 64 and 32 bit? Wouldn't it be easier to have a helper that routes to the
>> correct unwinder, c.f. gen_intermediate_code
>
> GDB can't switch modes, so there is *never* any mode switching.

Hmm but code can. We do want to solve the gdb mode switching case as
well although that requires some work on the gdbstub.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 9/9] target/arm: Implement TCGCPUOps for plugin register reads

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h   |  7 +++++--
 target/arm/cpu.c         | 38 ++++++++++++++++++++++++++++++++++++++
 target/arm/cpu64.c       | 25 +++++++++++++++++++++++++
 target/arm/tcg/cpu-v7m.c |  2 ++
 4 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index dc53d86249..fe28937515 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -358,11 +358,14 @@ void init_cpreg_list(ARMCPU *cpu);
 void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu);
 void arm_translate_init(void);
 
+#ifdef CONFIG_TCG
 void arm_restore_state_to_opc(CPUState *cs,
                               const TranslationBlock *tb,
                               const uint64_t *data);
-
-#ifdef CONFIG_TCG
+bool arm_plugin_need_unwind_for_reg(CPUState *cs, int reg);
+int arm_plugin_unwind_read_reg(CPUState *cs, GByteArray *buf, int reg,
+                               const TranslationBlock *tb,
+                               const uint64_t *data);
 void arm_cpu_synchronize_from_tb(CPUState *cs, const TranslationBlock *tb);
 bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request);
 #endif /* CONFIG_TCG */
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 3cd4711064..e8ac3da351 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -29,6 +29,7 @@
 #include "cpu.h"
 #ifdef CONFIG_TCG
 #include "hw/core/tcg-cpu-ops.h"
+#include "gdbstub/helpers.h"
 #endif /* CONFIG_TCG */
 #include "internals.h"
 #include "cpu-features.h"
@@ -120,6 +121,41 @@ void arm_restore_state_to_opc(CPUState *cs,
         env->exception.syndrome = data[2] << ARM_INSN_START_WORD2_SHIFT;
     }
 }
+
+bool arm_plugin_need_unwind_for_reg(CPUState *cs, int reg)
+{
+    return reg == 15 || reg == 25; /* pc (r15) or cpsr */
+}
+
+int arm_plugin_unwind_read_reg(CPUState *cs, GByteArray *buf, int reg,
+                               const TranslationBlock *tb,
+                               const uint64_t *data)
+{
+    CPUARMState *env = cpu_env(cs);
+    uint32_t val, condexec;
+
+    switch (reg) {
+    case 15: /* PC */
+        val = data[0];
+        if (tb_cflags(tb) & CF_PCREL) {
+            val |= env->regs[15] & TARGET_PAGE_MASK;
+        }
+        break;
+    case 25: /* CPSR, or XPSR for M-profile */
+        if (arm_feature(env, ARM_FEATURE_M)) {
+            val = xpsr_read(env);
+        } else {
+            val = cpsr_read(env);
+        }
+        condexec = data[1] & 0xff;
+        val = (val & ~(3 << 25)) | ((condexec & 3) << 25);
+        val = (val & ~(0xfc << 8)) | ((condexec & 0xfc) << 8);
+        break;
+    default:
+        g_assert_not_reached();
+    }
+    return gdb_get_reg32(buf, val);
+}
 #endif /* CONFIG_TCG */
 
 /*
@@ -2657,6 +2693,8 @@ static const TCGCPUOps arm_tcg_ops = {
     .synchronize_from_tb = arm_cpu_synchronize_from_tb,
     .debug_excp_handler = arm_debug_excp_handler,
     .restore_state_to_opc = arm_restore_state_to_opc,
+    .plugin_need_unwind_for_reg = arm_plugin_need_unwind_for_reg,
+    .plugin_unwind_read_reg = arm_plugin_unwind_read_reg,
 
 #ifdef CONFIG_USER_ONLY
     .record_sigsegv = arm_cpu_record_sigsegv,
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 7ba80099af..1595be5d8f 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -33,6 +33,8 @@
 #include "hw/qdev-properties.h"
 #ifdef CONFIG_TCG
 #include "hw/core/tcg-cpu-ops.h"
+#include "exec/translation-block.h"
+#include "gdbstub/helpers.h"
 #endif
 #include "internals.h"
 #include "cpu-features.h"
@@ -797,11 +799,34 @@ static const gchar *aarch64_gdb_arch_name(CPUState *cs)
 }
 
 #ifdef CONFIG_TCG
+static bool aarch64_plugin_need_unwind_for_reg(CPUState *cs, int reg)
+{
+    return reg == 32; /* pc */
+}
+
+static int aarch64_plugin_unwind_read_reg(CPUState *cs, GByteArray *buf,
+                                          int reg, const TranslationBlock *tb,
+                                          const uint64_t *data)
+{
+    CPUARMState *env = cpu_env(cs);
+    uint64_t val;
+
+    assert(reg == 32);
+
+    val = data[0];
+    if (tb_cflags(tb) & CF_PCREL) {
+        val |= env->pc & TARGET_PAGE_MASK;
+    }
+    return gdb_get_reg64(buf, val);
+}
+
 static const TCGCPUOps aarch64_tcg_ops = {
     .initialize = arm_translate_init,
     .synchronize_from_tb = arm_cpu_synchronize_from_tb,
     .debug_excp_handler = arm_debug_excp_handler,
     .restore_state_to_opc = arm_restore_state_to_opc,
+    .plugin_need_unwind_for_reg = aarch64_plugin_need_unwind_for_reg,
+    .plugin_unwind_read_reg = aarch64_plugin_unwind_read_reg,
 
 #ifdef CONFIG_USER_ONLY
     .record_sigsegv = arm_cpu_record_sigsegv,
diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c
index c059c681e9..47e44f70c7 100644
--- a/target/arm/tcg/cpu-v7m.c
+++ b/target/arm/tcg/cpu-v7m.c
@@ -237,6 +237,8 @@ static const TCGCPUOps arm_v7m_tcg_ops = {
     .synchronize_from_tb = arm_cpu_synchronize_from_tb,
     .debug_excp_handler = arm_debug_excp_handler,
     .restore_state_to_opc = arm_restore_state_to_opc,
+    .plugin_need_unwind_for_reg = arm_plugin_need_unwind_for_reg,
+    .plugin_unwind_read_reg = arm_plugin_unwind_read_reg,
 
 #ifdef CONFIG_USER_ONLY
     .record_sigsegv = arm_cpu_record_sigsegv,
-- 
2.34.1

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

[Philippe Mathieu-Daudé]

On 6/6/24 05:29, Richard Henderson wrote:
> 
> 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,arm} 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.  :-)

Do you mean on ARM, aarch64_tcg_ops i.e.? Because X86 LGTM.

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

[Alex Bennée]

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

> 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,arm} 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.  :-)

I've made a few comments but yeah I think we just have to live with the
extra helpers. The only other option would be pre-notifying the gdb
subsystem about which registers are "lazy" which I think amounts to the
same thing.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro