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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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