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

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

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