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 18 16:54, Alex Bennée wrote:
>>
>> Aaron Lindsay OS <aaron@os.amperecomputing.com> 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?

No it is a monotonically increasing cpu_index for each new CPUState
created. So the first thread is 1 and the second is 2 no matter what the
thread id is.

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

We don't actually use this prototype anymore. I had removed the concept
of vcpu_index from the translation time hooks (so people don't get any
ideas about it's significance there). However we do use vcpu_index with
the udata form.

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

So we do use it in the example plugins (hotpages tracks which vCPUs have
written to which pages). I think it is useful information for a plugin
but I think if you want per-vCPU structures in your plugin __thread is
going to be the easiest solution.

--
Alex Bennée