Pierrick Bouvier <pierrick.bouv...@linaro.org> writes: > Hi Rowan, thanks for your contribution. > > To give some context on the answer, we are currently working to add a > similar "read_memory" API, but associated to memory callbacks for > plugins > (https://lore.kernel.org/qemu-devel/20240724194708.1843704-1-pierrick.bouv...@linaro.org/T/#t). > > A key aspect of what you propose here, is that the memory may have > changed during the write time, and when you read it, while what we > propose guarantees to track every change correctly. > > It's not a bad thing, and both API are definitely complementary. When > talking to Alex, he was keen to add a global read_memory API (like you > propose), after we merge the first one. > > @Alex: any thought on this?
I'd like to get the memory callback API merged first - mostly because that is the API that will absolutely reflect what was loaded or stored to a given memory location. For precise instrumentation that is the one to use. However I agree the ability to read the state of memory outside of loads and stores is useful especially for something like this. It's not unreasonable to assume the memory state of arguments going into a syscall isn't being messed with and it is easier to track pointers and strings with a more general purpose API. > > Regarding your patch, it would be much easier if you could split that > in different commits. Adding API first, then modify each plugin in a > different commit. This way, it would be easier to review. 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). > > On 8/21/24 16:56, Rowan Hart wrote: >> Signed-off-by: Rowan Hart <rowanbh...@gmail.com> >> --- >> docs/about/emulation.rst | 16 ++++- >> include/qemu/qemu-plugin.h | 24 +++++++- >> plugins/api.c | 21 +++++++ >> plugins/qemu-plugins.symbols | 1 + >> tests/tcg/plugins/mem.c | 37 +++++++++++- >> tests/tcg/plugins/syscall.c | 113 +++++++++++++++++++++++++++++++++++ >> 6 files changed, 208 insertions(+), 4 deletions(-) >> diff --git a/docs/about/emulation.rst b/docs/about/emulation.rst >> index eea1261baa..9c68e37887 100644 >> --- a/docs/about/emulation.rst >> +++ b/docs/about/emulation.rst >> @@ -354,6 +354,8 @@ Behaviour can be tweaked with the following arguments: >> - Use callbacks on each memory instrumentation. >> * - hwaddr=true|false >> - Count IO accesses (only for system emulation) >> + * - read=true|false >> + - Read the memory content of each access and display it >> System Calls >> ............ >> @@ -388,6 +390,19 @@ run:: >> 160 1 0 >> 135 1 0 >> +Behaviour can be tweaked with the following arguments: >> + >> +.. list-table:: Syscall plugin arguments >> + :widths: 20 80 >> + :header-rows: 1 >> + >> + * - Option >> + - Description >> + * - print=true|false >> + - Print the number of times each syscall is called >> + * - log_writes=true|false >> + - Log the buffer of each write syscall in hexdump format >> + >> Test inline operations >> ...................... >> @@ -777,4 +792,3 @@ Other emulation features >> When running system emulation you can also enable deterministic >> execution which allows for repeatable record/replay debugging. See >> :ref:`Record/Replay<replay>` for more details. >> - >> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h >> index c71c705b69..d4ec73574b 100644 >> --- a/include/qemu/qemu-plugin.h >> +++ b/include/qemu/qemu-plugin.h >> @@ -57,11 +57,19 @@ typedef uint64_t qemu_plugin_id_t; >> * - Remove qemu_plugin_register_vcpu_{tb, insn, mem}_exec_inline. >> * Those functions are replaced by *_per_vcpu variants, which guarantee >> * thread-safety for operations. >> + * >> + * version 3: >> + * - modified arguments and return value of qemu_plugin_insn_data to copy >> + * the data into a user-provided buffer instead of returning a pointer >> + * to the data. >> + * > > Is it a change left on your side, or a bad diff? > >> + * version 4: >> + * - added qemu_plugin_read_memory_vaddr >> */ >> extern QEMU_PLUGIN_EXPORT int qemu_plugin_version; >> -#define QEMU_PLUGIN_VERSION 3 >> +#define QEMU_PLUGIN_VERSION 4 >> /** >> * struct qemu_info_t - system information for plugins >> @@ -852,6 +860,20 @@ typedef struct { >> QEMU_PLUGIN_API >> GArray *qemu_plugin_get_registers(void); >> +/** >> + * qemu_plugin_read_memory_vaddr() - read from memory using a virtual >> address >> + * >> + * @addr: A virtual address to read from >> + * @len: The number of bytes to read, starting from @addr >> + * >> + * Returns a GByteArray with the read memory. Ownership of the GByteArray is >> + * transferred to the caller, which is responsible for deallocating it after >> + * use. On failure returns NULL. We should definitely point out the pitfalls w.r.t callbacks here. >> + */ >> +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). The registers case certainly does it this way so it makes sense to be consistent. It also allows the plugins to re-use buffers if it wants and makes managing lifetime a bit easier. <snip> >> } else { >> fprintf(stderr, "unsupported argument: %s\n", argv[i]); >> return -1; >> @@ -137,6 +234,22 @@ QEMU_PLUGIN_EXPORT int >> qemu_plugin_install(qemu_plugin_id_t id, >> statistics = g_hash_table_new_full(NULL, g_direct_equal, NULL, >> g_free); >> } >> + if (do_log_writes) { >> + for (const struct SyscallInfo *syscall_info = arch_syscall_info; >> + syscall_info->name != NULL; syscall_info++) { >> + >> + if (g_strcmp0(syscall_info->name, info->target_name) == 0) { >> + write_sysno = syscall_info->write_sysno; >> + break; >> + } >> + } >> + >> + if (write_sysno == -1) { >> + fprintf(stderr, "write syscall number not found\n"); >> + return -1; >> + } >> + } >> + > > It's good! I appreciate to see this feature. > >> qemu_plugin_register_vcpu_syscall_cb(id, vcpu_syscall); >> qemu_plugin_register_vcpu_syscall_ret_cb(id, vcpu_syscall_ret); >> qemu_plugin_register_atexit_cb(id, plugin_exit, NULL); There was someone on IRC looking to trace system calls in system mode (by tracking the syscall instruction and reading the registers at the time). I wonder if we could make this plugin do the right thing in both modes? > > Thanks, > Pierrick -- Alex Bennée Virtualisation Tech Lead @ Linaro
