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

[Rowan Hart <rowanbhart@gmail.com>]

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