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

[Aaron Lindsay OS]

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 <address@hidden>
> 
> ---
> 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.

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

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

-Aaron