[PATCH v4 8/9] plugins: Remove use of qemu_plugin_read_register where it is not permitted

Rowan Hart posted 9 patches 7 months ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Alexandre Iooss <erdnaxe@crans.org>, Mahmoud Mandour <ma.mandourr@gmail.com>, Pierrick Bouvier <pierrick.bouvier@linaro.org>, Eduardo Habkost <eduardo@habkost.net>
There is a newer version of this series
[PATCH v4 8/9] plugins: Remove use of qemu_plugin_read_register where it is not permitted
Posted by Rowan Hart 7 months ago
This patch is required to make the insn plugin work after adding
enforcement of QEMU_PLUGIN_CB_ flags in calls to read or write
registers. Previously, these flags were not enforced and the API could
be called from anywhere, but this was not intended as described by the
documentation. Now, the flags are enforced and qemu_plugin_read_register
can no longer be called from a vcpu_init callback because it does not
request the QEMU_PLUGIN_CB_ flag (nor does it have a mechanism to do
so).

Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
---
 tests/tcg/plugins/insn.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/tests/tcg/plugins/insn.c b/tests/tcg/plugins/insn.c
index 0c723cb9ed..265d3ebe9e 100644
--- a/tests/tcg/plugins/insn.c
+++ b/tests/tcg/plugins/insn.c
@@ -81,25 +81,6 @@ static Instruction * get_insn_record(const char *disas, uint64_t vaddr, Match *m
     return record;
 }
 
-/*
- * Initialise a new vcpu with reading the register list
- */
-static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
-{
-    g_autoptr(GArray) reg_list = qemu_plugin_get_registers();
-    g_autoptr(GByteArray) reg_value = g_byte_array_new();
-
-    if (reg_list) {
-        for (int i = 0; i < reg_list->len; i++) {
-            qemu_plugin_reg_descriptor *rd = &g_array_index(
-                reg_list, qemu_plugin_reg_descriptor, i);
-            int count = qemu_plugin_read_register(rd->handle, reg_value);
-            g_assert(count > 0);
-        }
-    }
-}
-
-
 static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata)
 {
     qemu_plugin_u64_add(insn_count, cpu_index, 1);
@@ -295,8 +276,7 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
     insn_count = qemu_plugin_scoreboard_u64(
         qemu_plugin_scoreboard_new(sizeof(uint64_t)));
 
-    /* Register init, translation block and exit callbacks */
-    qemu_plugin_register_vcpu_init_cb(id, vcpu_init);
+    /* Register translation block and exit callbacks */
     qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
     qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
     return 0;
-- 
2.49.0
Re: [PATCH v4 8/9] plugins: Remove use of qemu_plugin_read_register where it is not permitted
Posted by Pierrick Bouvier 6 months, 3 weeks ago
On 5/22/25 7:03 PM, Rowan Hart wrote:
> This patch is required to make the insn plugin work after adding
> enforcement of QEMU_PLUGIN_CB_ flags in calls to read or write
> registers. Previously, these flags were not enforced and the API could
> be called from anywhere, but this was not intended as described by the
> documentation. Now, the flags are enforced and qemu_plugin_read_register
> can no longer be called from a vcpu_init callback because it does not
> request the QEMU_PLUGIN_CB_ flag (nor does it have a mechanism to do
> so).
> 
> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
> ---
>   tests/tcg/plugins/insn.c | 22 +---------------------
>   1 file changed, 1 insertion(+), 21 deletions(-)
> 
> diff --git a/tests/tcg/plugins/insn.c b/tests/tcg/plugins/insn.c
> index 0c723cb9ed..265d3ebe9e 100644
> --- a/tests/tcg/plugins/insn.c
> +++ b/tests/tcg/plugins/insn.c
> @@ -81,25 +81,6 @@ static Instruction * get_insn_record(const char *disas, uint64_t vaddr, Match *m
>       return record;
>   }
>   
> -/*
> - * Initialise a new vcpu with reading the register list
> - */
> -static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
> -{
> -    g_autoptr(GArray) reg_list = qemu_plugin_get_registers();
> -    g_autoptr(GByteArray) reg_value = g_byte_array_new();
> -
> -    if (reg_list) {
> -        for (int i = 0; i < reg_list->len; i++) {
> -            qemu_plugin_reg_descriptor *rd = &g_array_index(
> -                reg_list, qemu_plugin_reg_descriptor, i);
> -            int count = qemu_plugin_read_register(rd->handle, reg_value);
> -            g_assert(count > 0);
> -        }
> -    }
> -}
> -
> -
>   static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata)
>   {
>       qemu_plugin_u64_add(insn_count, cpu_index, 1);
> @@ -295,8 +276,7 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
>       insn_count = qemu_plugin_scoreboard_u64(
>           qemu_plugin_scoreboard_new(sizeof(uint64_t)));
>   
> -    /* Register init, translation block and exit callbacks */
> -    qemu_plugin_register_vcpu_init_cb(id, vcpu_init);
> +    /* Register translation block and exit callbacks */
>       qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
>       qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
>       return 0;

Let's see what Alex thinks about this, as he asked to enforce those 
flags, and may have an idea about how to deal with this.