[PATCH v3] Loading new machines and devices from external modules

Drap Anton posted 1 patch 1 year, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220822085041.127776-1-anton.drap@auriga.com
Maintainers: Markus Armbruster <armbru@redhat.com>, John Snow <jsnow@redhat.com>, Cleber Rosa <crosa@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
include/qapi/qmp/qjson.h    |   2 +
include/qemu/module.h       |   4 +-
qemu-options.hx             |  38 ++++++
qobject/qjson.c             |  14 ++
scripts/modinfo-generate.py |   7 +-
softmmu/vl.c                |  14 ++
util/module.c               | 249 +++++++++++++++++++++++++++++++++++-
7 files changed, 320 insertions(+), 8 deletions(-)
[PATCH v3] Loading new machines and devices from external modules
Posted by Drap Anton 1 year, 8 months ago
From: "Drap, Anton" <anton.drap@auriga.com>

    Current QEMU politics is to have all the supported
platforms inside QEMU source tree, but actually simulator core
development, development of the devices standard library and
development of virtual platforms are three different tasks.
    Moreover different people interested in different parts of QEMU.
QEMU core developers not interested in supporting and maintaining
tons of platforms available on the market. Virtual platform developers
not interested and usually don’t have resources to merge their changes
upstream. So we have a lots of abandoned QEMU forks for different
platforms.
    For example we’re now working on Raspberry Pi 4b implementation for our
internal needs and we’re planning to merge it upstream. It’s based on some
QEMU fork author of which wasn’t able to complete it and commit upstream.
And it can’t be used with later QEMU without some efforts to port it
to newer QEMU version. Nobody supports and maintaining it since constant
efforts necessary to be in sync with QEMU mainline. So my opinion is that
core development, core device library and virtual platform development
should be divided to make life easier for everybody. And this changes is
first step to it.
    About legal reasons and GPL violations. Possibility to make .so
with machine separately and load it without providing sources is
a legal risk and can’t be completely solved with technical actions.
Ban on external modules just makes it more difficult for everybody
to use not upstream code (including GPL violators, but not only for them)
and doesn’t block ability to distribute full QEMU fork with closed models
without providing sources. So I don’t see any reason to make technical
limitations which actually can’t solve legal problem.
    This patch is to add two parameters `add_machine` and `add_modinfo`.
`add_machine` is to add machines from external modules.
`add_modinfo` is to add devices from external modules, needed for a new
machine, for example.
Additional, 'arch' parameter of QemuModinfo is changed to a list.

Signed-off-by: Drap Anton <anton.drap@auriga.com>
---
v2: subject is changed
v3: 
    - description is changed
    - rebased on last master
---
 include/qapi/qmp/qjson.h    |   2 +
 include/qemu/module.h       |   4 +-
 qemu-options.hx             |  38 ++++++
 qobject/qjson.c             |  14 ++
 scripts/modinfo-generate.py |   7 +-
 softmmu/vl.c                |  14 ++
 util/module.c               | 249 +++++++++++++++++++++++++++++++++++-
 7 files changed, 320 insertions(+), 8 deletions(-)

diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index 7bd8d2de1b..bd1a44ce90 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -25,6 +25,8 @@ QDict *qdict_from_vjsonf_nofail(const char *string, va_list ap)
 QDict *qdict_from_jsonf_nofail(const char *string, ...)
     G_GNUC_PRINTF(1, 2);
 
+QDict *qdict_from_json_nofail_nofmt(const char *string);
+
 GString *qobject_to_json(const QObject *obj);
 GString *qobject_to_json_pretty(const QObject *obj, bool pretty);
 
diff --git a/include/qemu/module.h b/include/qemu/module.h
index bd73607104..cdd3a3ceef 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -73,6 +73,8 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail);
 void module_load_qom_one(const char *type);
 void module_load_qom_all(void);
 void module_allow_arch(const char *arch);
+bool load_external_modules(const char *mods_list);
+bool add_modinfo(const char *filename);
 
 /**
  * DOC: module info annotation macros
@@ -154,7 +156,7 @@ void module_allow_arch(const char *arch);
 typedef struct QemuModinfo QemuModinfo;
 struct QemuModinfo {
     const char *name;
-    const char *arch;
+    const char **arch;
     const char **objs;
     const char **deps;
     const char **opts;
diff --git a/qemu-options.hx b/qemu-options.hx
index 3f23a42fa8..4913fdd775 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -5540,6 +5540,44 @@ SRST
             (qemu) qom-set /objects/iothread1 poll-max-ns 100000
 ERST
 
+DEF("add_machines", HAS_ARG, QEMU_OPTION_add_machines, \
+    "-add_machines libname[,...]\n",
+    QEMU_ARCH_ALL)
+SRST
+``-add_machines libname[,...]``
+    Add machines from external modules.
+    For example:
+
+    ::
+
+        -add_machines custom-arm-machine,custom-arm-machine2
+ERST
+
+DEF("add_modinfo", HAS_ARG, QEMU_OPTION_add_modinfo, \
+    "-add_modinfo filename\n",
+    QEMU_ARCH_ALL)
+SRST
+``-add_modinfo filename``
+    Extend modinfo from file. Used to add devices from external modules.
+    Modinfo extention file is a JSON file with dictionary of modules:
+    {
+    "short name of module": {"name": "module-name",
+                             "arch": ["supported_arch_1", "supported_arch_2",],
+                             "objs": ["object1_description", "object2_description",],
+                             "deps": ["depend_of_module_name1", "depend_of_module_name2",],
+                             "opts": ["option1", "option2",]
+                            }
+    }
+
+    Architectures should be designated as they are printed by ./configure --help in target list
+    without "-softmmu" or "-linux-user" suffixes. e.g.: "arm", "x86_64", "riscv32", etc.
+
+    For example:
+
+    ::
+
+        -add_modinfo modinfo_extention.json
+ERST
 
 HXCOMM This is the last statement. Insert new options before this line!
 
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 167fcb429c..6045264594 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -148,6 +148,20 @@ QDict *qdict_from_jsonf_nofail(const char *string, ...)
     return qdict;
 }
 
+/*
+ * Parse @string as JSON object without %-escapes interpolated.
+ * Abort on error.  Do not use with untrusted @string.
+ * Return the resulting QDict.  It is never null.
+ */
+QDict *qdict_from_json_nofail_nofmt(const char *string)
+{
+    QDict *qdict;
+    qdict = qobject_to(QDict, qobject_from_json(string, &error_abort));
+    assert(qdict);
+    return qdict;
+}
+
+
 static void to_json(JSONWriter *writer, const char *name,
                     const QObject *obj)
 {
diff --git a/scripts/modinfo-generate.py b/scripts/modinfo-generate.py
index b1538fcced..ddfe444fea 100755
--- a/scripts/modinfo-generate.py
+++ b/scripts/modinfo-generate.py
@@ -33,7 +33,7 @@ def parse_line(line):
     return (kind, data)
 
 def generate(name, lines, enabled):
-    arch = ""
+    arch = []
     objs = []
     deps = []
     opts = []
@@ -47,7 +47,7 @@ def generate(name, lines, enabled):
             elif kind == 'opts':
                 opts.append(data)
             elif kind == 'arch':
-                arch = data;
+                arch.append(data)
             elif kind == 'kconfig':
                 # don't add a module which dependency is not enabled
                 # in kconfig
@@ -61,8 +61,7 @@ def generate(name, lines, enabled):
                 exit(1)
 
     print("    .name = \"%s\"," % name)
-    if arch != "":
-        print("    .arch = %s," % arch)
+    print_array("arch", arch)
     print_array("objs", objs)
     print_array("deps", deps)
     print_array("opts", opts)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 706bd7cff7..9081d5bd8b 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -3460,6 +3460,20 @@ void qemu_init(int argc, char **argv, char **envp)
             case QEMU_OPTION_enable_sync_profile:
                 qsp_enable();
                 break;
+            case QEMU_OPTION_add_machines:
+                info_report("External machines loading: %s", optarg);
+                if (!load_external_modules(optarg)) {
+                    error_report("Modules loading error. Modules %s", optarg);
+                    exit(1);
+                }
+                break;
+            case QEMU_OPTION_add_modinfo:
+                info_report("Modinfo parsing: %s", optarg);
+                if (!add_modinfo(optarg)) {
+                    error_report("Modinfo (%s) adding error", optarg);
+                    exit(1);
+                }
+                break;
             case QEMU_OPTION_nouserconfig:
                 /* Nothing to be parsed here. Especially, do not error out below. */
                 break;
diff --git a/util/module.c b/util/module.c
index 8ddb0e18f5..c2eec5d1c3 100644
--- a/util/module.c
+++ b/util/module.c
@@ -21,6 +21,10 @@
 #include "qemu/module.h"
 #include "qemu/cutils.h"
 #include "qemu/config-file.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qstring.h"
 #ifdef CONFIG_MODULE_UPGRADES
 #include "qemu-version.h"
 #endif
@@ -124,6 +128,7 @@ void module_init_info(const QemuModinfo *info)
     module_info = info;
 }
 
+
 void module_allow_arch(const char *arch)
 {
     module_arch = arch;
@@ -136,10 +141,20 @@ static bool module_check_arch(const QemuModinfo *modinfo)
             /* no arch set -> ignore all */
             return false;
         }
-        if (strcmp(module_arch, modinfo->arch) != 0) {
-            /* mismatch */
-            return false;
+
+        const char **arch_list = modinfo->arch;
+        const char *arch;
+
+        while ((arch = *(arch_list++))) {
+
+            if (strcmp(module_arch, arch) == 0) {
+                return true;
+            }
         }
+
+        /* modinfo->arch is not empty but no match found */
+        /* current arch is not supported */
+        return false;
     }
     return true;
 }
@@ -314,6 +329,32 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
 
 static bool module_loaded_qom_all;
 
+static void modinfo_prepend(QemuModinfo **modinfo, uint32_t mod_count,
+                     const QemuModinfo *modinfo_ext) {
+    const QemuModinfo *pinfo;
+    uint32_t mod_count_new;
+    uint32_t mod_count_ext = 0;
+    uint32_t i;
+
+    for (pinfo = modinfo_ext; pinfo->name != NULL; ++pinfo) {
+        ++mod_count_ext;
+    }
+
+    /* 1 for end of list */
+    mod_count_new = mod_count + mod_count_ext + 1;
+    *modinfo = g_realloc(*modinfo, mod_count_new * sizeof(**modinfo));
+    memmove((*modinfo) + mod_count_ext,
+            *modinfo,
+            mod_count * sizeof(**modinfo));
+    /* last entry with null name treat as end of array */
+    (*modinfo)[mod_count_new - 1].name = NULL;
+
+    for (pinfo = modinfo_ext, i = 0; pinfo->name != NULL; ++pinfo, ++i) {
+        (*modinfo)[i] = *pinfo;
+    }
+}
+
+
 void module_load_qom_one(const char *type)
 {
     const QemuModinfo *modinfo;
@@ -376,11 +417,213 @@ void qemu_load_module_for_opts(const char *group)
     }
 }
 
+bool load_external_modules(const char *mods_list)
+{
+    bool res = false;
+    g_auto(GStrv) mod_names = NULL;
+
+    mod_names = g_strsplit(mods_list, ",", -1);
+    for (int i = 0; mod_names[i]; ++i) {
+        res = module_load_one("", mod_names[i], false);
+        if (!res) {
+            error_report("Module %s not found", mod_names[i]);
+            break;
+        }
+        info_report("Module %s loaded", mod_names[i]);
+    }
+
+    return res;
+}
+
+bool add_modinfo(const char *filename)
+{
+    g_autofree char *buf = NULL;
+    gsize buflen;
+    GError *gerr = NULL;
+    QDict *modinfo_dict;
+    QList *arch;
+    QList *objs;
+    QList *deps;
+    QList *opts;
+    const QDictEntry *entry;
+    uint32_t i = 0;
+    uint32_t mod_count = 0;
+    QemuModinfo *modinfo_ext;
+
+    if (!g_file_get_contents(filename, &buf, &buflen, &gerr)) {
+        fprintf(stderr, "Cannot open modinfo extension file %s: %s\n",
+                filename, gerr->message);
+        g_error_free(gerr);
+        return false;
+    }
+
+    modinfo_dict = qdict_from_json_nofail_nofmt(buf);
+
+    if (!modinfo_dict) {
+        fprintf(stderr, "Invalid modinfo (%s) format: parsing json error\n",
+                filename);
+        g_error_free(gerr);
+        return false;
+    }
+
+    for (entry = qdict_first(modinfo_dict); entry;
+         entry = qdict_next(modinfo_dict, entry)) {
+        mod_count++;
+    }
+    if (mod_count == 0) {
+        return true;
+    }
+
+    modinfo_ext = g_malloc0(sizeof(*modinfo_ext) * (mod_count + 1));
+    /* last entry with null name treat as end of array */
+    modinfo_ext[mod_count].name = NULL;
+
+    for (entry = qdict_first(modinfo_dict), i = 0; entry;
+         entry = qdict_next(modinfo_dict, entry), ++i) {
+
+        QListEntry *qlist_entry;
+        QDict *module_dict;
+        QemuModinfo *modinfo;
+        size_t list_size;
+        uint32_t n = 0;
+
+        if (qobject_type(entry->value) != QTYPE_QDICT) {
+            fprintf(stderr, "Invalid modinfo (%s) format: entry is"
+                    " not dictionary\n", filename);
+            return false;
+        }
+
+        module_dict = qobject_to(QDict, entry->value);
+        modinfo = &modinfo_ext[i];
+
+        modinfo->name = g_strdup(qdict_get_str(module_dict, "name"));
+
+        arch = qdict_get_qlist(module_dict, "arch");
+        if (arch) {
+            n = 0;
+            list_size = qlist_size(arch);
+            modinfo->arch = g_malloc((list_size + 1) * sizeof(*modinfo->arch));
+            modinfo->arch[list_size] = NULL;
+            QLIST_FOREACH_ENTRY(arch, qlist_entry) {
+                if (qobject_type(qlist_entry->value) != QTYPE_QSTRING) {
+                    fprintf(stderr, "Invalid modinfo (%s) format: arch\n\n",
+                            filename);
+                    return false;
+                }
+                QString *qstr = qobject_to(QString, qlist_entry->value);
+                modinfo->arch[n++] = g_strdup(qstring_get_str(qstr));
+            }
+        } else {
+             modinfo->arch = NULL;
+        }
+
+        objs = qdict_get_qlist(module_dict, "objs");
+        if (objs) {
+            n = 0;
+            list_size = qlist_size(objs);
+            modinfo->objs = g_malloc((list_size + 1) * sizeof(*modinfo->objs));
+            modinfo->objs[list_size] = NULL;
+            QLIST_FOREACH_ENTRY(objs, qlist_entry) {
+                if (qobject_type(qlist_entry->value) != QTYPE_QSTRING) {
+                    fprintf(stderr, "Invalid modinfo (%s) format: objs\n\n",
+                            filename);
+                    return false;
+                }
+                QString *qstr = qobject_to(QString, qlist_entry->value);
+                modinfo->objs[n++] = g_strdup(qstring_get_str(qstr));
+            }
+        } else {
+             modinfo->objs = NULL;
+        }
+
+        deps = qdict_get_qlist(module_dict, "deps");
+        if (deps) {
+            n = 0;
+            list_size = qlist_size(deps);
+            modinfo->deps = g_malloc((list_size + 1) * sizeof(*modinfo->deps));
+            modinfo->deps[list_size] = NULL;
+            QLIST_FOREACH_ENTRY(deps, qlist_entry) {
+                if (qobject_type(qlist_entry->value) != QTYPE_QSTRING) {
+                    fprintf(stderr, "Invalid modinfo (%s) format: deps",
+                            filename);
+                    return false;
+                }
+                QString *qstr = qobject_to(QString, qlist_entry->value);
+                modinfo->deps[n++] = g_strdup(qstring_get_str(qstr));
+            }
+        } else {
+             modinfo->deps = NULL;
+        }
+
+        opts = qdict_get_qlist(module_dict, "opts");
+        if (opts) {
+            n = 0;
+            list_size = qlist_size(opts);
+            modinfo->opts = g_malloc((list_size + 1) * sizeof(*modinfo->opts));
+            modinfo->opts[list_size] = NULL;
+            QLIST_FOREACH_ENTRY(opts, qlist_entry) {
+                if (qobject_type(qlist_entry->value) != QTYPE_QSTRING) {
+                    fprintf(stderr, "Invalid modinfo (%s) format: opts\n",
+                            filename);
+                    return false;
+                }
+                QString *qstr = qobject_to(QString, qlist_entry->value);
+                modinfo->opts[n++] = g_strdup(qstring_get_str(qstr));
+            }
+        } else {
+             modinfo->opts = NULL;
+        }
+    }
+
+    qobject_unref(modinfo_dict);
+
+    modinfo_prepend(&modinfo_ext, mod_count, module_info);
+    module_init_info(modinfo_ext);
+    return true;
+}
+
+void modinfo_prepend(QemuModinfo **modinfo, uint32_t mod_count,
+                     const QemuModinfo *modinfo_ext)
+{
+    const QemuModinfo *pinfo;
+    uint32_t mod_count_new;
+    uint32_t mod_count_ext = 0;
+    uint32_t i;
+
+    for (pinfo = modinfo_ext; pinfo->name != NULL; ++pinfo) {
+        ++mod_count_ext;
+    }
+
+    /* 1 for end of list */
+    mod_count_new = mod_count + mod_count_ext + 1;
+    *modinfo = g_realloc(*modinfo, mod_count_new * sizeof(**modinfo));
+    memmove((*modinfo) + mod_count_ext,
+            *modinfo,
+            mod_count * sizeof(**modinfo));
+    /* last entry with null name treat as end of array */
+    (*modinfo)[mod_count_new - 1].name = NULL;
+
+    for (pinfo = modinfo_ext, i = 0; pinfo->name != NULL; ++pinfo, ++i) {
+        (*modinfo)[i] = *pinfo;
+    }
+}
+
+
 #else
 
 void module_allow_arch(const char *arch) {}
 void qemu_load_module_for_opts(const char *group) {}
 void module_load_qom_one(const char *type) {}
 void module_load_qom_all(void) {}
+bool load_external_modules(const char *mods_list)
+{
+    fprintf(stderr, "Modules are not enabled\n");
+    return false;
+}
+bool add_modinfo(const char *filename)
+{
+    fprintf(stderr, "Modules are not enabled\n");
+    return false;
+}
 
 #endif
-- 
2.34.1


Re: [PATCH v3] Loading new machines and devices from external modules
Posted by Peter Maydell 1 year, 8 months ago
On Mon, 22 Aug 2022 at 09:53, Drap Anton <drapas86@gmail.com> wrote:
>
> From: "Drap, Anton" <anton.drap@auriga.com>

>     This patch is to add two parameters `add_machine` and `add_modinfo`.
> `add_machine` is to add machines from external modules.
> `add_modinfo` is to add devices from external modules, needed for a new
> machine, for example.
> Additional, 'arch' parameter of QemuModinfo is changed to a list.

I don't think there's much point in your continuing to post
versions of this patchset, because the answer remains "we don't
want to do this, as a policy and design decision". Until and
unless you persuade us that it's a good idea to change that
decision, time spent on code changes doesn't seem like a good
use of effort to me.

thanks
-- PMM
RE: [PATCH v3] Loading new machines and devices from external modules
Posted by Sebelev, Vladimir 1 year, 8 months ago
Hi Peter,

Anton previously sent explanation of our position. Nobody commented. Could you please comment on it? It's necessary for us to better understand your position. From our point of view technical ban of external modules loading doesn't solve any of mentioned problems, but makes VP developer life harder.

Best Regards,
Vladimir

-----Original Message-----
From: Peter Maydell <peter.maydell@linaro.org> 
Sent: Monday, August 22, 2022 12:23 PM
To: Drap Anton <drapas86@gmail.com>
Cc: qemu-devel@nongnu.org; Sebelev, Vladimir <vladimir.sebelev@auriga.com>; Drap, Anton <anton.drap@auriga.com>; Daniel P. Berrange <berrange@redhat.com>; Markus Armbruster <armbru@redhat.com>
Subject: Re: [PATCH v3] Loading new machines and devices from external modules

On Mon, 22 Aug 2022 at 09:53, Drap Anton <drapas86@gmail.com> wrote:
>
> From: "Drap, Anton" <anton.drap@auriga.com>

>     This patch is to add two parameters `add_machine` and `add_modinfo`.
> `add_machine` is to add machines from external modules.
> `add_modinfo` is to add devices from external modules, needed for a 
> new machine, for example.
> Additional, 'arch' parameter of QemuModinfo is changed to a list.

I don't think there's much point in your continuing to post versions of this patchset, because the answer remains "we don't want to do this, as a policy and design decision". Until and unless you persuade us that it's a good idea to change that decision, time spent on code changes doesn't seem like a good use of effort to me.

thanks
-- PMM
Re: [PATCH v3] Loading new machines and devices from external modules
Posted by Peter Maydell 1 year, 8 months ago
On Fri, 26 Aug 2022 at 08:51, Sebelev, Vladimir
<vladimir.sebelev@auriga.com> wrote:
> Anton previously sent explanation of our position. Nobody
> commented. Could you please comment on it? It's necessary for
> us to better understand your position.

The discussion was on this thread:
https://lore.kernel.org/qemu-devel/Yta+06u01e16zKTd@redhat.com/
Dan's email is a pretty straightforward summary of what
upstream's position is, and why we have that view.

I understand that the way QEMU is today is not helpful for you,
but it is the way it is. QEMU is not and doesn't really
seek to be a standardized simulator platform with a fixed
API that people can build device models for and then
stitch those device models together into platforms, and
have that work across different QEMU versions; and
wishing that it was doesn't make it so. The gap between
here and there is not just a 300 line patch, it is:

 * a significantly different/enlarged set of contributors
   (in the sense of "companies willing to invest money to work
   on the project and have it fulfil their requirements")
 * a major shift in project focus (for instance, it would
   have to care a lot more about providing well documented
   stable APIs to devices; it might need to care less about
   emulation performance improvements if they required
   breaking changes to existing devices)
 * a lot of work and restructuring to be able to provide
   that "we are a simulator platform with a clean interface
   that makes it easy to write and wire up device models"
   interface. A lot of work would be needed to clean up and
   separate out what are currently ad-hoc and legacy
   interfaces between different pieces of the codebase.
   We'd want to consider whether we wanted to provide
   SystemC interfaces and compatibility. Etc.

Perhaps it would be cool if that open source project existed;
but QEMU isn't currently it. The path to maybe causing QEMU
to get closer to that idea is not "throw a short patch onto
the mailing list", but to try to build a community of people
who (a) agree with you (b) are willing to contribute to the
upstream project to do the heavy lifting in the third
bullet point above (c) can make the case over time for
"these changes are good not just for us but also for you;
we're going to be around for the long term to help maintain
them; and we do our share of the work on the common parts
of the system that everybody benefits from".

QEMU has had big changes in focus before -- most notably
the addition of support for KVM and similar virtualization
interfaces to what was originally purely an emulator.
So it's not impossible -- but it isn't easy.

(Part 'c' is helpful for pretty much every non-trivial contribution
to any open source project, incidentally. But the more you're
trying to propose a major change in project focus the more
it matters.)

> From our point of view technical ban of external modules
> loading doesn't solve any of mentioned problems, but makes
> VP developer life harder.

Anything we provide as a user-facing interface is something
we have to support. Not providing "load machine from
external .so file" solves the problem of "this is not
something we want to support or think we are able to support".

For example, this patch doesn't solve the kind of problem you
list yourself in the commit message here:

> For example we’re now working on Raspberry Pi 4b implementation for our
> internal needs and we’re planning to merge it upstream. It’s based on some
> QEMU fork author of which wasn’t able to complete it and commit upstream.
> And it can’t be used with later QEMU without some efforts to port it
> to newer QEMU version.

Having that in an external .so that's loaded via an -add_machines
command line does nothing to make it work with multiple QEMU
versions: it will still only work with the QEMU version it
was based on, because newer ones will provide different
APIs and will have removed old functions that the code in
the .so is relying on. So providing a "-add_machines" option
is just going to cause a lot of bug reports of the form "my .so
doesn't work" where the answer is "you need to use the exact
matching QEMU version that the .so was built for". At which point
you might as well just have built the machine model into the
QEMU binary.

thanks
-- PMM
Re: [PATCH v3] Loading new machines and devices from external modules
Posted by Daniel P. Berrangé 1 year, 8 months ago
On Fri, Aug 26, 2022 at 07:51:20AM +0000, Sebelev, Vladimir wrote:
> Hi Peter,
> 
> Anton previously sent explanation of our position. Nobody commented.
> Could you please comment on it? It's necessary for us to better
> understand your position. From our point of view technical ban of
> external modules loading doesn't solve any of mentioned problems,
> but makes VP developer life harder.

In addition to what Peter just said, I'd like to quote the commit
message itself

[quote]
Moreover different people interested in different parts of QEMU.
QEMU core developers not interested in supporting and maintaining
tons of platforms available on the market. Virtual platform developers
not interested and usually don’t have resources to merge their changes
upstream. So we have a lots of abandoned QEMU forks for different
platforms
[/quote]

This is essentially saying that there are lots of people (often
working for commercial companies) using QEMU and they are not
interested in contributing upstream. At the same time complaining
it is too hard to maintain their forked code. The proposal is then
asking QEMU upstream to change its architecture & support public
API for plugins in order to make it easier for these people to
use QEMU while /still/ not contributing anything back to upstream.

That is not exactly giving a compelling story of the benefits for
the QEMU community. The benefit is all about people who are
intentionally outside the community and wish to remain that way,
while giving QEMU contributors an extra maint burden to support
external plugins indefinitely.

As for the point about the technical limitations and interactions
with licensing not being perfect, and people already likely ignoring
the licensing rules. That is very true, but at the same time, that is
not a reason to abandon the community's licensing goals/intent. Again
this is focusing on benefits to people who want to use QEMU without
contributing back.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|