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

[Halil Pasic <pasic@linux.ibm.com>]

On Fri, 5 Mar 2021 12:54:42 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> 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...
> 

Thanks Conny! Gerd was so kind to provide review from that perspective.
I'm hoping on his continued feedback :)

Have a nice weekend!