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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 1/9/25 06:04, 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.
> 
>>   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;
>> +}
>