diff mbox series

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

Message ID 20210312172821.31647-12-alex.bennee@linaro.org
State Superseded
Headers show
Series plugins/next (phys addr, syscalls, lots of docs) | expand

Commit Message

Alex Bennée March 12, 2021, 5:28 p.m. UTC
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
 include/qemu/qemu-plugin.h | 52 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

-- 
2.20.1

Comments

Xingtao Yao (Fujitsu)" via March 12, 2021, 6:36 p.m. UTC | #1
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
Alex Bennée March 16, 2021, 1:48 p.m. UTC | #2
Aaron Lindsay <aaron@os.amperecomputing.com> writes:

> 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?


target.

>

> -Aaron



-- 
Alex Bennée
Xingtao Yao (Fujitsu)" via March 16, 2021, 2 p.m. UTC | #3
On Mar 16 13:48, Alex Bennée wrote:
> Aaron Lindsay <aaron@os.amperecomputing.com> writes:

> > On Mar 12 17:28, Alex Bennée wrote:

> >> + * @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?

> 

> target.


An observation: We're exposing a target physical address here as a void
* and for memory accesses (qemu_plugin_hwaddr_phys_addr) as a uint64_t.

-Aaron
diff mbox series

Patch

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
+ */
 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_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
+ * @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);
 
 /*