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 :)
> 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. >> +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. >> } >> 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! -Rowan
