[PATCH 0/2] add explicit virtual time callback for plugins

[PATCH 0/2] add explicit virtual time callback for plugins

[Alex Bennée]

When we added qemu_plugin_update_ns() to advance time we missed the
fact that users of virtual clock don't get updated. Rather than
implement the logic inside QEMU to keep track of the magic number lets
just delegate it to the plugin instead.

Compile tested only.

Alex.

Alex Bennée (2):
  accel/tcg: add get_virtual_clock for TCG
  plugins: add qemu_plugin_register_time_cb support

 include/qemu/plugin-event.h |  1 +
 include/qemu/plugin.h       |  9 +++++++++
 include/qemu/qemu-plugin.h  | 18 ++++++++++++++++++
 accel/tcg/tcg-accel-ops.c   | 11 +++++++++++
 plugins/api-system.c        |  8 ++++++++
 plugins/core.c              | 22 ++++++++++++++++++++++
 6 files changed, 69 insertions(+)

-- 
2.39.5

[PATCH 1/2] accel/tcg: add get_virtual_clock for TCG

[Alex Bennée]

Rather than allowing cpus_get_virtual_clock() to fall through to
cpu_get_clock() introduce a TCG handler so it can make a decision
about what time it is.

Initially this just calls cpu_get_clock() as before but this will
change in later commits.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/tcg-accel-ops.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index d9b662efe3..1432d1c5b1 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -197,6 +197,11 @@ static inline void tcg_remove_all_breakpoints(CPUState *cpu)
     cpu_watchpoint_remove_all(cpu, BP_GDB);
 }
 
+static int64_t tcg_get_virtual_clock(void)
+{
+    return cpu_get_clock();
+}
+
 static void tcg_accel_ops_init(AccelOpsClass *ops)
 {
     if (qemu_tcg_mttcg_enabled()) {
@@ -212,6 +217,7 @@ static void tcg_accel_ops_init(AccelOpsClass *ops)
             ops->get_virtual_clock = icount_get;
             ops->get_elapsed_ticks = icount_get;
         } else {
+            ops->get_virtual_clock = tcg_get_virtual_clock;
             ops->handle_interrupt = tcg_handle_interrupt;
         }
     }
-- 
2.39.5

[PATCH 2/2] plugins: add qemu_plugin_register_time_cb support

[Alex Bennée]

This allows the a plugin which has control of time to supply a callback
so it can control the reported virtual time instead of using the
default cpu_get_clock().

Time control plugins still need to call qemu_plugin_update_ns() to
ensure timers are moved forward.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/plugin-event.h |  1 +
 include/qemu/plugin.h       |  9 +++++++++
 include/qemu/qemu-plugin.h  | 18 ++++++++++++++++++
 accel/tcg/tcg-accel-ops.c   |  5 +++++
 plugins/api-system.c        |  8 ++++++++
 plugins/core.c              | 22 ++++++++++++++++++++++
 6 files changed, 63 insertions(+)

diff --git a/include/qemu/plugin-event.h b/include/qemu/plugin-event.h
index 7056d8427b..ae9ec5ce85 100644
--- a/include/qemu/plugin-event.h
+++ b/include/qemu/plugin-event.h
@@ -20,6 +20,7 @@ enum qemu_plugin_event {
     QEMU_PLUGIN_EV_VCPU_SYSCALL_RET,
     QEMU_PLUGIN_EV_FLUSH,
     QEMU_PLUGIN_EV_ATEXIT,
+    QEMU_PLUGIN_EV_GET_TIME,
     QEMU_PLUGIN_EV_MAX, /* total number of plugin events we support */
 };
 
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 9726a9ebf3..a9371a9a42 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -63,6 +63,7 @@ union qemu_plugin_cb_sig {
     qemu_plugin_vcpu_mem_cb_t        vcpu_mem;
     qemu_plugin_vcpu_syscall_cb_t    vcpu_syscall;
     qemu_plugin_vcpu_syscall_ret_cb_t vcpu_syscall_ret;
+    qemu_plugin_time_cb_t            time;
     void *generic;
 };
 
@@ -175,6 +176,14 @@ void qemu_plugin_flush_cb(void);
 
 void qemu_plugin_atexit_cb(void);
 
+/**
+ * qemu_plugin_maybe_fetch_time() - fetch virtual time from plugin
+ * @tptr: pointer to int64_t for result
+ *
+ * Returns true if the plugin has set time, otherwise false
+ */
+bool qemu_plugin_maybe_fetch_time(int64_t *tptr);
+
 void qemu_plugin_add_dyn_cb_arr(GArray *arr);
 
 static inline void qemu_plugin_disable_mem_helpers(CPUState *cpu)
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 3a850aa216..c5f1cad8fb 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -713,6 +713,22 @@ void qemu_plugin_register_vcpu_mem_inline_per_vcpu(
 QEMU_PLUGIN_API
 const void *qemu_plugin_request_time_control(void);
 
+/**
+ * typedef qemu_plugin_vcpu_mem_cb_t - time callback function
+ * Returns time in ns (starting from 0)
+ */
+typedef int64_t (*qemu_plugin_time_cb_t) (void);
+
+/**
+ * qemu_plugin_register_time_cb() - register a time callback
+ *
+ * This can only be called once a plugin has successfully called
+ * qemu_plugin_request_time_control(). The callback will get called
+ * whenever qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) is called.
+ */
+QEMU_PLUGIN_API
+void qemu_plugin_register_time_cb(qemu_plugin_id_t id, const void *handle, qemu_plugin_time_cb_t cb);
+
 /**
  * qemu_plugin_update_ns() - update system emulation time
  * @handle: opaque handle returned by qemu_plugin_request_time_control()
@@ -723,6 +739,8 @@ const void *qemu_plugin_request_time_control(void);
  * user-mode emulation the time is not changed by this as all reported
  * time comes from the host kernel.
  *
+ * This allows QEMU to execute any pending timers.
+ *
  * Start time is 0.
  */
 QEMU_PLUGIN_API
diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index 1432d1c5b1..5ed748f5cc 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -33,6 +33,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/guest-random.h"
 #include "qemu/timer.h"
+#include "qemu/plugin.h"
 #include "exec/cputlb.h"
 #include "exec/hwaddr.h"
 #include "exec/tb-flush.h"
@@ -199,6 +200,10 @@ static inline void tcg_remove_all_breakpoints(CPUState *cpu)
 
 static int64_t tcg_get_virtual_clock(void)
 {
+    int64_t from_plugin;
+    if (qemu_plugin_maybe_fetch_time(&from_plugin)) {
+        return from_plugin;
+    }
     return cpu_get_clock();
 }
 
diff --git a/plugins/api-system.c b/plugins/api-system.c
index cc190b167e..0f2a3eb5a6 100644
--- a/plugins/api-system.c
+++ b/plugins/api-system.c
@@ -17,6 +17,7 @@
 #include "hw/boards.h"
 #include "qemu/plugin-memory.h"
 #include "qemu/plugin.h"
+#include "plugin.h"
 
 /*
  * In system mode we cannot trace the binary being executed so the
@@ -129,3 +130,10 @@ void qemu_plugin_update_ns(const void *handle, int64_t new_time)
                          RUN_ON_CPU_HOST_ULONG(new_time));
     }
 }
+
+void qemu_plugin_register_time_cb(qemu_plugin_id_t id, const void *handle, qemu_plugin_time_cb_t cb)
+{
+    if (handle == &has_control) {
+        plugin_register_cb(id, QEMU_PLUGIN_EV_GET_TIME, cb);
+    }
+}
diff --git a/plugins/core.c b/plugins/core.c
index eb9281fe54..d56b4c9d48 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -571,6 +571,28 @@ void qemu_plugin_flush_cb(void)
     plugin_cb__simple(QEMU_PLUGIN_EV_FLUSH);
 }
 
+/*
+ * Disable CFI checks.
+ * The callback function has been loaded from an external library so we do not
+ * have type information
+ */
+QEMU_DISABLE_CFI
+bool qemu_plugin_maybe_fetch_time(int64_t *tptr)
+{
+    enum qemu_plugin_event ev = QEMU_PLUGIN_EV_GET_TIME;
+
+    /* there should only be one callback */
+    if (!QLIST_EMPTY(&plugin.cb_lists[ev])) {
+        struct qemu_plugin_cb *cb = QLIST_FIRST(&plugin.cb_lists[ev]);
+        qemu_plugin_time_cb_t func = cb->f.generic;
+        *tptr = func();
+        return true;
+    }
+
+    return false;
+}
+
+
 void exec_inline_op(enum plugin_dyn_cb_type type,
                     struct qemu_plugin_inline_cb *cb,
                     int cpu_index)
-- 
2.39.5

Re: [PATCH 1/2] accel/tcg: add get_virtual_clock for TCG

[Pierrick Bouvier]

On 4/3/25 04:38, Alex Bennée wrote:
> Rather than allowing cpus_get_virtual_clock() to fall through to
> cpu_get_clock() introduce a TCG handler so it can make a decision
> about what time it is.
> 
> Initially this just calls cpu_get_clock() as before but this will
> change in later commits.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   accel/tcg/tcg-accel-ops.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> index d9b662efe3..1432d1c5b1 100644
> --- a/accel/tcg/tcg-accel-ops.c
> +++ b/accel/tcg/tcg-accel-ops.c
> @@ -197,6 +197,11 @@ static inline void tcg_remove_all_breakpoints(CPUState *cpu)
>       cpu_watchpoint_remove_all(cpu, BP_GDB);
>   }
>   
> +static int64_t tcg_get_virtual_clock(void)
> +{
> +    return cpu_get_clock();
> +}
> +
>   static void tcg_accel_ops_init(AccelOpsClass *ops)
>   {
>       if (qemu_tcg_mttcg_enabled()) {
> @@ -212,6 +217,7 @@ static void tcg_accel_ops_init(AccelOpsClass *ops)
>               ops->get_virtual_clock = icount_get;
>               ops->get_elapsed_ticks = icount_get;
>           } else {
> +            ops->get_virtual_clock = tcg_get_virtual_clock;
>               ops->handle_interrupt = tcg_handle_interrupt;
>           }
>       }

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 2/2] plugins: add qemu_plugin_register_time_cb support

[Pierrick Bouvier]

On 4/3/25 04:38, Alex Bennée wrote:
> This allows the a plugin which has control of time to supply a callback
> so it can control the reported virtual time instead of using the
> default cpu_get_clock().
> 
> Time control plugins still need to call qemu_plugin_update_ns() to
> ensure timers are moved forward.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/qemu/plugin-event.h |  1 +
>   include/qemu/plugin.h       |  9 +++++++++
>   include/qemu/qemu-plugin.h  | 18 ++++++++++++++++++
>   accel/tcg/tcg-accel-ops.c   |  5 +++++
>   plugins/api-system.c        |  8 ++++++++
>   plugins/core.c              | 22 ++++++++++++++++++++++
>   6 files changed, 63 insertions(+)
> 
> diff --git a/include/qemu/plugin-event.h b/include/qemu/plugin-event.h
> index 7056d8427b..ae9ec5ce85 100644
> --- a/include/qemu/plugin-event.h
> +++ b/include/qemu/plugin-event.h
> @@ -20,6 +20,7 @@ enum qemu_plugin_event {
>       QEMU_PLUGIN_EV_VCPU_SYSCALL_RET,
>       QEMU_PLUGIN_EV_FLUSH,
>       QEMU_PLUGIN_EV_ATEXIT,
> +    QEMU_PLUGIN_EV_GET_TIME,
>       QEMU_PLUGIN_EV_MAX, /* total number of plugin events we support */
>   };
>   
> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
> index 9726a9ebf3..a9371a9a42 100644
> --- a/include/qemu/plugin.h
> +++ b/include/qemu/plugin.h
> @@ -63,6 +63,7 @@ union qemu_plugin_cb_sig {
>       qemu_plugin_vcpu_mem_cb_t        vcpu_mem;
>       qemu_plugin_vcpu_syscall_cb_t    vcpu_syscall;
>       qemu_plugin_vcpu_syscall_ret_cb_t vcpu_syscall_ret;
> +    qemu_plugin_time_cb_t            time;
>       void *generic;
>   };
>   
> @@ -175,6 +176,14 @@ void qemu_plugin_flush_cb(void);
>   
>   void qemu_plugin_atexit_cb(void);
>   
> +/**
> + * qemu_plugin_maybe_fetch_time() - fetch virtual time from plugin
> + * @tptr: pointer to int64_t for result
> + *
> + * Returns true if the plugin has set time, otherwise false
> + */
> +bool qemu_plugin_maybe_fetch_time(int64_t *tptr);
> +
>   void qemu_plugin_add_dyn_cb_arr(GArray *arr);
>   
>   static inline void qemu_plugin_disable_mem_helpers(CPUState *cpu)
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 3a850aa216..c5f1cad8fb 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -713,6 +713,22 @@ void qemu_plugin_register_vcpu_mem_inline_per_vcpu(
>   QEMU_PLUGIN_API
>   const void *qemu_plugin_request_time_control(void);
>   
> +/**
> + * typedef qemu_plugin_vcpu_mem_cb_t - time callback function
> + * Returns time in ns (starting from 0)
> + */
> +typedef int64_t (*qemu_plugin_time_cb_t) (void);
> +
> +/**
> + * qemu_plugin_register_time_cb() - register a time callback
> + *
> + * This can only be called once a plugin has successfully called
> + * qemu_plugin_request_time_control(). The callback will get called
> + * whenever qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) is called.
> + */
> +QEMU_PLUGIN_API
> +void qemu_plugin_register_time_cb(qemu_plugin_id_t id, const void *handle, qemu_plugin_time_cb_t cb);
> +
>   /**
>    * qemu_plugin_update_ns() - update system emulation time
>    * @handle: opaque handle returned by qemu_plugin_request_time_control()
> @@ -723,6 +739,8 @@ const void *qemu_plugin_request_time_control(void);
>    * user-mode emulation the time is not changed by this as all reported
>    * time comes from the host kernel.
>    *
> + * This allows QEMU to execute any pending timers.
> + *
>    * Start time is 0.
>    */
>   QEMU_PLUGIN_API
> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> index 1432d1c5b1..5ed748f5cc 100644
> --- a/accel/tcg/tcg-accel-ops.c
> +++ b/accel/tcg/tcg-accel-ops.c
> @@ -33,6 +33,7 @@
>   #include "qemu/main-loop.h"
>   #include "qemu/guest-random.h"
>   #include "qemu/timer.h"
> +#include "qemu/plugin.h"
>   #include "exec/cputlb.h"
>   #include "exec/hwaddr.h"
>   #include "exec/tb-flush.h"
> @@ -199,6 +200,10 @@ static inline void tcg_remove_all_breakpoints(CPUState *cpu)
>   
>   static int64_t tcg_get_virtual_clock(void)
>   {
> +    int64_t from_plugin;
> +    if (qemu_plugin_maybe_fetch_time(&from_plugin)) {
> +        return from_plugin;
> +    }
>       return cpu_get_clock();
>   }
>   
> diff --git a/plugins/api-system.c b/plugins/api-system.c
> index cc190b167e..0f2a3eb5a6 100644
> --- a/plugins/api-system.c
> +++ b/plugins/api-system.c
> @@ -17,6 +17,7 @@
>   #include "hw/boards.h"
>   #include "qemu/plugin-memory.h"
>   #include "qemu/plugin.h"
> +#include "plugin.h"
>   
>   /*
>    * In system mode we cannot trace the binary being executed so the
> @@ -129,3 +130,10 @@ void qemu_plugin_update_ns(const void *handle, int64_t new_time)
>                            RUN_ON_CPU_HOST_ULONG(new_time));
>       }
>   }
> +
> +void qemu_plugin_register_time_cb(qemu_plugin_id_t id, const void *handle, qemu_plugin_time_cb_t cb)
> +{
> +    if (handle == &has_control) {
> +        plugin_register_cb(id, QEMU_PLUGIN_EV_GET_TIME, cb);
> +    }
> +}
> diff --git a/plugins/core.c b/plugins/core.c
> index eb9281fe54..d56b4c9d48 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -571,6 +571,28 @@ void qemu_plugin_flush_cb(void)
>       plugin_cb__simple(QEMU_PLUGIN_EV_FLUSH);
>   }
>   
> +/*
> + * Disable CFI checks.
> + * The callback function has been loaded from an external library so we do not
> + * have type information
> + */
> +QEMU_DISABLE_CFI
> +bool qemu_plugin_maybe_fetch_time(int64_t *tptr)
> +{
> +    enum qemu_plugin_event ev = QEMU_PLUGIN_EV_GET_TIME;
> +
> +    /* there should only be one callback */
> +    if (!QLIST_EMPTY(&plugin.cb_lists[ev])) {
> +        struct qemu_plugin_cb *cb = QLIST_FIRST(&plugin.cb_lists[ev]);
> +        qemu_plugin_time_cb_t func = cb->f.generic;
> +        *tptr = func();
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +
>   void exec_inline_op(enum plugin_dyn_cb_type type,
>                       struct qemu_plugin_inline_cb *cb,
>                       int cpu_index)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 1/2] accel/tcg: add get_virtual_clock for TCG

[Mark Burton]

In principle I like this, but 
1/ throughout the API can we please make everything consistent sure that all registrations take a handle (void *) and all callbacks functions pass that handle (and the ID)
 - right now, some things do, some things dont, and this specific case seems to take a handle on registration, but does not provide it on callback (!)

(This is the current implementation :
typedef int64_t (*qemu_plugin_time_cb_t) (void);
...
QEMU_PLUGIN_API void qemu_plugin_register_time_cb(qemu_plugin_id_t id, const void *handle, qemu_plugin_time_cb_t cb);
)

2/ The current implementation makes use of the callback _ONLY_ in the case of single TCG — it’s most interesting when we have MTTCG enabled (and I see no reason not to provide the same mechanism for any other accelerator if/when anything in QEMU requests ’the time’.


Cheers
Mark.


> On 3 Apr 2025, at 13:38, Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> 
> Rather than allowing cpus_get_virtual_clock() to fall through to
> cpu_get_clock() introduce a TCG handler so it can make a decision
> about what time it is.
> 
> Initially this just calls cpu_get_clock() as before but this will
> change in later commits.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> accel/tcg/tcg-accel-ops.c | 6 ++++++
> 1 file changed, 6 insertions(+)
> 
> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> index d9b662efe3..1432d1c5b1 100644
> --- a/accel/tcg/tcg-accel-ops.c
> +++ b/accel/tcg/tcg-accel-ops.c
> @@ -197,6 +197,11 @@ static inline void tcg_remove_all_breakpoints(CPUState *cpu)
>     cpu_watchpoint_remove_all(cpu, BP_GDB);
> }
> 
> +static int64_t tcg_get_virtual_clock(void)
> +{
> +    return cpu_get_clock();
> +}
> +
> static void tcg_accel_ops_init(AccelOpsClass *ops)
> {
>     if (qemu_tcg_mttcg_enabled()) {
> @@ -212,6 +217,7 @@ static void tcg_accel_ops_init(AccelOpsClass *ops)
>             ops->get_virtual_clock = icount_get;
>             ops->get_elapsed_ticks = icount_get;
>         } else {
> +            ops->get_virtual_clock = tcg_get_virtual_clock;
>             ops->handle_interrupt = tcg_handle_interrupt;
>         }
>     }
> --
> 2.39.5
> 

Re: [PATCH 1/2] accel/tcg: add get_virtual_clock for TCG

[Alex Bennée]

Mark Burton <mburton@qti.qualcomm.com> writes:

> In principle I like this, but 
> 1/ throughout the API can we please make everything consistent sure that all registrations take a handle (void *) and all callbacks functions pass that handle (and the ID)
>  - right now, some things do, some things dont, and this specific case
> seems to take a handle on registration, but does not provide it on
> callback (!)

The handle is something the plugin should have already. The plugin id is
needed so the framework knows who to deliver the callback back to.

>
> (This is the current implementation :
> typedef int64_t (*qemu_plugin_time_cb_t) (void);
> ...
> QEMU_PLUGIN_API void qemu_plugin_register_time_cb(qemu_plugin_id_t id, const void *handle, qemu_plugin_time_cb_t cb);
> )
>
> 2/ The current implementation makes use of the callback _ONLY_ in the
> case of single TCG — it’s most interesting when we have MTTCG enabled

Ahh - as I said compile tested only ;-)

I can fix that for v2.


> (and I see no reason not to provide the same mechanism for any other
> accelerator if/when anything in QEMU requests ’the time’.

That would mean making a clear separation in plugins for things that are
"events" which we do do from other hypervisors and "instrumentation"
which can only be done under TCG.


> 
>
> Cheers
> Mark.
>
>
>> On 3 Apr 2025, at 13:38, Alex Bennée <alex.bennee@linaro.org> wrote:
>> 
>> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
>> 
>> Rather than allowing cpus_get_virtual_clock() to fall through to
>> cpu_get_clock() introduce a TCG handler so it can make a decision
>> about what time it is.
>> 
>> Initially this just calls cpu_get_clock() as before but this will
>> change in later commits.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> accel/tcg/tcg-accel-ops.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>> 
>> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
>> index d9b662efe3..1432d1c5b1 100644
>> --- a/accel/tcg/tcg-accel-ops.c
>> +++ b/accel/tcg/tcg-accel-ops.c
>> @@ -197,6 +197,11 @@ static inline void tcg_remove_all_breakpoints(CPUState *cpu)
>>     cpu_watchpoint_remove_all(cpu, BP_GDB);
>> }
>> 
>> +static int64_t tcg_get_virtual_clock(void)
>> +{
>> +    return cpu_get_clock();
>> +}
>> +
>> static void tcg_accel_ops_init(AccelOpsClass *ops)
>> {
>>     if (qemu_tcg_mttcg_enabled()) {
>> @@ -212,6 +217,7 @@ static void tcg_accel_ops_init(AccelOpsClass *ops)
>>             ops->get_virtual_clock = icount_get;
>>             ops->get_elapsed_ticks = icount_get;
>>         } else {
>> +            ops->get_virtual_clock = tcg_get_virtual_clock;
>>             ops->handle_interrupt = tcg_handle_interrupt;
>>         }
>>     }
>> --
>> 2.39.5
>> 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/2] accel/tcg: add get_virtual_clock for TCG

[Mark Burton]

> On 8 Apr 2025, at 10:49, Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> 
> Mark Burton <mburton@qti.qualcomm.com> writes:
> 
>> In principle I like this, but
>> 1/ throughout the API can we please make everything consistent sure that all registrations take a handle (void *) and all callbacks functions pass that handle (and the ID)
>> - right now, some things do, some things dont, and this specific case
>> seems to take a handle on registration, but does not provide it on
>> callback (!)
> 
> The handle is something the plugin should have already. The plugin id is
> needed so the framework knows who to deliver the callback back to.

The plugin gave QEMU a handle that it expects to be called with, such that it does not need to do any lookup. Without that handle we would have to assume a static global somewhere, which is not a nice design. Since we may handle several plugins, all be it in this case having multiple plugins registering a time handler would seem odd, none the less it’s much cleaner throughout the whole API if we have a single consistent approach? Having the handle (which you already require on the registration) is a much nicer patten, but it needs to be followed through?

> 
>> 
>> (This is the current implementation :
>> typedef int64_t (*qemu_plugin_time_cb_t) (void);
>> ...
>> QEMU_PLUGIN_API void qemu_plugin_register_time_cb(qemu_plugin_id_t id, const void *handle, qemu_plugin_time_cb_t cb);
>> )
>> 
>> 2/ The current implementation makes use of the callback _ONLY_ in the
>> case of single TCG — it’s most interesting when we have MTTCG enabled
> 
> Ahh - as I said compile tested only ;-)
> 
> I can fix that for v2.

:-)

> 
> 
>> (and I see no reason not to provide the same mechanism for any other
>> accelerator if/when anything in QEMU requests ’the time’.
> 
> That would mean making a clear separation in plugins for things that are
> "events" which we do do from other hypervisors and "instrumentation"
> which can only be done under TCG.
> 

All for clarity

Cheers
Mark.

> 
>> 
>> 
>> Cheers
>> Mark.
>> 
>> 
>>> On 3 Apr 2025, at 13:38, Alex Bennée <alex.bennee@linaro.org> wrote:
>>> 
>>> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
>>> 
>>> Rather than allowing cpus_get_virtual_clock() to fall through to
>>> cpu_get_clock() introduce a TCG handler so it can make a decision
>>> about what time it is.
>>> 
>>> Initially this just calls cpu_get_clock() as before but this will
>>> change in later commits.
>>> 
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>> accel/tcg/tcg-accel-ops.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>> 
>>> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
>>> index d9b662efe3..1432d1c5b1 100644
>>> --- a/accel/tcg/tcg-accel-ops.c
>>> +++ b/accel/tcg/tcg-accel-ops.c
>>> @@ -197,6 +197,11 @@ static inline void tcg_remove_all_breakpoints(CPUState *cpu)
>>>    cpu_watchpoint_remove_all(cpu, BP_GDB);
>>> }
>>> 
>>> +static int64_t tcg_get_virtual_clock(void)
>>> +{
>>> +    return cpu_get_clock();
>>> +}
>>> +
>>> static void tcg_accel_ops_init(AccelOpsClass *ops)
>>> {
>>>    if (qemu_tcg_mttcg_enabled()) {
>>> @@ -212,6 +217,7 @@ static void tcg_accel_ops_init(AccelOpsClass *ops)
>>>            ops->get_virtual_clock = icount_get;
>>>            ops->get_elapsed_ticks = icount_get;
>>>        } else {
>>> +            ops->get_virtual_clock = tcg_get_virtual_clock;
>>>            ops->handle_interrupt = tcg_handle_interrupt;
>>>        }
>>>    }
>>> --
>>> 2.39.5
>>> 
> 
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro