Re: [RFC PATCH v3 04/11] contrib/plugins: add plugin showcasing new dicontinuity related API

[Julian Ganz]

Hi Alex,

January 9, 2025 at 3:04 PM, "Alex Bennée" wrote:
> Julian Ganz <neither@nut.email> writes:
> > We recently introduced new plugin API for registration of discontinuity
> >  related callbacks. This change introduces a minimal plugin showcasing
> >  the new API. It simply counts the occurances of interrupts, exceptions
> >  and host calls per CPU and reports the counts when exitting.
> >  ---
> >  contrib/plugins/meson.build | 3 +-
> >  contrib/plugins/traps.c | 96 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 98 insertions(+), 1 deletion(-)
> >  create mode 100644 contrib/plugins/traps.c
> > 
> >  diff --git a/contrib/plugins/meson.build b/contrib/plugins/meson.build
> >  index 63a32c2b4f..9a3015e1c1 100644
> >  --- a/contrib/plugins/meson.build
> >  +++ b/contrib/plugins/meson.build
> >  @@ -1,5 +1,6 @@
> >  contrib_plugins = ['bbv', 'cache', 'cflow', 'drcov', 'execlog', 
> > 'hotblocks',
> >  - 'hotpages', 'howvec', 'hwprofile', 'ips', 'stoptrigger']
> >  + 'hotpages', 'howvec', 'hwprofile', 'ips', 'stoptrigger',
> >  + 'traps']
> > 
> I wonder if this is better in tests/tcg/plugins? We need to do something
> to ensure it gets covered by CI although we might want to be smarter
> about running it together with a test binary that will actually pick up
> something.

The callback is intended as an example. The patch-series does contain a
dedicated testing plugin. And iirc the contrib plugins are now built
with the rest of qemu anyway?

> > +QEMU_PLUGIN_EXPORT
> >  +int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
> >  + int argc, char **argv)
> >  +{
> >  + if (!info->system_emulation) {
> >  + fputs("trap plugin can only be used in system emulation mode.\n",
> >  + stderr);
> >  + return -1;
> >  + }
> >  +
> >  + max_vcpus = info->system.max_vcpus;
> >  + traps = qemu_plugin_scoreboard_new(sizeof(TrapCounters));
> >  + qemu_plugin_register_vcpu_init_cb(id, vcpu_init);
> >  + qemu_plugin_vcpu_for_each(id, vcpu_init);
> > 
> Hmm at first glances this seems redundant - however I guess this is
> covering the use case you load the plugin after the system is up and
> running.

Yep, but really that was just me being paranoid.

> I wonder if you have unearthed a foot-gun in the API that is easy to
> fall into? Maybe we should expand qemu_plugin_register_vcpu_init_cb to
> call the call back immediately for existing vcpus?

Would probably not hurt.

Regards,
Julian