Re: [PATCH v1 11/14] plugins: expand kernel-doc for instruction query and instrumentation

[Aaron Lindsay]

A few clarifications inline:

On Mar 12 17:28, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/qemu/qemu-plugin.h | 52 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index dc05fc1932..d4adce730a 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -327,21 +327,69 @@ void qemu_plugin_register_vcpu_insn_exec_inline(struct 
> qemu_plugin_insn *insn,
>                                                  enum qemu_plugin_op op,
>                                                  void *ptr, uint64_t imm);
>  
> -/*
> - * Helpers to query information about the instructions in a block
> +/**
> + * qemu_plugin_tb_n_insns() - query helper for number of insns in TB
> + * @tb: opaque handle to TB passed to callback
> + *
> + * Returns: number of instructions in this block
>   */
>  size_t qemu_plugin_tb_n_insns(const struct qemu_plugin_tb *tb);
>  
> +/**
> + * qemu_plugin_tb_vaddr() - query helper for vaddr of TB start
> + * @tb: opaque handle to TB passed to callback
> + *
> + * Returns: virtual address of block start
> + */
>  uint64_t qemu_plugin_tb_vaddr(const struct qemu_plugin_tb *tb);
>  
> +/**
> + * qemu_plugin_tb_get_insn() - retrieve handle for instruction
> + * @tb: opaque handle to TB passed to callback
> + * @idx: instruction number, 0 indexed
> + *
> + * The returned handle can be used in follow up helper queries as well
> + * as when instrumenting an instruction. It is only valid for the
> + * lifetime of the callback.
> + *
> + * Returns: opaque handle to instruction
> + */
>  struct qemu_plugin_insn *
>  qemu_plugin_tb_get_insn(const struct qemu_plugin_tb *tb, size_t idx);
>  
> +/**
> + * qemu_plugin_insn_data() - return ptr to instruction data
> + * @insn: opaque instruction handle from qemu_plugin_tb_get_insn()
> + *
> + * Note: data is only valid for duration of callback. See
> + * qemu_plugin_insn_size() to calculate size of stream.
> + *
> + * Returns: pointer to a stream of bytes

Maybe this could be a little more explicit, something like "Returns:
pointer to a stream of bytes containing the value of this instruction's
opcode"?

> + */
>  const void *qemu_plugin_insn_data(const struct qemu_plugin_insn *insn);
>  
> +/**
> + * qemu_plugin_insn_size() - return size of instruction
> + * @insn: opaque instruction handle from qemu_plugin_tb_get_insn()
> + *
> + * Returns: size of instruction

size in bytes?

> + */
>  size_t qemu_plugin_insn_size(const struct qemu_plugin_insn *insn);
>  
> +/**
> + * qemu_plugin_insn_vaddr() - return vaddr of instruction
> + * @insn: opaque instruction handle from qemu_plugin_tb_get_insn()
> + *
> + * Returns: virtual address of instruction
> + */
>  uint64_t qemu_plugin_insn_vaddr(const struct qemu_plugin_insn *insn);
> +
> +/**
> + * qemu_plugin_insn_haddr() - return vaddr of instruction

Copypasta: s/vaddr/haddr/ ?

> + * @insn: opaque instruction handle from qemu_plugin_tb_get_insn()
> + *
> + * Returns: hardware (physical) address of instruction
> + */
>  void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn);

Is this the physical address of the instruction on the host or target?

-Aaron