> On 8 Apr 2025, at 10:49, Alex Bennée <alex.ben...@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 <mbur...@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.ben...@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.ben...@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
