[PATCH 0/2] modules: Improve modinfo.c architecture support

[PATCH 0/2] modules: Improve modinfo.c architecture support

[Jose R. Ziviani]

When building a single target, the build system detects the architecture
and generates a modinfo.c with modules related to that arch only.
However, when more than one target is built, modinfo.c is generated with
modules available for each architecture - without any way to know what
arch supports what.

The problem is when executing the target, it will try to load modules
that is not supported by it and will throw errors to users, for
instance:

$ ./configure --enable-modules # all targets
$ ./qemu-system-avr -nodefaults -display none -accel tcg -M none -device help | head
Failed to open module: /.../hw-display-qxl.so: undefined symbol: vga_ioport_read
Failed to open module: /.../hw-display-virtio-gpu.so: undefined symbol: virtio_vmstate_info
Failed to open module: /.../hw-display-virtio-gpu.so: undefined symbol: virtio_vmstate_info
Failed to open module: /.../hw-display-virtio-gpu-gl.so: undefined symbol: virtio_gpu_ctrl_response
Failed to open module: /.../hw-display-virtio-gpu-pci.so: undefined symbol: virtio_instance_init_common
Failed to open module: /.../hw-display-virtio-gpu-pci.so: undefined symbol: virtio_instance_init_common
Failed to open module: /.../hw-display-virtio-gpu-pci-gl.so: undefined symbol: virtio_instance_init_common
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga-gl.so: undefined symbol: have_vga
Failed to open module: /.../hw-usb-smartcard.so: undefined symbol: ccid_card_ccid_attach
Failed to open module: /.../hw-usb-redirect.so: undefined symbol: vmstate_usb_device
Failed to open module: /.../hw-usb-host.so: undefined symbol: vmstate_usb_device
...

$ ./qemu-system-s390x -nodefaults -display none -accel tcg -M none -device help | head 
Failed to open module: /.../hw-display-qxl.so: undefined symbol: vga_ioport_read
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga-gl.so: undefined symbol: have_vga
Failed to open module: /.../hw-usb-smartcard.so: undefined symbol: ccid_card_ccid_attach
Failed to open module: /.../hw-usb-redirect.so: undefined symbol: vmstate_usb_device
Failed to open module: /.../hw-usb-host.so: undefined symbol: vmstate_usb_device

This patchset tries to improve by collecting the modules are currently
enabled for each target, obtaining information in meson from kconfig
and passing that to modinfo.c, which now uses a list to store supported
architectures for each module.

$ ./qemu-system-s390x -nodefaults -display none -accel tcg -M none -device help | head
Controller/Bridge/Hub devices:
name "pci-bridge", bus PCI, desc "Standard PCI Bridge"
name "pci-bridge-seat", bus PCI, desc "Standard PCI Bridge (multiseat)"
...

$ ./qemu-system-avr -nodefaults -display none -accel tcg -M none -device help | head
...
Misc devices:
name "guest-loader", desc "Guest Loader"
name "loader", desc "Generic Loader"

Jose R. Ziviani (2):
  meson: introduce modules_arch
  modules: use a list of supported arch for each module

 hw/display/meson.build      | 48 +++++++++++++++++++++++++++++++++++++
 hw/usb/meson.build          | 36 ++++++++++++++++++++++++++++
 include/qemu/module.h       |  2 +-
 meson.build                 | 19 +++++++++++----
 scripts/modinfo-collect.py  | 10 ++++++++
 scripts/modinfo-generate.py |  7 +++---
 util/module.c               | 18 ++++++++++----
 7 files changed, 125 insertions(+), 15 deletions(-)

-- 
2.33.0

[PATCH 1/2] meson: introduce modules_arch

[Jose R. Ziviani]

This variable keeps track of all modules enabled for a target
architecture. This will be used in modinfo to refine the
architectures that can really load the .so to avoid errors.

Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
---
 hw/display/meson.build | 48 ++++++++++++++++++++++++++++++++++++++++++
 hw/usb/meson.build     | 36 +++++++++++++++++++++++++++++++
 meson.build            |  1 +
 3 files changed, 85 insertions(+)

diff --git a/hw/display/meson.build b/hw/display/meson.build
index 861c43ff98..ba06f58ff1 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -43,6 +43,18 @@ if config_all_devices.has_key('CONFIG_QXL')
   qxl_ss.add(when: 'CONFIG_QXL', if_true: [files('qxl.c', 'qxl-logger.c', 'qxl-render.c'),
                                            pixman, spice])
   hw_display_modules += {'qxl': qxl_ss}
+
+  archs = []
+  foreach target: target_dirs
+    if target.endswith('-softmmu')
+      cfg_target = config_target_mak[target]
+      if cfg_target.has_key('CONFIG_QXL') and cfg_target['CONFIG_QXL'] == 'y'
+        archs += [cfg_target['TARGET_NAME']]
+      endif
+    endif
+  endforeach
+
+  modules_arch += {'qxl': archs}
 endif
 
 softmmu_ss.add(when: 'CONFIG_DPCD', if_true: files('dpcd.c'))
@@ -65,6 +77,18 @@ if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
   virtio_gpu_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', virgl, opengl],
                        if_true: [files('virtio-gpu-gl.c', 'virtio-gpu-virgl.c'), pixman, virgl])
   hw_display_modules += {'virtio-gpu-gl': virtio_gpu_gl_ss}
+
+  archs = []
+  foreach target: target_dirs
+    if target.endswith('-softmmu')
+      cfg_target = config_target_mak[target]
+      if cfg_target.has_key('CONFIG_VIRTIO_GPU') and cfg_target['CONFIG_VIRTIO_GPU'] == 'y'
+        archs += [cfg_target['TARGET_NAME']]
+      endif
+    endif
+  endforeach
+
+  modules_arch += {'virtio-gpu': archs, 'virtio-gpu-gl': archs}
 endif
 
 if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
@@ -79,6 +103,18 @@ if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
   virtio_gpu_pci_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI', virgl, opengl],
                            if_true: [files('virtio-gpu-pci-gl.c'), pixman])
   hw_display_modules += {'virtio-gpu-pci-gl': virtio_gpu_pci_gl_ss}
+
+  archs = []
+  foreach target: target_dirs
+    if target.endswith('-softmmu')
+      cfg_target = config_target_mak[target]
+      if cfg_target.has_key('CONFIG_VIRTIO_PCI') and cfg_target['CONFIG_VIRTIO_PCI'] == 'y'
+        archs += [cfg_target['TARGET_NAME']]
+      endif
+    endif
+  endforeach
+
+  modules_arch += {'virtio-gpu-pci': archs, 'virtio-gpu-pci-gl': archs}
 endif
 
 if config_all_devices.has_key('CONFIG_VIRTIO_VGA')
@@ -93,6 +129,18 @@ if config_all_devices.has_key('CONFIG_VIRTIO_VGA')
   virtio_vga_gl_ss.add(when: ['CONFIG_VIRTIO_VGA', virgl, opengl],
                        if_true: [files('virtio-vga-gl.c'), pixman])
   hw_display_modules += {'virtio-vga-gl': virtio_vga_gl_ss}
+
+  archs = []
+  foreach target: target_dirs
+    if target.endswith('-softmmu')
+      cfg_target = config_target_mak[target]
+      if cfg_target.has_key('CONFIG_VIRTIO_VGA') and cfg_target['CONFIG_VIRTIO_VGA'] == 'y'
+        archs += [cfg_target['TARGET_NAME']]
+      endif
+    endif
+  endforeach
+
+  modules_arch += {'virtio-vga': archs, 'virtio-vga-gl': archs}
 endif
 
 specific_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_lcdc.c'))
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index de853d780d..6b889d2ee2 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -54,6 +54,18 @@ if cacard.found()
   usbsmartcard_ss.add(when: 'CONFIG_USB_SMARTCARD',
                       if_true: [cacard, files('ccid-card-emulated.c', 'ccid-card-passthru.c')])
   hw_usb_modules += {'smartcard': usbsmartcard_ss}
+
+  archs = []
+  foreach target: target_dirs
+    if target.endswith('-softmmu')
+      cfg_target = config_target_mak[target]
+      if cfg_target.has_key('CONFIG_USB_SMARTCARD') and cfg_target['CONFIG_USB_SMARTCARD'] == 'y'
+        archs += [cfg_target['TARGET_NAME']]
+      endif
+    endif
+  endforeach
+
+  modules_arch += {'smartcard': archs}
 endif
 
 # U2F
@@ -69,6 +81,18 @@ if usbredir.found()
   usbredir_ss.add(when: 'CONFIG_USB',
                   if_true: [usbredir, files('redirect.c', 'quirks.c')])
   hw_usb_modules += {'redirect': usbredir_ss}
+
+  archs = []
+  foreach target: target_dirs
+    if target.endswith('-softmmu')
+      cfg_target = config_target_mak[target]
+      if cfg_target.has_key('CONFIG_USB') and cfg_target['CONFIG_USB'] == 'y'
+        archs += [cfg_target['TARGET_NAME']]
+      endif
+    endif
+  endforeach
+
+  modules_arch += {'redirect': archs}
 endif
 
 # usb pass-through
@@ -77,6 +101,18 @@ if libusb.found()
   usbhost_ss.add(when: ['CONFIG_USB', libusb],
                  if_true: files('host-libusb.c'))
   hw_usb_modules += {'host': usbhost_ss}
+
+  archs = []
+  foreach target: target_dirs
+    if target.endswith('-softmmu')
+      cfg_target = config_target_mak[target]
+      if cfg_target.has_key('CONFIG_USB') and cfg_target['CONFIG_USB'] == 'y'
+        archs += [cfg_target['TARGET_NAME']]
+      endif
+    endif
+  endforeach
+
+  modules_arch += {'host': archs}
 endif
 
 softmmu_ss.add(when: ['CONFIG_USB', 'CONFIG_XEN', libusb], if_true: files('xen-usb.c'))
diff --git a/meson.build b/meson.build
index 2711cbb789..d1d3fd84ec 100644
--- a/meson.build
+++ b/meson.build
@@ -2071,6 +2071,7 @@ tcg_module_ss = ss.source_set()
 
 modules = {}
 target_modules = {}
+modules_arch = {}
 hw_arch = {}
 target_arch = {}
 target_softmmu_arch = {}
-- 
2.33.0

[PATCH 2/2] modules: use a list of supported arch for each module

[Jose R. Ziviani]

When compiling QEMU with more than one target, for instance,
--target-list=s390x-softmmu,x86_64-softmmu, modinfo.c will be
filled with modules available for both, with no specification
of what modules can/cannot be loaded for a particular target.

This will cause message errors when executing the target that
shouldn't be loading that module, such as:

$ qemu-system-s390x -nodefaults -display none -accel qtest -M none -device help
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: vmstate_vga_common

This patch changes the module infrastructure to use a list of
architectures, obtained during the build time, to specify what
targets can load each module.

Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
---
 include/qemu/module.h       |  2 +-
 meson.build                 | 18 +++++++++++++-----
 scripts/modinfo-collect.py  | 10 ++++++++++
 scripts/modinfo-generate.py |  7 +++----
 util/module.c               | 18 +++++++++++++-----
 5 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index 3deac0078b..3b487c646c 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -144,7 +144,7 @@ void module_allow_arch(const char *arch);
 typedef struct QemuModinfo QemuModinfo;
 struct QemuModinfo {
     const char *name;
-    const char *arch;
+    const char **archs;
     const char **objs;
     const char **deps;
     const char **opts;
diff --git a/meson.build b/meson.build
index d1d3fd84ec..efba275092 100644
--- a/meson.build
+++ b/meson.build
@@ -2343,11 +2343,19 @@ foreach d, list : modules
         # unique when it comes to lookup in compile_commands.json.
         # Depnds on a mesion version with
         # https://github.com/mesonbuild/meson/pull/8900
-        modinfo_files += custom_target(d + '-' + m + '.modinfo',
-                                       output: d + '-' + m + '.modinfo',
-                                       input: module_ss.sources() + genh,
-                                       capture: true,
-                                       command: [modinfo_collect, module_ss.sources()])
+        if modules_arch.has_key(m)
+          modinfo_files += custom_target(d + '-' + m + '.modinfo',
+                                        output: d + '-' + m + '.modinfo',
+                                        input: module_ss.sources() + genh,
+                                        capture: true,
+                                        command: [modinfo_collect, module_ss.sources(), '--archs', modules_arch[m]])
+        else
+          modinfo_files += custom_target(d + '-' + m + '.modinfo',
+                                        output: d + '-' + m + '.modinfo',
+                                        input: module_ss.sources() + genh,
+                                        capture: true,
+                                        command: [modinfo_collect, module_ss.sources()])
+        endif
       endif
     else
       if d == 'block'
diff --git a/scripts/modinfo-collect.py b/scripts/modinfo-collect.py
index 4acb188c3e..739cd23e2f 100755
--- a/scripts/modinfo-collect.py
+++ b/scripts/modinfo-collect.py
@@ -50,6 +50,16 @@ def main(args):
         print("MODINFO_START arch \"%s\" MODINFO_END" % arch)
     with open('compile_commands.json') as f:
         compile_commands = json.load(f)
+
+    try:
+        arch_idx = args.index('--archs')
+        archs = args[arch_idx + 1:]
+        args = args[:arch_idx]
+        for arch in archs:
+            print("MODINFO_START arch \"%s\" MODINFO_END" % arch)
+    except ValueError:
+        pass
+
     for src in args:
         print("MODINFO_DEBUG src %s" % src)
         command = find_command(src, target, compile_commands)
diff --git a/scripts/modinfo-generate.py b/scripts/modinfo-generate.py
index f559eed007..e1d13acd92 100755
--- a/scripts/modinfo-generate.py
+++ b/scripts/modinfo-generate.py
@@ -33,7 +33,7 @@ def parse_line(line):
     return (kind, data)
 
 def generate(name, lines):
-    arch = ""
+    archs = []
     objs = []
     deps = []
     opts = []
@@ -47,14 +47,13 @@ def generate(name, lines):
             elif kind == 'opts':
                 opts.append(data)
             elif kind == 'arch':
-                arch = data;
+                archs.append(data);
             else:
                 print("unknown:", kind)
                 exit(1)
 
     print("    .name = \"%s\"," % name)
-    if arch != "":
-        print("    .arch = %s," % arch)
+    print_array("archs", archs)
     print_array("objs", objs)
     print_array("deps", deps)
     print_array("opts", opts)
diff --git a/util/module.c b/util/module.c
index 6bb4ad915a..7009143bfc 100644
--- a/util/module.c
+++ b/util/module.c
@@ -131,16 +131,24 @@ void module_allow_arch(const char *arch)
 
 static bool module_check_arch(const QemuModinfo *modinfo)
 {
-    if (modinfo->arch) {
+    const char **arch;
+
+    if (modinfo->archs) {
         if (!module_arch) {
             /* no arch set -> ignore all */
             return false;
         }
-        if (strcmp(module_arch, modinfo->arch) != 0) {
-            /* mismatch */
-            return false;
+
+        for (arch = modinfo->archs; *arch != NULL; arch++) {
+            if (strcmp(module_arch, *arch) == 0) {
+                return true;
+            }
         }
+
+        /* mismatch */
+        return false;
     }
+
     return true;
 }
 
@@ -245,7 +253,7 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
     g_hash_table_add(loaded_modules, module_name);
 
     for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
-        if (modinfo->arch) {
+        if (modinfo->archs) {
             if (strcmp(modinfo->name, module_name) == 0) {
                 if (!module_check_arch(modinfo)) {
                     return false;
-- 
2.33.0

Re: [PATCH 1/2] meson: introduce modules_arch

[Gerd Hoffmann]

  Hi,

> This variable keeps track of all modules enabled for a target
> architecture. This will be used in modinfo to refine the
> architectures that can really load the .so to avoid errors.

I think this is the wrong approach.  The reason why modules are
not loading is typically *not* the architecture, but a feature
or subsystem the device needs not being compiled in.  Often the
subsystem is a bus (pci, usb, ccw), but there are also other
cases (virtio, vga).

We can stick that into modinfo, simliar to module_dep() but for bits
provided by core qemu instead of other modules.  i.e. add something
along the lines of ...

	module_need(BUS_PCI);

... to the modules, store that in modinfo and check it before trying
to load.

That would also allow to remove hacks like commit 2dd9d8cfb4f3 ("s390x:
add have_virtio_ccw")

take care,
  Gerd

Re: [PATCH 1/2] meson: introduce modules_arch

[Jose R. Ziviani]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

Hello!

On Fri, Sep 17, 2021 at 09:14:04AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > This variable keeps track of all modules enabled for a target
> > architecture. This will be used in modinfo to refine the
> > architectures that can really load the .so to avoid errors.
> 
> I think this is the wrong approach.  The reason why modules are
> not loading is typically *not* the architecture, but a feature
> or subsystem the device needs not being compiled in.  Often the
> subsystem is a bus (pci, usb, ccw), but there are also other
> cases (virtio, vga).
> 
> We can stick that into modinfo, simliar to module_dep() but for bits
> provided by core qemu instead of other modules.  i.e. add something
> along the lines of ...

Yes, I really like your approach, makes more sense indeed. But, how do I
get the core modules that other modules depend on?

I see that Kconfig already has something in this line:

config VGA  (from hw/display)
    bool

config PCI  (from hw/pci)
    bool

config QXL  (from hw/display)
    bool
    depends on SPICE && PCI
    select VGA

I assume that independent entries (like VGA and PCI) are core and that I
can rely on it to add
  module_need(PCI)
  module_need(VGA)
for hw-display-qxl. Am I right?

Thanks for reviewing it!!

> 
> 	module_need(BUS_PCI);
> 
> ... to the modules, store that in modinfo and check it before trying
> to load.
> 
> That would also allow to remove hacks like commit 2dd9d8cfb4f3 ("s390x:
> add have_virtio_ccw")
> 
> take care,
>   Gerd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] meson: introduce modules_arch

[Gerd Hoffmann]

  Hi,

> Yes, I really like your approach, makes more sense indeed. But, how do I
> get the core modules that other modules depend on?
> 
> I see that Kconfig already has something in this line:
> 
> config VGA  (from hw/display)
>     bool
> 
> config PCI  (from hw/pci)
>     bool
> 
> config QXL  (from hw/display)
>     bool
>     depends on SPICE && PCI
>     select VGA
> 
> I assume that independent entries (like VGA and PCI) are core and that I
> can rely on it to add
>   module_need(PCI)
>   module_need(VGA)
> for hw-display-qxl. Am I right?

Yes, looking at kconfig for core dependencies makes sense.

take care,
  Gerd

Re: [PATCH 1/2] meson: introduce modules_arch

[Jose R. Ziviani]

[-- Attachment #1: Type: text/plain, Size: 1380 bytes --]

On Mon, Sep 20, 2021 at 07:15:32AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > Yes, I really like your approach, makes more sense indeed. But, how do I
> > get the core modules that other modules depend on?
> > 
> > I see that Kconfig already has something in this line:
> > 
> > config VGA  (from hw/display)
> >     bool
> > 
> > config PCI  (from hw/pci)
> >     bool
> > 
> > config QXL  (from hw/display)
> >     bool
> >     depends on SPICE && PCI
> >     select VGA
> > 
> > I assume that independent entries (like VGA and PCI) are core and that I
> > can rely on it to add
> >   module_need(PCI)
> >   module_need(VGA)
> > for hw-display-qxl. Am I right?
> 
> Yes, looking at kconfig for core dependencies makes sense.

But, in anyway, I'll still need to store the target architecture that
can use such core module, like I did here in this patch. Otherwise,
if I compile different targets at the same time, I'll end up with the
same problem of targets trying to load wrong modules.

I thought of using qom, but I think it will pollute module.c.

What do you think if I simply create one modinfo.c per target, like
modinfo-s390x.c, modinfo-avr.c, etc? Each will only have the data
structure filled with the right modules and linked only to its own
qemu-system-arch.

Best regards,

Jose R Ziviani

> 
> take care,
>   Gerd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] meson: introduce modules_arch

[Paolo Bonzini]

On 20/09/21 15:02, Jose R. Ziviani wrote:
> But, in anyway, I'll still need to store the target architecture that
> can use such core module, like I did here in this patch. Otherwise,
> if I compile different targets at the same time, I'll end up with the
> same problem of targets trying to load wrong modules.
> 
> I thought of using qom, but I think it will pollute module.c.

Alternatively, you could C-ify the contents of config-devices.mak, and 
embed them in the per-arch modinfo-*.c; and record CONFIG_* symbols for 
each module (e.g. '{ "CONFIG_QXL", "hw-display-qxl" }' from a 
'module_config("CONFIG_QXL")' line in the global modinfo.c file.  Then 
before loading a module you do a binary search on the per-arch 
config-devices array.

I hope the above is readable. :)

Paolo

Re: [PATCH 1/2] meson: introduce modules_arch

[Gerd Hoffmann]

  Hi,

> But, in anyway, I'll still need to store the target architecture that
> can use such core module, like I did here in this patch. Otherwise,
> if I compile different targets at the same time, I'll end up with the
> same problem of targets trying to load wrong modules.

That all works just fine today.  If you have target-specific modules
(i.e. source files added to specific_ss instead of softmmu_ss when
compiling into core qemu) you only need to add those to the
target_modules[] (instead of modules[]) and you are set.

In-tree example: qtest accelerator.

take care,
  Gerd

Re: [PATCH 1/2] meson: introduce modules_arch

[Jose R. Ziviani]

[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]

Hello!!

On Tue, Sep 21, 2021 at 07:25:42AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > But, in anyway, I'll still need to store the target architecture that
> > can use such core module, like I did here in this patch. Otherwise,
> > if I compile different targets at the same time, I'll end up with the
> > same problem of targets trying to load wrong modules.
> 
> That all works just fine today.  If you have target-specific modules
> (i.e. source files added to specific_ss instead of softmmu_ss when
> compiling into core qemu) you only need to add those to the
> target_modules[] (instead of modules[]) and you are set.
> 
> In-tree example: qtest accelerator.

Exactly, but what it doesn't seem to work (or I'm not understanding it
well) is: how the target will know whether it can or cannot load a
module.

For example, suppose I build target-list=s390x-softmmu,x86_64-softmmu.
Both targets will be linked to the same modinfo.c object, which will
have a 'hw-display-virtio-gpu-pci' entry (it wouldn't if I build only
s390x-softmmu). When I execute ./qemu-system-s390x, it will try to
load hw-display-virtio-gpu-pci and will throw the errors I mentioned
earlier.

If, for example, I add that module_need(PCI_BUS), we will continue
not knowing whether the target in execution has the required bus
(without injecting dependencies in module.c). But it would be easier if
we have such information in modinfo.c directly (of an modinfo-<arch>.c).
I understood that it's not an arch issue, it's the target that doesn't
current implement such core module. But, in practice, we will end up
need to query that information.

Please, correct me if I'm not getting the point correctly.

Thank you!!


> 
> take care,
>   Gerd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] meson: introduce modules_arch

[Jose R. Ziviani]

[-- Attachment #1: Type: text/plain, Size: 1105 bytes --]

Hello!!

On Mon, Sep 20, 2021 at 09:03:28PM +0200, Paolo Bonzini wrote:
> On 20/09/21 15:02, Jose R. Ziviani wrote:
> > But, in anyway, I'll still need to store the target architecture that
> > can use such core module, like I did here in this patch. Otherwise,
> > if I compile different targets at the same time, I'll end up with the
> > same problem of targets trying to load wrong modules.
> > 
> > I thought of using qom, but I think it will pollute module.c.
> 
> Alternatively, you could C-ify the contents of config-devices.mak, and embed
> them in the per-arch modinfo-*.c; and record CONFIG_* symbols for each
> module (e.g. '{ "CONFIG_QXL", "hw-display-qxl" }' from a
> 'module_config("CONFIG_QXL")' line in the global modinfo.c file.  Then
> before loading a module you do a binary search on the per-arch
> config-devices array.

With a per-arch modinfo-*.c we don't even need a modinfo.c global, do
we?

Each target could be linked to its own modinfo-target.c only.

> 
> I hope the above is readable. :)

Absolutely, thank you for your suggestion!!

> 
> Paolo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] meson: introduce modules_arch

[Gerd Hoffmann]

On Tue, Sep 21, 2021 at 10:35:04AM -0300, Jose R. Ziviani wrote:
> Hello!!
> 
> On Tue, Sep 21, 2021 at 07:25:42AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > But, in anyway, I'll still need to store the target architecture that
> > > can use such core module, like I did here in this patch. Otherwise,
> > > if I compile different targets at the same time, I'll end up with the
> > > same problem of targets trying to load wrong modules.
> > 
> > That all works just fine today.  If you have target-specific modules
> > (i.e. source files added to specific_ss instead of softmmu_ss when
> > compiling into core qemu) you only need to add those to the
> > target_modules[] (instead of modules[]) and you are set.
> > 
> > In-tree example: qtest accelerator.
> 
> Exactly, but what it doesn't seem to work (or I'm not understanding it
> well) is: how the target will know whether it can or cannot load a
> module.

Well, for target-specific modules that is easy:  You get one module per
target, and each target loads the matching module only.

> For example, suppose I build target-list=s390x-softmmu,x86_64-softmmu.
> Both targets will be linked to the same modinfo.c object, which will
> have a 'hw-display-virtio-gpu-pci' entry (it wouldn't if I build only
> s390x-softmmu). When I execute ./qemu-system-s390x, it will try to
> load hw-display-virtio-gpu-pci and will throw the errors I mentioned
> earlier.

Yes.  That isn't a target-specific module.  It will load into any target
which has pci support.  You can add aarch64-softmmu to the list above,
and then notice that both aarch64-softmmu and x86_64-softmmu can load
the very same hw-display-virtio-gpu-pci module.

> If, for example, I add that module_need(PCI_BUS), we will continue
> not knowing whether the target in execution has the required bus
> (without injecting dependencies in module.c).

Yes, you'll need a 'module_provides(PCI_BUS)' somewhere in the pci
initialization code (likewise for the other features), so the module
code knows which features are present and can check that against the
list of 'module_needs(...)' of the module.

Trying to have kconfig export that information instead of adding
"module_needs()" + "module_provides()" annotations to the source
should work too.  Not sure which is easier.

With the kconfig approach you have all information needed at compile
time, so you can do compile-time filtering and build per-target modinfo
lists (paolo's idea) instead of using a single list with
runtime-filtering (which is what we have now).

take care,
  Gerd

Re: [PATCH 1/2] meson: introduce modules_arch

[Paolo Bonzini]

On 21/09/21 15:46, Jose R. Ziviani wrote:
>> Alternatively, you could C-ify the contents of config-devices.mak, and embed
>> them in the per-arch modinfo-*.c; and record CONFIG_* symbols for each
>> module (e.g. '{ "CONFIG_QXL", "hw-display-qxl" }' from a
>> 'module_config("CONFIG_QXL")' line in the global modinfo.c file.  Then
>> before loading a module you do a binary search on the per-arch
>> config-devices array.
> With a per-arch modinfo-*.c we don't even need a modinfo.c global, do
> we?
> 
> Each target could be linked to its own modinfo-target.c only.

Yes, I suppose you don't need it.  However, you may want to use 
different Python scripts to generate modinfo-*.c (currently from 
config-devices.mak only) and modinfo.c (from compile_commands.json and 
various sources), so it may be handy to separate them.

Paolo