| Message ID | 20250403113851.4182190-2-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | add explicit virtual time callback for plugins | expand |
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>
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 >>
> 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
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; } }
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(+)