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

[Cornelia Huck]

On Tue,  2 Mar 2021 18:35:44 +0100
Halil Pasic <pasic@linux.ibm.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. Because registering
> virtio-gpu-ccw would make non-s390x emulator fail due to a missing
> parent type, if built as a module, before registering it, we check
> if the ancestor types are already registered.
> 
> 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>
> ---
> 
> As explained in [1] the previous idea of type_register_mayfail() does
> not work. The next best thing is to check if all types we need are
> already registered before registering virtio-gpu-ccw from the module. It
> is reasonable to assume that when the module is loaded, the ancestors
> are already registered (which is not the case if the device is a
> built in one).
> 
> The alternatives to this approch I could identify are:
> * A poor mans version of this which checks for the parent
> * Emulator specific modules:
>   * An emulator specific directory within the modules directory which
>     is ignored by the other emulators.
>   * A way to tell the shared util library the name of this directory,
>     and the code to check it if set.
>   * Build hw-s390x-virtio-gpu-ccw so it lands in this special directory
>     in the build tree, and install it there as well.
>   I've spend some time with looking into this, but I came to the
>   conclusion that the two latter points look hairy.
> 
> [1] 
> https://lore.kernel.org/qemu-devel/20210222125548.346166-1-pasic@linux.ibm.com/T/#maf0608df5479f87b23606f01f732740d2617b458
> ---
>  hw/s390x/meson.build         |  7 ++++-
>  hw/s390x/virtio-ccw-gpu.c    |  5 ++++
>  include/hw/s390x/css.h       |  7 -----
>  include/hw/s390x/s390_flic.h |  3 +++
>  include/qom/object.h         | 10 ++++++++
>  qom/object.c                 | 50 ++++++++++++++++++++++++++++++++++++
>  target/s390x/cpu.h           |  9 ++++---
>  util/module.c                |  1 +
>  8 files changed, 81 insertions(+), 11 deletions(-)

The s390x part looks fine, but I'm not that well versed in the object
and module stuff...