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

[Alex Bennée]

"Julian Ganz" <neither@nut.email> writes:

> 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?

They do - however we generate additional tests with tests/tcg/plugins
with the existing multiarch linux-user and softmmu check-tcg tests. Its
a fairly dumb expansion though:

  # We need to ensure expand the run-plugin-TEST-with-PLUGIN
  # pre-requistes manually here as we can't use stems to handle it. We
  # only expand MULTIARCH_TESTS which are common on most of our targets
  # to avoid an exponential explosion as new tests are added. We also
  # add some special helpers the run-plugin- rules can use below.
  # In more, extra tests can be added using ADDITIONAL_PLUGINS_TESTS variable.

  ifneq ($(MULTIARCH_TESTS),)
  $(foreach p,$(PLUGINS), \
          $(foreach t,$(MULTIARCH_TESTS) $(ADDITIONAL_PLUGINS_TESTS),\
                  $(eval run-plugin-$(t)-with-$(p): $t $p) \
                  $(eval RUN_TESTS+=run-plugin-$(t)-with-$(p))))
  endif # MULTIARCH_TESTS
  endif # CONFIG_PLUGIN

We also have a hand-hacked test for validating memory instrumentation:

  # Test plugin memory access instrumentation
  run-plugin-test-plugin-mem-access-with-libmem.so: \
          PLUGIN_ARGS=$(COMMA)print-accesses=true
  run-plugin-test-plugin-mem-access-with-libmem.so: \
          CHECK_PLUGIN_OUTPUT_COMMAND= \
          $(SRC_PATH)/tests/tcg/multiarch/check-plugin-output.sh \
          $(QEMU) $<

  test-plugin-mem-access: CFLAGS+=-pthread -O0
  test-plugin-mem-access: LDFLAGS+=-pthread -O0

That said as I mention in the reply to the cover letter the traps stuff
might be better exercised with the functional test so could utilise a
plugin built in contrib just as easily.

>
>> > +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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro