Re: [PATCH v2] TCG plugin API extension to read guest memory content by an address

[Alex Bennée]

Mikhail Tyutin <m.tyutin@yadro.com> writes:

> TCG plugin API extension to read guest memory content. 
> qemu_plugin_vcpu_read_phys_mem()
>
> function can be used by TCG plugin inside of qemu_plugin_vcpu_mem_cb_t 
> callback to adjust
>
> received address according to internal memory mappings and read content of 
> guest memory.
>
> Works for both user-level and system-level emulation modes.
>
>  
>
> What's new in v2:
>
> * Increment QEMU_PLUGIN_VERSION instead of custom define
>
> * Example of qemu_plugin_vcpu_read_phys_mem() usage in memtrace.c
>
> * Use cpu_memory_rw_debug() to get memory content in both user-level and
>
>   system-level emulation modes
>
>  
>
> Signed-off-by: Mikhail Tyutin <m.tyutin@yadro.com>
>
> Signed-off-by: Aleksey Titov <a.titov@yadro.com>

Not sure what happened with the formatting of this patch, I think there
is an html part getting in the way.

>
> ---
>
> contrib/plugins/Makefile     |  1 +
>
> contrib/plugins/memtrace.c   | 76 ++++++++++++++++++++++++++++++++++++
>
> include/qemu/qemu-plugin.h   | 18 ++++++++-
>
> plugins/api.c                | 16 ++++++++
>
> plugins/qemu-plugins.symbols |  1 +
>
> 5 files changed, 111 insertions(+), 1 deletion(-)
>
> create mode 100644 contrib/plugins/memtrace.c
>
>  
>
> diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
>
> index 23e0396687..cf27554616 100644
>
> --- a/contrib/plugins/Makefile
>
> +++ b/contrib/plugins/Makefile
>
> @@ -21,6 +21,7 @@ NAMES += lockstep
>
> NAMES += hwprofile
>
> NAMES += cache
>
> NAMES += drcov
>
> +NAMES += memtrace
>
>  SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>
> diff --git a/contrib/plugins/memtrace.c b/contrib/plugins/memtrace.c
>
> new file mode 100644
>
> index 0000000000..16c1553f47
>
> --- /dev/null
>
> +++ b/contrib/plugins/memtrace.c
>
> @@ -0,0 +1,76 @@
>
> +/*
>
> + * Copyright (C) 2023, Mikhail Tyutin <m.tyutin@yadro.com>
>
> + *
>
> + * Log all memory accesses including content of the access.
>
> + * 
>
> + * License: GNU GPL, version 2 or later.
>
> + *   See the COPYING file in the top-level directory.
>
> + */
>
> +
>
> +#include <glib.h>
>
> +
>
> +#include <qemu-plugin.h>
>
> +
>
> +
>
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>
> +
>
> +
>
> +static void vcpu_mem_access(uint32_t vcpuIndex, qemu_plugin_meminfo_t 
> memInfo,
>
> +                            uint64_t vaddr, void *userdata)
>
> +{
>
> +    unsigned size = 1 << qemu_plugin_mem_size_shift(memInfo);
>
> +    char* memContent = g_malloc(size);
>
> +    unsigned i;
>
> +    GString* logLine = g_string_new(NULL);
>
> +
>
> +    qemu_plugin_vcpu_read_phys_mem(vcpuIndex, vaddr, memContent,
> size);

So the problem with this approach is the memory value you read here may not be
the same as the value that was read by the instruction. This could
because of a few reasons:

  - an mmio write changes underlying memory layout
  - another thread changes memory after the access

I think a better way to get this information would be to register a new
type of call-back which can duplicate the value in the store/load and
pass it directly to the callback. It might even be worth just fixing up
the existing callback and breaking compatibility rather than having two
callback types?

We didn't do this originally as we were being cautious about any
attempts to use plugins to workaround the GPL for doing HW emulation -
however I don't think adding the memory values to the callbacks greatly
increases that risk.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro