diff mbox series

[PULL,19/34] disas: Use translator_st to get disassembly data

Message ID 20240515075247.68024-20-richard.henderson@linaro.org
State New
Headers show
Series [PULL,01/34] accel/tcg: Use vaddr in translator_ld* | expand

Commit Message

Richard Henderson May 15, 2024, 7:52 a.m. UTC
Read from already translated pages, or saved mmio data.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/disas/disas.h     |  5 +++--
 include/exec/translator.h |  4 ++--
 include/qemu/typedefs.h   |  1 +
 accel/tcg/translator.c    |  2 +-
 disas/disas-common.c      | 14 --------------
 disas/disas-mon.c         | 15 +++++++++++++++
 disas/disas-target.c      | 19 +++++++++++++++++--
 plugins/api.c             |  4 ++--
 8 files changed, 41 insertions(+), 23 deletions(-)

Comments

Bernhard Beschow May 20, 2024, 11:50 a.m. UTC | #1
Am 15. Mai 2024 07:52:32 UTC schrieb Richard Henderson <richard.henderson@linaro.org>:
>Read from already translated pages, or saved mmio data.
>
>Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>---
> include/disas/disas.h     |  5 +++--
> include/exec/translator.h |  4 ++--
> include/qemu/typedefs.h   |  1 +
> accel/tcg/translator.c    |  2 +-
> disas/disas-common.c      | 14 --------------
> disas/disas-mon.c         | 15 +++++++++++++++
> disas/disas-target.c      | 19 +++++++++++++++++--
> plugins/api.c             |  4 ++--
> 8 files changed, 41 insertions(+), 23 deletions(-)

Hi,

this patch unfortunately breaks the "execlog" plugin which doesn't decode the mnemonics correctly any longer. When launching `qemu-system-x86_64 -plugin /path/to/qemu-build/contrib/plugins/libexeclog.so -d plugin`, it outputs either "addb %al, (%bx, %si)" or ".byte 0x00", regardless of which instruction was actually executed. It seems to invoke the disassembler on zero-initialized data. Reverting the patch fixes the problem.

Moreover, patch 11 in this pull request "[PULL 11/34] plugins: Use translator_st for qemu_plugin_insn_data" causes the plugin to not print the correct machine code any longer, instead just printing "0x0". I haven't investigated whether reverting that patch fixes the problem since it doesn't revert cleanly.

It would be nice if somebody could look into this since I'm also trying to hunt down an alignment problem in the ARM emulator introduced in 9.0 which now prevents my guest from booting, and the execlog plugin is one of the tools I use for investigation.

Best regards,
Bernhard
diff mbox series

Patch

diff --git a/include/disas/disas.h b/include/disas/disas.h
index 54a5e68443..c702b1effc 100644
--- a/include/disas/disas.h
+++ b/include/disas/disas.h
@@ -4,14 +4,15 @@ 
 /* Disassemble this for me please... (debugging). */
 #ifdef CONFIG_TCG
 void disas(FILE *out, const void *code, size_t size);
-void target_disas(FILE *out, CPUState *cpu, uint64_t code, size_t size);
+void target_disas(FILE *out, CPUState *cpu, const DisasContextBase *db);
 #endif
 
 void monitor_disas(Monitor *mon, CPUState *cpu, uint64_t pc,
                    int nb_insn, bool is_physical);
 
 #ifdef CONFIG_PLUGIN
-char *plugin_disas(CPUState *cpu, uint64_t addr, size_t size);
+char *plugin_disas(CPUState *cpu, const DisasContextBase *db,
+                   uint64_t addr, size_t size);
 #endif
 
 /* Look up symbol for debugging purpose.  Returns "" if unknown. */
diff --git a/include/exec/translator.h b/include/exec/translator.h
index 31c39ab63c..411ce2b47e 100644
--- a/include/exec/translator.h
+++ b/include/exec/translator.h
@@ -79,7 +79,7 @@  typedef enum DisasJumpType {
  *
  * Architecture-agnostic disassembly context.
  */
-typedef struct DisasContextBase {
+struct DisasContextBase {
     TranslationBlock *tb;
     vaddr pc_first;
     vaddr pc_next;
@@ -103,7 +103,7 @@  typedef struct DisasContextBase {
     int record_start;
     int record_len;
     uint8_t record[32];
-} DisasContextBase;
+};
 
 /**
  * TranslatorOps:
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index b47e7179e2..9d222dc376 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -42,6 +42,7 @@  typedef struct CPUPluginState CPUPluginState;
 typedef struct CPUState CPUState;
 typedef struct DeviceState DeviceState;
 typedef struct DirtyBitmapSnapshot DirtyBitmapSnapshot;
+typedef struct DisasContextBase DisasContextBase;
 typedef struct DisplayChangeListener DisplayChangeListener;
 typedef struct DriveInfo DriveInfo;
 typedef struct DumpState DumpState;
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index ccd22dcd95..00322c6fd9 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -231,7 +231,7 @@  void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
             if (!ops->disas_log ||
                 !ops->disas_log(db, cpu, logfile)) {
                 fprintf(logfile, "IN: %s\n", lookup_symbol(db->pc_first));
-                target_disas(logfile, cpu, db->pc_first, db->tb->size);
+                target_disas(logfile, cpu, db);
             }
             fprintf(logfile, "\n");
             qemu_log_unlock(logfile);
diff --git a/disas/disas-common.c b/disas/disas-common.c
index ce9f82b711..de61f6d8a1 100644
--- a/disas/disas-common.c
+++ b/disas/disas-common.c
@@ -8,25 +8,12 @@ 
 #include "disas/capstone.h"
 #include "hw/core/cpu.h"
 #include "exec/tswap.h"
-#include "exec/memory.h"
 #include "disas-internal.h"
 
 
 /* Filled in by elfload.c.  Simplistic, but will do for now. */
 struct syminfo *syminfos = NULL;
 
-/*
- * Get LENGTH bytes from info's buffer, at target address memaddr.
- * Transfer them to myaddr.
- */
-static int target_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length,
-                              struct disassemble_info *info)
-{
-    CPUDebug *s = container_of(info, CPUDebug, info);
-    int r = cpu_memory_rw_debug(s->cpu, memaddr, myaddr, length, 0);
-    return r ? EIO : 0;
-}
-
 /*
  * Print an error message.  We can assume that this is in response to
  * an error return from {host,target}_read_memory.
@@ -73,7 +60,6 @@  void disas_initialize_debug_target(CPUDebug *s, CPUState *cpu)
     disas_initialize_debug(s);
 
     s->cpu = cpu;
-    s->info.read_memory_func = target_read_memory;
     s->info.print_address_func = print_address;
     if (target_words_bigendian()) {
         s->info.endian = BFD_ENDIAN_BIG;
diff --git a/disas/disas-mon.c b/disas/disas-mon.c
index 5d6d9aa02d..37bf16ac79 100644
--- a/disas/disas-mon.c
+++ b/disas/disas-mon.c
@@ -11,6 +11,19 @@ 
 #include "hw/core/cpu.h"
 #include "monitor/monitor.h"
 
+/*
+ * Get LENGTH bytes from info's buffer, at target address memaddr.
+ * Transfer them to myaddr.
+ */
+static int
+virtual_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length,
+                    struct disassemble_info *info)
+{
+    CPUDebug *s = container_of(info, CPUDebug, info);
+    int r = cpu_memory_rw_debug(s->cpu, memaddr, myaddr, length, 0);
+    return r ? EIO : 0;
+}
+
 static int
 physical_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length,
                      struct disassemble_info *info)
@@ -38,6 +51,8 @@  void monitor_disas(Monitor *mon, CPUState *cpu, uint64_t pc,
 
     if (is_physical) {
         s.info.read_memory_func = physical_read_memory;
+    } else {
+        s.info.read_memory_func = virtual_read_memory;
     }
     s.info.buffer_vma = pc;
 
diff --git a/disas/disas-target.c b/disas/disas-target.c
index 82313b2a67..48f3a365dc 100644
--- a/disas/disas-target.c
+++ b/disas/disas-target.c
@@ -6,16 +6,28 @@ 
 #include "qemu/osdep.h"
 #include "disas/disas.h"
 #include "disas/capstone.h"
+#include "exec/translator.h"
 #include "disas-internal.h"
 
 
-void target_disas(FILE *out, CPUState *cpu, uint64_t code, size_t size)
+static int translator_read_memory(bfd_vma memaddr, bfd_byte *myaddr,
+                                  int length, struct disassemble_info *info)
 {
+    const DisasContextBase *db = info->application_data;
+    return translator_st(db, myaddr, memaddr, length) ? 0 : EIO;
+}
+
+void target_disas(FILE *out, CPUState *cpu, const struct DisasContextBase *db)
+{
+    uint64_t code = db->pc_first;
+    size_t size = translator_st_len(db);
     uint64_t pc;
     int count;
     CPUDebug s;
 
     disas_initialize_debug_target(&s, cpu);
+    s.info.read_memory_func = translator_read_memory;
+    s.info.application_data = (void *)db;
     s.info.fprintf_func = fprintf;
     s.info.stream = out;
     s.info.buffer_vma = code;
@@ -58,12 +70,15 @@  static void plugin_print_address(bfd_vma addr, struct disassemble_info *info)
  * there is left over it usually indicates the front end has read more
  * bytes than it needed.
  */
-char *plugin_disas(CPUState *cpu, uint64_t addr, size_t size)
+char *plugin_disas(CPUState *cpu, const DisasContextBase *db,
+                   uint64_t addr, size_t size)
 {
     CPUDebug s;
     GString *ds = g_string_new(NULL);
 
     disas_initialize_debug_target(&s, cpu);
+    s.info.read_memory_func = translator_read_memory;
+    s.info.application_data = (void *)db;
     s.info.fprintf_func = disas_gstring_printf;
     s.info.stream = (FILE *)ds;  /* abuse this slot */
     s.info.buffer_vma = addr;
diff --git a/plugins/api.c b/plugins/api.c
index 02014d4c6e..b04c5e1928 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -271,8 +271,8 @@  void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn)
 
 char *qemu_plugin_insn_disas(const struct qemu_plugin_insn *insn)
 {
-    CPUState *cpu = current_cpu;
-    return plugin_disas(cpu, insn->vaddr, insn->len);
+    return plugin_disas(tcg_ctx->cpu, tcg_ctx->plugin_db,
+                        insn->vaddr, insn->len);
 }
 
 const char *qemu_plugin_insn_symbol(const struct qemu_plugin_insn *insn)