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

[Halil Pasic]

On Wed, 24 Feb 2021 17:46:34 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 24 Feb 2021 12:36:17 +0100
> Gerd Hoffmann <kraxel@redhat.com> wrote:
[..]
> >   
> > > -static TypeImpl *type_register_internal(const TypeInfo *info)
> > > +static TypeImpl *type_register_internal(const TypeInfo *info, bool 
> > > mayfail)
> > >  {
> > >      TypeImpl *ti;
> > >      ti = type_new(info);    
> > 
> > Hmm, type_register_internal seems to not look at the new mayfail flag.
> > Patch looks incomplete ...  
> 
> It definitely is. I messed up my smoke test (used the wrong executable)
> so I did not notice.
> 

I'm pretty confident the whole approach with type_register_mayfail()
is not a good one, because of how the type-system in QEMU works. Let me
try to explain.

First the situation before/without modules. QEMU runtime type-system is
built in two non-overlapping phases. First all types are registered using
type_register() and co. which take a TypeInfo as an argument. At this stage,
the references to other types are name strings, and dependencies do not
matter. Types can be registered in an arbitrary order. The second phase
is a lazy and full instantiaton of the types which involves a call to
type_initialize(). This can be triggered at various point, but here we
need types we depend on registered.

In other words the problem is not that type_register() fails with an
abort. The abort comes much later. E.g. from qdev_get_device_class().

$ gdb -q -ex r --args ./qemu-system-x86_64 -device virtio-gpu-ccw
[..]
Type 'virtio-gpu-ccw' is missing its parent 'virtio-ccw-device'

Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted.
[..]
(gdb) bt -frame-info short-location -no-filters -frame-arguments none
#0  0x000003fffd34a9e6 in raise ()
#1  0x000003fffd32b848 in abort ()
#2  0x000002aa009133f2 in type_get_parent (type=..., type=..., mayfail=...)
#3  type_get_parent (mayfail=..., type=...)
#4  type_class_get_size (ti=...)
#5  type_initialize (ti=..., mayfail=...)
#6  0x000002aa00914b10 in object_class_by_name (typename=...)
#7  0x000002aa005ec7e0 in qdev_get_device_class (errp=..., driver=...)
#8  qdev_device_add (opts=..., errp=...)
#9  0x000002aa007f73ce in device_init_func (opaque=..., opts=..., errp=...)
#10 0x000002aa00aa084e in qemu_opts_foreach (list=..., func=..., opaque=..., 
errp=...)
#11 0x000002aa007fa538 in qemu_create_cli_devices ()
#12 qmp_x_exit_preconfig (errp=...)
#13 0x000002aa007fdb88 in qemu_init (argc=..., argv=..., envp=...)
#14 0x000002aa00416b72 in main (argc=..., argv=..., envp=...)
(gdb)

The only approaches I can think of to make type_register_mayfail()
"work" involve adding a dependency check in type_register_internal()
before the call to type_table_add() is made. This can "work" for modules,
because for types loaded from we can hope, that all dependencies are
already, as modules are loaded relatively late.  I have experimented,
with achieving this via type_initialize(ti, mayfail) because my
incomplete patch has done some work in that direction. It "works" but
it turned out *very ugly*. I can send it to you if you like, but I don't
think there is a point in burdening the list with it. A somewhat better
approach would be to introduce a dedicated function for
dependency-checking a TypeInfo. But the more I'm thinking about this, the
more I'm in favor of inventing the target specific modules (despite the
fact, that I'm not very confident about the doing part).

How do you think we should proceed?

Many thanks for your help up till now.

Regards,
Halil