Re: [PATCH v6 37/54] plugin: expand the plugin_init function to include an info block

["Alex Bennée" <alex.bennee@linaro.org>]

Aaron Lindsay OS <aaron@os.amperecomputing.com> writes:

> On Oct 17 14:15, Alex Bennée wrote:
>> This provides a limited amount of info to plugins about the guest
>> system that will allow them to make some additional decisions on
>> setup.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v6
>>   - split and move to pre example plugins
>>   - checkpatch fixes
>> ---
>>  include/qemu/qemu-plugin.h | 26 ++++++++++++++++++++++++--
>>  plugins/loader.c           | 23 +++++++++++++++++++----
>>  2 files changed, 43 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index c213d1dd19..784f1dfc3d 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>> @@ -38,9 +38,27 @@
>>
>>  typedef uint64_t qemu_plugin_id_t;
>>
>> +typedef struct {
>> +    /* string describing architecture */
>
> Might be worth noting that this is set to the value of TARGET_NAME qemu
> was built with, and pointing to documentation about the possible values
> it may hold.

OK.

>> +    const char *target_name;
>> +    /* is this a full system emulation? */
>> +    bool system_emulation;
>
> It seems that 'system_emulation' is meant primarily in opposition to
> user-mode. I'm wondering if this could/should this be an enum of the
> execution mode being used to allow for future expansion? Or, if your
> intention here is mostly to allow the user to detect when the *_vcpus
> variables are valid, could it be renamed or commented differently to
> make that link more clear?

The only other operating mode that's ever been mooted is softmmu-user
(and no implementation has been done so far). Even then I don't think
that is a distinction that should be reported to the plugin as we are
trying not to leak implementation details.

But yes the practical upshot is for system emulation you at least have
sort of bounded size for how many threads you may have running.

>
>> +    union {
>> +        /*
>> +         * smp_vcpus may change if vCPUs can be hot-plugged, max_vcpus
>> +         * is the system-wide limit.
>> +         */
>> +        struct {
>> +            int smp_vcpus;
>> +            int max_vcpus;
>> +        } system;
>> +    };
>> +} qemu_info_t;
>
> [...]
>
>> @@ -241,11 +245,22 @@ static void plugin_desc_free(struct qemu_plugin_desc *desc)
>>  int qemu_plugin_load_list(QemuPluginList *head)
>>  {
>>      struct qemu_plugin_desc *desc, *next;
>> +    g_autofree qemu_info_t *info = g_new0(qemu_info_t, 1);
>> +
>> +    info->target_name = TARGET_NAME;
>> +#ifndef CONFIG_USER_ONLY
>> +    MachineState *ms = MACHINE(qdev_get_machine());
>> +    info->system_emulation = true;
>> +    info->system.smp_vcpus = ms->smp.cpus;
>> +    info->system.max_vcpus = ms->smp.max_cpus;
>> +#else
>> +    info->system_emulation = false;
>
> Thinking "out loud" here - I wonder if it would be helpful to set the
> *_vcpus variables even for user mode here. It might allow unconditional
> allocation of "per-cpu" structures that the plugin might need - without
> first needing to check whether the *_vcpus variables were valid.

but what too? It would certainly be wrong because any user-space process
could create (and destroy) thousands of threads.

We could consider just asking plugins to deal with threads with their
own __thread variables but in that case we'd need to expose some sort of
thread exit/cleanup method so they can collect data from threads and
safely place it somewhere else - but I suspect that is a hairy
programming model to impose on plugins.

So far all the example plugins have just used locks to serialise things
and it's not been too bad. I guess we could do with an example that
tries to use this information to get an idea of how grungy the interface
is. Perhaps exposing the vCPUs at this point is pointless and we should
just stick to TARGET_NAME for now?

>
> -Aaron


--
Alex Bennée