qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] plugins: add plugin API to read guest memory

From: Pierrick Bouvier
Subject: Re: [PATCH] plugins: add plugin API to read guest memory
Date: Mon, 26 Aug 2024 12:11:08 -0700
User-agent: Mozilla Thunderbird 
On 8/26/24 11:47, Rowan Hart wrote:

Alex & Pierrick,

Thank you for the feedback! This is my first contribution to QEMU, so I'm glad
it at least passes the initial smell test :)



Sure, no worries, you did well!


I'll make my comments in this patch, but for v2, please split those individual
commits, and a cover letter, describing your changes (https://github.com/
stefanha/git-publish is a great tool if you want to easily push series).


Will do! Mailing list dev is new to me but I will do a practice run to try and
not mess it up.
If you already configured git send-email, git-publish just does all the rest for you (collecting reviewers, prepare patches, and send using git send-email). In more, it tracks you different versions, so it's pretty easy to iterate on a series. 





+QEMU_PLUGIN_API
+GByteArray *qemu_plugin_read_memory_vaddr(uint64_t addr,
+                                          size_t len);
+


IMHO, it would be better to pass the buffer as a parameter, instead of 
allocating a new one everytime.

bool qemu_plugin_read_memory_vaddr(uint64_t addr, GByteArray *buf, size_t len).


Sure, this makes perfect sense. I considered both and picked wrong so not hard
to change.


   /**
    * qemu_plugin_read_register() - read register for current vCPU
    *
diff --git a/plugins/api.c b/plugins/api.c
index 2ff13d09de..f210ca166a 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -527,6 +527,27 @@ GArray *qemu_plugin_get_registers(void)
       return create_register_handles(regs);
   }
   +GByteArray *qemu_plugin_read_memory_vaddr(vaddr addr, size_t len)
+{
+    g_assert(current_cpu);
+
+    if (len == 0) {
+        return NULL;
+    }
+
+    GByteArray *buf = g_byte_array_sized_new(len);
+    g_byte_array_set_size(buf, len);
+
+    int result = cpu_memory_rw_debug(current_cpu, addr, buf->data, buf->len, 
0);
+
+    if (result < 0) {
+        g_byte_array_unref(buf);
+        return NULL;
+    }
+
+    return buf;
+}
+
   int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray 
*buf)
   {
       g_assert(current_cpu);
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index ca773d8d9f..3ad479a924 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -20,6 +20,7 @@
     qemu_plugin_num_vcpus;
     qemu_plugin_outs;
     qemu_plugin_path_to_binary;
+  qemu_plugin_read_memory_vaddr;
     qemu_plugin_read_register;
     qemu_plugin_register_atexit_cb;
     qemu_plugin_register_flush_cb;
diff --git a/tests/tcg/plugins/mem.c b/tests/tcg/plugins/mem.c
index b650dddcce..c65d48680b 100644
--- a/tests/tcg/plugins/mem.c
+++ b/tests/tcg/plugins/mem.c
@@ -24,7 +24,7 @@ typedef struct {
   static struct qemu_plugin_scoreboard *counts;
   static qemu_plugin_u64 mem_count;
   static qemu_plugin_u64 io_count;
-static bool do_inline, do_callback;
+static bool do_inline, do_callback, do_read;
   static bool do_haddr;
   static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
   @@ -58,6 +58,30 @@ static void vcpu_mem(unsigned int cpu_index, 
qemu_plugin_meminfo_t meminfo,
       } else {
           qemu_plugin_u64_add(mem_count, cpu_index, 1);
       }
+
+    if (do_read) {
+        size_t size = qemu_plugin_mem_size_shift(meminfo);
+        GByteArray *data = qemu_plugin_read_memory_vaddr(vaddr, size);
+
+        if (data) {
+            g_autoptr(GString) out = g_string_new("");
+
+            if (qemu_plugin_mem_is_store(meminfo)) {
+                g_string_append(out, "store: ");
+            } else {
+                g_string_append(out, "load: ");
+            }
+
+            g_string_append_printf(out, "vaddr: 0x%" PRIx64 " data: 0x",
+                                   vaddr);
+            for (size_t i = 0; i < data->len; i++) {
+                g_string_append_printf(out, "%02x", data->data[i]);
+            }
+            g_string_append(out, "\n");
+            qemu_plugin_outs(out->str);
+            g_byte_array_free(data, TRUE);
+        }
+    }


See other series, merging an API for getting values read on a memory access. 
It's a better fit to implement this. So I think it's better to drop this plugin 
modification.


Ok, will drop this one and keep the modification to the syscalls plugin only as
an example of how to use the API.



Good, thanks!


   }
     static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
@@ -86,7 +110,6 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t 
id,
                                              const qemu_info_t *info,
                                              int argc, char **argv)
   {
-
       for (int i = 0; i < argc; i++) {
           char *opt = argv[i];
           g_auto(GStrv) tokens = g_strsplit(opt, "=", 2);
@@ -117,6 +140,11 @@ QEMU_PLUGIN_EXPORT int 
qemu_plugin_install(qemu_plugin_id_t id,
                   fprintf(stderr, "boolean argument parsing failed: %s\n", 
opt);
                   return -1;
               }
+        } else if (g_strcmp0(tokens[0], "read") == 0) {
+            if (!qemu_plugin_bool_parse(tokens[0], tokens[1], &do_read)) {
+                fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
+                return -1;
+            }
           } else {
               fprintf(stderr, "option parsing failed: %s\n", opt);
               return -1;
@@ -129,6 +157,11 @@ QEMU_PLUGIN_EXPORT int 
qemu_plugin_install(qemu_plugin_id_t id,
           return -1;
       }
   +    if (do_read && !do_callback) {
+        fprintf(stderr, "can't enable memory reading without callback\n");
+        return -1;
+    }
+
       counts = qemu_plugin_scoreboard_new(sizeof(CPUCount));
       mem_count = qemu_plugin_scoreboard_u64_in_struct(
           counts, CPUCount, mem_count);
diff --git a/tests/tcg/plugins/syscall.c b/tests/tcg/plugins/syscall.c
index 72e1a5bf90..67c7c404c4 100644
--- a/tests/tcg/plugins/syscall.c
+++ b/tests/tcg/plugins/syscall.c
@@ -22,8 +22,56 @@ typedef struct {
       int64_t errors;
   } SyscallStats;
   +struct SyscallInfo {
+    const char *name;
+    int64_t write_sysno;
+};
+
+const struct SyscallInfo arch_syscall_info[] = {
+    { "aarch64", 64 },
+    { "aarch64_be", 64 },
+    { "alpha", 4 },
+    { "arm", 4 },
+    { "armeb", 4 },
+    { "avr", -1 },
+    { "cris", -1 },
+    { "hexagon", 64 },
+    { "hppa", -1 },
+    { "i386", 4 },
+    { "loongarch64", -1 },
+    { "m68k", 4 },
+    { "microblaze", 4 },
+    { "microblazeel", 4 },
+    { "mips", 1 },
+    { "mips64", 1 },
+    { "mips64el", 1 },
+    { "mipsel", 1 },
+    { "mipsn32", 1 },
+    { "mipsn32el", 1 },
+    { "or1k", -1 },
+    { "ppc", 4 },
+    { "ppc64", 4 },
+    { "ppc64le", 4 },
+    { "riscv32", 64 },
+    { "riscv64", 64 },
+    { "rx", -1 },
+    { "s390x", -1 },
+    { "sh4", -1 },
+    { "sh4eb", -1 },
+    { "sparc", 4 },
+    { "sparc32plus", 4 },
+    { "sparc64", 4 },
+    { "tricore", -1 },
+    { "x86_64", 1 },
+    { "xtensa", 13 },
+    { "xtensaeb", 13 },
+    { NULL, -1 },
+};
+
   static GMutex lock;
   static GHashTable *statistics;
+static bool do_log_writes;
+static int64_t write_sysno = -1;
     static SyscallStats *get_or_create_entry(int64_t num)
   {
@@ -54,6 +102,51 @@ static void vcpu_syscall(qemu_plugin_id_t id, unsigned int 
vcpu_index,
           g_autofree gchar *out = g_strdup_printf("syscall #%" PRIi64 "\n", 
num);
           qemu_plugin_outs(out);
       }
+
+    if (do_log_writes && num == write_sysno) {
+        GByteArray *data = qemu_plugin_read_memory_vaddr(a2, a3);
+
+        g_autoptr(GString) out = g_string_new("");
+
+        size_t rows = (data->len % 16 == 0)
+            ? (data->len / 16)
+            : ((data->len / 16) + 1);
+        for (size_t row = 0; row < rows; row++) {
+            size_t len = (rows != data->len / 16 && row == rows - 1)
+                ? (data->len % 16)
+                : 16;
+            for (size_t i = 0; i < len; i++) {
+                g_string_append_printf(out, "%02x ",
+                    data->data[(row * 16) + i]);
+                if (i != 0 && i % 4 == 0) {
+                    g_string_append(out, " ");
+                }
+            }
+            for (size_t i = len; i < 16; i++) {
+                g_string_append(out, "   ");
+                if (i != 0 && i % 4 == 0) {
+                    g_string_append(out, " ");
+                }
+            }
+            g_string_append(out, " | ");
+            for (size_t i = 0; i < len; i++) {
+                g_string_append_printf(out,
+                    (data->data[(row * 16) + i] >= 0x21
+                        && data->data[(row * 16) + i] <= 0x7e)
+                    ? "%c"
+                    : ".", data->data[(row * 16) + i]);
+                if (i != 0 && i % 4 == 0) {
+                    g_string_append(out, " ");
+                }
+            }
+            g_string_append(out, "\n");
+        }


Could you add a comment to show what is expected format? From the code, with 
all these loops and ternary expressions, it's not easy to follow.


Indeed. I will just generally simplify this

Thanks!



You're welcome.


-Rowan


Pierrick
reply via email to
[Prev in Thread] Current Thread [Next in Thread]