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

Commit Message

Julian Ganz Dec. 2, 2024, 7:26 p.m. UTC

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

Comments

Pierrick Bouvier Dec. 4, 2024, 11:14 p.m. UTC | #1

On 12/2/24 11:26, Julian Ganz wrote:
> 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']
>   if host_os != 'windows'
>     # lockstep uses socket.h
>     contrib_plugins += 'lockstep'
> diff --git a/contrib/plugins/traps.c b/contrib/plugins/traps.c
> new file mode 100644
> index 0000000000..ecd4beac5f
> --- /dev/null
> +++ b/contrib/plugins/traps.c
> @@ -0,0 +1,96 @@
> +/*
> + * Copyright (C) 2024, Julian Ganz <[e-mail]>
> + *
> + * Traps - count traps
> + *
> + * License: GNU GPL, version 2 or later.
> + *   See the COPYING file in the top-level directory.
> + */
> +
> +#include <stdio.h>
> +
> +#include <qemu-plugin.h>
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
> +
> +typedef struct {
> +    uint64_t interrupts;
> +    uint64_t exceptions;
> +    uint64_t hostcalls;
> +    bool active;

The active field can be removed, as you can query 
qemu_plugin_num_vcpus() to know (dynamically) how many vcpus were created.

> +} TrapCounters;
> +
> +static struct qemu_plugin_scoreboard *traps;
> +static size_t max_vcpus;
> +
> +static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
> +{
> +    TrapCounters* rec = qemu_plugin_scoreboard_find(traps, vcpu_index);
> +    rec->active = true;
> +}

Once active is removed, this function can be removed too.

> +
> +static void vcpu_discon(qemu_plugin_id_t id, unsigned int vcpu_index,
> +                        enum qemu_plugin_discon_type type, uint64_t from_pc,
> +                        uint64_t to_pc)
> +{
> +    TrapCounters* rec = qemu_plugin_scoreboard_find(traps, vcpu_index);
> +    switch (type) {
> +    case QEMU_PLUGIN_DISCON_INTERRUPT:
> +        rec->interrupts++;
> +        break;
> +    case QEMU_PLUGIN_DISCON_EXCEPTION:
> +        rec->exceptions++;
> +        break;
> +    case QEMU_PLUGIN_DISCON_HOSTCALL:
> +        rec->hostcalls++;
> +        break;
> +    default:
> +        /* unreachable */

You can use g_assert_not_reached() ensure it's unreachable. We removed 
assert(0) from the codebase and replaced those with it.

> +        break;
> +    }
> +}
> +
> +static void plugin_exit(qemu_plugin_id_t id, void *p)
> +{
> +    g_autoptr(GString) report;
> +    report = g_string_new("VCPU, interrupts, exceptions, hostcalls\n");
> +    int vcpu;
> +
> +    for (vcpu = 0; vcpu < max_vcpus; vcpu++) {

vcpu < qemu_plugin_num_vcpus()

> +        TrapCounters *rec = qemu_plugin_scoreboard_find(traps, vcpu);
> +        if (rec->active) {
> +            g_string_append_printf(report,
> +                                   "% 4d, % 10"PRId64", % 10"PRId64", % 10"
> +                                   PRId64"\n",
> +                                   vcpu,
> +                                   rec->interrupts, rec->exceptions,
> +                                   rec->hostcalls);
> +        }
> +    }
> +
> +    qemu_plugin_outs(report->str);
> +    qemu_plugin_scoreboard_free(traps);
> +}
> +
> +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);
> +
> +    qemu_plugin_register_vcpu_discon_cb(id, QEMU_PLUGIN_DISCON_TRAPS,
> +                                        vcpu_discon);
> +

The change from QEMU_PLUGIN_DISCON_TRAPS to QEMU_PLUGIN_DISCON_ALL 
should be included in this patch, instead of next one.

> +    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
> +
> +    return 0;
> +}

Julian Ganz Dec. 5, 2024, 1 p.m. UTC | #2

Hi Pierrick,

December 5, 2024 at 12:14 AM, "Pierrick Bouvier" wrote:
> On 12/2/24 11:26, Julian Ganz wrote:
> >  +typedef struct {
> >  + uint64_t interrupts;
> >  + uint64_t exceptions;
> >  + uint64_t hostcalls;
> >  + bool active;
> > 
> The active field can be removed, as you can query qemu_plugin_num_vcpus() to know (dynamically) how many vcpus were created.

Yes, if the ids of dynamically initialized VCPUs are contiguous. I
wasn't sure they really are. And I distinctly remember we originally
used some query function and ended up with the maximum number of VCPUs
supported rather then those actually used. But that may have been
another function, or some unfortunate result of me being too cautious
and doing

| qemu_plugin_vcpu_for_each(id, vcpu_init);

in qemu_plugin_install.

> > 
> > + break;
> >  + }
> >  +}
> >  +
> >  +static void plugin_exit(qemu_plugin_id_t id, void *p)
> >  +{
> >  + g_autoptr(GString) report;
> >  + report = g_string_new("VCPU, interrupts, exceptions, hostcalls\n");
> >  + int vcpu;
> >  +
> >  + for (vcpu = 0; vcpu < max_vcpus; vcpu++) {
> > 
> vcpu < qemu_plugin_num_vcpus()

Yes, max_vcpus was introduced as an optimization. If we can rely on all
VCPUs with id < qemu_plugin_num_vcpus() having been active at some point
this becomes unnecessary.

> >  + qemu_plugin_register_vcpu_discon_cb(id, QEMU_PLUGIN_DISCON_TRAPS,
> >  + vcpu_discon);
> >  +
> > 
> The change from QEMU_PLUGIN_DISCON_TRAPS to QEMU_PLUGIN_DISCON_ALL should be included in this patch, instead of next one.

Ah, thanks for pointing that out. I likely fumbled this at some point when rebasing.

Regards,
Julian Ganz

Pierrick Bouvier Dec. 5, 2024, 5:23 p.m. UTC | #3

On 12/5/24 05:00, Julian Ganz wrote:
> Hi Pierrick,
> 
> December 5, 2024 at 12:14 AM, "Pierrick Bouvier" wrote:
>> On 12/2/24 11:26, Julian Ganz wrote:
>>>   +typedef struct {
>>>   + uint64_t interrupts;
>>>   + uint64_t exceptions;
>>>   + uint64_t hostcalls;
>>>   + bool active;
>>>
>> The active field can be removed, as you can query qemu_plugin_num_vcpus() to know (dynamically) how many vcpus were created.
> 
> Yes, if the ids of dynamically initialized VCPUs are contiguous. I
> wasn't sure they really are. And I distinctly remember we originally
> used some query function and ended up with the maximum number of VCPUs
> supported rather then those actually used. But that may have been
> another function, or some unfortunate result of me being too cautious
> and doing
> 

It's a valid concern, and the good news is that they are guaranteed to 
be contiguous.
In system mode, we initialize all vcpus *before* executing anything.
In user mode, they are spawned when new threads appear.

In reality, qemu_plugin_num_vcpus() returns the maximum number of vcpus 
we had at some point (vs the real number of vcpus running).
It was introduced with scoreboards, to ensure the user can know how many 
entries they contain, in a safe way.

Most of the usage we had for this was to allocate structures and collect 
information per vcpu, especially at the end of execution (where some 
vcpus will have exited already in user-mode). So choosing to return the 
max is a valid abstraction.

> | qemu_plugin_vcpu_for_each(id, vcpu_init);
> 
> in qemu_plugin_install.
> 

And indeed, qemu_plugin_vcpu_for_each(id, vcpu_init) will only iterate 
on active cpus.

>>>
>>> + break;
>>>   + }
>>>   +}
>>>   +
>>>   +static void plugin_exit(qemu_plugin_id_t id, void *p)
>>>   +{
>>>   + g_autoptr(GString) report;
>>>   + report = g_string_new("VCPU, interrupts, exceptions, hostcalls\n");
>>>   + int vcpu;
>>>   +
>>>   + for (vcpu = 0; vcpu < max_vcpus; vcpu++) {
>>>
>> vcpu < qemu_plugin_num_vcpus()
> 
> Yes, max_vcpus was introduced as an optimization. If we can rely on all
> VCPUs with id < qemu_plugin_num_vcpus() having been active at some point
> this becomes unnecessary.
> 
>>>   + qemu_plugin_register_vcpu_discon_cb(id, QEMU_PLUGIN_DISCON_TRAPS,
>>>   + vcpu_discon);
>>>   +
>>>
>> The change from QEMU_PLUGIN_DISCON_TRAPS to QEMU_PLUGIN_DISCON_ALL should be included in this patch, instead of next one.
> 
> Ah, thanks for pointing that out. I likely fumbled this at some point when rebasing.
> 
> Regards,
> Julian Ganz

Alex Bennée Jan. 9, 2025, 2:04 p.m. UTC | #4

Julian Ganz <[e-mail]> 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.

>  if host_os != 'windows'
>    # lockstep uses socket.h
>    contrib_plugins += 'lockstep'
<snip>
> +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.

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?

> +
> +    qemu_plugin_register_vcpu_discon_cb(id, QEMU_PLUGIN_DISCON_TRAPS,
> +                                        vcpu_discon);
> +
> +    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
> +
> +    return 0;
> +}

Pierrick Bouvier Jan. 9, 2025, 10:10 p.m. UTC | #5

On 1/9/25 06:04, Alex Bennée wrote:
> Julian Ganz <[e-mail]> 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.
> 
>>   if host_os != 'windows'
>>     # lockstep uses socket.h
>>     contrib_plugins += 'lockstep'
> <snip>
>> +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.
> 
> 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?
> 

I'm not sure to see how we can load a plugin after the cpus have been 
initialized?
In case someone wants to dynamically create a new vcpu_init callback 
(after some event or translation for instance), it should be the 
responsibility of plugin to call it for existing vcpus.

qemu_plugin_vcpu_for_each is still useful in case you want to only 
iterate currently active cpus (and not the one who exited already in 
user mode).

>> +
>> +    qemu_plugin_register_vcpu_discon_cb(id, QEMU_PLUGIN_DISCON_TRAPS,
>> +                                        vcpu_discon);
>> +
>> +    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
>> +
>> +    return 0;
>> +}
>

Julian Ganz Jan. 10, 2025, 11:49 a.m. UTC | #6

Hi Alex,

January 9, 2025 at 3:04 PM, "Alex Bennée" wrote:
> Julian Ganz <[e-mail]> 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

Alex Bennée Jan. 10, 2025, 3:15 p.m. UTC | #7

"Julian Ganz" <[e-mail]> writes:

> Hi Alex,
>
> January 9, 2025 at 3:04 PM, "Alex Bennée" wrote:
>> Julian Ganz <[e-mail]> 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

Pierrick Bouvier Jan. 10, 2025, 9:02 p.m. UTC | #8

On 1/10/25 07:15, Alex Bennée wrote:
> "Julian Ganz" <[e-mail]> writes:
> 
>> Hi Alex,
>>
>> January 9, 2025 at 3:04 PM, "Alex Bennée" wrote:
>>> Julian Ganz <[e-mail]> 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.
> 

I agree, as it was discussed in previous versions, we should add a 
functional test for this. I'm not sure if we should write a custom and 
complicated test, or simply boot and shutdown an existing image, and 
call it a day.

Do you have any opinion on this Alex?

>>
>>>> +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 Jan. 11, 2025, 12:15 p.m. UTC | #9

Pierrick Bouvier <[e-mail]> writes:

> On 1/10/25 07:15, Alex Bennée wrote:
>> "Julian Ganz" <[e-mail]> writes:
>> 
>>> Hi Alex,
>>>
>>> January 9, 2025 at 3:04 PM, "Alex Bennée" wrote:
>>>> Julian Ganz <[e-mail]> 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.
>> 
>
> I agree, as it was discussed in previous versions, we should add a
> functional test for this. I'm not sure if we should write a custom and
> complicated test, or simply boot and shutdown an existing image, and
> call it a day.
>
> Do you have any opinion on this Alex?

An existing image based test would be fine although I'd favour one that
had multiple exception types (e.g. something with firmware and
hypervisor transitions on Arm or equivalent on other arches.)

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

Patch

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']
 if host_os != 'windows'
   # lockstep uses socket.h
   contrib_plugins += 'lockstep'
diff --git a/contrib/plugins/traps.c b/contrib/plugins/traps.c
new file mode 100644
index 0000000000..ecd4beac5f
--- /dev/null
+++ b/contrib/plugins/traps.c
@@ -0,0 +1,96 @@ 
+/*
+ * Copyright (C) 2024, Julian Ganz <[e-mail]>
+ *
+ * Traps - count traps
+ *
+ * License: GNU GPL, version 2 or later.
+ *   See the COPYING file in the top-level directory.
+ */
+
+#include <stdio.h>
+
+#include <qemu-plugin.h>
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+typedef struct {
+    uint64_t interrupts;
+    uint64_t exceptions;
+    uint64_t hostcalls;
+    bool active;
+} TrapCounters;
+
+static struct qemu_plugin_scoreboard *traps;
+static size_t max_vcpus;
+
+static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
+{
+    TrapCounters* rec = qemu_plugin_scoreboard_find(traps, vcpu_index);
+    rec->active = true;
+}
+
+static void vcpu_discon(qemu_plugin_id_t id, unsigned int vcpu_index,
+                        enum qemu_plugin_discon_type type, uint64_t from_pc,
+                        uint64_t to_pc)
+{
+    TrapCounters* rec = qemu_plugin_scoreboard_find(traps, vcpu_index);
+    switch (type) {
+    case QEMU_PLUGIN_DISCON_INTERRUPT:
+        rec->interrupts++;
+        break;
+    case QEMU_PLUGIN_DISCON_EXCEPTION:
+        rec->exceptions++;
+        break;
+    case QEMU_PLUGIN_DISCON_HOSTCALL:
+        rec->hostcalls++;
+        break;
+    default:
+        /* unreachable */
+        break;
+    }
+}
+
+static void plugin_exit(qemu_plugin_id_t id, void *p)
+{
+    g_autoptr(GString) report;
+    report = g_string_new("VCPU, interrupts, exceptions, hostcalls\n");
+    int vcpu;
+
+    for (vcpu = 0; vcpu < max_vcpus; vcpu++) {
+        TrapCounters *rec = qemu_plugin_scoreboard_find(traps, vcpu);
+        if (rec->active) {
+            g_string_append_printf(report,
+                                   "% 4d, % 10"PRId64", % 10"PRId64", % 10"
+                                   PRId64"\n",
+                                   vcpu,
+                                   rec->interrupts, rec->exceptions,
+                                   rec->hostcalls);
+        }
+    }
+
+    qemu_plugin_outs(report->str);
+    qemu_plugin_scoreboard_free(traps);
+}
+
+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);
+
+    qemu_plugin_register_vcpu_discon_cb(id, QEMU_PLUGIN_DISCON_TRAPS,
+                                        vcpu_discon);
+
+    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
+
+    return 0;
+}