qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC 3/4] tcg/plugins: Support for inter-plugin interactions


From: Alex Bennée
Subject: Re: [RFC 3/4] tcg/plugins: Support for inter-plugin interactions
Date: Wed, 21 Sep 2022 15:51:52 +0100
User-agent: mu4e 1.9.0; emacs 28.2.50

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]