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

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(-)


base-commit: 1af5629673bb5c1592d993f9fb6119a62845f576

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.

polite ping

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

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!

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.

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.  This way there
won't be any tricky module loading order requirements.


> 
> 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(-)

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

> 
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index 2a7818d94b..7ac972afcf 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
>  virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true: files('virtio-ccw-balloon.c'))
>  virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-ccw-blk.c'))
>  virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('virtio-ccw-crypto.c'))
> -virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: files('virtio-ccw-gpu.c'))
>  virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: files('virtio-ccw-input.c'))
>  virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-ccw-net.c'))
>  virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-ccw-rng.c'))
> @@ -46,3 +45,9 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'
>  s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
>  
>  hw_arch += {'s390x': s390x_ss}
> +
> +hw_s390x_modules = {}
> +virtio_gpu_ccw_ss = ss.source_set()
> +virtio_gpu_ccw_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: [files('virtio-ccw-gpu.c'), pixman])
> +hw_s390x_modules += {'virtio-gpu-ccw': virtio_gpu_ccw_ss}
> +modules += {'hw-s390x': hw_s390x_modules}
> diff --git a/hw/s390x/virtio-ccw-gpu.c b/hw/s390x/virtio-ccw-gpu.c
> index c301e2586b..ccdf6ac20f 100644
> --- a/hw/s390x/virtio-ccw-gpu.c
> +++ b/hw/s390x/virtio-ccw-gpu.c
> @@ -62,6 +62,11 @@ 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/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 08c869ab0a..7858666307 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -12,7 +12,6 @@
>  #ifndef CSS_H
>  #define CSS_H
>  
> -#include "cpu.h"
>  #include "hw/s390x/adapter.h"
>  #include "hw/s390x/s390_flic.h"
>  #include "hw/s390x/ioinst.h"
> @@ -233,12 +232,6 @@ uint32_t css_get_adapter_id(CssIoAdapterType type, uint8_t isc);
>  void css_register_io_adapters(CssIoAdapterType type, bool swap, bool maskable,
>                                uint8_t flags, Error **errp);
>  
> -#ifndef CONFIG_KVM
> -#define S390_ADAPTER_SUPPRESSIBLE 0x01
> -#else
> -#define S390_ADAPTER_SUPPRESSIBLE KVM_S390_ADAPTER_SUPPRESSIBLE
> -#endif
> -
>  #ifndef CONFIG_USER_ONLY
>  SubchDev *css_find_subch(uint8_t m, uint8_t cssid, uint8_t ssid,
>                           uint16_t schid);
> diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h
> index e91b15d2d6..3907a13d07 100644
> --- a/include/hw/s390x/s390_flic.h
> +++ b/include/hw/s390x/s390_flic.h
> @@ -134,6 +134,9 @@ void s390_flic_init(void);
>  S390FLICState *s390_get_flic(void);
>  QEMUS390FLICState *s390_get_qemu_flic(S390FLICState *fs);
>  S390FLICStateClass *s390_get_flic_class(S390FLICState *fs);
> +void s390_crw_mchk(void);
> +void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr,
> +                       uint32_t io_int_parm, uint32_t io_int_word);
>  bool ais_needed(void *opaque);
>  
>  #endif /* HW_S390_FLIC_H */
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 6721cd312e..7aa996a042 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -815,6 +815,16 @@ ObjectClass *object_get_class(Object *obj);
>   */
>  const char *object_get_typename(const Object *obj);
>  
> +/**
> + * type_ancestors_registered:
> + * @info: The #TypeInfo of the type
> + *
> + * Returns: true if all the ancestor types, that is classes and interfaces this
> + * type inherits form are all already registered, false if there is an ancestor
> + * that ain't registered yet
> + */
> +bool type_ancestors_registered(const TypeInfo *info);
> +
>  /**
>   * type_register_static:
>   * @info: The #TypeInfo of the new type.
> diff --git a/qom/object.c b/qom/object.c
> index 491823db4a..03bd9aa2ba 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -281,6 +281,56 @@ static void object_property_free(gpointer data)
>      g_free(prop);
>  }
>  
> +static TypeImpl *type_get_parent_const(const TypeImpl *ti)
> +{
> +    return ti->parent_type ? ti->parent_type : type_get_by_name(ti->parent);
> +}
> +
> +
> +static bool __type_ancestors_registered(const TypeImpl *ti)
> +{
> +    TypeImpl *parent;
> +    int i;
> +
> +    if (!ti) {
> +        return false;
> +    }
> +
> +    if (ti->class) {
> +        /* fully initialized */
> +        return true;
> +    }
> +
> +    for (i = 0; i < ti->num_interfaces; i++) {
> +        if (!type_get_by_name(ti->interfaces[i].typename)) {
> +            return false;
> +        }
> +    }
> +    if (ti->parent) {
> +        parent = type_get_parent_const(ti);
> +        if (!parent) {
> +            return false;
> +        }
> +        return __type_ancestors_registered(parent);
> +    }
> +    return true;
> +}
> +
> +bool type_ancestors_registered(const TypeInfo *info)
> +{
> +    int i;
> +
> +    for (i = 0; info->interfaces && info->interfaces[i].type; i++) {
> +        if (!type_get_by_name(info->interfaces[i].type)) {
> +            return false;
> +        }
> +    }
> +    if (info->parent) {
> +        return __type_ancestors_registered(type_get_by_name(info->parent));
> +    }
> +    return true;
> +}
> +
>  static void type_initialize(TypeImpl *ti)
>  {
>      TypeImpl *parent;
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 60d434d5ed..b434b905c0 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -40,6 +40,12 @@
>  
>  #define S390_MAX_CPUS 248
>  
> +#ifndef CONFIG_KVM
> +#define S390_ADAPTER_SUPPRESSIBLE 0x01
> +#else
> +#define S390_ADAPTER_SUPPRESSIBLE KVM_S390_ADAPTER_SUPPRESSIBLE
> +#endif
> +
>  typedef struct PSW {
>      uint64_t mask;
>      uint64_t addr;
> @@ -806,9 +812,6 @@ int cpu_s390x_signal_handler(int host_signum, void *pinfo, void *puc);
>  
>  
>  /* interrupt.c */
> -void s390_crw_mchk(void);
> -void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr,
> -                       uint32_t io_int_parm, uint32_t io_int_word);
>  #define RA_IGNORED                  0
>  void s390_program_interrupt(CPUS390XState *env, uint32_t code, uintptr_t ra);
>  /* service interrupts are floating therefore we must not pass an cpustate */
> diff --git a/util/module.c b/util/module.c
> index c65060c167..cbe89fede6 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -304,6 +304,7 @@ static struct {
>      { "virtio-gpu-pci-base",   "hw-", "display-virtio-gpu-pci" },
>      { "virtio-gpu-pci",        "hw-", "display-virtio-gpu-pci" },
>      { "vhost-user-gpu-pci",    "hw-", "display-virtio-gpu-pci" },
> +    { "virtio-gpu-ccw",        "hw-", "s390x-virtio-gpu-ccw"   },
>      { "virtio-vga-base",       "hw-", "display-virtio-vga"    },
>      { "virtio-vga",            "hw-", "display-virtio-vga"    },
>      { "vhost-user-vga",        "hw-", "display-virtio-vga"    },
> 
> base-commit: 1af5629673bb5c1592d993f9fb6119a62845f576
> -- 
> 2.25.1
>

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;

On Fri, Mar 05, 2021 at 04:46:03PM -0500, Eduardo Habkost 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.

Well, yes, in a non-modular world this will work perfectly fine.  We
only compile objects actually supported into the system emulator.

With modular builds we have the situation that we have shared modules
which may or may not work in specific system emulators, for example
hw-display-virtio-gpu-pci.so or the ccw module added by this patch,
because the given system emulator doesn't provide the needed support.
We have some which don't support pci (avr for example).  Likewise ccw
is supported by s390x only.

When loading the ccw module into a non-s390x the object initialization
fails due to the missing parent object and we run into an assert.

Loading a pci module into a non-pci system emulator would have an
simliar effect, except that those modules don't load in the first
place due to missing symbol references for pci_* functions.

So we are looking for some way to deal with the situation, i.e.
avoid failing type initialization (we could also catch type
initialitation failues, but there are *lots* if places in qemu ...).
So not registering a type where we know it will fail is the idea here,
taking advantage of the fact that in case of modules the types are
actually loaded and initialized in order, so if the parent type isn't
present at registration time it wouldn't show up later.

Hmm, while thinking about it, a completely different idea:  Maybe add
explicit symbol references instead?  i.e. add "have_$feature = 1"
variables (either unconditionally, or only for the cases where we don't
have symbol references anyway), then reference them in modules needing
the feature like this:

	if (have_bus_ccw) {
		type_register(&type_virtio_gpu_ccw);
	}

take care,
  Gerd

On Tue, Mar 09, 2021 at 01:45:12PM +0100, Gerd Hoffmann wrote:
> On Fri, Mar 05, 2021 at 04:46:03PM -0500, Eduardo Habkost 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.
> 
> Well, yes, in a non-modular world this will work perfectly fine.  We
> only compile objects actually supported into the system emulator.
> 
> With modular builds we have the situation that we have shared modules
> which may or may not work in specific system emulators, for example
> hw-display-virtio-gpu-pci.so or the ccw module added by this patch,
> because the given system emulator doesn't provide the needed support.
> We have some which don't support pci (avr for example).  Likewise ccw
> is supported by s390x only.

We could solve this by having a different location for loading modules
for each target.

  *  /usr/$LIB/qemu/

    All the real .so's go in here as today

  *  /usr/$LIB/qemu/$TARGET

    Populated with symlinks to the .so's in the parent dir,
    but /only/ those which are valid for this $TARGET

Then change QEMU  to load from the second dir.


Regards,
Daniel

On Tue, 9 Mar 2021 13:21:14 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> > Well, yes, in a non-modular world this will work perfectly fine.  We
> > only compile objects actually supported into the system emulator.
> > 
> > With modular builds we have the situation that we have shared modules
> > which may or may not work in specific system emulators, for example
> > hw-display-virtio-gpu-pci.so or the ccw module added by this patch,
> > because the given system emulator doesn't provide the needed support.
> > We have some which don't support pci (avr for example).  Likewise ccw
> > is supported by s390x only.  
> 
> We could solve this by having a different location for loading modules
> for each target.
> 
>   *  /usr/$LIB/qemu/
> 
>     All the real .so's go in here as today
> 
>   *  /usr/$LIB/qemu/$TARGET
> 
>     Populated with symlinks to the .so's in the parent dir,
>     but /only/ those which are valid for this $TARGET
> 
> Then change QEMU  to load from the second dir.

I like the idea. I also prefer not trying to load a module X
that we know is not supported with, and ain't going to work with
target Y, over doing the load and making something fail afterwards.

I've not only started working on this, but I already have something
kind of works. But I'm not quite satisfied with it. I will spend some
more cycles before sending it out, but I doubt I will be able to get it
in a 'looks nice' shape. Thanks you!

Regards,
Halil