QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] tcg plugins: expose an API version concept
@ 2019-11-04 13:18 Alex Bennée
  2019-11-04 16:23 ` Philippe Mathieu-Daudé
  2019-11-08 14:43 ` Peter Maydell
  0 siblings, 2 replies; 4+ messages in thread
From: Alex Bennée @ 2019-11-04 13:18 UTC (permalink / raw)
  To: qemu-devel; +Cc: peter.maydell, cota, Alex Bennée

This is a very simple versioning API which allows the plugin
infrastructure to check the API a plugin was built against. We also
expose a min/cur API version to the plugin via the info block in case
it wants to avoid using old deprecated APIs in the future.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/qemu-plugin.h | 19 +++++++++++++++++++
 plugins/loader.c           | 15 +++++++++++++++
 plugins/plugin.h           |  2 ++
 tests/plugin/bb.c          |  2 ++
 tests/plugin/empty.c       |  2 ++
 tests/plugin/hotpages.c    |  2 ++
 tests/plugin/howvec.c      |  2 ++
 tests/plugin/insn.c        |  2 ++
 tests/plugin/mem.c         |  2 ++
 9 files changed, 48 insertions(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index a00a7deb461..5502e112c81 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -38,9 +38,28 @@
 
 typedef uint64_t qemu_plugin_id_t;
 
+/*
+ * Versioning plugins:
+ *
+ * The plugin API will pass a minimum and current API version that
+ * QEMU currently supports. The minimum API will be incremented if an
+ * API needs to be deprecated.
+ *
+ * The plugins export the API they were built against by exposing the
+ * symbol qemu_plugin_version which can be checked.
+ */
+
+extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
+
+#define QEMU_PLUGIN_VERSION 0
+
 typedef struct {
     /* string describing architecture */
     const char *target_name;
+    struct {
+        int min;
+        int cur;
+    } version;
     /* is this a full system emulation? */
     bool system_emulation;
     union {
diff --git a/plugins/loader.c b/plugins/loader.c
index ce724ed5839..1bcca909691 100644
--- a/plugins/loader.c
+++ b/plugins/loader.c
@@ -178,6 +178,19 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info)
         goto err_symbol;
     }
 
+    if (!g_module_symbol(ctx->handle, "qemu_plugin_version", &sym)) {
+        warn_report("%s: missing version %s", __func__, g_module_error());
+    } else {
+        int version = *(int *)sym;
+        if (version < QEMU_PLUGIN_MIN_VERSION ||
+            version > QEMU_PLUGIN_VERSION) {
+            error_report("%s: bad plugin version %d vs %d/%d",
+                         __func__, version, QEMU_PLUGIN_MIN_VERSION,
+                         QEMU_PLUGIN_VERSION);
+            goto err_symbol;
+        }
+    }
+
     qemu_rec_mutex_lock(&plugin.lock);
 
     /* find an unused random id with &ctx as the seed */
@@ -248,6 +261,8 @@ int qemu_plugin_load_list(QemuPluginList *head)
     g_autofree qemu_info_t *info = g_new0(qemu_info_t, 1);
 
     info->target_name = TARGET_NAME;
+    info->version.min = QEMU_PLUGIN_MIN_VERSION;
+    info->version.cur = QEMU_PLUGIN_VERSION;
 #ifndef CONFIG_USER_ONLY
     MachineState *ms = MACHINE(qdev_get_machine());
     info->system_emulation = true;
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 5482168d797..1aa29dcaddf 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -14,6 +14,8 @@
 
 #include <gmodule.h>
 
+#define QEMU_PLUGIN_MIN_VERSION 0
+
 /* global state */
 struct qemu_plugin_state {
     QTAILQ_HEAD(, qemu_plugin_ctx) ctxs;
diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
index 45e1de5bd68..f30bea08dcc 100644
--- a/tests/plugin/bb.c
+++ b/tests/plugin/bb.c
@@ -14,6 +14,8 @@
 
 #include <qemu-plugin.h>
 
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
 static uint64_t bb_count;
 static uint64_t insn_count;
 static bool do_inline;
diff --git a/tests/plugin/empty.c b/tests/plugin/empty.c
index 3f60f690278..8fa6bacd93d 100644
--- a/tests/plugin/empty.c
+++ b/tests/plugin/empty.c
@@ -13,6 +13,8 @@
 
 #include <qemu-plugin.h>
 
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
 /*
  * Empty TB translation callback.
  * This allows us to measure the overhead of injecting and then
diff --git a/tests/plugin/hotpages.c b/tests/plugin/hotpages.c
index 77df07a3ccf..ecd6c187327 100644
--- a/tests/plugin/hotpages.c
+++ b/tests/plugin/hotpages.c
@@ -18,6 +18,8 @@
 
 #include <qemu-plugin.h>
 
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 
 static uint64_t page_size = 4096;
diff --git a/tests/plugin/howvec.c b/tests/plugin/howvec.c
index 58fa675e348..4ca555e1239 100644
--- a/tests/plugin/howvec.c
+++ b/tests/plugin/howvec.c
@@ -20,6 +20,8 @@
 
 #include <qemu-plugin.h>
 
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 
 typedef enum {
diff --git a/tests/plugin/insn.c b/tests/plugin/insn.c
index e5fd07fb64b..0a8f5a0000e 100644
--- a/tests/plugin/insn.c
+++ b/tests/plugin/insn.c
@@ -14,6 +14,8 @@
 
 #include <qemu-plugin.h>
 
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
 static uint64_t insn_count;
 static bool do_inline;
 
diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
index d9673889896..878abf09d19 100644
--- a/tests/plugin/mem.c
+++ b/tests/plugin/mem.c
@@ -14,6 +14,8 @@
 
 #include <qemu-plugin.h>
 
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
 static uint64_t mem_count;
 static uint64_t io_count;
 static bool do_inline;
-- 
2.20.1



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] tcg plugins: expose an API version concept
  2019-11-04 13:18 [PATCH] tcg plugins: expose an API version concept Alex Bennée
@ 2019-11-04 16:23 ` Philippe Mathieu-Daudé
  2019-11-08 14:43 ` Peter Maydell
  1 sibling, 0 replies; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-04 16:23 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel; +Cc: peter.maydell, cota

On 11/4/19 2:18 PM, Alex Bennée wrote:
> This is a very simple versioning API which allows the plugin
> infrastructure to check the API a plugin was built against. We also
> expose a min/cur API version to the plugin via the info block in case
> it wants to avoid using old deprecated APIs in the future.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/qemu/qemu-plugin.h | 19 +++++++++++++++++++
>   plugins/loader.c           | 15 +++++++++++++++
>   plugins/plugin.h           |  2 ++
>   tests/plugin/bb.c          |  2 ++
>   tests/plugin/empty.c       |  2 ++
>   tests/plugin/hotpages.c    |  2 ++
>   tests/plugin/howvec.c      |  2 ++
>   tests/plugin/insn.c        |  2 ++
>   tests/plugin/mem.c         |  2 ++
>   9 files changed, 48 insertions(+)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index a00a7deb461..5502e112c81 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -38,9 +38,28 @@
>   
>   typedef uint64_t qemu_plugin_id_t;
>   
> +/*
> + * Versioning plugins:
> + *
> + * The plugin API will pass a minimum and current API version that
> + * QEMU currently supports. The minimum API will be incremented if an
> + * API needs to be deprecated.
> + *
> + * The plugins export the API they were built against by exposing the
> + * symbol qemu_plugin_version which can be checked.
> + */
> +
> +extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
> +
> +#define QEMU_PLUGIN_VERSION 0
> +
>   typedef struct {
>       /* string describing architecture */
>       const char *target_name;
> +    struct {
> +        int min;
> +        int cur;
> +    } version;
>       /* is this a full system emulation? */
>       bool system_emulation;
>       union {
> diff --git a/plugins/loader.c b/plugins/loader.c
> index ce724ed5839..1bcca909691 100644
> --- a/plugins/loader.c
> +++ b/plugins/loader.c
> @@ -178,6 +178,19 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info)
>           goto err_symbol;
>       }
>   
> +    if (!g_module_symbol(ctx->handle, "qemu_plugin_version", &sym)) {
> +        warn_report("%s: missing version %s", __func__, g_module_error());
> +    } else {
> +        int version = *(int *)sym;
> +        if (version < QEMU_PLUGIN_MIN_VERSION ||
> +            version > QEMU_PLUGIN_VERSION) {
> +            error_report("%s: bad plugin version %d vs %d/%d",
> +                         __func__, version, QEMU_PLUGIN_MIN_VERSION,
> +                         QEMU_PLUGIN_VERSION);
> +            goto err_symbol;
> +        }
> +    }
> +
>       qemu_rec_mutex_lock(&plugin.lock);
>   
>       /* find an unused random id with &ctx as the seed */
> @@ -248,6 +261,8 @@ int qemu_plugin_load_list(QemuPluginList *head)
>       g_autofree qemu_info_t *info = g_new0(qemu_info_t, 1);
>   
>       info->target_name = TARGET_NAME;
> +    info->version.min = QEMU_PLUGIN_MIN_VERSION;
> +    info->version.cur = QEMU_PLUGIN_VERSION;
>   #ifndef CONFIG_USER_ONLY
>       MachineState *ms = MACHINE(qdev_get_machine());
>       info->system_emulation = true;
> diff --git a/plugins/plugin.h b/plugins/plugin.h
> index 5482168d797..1aa29dcaddf 100644
> --- a/plugins/plugin.h
> +++ b/plugins/plugin.h
> @@ -14,6 +14,8 @@
>   
>   #include <gmodule.h>
>   
> +#define QEMU_PLUGIN_MIN_VERSION 0
> +
>   /* global state */
>   struct qemu_plugin_state {
>       QTAILQ_HEAD(, qemu_plugin_ctx) ctxs;
> diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
> index 45e1de5bd68..f30bea08dcc 100644
> --- a/tests/plugin/bb.c
> +++ b/tests/plugin/bb.c
> @@ -14,6 +14,8 @@
>   
>   #include <qemu-plugin.h>
>   
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
> +
>   static uint64_t bb_count;
>   static uint64_t insn_count;
>   static bool do_inline;
> diff --git a/tests/plugin/empty.c b/tests/plugin/empty.c
> index 3f60f690278..8fa6bacd93d 100644
> --- a/tests/plugin/empty.c
> +++ b/tests/plugin/empty.c
> @@ -13,6 +13,8 @@
>   
>   #include <qemu-plugin.h>
>   
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
> +
>   /*
>    * Empty TB translation callback.
>    * This allows us to measure the overhead of injecting and then
> diff --git a/tests/plugin/hotpages.c b/tests/plugin/hotpages.c
> index 77df07a3ccf..ecd6c187327 100644
> --- a/tests/plugin/hotpages.c
> +++ b/tests/plugin/hotpages.c
> @@ -18,6 +18,8 @@
>   
>   #include <qemu-plugin.h>
>   
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
> +
>   #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>   
>   static uint64_t page_size = 4096;
> diff --git a/tests/plugin/howvec.c b/tests/plugin/howvec.c
> index 58fa675e348..4ca555e1239 100644
> --- a/tests/plugin/howvec.c
> +++ b/tests/plugin/howvec.c
> @@ -20,6 +20,8 @@
>   
>   #include <qemu-plugin.h>
>   
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
> +
>   #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>   
>   typedef enum {
> diff --git a/tests/plugin/insn.c b/tests/plugin/insn.c
> index e5fd07fb64b..0a8f5a0000e 100644
> --- a/tests/plugin/insn.c
> +++ b/tests/plugin/insn.c
> @@ -14,6 +14,8 @@
>   
>   #include <qemu-plugin.h>
>   
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
> +
>   static uint64_t insn_count;
>   static bool do_inline;
>   
> diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
> index d9673889896..878abf09d19 100644
> --- a/tests/plugin/mem.c
> +++ b/tests/plugin/mem.c
> @@ -14,6 +14,8 @@
>   
>   #include <qemu-plugin.h>
>   
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
> +
>   static uint64_t mem_count;
>   static uint64_t io_count;
>   static bool do_inline;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] tcg plugins: expose an API version concept
  2019-11-04 13:18 [PATCH] tcg plugins: expose an API version concept Alex Bennée
  2019-11-04 16:23 ` Philippe Mathieu-Daudé
@ 2019-11-08 14:43 ` Peter Maydell
  2019-11-08 16:24   ` Alex Bennée
  1 sibling, 1 reply; 4+ messages in thread
From: Peter Maydell @ 2019-11-08 14:43 UTC (permalink / raw)
  To: Alex Bennée; +Cc: Emilio G. Cota, QEMU Developers

On Mon, 4 Nov 2019 at 13:18, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> This is a very simple versioning API which allows the plugin
> infrastructure to check the API a plugin was built against. We also
> expose a min/cur API version to the plugin via the info block in case
> it wants to avoid using old deprecated APIs in the future.

I think the general idea here is fine, and I want to see
us get the version-check into 4.2, because I don't think
we should release a QEMU which doesn't do a basic version
sanity check. I have some minor comments below.


> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/qemu/qemu-plugin.h | 19 +++++++++++++++++++
>  plugins/loader.c           | 15 +++++++++++++++
>  plugins/plugin.h           |  2 ++
>  tests/plugin/bb.c          |  2 ++
>  tests/plugin/empty.c       |  2 ++
>  tests/plugin/hotpages.c    |  2 ++
>  tests/plugin/howvec.c      |  2 ++
>  tests/plugin/insn.c        |  2 ++
>  tests/plugin/mem.c         |  2 ++
>  9 files changed, 48 insertions(+)
>
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index a00a7deb461..5502e112c81 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h

As an aside, is this the header file used for building
the plugins? It seems a bit odd to have put it
in include/qemu, which is for headers used when building
QEMU itself.

> @@ -38,9 +38,28 @@
>
>  typedef uint64_t qemu_plugin_id_t;
>
> +/*
> + * Versioning plugins:
> + *
> + * The plugin API will pass a minimum and current API version that
> + * QEMU currently supports. The minimum API will be incremented if an
> + * API needs to be deprecated.
> + *
> + * The plugins export the API they were built against by exposing the
> + * symbol qemu_plugin_version which can be checked.
> + */
> +
> +extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
> +
> +#define QEMU_PLUGIN_VERSION 0
> +
>  typedef struct {
>      /* string describing architecture */
>      const char *target_name;
> +    struct {
> +        int min;
> +        int cur;
> +    } version;
>      /* is this a full system emulation? */
>      bool system_emulation;
>      union {
> diff --git a/plugins/loader.c b/plugins/loader.c
> index ce724ed5839..1bcca909691 100644
> --- a/plugins/loader.c
> +++ b/plugins/loader.c
> @@ -178,6 +178,19 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info)
>          goto err_symbol;
>      }
>
> +    if (!g_module_symbol(ctx->handle, "qemu_plugin_version", &sym)) {
> +        warn_report("%s: missing version %s", __func__, g_module_error());

Failure to declare the required version should be an error,
not just a warning.

> +    } else {
> +        int version = *(int *)sym;
> +        if (version < QEMU_PLUGIN_MIN_VERSION ||
> +            version > QEMU_PLUGIN_VERSION) {
> +            error_report("%s: bad plugin version %d vs %d/%d",
> +                         __func__, version, QEMU_PLUGIN_MIN_VERSION,
> +                         QEMU_PLUGIN_VERSION);

I think this message is too cryptic, and would prefer
something more like:
 "TCG plugin %s requires API version %d, but this QEMU supports only
versions %d to %d"

where the first %s is the name of the plugin binary, not
the __func__ of this function (which is not useful to
the end user).

Better still, special case the MIN_VERSION == VERSION
case to avoid saying "versions 1 to 1".

> +            goto err_symbol;
> +        }
> +    }
> +
>      qemu_rec_mutex_lock(&plugin.lock);
>
>      /* find an unused random id with &ctx as the seed */
> @@ -248,6 +261,8 @@ int qemu_plugin_load_list(QemuPluginList *head)
>      g_autofree qemu_info_t *info = g_new0(qemu_info_t, 1);
>
>      info->target_name = TARGET_NAME;
> +    info->version.min = QEMU_PLUGIN_MIN_VERSION;
> +    info->version.cur = QEMU_PLUGIN_VERSION;
>  #ifndef CONFIG_USER_ONLY
>      MachineState *ms = MACHINE(qdev_get_machine());
>      info->system_emulation = true;

thanks
-- PMM


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] tcg plugins: expose an API version concept
  2019-11-08 14:43 ` Peter Maydell
@ 2019-11-08 16:24   ` Alex Bennée
  0 siblings, 0 replies; 4+ messages in thread
From: Alex Bennée @ 2019-11-08 16:24 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Emilio G. Cota, QEMU Developers


Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 4 Nov 2019 at 13:18, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> This is a very simple versioning API which allows the plugin
>> infrastructure to check the API a plugin was built against. We also
>> expose a min/cur API version to the plugin via the info block in case
>> it wants to avoid using old deprecated APIs in the future.
>
> I think the general idea here is fine, and I want to see
> us get the version-check into 4.2, because I don't think
> we should release a QEMU which doesn't do a basic version
> sanity check. I have some minor comments below.
>
>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  include/qemu/qemu-plugin.h | 19 +++++++++++++++++++
>>  plugins/loader.c           | 15 +++++++++++++++
>>  plugins/plugin.h           |  2 ++
>>  tests/plugin/bb.c          |  2 ++
>>  tests/plugin/empty.c       |  2 ++
>>  tests/plugin/hotpages.c    |  2 ++
>>  tests/plugin/howvec.c      |  2 ++
>>  tests/plugin/insn.c        |  2 ++
>>  tests/plugin/mem.c         |  2 ++
>>  9 files changed, 48 insertions(+)
>>
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index a00a7deb461..5502e112c81 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>
> As an aside, is this the header file used for building
> the plugins? It seems a bit odd to have put it
> in include/qemu, which is for headers used when building
> QEMU itself.

It's also included by the internal plugin header. We could put it
somewhere else. include/api/? Where do the QMP specs end up?

I'll make the other changes in v2.

--
Alex Bennée


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-04 13:18 [PATCH] tcg plugins: expose an API version concept Alex Bennée
2019-11-04 16:23 ` Philippe Mathieu-Daudé
2019-11-08 14:43 ` Peter Maydell
2019-11-08 16:24   ` Alex Bennée

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git