Re: [RFC PATCH 1/1] QEMU plugin interface extension

["Alex Bennée" <alex.bennee@linaro.org>]

Florian Hauschild <florian.hauschild@fs.ei.tum.de> writes:

> This extension covers functions:
>   * to read and write guest memory
>   * to read and write guest registers
>   * to flush tb cache
>   * to control single stepping of qemu from plugin

Next time please split the functionality into separate patches to aid
review.

>
> These changes allow the user to
>   * collect more information about the behaviour of the system

More introspection into guest state is a welcome improvement.

>   * change the guest state with a plugin during execution

I have no in principle objection to this but the wider community may
disagree. We are wary of plugins being used for GPL avoidance reasons.

>   * control cache of tcg

Why? From the plugins point of view the internal state of the translator
should be irrelevant. If it's not it's a bug.

>   * allow for precise instrumentation in execution flow

More precise than every instruction and memory access? How exactly?

>
> Signed-off-by: Florian Hauschild <florian.hauschild@fs.ei.tum.de>
> ---
>  include/qemu/qemu-plugin.h   |  35 ++++++++++++
>  plugins/meson.build          |   1 +
>  plugins/readwriteextension.c | 106 +++++++++++++++++++++++++++++++++++
>  3 files changed, 142 insertions(+)
>  create mode 100644 plugins/readwriteextension.c
>
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index e6e815abc5..c7a0c5f379 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -577,4 +577,39 @@ int qemu_plugin_n_max_vcpus(void);
>   */
>  void qemu_plugin_outs(const char *string);
>  
> +
> +/**
> + * read_reg() read a register
> + * @reg: Number of the register
> + *
> + * Returns the value of the register
> + */
> +uint64_t read_reg(int reg);
> +
> +/**
> + * write_reg() - write to a register
> + * @reg: number of the register
> + * @val: value written to register
> + */
> +void write_reg(int reg, uint64_t val);
> +
> +/**
> + * plugin_flush_tb() - Flush the tb cache
> + */
> +void plugin_flush_tb(void);
> +
> +/**
> + * plugin_rw_memory_cpu() - Function to read from and write to a guest address.
> + * @address: baseaddress of the memory section
> + * @buffer: buffer managed by caller the value should be written to
> + * @buf_size: size of the buffer and memory size read/written.
> + * @write: 1 if write, 0 if read
> + */
> +int plugin_rw_memory_cpu(uint64_t address, uint8_t buffer[], size_t buf_size, char write);
> +
> +/**
> + * plugin_single_step() - Function to change single step behaviour from the plugin.
> + */
> +void plugin_single_step(int enable);
> +
>  #endif /* QEMU_PLUGIN_API_H */
> diff --git a/plugins/meson.build b/plugins/meson.build
> index e77723010e..b95cbab0b1 100644
> --- a/plugins/meson.build
> +++ b/plugins/meson.build
> @@ -10,4 +10,5 @@ specific_ss.add(when: 'CONFIG_PLUGIN', if_true: [files(
>    'loader.c',
>    'core.c',
>    'api.c',
> +  'readwriteextension.c',
>  ), declare_dependency(link_args: plugin_ldflags)])
> diff --git a/plugins/readwriteextension.c b/plugins/readwriteextension.c
> new file mode 100644
> index 0000000000..47460c396f
> --- /dev/null
> +++ b/plugins/readwriteextension.c
> @@ -0,0 +1,106 @@
> +/**
> + * QEMU Plugin read write extension code
> + *
> + * This is the code that allows the plugin to read and write
> + * memory and registers and flush the tb cache. Also allows
> + * to set QEMU into singlestep mode from Plugin.
> + *
> + * Based on plugin interface:
> + * Copyright (C) 2017, Emilio G. Cota <cota@braap.org>
> + * Copyright (C) 2019, Linaro
> + *
> + * Copyright (C) 2021 Florian Hauschild <florian.hauschild@tum.de>
> + *
> + * License: GNU GPL, version 2 or later.
> + *   See the COPYING file in the top-level directory.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +
> +
> +#include "qemu/osdep.h"
> +#include "qemu/plugin.h"
> +#include "hw/core/cpu.h"
> +#include "cpu.h"
> +#include "exec/exec-all.h"
> +
> +void plugin_async_flush_tb(CPUState *cpu, run_on_cpu_data arg);
> +void plugin_async_flush_tb(CPUState *cpu, run_on_cpu_data arg)
> +{
> +    g_assert(cpu_in_exclusive_context(cpu));
> +    tb_flush(cpu);
> +}
> +
> +
> +
> +int plugin_rw_memory_cpu(uint64_t address, uint8_t buffer[], size_t buf_size, char write)
> +{
> +    return cpu_memory_rw_debug(current_cpu, address, buffer, buf_size, write);

Accessing memory during a plugin event is tricky. There is no way to
know if memory has changed underneath you. Would it instead be more
useful to derive the access from instrumented instructions? That way you
can know the exact value read or written at that exact moment in time.

> +}
> +
> +
> +void plugin_flush_tb(void)
> +{
> +    async_safe_run_on_cpu(current_cpu, plugin_async_flush_tb, RUN_ON_CPU_NULL);
> +}
> +
> +static int plugin_read_register(CPUState *cpu, GByteArray *buf, int reg)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    if (reg < cc->gdb_num_core_regs) {
> +        return cc->gdb_read_register(cpu, buf, reg);
> +    }
> +    return 0;
> +}

I'm not super keen on exposing gdb register id's to the plugins. For one
thing they can be dynamic and there is no way for the plugins to know
what the mapping is. I'd rather abstract the registers in a way that can
be cleanly used by HMP, logging and plugins.

> +
> +uint64_t read_reg(int reg)
> +{
> +    GByteArray *val = g_byte_array_new();
> +    uint64_t reg_ret = 0;
> +    int ret_bytes = plugin_read_register(current_cpu, val, reg);
> +    if (ret_bytes == 1) {
> +        reg_ret = val->data[0];
> +    }
> +    if (ret_bytes == 2) {
> +        reg_ret = *(uint16_t *) &(val->data[0]);
> +    }
> +    if (ret_bytes == 4) {
> +        reg_ret = *(uint32_t *) &(val->data[0]);
> +    }
> +    if (ret_bytes == 8) {
> +        reg_ret = *(uint64_t *) &(val->data[0]);
> +    }
> +    return reg_ret;
> +}

There are larger registers than 8 bytes - for example the various vector
implementations. They need to be accessible via this interface.

> +
> +void write_reg(int reg, uint64_t val)
> +{
> +    CPUState *cpu = current_cpu;
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (reg < cc->gdb_num_core_regs) {
> +        cc->gdb_write_register(cpu, (uint8_t *) &val, reg);
> +    }
> +}
> +
> +void plugin_single_step(int enable)
> +{
> +    /* singlestep is set in softmmu/vl.c*/
> +    static int orig_value;
> +    static int executed = 1;
> +
> +    if (unlikely(executed == 1)) {
> +        orig_value = singlestep;
> +        executed = 2;
> +    }
> +
> +    if (enable == 1) {
> +        singlestep = 1;
> +    } else {
> +        singlestep = orig_value;
> +    }
> +
> +    tb_flush(current_cpu);
> +}

Again why? What does this gain over instrumenting instructions themselves?

-- 
Alex Bennée