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