Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Thomas Huth]

On 18/02/2021 03.22, Halil Pasic 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 <pasic@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?

 Thomas


> +  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_dependencies])
> +  hw_s390x_modules += {'virtio-gpu-ccw': virtio_gpu_ccw_ss}
> +
> +  modules += {'hw-s390x': hw_s390x_modules}
> +endif
> diff --git a/util/module.c b/util/module.c
> index c65060c167..cbe89fede6 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -304,6 +304,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"    },
>
> base-commit: 1af5629673bb5c1592d993f9fb6119a62845f576