On Tue, Oct 6, 2020 at 7:29 PM Alex Bennée <
alex.bennee@linaro.org> wrote:
>
>
> Yonggang Luo <
luoyonggang@gmail.com> writes:
>
> > We removed the need of .symbols file, so is the
> > configure script, if we one expose a function to qemu-plugin
> > just need prefix the function with QEMU_PLUGIN_EXPORT
> >
> > Signed-off-by: Yonggang Luo <
luoyonggang@gmail.com>
> > ---
> > Makefile | 1 -
> > configure | 71 -------------
> > contrib/plugins/hotblocks.c | 1 +
> > contrib/plugins/hotpages.c | 1 +
> > contrib/plugins/howvec.c | 1 +
> > contrib/plugins/lockstep.c | 1 +
> > include/qemu/qemu-plugin.h | 197 +++++++++++++++++++++++++++--------
> > meson.build | 6 +-
> > plugins/api.c | 62 +++++------
> > plugins/core.c | 10 +-
> > plugins/loader.c | 50 ++++++++-
> > plugins/meson.build | 10 +-
> > plugins/plugin.h | 1 +
> > plugins/qemu-plugins.symbols | 40 -------
> > tests/plugin/bb.c | 1 +
> > tests/plugin/empty.c | 1 +
> > tests/plugin/insn.c | 1 +
> > tests/plugin/mem.c | 1 +
> > 18 files changed, 251 insertions(+), 205 deletions(-)
> > delete mode 100644 plugins/qemu-plugins.symbols
> >
> > diff --git a/Makefile b/Makefile
> > index 54fc1a9d10..9981dd5209 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -105,7 +105,6 @@ config-host.mak: $(SRC_PATH)/configure $(SRC_PATH)/pc-bios $(SRC_PATH)/VERSION
> >
> > # Force configure to re-run if the API symbols are updated
> > ifeq ($(CONFIG_PLUGIN),y)
> > -config-host.mak: $(SRC_PATH)/plugins/qemu-plugins.symbols
> >
> > .PHONY: plugins
> > plugins:
> > diff --git a/configure b/configure
> > index 1c21a73c3b..ea447919fc 100755
> > --- a/configure
> > +++ b/configure
> > @@ -5435,61 +5435,6 @@ if compile_prog "" "" ; then
> > atomic64=yes
> > fi
> >
> > -#########################################
> > -# See if --dynamic-list is supported by the linker
> > -ld_dynamic_list="no"
> > -if test "$static" = "no" ; then
> > - cat > $TMPTXT <<EOF
> > -{
> > - foo;
> > -};
> > -EOF
> > -
> > - cat > $TMPC <<EOF
> > -#include <stdio.h>
> > -void foo(void);
> > -
> > -void foo(void)
> > -{
> > - printf("foo\n");
> > -}
> > -
> > -int main(void)
> > -{
> > - foo();
> > - return 0;
> > -}
> > -EOF
> > -
> > - if compile_prog "" "-Wl,--dynamic-list=$TMPTXT" ; then
> > - ld_dynamic_list="yes"
> > - fi
> > -fi
> > -
> > -#########################################
> > -# See if -exported_symbols_list is supported by the linker
> > -
> > -ld_exported_symbols_list="no"
> > -if test "$static" = "no" ; then
> > - cat > $TMPTXT <<EOF
> > - _foo
> > -EOF
> > -
> > - if compile_prog "" "-Wl,-exported_symbols_list,$TMPTXT" ; then
> > - ld_exported_symbols_list="yes"
> > - fi
> > -fi
> > -
> > -if test "$plugins" = "yes" &&
> > - test "$ld_dynamic_list" = "no" &&
> > - test "$ld_exported_symbols_list" = "no" ; then
> > - error_exit \
> > - "Plugin support requires dynamic linking and specifying a set of symbols " \
> > - "that are exported to plugins. Unfortunately your linker doesn't " \
> > - "support the flag (--dynamic-list or -exported_symbols_list) used " \
> > - "for this purpose. You can't build with --static."
> > -fi
> > -
> > ########################################
> > # See if __attribute__((alias)) is supported.
> > # This false for Xcode 9, but has been remedied for Xcode 10.
> > @@ -7074,22 +7019,6 @@ fi
> >
> > if test "$plugins" = "yes" ; then
> > echo "CONFIG_PLUGIN=y" >> $config_host_mak
> > - # Copy the export object list to the build dir
> > - if test "$ld_dynamic_list" = "yes" ; then
> > - echo "CONFIG_HAS_LD_DYNAMIC_LIST=yes" >> $config_host_mak
> > - ld_symbols=qemu-plugins-ld.symbols
> > - cp "$source_path/plugins/qemu-plugins.symbols" $ld_symbols
> > - elif test "$ld_exported_symbols_list" = "yes" ; then
> > - echo "CONFIG_HAS_LD_EXPORTED_SYMBOLS_LIST=yes" >> $config_host_mak
> > - ld64_symbols=qemu-plugins-ld64.symbols
> > - echo "# Automatically generated by configure - do not modify" > $ld64_symbols
> > - grep 'qemu_' "$source_path/plugins/qemu-plugins.symbols" | sed 's/;//g' | \
> > - sed -E 's/^[[:space:]]*(.*)/_\1/' >> $ld64_symbols
> > - else
> > - error_exit \
> > - "If \$plugins=yes, either \$ld_dynamic_list or " \
> > - "\$ld_exported_symbols_list should have been set to 'yes'."
> > - fi
> > fi
> >
> > if test -n "$gdb_bin" ; then
> > diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
> > index 37435a3fc7..39e77d2980 100644
> > --- a/contrib/plugins/hotblocks.c
> > +++ b/contrib/plugins/hotblocks.c
> > @@ -13,6 +13,7 @@
> > #include <stdio.h>
> > #include <glib.h>
> >
> > +#define QEMU_PLUGIN_IMPLEMENTATION
> > #include <qemu-plugin.h>
>
> As mentioned in earlier patch we should be able to just have the tweak
> in api.c and avoid touching all the plugins themselves.
> >
> > -#define QEMU_PLUGIN_VERSION 0
> > +#define QEMU_PLUGIN_VERSION 1
> > +
> > +typedef void *(*qemu_plugin_global_dlsym_t)(void* context, const char *name);
> >
> > typedef struct {
> > /* string describing architecture */
> > @@ -73,8 +71,23 @@ typedef struct {
> > int max_vcpus;
> > } system;
> > };
> > + void *context;
> > + qemu_plugin_global_dlsym_t dlsym;
> > } qemu_info_t;
> >
> > +/**
> > + * qemu_plugin_initialize() - Initialize a plugin before install
> > + * @info: a block describing some details about the guest
> > + *
> > + * All plugins must export this symbol, and in most case using qemu-plugin.h
> > + * provided implementation directly.
> > + * For plugin provide this function, the QEMU_PLUGIN_VERSION should >= 1
> > + *
> > + * Note: This function only used to loading qemu's exported functions, nothing
> > + * else should doding in this function.
> > + */
> > +QEMU_PLUGIN_EXPORT int qemu_plugin_initialize(const qemu_info_t *info);
> > +
>
> So this is essentially working around the linker/dlopen stage and
> manually linking in all the API functions? Does this affect the
> efficiency of the API calls?
> > -void qemu_plugin_outs(const char *string);
> > +typedef void (*qemu_plugin_outs_t)(const char *string);
> > +
> > +#if !defined(QEMU_PLUGIN_API_IMPLEMENTATION)
> > +#if defined(QEMU_PLUGIN_IMPLEMENTATION)
> > +#define QEMU_PLUGIN_EXTERN
> > +#else
> > +#define QEMU_PLUGIN_EXTERN extern
> > +#endif
>
> As mentioned in the earlier patch I want to understand why the extern is
> required. Could we avoid it with a parameter to the compiler when
> building plugins?
Hi, I've publisehd with v5 of the patch and explain that,
If we only have a single .c file in a plugin, then define
QEMU_PLUGIN_EXTERN to empty is OK, but if we have multiple .c files
in a plugin, then we need distinguish the implementation and the
deceleration. only the main .c file should define the macro QEMU_PLUGIN_IMPLEMENTATION
other sources are user and should use extern
>
> <snip>
> >
> > static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info)
> > {
> > + qemu_plugin_initialize_func_t initialize = NULL;
> > qemu_plugin_install_func_t install;
> > struct qemu_plugin_ctx *ctx;
> > gpointer sym;
> > int rc;
> > + int version = -1;
> >
> > ctx = qemu_memalign(qemu_dcache_linesize, sizeof(*ctx));
> > memset(ctx, 0, sizeof(*ctx));
> > @@ -184,7 +187,7 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info)
> > desc->path, g_module_error());
> > goto err_symbol;
> > } else {
> > - int version = *(int *)sym;
> > + version = *(int *)sym;
> > if (version < QEMU_PLUGIN_MIN_VERSION) {
> > error_report("TCG plugin %s requires API version %d, but "
> > "this QEMU supports only a minimum version of %d",
> > @@ -198,6 +201,21 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info)
> > }
> > }
> >
> > + if (version >= QEMU_PLUGIN_VERSION_1) {
> > + /* This version should call to qemu_plugin_initialize first */
> > + if (!g_module_symbol(ctx->handle, "qemu_plugin_initialize", &sym)) {
> > + error_report("%s: %s", __func__, g_module_error());
> > + goto err_symbol;
> > + }
> > + initialize = (qemu_plugin_initialize_func_t) sym;
> > + /* symbol was found; it could be NULL though */
> > + if (initialize == NULL) {
> > + error_report("%s: %s: qemu_plugin_initialize is NULL",
> > + __func__, desc->path);
> > + goto err_symbol;
> > + }
> > + }
> > +
> > qemu_rec_mutex_lock(&plugin.lock);
> >
> > /* find an unused random id with &ctx as the seed */
> > @@ -216,6 +234,16 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info)
> > }
> > }
> > QTAILQ_INSERT_TAIL(&plugin.ctxs, ctx, entry);
> > + if (initialize != NULL) {
> > + rc = initialize(info);
> > + if (rc) {
> > + error_report("%s: qemu_plugin_initialize returned error code %d",
> > + __func__, rc);
> > + /* qemu_plugin_initialize only loading function symbols */
> > + goto err_symbol;
> > + }
> > + }
> > +
> > ctx->installing = true;
> > rc = install(ctx->id, info, desc->argc, desc->argv);
> > ctx->installing = false;
> > @@ -254,6 +282,17 @@ static void plugin_desc_free(struct qemu_plugin_desc *desc)
> > g_free(desc);
> > }
> >
> > +static void *qemu_plugin_global_dlsym(void* context, const char *name)
> > +{
> > + GModule *global_handle = context;
> > + gpointer sym = NULL;
> > + if (!g_module_symbol(global_handle, name, &sym)) {
> > + error_report("%s: %s", __func__, g_module_error());
> > + return NULL;
> > + }
> > + return sym;
> > +}
> > +
> > /**
> > * qemu_plugin_load_list - load a list of plugins
> > * @head: head of the list of descriptors of the plugins to be loaded
> > @@ -267,6 +306,7 @@ int qemu_plugin_load_list(QemuPluginList *head)
> > {
> > struct qemu_plugin_desc *desc, *next;
> > g_autofree qemu_info_t *info = g_new0(qemu_info_t, 1);
> > + GModule *global_handle = NULL;
> >
> > info->target_name = TARGET_NAME;
> > info->version.min = QEMU_PLUGIN_MIN_VERSION;
> > @@ -276,6 +316,12 @@ int qemu_plugin_load_list(QemuPluginList *head)
> > info->system_emulation = true;
> > info->system.smp_vcpus = ms->smp.cpus;
> > info->system.max_vcpus = ms->smp.max_cpus;
> > + global_handle = g_module_open(NULL, G_MODULE_BIND_LOCAL);
> > + if (global_handle == NULL) {
> > + goto err_dlopen;
> > + }
> > + info->dlsym = qemu_plugin_global_dlsym;
> > + info->context = (void*)global_handle;
> > #else
> > info->system_emulation = false;
> > #endif
> > @@ -289,6 +335,8 @@ int qemu_plugin_load_list(QemuPluginList *head)
> > }
> > QTAILQ_REMOVE(head, desc, entry);
> > }
> > +
> > +err_dlopen:
> > return 0;
>
> This doesn't compile cleanly for both linux-user and softmmu:
>
> Compiling C object libqemu-aarch64-linux-user.fa.p/tcg_tcg-common.c.o
> ../../plugins/loader.c: In function ‘qemu_plugin_load_list’:
> ../../plugins/loader.c:339:1: error: label ‘err_dlopen’ defined but not used [-Werror=unused-label]
> err_dlopen:
> ^~~~~~~~~~
> ../../plugins/loader.c:309:14: error: unused variable ‘global_handle’ [-Werror=unused-variable]
> GModule *global_handle = NULL;
> ^~~~~~~~~~~~~
> At top level:
> ../../plugins/loader.c:285:14: error: ‘qemu_plugin_global_dlsym’ defined but not used [-Werror=unused-function]
> static void *qemu_plugin_global_dlsym(void* context, const char *name)
> ^~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make: *** [Makefile.ninja:6703: libqemu-aarch64-linux-user.fa.p/plugins_loader.c.o] Error 1
> make: *** Waiting for unfinished jobs....
>
> --
> Alex Bennée
--
此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo