| Message ID | 20210610055755.538119-18-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | modules: add metadata database | expand |
On Thu, Jun 10, 2021 at 07:57:54AM +0200, Gerd Hoffmann wrote: > Add module_allow_arch() to set the target architecture. > In case a module is limited to some arch verify arches > match and ignore the module if not. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > include/qemu/module.h | 1 + > softmmu/vl.c | 3 +++ > util/module.c | 15 +++++++++++++++ > 3 files changed, 19 insertions(+) > > diff --git a/include/qemu/module.h b/include/qemu/module.h > index d3cab3c25a2f..7825f6d8c847 100644 > --- a/include/qemu/module.h > +++ b/include/qemu/module.h > @@ -72,6 +72,7 @@ void module_call_init(module_init_type type); > bool module_load_one(const char *prefix, const char *lib_name, bool mayfail); > void module_load_qom_one(const char *type); > void module_load_qom_all(void); > +void module_allow_arch(const char *arch); > > /* > * macros to store module metadata in a .modinfo section. > diff --git a/softmmu/vl.c b/softmmu/vl.c > index ba26a042b284..96316774fcc9 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -126,6 +126,8 @@ > #include "sysemu/iothread.h" > #include "qemu/guest-random.h" > > +#include "config-host.h" > + > #define MAX_VIRTIO_CONSOLES 1 > > typedef struct BlockdevOptionsQueueEntry { > @@ -2723,6 +2725,7 @@ void qemu_init(int argc, char **argv, char **envp) > error_init(argv[0]); > qemu_init_exec_dir(argv[0]); > > + module_allow_arch(TARGET_NAME); > qemu_init_subsystems(); > > /* first pass of option parsing */ > diff --git a/util/module.c b/util/module.c > index 6e4199169c41..564b8e3da760 100644 > --- a/util/module.c > +++ b/util/module.c > @@ -122,6 +122,12 @@ void module_call_init(module_init_type type) > static Modules *modinfo; > static char *module_dirs[5]; > static int module_ndirs; > +static const char *module_arch; > + > +void module_allow_arch(const char *arch) > +{ > + module_arch = arch; > +} > > static void module_load_path_init(void) > { > @@ -295,6 +301,14 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail) > module_load_modinfo(); > > for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) { > + if (modlist->value->has_arch) { > + if (strcmp(modlist->value->name, module_name) == 0) { > + if (!module_arch || > + strcmp(modlist->value->arch, module_arch) != 0) { > + return false; > + } > + } > + } I have a little hard time following the code paths, but IIUC, with this change, instead of "module.so" we would have multiple copies of something like "module-$arch.so" ? Then we load them all, read their modinfo section and discard the ones with non-matching arch ? If that is a correct understanding, then I wonder about the scalability of it. We have 30 system emulator targets right now, so if we get even a few arch specific modues, that's going to be alot of modules we're loading, checking and discarding. Wouldn't it be simpler if we just made use of the directory layout /usr/lib64/qemu/$mod.so for common modules and /usr/lib64/qemu/$arch/$mod.so for arch specific modules. That would let us load only the modules we know are applicable for the system target arch and not need this post-load filtering from metadata. Regards, Daniel
Hi, > > for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) { > > + if (modlist->value->has_arch) { > > + if (strcmp(modlist->value->name, module_name) == 0) { > > + if (!module_arch || > > + strcmp(modlist->value->arch, module_arch) != 0) { > > + return false; > > + } > > + } > > + } > > I have a little hard time following the code paths, but IIUC, with this > change, instead of "module.so" we would have multiple copies of something > like "module-$arch.so" ? Not yet with this series, but easily doable on top of this (see other patch series sent today). > Then we load them all, read their modinfo section > and discard the ones with non-matching arch ? No. There is a utility reading the modinfo section (patch #2), write out the info as json (patch #2 has the schema), then qemu will read that json file (patch #13) ... > for arch specific modules. That would let us load only the modules we know > are applicable for the system target arch and not need this post-load > filtering from metadata. ... so it's pre-load filtering, not post-load. take care, Gerd
On Thu, Jun 10, 2021 at 02:57:21PM +0200, Gerd Hoffmann wrote: > Hi, > > > > for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) { > > > + if (modlist->value->has_arch) { > > > + if (strcmp(modlist->value->name, module_name) == 0) { > > > + if (!module_arch || > > > + strcmp(modlist->value->arch, module_arch) != 0) { > > > + return false; > > > + } > > > + } > > > + } > > > > I have a little hard time following the code paths, but IIUC, with this > > change, instead of "module.so" we would have multiple copies of something > > like "module-$arch.so" ? > > Not yet with this series, but easily doable on top of this (see other > patch series sent today). > > > Then we load them all, read their modinfo section > > and discard the ones with non-matching arch ? > > No. There is a utility reading the modinfo section (patch #2), write > out the info as json (patch #2 has the schema), then qemu will read that > json file (patch #13) ... Ah ok, missed that. Is the JSON file completely static, listing all modules that were built regardless of whether they are currently installed, or would it need to be refreshed when installing/uninstalling RPMs with modules ? I would think we can do the former and simply handle missing modules on disk fairly easily Regards, Daniel
Hi, > Is the JSON file completely static, listing all modules that were built > regardless of whether they are currently installed, or would it need to > be refreshed when installing/uninstalling RPMs with modules ? I would > think we can do the former and simply handle missing modules on disk > fairly easily We can do both. The file is generated and installed as part of the build/install process, and it can be simply used as-is even if some of the modules are missing. It's also possible to update the modinfo.json file in postinstall / postuninstall by simply running qemu-modinfo, so only the modules actually installed are listed there. take care, Gerd
On 6/10/21 2:35 PM, Daniel P. Berrangé wrote: > On Thu, Jun 10, 2021 at 07:57:54AM +0200, Gerd Hoffmann wrote: >> Add module_allow_arch() to set the target architecture. >> In case a module is limited to some arch verify arches >> match and ignore the module if not. >> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >> --- >> include/qemu/module.h | 1 + >> softmmu/vl.c | 3 +++ >> util/module.c | 15 +++++++++++++++ >> 3 files changed, 19 insertions(+) >> >> diff --git a/include/qemu/module.h b/include/qemu/module.h >> index d3cab3c25a2f..7825f6d8c847 100644 >> --- a/include/qemu/module.h >> +++ b/include/qemu/module.h >> @@ -72,6 +72,7 @@ void module_call_init(module_init_type type); >> bool module_load_one(const char *prefix, const char *lib_name, bool mayfail); >> void module_load_qom_one(const char *type); >> void module_load_qom_all(void); >> +void module_allow_arch(const char *arch); >> >> /* >> * macros to store module metadata in a .modinfo section. >> diff --git a/softmmu/vl.c b/softmmu/vl.c >> index ba26a042b284..96316774fcc9 100644 >> --- a/softmmu/vl.c >> +++ b/softmmu/vl.c >> @@ -126,6 +126,8 @@ >> #include "sysemu/iothread.h" >> #include "qemu/guest-random.h" >> >> +#include "config-host.h" >> + >> #define MAX_VIRTIO_CONSOLES 1 >> >> typedef struct BlockdevOptionsQueueEntry { >> @@ -2723,6 +2725,7 @@ void qemu_init(int argc, char **argv, char **envp) >> error_init(argv[0]); >> qemu_init_exec_dir(argv[0]); >> >> + module_allow_arch(TARGET_NAME); >> qemu_init_subsystems(); >> >> /* first pass of option parsing */ >> diff --git a/util/module.c b/util/module.c >> index 6e4199169c41..564b8e3da760 100644 >> --- a/util/module.c >> +++ b/util/module.c >> @@ -122,6 +122,12 @@ void module_call_init(module_init_type type) >> static Modules *modinfo; >> static char *module_dirs[5]; >> static int module_ndirs; >> +static const char *module_arch; >> + >> +void module_allow_arch(const char *arch) >> +{ >> + module_arch = arch; >> +} >> >> static void module_load_path_init(void) >> { >> @@ -295,6 +301,14 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail) >> module_load_modinfo(); >> >> for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) { >> + if (modlist->value->has_arch) { >> + if (strcmp(modlist->value->name, module_name) == 0) { >> + if (!module_arch || >> + strcmp(modlist->value->arch, module_arch) != 0) { >> + return false; >> + } >> + } >> + } > > I have a little hard time following the code paths, but IIUC, with this > change, instead of "module.so" we would have multiple copies of something > like "module-$arch.so" ? Then we load them all, read their modinfo section > and discard the ones with non-matching arch ? > > If that is a correct understanding, then I wonder about the scalability of > it. We have 30 system emulator targets right now, so if we get even a few > arch specific modues, that's going to be alot of modules we're loading, > checking and discarding. > > Wouldn't it be simpler if we just made use of the directory layout > /usr/lib64/qemu/$mod.so for common modules and /usr/lib64/qemu/$arch/$mod.so > for arch specific modules. That would let us load only the modules we know > are applicable for the system target arch and not need this post-load > filtering from metadata. > > Regards, > Daniel > agreed. Claudio
On 6/10/21 4:03 PM, Gerd Hoffmann wrote: > Hi, > >> Is the JSON file completely static, listing all modules that were built >> regardless of whether they are currently installed, or would it need to >> be refreshed when installing/uninstalling RPMs with modules ? I would >> think we can do the former and simply handle missing modules on disk >> fairly easily > > We can do both. The file is generated and installed as part of the > build/install process, and it can be simply used as-is even if some of > the modules are missing. > > It's also possible to update the modinfo.json file in postinstall / > postuninstall by simply running qemu-modinfo, so only the modules > actually installed are listed there. > > take care, > Gerd > > I fail to see why that extra complication is needed at all. Why don't we just build the modules for the targets we intend to build, and install them as .so files in a target arch directory? What problem is the json solving? Sorry if obvious. Thanks, Claudio
Hi, > > We can do both. The file is generated and installed as part of the > > build/install process, and it can be simply used as-is even if some of > > the modules are missing. > > > > It's also possible to update the modinfo.json file in postinstall / > > postuninstall by simply running qemu-modinfo, so only the modules > > actually installed are listed there. > > I fail to see why that extra complication is needed at all. > > Why don't we just build the modules for the targets we intend to build, > and install them as .so files in a target arch directory? There is more meta-data we need for modules: Which QOM types are implemented by which module (for on-demand loading), dependencies between modules and also which command line options (aka QemuOpts) are handled by which modules. Possibly more in the future, maybe we'll support modules registering monitor commands dynamically some day (like usb-host implementing "info usbhost" or tcg implementing "info jit" + "info opcount") and we'd like store that information in the module database too. If we have such a module database anyway it IMHO makes alot of sense to simply store the target arch there too instead of using something else. > What problem is the json solving? Well, the goal is to store meta-data about modules, in a way which: (a) Doesn't require manually maintained lists. This is what we have right now (arrays in utils/module.c). Works ok-ish for a small number of modules, but becomes increasingly problematic with the growing number of modules. (b) Doesn't require opening each individual module on each qemu run to get the information. I'm not particularly attached to using json for that, it is just that we already have infrastructure to parse/serialize structs from/to json because we need that for qapi anyway. take care, Gerd
On Mon, Jun 14, 2021 at 03:44:53PM +0200, Gerd Hoffmann wrote: > Hi, > > > > We can do both. The file is generated and installed as part of the > > > build/install process, and it can be simply used as-is even if some of > > > the modules are missing. > > > > > > It's also possible to update the modinfo.json file in postinstall / > > > postuninstall by simply running qemu-modinfo, so only the modules > > > actually installed are listed there. > > > > I fail to see why that extra complication is needed at all. > > > > Why don't we just build the modules for the targets we intend to build, > > and install them as .so files in a target arch directory? > > There is more meta-data we need for modules: Which QOM types are > implemented by which module (for on-demand loading), dependencies > between modules and also which command line options (aka QemuOpts) > are handled by which modules. > > Possibly more in the future, maybe we'll support modules registering > monitor commands dynamically some day (like usb-host implementing > "info usbhost" or tcg implementing "info jit" + "info opcount") and > we'd like store that information in the module database too. > > If we have such a module database anyway it IMHO makes alot of sense > to simply store the target arch there too instead of using something > else. > > > What problem is the json solving? > > Well, the goal is to store meta-data about modules, in a way which: > > (a) Doesn't require manually maintained lists. This is what we have > right now (arrays in utils/module.c). Works ok-ish for a small > number of modules, but becomes increasingly problematic with the > growing number of modules. > (b) Doesn't require opening each individual module on each qemu run > to get the information. > > I'm not particularly attached to using json for that, it is just that > we already have infrastructure to parse/serialize structs from/to json > because we need that for qapi anyway. If we can generate json, we could generate .c code which has all the data statically declared and just link it in to QEMU. If we don't need ability to update the metadata post-build then it would be equivalent functionally. If we need to be able to update the metadata to precisely matcch the set of installed modules though, then a separate json file looks best. Regards, Daniel
Hi, > > I'm not particularly attached to using json for that, it is just that > > we already have infrastructure to parse/serialize structs from/to json > > because we need that for qapi anyway. > > If we can generate json, we could generate .c code which has all the > data statically declared and just link it in to QEMU. Yes, I think that would work too. take care, Gerd
diff --git a/include/qemu/module.h b/include/qemu/module.h index d3cab3c25a2f..7825f6d8c847 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -72,6 +72,7 @@ void module_call_init(module_init_type type); bool module_load_one(const char *prefix, const char *lib_name, bool mayfail); void module_load_qom_one(const char *type); void module_load_qom_all(void); +void module_allow_arch(const char *arch); /* * macros to store module metadata in a .modinfo section. diff --git a/softmmu/vl.c b/softmmu/vl.c index ba26a042b284..96316774fcc9 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -126,6 +126,8 @@ #include "sysemu/iothread.h" #include "qemu/guest-random.h" +#include "config-host.h" + #define MAX_VIRTIO_CONSOLES 1 typedef struct BlockdevOptionsQueueEntry { @@ -2723,6 +2725,7 @@ void qemu_init(int argc, char **argv, char **envp) error_init(argv[0]); qemu_init_exec_dir(argv[0]); + module_allow_arch(TARGET_NAME); qemu_init_subsystems(); /* first pass of option parsing */ diff --git a/util/module.c b/util/module.c index 6e4199169c41..564b8e3da760 100644 --- a/util/module.c +++ b/util/module.c @@ -122,6 +122,12 @@ void module_call_init(module_init_type type) static Modules *modinfo; static char *module_dirs[5]; static int module_ndirs; +static const char *module_arch; + +void module_allow_arch(const char *arch) +{ + module_arch = arch; +} static void module_load_path_init(void) { @@ -295,6 +301,14 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail) module_load_modinfo(); for (modlist = modinfo->list; modlist != NULL; modlist = modlist->next) { + if (modlist->value->has_arch) { + if (strcmp(modlist->value->name, module_name) == 0) { + if (!module_arch || + strcmp(modlist->value->arch, module_arch) != 0) { + return false; + } + } + } if (modlist->value->has_deps) { if (strcmp(modlist->value->name, module_name) == 0) { /* we depend on other module(s) */ @@ -401,6 +415,7 @@ void qemu_load_module_for_opts(const char *group) #else +void module_allow_arch(const char *arch) {} void qemu_load_module_for_opts(const char *group) {} void module_load_qom_one(const char *type) {} void module_load_qom_all(void) {}
Add module_allow_arch() to set the target architecture. In case a module is limited to some arch verify arches match and ignore the module if not. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/qemu/module.h | 1 + softmmu/vl.c | 3 +++ util/module.c | 15 +++++++++++++++ 3 files changed, 19 insertions(+)