* [PATCH 0/2] modules: Improve modinfo.c architecture support @ 2021-09-17 1:29 Jose R. Ziviani 2021-09-17 1:29 ` [PATCH 1/2] meson: introduce modules_arch Jose R. Ziviani 2021-09-17 1:29 ` [PATCH 2/2] modules: use a list of supported arch for each module Jose R. Ziviani 0 siblings, 2 replies; 13+ messages in thread From: Jose R. Ziviani @ 2021-09-17 1:29 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, kraxel, 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] meson: introduce modules_arch 2021-09-17 1:29 [PATCH 0/2] modules: Improve modinfo.c architecture support Jose R. Ziviani @ 2021-09-17 1:29 ` Jose R. Ziviani 2021-09-17 7:14 ` Gerd Hoffmann 2021-09-17 1:29 ` [PATCH 2/2] modules: use a list of supported arch for each module Jose R. Ziviani 1 sibling, 1 reply; 13+ messages in thread From: Jose R. Ziviani @ 2021-09-17 1:29 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, kraxel, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] meson: introduce modules_arch 2021-09-17 1:29 ` [PATCH 1/2] meson: introduce modules_arch Jose R. Ziviani @ 2021-09-17 7:14 ` Gerd Hoffmann 2021-09-17 13:06 ` Jose R. Ziviani 0 siblings, 1 reply; 13+ messages in thread From: Gerd Hoffmann @ 2021-09-17 7:14 UTC (permalink / raw) To: Jose R. Ziviani; +Cc: pbonzini, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] meson: introduce modules_arch 2021-09-17 7:14 ` Gerd Hoffmann @ 2021-09-17 13:06 ` Jose R. Ziviani 2021-09-20 5:15 ` Gerd Hoffmann 0 siblings, 1 reply; 13+ messages in thread From: Jose R. Ziviani @ 2021-09-17 13:06 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: pbonzini, qemu-devel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] meson: introduce modules_arch 2021-09-17 13:06 ` Jose R. Ziviani @ 2021-09-20 5:15 ` Gerd Hoffmann 2021-09-20 13:02 ` Jose R. Ziviani 0 siblings, 1 reply; 13+ messages in thread From: Gerd Hoffmann @ 2021-09-20 5:15 UTC (permalink / raw) To: Jose R. Ziviani; +Cc: pbonzini, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] meson: introduce modules_arch 2021-09-20 5:15 ` Gerd Hoffmann @ 2021-09-20 13:02 ` Jose R. Ziviani 2021-09-20 19:03 ` Paolo Bonzini 2021-09-21 5:25 ` Gerd Hoffmann 0 siblings, 2 replies; 13+ messages in thread From: Jose R. Ziviani @ 2021-09-20 13:02 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: pbonzini, qemu-devel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] meson: introduce modules_arch 2021-09-20 13:02 ` Jose R. Ziviani @ 2021-09-20 19:03 ` Paolo Bonzini 2021-09-21 13:46 ` Jose R. Ziviani 2021-09-21 5:25 ` Gerd Hoffmann 1 sibling, 1 reply; 13+ messages in thread From: Paolo Bonzini @ 2021-09-20 19:03 UTC (permalink / raw) To: Jose R. Ziviani, Gerd Hoffmann; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] meson: introduce modules_arch 2021-09-20 19:03 ` Paolo Bonzini @ 2021-09-21 13:46 ` Jose R. Ziviani 2021-09-23 7:18 ` Paolo Bonzini 0 siblings, 1 reply; 13+ messages in thread From: Jose R. Ziviani @ 2021-09-21 13:46 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Gerd Hoffmann, qemu-devel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] meson: introduce modules_arch 2021-09-21 13:46 ` Jose R. Ziviani @ 2021-09-23 7:18 ` Paolo Bonzini 0 siblings, 0 replies; 13+ messages in thread From: Paolo Bonzini @ 2021-09-23 7:18 UTC (permalink / raw) To: Jose R. Ziviani; +Cc: Gerd Hoffmann, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] meson: introduce modules_arch 2021-09-20 13:02 ` Jose R. Ziviani 2021-09-20 19:03 ` Paolo Bonzini @ 2021-09-21 5:25 ` Gerd Hoffmann 2021-09-21 13:35 ` Jose R. Ziviani 1 sibling, 1 reply; 13+ messages in thread From: Gerd Hoffmann @ 2021-09-21 5:25 UTC (permalink / raw) To: Jose R. Ziviani; +Cc: pbonzini, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] meson: introduce modules_arch 2021-09-21 5:25 ` Gerd Hoffmann @ 2021-09-21 13:35 ` Jose R. Ziviani 2021-09-21 15:34 ` Gerd Hoffmann 0 siblings, 1 reply; 13+ messages in thread From: Jose R. Ziviani @ 2021-09-21 13:35 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: pbonzini, qemu-devel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] meson: introduce modules_arch 2021-09-21 13:35 ` Jose R. Ziviani @ 2021-09-21 15:34 ` Gerd Hoffmann 0 siblings, 0 replies; 13+ messages in thread From: Gerd Hoffmann @ 2021-09-21 15:34 UTC (permalink / raw) To: Jose R. Ziviani; +Cc: pbonzini, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] modules: use a list of supported arch for each module 2021-09-17 1:29 [PATCH 0/2] modules: Improve modinfo.c architecture support Jose R. Ziviani 2021-09-17 1:29 ` [PATCH 1/2] meson: introduce modules_arch Jose R. Ziviani @ 2021-09-17 1:29 ` Jose R. Ziviani 1 sibling, 0 replies; 13+ messages in thread From: Jose R. Ziviani @ 2021-09-17 1:29 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, kraxel, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-09-23 7:18 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-17 1:29 [PATCH 0/2] modules: Improve modinfo.c architecture support Jose R. Ziviani 2021-09-17 1:29 ` [PATCH 1/2] meson: introduce modules_arch Jose R. Ziviani 2021-09-17 7:14 ` Gerd Hoffmann 2021-09-17 13:06 ` Jose R. Ziviani 2021-09-20 5:15 ` Gerd Hoffmann 2021-09-20 13:02 ` Jose R. Ziviani 2021-09-20 19:03 ` Paolo Bonzini 2021-09-21 13:46 ` Jose R. Ziviani 2021-09-23 7:18 ` Paolo Bonzini 2021-09-21 5:25 ` Gerd Hoffmann 2021-09-21 13:35 ` Jose R. Ziviani 2021-09-21 15:34 ` Gerd Hoffmann 2021-09-17 1:29 ` [PATCH 2/2] modules: use a list of supported arch for each module Jose R. Ziviani
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.