| Message ID | 20210917012904.26544-2-jziviani@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | modules: Improve modinfo.c architecture support | expand |
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
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 >
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
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 >
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
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
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 >
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
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
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 --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 = {}
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(+)