diff mbox series

[1/2] meson: introduce modules_arch

Message ID 20210917012904.26544-2-jziviani@suse.de (mailing list archive)
State New, archived
Headers show
Series modules: Improve modinfo.c architecture support | expand

Commit Message

Jose R. Ziviani Sept. 17, 2021, 1:29 a.m. UTC
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(+)

Comments

Gerd Hoffmann Sept. 17, 2021, 7:14 a.m. UTC | #1
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
Jose R. Ziviani Sept. 17, 2021, 1:06 p.m. UTC | #2
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
>
Gerd Hoffmann Sept. 20, 2021, 5:15 a.m. UTC | #3
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
Jose R. Ziviani Sept. 20, 2021, 1:02 p.m. UTC | #4
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
>
Paolo Bonzini Sept. 20, 2021, 7:03 p.m. UTC | #5
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
Gerd Hoffmann Sept. 21, 2021, 5:25 a.m. UTC | #6
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
Jose R. Ziviani Sept. 21, 2021, 1:35 p.m. UTC | #7
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
>
Jose R. Ziviani Sept. 21, 2021, 1:46 p.m. UTC | #8
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
Gerd Hoffmann Sept. 21, 2021, 3:34 p.m. UTC | #9
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
Paolo Bonzini Sept. 23, 2021, 7:18 a.m. UTC | #10
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
diff mbox series

Patch

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 = {}