[PATCH 0/4] QOM: Singleton interface

[PATCH 0/4] QOM: Singleton interface

[Peter Xu]

This patchset introduces the singleton interface for QOM.

The singleton interface is as simple as "this class can only create one
instance".

We used to have similar demand when working on all kinds of vIOMMUs,
because in most cases that I am aware of, vIOMMU must be a singleton as
it's closely bound to the machine and also the root PCIe systems.  We used
to have a bunch of special code guarding those, for example, X86 has
pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
than once.

There is a similar demand raising recently (even if the problem existed
over years) when we were looking at a migration bug, that part of it was
involved with the current_migration global pointer being referenced
anywhere, even after the migration object was finalize()ed.  So far without
singleton support, there's no way to reset the variable properly.
Declaring migration object to be a singleton also just makes sense, e.g.,
dynamically creating migration objects on the fly with QMP commands doesn't
sound right..

The idea behind is pretty simple: any object that can only be created once
can now declare the TYPE_SINGLETON interface, then QOM facilities will make
sure it won't be created more than once.  For example, qom-list-properties,
device-list-properties, etc., will be smart enough to not try to create
temporary singleton objects now.  Meanwhile, we also guard device-add paths
so that it'll fail properly if it's created more than once (and iff it's a
TYPE_DEVICE first).

The 1st patch introduces the QOM interface, while I made both x86-iommu and
migration as the initial users of it, so it's really more about the 1st
patch to discuss first.  I didn't yet touch ARM/SMMU as it's also more
complicated; I also didn't yet dig anything else that may apply, but I do
feel like this can apply more than the two attached here.  Hopefully the
two use cases can be good enough to start the discussion.

Thanks,

Peter Xu (4):
  qom: TYPE_SINGLETON interface
  x86/iommu: Make x86-iommu a singleton object
  migration: Make migration object a singleton object
  migration: Reset current_migration properly

 include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++
 hw/i386/x86-iommu.c             | 14 ++++++++++
 migration/migration.c           | 36 ++++++++++++++++++++++---
 qom/object.c                    |  3 +++
 qom/object_interfaces.c         | 24 +++++++++++++++++
 qom/qom-qmp-cmds.c              | 22 ++++++++++++---
 system/qdev-monitor.c           |  7 +++++
 7 files changed, 147 insertions(+), 6 deletions(-)

-- 
2.45.0

[PATCH 1/4] qom: TYPE_SINGLETON interface

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++
 qom/object.c                    |  3 +++
 qom/object_interfaces.c         | 24 +++++++++++++++++
 qom/qom-qmp-cmds.c              | 22 ++++++++++++---
 system/qdev-monitor.c           |  7 +++++
 5 files changed, 100 insertions(+), 3 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 02b11a7ef0..9b2cc0e554 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -177,4 +177,51 @@ bool user_creatable_del(const char *id, Error **errp);
  */
 void user_creatable_cleanup(void);
 
+#define TYPE_SINGLETON "singleton"
+
+typedef struct SingletonClass SingletonClass;
+DECLARE_CLASS_CHECKERS(SingletonClass, SINGLETON, TYPE_SINGLETON)
+
+/**
+ * SingletonClass:
+ *
+ * @parent_class: the base class
+ * @get_instance: fetch the singleton instance if it is created,
+ *                NULL otherwise.
+ *
+ * Singleton class describes the type of object classes that can only
+ * provide one instance for the whole lifecycle of QEMU.  It will fail the
+ * operation if one attemps to create more than one instance.
+ *
+ * One can fetch the single object using class's get_instance() callback if
+ * it was created before.  This can be useful for operations like QMP
+ * qom-list-properties, where dynamically creating an object might not be
+ * feasible.
+ */
+struct SingletonClass {
+    /* <private> */
+    InterfaceClass parent_class;
+    /* <public> */
+    Object *(*get_instance)(Error **errp);
+};
+
+/**
+ * object_class_is_singleton:
+ *
+ * @class: the class to detect singleton
+ *
+ * Returns: true if it's a singleton class, false otherwise.
+ */
+bool object_class_is_singleton(ObjectClass *class);
+
+/**
+ * singleton_get_instance:
+ *
+ * @class: the class to fetch singleton instance
+ *
+ * Returns: the object* if the class is a singleton class and the singleton
+ *          object is created, NULL otherwise.
+ */
+Object *singleton_get_instance(ObjectClass *class);
+
 #endif
diff --git a/qom/object.c b/qom/object.c
index 11424cf471..ded299ae1a 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -553,6 +553,9 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type
     g_assert(type->abstract == false);
     g_assert(size >= type->instance_size);
 
+    /* Singleton class can only create one object */
+    g_assert(!singleton_get_instance(type->class));
+
     memset(obj, 0, type->instance_size);
     obj->class = type->class;
     object_ref(obj);
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index e0833c8bfe..6766060d0a 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -354,6 +354,23 @@ void user_creatable_cleanup(void)
     object_unparent(object_get_objects_root());
 }
 
+bool object_class_is_singleton(ObjectClass *class)
+{
+    return !!object_class_dynamic_cast(class, TYPE_SINGLETON);
+}
+
+Object *singleton_get_instance(ObjectClass *class)
+{
+    SingletonClass *singleton =
+        (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON);
+
+    if (!singleton) {
+        return NULL;
+    }
+
+    return singleton->get_instance(&error_abort);
+}
+
 static void register_types(void)
 {
     static const TypeInfo uc_interface_info = {
@@ -362,7 +379,14 @@ static void register_types(void)
         .class_size = sizeof(UserCreatableClass),
     };
 
+    static const TypeInfo singleton_interface_info = {
+        .name          = TYPE_SINGLETON,
+        .parent        = TYPE_INTERFACE,
+        .class_size = sizeof(SingletonClass),
+    };
+
     type_register_static(&uc_interface_info);
+    type_register_static(&singleton_interface_info);
 }
 
 type_init(register_types)
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index e91a235347..ecc1cf781c 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
     ObjectProperty *prop;
     ObjectPropertyIterator iter;
     ObjectPropertyInfoList *prop_list = NULL;
+    bool create;
 
     klass = module_object_class_by_name(typename);
     if (klass == NULL) {
@@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
         return NULL;
     }
 
-    obj = object_new(typename);
+    /* Avoid creating multiple instances if the class is a singleton */
+    create = !object_class_is_singleton(klass) ||
+        !singleton_get_instance(klass);
+
+    if (create) {
+        obj = object_new(typename);
+    } else {
+        obj = singleton_get_instance(klass);
+    }
 
     object_property_iter_init(&iter, obj);
     while ((prop = object_property_iter_next(&iter))) {
@@ -172,7 +181,9 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
         QAPI_LIST_PREPEND(prop_list, info);
     }
 
-    object_unref(obj);
+    if (create) {
+        object_unref(obj);
+    }
 
     return prop_list;
 }
@@ -199,7 +210,12 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
         return NULL;
     }
 
-    if (object_class_is_abstract(klass)) {
+    /*
+     * Abstract classes are not for instantiations, meanwhile avoid
+     * creating temporary singleton objects because it can cause conflicts
+     * if there's already one created.
+     */
+    if (object_class_is_abstract(klass) || object_class_is_singleton(klass)) {
         object_class_property_iter_init(&iter, klass);
     } else {
         obj = object_new(typename);
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 44994ea0e1..1310f35c9f 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -36,6 +36,7 @@
 #include "qemu/option.h"
 #include "qemu/qemu-print.h"
 #include "qemu/option_int.h"
+#include "qom/object_interfaces.h"
 #include "sysemu/block-backend.h"
 #include "migration/misc.h"
 #include "qemu/cutils.h"
@@ -643,6 +644,12 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
         return NULL;
     }
 
+    if (singleton_get_instance(OBJECT_CLASS(dc))) {
+        error_setg(errp, "Class '%s' only supports one instance",
+                   driver);
+        return NULL;
+    }
+
     /* find bus */
     path = qdict_get_try_str(opts, "bus");
     if (path != NULL) {
-- 
2.45.0

[PATCH 2/4] x86/iommu: Make x86-iommu a singleton object

[Peter Xu]

X86 IOMMUs cannot be created more than one on a system yet.  Make it a
singleton so it guards the system from accidentally create yet another
IOMMU object when one already presents.

Now if someone tries to create more than one, e.g., via:

  ./qemu -M q35 -device intel-iommu -device intel-iommu

The error will change from:

  qemu-system-x86_64: -device intel-iommu: QEMU does not support multiple vIOMMUs for x86 yet.

To:

  qemu-system-x86_64: -device intel-iommu: Class 'intel-iommu' only supports one instance

Unfortunately, yet we can't remove the singleton check in the machine
hook (pc_machine_device_pre_plug_cb), because there can also be
virtio-iommu involved, which doesn't share a common parent class yet.

But with this, it should be closer to reach that goal to check singleton by
QOM one day.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/x86-iommu.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index 60af896225..4bfeb08705 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -26,6 +26,7 @@
 #include "qemu/error-report.h"
 #include "trace.h"
 #include "sysemu/kvm.h"
+#include "qom/object_interfaces.h"
 
 void x86_iommu_iec_register_notifier(X86IOMMUState *iommu,
                                      iec_notify_fn fn, void *data)
@@ -133,10 +134,19 @@ static Property x86_iommu_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static Object *x86_iommu_get_instance(Error **errp)
+{
+    return OBJECT(x86_iommu_get_default());
+}
+
 static void x86_iommu_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    SingletonClass *singleton = SINGLETON_CLASS(klass);
+
     dc->realize = x86_iommu_realize;
+    singleton->get_instance = x86_iommu_get_instance;
+
     device_class_set_props(dc, x86_iommu_properties);
 }
 
@@ -152,6 +162,10 @@ static const TypeInfo x86_iommu_info = {
     .class_init    = x86_iommu_class_init,
     .class_size    = sizeof(X86IOMMUClass),
     .abstract      = true,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_SINGLETON },
+        { }
+    }
 };
 
 static void x86_iommu_register_types(void)
-- 
2.45.0

[PATCH 3/4] migration: Make migration object a singleton object

[Peter Xu]

This makes the migration object a singleton unit.  After this, we can do
something slightly tricky later on with the guarantee that nobody will be
able to create the object twice.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index bcb735869b..1b5285af95 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -45,6 +45,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qnull.h"
 #include "qemu/rcu.h"
+#include "qom/object_interfaces.h"
 #include "postcopy-ram.h"
 #include "qemu/thread.h"
 #include "trace.h"
@@ -3855,11 +3856,19 @@ fail:
     migrate_fd_cleanup(s);
 }
 
+static Object* migration_get_instance(Error **errp)
+{
+    return OBJECT(current_migration);
+}
+
 static void migration_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    SingletonClass *singleton = SINGLETON_CLASS(klass);
 
     dc->user_creatable = false;
+    singleton->get_instance = migration_get_instance;
+
     device_class_set_props(dc, migration_properties);
 }
 
@@ -3932,6 +3941,10 @@ static const TypeInfo migration_type = {
     .instance_size = sizeof(MigrationState),
     .instance_init = migration_instance_init,
     .instance_finalize = migration_instance_finalize,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_SINGLETON },
+        { }
+    }
 };
 
 static void register_migration_types(void)
-- 
2.45.0

[PATCH 4/4] migration: Reset current_migration properly

[Peter Xu]

current_migration is never reset, even if the migration object is freed
already.  It means anyone references that can trigger UAF and it'll be hard
to debug.

Properly clear the pointer now, so far the only way to do is via
finalize() as we know there's only one instance of it, meanwhile QEMU won't
know who holds the refcount, so it can't reset the variable manually but
only in finalize().

To make it more readable, also initialize the variable in the
instance_init() so it's very well paired at least.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 1b5285af95..74812ca785 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -233,9 +233,11 @@ static int migration_stop_vm(MigrationState *s, RunState state)
 
 void migration_object_init(void)
 {
-    /* This can only be called once. */
-    assert(!current_migration);
-    current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
+    /* This creates the singleton migration object */
+    object_new(TYPE_MIGRATION);
+
+    /* This should be set now when initialize the singleton object */
+    assert(current_migration);
 
     /*
      * Init the migrate incoming object as well no matter whether
@@ -3886,12 +3888,27 @@ static void migration_instance_finalize(Object *obj)
     qemu_sem_destroy(&ms->rp_state.rp_pong_acks);
     qemu_sem_destroy(&ms->postcopy_qemufile_src_sem);
     error_free(ms->error);
+
+    /*
+     * We know we only have one intance of migration, and when reaching
+     * here it means migration object is gone.  Clear the global reference
+     * to reflect that.
+     */
+    current_migration = NULL;
 }
 
 static void migration_instance_init(Object *obj)
 {
     MigrationState *ms = MIGRATION_OBJ(obj);
 
+    /*
+     * There can only be one migration object globally. Keep a record of
+     * the pointer in current_migration, which will be reset after the
+     * object finalize().
+     */
+    assert(!current_migration);
+    current_migration = ms;
+
     ms->state = MIGRATION_STATUS_NONE;
     ms->mbps = -1;
     ms->pages_per_second = -1;
-- 
2.45.0

Re: [PATCH 3/4] migration: Make migration object a singleton object

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> This makes the migration object a singleton unit.  After this, we can do
> something slightly tricky later on with the guarantee that nobody will be
> able to create the object twice.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index bcb735869b..1b5285af95 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -45,6 +45,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/qnull.h"
>  #include "qemu/rcu.h"
> +#include "qom/object_interfaces.h"
>  #include "postcopy-ram.h"
>  #include "qemu/thread.h"
>  #include "trace.h"
> @@ -3855,11 +3856,19 @@ fail:
>      migrate_fd_cleanup(s);
>  }
>  
> +static Object* migration_get_instance(Error **errp)

static Object *migration_get_instance(Error **errp)
              ^

> +{
> +    return OBJECT(current_migration);
> +}
> +
>  static void migration_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    SingletonClass *singleton = SINGLETON_CLASS(klass);
>  
>      dc->user_creatable = false;
> +    singleton->get_instance = migration_get_instance;
> +
>      device_class_set_props(dc, migration_properties);
>  }
>  
> @@ -3932,6 +3941,10 @@ static const TypeInfo migration_type = {
>      .instance_size = sizeof(MigrationState),
>      .instance_init = migration_instance_init,
>      .instance_finalize = migration_instance_finalize,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_SINGLETON },
> +        { }
> +    }
>  };
>  
>  static void register_migration_types(void)

Re: [PATCH 4/4] migration: Reset current_migration properly

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> current_migration is never reset, even if the migration object is freed
> already.  It means anyone references that can trigger UAF and it'll be hard
> to debug.
>
> Properly clear the pointer now, so far the only way to do is via
> finalize() as we know there's only one instance of it, meanwhile QEMU won't
> know who holds the refcount, so it can't reset the variable manually but
> only in finalize().
>
> To make it more readable, also initialize the variable in the
> instance_init() so it's very well paired at least.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 1b5285af95..74812ca785 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -233,9 +233,11 @@ static int migration_stop_vm(MigrationState *s, RunState state)
>  
>  void migration_object_init(void)
>  {
> -    /* This can only be called once. */
> -    assert(!current_migration);
> -    current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
> +    /* This creates the singleton migration object */
> +    object_new(TYPE_MIGRATION);
> +
> +    /* This should be set now when initialize the singleton object */
> +    assert(current_migration);
>  
>      /*
>       * Init the migrate incoming object as well no matter whether
> @@ -3886,12 +3888,27 @@ static void migration_instance_finalize(Object *obj)
>      qemu_sem_destroy(&ms->rp_state.rp_pong_acks);
>      qemu_sem_destroy(&ms->postcopy_qemufile_src_sem);
>      error_free(ms->error);
> +
> +    /*
> +     * We know we only have one intance of migration, and when reaching

instance

> +     * here it means migration object is gone.  Clear the global reference
> +     * to reflect that.

Not really gone at this point. The free only happens when this function
returns.

> +     */
> +    current_migration = NULL;
>  }
>  
>  static void migration_instance_init(Object *obj)
>  {
>      MigrationState *ms = MIGRATION_OBJ(obj);
>  
> +    /*
> +     * There can only be one migration object globally. Keep a record of
> +     * the pointer in current_migration, which will be reset after the
> +     * object finalize().
> +     */
> +    assert(!current_migration);
> +    current_migration = ms;
> +
>      ms->state = MIGRATION_STATUS_NONE;
>      ms->mbps = -1;
>      ms->pages_per_second = -1;

Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Philippe Mathieu-Daudé]

Hi Peter,

(Cc'ing Mark)

On 24/10/24 13:56, Peter Xu wrote:
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++
>   qom/object.c                    |  3 +++
>   qom/object_interfaces.c         | 24 +++++++++++++++++
>   qom/qom-qmp-cmds.c              | 22 ++++++++++++---
>   system/qdev-monitor.c           |  7 +++++
>   5 files changed, 100 insertions(+), 3 deletions(-)


> +/**
> + * SingletonClass:
> + *
> + * @parent_class: the base class
> + * @get_instance: fetch the singleton instance if it is created,
> + *                NULL otherwise.
> + *
> + * Singleton class describes the type of object classes that can only
> + * provide one instance for the whole lifecycle of QEMU.  It will fail the
> + * operation if one attemps to create more than one instance.
> + *
> + * One can fetch the single object using class's get_instance() callback if
> + * it was created before.  This can be useful for operations like QMP
> + * qom-list-properties, where dynamically creating an object might not be
> + * feasible.
> + */
> +struct SingletonClass {
> +    /* <private> */
> +    InterfaceClass parent_class;
> +    /* <public> */
> +    Object *(*get_instance)(Error **errp);

IMHO asking get_instance() is overkill ...

> +};
> +
> +/**
> + * object_class_is_singleton:
> + *
> + * @class: the class to detect singleton
> + *
> + * Returns: true if it's a singleton class, false otherwise.
> + */
> +bool object_class_is_singleton(ObjectClass *class);
> +
> +/**
> + * singleton_get_instance:
> + *
> + * @class: the class to fetch singleton instance
> + *
> + * Returns: the object* if the class is a singleton class and the singleton
> + *          object is created, NULL otherwise.
> + */
> +Object *singleton_get_instance(ObjectClass *class);
> +
>   #endif

> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index e0833c8bfe..6766060d0a 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -354,6 +354,23 @@ void user_creatable_cleanup(void)
>       object_unparent(object_get_objects_root());
>   }
>   
> +bool object_class_is_singleton(ObjectClass *class)
> +{
> +    return !!object_class_dynamic_cast(class, TYPE_SINGLETON);
> +}
> +
> +Object *singleton_get_instance(ObjectClass *class)
> +{

... when we can use object_resolve_type_unambiguous:

       return 
object_resolve_type_unambiguous(object_class_get_name(class, NULL));

BTW should we pass Error** argument to singleton_get_instance()?

> +    SingletonClass *singleton =
> +        (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON);
> +
> +    if (!singleton) {
> +        return NULL;
> +    }
> +
> +    return singleton->get_instance(&error_abort);
> +}

Alternatively call object_resolve_type_unambiguous() in instance_init()?

Regards,

Phil.

Re: [PATCH 4/4] migration: Reset current_migration properly

[Peter Xu]

On Thu, Oct 24, 2024 at 04:34:44PM -0300, Fabiano Rosas wrote:
> > +     * here it means migration object is gone.  Clear the global reference
> > +     * to reflect that.
> 
> Not really gone at this point. The free only happens when this function
> returns.

How about "is going away"?

-- 
Peter Xu

Re: [PATCH 4/4] migration: Reset current_migration properly

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Oct 24, 2024 at 04:34:44PM -0300, Fabiano Rosas wrote:
>> > +     * here it means migration object is gone.  Clear the global reference
>> > +     * to reflect that.
>> 
>> Not really gone at this point. The free only happens when this function
>> returns.
>
> How about "is going away"?

Yep

Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Peter Xu]

On Thu, Oct 24, 2024 at 05:02:19PM -0300, Philippe Mathieu-Daudé wrote:
> Hi Peter,

Hi, Phil,

Thanks for the quick reviews!

> 
> (Cc'ing Mark)
> 
> On 24/10/24 13:56, Peter Xu wrote:
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++
> >   qom/object.c                    |  3 +++
> >   qom/object_interfaces.c         | 24 +++++++++++++++++
> >   qom/qom-qmp-cmds.c              | 22 ++++++++++++---
> >   system/qdev-monitor.c           |  7 +++++
> >   5 files changed, 100 insertions(+), 3 deletions(-)
> 
> 
> > +/**
> > + * SingletonClass:
> > + *
> > + * @parent_class: the base class
> > + * @get_instance: fetch the singleton instance if it is created,
> > + *                NULL otherwise.
> > + *
> > + * Singleton class describes the type of object classes that can only
> > + * provide one instance for the whole lifecycle of QEMU.  It will fail the
> > + * operation if one attemps to create more than one instance.
> > + *
> > + * One can fetch the single object using class's get_instance() callback if
> > + * it was created before.  This can be useful for operations like QMP
> > + * qom-list-properties, where dynamically creating an object might not be
> > + * feasible.
> > + */
> > +struct SingletonClass {
> > +    /* <private> */
> > +    InterfaceClass parent_class;
> > +    /* <public> */
> > +    Object *(*get_instance)(Error **errp);
> 
> IMHO asking get_instance() is overkill ...
> 
> > +};
> > +
> > +/**
> > + * object_class_is_singleton:
> > + *
> > + * @class: the class to detect singleton
> > + *
> > + * Returns: true if it's a singleton class, false otherwise.
> > + */
> > +bool object_class_is_singleton(ObjectClass *class);
> > +
> > +/**
> > + * singleton_get_instance:
> > + *
> > + * @class: the class to fetch singleton instance
> > + *
> > + * Returns: the object* if the class is a singleton class and the singleton
> > + *          object is created, NULL otherwise.
> > + */
> > +Object *singleton_get_instance(ObjectClass *class);
> > +
> >   #endif
> 
> > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > index e0833c8bfe..6766060d0a 100644
> > --- a/qom/object_interfaces.c
> > +++ b/qom/object_interfaces.c
> > @@ -354,6 +354,23 @@ void user_creatable_cleanup(void)
> >       object_unparent(object_get_objects_root());
> >   }
> > +bool object_class_is_singleton(ObjectClass *class)
> > +{
> > +    return !!object_class_dynamic_cast(class, TYPE_SINGLETON);
> > +}
> > +
> > +Object *singleton_get_instance(ObjectClass *class)
> > +{
> 
> ... when we can use object_resolve_type_unambiguous:
> 
>       return object_resolve_type_unambiguous(object_class_get_name(class,
> NULL));

I think an issue is migration object is nowhere to be find under
object_get_root(), so it won't work there.  A side benefit is, it's also
faster..

How about I use object_resolve_type_unambiguous() as a fallback?  Then it's
used only if get_instance() is not provided.

> 
> BTW should we pass Error** argument to singleton_get_instance()?

I didn't expect it to fail, hence I didn't even add it to make it more like
"this will always success or it asserts" kind of things.  I left Error** in
the hook just to be slightly flexible, but I always pass in error_abort()
in this patch.

I can either add Error** if anyone thinks it useful, or even drop Error**
in ->get_instance() since it's mostly not used at least for now.

Let me know!

> 
> > +    SingletonClass *singleton =
> > +        (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON);
> > +
> > +    if (!singleton) {
> > +        return NULL;
> > +    }
> > +
> > +    return singleton->get_instance(&error_abort);
> > +}
> 
> Alternatively call object_resolve_type_unambiguous() in instance_init()?

Thanks,

-- 
Peter Xu

Re: [PATCH 0/4] QOM: Singleton interface

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> This patchset introduces the singleton interface for QOM.
>
> The singleton interface is as simple as "this class can only create one
> instance".
>
> We used to have similar demand when working on all kinds of vIOMMUs,
> because in most cases that I am aware of, vIOMMU must be a singleton as
> it's closely bound to the machine and also the root PCIe systems.  We used
> to have a bunch of special code guarding those, for example, X86 has
> pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
> than once.
>
> There is a similar demand raising recently (even if the problem existed
> over years) when we were looking at a migration bug, that part of it was
> involved with the current_migration global pointer being referenced
> anywhere, even after the migration object was finalize()ed.  So far without
> singleton support, there's no way to reset the variable properly.
> Declaring migration object to be a singleton also just makes sense, e.g.,
> dynamically creating migration objects on the fly with QMP commands doesn't
> sound right..
>
> The idea behind is pretty simple: any object that can only be created once
> can now declare the TYPE_SINGLETON interface, then QOM facilities will make
> sure it won't be created more than once.  For example, qom-list-properties,
> device-list-properties, etc., will be smart enough to not try to create
> temporary singleton objects now.

QOM design assumption: object_new() followed by object_unref() is always
possible and has no side effect.

QOM introspection relies on this.  Your PATCH 1 makes non-class
properties of singletons invisible in introspection.  Making something
with such properties a singleton would be a regression.

Anything that violates the design assumption must be delayed to a
suitable state transition.  For devices (subtypes of TYPE_DEVICE), this
is ->realize().  For user-creatable objects (provide interface
TYPE_USER_CREATABLE), this is ->complete().  For anything else, it's
something else that probably doesn't even exist, yet.  See "Problem 5:
QOM lacks a clear life cycle" in

    Subject: Dynamic & heterogeneous machines, initial configuration: problems
    Date: Wed, 31 Jan 2024 21:14:21 +0100
    Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
    https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/

>                                   Meanwhile, we also guard device-add paths
> so that it'll fail properly if it's created more than once (and iff it's a
> TYPE_DEVICE first).

[...]

Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++
>  qom/object.c                    |  3 +++
>  qom/object_interfaces.c         | 24 +++++++++++++++++
>  qom/qom-qmp-cmds.c              | 22 ++++++++++++---
>  system/qdev-monitor.c           |  7 +++++
>  5 files changed, 100 insertions(+), 3 deletions(-)
>
> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> index 02b11a7ef0..9b2cc0e554 100644
> --- a/include/qom/object_interfaces.h
> +++ b/include/qom/object_interfaces.h
> @@ -177,4 +177,51 @@ bool user_creatable_del(const char *id, Error **errp);
>   */
>  void user_creatable_cleanup(void);
>  
> +#define TYPE_SINGLETON "singleton"
> +
> +typedef struct SingletonClass SingletonClass;
> +DECLARE_CLASS_CHECKERS(SingletonClass, SINGLETON, TYPE_SINGLETON)
> +
> +/**
> + * SingletonClass:
> + *
> + * @parent_class: the base class
> + * @get_instance: fetch the singleton instance if it is created,
> + *                NULL otherwise.
> + *
> + * Singleton class describes the type of object classes that can only
> + * provide one instance for the whole lifecycle of QEMU.  It will fail the
> + * operation if one attemps to create more than one instance.
> + *
> + * One can fetch the single object using class's get_instance() callback if
> + * it was created before.  This can be useful for operations like QMP
> + * qom-list-properties, where dynamically creating an object might not be
> + * feasible.
> + */
> +struct SingletonClass {
> +    /* <private> */
> +    InterfaceClass parent_class;
> +    /* <public> */
> +    Object *(*get_instance)(Error **errp);
> +};
> +
> +/**
> + * object_class_is_singleton:
> + *
> + * @class: the class to detect singleton
> + *
> + * Returns: true if it's a singleton class, false otherwise.
> + */
> +bool object_class_is_singleton(ObjectClass *class);
> +
> +/**
> + * singleton_get_instance:
> + *
> + * @class: the class to fetch singleton instance
> + *
> + * Returns: the object* if the class is a singleton class and the singleton
> + *          object is created, NULL otherwise.
> + */
> +Object *singleton_get_instance(ObjectClass *class);

A non-null return value can become dangling when the object gets
destroyed.  This could conceivably happen in another thread.

The obviously safe interface would take a reference the caller must
unref when done.

> +
>  #endif
> diff --git a/qom/object.c b/qom/object.c
> index 11424cf471..ded299ae1a 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -553,6 +553,9 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type
>      g_assert(type->abstract == false);
>      g_assert(size >= type->instance_size);
>  
> +    /* Singleton class can only create one object */
> +    g_assert(!singleton_get_instance(type->class));
> +
>      memset(obj, 0, type->instance_size);
>      obj->class = type->class;
>      object_ref(obj);
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index e0833c8bfe..6766060d0a 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -354,6 +354,23 @@ void user_creatable_cleanup(void)
>      object_unparent(object_get_objects_root());
>  }
>  
> +bool object_class_is_singleton(ObjectClass *class)
> +{
> +    return !!object_class_dynamic_cast(class, TYPE_SINGLETON);
> +}
> +
> +Object *singleton_get_instance(ObjectClass *class)
> +{
> +    SingletonClass *singleton =
> +        (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON);
> +
> +    if (!singleton) {
> +        return NULL;
> +    }
> +
> +    return singleton->get_instance(&error_abort);
> +}
> +
>  static void register_types(void)
>  {
>      static const TypeInfo uc_interface_info = {
> @@ -362,7 +379,14 @@ static void register_types(void)
>          .class_size = sizeof(UserCreatableClass),
>      };
>  
> +    static const TypeInfo singleton_interface_info = {
> +        .name          = TYPE_SINGLETON,
> +        .parent        = TYPE_INTERFACE,
> +        .class_size = sizeof(SingletonClass),
> +    };
> +
>      type_register_static(&uc_interface_info);
> +    type_register_static(&singleton_interface_info);
>  }
>  
>  type_init(register_types)
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index e91a235347..ecc1cf781c 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
>      ObjectProperty *prop;
>      ObjectPropertyIterator iter;
>      ObjectPropertyInfoList *prop_list = NULL;
> +    bool create;
>  
>      klass = module_object_class_by_name(typename);
>      if (klass == NULL) {
> @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
>          return NULL;
>      }
>  
> -    obj = object_new(typename);
> +    /* Avoid creating multiple instances if the class is a singleton */
> +    create = !object_class_is_singleton(klass) ||
> +        !singleton_get_instance(klass);
> +
> +    if (create) {
> +        obj = object_new(typename);

If the class is not a singleton or else if no instance exists, we create
a temporary instance.

> +    } else {
> +        obj = singleton_get_instance(klass);

If the class is a singleton and the instance exists, we use that
instead.

Any properties the instance has created dynamically after object_new()
are visible to introspection.  This is not the case when we create a
temporary instance.  Such subtle differences are problematic.  If we
decide we're okay with this one, we need to document it.

> +    }
>  
>      object_property_iter_init(&iter, obj);
>      while ((prop = object_property_iter_next(&iter))) {
> @@ -172,7 +181,9 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
>          QAPI_LIST_PREPEND(prop_list, info);
>      }
>  
> -    object_unref(obj);
> +    if (create) {
> +        object_unref(obj);
> +    }
>  
>      return prop_list;
>  }
> @@ -199,7 +210,12 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
>          return NULL;
>      }
>  
> -    if (object_class_is_abstract(klass)) {
> +    /*
> +     * Abstract classes are not for instantiations, meanwhile avoid
> +     * creating temporary singleton objects because it can cause conflicts
> +     * if there's already one created.
> +     */
> +    if (object_class_is_abstract(klass) || object_class_is_singleton(klass)) {
>          object_class_property_iter_init(&iter, klass);
>      } else {
>          obj = object_new(typename);

If the class is a singleton, we treat it as if it was abstract.  Its
instance properties, if any, are not visible in qom-list-properties.
This is a defect.  If you make an existing class with instance
properties a singleton, the defect is a regression.

> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 44994ea0e1..1310f35c9f 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -36,6 +36,7 @@
>  #include "qemu/option.h"
>  #include "qemu/qemu-print.h"
>  #include "qemu/option_int.h"
> +#include "qom/object_interfaces.h"
>  #include "sysemu/block-backend.h"
>  #include "migration/misc.h"
>  #include "qemu/cutils.h"
> @@ -643,6 +644,12 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>          return NULL;
>      }
>  
> +    if (singleton_get_instance(OBJECT_CLASS(dc))) {
> +        error_setg(errp, "Class '%s' only supports one instance",
> +                   driver);
> +        return NULL;
> +    }
> +
>      /* find bus */
>      path = qdict_get_try_str(opts, "bus");
>      if (path != NULL) {

Re: [PATCH 2/4] x86/iommu: Make x86-iommu a singleton object

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> X86 IOMMUs cannot be created more than one on a system yet.  Make it a
> singleton so it guards the system from accidentally create yet another
> IOMMU object when one already presents.
>
> Now if someone tries to create more than one, e.g., via:
>
>   ./qemu -M q35 -device intel-iommu -device intel-iommu
>
> The error will change from:
>
>   qemu-system-x86_64: -device intel-iommu: QEMU does not support multiple vIOMMUs for x86 yet.
>
> To:
>
>   qemu-system-x86_64: -device intel-iommu: Class 'intel-iommu' only supports one instance
>
> Unfortunately, yet we can't remove the singleton check in the machine
> hook (pc_machine_device_pre_plug_cb), because there can also be
> virtio-iommu involved, which doesn't share a common parent class yet.
>
> But with this, it should be closer to reach that goal to check singleton by
> QOM one day.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

$ qemu-system-x86_64 -device amd-iommu,help
/work/armbru/qemu/include/hw/boards.h:24:MACHINE: Object 0x56473906f960 is not an instance of type machine
Aborted (core dumped)

(gdb) bt
#0  0x00007ffff4e43834 in __pthread_kill_implementation () at /lib64/libc.so.6
#1  0x00007ffff4df18ee in raise () at /lib64/libc.so.6
#2  0x00007ffff4dd98ff in abort () at /lib64/libc.so.6
#3  0x0000555555f75ef3 in object_dynamic_cast_assert
    (obj=0x555557e03960, typename=0x5555563c403e "machine", file=0x5555563c4018 "/work/armbru/qemu/include/hw/boards.h", line=24, func=0x5555563c4290 <__func__.7> "MACHINE") at ../qom/object.c:936
#4  0x0000555555d5db0f in MACHINE (obj=0x555557e03960)
    at /work/armbru/qemu/include/hw/boards.h:24
#5  0x0000555555d5e030 in x86_iommu_get_default () at ../hw/i386/x86-iommu.c:83
#6  0x0000555555d5e262 in x86_iommu_get_instance
    (errp=0x5555573d4918 <error_abort>) at ../hw/i386/x86-iommu.c:139
#7  0x0000555555f7c27c in singleton_get_instance (class=0x555557e00320)
    at ../qom/object_interfaces.c:371
#8  0x000055555612a842 in qmp_device_list_properties
    (typename=0x555557e001d0 "amd-iommu", errp=0x7fffffffda38)
    at ../qom/qom-qmp-cmds.c:147
#9  0x0000555555bf20b2 in qdev_device_help (opts=0x555557e001f0)
    at ../system/qdev-monitor.c:314
#10 0x0000555555bfe06d in device_help_func
    (opaque=0x0, opts=0x555557e001f0, errp=0x0) at ../system/vl.c:1208
#11 0x0000555556217186 in qemu_opts_foreach
    (list=0x55555729e5c0 <qemu_device_opts>, func=0x555555bfe04d <device_help_func>, opaque=0x0, errp=0x0) at ../util/qemu-option.c:1135
#12 0x0000555555c01d56 in qemu_process_help_options () at ../system/vl.c:2555
#13 0x0000555555c04d81 in qemu_init (argc=3, argv=0x7fffffffde28)
    at ../system/vl.c:3654
#14 0x000055555612ffae in main (argc=3, argv=0x7fffffffde28)
    at ../system/main.c:47

Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Daniel P. Berrangé]

On Thu, Oct 24, 2024 at 12:56:24PM -0400, Peter Xu wrote:

Adding significant new functionality to QOM should really come
with a commit message explaining the rationale and the design
choices

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++
>  qom/object.c                    |  3 +++
>  qom/object_interfaces.c         | 24 +++++++++++++++++
>  qom/qom-qmp-cmds.c              | 22 ++++++++++++---
>  system/qdev-monitor.c           |  7 +++++
>  5 files changed, 100 insertions(+), 3 deletions(-)

snip

> + * Singleton class describes the type of object classes that can only
> + * provide one instance for the whole lifecycle of QEMU.  It will fail the
> + * operation if one attemps to create more than one instance.
> + *
> + * One can fetch the single object using class's get_instance() callback if
> + * it was created before.  This can be useful for operations like QMP
> + * qom-list-properties, where dynamically creating an object might not be
> + * feasible.

snip

> +/**
> + * singleton_get_instance:
> + *
> + * @class: the class to fetch singleton instance
> + *
> + * Returns: the object* if the class is a singleton class and the singleton
> + *          object is created, NULL otherwise.
> + */
> +Object *singleton_get_instance(ObjectClass *class);

With this design, all code that uses a given type needs to know
whether or not it is intended to be a singleton. If some code
somewhere mistakenly calls 'object_new' instead of 'singleton_get_instance',
the singleton type  is no longer a singleton, except you handle this by
adding an assert in object_initialize_with_type.

This is still a bit of a loaded foot-gun IMHO, as we don't want random
code asserting.

> diff --git a/qom/object.c b/qom/object.c
> index 11424cf471..ded299ae1a 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -553,6 +553,9 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type
>      g_assert(type->abstract == false);
>      g_assert(size >= type->instance_size);
>  
> +    /* Singleton class can only create one object */
> +    g_assert(!singleton_get_instance(type->class));
> +
>      memset(obj, 0, type->instance_size);
>      obj->class = type->class;
>      object_ref(obj);

> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index e91a235347..ecc1cf781c 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
>      ObjectProperty *prop;
>      ObjectPropertyIterator iter;
>      ObjectPropertyInfoList *prop_list = NULL;
> +    bool create;
>  
>      klass = module_object_class_by_name(typename);
>      if (klass == NULL) {
> @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
>          return NULL;
>      }
>  
> -    obj = object_new(typename);
> +    /* Avoid creating multiple instances if the class is a singleton */
> +    create = !object_class_is_singleton(klass) ||
> +        !singleton_get_instance(klass);
> +
> +    if (create) {
> +        obj = object_new(typename);
> +    } else {
> +        obj = singleton_get_instance(klass);
> +    }

This is the first foot-gun example.

I really strongly dislike that the design pattern forces us to
create code like this, as we can never be confident we've
correctly identified all the places which may call object_new
on a singleton...

> @@ -172,7 +181,9 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
>          QAPI_LIST_PREPEND(prop_list, info);
>      }
>  
> -    object_unref(obj);
> +    if (create) {
> +        object_unref(obj);
> +    }

...and this just compounds the ugliness.

> @@ -199,7 +210,12 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
>          return NULL;
>      }
>  
> -    if (object_class_is_abstract(klass)) {
> +    /*
> +     * Abstract classes are not for instantiations, meanwhile avoid
> +     * creating temporary singleton objects because it can cause conflicts
> +     * if there's already one created.
> +     */

Another example of the foot-gun firing at random code

> +    if (object_class_is_abstract(klass) || object_class_is_singleton(klass)) {
>          object_class_property_iter_init(&iter, klass);
>      } else {
>          obj = object_new(typename);


With changes to QOM, I think it is generally informative to look at how
GLib has handled the problem, since the QOM design has heavily borrowed
from its GObject design.

In GObject, singletons are handled in a very differnt way. It has a
concept of a "constructor" function against the class, which is what is
responsible for allocating the object. By default the 'constructor' will
call g_new0, but a class which wishes to become a singleton will override
the 'constructor' function to allocate on first call, and return the
cached object on subsequent calls. This is illustrated here:

  https://gitlab.gnome.org/GNOME/glib/-/blob/main/gobject/gobject.h#L297

The key benefit of this is that everything can carry on calling
g_object_new() as before, as it will just "do the right thing"
in terms of allocation.

In QOM, we don't have a 'constructor' class function, we just directly
call g_malloc from object_new_with_type. This is because at the time,
we didn't see an immediate need for it. We could easily change that
though to introduce the concept of a 'constructor', which could
probably make singletons work without needing updates to existing code.



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/4] QOM: Singleton interface

[Peter Xu]

On Fri, Oct 25, 2024 at 09:38:31AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > This patchset introduces the singleton interface for QOM.
> >
> > The singleton interface is as simple as "this class can only create one
> > instance".
> >
> > We used to have similar demand when working on all kinds of vIOMMUs,
> > because in most cases that I am aware of, vIOMMU must be a singleton as
> > it's closely bound to the machine and also the root PCIe systems.  We used
> > to have a bunch of special code guarding those, for example, X86 has
> > pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
> > than once.
> >
> > There is a similar demand raising recently (even if the problem existed
> > over years) when we were looking at a migration bug, that part of it was
> > involved with the current_migration global pointer being referenced
> > anywhere, even after the migration object was finalize()ed.  So far without
> > singleton support, there's no way to reset the variable properly.
> > Declaring migration object to be a singleton also just makes sense, e.g.,
> > dynamically creating migration objects on the fly with QMP commands doesn't
> > sound right..
> >
> > The idea behind is pretty simple: any object that can only be created once
> > can now declare the TYPE_SINGLETON interface, then QOM facilities will make
> > sure it won't be created more than once.  For example, qom-list-properties,
> > device-list-properties, etc., will be smart enough to not try to create
> > temporary singleton objects now.
> 
> QOM design assumption: object_new() followed by object_unref() is always
> possible and has no side effect.

I see.

> 
> QOM introspection relies on this.  Your PATCH 1 makes non-class
> properties of singletons invisible in introspection.  Making something
> with such properties a singleton would be a regression.
> 
> Anything that violates the design assumption must be delayed to a
> suitable state transition.  For devices (subtypes of TYPE_DEVICE), this
> is ->realize().  For user-creatable objects (provide interface
> TYPE_USER_CREATABLE), this is ->complete().  For anything else, it's
> something else that probably doesn't even exist, yet.  See "Problem 5:
> QOM lacks a clear life cycle" in
> 
>     Subject: Dynamic & heterogeneous machines, initial configuration: problems
>     Date: Wed, 31 Jan 2024 21:14:21 +0100
>     Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
>     https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/

The major challenge here might be that, in migration's use case we want to
do something after the last refcount is released.

IOW, I don't yet see an easy way to explicit do qdev_unrealize() (even if
migration object will use realize(), while it doesn't yet..), because there
can be more than one thread holding refcount of the object, and we don't
know where to invoke it, and we don't want to do the cleanup if the other
one still holds a refcount.

The only sane place now is in instance_finalize(), which is the only clean
place so far that is invoked after last refcount dropped.

Maybe I can also try do that with a "magic property" with its set/get, as
that's indeed the other hook (basically, object_property_del_all()) that is
only invoked after the final refcount is released.  However I think we
still need the singleton idea somehow..

Thanks,

-- 
Peter Xu

Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Philippe Mathieu-Daudé]

On 24/10/24 17:53, Peter Xu wrote:
> On Thu, Oct 24, 2024 at 05:02:19PM -0300, Philippe Mathieu-Daudé wrote:
>> Hi Peter,
> 
> Hi, Phil,
> 
> Thanks for the quick reviews!
> 
>>
>> (Cc'ing Mark)
>>
>> On 24/10/24 13:56, Peter Xu wrote:
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>    include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++
>>>    qom/object.c                    |  3 +++
>>>    qom/object_interfaces.c         | 24 +++++++++++++++++
>>>    qom/qom-qmp-cmds.c              | 22 ++++++++++++---
>>>    system/qdev-monitor.c           |  7 +++++
>>>    5 files changed, 100 insertions(+), 3 deletions(-)
>>
>>
>>> +/**
>>> + * SingletonClass:
>>> + *
>>> + * @parent_class: the base class
>>> + * @get_instance: fetch the singleton instance if it is created,
>>> + *                NULL otherwise.
>>> + *
>>> + * Singleton class describes the type of object classes that can only
>>> + * provide one instance for the whole lifecycle of QEMU.  It will fail the
>>> + * operation if one attemps to create more than one instance.
>>> + *
>>> + * One can fetch the single object using class's get_instance() callback if
>>> + * it was created before.  This can be useful for operations like QMP
>>> + * qom-list-properties, where dynamically creating an object might not be
>>> + * feasible.
>>> + */
>>> +struct SingletonClass {
>>> +    /* <private> */
>>> +    InterfaceClass parent_class;
>>> +    /* <public> */
>>> +    Object *(*get_instance)(Error **errp);
>>
>> IMHO asking get_instance() is overkill ...
>>
>>> +};
>>> +
>>> +/**
>>> + * object_class_is_singleton:
>>> + *
>>> + * @class: the class to detect singleton
>>> + *
>>> + * Returns: true if it's a singleton class, false otherwise.
>>> + */
>>> +bool object_class_is_singleton(ObjectClass *class);
>>> +
>>> +/**
>>> + * singleton_get_instance:
>>> + *
>>> + * @class: the class to fetch singleton instance
>>> + *
>>> + * Returns: the object* if the class is a singleton class and the singleton
>>> + *          object is created, NULL otherwise.
>>> + */
>>> +Object *singleton_get_instance(ObjectClass *class);
>>> +
>>>    #endif
>>
>>> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
>>> index e0833c8bfe..6766060d0a 100644
>>> --- a/qom/object_interfaces.c
>>> +++ b/qom/object_interfaces.c
>>> @@ -354,6 +354,23 @@ void user_creatable_cleanup(void)
>>>        object_unparent(object_get_objects_root());
>>>    }
>>> +bool object_class_is_singleton(ObjectClass *class)
>>> +{
>>> +    return !!object_class_dynamic_cast(class, TYPE_SINGLETON);
>>> +}
>>> +
>>> +Object *singleton_get_instance(ObjectClass *class)
>>> +{
>>
>> ... when we can use object_resolve_type_unambiguous:
>>
>>        return object_resolve_type_unambiguous(object_class_get_name(class,
>> NULL));
> 
> I think an issue is migration object is nowhere to be find under
> object_get_root(), so it won't work there.  A side benefit is, it's also
> faster..

Maybe a simpler alternative is to add a field in ObjectClass, maintain
a single GHashTable to store TypeName -> Instance and retrieve as:

Object *singleton_get_instance(ObjectClass *class)
{
     return g_hash_table_lookup(&singletons,
                                object_class_get_name(class));
}

TBH the TYPE_SINGLETON interface seems a bit over-engineered and its
implementation error prone.

> How about I use object_resolve_type_unambiguous() as a fallback?  Then it's
> used only if get_instance() is not provided.
> 
>>
>> BTW should we pass Error** argument to singleton_get_instance()?
> 
> I didn't expect it to fail, hence I didn't even add it to make it more like
> "this will always success or it asserts" kind of things.  I left Error** in
> the hook just to be slightly flexible, but I always pass in error_abort()
> in this patch.
> 
> I can either add Error** if anyone thinks it useful, or even drop Error**
> in ->get_instance() since it's mostly not used at least for now.
> 
> Let me know!
> 
>>
>>> +    SingletonClass *singleton =
>>> +        (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON);
>>> +
>>> +    if (!singleton) {
>>> +        return NULL;
>>> +    }
>>> +
>>> +    return singleton->get_instance(&error_abort);
>>> +}
>>
>> Alternatively call object_resolve_type_unambiguous() in instance_init()?
> 
> Thanks,
> 

Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Peter Xu]

On Fri, Oct 25, 2024 at 10:07:59AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++
> >  qom/object.c                    |  3 +++
> >  qom/object_interfaces.c         | 24 +++++++++++++++++
> >  qom/qom-qmp-cmds.c              | 22 ++++++++++++---
> >  system/qdev-monitor.c           |  7 +++++
> >  5 files changed, 100 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> > index 02b11a7ef0..9b2cc0e554 100644
> > --- a/include/qom/object_interfaces.h
> > +++ b/include/qom/object_interfaces.h
> > @@ -177,4 +177,51 @@ bool user_creatable_del(const char *id, Error **errp);
> >   */
> >  void user_creatable_cleanup(void);
> >  
> > +#define TYPE_SINGLETON "singleton"
> > +
> > +typedef struct SingletonClass SingletonClass;
> > +DECLARE_CLASS_CHECKERS(SingletonClass, SINGLETON, TYPE_SINGLETON)
> > +
> > +/**
> > + * SingletonClass:
> > + *
> > + * @parent_class: the base class
> > + * @get_instance: fetch the singleton instance if it is created,
> > + *                NULL otherwise.
> > + *
> > + * Singleton class describes the type of object classes that can only
> > + * provide one instance for the whole lifecycle of QEMU.  It will fail the
> > + * operation if one attemps to create more than one instance.
> > + *
> > + * One can fetch the single object using class's get_instance() callback if
> > + * it was created before.  This can be useful for operations like QMP
> > + * qom-list-properties, where dynamically creating an object might not be
> > + * feasible.
> > + */
> > +struct SingletonClass {
> > +    /* <private> */
> > +    InterfaceClass parent_class;
> > +    /* <public> */
> > +    Object *(*get_instance)(Error **errp);
> > +};
> > +
> > +/**
> > + * object_class_is_singleton:
> > + *
> > + * @class: the class to detect singleton
> > + *
> > + * Returns: true if it's a singleton class, false otherwise.
> > + */
> > +bool object_class_is_singleton(ObjectClass *class);
> > +
> > +/**
> > + * singleton_get_instance:
> > + *
> > + * @class: the class to fetch singleton instance
> > + *
> > + * Returns: the object* if the class is a singleton class and the singleton
> > + *          object is created, NULL otherwise.
> > + */
> > +Object *singleton_get_instance(ObjectClass *class);
> 
> A non-null return value can become dangling when the object gets
> destroyed.  This could conceivably happen in another thread.
> 
> The obviously safe interface would take a reference the caller must
> unref when done.

Ouch, thanks for spotting this!  Yes we definitely need a refcount at
least..

> 
> > +
> >  #endif
> > diff --git a/qom/object.c b/qom/object.c
> > index 11424cf471..ded299ae1a 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -553,6 +553,9 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type
> >      g_assert(type->abstract == false);
> >      g_assert(size >= type->instance_size);
> >  
> > +    /* Singleton class can only create one object */
> > +    g_assert(!singleton_get_instance(type->class));
> > +
> >      memset(obj, 0, type->instance_size);
> >      obj->class = type->class;
> >      object_ref(obj);
> > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > index e0833c8bfe..6766060d0a 100644
> > --- a/qom/object_interfaces.c
> > +++ b/qom/object_interfaces.c
> > @@ -354,6 +354,23 @@ void user_creatable_cleanup(void)
> >      object_unparent(object_get_objects_root());
> >  }
> >  
> > +bool object_class_is_singleton(ObjectClass *class)
> > +{
> > +    return !!object_class_dynamic_cast(class, TYPE_SINGLETON);
> > +}
> > +
> > +Object *singleton_get_instance(ObjectClass *class)
> > +{
> > +    SingletonClass *singleton =
> > +        (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON);
> > +
> > +    if (!singleton) {
> > +        return NULL;
> > +    }
> > +
> > +    return singleton->get_instance(&error_abort);
> > +}
> > +
> >  static void register_types(void)
> >  {
> >      static const TypeInfo uc_interface_info = {
> > @@ -362,7 +379,14 @@ static void register_types(void)
> >          .class_size = sizeof(UserCreatableClass),
> >      };
> >  
> > +    static const TypeInfo singleton_interface_info = {
> > +        .name          = TYPE_SINGLETON,
> > +        .parent        = TYPE_INTERFACE,
> > +        .class_size = sizeof(SingletonClass),
> > +    };
> > +
> >      type_register_static(&uc_interface_info);
> > +    type_register_static(&singleton_interface_info);
> >  }
> >  
> >  type_init(register_types)
> > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> > index e91a235347..ecc1cf781c 100644
> > --- a/qom/qom-qmp-cmds.c
> > +++ b/qom/qom-qmp-cmds.c
> > @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
> >      ObjectProperty *prop;
> >      ObjectPropertyIterator iter;
> >      ObjectPropertyInfoList *prop_list = NULL;
> > +    bool create;
> >  
> >      klass = module_object_class_by_name(typename);
> >      if (klass == NULL) {
> > @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
> >          return NULL;
> >      }
> >  
> > -    obj = object_new(typename);
> > +    /* Avoid creating multiple instances if the class is a singleton */
> > +    create = !object_class_is_singleton(klass) ||
> > +        !singleton_get_instance(klass);
> > +
> > +    if (create) {
> > +        obj = object_new(typename);
> 
> If the class is not a singleton or else if no instance exists, we create
> a temporary instance.
> 
> > +    } else {
> > +        obj = singleton_get_instance(klass);
> 
> If the class is a singleton and the instance exists, we use that
> instead.
> 
> Any properties the instance has created dynamically after object_new()
> are visible to introspection.  This is not the case when we create a
> temporary instance.  Such subtle differences are problematic.  If we
> decide we're okay with this one, we need to document it.

I am thinking what's the major use case for device-list-properties.

If it's for mgmt to query a device property list for set/get before
operating on a real instance (which represents already somewhere in the
device tree), I hope it's fine.  Basically, it means whatever queried here
can always be used with qom-get/qom-set for this singleton as long as it's
already created and present.

I hope Libvirt cannot cache this results anyway, because we already have:

##
# @device-list-properties:
# ...
# .. note:: Objects can create properties at runtime, for example to
#    describe links between different devices and/or objects.  These
#    properties are not included in the output of this command.

So I think it means mgmt cannot cache these results, but only query before
qom-set/qom-get to make sure it's valid.  In that case, maybe we can change
that to s/are not included/may not be included/?

> 
> > +    }
> >  
> >      object_property_iter_init(&iter, obj);
> >      while ((prop = object_property_iter_next(&iter))) {
> > @@ -172,7 +181,9 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
> >          QAPI_LIST_PREPEND(prop_list, info);
> >      }
> >  
> > -    object_unref(obj);
> > +    if (create) {
> > +        object_unref(obj);
> > +    }
> >  
> >      return prop_list;
> >  }
> > @@ -199,7 +210,12 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
> >          return NULL;
> >      }
> >  
> > -    if (object_class_is_abstract(klass)) {
> > +    /*
> > +     * Abstract classes are not for instantiations, meanwhile avoid
> > +     * creating temporary singleton objects because it can cause conflicts
> > +     * if there's already one created.
> > +     */
> > +    if (object_class_is_abstract(klass) || object_class_is_singleton(klass)) {
> >          object_class_property_iter_init(&iter, klass);
> >      } else {
> >          obj = object_new(typename);
> 
> If the class is a singleton, we treat it as if it was abstract.  Its
> instance properties, if any, are not visible in qom-list-properties.
> This is a defect.  If you make an existing class with instance
> properties a singleton, the defect is a regression.

I can switch to get_instance() for singleton when it's available.  Would
that work?

Thanks!

> 
> > diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> > index 44994ea0e1..1310f35c9f 100644
> > --- a/system/qdev-monitor.c
> > +++ b/system/qdev-monitor.c
> > @@ -36,6 +36,7 @@
> >  #include "qemu/option.h"
> >  #include "qemu/qemu-print.h"
> >  #include "qemu/option_int.h"
> > +#include "qom/object_interfaces.h"
> >  #include "sysemu/block-backend.h"
> >  #include "migration/misc.h"
> >  #include "qemu/cutils.h"
> > @@ -643,6 +644,12 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
> >          return NULL;
> >      }
> >  
> > +    if (singleton_get_instance(OBJECT_CLASS(dc))) {
> > +        error_setg(errp, "Class '%s' only supports one instance",
> > +                   driver);
> > +        return NULL;
> > +    }
> > +
> >      /* find bus */
> >      path = qdict_get_try_str(opts, "bus");
> >      if (path != NULL) {
> 

-- 
Peter Xu

Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Peter Xu]

On Fri, Oct 25, 2024 at 10:51:21AM +0100, Daniel P. Berrangé wrote:
> On Thu, Oct 24, 2024 at 12:56:24PM -0400, Peter Xu wrote:
> 
> Adding significant new functionality to QOM should really come
> with a commit message explaining the rationale and the design
> choices
> 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++
> >  qom/object.c                    |  3 +++
> >  qom/object_interfaces.c         | 24 +++++++++++++++++
> >  qom/qom-qmp-cmds.c              | 22 ++++++++++++---
> >  system/qdev-monitor.c           |  7 +++++
> >  5 files changed, 100 insertions(+), 3 deletions(-)
> 
> snip
> 
> > + * Singleton class describes the type of object classes that can only
> > + * provide one instance for the whole lifecycle of QEMU.  It will fail the
> > + * operation if one attemps to create more than one instance.
> > + *
> > + * One can fetch the single object using class's get_instance() callback if
> > + * it was created before.  This can be useful for operations like QMP
> > + * qom-list-properties, where dynamically creating an object might not be
> > + * feasible.
> 
> snip
> 
> > +/**
> > + * singleton_get_instance:
> > + *
> > + * @class: the class to fetch singleton instance
> > + *
> > + * Returns: the object* if the class is a singleton class and the singleton
> > + *          object is created, NULL otherwise.
> > + */
> > +Object *singleton_get_instance(ObjectClass *class);
> 
> With this design, all code that uses a given type needs to know
> whether or not it is intended to be a singleton. If some code
> somewhere mistakenly calls 'object_new' instead of 'singleton_get_instance',
> the singleton type  is no longer a singleton, except you handle this by
> adding an assert in object_initialize_with_type.

Yes, that's really the current goal, and why I added that assert(), so it
fails any attempts trying to create one more instance of it, because
normally it's by accident.  The theory issue to solve is when some class is
not ready for being created more than once, so it must not happen.

My plan was to properly guard qdev creation with this which can fail
pretty, so it's a "programming error" otherwise.

> 
> This is still a bit of a loaded foot-gun IMHO, as we don't want random
> code asserting.
> 
> > diff --git a/qom/object.c b/qom/object.c
> > index 11424cf471..ded299ae1a 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -553,6 +553,9 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type
> >      g_assert(type->abstract == false);
> >      g_assert(size >= type->instance_size);
> >  
> > +    /* Singleton class can only create one object */
> > +    g_assert(!singleton_get_instance(type->class));
> > +
> >      memset(obj, 0, type->instance_size);
> >      obj->class = type->class;
> >      object_ref(obj);
> 
> > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> > index e91a235347..ecc1cf781c 100644
> > --- a/qom/qom-qmp-cmds.c
> > +++ b/qom/qom-qmp-cmds.c
> > @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
> >      ObjectProperty *prop;
> >      ObjectPropertyIterator iter;
> >      ObjectPropertyInfoList *prop_list = NULL;
> > +    bool create;
> >  
> >      klass = module_object_class_by_name(typename);
> >      if (klass == NULL) {
> > @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
> >          return NULL;
> >      }
> >  
> > -    obj = object_new(typename);
> > +    /* Avoid creating multiple instances if the class is a singleton */
> > +    create = !object_class_is_singleton(klass) ||
> > +        !singleton_get_instance(klass);
> > +
> > +    if (create) {
> > +        obj = object_new(typename);
> > +    } else {
> > +        obj = singleton_get_instance(klass);
> > +    }
> 
> This is the first foot-gun example.
> 
> I really strongly dislike that the design pattern forces us to
> create code like this, as we can never be confident we've
> correctly identified all the places which may call object_new
> on a singleton...

Yeah I agree it's not pretty, IMHO it's a trade-off comparing to glib's,
I'll comment below for that.

Meanwhile I hope there should be very limited places in QEMU to randomly
create "any" object on the fly.. so far only qom/device-list-properties
that I see.

> 
> > @@ -172,7 +181,9 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
> >          QAPI_LIST_PREPEND(prop_list, info);
> >      }
> >  
> > -    object_unref(obj);
> > +    if (create) {
> > +        object_unref(obj);
> > +    }
> 
> ...and this just compounds the ugliness.
> 
> > @@ -199,7 +210,12 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
> >          return NULL;
> >      }
> >  
> > -    if (object_class_is_abstract(klass)) {
> > +    /*
> > +     * Abstract classes are not for instantiations, meanwhile avoid
> > +     * creating temporary singleton objects because it can cause conflicts
> > +     * if there's already one created.
> > +     */
> 
> Another example of the foot-gun firing at random code
> 
> > +    if (object_class_is_abstract(klass) || object_class_is_singleton(klass)) {
> >          object_class_property_iter_init(&iter, klass);
> >      } else {
> >          obj = object_new(typename);
> 
> 
> With changes to QOM, I think it is generally informative to look at how
> GLib has handled the problem, since the QOM design has heavily borrowed
> from its GObject design.
> 
> In GObject, singletons are handled in a very differnt way. It has a
> concept of a "constructor" function against the class, which is what is
> responsible for allocating the object. By default the 'constructor' will
> call g_new0, but a class which wishes to become a singleton will override
> the 'constructor' function to allocate on first call, and return the
> cached object on subsequent calls. This is illustrated here:
> 
>   https://gitlab.gnome.org/GNOME/glib/-/blob/main/gobject/gobject.h#L297
> 
> The key benefit of this is that everything can carry on calling
> g_object_new() as before, as it will just "do the right thing"
> in terms of allocation.
> 
> In QOM, we don't have a 'constructor' class function, we just directly
> call g_malloc from object_new_with_type. This is because at the time,
> we didn't see an immediate need for it. We could easily change that
> though to introduce the concept of a 'constructor', which could
> probably make singletons work without needing updates to existing code.

I think glib's implementation is not thread safe on its own... consider two
threads invoke g_object_new() on the singleton without proper locking.  I
am guessing it could be relevant to glib's heavy event model.

And that's fundamentally what I want to have for QEMU's migration object,
so that it doesn't need locking on its own of the singleton idea: the
locking, if necessary, should be done in get_instance(), in this case.
So I did it wrong indeed in this current series at least there, where it
should need to take migration's new mutex, then take a refcount before
return, rightfully as Markus pointed out elsewhere.

The other concern I have with glib's singleton is, fundamentally
object_new() didn't really create a new object, which can be against the
gut feelings of whoever read it.  I'm not sure whether that's really what
we want.. or maybe that's only my own concern, so it might be subjective.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Peter Xu]

On Fri, Oct 25, 2024 at 12:11:51PM -0300, Philippe Mathieu-Daudé wrote:
> On 24/10/24 17:53, Peter Xu wrote:
> > On Thu, Oct 24, 2024 at 05:02:19PM -0300, Philippe Mathieu-Daudé wrote:
> > > Hi Peter,
> > 
> > Hi, Phil,
> > 
> > Thanks for the quick reviews!
> > 
> > > 
> > > (Cc'ing Mark)
> > > 
> > > On 24/10/24 13:56, Peter Xu wrote:
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >    include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++
> > > >    qom/object.c                    |  3 +++
> > > >    qom/object_interfaces.c         | 24 +++++++++++++++++
> > > >    qom/qom-qmp-cmds.c              | 22 ++++++++++++---
> > > >    system/qdev-monitor.c           |  7 +++++
> > > >    5 files changed, 100 insertions(+), 3 deletions(-)
> > > 
> > > 
> > > > +/**
> > > > + * SingletonClass:
> > > > + *
> > > > + * @parent_class: the base class
> > > > + * @get_instance: fetch the singleton instance if it is created,
> > > > + *                NULL otherwise.
> > > > + *
> > > > + * Singleton class describes the type of object classes that can only
> > > > + * provide one instance for the whole lifecycle of QEMU.  It will fail the
> > > > + * operation if one attemps to create more than one instance.
> > > > + *
> > > > + * One can fetch the single object using class's get_instance() callback if
> > > > + * it was created before.  This can be useful for operations like QMP
> > > > + * qom-list-properties, where dynamically creating an object might not be
> > > > + * feasible.
> > > > + */
> > > > +struct SingletonClass {
> > > > +    /* <private> */
> > > > +    InterfaceClass parent_class;
> > > > +    /* <public> */
> > > > +    Object *(*get_instance)(Error **errp);
> > > 
> > > IMHO asking get_instance() is overkill ...
> > > 
> > > > +};
> > > > +
> > > > +/**
> > > > + * object_class_is_singleton:
> > > > + *
> > > > + * @class: the class to detect singleton
> > > > + *
> > > > + * Returns: true if it's a singleton class, false otherwise.
> > > > + */
> > > > +bool object_class_is_singleton(ObjectClass *class);
> > > > +
> > > > +/**
> > > > + * singleton_get_instance:
> > > > + *
> > > > + * @class: the class to fetch singleton instance
> > > > + *
> > > > + * Returns: the object* if the class is a singleton class and the singleton
> > > > + *          object is created, NULL otherwise.
> > > > + */
> > > > +Object *singleton_get_instance(ObjectClass *class);
> > > > +
> > > >    #endif
> > > 
> > > > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > > > index e0833c8bfe..6766060d0a 100644
> > > > --- a/qom/object_interfaces.c
> > > > +++ b/qom/object_interfaces.c
> > > > @@ -354,6 +354,23 @@ void user_creatable_cleanup(void)
> > > >        object_unparent(object_get_objects_root());
> > > >    }
> > > > +bool object_class_is_singleton(ObjectClass *class)
> > > > +{
> > > > +    return !!object_class_dynamic_cast(class, TYPE_SINGLETON);
> > > > +}
> > > > +
> > > > +Object *singleton_get_instance(ObjectClass *class)
> > > > +{
> > > 
> > > ... when we can use object_resolve_type_unambiguous:
> > > 
> > >        return object_resolve_type_unambiguous(object_class_get_name(class,
> > > NULL));
> > 
> > I think an issue is migration object is nowhere to be find under
> > object_get_root(), so it won't work there.  A side benefit is, it's also
> > faster..
> 
> Maybe a simpler alternative is to add a field in ObjectClass, maintain
> a single GHashTable to store TypeName -> Instance and retrieve as:
> 
> Object *singleton_get_instance(ObjectClass *class)
> {
>     return g_hash_table_lookup(&singletons,
>                                object_class_get_name(class));
> }

This may need some proper locking too as Markus pointed out, so that the
object needs to boost its refcount before returned.  It might be a problem
if the singleton class has its own locking.. I'll need to think about it.

In migration's case, there's going to be a new migration mutex protecting
the singleton object being updated (not included in this series, posted
elsewhere [1]).

[1] https://lore.kernel.org/r/20241024213056.1395400-9-peterx@redhat.com

> 
> TBH the TYPE_SINGLETON interface seems a bit over-engineered and its
> implementation error prone.

Please feel free to suggest something else if you come up with.

The issue so far is qom/device-list-properties now can randomly create
migration object, which is defined to be global, while the plan is we need
to do something tricky in instance_finalize() for migration object (operate
on global vars; which is tricky but it could solve some issue we
encounter), so we must make sure it's never created more than once.

Thanks,

> 
> > How about I use object_resolve_type_unambiguous() as a fallback?  Then it's
> > used only if get_instance() is not provided.
> > 
> > > 
> > > BTW should we pass Error** argument to singleton_get_instance()?
> > 
> > I didn't expect it to fail, hence I didn't even add it to make it more like
> > "this will always success or it asserts" kind of things.  I left Error** in
> > the hook just to be slightly flexible, but I always pass in error_abort()
> > in this patch.
> > 
> > I can either add Error** if anyone thinks it useful, or even drop Error**
> > in ->get_instance() since it's mostly not used at least for now.
> > 
> > Let me know!
> > 
> > > 
> > > > +    SingletonClass *singleton =
> > > > +        (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON);
> > > > +
> > > > +    if (!singleton) {
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    return singleton->get_instance(&error_abort);
> > > > +}
> > > 
> > > Alternatively call object_resolve_type_unambiguous() in instance_init()?
> > 
> > Thanks,
> > 
> 

-- 
Peter Xu

Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Daniel P. Berrangé]

On Fri, Oct 25, 2024 at 12:17:02PM -0400, Peter Xu wrote:
> On Fri, Oct 25, 2024 at 10:51:21AM +0100, Daniel P. Berrangé wrote:
> > > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> > > index e91a235347..ecc1cf781c 100644
> > > --- a/qom/qom-qmp-cmds.c
> > > +++ b/qom/qom-qmp-cmds.c
> > > @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
> > >      ObjectProperty *prop;
> > >      ObjectPropertyIterator iter;
> > >      ObjectPropertyInfoList *prop_list = NULL;
> > > +    bool create;
> > >  
> > >      klass = module_object_class_by_name(typename);
> > >      if (klass == NULL) {
> > > @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
> > >          return NULL;
> > >      }
> > >  
> > > -    obj = object_new(typename);
> > > +    /* Avoid creating multiple instances if the class is a singleton */
> > > +    create = !object_class_is_singleton(klass) ||
> > > +        !singleton_get_instance(klass);
> > > +
> > > +    if (create) {
> > > +        obj = object_new(typename);
> > > +    } else {
> > > +        obj = singleton_get_instance(klass);
> > > +    }
> > 
> > This is the first foot-gun example.
> > 
> > I really strongly dislike that the design pattern forces us to
> > create code like this, as we can never be confident we've
> > correctly identified all the places which may call object_new
> > on a singleton...
> 
> Yeah I agree it's not pretty, IMHO it's a trade-off comparing to glib's,
> I'll comment below for that.
> 
> Meanwhile I hope there should be very limited places in QEMU to randomly
> create "any" object on the fly.. so far only qom/device-list-properties
> that I see.

There's -object/-device CLI, and their corresponding object_add
/ device_add commands.

They are not relevant for the migration object, but you're adding
this feature to QOM, so we have to take them into account. If anyone
defines another Object or Device class as a singleton, we will have
quite a few places where we can trigger the assert. This is especially
bad if we trigger it from anything in QMP as that kills a running
guest.

> 
> I think glib's implementation is not thread safe on its own... consider two
> threads invoke g_object_new() on the singleton without proper locking.  I
> am guessing it could be relevant to glib's heavy event model.

I've not checked the full code path, to see if they have a serialization
point somewhere it the g_object_new call chain, but yes, it is a valid
concern that would need to be resolved.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Peter Xu]

[-- Attachment #1: Type: text/plain, Size: 416 bytes --]

On Fri, Oct 25, 2024, 5:51 a.m. Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Oct 24, 2024 at 12:56:24PM -0400, Peter Xu wrote:
>
> Adding significant new functionality to QOM should really come
> with a commit message explaining the rationale and the design
> choices
>

Ohh i definitely missed it, my bad.  Luckily i still wrote the cover
letter. I'll prepare that if there is v2.

>

[-- Attachment #2: Type: text/html, Size: 908 bytes --]

Re: [PATCH 2/4] x86/iommu: Make x86-iommu a singleton object

[Peter Xu]

On Fri, Oct 25, 2024 at 11:25:23AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > X86 IOMMUs cannot be created more than one on a system yet.  Make it a
> > singleton so it guards the system from accidentally create yet another
> > IOMMU object when one already presents.
> >
> > Now if someone tries to create more than one, e.g., via:
> >
> >   ./qemu -M q35 -device intel-iommu -device intel-iommu
> >
> > The error will change from:
> >
> >   qemu-system-x86_64: -device intel-iommu: QEMU does not support multiple vIOMMUs for x86 yet.
> >
> > To:
> >
> >   qemu-system-x86_64: -device intel-iommu: Class 'intel-iommu' only supports one instance
> >
> > Unfortunately, yet we can't remove the singleton check in the machine
> > hook (pc_machine_device_pre_plug_cb), because there can also be
> > virtio-iommu involved, which doesn't share a common parent class yet.
> >
> > But with this, it should be closer to reach that goal to check singleton by
> > QOM one day.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> $ qemu-system-x86_64 -device amd-iommu,help
> /work/armbru/qemu/include/hw/boards.h:24:MACHINE: Object 0x56473906f960 is not an instance of type machine
> Aborted (core dumped)
> 
> (gdb) bt
> #0  0x00007ffff4e43834 in __pthread_kill_implementation () at /lib64/libc.so.6
> #1  0x00007ffff4df18ee in raise () at /lib64/libc.so.6
> #2  0x00007ffff4dd98ff in abort () at /lib64/libc.so.6
> #3  0x0000555555f75ef3 in object_dynamic_cast_assert
>     (obj=0x555557e03960, typename=0x5555563c403e "machine", file=0x5555563c4018 "/work/armbru/qemu/include/hw/boards.h", line=24, func=0x5555563c4290 <__func__.7> "MACHINE") at ../qom/object.c:936
> #4  0x0000555555d5db0f in MACHINE (obj=0x555557e03960)
>     at /work/armbru/qemu/include/hw/boards.h:24
> #5  0x0000555555d5e030 in x86_iommu_get_default () at ../hw/i386/x86-iommu.c:83
> #6  0x0000555555d5e262 in x86_iommu_get_instance
>     (errp=0x5555573d4918 <error_abort>) at ../hw/i386/x86-iommu.c:139
> #7  0x0000555555f7c27c in singleton_get_instance (class=0x555557e00320)
>     at ../qom/object_interfaces.c:371
> #8  0x000055555612a842 in qmp_device_list_properties
>     (typename=0x555557e001d0 "amd-iommu", errp=0x7fffffffda38)
>     at ../qom/qom-qmp-cmds.c:147
> #9  0x0000555555bf20b2 in qdev_device_help (opts=0x555557e001f0)
>     at ../system/qdev-monitor.c:314
> #10 0x0000555555bfe06d in device_help_func
>     (opaque=0x0, opts=0x555557e001f0, errp=0x0) at ../system/vl.c:1208
> #11 0x0000555556217186 in qemu_opts_foreach
>     (list=0x55555729e5c0 <qemu_device_opts>, func=0x555555bfe04d <device_help_func>, opaque=0x0, errp=0x0) at ../util/qemu-option.c:1135
> #12 0x0000555555c01d56 in qemu_process_help_options () at ../system/vl.c:2555
> #13 0x0000555555c04d81 in qemu_init (argc=3, argv=0x7fffffffde28)
>     at ../system/vl.c:3654
> #14 0x000055555612ffae in main (argc=3, argv=0x7fffffffde28)
>     at ../system/main.c:47
> 

Thanks for the report!

It turns out that qdev_get_machine() cannot be invoked too early, and the
singleton code can make it earlier..

We may want a pre-requisite patch to allow qdev_get_machine() to be invoked
anytime, like:

===8<===
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index db36f54d91..7ceae47139 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -831,6 +831,16 @@ Object *qdev_get_machine(void)
 {
     static Object *dev;
 
+    if (!phase_check(PHASE_MACHINE_CREATED)) {
+        /*
+         * When the machine is not created, below can wrongly create
+         * /machine to be a container.. this enables qdev_get_machine() to
+         * be used at any time and return NULL properly when machine is not
+         * created.
+         */
+        return NULL;
+    }
+
     if (dev == NULL) {
         dev = container_get(object_get_root(), "/machine");
     }
===8<===

I hope it makes sense on its own. Then callers who can be invoked earlier
could then handle NULL properly, in this case..

===8<===
diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index 4bfeb08705..fceb7adfe0 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -80,9 +80,15 @@ void x86_iommu_irq_to_msi_message(X86IOMMUIrq *irq, MSIMessage *msg_out)
 
 X86IOMMUState *x86_iommu_get_default(void)
 {
-    MachineState *ms = MACHINE(qdev_get_machine());
-    PCMachineState *pcms =
-        PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE));
+    Object *machine = qdev_get_machine();
+    PCMachineState *pcms;
+
+    /* If machine has not been created, so is the vIOMMU */
+    if (!machine) {
+        return NULL;
+    }
+
+    pcms = PC_MACHINE(object_dynamic_cast(machine, TYPE_PC_MACHINE));
 
     if (pcms &&
         object_dynamic_cast(OBJECT(pcms->iommu), TYPE_X86_IOMMU_DEVICE)) {
===8<===

I'll make sure this works if I'll repost.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Peter Xu]

On Fri, Oct 25, 2024 at 05:22:01PM +0100, Daniel P. Berrangé wrote:
> On Fri, Oct 25, 2024 at 12:17:02PM -0400, Peter Xu wrote:
> > On Fri, Oct 25, 2024 at 10:51:21AM +0100, Daniel P. Berrangé wrote:
> > > > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> > > > index e91a235347..ecc1cf781c 100644
> > > > --- a/qom/qom-qmp-cmds.c
> > > > +++ b/qom/qom-qmp-cmds.c
> > > > @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
> > > >      ObjectProperty *prop;
> > > >      ObjectPropertyIterator iter;
> > > >      ObjectPropertyInfoList *prop_list = NULL;
> > > > +    bool create;
> > > >  
> > > >      klass = module_object_class_by_name(typename);
> > > >      if (klass == NULL) {
> > > > @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
> > > >          return NULL;
> > > >      }
> > > >  
> > > > -    obj = object_new(typename);
> > > > +    /* Avoid creating multiple instances if the class is a singleton */
> > > > +    create = !object_class_is_singleton(klass) ||
> > > > +        !singleton_get_instance(klass);
> > > > +
> > > > +    if (create) {
> > > > +        obj = object_new(typename);
> > > > +    } else {
> > > > +        obj = singleton_get_instance(klass);
> > > > +    }
> > > 
> > > This is the first foot-gun example.
> > > 
> > > I really strongly dislike that the design pattern forces us to
> > > create code like this, as we can never be confident we've
> > > correctly identified all the places which may call object_new
> > > on a singleton...
> > 
> > Yeah I agree it's not pretty, IMHO it's a trade-off comparing to glib's,
> > I'll comment below for that.
> > 
> > Meanwhile I hope there should be very limited places in QEMU to randomly
> > create "any" object on the fly.. so far only qom/device-list-properties
> > that I see.
> 
> There's -object/-device CLI, and their corresponding object_add
> / device_add commands.

Ah I didn't mention that when reply, but this patch blocks properly any
device-add for singleton objects for more than once.  Please see the change
in qdev_device_add_from_qdict().

For migration object, it means it'll always fail because migration object
is created very early, before someone can try to create it.  Not to mention
it also has dc->hotpluggable==false, so things like -device will never work
on migration object.

For object-add, IIUC migration object should always be safe because it has
no USER_CREATABLE interface.

> 
> They are not relevant for the migration object, but you're adding
> this feature to QOM, so we have to take them into account. If anyone
> defines another Object or Device class as a singleton, we will have
> quite a few places where we can trigger the assert. This is especially
> bad if we trigger it from anything in QMP as that kills a running
> guest.
> 
> > 
> > I think glib's implementation is not thread safe on its own... consider two
> > threads invoke g_object_new() on the singleton without proper locking.  I
> > am guessing it could be relevant to glib's heavy event model.
> 
> I've not checked the full code path, to see if they have a serialization
> point somewhere it the g_object_new call chain, but yes, it is a valid
> concern that would need to be resolved.

Thanks,

-- 
Peter Xu

Re: [PATCH 2/4] x86/iommu: Make x86-iommu a singleton object

[Peter Xu]

On Fri, Oct 25, 2024 at 05:55:59PM -0400, Peter Xu wrote:
> On Fri, Oct 25, 2024 at 11:25:23AM +0200, Markus Armbruster wrote:
> > Peter Xu <peterx@redhat.com> writes:
> > 
> > > X86 IOMMUs cannot be created more than one on a system yet.  Make it a
> > > singleton so it guards the system from accidentally create yet another
> > > IOMMU object when one already presents.
> > >
> > > Now if someone tries to create more than one, e.g., via:
> > >
> > >   ./qemu -M q35 -device intel-iommu -device intel-iommu
> > >
> > > The error will change from:
> > >
> > >   qemu-system-x86_64: -device intel-iommu: QEMU does not support multiple vIOMMUs for x86 yet.
> > >
> > > To:
> > >
> > >   qemu-system-x86_64: -device intel-iommu: Class 'intel-iommu' only supports one instance
> > >
> > > Unfortunately, yet we can't remove the singleton check in the machine
> > > hook (pc_machine_device_pre_plug_cb), because there can also be
> > > virtio-iommu involved, which doesn't share a common parent class yet.
> > >
> > > But with this, it should be closer to reach that goal to check singleton by
> > > QOM one day.
> > >
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > 
> > $ qemu-system-x86_64 -device amd-iommu,help
> > /work/armbru/qemu/include/hw/boards.h:24:MACHINE: Object 0x56473906f960 is not an instance of type machine
> > Aborted (core dumped)
> > 
> > (gdb) bt
> > #0  0x00007ffff4e43834 in __pthread_kill_implementation () at /lib64/libc.so.6
> > #1  0x00007ffff4df18ee in raise () at /lib64/libc.so.6
> > #2  0x00007ffff4dd98ff in abort () at /lib64/libc.so.6
> > #3  0x0000555555f75ef3 in object_dynamic_cast_assert
> >     (obj=0x555557e03960, typename=0x5555563c403e "machine", file=0x5555563c4018 "/work/armbru/qemu/include/hw/boards.h", line=24, func=0x5555563c4290 <__func__.7> "MACHINE") at ../qom/object.c:936
> > #4  0x0000555555d5db0f in MACHINE (obj=0x555557e03960)
> >     at /work/armbru/qemu/include/hw/boards.h:24
> > #5  0x0000555555d5e030 in x86_iommu_get_default () at ../hw/i386/x86-iommu.c:83
> > #6  0x0000555555d5e262 in x86_iommu_get_instance
> >     (errp=0x5555573d4918 <error_abort>) at ../hw/i386/x86-iommu.c:139
> > #7  0x0000555555f7c27c in singleton_get_instance (class=0x555557e00320)
> >     at ../qom/object_interfaces.c:371
> > #8  0x000055555612a842 in qmp_device_list_properties
> >     (typename=0x555557e001d0 "amd-iommu", errp=0x7fffffffda38)
> >     at ../qom/qom-qmp-cmds.c:147
> > #9  0x0000555555bf20b2 in qdev_device_help (opts=0x555557e001f0)
> >     at ../system/qdev-monitor.c:314
> > #10 0x0000555555bfe06d in device_help_func
> >     (opaque=0x0, opts=0x555557e001f0, errp=0x0) at ../system/vl.c:1208
> > #11 0x0000555556217186 in qemu_opts_foreach
> >     (list=0x55555729e5c0 <qemu_device_opts>, func=0x555555bfe04d <device_help_func>, opaque=0x0, errp=0x0) at ../util/qemu-option.c:1135
> > #12 0x0000555555c01d56 in qemu_process_help_options () at ../system/vl.c:2555
> > #13 0x0000555555c04d81 in qemu_init (argc=3, argv=0x7fffffffde28)
> >     at ../system/vl.c:3654
> > #14 0x000055555612ffae in main (argc=3, argv=0x7fffffffde28)
> >     at ../system/main.c:47
> > 
> 
> Thanks for the report!
> 
> It turns out that qdev_get_machine() cannot be invoked too early, and the
> singleton code can make it earlier..
> 
> We may want a pre-requisite patch to allow qdev_get_machine() to be invoked
> anytime, like:
> 
> ===8<===
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index db36f54d91..7ceae47139 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -831,6 +831,16 @@ Object *qdev_get_machine(void)
>  {
>      static Object *dev;
>  
> +    if (!phase_check(PHASE_MACHINE_CREATED)) {
> +        /*
> +         * When the machine is not created, below can wrongly create
> +         * /machine to be a container.. this enables qdev_get_machine() to
> +         * be used at any time and return NULL properly when machine is not
> +         * created.
> +         */
> +        return NULL;
> +    }
> +
>      if (dev == NULL) {
>          dev = container_get(object_get_root(), "/machine");
>      }
> ===8<===
> 
> I hope it makes sense on its own.

My apologies, spoke too soon here.  This helper is used too after machine
is created, but right before switching to PHASE_MACHINE_CREATE stage..

So we need another way, like:

===8<===
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index db36f54d91..36a9fdb428 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -832,7 +832,13 @@ Object *qdev_get_machine(void)
     static Object *dev;
 
     if (dev == NULL) {
-        dev = container_get(object_get_root(), "/machine");
+        /*
+         * NOTE: dev can keep being NULL if machine is not yet created!
+         * In which case the function will properly return NULL.
+         *
+         * Whenever machine object is created and found once, we cache it.
+         */
+        dev = object_resolve_path_component(object_get_root(), "machine");
     }
 
     return dev;
===8<===

The idea is still the same.  Meanwhile I'll test more to see whether it has
other issues.

Thanks,

> Then callers who can be invoked earlier
> could then handle NULL properly, in this case..
> 
> ===8<===
> diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
> index 4bfeb08705..fceb7adfe0 100644
> --- a/hw/i386/x86-iommu.c
> +++ b/hw/i386/x86-iommu.c
> @@ -80,9 +80,15 @@ void x86_iommu_irq_to_msi_message(X86IOMMUIrq *irq, MSIMessage *msg_out)
>  
>  X86IOMMUState *x86_iommu_get_default(void)
>  {
> -    MachineState *ms = MACHINE(qdev_get_machine());
> -    PCMachineState *pcms =
> -        PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE));
> +    Object *machine = qdev_get_machine();
> +    PCMachineState *pcms;
> +
> +    /* If machine has not been created, so is the vIOMMU */
> +    if (!machine) {
> +        return NULL;
> +    }
> +
> +    pcms = PC_MACHINE(object_dynamic_cast(machine, TYPE_PC_MACHINE));
>  
>      if (pcms &&
>          object_dynamic_cast(OBJECT(pcms->iommu), TYPE_X86_IOMMU_DEVICE)) {
> ===8<===
> 
> I'll make sure this works if I'll repost.
> 
> Thanks,
> 
> -- 
> Peter Xu

-- 
Peter Xu

Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Peter Xu]

On Fri, Oct 25, 2024 at 06:10:46PM -0400, Peter Xu wrote:
> On Fri, Oct 25, 2024 at 05:22:01PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Oct 25, 2024 at 12:17:02PM -0400, Peter Xu wrote:
> > > On Fri, Oct 25, 2024 at 10:51:21AM +0100, Daniel P. Berrangé wrote:
> > > > > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> > > > > index e91a235347..ecc1cf781c 100644
> > > > > --- a/qom/qom-qmp-cmds.c
> > > > > +++ b/qom/qom-qmp-cmds.c
> > > > > @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
> > > > >      ObjectProperty *prop;
> > > > >      ObjectPropertyIterator iter;
> > > > >      ObjectPropertyInfoList *prop_list = NULL;
> > > > > +    bool create;
> > > > >  
> > > > >      klass = module_object_class_by_name(typename);
> > > > >      if (klass == NULL) {
> > > > > @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
> > > > >          return NULL;
> > > > >      }
> > > > >  
> > > > > -    obj = object_new(typename);
> > > > > +    /* Avoid creating multiple instances if the class is a singleton */
> > > > > +    create = !object_class_is_singleton(klass) ||
> > > > > +        !singleton_get_instance(klass);
> > > > > +
> > > > > +    if (create) {
> > > > > +        obj = object_new(typename);
> > > > > +    } else {
> > > > > +        obj = singleton_get_instance(klass);
> > > > > +    }
> > > > 
> > > > This is the first foot-gun example.
> > > > 
> > > > I really strongly dislike that the design pattern forces us to
> > > > create code like this, as we can never be confident we've
> > > > correctly identified all the places which may call object_new
> > > > on a singleton...
> > > 
> > > Yeah I agree it's not pretty, IMHO it's a trade-off comparing to glib's,
> > > I'll comment below for that.
> > > 
> > > Meanwhile I hope there should be very limited places in QEMU to randomly
> > > create "any" object on the fly.. so far only qom/device-list-properties
> > > that I see.
> > 
> > There's -object/-device CLI, and their corresponding object_add
> > / device_add commands.
> 
> Ah I didn't mention that when reply, but this patch blocks properly any
> device-add for singleton objects for more than once.  Please see the change
> in qdev_device_add_from_qdict().
> 
> For migration object, it means it'll always fail because migration object
> is created very early, before someone can try to create it.  Not to mention
> it also has dc->hotpluggable==false, so things like -device will never work
> on migration object.
> 
> For object-add, IIUC migration object should always be safe because it has
> no USER_CREATABLE interface.
> 
> > 
> > They are not relevant for the migration object, but you're adding
> > this feature to QOM, so we have to take them into account. If anyone
> > defines another Object or Device class as a singleton, we will have
> > quite a few places where we can trigger the assert. This is especially
> > bad if we trigger it from anything in QMP as that kills a running
> > guest.

Sorry, for some reason I think I didn't notice the 2nd paragraph.. And
somehow I was only thinking the migration use case.

Indeed this part is not easy to get right.  I hope this is not a blocker
yet so far.  We can have a look when I send the new version; I'll start to
mark the series RFC.

Meanwhile I'll have a closer look, hopefully object_class_is_abstract() is
a good point of reference where I can track most of dynamic creations of
objects, and I'll go from there.

My current plan is we can have one helper object_new_allowed(), where it
should contain both the check over abstract & singleton classes before any
instantiations happen.

-- 
Peter Xu

Re: [PATCH 0/4] QOM: Singleton interface

[Daniel P. Berrangé]

On Fri, Oct 25, 2024 at 11:01:56AM -0400, Peter Xu wrote:
> On Fri, Oct 25, 2024 at 09:38:31AM +0200, Markus Armbruster wrote:
> > Peter Xu <peterx@redhat.com> writes:
> > 
> > > This patchset introduces the singleton interface for QOM.
> > >
> > > The singleton interface is as simple as "this class can only create one
> > > instance".
> > >
> > > We used to have similar demand when working on all kinds of vIOMMUs,
> > > because in most cases that I am aware of, vIOMMU must be a singleton as
> > > it's closely bound to the machine and also the root PCIe systems.  We used
> > > to have a bunch of special code guarding those, for example, X86 has
> > > pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
> > > than once.
> > >
> > > There is a similar demand raising recently (even if the problem existed
> > > over years) when we were looking at a migration bug, that part of it was
> > > involved with the current_migration global pointer being referenced
> > > anywhere, even after the migration object was finalize()ed.  So far without
> > > singleton support, there's no way to reset the variable properly.
> > > Declaring migration object to be a singleton also just makes sense, e.g.,
> > > dynamically creating migration objects on the fly with QMP commands doesn't
> > > sound right..
> > >
> > > The idea behind is pretty simple: any object that can only be created once
> > > can now declare the TYPE_SINGLETON interface, then QOM facilities will make
> > > sure it won't be created more than once.  For example, qom-list-properties,
> > > device-list-properties, etc., will be smart enough to not try to create
> > > temporary singleton objects now.
> > 
> > QOM design assumption: object_new() followed by object_unref() is always
> > possible and has no side effect.
> 
> I see.
> 
> > 
> > QOM introspection relies on this.  Your PATCH 1 makes non-class
> > properties of singletons invisible in introspection.  Making something
> > with such properties a singleton would be a regression.
> > 
> > Anything that violates the design assumption must be delayed to a
> > suitable state transition.  For devices (subtypes of TYPE_DEVICE), this
> > is ->realize().  For user-creatable objects (provide interface
> > TYPE_USER_CREATABLE), this is ->complete().  For anything else, it's
> > something else that probably doesn't even exist, yet.  See "Problem 5:
> > QOM lacks a clear life cycle" in
> > 
> >     Subject: Dynamic & heterogeneous machines, initial configuration: problems
> >     Date: Wed, 31 Jan 2024 21:14:21 +0100
> >     Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
> >     https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
> 
> The major challenge here might be that, in migration's use case we want to
> do something after the last refcount is released.

This is rather a strange idea, and feels back to front. An object's last
refcount must never be released until code has entirely finished using
the object.

> IOW, I don't yet see an easy way to explicit do qdev_unrealize() (even if
> migration object will use realize(), while it doesn't yet..), because there
> can be more than one thread holding refcount of the object, and we don't
> know where to invoke it, and we don't want to do the cleanup if the other
> one still holds a refcount.

This sounds like the code is missing some synchronization mechanism
beween the threads, which need to communicate with each other when
they are "done", so that the "primary" thread can then finally
release any resources.

> Maybe I can also try do that with a "magic property" with its set/get, as
> that's indeed the other hook (basically, object_property_del_all()) that is
> only invoked after the final refcount is released.  However I think we
> still need the singleton idea somehow..

Based on the description above it feels like the problem is outside
of QOM, around the lack of synchronization across threads.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/4] x86/iommu: Make x86-iommu a singleton object

[Daniel P. Berrangé]

On Thu, Oct 24, 2024 at 12:56:25PM -0400, Peter Xu wrote:
> X86 IOMMUs cannot be created more than one on a system yet.  Make it a
> singleton so it guards the system from accidentally create yet another
> IOMMU object when one already presents.
> 
> Now if someone tries to create more than one, e.g., via:
> 
>   ./qemu -M q35 -device intel-iommu -device intel-iommu
> 
> The error will change from:
> 
>   qemu-system-x86_64: -device intel-iommu: QEMU does not support multiple vIOMMUs for x86 yet.
> 
> To:
> 
>   qemu-system-x86_64: -device intel-iommu: Class 'intel-iommu' only supports one instance
> 
> Unfortunately, yet we can't remove the singleton check in the machine
> hook (pc_machine_device_pre_plug_cb), because there can also be
> virtio-iommu involved, which doesn't share a common parent class yet.

Presumably the 'class' reported is the one that the user requested,
but this would imply if we were to do

  qemu-system-x86_64 -device intel-iommu -device virtio-iommu

Then QEMU would report

   "Class 'virtio-iommu' only supports one instance"

at which point the user is wondering, huh, I only requested one virtio-iommu
instance ?

IOW, the current error message would be better as it is not referring to a
specific subclass, but rather to the more general fact that only a single
IOMMU is permitted, no matter what it's impl is.

> 
> But with this, it should be closer to reach that goal to check singleton by
> QOM one day.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/x86-iommu.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
> index 60af896225..4bfeb08705 100644
> --- a/hw/i386/x86-iommu.c
> +++ b/hw/i386/x86-iommu.c
> @@ -26,6 +26,7 @@
>  #include "qemu/error-report.h"
>  #include "trace.h"
>  #include "sysemu/kvm.h"
> +#include "qom/object_interfaces.h"
>  
>  void x86_iommu_iec_register_notifier(X86IOMMUState *iommu,
>                                       iec_notify_fn fn, void *data)
> @@ -133,10 +134,19 @@ static Property x86_iommu_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static Object *x86_iommu_get_instance(Error **errp)
> +{
> +    return OBJECT(x86_iommu_get_default());
> +}
> +
>  static void x86_iommu_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    SingletonClass *singleton = SINGLETON_CLASS(klass);
> +
>      dc->realize = x86_iommu_realize;
> +    singleton->get_instance = x86_iommu_get_instance;
> +
>      device_class_set_props(dc, x86_iommu_properties);
>  }
>  
> @@ -152,6 +162,10 @@ static const TypeInfo x86_iommu_info = {
>      .class_init    = x86_iommu_class_init,
>      .class_size    = sizeof(X86IOMMUClass),
>      .abstract      = true,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_SINGLETON },
> +        { }
> +    }
>  };
>  
>  static void x86_iommu_register_types(void)
> -- 
> 2.45.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/4] x86/iommu: Make x86-iommu a singleton object

[Peter Xu]

On Tue, Oct 29, 2024 at 10:47:06AM +0000, Daniel P. Berrangé wrote:
> On Thu, Oct 24, 2024 at 12:56:25PM -0400, Peter Xu wrote:
> > X86 IOMMUs cannot be created more than one on a system yet.  Make it a
> > singleton so it guards the system from accidentally create yet another
> > IOMMU object when one already presents.
> > 
> > Now if someone tries to create more than one, e.g., via:
> > 
> >   ./qemu -M q35 -device intel-iommu -device intel-iommu
> > 
> > The error will change from:
> > 
> >   qemu-system-x86_64: -device intel-iommu: QEMU does not support multiple vIOMMUs for x86 yet.
> > 
> > To:
> > 
> >   qemu-system-x86_64: -device intel-iommu: Class 'intel-iommu' only supports one instance
> > 
> > Unfortunately, yet we can't remove the singleton check in the machine
> > hook (pc_machine_device_pre_plug_cb), because there can also be
> > virtio-iommu involved, which doesn't share a common parent class yet.
> 
> Presumably the 'class' reported is the one that the user requested,
> but this would imply if we were to do
> 
>   qemu-system-x86_64 -device intel-iommu -device virtio-iommu
> 
> Then QEMU would report
> 
>    "Class 'virtio-iommu' only supports one instance"
> 
> at which point the user is wondering, huh, I only requested one virtio-iommu
> instance ?
> 
> IOW, the current error message would be better as it is not referring to a
> specific subclass, but rather to the more general fact that only a single
> IOMMU is permitted, no matter what it's impl is.

True.. though IIUC this is more or less a cosmetic change only.  E.g., if
we want (assuming after we could have object_new_allowed(Error **errp),
checking both abstract + singleton classes) we could make the error points
to the base class (rather than the top class to be initiated) that declared
TYPE_SINGLETON when it failed due to the singleton check.

One step further, we can even provide a custom Error for any singleton
class to say whatever it wants if it hits a duplicate.

So to me it's a separate issue from whether we would like to have a generic
way to define a singleton class.  I am still ok if we want to avoid
introducing the singleton, but just to mention I believe it can report
something similar as before if we want.

Thanks,

-- 
Peter Xu

Re: [PATCH 0/4] QOM: Singleton interface

[Peter Xu]

On Tue, Oct 29, 2024 at 10:42:58AM +0000, Daniel P. Berrangé wrote:
> On Fri, Oct 25, 2024 at 11:01:56AM -0400, Peter Xu wrote:
> > On Fri, Oct 25, 2024 at 09:38:31AM +0200, Markus Armbruster wrote:
> > > Peter Xu <peterx@redhat.com> writes:
> > > 
> > > > This patchset introduces the singleton interface for QOM.
> > > >
> > > > The singleton interface is as simple as "this class can only create one
> > > > instance".
> > > >
> > > > We used to have similar demand when working on all kinds of vIOMMUs,
> > > > because in most cases that I am aware of, vIOMMU must be a singleton as
> > > > it's closely bound to the machine and also the root PCIe systems.  We used
> > > > to have a bunch of special code guarding those, for example, X86 has
> > > > pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
> > > > than once.
> > > >
> > > > There is a similar demand raising recently (even if the problem existed
> > > > over years) when we were looking at a migration bug, that part of it was
> > > > involved with the current_migration global pointer being referenced
> > > > anywhere, even after the migration object was finalize()ed.  So far without
> > > > singleton support, there's no way to reset the variable properly.
> > > > Declaring migration object to be a singleton also just makes sense, e.g.,
> > > > dynamically creating migration objects on the fly with QMP commands doesn't
> > > > sound right..
> > > >
> > > > The idea behind is pretty simple: any object that can only be created once
> > > > can now declare the TYPE_SINGLETON interface, then QOM facilities will make
> > > > sure it won't be created more than once.  For example, qom-list-properties,
> > > > device-list-properties, etc., will be smart enough to not try to create
> > > > temporary singleton objects now.
> > > 
> > > QOM design assumption: object_new() followed by object_unref() is always
> > > possible and has no side effect.
> > 
> > I see.
> > 
> > > 
> > > QOM introspection relies on this.  Your PATCH 1 makes non-class
> > > properties of singletons invisible in introspection.  Making something
> > > with such properties a singleton would be a regression.
> > > 
> > > Anything that violates the design assumption must be delayed to a
> > > suitable state transition.  For devices (subtypes of TYPE_DEVICE), this
> > > is ->realize().  For user-creatable objects (provide interface
> > > TYPE_USER_CREATABLE), this is ->complete().  For anything else, it's
> > > something else that probably doesn't even exist, yet.  See "Problem 5:
> > > QOM lacks a clear life cycle" in
> > > 
> > >     Subject: Dynamic & heterogeneous machines, initial configuration: problems
> > >     Date: Wed, 31 Jan 2024 21:14:21 +0100
> > >     Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
> > >     https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
> > 
> > The major challenge here might be that, in migration's use case we want to
> > do something after the last refcount is released.
> 
> This is rather a strange idea, and feels back to front. An object's last
> refcount must never be released until code has entirely finished using
> the object.
> 
> > IOW, I don't yet see an easy way to explicit do qdev_unrealize() (even if
> > migration object will use realize(), while it doesn't yet..), because there
> > can be more than one thread holding refcount of the object, and we don't
> > know where to invoke it, and we don't want to do the cleanup if the other
> > one still holds a refcount.
> 
> This sounds like the code is missing some synchronization mechanism
> beween the threads, which need to communicate with each other when
> they are "done", so that the "primary" thread can then finally
> release any resources.
> 
> > Maybe I can also try do that with a "magic property" with its set/get, as
> > that's indeed the other hook (basically, object_property_del_all()) that is
> > only invoked after the final refcount is released.  However I think we
> > still need the singleton idea somehow..
> 
> Based on the description above it feels like the problem is outside
> of QOM, around the lack of synchronization across threads.

Right, this used to be discussed here and you were also in the loop:

https://lore.kernel.org/qemu-devel/20190228122822.GD4970@work-vm/

I kind of agree join() would be perfect, disregard the question on whether
QEMU would still benefit from a singleton interface, no matter migration
would rely on that for refcounting, or simply it'll also make sense to just
avoid people creating random migration objects.

So yes, I think migration can benefit from singleton idea for more than one
reason, while the refcount issue here might be even better resolved by
join() in main.

It's just that in the thread Dave raised a few points on possible
challenges on join() in the main thread.  I'm not sure whether we should go
that route instead.  Obviously I am digging one of our legacy rabbit holes
from when I started to look at this "access current_migration anywhere"
issue reported from VFIO.  But if some of us think we should give it a
shot, I can try.  After all, I started digging.

Another simpler but direct solution to "access current_migration" is, we
simply don't free it at all, leaving process exit() to do the job.  Then I
can drop all object_[un]ref() for the migration object.  I think that could
work too, but ugly, for the refcount issue.

Thanks,

-- 
Peter Xu

Re: [PATCH 0/4] QOM: Singleton interface

[Daniel P. Berrangé]

On Tue, Oct 29, 2024 at 10:45:25AM -0400, Peter Xu wrote:
> On Tue, Oct 29, 2024 at 10:42:58AM +0000, Daniel P. Berrangé wrote:
> > On Fri, Oct 25, 2024 at 11:01:56AM -0400, Peter Xu wrote:
> > > On Fri, Oct 25, 2024 at 09:38:31AM +0200, Markus Armbruster wrote:
> > > > Peter Xu <peterx@redhat.com> writes:
> > > > 
> > > > > This patchset introduces the singleton interface for QOM.
> > > > >
> > > > > The singleton interface is as simple as "this class can only create one
> > > > > instance".
> > > > >
> > > > > We used to have similar demand when working on all kinds of vIOMMUs,
> > > > > because in most cases that I am aware of, vIOMMU must be a singleton as
> > > > > it's closely bound to the machine and also the root PCIe systems.  We used
> > > > > to have a bunch of special code guarding those, for example, X86 has
> > > > > pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
> > > > > than once.
> > > > >
> > > > > There is a similar demand raising recently (even if the problem existed
> > > > > over years) when we were looking at a migration bug, that part of it was
> > > > > involved with the current_migration global pointer being referenced
> > > > > anywhere, even after the migration object was finalize()ed.  So far without
> > > > > singleton support, there's no way to reset the variable properly.
> > > > > Declaring migration object to be a singleton also just makes sense, e.g.,
> > > > > dynamically creating migration objects on the fly with QMP commands doesn't
> > > > > sound right..
> > > > >
> > > > > The idea behind is pretty simple: any object that can only be created once
> > > > > can now declare the TYPE_SINGLETON interface, then QOM facilities will make
> > > > > sure it won't be created more than once.  For example, qom-list-properties,
> > > > > device-list-properties, etc., will be smart enough to not try to create
> > > > > temporary singleton objects now.
> > > > 
> > > > QOM design assumption: object_new() followed by object_unref() is always
> > > > possible and has no side effect.
> > > 
> > > I see.
> > > 
> > > > 
> > > > QOM introspection relies on this.  Your PATCH 1 makes non-class
> > > > properties of singletons invisible in introspection.  Making something
> > > > with such properties a singleton would be a regression.
> > > > 
> > > > Anything that violates the design assumption must be delayed to a
> > > > suitable state transition.  For devices (subtypes of TYPE_DEVICE), this
> > > > is ->realize().  For user-creatable objects (provide interface
> > > > TYPE_USER_CREATABLE), this is ->complete().  For anything else, it's
> > > > something else that probably doesn't even exist, yet.  See "Problem 5:
> > > > QOM lacks a clear life cycle" in
> > > > 
> > > >     Subject: Dynamic & heterogeneous machines, initial configuration: problems
> > > >     Date: Wed, 31 Jan 2024 21:14:21 +0100
> > > >     Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
> > > >     https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
> > > 
> > > The major challenge here might be that, in migration's use case we want to
> > > do something after the last refcount is released.
> > 
> > This is rather a strange idea, and feels back to front. An object's last
> > refcount must never be released until code has entirely finished using
> > the object.
> > 
> > > IOW, I don't yet see an easy way to explicit do qdev_unrealize() (even if
> > > migration object will use realize(), while it doesn't yet..), because there
> > > can be more than one thread holding refcount of the object, and we don't
> > > know where to invoke it, and we don't want to do the cleanup if the other
> > > one still holds a refcount.
> > 
> > This sounds like the code is missing some synchronization mechanism
> > beween the threads, which need to communicate with each other when
> > they are "done", so that the "primary" thread can then finally
> > release any resources.
> > 
> > > Maybe I can also try do that with a "magic property" with its set/get, as
> > > that's indeed the other hook (basically, object_property_del_all()) that is
> > > only invoked after the final refcount is released.  However I think we
> > > still need the singleton idea somehow..
> > 
> > Based on the description above it feels like the problem is outside
> > of QOM, around the lack of synchronization across threads.
> 
> Right, this used to be discussed here and you were also in the loop:
> 
> https://lore.kernel.org/qemu-devel/20190228122822.GD4970@work-vm/
> 
> I kind of agree join() would be perfect, disregard the question on whether
> QEMU would still benefit from a singleton interface, no matter migration
> would rely on that for refcounting, or simply it'll also make sense to just
> avoid people creating random migration objects.
> 
> So yes, I think migration can benefit from singleton idea for more than one
> reason, while the refcount issue here might be even better resolved by
> join() in main.
> 
> It's just that in the thread Dave raised a few points on possible
> challenges on join() in the main thread.  I'm not sure whether we should go
> that route instead.  Obviously I am digging one of our legacy rabbit holes
> from when I started to look at this "access current_migration anywhere"
> issue reported from VFIO.  But if some of us think we should give it a
> shot, I can try.  After all, I started digging.
> 
> Another simpler but direct solution to "access current_migration" is, we
> simply don't free it at all, leaving process exit() to do the job.  Then I
> can drop all object_[un]ref() for the migration object.  I think that could
> work too, but ugly, for the refcount issue.

I tend to feel that having MigrationState exist for the whole lifetime
of QEMU is a bug, forced on us by the unfortunate need to call
migrate-set-parameters/capabilities separately from the migrate
command, and by the need to query migrate info an arbitrary amount of
time after it finishes.

This puts libvirt in the awkward position of having to manually reset
all migration parameters, just to ensure earlier settings don't
accidentally affect a future migration operation :-( This is a design
that encourages mistakes.

Rather than MigrationState become a singleton, I'd really encourage
trying to see if we can fix the lifecycle design problems, so that
we have a clear beginning & end of the migration operation, with the
state discarded at the end, such that every future migrate starts
from a clean base.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/4] QOM: Singleton interface

[Peter Xu]

On Tue, Oct 29, 2024 at 04:04:50PM +0000, Daniel P. Berrangé wrote:
> On Tue, Oct 29, 2024 at 10:45:25AM -0400, Peter Xu wrote:
> > On Tue, Oct 29, 2024 at 10:42:58AM +0000, Daniel P. Berrangé wrote:
> > > On Fri, Oct 25, 2024 at 11:01:56AM -0400, Peter Xu wrote:
> > > > On Fri, Oct 25, 2024 at 09:38:31AM +0200, Markus Armbruster wrote:
> > > > > Peter Xu <peterx@redhat.com> writes:
> > > > > 
> > > > > > This patchset introduces the singleton interface for QOM.
> > > > > >
> > > > > > The singleton interface is as simple as "this class can only create one
> > > > > > instance".
> > > > > >
> > > > > > We used to have similar demand when working on all kinds of vIOMMUs,
> > > > > > because in most cases that I am aware of, vIOMMU must be a singleton as
> > > > > > it's closely bound to the machine and also the root PCIe systems.  We used
> > > > > > to have a bunch of special code guarding those, for example, X86 has
> > > > > > pc_machine_device_pre_plug_cb() just to detect when vIOMMU is created more
> > > > > > than once.
> > > > > >
> > > > > > There is a similar demand raising recently (even if the problem existed
> > > > > > over years) when we were looking at a migration bug, that part of it was
> > > > > > involved with the current_migration global pointer being referenced
> > > > > > anywhere, even after the migration object was finalize()ed.  So far without
> > > > > > singleton support, there's no way to reset the variable properly.
> > > > > > Declaring migration object to be a singleton also just makes sense, e.g.,
> > > > > > dynamically creating migration objects on the fly with QMP commands doesn't
> > > > > > sound right..
> > > > > >
> > > > > > The idea behind is pretty simple: any object that can only be created once
> > > > > > can now declare the TYPE_SINGLETON interface, then QOM facilities will make
> > > > > > sure it won't be created more than once.  For example, qom-list-properties,
> > > > > > device-list-properties, etc., will be smart enough to not try to create
> > > > > > temporary singleton objects now.
> > > > > 
> > > > > QOM design assumption: object_new() followed by object_unref() is always
> > > > > possible and has no side effect.
> > > > 
> > > > I see.
> > > > 
> > > > > 
> > > > > QOM introspection relies on this.  Your PATCH 1 makes non-class
> > > > > properties of singletons invisible in introspection.  Making something
> > > > > with such properties a singleton would be a regression.
> > > > > 
> > > > > Anything that violates the design assumption must be delayed to a
> > > > > suitable state transition.  For devices (subtypes of TYPE_DEVICE), this
> > > > > is ->realize().  For user-creatable objects (provide interface
> > > > > TYPE_USER_CREATABLE), this is ->complete().  For anything else, it's
> > > > > something else that probably doesn't even exist, yet.  See "Problem 5:
> > > > > QOM lacks a clear life cycle" in
> > > > > 
> > > > >     Subject: Dynamic & heterogeneous machines, initial configuration: problems
> > > > >     Date: Wed, 31 Jan 2024 21:14:21 +0100
> > > > >     Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
> > > > >     https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
> > > > 
> > > > The major challenge here might be that, in migration's use case we want to
> > > > do something after the last refcount is released.
> > > 
> > > This is rather a strange idea, and feels back to front. An object's last
> > > refcount must never be released until code has entirely finished using
> > > the object.
> > > 
> > > > IOW, I don't yet see an easy way to explicit do qdev_unrealize() (even if
> > > > migration object will use realize(), while it doesn't yet..), because there
> > > > can be more than one thread holding refcount of the object, and we don't
> > > > know where to invoke it, and we don't want to do the cleanup if the other
> > > > one still holds a refcount.
> > > 
> > > This sounds like the code is missing some synchronization mechanism
> > > beween the threads, which need to communicate with each other when
> > > they are "done", so that the "primary" thread can then finally
> > > release any resources.
> > > 
> > > > Maybe I can also try do that with a "magic property" with its set/get, as
> > > > that's indeed the other hook (basically, object_property_del_all()) that is
> > > > only invoked after the final refcount is released.  However I think we
> > > > still need the singleton idea somehow..
> > > 
> > > Based on the description above it feels like the problem is outside
> > > of QOM, around the lack of synchronization across threads.
> > 
> > Right, this used to be discussed here and you were also in the loop:
> > 
> > https://lore.kernel.org/qemu-devel/20190228122822.GD4970@work-vm/
> > 
> > I kind of agree join() would be perfect, disregard the question on whether
> > QEMU would still benefit from a singleton interface, no matter migration
> > would rely on that for refcounting, or simply it'll also make sense to just
> > avoid people creating random migration objects.
> > 
> > So yes, I think migration can benefit from singleton idea for more than one
> > reason, while the refcount issue here might be even better resolved by
> > join() in main.
> > 
> > It's just that in the thread Dave raised a few points on possible
> > challenges on join() in the main thread.  I'm not sure whether we should go
> > that route instead.  Obviously I am digging one of our legacy rabbit holes
> > from when I started to look at this "access current_migration anywhere"
> > issue reported from VFIO.  But if some of us think we should give it a
> > shot, I can try.  After all, I started digging.
> > 
> > Another simpler but direct solution to "access current_migration" is, we
> > simply don't free it at all, leaving process exit() to do the job.  Then I
> > can drop all object_[un]ref() for the migration object.  I think that could
> > work too, but ugly, for the refcount issue.
> 
> I tend to feel that having MigrationState exist for the whole lifetime
> of QEMU is a bug, forced on us by the unfortunate need to call
> migrate-set-parameters/capabilities separately from the migrate
> command, and by the need to query migrate info an arbitrary amount of
> time after it finishes.
> 
> This puts libvirt in the awkward position of having to manually reset
> all migration parameters, just to ensure earlier settings don't
> accidentally affect a future migration operation :-( This is a design
> that encourages mistakes.

I think it would still be easy to add "cap" & "params" arguments support
for the "migrate" QMP command without breaking the current API, iff that
helps in whatever form.  When present, it simply applies the caps and/or
param list first before invoking the migrate command, fail the command if
cap / param check fails.

But I'm not sure whether that's a concern at all for Libvirt, if what
Libvirt currently does is having separate "migrate-set-*" commands prior to
the "migrate".  I may have overlooked the real issue behind on how that
could complicate Libvirt.

> 
> Rather than MigrationState become a singleton, I'd really encourage
> trying to see if we can fix the lifecycle design problems, so that
> we have a clear beginning & end of the migration operation, with the
> state discarded at the end, such that every future migrate starts
> from a clean base.

I assume it implies the join() path as a start.  As long as we're ok we
risk slow quits of QEMU, as I would expect bug reports coming afterwards,
we could try.  I believe there's no way we can resolve all such issues in
one shot.  I doubt whether some of those can be too tricky so we may wish
to go back at some point, spending too much effort but without a direct
benefit yet so far.

Meanwhile we still have the immediate concern that current_migration can
points to freed memory right now with QEMU master branch.  That's simply
wrong.

I thought it was still an improvement to have the singleton idea, so that
even afterwards we can join() properly the idea may still make sense on its
own.  It is also not against the ultimate willingness that we want to
create an obj at start of migration, and clearly destroys it after
migration completes.  When that comes we keep migration object singleton,
but then the tricky operations on current_migration can be gone.

That still guarantees from QOM level that e.g. nobody tries to initiate
migration twice, for example, or accidentally create it somewhere, like in
qom-list-properties.

There's yet another short term plan that I can fix UAF for
current_migration, which is to have a refcount for migration alone on top
of Object, then it only object_unref() on the migration object after we
know the last real refcount is released.  That won't need any QOM change,
so keep the complexity internally.  But I prefer the current proposal,
assuming it could be still helpful in general for QEMU, even though I'm not
sure.

So.. I'm not sure whether we should ignore the current UAF until a whole
refactor of any kind to land, or we should do something to fix it, which is
what I'm trying with the series.

Thanks,

-- 
Peter Xu

Re: [PATCH 0/4] QOM: Singleton interface

[Daniel P. Berrangé]

On Tue, Oct 29, 2024 at 01:05:04PM -0400, Peter Xu wrote:
> But I'm not sure whether that's a concern at all for Libvirt, if what
> Libvirt currently does is having separate "migrate-set-*" commands prior to
> the "migrate".  I may have overlooked the real issue behind on how that
> could complicate Libvirt.
> 
> > 
> > Rather than MigrationState become a singleton, I'd really encourage
> > trying to see if we can fix the lifecycle design problems, so that
> > we have a clear beginning & end of the migration operation, with the
> > state discarded at the end, such that every future migrate starts
> > from a clean base.
> 
> I assume it implies the join() path as a start.  As long as we're ok we
> risk slow quits of QEMU, as I would expect bug reports coming afterwards,
> we could try.  I believe there's no way we can resolve all such issues in
> one shot.  I doubt whether some of those can be too tricky so we may wish
> to go back at some point, spending too much effort but without a direct
> benefit yet so far.
> 
> Meanwhile we still have the immediate concern that current_migration can
> points to freed memory right now with QEMU master branch.  That's simply
> wrong.

Yeah, if we're accessing freed memory, that's a segv/abrt danger, and
needs fixing quickly.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/4] x86/iommu: Make x86-iommu a singleton object

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Oct 25, 2024 at 05:55:59PM -0400, Peter Xu wrote:
>> On Fri, Oct 25, 2024 at 11:25:23AM +0200, Markus Armbruster wrote:
>> > Peter Xu <peterx@redhat.com> writes:
>> > 
>> > > X86 IOMMUs cannot be created more than one on a system yet.  Make it a
>> > > singleton so it guards the system from accidentally create yet another
>> > > IOMMU object when one already presents.
>> > >
>> > > Now if someone tries to create more than one, e.g., via:
>> > >
>> > >   ./qemu -M q35 -device intel-iommu -device intel-iommu
>> > >
>> > > The error will change from:
>> > >
>> > >   qemu-system-x86_64: -device intel-iommu: QEMU does not support multiple vIOMMUs for x86 yet.
>> > >
>> > > To:
>> > >
>> > >   qemu-system-x86_64: -device intel-iommu: Class 'intel-iommu' only supports one instance
>> > >
>> > > Unfortunately, yet we can't remove the singleton check in the machine
>> > > hook (pc_machine_device_pre_plug_cb), because there can also be
>> > > virtio-iommu involved, which doesn't share a common parent class yet.
>> > >
>> > > But with this, it should be closer to reach that goal to check singleton by
>> > > QOM one day.
>> > >
>> > > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > 
>> > $ qemu-system-x86_64 -device amd-iommu,help
>> > /work/armbru/qemu/include/hw/boards.h:24:MACHINE: Object 0x56473906f960 is not an instance of type machine
>> > Aborted (core dumped)

[...]

>> Thanks for the report!
>> 
>> It turns out that qdev_get_machine() cannot be invoked too early, and the
>> singleton code can make it earlier..
>> 
>> We may want a pre-requisite patch to allow qdev_get_machine() to be invoked
>> anytime, like:
>> 
>> ===8<===
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index db36f54d91..7ceae47139 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -831,6 +831,16 @@ Object *qdev_get_machine(void)
>>  {
>>      static Object *dev;
>>  
>> +    if (!phase_check(PHASE_MACHINE_CREATED)) {
>> +        /*
>> +         * When the machine is not created, below can wrongly create
>> +         * /machine to be a container.. this enables qdev_get_machine() to
>> +         * be used at any time and return NULL properly when machine is not
>> +         * created.
>> +         */
>> +        return NULL;
>> +    }
>> +
>>      if (dev == NULL) {
>>          dev = container_get(object_get_root(), "/machine");
>>      }
>> ===8<===
>> 
>> I hope it makes sense on its own.
>
> My apologies, spoke too soon here.  This helper is used too after machine
> is created, but right before switching to PHASE_MACHINE_CREATE stage..

container_get() is a trap.

When the object to be gotten is always "container", it merely
complicates container creation: it's implicitly created on first get.
Which of the calls creates may be less than obvious.

When the object to be gotten is something else, such as a machine,
container_get() before creation is *wrong*, and will lead to trouble
later.

In my opinion:

* Hiding creation in getters is a bad idea unless creation has no
  material side effects.

* Getting anything but a container with container_get() is in bad taste.


> So we need another way, like:
>
> ===8<===
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index db36f54d91..36a9fdb428 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -832,7 +832,13 @@ Object *qdev_get_machine(void)
>      static Object *dev;
>  
>      if (dev == NULL) {
> -        dev = container_get(object_get_root(), "/machine");
> +        /*
> +         * NOTE: dev can keep being NULL if machine is not yet created!
> +         * In which case the function will properly return NULL.
> +         *
> +         * Whenever machine object is created and found once, we cache it.
> +         */
> +        dev = object_resolve_path_component(object_get_root(), "machine");
>      }
>  
>      return dev;

Now returns null instead of a bogus container when called before machine
creation.  Improvement of sorts.  But none of the callers expect null...
shouldn't we assert(dev) here?

Hmm, below you add a caller that checks for null.

Another nice mess.

> ===8<===
>
> The idea is still the same.  Meanwhile I'll test more to see whether it has
> other issues.
>
> Thanks,
>
>> Then callers who can be invoked earlier
>> could then handle NULL properly, in this case..
>> 
>> ===8<===
>> diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
>> index 4bfeb08705..fceb7adfe0 100644
>> --- a/hw/i386/x86-iommu.c
>> +++ b/hw/i386/x86-iommu.c
>> @@ -80,9 +80,15 @@ void x86_iommu_irq_to_msi_message(X86IOMMUIrq *irq, MSIMessage *msg_out)
>>  
>>  X86IOMMUState *x86_iommu_get_default(void)
>>  {
>> -    MachineState *ms = MACHINE(qdev_get_machine());
>> -    PCMachineState *pcms =
>> -        PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE));
>> +    Object *machine = qdev_get_machine();
>> +    PCMachineState *pcms;
>> +
>> +    /* If machine has not been created, so is the vIOMMU */
>> +    if (!machine) {
>> +        return NULL;
>> +    }
>> +
>> +    pcms = PC_MACHINE(object_dynamic_cast(machine, TYPE_PC_MACHINE));
>>  
>>      if (pcms &&
>>          object_dynamic_cast(OBJECT(pcms->iommu), TYPE_X86_IOMMU_DEVICE)) {
>> ===8<===
>> 
>> I'll make sure this works if I'll repost.
>> 
>> Thanks,
>> 
>> -- 
>> Peter Xu

Re: [PATCH 2/4] x86/iommu: Make x86-iommu a singleton object

[Peter Xu]

On Thu, Nov 07, 2024 at 12:12:10PM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Fri, Oct 25, 2024 at 05:55:59PM -0400, Peter Xu wrote:
> >> On Fri, Oct 25, 2024 at 11:25:23AM +0200, Markus Armbruster wrote:
> >> > Peter Xu <peterx@redhat.com> writes:
> >> > 
> >> > > X86 IOMMUs cannot be created more than one on a system yet.  Make it a
> >> > > singleton so it guards the system from accidentally create yet another
> >> > > IOMMU object when one already presents.
> >> > >
> >> > > Now if someone tries to create more than one, e.g., via:
> >> > >
> >> > >   ./qemu -M q35 -device intel-iommu -device intel-iommu
> >> > >
> >> > > The error will change from:
> >> > >
> >> > >   qemu-system-x86_64: -device intel-iommu: QEMU does not support multiple vIOMMUs for x86 yet.
> >> > >
> >> > > To:
> >> > >
> >> > >   qemu-system-x86_64: -device intel-iommu: Class 'intel-iommu' only supports one instance
> >> > >
> >> > > Unfortunately, yet we can't remove the singleton check in the machine
> >> > > hook (pc_machine_device_pre_plug_cb), because there can also be
> >> > > virtio-iommu involved, which doesn't share a common parent class yet.
> >> > >
> >> > > But with this, it should be closer to reach that goal to check singleton by
> >> > > QOM one day.
> >> > >
> >> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> > 
> >> > $ qemu-system-x86_64 -device amd-iommu,help
> >> > /work/armbru/qemu/include/hw/boards.h:24:MACHINE: Object 0x56473906f960 is not an instance of type machine
> >> > Aborted (core dumped)
> 
> [...]
> 
> >> Thanks for the report!
> >> 
> >> It turns out that qdev_get_machine() cannot be invoked too early, and the
> >> singleton code can make it earlier..
> >> 
> >> We may want a pre-requisite patch to allow qdev_get_machine() to be invoked
> >> anytime, like:
> >> 
> >> ===8<===
> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >> index db36f54d91..7ceae47139 100644
> >> --- a/hw/core/qdev.c
> >> +++ b/hw/core/qdev.c
> >> @@ -831,6 +831,16 @@ Object *qdev_get_machine(void)
> >>  {
> >>      static Object *dev;
> >>  
> >> +    if (!phase_check(PHASE_MACHINE_CREATED)) {
> >> +        /*
> >> +         * When the machine is not created, below can wrongly create
> >> +         * /machine to be a container.. this enables qdev_get_machine() to
> >> +         * be used at any time and return NULL properly when machine is not
> >> +         * created.
> >> +         */
> >> +        return NULL;
> >> +    }
> >> +
> >>      if (dev == NULL) {
> >>          dev = container_get(object_get_root(), "/machine");
> >>      }
> >> ===8<===
> >> 
> >> I hope it makes sense on its own.
> >
> > My apologies, spoke too soon here.  This helper is used too after machine
> > is created, but right before switching to PHASE_MACHINE_CREATE stage..
> 
> container_get() is a trap.

I had the same feeling..  Though I'd confess I'm not familiar enough with
this part of code.

> 
> When the object to be gotten is always "container", it merely
> complicates container creation: it's implicitly created on first get.
> Which of the calls creates may be less than obvious.
> 
> When the object to be gotten is something else, such as a machine,
> container_get() before creation is *wrong*, and will lead to trouble
> later.
> 
> In my opinion:
> 
> * Hiding creation in getters is a bad idea unless creation has no
>   material side effects.
> 
> * Getting anything but a container with container_get() is in bad taste.

Agreed.

IMHO container_get() interface might still be ok to implicitly create
containers, but only if it will: (1) always make sure what it walks is a
container along the way, and (2) never return any non-container.

> 
> 
> > So we need another way, like:
> >
> > ===8<===
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index db36f54d91..36a9fdb428 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -832,7 +832,13 @@ Object *qdev_get_machine(void)
> >      static Object *dev;
> >  
> >      if (dev == NULL) {
> > -        dev = container_get(object_get_root(), "/machine");
> > +        /*
> > +         * NOTE: dev can keep being NULL if machine is not yet created!
> > +         * In which case the function will properly return NULL.
> > +         *
> > +         * Whenever machine object is created and found once, we cache it.
> > +         */
> > +        dev = object_resolve_path_component(object_get_root(), "machine");
> >      }
> >  
> >      return dev;
> 
> Now returns null instead of a bogus container when called before machine
> creation.  Improvement of sorts.  But none of the callers expect null...
> shouldn't we assert(dev) here?
> 
> Hmm, below you add a caller that checks for null.
> 
> Another nice mess.

I plan to put aside the application of singletons to x86-iommu as of now,
due to the fact that qdev complexity may better be done separately.

IOW, before that, I wonder whether we should clean up the container_get()
as you discussed: it doesn't sound like a good interface to return
non-container objects.

I had a quick look, I only see two outliers of such, and besides the
"abuse" in qdev_get_machine(), the only other one is
e500_pcihost_bridge_realize():

*** hw/core/qdev.c:
qdev_get_machine[820]          dev = container_get(object_get_root(), "/machine");

*** hw/pci-host/ppce500.c:
e500_pcihost_bridge_realize[422] PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),

If any of us thinks this is the right way to go, I can try to clean it up
(for 10.0).  qdev_get_machine() may still need to be able to return NULL
when singleton applies to IOMMUs, but that can be for later.  Before that,
we can still assert(qdev), I think.

Just to mention I've posted rfcv2 for this series, again feel free to
ignore patch 3-5 as of now:

[PATCH RFC v2 0/7] QOM: Singleton interface
https://lore.kernel.org/r/20241029211607.2114845-1-peterx@redhat.com

I think the plan is Dan may keep collecting feedbacks on his other rfc:

[RFC 0/5] RFC: require error handling for dynamically created objects
https://lore.kernel.org/r/20241031155350.3240361-1-berrange@redhat.com

Then after Dan's lands, I'll rebase my rfcv2 on top of his, dropping
iommu/qdev changes.

Thanks,

-- 
Peter Xu

Re: [PATCH 2/4] x86/iommu: Make x86-iommu a singleton object

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Nov 07, 2024 at 12:12:10PM +0100, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Fri, Oct 25, 2024 at 05:55:59PM -0400, Peter Xu wrote:
>> >> On Fri, Oct 25, 2024 at 11:25:23AM +0200, Markus Armbruster wrote:
>> >> > Peter Xu <peterx@redhat.com> writes:
>> >> > 
>> >> > > X86 IOMMUs cannot be created more than one on a system yet.  Make it a
>> >> > > singleton so it guards the system from accidentally create yet another
>> >> > > IOMMU object when one already presents.
>> >> > >
>> >> > > Now if someone tries to create more than one, e.g., via:
>> >> > >
>> >> > >   ./qemu -M q35 -device intel-iommu -device intel-iommu
>> >> > >
>> >> > > The error will change from:
>> >> > >
>> >> > >   qemu-system-x86_64: -device intel-iommu: QEMU does not support multiple vIOMMUs for x86 yet.
>> >> > >
>> >> > > To:
>> >> > >
>> >> > >   qemu-system-x86_64: -device intel-iommu: Class 'intel-iommu' only supports one instance
>> >> > >
>> >> > > Unfortunately, yet we can't remove the singleton check in the machine
>> >> > > hook (pc_machine_device_pre_plug_cb), because there can also be
>> >> > > virtio-iommu involved, which doesn't share a common parent class yet.
>> >> > >
>> >> > > But with this, it should be closer to reach that goal to check singleton by
>> >> > > QOM one day.
>> >> > >
>> >> > > Signed-off-by: Peter Xu <peterx@redhat.com>
>> >> > 
>> >> > $ qemu-system-x86_64 -device amd-iommu,help
>> >> > /work/armbru/qemu/include/hw/boards.h:24:MACHINE: Object 0x56473906f960 is not an instance of type machine
>> >> > Aborted (core dumped)
>> 
>> [...]
>> 
>> >> Thanks for the report!
>> >> 
>> >> It turns out that qdev_get_machine() cannot be invoked too early, and the
>> >> singleton code can make it earlier..
>> >> 
>> >> We may want a pre-requisite patch to allow qdev_get_machine() to be invoked
>> >> anytime, like:
>> >> 
>> >> ===8<===
>> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> >> index db36f54d91..7ceae47139 100644
>> >> --- a/hw/core/qdev.c
>> >> +++ b/hw/core/qdev.c
>> >> @@ -831,6 +831,16 @@ Object *qdev_get_machine(void)
>> >>  {
>> >>      static Object *dev;
>> >>  
>> >> +    if (!phase_check(PHASE_MACHINE_CREATED)) {
>> >> +        /*
>> >> +         * When the machine is not created, below can wrongly create
>> >> +         * /machine to be a container.. this enables qdev_get_machine() to
>> >> +         * be used at any time and return NULL properly when machine is not
>> >> +         * created.
>> >> +         */
>> >> +        return NULL;
>> >> +    }
>> >> +
>> >>      if (dev == NULL) {
>> >>          dev = container_get(object_get_root(), "/machine");
>> >>      }
>> >> ===8<===
>> >> 
>> >> I hope it makes sense on its own.
>> >
>> > My apologies, spoke too soon here.  This helper is used too after machine
>> > is created, but right before switching to PHASE_MACHINE_CREATE stage..
>> 
>> container_get() is a trap.
>
> I had the same feeling..  Though I'd confess I'm not familiar enough with
> this part of code.
>
>> 
>> When the object to be gotten is always "container", it merely
>> complicates container creation: it's implicitly created on first get.
>> Which of the calls creates may be less than obvious.
>> 
>> When the object to be gotten is something else, such as a machine,
>> container_get() before creation is *wrong*, and will lead to trouble
>> later.
>> 
>> In my opinion:
>> 
>> * Hiding creation in getters is a bad idea unless creation has no
>>   material side effects.
>> 
>> * Getting anything but a container with container_get() is in bad taste.
>
> Agreed.
>
> IMHO container_get() interface might still be ok to implicitly create
> containers,

Creation on demand is fine when we want to create the thing only when
there is demand.

I guess it can also be okay when we want to create it always, but don't
want to decide when exactly (must be before first use), although I
suspect that's just lazy more often than not.

>             but only if it will: (1) always make sure what it walks is a
> container along the way, and (2) never return any non-container.

Yes.  Anything else invites abuse.

>> > So we need another way, like:
>> >
>> > ===8<===
>> >
>> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> > index db36f54d91..36a9fdb428 100644
>> > --- a/hw/core/qdev.c
>> > +++ b/hw/core/qdev.c
>> > @@ -832,7 +832,13 @@ Object *qdev_get_machine(void)
>> >      static Object *dev;
>> >  
>> >      if (dev == NULL) {
>> > -        dev = container_get(object_get_root(), "/machine");
>> > +        /*
>> > +         * NOTE: dev can keep being NULL if machine is not yet created!
>> > +         * In which case the function will properly return NULL.
>> > +         *
>> > +         * Whenever machine object is created and found once, we cache it.
>> > +         */
>> > +        dev = object_resolve_path_component(object_get_root(), "machine");
>> >      }
>> >  
>> >      return dev;
>> 
>> Now returns null instead of a bogus container when called before machine
>> creation.  Improvement of sorts.  But none of the callers expect null...
>> shouldn't we assert(dev) here?
>> 
>> Hmm, below you add a caller that checks for null.
>> 
>> Another nice mess.
>
> I plan to put aside the application of singletons to x86-iommu as of now,
> due to the fact that qdev complexity may better be done separately.
>
> IOW, before that, I wonder whether we should clean up the container_get()
> as you discussed: it doesn't sound like a good interface to return
> non-container objects.
>
> I had a quick look, I only see two outliers of such, and besides the
> "abuse" in qdev_get_machine(), the only other one is
> e500_pcihost_bridge_realize():
>
> *** hw/core/qdev.c:
> qdev_get_machine[820]          dev = container_get(object_get_root(), "/machine");
>
> *** hw/pci-host/ppce500.c:
> e500_pcihost_bridge_realize[422] PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
                                                                "/e500-ccsr"));

Yes, this abuses container_get() to get an "e500-ccsr", which is a
device, not a container.

By the way, intentation is confusing here.

> If any of us thinks this is the right way to go, I can try to clean it up
> (for 10.0).  qdev_get_machine() may still need to be able to return NULL
> when singleton applies to IOMMUs, but that can be for later.  Before that,
> we can still assert(qdev), I think.

I think it's worthwhile.

> Just to mention I've posted rfcv2 for this series, again feel free to
> ignore patch 3-5 as of now:
>
> [PATCH RFC v2 0/7] QOM: Singleton interface
> https://lore.kernel.org/r/20241029211607.2114845-1-peterx@redhat.com
>
> I think the plan is Dan may keep collecting feedbacks on his other rfc:
>
> [RFC 0/5] RFC: require error handling for dynamically created objects
> https://lore.kernel.org/r/20241031155350.3240361-1-berrange@redhat.com
>
> Then after Dan's lands, I'll rebase my rfcv2 on top of his, dropping
> iommu/qdev changes.
>
> Thanks,

Makes sense.  Thanks!

Re: [PATCH 0/4] QOM: Singleton interface

[Markus Armbruster]

Looked at this thread again to refresh my memory on the proposed
singleton interface, and found I have something to add.

Peter Xu <peterx@redhat.com> writes:

> On Tue, Oct 29, 2024 at 04:04:50PM +0000, Daniel P. Berrangé wrote:
>> I tend to feel that having MigrationState exist for the whole lifetime
>> of QEMU is a bug, forced on us by the unfortunate need to call
>> migrate-set-parameters/capabilities separately from the migrate
>> command, and by the need to query migrate info an arbitrary amount of
>> time after it finishes.
>> 
>> This puts libvirt in the awkward position of having to manually reset
>> all migration parameters, just to ensure earlier settings don't
>> accidentally affect a future migration operation :-( This is a design
>> that encourages mistakes.
>
> I think it would still be easy to add "cap" & "params" arguments support
> for the "migrate" QMP command without breaking the current API, iff that
> helps in whatever form.  When present, it simply applies the caps and/or
> param list first before invoking the migrate command, fail the command if
> cap / param check fails.
>
> But I'm not sure whether that's a concern at all for Libvirt, if what
> Libvirt currently does is having separate "migrate-set-*" commands prior to
> the "migrate".  I may have overlooked the real issue behind on how that
> could complicate Libvirt.

I think Daniel's point is that the interface's reliance on global state
makes it awkward to use.

Migration configuration is global state.  It's split into "capabilities"
and "parameters", but that's detail.  We have commands to query and
change this state.

When Libvirt connects to a QEMU process, it has no idea what the global
migration configuration is.  To get it into a known state, it has to set
*everything*.  It cannot rely on defaults.

It even has to set things it doesn't know!  When we add a new parameter
to QEMU, libvirt needs to be updated to reset it to its default even
when libvirt has no need for it.  When you use a version of libvirt that
lacks this update, it remains whatever it was.  The migration interface
becomes accidentally stateful at the libvirt level, which is
undesirable.

Compare this to the more modern interface we have for other long-running
tasks: jobs.

There is a job-specific command that creates the job: blockdev-backup,
block-commit, blockdev-mirror, block-stream, blockdev-create,
snapshot-save, snapshot-load, snapshot-delete, ...  Each command takes
the entire job configuration as arguments.  Libvirt does not need
updating for new parameters: these simply remain at their default
values.

Bonus: there are generic commands to control and monitor jobs:
job-pause, job-resume, job-cancel, job-complete, job-dismiss,
job-finalize, query-jobs.

[...]

Re: [PATCH 0/4] QOM: Singleton interface

[Peter Xu]

On Wed, Dec 11, 2024 at 09:19:32AM +0100, Markus Armbruster wrote:
> Looked at this thread again to refresh my memory on the proposed
> singleton interface, and found I have something to add.
> 
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Oct 29, 2024 at 04:04:50PM +0000, Daniel P. Berrangé wrote:
> >> I tend to feel that having MigrationState exist for the whole lifetime
> >> of QEMU is a bug, forced on us by the unfortunate need to call
> >> migrate-set-parameters/capabilities separately from the migrate
> >> command, and by the need to query migrate info an arbitrary amount of
> >> time after it finishes.
> >> 
> >> This puts libvirt in the awkward position of having to manually reset
> >> all migration parameters, just to ensure earlier settings don't
> >> accidentally affect a future migration operation :-( This is a design
> >> that encourages mistakes.
> >
> > I think it would still be easy to add "cap" & "params" arguments support
> > for the "migrate" QMP command without breaking the current API, iff that
> > helps in whatever form.  When present, it simply applies the caps and/or
> > param list first before invoking the migrate command, fail the command if
> > cap / param check fails.
> >
> > But I'm not sure whether that's a concern at all for Libvirt, if what
> > Libvirt currently does is having separate "migrate-set-*" commands prior to
> > the "migrate".  I may have overlooked the real issue behind on how that
> > could complicate Libvirt.
> 
> I think Daniel's point is that the interface's reliance on global state
> makes it awkward to use.
> 
> Migration configuration is global state.  It's split into "capabilities"
> and "parameters", but that's detail.  We have commands to query and
> change this state.
> 
> When Libvirt connects to a QEMU process, it has no idea what the global
> migration configuration is.  To get it into a known state, it has to set
> *everything*.  It cannot rely on defaults.
> 
> It even has to set things it doesn't know!  When we add a new parameter
> to QEMU, libvirt needs to be updated to reset it to its default even
> when libvirt has no need for it.  When you use a version of libvirt that
> lacks this update, it remains whatever it was.  The migration interface
> becomes accidentally stateful at the libvirt level, which is
> undesirable.
> 
> Compare this to the more modern interface we have for other long-running
> tasks: jobs.
> 
> There is a job-specific command that creates the job: blockdev-backup,
> block-commit, blockdev-mirror, block-stream, blockdev-create,
> snapshot-save, snapshot-load, snapshot-delete, ...  Each command takes
> the entire job configuration as arguments.  Libvirt does not need
> updating for new parameters: these simply remain at their default
> values.
> 
> Bonus: there are generic commands to control and monitor jobs:
> job-pause, job-resume, job-cancel, job-complete, job-dismiss,
> job-finalize, query-jobs.

Yes, migration is state-ful from that regard.  IMHO it is still ok because
unlike most jobs, migration task cannot have more than one.

Reusing jobs interface may work, but migation existed for so long a time
with its own APIs, I am not sure we'll get real benefit by reusing them.
At the meantime it may not 100% map to what migration wants (e.g.,
migrate_start_postcopy, postcopy recoveries, etc.).

OTOH, we definitely don't want to use the internal impl of jobs, because we
don't want to add either AIO or more coroutines into migration core - we
need to still use coroutine on dest QEMU but that's mostly only because of
legacy reasons.. and besides that and some very corner use case
(e.g. channel setups), migration is almost thread-based now.  A mixture of
threads and coroutines is too error prone and undebuggable, IMHO.

Going back to the "allow migrate QMP command take caps / parameters", we
still try to do that at some point.  But I recall we discussed about this
offlist (between Dan, probably Peter Krempa and myself), I believe the
conclusion is it'll make the API cleaner, but without no real benefit yet
so far.  Meanwhile there're some parameters that must be still stateful,
like a few max*-bandwidth or downtime_limit parameters.  They need to be
able to be changed on the fly, especially during migration task running.

Thanks,

-- 
Peter Xu