Re: [RFC PATCH 1/2] modules: introduce target specific modules

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Halil,

On 3/16/21 1:26 PM, Halil Pasic wrote:
> After some back-and-forth in [1] Daniel suggested that
> we could/should make qemu modules per-target by introducing a
> dedicated modules directory for each target, which can symlink the
> modules that do work with and do make sense for the given target.
> 
> That way we can avoid trying to load modules we know won't work and
> coming up with convoluted ways for making subsequent failures graceful.
> The topic of per-target modules was discussed before [1] but, the
> idea with the symlinks originates from [1].
> 
> This patch introduces  this new scheme of loading modules without
> actually introducing any changes to what modules are available to what
> targets. For the exploitation have look at 'hw/s390x: modularize
> virtio-gpu-ccw'.
> 
> [1] https://mail.gnu.org/archive/html/qemu-s390x/2021-03/msg00206.html
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Suggested-by: "Daniel P. Berrangé" <berrange@redhat.com>
> ---
>  hw/s390x/meson.build               |  1 -
>  include/qemu/module.h              |  2 ++
>  meson.build                        | 43 +++++++++++++++++++++++++++++-
>  roms/SLOF                          |  2 +-
>  roms/opensbi                       |  2 +-
>  scripts/call_generated_script.sh   |  6 +++++
>  scripts/modules/target-symlinks.sh | 31 +++++++++++++++++++++
>  softmmu/runstate.c                 |  1 +
>  util/module.c                      | 13 +++++++--
>  9 files changed, 95 insertions(+), 6 deletions(-)
>  create mode 100755 scripts/call_generated_script.sh
>  create mode 100755 scripts/modules/target-symlinks.sh

> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 944d403cbd..85a59fde81 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -73,4 +73,6 @@ 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 set_emulator_modules_dir(char const *dir_name);
> +
>  #endif
> diff --git a/meson.build b/meson.build
> index a7d2dd429d..8926968182 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1749,6 +1749,7 @@ user_ss = ss.source_set()
>  util_ss = ss.source_set()
>  
>  modules = {}
> +modules_restricted = {}
>  hw_arch = {}
>  target_arch = {}
>  target_softmmu_arch = {}
> @@ -2092,14 +2093,54 @@ common_ss.add(hwcore)
>  # Targets #
>  ###########
>  
> +module_targets = []
>  foreach m : block_mods + softmmu_mods
> -  shared_module(m.name(),
> +   module_targets += shared_module(m.name(),
>                  name_prefix: '',
>                  link_whole: m,
>                  install: true,
>                  install_dir: qemu_moddir)
>  endforeach
>  
> +foreach target : target_dirs
> +  if not target.endswith('-softmmu')
> +    continue
> +  endif
> +  filtered_module_targets = []
> +  foreach m : module_targets
> +    restricted_to = modules_restricted.get(m.name(), [])
> +    if restricted_to.length() == 0 or restricted_to.contains(target)
> +      filtered_module_targets += m
> +    endif
> +  endforeach
> +  s = custom_target('Make symbolic links script for ' + target + ' modules' ,
> +		   input: filtered_module_targets,
> +		   output: 'make_mod_symlinks_'+target+'.sh',
> +		   install: false,
> +		   depends: filtered_module_targets,
> +		   build_by_default: true,
> +		   build_always_stale: true,
> +		   command: [
> +                     meson.current_source_dir() / 'scripts/modules/target-symlinks.sh',
> +		     '@OUTPUT@',
> +		     target,
> +		     '@INPUT@'
> +		   ])
> +
> +  # We run the script as a part of the build so things keep working form the
> +  # build tree (without requiring an instalation). I couldn't find a nicer way.
> +  custom_target('Run symbolic links script for ' + target + ' modules' ,
> +		   depends: s,
> +		   output: 'make_mod_symlinks_'+target+'.sh.dummy',
> +		   install: false,
> +		   build_by_default: true,
> +		   build_always_stale: true,
> +		   command: [
> +		     s.full_path(),
> +                     meson.current_build_dir()
> +		   ])
> +  meson.add_install_script(meson.current_source_dir() / 'scripts/call_generated_script.sh', s.full_path(), qemu_moddir)
> +endforeach
>  softmmu_ss.add(authz, blockdev, chardev, crypto, io, qmp)
>  common_ss.add(qom, qemuutil)
>  
> diff --git a/roms/SLOF b/roms/SLOF
> index 33a7322de1..e18ddad851 160000
> --- a/roms/SLOF
> +++ b/roms/SLOF
> @@ -1 +1 @@
> -Subproject commit 33a7322de13e9dca4b38851a345a58d37e7a441d
> +Subproject commit e18ddad8516ff2cfe36ec130200318f7251aa78c
> diff --git a/roms/opensbi b/roms/opensbi
> index 234ed8e427..a98258d0b5 160000
> --- a/roms/opensbi
> +++ b/roms/opensbi
> @@ -1 +1 @@
> -Subproject commit 234ed8e427f4d92903123199f6590d144e0d9351
> +Subproject commit a98258d0b537a295f517bbc8d813007336731fa9

While your patch deals with "target modules", the 2 submodule
changes are unrelated, right?