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

[Halil Pasic]

On Fri, 5 Mar 2021 16:46:03 -0500
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Mar 02, 2021 at 06:35:44PM +0100, 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. 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.  
> 
> I don't understand what makes it necessary to make the
> type_register() call conditional.  Calling type_register() before
> the parent types are registered is perfectly valid.
> 

Registering a type that ain't going to become a complete type in time
is in my understanding invalid. Why? Because the unsuspecting
client code is about to trigger an abort when it attempts to use
the registered, thus advertised type. That is the state of the art
today. Of course we can morph that, adn make zombie TypeImpl's perfectly
valid.

> I don't think we should prevent type_register() from being
> called.  We just need to prevent type_initialize() from being
> called unless the type definition is complete.  

Yes, but for that we need to catch when type_initialize() is about to be
called, and preferably we should also know if we are dealing with a
modularized type, which is allowed to fail this way, or are we dealing
with a good old built in type, where trying to initialize an incomplete
type is still a result of a programming mistake.

So we would have to catch all the client code, which might be actually
manageable (contrary to my first intuition).

I did a little prototyping. I will post a patch on top of this
patch with the results.

I'm not confident about what I did in that prototype code since for
instance *object_class_by_name gets called about 80 places:
$ git grep -e object_class_by_name|wc -l
82

And to make things even harder to reason about type_initialize() can be 
called through following public interfaces:
object_class_by_name()
object_class_get_parent()
object_initialize()
object_class_foreach()
object_new_with_class()
object_new()
object_new_with_propv()
and here I stopped looking.

If we decide to sacrifice the assertions, and make incomplete types
first class citizens regardless their origin (registered form a module
or form core qemu), we would still have to take care of some 8
invocations of type_initialize(). (Sacrificing the assertions
may be a good idea if we have appropriate test coverage which
would complain about the functionality that we silently discarded.)

Another option to preserve the old behavior for the vast majority of
types would be to mark "special" TypeImpl's as 'have to be careful
before calling type_initialize()', but that doesn't sound very beutiful
either. 

I can try myself at any of these, but I don't mind if somebody who has
a better understanding of object.c takes over.

The reason why I choose to propose making virtio-gpu-ccw call
type_register() conditionally if built as a module, is because
it was easy to reason about the impact of that: it impacts
virtio-gpu-ccw and that is it. Yes, it smells like technical
debt.

> This way there
> won't be any tricky module loading order requirements.
> 

While I don't fully understand the situation when types are registered
from modules in an up and running qemu, I do see a significant
benefit in modules being able to register a type without having all
dependencies met with respect to loading order requirements. 

[..]
> I suggest splitting the QOM core changes and s390x-specific
> changes into separate patches, so they can be reviewed and
> included separately.

No problem, I can do that.

---------------------------8<------------------------------
From: Halil Pasic <pasic@linux.ibm.com>
Date: Mon, 8 Mar 2021 21:34:00 +0100
Subject: [PATCH 1/1] WIP: prevent type_initialize() with incomplete type

Eduardo suggested that instead of checking if the type is going to be a
complete one, before registering it form the virtio-gpu-ccw module we
should actually prevent a type_initialize() from being called unless
the type is complete.

Now if we do that for each and every type we may end up hiding bugs.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
 hw/s390x/virtio-ccw-gpu.c |  5 -----
 qom/object.c              | 22 +++++++++++++++++++++-
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/virtio-ccw-gpu.c b/hw/s390x/virtio-ccw-gpu.c
index ccdf6ac20f..c301e2586b 100644
--- a/hw/s390x/virtio-ccw-gpu.c
+++ b/hw/s390x/virtio-ccw-gpu.c
@@ -62,11 +62,6 @@ static const TypeInfo virtio_ccw_gpu = {
 
 static void virtio_ccw_gpu_register(void)
 {
-#ifdef CONFIG_MODULES
-    if (!type_ancestors_registered(&virtio_ccw_gpu)) {
-        return;
-    }
-#endif
     type_register_static(&virtio_ccw_gpu);
 }
 
diff --git a/qom/object.c b/qom/object.c
index 03bd9aa2ba..77c159f827 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1061,6 +1061,23 @@ const char *object_class_get_name(ObjectClass *klass)
     return klass->type->name;
 }
 
+static ObjectClass *__module_object_class_by_name(const char *typename)
+{
+    TypeImpl *type = type_get_by_name(typename);
+
+    if (!type) {
+        return NULL;
+    }
+
+    if (!__type_ancestors_registered(type)) {
+        return NULL;
+    }
+
+    type_initialize(type);
+
+    return type->class;
+}
+
 ObjectClass *object_class_by_name(const char *typename)
 {
     TypeImpl *type = type_get_by_name(typename);
@@ -1082,7 +1099,7 @@ ObjectClass *module_object_class_by_name(const char 
*typename)
 #ifdef CONFIG_MODULES
     if (!oc) {
         module_load_qom_one(typename);
-        oc = object_class_by_name(typename);
+        oc = __module_object_class_by_name(typename);
     }
 #endif
     return oc;
@@ -1116,6 +1133,9 @@ static void object_class_foreach_tramp(gpointer key, 
gpointer value,
     TypeImpl *type = value;
     ObjectClass *k;
 
+    if (!__type_ancestors_registered(type)) {
+        return;
+    }
     type_initialize(type);
     k = type->class;