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

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
> 

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

> 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

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>