[PATCH v1 0/7] plugins/next (windows, leaks, tcg tracing)

[PATCH v1 0/7] plugins/next (windows, leaks, tcg tracing)

[Alex Bennée]

Hi,

This is my current plugins queue. It has a few fixes from Yonggang and
Mahmoud as well as some minor tweaks to the TCG tracing. I've also
marked an intention to deprecate following the discussion we had in:

  Subject: trace_FOO_tcg bit-rotted?
  Date: Tue, 06 Apr 2021 17:00:20 +0100
  Message-ID: <87eefnwd0l.fsf@linaro.org>

After the fixes to the tool I've actually managed to implement some
trace points in the generic code but I was still running into issues
in translator specific code. For the time being the tracing
documentation just references TCG plugins as another approach to
solving these sort of instrumentation problems.

The following still need review:

 - tcg: add trace events for [exit|goto]_tb and goto_ptr
 - scripts/tracetool: don't barf validating TCG types
 - docs: mark intention to deprecate TCG tracing functionality

Alex Bennée (3):
  docs: mark intention to deprecate TCG tracing functionality
  scripts/tracetool: don't barf validating TCG types
  tcg: add trace events for [exit|goto]_tb and goto_ptr

Mahmoud Mandour (2):
  plugins/hotblocks: Properly freed the hash table values
  plugins/hotpages: Properly freed the hash table values

Yonggang Luo (2):
  plugins: Update qemu-plugins.symbols to match qemu-plugins.h
  plugins: Move all typedef and type declaration to the front of the
    qemu-plugin.h

 docs/devel/tcg-plugins.rst    |   2 +
 docs/devel/tracing.rst        |   7 ++
 docs/system/deprecated.rst    |  13 +++
 include/qemu/qemu-plugin.h    | 187 +++++++++++++++++-----------------
 contrib/plugins/hotblocks.c   |   3 +-
 contrib/plugins/hotpages.c    |   3 +-
 tcg/tcg-op.c                  |   8 ++
 plugins/qemu-plugins.symbols  |  25 +++--
 scripts/tracetool/__init__.py |   7 +-
 trace-events                  |  12 +++
 10 files changed, 155 insertions(+), 112 deletions(-)

-- 
2.20.1

[PATCH v1 1/7] plugins: Update qemu-plugins.symbols to match qemu-plugins.h

[Alex Bennée]

From: Yonggang Luo <luoyonggang@gmail.com>

Reorder the function symbols that consistence with qemu-plugins.h

Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20210318185555.434-2-luoyonggang@gmail.com>
---
 plugins/qemu-plugins.symbols | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index 4bdb381f48..a0ac1df62a 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -5,35 +5,34 @@
   qemu_plugin_register_vcpu_exit_cb;
   qemu_plugin_register_vcpu_idle_cb;
   qemu_plugin_register_vcpu_resume_cb;
-  qemu_plugin_register_vcpu_insn_exec_cb;
-  qemu_plugin_register_vcpu_insn_exec_inline;
-  qemu_plugin_register_vcpu_mem_cb;
-  qemu_plugin_register_vcpu_mem_haddr_cb;
-  qemu_plugin_register_vcpu_mem_inline;
-  qemu_plugin_ram_addr_from_host;
   qemu_plugin_register_vcpu_tb_trans_cb;
   qemu_plugin_register_vcpu_tb_exec_cb;
   qemu_plugin_register_vcpu_tb_exec_inline;
-  qemu_plugin_register_flush_cb;
-  qemu_plugin_register_vcpu_syscall_cb;
-  qemu_plugin_register_vcpu_syscall_ret_cb;
-  qemu_plugin_register_atexit_cb;
+  qemu_plugin_register_vcpu_insn_exec_cb;
+  qemu_plugin_register_vcpu_insn_exec_inline;
   qemu_plugin_tb_n_insns;
-  qemu_plugin_tb_get_insn;
   qemu_plugin_tb_vaddr;
+  qemu_plugin_tb_get_insn;
   qemu_plugin_insn_data;
   qemu_plugin_insn_size;
   qemu_plugin_insn_vaddr;
   qemu_plugin_insn_haddr;
-  qemu_plugin_insn_disas;
   qemu_plugin_mem_size_shift;
   qemu_plugin_mem_is_sign_extended;
   qemu_plugin_mem_is_big_endian;
   qemu_plugin_mem_is_store;
   qemu_plugin_get_hwaddr;
   qemu_plugin_hwaddr_is_io;
-  qemu_plugin_hwaddr_to_raddr;
+  qemu_plugin_hwaddr_phys_addr;
+  qemu_plugin_hwaddr_device_name;
+  qemu_plugin_register_vcpu_mem_cb;
+  qemu_plugin_register_vcpu_mem_inline;
+  qemu_plugin_register_vcpu_syscall_cb;
+  qemu_plugin_register_vcpu_syscall_ret_cb;
+  qemu_plugin_insn_disas;
   qemu_plugin_vcpu_for_each;
+  qemu_plugin_register_flush_cb;
+  qemu_plugin_register_atexit_cb;
   qemu_plugin_n_vcpus;
   qemu_plugin_n_max_vcpus;
   qemu_plugin_outs;
-- 
2.20.1

[PATCH v1 2/7] plugins: Move all typedef and type declaration to the front of the qemu-plugin.h

[Alex Bennée]

From: Yonggang Luo <luoyonggang@gmail.com>

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

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 97cdfd7761..2cb17f3051 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -81,27 +81,6 @@ typedef struct qemu_info_t {
     };
 } qemu_info_t;
 
-/**
- * qemu_plugin_install() - Install a plugin
- * @id: this plugin's opaque ID
- * @info: a block describing some details about the guest
- * @argc: number of arguments
- * @argv: array of arguments (@argc elements)
- *
- * 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. @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,
-                                           int argc, char **argv);
-
 /**
  * typedef qemu_plugin_simple_cb_t - simple callback
  * @id: the unique qemu_plugin_id_t
@@ -135,6 +114,98 @@ typedef void (*qemu_plugin_vcpu_simple_cb_t)(qemu_plugin_id_t id,
 typedef void (*qemu_plugin_vcpu_udata_cb_t)(unsigned int vcpu_index,
                                             void *userdata);
 
+/** 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 - 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,
+    QEMU_PLUGIN_CB_R_REGS,
+    QEMU_PLUGIN_CB_RW_REGS,
+};
+
+enum qemu_plugin_mem_rw {
+    QEMU_PLUGIN_MEM_R = 1,
+    QEMU_PLUGIN_MEM_W,
+    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);
+
+/**
+ * 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,
+};
+
+/**
+ * typedef qemu_plugin_meminfo_t - opaque memory transaction handle
+ *
+ * This can be further queried using the qemu_plugin_mem_* query
+ * functions.
+ */
+typedef uint32_t qemu_plugin_meminfo_t;
+/** struct qemu_plugin_hwaddr - opaque hw address handle */
+struct qemu_plugin_hwaddr;
+
+typedef void
+(*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
+                             qemu_plugin_meminfo_t info, uint64_t vaddr,
+                             void *userdata);
+
+typedef void
+(*qemu_plugin_vcpu_syscall_cb_t)(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);
+typedef void
+(*qemu_plugin_vcpu_syscall_ret_cb_t)(qemu_plugin_id_t id, unsigned int vcpu_idx,
+                                     int64_t num, int64_t ret);
+
+/**
+ * qemu_plugin_install() - Install a plugin
+ * @id: this plugin's opaque ID
+ * @info: a block describing some details about the guest
+ * @argc: number of arguments
+ * @argv: array of arguments (@argc elements)
+ *
+ * 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. @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,
+                                           int argc, char **argv);
+
 /**
  * qemu_plugin_uninstall() - Uninstall a plugin
  * @id: this plugin's opaque ID
@@ -205,41 +276,6 @@ 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);
 
-/** 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 - 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,
-    QEMU_PLUGIN_CB_R_REGS,
-    QEMU_PLUGIN_CB_RW_REGS,
-};
-
-enum qemu_plugin_mem_rw {
-    QEMU_PLUGIN_MEM_R = 1,
-    QEMU_PLUGIN_MEM_W,
-    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
@@ -269,18 +305,6 @@ 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,
-};
-
 /**
  * qemu_plugin_register_vcpu_tb_exec_inline() - execution inline op
  * @tb: the opaque qemu_plugin_tb handle for the translation
@@ -393,16 +417,6 @@ uint64_t qemu_plugin_insn_vaddr(const struct qemu_plugin_insn *insn);
  */
 void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn);
 
-/**
- * typedef qemu_plugin_meminfo_t - opaque memory transaction handle
- *
- * This can be further queried using the qemu_plugin_mem_* query
- * functions.
- */
-typedef uint32_t qemu_plugin_meminfo_t;
-/** struct qemu_plugin_hwaddr - opaque hw address handle */
-struct qemu_plugin_hwaddr;
-
 /**
  * qemu_plugin_mem_size_shift() - get size of access
  * @info: opaque memory transaction handle
@@ -480,11 +494,6 @@ uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr);
  */
 const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h);
 
-typedef void
-(*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
-                             qemu_plugin_meminfo_t info, uint64_t vaddr,
-                             void *userdata);
-
 void qemu_plugin_register_vcpu_mem_cb(struct qemu_plugin_insn *insn,
                                       qemu_plugin_vcpu_mem_cb_t cb,
                                       enum qemu_plugin_cb_flags flags,
@@ -496,21 +505,9 @@ void qemu_plugin_register_vcpu_mem_inline(struct qemu_plugin_insn *insn,
                                           enum qemu_plugin_op op, void *ptr,
                                           uint64_t imm);
 
-
-
-typedef void
-(*qemu_plugin_vcpu_syscall_cb_t)(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);
-
 void qemu_plugin_register_vcpu_syscall_cb(qemu_plugin_id_t id,
                                           qemu_plugin_vcpu_syscall_cb_t cb);
 
-typedef void
-(*qemu_plugin_vcpu_syscall_ret_cb_t)(qemu_plugin_id_t id, unsigned int vcpu_idx,
-                                     int64_t num, int64_t ret);
-
 void
 qemu_plugin_register_vcpu_syscall_ret_cb(qemu_plugin_id_t id,
                                          qemu_plugin_vcpu_syscall_ret_cb_t cb);
-- 
2.20.1

[PATCH v1 3/7] plugins/hotblocks: Properly freed the hash table values

[Alex Bennée]

From: Mahmoud Mandour <ma.mandourr@gmail.com>

Freed the values stored in the hash table ``hotblocks``
returned by ``g_hash_table_get_values()`` by freeing the sorted
list and destroyed the hash table afterward.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20210422005043.3569-2-ma.mandourr@gmail.com>
---
 contrib/plugins/hotblocks.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
index 4b08340143..64692c0670 100644
--- a/contrib/plugins/hotblocks.c
+++ b/contrib/plugins/hotblocks.c
@@ -68,10 +68,11 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
                                    rec->insns, rec->exec_count);
         }
 
-        g_list_free(it);
+        g_list_free_full(it, g_free);
         g_mutex_unlock(&lock);
     }
 
+    g_hash_table_destroy(hotblocks);
     qemu_plugin_outs(report->str);
 }
 
-- 
2.20.1

[PATCH v1 4/7] plugins/hotpages: Properly freed the hash table values

[Alex Bennée]

From: Mahmoud Mandour <ma.mandourr@gmail.com>

Allocated ``pages`` hash table through ``g_hash_table_new_full`` to
add a freeing function & destroyed the hash table on exit.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20210422005043.3569-3-ma.mandourr@gmail.com>
---
 contrib/plugins/hotpages.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/plugins/hotpages.c b/contrib/plugins/hotpages.c
index bf53267532..9cf7f02c77 100644
--- a/contrib/plugins/hotpages.c
+++ b/contrib/plugins/hotpages.c
@@ -97,13 +97,14 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
         g_list_free(it);
     }
 
+    g_hash_table_destroy(pages);
     qemu_plugin_outs(report->str);
 }
 
 static void plugin_init(void)
 {
     page_mask = (page_size - 1);
-    pages = g_hash_table_new(NULL, g_direct_equal);
+    pages = g_hash_table_new_full(NULL, g_direct_equal, NULL, g_free);
 }
 
 static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
-- 
2.20.1

[PATCH v1 5/7] docs: mark intention to deprecate TCG tracing functionality

[Alex Bennée]

Currently attempts to add a new TCG trace events results in failures
to build. Previous discussions have suggested maybe it's time to mark
the feature as deprecated and push people towards using plugins.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Luis Vilanova <vilanova@imperial.ac.uk>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 docs/devel/tcg-plugins.rst |  2 ++
 docs/devel/tracing.rst     |  7 +++++++
 docs/system/deprecated.rst | 13 +++++++++++++
 3 files changed, 22 insertions(+)

diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
index 18c6581d85..edf04e3091 100644
--- a/docs/devel/tcg-plugins.rst
+++ b/docs/devel/tcg-plugins.rst
@@ -3,6 +3,8 @@
    Copyright (c) 2019, Linaro Limited
    Written by Emilio Cota and Alex Bennée
 
+.. _tcgplugin-ref:
+
 ================
 QEMU TCG Plugins
 ================
diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst
index ba83954899..6b0f46cd54 100644
--- a/docs/devel/tracing.rst
+++ b/docs/devel/tracing.rst
@@ -414,6 +414,13 @@ disabled, this check will have no performance impact.
 "tcg"
 -----
 
+.. warning::
+   The ability to add new TCG trace points relies on a having a good
+   understanding of the TCG internals. In the meantime TCG plugins
+   have been introduced which solve many of the same problems with
+   more of a focus on analysing guest code. See :ref:`tcgplugin-ref`
+   for more details.
+
 Guest code generated by TCG can be traced by defining an event with the "tcg"
 event property. Internally, this property generates two events:
 "<eventname>_trans" to trace the event at translation time, and
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 80cae86252..0c9d3c1e1e 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -312,6 +312,19 @@ The ``I7200`` guest CPU relies on the nanoMIPS ISA, which is deprecated
 (the ISA has never been upstreamed to a compiler toolchain). Therefore
 this CPU is also deprecated.
 
+TCG introspection features
+--------------------------
+
+TCG trace-events (since 6.1)
+''''''''''''''''''''''''''''
+
+The ability to add new TCG trace points has bit rotted and as the
+feature can be replicated with TCG plugins it will be deprecated. If
+any user is currently using this feature and needs help with
+converting to using TCG plugins they should contact the qemu-devel
+mailing list.
+
+
 Related binaries
 ----------------
 
-- 
2.20.1

Re: [PATCH v1 5/7] docs: mark intention to deprecate TCG tracing functionality

[Daniel P. Berrangé]

On Wed, May 05, 2021 at 10:22:57AM +0100, Alex Bennée wrote:
> Currently attempts to add a new TCG trace events results in failures
> to build. Previous discussions have suggested maybe it's time to mark
> the feature as deprecated and push people towards using plugins.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Luis Vilanova <vilanova@imperial.ac.uk>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/devel/tcg-plugins.rst |  2 ++
>  docs/devel/tracing.rst     |  7 +++++++
>  docs/system/deprecated.rst | 13 +++++++++++++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
> index 18c6581d85..edf04e3091 100644
> --- a/docs/devel/tcg-plugins.rst
> +++ b/docs/devel/tcg-plugins.rst
> @@ -3,6 +3,8 @@
>     Copyright (c) 2019, Linaro Limited
>     Written by Emilio Cota and Alex Bennée
>  
> +.. _tcgplugin-ref:
> +
>  ================
>  QEMU TCG Plugins
>  ================
> diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst
> index ba83954899..6b0f46cd54 100644
> --- a/docs/devel/tracing.rst
> +++ b/docs/devel/tracing.rst
> @@ -414,6 +414,13 @@ disabled, this check will have no performance impact.
>  "tcg"
>  -----
>  
> +.. warning::
> +   The ability to add new TCG trace points relies on a having a good
> +   understanding of the TCG internals. In the meantime TCG plugins
> +   have been introduced which solve many of the same problems with
> +   more of a focus on analysing guest code. See :ref:`tcgplugin-ref`
> +   for more details.
> +
>  Guest code generated by TCG can be traced by defining an event with the "tcg"
>  event property. Internally, this property generates two events:
>  "<eventname>_trans" to trace the event at translation time, and
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 80cae86252..0c9d3c1e1e 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -312,6 +312,19 @@ The ``I7200`` guest CPU relies on the nanoMIPS ISA, which is deprecated
>  (the ISA has never been upstreamed to a compiler toolchain). Therefore
>  this CPU is also deprecated.
>  
> +TCG introspection features
> +--------------------------
> +
> +TCG trace-events (since 6.1)
> +''''''''''''''''''''''''''''
> +
> +The ability to add new TCG trace points has bit rotted and as the

When you say this "has bit rotted", just how bad is the situation ?

Is the TCG tracing still usable at all, or is is fully broken
already ?


> +feature can be replicated with TCG plugins it will be deprecated. If
> +any user is currently using this feature and needs help with
> +converting to using TCG plugins they should contact the qemu-devel
> +mailing list.
> +

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 5/7] docs: mark intention to deprecate TCG tracing functionality

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, May 05, 2021 at 10:22:57AM +0100, Alex Bennée wrote:
>> Currently attempts to add a new TCG trace events results in failures
>> to build. Previous discussions have suggested maybe it's time to mark
>> the feature as deprecated and push people towards using plugins.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Luis Vilanova <vilanova@imperial.ac.uk>
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  docs/devel/tcg-plugins.rst |  2 ++
>>  docs/devel/tracing.rst     |  7 +++++++
>>  docs/system/deprecated.rst | 13 +++++++++++++
>>  3 files changed, 22 insertions(+)
>> 
>> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
>> index 18c6581d85..edf04e3091 100644
>> --- a/docs/devel/tcg-plugins.rst
>> +++ b/docs/devel/tcg-plugins.rst
>> @@ -3,6 +3,8 @@
>>     Copyright (c) 2019, Linaro Limited
>>     Written by Emilio Cota and Alex Bennée
>>  
>> +.. _tcgplugin-ref:
>> +
>>  ================
>>  QEMU TCG Plugins
>>  ================
>> diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst
>> index ba83954899..6b0f46cd54 100644
>> --- a/docs/devel/tracing.rst
>> +++ b/docs/devel/tracing.rst
>> @@ -414,6 +414,13 @@ disabled, this check will have no performance impact.
>>  "tcg"
>>  -----
>>  
>> +.. warning::
>> +   The ability to add new TCG trace points relies on a having a good
>> +   understanding of the TCG internals. In the meantime TCG plugins
>> +   have been introduced which solve many of the same problems with
>> +   more of a focus on analysing guest code. See :ref:`tcgplugin-ref`
>> +   for more details.
>> +
>>  Guest code generated by TCG can be traced by defining an event with the "tcg"
>>  event property. Internally, this property generates two events:
>>  "<eventname>_trans" to trace the event at translation time, and
>> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
>> index 80cae86252..0c9d3c1e1e 100644
>> --- a/docs/system/deprecated.rst
>> +++ b/docs/system/deprecated.rst
>> @@ -312,6 +312,19 @@ The ``I7200`` guest CPU relies on the nanoMIPS ISA, which is deprecated
>>  (the ISA has never been upstreamed to a compiler toolchain). Therefore
>>  this CPU is also deprecated.
>>  
>> +TCG introspection features
>> +--------------------------
>> +
>> +TCG trace-events (since 6.1)
>> +''''''''''''''''''''''''''''
>> +
>> +The ability to add new TCG trace points has bit rotted and as the
>
> When you say this "has bit rotted", just how bad is the situation ?
>
> Is the TCG tracing still usable at all, or is is fully broken
> already ?

Well patches 6/7 got it working for generic TCG things. I haven't been
able to get the architecture one working but I suspect that is some sort
of interaction between the per-arch trace header generation that I
haven't quite figured out yet.

It's currently broken without the included patches because it's not
really being exercised by anything.

>> +feature can be replicated with TCG plugins it will be deprecated. If
>> +any user is currently using this feature and needs help with
>> +converting to using TCG plugins they should contact the qemu-devel
>> +mailing list.
>> +
>
> Regards,
> Daniel


-- 
Alex Bennée

Re: [PATCH v1 5/7] docs: mark intention to deprecate TCG tracing functionality

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Daniel P. Berrangé <berrange@redhat.com> writes:
>
>> On Wed, May 05, 2021 at 10:22:57AM +0100, Alex Bennée wrote:
<snip>
>>> +TCG introspection features
>>> +--------------------------
>>> +
>>> +TCG trace-events (since 6.1)
>>> +''''''''''''''''''''''''''''
>>> +
>>> +The ability to add new TCG trace points has bit rotted and as the
>>
>> When you say this "has bit rotted", just how bad is the situation ?
>>
>> Is the TCG tracing still usable at all, or is is fully broken
>> already ?
>
> Well patches 6/7 got it working for generic TCG things. I haven't been
> able to get the architecture one working but I suspect that is some sort
> of interaction between the per-arch trace header generation that I
> haven't quite figured out yet.

Ahh it's since 7609ffb919 (trace: fix tcg tracing build breakage) which
limited tcg/vcpu events to the root trace-events file.

-- 
Alex Bennée

Re: [PATCH v1 5/7] docs: mark intention to deprecate TCG tracing functionality

[Daniel P. Berrangé]

On Wed, May 05, 2021 at 11:41:46AM +0100, Alex Bennée wrote:
> 
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
> > Daniel P. Berrangé <berrange@redhat.com> writes:
> >
> >> On Wed, May 05, 2021 at 10:22:57AM +0100, Alex Bennée wrote:
> <snip>
> >>> +TCG introspection features
> >>> +--------------------------
> >>> +
> >>> +TCG trace-events (since 6.1)
> >>> +''''''''''''''''''''''''''''
> >>> +
> >>> +The ability to add new TCG trace points has bit rotted and as the
> >>
> >> When you say this "has bit rotted", just how bad is the situation ?
> >>
> >> Is the TCG tracing still usable at all, or is is fully broken
> >> already ?
> >
> > Well patches 6/7 got it working for generic TCG things. I haven't been
> > able to get the architecture one working but I suspect that is some sort
> > of interaction between the per-arch trace header generation that I
> > haven't quite figured out yet.
> 
> Ahh it's since 7609ffb919 (trace: fix tcg tracing build breakage) which
> limited tcg/vcpu events to the root trace-events file.

That commit is from release 2.10.0.

The other commit mentioned in patch 6 (73ff061032) is from 2.12.0.

So no one has been able to use this feature for 3+ years already.

Is it actually worth fixing and then deprecating for 2 releases before
deleting, as opposed to just deleting the broken code today on basis
that it can't have any current users ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 5/7] docs: mark intention to deprecate TCG tracing functionality

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, May 05, 2021 at 11:41:46AM +0100, Alex Bennée wrote:
>> 
>> Alex Bennée <alex.bennee@linaro.org> writes:
>> 
>> > Daniel P. Berrangé <berrange@redhat.com> writes:
>> >
>> >> On Wed, May 05, 2021 at 10:22:57AM +0100, Alex Bennée wrote:
>> <snip>
>> >>> +TCG introspection features
>> >>> +--------------------------
>> >>> +
>> >>> +TCG trace-events (since 6.1)
>> >>> +''''''''''''''''''''''''''''
>> >>> +
>> >>> +The ability to add new TCG trace points has bit rotted and as the
>> >>
>> >> When you say this "has bit rotted", just how bad is the situation ?
>> >>
>> >> Is the TCG tracing still usable at all, or is is fully broken
>> >> already ?
>> >
>> > Well patches 6/7 got it working for generic TCG things. I haven't been
>> > able to get the architecture one working but I suspect that is some sort
>> > of interaction between the per-arch trace header generation that I
>> > haven't quite figured out yet.
>> 
>> Ahh it's since 7609ffb919 (trace: fix tcg tracing build breakage) which
>> limited tcg/vcpu events to the root trace-events file.
>
> That commit is from release 2.10.0.
>
> The other commit mentioned in patch 6 (73ff061032) is from 2.12.0.
>
> So no one has been able to use this feature for 3+ years already.
>
> Is it actually worth fixing and then deprecating for 2 releases before
> deleting, as opposed to just deleting the broken code today on basis
> that it can't have any current users ?

Well I can get it up and running with the aforementioned patches and it
seems reasonable to give some notice. I'm happy to defer to Stefan here
though as it's his sub-system.

>
> Regards,
> Daniel


-- 
Alex Bennée

Re: [PATCH v1 5/7] docs: mark intention to deprecate TCG tracing functionality

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

On Mon, May 17, 2021 at 11:47:11AM +0100, Alex Bennée wrote:
> 
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Wed, May 05, 2021 at 11:41:46AM +0100, Alex Bennée wrote:
> >> 
> >> Alex Bennée <alex.bennee@linaro.org> writes:
> >> 
> >> > Daniel P. Berrangé <berrange@redhat.com> writes:
> >> >
> >> >> On Wed, May 05, 2021 at 10:22:57AM +0100, Alex Bennée wrote:
> >> <snip>
> >> >>> +TCG introspection features
> >> >>> +--------------------------
> >> >>> +
> >> >>> +TCG trace-events (since 6.1)
> >> >>> +''''''''''''''''''''''''''''
> >> >>> +
> >> >>> +The ability to add new TCG trace points has bit rotted and as the
> >> >>
> >> >> When you say this "has bit rotted", just how bad is the situation ?
> >> >>
> >> >> Is the TCG tracing still usable at all, or is is fully broken
> >> >> already ?
> >> >
> >> > Well patches 6/7 got it working for generic TCG things. I haven't been
> >> > able to get the architecture one working but I suspect that is some sort
> >> > of interaction between the per-arch trace header generation that I
> >> > haven't quite figured out yet.
> >> 
> >> Ahh it's since 7609ffb919 (trace: fix tcg tracing build breakage) which
> >> limited tcg/vcpu events to the root trace-events file.
> >
> > That commit is from release 2.10.0.
> >
> > The other commit mentioned in patch 6 (73ff061032) is from 2.12.0.
> >
> > So no one has been able to use this feature for 3+ years already.
> >
> > Is it actually worth fixing and then deprecating for 2 releases before
> > deleting, as opposed to just deleting the broken code today on basis
> > that it can't have any current users ?
> 
> Well I can get it up and running with the aforementioned patches and it
> seems reasonable to give some notice. I'm happy to defer to Stefan here
> though as it's his sub-system.

Lluís Vilanova was the author and probably main user. He mentioned he's
been away from QEMU for a while.

If you want to drop the feature, I think that's fine since it has
already been broken for over 3 years. If someone wants it back then it
can be added via TCG plugins in the future.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v1 6/7] scripts/tracetool: don't barf validating TCG types

[Alex Bennée]

TCG types will be transformed into the appropriate host types later on
in the tool. For example adding the following trace point:

  tcg goto_ptr(TCGv_ptr ptr) "", "ptr=%p"

would trigger:

  ValueError: Error at /home/alex/lsrc/qemu.git/./trace-events:149: Argument type 'TCGv_ptr' is not allowed. Only standard C types and fixed size integer types should be used. struct, union, and other complex pointer types should be declared as 'void *'

Rather than expand ALLOWED_TYPES just directly handle TCGv types in validate_type.

Fixes: 73ff061032 ("trace: only permit standard C types and fixed size integer types")
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Matheus Ferst <matheus.ferst@eldorado.org.br>
Message-Id: <20210406165307.5993-1-alex.bennee@linaro.org>

---
v2
  - do workaround directly in validate_type
---
 scripts/tracetool/__init__.py | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 5bc94d95cf..ce5fd23191 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -87,10 +87,9 @@ def out(*lines, **kwargs):
     "ssize_t",
     "uintptr_t",
     "ptrdiff_t",
-    # Magic substitution is done by tracetool
-    "TCGv",
 ]
 
+
 def validate_type(name):
     bits = name.split(" ")
     for bit in bits:
@@ -99,6 +98,10 @@ def validate_type(name):
             continue
         if bit == "const":
             continue
+        # Magic substitution of TCGv types will be done later
+        # using tracetool.transform.TCG_2_HOST
+        if bit.startswith("TCGv") and bit != "TCGv_vec":
+            continue
         if bit not in ALLOWED_TYPES:
             raise ValueError("Argument type '%s' is not allowed. "
                              "Only standard C types and fixed size integer "
-- 
2.20.1

[PATCH v1 7/7] tcg: add trace events for [exit|goto]_tb and goto_ptr

[Alex Bennée]

These are generic trace points in common helper functions used by all
front ends. They mainly serve as additional in-tree examples of TCG
trace points and can be used to compare and contrast with getting
similar information from the TCG plugins.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tcg/tcg-op.c |  8 ++++++++
 trace-events | 12 ++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 70475773f4..46b00f6e9b 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -2687,6 +2687,8 @@ void tcg_gen_exit_tb(const TranslationBlock *tb, unsigned idx)
         tcg_debug_assert(idx == TB_EXIT_REQUESTED);
     }
 
+    trace_exit_tb_tcg(val & ~TB_EXIT_MASK, val & TB_EXIT_MASK);
+
     plugin_gen_disable_mem_helpers();
     tcg_gen_op1i(INDEX_op_exit_tb, val);
 }
@@ -2703,6 +2705,9 @@ void tcg_gen_goto_tb(unsigned idx)
     plugin_gen_disable_mem_helpers();
     /* When not chaining, we simply fall through to the "fallback" exit.  */
     if (!qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
+
+        trace_goto_tb_tcg(idx);
+
         tcg_gen_op1i(INDEX_op_goto_tb, idx);
     }
 }
@@ -2715,6 +2720,9 @@ void tcg_gen_lookup_and_goto_ptr(void)
         plugin_gen_disable_mem_helpers();
         ptr = tcg_temp_new_ptr();
         gen_helper_lookup_tb_ptr(ptr, cpu_env);
+
+        trace_goto_ptr_tcg(ptr);
+
         tcg_gen_op1i(INDEX_op_goto_ptr, tcgv_ptr_arg(ptr));
         tcg_temp_free_ptr(ptr);
     } else {
diff --git a/trace-events b/trace-events
index ac7cef9335..1fcb8c4dda 100644
--- a/trace-events
+++ b/trace-events
@@ -136,6 +136,18 @@ vcpu guest_cpu_reset(void)
 # Targets: TCG(all)
 vcpu tcg guest_mem_before(TCGv vaddr, uint16_t info) "info=%d", "vaddr=0x%016"PRIx64" info=%d"
 
+# Mode: user, softmmu
+# Targets: TCG(all)
+tcg exit_tb(uint64_t ptr, uint64_t idx) "tb=0x%016"PRIx64"/%"PRId64"", "tb=0x%016"PRIx64"/%"PRId64""
+
+# Mode: user, softmmu
+# Targets: TCG(all)
+tcg goto_tb(uint64_t idx) "idx=%"PRId64"", "idx=%"PRId64""
+
+# Mode: user, softmmu
+# Targets: TCG(all)
+tcg goto_ptr(TCGv_ptr ptr) "", "ptr=%p"
+
 # include/user/syscall-trace.h
 
 # @num: System call number.
-- 
2.20.1