Message ID | 20210302173544.3704179-1-pasic@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [v3,1/1] hw/s390x: modularize virtio-gpu-ccw | expand |
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
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" },
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