qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/17] TCG Plugin inline operation enhancement
@ 2024-02-06  9:24 Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 01/17] plugins: remove previous n_vcpus functions from API Pierrick Bouvier
                   ` (17 more replies)
  0 siblings, 18 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

This series adds a new thread-safe API to declare inline operation
inside plugins. As well, it removes the existing non thread-safe API,
and migrates all existing plugins to use it.

Tested on Linux (user, system) for i386, x86_64 and aarch64.

To give some context, this a long term series of work around plugins,
with the goal to be able to do basic operations in a more performant and
accurate way. This will mean to add more inline operations and
conditional callbacks.

One final target of this work is to implement a plugin that implements
the icount=auto feature, and allow QEMU to run at a given "frequency"
based on number of instructions executed, without QEMU needing to keep
track of this.

Another final target is to be able to detect control flow changes in an
efficient and elegant way, by combining inline operation and conditional
callbacks.

v2
--

Implement scoreboard API (cpu local storage), so plugins don't have to deal
with how many cpus are used.

Since plugins have been modified again, I didn't transfer any reviewed-by on
those commits.

v3
--

- introduce qemu_plugin_num_vcpus (how many cpus were initialized)
- fix order of plugin init/idle/resume callbacks
- scoreboard:
  - renamed qemu_plugin_u64_t -> qemu_plugin_u64
  - some functions rename for scoreboard api
  - qemu_plugin_u64 has only value based function (vs address before)
- various cleanup thanks to review of previous series

Based-on: 20240122145610.413836-1-alex.bennee@linaro.org

 Version number: 3
 Branches:
          base:  plugin_registers_v3_20240122145610.413836-1-alex.bennee@linaro.org
          topic: plugin_inline_per_vcpu

 To: qemu-devel@nongnu.org

 Pierrick Bouvier (17):
       plugins: remove previous n_vcpus functions from API
       plugins: add qemu_plugin_num_vcpus function
       plugins: fix order of init/idle/resume callback
       cpu: call plugin init hook asynchronously
       plugins: scoreboard API
       docs/devel: plugins can trigger a tb flush
       plugins: implement inline operation relative to cpu_index
       plugins: add inline operation per vcpu
       tests/plugin: add test plugin for inline operations
       tests/plugin/mem: migrate to new per_vcpu API
       tests/plugin/insn: migrate to new per_vcpu API
       tests/plugin/bb: migrate to new per_vcpu API
       contrib/plugins/hotblocks: migrate to new per_vcpu API
       contrib/plugins/howvec: migrate to new per_vcpu API
       plugins: remove non per_vcpu inline operation from API
       plugins: cleanup codepath for previous inline operation
       MAINTAINERS: Add myself as reviewer for TCG Plugins

  MAINTAINERS                     |   1 +
  docs/devel/multi-thread-tcg.rst |   1 +
  include/qemu/plugin.h           |   6 ++
  include/qemu/qemu-plugin.h      | 151 ++++++++++++++++++++++++--------
  plugins/plugin.h                |  24 ++++--
  accel/tcg/plugin-gen.c          |  65 +++++++++++---
  contrib/plugins/cache.c         |   2 +-
  contrib/plugins/hotblocks.c     |  50 ++++++-----
  contrib/plugins/howvec.c        |  53 ++++++++----
  hw/core/cpu-common.c            |   9 +-
  plugins/api.c                   | 121 ++++++++++++++++----------
  plugins/core.c                  | 104 +++++++++++++++++++---
  tests/plugin/bb.c               |  63 ++++++--------
  tests/plugin/inline.c           | 186 ++++++++++++++++++++++++++++++++++++++++
  tests/plugin/insn.c             | 106 +++++++++++------------
  tests/plugin/mem.c              |  40 +++++----
  plugins/qemu-plugins.symbols    |  16 ++--
  tests/plugin/meson.build        |   2 +-
  18 files changed, 745 insertions(+), 255 deletions(-)

Pierrick Bouvier (17):
  plugins: remove previous n_vcpus functions from API
  plugins: add qemu_plugin_num_vcpus function
  plugins: fix order of init/idle/resume callback
  cpu: call plugin init hook asynchronously
  plugins: scoreboard API
  docs/devel: plugins can trigger a tb flush
  plugins: implement inline operation relative to cpu_index
  plugins: add inline operation per vcpu
  tests/plugin: add test plugin for inline operations
  tests/plugin/mem: migrate to new per_vcpu API
  tests/plugin/insn: migrate to new per_vcpu API
  tests/plugin/bb: migrate to new per_vcpu API
  contrib/plugins/hotblocks: migrate to new per_vcpu API
  contrib/plugins/howvec: migrate to new per_vcpu API
  plugins: remove non per_vcpu inline operation from API
  plugins: cleanup codepath for previous inline operation
  MAINTAINERS: Add myself as reviewer for TCG Plugins

 MAINTAINERS                     |   1 +
 docs/devel/multi-thread-tcg.rst |   1 +
 include/qemu/plugin.h           |   6 ++
 include/qemu/qemu-plugin.h      | 151 ++++++++++++++++++++------
 plugins/plugin.h                |  24 +++--
 accel/tcg/plugin-gen.c          |  65 +++++++++--
 contrib/plugins/cache.c         |   2 +-
 contrib/plugins/hotblocks.c     |  50 +++++----
 contrib/plugins/howvec.c        |  53 ++++++---
 hw/core/cpu-common.c            |   9 +-
 plugins/api.c                   | 121 +++++++++++++--------
 plugins/core.c                  | 104 ++++++++++++++++--
 tests/plugin/bb.c               |  63 +++++------
 tests/plugin/inline.c           | 186 ++++++++++++++++++++++++++++++++
 tests/plugin/insn.c             | 106 +++++++++---------
 tests/plugin/mem.c              |  40 ++++---
 plugins/qemu-plugins.symbols    |  16 ++-
 tests/plugin/meson.build        |   2 +-
 18 files changed, 745 insertions(+), 255 deletions(-)
 create mode 100644 tests/plugin/inline.c

-- 
2.43.0



^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v3 01/17] plugins: remove previous n_vcpus functions from API
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 02/17] plugins: add qemu_plugin_num_vcpus function Pierrick Bouvier
                   ` (16 subsequent siblings)
  17 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

This information is already accessible using qemu_info_t during plugin
install.

We will introduce another function (qemu_plugin_num_vcpus) which
represent how many cpus were enabled, by tracking new cpu indexes.

It's a breaking change, so we bump API version.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/qemu/qemu-plugin.h   | 10 +++-------
 plugins/plugin.h             |  2 +-
 contrib/plugins/cache.c      |  2 +-
 plugins/api.c                | 30 ------------------------------
 plugins/qemu-plugins.symbols |  2 --
 5 files changed, 5 insertions(+), 41 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 2c1930e7e45..0e506ee644d 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -51,11 +51,13 @@ typedef uint64_t qemu_plugin_id_t;
  *
  * The plugins export the API they were built against by exposing the
  * symbol qemu_plugin_version which can be checked.
+ *
+ * version 2: removed qemu_plugin_n_vcpus and qemu_plugin_n_max_vcpus
  */
 
 extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
 
-#define QEMU_PLUGIN_VERSION 1
+#define QEMU_PLUGIN_VERSION 2
 
 /**
  * struct qemu_info_t - system information for plugins
@@ -644,12 +646,6 @@ QEMU_PLUGIN_API
 void qemu_plugin_register_atexit_cb(qemu_plugin_id_t id,
                                     qemu_plugin_udata_cb_t cb, void *userdata);
 
-/* returns -1 in user-mode */
-int qemu_plugin_n_vcpus(void);
-
-/* returns -1 in user-mode */
-int qemu_plugin_n_max_vcpus(void);
-
 /**
  * qemu_plugin_outs() - output string via QEMU's logging system
  * @string: a string
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 5eb2fdbc85e..90f3f324ab6 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -15,7 +15,7 @@
 #include <gmodule.h>
 #include "qemu/qht.h"
 
-#define QEMU_PLUGIN_MIN_VERSION 0
+#define QEMU_PLUGIN_MIN_VERSION 2
 
 /* global state */
 struct qemu_plugin_state {
diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
index 9e7ade3b374..c5c8ac75a9c 100644
--- a/contrib/plugins/cache.c
+++ b/contrib/plugins/cache.c
@@ -767,7 +767,7 @@ int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
 
     policy = LRU;
 
-    cores = sys ? qemu_plugin_n_vcpus() : 1;
+    cores = sys ? info->system.smp_vcpus : 1;
 
     for (i = 0; i < argc; i++) {
         char *opt = argv[i];
diff --git a/plugins/api.c b/plugins/api.c
index 8d5cca53295..2aaae4e47f3 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -353,36 +353,6 @@ const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h)
 #endif
 }
 
-/*
- * Queries to the number and potential maximum number of vCPUs there
- * will be. This helps the plugin dimension per-vcpu arrays.
- */
-
-#ifndef CONFIG_USER_ONLY
-static MachineState * get_ms(void)
-{
-    return MACHINE(qdev_get_machine());
-}
-#endif
-
-int qemu_plugin_n_vcpus(void)
-{
-#ifdef CONFIG_USER_ONLY
-    return -1;
-#else
-    return get_ms()->smp.cpus;
-#endif
-}
-
-int qemu_plugin_n_max_vcpus(void)
-{
-#ifdef CONFIG_USER_ONLY
-    return -1;
-#else
-    return get_ms()->smp.max_cpus;
-#endif
-}
-
 /*
  * Plugin output
  */
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index 6963585c1ea..6088dd3714a 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -17,8 +17,6 @@
   qemu_plugin_mem_is_sign_extended;
   qemu_plugin_mem_is_store;
   qemu_plugin_mem_size_shift;
-  qemu_plugin_n_max_vcpus;
-  qemu_plugin_n_vcpus;
   qemu_plugin_outs;
   qemu_plugin_path_to_binary;
   qemu_plugin_read_register;
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 02/17] plugins: add qemu_plugin_num_vcpus function
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 01/17] plugins: remove previous n_vcpus functions from API Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 03/17] plugins: fix order of init/idle/resume callback Pierrick Bouvier
                   ` (15 subsequent siblings)
  17 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

We now keep track of how many vcpus were started. This way, a plugin can
easily query number of any vcpus at any point of execution, which
unifies user and system mode workflows.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/qemu/qemu-plugin.h   | 3 +++
 plugins/plugin.h             | 4 ++++
 plugins/api.c                | 5 +++++
 plugins/core.c               | 6 ++++++
 plugins/qemu-plugins.symbols | 1 +
 5 files changed, 19 insertions(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 0e506ee644d..e94ae4d2abb 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -646,6 +646,9 @@ QEMU_PLUGIN_API
 void qemu_plugin_register_atexit_cb(qemu_plugin_id_t id,
                                     qemu_plugin_udata_cb_t cb, void *userdata);
 
+/* returns how many vcpus were started at this point */
+int qemu_plugin_num_vcpus(void);
+
 /**
  * qemu_plugin_outs() - output string via QEMU's logging system
  * @string: a string
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 90f3f324ab6..00b3509f708 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -44,6 +44,8 @@ struct qemu_plugin_state {
      * the code cache is flushed.
      */
     struct qht dyn_cb_arr_ht;
+    /* How many vcpus were started */
+    int num_vcpus;
 };
 
 
@@ -97,4 +99,6 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
 
 void exec_inline_op(struct qemu_plugin_dyn_cb *cb);
 
+int plugin_num_vcpus(void);
+
 #endif /* PLUGIN_H */
diff --git a/plugins/api.c b/plugins/api.c
index 2aaae4e47f3..dbfc5e83729 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -353,6 +353,11 @@ const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h)
 #endif
 }
 
+int qemu_plugin_num_vcpus(void)
+{
+    return plugin_num_vcpus();
+}
+
 /*
  * Plugin output
  */
diff --git a/plugins/core.c b/plugins/core.c
index 49588285dd0..9e64820ad02 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -214,6 +214,7 @@ void qemu_plugin_vcpu_init_hook(CPUState *cpu)
     bool success;
 
     qemu_rec_mutex_lock(&plugin.lock);
+    plugin.num_vcpus = MAX(plugin.num_vcpus, cpu->cpu_index + 1);
     plugin_cpu_update__locked(&cpu->cpu_index, NULL, NULL);
     success = g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index,
                                   &cpu->cpu_index);
@@ -571,3 +572,8 @@ static void __attribute__((__constructor__)) plugin_init(void)
              QHT_MODE_AUTO_RESIZE);
     atexit(qemu_plugin_atexit_cb);
 }
+
+int plugin_num_vcpus(void)
+{
+    return plugin.num_vcpus;
+}
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index 6088dd3714a..27fe97239be 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -17,6 +17,7 @@
   qemu_plugin_mem_is_sign_extended;
   qemu_plugin_mem_is_store;
   qemu_plugin_mem_size_shift;
+  qemu_plugin_num_vcpus;
   qemu_plugin_outs;
   qemu_plugin_path_to_binary;
   qemu_plugin_read_register;
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 03/17] plugins: fix order of init/idle/resume callback
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 01/17] plugins: remove previous n_vcpus functions from API Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 02/17] plugins: add qemu_plugin_num_vcpus function Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-07  2:59   ` Richard Henderson
  2024-02-06  9:24 ` [PATCH v3 04/17] cpu: call plugin init hook asynchronously Pierrick Bouvier
                   ` (14 subsequent siblings)
  17 siblings, 1 reply; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

We found that vcpu_init_hook was called *after* idle callback.
vcpu_init is called from cpu_realize_fn, while idle/resume cb are called
from qemu_wait_io_event (in vcpu thread).

This change ensures we only call idle and resume cb only once a plugin
was init for a given vcpu.

Next change in the series will run vcpu_init asynchronously, which will
make it run *after* resume callback as well. So we fix this now.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 plugins/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/plugins/core.c b/plugins/core.c
index 9e64820ad02..609d9d5c184 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -392,12 +392,17 @@ void qemu_plugin_vcpu_syscall_ret(CPUState *cpu, int64_t num, int64_t ret)
 
 void qemu_plugin_vcpu_idle_cb(CPUState *cpu)
 {
-    plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_IDLE);
+    /* idle and resume cb may be called before init, ignore in this case */
+    if (cpu->cpu_index < plugin.num_vcpus) {
+        plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_IDLE);
+    }
 }
 
 void qemu_plugin_vcpu_resume_cb(CPUState *cpu)
 {
-    plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_RESUME);
+    if (cpu->cpu_index < plugin.num_vcpus) {
+        plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_RESUME);
+    }
 }
 
 void qemu_plugin_register_vcpu_idle_cb(qemu_plugin_id_t id,
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 04/17] cpu: call plugin init hook asynchronously
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (2 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 03/17] plugins: fix order of init/idle/resume callback Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-07  3:00   ` Richard Henderson
  2024-02-06  9:24 ` [PATCH v3 05/17] plugins: scoreboard API Pierrick Bouvier
                   ` (13 subsequent siblings)
  17 siblings, 1 reply; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

This ensures we run during a cpu_exec, which allows to call start/end
exclusive from this init hook (needed for new scoreboard API introduced
later).

async work is run before any tb is translated/executed, so we can
guarantee plugin init will be called before any other hook.

The previous change made sure that any idle/resume cb call will not be
done before initializing plugin for a given vcpu.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 hw/core/cpu-common.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index fe16d0d9df8..68786360ea5 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -194,6 +194,11 @@ static void cpu_common_parse_features(const char *typename, char *features,
     }
 }
 
+static void qemu_plugin_vcpu_init__async(CPUState *cpu, run_on_cpu_data unused)
+{
+    qemu_plugin_vcpu_init_hook(cpu);
+}
+
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cpu = CPU(dev);
@@ -217,9 +222,9 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
         cpu_resume(cpu);
     }
 
-    /* Plugin initialization must wait until the cpu is fully realized. */
+    /* Plugin initialization must wait until the cpu start executing code */
     if (tcg_enabled()) {
-        qemu_plugin_vcpu_init_hook(cpu);
+        async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
     }
 
     /* NOTE: latest generic point where the cpu is fully realized */
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 05/17] plugins: scoreboard API
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (3 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 04/17] cpu: call plugin init hook asynchronously Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-07  3:21   ` Richard Henderson
  2024-02-06  9:24 ` [PATCH v3 06/17] docs/devel: plugins can trigger a tb flush Pierrick Bouvier
                   ` (12 subsequent siblings)
  17 siblings, 1 reply; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

We introduce a cpu local storage, automatically managed (and extended)
by QEMU itself. Plugin allocate a scoreboard, and don't have to deal
with how many cpus are launched.

This API will be used by new inline functions but callbacks can benefit
from this as well. This way, they can operate without a global lock for
simple operations.

At any point during execution, any scoreboard will be dimensioned with
at least qemu_plugin_num_vcpus entries.

New functions:
- qemu_plugin_scoreboard_find
- qemu_plugin_scoreboard_free
- qemu_plugin_scoreboard_new

In more, we define a qemu_plugin_u64, which is a simple struct holding
a pointer to a scoreboard, and a given offset.
This allows to have a scoreboard containing structs, without having to
bring offset for all operations on a specific field.

Since most of the plugins are simply collecting a sum of per-cpu values,
qemu_plugin_u64 directly support this operation as well.

New functions:
- qemu_plugin_u64_add
- qemu_plugin_u64_get
- qemu_plugin_u64_set
- qemu_plugin_u64_sum
New macros:
- qemu_plugin_scoreboard_u64
- qemu_plugin_scoreboard_u64_in_struct

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/qemu/plugin.h        |  5 +++
 include/qemu/qemu-plugin.h   | 83 ++++++++++++++++++++++++++++++++++++
 plugins/plugin.h             |  7 +++
 plugins/api.c                | 53 +++++++++++++++++++++++
 plugins/core.c               | 67 +++++++++++++++++++++++++++++
 plugins/qemu-plugins.symbols |  7 +++
 6 files changed, 222 insertions(+)

diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index b0c5ac68293..af4aeef4d78 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -112,6 +112,11 @@ struct qemu_plugin_insn {
     bool mem_only;
 };
 
+/* A scoreboard is an array of values, indexed by vcpu_index */
+struct qemu_plugin_scoreboard {
+    GArray *data;
+};
+
 /*
  * qemu_plugin_insn allocate and cleanup functions. We don't expect to
  * cleanup many of these structures. They are reused for each fresh
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index e94ae4d2abb..a48586ef0c1 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -222,6 +222,19 @@ void qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id,
 struct qemu_plugin_tb;
 /** struct qemu_plugin_insn - Opaque handle for a translated instruction */
 struct qemu_plugin_insn;
+/** struct qemu_plugin_scoreboard - Opaque handle for a scoreboard */
+struct qemu_plugin_scoreboard;
+
+/**
+ * typedef qemu_plugin_u64 - uint64_t member of an entry in a scoreboard
+ *
+ * This field allows to access a specific uint64_t member in one given entry,
+ * located at a specified offset. Inline operations expect this as entry.
+ */
+typedef struct {
+    struct qemu_plugin_scoreboard *score;
+    size_t offset;
+} qemu_plugin_u64;
 
 /**
  * enum qemu_plugin_cb_flags - type of callback
@@ -753,5 +766,75 @@ int qemu_plugin_read_register(unsigned int vcpu,
                               struct qemu_plugin_register *handle,
                               GByteArray *buf);
 
+/**
+ * qemu_plugin_scoreboard_new() - alloc a new scoreboard
+ *
+ * @element_size: size (in bytes) for one entry
+ *
+ * Returns a pointer to a new scoreboard. It must be freed using
+ * qemu_plugin_scoreboard_free.
+ */
+QEMU_PLUGIN_API
+struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size);
+
+/**
+ * qemu_plugin_scoreboard_free() - free a scoreboard
+ * @score: scoreboard to free
+ */
+QEMU_PLUGIN_API
+void qemu_plugin_scoreboard_free(struct qemu_plugin_scoreboard *score);
+
+/**
+ * qemu_plugin_scoreboard_find() - get pointer to an entry of a scoreboard
+ * @score: scoreboard to query
+ * @vcpu_index: entry index
+ *
+ * Returns address of entry of a scoreboard matching a given vcpu_index. This
+ * address can be modified later if scoreboard is resized.
+ */
+QEMU_PLUGIN_API
+void *qemu_plugin_scoreboard_find(struct qemu_plugin_scoreboard *score,
+                                  unsigned int vcpu_index);
+
+/* Macros to define a qemu_plugin_u64 */
+#define qemu_plugin_scoreboard_u64(score) \
+    (qemu_plugin_u64) {score, 0}
+#define qemu_plugin_scoreboard_u64_in_struct(score, type, member) \
+    (qemu_plugin_u64) {score, offsetof(type, member)}
+
+/**
+ * qemu_plugin_u64_add() - add a value to a qemu_plugin_u64 for a given vcpu
+ * @entry: entry to query
+ * @vcpu_index: entry index
+ * @added: value to add
+ */
+QEMU_PLUGIN_API
+void qemu_plugin_u64_add(qemu_plugin_u64 entry, unsigned int vcpu_index,
+                         uint64_t added);
+
+/**
+ * qemu_plugin_u64_get() - get value of a qemu_plugin_u64 for a given vcpu
+ * @entry: entry to query
+ * @vcpu_index: entry index
+ */
+QEMU_PLUGIN_API
+uint64_t qemu_plugin_u64_get(qemu_plugin_u64 entry, unsigned int vcpu_index);
+
+/**
+ * qemu_plugin_u64_set() - set value of a qemu_plugin_u64 for a given vcpu
+ * @entry: entry to query
+ * @vcpu_index: entry index
+ * @val: new value
+ */
+QEMU_PLUGIN_API
+void qemu_plugin_u64_set(qemu_plugin_u64 entry, unsigned int vcpu_index,
+                         uint64_t val);
+
+/**
+ * qemu_plugin_u64_sum() - return sum of all vcpu entries in a scoreboard
+ * @entry: entry to sum
+ */
+QEMU_PLUGIN_API
+uint64_t qemu_plugin_u64_sum(qemu_plugin_u64 entry);
 
 #endif /* QEMU_QEMU_PLUGIN_H */
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 00b3509f708..fd93a372803 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -31,6 +31,9 @@ struct qemu_plugin_state {
      * but with the HT we avoid adding a field to CPUState.
      */
     GHashTable *cpu_ht;
+    /* Scoreboards, indexed by their addresses. */
+    GHashTable *scoreboards;
+    size_t scoreboard_alloc_size;
     DECLARE_BITMAP(mask, QEMU_PLUGIN_EV_MAX);
     /*
      * @lock protects the struct as well as ctx->uninstalling.
@@ -101,4 +104,8 @@ void exec_inline_op(struct qemu_plugin_dyn_cb *cb);
 
 int plugin_num_vcpus(void);
 
+struct qemu_plugin_scoreboard *plugin_scoreboard_new(size_t element_size);
+
+void plugin_scoreboard_free(struct qemu_plugin_scoreboard *score);
+
 #endif /* PLUGIN_H */
diff --git a/plugins/api.c b/plugins/api.c
index dbfc5e83729..15edad6769b 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -521,3 +521,56 @@ static void __attribute__((__constructor__)) qemu_api_init(void)
     qemu_mutex_init(&reg_handle_lock);
 
 }
+
+struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size)
+{
+    return plugin_scoreboard_new(element_size);
+}
+
+void qemu_plugin_scoreboard_free(struct qemu_plugin_scoreboard *score)
+{
+    plugin_scoreboard_free(score);
+}
+
+void *qemu_plugin_scoreboard_find(struct qemu_plugin_scoreboard *score,
+                                  unsigned int vcpu_index)
+{
+    g_assert(vcpu_index < qemu_plugin_num_vcpus());
+    /* we can't use g_array_index since entry size is not statically known */
+    char *base_ptr = score->data->data;
+    return base_ptr + vcpu_index * g_array_get_element_size(score->data);
+}
+
+static uint64_t *plugin_u64_address(qemu_plugin_u64 entry,
+                                    unsigned int vcpu_index)
+{
+    char *ptr = qemu_plugin_scoreboard_find(entry.score, vcpu_index);
+    return (uint64_t *)(ptr + entry.offset);
+}
+
+void qemu_plugin_u64_add(qemu_plugin_u64 entry, unsigned int vcpu_index,
+                         uint64_t added)
+{
+    *plugin_u64_address(entry, vcpu_index) += added;
+}
+
+uint64_t qemu_plugin_u64_get(qemu_plugin_u64 entry,
+                             unsigned int vcpu_index)
+{
+    return *plugin_u64_address(entry, vcpu_index);
+}
+
+void qemu_plugin_u64_set(qemu_plugin_u64 entry, unsigned int vcpu_index,
+                         uint64_t val)
+{
+    *plugin_u64_address(entry, vcpu_index) = val;
+}
+
+uint64_t qemu_plugin_u64_sum(qemu_plugin_u64 entry)
+{
+    uint64_t total = 0;
+    for (int i = 0; i < qemu_plugin_num_vcpus(); ++i) {
+        total += qemu_plugin_u64_get(entry, i);
+    }
+    return total;
+}
diff --git a/plugins/core.c b/plugins/core.c
index 609d9d5c184..fd8604bcb79 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -209,6 +209,44 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum qemu_plugin_event ev,
     do_plugin_register_cb(id, ev, func, udata);
 }
 
+static void plugin_resize_one_scoreboard(gpointer key,
+                                         gpointer value,
+                                         gpointer user_data)
+{
+    struct qemu_plugin_scoreboard *score =
+        (struct qemu_plugin_scoreboard *) value;
+    size_t new_alloc_size = (size_t) user_data;
+    g_array_set_size(score->data, new_alloc_size);
+}
+
+static void plugin_grow_scoreboards__locked(CPUState *cpu)
+{
+    if (cpu->cpu_index < plugin.scoreboard_alloc_size) {
+        return;
+    }
+
+    bool need_realloc = FALSE;
+    while (cpu->cpu_index >= plugin.scoreboard_alloc_size) {
+        plugin.scoreboard_alloc_size *= 2;
+        need_realloc = TRUE;
+    }
+
+
+    if (!need_realloc || g_hash_table_size(plugin.scoreboards) == 0) {
+        /* nothing to do, we just updated sizes for future scoreboards */
+        return;
+    }
+
+    /* cpus must be stopped, as tb might still use an existing scoreboard. */
+    start_exclusive();
+    g_hash_table_foreach(plugin.scoreboards,
+                         &plugin_resize_one_scoreboard,
+                         GSIZE_TO_POINTER(plugin.scoreboard_alloc_size));
+    /* force all tb to be flushed, as scoreboard pointers were changed. */
+    tb_flush(cpu);
+    end_exclusive();
+}
+
 void qemu_plugin_vcpu_init_hook(CPUState *cpu)
 {
     bool success;
@@ -219,6 +257,7 @@ void qemu_plugin_vcpu_init_hook(CPUState *cpu)
     success = g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index,
                                   &cpu->cpu_index);
     g_assert(success);
+    plugin_grow_scoreboards__locked(cpu);
     qemu_rec_mutex_unlock(&plugin.lock);
 
     plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_INIT);
@@ -572,6 +611,8 @@ static void __attribute__((__constructor__)) plugin_init(void)
     qemu_rec_mutex_init(&plugin.lock);
     plugin.id_ht = g_hash_table_new(g_int64_hash, g_int64_equal);
     plugin.cpu_ht = g_hash_table_new(g_int_hash, g_int_equal);
+    plugin.scoreboards = g_hash_table_new(g_int64_hash, g_int64_equal);
+    plugin.scoreboard_alloc_size = 16; /* avoid frequent reallocation */
     QTAILQ_INIT(&plugin.ctxs);
     qht_init(&plugin.dyn_cb_arr_ht, plugin_dyn_cb_arr_cmp, 16,
              QHT_MODE_AUTO_RESIZE);
@@ -582,3 +623,29 @@ int plugin_num_vcpus(void)
 {
     return plugin.num_vcpus;
 }
+
+struct qemu_plugin_scoreboard *plugin_scoreboard_new(size_t element_size)
+{
+    struct qemu_plugin_scoreboard *score =
+        g_malloc0(sizeof(struct qemu_plugin_scoreboard));
+    score->data = g_array_new(FALSE, TRUE, element_size);
+    g_array_set_size(score->data, plugin.scoreboard_alloc_size);
+
+    qemu_rec_mutex_lock(&plugin.lock);
+    bool inserted = g_hash_table_insert(plugin.scoreboards, score, score);
+    g_assert(inserted);
+    qemu_rec_mutex_unlock(&plugin.lock);
+
+    return score;
+}
+
+void plugin_scoreboard_free(struct qemu_plugin_scoreboard *score)
+{
+    qemu_rec_mutex_lock(&plugin.lock);
+    bool removed = g_hash_table_remove(plugin.scoreboards, score);
+    g_assert(removed);
+    qemu_rec_mutex_unlock(&plugin.lock);
+
+    g_array_free(score->data, TRUE);
+    g_free(score);
+}
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index 27fe97239be..6204453d0fd 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -37,10 +37,17 @@
   qemu_plugin_register_vcpu_tb_exec_inline;
   qemu_plugin_register_vcpu_tb_trans_cb;
   qemu_plugin_reset;
+  qemu_plugin_scoreboard_free;
+  qemu_plugin_scoreboard_find;
+  qemu_plugin_scoreboard_new;
   qemu_plugin_start_code;
   qemu_plugin_tb_get_insn;
   qemu_plugin_tb_n_insns;
   qemu_plugin_tb_vaddr;
+  qemu_plugin_u64_add;
+  qemu_plugin_u64_get;
+  qemu_plugin_u64_set;
+  qemu_plugin_u64_sum;
   qemu_plugin_uninstall;
   qemu_plugin_vcpu_for_each;
 };
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 06/17] docs/devel: plugins can trigger a tb flush
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (4 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 05/17] plugins: scoreboard API Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 07/17] plugins: implement inline operation relative to cpu_index Pierrick Bouvier
                   ` (11 subsequent siblings)
  17 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

When scoreboards need to be reallocated.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 docs/devel/multi-thread-tcg.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/docs/devel/multi-thread-tcg.rst b/docs/devel/multi-thread-tcg.rst
index 7302c3bf534..1420789fff3 100644
--- a/docs/devel/multi-thread-tcg.rst
+++ b/docs/devel/multi-thread-tcg.rst
@@ -109,6 +109,7 @@ including:
   - debugging operations (breakpoint insertion/removal)
   - some CPU helper functions
   - linux-user spawning its first thread
+  - operations related to TCG Plugins
 
 This is done with the async_safe_run_on_cpu() mechanism to ensure all
 vCPUs are quiescent when changes are being made to shared global
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 07/17] plugins: implement inline operation relative to cpu_index
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (5 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 06/17] docs/devel: plugins can trigger a tb flush Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-07  3:42   ` Richard Henderson
  2024-02-06  9:24 ` [PATCH v3 08/17] plugins: add inline operation per vcpu Pierrick Bouvier
                   ` (10 subsequent siblings)
  17 siblings, 1 reply; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

Instead of working on a fixed memory location, allow to address it based
on cpu_index, an element size and a given offset.
Result address: ptr + offset + cpu_index * element_size.

With this, we can target a member in a struct array from a base pointer.

Current semantic is not modified, thus inline operation still targets
always the same memory location.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 plugins/plugin.h       |  2 +-
 accel/tcg/plugin-gen.c | 65 +++++++++++++++++++++++++++++++++++-------
 plugins/api.c          |  3 +-
 plugins/core.c         | 12 +++++---
 4 files changed, 65 insertions(+), 17 deletions(-)

diff --git a/plugins/plugin.h b/plugins/plugin.h
index fd93a372803..77ed10689ca 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -100,7 +100,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
                                  enum qemu_plugin_mem_rw rw,
                                  void *udata);
 
-void exec_inline_op(struct qemu_plugin_dyn_cb *cb);
+void exec_inline_op(struct qemu_plugin_dyn_cb *cb, int cpu_index);
 
 int plugin_num_vcpus(void);
 
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index b37ce7683e6..68dee4c68d3 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -132,16 +132,28 @@ static void gen_empty_udata_cb_no_rwg(void)
  */
 static void gen_empty_inline_cb(void)
 {
+    TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
+    TCGv_ptr cpu_index_as_ptr = tcg_temp_ebb_new_ptr();
     TCGv_i64 val = tcg_temp_ebb_new_i64();
     TCGv_ptr ptr = tcg_temp_ebb_new_ptr();
 
+    tcg_gen_ld_i32(cpu_index, tcg_env,
+                   -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
+    /* pass an immediate != 0 so that it doesn't get optimized away */
+    tcg_gen_muli_i32(cpu_index, cpu_index, 0xdeadbeef);
+    tcg_gen_ext_i32_ptr(cpu_index_as_ptr, cpu_index);
+
     tcg_gen_movi_ptr(ptr, 0);
+    tcg_gen_add_ptr(ptr, ptr, cpu_index_as_ptr);
     tcg_gen_ld_i64(val, ptr, 0);
     /* pass an immediate != 0 so that it doesn't get optimized away */
     tcg_gen_addi_i64(val, val, 0xdeadface);
+
     tcg_gen_st_i64(val, ptr, 0);
     tcg_temp_free_ptr(ptr);
     tcg_temp_free_i64(val);
+    tcg_temp_free_ptr(cpu_index_as_ptr);
+    tcg_temp_free_i32(cpu_index);
 }
 
 static void gen_empty_mem_cb(TCGv_i64 addr, uint32_t info)
@@ -289,12 +301,37 @@ static TCGOp *copy_const_ptr(TCGOp **begin_op, TCGOp *op, void *ptr)
     return op;
 }
 
+static TCGOp *copy_ld_i32(TCGOp **begin_op, TCGOp *op)
+{
+    return copy_op(begin_op, op, INDEX_op_ld_i32);
+}
+
+static TCGOp *copy_ext_i32_ptr(TCGOp **begin_op, TCGOp *op)
+{
+    if (UINTPTR_MAX == UINT32_MAX) {
+        op = copy_op(begin_op, op, INDEX_op_mov_i32);
+    } else {
+        op = copy_op(begin_op, op, INDEX_op_ext_i32_i64);
+    }
+    return op;
+}
+
+static TCGOp *copy_add_ptr(TCGOp **begin_op, TCGOp *op)
+{
+    if (UINTPTR_MAX == UINT32_MAX) {
+        op = copy_op(begin_op, op, INDEX_op_add_i32);
+    } else {
+        op = copy_op(begin_op, op, INDEX_op_add_i64);
+    }
+    return op;
+}
+
 static TCGOp *copy_ld_i64(TCGOp **begin_op, TCGOp *op)
 {
     if (TCG_TARGET_REG_BITS == 32) {
         /* 2x ld_i32 */
-        op = copy_op(begin_op, op, INDEX_op_ld_i32);
-        op = copy_op(begin_op, op, INDEX_op_ld_i32);
+        op = copy_ld_i32(begin_op, op);
+        op = copy_ld_i32(begin_op, op);
     } else {
         /* ld_i64 */
         op = copy_op(begin_op, op, INDEX_op_ld_i64);
@@ -330,6 +367,13 @@ static TCGOp *copy_add_i64(TCGOp **begin_op, TCGOp *op, uint64_t v)
     return op;
 }
 
+static TCGOp *copy_mul_i32(TCGOp **begin_op, TCGOp *op, uint32_t v)
+{
+    op = copy_op(begin_op, op, INDEX_op_mul_i32);
+    op->args[2] = tcgv_i32_arg(tcg_constant_i32(v));
+    return op;
+}
+
 static TCGOp *copy_st_ptr(TCGOp **begin_op, TCGOp *op)
 {
     if (UINTPTR_MAX == UINT32_MAX) {
@@ -395,18 +439,17 @@ static TCGOp *append_inline_cb(const struct qemu_plugin_dyn_cb *cb,
                                TCGOp *begin_op, TCGOp *op,
                                int *unused)
 {
-    /* const_ptr */
-    op = copy_const_ptr(&begin_op, op, cb->userp);
-
-    /* ld_i64 */
+    char *ptr = cb->userp;
+    size_t elem_size = 0;
+    size_t offset = 0;
+    op = copy_ld_i32(&begin_op, op);
+    op = copy_mul_i32(&begin_op, op, elem_size);
+    op = copy_ext_i32_ptr(&begin_op, op);
+    op = copy_const_ptr(&begin_op, op, ptr + offset);
+    op = copy_add_ptr(&begin_op, op);
     op = copy_ld_i64(&begin_op, op);
-
-    /* add_i64 */
     op = copy_add_i64(&begin_op, op, cb->inline_insn.imm);
-
-    /* st_i64 */
     op = copy_st_i64(&begin_op, op);
-
     return op;
 }
 
diff --git a/plugins/api.c b/plugins/api.c
index 15edad6769b..dedcbdfd30d 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -106,7 +106,8 @@ void qemu_plugin_register_vcpu_tb_exec_inline(struct qemu_plugin_tb *tb,
                                               void *ptr, uint64_t imm)
 {
     if (!tb->mem_only) {
-        plugin_register_inline_op(&tb->cbs[PLUGIN_CB_INLINE], 0, op, ptr, imm);
+        plugin_register_inline_op(&tb->cbs[PLUGIN_CB_INLINE],
+                                  0, op, ptr, imm);
     }
 }
 
diff --git a/plugins/core.c b/plugins/core.c
index fd8604bcb79..863c2e64217 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -320,7 +320,8 @@ static struct qemu_plugin_dyn_cb *plugin_get_dyn_cb(GArray **arr)
 
 void plugin_register_inline_op(GArray **arr,
                                enum qemu_plugin_mem_rw rw,
-                               enum qemu_plugin_op op, void *ptr,
+                               enum qemu_plugin_op op,
+                               void *ptr,
                                uint64_t imm)
 {
     struct qemu_plugin_dyn_cb *dyn_cb;
@@ -476,9 +477,12 @@ void qemu_plugin_flush_cb(void)
     plugin_cb__simple(QEMU_PLUGIN_EV_FLUSH);
 }
 
-void exec_inline_op(struct qemu_plugin_dyn_cb *cb)
+void exec_inline_op(struct qemu_plugin_dyn_cb *cb, int cpu_index)
 {
-    uint64_t *val = cb->userp;
+    char *ptr = cb->userp;
+    size_t elem_size = 0;
+    size_t offset = 0;
+    uint64_t *val = (uint64_t *)(ptr + offset + cpu_index * elem_size);
 
     switch (cb->inline_insn.op) {
     case QEMU_PLUGIN_INLINE_ADD_U64:
@@ -511,7 +515,7 @@ void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
                            vaddr, cb->userp);
             break;
         case PLUGIN_CB_INLINE:
-            exec_inline_op(cb);
+            exec_inline_op(cb, cpu->cpu_index);
             break;
         default:
             g_assert_not_reached();
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 08/17] plugins: add inline operation per vcpu
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (6 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 07/17] plugins: implement inline operation relative to cpu_index Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-07  3:45   ` Richard Henderson
  2024-02-06  9:24 ` [PATCH v3 09/17] tests/plugin: add test plugin for inline operations Pierrick Bouvier
                   ` (9 subsequent siblings)
  17 siblings, 1 reply; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

Extends API with three new functions:
qemu_plugin_register_vcpu_{tb, insn, mem}_exec_inline_per_vcpu().

Those functions takes a qemu_plugin_u64_t as input.

This allows to have a thread-safe and type-safe version of inline
operations.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/qemu/plugin.h        |  1 +
 include/qemu/qemu-plugin.h   | 51 +++++++++++++++++++++++++++++++++++-
 plugins/plugin.h             |  6 +++++
 accel/tcg/plugin-gen.c       |  7 +++++
 plugins/api.c                | 37 +++++++++++++++++++++++++-
 plugins/core.c               | 23 ++++++++++++++++
 plugins/qemu-plugins.symbols |  3 +++
 7 files changed, 126 insertions(+), 2 deletions(-)

diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index af4aeef4d78..0fd72544615 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -92,6 +92,7 @@ struct qemu_plugin_dyn_cb {
     /* fields specific to each dyn_cb type go here */
     union {
         struct {
+            qemu_plugin_u64 entry;
             enum qemu_plugin_op op;
             uint64_t imm;
         } inline_insn;
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index a48586ef0c1..cdf1266d724 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -328,6 +328,22 @@ void qemu_plugin_register_vcpu_tb_exec_inline(struct qemu_plugin_tb *tb,
                                               enum qemu_plugin_op op,
                                               void *ptr, uint64_t imm);
 
+/**
+ * qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu() - execution inline op
+ * @tb: the opaque qemu_plugin_tb handle for the translation
+ * @op: the type of qemu_plugin_op (e.g. ADD_U64)
+ * @entry: entry to run op
+ * @imm: the op data (e.g. 1)
+ *
+ * Insert an inline op on a given scoreboard entry.
+ */
+QEMU_PLUGIN_API
+void qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
+    struct qemu_plugin_tb *tb,
+    enum qemu_plugin_op op,
+    qemu_plugin_u64 entry,
+    uint64_t imm);
+
 /**
  * qemu_plugin_register_vcpu_insn_exec_cb() - register insn execution cb
  * @insn: the opaque qemu_plugin_insn handle for an instruction
@@ -358,6 +374,22 @@ void qemu_plugin_register_vcpu_insn_exec_inline(struct qemu_plugin_insn *insn,
                                                 enum qemu_plugin_op op,
                                                 void *ptr, uint64_t imm);
 
+/**
+ * qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu() - insn exec inline op
+ * @insn: the opaque qemu_plugin_insn handle for an instruction
+ * @op: the type of qemu_plugin_op (e.g. ADD_U64)
+ * @entry: entry to run op
+ * @imm: the op data (e.g. 1)
+ *
+ * Insert an inline op to every time an instruction executes.
+ */
+QEMU_PLUGIN_API
+void qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(
+    struct qemu_plugin_insn *insn,
+    enum qemu_plugin_op op,
+    qemu_plugin_u64 entry,
+    uint64_t imm);
+
 /**
  * qemu_plugin_tb_n_insns() - query helper for number of insns in TB
  * @tb: opaque handle to TB passed to callback
@@ -583,7 +615,24 @@ void qemu_plugin_register_vcpu_mem_inline(struct qemu_plugin_insn *insn,
                                           enum qemu_plugin_op op, void *ptr,
                                           uint64_t imm);
 
-
+/**
+ * qemu_plugin_register_vcpu_mem_inline_per_vcpu() - inline op for mem access
+ * @insn: handle for instruction to instrument
+ * @rw: apply to reads, writes or both
+ * @op: the op, of type qemu_plugin_op
+ * @entry: entry to run op
+ * @imm: immediate data for @op
+ *
+ * This registers a inline op every memory access generated by the
+ * instruction.
+ */
+QEMU_PLUGIN_API
+void qemu_plugin_register_vcpu_mem_inline_per_vcpu(
+    struct qemu_plugin_insn *insn,
+    enum qemu_plugin_mem_rw rw,
+    enum qemu_plugin_op op,
+    qemu_plugin_u64 entry,
+    uint64_t imm);
 
 typedef void
 (*qemu_plugin_vcpu_syscall_cb_t)(qemu_plugin_id_t id, unsigned int vcpu_index,
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 77ed10689ca..1ece1b6707a 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -74,6 +74,12 @@ void plugin_register_inline_op(GArray **arr,
                                enum qemu_plugin_op op, void *ptr,
                                uint64_t imm);
 
+void plugin_register_inline_op_on_entry(GArray **arr,
+                                        enum qemu_plugin_mem_rw rw,
+                                        enum qemu_plugin_op op,
+                                        qemu_plugin_u64 entry,
+                                        uint64_t imm);
+
 void plugin_reset_uninstall(qemu_plugin_id_t id,
                             qemu_plugin_simple_cb_t cb,
                             bool reset);
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 68dee4c68d3..4930e674c7d 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -442,6 +442,13 @@ static TCGOp *append_inline_cb(const struct qemu_plugin_dyn_cb *cb,
     char *ptr = cb->userp;
     size_t elem_size = 0;
     size_t offset = 0;
+    if (!ptr) {
+        /* use inline entry */
+        ptr = cb->inline_insn.entry.score->data->data;
+        elem_size = g_array_get_element_size(cb->inline_insn.entry.score->data);
+        offset = cb->inline_insn.entry.offset;
+    }
+
     op = copy_ld_i32(&begin_op, op);
     op = copy_mul_i32(&begin_op, op, elem_size);
     op = copy_ext_i32_ptr(&begin_op, op);
diff --git a/plugins/api.c b/plugins/api.c
index dedcbdfd30d..be2badda9fa 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -111,6 +111,18 @@ void qemu_plugin_register_vcpu_tb_exec_inline(struct qemu_plugin_tb *tb,
     }
 }
 
+void qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
+    struct qemu_plugin_tb *tb,
+    enum qemu_plugin_op op,
+    qemu_plugin_u64 entry,
+    uint64_t imm)
+{
+    if (!tb->mem_only) {
+        plugin_register_inline_op_on_entry(
+            &tb->cbs[PLUGIN_CB_INLINE], 0, op, entry, imm);
+    }
+}
+
 void qemu_plugin_register_vcpu_insn_exec_cb(struct qemu_plugin_insn *insn,
                                             qemu_plugin_vcpu_udata_cb_t cb,
                                             enum qemu_plugin_cb_flags flags,
@@ -136,6 +148,18 @@ void qemu_plugin_register_vcpu_insn_exec_inline(struct qemu_plugin_insn *insn,
     }
 }
 
+void qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(
+    struct qemu_plugin_insn *insn,
+    enum qemu_plugin_op op,
+    qemu_plugin_u64 entry,
+    uint64_t imm)
+{
+    if (!insn->mem_only) {
+        plugin_register_inline_op_on_entry(
+            &insn->cbs[PLUGIN_CB_INSN][PLUGIN_CB_INLINE], 0, op, entry, imm);
+    }
+}
+
 
 /*
  * We always plant memory instrumentation because they don't finalise until
@@ -148,7 +172,7 @@ void qemu_plugin_register_vcpu_mem_cb(struct qemu_plugin_insn *insn,
                                       void *udata)
 {
     plugin_register_vcpu_mem_cb(&insn->cbs[PLUGIN_CB_MEM][PLUGIN_CB_REGULAR],
-                                    cb, flags, rw, udata);
+                                cb, flags, rw, udata);
 }
 
 void qemu_plugin_register_vcpu_mem_inline(struct qemu_plugin_insn *insn,
@@ -160,6 +184,17 @@ void qemu_plugin_register_vcpu_mem_inline(struct qemu_plugin_insn *insn,
                               rw, op, ptr, imm);
 }
 
+void qemu_plugin_register_vcpu_mem_inline_per_vcpu(
+    struct qemu_plugin_insn *insn,
+    enum qemu_plugin_mem_rw rw,
+    enum qemu_plugin_op op,
+    qemu_plugin_u64 entry,
+    uint64_t imm)
+{
+    plugin_register_inline_op_on_entry(
+        &insn->cbs[PLUGIN_CB_MEM][PLUGIN_CB_INLINE], rw, op, entry, imm);
+}
+
 void qemu_plugin_register_vcpu_tb_trans_cb(qemu_plugin_id_t id,
                                            qemu_plugin_vcpu_tb_trans_cb_t cb)
 {
diff --git a/plugins/core.c b/plugins/core.c
index 863c2e64217..48e351d7631 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -334,6 +334,23 @@ void plugin_register_inline_op(GArray **arr,
     dyn_cb->inline_insn.imm = imm;
 }
 
+void plugin_register_inline_op_on_entry(GArray **arr,
+                                        enum qemu_plugin_mem_rw rw,
+                                        enum qemu_plugin_op op,
+                                        qemu_plugin_u64 entry,
+                                        uint64_t imm)
+{
+    struct qemu_plugin_dyn_cb *dyn_cb;
+
+    dyn_cb = plugin_get_dyn_cb(arr);
+    dyn_cb->userp = NULL;
+    dyn_cb->type = PLUGIN_CB_INLINE;
+    dyn_cb->rw = rw;
+    dyn_cb->inline_insn.entry = entry;
+    dyn_cb->inline_insn.op = op;
+    dyn_cb->inline_insn.imm = imm;
+}
+
 void plugin_register_dyn_cb__udata(GArray **arr,
                                    qemu_plugin_vcpu_udata_cb_t cb,
                                    enum qemu_plugin_cb_flags flags,
@@ -482,6 +499,12 @@ void exec_inline_op(struct qemu_plugin_dyn_cb *cb, int cpu_index)
     char *ptr = cb->userp;
     size_t elem_size = 0;
     size_t offset = 0;
+    if (!ptr) {
+        /* use inline entry */
+        ptr = cb->inline_insn.entry.score->data->data;
+        elem_size = g_array_get_element_size(cb->inline_insn.entry.score->data);
+        offset = cb->inline_insn.entry.offset;
+    }
     uint64_t *val = (uint64_t *)(ptr + offset + cpu_index * elem_size);
 
     switch (cb->inline_insn.op) {
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index 6204453d0fd..0d8141b85f1 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -28,13 +28,16 @@
   qemu_plugin_register_vcpu_init_cb;
   qemu_plugin_register_vcpu_insn_exec_cb;
   qemu_plugin_register_vcpu_insn_exec_inline;
+  qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu;
   qemu_plugin_register_vcpu_mem_cb;
   qemu_plugin_register_vcpu_mem_inline;
+  qemu_plugin_register_vcpu_mem_inline_per_vcpu;
   qemu_plugin_register_vcpu_resume_cb;
   qemu_plugin_register_vcpu_syscall_cb;
   qemu_plugin_register_vcpu_syscall_ret_cb;
   qemu_plugin_register_vcpu_tb_exec_cb;
   qemu_plugin_register_vcpu_tb_exec_inline;
+  qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu;
   qemu_plugin_register_vcpu_tb_trans_cb;
   qemu_plugin_reset;
   qemu_plugin_scoreboard_free;
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 09/17] tests/plugin: add test plugin for inline operations
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (7 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 08/17] plugins: add inline operation per vcpu Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 10/17] tests/plugin/mem: migrate to new per_vcpu API Pierrick Bouvier
                   ` (8 subsequent siblings)
  17 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

For now, it simply performs instruction, bb and mem count, and ensure
that inline vs callback versions have the same result. Later, we'll
extend it when new inline operations are added.

Use existing plugins to test everything works is a bit cumbersome, as
different events are treated in different plugins. Thus, this new one.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 tests/plugin/inline.c    | 186 +++++++++++++++++++++++++++++++++++++++
 tests/plugin/meson.build |   2 +-
 2 files changed, 187 insertions(+), 1 deletion(-)
 create mode 100644 tests/plugin/inline.c

diff --git a/tests/plugin/inline.c b/tests/plugin/inline.c
new file mode 100644
index 00000000000..0163e9b51c5
--- /dev/null
+++ b/tests/plugin/inline.c
@@ -0,0 +1,186 @@
+/*
+ * Copyright (C) 2023, Pierrick Bouvier <pierrick.bouvier@linaro.org>
+ *
+ * Demonstrates and tests usage of inline ops.
+ *
+ * License: GNU GPL, version 2 or later.
+ *   See the COPYING file in the top-level directory.
+ */
+
+#include <glib.h>
+#include <stdint.h>
+#include <stdio.h>
+
+#include <qemu-plugin.h>
+
+typedef struct {
+    uint64_t count_tb;
+    uint64_t count_tb_inline;
+    uint64_t count_insn;
+    uint64_t count_insn_inline;
+    uint64_t count_mem;
+    uint64_t count_mem_inline;
+} CPUCount;
+
+static struct qemu_plugin_scoreboard *counts;
+static qemu_plugin_u64 count_tb;
+static qemu_plugin_u64 count_tb_inline;
+static qemu_plugin_u64 count_insn;
+static qemu_plugin_u64 count_insn_inline;
+static qemu_plugin_u64 count_mem;
+static qemu_plugin_u64 count_mem_inline;
+
+static uint64_t global_count_tb;
+static uint64_t global_count_insn;
+static uint64_t global_count_mem;
+static unsigned int max_cpu_index;
+static GMutex tb_lock;
+static GMutex insn_lock;
+static GMutex mem_lock;
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+static void stats_insn(void)
+{
+    const uint64_t expected = global_count_insn;
+    const uint64_t per_vcpu = qemu_plugin_u64_sum(count_insn);
+    const uint64_t inl_per_vcpu =
+        qemu_plugin_u64_sum(count_insn_inline);
+    printf("insn: %" PRIu64 "\n", expected);
+    printf("insn: %" PRIu64 " (per vcpu)\n", per_vcpu);
+    printf("insn: %" PRIu64 " (per vcpu inline)\n", inl_per_vcpu);
+    g_assert(expected > 0);
+    g_assert(per_vcpu == expected);
+    g_assert(inl_per_vcpu == expected);
+}
+
+static void stats_tb(void)
+{
+    const uint64_t expected = global_count_tb;
+    const uint64_t per_vcpu = qemu_plugin_u64_sum(count_tb);
+    const uint64_t inl_per_vcpu =
+        qemu_plugin_u64_sum(count_tb_inline);
+    printf("tb: %" PRIu64 "\n", expected);
+    printf("tb: %" PRIu64 " (per vcpu)\n", per_vcpu);
+    printf("tb: %" PRIu64 " (per vcpu inline)\n", inl_per_vcpu);
+    g_assert(expected > 0);
+    g_assert(per_vcpu == expected);
+    g_assert(inl_per_vcpu == expected);
+}
+
+static void stats_mem(void)
+{
+    const uint64_t expected = global_count_mem;
+    const uint64_t per_vcpu = qemu_plugin_u64_sum(count_mem);
+    const uint64_t inl_per_vcpu =
+        qemu_plugin_u64_sum(count_mem_inline);
+    printf("mem: %" PRIu64 "\n", expected);
+    printf("mem: %" PRIu64 " (per vcpu)\n", per_vcpu);
+    printf("mem: %" PRIu64 " (per vcpu inline)\n", inl_per_vcpu);
+    g_assert(expected > 0);
+    g_assert(per_vcpu == expected);
+    g_assert(inl_per_vcpu == expected);
+}
+
+static void plugin_exit(qemu_plugin_id_t id, void *udata)
+{
+    const unsigned int num_cpus = qemu_plugin_num_vcpus();
+    g_assert(num_cpus == max_cpu_index + 1);
+
+    for (int i = 0; i < num_cpus ; ++i) {
+        const uint64_t tb = qemu_plugin_u64_get(count_tb, i);
+        const uint64_t tb_inline = qemu_plugin_u64_get(count_tb_inline, i);
+        const uint64_t insn = qemu_plugin_u64_get(count_insn, i);
+        const uint64_t insn_inline = qemu_plugin_u64_get(count_insn_inline, i);
+        const uint64_t mem = qemu_plugin_u64_get(count_mem, i);
+        const uint64_t mem_inline = qemu_plugin_u64_get(count_mem_inline, i);
+        printf("cpu %d: tb (%" PRIu64 ", %" PRIu64 ") | "
+               "insn (%" PRIu64 ", %" PRIu64 ") | "
+               "mem (%" PRIu64 ", %" PRIu64 ")"
+               "\n",
+               i, tb, tb_inline, insn, insn_inline, mem, mem_inline);
+        g_assert(tb == tb_inline);
+        g_assert(insn == insn_inline);
+        g_assert(mem == mem_inline);
+    }
+
+    stats_tb();
+    stats_insn();
+    stats_mem();
+
+    qemu_plugin_scoreboard_free(counts);
+}
+
+static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
+{
+    qemu_plugin_u64_add(count_tb, cpu_index, 1);
+    g_mutex_lock(&tb_lock);
+    max_cpu_index = MAX(max_cpu_index, cpu_index);
+    global_count_tb++;
+    g_mutex_unlock(&tb_lock);
+}
+
+static void vcpu_insn_exec(unsigned int cpu_index, void *udata)
+{
+    qemu_plugin_u64_add(count_insn, cpu_index, 1);
+    g_mutex_lock(&insn_lock);
+    global_count_insn++;
+    g_mutex_unlock(&insn_lock);
+}
+
+static void vcpu_mem_access(unsigned int cpu_index,
+                            qemu_plugin_meminfo_t info,
+                            uint64_t vaddr,
+                            void *userdata)
+{
+    qemu_plugin_u64_add(count_mem, cpu_index, 1);
+    g_mutex_lock(&mem_lock);
+    global_count_mem++;
+    g_mutex_unlock(&mem_lock);
+}
+
+static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
+{
+    qemu_plugin_register_vcpu_tb_exec_cb(
+        tb, vcpu_tb_exec, QEMU_PLUGIN_CB_NO_REGS, 0);
+    qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
+        tb, QEMU_PLUGIN_INLINE_ADD_U64, count_tb_inline, 1);
+
+    for (int idx = 0; idx < qemu_plugin_tb_n_insns(tb); ++idx) {
+        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, idx);
+        qemu_plugin_register_vcpu_insn_exec_cb(
+            insn, vcpu_insn_exec, QEMU_PLUGIN_CB_NO_REGS, 0);
+        qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(
+            insn, QEMU_PLUGIN_INLINE_ADD_U64, count_insn_inline, 1);
+        qemu_plugin_register_vcpu_mem_cb(insn, &vcpu_mem_access,
+                                         QEMU_PLUGIN_CB_NO_REGS,
+                                         QEMU_PLUGIN_MEM_RW, 0);
+        qemu_plugin_register_vcpu_mem_inline_per_vcpu(
+            insn, QEMU_PLUGIN_MEM_RW,
+            QEMU_PLUGIN_INLINE_ADD_U64,
+            count_mem_inline, 1);
+    }
+}
+
+QEMU_PLUGIN_EXPORT
+int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
+                        int argc, char **argv)
+{
+    counts = qemu_plugin_scoreboard_new(sizeof(CPUCount));
+    count_tb = qemu_plugin_scoreboard_u64_in_struct(
+        counts, CPUCount, count_tb);
+    count_insn = qemu_plugin_scoreboard_u64_in_struct(
+        counts, CPUCount, count_insn);
+    count_mem = qemu_plugin_scoreboard_u64_in_struct(
+        counts, CPUCount, count_mem);
+    count_tb_inline = qemu_plugin_scoreboard_u64_in_struct(
+        counts, CPUCount, count_tb_inline);
+    count_insn_inline = qemu_plugin_scoreboard_u64_in_struct(
+        counts, CPUCount, count_insn_inline);
+    count_mem_inline = qemu_plugin_scoreboard_u64_in_struct(
+        counts, CPUCount, count_mem_inline);
+    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
+    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
+
+    return 0;
+}
diff --git a/tests/plugin/meson.build b/tests/plugin/meson.build
index e18183aaeda..9eece5bab51 100644
--- a/tests/plugin/meson.build
+++ b/tests/plugin/meson.build
@@ -1,6 +1,6 @@
 t = []
 if get_option('plugins')
-  foreach i : ['bb', 'empty', 'insn', 'mem', 'syscall']
+  foreach i : ['bb', 'empty', 'inline', 'insn', 'mem', 'syscall']
     if host_os == 'windows'
       t += shared_module(i, files(i + '.c') + '../../contrib/plugins/win32_linker.c',
                         include_directories: '../../include/qemu',
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 10/17] tests/plugin/mem: migrate to new per_vcpu API
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (8 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 09/17] tests/plugin: add test plugin for inline operations Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 11/17] tests/plugin/insn: " Pierrick Bouvier
                   ` (7 subsequent siblings)
  17 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 tests/plugin/mem.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
index 44e91065ba7..d4729f5e015 100644
--- a/tests/plugin/mem.c
+++ b/tests/plugin/mem.c
@@ -16,9 +16,14 @@
 
 QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
 
-static uint64_t inline_mem_count;
-static uint64_t cb_mem_count;
-static uint64_t io_count;
+typedef struct {
+    uint64_t mem_count;
+    uint64_t io_count;
+} CPUCount;
+
+static struct qemu_plugin_scoreboard *counts;
+static qemu_plugin_u64 mem_count;
+static qemu_plugin_u64 io_count;
 static bool do_inline, do_callback;
 static bool do_haddr;
 static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
@@ -27,16 +32,16 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
 {
     g_autoptr(GString) out = g_string_new("");
 
-    if (do_inline) {
-        g_string_printf(out, "inline mem accesses: %" PRIu64 "\n", inline_mem_count);
-    }
-    if (do_callback) {
-        g_string_append_printf(out, "callback mem accesses: %" PRIu64 "\n", cb_mem_count);
+    if (do_inline || do_callback) {
+        g_string_printf(out, "mem accesses: %" PRIu64 "\n",
+                        qemu_plugin_u64_sum(mem_count));
     }
     if (do_haddr) {
-        g_string_append_printf(out, "io accesses: %" PRIu64 "\n", io_count);
+        g_string_append_printf(out, "io accesses: %" PRIu64 "\n",
+                               qemu_plugin_u64_sum(io_count));
     }
     qemu_plugin_outs(out->str);
+    qemu_plugin_scoreboard_free(counts);
 }
 
 static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
@@ -46,12 +51,12 @@ static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
         struct qemu_plugin_hwaddr *hwaddr;
         hwaddr = qemu_plugin_get_hwaddr(meminfo, vaddr);
         if (qemu_plugin_hwaddr_is_io(hwaddr)) {
-            io_count++;
+            qemu_plugin_u64_add(io_count, cpu_index, 1);
         } else {
-            cb_mem_count++;
+            qemu_plugin_u64_add(mem_count, cpu_index, 1);
         }
     } else {
-        cb_mem_count++;
+        qemu_plugin_u64_add(mem_count, cpu_index, 1);
     }
 }
 
@@ -64,9 +69,10 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
         struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
 
         if (do_inline) {
-            qemu_plugin_register_vcpu_mem_inline(insn, rw,
-                                                 QEMU_PLUGIN_INLINE_ADD_U64,
-                                                 &inline_mem_count, 1);
+            qemu_plugin_register_vcpu_mem_inline_per_vcpu(
+                insn, rw,
+                QEMU_PLUGIN_INLINE_ADD_U64,
+                mem_count, 1);
         }
         if (do_callback) {
             qemu_plugin_register_vcpu_mem_cb(insn, vcpu_mem,
@@ -117,6 +123,10 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
         }
     }
 
+    counts = qemu_plugin_scoreboard_new(sizeof(CPUCount));
+    mem_count = qemu_plugin_scoreboard_u64_in_struct(
+        counts, CPUCount, mem_count);
+    io_count = qemu_plugin_scoreboard_u64_in_struct(counts, CPUCount, io_count);
     qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
     qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
     return 0;
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 11/17] tests/plugin/insn: migrate to new per_vcpu API
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (9 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 10/17] tests/plugin/mem: migrate to new per_vcpu API Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 12/17] tests/plugin/bb: " Pierrick Bouvier
                   ` (6 subsequent siblings)
  17 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 tests/plugin/insn.c | 106 +++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 56 deletions(-)

diff --git a/tests/plugin/insn.c b/tests/plugin/insn.c
index 5fd3017c2b3..0d0a4cd1c34 100644
--- a/tests/plugin/insn.c
+++ b/tests/plugin/insn.c
@@ -16,25 +16,21 @@
 
 QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
 
-#define MAX_CPUS 8 /* lets not go nuts */
-
-typedef struct {
-    uint64_t insn_count;
-} InstructionCount;
-
-static InstructionCount counts[MAX_CPUS];
-static uint64_t inline_insn_count;
+static qemu_plugin_u64 insn_count;
 
 static bool do_inline;
 static bool do_size;
 static GArray *sizes;
 
+typedef struct {
+    uint64_t hits;
+    uint64_t last_hit;
+    uint64_t total_delta;
+} MatchCount;
+
 typedef struct {
     char *match_string;
-    uint64_t hits[MAX_CPUS];
-    uint64_t last_hit[MAX_CPUS];
-    uint64_t total_delta[MAX_CPUS];
-    GPtrArray *history[MAX_CPUS];
+    struct qemu_plugin_scoreboard *counts; /* MatchCount */
 } Match;
 
 static GArray *matches;
@@ -48,41 +44,40 @@ typedef struct {
 
 static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata)
 {
-    unsigned int i = cpu_index % MAX_CPUS;
-    InstructionCount *c = &counts[i];
-
-    c->insn_count++;
+    qemu_plugin_u64_add(insn_count, cpu_index, 1);
 }
 
 static void vcpu_insn_matched_exec_before(unsigned int cpu_index, void *udata)
 {
-    unsigned int i = cpu_index % MAX_CPUS;
     Instruction *insn = (Instruction *) udata;
-    Match *match = insn->match;
+    Match *insn_match = insn->match;
+    MatchCount *match = qemu_plugin_scoreboard_find(insn_match->counts,
+                                                    cpu_index);
+
     g_autoptr(GString) ts = g_string_new("");
 
     insn->hits++;
     g_string_append_printf(ts, "0x%" PRIx64 ", '%s', %"PRId64 " hits",
                            insn->vaddr, insn->disas, insn->hits);
 
-    uint64_t icount = counts[i].insn_count;
-    uint64_t delta = icount - match->last_hit[i];
+    uint64_t icount = qemu_plugin_u64_get(insn_count, cpu_index);
+    uint64_t delta = icount - match->last_hit;
 
-    match->hits[i]++;
-    match->total_delta[i] += delta;
+    match->hits++;
+    match->total_delta += delta;
 
     g_string_append_printf(ts,
-                           ", %"PRId64" match hits, "
-                           "Δ+%"PRId64 " since last match,"
+                           " , cpu %u,"
+                           " %"PRId64" match hits,"
+                           " Δ+%"PRId64 " since last match,"
                            " %"PRId64 " avg insns/match\n",
-                           match->hits[i], delta,
-                           match->total_delta[i] / match->hits[i]);
+                           cpu_index,
+                           match->hits, delta,
+                           match->total_delta / match->hits);
 
-    match->last_hit[i] = icount;
+    match->last_hit = icount;
 
     qemu_plugin_outs(ts->str);
-
-    g_ptr_array_add(match->history[i], insn);
 }
 
 static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
@@ -94,8 +89,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
         struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
 
         if (do_inline) {
-            qemu_plugin_register_vcpu_insn_exec_inline(
-                insn, QEMU_PLUGIN_INLINE_ADD_U64, &inline_insn_count, 1);
+            qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(
+                insn, QEMU_PLUGIN_INLINE_ADD_U64, insn_count, 1);
         } else {
             uint64_t vaddr = qemu_plugin_insn_vaddr(insn);
             qemu_plugin_register_vcpu_insn_exec_cb(
@@ -117,10 +112,9 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
          * information about the instruction which we also need to
          * save if there is a hit.
          */
-        if (matches) {
+        if (matches->len) {
             char *insn_disas = qemu_plugin_insn_disas(insn);
-            int j;
-            for (j = 0; j < matches->len; j++) {
+            for (int j = 0; j < matches->len; j++) {
                 Match *m = &g_array_index(matches, Match, j);
                 if (g_str_has_prefix(insn_disas, m->match_string)) {
                     Instruction *rec = g_new0(Instruction, 1);
@@ -150,36 +144,33 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
                                        "len %d bytes: %ld insns\n", i, *cnt);
             }
         }
-    } else if (do_inline) {
-        g_string_append_printf(out, "insns: %" PRIu64 "\n", inline_insn_count);
     } else {
-        uint64_t total_insns = 0;
-        for (i = 0; i < MAX_CPUS; i++) {
-            InstructionCount *c = &counts[i];
-            if (c->insn_count) {
-                g_string_append_printf(out, "cpu %d insns: %" PRIu64 "\n",
-                                       i, c->insn_count);
-                total_insns += c->insn_count;
-            }
+        for (i = 0; i < qemu_plugin_num_vcpus(); i++) {
+            g_string_append_printf(out, "cpu %d insns: %" PRIu64 "\n",
+                                   i, qemu_plugin_u64_get(insn_count, i));
         }
         g_string_append_printf(out, "total insns: %" PRIu64 "\n",
-                               total_insns);
+                               qemu_plugin_u64_sum(insn_count));
     }
     qemu_plugin_outs(out->str);
+
+    qemu_plugin_scoreboard_free(insn_count.score);
+    for (i = 0; i < matches->len; ++i) {
+        Match *m = &g_array_index(matches, Match, i);
+        g_free(m->match_string);
+        qemu_plugin_scoreboard_free(m->counts);
+    }
+    g_array_free(matches, TRUE);
+    g_array_free(sizes, TRUE);
 }
 
 
 /* Add a match to the array of matches */
 static void parse_match(char *match)
 {
-    Match new_match = { .match_string = match };
-    int i;
-    for (i = 0; i < MAX_CPUS; i++) {
-        new_match.history[i] = g_ptr_array_new();
-    }
-    if (!matches) {
-        matches = g_array_new(false, true, sizeof(Match));
-    }
+    Match new_match = {
+        .match_string = g_strdup(match),
+        .counts = qemu_plugin_scoreboard_new(sizeof(MatchCount)) };
     g_array_append_val(matches, new_match);
 }
 
@@ -187,6 +178,10 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
                                            const qemu_info_t *info,
                                            int argc, char **argv)
 {
+    matches = g_array_new(false, true, sizeof(Match));
+    /* null terminated so 0 is not a special case */
+    sizes = g_array_new(true, true, sizeof(unsigned long));
+
     for (int i = 0; i < argc; i++) {
         char *opt = argv[i];
         g_auto(GStrv) tokens = g_strsplit(opt, "=", 2);
@@ -208,9 +203,8 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
         }
     }
 
-    if (do_size) {
-        sizes = g_array_new(true, true, sizeof(unsigned long));
-    }
+    insn_count = qemu_plugin_scoreboard_u64(
+        qemu_plugin_scoreboard_new(sizeof(uint64_t)));
 
     qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
     qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 12/17] tests/plugin/bb: migrate to new per_vcpu API
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (10 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 11/17] tests/plugin/insn: " Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 13/17] contrib/plugins/hotblocks: " Pierrick Bouvier
                   ` (5 subsequent siblings)
  17 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 tests/plugin/bb.c | 63 +++++++++++++++++++----------------------------
 1 file changed, 26 insertions(+), 37 deletions(-)

diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
index df50d1fd3bc..36776dee1e1 100644
--- a/tests/plugin/bb.c
+++ b/tests/plugin/bb.c
@@ -17,27 +17,25 @@
 QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
 
 typedef struct {
-    GMutex lock;
-    int index;
     uint64_t bb_count;
     uint64_t insn_count;
 } CPUCount;
 
-/* Used by the inline & linux-user counts */
+static struct qemu_plugin_scoreboard *counts;
+static qemu_plugin_u64 bb_count;
+static qemu_plugin_u64 insn_count;
+
 static bool do_inline;
-static CPUCount inline_count;
-
 /* Dump running CPU total on idle? */
 static bool idle_report;
-static GPtrArray *counts;
-static int max_cpus;
 
-static void gen_one_cpu_report(CPUCount *count, GString *report)
+static void gen_one_cpu_report(CPUCount *count, GString *report,
+                               unsigned int cpu_index)
 {
     if (count->bb_count) {
         g_string_append_printf(report, "CPU%d: "
                                "bb's: %" PRIu64", insns: %" PRIu64 "\n",
-                               count->index,
+                               cpu_index,
                                count->bb_count, count->insn_count);
     }
 }
@@ -46,20 +44,23 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
 {
     g_autoptr(GString) report = g_string_new("");
 
-    if (do_inline || !max_cpus) {
-        g_string_printf(report, "bb's: %" PRIu64", insns: %" PRIu64 "\n",
-                        inline_count.bb_count, inline_count.insn_count);
-    } else {
-        g_ptr_array_foreach(counts, (GFunc) gen_one_cpu_report, report);
+    for (int i = 0; i < qemu_plugin_num_vcpus(); ++i) {
+        CPUCount *count = qemu_plugin_scoreboard_find(counts, i);
+        gen_one_cpu_report(count, report, i);
     }
+    g_string_append_printf(report, "Total: "
+                           "bb's: %" PRIu64", insns: %" PRIu64 "\n",
+                           qemu_plugin_u64_sum(bb_count),
+                           qemu_plugin_u64_sum(insn_count));
     qemu_plugin_outs(report->str);
+    qemu_plugin_scoreboard_free(counts);
 }
 
 static void vcpu_idle(qemu_plugin_id_t id, unsigned int cpu_index)
 {
-    CPUCount *count = g_ptr_array_index(counts, cpu_index);
+    CPUCount *count = qemu_plugin_scoreboard_find(counts, cpu_index);
     g_autoptr(GString) report = g_string_new("");
-    gen_one_cpu_report(count, report);
+    gen_one_cpu_report(count, report, cpu_index);
 
     if (report->len > 0) {
         g_string_prepend(report, "Idling ");
@@ -69,14 +70,11 @@ static void vcpu_idle(qemu_plugin_id_t id, unsigned int cpu_index)
 
 static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
 {
-    CPUCount *count = max_cpus ?
-        g_ptr_array_index(counts, cpu_index) : &inline_count;
+    CPUCount *count = qemu_plugin_scoreboard_find(counts, cpu_index);
 
     uintptr_t n_insns = (uintptr_t)udata;
-    g_mutex_lock(&count->lock);
     count->insn_count += n_insns;
     count->bb_count++;
-    g_mutex_unlock(&count->lock);
 }
 
 static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
@@ -84,11 +82,10 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
     size_t n_insns = qemu_plugin_tb_n_insns(tb);
 
     if (do_inline) {
-        qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
-                                                 &inline_count.bb_count, 1);
-        qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
-                                                 &inline_count.insn_count,
-                                                 n_insns);
+        qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
+            tb, QEMU_PLUGIN_INLINE_ADD_U64, bb_count, 1);
+        qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
+            tb, QEMU_PLUGIN_INLINE_ADD_U64, insn_count, n_insns);
     } else {
         qemu_plugin_register_vcpu_tb_exec_cb(tb, vcpu_tb_exec,
                                              QEMU_PLUGIN_CB_NO_REGS,
@@ -121,18 +118,10 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
         }
     }
 
-    if (info->system_emulation && !do_inline) {
-        max_cpus = info->system.max_vcpus;
-        counts = g_ptr_array_new();
-        for (i = 0; i < max_cpus; i++) {
-            CPUCount *count = g_new0(CPUCount, 1);
-            g_mutex_init(&count->lock);
-            count->index = i;
-            g_ptr_array_add(counts, count);
-        }
-    } else if (!do_inline) {
-        g_mutex_init(&inline_count.lock);
-    }
+    counts = qemu_plugin_scoreboard_new(sizeof(CPUCount));
+    bb_count = qemu_plugin_scoreboard_u64_in_struct(counts, CPUCount, bb_count);
+    insn_count = qemu_plugin_scoreboard_u64_in_struct(
+        counts, CPUCount, insn_count);
 
     if (idle_report) {
         qemu_plugin_register_vcpu_idle_cb(id, vcpu_idle);
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 13/17] contrib/plugins/hotblocks: migrate to new per_vcpu API
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (11 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 12/17] tests/plugin/bb: " Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 14/17] contrib/plugins/howvec: " Pierrick Bouvier
                   ` (4 subsequent siblings)
  17 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 contrib/plugins/hotblocks.c | 50 ++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
index 4de1b134944..02bc5078bdd 100644
--- a/contrib/plugins/hotblocks.c
+++ b/contrib/plugins/hotblocks.c
@@ -34,8 +34,8 @@ static guint64 limit = 20;
  */
 typedef struct {
     uint64_t start_addr;
-    uint64_t exec_count;
-    int      trans_count;
+    struct qemu_plugin_scoreboard *exec_count;
+    int trans_count;
     unsigned long insns;
 } ExecCount;
 
@@ -43,7 +43,17 @@ static gint cmp_exec_count(gconstpointer a, gconstpointer b)
 {
     ExecCount *ea = (ExecCount *) a;
     ExecCount *eb = (ExecCount *) b;
-    return ea->exec_count > eb->exec_count ? -1 : 1;
+    uint64_t count_a =
+        qemu_plugin_u64_sum(qemu_plugin_scoreboard_u64(ea->exec_count));
+    uint64_t count_b =
+        qemu_plugin_u64_sum(qemu_plugin_scoreboard_u64(eb->exec_count));
+    return count_a > count_b ? -1 : 1;
+}
+
+static void exec_count_free(gpointer key, gpointer value, gpointer user_data)
+{
+    ExecCount *cnt = value;
+    qemu_plugin_scoreboard_free(cnt->exec_count);
 }
 
 static void plugin_exit(qemu_plugin_id_t id, void *p)
@@ -52,7 +62,6 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
     GList *counts, *it;
     int i;
 
-    g_mutex_lock(&lock);
     g_string_append_printf(report, "%d entries in the hash table\n",
                            g_hash_table_size(hotblocks));
     counts = g_hash_table_get_values(hotblocks);
@@ -63,16 +72,21 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
 
         for (i = 0; i < limit && it->next; i++, it = it->next) {
             ExecCount *rec = (ExecCount *) it->data;
-            g_string_append_printf(report, "0x%016"PRIx64", %d, %ld, %"PRId64"\n",
-                                   rec->start_addr, rec->trans_count,
-                                   rec->insns, rec->exec_count);
+            g_string_append_printf(
+                report, "0x%016"PRIx64", %d, %ld, %"PRId64"\n",
+                rec->start_addr, rec->trans_count,
+                rec->insns,
+                qemu_plugin_u64_sum(
+                    qemu_plugin_scoreboard_u64(rec->exec_count)));
         }
 
         g_list_free(it);
     }
-    g_mutex_unlock(&lock);
 
     qemu_plugin_outs(report->str);
+
+    g_hash_table_foreach(hotblocks, exec_count_free, NULL);
+    g_hash_table_destroy(hotblocks);
 }
 
 static void plugin_init(void)
@@ -82,15 +96,9 @@ static void plugin_init(void)
 
 static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
 {
-    ExecCount *cnt;
-    uint64_t hash = (uint64_t) udata;
-
-    g_mutex_lock(&lock);
-    cnt = (ExecCount *) g_hash_table_lookup(hotblocks, (gconstpointer) hash);
-    /* should always succeed */
-    g_assert(cnt);
-    cnt->exec_count++;
-    g_mutex_unlock(&lock);
+    ExecCount *cnt = (ExecCount *)udata;
+    qemu_plugin_u64_add(qemu_plugin_scoreboard_u64(cnt->exec_count),
+                        cpu_index, 1);
 }
 
 /*
@@ -114,18 +122,20 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
         cnt->start_addr = pc;
         cnt->trans_count = 1;
         cnt->insns = insns;
+        cnt->exec_count = qemu_plugin_scoreboard_new(sizeof(uint64_t));
         g_hash_table_insert(hotblocks, (gpointer) hash, (gpointer) cnt);
     }
 
     g_mutex_unlock(&lock);
 
     if (do_inline) {
-        qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
-                                                 &cnt->exec_count, 1);
+        qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
+            tb, QEMU_PLUGIN_INLINE_ADD_U64,
+            qemu_plugin_scoreboard_u64(cnt->exec_count), 1);
     } else {
         qemu_plugin_register_vcpu_tb_exec_cb(tb, vcpu_tb_exec,
                                              QEMU_PLUGIN_CB_NO_REGS,
-                                             (void *)hash);
+                                             (void *)cnt);
     }
 }
 
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 14/17] contrib/plugins/howvec: migrate to new per_vcpu API
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (12 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 13/17] contrib/plugins/hotblocks: " Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 15/17] plugins: remove non per_vcpu inline operation from API Pierrick Bouvier
                   ` (3 subsequent siblings)
  17 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 contrib/plugins/howvec.c | 53 ++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/contrib/plugins/howvec.c b/contrib/plugins/howvec.c
index 644a7856bb2..2d10c87e0fb 100644
--- a/contrib/plugins/howvec.c
+++ b/contrib/plugins/howvec.c
@@ -43,13 +43,13 @@ typedef struct {
     uint32_t mask;
     uint32_t pattern;
     CountType what;
-    uint64_t count;
+    qemu_plugin_u64 count;
 } InsnClassExecCount;
 
 typedef struct {
     char *insn;
     uint32_t opcode;
-    uint64_t count;
+    qemu_plugin_u64 count;
     InsnClassExecCount *class;
 } InsnExecCount;
 
@@ -159,7 +159,9 @@ static gint cmp_exec_count(gconstpointer a, gconstpointer b)
 {
     InsnExecCount *ea = (InsnExecCount *) a;
     InsnExecCount *eb = (InsnExecCount *) b;
-    return ea->count > eb->count ? -1 : 1;
+    uint64_t count_a = qemu_plugin_u64_sum(ea->count);
+    uint64_t count_b = qemu_plugin_u64_sum(eb->count);
+    return count_a > count_b ? -1 : 1;
 }
 
 static void free_record(gpointer data)
@@ -167,12 +169,14 @@ static void free_record(gpointer data)
     InsnExecCount *rec = (InsnExecCount *) data;
     g_free(rec->insn);
     g_free(rec);
+    qemu_plugin_scoreboard_free(rec->count.score);
 }
 
 static void plugin_exit(qemu_plugin_id_t id, void *p)
 {
     g_autoptr(GString) report = g_string_new("Instruction Classes:\n");
     int i;
+    uint64_t total_count;
     GList *counts;
     InsnClassExecCount *class = NULL;
 
@@ -180,11 +184,12 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
         class = &class_table[i];
         switch (class->what) {
         case COUNT_CLASS:
-            if (class->count || verbose) {
+            total_count = qemu_plugin_u64_sum(class->count);
+            if (total_count || verbose) {
                 g_string_append_printf(report,
                                        "Class: %-24s\t(%" PRId64 " hits)\n",
                                        class->class,
-                                       class->count);
+                                       total_count);
             }
             break;
         case COUNT_INDIVIDUAL:
@@ -212,7 +217,7 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
                                    "Instr: %-24s\t(%" PRId64 " hits)"
                                    "\t(op=0x%08x/%s)\n",
                                    rec->insn,
-                                   rec->count,
+                                   qemu_plugin_u64_sum(rec->count),
                                    rec->opcode,
                                    rec->class ?
                                    rec->class->class : "un-categorised");
@@ -221,6 +226,12 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
     }
 
     g_hash_table_destroy(insns);
+    for (i = 0; i < ARRAY_SIZE(class_tables); i++) {
+        for (int j = 0; j < class_tables[i].table_sz; ++j) {
+            qemu_plugin_scoreboard_free(class_tables[i].table[j].count.score);
+        }
+    }
+
 
     qemu_plugin_outs(report->str);
 }
@@ -232,11 +243,12 @@ static void plugin_init(void)
 
 static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata)
 {
-    uint64_t *count = (uint64_t *) udata;
-    (*count)++;
+    struct qemu_plugin_scoreboard *score = udata;
+    qemu_plugin_u64_add(qemu_plugin_scoreboard_u64(score), cpu_index, 1);
 }
 
-static uint64_t *find_counter(struct qemu_plugin_insn *insn)
+static struct qemu_plugin_scoreboard *find_counter(
+    struct qemu_plugin_insn *insn)
 {
     int i;
     uint64_t *cnt = NULL;
@@ -265,7 +277,7 @@ static uint64_t *find_counter(struct qemu_plugin_insn *insn)
     case COUNT_NONE:
         return NULL;
     case COUNT_CLASS:
-        return &class->count;
+        return class->count.score;
     case COUNT_INDIVIDUAL:
     {
         InsnExecCount *icount;
@@ -279,13 +291,16 @@ static uint64_t *find_counter(struct qemu_plugin_insn *insn)
             icount->opcode = opcode;
             icount->insn = qemu_plugin_insn_disas(insn);
             icount->class = class;
+            struct qemu_plugin_scoreboard *score =
+                qemu_plugin_scoreboard_new(sizeof(uint64_t));
+            icount->count = qemu_plugin_scoreboard_u64(score);
 
             g_hash_table_insert(insns, GUINT_TO_POINTER(opcode),
                                 (gpointer) icount);
         }
         g_mutex_unlock(&lock);
 
-        return &icount->count;
+        return icount->count.score;
     }
     default:
         g_assert_not_reached();
@@ -300,14 +315,14 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
     size_t i;
 
     for (i = 0; i < n; i++) {
-        uint64_t *cnt;
         struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
-        cnt = find_counter(insn);
+        struct qemu_plugin_scoreboard *cnt = find_counter(insn);
 
         if (cnt) {
             if (do_inline) {
-                qemu_plugin_register_vcpu_insn_exec_inline(
-                    insn, QEMU_PLUGIN_INLINE_ADD_U64, cnt, 1);
+                qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(
+                    insn, QEMU_PLUGIN_INLINE_ADD_U64,
+                    qemu_plugin_scoreboard_u64(cnt), 1);
             } else {
                 qemu_plugin_register_vcpu_insn_exec_cb(
                     insn, vcpu_insn_exec_before, QEMU_PLUGIN_CB_NO_REGS, cnt);
@@ -322,6 +337,14 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
 {
     int i;
 
+    for (i = 0; i < ARRAY_SIZE(class_tables); i++) {
+        for (int j = 0; j < class_tables[i].table_sz; ++j) {
+            struct qemu_plugin_scoreboard *score =
+                qemu_plugin_scoreboard_new(sizeof(uint64_t));
+            class_tables[i].table[j].count = qemu_plugin_scoreboard_u64(score);
+        }
+    }
+
     /* Select a class table appropriate to the guest architecture */
     for (i = 0; i < ARRAY_SIZE(class_tables); i++) {
         ClassSelector *entry = &class_tables[i];
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 15/17] plugins: remove non per_vcpu inline operation from API
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (13 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 14/17] contrib/plugins/howvec: " Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 16/17] plugins: cleanup codepath for previous inline operation Pierrick Bouvier
                   ` (2 subsequent siblings)
  17 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

Now we have a thread-safe equivalent of inline operation, and that all
plugins were changed to use it, there is no point to keep the old API.

In more, it will help when we implement more functionality (conditional
callbacks), as we can assume that we operate on a scoreboard.

Bump API version as it's a breaking change for existing plugins.
Bump min API version too, as we removed some functions.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/qemu/qemu-plugin.h   | 58 ++++--------------------------------
 plugins/plugin.h             |  2 +-
 plugins/api.c                | 29 ------------------
 plugins/qemu-plugins.symbols |  3 --
 4 files changed, 6 insertions(+), 86 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index cdf1266d724..9b11098ff82 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -53,11 +53,15 @@ typedef uint64_t qemu_plugin_id_t;
  * symbol qemu_plugin_version which can be checked.
  *
  * version 2: removed qemu_plugin_n_vcpus and qemu_plugin_n_max_vcpus
+ * version 3:
+ * Remove qemu_plugin_register_vcpu_{tb, insn, mem}_exec_inline.
+ * Those functions are replaced by *_per_vcpu variants, which guarantees
+ * thread-safety for operations.
  */
 
 extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
 
-#define QEMU_PLUGIN_VERSION 2
+#define QEMU_PLUGIN_VERSION 3
 
 /**
  * struct qemu_info_t - system information for plugins
@@ -309,25 +313,6 @@ 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
- * @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)
- *
- * 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.
- */
-QEMU_PLUGIN_API
-void qemu_plugin_register_vcpu_tb_exec_inline(struct qemu_plugin_tb *tb,
-                                              enum qemu_plugin_op op,
-                                              void *ptr, uint64_t imm);
-
 /**
  * qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu() - execution inline op
  * @tb: the opaque qemu_plugin_tb handle for the translation
@@ -359,21 +344,6 @@ void qemu_plugin_register_vcpu_insn_exec_cb(struct qemu_plugin_insn *insn,
                                             enum qemu_plugin_cb_flags flags,
                                             void *userdata);
 
-/**
- * qemu_plugin_register_vcpu_insn_exec_inline() - insn execution inline op
- * @insn: the opaque qemu_plugin_insn handle for an instruction
- * @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)
- *
- * Insert an inline op to every time an instruction executes. Useful
- * if you just want to increment a single counter somewhere in memory.
- */
-QEMU_PLUGIN_API
-void qemu_plugin_register_vcpu_insn_exec_inline(struct qemu_plugin_insn *insn,
-                                                enum qemu_plugin_op op,
-                                                void *ptr, uint64_t imm);
-
 /**
  * qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu() - insn exec inline op
  * @insn: the opaque qemu_plugin_insn handle for an instruction
@@ -597,24 +567,6 @@ void qemu_plugin_register_vcpu_mem_cb(struct qemu_plugin_insn *insn,
                                       enum qemu_plugin_mem_rw rw,
                                       void *userdata);
 
-/**
- * qemu_plugin_register_vcpu_mem_inline() - register an inline op to any memory access
- * @insn: handle for instruction to instrument
- * @rw: apply to reads, writes or both
- * @op: the op, of type qemu_plugin_op
- * @ptr: pointer memory for the op
- * @imm: immediate data for @op
- *
- * This registers a inline op every memory access generated by the
- * instruction. This provides for a lightweight but not thread-safe
- * way of counting the number of operations done.
- */
-QEMU_PLUGIN_API
-void qemu_plugin_register_vcpu_mem_inline(struct qemu_plugin_insn *insn,
-                                          enum qemu_plugin_mem_rw rw,
-                                          enum qemu_plugin_op op, void *ptr,
-                                          uint64_t imm);
-
 /**
  * qemu_plugin_register_vcpu_mem_inline_per_vcpu() - inline op for mem access
  * @insn: handle for instruction to instrument
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 1ece1b6707a..8e485cfbd58 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -15,7 +15,7 @@
 #include <gmodule.h>
 #include "qemu/qht.h"
 
-#define QEMU_PLUGIN_MIN_VERSION 2
+#define QEMU_PLUGIN_MIN_VERSION 3
 
 /* global state */
 struct qemu_plugin_state {
diff --git a/plugins/api.c b/plugins/api.c
index be2badda9fa..45eabab8686 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -101,16 +101,6 @@ void qemu_plugin_register_vcpu_tb_exec_cb(struct qemu_plugin_tb *tb,
     }
 }
 
-void qemu_plugin_register_vcpu_tb_exec_inline(struct qemu_plugin_tb *tb,
-                                              enum qemu_plugin_op op,
-                                              void *ptr, uint64_t imm)
-{
-    if (!tb->mem_only) {
-        plugin_register_inline_op(&tb->cbs[PLUGIN_CB_INLINE],
-                                  0, op, ptr, imm);
-    }
-}
-
 void qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
     struct qemu_plugin_tb *tb,
     enum qemu_plugin_op op,
@@ -138,16 +128,6 @@ void qemu_plugin_register_vcpu_insn_exec_cb(struct qemu_plugin_insn *insn,
     }
 }
 
-void qemu_plugin_register_vcpu_insn_exec_inline(struct qemu_plugin_insn *insn,
-                                                enum qemu_plugin_op op,
-                                                void *ptr, uint64_t imm)
-{
-    if (!insn->mem_only) {
-        plugin_register_inline_op(&insn->cbs[PLUGIN_CB_INSN][PLUGIN_CB_INLINE],
-                                  0, op, ptr, imm);
-    }
-}
-
 void qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(
     struct qemu_plugin_insn *insn,
     enum qemu_plugin_op op,
@@ -175,15 +155,6 @@ void qemu_plugin_register_vcpu_mem_cb(struct qemu_plugin_insn *insn,
                                 cb, flags, rw, udata);
 }
 
-void qemu_plugin_register_vcpu_mem_inline(struct qemu_plugin_insn *insn,
-                                          enum qemu_plugin_mem_rw rw,
-                                          enum qemu_plugin_op op, void *ptr,
-                                          uint64_t imm)
-{
-    plugin_register_inline_op(&insn->cbs[PLUGIN_CB_MEM][PLUGIN_CB_INLINE],
-                              rw, op, ptr, imm);
-}
-
 void qemu_plugin_register_vcpu_mem_inline_per_vcpu(
     struct qemu_plugin_insn *insn,
     enum qemu_plugin_mem_rw rw,
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index 0d8141b85f1..a9fac056c7f 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -27,16 +27,13 @@
   qemu_plugin_register_vcpu_idle_cb;
   qemu_plugin_register_vcpu_init_cb;
   qemu_plugin_register_vcpu_insn_exec_cb;
-  qemu_plugin_register_vcpu_insn_exec_inline;
   qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu;
   qemu_plugin_register_vcpu_mem_cb;
-  qemu_plugin_register_vcpu_mem_inline;
   qemu_plugin_register_vcpu_mem_inline_per_vcpu;
   qemu_plugin_register_vcpu_resume_cb;
   qemu_plugin_register_vcpu_syscall_cb;
   qemu_plugin_register_vcpu_syscall_ret_cb;
   qemu_plugin_register_vcpu_tb_exec_cb;
-  qemu_plugin_register_vcpu_tb_exec_inline;
   qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu;
   qemu_plugin_register_vcpu_tb_trans_cb;
   qemu_plugin_reset;
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 16/17] plugins: cleanup codepath for previous inline operation
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (14 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 15/17] plugins: remove non per_vcpu inline operation from API Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-06  9:24 ` [PATCH v3 17/17] MAINTAINERS: Add myself as reviewer for TCG Plugins Pierrick Bouvier
  2024-02-07 15:45 ` [PATCH v3 00/17] TCG Plugin inline operation enhancement Alex Bennée
  17 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 plugins/plugin.h       |  5 -----
 accel/tcg/plugin-gen.c | 13 ++++---------
 plugins/core.c         | 29 ++++-------------------------
 3 files changed, 8 insertions(+), 39 deletions(-)

diff --git a/plugins/plugin.h b/plugins/plugin.h
index 8e485cfbd58..ba52a5995db 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -69,11 +69,6 @@ struct qemu_plugin_ctx {
 
 struct qemu_plugin_ctx *plugin_id_to_ctx_locked(qemu_plugin_id_t id);
 
-void plugin_register_inline_op(GArray **arr,
-                               enum qemu_plugin_mem_rw rw,
-                               enum qemu_plugin_op op, void *ptr,
-                               uint64_t imm);
-
 void plugin_register_inline_op_on_entry(GArray **arr,
                                         enum qemu_plugin_mem_rw rw,
                                         enum qemu_plugin_op op,
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 4930e674c7d..0d613956734 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -439,15 +439,10 @@ static TCGOp *append_inline_cb(const struct qemu_plugin_dyn_cb *cb,
                                TCGOp *begin_op, TCGOp *op,
                                int *unused)
 {
-    char *ptr = cb->userp;
-    size_t elem_size = 0;
-    size_t offset = 0;
-    if (!ptr) {
-        /* use inline entry */
-        ptr = cb->inline_insn.entry.score->data->data;
-        elem_size = g_array_get_element_size(cb->inline_insn.entry.score->data);
-        offset = cb->inline_insn.entry.offset;
-    }
+    char *ptr = cb->inline_insn.entry.score->data->data;
+    size_t elem_size = g_array_get_element_size(
+        cb->inline_insn.entry.score->data);
+    size_t offset = cb->inline_insn.entry.offset;
 
     op = copy_ld_i32(&begin_op, op);
     op = copy_mul_i32(&begin_op, op, elem_size);
diff --git a/plugins/core.c b/plugins/core.c
index 48e351d7631..7f1b8dbfd00 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -318,22 +318,6 @@ static struct qemu_plugin_dyn_cb *plugin_get_dyn_cb(GArray **arr)
     return &g_array_index(cbs, struct qemu_plugin_dyn_cb, cbs->len - 1);
 }
 
-void plugin_register_inline_op(GArray **arr,
-                               enum qemu_plugin_mem_rw rw,
-                               enum qemu_plugin_op op,
-                               void *ptr,
-                               uint64_t imm)
-{
-    struct qemu_plugin_dyn_cb *dyn_cb;
-
-    dyn_cb = plugin_get_dyn_cb(arr);
-    dyn_cb->userp = ptr;
-    dyn_cb->type = PLUGIN_CB_INLINE;
-    dyn_cb->rw = rw;
-    dyn_cb->inline_insn.op = op;
-    dyn_cb->inline_insn.imm = imm;
-}
-
 void plugin_register_inline_op_on_entry(GArray **arr,
                                         enum qemu_plugin_mem_rw rw,
                                         enum qemu_plugin_op op,
@@ -496,15 +480,10 @@ void qemu_plugin_flush_cb(void)
 
 void exec_inline_op(struct qemu_plugin_dyn_cb *cb, int cpu_index)
 {
-    char *ptr = cb->userp;
-    size_t elem_size = 0;
-    size_t offset = 0;
-    if (!ptr) {
-        /* use inline entry */
-        ptr = cb->inline_insn.entry.score->data->data;
-        elem_size = g_array_get_element_size(cb->inline_insn.entry.score->data);
-        offset = cb->inline_insn.entry.offset;
-    }
+    char *ptr = cb->inline_insn.entry.score->data->data;
+    size_t elem_size = g_array_get_element_size(
+        cb->inline_insn.entry.score->data);
+    size_t offset = cb->inline_insn.entry.offset;
     uint64_t *val = (uint64_t *)(ptr + offset + cpu_index * elem_size);
 
     switch (cb->inline_insn.op) {
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 17/17] MAINTAINERS: Add myself as reviewer for TCG Plugins
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (15 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 16/17] plugins: cleanup codepath for previous inline operation Pierrick Bouvier
@ 2024-02-06  9:24 ` Pierrick Bouvier
  2024-02-07 15:45 ` [PATCH v3 00/17] TCG Plugin inline operation enhancement Alex Bennée
  17 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-06  9:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Alex Bennée, Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang,
	Pierrick Bouvier, Paolo Bonzini, Philippe Mathieu-Daudé

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index dfaca8323e9..80528d3dc63 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3676,6 +3676,7 @@ TCG Plugins
 M: Alex Bennée <alex.bennee@linaro.org>
 R: Alexandre Iooss <erdnaxe@crans.org>
 R: Mahmoud Mandour <ma.mandourr@gmail.com>
+R: Pierrick Bouvier <pierrick.bouvier@linaro.org>
 S: Maintained
 F: docs/devel/tcg-plugins.rst
 F: plugins/
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 03/17] plugins: fix order of init/idle/resume callback
  2024-02-06  9:24 ` [PATCH v3 03/17] plugins: fix order of init/idle/resume callback Pierrick Bouvier
@ 2024-02-07  2:59   ` Richard Henderson
  0 siblings, 0 replies; 33+ messages in thread
From: Richard Henderson @ 2024-02-07  2:59 UTC (permalink / raw)
  To: Pierrick Bouvier, qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Alex Bennée,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/6/24 19:24, Pierrick Bouvier wrote:
> We found that vcpu_init_hook was called*after*  idle callback.
> vcpu_init is called from cpu_realize_fn, while idle/resume cb are called
> from qemu_wait_io_event (in vcpu thread).
> 
> This change ensures we only call idle and resume cb only once a plugin
> was init for a given vcpu.
> 
> Next change in the series will run vcpu_init asynchronously, which will
> make it run*after*  resume callback as well. So we fix this now.
> 
> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> ---
>   plugins/core.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 04/17] cpu: call plugin init hook asynchronously
  2024-02-06  9:24 ` [PATCH v3 04/17] cpu: call plugin init hook asynchronously Pierrick Bouvier
@ 2024-02-07  3:00   ` Richard Henderson
  0 siblings, 0 replies; 33+ messages in thread
From: Richard Henderson @ 2024-02-07  3:00 UTC (permalink / raw)
  To: Pierrick Bouvier, qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Alex Bennée,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/6/24 19:24, Pierrick Bouvier wrote:
> This ensures we run during a cpu_exec, which allows to call start/end
> exclusive from this init hook (needed for new scoreboard API introduced
> later).
> 
> async work is run before any tb is translated/executed, so we can
> guarantee plugin init will be called before any other hook.
> 
> The previous change made sure that any idle/resume cb call will not be
> done before initializing plugin for a given vcpu.
> 
> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> ---
>   hw/core/cpu-common.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 05/17] plugins: scoreboard API
  2024-02-06  9:24 ` [PATCH v3 05/17] plugins: scoreboard API Pierrick Bouvier
@ 2024-02-07  3:21   ` Richard Henderson
  2024-02-07  5:59     ` Pierrick Bouvier
  0 siblings, 1 reply; 33+ messages in thread
From: Richard Henderson @ 2024-02-07  3:21 UTC (permalink / raw)
  To: Pierrick Bouvier, qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Alex Bennée,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/6/24 19:24, Pierrick Bouvier wrote:
> We introduce a cpu local storage, automatically managed (and extended)
> by QEMU itself. Plugin allocate a scoreboard, and don't have to deal
> with how many cpus are launched.
> 
> This API will be used by new inline functions but callbacks can benefit
> from this as well. This way, they can operate without a global lock for
> simple operations.
> 
> At any point during execution, any scoreboard will be dimensioned with
> at least qemu_plugin_num_vcpus entries.
> 
> New functions:
> - qemu_plugin_scoreboard_find
> - qemu_plugin_scoreboard_free
> - qemu_plugin_scoreboard_new
> 
> In more, we define a qemu_plugin_u64, which is a simple struct holding
> a pointer to a scoreboard, and a given offset.
> This allows to have a scoreboard containing structs, without having to
> bring offset for all operations on a specific field.
> 
> Since most of the plugins are simply collecting a sum of per-cpu values,
> qemu_plugin_u64 directly support this operation as well.
> 
> New functions:
> - qemu_plugin_u64_add
> - qemu_plugin_u64_get
> - qemu_plugin_u64_set
> - qemu_plugin_u64_sum
> New macros:
> - qemu_plugin_scoreboard_u64
> - qemu_plugin_scoreboard_u64_in_struct

I think the u64 stuff should be a second patch built upon the basic scoreboard support.

> +/* A scoreboard is an array of values, indexed by vcpu_index */
> +struct qemu_plugin_scoreboard {
> +    GArray *data;
> +};

Unnecessary?  Generates an extra pointer dereference for no apparent benefit. 
Alternately, might be useful for other data structure changes...

> +/**
> + * typedef qemu_plugin_u64 - uint64_t member of an entry in a scoreboard
> + *
> + * This field allows to access a specific uint64_t member in one given entry,
> + * located at a specified offset. Inline operations expect this as entry.
> + */
> +typedef struct {
> +    struct qemu_plugin_scoreboard *score;

Embed the struct instead?

> @@ -31,6 +31,9 @@ struct qemu_plugin_state {
>        * but with the HT we avoid adding a field to CPUState.
>        */
>       GHashTable *cpu_ht;
> +    /* Scoreboards, indexed by their addresses. */
> +    GHashTable *scoreboards;

Why a hash table?  All you want is to be able to iterate through all, and add/remove 
easily.  Seems like QLIST from <qemu/queue.h> would be better, and the QLIST_ENTRY member 
would make struct qemu_plugin_scoreboard useful.


r~


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 07/17] plugins: implement inline operation relative to cpu_index
  2024-02-06  9:24 ` [PATCH v3 07/17] plugins: implement inline operation relative to cpu_index Pierrick Bouvier
@ 2024-02-07  3:42   ` Richard Henderson
  2024-02-07  6:01     ` Pierrick Bouvier
  0 siblings, 1 reply; 33+ messages in thread
From: Richard Henderson @ 2024-02-07  3:42 UTC (permalink / raw)
  To: Pierrick Bouvier, qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Alex Bennée,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/6/24 19:24, Pierrick Bouvier wrote:
> Instead of working on a fixed memory location, allow to address it based
> on cpu_index, an element size and a given offset.
> Result address: ptr + offset + cpu_index * element_size.
> 
> With this, we can target a member in a struct array from a base pointer.
> 
> Current semantic is not modified, thus inline operation still targets
> always the same memory location.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   plugins/plugin.h       |  2 +-
>   accel/tcg/plugin-gen.c | 65 +++++++++++++++++++++++++++++++++++-------
>   plugins/api.c          |  3 +-
>   plugins/core.c         | 12 +++++---
>   4 files changed, 65 insertions(+), 17 deletions(-)
> 
> diff --git a/plugins/plugin.h b/plugins/plugin.h
> index fd93a372803..77ed10689ca 100644
> --- a/plugins/plugin.h
> +++ b/plugins/plugin.h
> @@ -100,7 +100,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
>                                    enum qemu_plugin_mem_rw rw,
>                                    void *udata);
>   
> -void exec_inline_op(struct qemu_plugin_dyn_cb *cb);
> +void exec_inline_op(struct qemu_plugin_dyn_cb *cb, int cpu_index);
>   
>   int plugin_num_vcpus(void);
>   
> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
> index b37ce7683e6..68dee4c68d3 100644
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -132,16 +132,28 @@ static void gen_empty_udata_cb_no_rwg(void)
>    */
>   static void gen_empty_inline_cb(void)
>   {
> +    TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
> +    TCGv_ptr cpu_index_as_ptr = tcg_temp_ebb_new_ptr();
>       TCGv_i64 val = tcg_temp_ebb_new_i64();
>       TCGv_ptr ptr = tcg_temp_ebb_new_ptr();
>   
> +    tcg_gen_ld_i32(cpu_index, tcg_env,
> +                   -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
> +    /* pass an immediate != 0 so that it doesn't get optimized away */
> +    tcg_gen_muli_i32(cpu_index, cpu_index, 0xdeadbeef);

You don't need a random immediate here.
You can just as easily use

     tcg_gen_mul_i32(cpu_index, cpu_index, cpu_index);

with a similar comment about the true size being inserted later.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 08/17] plugins: add inline operation per vcpu
  2024-02-06  9:24 ` [PATCH v3 08/17] plugins: add inline operation per vcpu Pierrick Bouvier
@ 2024-02-07  3:45   ` Richard Henderson
  2024-02-07  6:05     ` Pierrick Bouvier
  0 siblings, 1 reply; 33+ messages in thread
From: Richard Henderson @ 2024-02-07  3:45 UTC (permalink / raw)
  To: Pierrick Bouvier, qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Alex Bennée,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/6/24 19:24, Pierrick Bouvier wrote:
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -442,6 +442,13 @@ static TCGOp *append_inline_cb(const struct qemu_plugin_dyn_cb *cb,
>       char *ptr = cb->userp;
>       size_t elem_size = 0;
>       size_t offset = 0;
> +    if (!ptr) {
> +        /* use inline entry */
> +        ptr = cb->inline_insn.entry.score->data->data;

This value will not survive the first resize.
You need to add a pointer dereference from the first "data".


r~


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 05/17] plugins: scoreboard API
  2024-02-07  3:21   ` Richard Henderson
@ 2024-02-07  5:59     ` Pierrick Bouvier
  2024-02-11  0:41       ` Richard Henderson
  0 siblings, 1 reply; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-07  5:59 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Alex Bennée,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/7/24 07:21, Richard Henderson wrote:
> On 2/6/24 19:24, Pierrick Bouvier wrote:
>> We introduce a cpu local storage, automatically managed (and extended)
>> by QEMU itself. Plugin allocate a scoreboard, and don't have to deal
>> with how many cpus are launched.
>>
>> This API will be used by new inline functions but callbacks can benefit
>> from this as well. This way, they can operate without a global lock for
>> simple operations.
>>
>> At any point during execution, any scoreboard will be dimensioned with
>> at least qemu_plugin_num_vcpus entries.
>>
>> New functions:
>> - qemu_plugin_scoreboard_find
>> - qemu_plugin_scoreboard_free
>> - qemu_plugin_scoreboard_new
>>
>> In more, we define a qemu_plugin_u64, which is a simple struct holding
>> a pointer to a scoreboard, and a given offset.
>> This allows to have a scoreboard containing structs, without having to
>> bring offset for all operations on a specific field.
>>
>> Since most of the plugins are simply collecting a sum of per-cpu values,
>> qemu_plugin_u64 directly support this operation as well.
>>
>> New functions:
>> - qemu_plugin_u64_add
>> - qemu_plugin_u64_get
>> - qemu_plugin_u64_set
>> - qemu_plugin_u64_sum
>> New macros:
>> - qemu_plugin_scoreboard_u64
>> - qemu_plugin_scoreboard_u64_in_struct
> 
> I think the u64 stuff should be a second patch built upon the basic scoreboard support.
> 

You're right, should be easier to review.

>> +/* A scoreboard is an array of values, indexed by vcpu_index */
>> +struct qemu_plugin_scoreboard {
>> +    GArray *data;
>> +};
> 
> Unnecessary?  Generates an extra pointer dereference for no apparent benefit.
> Alternately, might be useful for other data structure changes...
> 

Thought to change it to a typedef after removing other members. Will do 
if you noticed this too.

>> +/**
>> + * typedef qemu_plugin_u64 - uint64_t member of an entry in a scoreboard
>> + *
>> + * This field allows to access a specific uint64_t member in one given entry,
>> + * located at a specified offset. Inline operations expect this as entry.
>> + */
>> +typedef struct {
>> +    struct qemu_plugin_scoreboard *score;
> 
> Embed the struct instead?
> 

Several qemu_plugin_u64 can point to the same scoreboard, so it has to 
be a pointer. It saves a scoreboard pointer + offset for a given entry.

>> @@ -31,6 +31,9 @@ struct qemu_plugin_state {
>>         * but with the HT we avoid adding a field to CPUState.
>>         */
>>        GHashTable *cpu_ht;
>> +    /* Scoreboards, indexed by their addresses. */
>> +    GHashTable *scoreboards;
> 
> Why a hash table?  All you want is to be able to iterate through all, and add/remove
> easily.  Seems like QLIST from <qemu/queue.h> would be better, and the QLIST_ENTRY member
> would make struct qemu_plugin_scoreboard useful.
> 

Thought that having O(1) removal was a nice property, compared to a 
linked list. I can switch to a QLIST if you still think it's better.

What do you mean by "make struct qemu_plugin_scoreboard useful"?

> 
> r~


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 07/17] plugins: implement inline operation relative to cpu_index
  2024-02-07  3:42   ` Richard Henderson
@ 2024-02-07  6:01     ` Pierrick Bouvier
  0 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-07  6:01 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Alex Bennée,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/7/24 07:42, Richard Henderson wrote:
> On 2/6/24 19:24, Pierrick Bouvier wrote:
>> Instead of working on a fixed memory location, allow to address it based
>> on cpu_index, an element size and a given offset.
>> Result address: ptr + offset + cpu_index * element_size.
>>
>> With this, we can target a member in a struct array from a base pointer.
>>
>> Current semantic is not modified, thus inline operation still targets
>> always the same memory location.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    plugins/plugin.h       |  2 +-
>>    accel/tcg/plugin-gen.c | 65 +++++++++++++++++++++++++++++++++++-------
>>    plugins/api.c          |  3 +-
>>    plugins/core.c         | 12 +++++---
>>    4 files changed, 65 insertions(+), 17 deletions(-)
>>
>> diff --git a/plugins/plugin.h b/plugins/plugin.h
>> index fd93a372803..77ed10689ca 100644
>> --- a/plugins/plugin.h
>> +++ b/plugins/plugin.h
>> @@ -100,7 +100,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
>>                                     enum qemu_plugin_mem_rw rw,
>>                                     void *udata);
>>    
>> -void exec_inline_op(struct qemu_plugin_dyn_cb *cb);
>> +void exec_inline_op(struct qemu_plugin_dyn_cb *cb, int cpu_index);
>>    
>>    int plugin_num_vcpus(void);
>>    
>> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
>> index b37ce7683e6..68dee4c68d3 100644
>> --- a/accel/tcg/plugin-gen.c
>> +++ b/accel/tcg/plugin-gen.c
>> @@ -132,16 +132,28 @@ static void gen_empty_udata_cb_no_rwg(void)
>>     */
>>    static void gen_empty_inline_cb(void)
>>    {
>> +    TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
>> +    TCGv_ptr cpu_index_as_ptr = tcg_temp_ebb_new_ptr();
>>        TCGv_i64 val = tcg_temp_ebb_new_i64();
>>        TCGv_ptr ptr = tcg_temp_ebb_new_ptr();
>>    
>> +    tcg_gen_ld_i32(cpu_index, tcg_env,
>> +                   -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
>> +    /* pass an immediate != 0 so that it doesn't get optimized away */
>> +    tcg_gen_muli_i32(cpu_index, cpu_index, 0xdeadbeef);
> 
> You don't need a random immediate here.
> You can just as easily use
> 
>       tcg_gen_mul_i32(cpu_index, cpu_index, cpu_index);
> 
> with a similar comment about the true size being inserted later.
> 

Followed the tcg_gen_addi_i64 that was using this pattern in the same 
file. I'll change this to what you recommend.

> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 08/17] plugins: add inline operation per vcpu
  2024-02-07  3:45   ` Richard Henderson
@ 2024-02-07  6:05     ` Pierrick Bouvier
  0 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-07  6:05 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Alex Bennée,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/7/24 07:45, Richard Henderson wrote:
> On 2/6/24 19:24, Pierrick Bouvier wrote:
>> --- a/accel/tcg/plugin-gen.c
>> +++ b/accel/tcg/plugin-gen.c
>> @@ -442,6 +442,13 @@ static TCGOp *append_inline_cb(const struct qemu_plugin_dyn_cb *cb,
>>        char *ptr = cb->userp;
>>        size_t elem_size = 0;
>>        size_t offset = 0;
>> +    if (!ptr) {
>> +        /* use inline entry */
>> +        ptr = cb->inline_insn.entry.score->data->data;
> 
> This value will not survive the first resize.
> You need to add a pointer dereference from the first "data".
> 

If you look at scoreboard patch, you'll notice tb are flushed when we 
resize, and thus, invalidate the pointer.

We discussed this with Alex previously, and he recommended to implement 
this, instead of adding another indirection.

By the way, this is what created the need to fix cpu_init hook call 
site, to be able to call start/end exclusive. Thus the related patches 
at the beginning of the series.

> 
> r~


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 00/17] TCG Plugin inline operation enhancement
  2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
                   ` (16 preceding siblings ...)
  2024-02-06  9:24 ` [PATCH v3 17/17] MAINTAINERS: Add myself as reviewer for TCG Plugins Pierrick Bouvier
@ 2024-02-07 15:45 ` Alex Bennée
  2024-02-07 16:06   ` Alex Bennée
  17 siblings, 1 reply; 33+ messages in thread
From: Alex Bennée @ 2024-02-07 15:45 UTC (permalink / raw)
  To: Pierrick Bouvier
  Cc: qemu-devel, Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> This series adds a new thread-safe API to declare inline operation
> inside plugins. As well, it removes the existing non thread-safe API,
> and migrates all existing plugins to use it.

I've cherry-picked 3 and 4 into plugins/next so I can re-base the
registers work. I'll have a look at the rest of the series once I'm back
at my desk.

>
> Tested on Linux (user, system) for i386, x86_64 and aarch64.
>
> To give some context, this a long term series of work around plugins,
> with the goal to be able to do basic operations in a more performant and
> accurate way. This will mean to add more inline operations and
> conditional callbacks.
>
> One final target of this work is to implement a plugin that implements
> the icount=auto feature, and allow QEMU to run at a given "frequency"
> based on number of instructions executed, without QEMU needing to keep
> track of this.
>
> Another final target is to be able to detect control flow changes in an
> efficient and elegant way, by combining inline operation and conditional
> callbacks.
>
> v2
> --
>
> Implement scoreboard API (cpu local storage), so plugins don't have to deal
> with how many cpus are used.
>
> Since plugins have been modified again, I didn't transfer any reviewed-by on
> those commits.
>
> v3
> --
>
> - introduce qemu_plugin_num_vcpus (how many cpus were initialized)
> - fix order of plugin init/idle/resume callbacks
> - scoreboard:
>   - renamed qemu_plugin_u64_t -> qemu_plugin_u64
>   - some functions rename for scoreboard api
>   - qemu_plugin_u64 has only value based function (vs address before)
> - various cleanup thanks to review of previous series
>
> Based-on: 20240122145610.413836-1-alex.bennee@linaro.org
>
>  Version number: 3
>  Branches:
>           base:  plugin_registers_v3_20240122145610.413836-1-alex.bennee@linaro.org
>           topic: plugin_inline_per_vcpu
>
>  To: qemu-devel@nongnu.org
>
>  Pierrick Bouvier (17):
>        plugins: remove previous n_vcpus functions from API
>        plugins: add qemu_plugin_num_vcpus function
>        plugins: fix order of init/idle/resume callback
>        cpu: call plugin init hook asynchronously
>        plugins: scoreboard API
>        docs/devel: plugins can trigger a tb flush
>        plugins: implement inline operation relative to cpu_index
>        plugins: add inline operation per vcpu
>        tests/plugin: add test plugin for inline operations
>        tests/plugin/mem: migrate to new per_vcpu API
>        tests/plugin/insn: migrate to new per_vcpu API
>        tests/plugin/bb: migrate to new per_vcpu API
>        contrib/plugins/hotblocks: migrate to new per_vcpu API
>        contrib/plugins/howvec: migrate to new per_vcpu API
>        plugins: remove non per_vcpu inline operation from API
>        plugins: cleanup codepath for previous inline operation
>        MAINTAINERS: Add myself as reviewer for TCG Plugins
>
>   MAINTAINERS                     |   1 +
>   docs/devel/multi-thread-tcg.rst |   1 +
>   include/qemu/plugin.h           |   6 ++
>   include/qemu/qemu-plugin.h      | 151 ++++++++++++++++++++++++--------
>   plugins/plugin.h                |  24 ++++--
>   accel/tcg/plugin-gen.c          |  65 +++++++++++---
>   contrib/plugins/cache.c         |   2 +-
>   contrib/plugins/hotblocks.c     |  50 ++++++-----
>   contrib/plugins/howvec.c        |  53 ++++++++----
>   hw/core/cpu-common.c            |   9 +-
>   plugins/api.c                   | 121 ++++++++++++++++----------
>   plugins/core.c                  | 104 +++++++++++++++++++---
>   tests/plugin/bb.c               |  63 ++++++--------
>   tests/plugin/inline.c           | 186 ++++++++++++++++++++++++++++++++++++++++
>   tests/plugin/insn.c             | 106 +++++++++++------------
>   tests/plugin/mem.c              |  40 +++++----
>   plugins/qemu-plugins.symbols    |  16 ++--
>   tests/plugin/meson.build        |   2 +-
>   18 files changed, 745 insertions(+), 255 deletions(-)
>
> Pierrick Bouvier (17):
>   plugins: remove previous n_vcpus functions from API
>   plugins: add qemu_plugin_num_vcpus function
>   plugins: fix order of init/idle/resume callback
>   cpu: call plugin init hook asynchronously
>   plugins: scoreboard API
>   docs/devel: plugins can trigger a tb flush
>   plugins: implement inline operation relative to cpu_index
>   plugins: add inline operation per vcpu
>   tests/plugin: add test plugin for inline operations
>   tests/plugin/mem: migrate to new per_vcpu API
>   tests/plugin/insn: migrate to new per_vcpu API
>   tests/plugin/bb: migrate to new per_vcpu API
>   contrib/plugins/hotblocks: migrate to new per_vcpu API
>   contrib/plugins/howvec: migrate to new per_vcpu API
>   plugins: remove non per_vcpu inline operation from API
>   plugins: cleanup codepath for previous inline operation
>   MAINTAINERS: Add myself as reviewer for TCG Plugins
>
>  MAINTAINERS                     |   1 +
>  docs/devel/multi-thread-tcg.rst |   1 +
>  include/qemu/plugin.h           |   6 ++
>  include/qemu/qemu-plugin.h      | 151 ++++++++++++++++++++------
>  plugins/plugin.h                |  24 +++--
>  accel/tcg/plugin-gen.c          |  65 +++++++++--
>  contrib/plugins/cache.c         |   2 +-
>  contrib/plugins/hotblocks.c     |  50 +++++----
>  contrib/plugins/howvec.c        |  53 ++++++---
>  hw/core/cpu-common.c            |   9 +-
>  plugins/api.c                   | 121 +++++++++++++--------
>  plugins/core.c                  | 104 ++++++++++++++++--
>  tests/plugin/bb.c               |  63 +++++------
>  tests/plugin/inline.c           | 186 ++++++++++++++++++++++++++++++++
>  tests/plugin/insn.c             | 106 +++++++++---------
>  tests/plugin/mem.c              |  40 ++++---
>  plugins/qemu-plugins.symbols    |  16 ++-
>  tests/plugin/meson.build        |   2 +-
>  18 files changed, 745 insertions(+), 255 deletions(-)
>  create mode 100644 tests/plugin/inline.c

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 00/17] TCG Plugin inline operation enhancement
  2024-02-07 15:45 ` [PATCH v3 00/17] TCG Plugin inline operation enhancement Alex Bennée
@ 2024-02-07 16:06   ` Alex Bennée
  2024-02-07 18:21     ` Pierrick Bouvier
  0 siblings, 1 reply; 33+ messages in thread
From: Alex Bennée @ 2024-02-07 16:06 UTC (permalink / raw)
  To: Pierrick Bouvier
  Cc: qemu-devel, Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

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

> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>
>> This series adds a new thread-safe API to declare inline operation
>> inside plugins. As well, it removes the existing non thread-safe API,
>> and migrates all existing plugins to use it.
>
> I've cherry-picked 3 and 4 into plugins/next so I can re-base the
> registers work. I'll have a look at the rest of the series once I'm back
> at my desk.

Actually belay that, I'm caught in a circular dependency because this is
based on the registers series. I'll sort it out tomorrow.

>
>>
>> Tested on Linux (user, system) for i386, x86_64 and aarch64.
>>
>> To give some context, this a long term series of work around plugins,
>> with the goal to be able to do basic operations in a more performant and
>> accurate way. This will mean to add more inline operations and
>> conditional callbacks.
>>
>> One final target of this work is to implement a plugin that implements
>> the icount=auto feature, and allow QEMU to run at a given "frequency"
>> based on number of instructions executed, without QEMU needing to keep
>> track of this.
>>
>> Another final target is to be able to detect control flow changes in an
>> efficient and elegant way, by combining inline operation and conditional
>> callbacks.
>>
>> v2
>> --
>>
>> Implement scoreboard API (cpu local storage), so plugins don't have to deal
>> with how many cpus are used.
>>
>> Since plugins have been modified again, I didn't transfer any reviewed-by on
>> those commits.
>>
>> v3
>> --
>>
>> - introduce qemu_plugin_num_vcpus (how many cpus were initialized)
>> - fix order of plugin init/idle/resume callbacks
>> - scoreboard:
>>   - renamed qemu_plugin_u64_t -> qemu_plugin_u64
>>   - some functions rename for scoreboard api
>>   - qemu_plugin_u64 has only value based function (vs address before)
>> - various cleanup thanks to review of previous series
>>
>> Based-on: 20240122145610.413836-1-alex.bennee@linaro.org
>>
>>  Version number: 3
>>  Branches:
>>           base:  plugin_registers_v3_20240122145610.413836-1-alex.bennee@linaro.org
>>           topic: plugin_inline_per_vcpu
>>
>>  To: qemu-devel@nongnu.org
>>
>>  Pierrick Bouvier (17):
>>        plugins: remove previous n_vcpus functions from API
>>        plugins: add qemu_plugin_num_vcpus function
>>        plugins: fix order of init/idle/resume callback
>>        cpu: call plugin init hook asynchronously
>>        plugins: scoreboard API
>>        docs/devel: plugins can trigger a tb flush
>>        plugins: implement inline operation relative to cpu_index
>>        plugins: add inline operation per vcpu
>>        tests/plugin: add test plugin for inline operations
>>        tests/plugin/mem: migrate to new per_vcpu API
>>        tests/plugin/insn: migrate to new per_vcpu API
>>        tests/plugin/bb: migrate to new per_vcpu API
>>        contrib/plugins/hotblocks: migrate to new per_vcpu API
>>        contrib/plugins/howvec: migrate to new per_vcpu API
>>        plugins: remove non per_vcpu inline operation from API
>>        plugins: cleanup codepath for previous inline operation
>>        MAINTAINERS: Add myself as reviewer for TCG Plugins
>>
>>   MAINTAINERS                     |   1 +
>>   docs/devel/multi-thread-tcg.rst |   1 +
>>   include/qemu/plugin.h           |   6 ++
>>   include/qemu/qemu-plugin.h      | 151 ++++++++++++++++++++++++--------
>>   plugins/plugin.h                |  24 ++++--
>>   accel/tcg/plugin-gen.c          |  65 +++++++++++---
>>   contrib/plugins/cache.c         |   2 +-
>>   contrib/plugins/hotblocks.c     |  50 ++++++-----
>>   contrib/plugins/howvec.c        |  53 ++++++++----
>>   hw/core/cpu-common.c            |   9 +-
>>   plugins/api.c                   | 121 ++++++++++++++++----------
>>   plugins/core.c                  | 104 +++++++++++++++++++---
>>   tests/plugin/bb.c               |  63 ++++++--------
>>   tests/plugin/inline.c           | 186 ++++++++++++++++++++++++++++++++++++++++
>>   tests/plugin/insn.c             | 106 +++++++++++------------
>>   tests/plugin/mem.c              |  40 +++++----
>>   plugins/qemu-plugins.symbols    |  16 ++--
>>   tests/plugin/meson.build        |   2 +-
>>   18 files changed, 745 insertions(+), 255 deletions(-)
>>
>> Pierrick Bouvier (17):
>>   plugins: remove previous n_vcpus functions from API
>>   plugins: add qemu_plugin_num_vcpus function
>>   plugins: fix order of init/idle/resume callback
>>   cpu: call plugin init hook asynchronously
>>   plugins: scoreboard API
>>   docs/devel: plugins can trigger a tb flush
>>   plugins: implement inline operation relative to cpu_index
>>   plugins: add inline operation per vcpu
>>   tests/plugin: add test plugin for inline operations
>>   tests/plugin/mem: migrate to new per_vcpu API
>>   tests/plugin/insn: migrate to new per_vcpu API
>>   tests/plugin/bb: migrate to new per_vcpu API
>>   contrib/plugins/hotblocks: migrate to new per_vcpu API
>>   contrib/plugins/howvec: migrate to new per_vcpu API
>>   plugins: remove non per_vcpu inline operation from API
>>   plugins: cleanup codepath for previous inline operation
>>   MAINTAINERS: Add myself as reviewer for TCG Plugins
>>
>>  MAINTAINERS                     |   1 +
>>  docs/devel/multi-thread-tcg.rst |   1 +
>>  include/qemu/plugin.h           |   6 ++
>>  include/qemu/qemu-plugin.h      | 151 ++++++++++++++++++++------
>>  plugins/plugin.h                |  24 +++--
>>  accel/tcg/plugin-gen.c          |  65 +++++++++--
>>  contrib/plugins/cache.c         |   2 +-
>>  contrib/plugins/hotblocks.c     |  50 +++++----
>>  contrib/plugins/howvec.c        |  53 ++++++---
>>  hw/core/cpu-common.c            |   9 +-
>>  plugins/api.c                   | 121 +++++++++++++--------
>>  plugins/core.c                  | 104 ++++++++++++++++--
>>  tests/plugin/bb.c               |  63 +++++------
>>  tests/plugin/inline.c           | 186 ++++++++++++++++++++++++++++++++
>>  tests/plugin/insn.c             | 106 +++++++++---------
>>  tests/plugin/mem.c              |  40 ++++---
>>  plugins/qemu-plugins.symbols    |  16 ++-
>>  tests/plugin/meson.build        |   2 +-
>>  18 files changed, 745 insertions(+), 255 deletions(-)
>>  create mode 100644 tests/plugin/inline.c

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 00/17] TCG Plugin inline operation enhancement
  2024-02-07 16:06   ` Alex Bennée
@ 2024-02-07 18:21     ` Pierrick Bouvier
  0 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-07 18:21 UTC (permalink / raw)
  To: Alex Bennée
  Cc: qemu-devel, Eduardo Habkost, Alexandre Iooss, Richard Henderson,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/7/24 20:06, Alex Bennée wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>
>>> This series adds a new thread-safe API to declare inline operation
>>> inside plugins. As well, it removes the existing non thread-safe API,
>>> and migrates all existing plugins to use it.
>>
>> I've cherry-picked 3 and 4 into plugins/next so I can re-base the
>> registers work. I'll have a look at the rest of the series once I'm back
>> at my desk.
> 
> Actually belay that, I'm caught in a circular dependency because this is
> based on the registers series. I'll sort it out tomorrow.
> 

You'll need 1 and 2 too, which allows to get number of cpus init, used 
in 4. You can as well submit this as part of register series if you 
prefer, so there won't be any conflict.

>>
>>>
>>> Tested on Linux (user, system) for i386, x86_64 and aarch64.
>>>
>>> To give some context, this a long term series of work around plugins,
>>> with the goal to be able to do basic operations in a more performant and
>>> accurate way. This will mean to add more inline operations and
>>> conditional callbacks.
>>>
>>> One final target of this work is to implement a plugin that implements
>>> the icount=auto feature, and allow QEMU to run at a given "frequency"
>>> based on number of instructions executed, without QEMU needing to keep
>>> track of this.
>>>
>>> Another final target is to be able to detect control flow changes in an
>>> efficient and elegant way, by combining inline operation and conditional
>>> callbacks.
>>>
>>> v2
>>> --
>>>
>>> Implement scoreboard API (cpu local storage), so plugins don't have to deal
>>> with how many cpus are used.
>>>
>>> Since plugins have been modified again, I didn't transfer any reviewed-by on
>>> those commits.
>>>
>>> v3
>>> --
>>>
>>> - introduce qemu_plugin_num_vcpus (how many cpus were initialized)
>>> - fix order of plugin init/idle/resume callbacks
>>> - scoreboard:
>>>    - renamed qemu_plugin_u64_t -> qemu_plugin_u64
>>>    - some functions rename for scoreboard api
>>>    - qemu_plugin_u64 has only value based function (vs address before)
>>> - various cleanup thanks to review of previous series
>>>
>>> Based-on: 20240122145610.413836-1-alex.bennee@linaro.org
>>>
>>>   Version number: 3
>>>   Branches:
>>>            base:  plugin_registers_v3_20240122145610.413836-1-alex.bennee@linaro.org
>>>            topic: plugin_inline_per_vcpu
>>>
>>>   To: qemu-devel@nongnu.org
>>>
>>>   Pierrick Bouvier (17):
>>>         plugins: remove previous n_vcpus functions from API
>>>         plugins: add qemu_plugin_num_vcpus function
>>>         plugins: fix order of init/idle/resume callback
>>>         cpu: call plugin init hook asynchronously
>>>         plugins: scoreboard API
>>>         docs/devel: plugins can trigger a tb flush
>>>         plugins: implement inline operation relative to cpu_index
>>>         plugins: add inline operation per vcpu
>>>         tests/plugin: add test plugin for inline operations
>>>         tests/plugin/mem: migrate to new per_vcpu API
>>>         tests/plugin/insn: migrate to new per_vcpu API
>>>         tests/plugin/bb: migrate to new per_vcpu API
>>>         contrib/plugins/hotblocks: migrate to new per_vcpu API
>>>         contrib/plugins/howvec: migrate to new per_vcpu API
>>>         plugins: remove non per_vcpu inline operation from API
>>>         plugins: cleanup codepath for previous inline operation
>>>         MAINTAINERS: Add myself as reviewer for TCG Plugins
>>>
>>>    MAINTAINERS                     |   1 +
>>>    docs/devel/multi-thread-tcg.rst |   1 +
>>>    include/qemu/plugin.h           |   6 ++
>>>    include/qemu/qemu-plugin.h      | 151 ++++++++++++++++++++++++--------
>>>    plugins/plugin.h                |  24 ++++--
>>>    accel/tcg/plugin-gen.c          |  65 +++++++++++---
>>>    contrib/plugins/cache.c         |   2 +-
>>>    contrib/plugins/hotblocks.c     |  50 ++++++-----
>>>    contrib/plugins/howvec.c        |  53 ++++++++----
>>>    hw/core/cpu-common.c            |   9 +-
>>>    plugins/api.c                   | 121 ++++++++++++++++----------
>>>    plugins/core.c                  | 104 +++++++++++++++++++---
>>>    tests/plugin/bb.c               |  63 ++++++--------
>>>    tests/plugin/inline.c           | 186 ++++++++++++++++++++++++++++++++++++++++
>>>    tests/plugin/insn.c             | 106 +++++++++++------------
>>>    tests/plugin/mem.c              |  40 +++++----
>>>    plugins/qemu-plugins.symbols    |  16 ++--
>>>    tests/plugin/meson.build        |   2 +-
>>>    18 files changed, 745 insertions(+), 255 deletions(-)
>>>
>>> Pierrick Bouvier (17):
>>>    plugins: remove previous n_vcpus functions from API
>>>    plugins: add qemu_plugin_num_vcpus function
>>>    plugins: fix order of init/idle/resume callback
>>>    cpu: call plugin init hook asynchronously
>>>    plugins: scoreboard API
>>>    docs/devel: plugins can trigger a tb flush
>>>    plugins: implement inline operation relative to cpu_index
>>>    plugins: add inline operation per vcpu
>>>    tests/plugin: add test plugin for inline operations
>>>    tests/plugin/mem: migrate to new per_vcpu API
>>>    tests/plugin/insn: migrate to new per_vcpu API
>>>    tests/plugin/bb: migrate to new per_vcpu API
>>>    contrib/plugins/hotblocks: migrate to new per_vcpu API
>>>    contrib/plugins/howvec: migrate to new per_vcpu API
>>>    plugins: remove non per_vcpu inline operation from API
>>>    plugins: cleanup codepath for previous inline operation
>>>    MAINTAINERS: Add myself as reviewer for TCG Plugins
>>>
>>>   MAINTAINERS                     |   1 +
>>>   docs/devel/multi-thread-tcg.rst |   1 +
>>>   include/qemu/plugin.h           |   6 ++
>>>   include/qemu/qemu-plugin.h      | 151 ++++++++++++++++++++------
>>>   plugins/plugin.h                |  24 +++--
>>>   accel/tcg/plugin-gen.c          |  65 +++++++++--
>>>   contrib/plugins/cache.c         |   2 +-
>>>   contrib/plugins/hotblocks.c     |  50 +++++----
>>>   contrib/plugins/howvec.c        |  53 ++++++---
>>>   hw/core/cpu-common.c            |   9 +-
>>>   plugins/api.c                   | 121 +++++++++++++--------
>>>   plugins/core.c                  | 104 ++++++++++++++++--
>>>   tests/plugin/bb.c               |  63 +++++------
>>>   tests/plugin/inline.c           | 186 ++++++++++++++++++++++++++++++++
>>>   tests/plugin/insn.c             | 106 +++++++++---------
>>>   tests/plugin/mem.c              |  40 ++++---
>>>   plugins/qemu-plugins.symbols    |  16 ++-
>>>   tests/plugin/meson.build        |   2 +-
>>>   18 files changed, 745 insertions(+), 255 deletions(-)
>>>   create mode 100644 tests/plugin/inline.c
> 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 05/17] plugins: scoreboard API
  2024-02-07  5:59     ` Pierrick Bouvier
@ 2024-02-11  0:41       ` Richard Henderson
  2024-02-11 14:26         ` Pierrick Bouvier
  0 siblings, 1 reply; 33+ messages in thread
From: Richard Henderson @ 2024-02-11  0:41 UTC (permalink / raw)
  To: Pierrick Bouvier, qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Alex Bennée,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/6/24 19:59, Pierrick Bouvier wrote:
>> Why a hash table?  All you want is to be able to iterate through all, and add/remove
>> easily.  Seems like QLIST from <qemu/queue.h> would be better, and the QLIST_ENTRY member
>> would make struct qemu_plugin_scoreboard useful.
>>
> 
> Thought that having O(1) removal was a nice property, compared to a linked list. I can 
> switch to a QLIST if you still think it's better.

QLIST is double-linked, so it's still O(1).


> What do you mean by "make struct qemu_plugin_scoreboard useful"?

As opposed to simply a typedef of GArray.


r~



^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 05/17] plugins: scoreboard API
  2024-02-11  0:41       ` Richard Henderson
@ 2024-02-11 14:26         ` Pierrick Bouvier
  2024-02-11 19:13           ` Richard Henderson
  0 siblings, 1 reply; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-11 14:26 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Alex Bennée,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/11/24 04:41, Richard Henderson wrote:
> On 2/6/24 19:59, Pierrick Bouvier wrote:
>>> Why a hash table?  All you want is to be able to iterate through all, and add/remove
>>> easily.  Seems like QLIST from <qemu/queue.h> would be better, and the QLIST_ENTRY member
>>> would make struct qemu_plugin_scoreboard useful.
>>>
>>
>> Thought that having O(1) removal was a nice property, compared to a linked list. I can
>> switch to a QLIST if you still think it's better.
> 
> QLIST is double-linked, so it's still O(1).
> 
>

Is it an "intrusive" linked list (where next and prev are part of the 
struct entry itself)?

>> What do you mean by "make struct qemu_plugin_scoreboard useful"?
> 
> As opposed to simply a typedef of GArray.
> 
> 
> r~
> 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 05/17] plugins: scoreboard API
  2024-02-11 14:26         ` Pierrick Bouvier
@ 2024-02-11 19:13           ` Richard Henderson
  2024-02-12  6:24             ` Pierrick Bouvier
  0 siblings, 1 reply; 33+ messages in thread
From: Richard Henderson @ 2024-02-11 19:13 UTC (permalink / raw)
  To: Pierrick Bouvier, qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Alex Bennée,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/11/24 04:26, Pierrick Bouvier wrote:
> On 2/11/24 04:41, Richard Henderson wrote:
>> On 2/6/24 19:59, Pierrick Bouvier wrote:
>>>> Why a hash table?  All you want is to be able to iterate through all, and add/remove
>>>> easily.  Seems like QLIST from <qemu/queue.h> would be better, and the QLIST_ENTRY member
>>>> would make struct qemu_plugin_scoreboard useful.
>>>>
>>>
>>> Thought that having O(1) removal was a nice property, compared to a linked list. I can
>>> switch to a QLIST if you still think it's better.
>>
>> QLIST is double-linked, so it's still O(1).
>>
>>
> 
> Is it an "intrusive" linked list (where next and prev are part of the struct entry itself)?

Yes.


r~


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 05/17] plugins: scoreboard API
  2024-02-11 19:13           ` Richard Henderson
@ 2024-02-12  6:24             ` Pierrick Bouvier
  0 siblings, 0 replies; 33+ messages in thread
From: Pierrick Bouvier @ 2024-02-12  6:24 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel
  Cc: Eduardo Habkost, Alexandre Iooss, Alex Bennée,
	Mahmoud Mandour, Marcel Apfelbaum, Yanan Wang, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 2/11/24 23:13, Richard Henderson wrote:
> On 2/11/24 04:26, Pierrick Bouvier wrote:
>> On 2/11/24 04:41, Richard Henderson wrote:
>>> On 2/6/24 19:59, Pierrick Bouvier wrote:
>>>>> Why a hash table?  All you want is to be able to iterate through all, and add/remove
>>>>> easily.  Seems like QLIST from <qemu/queue.h> would be better, and the QLIST_ENTRY member
>>>>> would make struct qemu_plugin_scoreboard useful.
>>>>>
>>>>
>>>> Thought that having O(1) removal was a nice property, compared to a linked list. I can
>>>> switch to a QLIST if you still think it's better.
>>>
>>> QLIST is double-linked, so it's still O(1).
>>>
>>>
>>
>> Is it an "intrusive" linked list (where next and prev are part of the struct entry itself)?
> 
> Yes.
>

Sounds good then, I'll switch to QLIST.

> 
> r~

^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2024-02-12  6:25 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-06  9:24 [PATCH v3 00/17] TCG Plugin inline operation enhancement Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 01/17] plugins: remove previous n_vcpus functions from API Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 02/17] plugins: add qemu_plugin_num_vcpus function Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 03/17] plugins: fix order of init/idle/resume callback Pierrick Bouvier
2024-02-07  2:59   ` Richard Henderson
2024-02-06  9:24 ` [PATCH v3 04/17] cpu: call plugin init hook asynchronously Pierrick Bouvier
2024-02-07  3:00   ` Richard Henderson
2024-02-06  9:24 ` [PATCH v3 05/17] plugins: scoreboard API Pierrick Bouvier
2024-02-07  3:21   ` Richard Henderson
2024-02-07  5:59     ` Pierrick Bouvier
2024-02-11  0:41       ` Richard Henderson
2024-02-11 14:26         ` Pierrick Bouvier
2024-02-11 19:13           ` Richard Henderson
2024-02-12  6:24             ` Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 06/17] docs/devel: plugins can trigger a tb flush Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 07/17] plugins: implement inline operation relative to cpu_index Pierrick Bouvier
2024-02-07  3:42   ` Richard Henderson
2024-02-07  6:01     ` Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 08/17] plugins: add inline operation per vcpu Pierrick Bouvier
2024-02-07  3:45   ` Richard Henderson
2024-02-07  6:05     ` Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 09/17] tests/plugin: add test plugin for inline operations Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 10/17] tests/plugin/mem: migrate to new per_vcpu API Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 11/17] tests/plugin/insn: " Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 12/17] tests/plugin/bb: " Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 13/17] contrib/plugins/hotblocks: " Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 14/17] contrib/plugins/howvec: " Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 15/17] plugins: remove non per_vcpu inline operation from API Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 16/17] plugins: cleanup codepath for previous inline operation Pierrick Bouvier
2024-02-06  9:24 ` [PATCH v3 17/17] MAINTAINERS: Add myself as reviewer for TCG Plugins Pierrick Bouvier
2024-02-07 15:45 ` [PATCH v3 00/17] TCG Plugin inline operation enhancement Alex Bennée
2024-02-07 16:06   ` Alex Bennée
2024-02-07 18:21     ` Pierrick Bouvier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).