* [RFC PATCH 1/2] modules: introduce target specific modules
2021-03-16 12:26 [RFC PATCH 0/2] hw/s390x: modularize virtio-gpu-ccw Halil Pasic
@ 2021-03-16 12:26 ` Halil Pasic
2021-03-18 11:36 ` Philippe Mathieu-Daudé
2021-03-16 12:26 ` [RFC PATCH 2/2] hw/s390x: modularize virtio-gpu-ccw Halil Pasic
1 sibling, 1 reply; 5+ messages in thread
From: Halil Pasic @ 2021-03-16 12:26 UTC (permalink / raw)
To: Cornelia Huck, Thomas Huth, Richard Henderson, David Hildenbrand,
Christian Borntraeger, Gerd Hoffmann, Paolo Bonzini,
Marc-André Lureau, Philippe Mathieu-Daudé,
Stefan Hajnoczi, David Gibson, Alexey Kardashevskiy,
Alistair Francis, Bin Meng, Palmer Dabbelt, Anup Patel,
Alex Bennée, Alejandro Jimenez, Eduardo Habkost,
Laurent Vivier, qemu-s390x, qemu-devel
Cc: Halil Pasic, Boris Fiuczynski, Daniel P. Berrangé,
Bruce Rogers
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/hw/s390x/meson.build b/hw/s390x/meson.build
index 91495b5631..0cee605f0a 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true: files('virtio-ccw-balloon.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-ccw-blk.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('virtio-ccw-crypto.c'))
-virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: files('virtio-ccw-gpu.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: files('virtio-ccw-input.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-ccw-net.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-ccw-rng.c'))
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
diff --git a/scripts/call_generated_script.sh b/scripts/call_generated_script.sh
new file mode 100755
index 0000000000..1743b39d7c
--- /dev/null
+++ b/scripts/call_generated_script.sh
@@ -0,0 +1,6 @@
+#!/bin/bash
+
+SCRIPT=$1
+shift
+
+${SCRIPT} "$@"
diff --git a/scripts/modules/target-symlinks.sh b/scripts/modules/target-symlinks.sh
new file mode 100755
index 0000000000..25a402a27f
--- /dev/null
+++ b/scripts/modules/target-symlinks.sh
@@ -0,0 +1,31 @@
+#!/bin/bash
+
+TARGET_FILE="$1"
+shift
+TARGET_DIR="$1"
+shift
+
+cat > "${TARGET_FILE}" <<EOF
+#!/bin/bash
+
+test \$# -eq 1 || exit 1
+if [ -z \${MESON_INSTALL_DESTDIR_PREFIX+N} ]; then
+ INSTALL_DIR="\${1}"
+else
+ INSTALL_DIR="\${MESON_INSTALL_DESTDIR_PREFIX}/\${1}"
+fi
+test -d "\${INSTALL_DIR}" || exit 1
+LINKS_DIR="\${INSTALL_DIR}/${TARGET_DIR}"
+mkdir -p \${LINKS_DIR}
+# clean up old symbolic links
+find \${LINKS_DIR} -iname '*.so' -type l -delete
+cd "\${LINKS_DIR}"
+
+EOF
+chmod u+x "${TARGET_FILE}"
+
+while test $# -gt 0
+do
+ echo ln -sfrt \"\$\{LINKS_DIR\}\" \"../"$1"\" >> "${TARGET_FILE}"
+ shift
+done
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index ce8977c6a2..6842827ad5 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -758,6 +758,7 @@ void qemu_init_subsystems(void)
atexit(qemu_run_exit_notifiers);
+ set_emulator_modules_dir(g_strconcat(TARGET_NAME, "-softmmu"));
module_call_init(MODULE_INIT_QOM);
module_call_init(MODULE_INIT_MIGRATION);
diff --git a/util/module.c b/util/module.c
index c65060c167..26fc893d98 100644
--- a/util/module.c
+++ b/util/module.c
@@ -64,6 +64,15 @@ static ModuleTypeList *find_type(module_init_type type)
return &init_type_list[type];
}
+static char const *emulator_modules_dir;
+
+void set_emulator_modules_dir(char const *dir_name)
+{
+ assert(dir_name);
+ assert(!emulator_modules_dir);
+ emulator_modules_dir = dir_name;
+}
+
void register_module_init(void (*fn)(void), module_init_type type)
{
ModuleEntry *e;
@@ -252,8 +261,8 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
assert(n_dirs <= ARRAY_SIZE(dirs));
for (i = 0; i < n_dirs; i++) {
- fname = g_strdup_printf("%s/%s%s",
- dirs[i], module_name, CONFIG_HOST_DSOSUF);
+ fname = g_strdup_printf("%s/%s/%s%s", dirs[i], emulator_modules_dir,
+ module_name, CONFIG_HOST_DSOSUF);
ret = module_load_file(fname, mayfail, export_symbols);
g_free(fname);
fname = NULL;
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [RFC PATCH 2/2] hw/s390x: modularize virtio-gpu-ccw
2021-03-16 12:26 [RFC PATCH 0/2] hw/s390x: modularize virtio-gpu-ccw Halil Pasic
2021-03-16 12:26 ` [RFC PATCH 1/2] modules: introduce target specific modules Halil Pasic
@ 2021-03-16 12:26 ` Halil Pasic
1 sibling, 0 replies; 5+ messages in thread
From: Halil Pasic @ 2021-03-16 12:26 UTC (permalink / raw)
To: Cornelia Huck, Thomas Huth, Richard Henderson, David Hildenbrand,
Christian Borntraeger, Gerd Hoffmann, Paolo Bonzini,
Marc-André Lureau, Philippe Mathieu-Daudé,
Stefan Hajnoczi, David Gibson, Alexey Kardashevskiy,
Alistair Francis, Bin Meng, Palmer Dabbelt, Anup Patel,
Alex Bennée, Alejandro Jimenez, Eduardo Habkost,
Laurent Vivier, qemu-s390x, qemu-devel
Cc: Halil Pasic, Boris Fiuczynski, Daniel P. Berrange, Bruce Rogers
Since the virtio-gpu-ccw device depends on the hw-display-virtio-gpu
module, which provides the type virtio-gpu-device, packaging the
hw-display-virtio-gpu module as a separate package that may or may not
be installed along with the qemu package leads to problems. Namely if
the hw-display-virtio-gpu is absent, qemu continues to advertise
virtio-gpu-ccw, but it aborts not only when one attempts using
virtio-gpu-ccw, but also when libvirtd's capability probing tries
to instantiate the type to introspect it.
Let us thus introduce a module named hw-s390x-virtio-gpu-ccw that
is going to provide the virtio-gpu-ccw device. The hw-s390x prefix
was chosen because it is not a portable device.
With virtio-gpu-ccw built as a module, the correct way to package a
modularized qemu is to require that hw-display-virtio-gpu must be
installed whenever the module hw-s390x-virtio-gpu-ccw.
The definition S390_ADAPTER_SUPPRESSIBLE was moved to "cpu.h", per
suggestion of Thomas Huth. From interface design perspective, IMHO, not
a good thing as it belongs to the public interface of
css_register_io_adapters(). We did this because CONFIG_KVM requeires
NEED_CPU_H and Thomas, and other commenters did not like the
consequences of that.
Moving the interrupt related declarations to s390_flic.h was suggested
by Cornelia Huck.
Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
hw/s390x/meson.build | 7 +++++++
include/hw/s390x/css.h | 7 -------
include/hw/s390x/s390_flic.h | 3 +++
target/s390x/cpu.h | 9 ++++++---
util/module.c | 1 +
5 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 0cee605f0a..ad2941f7ca 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -47,3 +47,10 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'
s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
hw_arch += {'s390x': s390x_ss}
+
+hw_s390x_modules = {}
+virtio_gpu_ccw_ss = ss.source_set()
+virtio_gpu_ccw_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: [files('virtio-ccw-gpu.c'), pixman])
+hw_s390x_modules += {'virtio-gpu-ccw': virtio_gpu_ccw_ss}
+modules += {'hw-s390x': hw_s390x_modules}
+modules_restricted += {'hw-s390x-virtio-gpu-ccw': ['s390x-softmmu']}
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 7901ab276c..bba7593d2e 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -12,7 +12,6 @@
#ifndef CSS_H
#define CSS_H
-#include "cpu.h"
#include "hw/s390x/adapter.h"
#include "hw/s390x/s390_flic.h"
#include "hw/s390x/ioinst.h"
@@ -233,12 +232,6 @@ uint32_t css_get_adapter_id(CssIoAdapterType type, uint8_t isc);
void css_register_io_adapters(CssIoAdapterType type, bool swap, bool maskable,
uint8_t flags, Error **errp);
-#ifndef CONFIG_KVM
-#define S390_ADAPTER_SUPPRESSIBLE 0x01
-#else
-#define S390_ADAPTER_SUPPRESSIBLE KVM_S390_ADAPTER_SUPPRESSIBLE
-#endif
-
#ifndef CONFIG_USER_ONLY
SubchDev *css_find_subch(uint8_t m, uint8_t cssid, uint8_t ssid,
uint16_t schid);
diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h
index e91b15d2d6..3907a13d07 100644
--- a/include/hw/s390x/s390_flic.h
+++ b/include/hw/s390x/s390_flic.h
@@ -134,6 +134,9 @@ void s390_flic_init(void);
S390FLICState *s390_get_flic(void);
QEMUS390FLICState *s390_get_qemu_flic(S390FLICState *fs);
S390FLICStateClass *s390_get_flic_class(S390FLICState *fs);
+void s390_crw_mchk(void);
+void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr,
+ uint32_t io_int_parm, uint32_t io_int_word);
bool ais_needed(void *opaque);
#endif /* HW_S390_FLIC_H */
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 60d434d5ed..b434b905c0 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -40,6 +40,12 @@
#define S390_MAX_CPUS 248
+#ifndef CONFIG_KVM
+#define S390_ADAPTER_SUPPRESSIBLE 0x01
+#else
+#define S390_ADAPTER_SUPPRESSIBLE KVM_S390_ADAPTER_SUPPRESSIBLE
+#endif
+
typedef struct PSW {
uint64_t mask;
uint64_t addr;
@@ -806,9 +812,6 @@ int cpu_s390x_signal_handler(int host_signum, void *pinfo, void *puc);
/* interrupt.c */
-void s390_crw_mchk(void);
-void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr,
- uint32_t io_int_parm, uint32_t io_int_word);
#define RA_IGNORED 0
void s390_program_interrupt(CPUS390XState *env, uint32_t code, uintptr_t ra);
/* service interrupts are floating therefore we must not pass an cpustate */
diff --git a/util/module.c b/util/module.c
index 26fc893d98..5638202357 100644
--- a/util/module.c
+++ b/util/module.c
@@ -313,6 +313,7 @@ static struct {
{ "virtio-gpu-pci-base", "hw-", "display-virtio-gpu-pci" },
{ "virtio-gpu-pci", "hw-", "display-virtio-gpu-pci" },
{ "vhost-user-gpu-pci", "hw-", "display-virtio-gpu-pci" },
+ { "virtio-gpu-ccw", "hw-", "s390x-virtio-gpu-ccw" },
{ "virtio-vga-base", "hw-", "display-virtio-vga" },
{ "virtio-vga", "hw-", "display-virtio-vga" },
{ "vhost-user-vga", "hw-", "display-virtio-vga" },
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread