* [RFC 0/4] Support interactions between TCG plugins
@ 2022-09-01 18:27 Andrew Fasano
2022-09-01 18:27 ` [RFC 1/4] docs/tcg-plugins: describe QPP API Andrew Fasano
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Andrew Fasano @ 2022-09-01 18:27 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.bennee, erdnaxe, ma.mandourr, Andrew Fasano
Hello,
I'm requesting comments on the following series of patches expanding the
TCG plugin system to add the "QEMU Plugin-to-Plugin (QPP)" interface
that allows for interactions between TCG plugins. The goal of this
interface is to enable plugins to expand on other plugins and reduce
code duplication. This patch series includes documentation and
significant comments, but a high-level summary is below along with a
discussion of the current implementation as well as the benefits and
drawbacks of these changes.
**Summary**
The QPP interface allows two types of interactions between plugins:
1) Exported functions: A plugin may wish to allow other plugins to call
one of the functions it has defined. To do this, the plugin must mark
the function definition as publicly visible with the QEMU_PLUGIN_EXPORT
macro and place a definition in an included header file using the
QPP_FUN_PROTOTYPE macro. Other plugins can then include this header and
call the exported function by combining the name of the target plugin
with the name of the exported function.
For example, consider a hypothetical plugin that collects a list of
cache misses. This plugin could export two functions using the QPP
interface: one to allow another plugin to query this list and another
to empty the list. This would enable the development of another plugin
that examines guest CPU state to identify process changes and reports
the cache misses per process. With the QPP interface, this second plugin
would not need to duplicate any logic from the first.
2) Callbacks: Multiple plugins may wish to take some action when some
event of interest occurs inside a running guest. To support modularity
and reduce code duplication, the QPP callback system allows this logic
to be contained in single plugin that detects whenever a given event
occurs and exposes a callback with a given name. Another plugin can then
request to have one of its own functions run whenever this event occurs.
Additional plugins could also use this same callback to run additional
logic whenever this event occurs.
For example, consider (again) a hypothetical plugin that detects when
the current guest process changes by analyzing the guest CPU state. This
plugin could define a callback named "on_process_change" and trigger
this callback event whenever it detects a process change. Other plugins
could then be developed that take various actions on process changes by
registering internal functions to run on this event.
These patches and examples are inspired by the PANDA project
(https://panda.re and https://github.com/panda-re/panda), a fork of QEMU
modified to support dynamic program analysis and reverse engineering.
PANDA also includes a large plugin system with a similar interface for
interactions between plugins. I'm one of the maintainers of PANDA
and have seen how the ability for plugins to interact with
other plugins reduces code duplication and enables the creation of many
useful plugins.
**Implementation Overview**
These patches modify the TCG plugin build system to define the
preprocessor variable CURRENT_PLUGIN to the name of the current plugin
based off its filename. This can be useful for plugin developers in
general and is used internally in the QPP implementation to determine
if an exported plugin function is defined in the current plugin or
in another.
These patches also add the function qemu_plugin_name_to_handle to the
core plugin API which uses the new internal function is_plugin_named.
The ability for plugins to get a handle to another plugin is necessary
for the inter-plugin interactions described below.
The QPP implementation is contained inside a header file plugin-qpp.h
that adds the macros QPP_CREATE_CB, QPP_RUN_CB, QPP_REG_CB,
QPP_REMOVE_CB, and QPP_FUN_PROTOTYPE. The first 4 of these are related
to the callback system and the last one is for exported functions.
The QPP_CREATE_CB macro is used by a plugin that wishes to create a
callback with a given name. The macro will create an array of function
pointers for every function that has been registered to run on this
callback event. Other plugins can register a local function
with this callback (i.e., add it to this list of function pointers)
or remove a previously-registered function using QPP_REG_CB or
QPP_REMOVE_CB, respectively. When a plugin wishes to run all the
registered callback functions for a given callback, it uses the
QPP_RUN_CB macro.
The QPP_FUN_PROTOYPE macro enables a plugin to expose a function
it defines to other plugins. This macro is used in a header file that
is included by both the plugin that defines a function as well as
plugins that wish to use the function. This prototype creates a
constructor function that runs on plugin load. If the target plugin
name differs from the value of CURRENT_PLUGIN, this function
will use qemu_plugin_name_to_handle to get a handle to the target
plugin and use g_module_symbol to resolve the target function in
that plugin. If this fails, it will print a warning and abort.
Finally, these patches include an example pair of plugins, qpp_srv and
qpp_client. The former provides a trivial QPP callback which simply runs
when the plugin is unloaded as well as two functions to add and subtract
integers. The latter of these plugins registers a function to run
on this callback and uses the two exported functions.
**Request for Comments**
This is my first time proposing any patches to QEMU and these are some
non-trivial changes, so I wanted to ask for feedback as opposed to just
sending these as a patch. I hope that others agree that there's value
in supporting interactions between plugins (particularly inter-plugin
callbacks), but I know there are many ways this could be implemented
and I'm sure there are ways this could be improved.
My goal in this design was to keep the process of creating and using
callbacks and exported functions as easy as possible for plugin
developers. I'm open to redesigning this interface in
a completely different way if that would be necessary to get it merged.
Although the example plugins I've included for now are trivial, if this
interface is merged I'd be willing to develop some more plugins that
build atop it. One of these could be something like the "syscalls2"
plugin in PANDA which analyzes guest code and provides distinct
callbacks for when a guest runs a system call:
https://github.com/panda-re/panda/blob/dev/panda/plugins/syscalls2.
Another could be something like the "callstack_isntr" plugin in PANDA
which manages a shadow stack, provides callbacks on function calls and
returns and also provides exported functions to get the current
callstack:
https://github.com/panda-re/panda/tree/dev/panda/plugins/callstack_instr
I see a handful of potential issues that I'll highlight below:
1) Unstable APIs: these inter-plugin interactions do not specify API
versions. If the behavior of a callback or exported function
changes without the type signature changing, it may lead to subtle
errors in other (unchanged) plugins that were using these functions.
If the signature of a plugin's callback or exported function,
changes, build time errors would be raised and necessary
changes could be made to in-tree plugins. However, out-of-tree
plugins would break.
2) Checkpatch.pl objects to some of these changes. Most importantly,
it takes issue with QPP_FUN_PROTOTYPE introducing multiple
statements outside of a do while loop. I believe the logic in this
macro cannot be wrapped in such a way as it is defining variables
that must be available outside of the scope of the macro (in
particular, these variables must be used by other QPP macros later).
Given that this logic is contained in a plugin instead of the main
QEMU binary, I'm hoping there's some leeway with this error, but
if it needs to be changed, I could split the logic into 5 distinct
macros that a plugin developer would then run in order. Or perhaps
someone here can suggest a better design.
Additional, I believe the script is raising a number of false
positive errors for the indentation of the multi-line macros, e.g.,
"suspect code indent for conditional statements (4, 6)" for
QPP_RUN_CB. If I'm wrong about this, I'm happy to adjust the
indentation. If it is an issue with the script, I could open a bug
report if that would help.
3) Decentralized interactions. This approach allows plugins to directly
interact with other plugins. An alternative design could register
exported functions and callbacks with the core plugin logic in
the main QEMU object and have all the interactions go through there.
Would a centralized design where plugins send requests through
the core plugin logic in the QEMU binary be better that allowing
direct interactions between the shared objects built for the plugins?
Does it seem like an interface like this would be worth merging? If so,
I'd welcome any and all suggestions on how to improve these changes.
Thank you,
Andrew Fasano
Andrew Fasano (4):
docs/tcg-plugins: describe QPP API
tcg/plugins: Automatically define CURRENT_PLUGIN
tcg/plugins: Support for inter-plugin interactions
tcg/plugins: Add example pair of QPP plugins
contrib/plugins/Makefile | 4 +-
contrib/plugins/qpp_client.c | 42 ++++++
contrib/plugins/qpp_client.h | 1 +
contrib/plugins/qpp_srv.c | 33 +++++
contrib/plugins/qpp_srv.h | 17 +++
docs/devel/tcg-plugins.rst | 76 +++++++++++
include/qemu/plugin-qpp.h | 252 +++++++++++++++++++++++++++++++++++
plugins/core.c | 11 ++
plugins/loader.c | 24 ++++
plugins/plugin.h | 5 +
plugins/qemu-plugins.symbols | 1 +
11 files changed, 465 insertions(+), 1 deletion(-)
create mode 100644 contrib/plugins/qpp_client.c
create mode 100644 contrib/plugins/qpp_client.h
create mode 100644 contrib/plugins/qpp_srv.c
create mode 100644 contrib/plugins/qpp_srv.h
create mode 100644 include/qemu/plugin-qpp.h
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC 1/4] docs/tcg-plugins: describe QPP API
2022-09-01 18:27 [RFC 0/4] Support interactions between TCG plugins Andrew Fasano
@ 2022-09-01 18:27 ` Andrew Fasano
2022-09-21 13:57 ` Alex Bennée
2022-09-01 18:27 ` [RFC 2/4] tcg/plugins: Automatically define CURRENT_PLUGIN Andrew Fasano
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Andrew Fasano @ 2022-09-01 18:27 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.bennee, erdnaxe, ma.mandourr, Andrew Fasano
Describe how multiple TCG plugins can interact using the QEMU
Plugin-to-Plugin API (QPP) with both callbacks and direct
function calls.
Signed-off-by: Andrew Fasano <fasano@mit.edu>
---
docs/devel/tcg-plugins.rst | 76 ++++++++++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)
diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
index a7cc44aa20..7985572027 100644
--- a/docs/devel/tcg-plugins.rst
+++ b/docs/devel/tcg-plugins.rst
@@ -441,3 +441,79 @@ The plugin has a number of arguments, all of them are optional:
associativity of the L2 cache, respectively. Setting any of the L2
configuration arguments implies ``l2=on``.
(default: N = 2097152 (2MB), B = 64, A = 16)
+
+Plugin-to-Plugin Interactions
+-----------------------------
+
+Plugins may interact with other plugins through the QEMU Plugin-to-Plugin
+("QPP") API by including ``qemu/plugin-qpp.h``. This API supports direct
+function calls between plugins as well as an inter-plugin callback system.
+This API allows for composition of plugins: plugins can make use of logic in
+other plugins without the need for code duplication.
+
+Plugin names
+~~~~~~~~~~~~
+Plugins are automatically given a name by removing the suffix from their
+filename. These plugin names will be used during QPP interactions as
+described below. A plugin can access its own name through the preprocessor
+variable ``CURRENT_PLUGIN``.
+
+QPP function calls
+~~~~~~~~~~~~~~~~~~
+When a plugin (e.g. ``plugin_a``) wishes to make some of its functions (e.g.
+``func_1``) available to other plugins, it must:
+
+1. Mark the function definition with the ``QEMU_PLUGIN_EXPORT`` macro. For
+example : ``QEMU_PLUGIN_EXPORT int func_1(int x) {...}``.
+2. Provide prototypes for exported functions in a header file (e.g.
+``plugin_a.h``) using the macro ``QPP_FUN_PROTOTYPE`` with arguments of the
+plugin's name, the function's return type, the function's name, and any
+arguments the function takes. For example:
+``QPP_FUN_PROTOTYPE(plugin_a, int, func_1, int);``.
+3. Import this header from the plugin.
+
+When other plugins wish to use the functions exported by ``plugin_a``, they
+must:
+
+1. Import the header file (e.g. ``plugin_a.h``) with the function prototype(s).
+2. Call the function when desired by combining the target plugin name, an
+ underscore, and the target function name, e.g. ``plugin_a_func_1()``.
+
+QPP callbacks
+~~~~~~~~~~~~~
+
+The QPP API also allows a plugin to define callback events and for other plugins
+to request to be notified whenever these events happens. The plugin that defines
+the callback is responsible for triggering the callback when it so wishes. Other
+plugins that wish to be notified on these events must define a function of an
+appropriate type and register it to run on this event.
+
+When a plugin (e.g. ``plugin_a``) wishes to define a callback (an event that
+other plugins can request to be notified about), it must:
+
+1. Define the callback using the ``QPP_CREATE_CB`` macro with a single argument
+ of the callback's name. For example: ``QPP_CREATE_CB(on_some_event);``.
+2. In a header file (e.g. ``plugin_a.h``) create a prototype for the callback
+ type with ``QPP_CB_PROTOTYPE`` with arguments of the callback's return type
+ (only ``void`` is currently supported), the name of the callback, and any
+ arguments the callback function will be called with. For example with a
+ callback named ``on_some_event`` that returns a void and takes an int and
+ a bool as an argument, you would use: ``QPP_CB_PROTOTYPE(void,
+ on_some_event, int, bool)``.
+3. Import this header from the plugin.
+4. When the plugin wishes to run any registered callback functions, it should
+ use the macro ``QPP_RUN_CB`` with the first argument set to the callback
+ name followed by the arguments as specified in the header. For example:
+ ``QPP_RUN_CB(on_some_event, 2, true);``.
+
+When other plugins wish to register a function to run on such an event, they
+must:
+
+1. Import the header file with the callback prototype(s) (e.g. ``plugin_a.h``)
+2. Define a function that matches the callback signature. For example:
+ ``void plugin_b_callback(int, bool) {...}``.
+3. Register this function to be run on the callback using the ``QPP_REG_CB``
+ macro with the first argument being the name of the plugin that provides the
+ callback (as a string), the second being the callback name, and the third as
+ the function that should be run. For example: ``QPP_REG_CB("plugin_a",
+ on_some_event, plugin_b_callback);``
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC 2/4] tcg/plugins: Automatically define CURRENT_PLUGIN
2022-09-01 18:27 [RFC 0/4] Support interactions between TCG plugins Andrew Fasano
2022-09-01 18:27 ` [RFC 1/4] docs/tcg-plugins: describe QPP API Andrew Fasano
@ 2022-09-01 18:27 ` Andrew Fasano
2022-09-21 14:00 ` Alex Bennée
2022-09-01 18:27 ` [RFC 3/4] tcg/plugins: Support for inter-plugin interactions Andrew Fasano
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Andrew Fasano @ 2022-09-01 18:27 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.bennee, erdnaxe, ma.mandourr, Andrew Fasano
Use plugin filenames to set the preprocessor variable CURRENT_PLUGIN
as a string during plugin compilation.
Signed-off-by: Andrew Fasano <fasano@mit.edu>
---
contrib/plugins/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
index df3499f4f2..b7720fea0f 100644
--- a/contrib/plugins/Makefile
+++ b/contrib/plugins/Makefile
@@ -34,7 +34,7 @@ CFLAGS += -I$(SRC_PATH)/include/qemu
all: $(SONAMES)
%.o: %.c
- $(CC) $(CFLAGS) -c -o $@ $<
+ $(CC) $(CFLAGS) -DCURRENT_PLUGIN=\"$(basename $@)\" -c -o $@ $<
lib%.so: %.o
$(CC) -shared -Wl,-soname,$@ -o $@ $^ $(LDLIBS)
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC 3/4] tcg/plugins: Support for inter-plugin interactions
2022-09-01 18:27 [RFC 0/4] Support interactions between TCG plugins Andrew Fasano
2022-09-01 18:27 ` [RFC 1/4] docs/tcg-plugins: describe QPP API Andrew Fasano
2022-09-01 18:27 ` [RFC 2/4] tcg/plugins: Automatically define CURRENT_PLUGIN Andrew Fasano
@ 2022-09-01 18:27 ` Andrew Fasano
2022-09-21 14:51 ` Alex Bennée
2022-09-01 18:27 ` [RFC 4/4] tcg/plugins: Add example pair of QPP plugins Andrew Fasano
2022-09-21 12:13 ` [RFC 0/4] Support interactions between TCG plugins Alex Bennée
4 siblings, 1 reply; 16+ messages in thread
From: Andrew Fasano @ 2022-09-01 18:27 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.bennee, erdnaxe, ma.mandourr, Andrew Fasano
Expand tcg-plugin system to allow for plugins to export functions
and callbacks that can be used by other plugins. Exported functions
can be called at runtime by other loaded plugins. Loaded plugins
can register functions with exported callbacks and have these
functions run whenever the callback is triggered.
Signed-off-by: Andrew Fasano <fasano@mit.edu>
---
include/qemu/plugin-qpp.h | 252 +++++++++++++++++++++++++++++++++++
plugins/core.c | 11 ++
plugins/loader.c | 24 ++++
plugins/plugin.h | 5 +
plugins/qemu-plugins.symbols | 1 +
5 files changed, 293 insertions(+)
create mode 100644 include/qemu/plugin-qpp.h
diff --git a/include/qemu/plugin-qpp.h b/include/qemu/plugin-qpp.h
new file mode 100644
index 0000000000..befa4f9b8b
--- /dev/null
+++ b/include/qemu/plugin-qpp.h
@@ -0,0 +1,252 @@
+#ifndef PLUGIN_QPP_H
+#define PLUGIN_QPP_H
+
+/*
+ * Facilities for "QEMU plugin to plugin" (QPP) interactions between tcg
+ * plugins. These allow for an inter-plugin callback system as well
+ * as direct function calls between loaded plugins. For more details see
+ * docs/devel/plugin.rst.
+ */
+
+#include <dlfcn.h>
+#include <gmodule.h>
+#include <assert.h>
+extern GModule * qemu_plugin_name_to_handle(const char *);
+
+#define PLUGIN_CONCAT(x, y) _PLUGIN_CONCAT(x, y)
+#define _PLUGIN_CONCAT(x, y) x##y
+#define QPP_NAME(plugin, fn) PLUGIN_CONCAT(plugin, PLUGIN_CONCAT(_, fn))
+#define QPP_MAX_CB 256
+#define _QPP_SETUP_NAME(plugin, fn) PLUGIN_CONCAT(_qpp_setup_, \
+ QPP_NAME(plugin, fn))
+
+/*
+ **************************************************************************
+ * The QPP_CREATE_CB and QPP_RUN_CB macros are to be used by a plugin that
+ * wishs to create and later trigger QPP-based callback events. These are
+ * events that the plugin can detect (i.e., through analysis of guest state)
+ * that may be of interest to other plugins.
+ **************************************************************************
+ */
+
+/*
+ * QPP_CREATE_CB(name) will create a callback by defining necessary internal
+ * functions and variables based off the provided name. It must be run before
+ * triggering the callback event (with QPP_RUN_CB). This macro will create the
+ * following variables and functions, based off the provided name:
+ *
+ * 1) qpp_[name]_cb is an array of function pointers storing the
+ * registered callbacks.
+ * 2) qpp_[name]_num_cb stores the number of functions stored with this
+ * callback.
+ * 3) qpp_add_cb_[name] is a function to add a pointer into the qpp_[name]_cb
+ * array and increment qpp_[name]_num_cb.
+ * 4) qpp_remove_cb_[name] finds a registered callback, deletes it, decrements
+ * _num_cb and shifts the _cb array appropriately to fill the gap.
+ *
+ * Important notes:
+ *
+ * 1) Multiple callbacks can be registered for the same event, however consumers
+ * can not control the order in which they are called and this order may
+ * change in the future.
+ *
+ * 2) If this macro is incorrectly used multiple times in the same plugin with
+ * the same callback name set, it will raise a compilation error since
+ * these variables would then be defined multiple times. The same callback
+ * name can, however, be created in distrinct plugins without issue.
+ */
+#define QPP_CREATE_CB(cb_name) \
+void qpp_add_cb_##cb_name(cb_name##_t fptr); \
+bool qpp_remove_cb_##cb_name(cb_name##_t fptr); \
+cb_name##_t * qpp_##cb_name##_cb[QPP_MAX_CB]; \
+int qpp_##cb_name##_num_cb; \
+ \
+void qpp_add_cb_##cb_name(cb_name##_t fptr) \
+{ \
+ assert(qpp_##cb_name##_num_cb < QPP_MAX_CB); \
+ qpp_##cb_name##_cb[qpp_##cb_name##_num_cb] = fptr; \
+ qpp_##cb_name##_num_cb += 1; \
+} \
+ \
+bool qpp_remove_cb_##cb_name(cb_name##_t fptr) \
+{ \
+ int i = 0; \
+ bool found = false; \
+ for (; i < MIN(QPP_MAX_CB, qpp_##cb_name##_num_cb); i++) {\
+ if (!found && fptr == qpp_##cb_name##_cb[i]) { \
+ found = true; \
+ qpp_##cb_name##_num_cb--; \
+ } \
+ if (found && i < QPP_MAX_CB - 2) { \
+ qpp_##cb_name##_cb[i] = qpp_##cb_name##_cb[i + 1]; \
+ } \
+ } \
+ return found; \
+}
+
+
+/*
+ * QPP_RUN_CB(name, args...) should be run by the plugin that created
+ * a callback whenever it wishes to trigger the callback functions that
+ * have been registered with its previously-created callback of the provided
+ * name. If this macro is run with a callback name that was not previously
+ * created, a compile time error will be raised.
+ */
+#define QPP_RUN_CB(cb_name, ...) do { \
+ int cb_idx; \
+ for (cb_idx = 0; cb_idx < qpp_##cb_name##_num_cb; cb_idx++) { \
+ if (qpp_##cb_name##_cb[cb_idx] != NULL) { \
+ qpp_##cb_name##_cb[cb_idx](__VA_ARGS__); \
+ } \
+ } \
+} while (false)
+
+/*
+ * A header file that defines a callback function should use
+ * the QPP_CB_PROTOTYPE macro to create the necessary types.
+ */
+#define QPP_CB_PROTOTYPE(fn_ret, name, ...) \
+ typedef fn_ret(PLUGIN_CONCAT(name, _t))(__VA_ARGS__);
+
+/*
+ *****************************************************************************
+ * The QPP_REG_CB and QPP_REMOVE_CB macros are to be used by plugins that
+ * wish to run internal logic when another plugin determines that some event
+ * has occured. The plugin name, target callback name, and a local function
+ * are provided to these macros.
+ *****************************************************************************
+ */
+
+/*
+ * When a plugin wishes to register a function `cb_func` with a callback
+ * `cb_name` provided `other_plugin`, it must use the QPP_REG_CB.
+ */
+#define QPP_REG_CB(other_plugin, cb_name, cb_func) \
+{ \
+ dlerror(); \
+ void *h = qemu_plugin_name_to_handle(other_plugin); \
+ if (!h) { \
+ fprintf(stderr, "In trying to add plugin callback, "\
+ "couldn't load %s plugin\n", \
+ other_plugin); \
+ assert(h); \
+ } \
+ void (*add_cb)(cb_name##_t fptr); \
+ if (g_module_symbol(h, "qpp_add_cb_" #cb_name, \
+ (gpointer *) &add_cb)) { \
+ add_cb(cb_func); \
+ } else { \
+ fprintf(stderr, "Could not find symbol " #cb_name \
+ " in " #other_plugin "\n"); \
+ } \
+}
+
+/*
+ * If a plugin wishes to disable a previously-registered `cb_func` it should
+ * use the QPP_REMOVE_CB macro.
+ */
+#define QPP_REMOVE_CB(other_plugin, cb_name, cb_func) \
+{ \
+ dlerror(); \
+ void *op = panda_get_plugin_by_name(other_plugin); \
+ if (!op) { \
+ fprintf(stderr, "In trying to remove plugin callback, " \
+ "couldn't load %s plugin\n", other_plugin); \
+ assert(op); \
+ } \
+ void (*rm_cb)(cb_name##_t fptr) = (void (*)(cb_name##_t)) \
+ dlsym(op, "qpp_remove_cb_" \
+ #cb_name); \
+ assert(rm_cb != 0); \
+ rm_cb(cb_func); \
+}
+
+/*
+ *****************************************************************************
+ * The QPP_FUN_PROTOTYPE enables a plugin to expose a local function to other
+ * plugins. The macro should be used in a header file that is included
+ * by both the plugin that defines the function as well as other plugins
+ * that wish to call the function.
+ *****************************************************************************
+ */
+
+/*
+ * A header file that defines an exported function should use the
+ * QPP_FUN_PROTOTYPE to create the necessary types. When other plugins
+ * include this header, a function pointer named `[plugin_name]_[fn]` will
+ * be created and available for the plugin to call.
+ *
+ * Note that these functions are not callbacks, but instead regular
+ * functions that are exported by one plugin such that they can be called by
+ * others.
+ *
+ * In particular, this macro will create a function pointer by combining the
+ * `plugin_name` with an underscore and the name provided in `fn`. It will
+ * create a function to run on plugin load, that initializes this function
+ * pointer to the function named `fn` inside the plugin named `plugin_name`.
+ * If this fails, an error will be printed and the plugin will abort.
+ *
+ * There are three distinct cases this macro must handle:
+ * 1) When the header is loaded by the plugin that defines the function.
+ * In this case, we do not need to find the symbol externally.
+ * qemu_plugin_name_to_handle will return NULL, we see that the
+ * target plugin matches CURRENT_PLUGIN and do nothing.
+ * 2) When the header is loaded by another plugin but the function
+ * is not available (i.e., the target plugin isn't loaded or the
+ * target plugin does not export the requested function). In this case we
+ * raise a fatal error.
+ * 3) When the header is loaded by another plugin. In this case
+ * we need to get a handle to the target plugin and then use
+ * g_module_symbol to resolve the symbol and store it as the fn_name.
+ * If we get the handle, but can't resolve the symbol, raise a fatal error.
+ *
+ * This plugin creates the following local variables and functions:
+ *
+ * 1) `fn`: A prototype for the provided function, with the specified arguments.
+ * This is necessary for case 1 above and irrelevant for the others.
+ * 2) A function pointer typedef for `[fn]_t` set to a pointer of the type of
+ * `fn`.
+ * 3) A function pointer (of type `[fn]_t`) named `[plugin_name]_[fn]`
+ * 4) A constructor function named "_qpp_setup_[plugin_name]_[fn]" that will
+ * run when the plugin is loaded. In case 1 above, it will do nothing. In
+ * case 2 it will print an error to stderr and abort. In case 3 it will
+ * update the function pointer "[plugin_name]_[fn]" to point to the target
+ * function.
+ *
+ * After this constructor runs, the plugin can directly call the target function
+ * using `[plugin_name]_[fn]()`.
+ *
+ * For example, consider a plugin named "my_plugin" that wishes to export a
+ * function named "my_func" that returns void and takes a single integer arg.
+ * This would work as follows:
+ * 1) The header file for the plugin, my_plugin.h, should include
+ * QPP_FUN_PROTOTYPE(my_plugin, void, my_func, int) which will define the
+ * function prototype and necessary internal state.
+ * 2) This header should be included by my_plugin.c which should also include
+ * QEMU_PLUGIN_EXPORT void my_func(int) {...} with the function definition.
+ * 3) Other plugins can get access to this function by including my_plugin.h
+ * which will set up the function pointer `my_plugin_my_func` automatically
+ * using the internal state here.
+ *
+ */
+#define QPP_FUN_PROTOTYPE(plugin_name, fn_ret, fn, args) \
+ fn_ret fn(args); \
+ typedef fn_ret(*PLUGIN_CONCAT(fn, _t))(args); \
+ fn##_t QPP_NAME(plugin_name, fn); \
+ void _QPP_SETUP_NAME(plugin_name, fn) (void); \
+ \
+ void __attribute__ ((constructor)) _QPP_SETUP_NAME(plugin_name, fn) (void) {\
+ GModule *h = qemu_plugin_name_to_handle(#plugin_name); \
+ if (!h && strcmp(CURRENT_PLUGIN, #plugin_name) != 0) { \
+ fprintf(stderr, "Error plugin %s cannot access %s. Is it loaded?\n", \
+ CURRENT_PLUGIN, #plugin_name); \
+ abort(); \
+ } else if (h && !g_module_symbol(h, #fn, \
+ (gpointer *)&QPP_NAME(plugin_name, fn))) { \
+ fprintf(stderr, "Error loading symbol " # fn " from plugin " \
+ # plugin_name "\n"); \
+ abort(); \
+ } \
+ }
+
+#endif /* PLUGIN_QPP_H */
diff --git a/plugins/core.c b/plugins/core.c
index 792262da08..81c714bbf4 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -236,6 +236,17 @@ void qemu_plugin_vcpu_exit_hook(CPUState *cpu)
qemu_rec_mutex_unlock(&plugin.lock);
}
+GModule *qemu_plugin_name_to_handle(const char* name)
+{
+ struct qemu_plugin_ctx *ctx;
+ QTAILQ_FOREACH(ctx, &plugin.ctxs, entry) {
+ if (is_plugin_named(*ctx, name)) {
+ return plugin_id_to_ctx_locked(ctx->id)->handle;
+ }
+ }
+ return NULL;
+}
+
struct plugin_for_each_args {
struct qemu_plugin_ctx *ctx;
qemu_plugin_vcpu_simple_cb_t cb;
diff --git a/plugins/loader.c b/plugins/loader.c
index 88c30bde2d..26d3c14661 100644
--- a/plugins/loader.c
+++ b/plugins/loader.c
@@ -265,6 +265,30 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info, E
return 1;
}
+/**
+ * is_plugin_named - compare a plugins's name to a provided name
+ * @ctx: the ctx for the plugin in question
+ * @name: the name that should be used in the comparison
+ *
+ * Returns true if the names match.
+ */
+
+bool is_plugin_named(struct qemu_plugin_ctx ctx, const char *name)
+{
+ char *plugin_basename = basename(ctx.desc->path);
+ /*
+ * First resolve the name of the shared object for the current plugin,
+ * then check if it could possibly be of the form libNAME.so
+ * where NAME is the provided name. If so, compare the strings.
+ */
+ if (plugin_basename == NULL || strlen(plugin_basename) != strlen(name) + 6) {
+ return false;
+ }
+
+ return strncmp(plugin_basename + 3, name,
+ strlen(plugin_basename) - 6) == 0;
+}
+
/* call after having removed @desc from the list */
static void plugin_desc_free(struct qemu_plugin_desc *desc)
{
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 5eb2fdbc85..69d9b09d4c 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -97,4 +97,9 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
void exec_inline_op(struct qemu_plugin_dyn_cb *cb);
+GModule *qemu_plugin_name_to_handle(const char* name);
+
+/* loader.c */
+bool is_plugin_named(struct qemu_plugin_ctx ctx, const char *name);
+
#endif /* PLUGIN_H */
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index 71f6c90549..d98c0bc38a 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -18,6 +18,7 @@
qemu_plugin_mem_size_shift;
qemu_plugin_n_max_vcpus;
qemu_plugin_n_vcpus;
+ qemu_plugin_name_to_handle;
qemu_plugin_outs;
qemu_plugin_path_to_binary;
qemu_plugin_register_atexit_cb;
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC 4/4] tcg/plugins: Add example pair of QPP plugins
2022-09-01 18:27 [RFC 0/4] Support interactions between TCG plugins Andrew Fasano
` (2 preceding siblings ...)
2022-09-01 18:27 ` [RFC 3/4] tcg/plugins: Support for inter-plugin interactions Andrew Fasano
@ 2022-09-01 18:27 ` Andrew Fasano
2022-09-21 15:22 ` Alex Bennée
2022-09-21 15:36 ` Alex Bennée
2022-09-21 12:13 ` [RFC 0/4] Support interactions between TCG plugins Alex Bennée
4 siblings, 2 replies; 16+ messages in thread
From: Andrew Fasano @ 2022-09-01 18:27 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.bennee, erdnaxe, ma.mandourr, Andrew Fasano
The first plugin, qpp_srv exposes two functions and one callback that other
plugins can leverage. These functions are described in the corresponding
header file.
The second plugin, qpp_client, imports this header file, registers its
own function to run on a qpp_srv-provided callback, and directly calls
into the two exposed functions in qpp_srv.
Signed-off-by: Andrew Fasano <fasano@mit.edu>
---
contrib/plugins/Makefile | 2 ++
contrib/plugins/qpp_client.c | 42 ++++++++++++++++++++++++++++++++++++
contrib/plugins/qpp_client.h | 1 +
contrib/plugins/qpp_srv.c | 33 ++++++++++++++++++++++++++++
contrib/plugins/qpp_srv.h | 17 +++++++++++++++
5 files changed, 95 insertions(+)
create mode 100644 contrib/plugins/qpp_client.c
create mode 100644 contrib/plugins/qpp_client.h
create mode 100644 contrib/plugins/qpp_srv.c
create mode 100644 contrib/plugins/qpp_srv.h
diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
index b7720fea0f..b7510de89c 100644
--- a/contrib/plugins/Makefile
+++ b/contrib/plugins/Makefile
@@ -21,6 +21,8 @@ NAMES += lockstep
NAMES += hwprofile
NAMES += cache
NAMES += drcov
+NAMES += qpp_srv
+NAMES += qpp_client
SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
diff --git a/contrib/plugins/qpp_client.c b/contrib/plugins/qpp_client.c
new file mode 100644
index 0000000000..de3335e167
--- /dev/null
+++ b/contrib/plugins/qpp_client.c
@@ -0,0 +1,42 @@
+#include <stdio.h>
+#include <qemu-plugin.h>
+#include <plugin-qpp.h>
+#include <glib.h>
+#include "qpp_srv.h"
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+void my_on_exit(int x, bool b)
+{
+ g_autoptr(GString) report = g_string_new("Client: on_exit runs with args: ");
+ g_string_append_printf(report, "%d, %d\n", x, b);
+ qemu_plugin_outs(report->str);
+
+ g_string_printf(report, "Client: calls qpp_srv's do_add(1): %d\n",
+ qpp_srv_do_add(1));
+ qemu_plugin_outs(report->str);
+
+ g_string_printf(report, "Client: calls qpp_srv's do_sub(1): %d\n",
+ qpp_srv_do_sub(1));
+ qemu_plugin_outs(report->str);
+}
+
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
+ const qemu_info_t *info, int argc, char **argv) {
+
+ /*
+ * Register our "my_on_exit" function to run on the on_exit QPP-callback
+ * exported by qpp_srv
+ */
+ QPP_REG_CB("qpp_srv", on_exit, my_on_exit);
+
+ g_autoptr(GString) report = g_string_new(CURRENT_PLUGIN ": Call "
+ "qpp_srv's do_add(0) => ");
+ g_string_append_printf(report, "%d\n", qpp_srv_do_add(0));
+ qemu_plugin_outs(report->str);
+
+ g_string_printf(report, "Client: registered on_exit callback\n");
+ return 0;
+}
+
diff --git a/contrib/plugins/qpp_client.h b/contrib/plugins/qpp_client.h
new file mode 100644
index 0000000000..573923f580
--- /dev/null
+++ b/contrib/plugins/qpp_client.h
@@ -0,0 +1 @@
+void my_on_exit(int, bool);
diff --git a/contrib/plugins/qpp_srv.c b/contrib/plugins/qpp_srv.c
new file mode 100644
index 0000000000..61a6ab38ed
--- /dev/null
+++ b/contrib/plugins/qpp_srv.c
@@ -0,0 +1,33 @@
+#include <stdio.h>
+#include <qemu-plugin.h>
+#include <plugin-qpp.h>
+#include <gmodule.h>
+#include "qpp_srv.h"
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+QPP_CREATE_CB(on_exit);
+
+static void plugin_exit(qemu_plugin_id_t id, void *p)
+{
+ qemu_plugin_outs(CURRENT_PLUGIN "exit triggered, running all registered"
+ " QPP callbacks\n");
+ QPP_RUN_CB(on_exit, 0, true);
+}
+
+QEMU_PLUGIN_EXPORT int do_add(int x)
+{
+ return x + 1;
+}
+
+QEMU_PLUGIN_EXPORT int do_sub(int x)
+{
+ return x - 1;
+}
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
+ const qemu_info_t *info, int argc, char **argv) {
+ qemu_plugin_outs("qpp_srv loaded\n");
+ qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
+ return 0;
+}
diff --git a/contrib/plugins/qpp_srv.h b/contrib/plugins/qpp_srv.h
new file mode 100644
index 0000000000..ceb26e3d2c
--- /dev/null
+++ b/contrib/plugins/qpp_srv.h
@@ -0,0 +1,17 @@
+#ifndef QPP_SRV_H
+#define QPP_SRV_H
+
+/*
+ * Prototype for the on_exit callback: callback functions should be
+ * of type `void f(int, bool)`
+ */
+QPP_CB_PROTOTYPE(void, on_exit, int, bool);
+
+/*
+ * Prototypes for the do_add and do_sub functions. Both return an int and
+ * take an int as an argument.
+ */
+QPP_FUN_PROTOTYPE(qpp_srv, int, do_add, int);
+QPP_FUN_PROTOTYPE(qpp_srv, int, do_sub, int);
+
+#endif /* QPP_SRV_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC 0/4] Support interactions between TCG plugins
2022-09-01 18:27 [RFC 0/4] Support interactions between TCG plugins Andrew Fasano
` (3 preceding siblings ...)
2022-09-01 18:27 ` [RFC 4/4] tcg/plugins: Add example pair of QPP plugins Andrew Fasano
@ 2022-09-21 12:13 ` Alex Bennée
2022-09-26 21:36 ` Andrew S. Fasano
4 siblings, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2022-09-21 12:13 UTC (permalink / raw)
To: Andrew Fasano; +Cc: qemu-devel, erdnaxe, ma.mandourr
Andrew Fasano <fasano@mit.edu> writes:
> Hello,
>
> I'm requesting comments on the following series of patches expanding the
> TCG plugin system to add the "QEMU Plugin-to-Plugin (QPP)" interface
> that allows for interactions between TCG plugins. The goal of this
> interface is to enable plugins to expand on other plugins and reduce
> code duplication. This patch series includes documentation and
> significant comments, but a high-level summary is below along with a
> discussion of the current implementation as well as the benefits and
> drawbacks of these changes.
Thanks for a very detailed cover letter. My initial thoughts are if we
are trying to reduce code duplication what about simply using a library
and linking it to the final plugin. I guess it depends on how
computational effort has been spent in calculating a particular piece of
state and if that is avoided by having this IPC mechanism instead of
just repeating the calculation.
> **Summary**
>
> The QPP interface allows two types of interactions between plugins:
>
> 1) Exported functions: A plugin may wish to allow other plugins to call
> one of the functions it has defined. To do this, the plugin must mark
> the function definition as publicly visible with the QEMU_PLUGIN_EXPORT
> macro and place a definition in an included header file using the
> QPP_FUN_PROTOTYPE macro. Other plugins can then include this header and
> call the exported function by combining the name of the target plugin
> with the name of the exported function.
>
> For example, consider a hypothetical plugin that collects a list of
> cache misses. This plugin could export two functions using the QPP
> interface: one to allow another plugin to query this list and another
> to empty the list. This would enable the development of another plugin
> that examines guest CPU state to identify process changes and reports
> the cache misses per process. With the QPP interface, this second plugin
> would not need to duplicate any logic from the first.
Thinking of this concrete example I guess the process change detection
is a fairly expensive operation that might be tuned to a particular
architecture? Is this something Panda currently derives from plugins
instead of the core QEMU code?
> 2) Callbacks: Multiple plugins may wish to take some action when some
> event of interest occurs inside a running guest. To support modularity
> and reduce code duplication, the QPP callback system allows this logic
> to be contained in single plugin that detects whenever a given event
> occurs and exposes a callback with a given name. Another plugin can then
> request to have one of its own functions run whenever this event occurs.
> Additional plugins could also use this same callback to run additional
> logic whenever this event occurs.
>
> For example, consider (again) a hypothetical plugin that detects when
> the current guest process changes by analyzing the guest CPU state. This
> plugin could define a callback named "on_process_change" and trigger
> this callback event whenever it detects a process change. Other plugins
> could then be developed that take various actions on process changes by
> registering internal functions to run on this event.
>
> These patches and examples are inspired by the PANDA project
> (https://panda.re and https://github.com/panda-re/panda), a fork of QEMU
> modified to support dynamic program analysis and reverse engineering.
> PANDA also includes a large plugin system with a similar interface for
> interactions between plugins. I'm one of the maintainers of PANDA
> and have seen how the ability for plugins to interact with
> other plugins reduces code duplication and enables the creation of many
> useful plugins.
Would another use-case be to export the PANDA APIs so you could use the
existing plugins on an upstream QEMU?
> **Implementation Overview**
>
> These patches modify the TCG plugin build system to define the
> preprocessor variable CURRENT_PLUGIN to the name of the current plugin
> based off its filename. This can be useful for plugin developers in
> general and is used internally in the QPP implementation to determine
> if an exported plugin function is defined in the current plugin or
> in another.
>
> These patches also add the function qemu_plugin_name_to_handle to the
> core plugin API which uses the new internal function is_plugin_named.
> The ability for plugins to get a handle to another plugin is necessary
> for the inter-plugin interactions described below.
>
> The QPP implementation is contained inside a header file plugin-qpp.h
> that adds the macros QPP_CREATE_CB, QPP_RUN_CB, QPP_REG_CB,
> QPP_REMOVE_CB, and QPP_FUN_PROTOTYPE. The first 4 of these are related
> to the callback system and the last one is for exported functions.
>
> The QPP_CREATE_CB macro is used by a plugin that wishes to create a
> callback with a given name. The macro will create an array of function
> pointers for every function that has been registered to run on this
> callback event. Other plugins can register a local function
> with this callback (i.e., add it to this list of function pointers)
> or remove a previously-registered function using QPP_REG_CB or
> QPP_REMOVE_CB, respectively. When a plugin wishes to run all the
> registered callback functions for a given callback, it uses the
> QPP_RUN_CB macro.
>
> The QPP_FUN_PROTOYPE macro enables a plugin to expose a function
> it defines to other plugins. This macro is used in a header file that
> is included by both the plugin that defines a function as well as
> plugins that wish to use the function. This prototype creates a
> constructor function that runs on plugin load. If the target plugin
> name differs from the value of CURRENT_PLUGIN, this function
> will use qemu_plugin_name_to_handle to get a handle to the target
> plugin and use g_module_symbol to resolve the target function in
> that plugin. If this fails, it will print a warning and abort.
>
> Finally, these patches include an example pair of plugins, qpp_srv and
> qpp_client. The former provides a trivial QPP callback which simply runs
> when the plugin is unloaded as well as two functions to add and subtract
> integers. The latter of these plugins registers a function to run
> on this callback and uses the two exported functions.
While having an example plugin for debugging is useful I think having a
more useful in-tree use-case is going to be important to justify the
merging of a quite large API into the code base.
> **Request for Comments**
>
> This is my first time proposing any patches to QEMU and these are some
> non-trivial changes, so I wanted to ask for feedback as opposed to just
> sending these as a patch. I hope that others agree that there's value
> in supporting interactions between plugins (particularly inter-plugin
> callbacks), but I know there are many ways this could be implemented
> and I'm sure there are ways this could be improved.
I'll comment on the details of the patches inline.
> My goal in this design was to keep the process of creating and using
> callbacks and exported functions as easy as possible for plugin
> developers. I'm open to redesigning this interface in
> a completely different way if that would be necessary to get it merged.
>
> Although the example plugins I've included for now are trivial, if this
> interface is merged I'd be willing to develop some more plugins that
> build atop it. One of these could be something like the "syscalls2"
> plugin in PANDA which analyzes guest code and provides distinct
> callbacks for when a guest runs a system call:
> https://github.com/panda-re/panda/blob/dev/panda/plugins/syscalls2.
> Another could be something like the "callstack_isntr" plugin in PANDA
> which manages a shadow stack, provides callbacks on function calls and
> returns and also provides exported functions to get the current
> callstack:
> https://github.com/panda-re/panda/tree/dev/panda/plugins/callstack_instr
I'm certainly keen to get more plugins merged into the tree. I'm aware
there are some gaps in the plugin API at the moment. The most pressing
one is getting access to register values which requires some
re-factoring of the core code. I have very incomplete WIP branch while I
consider the API (not wired to plugins yet):
https://gitlab.com/stsquad/qemu/-/tree/introspection/registers
> I see a handful of potential issues that I'll highlight below:
> 1) Unstable APIs: these inter-plugin interactions do not specify API
> versions. If the behavior of a callback or exported function
> changes without the type signature changing, it may lead to subtle
> errors in other (unchanged) plugins that were using these functions.
>
> If the signature of a plugin's callback or exported function,
> changes, build time errors would be raised and necessary
> changes could be made to in-tree plugins. However, out-of-tree
> plugins would break.
I'm not overly concerned about out-of-tree plugins for the time being as
long as we keep the in-tree examples tested and working. That said we do
implement an API versioning scheme for the core plugin API. Maybe
something similar can be done for plugin APIs?
> 2) Checkpatch.pl objects to some of these changes. Most importantly,
> it takes issue with QPP_FUN_PROTOTYPE introducing multiple
> statements outside of a do while loop. I believe the logic in this
> macro cannot be wrapped in such a way as it is defining variables
> that must be available outside of the scope of the macro (in
> particular, these variables must be used by other QPP macros later).
>
> Given that this logic is contained in a plugin instead of the main
> QEMU binary, I'm hoping there's some leeway with this error, but
> if it needs to be changed, I could split the logic into 5 distinct
> macros that a plugin developer would then run in order. Or perhaps
> someone here can suggest a better design.
>
> Additional, I believe the script is raising a number of false
> positive errors for the indentation of the multi-line macros, e.g.,
> "suspect code indent for conditional statements (4, 6)" for
> QPP_RUN_CB. If I'm wrong about this, I'm happy to adjust the
> indentation. If it is an issue with the script, I could open a bug
> report if that would help.
checkpatch.pl isn't gating and certainly macro magic can trip it up.
That said we try to avoid adding new deep magic into the code base. I
shall see what the patch does.
> 3) Decentralized interactions. This approach allows plugins to directly
> interact with other plugins. An alternative design could register
> exported functions and callbacks with the core plugin logic in
> the main QEMU object and have all the interactions go through there.
> Would a centralized design where plugins send requests through
> the core plugin logic in the QEMU binary be better that allowing
> direct interactions between the shared objects built for the
> plugins?
I shall see when I read the code but it does make me wander if we are
just implementing a dynamic linker by another name. Maybe something like
the callback/event system would be better marshalled by QEMU itself? I
wonder if the dlload mechanism could just automatically make all
non-static plugin functions exportable and then just complain if we get
a clash or missing symbol when it is loaded?
> Does it seem like an interface like this would be worth merging? If so,
> I'd welcome any and all suggestions on how to improve these changes.
I'm certainly open to it but I do think we will need some more concrete
examples using such an API before we could consider merging it. I don't
want to merge something that would only be used by out-of-tree plugins
because it would impose a maintenance burden for no project gain.
>
> Thank you,
> Andrew Fasano
>
> Andrew Fasano (4):
> docs/tcg-plugins: describe QPP API
> tcg/plugins: Automatically define CURRENT_PLUGIN
> tcg/plugins: Support for inter-plugin interactions
> tcg/plugins: Add example pair of QPP plugins
>
> contrib/plugins/Makefile | 4 +-
> contrib/plugins/qpp_client.c | 42 ++++++
> contrib/plugins/qpp_client.h | 1 +
> contrib/plugins/qpp_srv.c | 33 +++++
> contrib/plugins/qpp_srv.h | 17 +++
> docs/devel/tcg-plugins.rst | 76 +++++++++++
> include/qemu/plugin-qpp.h | 252 +++++++++++++++++++++++++++++++++++
> plugins/core.c | 11 ++
> plugins/loader.c | 24 ++++
> plugins/plugin.h | 5 +
> plugins/qemu-plugins.symbols | 1 +
> 11 files changed, 465 insertions(+), 1 deletion(-)
> create mode 100644 contrib/plugins/qpp_client.c
> create mode 100644 contrib/plugins/qpp_client.h
> create mode 100644 contrib/plugins/qpp_srv.c
> create mode 100644 contrib/plugins/qpp_srv.h
> create mode 100644 include/qemu/plugin-qpp.h
--
Alex Bennée
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC 1/4] docs/tcg-plugins: describe QPP API
2022-09-01 18:27 ` [RFC 1/4] docs/tcg-plugins: describe QPP API Andrew Fasano
@ 2022-09-21 13:57 ` Alex Bennée
0 siblings, 0 replies; 16+ messages in thread
From: Alex Bennée @ 2022-09-21 13:57 UTC (permalink / raw)
To: Andrew Fasano; +Cc: qemu-devel, erdnaxe, ma.mandourr
Andrew Fasano <fasano@mit.edu> writes:
> Describe how multiple TCG plugins can interact using the QEMU
> Plugin-to-Plugin API (QPP) with both callbacks and direct
> function calls.
Looks ok at first glance. I suspect it is quickly coming to the point we
need to split the examples and the API apart in the docs to stop things
getting too messy.
>
> Signed-off-by: Andrew Fasano <fasano@mit.edu>
> ---
> docs/devel/tcg-plugins.rst | 76 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
> index a7cc44aa20..7985572027 100644
> --- a/docs/devel/tcg-plugins.rst
> +++ b/docs/devel/tcg-plugins.rst
> @@ -441,3 +441,79 @@ The plugin has a number of arguments, all of them are optional:
> associativity of the L2 cache, respectively. Setting any of the L2
> configuration arguments implies ``l2=on``.
> (default: N = 2097152 (2MB), B = 64, A = 16)
> +
> +Plugin-to-Plugin Interactions
> +-----------------------------
> +
> +Plugins may interact with other plugins through the QEMU Plugin-to-Plugin
> +("QPP") API by including ``qemu/plugin-qpp.h``. This API supports direct
> +function calls between plugins as well as an inter-plugin callback system.
> +This API allows for composition of plugins: plugins can make use of logic in
> +other plugins without the need for code duplication.
> +
> +Plugin names
> +~~~~~~~~~~~~
> +Plugins are automatically given a name by removing the suffix from their
> +filename. These plugin names will be used during QPP interactions as
> +described below. A plugin can access its own name through the preprocessor
> +variable ``CURRENT_PLUGIN``.
> +
> +QPP function calls
> +~~~~~~~~~~~~~~~~~~
> +When a plugin (e.g. ``plugin_a``) wishes to make some of its functions (e.g.
> +``func_1``) available to other plugins, it must:
> +
> +1. Mark the function definition with the ``QEMU_PLUGIN_EXPORT`` macro. For
> +example : ``QEMU_PLUGIN_EXPORT int func_1(int x) {...}``.
> +2. Provide prototypes for exported functions in a header file (e.g.
> +``plugin_a.h``) using the macro ``QPP_FUN_PROTOTYPE`` with arguments of the
> +plugin's name, the function's return type, the function's name, and any
> +arguments the function takes. For example:
> +``QPP_FUN_PROTOTYPE(plugin_a, int, func_1, int);``.
> +3. Import this header from the plugin.
> +
> +When other plugins wish to use the functions exported by ``plugin_a``, they
> +must:
> +
> +1. Import the header file (e.g. ``plugin_a.h``) with the function prototype(s).
> +2. Call the function when desired by combining the target plugin name, an
> + underscore, and the target function name, e.g. ``plugin_a_func_1()``.
> +
> +QPP callbacks
> +~~~~~~~~~~~~~
> +
> +The QPP API also allows a plugin to define callback events and for other plugins
> +to request to be notified whenever these events happens. The plugin that defines
> +the callback is responsible for triggering the callback when it so wishes. Other
> +plugins that wish to be notified on these events must define a function of an
> +appropriate type and register it to run on this event.
> +
> +When a plugin (e.g. ``plugin_a``) wishes to define a callback (an event that
> +other plugins can request to be notified about), it must:
> +
> +1. Define the callback using the ``QPP_CREATE_CB`` macro with a single argument
> + of the callback's name. For example: ``QPP_CREATE_CB(on_some_event);``.
> +2. In a header file (e.g. ``plugin_a.h``) create a prototype for the callback
> + type with ``QPP_CB_PROTOTYPE`` with arguments of the callback's return type
> + (only ``void`` is currently supported), the name of the callback, and any
> + arguments the callback function will be called with. For example with a
> + callback named ``on_some_event`` that returns a void and takes an int and
> + a bool as an argument, you would use: ``QPP_CB_PROTOTYPE(void,
> + on_some_event, int, bool)``.
> +3. Import this header from the plugin.
> +4. When the plugin wishes to run any registered callback functions, it should
> + use the macro ``QPP_RUN_CB`` with the first argument set to the callback
> + name followed by the arguments as specified in the header. For example:
> + ``QPP_RUN_CB(on_some_event, 2, true);``.
> +
> +When other plugins wish to register a function to run on such an event, they
> +must:
> +
> +1. Import the header file with the callback prototype(s) (e.g. ``plugin_a.h``)
> +2. Define a function that matches the callback signature. For example:
> + ``void plugin_b_callback(int, bool) {...}``.
> +3. Register this function to be run on the callback using the ``QPP_REG_CB``
> + macro with the first argument being the name of the plugin that provides the
> + callback (as a string), the second being the callback name, and the third as
> + the function that should be run. For example: ``QPP_REG_CB("plugin_a",
> + on_some_event, plugin_b_callback);``
--
Alex Bennée
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC 2/4] tcg/plugins: Automatically define CURRENT_PLUGIN
2022-09-01 18:27 ` [RFC 2/4] tcg/plugins: Automatically define CURRENT_PLUGIN Andrew Fasano
@ 2022-09-21 14:00 ` Alex Bennée
2022-09-26 21:37 ` Andrew S. Fasano
0 siblings, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2022-09-21 14:00 UTC (permalink / raw)
To: Andrew Fasano; +Cc: qemu-devel, erdnaxe, ma.mandourr
Andrew Fasano <fasano@mit.edu> writes:
> Use plugin filenames to set the preprocessor variable CURRENT_PLUGIN
> as a string during plugin compilation.
>
> Signed-off-by: Andrew Fasano <fasano@mit.edu>
> ---
> contrib/plugins/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
> index df3499f4f2..b7720fea0f 100644
> --- a/contrib/plugins/Makefile
> +++ b/contrib/plugins/Makefile
> @@ -34,7 +34,7 @@ CFLAGS += -I$(SRC_PATH)/include/qemu
> all: $(SONAMES)
>
> %.o: %.c
> - $(CC) $(CFLAGS) -c -o $@ $<
> + $(CC) $(CFLAGS) -DCURRENT_PLUGIN=\"$(basename $@)\" -c -o $@ $<
While all plugins are currently single files this seems a little clumsy.
We can already check exported plugin symbols in loader.c (see
qemu_plugin_version) so maybe it would be better to declare an API
update and mandate any plugin object also needs to define a
qemu_plugin_name with a null terminated string?
>
> lib%.so: %.o
> $(CC) -shared -Wl,-soname,$@ -o $@ $^ $(LDLIBS)
--
Alex Bennée
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC 3/4] tcg/plugins: Support for inter-plugin interactions
2022-09-01 18:27 ` [RFC 3/4] tcg/plugins: Support for inter-plugin interactions Andrew Fasano
@ 2022-09-21 14:51 ` Alex Bennée
2022-09-26 21:37 ` Andrew S. Fasano
0 siblings, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2022-09-21 14:51 UTC (permalink / raw)
To: Andrew Fasano; +Cc: qemu-devel, erdnaxe, ma.mandourr
Andrew Fasano <fasano@mit.edu> writes:
> Expand tcg-plugin system to allow for plugins to export functions
> and callbacks that can be used by other plugins. Exported functions
> can be called at runtime by other loaded plugins. Loaded plugins
> can register functions with exported callbacks and have these
> functions run whenever the callback is triggered.
Could you please split the callback and function handling in future
patches to aid review.
>
> Signed-off-by: Andrew Fasano <fasano@mit.edu>
> ---
> include/qemu/plugin-qpp.h | 252 +++++++++++++++++++++++++++++++++++
> plugins/core.c | 11 ++
> plugins/loader.c | 24 ++++
> plugins/plugin.h | 5 +
> plugins/qemu-plugins.symbols | 1 +
> 5 files changed, 293 insertions(+)
> create mode 100644 include/qemu/plugin-qpp.h
>
> diff --git a/include/qemu/plugin-qpp.h b/include/qemu/plugin-qpp.h
> new file mode 100644
> index 0000000000..befa4f9b8b
> --- /dev/null
> +++ b/include/qemu/plugin-qpp.h
> @@ -0,0 +1,252 @@
> +#ifndef PLUGIN_QPP_H
> +#define PLUGIN_QPP_H
> +
> +/*
> + * Facilities for "QEMU plugin to plugin" (QPP) interactions between tcg
> + * plugins. These allow for an inter-plugin callback system as well
> + * as direct function calls between loaded plugins. For more details see
> + * docs/devel/plugin.rst.
> + */
> +
> +#include <dlfcn.h>
> +#include <gmodule.h>
> +#include <assert.h>
> +extern GModule * qemu_plugin_name_to_handle(const char *);
Plugin API functions should have /** */ kernel-doc annotations for the
benefit of the autogenerated API docs. Moreover handles to plugins are
usually anonymous pointer types to discourage them fishing about in the
contents.
The fact we expose GModule to the plugin to do the linking makes me
think that maybe the linking should be pushed into loader and be done on
behalf of the plugin.
> +
> +#define PLUGIN_CONCAT(x, y) _PLUGIN_CONCAT(x, y)
> +#define _PLUGIN_CONCAT(x, y) x##y
> +#define QPP_NAME(plugin, fn) PLUGIN_CONCAT(plugin, PLUGIN_CONCAT(_, fn))
> +#define QPP_MAX_CB 256
> +#define _QPP_SETUP_NAME(plugin, fn) PLUGIN_CONCAT(_qpp_setup_, \
> + QPP_NAME(plugin, fn))
> +
> +/*
> + **************************************************************************
> + * The QPP_CREATE_CB and QPP_RUN_CB macros are to be used by a plugin that
> + * wishs to create and later trigger QPP-based callback events. These are
> + * events that the plugin can detect (i.e., through analysis of guest state)
> + * that may be of interest to other plugins.
> + **************************************************************************
> + */
> +
> +/*
> + * QPP_CREATE_CB(name) will create a callback by defining necessary internal
> + * functions and variables based off the provided name. It must be run before
> + * triggering the callback event (with QPP_RUN_CB). This macro will create the
> + * following variables and functions, based off the provided name:
> + *
> + * 1) qpp_[name]_cb is an array of function pointers storing the
> + * registered callbacks.
> + * 2) qpp_[name]_num_cb stores the number of functions stored with this
> + * callback.
> + * 3) qpp_add_cb_[name] is a function to add a pointer into the qpp_[name]_cb
> + * array and increment qpp_[name]_num_cb.
> + * 4) qpp_remove_cb_[name] finds a registered callback, deletes it, decrements
> + * _num_cb and shifts the _cb array appropriately to fill the gap.
> + *
> + * Important notes:
> + *
> + * 1) Multiple callbacks can be registered for the same event, however consumers
> + * can not control the order in which they are called and this order may
> + * change in the future.
> + *
> + * 2) If this macro is incorrectly used multiple times in the same plugin with
> + * the same callback name set, it will raise a compilation error since
> + * these variables would then be defined multiple times. The same callback
> + * name can, however, be created in distrinct plugins without issue.
> + */
> +#define QPP_CREATE_CB(cb_name) \
> +void qpp_add_cb_##cb_name(cb_name##_t fptr); \
> +bool qpp_remove_cb_##cb_name(cb_name##_t fptr); \
> +cb_name##_t * qpp_##cb_name##_cb[QPP_MAX_CB]; \
> +int qpp_##cb_name##_num_cb; \
> + \
> +void qpp_add_cb_##cb_name(cb_name##_t fptr) \
> +{ \
> + assert(qpp_##cb_name##_num_cb < QPP_MAX_CB); \
> + qpp_##cb_name##_cb[qpp_##cb_name##_num_cb] = fptr; \
> + qpp_##cb_name##_num_cb += 1; \
> +} \
> + \
> +bool qpp_remove_cb_##cb_name(cb_name##_t fptr) \
> +{ \
> + int i = 0; \
> + bool found = false; \
> + for (; i < MIN(QPP_MAX_CB, qpp_##cb_name##_num_cb); i++) {\
> + if (!found && fptr == qpp_##cb_name##_cb[i]) { \
> + found = true; \
> + qpp_##cb_name##_num_cb--; \
> + } \
> + if (found && i < QPP_MAX_CB - 2) { \
> + qpp_##cb_name##_cb[i] = qpp_##cb_name##_cb[i + 1]; \
> + } \
> + } \
> + return found; \
> +}
This house keeping definitely makes me think event handling would be
better handled inside of the core plugin code. Instead of jumping
through macro hoops to track state you could have something like:
void * handle = qemu_plugin_register_event_trigger("plugin name", "event name")
...
qemu_plugin_trigger_event(handle, void *ev_data);
and then on the listener side:
qemu_plugin_register_event_listener("plugin name", "event name", my_fn, user_data);
...
my_fun(void *ev_data, void *udata) {
.. do stuff ..
}
it would also allow both plugins and the core code to introspect which
events have been added.
> +
> +
> +/*
> + * QPP_RUN_CB(name, args...) should be run by the plugin that created
> + * a callback whenever it wishes to trigger the callback functions that
> + * have been registered with its previously-created callback of the provided
> + * name. If this macro is run with a callback name that was not previously
> + * created, a compile time error will be raised.
> + */
> +#define QPP_RUN_CB(cb_name, ...) do { \
> + int cb_idx; \
> + for (cb_idx = 0; cb_idx < qpp_##cb_name##_num_cb; cb_idx++) { \
> + if (qpp_##cb_name##_cb[cb_idx] != NULL) { \
> + qpp_##cb_name##_cb[cb_idx](__VA_ARGS__); \
> + } \
> + } \
> +} while (false)
> +
> +/*
> + * A header file that defines a callback function should use
> + * the QPP_CB_PROTOTYPE macro to create the necessary types.
> + */
> +#define QPP_CB_PROTOTYPE(fn_ret, name, ...) \
> + typedef fn_ret(PLUGIN_CONCAT(name, _t))(__VA_ARGS__);
If we stick to a single event prototype with event data (which can be
specified by the other plugin header) and user data that should be
flexible enough and avoid additional complications?
> +
> +/*
> + *****************************************************************************
> + * The QPP_REG_CB and QPP_REMOVE_CB macros are to be used by plugins that
> + * wish to run internal logic when another plugin determines that some event
> + * has occured. The plugin name, target callback name, and a local function
> + * are provided to these macros.
> + *****************************************************************************
> + */
> +
> +/*
> + * When a plugin wishes to register a function `cb_func` with a callback
> + * `cb_name` provided `other_plugin`, it must use the QPP_REG_CB.
> + */
> +#define QPP_REG_CB(other_plugin, cb_name, cb_func) \
> +{ \
> + dlerror(); \
> + void *h = qemu_plugin_name_to_handle(other_plugin); \
> + if (!h) { \
> + fprintf(stderr, "In trying to add plugin callback, "\
> + "couldn't load %s plugin\n", \
> + other_plugin); \
> + assert(h); \
> + } \
> + void (*add_cb)(cb_name##_t fptr); \
> + if (g_module_symbol(h, "qpp_add_cb_" #cb_name, \
> + (gpointer *) &add_cb)) { \
> + add_cb(cb_func); \
> + } else { \
> + fprintf(stderr, "Could not find symbol " #cb_name \
> + " in " #other_plugin "\n"); \
> + } \
Another benefit of doing the linking in loader would be more graceful
handling of the error cases and reporting.
> +}
> +
> +/*
> + * If a plugin wishes to disable a previously-registered `cb_func` it should
> + * use the QPP_REMOVE_CB macro.
> + */
> +#define QPP_REMOVE_CB(other_plugin, cb_name, cb_func) \
> +{ \
> + dlerror(); \
> + void *op = panda_get_plugin_by_name(other_plugin); \
> + if (!op) { \
> + fprintf(stderr, "In trying to remove plugin callback, " \
> + "couldn't load %s plugin\n", other_plugin); \
> + assert(op); \
> + } \
> + void (*rm_cb)(cb_name##_t fptr) = (void (*)(cb_name##_t)) \
> + dlsym(op, "qpp_remove_cb_" \
> + #cb_name); \
> + assert(rm_cb != 0); \
> + rm_cb(cb_func); \
> +}
> +
> +/*
> + *****************************************************************************
> + * The QPP_FUN_PROTOTYPE enables a plugin to expose a local function to other
> + * plugins. The macro should be used in a header file that is included
> + * by both the plugin that defines the function as well as other plugins
> + * that wish to call the function.
> + *****************************************************************************
> + */
> +
> +/*
> + * A header file that defines an exported function should use the
> + * QPP_FUN_PROTOTYPE to create the necessary types. When other plugins
> + * include this header, a function pointer named `[plugin_name]_[fn]` will
> + * be created and available for the plugin to call.
> + *
> + * Note that these functions are not callbacks, but instead regular
> + * functions that are exported by one plugin such that they can be called by
> + * others.
> + *
> + * In particular, this macro will create a function pointer by combining the
> + * `plugin_name` with an underscore and the name provided in `fn`. It will
> + * create a function to run on plugin load, that initializes this function
> + * pointer to the function named `fn` inside the plugin named `plugin_name`.
> + * If this fails, an error will be printed and the plugin will abort.
This replicates a bunch of what the QEMU_PLUGIN_EXPORT does but also
adds some concatenation rules. Would it not be enough just to export the
name and then restrict the linking in loader.c to searching plugins in a
declared list from the source plugin?
> + *
> + * There are three distinct cases this macro must handle:
> + * 1) When the header is loaded by the plugin that defines the function.
> + * In this case, we do not need to find the symbol externally.
> + * qemu_plugin_name_to_handle will return NULL, we see that the
> + * target plugin matches CURRENT_PLUGIN and do nothing.
> + * 2) When the header is loaded by another plugin but the function
> + * is not available (i.e., the target plugin isn't loaded or the
> + * target plugin does not export the requested function). In this case we
> + * raise a fatal error.
> + * 3) When the header is loaded by another plugin. In this case
> + * we need to get a handle to the target plugin and then use
> + * g_module_symbol to resolve the symbol and store it as the fn_name.
> + * If we get the handle, but can't resolve the symbol, raise a fatal error.
> + *
> + * This plugin creates the following local variables and functions:
> + *
> + * 1) `fn`: A prototype for the provided function, with the specified arguments.
> + * This is necessary for case 1 above and irrelevant for the others.
> + * 2) A function pointer typedef for `[fn]_t` set to a pointer of the type of
> + * `fn`.
> + * 3) A function pointer (of type `[fn]_t`) named `[plugin_name]_[fn]`
> + * 4) A constructor function named "_qpp_setup_[plugin_name]_[fn]" that will
> + * run when the plugin is loaded. In case 1 above, it will do nothing. In
> + * case 2 it will print an error to stderr and abort. In case 3 it will
> + * update the function pointer "[plugin_name]_[fn]" to point to the target
> + * function.
> + *
> + * After this constructor runs, the plugin can directly call the target function
> + * using `[plugin_name]_[fn]()`.
> + *
> + * For example, consider a plugin named "my_plugin" that wishes to export a
> + * function named "my_func" that returns void and takes a single integer arg.
> + * This would work as follows:
> + * 1) The header file for the plugin, my_plugin.h, should include
> + * QPP_FUN_PROTOTYPE(my_plugin, void, my_func, int) which will define the
> + * function prototype and necessary internal state.
> + * 2) This header should be included by my_plugin.c which should also include
> + * QEMU_PLUGIN_EXPORT void my_func(int) {...} with the function definition.
> + * 3) Other plugins can get access to this function by including my_plugin.h
> + * which will set up the function pointer `my_plugin_my_func` automatically
> + * using the internal state here.
> + *
> + */
> +#define QPP_FUN_PROTOTYPE(plugin_name, fn_ret, fn, args) \
> + fn_ret fn(args); \
> + typedef fn_ret(*PLUGIN_CONCAT(fn, _t))(args); \
> + fn##_t QPP_NAME(plugin_name, fn); \
> + void _QPP_SETUP_NAME(plugin_name, fn) (void); \
> + \
> + void __attribute__ ((constructor)) _QPP_SETUP_NAME(plugin_name, fn) (void) {\
> + GModule *h = qemu_plugin_name_to_handle(#plugin_name); \
> + if (!h && strcmp(CURRENT_PLUGIN, #plugin_name) != 0) { \
> + fprintf(stderr, "Error plugin %s cannot access %s. Is it loaded?\n", \
> + CURRENT_PLUGIN, #plugin_name); \
> + abort(); \
> + } else if (h && !g_module_symbol(h, #fn, \
> + (gpointer *)&QPP_NAME(plugin_name, fn))) { \
> + fprintf(stderr, "Error loading symbol " # fn " from plugin " \
> + # plugin_name "\n"); \
> + abort(); \
> + } \
> + }
While nothing stops a plugin calling abort() itself I'd rather avoid
having it inserted by macro magic. Another argument for doing this
inside the core code.
> +
> +#endif /* PLUGIN_QPP_H */
> diff --git a/plugins/core.c b/plugins/core.c
> index 792262da08..81c714bbf4 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -236,6 +236,17 @@ void qemu_plugin_vcpu_exit_hook(CPUState *cpu)
> qemu_rec_mutex_unlock(&plugin.lock);
> }
>
> +GModule *qemu_plugin_name_to_handle(const char* name)
> +{
> + struct qemu_plugin_ctx *ctx;
> + QTAILQ_FOREACH(ctx, &plugin.ctxs, entry) {
> + if (is_plugin_named(*ctx, name)) {
> + return plugin_id_to_ctx_locked(ctx->id)->handle;
The _locked prefix in QEMU indicated the function expects a lock to be
held. In this case you could use QEMU_LOCK_GUARD(&plugin.lock); at the
head of the function.
> + }
> + }
> + return NULL;
> +}
> +
> struct plugin_for_each_args {
> struct qemu_plugin_ctx *ctx;
> qemu_plugin_vcpu_simple_cb_t cb;
> diff --git a/plugins/loader.c b/plugins/loader.c
> index 88c30bde2d..26d3c14661 100644
> --- a/plugins/loader.c
> +++ b/plugins/loader.c
> @@ -265,6 +265,30 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info, E
> return 1;
> }
>
> +/**
> + * is_plugin_named - compare a plugins's name to a provided name
> + * @ctx: the ctx for the plugin in question
> + * @name: the name that should be used in the comparison
> + *
> + * Returns true if the names match.
> + */
> +
> +bool is_plugin_named(struct qemu_plugin_ctx ctx, const char *name)
> +{
> + char *plugin_basename = basename(ctx.desc->path);
> + /*
> + * First resolve the name of the shared object for the current plugin,
> + * then check if it could possibly be of the form libNAME.so
> + * where NAME is the provided name. If so, compare the strings.
> + */
> + if (plugin_basename == NULL || strlen(plugin_basename) != strlen(name) + 6) {
> + return false;
> + }
> +
> + return strncmp(plugin_basename + 3, name,
> + strlen(plugin_basename) - 6) == 0;
I'd rather favour just making plugin names mandatory with the API boost.
> +}
> +
> /* call after having removed @desc from the list */
> static void plugin_desc_free(struct qemu_plugin_desc *desc)
> {
> diff --git a/plugins/plugin.h b/plugins/plugin.h
> index 5eb2fdbc85..69d9b09d4c 100644
> --- a/plugins/plugin.h
> +++ b/plugins/plugin.h
> @@ -97,4 +97,9 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
>
> void exec_inline_op(struct qemu_plugin_dyn_cb *cb);
>
> +GModule *qemu_plugin_name_to_handle(const char* name);
exported functions shouldn't need to be declared twice, but as above we
can drop this I think.
> +
> +/* loader.c */
> +bool is_plugin_named(struct qemu_plugin_ctx ctx, const char *name);
> +
> #endif /* PLUGIN_H */
> diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
> index 71f6c90549..d98c0bc38a 100644
> --- a/plugins/qemu-plugins.symbols
> +++ b/plugins/qemu-plugins.symbols
> @@ -18,6 +18,7 @@
> qemu_plugin_mem_size_shift;
> qemu_plugin_n_max_vcpus;
> qemu_plugin_n_vcpus;
> + qemu_plugin_name_to_handle;
> qemu_plugin_outs;
> qemu_plugin_path_to_binary;
> qemu_plugin_register_atexit_cb;
--
Alex Bennée
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC 4/4] tcg/plugins: Add example pair of QPP plugins
2022-09-01 18:27 ` [RFC 4/4] tcg/plugins: Add example pair of QPP plugins Andrew Fasano
@ 2022-09-21 15:22 ` Alex Bennée
2022-09-26 21:38 ` Andrew S. Fasano
2022-09-21 15:36 ` Alex Bennée
1 sibling, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2022-09-21 15:22 UTC (permalink / raw)
To: Andrew Fasano; +Cc: qemu-devel, erdnaxe, ma.mandourr
Andrew Fasano <fasano@mit.edu> writes:
> The first plugin, qpp_srv exposes two functions and one callback that other
> plugins can leverage. These functions are described in the corresponding
> header file.
>
> The second plugin, qpp_client, imports this header file, registers its
> own function to run on a qpp_srv-provided callback, and directly calls
> into the two exposed functions in qpp_srv.
I'll just sketch out how I would change the API in this example plugin:
>
> Signed-off-by: Andrew Fasano <fasano@mit.edu>
> ---
> contrib/plugins/Makefile | 2 ++
> contrib/plugins/qpp_client.c | 42 ++++++++++++++++++++++++++++++++++++
> contrib/plugins/qpp_client.h | 1 +
> contrib/plugins/qpp_srv.c | 33 ++++++++++++++++++++++++++++
> contrib/plugins/qpp_srv.h | 17 +++++++++++++++
> 5 files changed, 95 insertions(+)
> create mode 100644 contrib/plugins/qpp_client.c
> create mode 100644 contrib/plugins/qpp_client.h
> create mode 100644 contrib/plugins/qpp_srv.c
> create mode 100644 contrib/plugins/qpp_srv.h
>
> diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
> index b7720fea0f..b7510de89c 100644
> --- a/contrib/plugins/Makefile
> +++ b/contrib/plugins/Makefile
> @@ -21,6 +21,8 @@ NAMES += lockstep
> NAMES += hwprofile
> NAMES += cache
> NAMES += drcov
> +NAMES += qpp_srv
> +NAMES += qpp_client
>
> SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>
> diff --git a/contrib/plugins/qpp_client.c b/contrib/plugins/qpp_client.c
> new file mode 100644
> index 0000000000..de3335e167
> --- /dev/null
> +++ b/contrib/plugins/qpp_client.c
> @@ -0,0 +1,42 @@
> +#include <stdio.h>
> +#include <qemu-plugin.h>
> +#include <plugin-qpp.h>
> +#include <glib.h>
> +#include "qpp_srv.h"
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
QEMU_PLUGIN_EXPORT const char *qemu_plugin_name = "qpp_client";
QEMU_PLUGIN_EXPORT const char *qemu_plugin_uses = "qpp_server";
> +
> +void my_on_exit(int x, bool b)
void my_on_exit(gpointer evdata, gpointer udata)
{
struct qpp_exit_event *info = (struct qpp_exit_event *) evdata;
x = info->x;
b = info->b;
> +{
> + g_autoptr(GString) report = g_string_new("Client: on_exit runs with args: ");
> + g_string_append_printf(report, "%d, %d\n", x, b);
> + qemu_plugin_outs(report->str);
> +
> + g_string_printf(report, "Client: calls qpp_srv's do_add(1): %d\n",
> + qpp_srv_do_add(1));
> + qemu_plugin_outs(report->str);
> +
> + g_string_printf(report, "Client: calls qpp_srv's do_sub(1): %d\n",
> + qpp_srv_do_sub(1));
> + qemu_plugin_outs(report->str);
> +}
> +
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
> + const qemu_info_t *info, int argc, char **argv) {
> +
> + /*
> + * Register our "my_on_exit" function to run on the on_exit QPP-callback
> + * exported by qpp_srv
> + */
> + QPP_REG_CB("qpp_srv", on_exit, my_on_exit);
qemu_plugin_register_event_listener("qpp_server", "exit", my_on_exit);
> +
> + g_autoptr(GString) report = g_string_new(CURRENT_PLUGIN ": Call "
> + "qpp_srv's do_add(0) => ");
> + g_string_append_printf(report, "%d\n", qpp_srv_do_add(0));
> + qemu_plugin_outs(report->str);
> +
> + g_string_printf(report, "Client: registered on_exit callback\n");
> + return 0;
> +}
> +
> diff --git a/contrib/plugins/qpp_client.h b/contrib/plugins/qpp_client.h
> new file mode 100644
> index 0000000000..573923f580
> --- /dev/null
> +++ b/contrib/plugins/qpp_client.h
> @@ -0,0 +1 @@
> +void my_on_exit(int, bool);
> diff --git a/contrib/plugins/qpp_srv.c b/contrib/plugins/qpp_srv.c
> new file mode 100644
> index 0000000000..61a6ab38ed
> --- /dev/null
> +++ b/contrib/plugins/qpp_srv.c
> @@ -0,0 +1,33 @@
> +#include <stdio.h>
> +#include <qemu-plugin.h>
> +#include <plugin-qpp.h>
> +#include <gmodule.h>
> +#include "qpp_srv.h"
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
QEMU_PLUGIN_EXPORT const char *qemu_plugin_name = "qpp_server";
> +
> +QPP_CREATE_CB(on_exit);
void *on_exit;
> +
> +static void plugin_exit(qemu_plugin_id_t id, void *p)
> +{
> + qemu_plugin_outs(CURRENT_PLUGIN "exit triggered, running all registered"
> + " QPP callbacks\n");
> + QPP_RUN_CB(on_exit, 0, true);
struct qpp_exit_event *info = g_new0(qpp_exit_event, 1);
info->x = 0;
info->b = true;
qemu_plugin_trigger_event(on_exit, info);
> +}
> +
> +QEMU_PLUGIN_EXPORT int do_add(int x)
QEMU_PLUGIN_EXPORT int qpp_srv_do_add(int x)
> +{
> + return x + 1;
> +}
> +
> +QEMU_PLUGIN_EXPORT int do_sub(int x)
QEMU_PLUGIN_EXPORT int qpp_srv_do_sub(int x)
> +{
> + return x - 1;
> +}
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
> + const qemu_info_t *info, int argc, char **argv) {
> + qemu_plugin_outs("qpp_srv loaded\n");
> + qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
> + return 0;
> +}
> diff --git a/contrib/plugins/qpp_srv.h b/contrib/plugins/qpp_srv.h
> new file mode 100644
> index 0000000000..ceb26e3d2c
> --- /dev/null
> +++ b/contrib/plugins/qpp_srv.h
> @@ -0,0 +1,17 @@
> +#ifndef QPP_SRV_H
> +#define QPP_SRV_H
> +
> +/*
> + * Prototype for the on_exit callback: callback functions should be
> + * of type `void f(int, bool)`
> + */
> +QPP_CB_PROTOTYPE(void, on_exit, int, bool);
can be dropped.
> +
> +/*
> + * Prototypes for the do_add and do_sub functions. Both return an int and
> + * take an int as an argument.
> + */
> +QPP_FUN_PROTOTYPE(qpp_srv, int, do_add, int);
> +QPP_FUN_PROTOTYPE(qpp_srv, int, do_sub, int);
int qpp_srv_do_add(int);
int qpp_srv_do_sub(int);
the linking is dealt with by loader.
> +
> +#endif /* QPP_SRV_H */
--
Alex Bennée
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC 4/4] tcg/plugins: Add example pair of QPP plugins
2022-09-01 18:27 ` [RFC 4/4] tcg/plugins: Add example pair of QPP plugins Andrew Fasano
2022-09-21 15:22 ` Alex Bennée
@ 2022-09-21 15:36 ` Alex Bennée
2022-09-26 21:38 ` Andrew S. Fasano
1 sibling, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2022-09-21 15:36 UTC (permalink / raw)
To: Andrew Fasano; +Cc: qemu-devel, erdnaxe, ma.mandourr
Andrew Fasano <fasano@mit.edu> writes:
> The first plugin, qpp_srv exposes two functions and one callback that other
> plugins can leverage. These functions are described in the corresponding
> header file.
>
> The second plugin, qpp_client, imports this header file, registers its
> own function to run on a qpp_srv-provided callback, and directly calls
> into the two exposed functions in qpp_srv.
>
> Signed-off-by: Andrew Fasano <fasano@mit.edu>
> ---
> contrib/plugins/Makefile | 2 ++
> contrib/plugins/qpp_client.c | 42 ++++++++++++++++++++++++++++++++++++
> contrib/plugins/qpp_client.h | 1 +
> contrib/plugins/qpp_srv.c | 33 ++++++++++++++++++++++++++++
> contrib/plugins/qpp_srv.h | 17 +++++++++++++++
Oh and I forgot this toy case should probably be in test/plugins/qpp with an
explicit test in tests/tcg/multiarch/Makefile to exercise it during
"make check-tcg". This should hopefully avoid having to mess with
PLUGINS in tests/tcg/Makefile.target.
<snip>
--
Alex Bennée
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC 0/4] Support interactions between TCG plugins
2022-09-21 12:13 ` [RFC 0/4] Support interactions between TCG plugins Alex Bennée
@ 2022-09-26 21:36 ` Andrew S. Fasano
0 siblings, 0 replies; 16+ messages in thread
From: Andrew S. Fasano @ 2022-09-26 21:36 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, erdnaxe, ma.mandourr
From: Alex Bennée <alex.bennee@linaro.org>
> Andrew Fasano <fasano@mit.edu> writes:
>
> > Hello,
> >
> > I'm requesting comments on the following series of patches expanding the
> > TCG plugin system to add the "QEMU Plugin-to-Plugin (QPP)" interface
> > that allows for interactions between TCG plugins. The goal of this
> > interface is to enable plugins to expand on other plugins and reduce
> > code duplication. This patch series includes documentation and
> > significant comments, but a high-level summary is below along with a
> > discussion of the current implementation as well as the benefits and
> > drawbacks of these changes.
>
> Thanks for a very detailed cover letter. My initial thoughts are if we
> are trying to reduce code duplication what about simply using a library
> and linking it to the final plugin. I guess it depends on how
> computational effort has been spent in calculating a particular piece of
> state and if that is avoided by having this IPC mechanism instead of
> just repeating the calculation.
Thanks for the detailed feedback Alex, I think I understand and agree with the
changes you've suggested. I'll work on those over the next couple weeks and
send along patches (or another RFC, let me know what you prefer) when they're
ready. And perhaps some follow up questions that come up along the way.
>
> > **Summary**
> >
> > The QPP interface allows two types of interactions between plugins:
> >
> > 1) Exported functions: A plugin may wish to allow other plugins to call
> > one of the functions it has defined. To do this, the plugin must mark
> > the function definition as publicly visible with the QEMU_PLUGIN_EXPORT
> > macro and place a definition in an included header file using the
> > QPP_FUN_PROTOTYPE macro. Other plugins can then include this header and
> > call the exported function by combining the name of the target plugin
> > with the name of the exported function.
> >
> > For example, consider a hypothetical plugin that collects a list of
> > cache misses. This plugin could export two functions using the QPP
> > interface: one to allow another plugin to query this list and another
> > to empty the list. This would enable the development of another plugin
> > that examines guest CPU state to identify process changes and reports
> > the cache misses per process. With the QPP interface, this second plugin
> > would not need to duplicate any logic from the first.
>
> Thinking of this concrete example I guess the process change detection
> is a fairly expensive operation that might be tuned to a particular
> architecture? Is this something Panda currently derives from plugins
> instead of the core QEMU code?
Process change detection can be somewhat complex. PANDA has a callback in
the core emulation code named on_asid_changed that fires whenever the address
space layout identifier (ASID) is changed in the CPUState. This fires on most
process changes for some architectures (x86), but this sort of event is too
low-level to always be correct and varies by architecture.
To provide accurate detection of when processes change, we have a PANDA plugin
for operating system introspection which, when given details of the guest OS,
can provide a callback on the process change as well as some APIs for querying
details about running processes in the guest. This callback and these APIs
are exposed to other plugins through our inter-plugin interface.
>
> > 2) Callbacks: Multiple plugins may wish to take some action when some
> > event of interest occurs inside a running guest. To support modularity
> > and reduce code duplication, the QPP callback system allows this logic
> > to be contained in single plugin that detects whenever a given event
> > occurs and exposes a callback with a given name. Another plugin can then
> > request to have one of its own functions run whenever this event occurs.
> > Additional plugins could also use this same callback to run additional
> > logic whenever this event occurs.
> >
> > For example, consider (again) a hypothetical plugin that detects when
> > the current guest process changes by analyzing the guest CPU state. This
> > plugin could define a callback named "on_process_change" and trigger
> > this callback event whenever it detects a process change. Other plugins
> > could then be developed that take various actions on process changes by
> > registering internal functions to run on this event.
> >
> > These patches and examples are inspired by the PANDA project
> > (https://panda.re and https://github.com/panda-re/panda), a fork of QEMU
> > modified to support dynamic program analysis and reverse engineering.
> > PANDA also includes a large plugin system with a similar interface for
> > interactions between plugins. I'm one of the maintainers of PANDA
> > and have seen how the ability for plugins to interact with
> > other plugins reduces code duplication and enables the creation of many
> > useful plugins.
>
> Would another use-case be to export the PANDA APIs so you could use the
> existing plugins on an upstream QEMU?
>
PANDA was designed and built by a bunch of researchers who wanted to quickly
get their research done (myself included). I think the design of its APIs
reflect this and aren't up to par with the code quality expected in QEMU. In
particular, the PANDA APIs directly expose the CPUState to plugins
and allow them to read and write any fields in it. This means PANDA plugins
are only compatible with the version of the emulator they are designed and
built against.
Although the QEMU plugin interface is smaller than the one provided by PANDA,
I think the QEMU design is much better. I'd be happy to help expand
the callbacks and APIs available to QEMU plugins to get to a point
where we can port some PANDA plugins to be in-tree QEMU plugins. I suspect
only a small subset (maybe about 15) of the ~45 PANDA plugins we have would be
worth merging, but it would depend on the types of plugins you'd want in-tree,
if they needed to work for all QEMU-supported architectures, and if plugins
would be allowed to modify guest state.
If anyone is interested, the full list of PANDA plugins and their source code
can be found in our repository at
https://github.com/panda-re/panda/tree/dev/panda/plugins/.
If any of these seem particularly interesting, I'd be happy to figure out
what (if any) new APIs they'd require, help build those APIs in QEMU, and then
change the plugin and try to get it merged into QEMU. I will point out
that some of them are built atop non-trivial emulation changes that would
likely not be wanted here (such as using LLVM IR instead of TCG to support
dynamic taint analysis).
> > **Implementation Overview**
[snip]
> While having an example plugin for debugging is useful I think having a
> more useful in-tree use-case is going to be important to justify the
> merging of a quite large API into the code base.
This sounds good to me, but I worry that QEMU plugins aren't (yet) able to
access enough information to build analyses complex enough to really
demonstrate the value of this type of interface. If plugins were able
to read guest registers, I could easily throw together a more powerful example
set of plugins such as the syscalls identification and logging plugins
described in my original message below.
Perhaps this is an argument for holding off on finalizing the details of
inter-plugin interactions until the core QEMU plugin interface is a bit
larger? I'd be okay with that as your response here shows that you're open to
the idea of allowing plugins to interact with other plugins which I see
as a key feature for building complex analyses in this interface. Knowing
that this is coming in the future (i.e., that the analyses I'm interested in
doing will eventually be possible with QEMU plugins) means I'd be willing to
contribute to other parts of the plugin interface before then.
> > **Request for Comments**
[snip]
> I'm certainly keen to get more plugins merged into the tree. I'm aware
> there are some gaps in the plugin API at the moment. The most pressing
> one is getting access to register values which requires some
> re-factoring of the core code. I have very incomplete WIP branch while I
> consider the API (not wired to plugins yet):
>
> https://gitlab.com/stsquad/qemu/-/tree/introspection/registers
>
I'm really glad to see you're trying to expose that information to plugins,
it's definitely a blocker for many analyses! I had tried out a different
implementation of this using the gdb_read_register functions. I don't
particularly like the code I wrote and it was painful to use, but it
was quite short and didn't require changes outside of the plugin code.
On the off chance that there's something useful in it, you can see the code at
https://github.com/AndrewFasano/futurepanda/commit/1c46cc89bf3e825f559a70ccb84b4f034cd2490b
> > I see a handful of potential issues that I'll highlight below:
> > 1) Unstable APIs: these inter-plugin interactions do not specify API
> > versions. If the behavior of a callback or exported function
> > changes without the type signature changing, it may lead to subtle
> > errors in other (unchanged) plugins that were using these functions.
> >
> > If the signature of a plugin's callback or exported function,
> > changes, build time errors would be raised and necessary
> > changes could be made to in-tree plugins. However, out-of-tree
> > plugins would break.
>
> I'm not overly concerned about out-of-tree plugins for the time being as
> long as we keep the in-tree examples tested and working. That said we do
> implement an API versioning scheme for the core plugin API. Maybe
> something similar can be done for plugin APIs?
Glad to hear that. I'll consider this as I think about the changes you've
suggested in the code.
[snip]
> > 3) Decentralized interactions. This approach allows plugins to directly
> > interact with other plugins. An alternative design could register
> > exported functions and callbacks with the core plugin logic in
> > the main QEMU object and have all the interactions go through there.
> > Would a centralized design where plugins send requests through
> > the core plugin logic in the QEMU binary be better that allowing
> > direct interactions between the shared objects built for the
> > plugins?
>
> I shall see when I read the code but it does make me wander if we are
> just implementing a dynamic linker by another name. Maybe something like
> the callback/event system would be better marshalled by QEMU itself? I
> wonder if the dlload mechanism could just automatically make all
> non-static plugin functions exportable and then just complain if we get
> a clash or missing symbol when it is loaded?
>
I think I'm coming around to the idea that this is a better approach. I'll
go in more details in response to your feedback on the code.
> > Does it seem like an interface like this would be worth merging? If so,
> > I'd welcome any and all suggestions on how to improve these changes.
>
> I'm certainly open to it but I do think we will need some more concrete
> examples using such an API before we could consider merging it. I don't
> want to merge something that would only be used by out-of-tree plugins
> because it would impose a maintenance burden for no project gain.
I'm more than happy to build some plugins that use these APIs. My goal here
is to move as much code as would be welcome into QEMU. Just wanted to figure
out these APIs before writing plugins that use them. In the future when
we've figured out more details of this API, I'll be sure to include some
additional plugins with the patch.
If there's any documentation or prior discussion about what types of plugins
are wanted in QEMU, I'd appreciate it if someone could share that. I assume
you don't want a full platform for dynamic program analysis to live in tree
and require maintenance for obscure features that are rarely used. But if
you're interested in things that might be useful for a reasonable fraction of
the users who build QEMU with plugins enabled, I think there's a lot of
valuable capabilities that could be ported over from PANDA.
Thanks so much for the reply and for your review of the code,
Andrew
>
> >
> > Thank you,
> > Andrew Fasano
> >
> > Andrew Fasano (4):
> > docs/tcg-plugins: describe QPP API
> > tcg/plugins: Automatically define CURRENT_PLUGIN
> > tcg/plugins: Support for inter-plugin interactions
> > tcg/plugins: Add example pair of QPP plugins
> >
> > contrib/plugins/Makefile | 4 +-
> > contrib/plugins/qpp_client.c | 42 ++++++
> > contrib/plugins/qpp_client.h | 1 +
> > contrib/plugins/qpp_srv.c | 33 +++++
> > contrib/plugins/qpp_srv.h | 17 +++
> > docs/devel/tcg-plugins.rst | 76 +++++++++++
> > include/qemu/plugin-qpp.h | 252 +++++++++++++++++++++++++++++++++++
> > plugins/core.c | 11 ++
> > plugins/loader.c | 24 ++++
> > plugins/plugin.h | 5 +
> > plugins/qemu-plugins.symbols | 1 +
> > 11 files changed, 465 insertions(+), 1 deletion(-)
> > create mode 100644 contrib/plugins/qpp_client.c
> > create mode 100644 contrib/plugins/qpp_client.h
> > create mode 100644 contrib/plugins/qpp_srv.c
> > create mode 100644 contrib/plugins/qpp_srv.h
> > create mode 100644 include/qemu/plugin-qpp.h
>
>
> --
> Alex Bennée
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC 2/4] tcg/plugins: Automatically define CURRENT_PLUGIN
2022-09-21 14:00 ` Alex Bennée
@ 2022-09-26 21:37 ` Andrew S. Fasano
0 siblings, 0 replies; 16+ messages in thread
From: Andrew S. Fasano @ 2022-09-26 21:37 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, erdnaxe, ma.mandourr
From: Alex Bennée <alex.bennee@linaro.org>
> Andrew Fasano <fasano@mit.edu> writes:
>
> > Use plugin filenames to set the preprocessor variable CURRENT_PLUGIN
> > as a string during plugin compilation.
> >
> > Signed-off-by: Andrew Fasano <fasano@mit.edu>
> > ---
> > contrib/plugins/Makefile | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
> > index df3499f4f2..b7720fea0f 100644
> > --- a/contrib/plugins/Makefile
> > +++ b/contrib/plugins/Makefile
> > @@ -34,7 +34,7 @@ CFLAGS += -I$(SRC_PATH)/include/qemu
> > all: $(SONAMES)
> >
> > %.o: %.c
> > - $(CC) $(CFLAGS) -c -o $@ $<
> > + $(CC) $(CFLAGS) -DCURRENT_PLUGIN=\"$(basename $@)\" -c -o $@ $<
>
> While all plugins are currently single files this seems a little clumsy.
>
> We can already check exported plugin symbols in loader.c (see
> qemu_plugin_version) so maybe it would be better to declare an API
> update and mandate any plugin object also needs to define a
> qemu_plugin_name with a null terminated string?
That design sounds good to me and would be good for multi-file plugins. Perhaps
plugins could also (optionally?) specify their own API versioning here.
>
> >
> > lib%.so: %.o
> > $(CC) -shared -Wl,-soname,$@ -o $@ $^ $(LDLIBS)
>
>
> --
> Alex Bennée
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC 3/4] tcg/plugins: Support for inter-plugin interactions
2022-09-21 14:51 ` Alex Bennée
@ 2022-09-26 21:37 ` Andrew S. Fasano
0 siblings, 0 replies; 16+ messages in thread
From: Andrew S. Fasano @ 2022-09-26 21:37 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, erdnaxe, ma.mandourr
> Andrew Fasano <fasano@mit.edu> writes:
>
> > Expand tcg-plugin system to allow for plugins to export functions
> > and callbacks that can be used by other plugins. Exported functions
> > can be called at runtime by other loaded plugins. Loaded plugins
> > can register functions with exported callbacks and have these
> > functions run whenever the callback is triggered.
>
> Could you please split the callback and function handling in future
> patches to aid review.
Will do, now that I see your response I understand why that would've been
better.
>
> >
> > Signed-off-by: Andrew Fasano <fasano@mit.edu>
> > ---
> > include/qemu/plugin-qpp.h | 252 +++++++++++++++++++++++++++++++++++
> > plugins/core.c | 11 ++
> > plugins/loader.c | 24 ++++
> > plugins/plugin.h | 5 +
> > plugins/qemu-plugins.symbols | 1 +
> > 5 files changed, 293 insertions(+)
> > create mode 100644 include/qemu/plugin-qpp.h
> >
> > diff --git a/include/qemu/plugin-qpp.h b/include/qemu/plugin-qpp.h
> > new file mode 100644
> > index 0000000000..befa4f9b8b
> > --- /dev/null
> > +++ b/include/qemu/plugin-qpp.h
> > @@ -0,0 +1,252 @@
> > +#ifndef PLUGIN_QPP_H
> > +#define PLUGIN_QPP_H
> > +
> > +/*
> > + * Facilities for "QEMU plugin to plugin" (QPP) interactions between tcg
> > + * plugins. These allow for an inter-plugin callback system as well
> > + * as direct function calls between loaded plugins. For more details see
> > + * docs/devel/plugin.rst.
> > + */
> > +
> > +#include <dlfcn.h>
> > +#include <gmodule.h>
> > +#include <assert.h>
> > +extern GModule * qemu_plugin_name_to_handle(const char *);
>
> Plugin API functions should have /** */ kernel-doc annotations for the
> benefit of the autogenerated API docs. Moreover handles to plugins are
> usually anonymous pointer types to discourage them fishing about in the
> contents.
>
> The fact we expose GModule to the plugin to do the linking makes me
> think that maybe the linking should be pushed into loader and be done on
> behalf of the plugin.
[snip]
> This house keeping definitely makes me think event handling would be
> better handled inside of the core plugin code. Instead of jumping
> through macro hoops to track state you could have something like:
>
> void * handle = qemu_plugin_register_event_trigger("plugin name", "event name")
> ...
> qemu_plugin_trigger_event(handle, void *ev_data);
>
> and then on the listener side:
>
> qemu_plugin_register_event_listener("plugin name", "event name", my_fn, user_data);
> ...
> my_fun(void *ev_data, void *udata) {
> .. do stuff ..
> }
>
> it would also allow both plugins and the core code to introspect which
> events have been added.
>
You have thoroughly convinced me that this logic would be better in the core
plugin code. I'll try to make that change such that plugins can be designed
like your example above.
> > +
> > +
> > +/*
> > + * QPP_RUN_CB(name, args...) should be run by the plugin that created
> > + * a callback whenever it wishes to trigger the callback functions that
> > + * have been registered with its previously-created callback of the provided
> > + * name. If this macro is run with a callback name that was not previously
> > + * created, a compile time error will be raised.
> > + */
> > +#define QPP_RUN_CB(cb_name, ...) do { \
> > + int cb_idx; \
> > + for (cb_idx = 0; cb_idx < qpp_##cb_name##_num_cb; cb_idx++) { \
> > + if (qpp_##cb_name##_cb[cb_idx] != NULL) { \
> > + qpp_##cb_name##_cb[cb_idx](__VA_ARGS__); \
> > + } \
> > + } \
> > +} while (false)
> > +
> > +/*
> > + * A header file that defines a callback function should use
> > + * the QPP_CB_PROTOTYPE macro to create the necessary types.
> > + */
> > +#define QPP_CB_PROTOTYPE(fn_ret, name, ...) \
> > + typedef fn_ret(PLUGIN_CONCAT(name, _t))(__VA_ARGS__);
>
> If we stick to a single event prototype with event data (which can be
> specified by the other plugin header) and user data that should be
> flexible enough and avoid additional complications?
>
Sure, removing the magic to handle variable argument would simplify
the implementation while keeping the flexibility here.
> > +
> > +/*
> > + *****************************************************************************
> > + * The QPP_REG_CB and QPP_REMOVE_CB macros are to be used by plugins that
> > + * wish to run internal logic when another plugin determines that some event
> > + * has occured. The plugin name, target callback name, and a local function
> > + * are provided to these macros.
> > + *****************************************************************************
> > + */
> > +
> > +/*
> > + * When a plugin wishes to register a function `cb_func` with a callback
> > + * `cb_name` provided `other_plugin`, it must use the QPP_REG_CB.
> > + */
> > +#define QPP_REG_CB(other_plugin, cb_name, cb_func) \
> > +{ \
> > + dlerror(); \
> > + void *h = qemu_plugin_name_to_handle(other_plugin); \
> > + if (!h) { \
> > + fprintf(stderr, "In trying to add plugin callback, "\
> > + "couldn't load %s plugin\n", \
> > + other_plugin); \
> > + assert(h); \
> > + } \
> > + void (*add_cb)(cb_name##_t fptr); \
> > + if (g_module_symbol(h, "qpp_add_cb_" #cb_name, \
> > + (gpointer *) &add_cb)) { \
> > + add_cb(cb_func); \
> > + } else { \
> > + fprintf(stderr, "Could not find symbol " #cb_name \
> > + " in " #other_plugin "\n"); \
> > + } \
>
> Another benefit of doing the linking in loader would be more graceful
> handling of the error cases and reporting.
Agreed.
>
> > +}
> > +
> > +/*
> > + * If a plugin wishes to disable a previously-registered `cb_func` it should
> > + * use the QPP_REMOVE_CB macro.
> > + */
> > +#define QPP_REMOVE_CB(other_plugin, cb_name, cb_func) \
> > +{ \
> > + dlerror(); \
> > + void *op = panda_get_plugin_by_name(other_plugin); \
> > + if (!op) { \
> > + fprintf(stderr, "In trying to remove plugin callback, " \
> > + "couldn't load %s plugin\n", other_plugin); \
> > + assert(op); \
> > + } \
> > + void (*rm_cb)(cb_name##_t fptr) = (void (*)(cb_name##_t)) \
> > + dlsym(op, "qpp_remove_cb_" \
> > + #cb_name); \
> > + assert(rm_cb != 0); \
> > + rm_cb(cb_func); \
> > +}
> > +
> > +/*
> > + *****************************************************************************
> > + * The QPP_FUN_PROTOTYPE enables a plugin to expose a local function to other
> > + * plugins. The macro should be used in a header file that is included
> > + * by both the plugin that defines the function as well as other plugins
> > + * that wish to call the function.
> > + *****************************************************************************
> > + */
> > +
> > +/*
> > + * A header file that defines an exported function should use the
> > + * QPP_FUN_PROTOTYPE to create the necessary types. When other plugins
> > + * include this header, a function pointer named `[plugin_name]_[fn]` will
> > + * be created and available for the plugin to call.
> > + *
> > + * Note that these functions are not callbacks, but instead regular
> > + * functions that are exported by one plugin such that they can be called by
> > + * others.
> > + *
> > + * In particular, this macro will create a function pointer by combining the
> > + * `plugin_name` with an underscore and the name provided in `fn`. It will
> > + * create a function to run on plugin load, that initializes this function
> > + * pointer to the function named `fn` inside the plugin named `plugin_name`.
> > + * If this fails, an error will be printed and the plugin will abort.
>
> This replicates a bunch of what the QEMU_PLUGIN_EXPORT does but also
> adds some concatenation rules. Would it not be enough just to export the
> name and then restrict the linking in loader.c to searching plugins in a
> declared list from the source plugin?
>
I don't think so, but I could certainly be wrong. From what I understand,
QEMU_PLUGIN_EXPORT just makes the labeled function globally visible so its
address can be resolved with dlsym after it's loaded with dlopen.
The initial version of the code I shared here handles this by using macros
that create a constructor that runs on plugin load. Inside these constructors,
the macro-generated code uses the new qemu_plugin_name_to_handle function
to get a handle to the target plugin, then g_module_symbol to map that to
a function pointer. Finally, with this function pointer in hand, it sets up
the function name such that it can be called.
This could be redesigned to move more of this logic into the plugin core and
keep the plugins from directly working with the opaque handles, but I believe
some macros would still be necessary to make the target function name
into an object that could be called. This change would shorten the
QPP_FUN_PROTOTYPE macro to something more like this:
#define QPP_FUN_PROTOTYPE(plugin_name, fn_ret, fn, args) \
fn_ret fn(args); \
typedef fn_ret(*PLUGIN_CONCAT(fn, _t))(args); \
fn##_t QPP_NAME(plugin_name, fn); \
void _QPP_SETUP_NAME(plugin_name, fn) (void); \
\
void __attribute__ ((constructor)) _QPP_SETUP_NAME(plugin_name, fn) (void) {\
qemu_plugin_initialize_qpp_fn_or_error(CURRENT_PLUGIN, plugin_name, \
#fn, &QPP_NAME(plugin_name, fn)) \
}
Then some of the logic could move to `qemu_plugin_initialize_qpp_fn_or_error`
which would be in the plugin core core and responsible for handling the
dynamic linking or raising an error. It could also support logging when
plugins load references to one another and (if desired) handle automatically
loading plugins as necessary.
Does that seem like an improvement? Or if there's something I'm missing that
would allow plugins to more easily call functions in another with various
type signatures and without needing to manually cast function pointers, I'm
all ears!
> > + *
> > + * There are three distinct cases this macro must handle:
> > + * 1) When the header is loaded by the plugin that defines the function.
> > + * In this case, we do not need to find the symbol externally.
> > + * qemu_plugin_name_to_handle will return NULL, we see that the
> > + * target plugin matches CURRENT_PLUGIN and do nothing.
> > + * 2) When the header is loaded by another plugin but the function
> > + * is not available (i.e., the target plugin isn't loaded or the
> > + * target plugin does not export the requested function). In this case we
> > + * raise a fatal error.
> > + * 3) When the header is loaded by another plugin. In this case
> > + * we need to get a handle to the target plugin and then use
> > + * g_module_symbol to resolve the symbol and store it as the fn_name.
> > + * If we get the handle, but can't resolve the symbol, raise a fatal error.
> > + *
> > + * This plugin creates the following local variables and functions:
> > + *
> > + * 1) `fn`: A prototype for the provided function, with the specified arguments.
> > + * This is necessary for case 1 above and irrelevant for the others.
> > + * 2) A function pointer typedef for `[fn]_t` set to a pointer of the type of
> > + * `fn`.
> > + * 3) A function pointer (of type `[fn]_t`) named `[plugin_name]_[fn]`
> > + * 4) A constructor function named "_qpp_setup_[plugin_name]_[fn]" that will
> > + * run when the plugin is loaded. In case 1 above, it will do nothing. In
> > + * case 2 it will print an error to stderr and abort. In case 3 it will
> > + * update the function pointer "[plugin_name]_[fn]" to point to the target
> > + * function.
> > + *
> > + * After this constructor runs, the plugin can directly call the target function
> > + * using `[plugin_name]_[fn]()`.
> > + *
> > + * For example, consider a plugin named "my_plugin" that wishes to export a
> > + * function named "my_func" that returns void and takes a single integer arg.
> > + * This would work as follows:
> > + * 1) The header file for the plugin, my_plugin.h, should include
> > + * QPP_FUN_PROTOTYPE(my_plugin, void, my_func, int) which will define the
> > + * function prototype and necessary internal state.
> > + * 2) This header should be included by my_plugin.c which should also include
> > + * QEMU_PLUGIN_EXPORT void my_func(int) {...} with the function definition.
> > + * 3) Other plugins can get access to this function by including my_plugin.h
> > + * which will set up the function pointer `my_plugin_my_func` automatically
> > + * using the internal state here.
> > + *
> > + */
> > +#define QPP_FUN_PROTOTYPE(plugin_name, fn_ret, fn, args) \
> > + fn_ret fn(args); \
> > + typedef fn_ret(*PLUGIN_CONCAT(fn, _t))(args); \
> > + fn##_t QPP_NAME(plugin_name, fn); \
> > + void _QPP_SETUP_NAME(plugin_name, fn) (void); \
> > + \
> > + void __attribute__ ((constructor)) _QPP_SETUP_NAME(plugin_name, fn) (void) {\
> > + GModule *h = qemu_plugin_name_to_handle(#plugin_name); \
> > + if (!h && strcmp(CURRENT_PLUGIN, #plugin_name) != 0) { \
> > + fprintf(stderr, "Error plugin %s cannot access %s. Is it loaded?\n", \
> > + CURRENT_PLUGIN, #plugin_name); \
> > + abort(); \
> > + } else if (h && !g_module_symbol(h, #fn, \
> > + (gpointer *)&QPP_NAME(plugin_name, fn))) { \
> > + fprintf(stderr, "Error loading symbol " # fn " from plugin " \
> > + # plugin_name "\n"); \
> > + abort(); \
> > + } \
> > + }
>
> While nothing stops a plugin calling abort() itself I'd rather avoid
> having it inserted by macro magic. Another argument for doing this
> inside the core code.
Sounds good to me.
>
> > +
> > +#endif /* PLUGIN_QPP_H */
> > diff --git a/plugins/core.c b/plugins/core.c
> > index 792262da08..81c714bbf4 100644
> > --- a/plugins/core.c
> > +++ b/plugins/core.c
> > @@ -236,6 +236,17 @@ void qemu_plugin_vcpu_exit_hook(CPUState *cpu)
> > qemu_rec_mutex_unlock(&plugin.lock);
> > }
> >
> > +GModule *qemu_plugin_name_to_handle(const char* name)
> > +{
> > + struct qemu_plugin_ctx *ctx;
> > + QTAILQ_FOREACH(ctx, &plugin.ctxs, entry) {
> > + if (is_plugin_named(*ctx, name)) {
> > + return plugin_id_to_ctx_locked(ctx->id)->handle;
>
> The _locked prefix in QEMU indicated the function expects a lock to be
> held. In this case you could use QEMU_LOCK_GUARD(&plugin.lock); at the
> head of the function.
Thanks, I didn't know that.
>
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > struct plugin_for_each_args {
> > struct qemu_plugin_ctx *ctx;
> > qemu_plugin_vcpu_simple_cb_t cb;
> > diff --git a/plugins/loader.c b/plugins/loader.c
> > index 88c30bde2d..26d3c14661 100644
> > --- a/plugins/loader.c
> > +++ b/plugins/loader.c
> > @@ -265,6 +265,30 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info, E
> > return 1;
> > }
> >
> > +/**
> > + * is_plugin_named - compare a plugins's name to a provided name
> > + * @ctx: the ctx for the plugin in question
> > + * @name: the name that should be used in the comparison
> > + *
> > + * Returns true if the names match.
> > + */
> > +
> > +bool is_plugin_named(struct qemu_plugin_ctx ctx, const char *name)
> > +{
> > + char *plugin_basename = basename(ctx.desc->path);
> > + /*
> > + * First resolve the name of the shared object for the current plugin,
> > + * then check if it could possibly be of the form libNAME.so
> > + * where NAME is the provided name. If so, compare the strings.
> > + */
> > + if (plugin_basename == NULL || strlen(plugin_basename) != strlen(name) + 6) {
> > + return false;
> > + }
> > +
> > + return strncmp(plugin_basename + 3, name,
> > + strlen(plugin_basename) - 6) == 0;
>
> I'd rather favour just making plugin names mandatory with the API boost.
I agree.
>
> > +}
> > +
> > /* call after having removed @desc from the list */
> > static void plugin_desc_free(struct qemu_plugin_desc *desc)
> > {
> > diff --git a/plugins/plugin.h b/plugins/plugin.h
> > index 5eb2fdbc85..69d9b09d4c 100644
> > --- a/plugins/plugin.h
> > +++ b/plugins/plugin.h
> > @@ -97,4 +97,9 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
> >
> > void exec_inline_op(struct qemu_plugin_dyn_cb *cb);
> >
> > +GModule *qemu_plugin_name_to_handle(const char* name);
>
> exported functions shouldn't need to be declared twice, but as above we
> can drop this I think.
>
> > +
> > +/* loader.c */
> > +bool is_plugin_named(struct qemu_plugin_ctx ctx, const char *name);
> > +
> > #endif /* PLUGIN_H */
> > diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
> > index 71f6c90549..d98c0bc38a 100644
> > --- a/plugins/qemu-plugins.symbols
> > +++ b/plugins/qemu-plugins.symbols
> > @@ -18,6 +18,7 @@
> > qemu_plugin_mem_size_shift;
> > qemu_plugin_n_max_vcpus;
> > qemu_plugin_n_vcpus;
> > + qemu_plugin_name_to_handle;
> > qemu_plugin_outs;
> > qemu_plugin_path_to_binary;
> > qemu_plugin_register_atexit_cb;
>
>
> --
> Alex Bennée
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC 4/4] tcg/plugins: Add example pair of QPP plugins
2022-09-21 15:22 ` Alex Bennée
@ 2022-09-26 21:38 ` Andrew S. Fasano
0 siblings, 0 replies; 16+ messages in thread
From: Andrew S. Fasano @ 2022-09-26 21:38 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, erdnaxe, ma.mandourr
From: Alex Bennée <alex.bennee@linaro.org>
> Andrew Fasano <fasano@mit.edu> writes:
>
> > The first plugin, qpp_srv exposes two functions and one callback that other
> > plugins can leverage. These functions are described in the corresponding
> > header file.
> >
> > The second plugin, qpp_client, imports this header file, registers its
> > own function to run on a qpp_srv-provided callback, and directly calls
> > into the two exposed functions in qpp_srv.
>
> I'll just sketch out how I would change the API in this example plugin:
[snip]
> QEMU_PLUGIN_EXPORT const char *qemu_plugin_name = "qpp_client";
This works. Perhaps plugins could (should?) also specify a version number
that other plugins could check before interacting with:
QEMU_PLUGIN_EXPORT const int qemu_plugin_version = 1;
> QEMU_PLUGIN_EXPORT const char *qemu_plugin_uses = "qpp_server";
I anticipate plugins could depend on multiple other plugins and they might
want to check the version of these plugins, so perhaps a function call would
be better? Something like:
int qpp_srv_version = load_qemu_plugin("qpp_server");
Perhaps returning negative values on error, otherwise the plugin version. Or
the version compatability checks could be standardized into the plugin core,
but that seems less important to me for now.
[snip]
> QEMU_PLUGIN_EXPORT int qpp_srv_do_add(int x)
I like the change to keep these with the plugin name as a prefix everywhere.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC 4/4] tcg/plugins: Add example pair of QPP plugins
2022-09-21 15:36 ` Alex Bennée
@ 2022-09-26 21:38 ` Andrew S. Fasano
0 siblings, 0 replies; 16+ messages in thread
From: Andrew S. Fasano @ 2022-09-26 21:38 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, erdnaxe, ma.mandourr
From: Alex Bennée <alex.bennee@linaro.org>
> Andrew Fasano <fasano@mit.edu> writes:
>
> > The first plugin, qpp_srv exposes two functions and one callback that other
> > plugins can leverage. These functions are described in the corresponding
> > header file.
> >
> > The second plugin, qpp_client, imports this header file, registers its
> > own function to run on a qpp_srv-provided callback, and directly calls
> > into the two exposed functions in qpp_srv.
> >
> > Signed-off-by: Andrew Fasano <fasano@mit.edu>
> > ---
> > contrib/plugins/Makefile | 2 ++
> > contrib/plugins/qpp_client.c | 42 ++++++++++++++++++++++++++++++++++++
> > contrib/plugins/qpp_client.h | 1 +
> > contrib/plugins/qpp_srv.c | 33 ++++++++++++++++++++++++++++
> > contrib/plugins/qpp_srv.h | 17 +++++++++++++++
>
> Oh and I forgot this toy case should probably be in test/plugins/qpp with an
> explicit test in tests/tcg/multiarch/Makefile to exercise it during
> "make check-tcg". This should hopefully avoid having to mess with
> PLUGINS in tests/tcg/Makefile.target.
Okay, that makes sense. Thanks so much for all the feedback!
>
> <snip>
>
> --
> Alex Bennée
Thanks,
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-09-26 21:47 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-01 18:27 [RFC 0/4] Support interactions between TCG plugins Andrew Fasano
2022-09-01 18:27 ` [RFC 1/4] docs/tcg-plugins: describe QPP API Andrew Fasano
2022-09-21 13:57 ` Alex Bennée
2022-09-01 18:27 ` [RFC 2/4] tcg/plugins: Automatically define CURRENT_PLUGIN Andrew Fasano
2022-09-21 14:00 ` Alex Bennée
2022-09-26 21:37 ` Andrew S. Fasano
2022-09-01 18:27 ` [RFC 3/4] tcg/plugins: Support for inter-plugin interactions Andrew Fasano
2022-09-21 14:51 ` Alex Bennée
2022-09-26 21:37 ` Andrew S. Fasano
2022-09-01 18:27 ` [RFC 4/4] tcg/plugins: Add example pair of QPP plugins Andrew Fasano
2022-09-21 15:22 ` Alex Bennée
2022-09-26 21:38 ` Andrew S. Fasano
2022-09-21 15:36 ` Alex Bennée
2022-09-26 21:38 ` Andrew S. Fasano
2022-09-21 12:13 ` [RFC 0/4] Support interactions between TCG plugins Alex Bennée
2022-09-26 21:36 ` Andrew S. Fasano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).