| Message ID | 20191017131615.19660-38-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support for TCG plugins | expand |
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. > + 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
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
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? 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
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
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 */ + const char *target_name; + /* is this a full system emulation? */ + bool system_emulation; + 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; + /** * qemu_plugin_install() - Install a plugin * @id: this plugin's opaque ID + * @info: a block describing some details about the guest * @argc: number of arguments * @argv: array of arguments (@argc elements) * @@ -49,10 +67,14 @@ typedef uint64_t qemu_plugin_id_t; * Note: Calling qemu_plugin_uninstall() from this function is a bug. To raise * an error during install, return !0. * + * Note: @info is only live during the call. Copy any information we + * want to keep. + * * Note: @argv remains valid throughout the lifetime of the loaded plugin. */ -QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, int argc, - char **argv); +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, + const qemu_info_t *info, + int argc, char **argv); /* * Prototypes for the various callback styles we will be registering diff --git a/plugins/loader.c b/plugins/loader.c index eaedff577c..ce724ed583 100644 --- a/plugins/loader.c +++ b/plugins/loader.c @@ -28,6 +28,10 @@ #include "hw/core/cpu.h" #include "cpu.h" #include "exec/exec-all.h" +#ifndef CONFIG_USER_ONLY +#include "hw/boards.h" +#endif + #include "plugin.h" /* @@ -58,7 +62,7 @@ QemuOptsList qemu_plugin_opts = { }, }; -typedef int (*qemu_plugin_install_func_t)(qemu_plugin_id_t, int, char **); +typedef int (*qemu_plugin_install_func_t)(qemu_plugin_id_t, const qemu_info_t *, int, char **); extern struct qemu_plugin_state plugin; @@ -145,7 +149,7 @@ static uint64_t xorshift64star(uint64_t x) return x * UINT64_C(2685821657736338717); } -static int plugin_load(struct qemu_plugin_desc *desc) +static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info) { qemu_plugin_install_func_t install; struct qemu_plugin_ctx *ctx; @@ -193,7 +197,7 @@ static int plugin_load(struct qemu_plugin_desc *desc) } QTAILQ_INSERT_TAIL(&plugin.ctxs, ctx, entry); ctx->installing = true; - rc = install(ctx->id, desc->argc, desc->argv); + rc = install(ctx->id, info, desc->argc, desc->argv); ctx->installing = false; if (rc) { error_report("%s: qemu_plugin_install returned error code %d", @@ -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; +#endif QTAILQ_FOREACH_SAFE(desc, head, entry, next) { int err; - err = plugin_load(desc); + err = plugin_load(desc, info); if (err) { return err; }
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(-) -- 2.20.1