On Thu, 18 Feb 2021 11:34:38 +0100 Halil Pasic <pa...@linux.ibm.com> wrote:
> On Thu, 18 Feb 2021 10:23:16 +0100 > Thomas Huth <th...@redhat.com> wrote: > > > > 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. > > > > > > Signed-off-by: Halil Pasic <pa...@linux.ibm.com> > > > --- > > > hw/s390x/meson.build | 17 ++++++++++++++++- > > > util/module.c | 1 + > > > 2 files changed, 17 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build > > > index 2a7818d94b..153b1309fb 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')) > > > @@ -46,3 +45,19 @@ 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} > > > + > > > +if target.startswith('s390x') > > > + hw_s390x_modules = {} > > > + > > > + hw_s390x_modules_c_args = ['-DNEED_CPU_H', > > > + '-DCONFIG_TARGET="@0@-config-target.h"'.format(target)] > > > + hw_s390x_modules_inc = [include_directories('../../target' / > > > config_target['TARGET_BASE_ARCH'])] > > > + hw_s390x_modules_dependencies = declare_dependency( > > > + include_directories: hw_s390x_modules_inc, compile_args: > > > hw_s390x_modules_c_args) > > > > Basically the patch looks fine to me, but I wonder why all that above lines > > (related to hw_s390x_modules_dependencies) are requred at all? The other > > display modules in hw/display/meson.build also do not need to re-define > > c_args for example? > > The explanation is simple. Unlike most devices, the ccw devices aren't > portable. In particular both css.c and css.h includes "cpu.h", and > virtio-ccw-gpu.c includes "qemu/osdep.h". Furthermore osdep.h contains: > #ifdef NEED_CPU_H > #include CONFIG_TARGET > #else > #include "exec/poison.h" > #endif > so if we don't have NEED_CPU_H, among others CONFIG_KVM is poisoned, and > CONFIG_KVM is used in "css.h". Frankly, I can't tell under what circumstances > does css need "cpu.h". s390_crw_mchk() and s390_io_interrupt() are in cpu.h. Nowadays, they use the flic to inject interrupts; but their earlier implementations had a dummy cpu state. I'm wondering whether s390_flic.h is a better place for functions injecting floating interrupts, now that we don't have the non-flic support anymore. > I managed to build the s390x-softmmu target > without it, but decided to put it back. Regarding "osdep.h", I just > assumed includes are done the way they are done for a good reason. Maybe > the includes can be changed in a way that the things you ask about become > unnecessary, but with the code as is they are necessary. Try to drop them > and check out what happens. > > Regards, > Halil >