On Fri, 5 Mar 2021 12:54:42 +0100 Cornelia Huck <coh...@redhat.com> wrote:
> On Tue, 2 Mar 2021 18:35:44 +0100 > Halil Pasic <pa...@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 <pa...@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-pa...@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!