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

[Rowan Hart]

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