[PATCH v2 00/18] modules: add metadata database

[PATCH v2 00/18] modules: add metadata database

[Gerd Hoffmann]

This patch series adds support for module metadata.  Here are the pieces
of the puzzle:

  (1) Macros are added to store metadata in a .modinfo elf section
      (idea stolen from the linux kernel).
  (2) A utility to scan modules, collect metadata from the .modinfo
      sections, store it in a file (modinfo.json) for later consumption
      by qemu.  Can also be easily inspected using 'jq'.
  (3) Adding annotations to the modules we have.
  (4) Drop hard-coded lists from utils/module.c

take care,
  Gerd

Gerd Hoffmann (18):
  modules: add metadata macros, add qxl module annotations
  qapi: add ModuleInfo schema
  modules: add qemu-modinfo utility
  modules: add virtio-gpu module annotations
  modules: add chardev module annotations
  modules: add audio module annotations
  modules: add usb-redir module annotations
  modules: add ccid module annotations
  modules: add ui module annotations
  modules: add s390x module annotations
  modules: add block module annotations
  modules: add module_load_path_init helper
  modules: load modinfo.json
  modules: use modinfo for dependencies
  modules: use modinfo for qom load
  modules: use modinfo for qemu opts load
  modules: check arch and block load on mismatch
  [fixup] module_load_modinfo

 include/qemu/module.h           |  23 +++
 audio/spiceaudio.c              |   2 +
 block/iscsi-opts.c              |   1 +
 chardev/baum.c                  |   1 +
 chardev/spice.c                 |   4 +
 hw/display/qxl.c                |   4 +
 hw/display/vhost-user-gpu-pci.c |   1 +
 hw/display/vhost-user-gpu.c     |   1 +
 hw/display/vhost-user-vga.c     |   1 +
 hw/display/virtio-gpu-base.c    |   1 +
 hw/display/virtio-gpu-gl.c      |   3 +
 hw/display/virtio-gpu-pci-gl.c  |   3 +
 hw/display/virtio-gpu-pci.c     |   2 +
 hw/display/virtio-gpu.c         |   1 +
 hw/display/virtio-vga-gl.c      |   3 +
 hw/display/virtio-vga.c         |   2 +
 hw/s390x/virtio-ccw-gpu.c       |   3 +
 hw/usb/ccid-card-emulated.c     |   1 +
 hw/usb/ccid-card-passthru.c     |   1 +
 hw/usb/redirect.c               |   1 +
 qemu-modinfo.c                  | 270 ++++++++++++++++++++++++++++++
 softmmu/vl.c                    |  20 +--
 stubs/module-opts.c             |   4 -
 ui/egl-headless.c               |   4 +
 ui/gtk.c                        |   4 +
 ui/sdl2.c                       |   4 +
 ui/spice-app.c                  |   3 +
 ui/spice-core.c                 |   5 +
 util/module.c                   | 282 +++++++++++++++++++-------------
 meson.build                     |  11 ++
 qapi/meson.build                |   1 +
 qapi/modules.json               |  36 ++++
 qapi/qapi-schema.json           |   1 +
 util/trace-events               |   3 +
 34 files changed, 576 insertions(+), 131 deletions(-)
 create mode 100644 qemu-modinfo.c
 create mode 100644 qapi/modules.json

-- 
2.31.1

[PATCH v2 01/18] modules: add metadata macros, add qxl module annotations

[Gerd Hoffmann]

Stealing an idea from the linux kernel:  Place module metadata
in an .modinfo elf section.  This patch adds macros and qxl module
annotations as example.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/qemu/module.h | 22 ++++++++++++++++++++++
 hw/display/qxl.c      |  4 ++++
 2 files changed, 26 insertions(+)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index 944d403cbd15..d3cab3c25a2f 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -73,4 +73,26 @@ 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);
 
+/*
+ * macros to store module metadata in a .modinfo section.
+ * qemu-modinfo utility will collect the metadata.
+ *
+ * Use "objdump -t -s -j .modinfo ${module}.so" to inspect.
+ */
+
+#define ___PASTE(a, b) a##b
+#define __PASTE(a, b) ___PASTE(a, b)
+
+#define modinfo(kind, value)                             \
+    static const char __PASTE(kind, __LINE__)[]          \
+        __attribute__((__used__))                        \
+        __attribute__((section(".modinfo")))             \
+        __attribute__((aligned(1)))                      \
+        = stringify(kind) "=" value
+
+#define module_obj(name) modinfo(obj, name)
+#define module_dep(name) modinfo(dep, name)
+#define module_arch(name) modinfo(arch, name)
+#define module_opts(name) modinfo(opts, name)
+
 #endif
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 6e1f8ff1b2a7..84f99088e0a0 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2522,6 +2522,7 @@ static const TypeInfo qxl_primary_info = {
     .parent        = TYPE_PCI_QXL,
     .class_init    = qxl_primary_class_init,
 };
+module_obj("qxl-vga");
 
 static void qxl_secondary_class_init(ObjectClass *klass, void *data)
 {
@@ -2538,6 +2539,7 @@ static const TypeInfo qxl_secondary_info = {
     .parent        = TYPE_PCI_QXL,
     .class_init    = qxl_secondary_class_init,
 };
+module_obj("qxl");
 
 static void qxl_register_types(void)
 {
@@ -2547,3 +2549,5 @@ static void qxl_register_types(void)
 }
 
 type_init(qxl_register_types)
+
+module_dep("ui-spice-core");
-- 
2.31.1

[PATCH v2 02/18] qapi: add ModuleInfo schema

[Gerd Hoffmann]

Add QAPI schema for the module info database.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qapi/meson.build      |  1 +
 qapi/modules.json     | 36 ++++++++++++++++++++++++++++++++++++
 qapi/qapi-schema.json |  1 +
 3 files changed, 38 insertions(+)
 create mode 100644 qapi/modules.json

diff --git a/qapi/meson.build b/qapi/meson.build
index 376f4ceafe74..596aa5d71168 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -36,6 +36,7 @@ qapi_all_modules = [
   'migration',
   'misc',
   'misc-target',
+  'modules',
   'net',
   'pragma',
   'qom',
diff --git a/qapi/modules.json b/qapi/modules.json
new file mode 100644
index 000000000000..5420977d8765
--- /dev/null
+++ b/qapi/modules.json
@@ -0,0 +1,36 @@
+# -*- Mode: Python -*-
+# vim: filetype=python
+
+##
+# @ModuleInfo:
+#
+# qemu module metadata
+#
+# @name: module name
+#
+# @objs: list of qom objects implemented by the module.
+#
+# @deps: list of other modules this module depends on.
+#
+# @arch: module architecture.
+#
+# @opts: qemu opts implemented by module.
+#
+# Since: 6.1
+##
+{ 'struct': 'ModuleInfo',
+  'data': { 'name'  : 'str',
+            '*objs' : ['str'],
+            '*deps' : ['str'],
+            '*arch' : 'str',
+            '*opts' : 'str'}}
+
+##
+# @Modules:
+#
+# qemu module list
+#
+# Since: 6.1
+##
+{ 'struct': 'Modules',
+  'data': { 'list' : ['ModuleInfo']}}
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index 4912b9744e69..5baa511c2ff5 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -93,3 +93,4 @@
 { 'include': 'audio.json' }
 { 'include': 'acpi.json' }
 { 'include': 'pci.json' }
+{ 'include': 'modules.json' }
-- 
2.31.1

[PATCH v2 03/18] modules: add qemu-modinfo utility

[Gerd Hoffmann]

Scan .modinfo sections of qemu modules,
write module metadata to modinfo.json.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-modinfo.c | 270 +++++++++++++++++++++++++++++++++++++++++++++++++
 meson.build    |  11 ++
 2 files changed, 281 insertions(+)
 create mode 100644 qemu-modinfo.c

diff --git a/qemu-modinfo.c b/qemu-modinfo.c
new file mode 100644
index 000000000000..611dbdb00683
--- /dev/null
+++ b/qemu-modinfo.c
@@ -0,0 +1,270 @@
+/*
+ * QEMU module parser
+ *
+ * read modules, find modinfo section, parse & store metadata.
+ *
+ * Copyright Red Hat, Inc. 2021
+ *
+ * Authors:
+ *     Gerd Hoffmann <kraxel@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "qemu/osdep.h"
+#include "elf.h"
+#include <stdint.h>
+#include <dirent.h>
+
+#include "qapi/qapi-types-modules.h"
+#include "qapi/qapi-visit-modules.h"
+#include "qapi/qobject-output-visitor.h"
+#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qstring.h"
+
+#if INTPTR_MAX == INT32_MAX
+# define Elf_Ehdr Elf32_Ehdr
+# define Elf_Shdr Elf32_Shdr
+# define ELFCLASS ELFCLASS32
+#elif INTPTR_MAX == INT64_MAX
+# define Elf_Ehdr Elf64_Ehdr
+# define Elf_Shdr Elf64_Shdr
+# define ELFCLASS ELFCLASS64
+#else
+# error Huh?  Neither 32-bit nor 64-bit host.
+#endif
+
+static const char *moddir = CONFIG_QEMU_MODDIR;
+static const char *dsosuf = CONFIG_HOST_DSOSUF;
+
+static ModuleInfo *modinfo(const char *module, char *info, size_t size)
+{
+    ModuleInfo *modinfo;
+    strList *sl;
+    size_t pos = 0, len;
+
+    modinfo = g_new0(ModuleInfo, 1);
+    modinfo->name = g_strdup(module);
+
+    if (info) {
+        do {
+            if (strncmp(info + pos, "obj=", 4) == 0) {
+                sl = g_new0(strList, 1);
+                sl->value = g_strdup(info + pos + 4);
+                sl->next = modinfo->objs;
+                modinfo->objs = sl;
+                modinfo->has_objs = true;
+            } else if (strncmp(info + pos, "dep=", 4) == 0) {
+                sl = g_new0(strList, 1);
+                sl->value = g_strdup(info + pos + 4);
+                sl->next = modinfo->deps;
+                modinfo->deps = sl;
+                modinfo->has_deps = true;
+            } else if (strncmp(info + pos, "arch=", 5) == 0) {
+                modinfo->arch = g_strdup(info + pos + 5);
+                modinfo->has_arch = true;
+            } else if (strncmp(info + pos, "opts=", 5) == 0) {
+                modinfo->opts = g_strdup(info + pos + 5);
+                modinfo->has_opts = true;
+            } else {
+                fprintf(stderr, "unknown tag: %s\n", info + pos);
+                exit(1);
+            }
+            len = strlen(info + pos) + 1;
+            pos += len;
+        } while (pos < size);
+    }
+
+    return modinfo;
+}
+
+static void elf_read_section_hdr(FILE *fp, Elf_Ehdr *ehdr,
+                                 int section, Elf_Shdr *shdr)
+{
+    size_t pos, len;
+    int ret;
+
+    pos = ehdr->e_shoff + section * ehdr->e_shentsize;
+    len = MIN(ehdr->e_shentsize, sizeof(*shdr));
+
+    ret = fseek(fp, pos, SEEK_SET);
+    if (ret != 0) {
+        fprintf(stderr, "seek error\n");
+        exit(1);
+    }
+
+    memset(shdr, 0, sizeof(*shdr));
+    ret = fread(shdr, len, 1, fp);
+    if (ret != 1) {
+        fprintf(stderr, "read error\n");
+        exit(1);
+    }
+}
+
+static void *elf_read_section(FILE *fp, Elf_Ehdr *ehdr,
+                              int section, size_t *size)
+{
+    Elf_Shdr shdr;
+    void *data;
+    int ret;
+
+    elf_read_section_hdr(fp, ehdr, section, &shdr);
+    if (shdr.sh_offset && shdr.sh_size) {
+        ret = fseek(fp, shdr.sh_offset, SEEK_SET);
+        if (ret != 0) {
+            fprintf(stderr, "seek error\n");
+            exit(1);
+        }
+
+        data = g_malloc(shdr.sh_size);
+        ret = fread(data, shdr.sh_size, 1, fp);
+        if (ret != 1) {
+            fprintf(stderr, "read error\n");
+            exit(1);
+        }
+        *size = shdr.sh_size;
+    } else {
+        data = NULL;
+        *size = 0;
+    }
+    return data;
+}
+
+static ModuleInfo *elf_parse_module(const char *module,
+                                    const char *filename)
+{
+    Elf_Ehdr ehdr;
+    Elf_Shdr shdr;
+    FILE *fp;
+    int ret, i;
+    char *str;
+    size_t str_size;
+    char *info;
+    size_t info_size;
+
+    fp = fopen(filename, "r");
+    if (NULL == fp) {
+        fprintf(stderr, "open %s: %s\n", filename, strerror(errno));
+        exit(1);
+    }
+
+    ret = fread(&ehdr, sizeof(ehdr), 1, fp);
+    if (ret != 1) {
+        fprintf(stderr, "read error (%s)\n", filename);
+        exit(1);
+    }
+
+    if (ehdr.e_ident[EI_MAG0] != ELFMAG0 ||
+        ehdr.e_ident[EI_MAG1] != ELFMAG1 ||
+        ehdr.e_ident[EI_MAG2] != ELFMAG2 ||
+        ehdr.e_ident[EI_MAG3] != ELFMAG3) {
+        fprintf(stderr, "not an elf file (%s)\n", filename);
+        exit(1);
+    }
+    if (ehdr.e_ident[EI_CLASS] != ELFCLASS64) {
+        fprintf(stderr, "elf class mismatch (%s)\n", filename);
+        exit(1);
+    }
+    if (ehdr.e_shoff == 0) {
+        fprintf(stderr, "no section header (%s)\n", filename);
+        exit(1);
+    }
+
+    /* read string table */
+    if (ehdr.e_shstrndx == 0) {
+        fprintf(stderr, "no section strings (%s)\n", filename);
+        exit(1);
+    }
+    str = elf_read_section(fp, &ehdr, ehdr.e_shstrndx, &str_size);
+    if (NULL == str) {
+        fprintf(stderr, "no section strings (%s)\n", filename);
+        exit(1);
+    }
+
+    /* find and read modinfo section */
+    info = NULL;
+    for (i = 0; i < ehdr.e_shnum; i++) {
+        elf_read_section_hdr(fp, &ehdr, i, &shdr);
+        if (!shdr.sh_name) {
+            continue;
+        }
+        if (strcmp(str + shdr.sh_name, ".modinfo") == 0) {
+            info = elf_read_section(fp, &ehdr, i, &info_size);
+        }
+    }
+    fclose(fp);
+
+    return modinfo(module, info, info_size);
+}
+
+int main(int argc, char **argv)
+{
+    DIR *dir;
+    FILE *fp;
+    ModuleInfo *modinfo;
+    ModuleInfoList *modlist;
+    Modules *modules;
+    Visitor *v;
+    QObject *obj;
+    Error *errp = NULL;
+    struct dirent *ent;
+    char *ext, *file, *name;
+    GString *gjson;
+    QString *qjson;
+    const char *json;
+
+    if (argc > 1) {
+        moddir = argv[1];
+    }
+
+    dir = opendir(moddir);
+    if (dir == NULL) {
+        fprintf(stderr, "opendir(%s): %s\n", moddir, strerror(errno));
+        exit(1);
+    }
+
+    modules = g_new0(Modules, 1);
+    while (NULL != (ent = readdir(dir))) {
+        ext = strrchr(ent->d_name, '.');
+        if (!ext) {
+            continue;
+        }
+        if (strcmp(ext, dsosuf) != 0) {
+            continue;
+        }
+
+        name = g_strndup(ent->d_name, ext - ent->d_name);
+        file = g_strdup_printf("%s/%s", moddir, ent->d_name);
+        modinfo = elf_parse_module(name, file);
+        g_free(file);
+        g_free(name);
+
+        modlist = g_new0(ModuleInfoList, 1);
+        modlist->value = modinfo;
+        modlist->next = modules->list;
+        modules->list = modlist;
+    }
+    closedir(dir);
+
+    v = qobject_output_visitor_new(&obj);
+    visit_type_Modules(v, NULL, &modules, &errp);
+    visit_complete(v, &obj);
+    visit_free(v);
+
+    gjson = qobject_to_json(obj);
+    qjson = qstring_from_gstring(gjson);
+    json = qstring_get_str(qjson);
+
+    file = g_strdup_printf("%s/modinfo.json", moddir);
+    fp = fopen(file, "w");
+    if (fp == NULL) {
+        fprintf(stderr, "open(%s): %s\n", file, strerror(errno));
+        exit(1);
+    }
+    fprintf(fp, "%s", json);
+    fclose(fp);
+
+    printf("%s written\n", file);
+    g_free(file);
+    return 0;
+}
diff --git a/meson.build b/meson.build
index d2a9ce91f556..9823c5889140 100644
--- a/meson.build
+++ b/meson.build
@@ -2380,6 +2380,17 @@ if xkbcommon.found()
                            dependencies: [qemuutil, xkbcommon], install: have_tools)
 endif
 
+if config_host.has_key('CONFIG_MODULES')
+   qemu_modinfo = executable('qemu-modinfo', files('qemu-modinfo.c') + genh,
+                             dependencies: [glib, qemuutil], install: have_tools)
+   custom_target('modinfo.json',
+                 input: [ softmmu_mods, block_mods ],
+                 output: 'modinfo.json',
+                 install: true,
+                 install_dir: qemu_moddir,
+                 command: [ qemu_modinfo, '.' ])
+endif
+
 if have_tools
   qemu_img = executable('qemu-img', [files('qemu-img.c'), hxdep],
              dependencies: [authz, block, crypto, io, qom, qemuutil], install: true)
-- 
2.31.1

[PATCH v2 04/18] modules: add virtio-gpu module annotations

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vhost-user-gpu-pci.c | 1 +
 hw/display/vhost-user-gpu.c     | 1 +
 hw/display/vhost-user-vga.c     | 1 +
 hw/display/virtio-gpu-base.c    | 1 +
 hw/display/virtio-gpu-gl.c      | 3 +++
 hw/display/virtio-gpu-pci-gl.c  | 3 +++
 hw/display/virtio-gpu-pci.c     | 2 ++
 hw/display/virtio-gpu.c         | 1 +
 hw/display/virtio-vga-gl.c      | 3 +++
 hw/display/virtio-vga.c         | 2 ++
 10 files changed, 18 insertions(+)

diff --git a/hw/display/vhost-user-gpu-pci.c b/hw/display/vhost-user-gpu-pci.c
index a02b23ecaf11..daefcf710159 100644
--- a/hw/display/vhost-user-gpu-pci.c
+++ b/hw/display/vhost-user-gpu-pci.c
@@ -43,6 +43,7 @@ static const VirtioPCIDeviceTypeInfo vhost_user_gpu_pci_info = {
     .instance_size = sizeof(VhostUserGPUPCI),
     .instance_init = vhost_user_gpu_pci_initfn,
 };
+module_obj(TYPE_VHOST_USER_GPU_PCI);
 
 static void vhost_user_gpu_pci_register_types(void)
 {
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 6cdaa1c73b9b..32ef0061f924 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -596,6 +596,7 @@ static const TypeInfo vhost_user_gpu_info = {
     .instance_finalize = vhost_user_gpu_instance_finalize,
     .class_init = vhost_user_gpu_class_init,
 };
+module_obj(TYPE_VHOST_USER_GPU);
 
 static void vhost_user_gpu_register_types(void)
 {
diff --git a/hw/display/vhost-user-vga.c b/hw/display/vhost-user-vga.c
index a34a99856d73..072c9c65bc75 100644
--- a/hw/display/vhost-user-vga.c
+++ b/hw/display/vhost-user-vga.c
@@ -44,6 +44,7 @@ static const VirtioPCIDeviceTypeInfo vhost_user_vga_info = {
     .instance_size = sizeof(VhostUserVGA),
     .instance_init = vhost_user_vga_inst_initfn,
 };
+module_obj(TYPE_VHOST_USER_VGA);
 
 static void vhost_user_vga_register_types(void)
 {
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index dd294276cb38..c8da4806e0bb 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -256,6 +256,7 @@ static const TypeInfo virtio_gpu_base_info = {
     .class_init = virtio_gpu_base_class_init,
     .abstract = true
 };
+module_obj(TYPE_VIRTIO_GPU_BASE);
 
 static void
 virtio_register_types(void)
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index d971b480806a..7ab93bf8c829 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -154,6 +154,7 @@ static const TypeInfo virtio_gpu_gl_info = {
     .instance_size = sizeof(VirtIOGPUGL),
     .class_init = virtio_gpu_gl_class_init,
 };
+module_obj(TYPE_VIRTIO_GPU_GL);
 
 static void virtio_register_types(void)
 {
@@ -161,3 +162,5 @@ static void virtio_register_types(void)
 }
 
 type_init(virtio_register_types)
+
+module_dep("hw-display-virtio-gpu");
diff --git a/hw/display/virtio-gpu-pci-gl.c b/hw/display/virtio-gpu-pci-gl.c
index 902dda345275..99b14a07185e 100644
--- a/hw/display/virtio-gpu-pci-gl.c
+++ b/hw/display/virtio-gpu-pci-gl.c
@@ -46,6 +46,7 @@ static const VirtioPCIDeviceTypeInfo virtio_gpu_gl_pci_info = {
     .instance_size = sizeof(VirtIOGPUGLPCI),
     .instance_init = virtio_gpu_gl_initfn,
 };
+module_obj(TYPE_VIRTIO_GPU_GL_PCI);
 
 static void virtio_gpu_gl_pci_register_types(void)
 {
@@ -53,3 +54,5 @@ static void virtio_gpu_gl_pci_register_types(void)
 }
 
 type_init(virtio_gpu_gl_pci_register_types)
+
+module_dep("hw-display-virtio-gpu-pci");
diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index d742a30aecf7..e36eee0c409b 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -64,6 +64,7 @@ static const TypeInfo virtio_gpu_pci_base_info = {
     .class_init = virtio_gpu_pci_base_class_init,
     .abstract = true
 };
+module_obj(TYPE_VIRTIO_GPU_PCI_BASE);
 
 #define TYPE_VIRTIO_GPU_PCI "virtio-gpu-pci"
 typedef struct VirtIOGPUPCI VirtIOGPUPCI;
@@ -90,6 +91,7 @@ static const VirtioPCIDeviceTypeInfo virtio_gpu_pci_info = {
     .instance_size = sizeof(VirtIOGPUPCI),
     .instance_init = virtio_gpu_initfn,
 };
+module_obj(TYPE_VIRTIO_GPU_PCI);
 
 static void virtio_gpu_pci_register_types(void)
 {
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 4d549377cbc1..68fd607c2711 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1419,6 +1419,7 @@ static const TypeInfo virtio_gpu_info = {
     .class_size = sizeof(VirtIOGPUClass),
     .class_init = virtio_gpu_class_init,
 };
+module_obj(TYPE_VIRTIO_GPU);
 
 static void virtio_register_types(void)
 {
diff --git a/hw/display/virtio-vga-gl.c b/hw/display/virtio-vga-gl.c
index c971340ebb1a..f22549097c5e 100644
--- a/hw/display/virtio-vga-gl.c
+++ b/hw/display/virtio-vga-gl.c
@@ -36,6 +36,7 @@ static VirtioPCIDeviceTypeInfo virtio_vga_gl_info = {
     .instance_size = sizeof(VirtIOVGAGL),
     .instance_init = virtio_vga_gl_inst_initfn,
 };
+module_obj(TYPE_VIRTIO_VGA_GL);
 
 static void virtio_vga_register_types(void)
 {
@@ -45,3 +46,5 @@ static void virtio_vga_register_types(void)
 }
 
 type_init(virtio_vga_register_types)
+
+module_dep("hw-display-virtio-vga");
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index d3c640406152..9e57f61e9edb 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -239,6 +239,7 @@ static TypeInfo virtio_vga_base_info = {
     .class_init    = virtio_vga_base_class_init,
     .abstract      = true,
 };
+module_obj(TYPE_VIRTIO_VGA_BASE);
 
 #define TYPE_VIRTIO_VGA "virtio-vga"
 
@@ -268,6 +269,7 @@ static VirtioPCIDeviceTypeInfo virtio_vga_info = {
     .instance_size = sizeof(VirtIOVGA),
     .instance_init = virtio_vga_inst_initfn,
 };
+module_obj(TYPE_VIRTIO_VGA);
 
 static void virtio_vga_register_types(void)
 {
-- 
2.31.1

[PATCH v2 05/18] modules: add chardev module annotations

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 chardev/baum.c  | 1 +
 chardev/spice.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/chardev/baum.c b/chardev/baum.c
index 5deca778bc44..79d618e35045 100644
--- a/chardev/baum.c
+++ b/chardev/baum.c
@@ -680,6 +680,7 @@ static const TypeInfo char_braille_type_info = {
     .instance_finalize = char_braille_finalize,
     .class_init = char_braille_class_init,
 };
+module_obj(TYPE_CHARDEV_BRAILLE);
 
 static void register_types(void)
 {
diff --git a/chardev/spice.c b/chardev/spice.c
index 1104426e3a11..3ffb3fdc0dac 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -366,6 +366,7 @@ static const TypeInfo char_spice_type_info = {
     .class_init = char_spice_class_init,
     .abstract = true,
 };
+module_obj(TYPE_CHARDEV_SPICE);
 
 static void char_spicevmc_class_init(ObjectClass *oc, void *data)
 {
@@ -396,6 +397,7 @@ static const TypeInfo char_spiceport_type_info = {
     .parent = TYPE_CHARDEV_SPICE,
     .class_init = char_spiceport_class_init,
 };
+module_obj(TYPE_CHARDEV_SPICEPORT);
 
 static void register_types(void)
 {
@@ -405,3 +407,5 @@ static void register_types(void)
 }
 
 type_init(register_types);
+
+module_dep("ui-spice-core");
-- 
2.31.1

[PATCH v2 06/18] modules: add audio module annotations

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 audio/spiceaudio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
index 999bfbde47c5..a8d370fe6f31 100644
--- a/audio/spiceaudio.c
+++ b/audio/spiceaudio.c
@@ -317,3 +317,5 @@ static void register_audio_spice(void)
     audio_driver_register(&spice_audio_driver);
 }
 type_init(register_audio_spice);
+
+module_dep("ui-spice-core");
-- 
2.31.1

[PATCH v2 07/18] modules: add usb-redir module annotations

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/redirect.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 6a75b0dc4ab2..4ec9326e0582 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -2608,6 +2608,7 @@ static const TypeInfo usbredir_dev_info = {
     .class_init    = usbredir_class_initfn,
     .instance_init = usbredir_instance_init,
 };
+module_obj(TYPE_USB_REDIR);
 
 static void usbredir_register_types(void)
 {
-- 
2.31.1

[PATCH v2 08/18] modules: add ccid module annotations

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/ccid-card-emulated.c | 1 +
 hw/usb/ccid-card-passthru.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index 5c76bed77aa0..6c8c0355e099 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -612,6 +612,7 @@ static const TypeInfo emulated_card_info = {
     .instance_size = sizeof(EmulatedState),
     .class_init    = emulated_class_initfn,
 };
+module_obj(TYPE_EMULATED_CCID);
 
 static void ccid_card_emulated_register_types(void)
 {
diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 7212d0d7fb5e..fa3040fb7154 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -414,6 +414,7 @@ static const TypeInfo passthru_card_info = {
     .instance_size = sizeof(PassthruState),
     .class_init    = passthru_class_initfn,
 };
+module_obj(TYPE_CCID_PASSTHRU);
 
 static void ccid_card_passthru_register_types(void)
 {
-- 
2.31.1

[PATCH v2 09/18] modules: add ui module annotations

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/egl-headless.c | 4 ++++
 ui/gtk.c          | 4 ++++
 ui/sdl2.c         | 4 ++++
 ui/spice-app.c    | 3 +++
 ui/spice-core.c   | 5 +++++
 5 files changed, 20 insertions(+)

diff --git a/ui/egl-headless.c b/ui/egl-headless.c
index da377a74af69..75404e0e8700 100644
--- a/ui/egl-headless.c
+++ b/ui/egl-headless.c
@@ -213,3 +213,7 @@ static void register_egl(void)
 }
 
 type_init(register_egl);
+
+#ifdef CONFIG_OPENGL
+module_dep("ui-opengl");
+#endif
diff --git a/ui/gtk.c b/ui/gtk.c
index 98046f577b9d..376b4d528daa 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2333,3 +2333,7 @@ static void register_gtk(void)
 }
 
 type_init(register_gtk);
+
+#ifdef CONFIG_OPENGL
+module_dep("ui-opengl");
+#endif
diff --git a/ui/sdl2.c b/ui/sdl2.c
index a203cb0239e1..36d9010cb6c1 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -918,3 +918,7 @@ static void register_sdl1(void)
 }
 
 type_init(register_sdl1);
+
+#ifdef CONFIG_OPENGL
+module_dep("ui-opengl");
+#endif
diff --git a/ui/spice-app.c b/ui/spice-app.c
index 4325ac2d9c54..641f4a9d53e3 100644
--- a/ui/spice-app.c
+++ b/ui/spice-app.c
@@ -221,3 +221,6 @@ static void register_spice_app(void)
 }
 
 type_init(register_spice_app);
+
+module_dep("ui-spice-core");
+module_dep("chardev-spice");
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 272d19b0c152..86d43783acac 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -1037,3 +1037,8 @@ static void spice_register_config(void)
     qemu_add_opts(&qemu_spice_opts);
 }
 opts_init(spice_register_config);
+module_opts("spice");
+
+#ifdef CONFIG_OPENGL
+module_dep("ui-opengl");
+#endif
-- 
2.31.1

[PATCH v2 10/18] modules: add s390x module annotations

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/s390x/virtio-ccw-gpu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/s390x/virtio-ccw-gpu.c b/hw/s390x/virtio-ccw-gpu.c
index 75a9e4bb3908..5868a2a07093 100644
--- a/hw/s390x/virtio-ccw-gpu.c
+++ b/hw/s390x/virtio-ccw-gpu.c
@@ -59,6 +59,7 @@ static const TypeInfo virtio_ccw_gpu = {
     .instance_init = virtio_ccw_gpu_instance_init,
     .class_init    = virtio_ccw_gpu_class_init,
 };
+module_obj(TYPE_VIRTIO_GPU_CCW);
 
 static void virtio_ccw_gpu_register(void)
 {
@@ -68,3 +69,5 @@ static void virtio_ccw_gpu_register(void)
 }
 
 type_init(virtio_ccw_gpu_register)
+
+module_arch("s390x");
-- 
2.31.1

[PATCH v2 11/18] modules: add block module annotations

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 block/iscsi-opts.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/iscsi-opts.c b/block/iscsi-opts.c
index afaf8837d6c1..4f2da405e645 100644
--- a/block/iscsi-opts.c
+++ b/block/iscsi-opts.c
@@ -68,3 +68,4 @@ static void iscsi_block_opts_init(void)
 }
 
 block_init(iscsi_block_opts_init);
+module_opts("iscsi");
-- 
2.31.1

[PATCH v2 12/18] modules: add module_load_path_init helper

[Gerd Hoffmann]

Factor out module search path initialization to the new
module_load_path_init() helper.  Also store the search path in
global variables and keep it so we have to do it only once.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/module.c | 58 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/util/module.c b/util/module.c
index eee8ff2de136..3a2d6dde9734 100644
--- a/util/module.c
+++ b/util/module.c
@@ -110,6 +110,36 @@ void module_call_init(module_init_type type)
 }
 
 #ifdef CONFIG_MODULES
+
+static char *module_dirs[5];
+static int module_ndirs;
+
+static void module_load_path_init(void)
+{
+    const char *search_dir;
+
+    if (module_ndirs) {
+        return;
+    }
+
+    search_dir = getenv("QEMU_MODULE_DIR");
+    if (search_dir != NULL) {
+        module_dirs[module_ndirs++] = g_strdup_printf("%s", search_dir);
+    }
+    module_dirs[module_ndirs++] = get_relocated_path(CONFIG_QEMU_MODDIR);
+    module_dirs[module_ndirs++] = g_strdup(qemu_get_exec_dir());
+
+#ifdef CONFIG_MODULE_UPGRADES
+    version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION),
+                             G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS "+-.~",
+                             '_');
+    module_dirs[module_ndirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
+#endif
+
+    assert(module_ndirs <= ARRAY_SIZE(module_dirs));
+
+}
+
 static int module_load_file(const char *fname, bool mayfail, bool export_symbols)
 {
     GModule *g_module;
@@ -204,10 +234,8 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
 #ifdef CONFIG_MODULE_UPGRADES
     char *version_dir;
 #endif
-    const char *search_dir;
-    char *dirs[5];
     char *module_name;
-    int i = 0, n_dirs = 0;
+    int i = 0;
     int ret, dep;
     bool export_symbols = false;
     static GHashTable *loaded_modules;
@@ -240,25 +268,11 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
     }
     g_hash_table_add(loaded_modules, module_name);
 
-    search_dir = getenv("QEMU_MODULE_DIR");
-    if (search_dir != NULL) {
-        dirs[n_dirs++] = g_strdup_printf("%s", search_dir);
-    }
-    dirs[n_dirs++] = get_relocated_path(CONFIG_QEMU_MODDIR);
-    dirs[n_dirs++] = g_strdup(qemu_get_exec_dir());
+    module_load_path_init();
 
-#ifdef CONFIG_MODULE_UPGRADES
-    version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION),
-                             G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS "+-.~",
-                             '_');
-    dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
-#endif
-
-    assert(n_dirs <= ARRAY_SIZE(dirs));
-
-    for (i = 0; i < n_dirs; i++) {
+    for (i = 0; i < module_ndirs; i++) {
         fname = g_strdup_printf("%s/%s%s",
-                dirs[i], module_name, CONFIG_HOST_DSOSUF);
+                module_dirs[i], module_name, CONFIG_HOST_DSOSUF);
         ret = module_load_file(fname, mayfail, export_symbols);
         g_free(fname);
         fname = NULL;
@@ -274,10 +288,6 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
         g_free(module_name);
     }
 
-    for (i = 0; i < n_dirs; i++) {
-        g_free(dirs[i]);
-    }
-
 #endif
     return success;
 }
-- 
2.31.1

[PATCH v2 13/18] modules: load modinfo.json

[Gerd Hoffmann]

Load and parse the module info database.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/module.c     | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 util/trace-events |  3 +++
 2 files changed, 57 insertions(+)

diff --git a/util/module.c b/util/module.c
index 3a2d6dde9734..b0ea8c57d438 100644
--- a/util/module.c
+++ b/util/module.c
@@ -20,9 +20,16 @@
 #include "qemu/queue.h"
 #include "qemu/module.h"
 #include "qemu/cutils.h"
+#include "qemu/error-report.h"
 #ifdef CONFIG_MODULE_UPGRADES
 #include "qemu-version.h"
 #endif
+#include "trace.h"
+
+#include "qapi/error.h"
+#include "qapi/qapi-types-modules.h"
+#include "qapi/qapi-visit-modules.h"
+#include "qapi/qobject-input-visitor.h"
 
 typedef struct ModuleEntry
 {
@@ -111,6 +118,7 @@ void module_call_init(module_init_type type)
 
 #ifdef CONFIG_MODULES
 
+static Modules *modinfo;
 static char *module_dirs[5];
 static int module_ndirs;
 
@@ -137,7 +145,52 @@ static void module_load_path_init(void)
 #endif
 
     assert(module_ndirs <= ARRAY_SIZE(module_dirs));
+}
 
+static void module_load_modinfo(void)
+{
+    char *file, *json;
+    FILE *fp;
+    int i, size;
+    Visitor *v;
+    Error *errp = NULL;
+
+    if (modinfo) {
+        return;
+    }
+
+    for (i = 0; i < module_ndirs; i++) {
+        file = g_strdup_printf("%s/modinfo.json", module_dirs[i]);
+        fp = fopen(file, "r");
+        if (fp != NULL) {
+            break;
+        }
+        g_free(file);
+    }
+    if (NULL == fp) {
+        warn_report("No modinfo.json file found.");
+        return;
+    } else {
+        trace_module_load_modinfo(file);
+    }
+
+    fseek(fp, 0, SEEK_END);
+    size = ftell(fp);
+    fseek(fp, 0, SEEK_SET);
+    json = g_malloc0(size + 1);
+    fread(json, size, 1, fp);
+    json[size] = 0;
+    fclose(fp);
+
+    v = qobject_input_visitor_new_str(json, NULL, &errp);
+    if (errp) {
+        error_reportf_err(errp, "parse error (%s)", file);
+        g_free(file);
+        return;
+    }
+    visit_type_Modules(v, NULL, &modinfo, &errp);
+    visit_free(v);
+    g_free(file);
 }
 
 static int module_load_file(const char *fname, bool mayfail, bool export_symbols)
@@ -269,6 +322,7 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
     g_hash_table_add(loaded_modules, module_name);
 
     module_load_path_init();
+    module_load_modinfo();
 
     for (i = 0; i < module_ndirs; i++) {
         fname = g_strdup_printf("%s/%s%s",
diff --git a/util/trace-events b/util/trace-events
index 806cac14a762..8b2afcbd109a 100644
--- a/util/trace-events
+++ b/util/trace-events
@@ -100,3 +100,6 @@ uffd_create_fd_api_failed(int err) "errno: %i"
 uffd_create_fd_api_noioctl(uint64_t ioctl_req, uint64_t ioctl_supp) "ioctl_req: 0x%" PRIx64 "ioctl_supp: 0x%" PRIx64
 uffd_register_memory_failed(void *addr, uint64_t length, uint64_t mode, int err) "addr: %p length: %" PRIu64 " mode: 0x%" PRIx64 " errno: %i"
 uffd_unregister_memory_failed(void *addr, uint64_t length, int err) "addr: %p length: %" PRIu64 " errno: %i"
+
+# module.c
+module_load_modinfo(const char *filename) "modinfo %s"
-- 
2.31.1

[PATCH v2 14/18] modules: use modinfo for dependencies

[Gerd Hoffmann]

Use module database for module dependencies.
Drop hard-coded dependency list.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/module.c | 55 ++++++++++++++++++++-------------------------------
 1 file changed, 21 insertions(+), 34 deletions(-)

diff --git a/util/module.c b/util/module.c
index b0ea8c57d438..fd5fc059e14a 100644
--- a/util/module.c
+++ b/util/module.c
@@ -254,28 +254,6 @@ static int module_load_file(const char *fname, bool mayfail, bool export_symbols
 out:
     return ret;
 }
-
-static const struct {
-    const char *name;
-    const char *dep;
-} module_deps[] = {
-    { "audio-spice",    "ui-spice-core" },
-    { "chardev-spice",  "ui-spice-core" },
-    { "hw-display-qxl", "ui-spice-core" },
-    { "ui-spice-app",   "ui-spice-core" },
-    { "ui-spice-app",   "chardev-spice" },
-
-    { "hw-display-virtio-gpu-gl", "hw-display-virtio-gpu" },
-    { "hw-display-virtio-gpu-pci-gl", "hw-display-virtio-gpu-pci" },
-    { "hw-display-virtio-vga-gl", "hw-display-virtio-vga" },
-
-#ifdef CONFIG_OPENGL
-    { "ui-egl-headless", "ui-opengl"    },
-    { "ui-gtk",          "ui-opengl"    },
-    { "ui-sdl",          "ui-opengl"    },
-    { "ui-spice-core",   "ui-opengl"    },
-#endif
-};
 #endif
 
 bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
@@ -289,9 +267,11 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
 #endif
     char *module_name;
     int i = 0;
-    int ret, dep;
+    int ret;
     bool export_symbols = false;
     static GHashTable *loaded_modules;
+    ModuleInfoList *modlist;
+    strList *sl;
 
     if (!g_module_supported()) {
         fprintf(stderr, "Module is not supported by system.\n");
@@ -304,17 +284,6 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
 
     module_name = g_strdup_printf("%s%s", prefix, lib_name);
 
-    for (dep = 0; dep < ARRAY_SIZE(module_deps); dep++) {
-        if (strcmp(module_name, module_deps[dep].name) == 0) {
-            /* we depend on another module */
-            module_load_one("", module_deps[dep].dep, false);
-        }
-        if (strcmp(module_name, module_deps[dep].dep) == 0) {
-            /* another module depends on us */
-            export_symbols = true;
-        }
-    }
-
     if (g_hash_table_contains(loaded_modules, module_name)) {
         g_free(module_name);
         return true;
@@ -324,6 +293,24 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
     module_load_path_init();
     module_load_modinfo();
 
+    for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) {
+        if (modlist->value->has_deps) {
+            if (strcmp(modlist->value->name, module_name) == 0) {
+                /* we depend on other module(s) */
+                for (sl = modlist->value->deps; sl != NULL; sl = sl->next) {
+                    module_load_one("", sl->value, false);
+                }
+            } else {
+                for (sl = modlist->value->deps; sl != NULL; sl = sl->next) {
+                    if (strcmp(module_name, sl->value) == 0) {
+                        /* another module depends on us */
+                        export_symbols = true;
+                    }
+                }
+            }
+        }
+    }
+
     for (i = 0; i < module_ndirs; i++) {
         fname = g_strdup_printf("%s/%s%s",
                 module_dirs[i], module_name, CONFIG_HOST_DSOSUF);
-- 
2.31.1

[PATCH v2 15/18] modules: use modinfo for qom load

[Gerd Hoffmann]

Use module database to figure which module implements a given QOM type.
Drop hard-coded object list.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/module.c | 83 +++++++++++++++++++--------------------------------
 1 file changed, 30 insertions(+), 53 deletions(-)

diff --git a/util/module.c b/util/module.c
index fd5fc059e14a..46bec1cfbec7 100644
--- a/util/module.c
+++ b/util/module.c
@@ -333,80 +333,57 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
     return success;
 }
 
-/*
- * Building devices and other qom objects modular is mostly useful in
- * case they have dependencies to external shared libraries, so we can
- * cut down the core qemu library dependencies.  Which is the case for
- * only a very few devices & objects.
- *
- * So with the expectation that this will be rather the exception than
- * the rule and the list will not gain that many entries, go with a
- * simple manually maintained list for now.
- *
- * The list must be sorted by module (module_load_qom_all() needs this).
- */
-static struct {
-    const char *type;
-    const char *prefix;
-    const char *module;
-} const qom_modules[] = {
-    { "ccid-card-passthru",    "hw-", "usb-smartcard"         },
-    { "ccid-card-emulated",    "hw-", "usb-smartcard"         },
-    { "usb-redir",             "hw-", "usb-redirect"          },
-    { "qxl-vga",               "hw-", "display-qxl"           },
-    { "qxl",                   "hw-", "display-qxl"           },
-    { "virtio-gpu-device",     "hw-", "display-virtio-gpu"    },
-    { "virtio-gpu-gl-device",  "hw-", "display-virtio-gpu-gl" },
-    { "vhost-user-gpu",        "hw-", "display-virtio-gpu"    },
-    { "virtio-gpu-pci-base",   "hw-", "display-virtio-gpu-pci" },
-    { "virtio-gpu-pci",        "hw-", "display-virtio-gpu-pci" },
-    { "virtio-gpu-gl-pci",     "hw-", "display-virtio-gpu-pci-gl" },
-    { "vhost-user-gpu-pci",    "hw-", "display-virtio-gpu-pci" },
-    { "virtio-gpu-ccw",        "hw-", "s390x-virtio-gpu-ccw"   },
-    { "virtio-vga-base",       "hw-", "display-virtio-vga"    },
-    { "virtio-vga",            "hw-", "display-virtio-vga"    },
-    { "virtio-vga-gl",         "hw-", "display-virtio-vga-gl" },
-    { "vhost-user-vga",        "hw-", "display-virtio-vga"    },
-    { "chardev-braille",       "chardev-", "baum"             },
-    { "chardev-spicevmc",      "chardev-", "spice"            },
-    { "chardev-spiceport",     "chardev-", "spice"            },
-};
+#ifdef CONFIG_MODULES
 
 static bool module_loaded_qom_all;
 
 void module_load_qom_one(const char *type)
 {
-    int i;
+    ModuleInfoList *modlist;
+    strList *sl;
 
     if (!type) {
         return;
     }
-    for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
-        if (strcmp(qom_modules[i].type, type) == 0) {
-            module_load_one(qom_modules[i].prefix,
-                            qom_modules[i].module,
-                            false);
-            return;
+
+    module_load_path_init();
+    module_load_modinfo();
+
+    for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) {
+        if (!modlist->value->has_objs) {
+            continue;
+        }
+        for (sl = modlist->value->objs; sl != NULL; sl = sl->next) {
+            if (strcmp(type, sl->value) == 0) {
+                module_load_one("", modlist->value->name, false);
+            }
         }
     }
 }
 
 void module_load_qom_all(void)
 {
-    int i;
+    ModuleInfoList *modlist;
 
     if (module_loaded_qom_all) {
         return;
     }
-    for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
-        if (i > 0 && (strcmp(qom_modules[i - 1].module,
-                             qom_modules[i].module) == 0 &&
-                      strcmp(qom_modules[i - 1].prefix,
-                             qom_modules[i].prefix) == 0)) {
-            /* one module implementing multiple types -> load only once */
+
+    module_load_path_init();
+    module_load_modinfo();
+
+    for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) {
+        if (!modlist->value->has_objs) {
             continue;
         }
-        module_load_one(qom_modules[i].prefix, qom_modules[i].module, true);
+        module_load_one("", modlist->value->name, false);
     }
     module_loaded_qom_all = true;
 }
+
+#else
+
+void module_load_qom_one(const char *type) {}
+void module_load_qom_all(void) {}
+
+#endif
-- 
2.31.1

[PATCH v2 16/18] modules: use modinfo for qemu opts load

[Gerd Hoffmann]

Use module database to figure which module adds given QemuOpts group.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 softmmu/vl.c        | 17 -----------------
 stubs/module-opts.c |  4 ----
 util/module.c       | 19 +++++++++++++++++++
 3 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index 326c1e908008..ba26a042b284 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2675,23 +2675,6 @@ void qmp_x_exit_preconfig(Error **errp)
     }
 }
 
-#ifdef CONFIG_MODULES
-void qemu_load_module_for_opts(const char *group)
-{
-    static bool spice_tried;
-    if (g_str_equal(group, "spice") && !spice_tried) {
-        ui_module_load_one("spice-core");
-        spice_tried = true;
-    }
-
-    static bool iscsi_tried;
-    if (g_str_equal(group, "iscsi") && !iscsi_tried) {
-        block_module_load_one("iscsi");
-        iscsi_tried = true;
-    }
-}
-#endif
-
 void qemu_init(int argc, char **argv, char **envp)
 {
     QemuOpts *opts;
diff --git a/stubs/module-opts.c b/stubs/module-opts.c
index a7d0e4ad6ead..5412429ea869 100644
--- a/stubs/module-opts.c
+++ b/stubs/module-opts.c
@@ -1,6 +1,2 @@
 #include "qemu/osdep.h"
 #include "qemu/config-file.h"
-
-void qemu_load_module_for_opts(const char *group)
-{
-}
diff --git a/util/module.c b/util/module.c
index 46bec1cfbec7..6e4199169c41 100644
--- a/util/module.c
+++ b/util/module.c
@@ -21,6 +21,7 @@
 #include "qemu/module.h"
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
+#include "qemu/config-file.h"
 #ifdef CONFIG_MODULE_UPGRADES
 #include "qemu-version.h"
 #endif
@@ -381,8 +382,26 @@ void module_load_qom_all(void)
     module_loaded_qom_all = true;
 }
 
+void qemu_load_module_for_opts(const char *group)
+{
+    ModuleInfoList *modlist;
+
+    module_load_path_init();
+    module_load_modinfo();
+
+    for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) {
+        if (!modlist->value->has_opts) {
+            continue;
+        }
+        if (strcmp(modlist->value->opts, group) == 0) {
+            module_load_one("", modlist->value->name, false);
+        }
+    }
+}
+
 #else
 
+void qemu_load_module_for_opts(const char *group) {}
 void module_load_qom_one(const char *type) {}
 void module_load_qom_all(void) {}
 
-- 
2.31.1

[PATCH v2 17/18] modules: check arch and block load on mismatch

[Gerd Hoffmann]

Add module_allow_arch() to set the target architecture.
In case a module is limited to some arch verify arches
match and ignore the module if not.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/qemu/module.h |  1 +
 softmmu/vl.c          |  3 +++
 util/module.c         | 15 +++++++++++++++
 3 files changed, 19 insertions(+)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index d3cab3c25a2f..7825f6d8c847 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -72,6 +72,7 @@ void module_call_init(module_init_type type);
 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);
 
 /*
  * macros to store module metadata in a .modinfo section.
diff --git a/softmmu/vl.c b/softmmu/vl.c
index ba26a042b284..96316774fcc9 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -126,6 +126,8 @@
 #include "sysemu/iothread.h"
 #include "qemu/guest-random.h"
 
+#include "config-host.h"
+
 #define MAX_VIRTIO_CONSOLES 1
 
 typedef struct BlockdevOptionsQueueEntry {
@@ -2723,6 +2725,7 @@ void qemu_init(int argc, char **argv, char **envp)
     error_init(argv[0]);
     qemu_init_exec_dir(argv[0]);
 
+    module_allow_arch(TARGET_NAME);
     qemu_init_subsystems();
 
     /* first pass of option parsing */
diff --git a/util/module.c b/util/module.c
index 6e4199169c41..564b8e3da760 100644
--- a/util/module.c
+++ b/util/module.c
@@ -122,6 +122,12 @@ void module_call_init(module_init_type type)
 static Modules *modinfo;
 static char *module_dirs[5];
 static int module_ndirs;
+static const char *module_arch;
+
+void module_allow_arch(const char *arch)
+{
+    module_arch = arch;
+}
 
 static void module_load_path_init(void)
 {
@@ -295,6 +301,14 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
     module_load_modinfo();
 
     for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) {
+        if (modlist->value->has_arch) {
+            if (strcmp(modlist->value->name, module_name) == 0) {
+                if (!module_arch ||
+                    strcmp(modlist->value->arch, module_arch) != 0) {
+                    return false;
+                }
+            }
+        }
         if (modlist->value->has_deps) {
             if (strcmp(modlist->value->name, module_name) == 0) {
                 /* we depend on other module(s) */
@@ -401,6 +415,7 @@ void qemu_load_module_for_opts(const char *group)
 
 #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) {}
-- 
2.31.1

[PATCH v2 18/18] [fixup] module_load_modinfo

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/module.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/util/module.c b/util/module.c
index 564b8e3da760..4f98cc74ae37 100644
--- a/util/module.c
+++ b/util/module.c
@@ -158,7 +158,7 @@ static void module_load_modinfo(void)
 {
     char *file, *json;
     FILE *fp;
-    int i, size;
+    int i, size, ret;
     Visitor *v;
     Error *errp = NULL;
 
@@ -185,8 +185,8 @@ static void module_load_modinfo(void)
     size = ftell(fp);
     fseek(fp, 0, SEEK_SET);
     json = g_malloc0(size + 1);
-    fread(json, size, 1, fp);
-    json[size] = 0;
+    ret = fread(json, 1, size, fp);
+    json[ret] = 0;
     fclose(fp);
 
     v = qobject_input_visitor_new_str(json, NULL, &errp);
-- 
2.31.1

Re: [PATCH v2 18/18] [fixup] module_load_modinfo

[Gerd Hoffmann]

On Thu, Jun 10, 2021 at 07:57:55AM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Oops.  That should have been squashed into patch #13.

take care,
  Gerd

Re: [PATCH v2 00/18] modules: add metadata database

[Claudio Fontana]

On 6/10/21 7:57 AM, Gerd Hoffmann wrote:
> This patch series adds support for module metadata.  Here are the pieces
> of the puzzle:
> 
>   (1) Macros are added to store metadata in a .modinfo elf section
>       (idea stolen from the linux kernel).
>   (2) A utility to scan modules, collect metadata from the .modinfo
>       sections, store it in a file (modinfo.json) for later consumption
>       by qemu.  Can also be easily inspected using 'jq'.
>   (3) Adding annotations to the modules we have.
>   (4) Drop hard-coded lists from utils/module.c
> 
> take care,
>   Gerd

The background has disappeared compared with V1.

V1 says:

"Background is that the hard-coded lists in util/module.c are somewhat
ugly and also wouldn't work very well with a large number of modules,
so I'm looking for something else."

Can you write more about what the actual high level goals of this series are?

We are in general making QEMU more and more difficult to get into,
requiring more and more investment for new contributors to get productive.

Is the additional complexity justified? What is the benefit, and is simplification a goal of the series as well?


> 
> Gerd Hoffmann (18):
>   modules: add metadata macros, add qxl module annotations
>   qapi: add ModuleInfo schema
>   modules: add qemu-modinfo utility
>   modules: add virtio-gpu module annotations
>   modules: add chardev module annotations
>   modules: add audio module annotations
>   modules: add usb-redir module annotations
>   modules: add ccid module annotations
>   modules: add ui module annotations
>   modules: add s390x module annotations
>   modules: add block module annotations
>   modules: add module_load_path_init helper
>   modules: load modinfo.json
>   modules: use modinfo for dependencies
>   modules: use modinfo for qom load
>   modules: use modinfo for qemu opts load
>   modules: check arch and block load on mismatch
>   [fixup] module_load_modinfo
> 
>  include/qemu/module.h           |  23 +++
>  audio/spiceaudio.c              |   2 +
>  block/iscsi-opts.c              |   1 +
>  chardev/baum.c                  |   1 +
>  chardev/spice.c                 |   4 +
>  hw/display/qxl.c                |   4 +
>  hw/display/vhost-user-gpu-pci.c |   1 +
>  hw/display/vhost-user-gpu.c     |   1 +
>  hw/display/vhost-user-vga.c     |   1 +
>  hw/display/virtio-gpu-base.c    |   1 +
>  hw/display/virtio-gpu-gl.c      |   3 +
>  hw/display/virtio-gpu-pci-gl.c  |   3 +
>  hw/display/virtio-gpu-pci.c     |   2 +
>  hw/display/virtio-gpu.c         |   1 +
>  hw/display/virtio-vga-gl.c      |   3 +
>  hw/display/virtio-vga.c         |   2 +
>  hw/s390x/virtio-ccw-gpu.c       |   3 +
>  hw/usb/ccid-card-emulated.c     |   1 +
>  hw/usb/ccid-card-passthru.c     |   1 +
>  hw/usb/redirect.c               |   1 +
>  qemu-modinfo.c                  | 270 ++++++++++++++++++++++++++++++
>  softmmu/vl.c                    |  20 +--
>  stubs/module-opts.c             |   4 -
>  ui/egl-headless.c               |   4 +
>  ui/gtk.c                        |   4 +
>  ui/sdl2.c                       |   4 +
>  ui/spice-app.c                  |   3 +
>  ui/spice-core.c                 |   5 +
>  util/module.c                   | 282 +++++++++++++++++++-------------
>  meson.build                     |  11 ++
>  qapi/meson.build                |   1 +
>  qapi/modules.json               |  36 ++++
>  qapi/qapi-schema.json           |   1 +
>  util/trace-events               |   3 +
>  34 files changed, 576 insertions(+), 131 deletions(-)
>  create mode 100644 qemu-modinfo.c
>  create mode 100644 qapi/modules.json
> 

Re: [PATCH v2 00/18] modules: add metadata database

[Gerd Hoffmann]

On Thu, Jun 10, 2021 at 10:32:39AM +0200, Claudio Fontana wrote:
> On 6/10/21 7:57 AM, Gerd Hoffmann wrote:
> > This patch series adds support for module metadata.  Here are the pieces
> > of the puzzle:
> > 
> >   (1) Macros are added to store metadata in a .modinfo elf section
> >       (idea stolen from the linux kernel).
> >   (2) A utility to scan modules, collect metadata from the .modinfo
> >       sections, store it in a file (modinfo.json) for later consumption
> >       by qemu.  Can also be easily inspected using 'jq'.
> >   (3) Adding annotations to the modules we have.
> >   (4) Drop hard-coded lists from utils/module.c
> > 
> > take care,
> >   Gerd
> 
> The background has disappeared compared with V1.
> 
> V1 says:
> 
> "Background is that the hard-coded lists in util/module.c are somewhat
> ugly and also wouldn't work very well with a large number of modules,
> so I'm looking for something else."

Well, it's point (4) now (a bit short indeed ...).

> Can you write more about what the actual high level goals of this series are?

Right now we have information about modules hard-coded in various places
in qemu.  Most obvious ones are module_deps[] and qom_modules[] (both in
util/module.c), but also qemu_load_module_for_opts() (in softmmu/vl.c)
and maybe more I missed.

So, when you go build some qom object modular today you'll have to go
add that to the qom_modules[] list.  With this patch series applied
you'll go add a 'module_obj("typename");' to your source file instead.

Same goes for other metadata, see the "add $foo module annotations"
patches for examples.

> We are in general making QEMU more and more difficult to get into,
> requiring more and more investment for new contributors to get
> productive.
> 
> Is the additional complexity justified? What is the benefit, and is
> simplification a goal of the series as well?

IMHO it is a simplification for developers.  Modules are more
self-contained with this in place.  You just add the annotation macros
and you are done.  No need to edit manually maintained lists at some
non-obvious place elsewhere in the tree.  Also no patch conflicts in
those lists.  We have type_init() + friends for simliar reasons.

The price for that simplification is the new utility needed which
collects and stores the metadata.  But that is something you only need
to worry about when actually working on module support.  The build
system keeps the database automatically up-to-date and most developers
shouldn't even notice that it is there.

take care,
  Gerd

Re: [PATCH v2 17/18] modules: check arch and block load on mismatch

[Daniel P. Berrangé]

On Thu, Jun 10, 2021 at 07:57:54AM +0200, Gerd Hoffmann wrote:
> Add module_allow_arch() to set the target architecture.
> In case a module is limited to some arch verify arches
> match and ignore the module if not.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/qemu/module.h |  1 +
>  softmmu/vl.c          |  3 +++
>  util/module.c         | 15 +++++++++++++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index d3cab3c25a2f..7825f6d8c847 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -72,6 +72,7 @@ void module_call_init(module_init_type type);
>  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);
>  
>  /*
>   * macros to store module metadata in a .modinfo section.
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ba26a042b284..96316774fcc9 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -126,6 +126,8 @@
>  #include "sysemu/iothread.h"
>  #include "qemu/guest-random.h"
>  
> +#include "config-host.h"
> +
>  #define MAX_VIRTIO_CONSOLES 1
>  
>  typedef struct BlockdevOptionsQueueEntry {
> @@ -2723,6 +2725,7 @@ void qemu_init(int argc, char **argv, char **envp)
>      error_init(argv[0]);
>      qemu_init_exec_dir(argv[0]);
>  
> +    module_allow_arch(TARGET_NAME);
>      qemu_init_subsystems();
>  
>      /* first pass of option parsing */
> diff --git a/util/module.c b/util/module.c
> index 6e4199169c41..564b8e3da760 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -122,6 +122,12 @@ void module_call_init(module_init_type type)
>  static Modules *modinfo;
>  static char *module_dirs[5];
>  static int module_ndirs;
> +static const char *module_arch;
> +
> +void module_allow_arch(const char *arch)
> +{
> +    module_arch = arch;
> +}
>  
>  static void module_load_path_init(void)
>  {
> @@ -295,6 +301,14 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>      module_load_modinfo();
>  
>      for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) {
> +        if (modlist->value->has_arch) {
> +            if (strcmp(modlist->value->name, module_name) == 0) {
> +                if (!module_arch ||
> +                    strcmp(modlist->value->arch, module_arch) != 0) {
> +                    return false;
> +                }
> +            }
> +        }

I have a little hard time following the code paths, but IIUC, with this
change, instead of "module.so" we would have multiple copies of something
like "module-$arch.so" ?  Then we load them all, read their modinfo section
and discard the ones with non-matching arch ?

If that is a correct understanding, then I wonder about the scalability of
it. We have 30 system emulator targets right now, so if we get even a few
arch specific modues, that's going to be alot of modules we're loading,
checking and discarding.

Wouldn't it be simpler if we just made use of the directory layout
/usr/lib64/qemu/$mod.so for common modules and /usr/lib64/qemu/$arch/$mod.so
for arch specific modules. That would let us load only the modules we know
are applicable for the system target arch and not need this post-load
filtering from metadata.

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 :|

Re: [PATCH v2 17/18] modules: check arch and block load on mismatch

[Gerd Hoffmann]

  Hi,

> >      for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) {
> > +        if (modlist->value->has_arch) {
> > +            if (strcmp(modlist->value->name, module_name) == 0) {
> > +                if (!module_arch ||
> > +                    strcmp(modlist->value->arch, module_arch) != 0) {
> > +                    return false;
> > +                }
> > +            }
> > +        }
> 
> I have a little hard time following the code paths, but IIUC, with this
> change, instead of "module.so" we would have multiple copies of something
> like "module-$arch.so" ?

Not yet with this series, but easily doable on top of this (see other
patch series sent today).

> Then we load them all, read their modinfo section
> and discard the ones with non-matching arch ?

No.  There is a utility reading the modinfo section (patch #2), write
out the info as json (patch #2 has the schema), then qemu will read that
json file (patch #13) ...

> for arch specific modules. That would let us load only the modules we know
> are applicable for the system target arch and not need this post-load
> filtering from metadata.

... so it's pre-load filtering, not post-load.

take care,
  Gerd

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Gerd Hoffmann]

  Hi Paolo,

> +if config_host.has_key('CONFIG_MODULES')
> +   qemu_modinfo = executable('qemu-modinfo', files('qemu-modinfo.c') + genh,
> +                             dependencies: [glib, qemuutil], install: have_tools)
> +   custom_target('modinfo.json',
> +                 input: [ softmmu_mods, block_mods ],
> +                 output: 'modinfo.json',
> +                 install: true,
> +                 install_dir: qemu_moddir,
> +                 command: [ qemu_modinfo, '.' ])
> +endif

I have trouble with this one.  Tried to declaring the modules as "input"
to make sure meson will only run qemu-modinfo when it is done building
the module.  But now and then I get build errors because qemu-modinfo
runs in parallel to a module build and qemu-modinfo throws an read error
because of that.

Any clue what is going on here?

thanks,
  Gerd

Re: [PATCH v2 17/18] modules: check arch and block load on mismatch

[Daniel P. Berrangé]

On Thu, Jun 10, 2021 at 02:57:21PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > >      for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) {
> > > +        if (modlist->value->has_arch) {
> > > +            if (strcmp(modlist->value->name, module_name) == 0) {
> > > +                if (!module_arch ||
> > > +                    strcmp(modlist->value->arch, module_arch) != 0) {
> > > +                    return false;
> > > +                }
> > > +            }
> > > +        }
> > 
> > I have a little hard time following the code paths, but IIUC, with this
> > change, instead of "module.so" we would have multiple copies of something
> > like "module-$arch.so" ?
> 
> Not yet with this series, but easily doable on top of this (see other
> patch series sent today).
> 
> > Then we load them all, read their modinfo section
> > and discard the ones with non-matching arch ?
> 
> No.  There is a utility reading the modinfo section (patch #2), write
> out the info as json (patch #2 has the schema), then qemu will read that
> json file (patch #13) ...

Ah ok, missed that.

Is the JSON file completely static, listing all modules that were built
regardless of whether they are currently installed, or would it need to
be refreshed when installing/uninstalling RPMs with modules ? I would
think we can do the former and simply handle missing modules on disk
fairly easily

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 :|

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Daniel P. Berrangé]

On Thu, Jun 10, 2021 at 03:04:24PM +0200, Gerd Hoffmann wrote:
>   Hi Paolo,
> 
> > +if config_host.has_key('CONFIG_MODULES')
> > +   qemu_modinfo = executable('qemu-modinfo', files('qemu-modinfo.c') + genh,
> > +                             dependencies: [glib, qemuutil], install: have_tools)
> > +   custom_target('modinfo.json',
> > +                 input: [ softmmu_mods, block_mods ],
> > +                 output: 'modinfo.json',
> > +                 install: true,
> > +                 install_dir: qemu_moddir,
> > +                 command: [ qemu_modinfo, '.' ])
> > +endif
> 
> I have trouble with this one.  Tried to declaring the modules as "input"
> to make sure meson will only run qemu-modinfo when it is done building
> the module.  But now and then I get build errors because qemu-modinfo
> runs in parallel to a module build and qemu-modinfo throws an read error
> because of that.

softmmu_mods and block_mods are both lists already, so this sets a
nested list and I wonder if it confuses meson  ? eg do you need

 input: softmmu_mods + block_mods

Alternatively there is option to do:

  'depends: softmmu_mods + block_mods

though the meson docs claim that's not required if they're
already listed against 'input:'


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 :|

Re: [PATCH v2 17/18] modules: check arch and block load on mismatch

[Gerd Hoffmann]

  Hi,

> Is the JSON file completely static, listing all modules that were built
> regardless of whether they are currently installed, or would it need to
> be refreshed when installing/uninstalling RPMs with modules ? I would
> think we can do the former and simply handle missing modules on disk
> fairly easily

We can do both.  The file is generated and installed as part of the
build/install process, and it can be simply used as-is even if some of
the modules are missing.

It's also possible to update the modinfo.json file in postinstall /
postuninstall by simply running qemu-modinfo, so only the modules
actually installed are listed there.

take care,
  Gerd

Re: [PATCH v2 02/18] qapi: add ModuleInfo schema

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Add QAPI schema for the module info database.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qapi/meson.build      |  1 +
>  qapi/modules.json     | 36 ++++++++++++++++++++++++++++++++++++
>  qapi/qapi-schema.json |  1 +
>  3 files changed, 38 insertions(+)
>  create mode 100644 qapi/modules.json
>
> diff --git a/qapi/meson.build b/qapi/meson.build
> index 376f4ceafe74..596aa5d71168 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -36,6 +36,7 @@ qapi_all_modules = [
>    'migration',
>    'misc',
>    'misc-target',
> +  'modules',
>    'net',
>    'pragma',
>    'qom',
> diff --git a/qapi/modules.json b/qapi/modules.json
> new file mode 100644
> index 000000000000..5420977d8765
> --- /dev/null
> +++ b/qapi/modules.json
> @@ -0,0 +1,36 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +

Missing a section heading like

   ##
   # = Loadable modules
   ##

Without it, the contents gets appended to whatever section precedes it
in qapi-schema.json, which is almost certainly not what you want.

> +##
> +# @ModuleInfo:
> +#
> +# qemu module metadata

It's spelled QEMU :)

Suggest "Loadable module meta-data".

> +#
> +# @name: module name
> +#
> +# @objs: list of qom objects implemented by the module.

s/qom objects/QOM types/

> +#
> +# @deps: list of other modules this module depends on.

Suggest to spell out that these are @name of other loadable modules.

> +#
> +# @arch: module architecture.

Semantics?

Should this be enum SysEmuTarget?

> +#
> +# @opts: qemu opts implemented by module.

Is this the name of a QemuOptsList?

Since this isn't a list, a module can only ever provide one
QemuOptsList.  Sure that's okay?

> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'ModuleInfo',
> +  'data': { 'name'  : 'str',
> +            '*objs' : ['str'],
> +            '*deps' : ['str'],
> +            '*arch' : 'str',
> +            '*opts' : 'str'}}
> +
> +##
> +# @Modules:
> +#
> +# qemu module list
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'Modules',
> +  'data': { 'list' : ['ModuleInfo']}}

This defines only types, no QMP commands or events.  Why do you need the
types to be QAPI types?

> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index 4912b9744e69..5baa511c2ff5 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -93,3 +93,4 @@
>  { 'include': 'audio.json' }
>  { 'include': 'acpi.json' }
>  { 'include': 'pci.json' }
> +{ 'include': 'modules.json' }

Is this the place you want the section to be?  Remember, generated
documentation follows source order.

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Paolo Bonzini]

On 10/06/21 15:13, Daniel P. Berrangé wrote:
> On Thu, Jun 10, 2021 at 03:04:24PM +0200, Gerd Hoffmann wrote:
>>    Hi Paolo,
>>
>>> +if config_host.has_key('CONFIG_MODULES')
>>> +   qemu_modinfo = executable('qemu-modinfo', files('qemu-modinfo.c') + genh,
>>> +                             dependencies: [glib, qemuutil], install: have_tools)
>>> +   custom_target('modinfo.json',
>>> +                 input: [ softmmu_mods, block_mods ],
>>> +                 output: 'modinfo.json',
>>> +                 install: true,
>>> +                 install_dir: qemu_moddir,
>>> +                 command: [ qemu_modinfo, '.' ])
>>> +endif
>>
>> I have trouble with this one.  Tried to declaring the modules as "input"
>> to make sure meson will only run qemu-modinfo when it is done building
>> the module.  But now and then I get build errors because qemu-modinfo
>> runs in parallel to a module build and qemu-modinfo throws an read error
>> because of that.
> 
> softmmu_mods and block_mods are both lists already, so this sets a
> nested list and I wonder if it confuses meson  ? eg do you need
> 
>   input: softmmu_mods + block_mods

No, that should be fine.  Perhaps it's because the inputs are not part 
of the command?  You can check what the build.ninja rule for 
modinfo.json looks like.

Also:

- it would be better to support both directories and file names, so that 
stale modules are not included in modinfo.json

- modinfo.json needs to be disabled on non-ELF platforms (x86, Windows). 
  One alternative is to use libbfd instead of including an ELF parser.

- If modinfo.json has to be installed, you need to build modinfo for the 
build machine in order to support cross compiling.  That however would 
require a cross libbfd, which is a pain.  Do you really need to install 
it?  Can the functionality instead be included in the main QEMU binary 
with a query-modules command or something like that.

Paolo

> Alternatively there is option to do:
> 
>    'depends: softmmu_mods + block_mods
> 
> though the meson docs claim that's not required if they're
> already listed against 'input:'

Re: [PATCH v2 17/18] modules: check arch and block load on mismatch

[Claudio Fontana]

On 6/10/21 2:35 PM, Daniel P. Berrangé wrote:
> On Thu, Jun 10, 2021 at 07:57:54AM +0200, Gerd Hoffmann wrote:
>> Add module_allow_arch() to set the target architecture.
>> In case a module is limited to some arch verify arches
>> match and ignore the module if not.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  include/qemu/module.h |  1 +
>>  softmmu/vl.c          |  3 +++
>>  util/module.c         | 15 +++++++++++++++
>>  3 files changed, 19 insertions(+)
>>
>> diff --git a/include/qemu/module.h b/include/qemu/module.h
>> index d3cab3c25a2f..7825f6d8c847 100644
>> --- a/include/qemu/module.h
>> +++ b/include/qemu/module.h
>> @@ -72,6 +72,7 @@ void module_call_init(module_init_type type);
>>  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);
>>  
>>  /*
>>   * macros to store module metadata in a .modinfo section.
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index ba26a042b284..96316774fcc9 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -126,6 +126,8 @@
>>  #include "sysemu/iothread.h"
>>  #include "qemu/guest-random.h"
>>  
>> +#include "config-host.h"
>> +
>>  #define MAX_VIRTIO_CONSOLES 1
>>  
>>  typedef struct BlockdevOptionsQueueEntry {
>> @@ -2723,6 +2725,7 @@ void qemu_init(int argc, char **argv, char **envp)
>>      error_init(argv[0]);
>>      qemu_init_exec_dir(argv[0]);
>>  
>> +    module_allow_arch(TARGET_NAME);
>>      qemu_init_subsystems();
>>  
>>      /* first pass of option parsing */
>> diff --git a/util/module.c b/util/module.c
>> index 6e4199169c41..564b8e3da760 100644
>> --- a/util/module.c
>> +++ b/util/module.c
>> @@ -122,6 +122,12 @@ void module_call_init(module_init_type type)
>>  static Modules *modinfo;
>>  static char *module_dirs[5];
>>  static int module_ndirs;
>> +static const char *module_arch;
>> +
>> +void module_allow_arch(const char *arch)
>> +{
>> +    module_arch = arch;
>> +}
>>  
>>  static void module_load_path_init(void)
>>  {
>> @@ -295,6 +301,14 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>      module_load_modinfo();
>>  
>>      for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) {
>> +        if (modlist->value->has_arch) {
>> +            if (strcmp(modlist->value->name, module_name) == 0) {
>> +                if (!module_arch ||
>> +                    strcmp(modlist->value->arch, module_arch) != 0) {
>> +                    return false;
>> +                }
>> +            }
>> +        }
> 
> I have a little hard time following the code paths, but IIUC, with this
> change, instead of "module.so" we would have multiple copies of something
> like "module-$arch.so" ?  Then we load them all, read their modinfo section
> and discard the ones with non-matching arch ?
> 
> If that is a correct understanding, then I wonder about the scalability of
> it. We have 30 system emulator targets right now, so if we get even a few
> arch specific modues, that's going to be alot of modules we're loading,
> checking and discarding.
> 
> Wouldn't it be simpler if we just made use of the directory layout
> /usr/lib64/qemu/$mod.so for common modules and /usr/lib64/qemu/$arch/$mod.so
> for arch specific modules. That would let us load only the modules we know
> are applicable for the system target arch and not need this post-load
> filtering from metadata.
> 
> Regards,
> Daniel
> 

agreed.

Claudio

Re: [PATCH v2 17/18] modules: check arch and block load on mismatch

[Claudio Fontana]

On 6/10/21 4:03 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>> Is the JSON file completely static, listing all modules that were built
>> regardless of whether they are currently installed, or would it need to
>> be refreshed when installing/uninstalling RPMs with modules ? I would
>> think we can do the former and simply handle missing modules on disk
>> fairly easily
> 
> We can do both.  The file is generated and installed as part of the
> build/install process, and it can be simply used as-is even if some of
> the modules are missing.
> 
> It's also possible to update the modinfo.json file in postinstall /
> postuninstall by simply running qemu-modinfo, so only the modules
> actually installed are listed there.
> 
> take care,
>   Gerd
> 
> 

I fail to see why that extra complication is needed at all.

Why don't we just build the modules for the targets we intend to build,
and install them as .so files in a target arch directory?

What problem is the json solving? Sorry if obvious.

Thanks,

Claudio  

Re: [PATCH v2 00/18] modules: add metadata database

[Claudio Fontana]

On 6/10/21 11:54 AM, Gerd Hoffmann wrote:
> On Thu, Jun 10, 2021 at 10:32:39AM +0200, Claudio Fontana wrote:
>> On 6/10/21 7:57 AM, Gerd Hoffmann wrote:
>>> This patch series adds support for module metadata.  Here are the pieces
>>> of the puzzle:
>>>
>>>   (1) Macros are added to store metadata in a .modinfo elf section
>>>       (idea stolen from the linux kernel).
>>>   (2) A utility to scan modules, collect metadata from the .modinfo
>>>       sections, store it in a file (modinfo.json) for later consumption
>>>       by qemu.  Can also be easily inspected using 'jq'.
>>>   (3) Adding annotations to the modules we have.
>>>   (4) Drop hard-coded lists from utils/module.c
>>>
>>> take care,
>>>   Gerd
>>
>> The background has disappeared compared with V1.
>>
>> V1 says:
>>
>> "Background is that the hard-coded lists in util/module.c are somewhat
>> ugly and also wouldn't work very well with a large number of modules,
>> so I'm looking for something else."
> 
> Well, it's point (4) now (a bit short indeed ...).
> 
>> Can you write more about what the actual high level goals of this series are?
> 
> Right now we have information about modules hard-coded in various places
> in qemu.  Most obvious ones are module_deps[] and qom_modules[] (both in
> util/module.c), but also qemu_load_module_for_opts() (in softmmu/vl.c)
> and maybe more I missed.

Maybe a good idea to find out.

> 
> So, when you go build some qom object modular today you'll have to go
> add that to the qom_modules[] list.  With this patch series applied
> you'll go add a 'module_obj("typename");' to your source file instead.
> 
> Same goes for other metadata, see the "add $foo module annotations"
> patches for examples.
> 
>> We are in general making QEMU more and more difficult to get into,
>> requiring more and more investment for new contributors to get
>> productive.
>>
>> Is the additional complexity justified? What is the benefit, and is
>> simplification a goal of the series as well?
> 
> IMHO it is a simplification for developers.  Modules are more

So the whole endevour here is to remove the need to update modules in two/three places?

It might simplify life for developers adding a module,
but it is at a cost for the developers trying to keep the plumbing to work in my opinion.

Based on the information you provided, the reason this whole series exists seems to be to remove the need to update modules in multiple places.

Should the design focus be on that and that only?

Is there a real need to copy over the mechanism from the kernel, or are the requirements actually different/simpler here?



> self-contained with this in place.  You just add the annotation macros
> and you are done.  No need to edit manually maintained lists at some
> non-obvious place elsewhere in the tree.  Also no patch conflicts in
> those lists.  We have type_init() + friends for simliar reasons.
> 
> The price for that simplification is the new utility needed which
> collects and stores the metadata.  But that is something you only need
> to worry about when actually working on module support.  The build
> system keeps the database automatically up-to-date and most developers
> shouldn't even notice that it is there.
> 
> take care,
>   Gerd
> 
> 

Re: [PATCH v2 17/18] modules: check arch and block load on mismatch

[Gerd Hoffmann]

  Hi,

> > We can do both.  The file is generated and installed as part of the
> > build/install process, and it can be simply used as-is even if some of
> > the modules are missing.
> > 
> > It's also possible to update the modinfo.json file in postinstall /
> > postuninstall by simply running qemu-modinfo, so only the modules
> > actually installed are listed there.
> 
> I fail to see why that extra complication is needed at all.
> 
> Why don't we just build the modules for the targets we intend to build,
> and install them as .so files in a target arch directory?

There is more meta-data we need for modules:  Which QOM types are
implemented by which module (for on-demand loading), dependencies
between modules and also which command line options (aka QemuOpts)
are handled by which modules.

Possibly more in the future, maybe we'll support modules registering
monitor commands dynamically some day (like usb-host implementing
"info usbhost" or tcg implementing "info jit" + "info opcount") and
we'd like store that information in the module database too.

If we have such a module database anyway it IMHO makes alot of sense
to simply store the target arch there too instead of using something
else.

> What problem is the json solving?

Well, the goal is to store meta-data about modules, in a way which:

  (a) Doesn't require manually maintained lists.  This is what we have
      right now (arrays in utils/module.c).  Works ok-ish for a small
      number of modules, but becomes increasingly problematic with the
      growing number of modules.
  (b) Doesn't require opening each individual module on each qemu run
      to get the information.

I'm not particularly attached to using json for that, it is just that
we already have infrastructure to parse/serialize structs from/to json
because we need that for qapi anyway.

take care,
  Gerd

Re: [PATCH v2 17/18] modules: check arch and block load on mismatch

[Daniel P. Berrangé]

On Mon, Jun 14, 2021 at 03:44:53PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > We can do both.  The file is generated and installed as part of the
> > > build/install process, and it can be simply used as-is even if some of
> > > the modules are missing.
> > > 
> > > It's also possible to update the modinfo.json file in postinstall /
> > > postuninstall by simply running qemu-modinfo, so only the modules
> > > actually installed are listed there.
> > 
> > I fail to see why that extra complication is needed at all.
> > 
> > Why don't we just build the modules for the targets we intend to build,
> > and install them as .so files in a target arch directory?
> 
> There is more meta-data we need for modules:  Which QOM types are
> implemented by which module (for on-demand loading), dependencies
> between modules and also which command line options (aka QemuOpts)
> are handled by which modules.
> 
> Possibly more in the future, maybe we'll support modules registering
> monitor commands dynamically some day (like usb-host implementing
> "info usbhost" or tcg implementing "info jit" + "info opcount") and
> we'd like store that information in the module database too.
> 
> If we have such a module database anyway it IMHO makes alot of sense
> to simply store the target arch there too instead of using something
> else.
> 
> > What problem is the json solving?
> 
> Well, the goal is to store meta-data about modules, in a way which:
> 
>   (a) Doesn't require manually maintained lists.  This is what we have
>       right now (arrays in utils/module.c).  Works ok-ish for a small
>       number of modules, but becomes increasingly problematic with the
>       growing number of modules.
>   (b) Doesn't require opening each individual module on each qemu run
>       to get the information.
> 
> I'm not particularly attached to using json for that, it is just that
> we already have infrastructure to parse/serialize structs from/to json
> because we need that for qapi anyway.

If we can generate json, we could generate .c code which has all the
data statically declared and just link it in to QEMU. If we don't need
ability to update the metadata post-build then it would be equivalent
functionally.  If we need to be able to update the metadata to precisely
matcch the set of installed modules though, then a separate json file
looks best.

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 :|

Re: [PATCH v2 00/18] modules: add metadata database

[Gerd Hoffmann]

 Hi,

> Based on the information you provided, the reason this whole series
> exists seems to be to remove the need to update modules in multiple
> places.

Well, I'm trying to improve the way we handle module meta-data (see
other mail just send for details).

> Is there a real need to copy over the mechanism from the kernel, or
> are the requirements actually different/simpler here?

Even though the exact kind of meta-data is different (qemu wants know
qom types implemented by module, kernel wants know what hardware a
module can drive) the fundamental problem is the same:  We need some
meta-data about modules for lookup.  The workflow is simliar too:
The meta-data is stored in a .modinfo elf section.  Then a utility
collects that data and stores them in a database.

For the kernel 'depmod' stores the data in
/lib/modules/$version/modules.$name (with modules.alias being the
largest database).

I'm trying to have qemu-modinfo store this in modinfo.json.

take care,
  Gerd

Re: [PATCH v2 17/18] modules: check arch and block load on mismatch

[Gerd Hoffmann]

  Hi,

> > I'm not particularly attached to using json for that, it is just that
> > we already have infrastructure to parse/serialize structs from/to json
> > because we need that for qapi anyway.
> 
> If we can generate json, we could generate .c code which has all the
> data statically declared and just link it in to QEMU.

Yes, I think that would work too.

take care,
  Gerd

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Paolo Bonzini]

On 14/06/21 10:34, Paolo Bonzini wrote:
> - modinfo.json needs to be disabled on non-ELF platforms (x86, Windows). 
>   One alternative is to use libbfd instead of including an ELF parser.
> 
> - If modinfo.json has to be installed, you need to build modinfo for the 
> build machine in order to support cross compiling.  That however would 
> require a cross libbfd, which is a pain.  Do you really need to install 
> it?  Can the functionality instead be included in the main QEMU binary 
> with a query-modules command or something like that.

Another possibility to eschew .o parsing is to add something like this 
to the sources

#ifdef QEMU_MODINFO
#define MODULE_METADATA(key, value) \
    =<>= MODINFO key value
#else
#define MODULE_METADATA(key, value)
#endif

MODULE_METADATA("opts", "spice")

A Python script could parse compile_commands.json, add -E -DQEMU_MODINFO 
to the command-line option, and look in the output for the metadata.

Paolo

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Gerd Hoffmann]

  Hi,

> > softmmu_mods and block_mods are both lists already, so this sets a
> > nested list and I wonder if it confuses meson  ? eg do you need
> > 
> >   input: softmmu_mods + block_mods
> 
> No, that should be fine.  Perhaps it's because the inputs are not part of
> the command?  You can check what the build.ninja rule for modinfo.json looks
> like.
> 
> Also:
> 
> - it would be better to support both directories and file names, so that
> stale modules are not included in modinfo.json

When turning qemu-modinfo into a pure build-utility (see below) there is
no reason to not explicitly list all module files on the command line.

> - modinfo.json needs to be disabled on non-ELF platforms (x86, Windows).

On windows modules are not supported.
Do we have any other non-ELF platforms?

> One alternative is to use libbfd instead of including an ELF parser.
> 
> - If modinfo.json has to be installed, you need to build modinfo for the
> build machine in order to support cross compiling.  That however would
> require a cross libbfd, which is a pain.  Do you really need to install it?

Do you mean installing modinfo.json or installing qemu-modinfo?  For the
latter I can see that not installing it removes some cross-compiling
headaches.  And, yes, we can turn this into a pure build utility
generating a static database (be it json or -- as suggested by Daniel --
a C source file with the data structures).

> Can the functionality instead be included in the main QEMU binary with a
> query-modules command or something like that.

Well, the meta-data database is meant for qemu itself, not external
users.  I was just using json because the infrastructure to serialize +
parse json exists.  Not sure a "query-modinfo" command makes sense.
Would be trivial to implement though if libvirt finds this useful
(assuming we stick to json).

I was toying with a completely different idea:  Have a "qemu
-generate-modinfo".  That would basically try to load each module, while
doing so record the type_register() (+ other register) calls the module
is doing, when done write out the database with the registrations done
by each module.

Problem with that approach is that it doesn't work for module
dependencies ...

Comments on the idea?  Suggestions for the module dependency problem?
Could maybe libbfd be used to find module (symbol) dependencies
automatically without writing a full dynamic linker?

take care,
  Gerd

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Daniel P. Berrangé]

On Mon, Jun 14, 2021 at 05:01:59PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > softmmu_mods and block_mods are both lists already, so this sets a
> > > nested list and I wonder if it confuses meson  ? eg do you need
> > > 
> > >   input: softmmu_mods + block_mods
> > 
> > No, that should be fine.  Perhaps it's because the inputs are not part of
> > the command?  You can check what the build.ninja rule for modinfo.json looks
> > like.
> > 
> > Also:
> > 
> > - it would be better to support both directories and file names, so that
> > stale modules are not included in modinfo.json
> 
> When turning qemu-modinfo into a pure build-utility (see below) there is
> no reason to not explicitly list all module files on the command line.
> 
> > - modinfo.json needs to be disabled on non-ELF platforms (x86, Windows).
> 
> On windows modules are not supported.

Does anyone recall why modules aren't supported on Windows

> Do we have any other non-ELF platforms?

macOS uses dynlib IIUC ?

> Problem with that approach is that it doesn't work for module
> dependencies ...
> 
> Comments on the idea?  Suggestions for the module dependency problem?
> Could maybe libbfd be used to find module (symbol) dependencies
> automatically without writing a full dynamic linker?

Is there any value in exploring use of libclang ?  It gives us a real
C parser that we can use to extract information from the C source. In
libvirt we have experimental patches (not yet merged) using libclang to
auto-generate XML parser helpers from struct annotations. It is quite
nice compared to any other hacks for extracting information from C
source files without using a proper parser.  libclang can be accessed
from Python3 via its bindings and IIUC should be usable on all our
build platforms


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 :|

Re: [PATCH v2 02/18] qapi: add ModuleInfo schema

[Gerd Hoffmann]

  Hi,

> > +# @arch: module architecture.
> 
> Semantics?
> 
> Should this be enum SysEmuTarget?

Probably, will check ...

> > +# @opts: qemu opts implemented by module.
> 
> Is this the name of a QemuOptsList?
> 
> Since this isn't a list, a module can only ever provide one
> QemuOptsList.  Sure that's okay?

For the current two in-tree cases yes, and I don't expect this to change
in the future.  We could turn this into a list though to make it
future-proof.

> > +{ 'struct': 'Modules',
> > +  'data': { 'list' : ['ModuleInfo']}}
> 
> This defines only types, no QMP commands or events.  Why do you need the
> types to be QAPI types?

Want re-use the code to serialize/parse json from/to structs.
(see patches #3 + #13).

> > --- a/qapi/qapi-schema.json
> > +++ b/qapi/qapi-schema.json
> > @@ -93,3 +93,4 @@
> >  { 'include': 'audio.json' }
> >  { 'include': 'acpi.json' }
> >  { 'include': 'pci.json' }
> > +{ 'include': 'modules.json' }
> 
> Is this the place you want the section to be?  Remember, generated
> documentation follows source order.

Ah, *this* the ordering is important for.  I'll check, was just
appending to the end of the list ...

thanks,
  Gerd

Re: [PATCH v2 02/18] qapi: add ModuleInfo schema

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> > +# @arch: module architecture.
>> 
>> Semantics?
>> 
>> Should this be enum SysEmuTarget?
>
> Probably, will check ...
>
>> > +# @opts: qemu opts implemented by module.
>> 
>> Is this the name of a QemuOptsList?
>> 
>> Since this isn't a list, a module can only ever provide one
>> QemuOptsList.  Sure that's okay?
>
> For the current two in-tree cases yes, and I don't expect this to change
> in the future.  We could turn this into a list though to make it
> future-proof.

If it's not much of a bother, then why not?

>> > +{ 'struct': 'Modules',
>> > +  'data': { 'list' : ['ModuleInfo']}}
>> 
>> This defines only types, no QMP commands or events.  Why do you need the
>> types to be QAPI types?
>
> Want re-use the code to serialize/parse json from/to structs.
> (see patches #3 + #13).

Okay, that's fair.

[...]

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Gerd Hoffmann]

  Hi,

> Another possibility to eschew .o parsing is to add something like this to
> the sources
> 
> #ifdef QEMU_MODINFO
> #define MODULE_METADATA(key, value) \
>    =<>= MODINFO key value
> #else
> #define MODULE_METADATA(key, value)
> #endif
> 
> MODULE_METADATA("opts", "spice")
> 
> A Python script could parse compile_commands.json, add -E -DQEMU_MODINFO to
> the command-line option, and look in the output for the metadata.

Hmm, worth trying, although I guess it would be easier to code this up
straight in meson.build and pull the information you need out of the
source set, especially as you'll know then which source files are
compiled into which module.

take care,
  Gerd

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Gerd Hoffmann]

> > Problem with that approach is that it doesn't work for module
> > dependencies ...
> > 
> > Comments on the idea?  Suggestions for the module dependency problem?
> > Could maybe libbfd be used to find module (symbol) dependencies
> > automatically without writing a full dynamic linker?
> 
> Is there any value in exploring use of libclang ?  It gives us a real
> C parser that we can use to extract information from the C source. In
> libvirt we have experimental patches (not yet merged) using libclang to
> auto-generate XML parser helpers from struct annotations. It is quite
> nice compared to any other hacks for extracting information from C
> source files without using a proper parser.  libclang can be accessed
> from Python3 via its bindings and IIUC should be usable on all our
> build platforms

Could you do something along the lines of ...

  (1) find constructors
  (2) find type_register() calls in the constructor and the
      TypeInfo structs passed to those calls.
  (3) inspect the TypeInfo structs to figure the QOM type names.

... with libclang?

take care,
  Gerd

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Gerd Hoffmann]

On Tue, Jun 15, 2021 at 06:49:15AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > Another possibility to eschew .o parsing is to add something like this to
> > the sources
> > 
> > #ifdef QEMU_MODINFO
> > #define MODULE_METADATA(key, value) \
> >    =<>= MODINFO key value
> > #else
> > #define MODULE_METADATA(key, value)
> > #endif
> > 
> > MODULE_METADATA("opts", "spice")
> > 
> > A Python script could parse compile_commands.json, add -E -DQEMU_MODINFO to
> > the command-line option, and look in the output for the metadata.
> 
> Hmm, worth trying, although I guess it would be easier to code this up
> straight in meson.build and pull the information you need out of the
> source set, especially as you'll know then which source files are
> compiled into which module.

Hmm, looks like I actually need both.  Seems there is no easy way to get
the cflags out of a source_set to construct a cpp command line.  Pulling
this out of compile_commands.json with jq works though.

With the patch below I get nice ${module}.modinfo files with the
metadata, now I only need a (probably python) script to collect
them and create a modinfo.c which we can link into qemu.

take care,
  Gerd

From 3edc033935d2dd4ec607ac6395548a327151ad06 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 15 Jun 2021 09:23:52 +0200
Subject: [PATCH] try -DQEMU_MODINFO

---
 include/qemu/module.h | 22 ++++++----------------
 meson.build           |  7 +++++++
 scripts/modinfo.sh    |  8 ++++++++
 3 files changed, 21 insertions(+), 16 deletions(-)
 create mode 100644 scripts/modinfo.sh

diff --git a/include/qemu/module.h b/include/qemu/module.h
index 7825f6d8c847..5acfa423dc4f 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -74,22 +74,12 @@ void module_load_qom_one(const char *type);
 void module_load_qom_all(void);
 void module_allow_arch(const char *arch);
 
-/*
- * macros to store module metadata in a .modinfo section.
- * qemu-modinfo utility will collect the metadata.
- *
- * Use "objdump -t -s -j .modinfo ${module}.so" to inspect.
- */
-
-#define ___PASTE(a, b) a##b
-#define __PASTE(a, b) ___PASTE(a, b)
-
-#define modinfo(kind, value)                             \
-    static const char __PASTE(kind, __LINE__)[]          \
-        __attribute__((__used__))                        \
-        __attribute__((section(".modinfo")))             \
-        __attribute__((aligned(1)))                      \
-        = stringify(kind) "=" value
+#ifdef QEMU_MODINFO
+# define modinfo(kind, value) \
+    MODINFO_START kind value MODINFO_END
+#else
+# define modinfo(kind, value)
+#endif
 
 #define module_obj(name) modinfo(obj, name)
 #define module_dep(name) modinfo(dep, name)
diff --git a/meson.build b/meson.build
index 46ebc07dbb67..d8661755adf9 100644
--- a/meson.build
+++ b/meson.build
@@ -2050,12 +2050,19 @@ target_modules += { 'accel' : { 'qtest': qtest_module_ss,
 
 block_mods = []
 softmmu_mods = []
+modinfo = find_program('scripts/modinfo.sh')
 foreach d, list : modules
   foreach m, module_ss : list
     if enable_modules and targetos != 'windows'
       module_ss = module_ss.apply(config_all, strict: false)
       sl = static_library(d + '-' + m, [genh, module_ss.sources()],
                           dependencies: [modulecommon, module_ss.dependencies()], pic: true)
+      custom_target(d + '-' + m + '.modinfo',
+                    output: d + '-' + m + '.modinfo',
+                    input: module_ss.sources(),
+                    build_by_default: true, # to be removed when added to a target
+                    capture: true,
+                    command: [modinfo, '@INPUT@'])
       if d == 'block'
         block_mods += sl
       else
diff --git a/scripts/modinfo.sh b/scripts/modinfo.sh
new file mode 100644
index 000000000000..8f4495d4523d
--- /dev/null
+++ b/scripts/modinfo.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+for input in "$@"; do
+    command=$(jq  -r ".[] | select(.file == \"$input\") | .command " compile_commands.json)
+    command="${command%% -M*}"
+    command="$command -DQEMU_MODINFO -E $input"
+    $command
+done | grep MODINFO
+exit 0
-- 
2.31.1

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Daniel P. Berrangé]

On Tue, Jun 15, 2021 at 06:54:41AM +0200, Gerd Hoffmann wrote:
> > > Problem with that approach is that it doesn't work for module
> > > dependencies ...
> > > 
> > > Comments on the idea?  Suggestions for the module dependency problem?
> > > Could maybe libbfd be used to find module (symbol) dependencies
> > > automatically without writing a full dynamic linker?
> > 
> > Is there any value in exploring use of libclang ?  It gives us a real
> > C parser that we can use to extract information from the C source. In
> > libvirt we have experimental patches (not yet merged) using libclang to
> > auto-generate XML parser helpers from struct annotations. It is quite
> > nice compared to any other hacks for extracting information from C
> > source files without using a proper parser.  libclang can be accessed
> > from Python3 via its bindings and IIUC should be usable on all our
> > build platforms
> 
> Could you do something along the lines of ...
> 
>   (1) find constructors
>   (2) find type_register() calls in the constructor and the
>       TypeInfo structs passed to those calls.
>   (3) inspect the TypeInfo structs to figure the QOM type names.
> 
> ... with libclang?

In theory that should all be doable. I'm not very familiar myself with
libclang, but IIUC you basically get given the abstract syntax tree
and have to traverse it to find the info you want. This is kind of
low level but the info should all be there if you know how to find
it.

As an answer to (1) and part of (2), the following code I hacked up
quickly, finds all constructors that contain "type_register" calls.
Would need to find the arg to the calls and match that up to the
static structs too.


from clang.cindex import Index, CursorKind

def is_constructor(cursor):
    for bit in cursor.get_children():
        if bit.kind == CursorKind.UNEXPOSED_ATTR:
            for tok in bit.get_tokens():
                if tok.spelling == "constructor":
                    return True
    return False

def find_constructors(cursor):
    for cursor in cursor.get_children():
        if cursor.kind == CursorKind.FUNCTION_DECL:
            if is_constructor(cursor):
                yield cursor

def has_type_register(cursor):
    for cursor in constructor.get_children():
        if cursor.kind == CursorKind.COMPOUND_STMT:
            for c in cursor.get_children():
                if c.kind == CursorKind.CALL_EXPR:
                    if c.displayname == "type_register":
                        return True
    return False
                
index = Index.create()
tu = index.parse("demo.c")
for constructor in find_constructors(tu.cursor):
    has_reg = has_type_register(constructor)
    if has_reg:
        print("Constructor with type_register: " + constructor.displayname)


I tested with a short example

#include <stdio.h>

struct Foo {
  int bar;
};

static void type_register(struct Foo *foo) {
  printf("%d\n", foo->bar);
}
  
__attribute__((constructor)) static void startit(void) 
{
  static struct Foo foo = { 42 };
  type_register(&foo);
}

int main(int argc, char **argv) {
  printf("Running main\n");
}


$ python demo.py
Constructor with type register: startit()


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 :|

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Gerd Hoffmann]

> > > A Python script could parse compile_commands.json, add -E -DQEMU_MODINFO to
> > > the command-line option, and look in the output for the metadata.
> > 
> > Hmm, worth trying, although I guess it would be easier to code this up
> > straight in meson.build and pull the information you need out of the
> > source set, especially as you'll know then which source files are
> > compiled into which module.
> 
> Hmm, looks like I actually need both.  Seems there is no easy way to get
> the cflags out of a source_set to construct a cpp command line.  Pulling
> this out of compile_commands.json with jq works though.

Well, easy until I look at target-specific modules where the
"source file" -> "command" mapping isn't unique any more.  Which makes
this route less attractive ...

Any idea on getting the cflags in meson.build  Or maybe I can somehow
ask meson to run the cpp pass only for a given source set?

thanks,
  Gerd

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Paolo Bonzini]

On 15/06/21 15:07, Gerd Hoffmann wrote:
>> Hmm, looks like I actually need both.  Seems there is no easy way to get
>> the cflags out of a source_set to construct a cpp command line.  Pulling
>> this out of compile_commands.json with jq works though.
> Well, easy until I look at target-specific modules where the
> "source file" -> "command" mapping isn't unique any more.  Which makes
> this route less attractive ...

I was almost giving up... but it looks like the result of 
extract_all_objects(recursive: true) can be passed to custom_target(). 
Then you can match it after compile_commands.json's "output" key.

Paolo

> Any idea on getting the cflags in meson.build  Or maybe I can somehow
> ask meson to run the cpp pass only for a given source set?

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Gerd Hoffmann]

On Tue, Jun 15, 2021 at 03:35:44PM +0200, Paolo Bonzini wrote:
> On 15/06/21 15:07, Gerd Hoffmann wrote:
> > > Hmm, looks like I actually need both.  Seems there is no easy way to get
> > > the cflags out of a source_set to construct a cpp command line.  Pulling
> > > this out of compile_commands.json with jq works though.
> > Well, easy until I look at target-specific modules where the
> > "source file" -> "command" mapping isn't unique any more.  Which makes
> > this route less attractive ...
> 
> I was almost giving up... but it looks like the result of
> extract_all_objects(recursive: true) can be passed to custom_target(). Then
> you can match it after compile_commands.json's "output" key.

Seems the custom_target commands do not land in compile_commands.json.

But I have figured meanwhile that looking for the target name in the
command line works reliable.  That will will match
-DCONFIG_TARGET="${target}-config-target.h".

Current WIP patch below, seems to work nicely.  Whole patch series needs
an overhaul now ...

From 70c96336e38f1a7f114aee2c7ef023546cc560e5 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 15 Jun 2021 09:23:52 +0200
Subject: [PATCH] try -DQEMU_MODINFO

---
 scripts/modinfo-collect.py  | 66 +++++++++++++++++++++++++++++++++++++
 scripts/modinfo-generate.py | 61 ++++++++++++++++++++++++++++++++++
 include/qemu/module.h       | 33 ++++++++++---------
 meson.build                 | 28 +++++++++++++++-
 4 files changed, 171 insertions(+), 17 deletions(-)
 create mode 100755 scripts/modinfo-collect.py
 create mode 100755 scripts/modinfo-generate.py

diff --git a/scripts/modinfo-collect.py b/scripts/modinfo-collect.py
new file mode 100755
index 000000000000..b804099cfd1e
--- /dev/null
+++ b/scripts/modinfo-collect.py
@@ -0,0 +1,66 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
+
+import os
+import sys
+import json
+import shlex
+import subprocess
+
+def find_command(src, target, compile_commands):
+    for command in compile_commands:
+        if command['file'] != src:
+            continue
+        if target != '' and command['command'].find(target) == -1:
+            continue
+        return command['command']
+    return 'false'
+
+def process_command(src, command):
+    skip = False
+    arg = False
+    out = []
+    for item in shlex.split(command):
+        if arg:
+            out.append(x)
+            arg = False
+            continue
+        if skip:
+            skip = False
+            continue
+        if item == '-MF' or item == '-MQ' or item == '-o':
+            skip = True
+            continue
+        if item == '-c':
+            skip = True
+            continue
+        out.append(item)
+    out.append('-DQEMU_MODINFO')
+    out.append('-E')
+    out.append(src)
+    return out
+
+def main(args):
+    target = ''
+    if args[0] == '--target':
+        args.pop(0)
+        target = args.pop(0)
+        print("MODINFO_DEBUG target %s" % target)
+        arch = target[:-8] # cut '-softmmu'
+        print("MODINFO_START arch \"%s\" MODINFO_END" % arch)
+    with open('compile_commands.json') as f:
+        compile_commands = json.load(f)
+    for src in args:
+        print("MODINFO_DEBUG src %s" % src)
+        command = find_command(src, target, compile_commands)
+        cmdline = process_command(src, command)
+        print("MODINFO_DEBUG cmd", cmdline)
+        result = subprocess.run(cmdline, capture_output = True, text = True)
+        if result.returncode != 0:
+            sys.exit(result.returncode)
+        for line in result.stdout.split('\n'):
+            if line.find('MODINFO') != -1:
+                print(line)
+
+if __name__ == "__main__":
+    main(sys.argv[1:])
diff --git a/scripts/modinfo-generate.py b/scripts/modinfo-generate.py
new file mode 100755
index 000000000000..b37d2e8edab9
--- /dev/null
+++ b/scripts/modinfo-generate.py
@@ -0,0 +1,61 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
+
+import os
+import sys
+
+def print_array(name, values):
+    if len(values) == 0:
+        return
+    print("    .%s = ((const char*[]){ %s, NULL })," % (name, ", ".join(values)))
+
+def generate(name, lines):
+    arch = ""
+    objs = []
+    deps = []
+    opts = []
+    for line in lines:
+        if line.startswith("MODINFO_START"):
+            items = line.split()
+            if items[1] == 'obj':
+                objs.append(items[2])
+            elif items[1] == 'dep':
+                deps.append(items[2])
+            elif items[1] == 'opts':
+                opts.append(items[2])
+            elif items[1] == 'arch':
+                arch = items[2];
+            else:
+                print("unknown:", items[1])
+                exit(1)
+
+    print("    .name = \"%s\"," % name)
+    if arch != "":
+        print("    .arch = %s," % arch)
+    print_array("objs", objs)
+    print_array("deps", deps)
+    print_array("opts", opts)
+    print("},{");
+
+def print_pre():
+    print("/* generated by scripts/modinfo.py */")
+    print("#include \"qemu/osdep.h\"")
+    print("#include \"qemu/module.h\"")
+    print("const QemuModinfo qemu_modinfo[] = {{")
+
+def print_post():
+    print("    /* end of list */")
+    print("}};")
+
+def main(args):
+    print_pre()
+    for modinfo in args:
+        with open(modinfo) as f:
+            lines = f.readlines()
+        print("    /* %s */" % modinfo)
+        (basename, ext) = os.path.splitext(modinfo)
+        generate(basename, lines)
+    print_post()
+
+if __name__ == "__main__":
+    main(sys.argv[1:])
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 7825f6d8c847..23e92fff8484 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -74,26 +74,27 @@ void module_load_qom_one(const char *type);
 void module_load_qom_all(void);
 void module_allow_arch(const char *arch);
 
-/*
- * macros to store module metadata in a .modinfo section.
- * qemu-modinfo utility will collect the metadata.
- *
- * Use "objdump -t -s -j .modinfo ${module}.so" to inspect.
- */
-
-#define ___PASTE(a, b) a##b
-#define __PASTE(a, b) ___PASTE(a, b)
-
-#define modinfo(kind, value)                             \
-    static const char __PASTE(kind, __LINE__)[]          \
-        __attribute__((__used__))                        \
-        __attribute__((section(".modinfo")))             \
-        __attribute__((aligned(1)))                      \
-        = stringify(kind) "=" value
+/* scripts/modinfo.sh collects module info (using -DQEMU_MODINFO) */
+#ifdef QEMU_MODINFO
+# define modinfo(kind, value) \
+    MODINFO_START kind value MODINFO_END
+#else
+# define modinfo(kind, value)
+#endif
 
 #define module_obj(name) modinfo(obj, name)
 #define module_dep(name) modinfo(dep, name)
 #define module_arch(name) modinfo(arch, name)
 #define module_opts(name) modinfo(opts, name)
 
+typedef struct QemuModinfo QemuModinfo;
+struct QemuModinfo {
+    const char *name;
+    const char *arch;
+    const char **objs;
+    const char **deps;
+    const char **opts;
+};
+extern const QemuModinfo qemu_modinfo[];
+
 #endif
diff --git a/meson.build b/meson.build
index 46ebc07dbb67..f5c7ba979981 100644
--- a/meson.build
+++ b/meson.build
@@ -2048,6 +2048,10 @@ target_modules += { 'accel' : { 'qtest': qtest_module_ss,
 # Library dependencies #
 ########################
 
+modinfo_collect = find_program('scripts/modinfo-collect.py')
+modinfo_generate = find_program('scripts/modinfo-generate.py')
+modinfo_files = []
+
 block_mods = []
 softmmu_mods = []
 foreach d, list : modules
@@ -2056,6 +2060,11 @@ foreach d, list : modules
       module_ss = module_ss.apply(config_all, strict: false)
       sl = static_library(d + '-' + m, [genh, module_ss.sources()],
                           dependencies: [modulecommon, module_ss.dependencies()], pic: true)
+      modinfo_files += custom_target(d + '-' + m + '.modinfo',
+                                     output: d + '-' + m + '.modinfo',
+                                     input: module_ss.sources(),
+                                     capture: true,
+                                     command: [modinfo_collect, '@INPUT@'])
       if d == 'block'
         block_mods += sl
       else
@@ -2084,12 +2093,18 @@ foreach d, list : target_modules
                     '-DCONFIG_DEVICES="@0@-config-devices.h"'.format(target)]
           target_module_ss = module_ss.apply(config_target, strict: false)
           if target_module_ss.sources() != []
-            sl = static_library(d + '-' + m + '-' + config_target['TARGET_NAME'],
+            module_name = d + '-' + m + '-' + config_target['TARGET_NAME']
+            sl = static_library(module_name,
                                 [genh, target_module_ss.sources()],
                                 dependencies: [modulecommon, target_module_ss.dependencies()],
                                 include_directories: target_inc,
                                 c_args: c_args,
                                 pic: true)
+            modinfo_files += custom_target(module_name + '.modinfo',
+                                           output: module_name + '.modinfo',
+                                           input: target_module_ss.sources(),
+                                           capture: true,
+                                           command: [modinfo_collect, '--target', target, '@INPUT@'])
             softmmu_mods += sl
           endif
         endif
@@ -2100,6 +2115,17 @@ foreach d, list : target_modules
   endforeach
 endforeach
 
+if enable_modules
+  modinfo_src = custom_target('modinfo.c',
+                              output: 'modinfo.c',
+                              input: modinfo_files,
+                              command: [modinfo_generate, '@INPUT@'],
+                              capture: true)
+  modinfo_lib = static_library('modinfo', modinfo_src)
+  modinfo_dep = declare_dependency(link_whole: modinfo_lib)
+  softmmu_ss.add(modinfo_dep)
+endif
+
 nm = find_program('nm')
 undefsym = find_program('scripts/undefsym.py')
 block_syms = custom_target('block.syms', output: 'block.syms',
-- 
2.31.1

Re: [PATCH v2 03/18] modules: add qemu-modinfo utility

[Paolo Bonzini]

On 16/06/21 11:16, Gerd Hoffmann wrote:
>> I was almost giving up... but it looks like the result of
>> extract_all_objects(recursive: true) can be passed to custom_target(). Then
>> you can match it after compile_commands.json's "output" key.
>
> Seems the custom_target commands do not land in compile_commands.json.

No, they don't.

The idea was expressed a bit too concisely. :)  I was thinking of using 
extract_all_objects on the module static library, passing the result to 
modinfo-collect, and looking up the names in compile_commands.json.

Paolo

> But I have figured meanwhile that looking for the target name in the
> command line works reliable.  That will will match
> -DCONFIG_TARGET="${target}-config-target.h".
> 
> Current WIP patch below, seems to work nicely.  Whole patch series needs
> an overhaul now ...