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

[Aaron Lindsay OS]

On Oct 18 16:54, Alex Bennée wrote:
> 
> Aaron Lindsay OS <address@hidden> writes:
> 
> > On Oct 17 14:15, Alex Bennée wrote:
> >> +    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.

Fair enough. My fear was that any other operating modes might require
different plugin behavior, but it sounds like you think that unlikely.
If we're attempting to keep the implementation details hidden, should we
name this variable in terms of what it means for plugin implementations
instead of what it means for QEMU? (Not sure this is a winner, but maybe
something like "hardware_threading_model" )

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

Hmm, right. To make sure I fully understand, does this mean that for
user-mode, `vcpu_index` in the callback function pointer type below is
actually the thread index?

typedef void (*qemu_plugin_vcpu_simple_cb_t)(qemu_plugin_id_t id,
                                             unsigned int vcpu_index);

If so, do we have some max number of threads we support? I suppose we
could set max_vcpux to that number, and smp_cpus to 1, though I'm not
sure if that would be helpful or not.

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

I'm not sure. I liked the idea of exposing the vCPUs because it
theoretically allows you to allocate per-cpu things up front, which can
be helpful... but maybe forcing users to deal with dynamically
allocating everything will make for more resilient plugins anyway?

-Aaron