[PATCH v1 00/14] plugins/next (phys addr, syscalls, lots of docs)

[PATCH v1 00/14] plugins/next (phys addr, syscalls, lots of docs)

[Alex Bennée]

Hi,

This is rolling up the plugins changes for 6.0. A couple of fixes and
I've also spent some time cleaning up the kernel-doc comments for the
developer manual.

The following need review:

  - plugins: expand kernel-doc for memory query and instrumentation
  - plugins: expand kernel-doc for instruction query and instrumentation
  - plugins: expand inline exec kernel-doc documentation.
  - plugins: add qemu_plugin_id_t to kernel-doc
  - plugins: add qemu_plugin_cb_flags to kernel-doc
  - plugins: expand the typedef kernel-docs for translation
  - plugins: expand the callback typedef kernel-docs
  - plugins: cleanup kernel-doc for qemu_plugin_install
  - plugins: expand kernel-doc for qemu_info_t
  - docs/devel: include the plugin API information from the headers


Aaron Lindsay (1):
  plugins: Expose physical addresses instead of device offsets

Alex Bennée (10):
  docs/devel: include the plugin API information from the headers
  plugins: expand kernel-doc for qemu_info_t
  plugins: cleanup kernel-doc for qemu_plugin_install
  plugins: expand the callback typedef kernel-docs
  plugins: expand the typedef kernel-docs for translation
  plugins: add qemu_plugin_cb_flags to kernel-doc
  plugins: add qemu_plugin_id_t to kernel-doc
  plugins: expand inline exec kernel-doc documentation.
  plugins: expand kernel-doc for instruction query and instrumentation
  plugins: expand kernel-doc for memory query and instrumentation

Matthias Weckbecker (1):
  plugins: new syscalls plugin

Yonggang Luo (2):
  plugins: getting qemu_plugin_get_hwaddr only expose one function
    prototype
  plugins: Fixes typo in qemu-plugin.h

 docs/devel/tcg-plugins.rst  |   5 +
 include/qemu/qemu-plugin.h  | 230 +++++++++++++++++++++++++++++-------
 contrib/plugins/hotpages.c  |   2 +-
 contrib/plugins/hwprofile.c |   2 +-
 plugins/api.c               |  25 ++--
 tests/plugin/syscall.c      |  49 ++++++++
 tests/plugin/meson.build    |   2 +-
 7 files changed, 257 insertions(+), 58 deletions(-)
 create mode 100644 tests/plugin/syscall.c

-- 
2.20.1

[PATCH v1 01/14] plugins: new syscalls plugin

[Alex Bennée]

From: Matthias Weckbecker <matthias@weckbecker.name>

This commit adds a new syscalls plugin that displays the syscalls
as they are executed and returned. This plugin outputs the number
of the syscall as well as the syscall return value.

Works in *-user only.

Essentially, this commit restores:

  https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg00846.html

by using the new QEMU plugin API.

Signed-off-by: Matthias Weckbecker <matthias@weckbecker.name>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200812115816.4454-1-matthias@weckbecker.name>
---
 tests/plugin/syscall.c   | 49 ++++++++++++++++++++++++++++++++++++++++
 tests/plugin/meson.build |  2 +-
 2 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 tests/plugin/syscall.c

diff --git a/tests/plugin/syscall.c b/tests/plugin/syscall.c
new file mode 100644
index 0000000000..53ee2ab6c4
--- /dev/null
+++ b/tests/plugin/syscall.c
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2020, Matthias Weckbecker <matthias@weckbecker.name>
+ *
+ * License: GNU GPL, version 2 or later.
+ *   See the COPYING file in the top-level directory.
+ */
+#include <inttypes.h>
+#include <assert.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <glib.h>
+
+#include <qemu-plugin.h>
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+static void vcpu_syscall(qemu_plugin_id_t id, unsigned int vcpu_index,
+                         int64_t num, uint64_t a1, uint64_t a2,
+                         uint64_t a3, uint64_t a4, uint64_t a5,
+                         uint64_t a6, uint64_t a7, uint64_t a8)
+{
+    g_autofree gchar *out = g_strdup_printf("syscall #%" PRIi64 "\n", num);
+    qemu_plugin_outs(out);
+}
+
+static void vcpu_syscall_ret(qemu_plugin_id_t id, unsigned int vcpu_idx,
+                             int64_t num, int64_t ret)
+{
+    g_autofree gchar *out;
+    out = g_strdup_printf("syscall #%" PRIi64 " returned -> %" PRIi64 "\n",
+            num, ret);
+    qemu_plugin_outs(out);
+}
+
+/* ************************************************************************* */
+
+static void plugin_exit(qemu_plugin_id_t id, void *p) {}
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
+                                           const qemu_info_t *info,
+                                           int argc, char **argv)
+{
+    qemu_plugin_register_vcpu_syscall_cb(id, vcpu_syscall);
+    qemu_plugin_register_vcpu_syscall_ret_cb(id, vcpu_syscall_ret);
+    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
+    return 0;
+}
diff --git a/tests/plugin/meson.build b/tests/plugin/meson.build
index 1eacfa6e35..2bbfc4b19e 100644
--- a/tests/plugin/meson.build
+++ b/tests/plugin/meson.build
@@ -1,5 +1,5 @@
 t = []
-foreach i : ['bb', 'empty', 'insn', 'mem']
+foreach i : ['bb', 'empty', 'insn', 'mem', 'syscall']
   t += shared_module(i, files(i + '.c'),
                      include_directories: '../../include/qemu',
                      dependencies: glib)
-- 
2.20.1

[PATCH v1 02/14] plugins: Expose physical addresses instead of device offsets

[Alex Bennée]

From: Aaron Lindsay <aaron@os.amperecomputing.com>

This allows plugins to query for full virtual-to-physical address
translation for a given `qemu_plugin_hwaddr` and stops exposing the
offset within the device itself. As this change breaks the API,
QEMU_PLUGIN_VERSION is incremented.

Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20210309202802.211756-1-aaron@os.amperecomputing.com>
---
 include/qemu/qemu-plugin.h  | 32 +++++++++++++++++++++++++-------
 contrib/plugins/hotpages.c  |  2 +-
 contrib/plugins/hwprofile.c |  2 +-
 plugins/api.c               | 17 ++++++++++++-----
 4 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index c66507fe8f..3303dce862 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -47,7 +47,7 @@ typedef uint64_t qemu_plugin_id_t;
 
 extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
 
-#define QEMU_PLUGIN_VERSION 0
+#define QEMU_PLUGIN_VERSION 1
 
 typedef struct {
     /* string describing architecture */
@@ -307,8 +307,8 @@ bool qemu_plugin_mem_is_sign_extended(qemu_plugin_meminfo_t info);
 bool qemu_plugin_mem_is_big_endian(qemu_plugin_meminfo_t info);
 bool qemu_plugin_mem_is_store(qemu_plugin_meminfo_t info);
 
-/*
- * qemu_plugin_get_hwaddr():
+/**
+ * qemu_plugin_get_hwaddr() - return handle for memory operation
  * @vaddr: the virtual address of the memory operation
  *
  * For system emulation returns a qemu_plugin_hwaddr handle to query
@@ -323,12 +323,30 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
                                                   uint64_t vaddr);
 
 /*
- * The following additional queries can be run on the hwaddr structure
- * to return information about it. For non-IO accesses the device
- * offset will be into the appropriate block of RAM.
+ * The following additional queries can be run on the hwaddr structure to
+ * return information about it - namely whether it is for an IO access and the
+ * physical address associated with the access.
+ */
+
+/**
+ * qemu_plugin_hwaddr_is_io() - query whether memory operation is IO
+ * @haddr: address handle from qemu_plugin_get_hwaddr()
+ *
+ * Returns true if the handle's memory operation is to memory-mapped IO, or
+ * false if it is to RAM
  */
 bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
-uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr);
+
+/**
+ * qemu_plugin_hwaddr_phys_addr() - query physical address for memory operation
+ * @haddr: address handle from qemu_plugin_get_hwaddr()
+ *
+ * Returns the physical address associated with the memory operation
+ *
+ * Note that the returned physical address may not be unique if you are dealing
+ * with multiple address spaces.
+ */
+uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr);
 
 /*
  * Returns a string representing the device. The string is valid for
diff --git a/contrib/plugins/hotpages.c b/contrib/plugins/hotpages.c
index eacc678eac..bf53267532 100644
--- a/contrib/plugins/hotpages.c
+++ b/contrib/plugins/hotpages.c
@@ -122,7 +122,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
         }
     } else {
         if (hwaddr && !qemu_plugin_hwaddr_is_io(hwaddr)) {
-            page = (uint64_t) qemu_plugin_hwaddr_device_offset(hwaddr);
+            page = (uint64_t) qemu_plugin_hwaddr_phys_addr(hwaddr);
         } else {
             page = vaddr;
         }
diff --git a/contrib/plugins/hwprofile.c b/contrib/plugins/hwprofile.c
index 6dac1d5f85..faf216ac00 100644
--- a/contrib/plugins/hwprofile.c
+++ b/contrib/plugins/hwprofile.c
@@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
         return;
     } else {
         const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
-        uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
+        uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
         bool is_write = qemu_plugin_mem_is_store(meminfo);
         DeviceCounts *counts;
 
diff --git a/plugins/api.c b/plugins/api.c
index 0b04380d57..3c7dc406e3 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -40,6 +40,7 @@
 #include "sysemu/sysemu.h"
 #include "tcg/tcg.h"
 #include "exec/exec-all.h"
+#include "exec/ram_addr.h"
 #include "disas/disas.h"
 #include "plugin.h"
 #ifndef CONFIG_USER_ONLY
@@ -298,19 +299,25 @@ bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr)
 #endif
 }
 
-uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr)
+uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr)
 {
 #ifdef CONFIG_SOFTMMU
     if (haddr) {
         if (!haddr->is_io) {
-            ram_addr_t ram_addr = qemu_ram_addr_from_host((void *) haddr->v.ram.hostaddr);
-            if (ram_addr == RAM_ADDR_INVALID) {
+            RAMBlock *block;
+            ram_addr_t offset;
+            void *hostaddr = (void *) haddr->v.ram.hostaddr;
+
+            block = qemu_ram_block_from_host(hostaddr, false, &offset);
+            if (!block) {
                 error_report("Bad ram pointer %"PRIx64"", haddr->v.ram.hostaddr);
                 abort();
             }
-            return ram_addr;
+
+            return block->offset + offset + block->mr->addr;
         } else {
-            return haddr->v.io.offset;
+            MemoryRegionSection *mrs = haddr->v.io.section;
+            return haddr->v.io.offset + mrs->mr->addr;
         }
     }
 #endif
-- 
2.20.1

[PATCH v1 03/14] docs/devel: include the plugin API information from the headers

[Alex Bennée]

We have kerneldoc tags for the headers so we might as well extract
them into our developer documentation whilst we are at it.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/tcg-plugins.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
index 39ce86ed96..18c6581d85 100644
--- a/docs/devel/tcg-plugins.rst
+++ b/docs/devel/tcg-plugins.rst
@@ -63,6 +63,11 @@ valid during the lifetime of the callback so it is important that any
 information that is needed is extracted during the callback and saved
 by the plugin.
 
+API
+===
+
+.. kernel-doc:: include/qemu/qemu-plugin.h
+
 Usage
 =====
 
-- 
2.20.1

Re: [PATCH v1 03/14] docs/devel: include the plugin API information from the headers

[Aaron Lindsay via]

On Mar 12 17:28, Alex Bennée wrote:
> We have kerneldoc tags for the headers so we might as well extract
> them into our developer documentation whilst we are at it.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>

> ---
>  docs/devel/tcg-plugins.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
> index 39ce86ed96..18c6581d85 100644
> --- a/docs/devel/tcg-plugins.rst
> +++ b/docs/devel/tcg-plugins.rst
> @@ -63,6 +63,11 @@ valid during the lifetime of the callback so it is important that any
>  information that is needed is extracted during the callback and saved
>  by the plugin.
>  
> +API
> +===
> +
> +.. kernel-doc:: include/qemu/qemu-plugin.h
> +
>  Usage
>  =====
>  
> -- 
> 2.20.1
> 

[PATCH v1 04/14] plugins: expand kernel-doc for qemu_info_t

[Alex Bennée]

It seems kernel-doc struggles a bit with typedef structs but with
enough encouragement we can get something out of it.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/qemu-plugin.h | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 3303dce862..4b84c6c293 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -49,22 +49,30 @@ extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
 
 #define QEMU_PLUGIN_VERSION 1
 
-typedef struct {
-    /* string describing architecture */
+/**
+ * struct qemu_info_t - system information for plugins
+ *
+ * This structure provides for some limited information about the
+ * system to allow the plugin to make decisions on how to proceed. For
+ * example it might only be suitable for running on some guest
+ * architectures or when under full system emulation.
+ */
+typedef struct qemu_info_t {
+    /** @target_name: string describing architecture */
     const char *target_name;
+    /** @version: minimum and current plugin API level */
     struct {
         int min;
         int cur;
     } version;
-    /* is this a full system emulation? */
+    /** @system_emulation: is this a full system emulation? */
     bool system_emulation;
     union {
-        /*
-         * smp_vcpus may change if vCPUs can be hot-plugged, max_vcpus
-         * is the system-wide limit.
-         */
+        /** @system: information relevant to system emulation */
         struct {
+            /** @system.smp_vcpus: initial number of vCPUs */
             int smp_vcpus;
+            /** @system.max_vcpus: maximum possible number of vCPUs */
             int max_vcpus;
         } system;
     };
-- 
2.20.1

Re: [PATCH v1 04/14] plugins: expand kernel-doc for qemu_info_t

[Aaron Lindsay via]

On Mar 12 17:28, Alex Bennée wrote:
> It seems kernel-doc struggles a bit with typedef structs but with
> enough encouragement we can get something out of it.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>

> ---
>  include/qemu/qemu-plugin.h | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 3303dce862..4b84c6c293 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -49,22 +49,30 @@ extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
>  
>  #define QEMU_PLUGIN_VERSION 1
>  
> -typedef struct {
> -    /* string describing architecture */
> +/**
> + * struct qemu_info_t - system information for plugins
> + *
> + * This structure provides for some limited information about the
> + * system to allow the plugin to make decisions on how to proceed. For
> + * example it might only be suitable for running on some guest
> + * architectures or when under full system emulation.
> + */
> +typedef struct qemu_info_t {
> +    /** @target_name: string describing architecture */
>      const char *target_name;
> +    /** @version: minimum and current plugin API level */
>      struct {
>          int min;
>          int cur;
>      } version;
> -    /* is this a full system emulation? */
> +    /** @system_emulation: is this a full system emulation? */
>      bool system_emulation;
>      union {
> -        /*
> -         * smp_vcpus may change if vCPUs can be hot-plugged, max_vcpus
> -         * is the system-wide limit.
> -         */
> +        /** @system: information relevant to system emulation */
>          struct {
> +            /** @system.smp_vcpus: initial number of vCPUs */
>              int smp_vcpus;
> +            /** @system.max_vcpus: maximum possible number of vCPUs */
>              int max_vcpus;
>          } system;
>      };
> -- 
> 2.20.1
> 

[PATCH v1 05/14] plugins: cleanup kernel-doc for qemu_plugin_install

[Alex Bennée]

kernel-doc doesn't like multiple Note sections. Also add an explicit
Return.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/qemu-plugin.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 4b84c6c293..ac1bb318da 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -85,15 +85,15 @@ typedef struct qemu_info_t {
  * @argc: number of arguments
  * @argv: array of arguments (@argc elements)
  *
- * All plugins must export this symbol.
- *
- * Note: Calling qemu_plugin_uninstall() from this function is a bug. To raise
- * an error during install, return !0.
+ * All plugins must export this symbol which is called when the plugin
+ * is first loaded. Calling qemu_plugin_uninstall() from this function
+ * is a bug.
  *
  * Note: @info is only live during the call. Copy any information we
- * want to keep.
+ * want to keep. @argv remains valid throughout the lifetime of the
+ * loaded plugin.
  *
- * Note: @argv remains valid throughout the lifetime of the loaded plugin.
+ * Return: 0 on successful loading, !0 for an error.
  */
 QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
                                            const qemu_info_t *info,
-- 
2.20.1

Re: [PATCH v1 05/14] plugins: cleanup kernel-doc for qemu_plugin_install

[Aaron Lindsay via]

On Mar 12 17:28, Alex Bennée wrote:
> kernel-doc doesn't like multiple Note sections. Also add an explicit
> Return.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>

> ---
>  include/qemu/qemu-plugin.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 4b84c6c293..ac1bb318da 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -85,15 +85,15 @@ typedef struct qemu_info_t {
>   * @argc: number of arguments
>   * @argv: array of arguments (@argc elements)
>   *
> - * All plugins must export this symbol.
> - *
> - * Note: Calling qemu_plugin_uninstall() from this function is a bug. To raise
> - * an error during install, return !0.
> + * All plugins must export this symbol which is called when the plugin
> + * is first loaded. Calling qemu_plugin_uninstall() from this function
> + * is a bug.
>   *
>   * Note: @info is only live during the call. Copy any information we
> - * want to keep.
> + * want to keep. @argv remains valid throughout the lifetime of the
> + * loaded plugin.
>   *
> - * Note: @argv remains valid throughout the lifetime of the loaded plugin.
> + * Return: 0 on successful loading, !0 for an error.
>   */
>  QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
>                                             const qemu_info_t *info,
> -- 
> 2.20.1
> 

[PATCH v1 06/14] plugins: expand the callback typedef kernel-docs

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/qemu-plugin.h | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index ac1bb318da..09b235f0b4 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -99,17 +99,36 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
                                            const qemu_info_t *info,
                                            int argc, char **argv);
 
-/*
- * Prototypes for the various callback styles we will be registering
- * in the following functions.
+/**
+ * typedef qemu_plugin_simple_cb_t - simple callback
+ * @id: the unique qemu_plugin_id_t
+ *
+ * This call-back passes no information aside from the unique @id.
  */
 typedef void (*qemu_plugin_simple_cb_t)(qemu_plugin_id_t id);
 
+/**
+ * typedef qemu_plugin_udata_cb_t - callback with user data
+ * @id: the unique qemu_plugin_id_t
+ * @userdata: a pointer to some user data supplied when the call-back
+ * was registered.
+ */
 typedef void (*qemu_plugin_udata_cb_t)(qemu_plugin_id_t id, void *userdata);
 
+/**
+ * typedef qemu_plugin_vcpu_simple_cb_t - vcpu callback
+ * @id: the unique qemu_plugin_id_t
+ * @vcpu_index: the current vcpu context
+ */
 typedef void (*qemu_plugin_vcpu_simple_cb_t)(qemu_plugin_id_t id,
                                              unsigned int vcpu_index);
 
+/**
+ * typedef qemu_plugin_vcpu_udata_cb_t - vcpu callback
+ * @vcpu_index: the current vcpu context
+ * @userdata: a pointer to some user data supplied when the call-back
+ * was registered.
+ */
 typedef void (*qemu_plugin_vcpu_udata_cb_t)(unsigned int vcpu_index,
                                             void *userdata);
 
-- 
2.20.1

Re: [PATCH v1 06/14] plugins: expand the callback typedef kernel-docs

[Aaron Lindsay via]

On Mar 12 17:28, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

One nit below, but otherwise:

Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>

> ---
>  include/qemu/qemu-plugin.h | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index ac1bb318da..09b235f0b4 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -99,17 +99,36 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
>                                             const qemu_info_t *info,
>                                             int argc, char **argv);
>  
> -/*
> - * Prototypes for the various callback styles we will be registering
> - * in the following functions.
> +/**
> + * typedef qemu_plugin_simple_cb_t - simple callback
> + * @id: the unique qemu_plugin_id_t
> + *
> + * This call-back passes no information aside from the unique @id.

Should we be consistent about always using 'callback' or 'call-back'
instead of alternating? I tend to use 'callback', but I'm not sure I
have a solid reason to prefer it.

-Aaron

>   */
>  typedef void (*qemu_plugin_simple_cb_t)(qemu_plugin_id_t id);
>  
> +/**
> + * typedef qemu_plugin_udata_cb_t - callback with user data
> + * @id: the unique qemu_plugin_id_t
> + * @userdata: a pointer to some user data supplied when the call-back
> + * was registered.
> + */
>  typedef void (*qemu_plugin_udata_cb_t)(qemu_plugin_id_t id, void *userdata);
>  
> +/**
> + * typedef qemu_plugin_vcpu_simple_cb_t - vcpu callback
> + * @id: the unique qemu_plugin_id_t
> + * @vcpu_index: the current vcpu context
> + */
>  typedef void (*qemu_plugin_vcpu_simple_cb_t)(qemu_plugin_id_t id,
>                                               unsigned int vcpu_index);
>  
> +/**
> + * typedef qemu_plugin_vcpu_udata_cb_t - vcpu callback
> + * @vcpu_index: the current vcpu context
> + * @userdata: a pointer to some user data supplied when the call-back
> + * was registered.
> + */
>  typedef void (*qemu_plugin_vcpu_udata_cb_t)(unsigned int vcpu_index,
>                                              void *userdata);
>  
> -- 
> 2.20.1
> 

Re: [PATCH v1 06/14] plugins: expand the callback typedef kernel-docs

[Alex Bennée]

Aaron Lindsay <aaron@os.amperecomputing.com> writes:

> On Mar 12 17:28, Alex Bennée wrote:
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> One nit below, but otherwise:
>
> Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>
>
>> ---
>>  include/qemu/qemu-plugin.h | 25 ++++++++++++++++++++++---
>>  1 file changed, 22 insertions(+), 3 deletions(-)
>> 
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index ac1bb318da..09b235f0b4 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>> @@ -99,17 +99,36 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
>>                                             const qemu_info_t *info,
>>                                             int argc, char **argv);
>>  
>> -/*
>> - * Prototypes for the various callback styles we will be registering
>> - * in the following functions.
>> +/**
>> + * typedef qemu_plugin_simple_cb_t - simple callback
>> + * @id: the unique qemu_plugin_id_t
>> + *
>> + * This call-back passes no information aside from the unique @id.
>
> Should we be consistent about always using 'callback' or 'call-back'
> instead of alternating? I tend to use 'callback', but I'm not sure I
> have a solid reason to prefer it.

No you are right we should stick to callback.

>
> -Aaron
>
>>   */
>>  typedef void (*qemu_plugin_simple_cb_t)(qemu_plugin_id_t id);
>>  
>> +/**
>> + * typedef qemu_plugin_udata_cb_t - callback with user data
>> + * @id: the unique qemu_plugin_id_t
>> + * @userdata: a pointer to some user data supplied when the call-back
>> + * was registered.
>> + */
>>  typedef void (*qemu_plugin_udata_cb_t)(qemu_plugin_id_t id, void *userdata);
>>  
>> +/**
>> + * typedef qemu_plugin_vcpu_simple_cb_t - vcpu callback
>> + * @id: the unique qemu_plugin_id_t
>> + * @vcpu_index: the current vcpu context
>> + */
>>  typedef void (*qemu_plugin_vcpu_simple_cb_t)(qemu_plugin_id_t id,
>>                                               unsigned int vcpu_index);
>>  
>> +/**
>> + * typedef qemu_plugin_vcpu_udata_cb_t - vcpu callback
>> + * @vcpu_index: the current vcpu context
>> + * @userdata: a pointer to some user data supplied when the call-back
>> + * was registered.
>> + */
>>  typedef void (*qemu_plugin_vcpu_udata_cb_t)(unsigned int vcpu_index,
>>                                              void *userdata);
>>  
>> -- 
>> 2.20.1
>> 


-- 
Alex Bennée

[PATCH v1 07/14] plugins: expand the typedef kernel-docs for translation

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/qemu-plugin.h | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 09b235f0b4..9ae3940d89 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -202,11 +202,9 @@ void qemu_plugin_register_vcpu_idle_cb(qemu_plugin_id_t id,
 void qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id,
                                          qemu_plugin_vcpu_simple_cb_t cb);
 
-/*
- * Opaque types that the plugin is given during the translation and
- * instrumentation phase.
- */
+/** struct qemu_plugin_tb - Opaque handle for a translation block */
 struct qemu_plugin_tb;
+/** struct qemu_plugin_insn - Opaque handle for a translated instruction */
 struct qemu_plugin_insn;
 
 enum qemu_plugin_cb_flags {
@@ -221,6 +219,14 @@ enum qemu_plugin_mem_rw {
     QEMU_PLUGIN_MEM_RW,
 };
 
+/**
+ * typedef qemu_plugin_vcpu_tb_trans_cb_t - translation callback
+ * @id: unique plugin id
+ * @tb: opaque handle used for querying and instrumenting a block.
+ */
+typedef void (*qemu_plugin_vcpu_tb_trans_cb_t)(qemu_plugin_id_t id,
+                                               struct qemu_plugin_tb *tb);
+
 /**
  * qemu_plugin_register_vcpu_tb_trans_cb() - register a translate cb
  * @id: plugin ID
@@ -233,9 +239,6 @@ enum qemu_plugin_mem_rw {
  * callbacks to be triggered when the block or individual instruction
  * executes.
  */
-typedef void (*qemu_plugin_vcpu_tb_trans_cb_t)(qemu_plugin_id_t id,
-                                               struct qemu_plugin_tb *tb);
-
 void qemu_plugin_register_vcpu_tb_trans_cb(qemu_plugin_id_t id,
                                            qemu_plugin_vcpu_tb_trans_cb_t cb);
 
-- 
2.20.1

Re: [PATCH v1 07/14] plugins: expand the typedef kernel-docs for translation

[Aaron Lindsay via]

On Mar 12 17:28, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>

> ---
>  include/qemu/qemu-plugin.h | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 09b235f0b4..9ae3940d89 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -202,11 +202,9 @@ void qemu_plugin_register_vcpu_idle_cb(qemu_plugin_id_t id,
>  void qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id,
>                                           qemu_plugin_vcpu_simple_cb_t cb);
>  
> -/*
> - * Opaque types that the plugin is given during the translation and
> - * instrumentation phase.
> - */
> +/** struct qemu_plugin_tb - Opaque handle for a translation block */
>  struct qemu_plugin_tb;
> +/** struct qemu_plugin_insn - Opaque handle for a translated instruction */
>  struct qemu_plugin_insn;
>  
>  enum qemu_plugin_cb_flags {
> @@ -221,6 +219,14 @@ enum qemu_plugin_mem_rw {
>      QEMU_PLUGIN_MEM_RW,
>  };
>  
> +/**
> + * typedef qemu_plugin_vcpu_tb_trans_cb_t - translation callback
> + * @id: unique plugin id
> + * @tb: opaque handle used for querying and instrumenting a block.
> + */
> +typedef void (*qemu_plugin_vcpu_tb_trans_cb_t)(qemu_plugin_id_t id,
> +                                               struct qemu_plugin_tb *tb);
> +
>  /**
>   * qemu_plugin_register_vcpu_tb_trans_cb() - register a translate cb
>   * @id: plugin ID
> @@ -233,9 +239,6 @@ enum qemu_plugin_mem_rw {
>   * callbacks to be triggered when the block or individual instruction
>   * executes.
>   */
> -typedef void (*qemu_plugin_vcpu_tb_trans_cb_t)(qemu_plugin_id_t id,
> -                                               struct qemu_plugin_tb *tb);
> -
>  void qemu_plugin_register_vcpu_tb_trans_cb(qemu_plugin_id_t id,
>                                             qemu_plugin_vcpu_tb_trans_cb_t cb);
>  
> -- 
> 2.20.1
> 

[PATCH v1 08/14] plugins: add qemu_plugin_cb_flags to kernel-doc

[Alex Bennée]

Also add a note to explain currently they are unused.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/qemu-plugin.h | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 9ae3940d89..c98866a637 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -207,10 +207,20 @@ struct qemu_plugin_tb;
 /** struct qemu_plugin_insn - Opaque handle for a translated instruction */
 struct qemu_plugin_insn;
 
+/**
+ * enum qemu_plugin_cb_flags - type of callback
+ *
+ * @QEMU_PLUGIN_CB_NO_REGS: callback does not access the CPU's regs
+ * @QEMU_PLUGIN_CB_R_REGS: callback reads the CPU's regs
+ * @QEMU_PLUGIN_CB_RW_REGS: callback reads and writes the CPU's regs
+ *
+ * Note: currently unused, plugins cannot read or change system
+ * register state.
+ */
 enum qemu_plugin_cb_flags {
-    QEMU_PLUGIN_CB_NO_REGS, /* callback does not access the CPU's regs */
-    QEMU_PLUGIN_CB_R_REGS,  /* callback reads the CPU's regs */
-    QEMU_PLUGIN_CB_RW_REGS, /* callback reads and writes the CPU's regs */
+    QEMU_PLUGIN_CB_NO_REGS,
+    QEMU_PLUGIN_CB_R_REGS,
+    QEMU_PLUGIN_CB_RW_REGS,
 };
 
 enum qemu_plugin_mem_rw {
-- 
2.20.1

Re: [PATCH v1 08/14] plugins: add qemu_plugin_cb_flags to kernel-doc

[Aaron Lindsay via]

On Mar 12 17:28, Alex Bennée wrote:
> Also add a note to explain currently they are unused.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

I'm personally interested in one clarification below, but don't think
that affects my:

Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>

> ---
>  include/qemu/qemu-plugin.h | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 9ae3940d89..c98866a637 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -207,10 +207,20 @@ struct qemu_plugin_tb;
>  /** struct qemu_plugin_insn - Opaque handle for a translated instruction */
>  struct qemu_plugin_insn;
>  
> +/**
> + * enum qemu_plugin_cb_flags - type of callback
> + *
> + * @QEMU_PLUGIN_CB_NO_REGS: callback does not access the CPU's regs
> + * @QEMU_PLUGIN_CB_R_REGS: callback reads the CPU's regs
> + * @QEMU_PLUGIN_CB_RW_REGS: callback reads and writes the CPU's regs
> + *
> + * Note: currently unused, plugins cannot read or change system
> + * register state.

They are unused in the sense that the current plugin interface does not
provide a way to make use of them. But are they completely free from
side effects?

-Aaron

Re: [PATCH v1 08/14] plugins: add qemu_plugin_cb_flags to kernel-doc

[Alex Bennée]

Aaron Lindsay <aaron@os.amperecomputing.com> writes:

> On Mar 12 17:28, Alex Bennée wrote:
>> Also add a note to explain currently they are unused.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> I'm personally interested in one clarification below, but don't think
> that affects my:
>
> Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>
>
>> ---
>>  include/qemu/qemu-plugin.h | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>> 
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index 9ae3940d89..c98866a637 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>> @@ -207,10 +207,20 @@ struct qemu_plugin_tb;
>>  /** struct qemu_plugin_insn - Opaque handle for a translated instruction */
>>  struct qemu_plugin_insn;
>>  
>> +/**
>> + * enum qemu_plugin_cb_flags - type of callback
>> + *
>> + * @QEMU_PLUGIN_CB_NO_REGS: callback does not access the CPU's regs
>> + * @QEMU_PLUGIN_CB_R_REGS: callback reads the CPU's regs
>> + * @QEMU_PLUGIN_CB_RW_REGS: callback reads and writes the CPU's regs
>> + *
>> + * Note: currently unused, plugins cannot read or change system
>> + * register state.
>
> They are unused in the sense that the current plugin interface does not
> provide a way to make use of them. But are they completely free from
> side effects?

They are free of side effects visible to the plugin. Under the covers it
uses the existing TCG_CALL_NO_* mechanics to ensure that register state
is synced to/from TCG temporaries before the callback.

>
> -Aaron


-- 
Alex Bennée

Re: [PATCH v1 08/14] plugins: add qemu_plugin_cb_flags to kernel-doc

[Aaron Lindsay via]

On Mar 16 13:40, Alex Bennée wrote:
> 
> Aaron Lindsay <aaron@os.amperecomputing.com> writes:
> 
> > On Mar 12 17:28, Alex Bennée wrote:
> >> Also add a note to explain currently they are unused.
> >> 
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >
> > I'm personally interested in one clarification below, but don't think
> > that affects my:
> >
> > Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> >
> >> ---
> >>  include/qemu/qemu-plugin.h | 16 +++++++++++++---
> >>  1 file changed, 13 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> >> index 9ae3940d89..c98866a637 100644
> >> --- a/include/qemu/qemu-plugin.h
> >> +++ b/include/qemu/qemu-plugin.h
> >> @@ -207,10 +207,20 @@ struct qemu_plugin_tb;
> >>  /** struct qemu_plugin_insn - Opaque handle for a translated instruction */
> >>  struct qemu_plugin_insn;
> >>  
> >> +/**
> >> + * enum qemu_plugin_cb_flags - type of callback
> >> + *
> >> + * @QEMU_PLUGIN_CB_NO_REGS: callback does not access the CPU's regs
> >> + * @QEMU_PLUGIN_CB_R_REGS: callback reads the CPU's regs
> >> + * @QEMU_PLUGIN_CB_RW_REGS: callback reads and writes the CPU's regs
> >> + *
> >> + * Note: currently unused, plugins cannot read or change system
> >> + * register state.
> >
> > They are unused in the sense that the current plugin interface does not
> > provide a way to make use of them. But are they completely free from
> > side effects?
> 
> They are free of side effects visible to the plugin. Under the covers it
> uses the existing TCG_CALL_NO_* mechanics to ensure that register state
> is synced to/from TCG temporaries before the callback.

I would currently find it useful to have that information included in
the documentation since there is no register state exposed and I am
basically hacking something together for my own use in the meantime...
but I understand that is in tension with the general philosophy of the
plugins to not expose implementation details.

-Aaron

[PATCH v1 09/14] plugins: add qemu_plugin_id_t to kernel-doc

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/qemu-plugin.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index c98866a637..5ac6fe5f02 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -32,6 +32,9 @@
   #define QEMU_PLUGIN_LOCAL  __attribute__((visibility("hidden")))
 #endif
 
+/**
+ * typedef qemu_plugin_id_t - Unique plugin ID
+ */
 typedef uint64_t qemu_plugin_id_t;
 
 /*
-- 
2.20.1

Re: [PATCH v1 09/14] plugins: add qemu_plugin_id_t to kernel-doc

[Aaron Lindsay via]

On Mar 12 17:28, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>

> ---
>  include/qemu/qemu-plugin.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index c98866a637..5ac6fe5f02 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -32,6 +32,9 @@
>    #define QEMU_PLUGIN_LOCAL  __attribute__((visibility("hidden")))
>  #endif
>  
> +/**
> + * typedef qemu_plugin_id_t - Unique plugin ID
> + */
>  typedef uint64_t qemu_plugin_id_t;
>  
>  /*
> -- 
> 2.20.1
> 

[PATCH v1 10/14] plugins: expand inline exec kernel-doc documentation.

[Alex Bennée]

Remove the extraneous @cb parameter and document the non-atomic nature
of the INLINE_ADD_U64 operation.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/qemu-plugin.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 5ac6fe5f02..dc05fc1932 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -269,6 +269,14 @@ void qemu_plugin_register_vcpu_tb_exec_cb(struct qemu_plugin_tb *tb,
                                           enum qemu_plugin_cb_flags flags,
                                           void *userdata);
 
+/**
+ * enum qemu_plugin_op - describes an inline op
+ *
+ * @QEMU_PLUGIN_INLINE_ADD_U64: add an immediate value uint64_t
+ *
+ * Note: currently only a single inline op is supported.
+ */
+
 enum qemu_plugin_op {
     QEMU_PLUGIN_INLINE_ADD_U64,
 };
@@ -283,6 +291,9 @@ enum qemu_plugin_op {
  * Insert an inline op to every time a translated unit executes.
  * Useful if you just want to increment a single counter somewhere in
  * memory.
+ *
+ * Note: ops are not atomic so in multi-threaded/multi-smp situations
+ * you will get inexact results.
  */
 void qemu_plugin_register_vcpu_tb_exec_inline(struct qemu_plugin_tb *tb,
                                               enum qemu_plugin_op op,
@@ -305,7 +316,6 @@ void qemu_plugin_register_vcpu_insn_exec_cb(struct qemu_plugin_insn *insn,
 /**
  * qemu_plugin_register_vcpu_insn_exec_inline() - insn execution inline op
  * @insn: the opaque qemu_plugin_insn handle for an instruction
- * @cb: callback function
  * @op: the type of qemu_plugin_op (e.g. ADD_U64)
  * @ptr: the target memory location for the op
  * @imm: the op data (e.g. 1)
-- 
2.20.1

Re: [PATCH v1 10/14] plugins: expand inline exec kernel-doc documentation.

[Aaron Lindsay via]

On Mar 12 17:28, Alex Bennée wrote:
> Remove the extraneous @cb parameter and document the non-atomic nature
> of the INLINE_ADD_U64 operation.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>

> ---
>  include/qemu/qemu-plugin.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 5ac6fe5f02..dc05fc1932 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -269,6 +269,14 @@ void qemu_plugin_register_vcpu_tb_exec_cb(struct qemu_plugin_tb *tb,
>                                            enum qemu_plugin_cb_flags flags,
>                                            void *userdata);
>  
> +/**
> + * enum qemu_plugin_op - describes an inline op
> + *
> + * @QEMU_PLUGIN_INLINE_ADD_U64: add an immediate value uint64_t
> + *
> + * Note: currently only a single inline op is supported.
> + */
> +
>  enum qemu_plugin_op {
>      QEMU_PLUGIN_INLINE_ADD_U64,
>  };
> @@ -283,6 +291,9 @@ enum qemu_plugin_op {
>   * Insert an inline op to every time a translated unit executes.
>   * Useful if you just want to increment a single counter somewhere in
>   * memory.
> + *
> + * Note: ops are not atomic so in multi-threaded/multi-smp situations
> + * you will get inexact results.
>   */
>  void qemu_plugin_register_vcpu_tb_exec_inline(struct qemu_plugin_tb *tb,
>                                                enum qemu_plugin_op op,
> @@ -305,7 +316,6 @@ void qemu_plugin_register_vcpu_insn_exec_cb(struct qemu_plugin_insn *insn,
>  /**
>   * qemu_plugin_register_vcpu_insn_exec_inline() - insn execution inline op
>   * @insn: the opaque qemu_plugin_insn handle for an instruction
> - * @cb: callback function
>   * @op: the type of qemu_plugin_op (e.g. ADD_U64)
>   * @ptr: the target memory location for the op
>   * @imm: the op data (e.g. 1)
> -- 
> 2.20.1
> 

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

[Alex Bennée]

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

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

[Aaron Lindsay via]

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

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

[Alex Bennée]

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

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

[Aaron Lindsay via]

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

[PATCH v1 12/14] plugins: expand kernel-doc for memory query and instrumentation

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/qemu-plugin.h | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index d4adce730a..aed868d42a 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -392,24 +392,45 @@ uint64_t qemu_plugin_insn_vaddr(const struct qemu_plugin_insn *insn);
  */
 void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn);
 
-/*
- * Memory Instrumentation
- *
- * The anonymous qemu_plugin_meminfo_t and qemu_plugin_hwaddr types
- * can be used in queries to QEMU to get more information about a
- * given memory access.
+/**
+ * typedef qemu_plugin_meminfo_t - opaque memory transaction handle
  */
 typedef uint32_t qemu_plugin_meminfo_t;
+/** struct qemu_plugin_hwaddr - opaque hw address handle */
 struct qemu_plugin_hwaddr;
 
-/* meminfo queries */
+/**
+ * qemu_plugin_mem_size_shift() - get size of access
+ * @info: opaque memory transaction handle
+ *
+ * Returns: size of access in ^2 (0=byte, 1=16bit, 2=32bit etc...)
+ */
 unsigned int qemu_plugin_mem_size_shift(qemu_plugin_meminfo_t info);
+/**
+ * qemu_plugin_mem_is_sign_extended() - was the access sign extended
+ * @info: opaque memory transaction handle
+ *
+ * Returns: true if it was, otherwise false
+ */
 bool qemu_plugin_mem_is_sign_extended(qemu_plugin_meminfo_t info);
+/**
+ * qemu_plugin_mem_is_big_endian() - was the access big endian
+ * @info: opaque memory transaction handle
+ *
+ * Returns: true if it was, otherwise false
+ */
 bool qemu_plugin_mem_is_big_endian(qemu_plugin_meminfo_t info);
+/**
+ * qemu_plugin_mem_is_store() - was the access a store
+ * @info: opaque memory transaction handle
+ *
+ * Returns: true if it was, otherwise false
+ */
 bool qemu_plugin_mem_is_store(qemu_plugin_meminfo_t info);
 
 /**
  * qemu_plugin_get_hwaddr() - return handle for memory operation
+ * @info: opaque memory info structure
  * @vaddr: the virtual address of the memory operation
  *
  * For system emulation returns a qemu_plugin_hwaddr handle to query
-- 
2.20.1

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

[Aaron Lindsay via]

On Mar 12 17:28, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Small comment below, but otherwise:

Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>

> ---
>  include/qemu/qemu-plugin.h | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index d4adce730a..aed868d42a 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -392,24 +392,45 @@ uint64_t qemu_plugin_insn_vaddr(const struct qemu_plugin_insn *insn);
>   */
>  void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn);
>  
> -/*
> - * Memory Instrumentation
> - *
> - * The anonymous qemu_plugin_meminfo_t and qemu_plugin_hwaddr types
> - * can be used in queries to QEMU to get more information about a
> - * given memory access.
> +/**
> + * typedef qemu_plugin_meminfo_t - opaque memory transaction handle

Would it still be useful to include the types of things you can do
with a qemu_plugin_meminfo_t here?

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

[Alex Bennée]

Aaron Lindsay <aaron@os.amperecomputing.com> writes:

> On Mar 12 17:28, Alex Bennée wrote:
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> Small comment below, but otherwise:
>
> Reviewed-by: Aaron Lindsay <aaron@os.amperecomputing.com>
>
>> ---
>>  include/qemu/qemu-plugin.h | 35 ++++++++++++++++++++++++++++-------
>>  1 file changed, 28 insertions(+), 7 deletions(-)
>> 
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index d4adce730a..aed868d42a 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>> @@ -392,24 +392,45 @@ uint64_t qemu_plugin_insn_vaddr(const struct qemu_plugin_insn *insn);
>>   */
>>  void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn);
>>  
>> -/*
>> - * Memory Instrumentation
>> - *
>> - * The anonymous qemu_plugin_meminfo_t and qemu_plugin_hwaddr types
>> - * can be used in queries to QEMU to get more information about a
>> - * given memory access.
>> +/**
>> + * typedef qemu_plugin_meminfo_t - opaque memory transaction handle
>
> Would it still be useful to include the types of things you can do
> with a qemu_plugin_meminfo_t here?

I've referenced it can be "..further queried using the qemu_plugin_mem_*
query functions."

-- 
Alex Bennée

[PATCH v1 13/14] plugins: getting qemu_plugin_get_hwaddr only expose one function prototype

[Alex Bennée]

From: Yonggang Luo <luoyonggang@gmail.com>

This is used for counting how much function are export to qemu plugin.

Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20201013002806.1447-2-luoyonggang@gmail.com>
---
 plugins/api.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/plugins/api.c b/plugins/api.c
index 3c7dc406e3..b22998cd7c 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -266,10 +266,12 @@ bool qemu_plugin_mem_is_store(qemu_plugin_meminfo_t info)
 
 #ifdef CONFIG_SOFTMMU
 static __thread struct qemu_plugin_hwaddr hwaddr_info;
+#endif
 
 struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
                                                   uint64_t vaddr)
 {
+#ifdef CONFIG_SOFTMMU
     CPUState *cpu = current_cpu;
     unsigned int mmu_idx = info >> TRACE_MEM_MMU_SHIFT;
     hwaddr_info.is_store = info & TRACE_MEM_ST;
@@ -281,14 +283,10 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
     }
 
     return &hwaddr_info;
-}
 #else
-struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
-                                                  uint64_t vaddr)
-{
     return NULL;
-}
 #endif
+}
 
 bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr)
 {
-- 
2.20.1

[PATCH v1 14/14] plugins: Fixes typo in qemu-plugin.h

[Alex Bennée]

From: Yonggang Luo <luoyonggang@gmail.com>

Getting the comment consistence with the function name

Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20201013002806.1447-3-luoyonggang@gmail.com>
---
 include/qemu/qemu-plugin.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index aed868d42a..19e77e345b 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -256,7 +256,7 @@ void qemu_plugin_register_vcpu_tb_trans_cb(qemu_plugin_id_t id,
                                            qemu_plugin_vcpu_tb_trans_cb_t cb);
 
 /**
- * qemu_plugin_register_vcpu_tb_trans_exec_cb() - register execution callback
+ * qemu_plugin_register_vcpu_tb_exec_cb() - register execution callback
  * @tb: the opaque qemu_plugin_tb handle for the translation
  * @cb: callback function
  * @flags: does the plugin read or write the CPU's registers?
@@ -282,7 +282,7 @@ enum qemu_plugin_op {
 };
 
 /**
- * qemu_plugin_register_vcpu_tb_trans_exec_inline() - execution inline op
+ * qemu_plugin_register_vcpu_tb_exec_inline() - execution inline op
  * @tb: the opaque qemu_plugin_tb handle for the translation
  * @op: the type of qemu_plugin_op (e.g. ADD_U64)
  * @ptr: the target memory location for the op
-- 
2.20.1