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

Commit Message

Halil Pasic Feb. 22, 2021, 12:55 p.m. UTC

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.

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.

Introducing type_register_mayfail() was suggested by Gerd Hoffmann.

Signed-off-by: Halil Pasic <[e-mail]>
---
 hw/s390x/meson.build         |  7 +++++-
 hw/s390x/virtio-ccw-gpu.c    |  4 ++++
 include/hw/s390x/css.h       |  7 ------
 include/hw/s390x/s390_flic.h |  3 +++
 include/qom/object.h         | 22 +++++++++++++++++++
 qom/object.c                 | 42 ++++++++++++++++++++++++------------
 target/s390x/cpu.h           |  9 +++++---
 util/module.c                |  1 +
 8 files changed, 70 insertions(+), 25 deletions(-)


base-commit: 1af5629673bb5c1592d993f9fb6119a62845f576

Comments

Boris Fiuczynski Feb. 22, 2021, 5:18 p.m. UTC | #1

Paolo, Daniel,
I am in general (s390 unrelated) a bit puzzled about the scenario of 
QEMU being modularized.
Libvirt probes QEMU executables for their capabilities and creates a 
capabilities cache of the probed QEMU binary. There are a few triggers 
that invalidate the cache. One is the QEMU binary changing.
Is there one for QEMU modules being installed or uninstalled?
How is that supposed to work?


On 2/22/21 1:55 PM, 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.
> 
> 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.
> 
> Introducing type_register_mayfail() was suggested by Gerd Hoffmann.
> 
> Signed-off-by: Halil Pasic <[e-mail]>
> ---
>   hw/s390x/meson.build         |  7 +++++-
>   hw/s390x/virtio-ccw-gpu.c    |  4 ++++
>   include/hw/s390x/css.h       |  7 ------
>   include/hw/s390x/s390_flic.h |  3 +++
>   include/qom/object.h         | 22 +++++++++++++++++++
>   qom/object.c                 | 42 ++++++++++++++++++++++++------------
>   target/s390x/cpu.h           |  9 +++++---
>   util/module.c                |  1 +
>   8 files changed, 70 insertions(+), 25 deletions(-)
> 
> 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..5ac9b6b2a6 100644
> --- a/hw/s390x/virtio-ccw-gpu.c
> +++ b/hw/s390x/virtio-ccw-gpu.c
> @@ -62,7 +62,11 @@ static const TypeInfo virtio_ccw_gpu = {
>   
>   static void virtio_ccw_gpu_register(void)
>   {
> +#ifdef CONFIG_MODULES
> +    type_register_static_mayfail(&virtio_ccw_gpu);
> +#else
>       type_register_static(&virtio_ccw_gpu);
> +#endif
>   }
>   
>   type_init(virtio_ccw_gpu_register)
> 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..3428546d91 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -826,6 +826,17 @@ const char *object_get_typename(const Object *obj);
>    */
>   Type type_register_static(const TypeInfo *info);
>   
> +/**
> + * type_register_static_mayfail:
> + * @info: The #TypeInfo of the new type.
> + *
> + * @info and all of the strings it points to should exist for the life time
> + * that the type is registered.
> + *
> + * Returns: the new #Type or NULL if missing a parent type.
> + */
> +Type type_register_static_mayfail(const TypeInfo *info);
> +
>   /**
>    * type_register:
>    * @info: The #TypeInfo of the new type
> @@ -837,6 +848,17 @@ Type type_register_static(const TypeInfo *info);
>    */
>   Type type_register(const TypeInfo *info);
>   
> +/**
> + * type_register_mayfail:
> + * @info: The #TypeInfo of the new type
> + *
> + * Unlike type_register_static(), this call does not require @info or its
> + * string members to continue to exist after the call returns.
> + *
> + * Returns: the new #Type or NULL if missing a parent type.
> + */
> +Type type_register_mayfail(const TypeInfo *info);
> +
>   /**
>    * type_register_static_array:
>    * @infos: The array of the new type #TypeInfo structures.
> diff --git a/qom/object.c b/qom/object.c
> index 491823db4a..ed217cbfb0 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -135,7 +135,7 @@ static TypeImpl *type_new(const TypeInfo *info)
>       return ti;
>   }
>   
> -static TypeImpl *type_register_internal(const TypeInfo *info)
> +static TypeImpl *type_register_internal(const TypeInfo *info, bool mayfail)
>   {
>       TypeImpl *ti;
>       ti = type_new(info);
> @@ -147,7 +147,13 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
>   TypeImpl *type_register(const TypeInfo *info)
>   {
>       assert(info->parent);
> -    return type_register_internal(info);
> +    return type_register_internal(info, false);
> +}
> +
> +TypeImpl *type_register_mayfail(const TypeInfo *info)
> +{
> +    assert(info->parent);
> +    return type_register_internal(info, true);
>   }
>   
>   TypeImpl *type_register_static(const TypeInfo *info)
> @@ -155,6 +161,11 @@ TypeImpl *type_register_static(const TypeInfo *info)
>       return type_register(info);
>   }
>   
> +TypeImpl *type_register_static_mayfail(const TypeInfo *info)
> +{
> +    return type_register_mayfail(info);
> +}
> +
>   void type_register_static_array(const TypeInfo *infos, int nr_infos)
>   {
>       int i;
> @@ -173,13 +184,16 @@ static TypeImpl *type_get_by_name(const char *name)
>       return type_table_lookup(name);
>   }
>   
> -static TypeImpl *type_get_parent(TypeImpl *type)
> +static TypeImpl *type_get_parent(TypeImpl *type, bool mayfail)
>   {
>       if (!type->parent_type && type->parent) {
>           type->parent_type = type_get_by_name(type->parent);
>           if (!type->parent_type) {
>               fprintf(stderr, "Type '%s' is missing its parent '%s'\n",
>                       type->name, type->parent);
> +            if (mayfail) {
> +                return NULL;
> +            }
>               abort();
>           }
>       }
> @@ -199,7 +213,7 @@ static size_t type_class_get_size(TypeImpl *ti)
>       }
>   
>       if (type_has_parent(ti)) {
> -        return type_class_get_size(type_get_parent(ti));
> +        return type_class_get_size(type_get_parent(ti, false));
>       }
>   
>       return sizeof(ObjectClass);
> @@ -212,7 +226,7 @@ static size_t type_object_get_size(TypeImpl *ti)
>       }
>   
>       if (type_has_parent(ti)) {
> -        return type_object_get_size(type_get_parent(ti));
> +        return type_object_get_size(type_get_parent(ti, false));
>       }
>   
>       return 0;
> @@ -236,7 +250,7 @@ static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
>               return true;
>           }
>   
> -        type = type_get_parent(type);
> +        type = type_get_parent(type, false);
>       }
>   
>       return false;
> @@ -307,7 +321,7 @@ static void type_initialize(TypeImpl *ti)
>       }
>       ti->class = g_malloc0(ti->class_size);
>   
> -    parent = type_get_parent(ti);
> +    parent = type_get_parent(ti, false);
>       if (parent) {
>           type_initialize(parent);
>           GSList *e;
> @@ -357,7 +371,7 @@ static void type_initialize(TypeImpl *ti)
>           if (parent->class_base_init) {
>               parent->class_base_init(ti->class, ti->class_data);
>           }
> -        parent = type_get_parent(parent);
> +        parent = type_get_parent(parent, false);
>       }
>   
>       if (ti->class_init) {
> @@ -368,7 +382,7 @@ static void type_initialize(TypeImpl *ti)
>   static void object_init_with_type(Object *obj, TypeImpl *ti)
>   {
>       if (type_has_parent(ti)) {
> -        object_init_with_type(obj, type_get_parent(ti));
> +        object_init_with_type(obj, type_get_parent(ti, false));
>       }
>   
>       if (ti->instance_init) {
> @@ -383,7 +397,7 @@ static void object_post_init_with_type(Object *obj, TypeImpl *ti)
>       }
>   
>       if (type_has_parent(ti)) {
> -        object_post_init_with_type(obj, type_get_parent(ti));
> +        object_post_init_with_type(obj, type_get_parent(ti, false));
>       }
>   }
>   
> @@ -674,7 +688,7 @@ static void object_deinit(Object *obj, TypeImpl *type)
>       }
>   
>       if (type_has_parent(type)) {
> -        object_deinit(obj, type_get_parent(type));
> +        object_deinit(obj, type_get_parent(type, false));
>       }
>   }
>   
> @@ -1040,7 +1054,7 @@ ObjectClass *module_object_class_by_name(const char *typename)
>   
>   ObjectClass *object_class_get_parent(ObjectClass *class)
>   {
> -    TypeImpl *type = type_get_parent(class->type);
> +    TypeImpl *type = type_get_parent(class->type, false);
>   
>       if (!type) {
>           return NULL;
> @@ -2791,8 +2805,8 @@ static void register_types(void)
>           .abstract = true,
>       };
>   
> -    type_interface = type_register_internal(&interface_info);
> -    type_register_internal(&object_info);
> +    type_interface = type_register_internal(&interface_info, false);
> +    type_register_internal(&object_info, false);
>   }
>   
>   type_init(register_types)
> 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
>

Daniel P. Berrangé Feb. 22, 2021, 5:30 p.m. UTC | #2

On Mon, Feb 22, 2021 at 06:18:57PM +0100, Boris Fiuczynski wrote:
> Paolo, Daniel,
> I am in general (s390 unrelated) a bit puzzled about the scenario of QEMU
> being modularized.
> Libvirt probes QEMU executables for their capabilities and creates a
> capabilities cache of the probed QEMU binary. There are a few triggers that
> invalidate the cache. One is the QEMU binary changing.
> Is there one for QEMU modules being installed or uninstalled?
> How is that supposed to work?

Libvirt doesn't check the modules specifically, but it does look at the
mtime on the directory containing modules, and that should be touched
when a moduled is added/removed.  This is since libvirt 6.8.0 or later.


Regards,
Daniel

Halil Pasic Feb. 23, 2021, 12:14 p.m. UTC | #3

On Mon, 22 Feb 2021 17:30:50 +0000
Daniel P. Berrangé <[e-mail]> wrote:

> > Paolo, Daniel,
> > I am in general (s390 unrelated) a bit puzzled about the scenario of QEMU
> > being modularized.
> > Libvirt probes QEMU executables for their capabilities and creates a
> > capabilities cache of the probed QEMU binary. There are a few triggers that
> > invalidate the cache. One is the QEMU binary changing.
> > Is there one for QEMU modules being installed or uninstalled?
> > How is that supposed to work?  
> 
> Libvirt doesn't check the modules specifically, but it does look at the
> mtime on the directory containing modules, and that should be touched
> when a moduled is added/removed.  This is since libvirt 6.8.0 or later.

It seems we are covered, at least upstream.
,
Regards,
Halil

Gerd Hoffmann Feb. 24, 2021, 11:36 a.m. UTC | #4

>  static void virtio_ccw_gpu_register(void)
>  {
> +#ifdef CONFIG_MODULES
> +    type_register_static_mayfail(&virtio_ccw_gpu);
> +#else
>      type_register_static(&virtio_ccw_gpu);
> +#endif

Move the ifdef to type_register_static_mayfail, so this is not
duplicated for every module which might need this?

> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h

Move this to a separate patch?
The "add type_register_mayfail" and "modularize virtio-gpu-ccw" changes
should be separate patches too.

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

take care,
  Gerd

Halil Pasic Feb. 24, 2021, 4:46 p.m. UTC | #5

On Wed, 24 Feb 2021 12:36:17 +0100
Gerd Hoffmann <[e-mail]> wrote:

> >  static void virtio_ccw_gpu_register(void)
> >  {
> > +#ifdef CONFIG_MODULES
> > +    type_register_static_mayfail(&virtio_ccw_gpu);
> > +#else
> >      type_register_static(&virtio_ccw_gpu);
> > +#endif  
> 
> Move the ifdef to type_register_static_mayfail, so this is not
> duplicated for every module which might need this?

I am concerned about a cluttered API. Having the documentation say:

/**
 * type_register_static_mayfail:
 * @info: The #TypeInfo of the new type.
 *
 * @info and all of the strings it points to should exist for the life time
 * that the type is registered.
 * 
 * If missing a parent type and if qom/object.c is built with CONFIG_MODULES
 * type_register_static_mayfail() differs from type_register_static only in not
 * printing an error and aborting but returning NULL. If qom/object.c is
 * built without CONFIG_MODULES type_register_static_mayfail() is same as
 * type_register_static() 
 * Returns: the new #Type or NULL if missing a parent type.
 */
Type type_register_static_mayfail(const TypeInfo *info);

does not feel right. Indeed modules seems to be the only
circumstance under which a failed type registration does not imply
a programming error. So I'm absolutely against shoving this logic
down into object.c. But I find the variant I posted nicer to document
and nicer to read: looking at virtio_ccw_gpu_register() one sees
immediately that if built as a module, it is OK if the registration
fails, and if built-in it is expected to work.

> 
> > --- a/include/hw/s390x/css.h
> > +++ b/include/hw/s390x/css.h  
> 
> Move this to a separate patch?
> The "add type_register_mayfail" and "modularize virtio-gpu-ccw" changes
> should be separate patches too.
> 
> > -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.

> 
> take care,
>   Gerd
> 
>

Gerd Hoffmann Feb. 25, 2021, 8:05 a.m. UTC | #6

Hi,

> a programming error. So I'm absolutely against shoving this logic
> down into object.c. But I find the variant I posted nicer to document
> and nicer to read: looking at virtio_ccw_gpu_register() one sees
> immediately that if built as a module, it is OK if the registration
> fails, and if built-in it is expected to work.

Ok, makes sense, and we'll probably have not that many cases where we
need this ...

take care,
  Gerd

Halil Pasic Feb. 25, 2021, 9:14 p.m. UTC | #7

On Wed, 24 Feb 2021 17:46:34 +0100
Halil Pasic <[e-mail]> wrote:

> On Wed, 24 Feb 2021 12:36:17 +0100
> Gerd Hoffmann <[e-mail]> 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

Gerd Hoffmann March 3, 2021, 7:07 a.m. UTC | #8

Hi,

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

Yes, for modules the lazy binding should not be needed and we should be
able to check for the parent at registration time.  module.c keeps track
of whenever phase1 init for builtin qom objects did happen already, so
we can use that instead of passing mayfail through a bunch of function
calls.  Quick & dirty test hack below.

BTW: "qemu-system-x86_64 -device help" tries to load all modules and is
a nice test case ;)

HTH,
  Gerd

commit 75ca3012e626318b562bcb51ecdc34400e25f2d0
Author: Gerd Hoffmann <[e-mail]>
Date:   Tue Mar 2 16:26:39 2021 +0100

    [hack] modular type init check

diff --git a/qom/object.c b/qom/object.c
index 491823db4a2d..01785e73f495 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -135,11 +135,22 @@ static TypeImpl *type_new(const TypeInfo *info)
     return ti;
 }
 
+/* HACK: util/module.c */
+extern bool modules_init_done[MODULE_INIT_MAX];
+static TypeImpl *type_get_by_name(const char *name);
+
 static TypeImpl *type_register_internal(const TypeInfo *info)
 {
     TypeImpl *ti;
     ti = type_new(info);
 
+    if (modules_init_done[MODULE_INIT_QOM]) {
+        if (ti->parent && !type_get_by_name(ti->parent)) {
+            g_free(ti);
+            return NULL;
+        }
+    }
+
     type_table_add(ti);
     return ti;
 }
diff --git a/util/module.c b/util/module.c
index cbe89fede628..b7b519eed62c 100644
--- a/util/module.c
+++ b/util/module.c
@@ -34,7 +34,7 @@ typedef struct ModuleEntry
 typedef QTAILQ_HEAD(, ModuleEntry) ModuleTypeList;
 
 static ModuleTypeList init_type_list[MODULE_INIT_MAX];
-static bool modules_init_done[MODULE_INIT_MAX];
+bool modules_init_done[MODULE_INIT_MAX];
 
 static ModuleTypeList dso_init_list;

Halil Pasic March 3, 2021, 2:36 p.m. UTC | #9

On Wed, 3 Mar 2021 08:07:50 +0100
Gerd Hoffmann <[e-mail]> wrote:

>   Hi,
> 
> > 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.  
> 
> Yes, for modules the lazy binding should not be needed and we should be
> able to check for the parent at registration time.  module.c keeps track
> of whenever phase1 init for builtin qom objects did happen already, so
> we can use that instead of passing mayfail through a bunch of function
> calls.  Quick & dirty test hack below.

Hi Gerd! Thank you very much for your patience. I've sent out a v3 yesterday,
which takes a similar, yet slightly different approach. I will comment
on the proposed diff below. Could you please have a look at my v3? I
would prefer if we can go forward with that solution, but I am more than
willing to prepare a v4 based on this proposal.


> 
> BTW: "qemu-system-x86_64 -device help" tries to load all modules and is
> a nice test case ;)

Yes, I've reported the problem in 
Message-ID: <[e-mail]>
using that, for the trace I took -device virtio-gpu-ccw because
the trace looked nicer to me. ;)

> 
> HTH,
>   Gerd
> 
> commit 75ca3012e626318b562bcb51ecdc34400e25f2d0
> Author: Gerd Hoffmann <[e-mail]>
> Date:   Tue Mar 2 16:26:39 2021 +0100
> 
>     [hack] modular type init check
> 
> diff --git a/qom/object.c b/qom/object.c
> index 491823db4a2d..01785e73f495 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -135,11 +135,22 @@ static TypeImpl *type_new(const TypeInfo *info)
>      return ti;
>  }
>  
> +/* HACK: util/module.c */
> +extern bool modules_init_done[MODULE_INIT_MAX];
> +static TypeImpl *type_get_by_name(const char *name);
> +
>  static TypeImpl *type_register_internal(const TypeInfo *info)
>  {
>      TypeImpl *ti;
>      ti = type_new(info);
>  
> +    if (modules_init_done[MODULE_INIT_QOM]) {
> +        if (ti->parent && !type_get_by_name(ti->parent)) {
> +            g_free(ti);

The function type_new() has some g_strdup() action. We would need a
type_delete() in order to clean those up properly if we are taking
this approach. In v3 I decided to check the info, and avoid instantiating
a ti so I don't have to clean it up.

> +            return NULL;
> +        }
> +    }
> +

This would change the postcondition of the type_register*() familiy
of functions. It effectively  makes type_register_*() a 'mayfail'
depending on a global state, which is
modules_init_done[MODULE_INIT_QOM].  It affects all modules. IMHO we should also update the documentation if we decide to move forward with this approach.

I will give this a spin later.

Please have a look at my v3 and let us decide how to move forward based
on that.

Regards,
Halil

[..]

Patch

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..5ac9b6b2a6 100644
--- a/hw/s390x/virtio-ccw-gpu.c
+++ b/hw/s390x/virtio-ccw-gpu.c
@@ -62,7 +62,11 @@  static const TypeInfo virtio_ccw_gpu = {
 
 static void virtio_ccw_gpu_register(void)
 {
+#ifdef CONFIG_MODULES
+    type_register_static_mayfail(&virtio_ccw_gpu);
+#else
     type_register_static(&virtio_ccw_gpu);
+#endif
 }
 
 type_init(virtio_ccw_gpu_register)
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..3428546d91 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -826,6 +826,17 @@  const char *object_get_typename(const Object *obj);
  */
 Type type_register_static(const TypeInfo *info);
 
+/**
+ * type_register_static_mayfail:
+ * @info: The #TypeInfo of the new type.
+ *
+ * @info and all of the strings it points to should exist for the life time
+ * that the type is registered.
+ *
+ * Returns: the new #Type or NULL if missing a parent type.
+ */
+Type type_register_static_mayfail(const TypeInfo *info);
+
 /**
  * type_register:
  * @info: The #TypeInfo of the new type
@@ -837,6 +848,17 @@  Type type_register_static(const TypeInfo *info);
  */
 Type type_register(const TypeInfo *info);
 
+/**
+ * type_register_mayfail:
+ * @info: The #TypeInfo of the new type
+ *
+ * Unlike type_register_static(), this call does not require @info or its
+ * string members to continue to exist after the call returns.
+ *
+ * Returns: the new #Type or NULL if missing a parent type.
+ */
+Type type_register_mayfail(const TypeInfo *info);
+
 /**
  * type_register_static_array:
  * @infos: The array of the new type #TypeInfo structures.
diff --git a/qom/object.c b/qom/object.c
index 491823db4a..ed217cbfb0 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -135,7 +135,7 @@  static TypeImpl *type_new(const TypeInfo *info)
     return ti;
 }
 
-static TypeImpl *type_register_internal(const TypeInfo *info)
+static TypeImpl *type_register_internal(const TypeInfo *info, bool mayfail)
 {
     TypeImpl *ti;
     ti = type_new(info);
@@ -147,7 +147,13 @@  static TypeImpl *type_register_internal(const TypeInfo *info)
 TypeImpl *type_register(const TypeInfo *info)
 {
     assert(info->parent);
-    return type_register_internal(info);
+    return type_register_internal(info, false);
+}
+
+TypeImpl *type_register_mayfail(const TypeInfo *info)
+{
+    assert(info->parent);
+    return type_register_internal(info, true);
 }
 
 TypeImpl *type_register_static(const TypeInfo *info)
@@ -155,6 +161,11 @@  TypeImpl *type_register_static(const TypeInfo *info)
     return type_register(info);
 }
 
+TypeImpl *type_register_static_mayfail(const TypeInfo *info)
+{
+    return type_register_mayfail(info);
+}
+
 void type_register_static_array(const TypeInfo *infos, int nr_infos)
 {
     int i;
@@ -173,13 +184,16 @@  static TypeImpl *type_get_by_name(const char *name)
     return type_table_lookup(name);
 }
 
-static TypeImpl *type_get_parent(TypeImpl *type)
+static TypeImpl *type_get_parent(TypeImpl *type, bool mayfail)
 {
     if (!type->parent_type && type->parent) {
         type->parent_type = type_get_by_name(type->parent);
         if (!type->parent_type) {
             fprintf(stderr, "Type '%s' is missing its parent '%s'\n",
                     type->name, type->parent);
+            if (mayfail) {
+                return NULL;
+            }
             abort();
         }
     }
@@ -199,7 +213,7 @@  static size_t type_class_get_size(TypeImpl *ti)
     }
 
     if (type_has_parent(ti)) {
-        return type_class_get_size(type_get_parent(ti));
+        return type_class_get_size(type_get_parent(ti, false));
     }
 
     return sizeof(ObjectClass);
@@ -212,7 +226,7 @@  static size_t type_object_get_size(TypeImpl *ti)
     }
 
     if (type_has_parent(ti)) {
-        return type_object_get_size(type_get_parent(ti));
+        return type_object_get_size(type_get_parent(ti, false));
     }
 
     return 0;
@@ -236,7 +250,7 @@  static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
             return true;
         }
 
-        type = type_get_parent(type);
+        type = type_get_parent(type, false);
     }
 
     return false;
@@ -307,7 +321,7 @@  static void type_initialize(TypeImpl *ti)
     }
     ti->class = g_malloc0(ti->class_size);
 
-    parent = type_get_parent(ti);
+    parent = type_get_parent(ti, false);
     if (parent) {
         type_initialize(parent);
         GSList *e;
@@ -357,7 +371,7 @@  static void type_initialize(TypeImpl *ti)
         if (parent->class_base_init) {
             parent->class_base_init(ti->class, ti->class_data);
         }
-        parent = type_get_parent(parent);
+        parent = type_get_parent(parent, false);
     }
 
     if (ti->class_init) {
@@ -368,7 +382,7 @@  static void type_initialize(TypeImpl *ti)
 static void object_init_with_type(Object *obj, TypeImpl *ti)
 {
     if (type_has_parent(ti)) {
-        object_init_with_type(obj, type_get_parent(ti));
+        object_init_with_type(obj, type_get_parent(ti, false));
     }
 
     if (ti->instance_init) {
@@ -383,7 +397,7 @@  static void object_post_init_with_type(Object *obj, TypeImpl *ti)
     }
 
     if (type_has_parent(ti)) {
-        object_post_init_with_type(obj, type_get_parent(ti));
+        object_post_init_with_type(obj, type_get_parent(ti, false));
     }
 }
 
@@ -674,7 +688,7 @@  static void object_deinit(Object *obj, TypeImpl *type)
     }
 
     if (type_has_parent(type)) {
-        object_deinit(obj, type_get_parent(type));
+        object_deinit(obj, type_get_parent(type, false));
     }
 }
 
@@ -1040,7 +1054,7 @@  ObjectClass *module_object_class_by_name(const char *typename)
 
 ObjectClass *object_class_get_parent(ObjectClass *class)
 {
-    TypeImpl *type = type_get_parent(class->type);
+    TypeImpl *type = type_get_parent(class->type, false);
 
     if (!type) {
         return NULL;
@@ -2791,8 +2805,8 @@  static void register_types(void)
         .abstract = true,
     };
 
-    type_interface = type_register_internal(&interface_info);
-    type_register_internal(&object_info);
+    type_interface = type_register_internal(&interface_info, false);
+    type_register_internal(&object_info, false);
 }
 
 type_init(register_types)
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"    },