Re: [PATCH 1/1] plugins: add API to read guest CPU memory from hwaddr

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

Rowan Hart <rowanbhart@gmail.com> writes:

Apologies for the delay, I realise this has been sitting on my review
queue too long.

> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
> ---
>  include/qemu/qemu-plugin.h   | 22 ++++++++++++++++++++++
>  plugins/api.c                | 17 +++++++++++++++++
>  plugins/qemu-plugins.symbols |  2 ++
>  3 files changed, 41 insertions(+)
>
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index c71c705b69..25f39c0960 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -868,6 +868,28 @@ QEMU_PLUGIN_API
>  int qemu_plugin_read_register(struct qemu_plugin_register *handle,
>                                GByteArray *buf);
>  
> +/**
> + * qemu_plugin_read_cpu_memory_hwaddr() - read CPU memory from hwaddr
> + *
> + * @addr: A virtual address to read from

A physical address

> + * @data: A byte array to store data into
> + * @len: The number of bytes to read, starting from @addr
> + *
> + * @len bytes of data is read starting at @addr and stored into @data. If @data
> + * is not large enough to hold @len bytes, it will be expanded to the necessary
> + * size, reallocating if necessary. @len must be greater than 0.
> + *
> + * This function does not ensure writes are flushed prior to reading, so
> + * callers should take care when calling this function in plugin callbacks to
> + * avoid attempting to read data which may not yet be written and should use
> + * the memory callback API instead.

Maybe we should be clear this can only be called from a vCPU context?
See bellow.

> + *
> + * Returns true on success and false on failure.
> + */
> +QEMU_PLUGIN_API
> +bool qemu_plugin_read_cpu_memory_hwaddr(uint64_t addr,
> +                                          GByteArray *data, size_t len);
> +
>  /**
>   * qemu_plugin_scoreboard_new() - alloc a new scoreboard
>   *
> diff --git a/plugins/api.c b/plugins/api.c
> index 2ff13d09de..c87bed6641 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -527,6 +527,22 @@ GArray *qemu_plugin_get_registers(void)
>      return create_register_handles(regs);
>  }
>  
> +bool qemu_plugin_read_cpu_memory_hwaddr(uint64_t addr,
> +                                        GByteArray *data, uint64_t len)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    if (len == 0) {
> +        return false;
> +    }
> +
> +    g_byte_array_set_size(data, len);
> +    cpu_physical_memory_rw(addr, (void *)data->data, len, 0);

One concern here is that cpu_physical_memory_* swallows any error
conditions so the user might not get what they are expecting even if we
return true here.

It would be safer I think to use address_space_read_full with
&address_space_memory and MEMTXATTRS_UNSPECIFIED and check the result so
we can signal to the user when it fails.

However that does gloss over some details because you can have multiple
address spaces. As each vCPU can potentially have its own maybe we want:

  g_assert(current_cpu);
  res = address_space_read_full(current_cpu->as, addr, attrs, buf, len);
  return res == MEMTX_OK ? true : false;

However even that elides a complexity because a CPU can have multiple
AddressSpaces depending on what mode it is in.

For example Arm can have normal, secure and potentially normal and
secure versions of tag memory. Currently we have no way of exposing that
information to plugins (and maybe we don't want to, currently
arm_addressspace() is only used for internal page table walking and some
stack stuff).

x86 has two potential address spaces with the extra one being used for
I think System Management Mode.


> +    return true;
> +#else
> +    return false;
> +#endif
> +}
> +
>  int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
>  {
>      g_assert(current_cpu);
> @@ -534,6 +550,7 @@ int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
>      return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1);
>  }
>  
> +
>  struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size)
>  {
>      return plugin_scoreboard_new(element_size);
> diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
> index ca773d8d9f..5d9cfd71bb 100644
> --- a/plugins/qemu-plugins.symbols
> +++ b/plugins/qemu-plugins.symbols
> @@ -20,6 +20,8 @@
>    qemu_plugin_num_vcpus;
>    qemu_plugin_outs;
>    qemu_plugin_path_to_binary;
> +  qemu_plugin_read_cpu_memory_hwaddr;
> +  qemu_plugin_read_io_memory_hwaddr;
>    qemu_plugin_read_register;
>    qemu_plugin_register_atexit_cb;
>    qemu_plugin_register_flush_cb;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro