[PATCH 0/5] QOM: Enforce container_get() to operate on containers only

[PATCH 0/5] QOM: Enforce container_get() to operate on containers only

[Peter Xu]

[This is not for 9.2 release, but for 10.0]

QEMU defines a frequently used helper container_get(), which (from its name
implies) should return a container object of a specific path, normally
starting from object_get_root() (aka, the root of QOM tree, "/"), or some
sub-directory of root.

We mostly use it correctly across the tree, except two use cases that may
abuse the helper to be similar to object_resolve_path_component():

  - qdev_get_machine()
  - e500_pcihost_bridge_realize()

In the two cases, container_get() might accidentally create a container
even if the goal of the caller is trying to fetch some non-container
object.

This series cleans this up by firstly remove these two abuses, replacing
the container_get() usages with object_resolve_path_component().
Meanwhile, the last patch adds not only rich comment explaining
container_get() usages, but also added a lightweight assertion to make sure
container_get() will always walk on container objects, and always return
container objects.

There's another implicit side effect that container_get() may silently
create missing container objects while walking the path.  We could switch
to an explicit way of using containers in the future, but now leaving that
behavior as-is.

Note, patch 3,4 were picked up from the previous singleton series [1], even
if the vIOMMU patch wasn't present.  It's because they're still required in
this cleanup series to either clean up container_get() user, or to avoid
breaking test_static_prop_subprocess() similarly there.  Looks like that
test is so far the only test that can try to realize() a qdev without a
machine object.

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

Thanks,

Peter Xu (5):
  qom: Add TYPE_CONTAINER macro
  ppc/e500: Avoid abuse of container_get()
  qdev: Make device_set_realized() always safe in tests
  qdev: Make qdev_get_machine() not use container_get()
  qom: Make container_get() strict to always walk or return container

 include/qom/object.h  |  3 ++-
 hw/arm/stellaris.c    |  2 +-
 hw/core/qdev.c        | 20 +++++++++++++++++---
 hw/pci-host/ppce500.c |  4 ++--
 qom/container.c       | 25 +++++++++++++++++++++++--
 qom/object.c          |  4 ++--
 6 files changed, 47 insertions(+), 11 deletions(-)

-- 
2.45.0

[PATCH 1/5] qom: Add TYPE_CONTAINER macro

[Peter Xu]

Provide a macro for the container type across QEMU source tree, rather than
hard code it every time.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qom/object.h | 3 ++-
 hw/arm/stellaris.c   | 2 +-
 qom/container.c      | 4 ++--
 qom/object.c         | 4 ++--
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 43c135984a..8162a1ef17 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -25,7 +25,8 @@ typedef struct TypeInfo TypeInfo;
 typedef struct InterfaceClass InterfaceClass;
 typedef struct InterfaceInfo InterfaceInfo;
 
-#define TYPE_OBJECT "object"
+#define  TYPE_OBJECT                 "object"
+#define  TYPE_CONTAINER              "container"
 
 typedef struct ObjectProperty ObjectProperty;
 
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 376746251e..6d518b9cde 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1053,7 +1053,7 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
     flash_size = (((board->dc0 & 0xffff) + 1) << 1) * 1024;
     sram_size = ((board->dc0 >> 18) + 1) * 1024;
 
-    soc_container = object_new("container");
+    soc_container = object_new(TYPE_CONTAINER);
     object_property_add_child(OBJECT(ms), "soc", soc_container);
 
     /* Flash programming is done via the SCU, so pretend it is ROM.  */
diff --git a/qom/container.c b/qom/container.c
index 455e8410c6..cfec92a944 100644
--- a/qom/container.c
+++ b/qom/container.c
@@ -15,7 +15,7 @@
 #include "qemu/module.h"
 
 static const TypeInfo container_info = {
-    .name          = "container",
+    .name          = TYPE_CONTAINER,
     .parent        = TYPE_OBJECT,
 };
 
@@ -37,7 +37,7 @@ Object *container_get(Object *root, const char *path)
     for (i = 1; parts[i] != NULL; i++, obj = child) {
         child = object_resolve_path_component(obj, parts[i]);
         if (!child) {
-            child = object_new("container");
+            child = object_new(TYPE_CONTAINER);
             object_property_add_child(obj, parts[i], child);
             object_unref(child);
         }
diff --git a/qom/object.c b/qom/object.c
index 9edc06d391..214d6eb4c1 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1739,7 +1739,7 @@ Object *object_get_root(void)
     static Object *root;
 
     if (!root) {
-        root = object_new("container");
+        root = object_new(TYPE_CONTAINER);
     }
 
     return root;
@@ -1755,7 +1755,7 @@ Object *object_get_internal_root(void)
     static Object *internal_root;
 
     if (!internal_root) {
-        internal_root = object_new("container");
+        internal_root = object_new(TYPE_CONTAINER);
     }
 
     return internal_root;
-- 
2.45.0

Re: [PATCH 1/5] qom: Add TYPE_CONTAINER macro

[Daniel P. Berrangé]

On Mon, Nov 18, 2024 at 05:13:26PM -0500, Peter Xu wrote:
> Provide a macro for the container type across QEMU source tree, rather than
> hard code it every time.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qom/object.h | 3 ++-
>  hw/arm/stellaris.c   | 2 +-
>  qom/container.c      | 4 ++--
>  qom/object.c         | 4 ++--
>  4 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 43c135984a..8162a1ef17 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -25,7 +25,8 @@ typedef struct TypeInfo TypeInfo;
>  typedef struct InterfaceClass InterfaceClass;
>  typedef struct InterfaceInfo InterfaceInfo;
>  
> -#define TYPE_OBJECT "object"
> +#define  TYPE_OBJECT                 "object"
> +#define  TYPE_CONTAINER              "container"

nitpick - 1 space too many before "TYPE_", and it is not worth
trying to vertically align "object" and "container", just leave
the existing line for TYPE_OBJECT unchanged.



>  
>  typedef struct ObjectProperty ObjectProperty;
>  
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 376746251e..6d518b9cde 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -1053,7 +1053,7 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>      flash_size = (((board->dc0 & 0xffff) + 1) << 1) * 1024;
>      sram_size = ((board->dc0 >> 18) + 1) * 1024;
>  
> -    soc_container = object_new("container");
> +    soc_container = object_new(TYPE_CONTAINER);
>      object_property_add_child(OBJECT(ms), "soc", soc_container);
>  
>      /* Flash programming is done via the SCU, so pretend it is ROM.  */
> diff --git a/qom/container.c b/qom/container.c
> index 455e8410c6..cfec92a944 100644
> --- a/qom/container.c
> +++ b/qom/container.c
> @@ -15,7 +15,7 @@
>  #include "qemu/module.h"
>  
>  static const TypeInfo container_info = {
> -    .name          = "container",
> +    .name          = TYPE_CONTAINER,
>      .parent        = TYPE_OBJECT,
>  };
>  
> @@ -37,7 +37,7 @@ Object *container_get(Object *root, const char *path)
>      for (i = 1; parts[i] != NULL; i++, obj = child) {
>          child = object_resolve_path_component(obj, parts[i]);
>          if (!child) {
> -            child = object_new("container");
> +            child = object_new(TYPE_CONTAINER);
>              object_property_add_child(obj, parts[i], child);
>              object_unref(child);
>          }
> diff --git a/qom/object.c b/qom/object.c
> index 9edc06d391..214d6eb4c1 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1739,7 +1739,7 @@ Object *object_get_root(void)
>      static Object *root;
>  
>      if (!root) {
> -        root = object_new("container");
> +        root = object_new(TYPE_CONTAINER);
>      }
>  
>      return root;
> @@ -1755,7 +1755,7 @@ Object *object_get_internal_root(void)
>      static Object *internal_root;
>  
>      if (!internal_root) {
> -        internal_root = object_new("container");
> +        internal_root = object_new(TYPE_CONTAINER);
>      }
>  
>      return internal_root;
> -- 
> 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 1/5] qom: Add TYPE_CONTAINER macro

[Peter Xu]

On Tue, Nov 19, 2024 at 09:42:45AM +0000, Daniel P. Berrangé wrote:
> > -#define TYPE_OBJECT "object"
> > +#define  TYPE_OBJECT                 "object"
> > +#define  TYPE_CONTAINER              "container"
> 
> nitpick - 1 space too many before "TYPE_", and it is not worth
> trying to vertically align "object" and "container", just leave
> the existing line for TYPE_OBJECT unchanged.

Sure.

-- 
Peter Xu

[PATCH 2/5] ppc/e500: Avoid abuse of container_get()

[Peter Xu]

container_get() is going to become strict on not allowing to return a
non-container.

Switch the e500 user to use object_resolve_path_component() explicitly.

Cc: Bharat Bhushan <r65777@freescale.com>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/pci-host/ppce500.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index b70631045a..65233b9e3f 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -418,8 +418,8 @@ static const VMStateDescription vmstate_ppce500_pci = {
 static void e500_pcihost_bridge_realize(PCIDevice *d, Error **errp)
 {
     PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
-    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
-                                  "/e500-ccsr"));
+    PPCE500CCSRState *ccsr = CCSR(
+        object_resolve_path_component(qdev_get_machine(), "e500-ccsr"));
 
     memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0", &ccsr->ccsr_space,
                              0, int128_get64(ccsr->ccsr_space.size));
-- 
2.45.0

[PATCH 3/5] qdev: Make device_set_realized() always safe in tests

[Peter Xu]

Currently, a device can be realized even before machine is created, but
only in one of QEMU's qtest, test-global-qdev-props.c.

Right now, the test_static_prop_subprocess() test (which creates one simple
object without machine created) will internally make "/machine" to be a
container, which may not be expected when developing the test.

Now explicitly support that case when there's no real "/machine" object
around, then unattached devices will be put under root ("/") rather than
"/machine".  Mostly only for this single test case, or for any future test
cases when some device needs to be realized before the machine is present.

This shouldn't affect anything else when QEMU runs as an emulator, as that
always relies on a real machine being created before realizing any devices.
It's because if "/machine" is wrongly created as a container, it'll fail
QEMU very soon later on qemu_create_machine() trying to create the real
machine, conflicting with the "/machine" container.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/qdev.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 5f13111b77..eff297e584 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -475,9 +475,17 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
 
         if (!obj->parent) {
             gchar *name = g_strdup_printf("device[%d]", unattached_count++);
+            Object *root = qdev_get_machine();
 
-            object_property_add_child(container_get(qdev_get_machine(),
-                                                    "/unattached"),
+            /*
+             * We could have qdev test cases trying to realize() a device
+             * without machine created.  In that case we use the root.
+             */
+            if (!root) {
+                root = object_get_root();
+            }
+
+            object_property_add_child(container_get(root, "/unattached"),
                                       name, obj);
             unattached_parent = true;
             g_free(name);
-- 
2.45.0

Re: [PATCH 3/5] qdev: Make device_set_realized() always safe in tests

[Daniel P. Berrangé]

On Mon, Nov 18, 2024 at 05:13:28PM -0500, Peter Xu wrote:
> Currently, a device can be realized even before machine is created, but
> only in one of QEMU's qtest, test-global-qdev-props.c.
> 
> Right now, the test_static_prop_subprocess() test (which creates one simple
> object without machine created) will internally make "/machine" to be a
> container, which may not be expected when developing the test.
> 
> Now explicitly support that case when there's no real "/machine" object
> around, then unattached devices will be put under root ("/") rather than
> "/machine".  Mostly only for this single test case, or for any future test
> cases when some device needs to be realized before the machine is present.
> 
> This shouldn't affect anything else when QEMU runs as an emulator, as that
> always relies on a real machine being created before realizing any devices.
> It's because if "/machine" is wrongly created as a container, it'll fail
> QEMU very soon later on qemu_create_machine() trying to create the real
> machine, conflicting with the "/machine" container.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/core/qdev.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 5f13111b77..eff297e584 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -475,9 +475,17 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>  
>          if (!obj->parent) {
>              gchar *name = g_strdup_printf("device[%d]", unattached_count++);
> +            Object *root = qdev_get_machine();
>  
> -            object_property_add_child(container_get(qdev_get_machine(),
> -                                                    "/unattached"),
> +            /*
> +             * We could have qdev test cases trying to realize() a device
> +             * without machine created.  In that case we use the root.
> +             */
> +            if (!root) {
> +                root = object_get_root();
> +            }

IMHO modifying the qdev.c code to workaround limitations of the test suite
is not a nice approach. Even if it is more work, I'd say it is better to
properly stub a /machine object in the test case, so that it complies with
expectations of qdev.c

> +
> +            object_property_add_child(container_get(root, "/unattached"),
>                                        name, obj);
>              unattached_parent = true;
>              g_free(name);

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 3/5] qdev: Make device_set_realized() always safe in tests

[Peter Xu]

On Tue, Nov 19, 2024 at 09:46:35AM +0000, Daniel P. Berrangé wrote:
> On Mon, Nov 18, 2024 at 05:13:28PM -0500, Peter Xu wrote:
> > Currently, a device can be realized even before machine is created, but
> > only in one of QEMU's qtest, test-global-qdev-props.c.
> > 
> > Right now, the test_static_prop_subprocess() test (which creates one simple
> > object without machine created) will internally make "/machine" to be a
> > container, which may not be expected when developing the test.
> > 
> > Now explicitly support that case when there's no real "/machine" object
> > around, then unattached devices will be put under root ("/") rather than
> > "/machine".  Mostly only for this single test case, or for any future test
> > cases when some device needs to be realized before the machine is present.
> > 
> > This shouldn't affect anything else when QEMU runs as an emulator, as that
> > always relies on a real machine being created before realizing any devices.
> > It's because if "/machine" is wrongly created as a container, it'll fail
> > QEMU very soon later on qemu_create_machine() trying to create the real
> > machine, conflicting with the "/machine" container.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/core/qdev.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 5f13111b77..eff297e584 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -475,9 +475,17 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> >  
> >          if (!obj->parent) {
> >              gchar *name = g_strdup_printf("device[%d]", unattached_count++);
> > +            Object *root = qdev_get_machine();
> >  
> > -            object_property_add_child(container_get(qdev_get_machine(),
> > -                                                    "/unattached"),
> > +            /*
> > +             * We could have qdev test cases trying to realize() a device
> > +             * without machine created.  In that case we use the root.
> > +             */
> > +            if (!root) {
> > +                root = object_get_root();
> > +            }
> 
> IMHO modifying the qdev.c code to workaround limitations of the test suite
> is not a nice approach. Even if it is more work, I'd say it is better to
> properly stub a /machine object in the test case, so that it complies with
> expectations of qdev.c

Yeah I can give it a shot.

Meanwhile I just noticed that the assertion I added in the last patch may
be too strict, considering that "/machine" is actually not a container
object itself..  I think no test crashed because all such users used
qdev_get_machine() as the 1st parameter to container_get() to start the
walk, then the container_get() won't walk the "/machine" object itself, but
anything afterwards.

I still think it's possible some other objects got to be used as a
container even if it's not TYPE_CONTAINER, like the machine object.

So maybe.. what we really want is not "assert everything is a container",
as fundamentally every object "can" be a container itself.. supporting
childs in the properties.  What we really need might be that we never try
to silently create containers where it shouldn't..

I'll need to rethink about the series a bit.

-- 
Peter Xu

[PATCH 4/5] qdev: Make qdev_get_machine() not use container_get()

[Peter Xu]

Currently, qdev_get_machine() has a slightly misuse of container_get(), as
the helper says "get a container" but in reality the goal is to get the
machine object.

Note that it _may_ get a container (at "/machine") in our current unit test
of test-qdev-global-props.c, but it's probably unexpected and worked by
accident..

Switch to an explicit object_resolve_path_component(), with a side benefit
that qdev_get_machine() can happen a lot, and we don't need to split the
string ("/machine") every time.  This also paves way for making the helper
container_get() never try to return a non-container at all.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/qdev.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index eff297e584..e828088c58 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -825,7 +825,13 @@ Object *qdev_get_machine(void)
     static Object *dev;
 
     if (dev == NULL) {
-        dev = container_get(object_get_root(), "/machine");
+        /*
+         * NOTE: when the machine is not yet created, this helper will
+         * also keep the cached object untouched and return NULL.  The next
+         * invoke of the helper will try to look for the machine again.
+         * It'll only cache the pointer when it's found the first time.
+         */
+        dev = object_resolve_path_component(object_get_root(), "machine");
     }
 
     return dev;
-- 
2.45.0

[PATCH 5/5] qom: Make container_get() strict to always walk or return container

[Peter Xu]

When used incorrectly, container_get() can silently create containers even
if the caller may not intend to do so.  Add a rich document describing the
helper, as container_get() should only be used in path lookups.

Add one object_dynamic_cast() check to make sure whatever objects the
helper walks will be a container object (including the one to be returned).
It is a programming error otherwise, hence assert that.

It may make container_get() tiny slower than before, but the hope is the
change is neglictable, as object_class_dynamic_cast() has a fast path just
for similar leaf use case.

Link: https://lore.kernel.org/r/87pln6ds8q.fsf@pond.sub.org
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 qom/container.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/qom/container.c b/qom/container.c
index cfec92a944..ff6e35f837 100644
--- a/qom/container.c
+++ b/qom/container.c
@@ -24,6 +24,20 @@ static void container_register_types(void)
     type_register_static(&container_info);
 }
 
+/**
+ * container_get(): Get the container object under specific path
+ *
+ * @root: The root path object to start walking from.  When starting from
+ *        root, one can pass in object_get_root().
+ * @path: The sub-path to lookup, must be an non-empty string starts with "/".
+ *
+ * Returns: The container object specified by @path.
+ *
+ * NOTE: the function may impplicitly create internal containers when the
+ * whole path is not yet created.  It's the caller's responsibility to make
+ * sure the path specified is always used as object containers, rather than
+ * any other type of objects.
+ */
 Object *container_get(Object *root, const char *path)
 {
     Object *obj, *child;
@@ -31,6 +45,7 @@ Object *container_get(Object *root, const char *path)
     int i;
 
     parts = g_strsplit(path, "/", 0);
+    /* "path" must be an non-empty string starting with "/" */
     assert(parts != NULL && parts[0] != NULL && !parts[0][0]);
     obj = root;
 
@@ -40,6 +55,12 @@ Object *container_get(Object *root, const char *path)
             child = object_new(TYPE_CONTAINER);
             object_property_add_child(obj, parts[i], child);
             object_unref(child);
+        } else {
+            /*
+             * Each object within the path must be a container object
+             * itself, including the object to be returned.
+             */
+            assert(object_dynamic_cast(child, TYPE_CONTAINER));
         }
     }
 
-- 
2.45.0

Re: [PATCH 5/5] qom: Make container_get() strict to always walk or return container

[Peter Xu]

On Mon, Nov 18, 2024 at 05:13:30PM -0500, Peter Xu wrote:
> When used incorrectly, container_get() can silently create containers even
> if the caller may not intend to do so.  Add a rich document describing the
> helper, as container_get() should only be used in path lookups.
> 
> Add one object_dynamic_cast() check to make sure whatever objects the
> helper walks will be a container object (including the one to be returned).
> It is a programming error otherwise, hence assert that.
> 
> It may make container_get() tiny slower than before, but the hope is the
> change is neglictable, as object_class_dynamic_cast() has a fast path just
> for similar leaf use case.

Just a heads up: out of curiosity, I tried to see whether the fast path hit
that I mentioned here (mostly, commit 793c96b54032 of Paolo's), and it
didn't..

It's fundamentally because all TypeImpl was allocated dynamically from
heap, including its type->name.  While typename should normally be const
strings that locates on RODATA sections, hence they should mostly never
hit when compare with pointers..

I was thinking whether we could add a strcmp() there too for the fast path,
but then I noticed that QEMU could have some pretty long type->name... so
that strcmp() idea may not be good if that's the case. E.g.:

  virtio-net-pci-non-transitional::conventional-pci-device

Which has 55 chars..

I don't have good idea to make the fast path hit here, so I'll at least
remove this paragraph if I'm going to repost.. I hope it's not a huge deal
to still do the sanity check here, as the container type is so special and
small, so that check should be fast regardless.

> 
> Link: https://lore.kernel.org/r/87pln6ds8q.fsf@pond.sub.org
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qom/container.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/qom/container.c b/qom/container.c
> index cfec92a944..ff6e35f837 100644
> --- a/qom/container.c
> +++ b/qom/container.c
> @@ -24,6 +24,20 @@ static void container_register_types(void)
>      type_register_static(&container_info);
>  }
>  
> +/**
> + * container_get(): Get the container object under specific path
> + *
> + * @root: The root path object to start walking from.  When starting from
> + *        root, one can pass in object_get_root().
> + * @path: The sub-path to lookup, must be an non-empty string starts with "/".
> + *
> + * Returns: The container object specified by @path.
> + *
> + * NOTE: the function may impplicitly create internal containers when the
> + * whole path is not yet created.  It's the caller's responsibility to make
> + * sure the path specified is always used as object containers, rather than
> + * any other type of objects.
> + */
>  Object *container_get(Object *root, const char *path)
>  {
>      Object *obj, *child;
> @@ -31,6 +45,7 @@ Object *container_get(Object *root, const char *path)
>      int i;
>  
>      parts = g_strsplit(path, "/", 0);
> +    /* "path" must be an non-empty string starting with "/" */
>      assert(parts != NULL && parts[0] != NULL && !parts[0][0]);
>      obj = root;
>  
> @@ -40,6 +55,12 @@ Object *container_get(Object *root, const char *path)
>              child = object_new(TYPE_CONTAINER);
>              object_property_add_child(obj, parts[i], child);
>              object_unref(child);
> +        } else {
> +            /*
> +             * Each object within the path must be a container object
> +             * itself, including the object to be returned.
> +             */
> +            assert(object_dynamic_cast(child, TYPE_CONTAINER));
>          }
>      }
>  
> -- 
> 2.45.0
> 

-- 
Peter Xu

Re: [PATCH 5/5] qom: Make container_get() strict to always walk or return container

[Paolo Bonzini]

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

Il mar 19 nov 2024, 00:06 Peter Xu <peterx@redhat.com> ha scritto:

> On Mon, Nov 18, 2024 at 05:13:30PM -0500, Peter Xu wrote:
> > When used incorrectly, container_get() can silently create containers
> even
> > if the caller may not intend to do so.  Add a rich document describing
> the
> > helper, as container_get() should only be used in path lookups.
> >
> > Add one object_dynamic_cast() check to make sure whatever objects the
> > helper walks will be a container object (including the one to be
> returned).
> > It is a programming error otherwise, hence assert that.
> >
> > It may make container_get() tiny slower than before, but the hope is the
> > change is neglictable, as object_class_dynamic_cast() has a fast path
> just
> > for similar leaf use case.
>
> Just a heads up: out of curiosity, I tried to see whether the fast path hit
> that I mentioned here (mostly, commit 793c96b54032 of Paolo's), and it
> didn't..
>
> It's fundamentally because all TypeImpl was allocated dynamically from
> heap, including its type->name.


Ah, that was supposed to be the difference between type_register() and
type_register_static().

Perhaps type->name could be allocated with g_intern_string()? And then if
object_dynamic_cast() is changed into a macro, with something like

#define qemu_cache_interned_string(s) \
  (__builtin_constant_p(s) \
   ? ({ static const char *interned; \
        interned = interned ?: g_intern_static_string(s); }) \
   : g_intern_string(s))

as the third parameter. This allows object_dynamic_cast() to use a simple
pointer equality for type name comparison, and the same can be applied to
object_class_dynamic_cast().

Whatever we do, we should do it before Rust code starts using
object_dynamic_cast!

Paolo

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

Re: [PATCH 5/5] qom: Make container_get() strict to always walk or return container

[Peter Xu]

On Tue, Nov 19, 2024 at 09:09:16AM +0100, Paolo Bonzini wrote:
> Il mar 19 nov 2024, 00:06 Peter Xu <peterx@redhat.com> ha scritto:
> 
> > On Mon, Nov 18, 2024 at 05:13:30PM -0500, Peter Xu wrote:
> > > When used incorrectly, container_get() can silently create containers
> > even
> > > if the caller may not intend to do so.  Add a rich document describing
> > the
> > > helper, as container_get() should only be used in path lookups.
> > >
> > > Add one object_dynamic_cast() check to make sure whatever objects the
> > > helper walks will be a container object (including the one to be
> > returned).
> > > It is a programming error otherwise, hence assert that.
> > >
> > > It may make container_get() tiny slower than before, but the hope is the
> > > change is neglictable, as object_class_dynamic_cast() has a fast path
> > just
> > > for similar leaf use case.
> >
> > Just a heads up: out of curiosity, I tried to see whether the fast path hit
> > that I mentioned here (mostly, commit 793c96b54032 of Paolo's), and it
> > didn't..
> >
> > It's fundamentally because all TypeImpl was allocated dynamically from
> > heap, including its type->name.
> 
> 
> Ah, that was supposed to be the difference between type_register() and
> type_register_static().

Ah... looks like they're the same now? As type_register_static() looks like
a wrapper of type_register().

I gave it a shot on booting a Linux guest with some pretty generic devices,
and see how much the pointer check hit.  Until I got the root login prompt,
I got 8 hits out of 35488.  So it's indeed hard to yet hit.. at least with
the current code base. :(

> 
> Perhaps type->name could be allocated with g_intern_string()? And then if
> object_dynamic_cast() is changed into a macro, with something like
> 
> #define qemu_cache_interned_string(s) \
>   (__builtin_constant_p(s) \
>    ? ({ static const char *interned; \
>         interned = interned ?: g_intern_static_string(s); }) \
>    : g_intern_string(s))
> 
> as the third parameter. This allows object_dynamic_cast() to use a simple
> pointer equality for type name comparison, and the same can be applied to
> object_class_dynamic_cast().

Interesting to know this facility!  Though, IIUC this may:

  - For builtin-consts, it grows 8 bytes for each call sites on the binary
    generated, even if (I think...) most of the sites are slow paths, and
    there're plenty of such calls..

  - For non-builtin strings, g_intern_string() will add one more hash
    operation for the whole string (and per discussed previously, looks
    like the string can be not always short..).

So I'm not 100% sure yet if this is what we want.

Do we have known places that we care a lot on object[_class]_dynamic_cast()
performance?  I can give it some measurement if there is, otherwise I'm
guessing whatever changes could fall into the noise.. then we can also
leave that for later, knowing that the fast path will hardly hit for now,
but that shouldn't be a major issue either, I assume.

> 
> Whatever we do, we should do it before Rust code starts using
> object_dynamic_cast!

Thanks!

-- 
Peter Xu

Re: [PATCH 5/5] qom: Make container_get() strict to always walk or return container

[Paolo Bonzini]

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

Il mar 19 nov 2024, 21:07 Peter Xu <peterx@redhat.com> ha scritto:

> > Ah, that was supposed to be the difference between type_register() and
> > type_register_static().
>
> Ah... looks like they're the same now? As type_register_static() looks like
> a wrapper of type_register().
>

And pretty much have always been... Zhao looked into it recently.

> Perhaps type->name could be allocated with g_intern_string()? And then if
> > object_dynamic_cast() is changed into a macro, with something like
> >
> > #define qemu_cache_interned_string(s) \
> >   (__builtin_constant_p(s) \
> >    ? ({ static const char *interned; \
> >         interned = interned ?: g_intern_static_string(s); }) \
> >    : g_intern_string(s))
> >
> > as the third parameter. This allows object_dynamic_cast() to use a simple
> > pointer equality for type name comparison, and the same can be applied to
> > object_class_dynamic_cast().
>
> Interesting to know this facility!  Though, IIUC this may:
>
>   - For builtin-consts, it grows 8 bytes for each call sites on the binary
>     generated, even if (I think...) most of the sites are slow paths, and
>     there're plenty of such calls..
>

Right, but all the fast paths will be here. Only a few will be of course.

  - For non-builtin strings, g_intern_string() will add one more hash
>     operation for the whole string (and per discussed previously, looks
>     like the string can be not always short..).
>

Yes, but non-const strings should be *really* rare and not fast paths.

So I'm not 100% sure yet if this is what we want.
>
> Do we have known places that we care a lot on object[_class]_dynamic_cast()
> performance?


The easiest way to check is probably to print the type of every successful
object_dynamic_cast and object_class_dynamic_cast. I suspect the result
will be virtio-blk-device and/or scsi-hd, but maybe those already do an
unsafe cast (pointer type cast) instead of object_dynamic_cast.

I can give it some measurement if there is, otherwise I'm
> guessing whatever changes could fall into the noise.


Yes, probably. At most you can identify if there any heavy places out of
the 34000 calls, and see if they can use an unsafe cast.

Paolo

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

Re: [PATCH 5/5] qom: Make container_get() strict to always walk or return container

[Peter Xu]

On Tue, Nov 19, 2024 at 09:30:09PM +0100, Paolo Bonzini wrote:
> >
> > Do we have known places that we care a lot on object[_class]_dynamic_cast()
> > performance?
> 
> The easiest way to check is probably to print the type of every successful
> object_dynamic_cast and object_class_dynamic_cast. I suspect the result
> will be virtio-blk-device and/or scsi-hd, but maybe those already do an
> unsafe cast (pointer type cast) instead of object_dynamic_cast.

Yes, it sounds more reasonable to me to optimize specific call sites so far
rather than provides something generic..  Though it could still be a
generic API so that devices can opt-in.  Maybe still not as fast as an
unsafe cast, though.. I think I'll leave that to block experts when it may
needs some good measurements.

> 
> I can give it some measurement if there is, otherwise I'm
> > guessing whatever changes could fall into the noise.
> 
> 
> Yes, probably. At most you can identify if there any heavy places out of
> the 34000 calls, and see if they can use an unsafe cast.

I can still trivially do this.

I traced qemu using bpf and interestingly in my case close to half (over
10000+) of the calls are about ahci_irq_lower() from different higher level
stack (yeah I used IDE in my setup.. with a split irqchi..), where it has:

    PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state),
                                                           TYPE_PCI_DEVICE);

So IIUC that can be open to a unsafe cast too, but considering IDE is ODD
FIXES stage, I'm not sure if I should send a patch at all.  However I
copied John regardless.

Thanks,

-- 
Peter Xu

Re: [PATCH 5/5] qom: Make container_get() strict to always walk or return container

[Paolo Bonzini]

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

Il mar 19 nov 2024, 22:43 Peter Xu <peterx@redhat.com> ha scritto:

> > The easiest way to check is probably to print the type of every
> successful
> > object_dynamic_cast and object_class_dynamic_cast. I suspect the result
> > will be virtio-blk-device and/or scsi-hd, but maybe those already do an
> > unsafe cast (pointer type cast) instead of object_dynamic_cast.
>
> Yes, it sounds more reasonable to me to optimize specific call sites so far
> rather than provides something generic.

Though it could still be a
> generic API so that devices can opt-in.


One of the things that I am excited about for Rust is checking at compile
time whether a cast is to a superclass, which makes it safe automatically.

> I can give it some measurement if there is, otherwise I'm
> > > guessing whatever changes could fall into the noise.
> >
> >
> > Yes, probably. At most you can identify if there any heavy places out of
> > the 34000 calls, and see if they can use an unsafe cast.
>
> I can still trivially do this.
>
> I traced qemu using bpf


Nice! I want to know more. :))
A

> and interestingly in my case close to half (over
> 10000+) of the calls are about ahci_irq_lower() from different higher level
> stack (yeah I used IDE in my setup.. with a split irqchi..), where it has:
>
>     PCIDevice *pci_dev = (PCIDevice *)
> object_dynamic_cast(OBJECT(dev_state),
>
>  TYPE_PCI_DEVICE);
>
> So IIUC that can be open to a unsafe cast too


Hmm no it can't because there's also sysbus AHCI. The fix would be to add
an AHCIClass and make irq toggling into a method there

but considering IDE is ODD FIXES stage, I'm not sure if I should send a
> patch at all.  However I copied John regardless.
>

Well, MAINTAINERS only says the kind of work that the maintainer is doing,
you can always do more. However it seems like not a small amount, so maybe
adding a comment is enough if somebody else wants to do it?

Paolo


> Thanks,
>
> --
> Peter Xu
>
>

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

Re: [PATCH 5/5] qom: Make container_get() strict to always walk or return container

[Peter Xu]

On Wed, Nov 20, 2024 at 12:45:19PM +0100, Paolo Bonzini wrote:
> Il mar 19 nov 2024, 22:43 Peter Xu <peterx@redhat.com> ha scritto:
> 
> > > The easiest way to check is probably to print the type of every
> > successful
> > > object_dynamic_cast and object_class_dynamic_cast. I suspect the result
> > > will be virtio-blk-device and/or scsi-hd, but maybe those already do an
> > > unsafe cast (pointer type cast) instead of object_dynamic_cast.
> >
> > Yes, it sounds more reasonable to me to optimize specific call sites so far
> > rather than provides something generic.
> 
> Though it could still be a
> > generic API so that devices can opt-in.
> 
> 
> One of the things that I am excited about for Rust is checking at compile
> time whether a cast is to a superclass, which makes it safe automatically.

I see.  However looks like it doesn't easily apply to the ahci example
below, where it could conditionally fail the cast (and where I got it
wrong..)?

> 
> > I can give it some measurement if there is, otherwise I'm
> > > > guessing whatever changes could fall into the noise.
> > >
> > >
> > > Yes, probably. At most you can identify if there any heavy places out of
> > > the 34000 calls, and see if they can use an unsafe cast.
> >
> > I can still trivially do this.
> >
> > I traced qemu using bpf
> 
> 
> Nice! I want to know more. :))

I also only learned it yesterday, where I only used to use k*probes
previously. :-) That's:

$ cat qemu.bpf
uprobe:/home/peterx/git/qemu/bin/qemu-system-x86_64:object_class_dynamic_cast
{
        @out[ustack()]++;
}
$ sudo bpftrace --usdt-file-activation ./qemu.bpf

> 
> > and interestingly in my case close to half (over
> > 10000+) of the calls are about ahci_irq_lower() from different higher level
> > stack (yeah I used IDE in my setup.. with a split irqchi..), where it has:
> >
> >     PCIDevice *pci_dev = (PCIDevice *)
> > object_dynamic_cast(OBJECT(dev_state),
> >
> >  TYPE_PCI_DEVICE);
> >
> > So IIUC that can be open to a unsafe cast too
> 
> 
> Hmm no it can't because there's also sysbus AHCI. The fix would be to add
> an AHCIClass and make irq toggling into a method there

Yep, I overlooked the lines of code later.. :(

> 
> but considering IDE is ODD FIXES stage, I'm not sure if I should send a
> > patch at all.  However I copied John regardless.
> >
> 
> Well, MAINTAINERS only says the kind of work that the maintainer is doing,
> you can always do more. However it seems like not a small amount, so maybe
> adding a comment is enough if somebody else wants to do it?

Can do.

-- 
Peter Xu

Re: [PATCH 5/5] qom: Make container_get() strict to always walk or return container

[Daniel P. Berrangé]

On Mon, Nov 18, 2024 at 05:13:30PM -0500, Peter Xu wrote:
> When used incorrectly, container_get() can silently create containers even
> if the caller may not intend to do so.  Add a rich document describing the
> helper, as container_get() should only be used in path lookups.
> 
> Add one object_dynamic_cast() check to make sure whatever objects the
> helper walks will be a container object (including the one to be returned).
> It is a programming error otherwise, hence assert that.
> 
> It may make container_get() tiny slower than before, but the hope is the
> change is neglictable, as object_class_dynamic_cast() has a fast path just
> for similar leaf use case.
> 
> Link: https://lore.kernel.org/r/87pln6ds8q.fsf@pond.sub.org
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qom/container.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/qom/container.c b/qom/container.c
> index cfec92a944..ff6e35f837 100644
> --- a/qom/container.c
> +++ b/qom/container.c
> @@ -24,6 +24,20 @@ static void container_register_types(void)
>      type_register_static(&container_info);
>  }
>  
> +/**
> + * container_get(): Get the container object under specific path
> + *
> + * @root: The root path object to start walking from.  When starting from
> + *        root, one can pass in object_get_root().
> + * @path: The sub-path to lookup, must be an non-empty string starts with "/".
> + *
> + * Returns: The container object specified by @path.
> + *
> + * NOTE: the function may impplicitly create internal containers when the
> + * whole path is not yet created.  It's the caller's responsibility to make
> + * sure the path specified is always used as object containers, rather than
> + * any other type of objects.
> + */

The docs are a welcome addition, but at the same time the docs won't get
read most of the time.

With this in mind, IMHO, it is a conceptually *terrible* design for us to
have a method called "get" which magically *creates* stuff as a side-effect
of its calling. We'd be well served by fixing that design problem.

If I look in the code at what calls we have to container_get, and more
specifically what "path" values we pass, there are not actually that many:

  /objects
  /chardevs
  /unattached
  /machine
  /peripheral
  /peripheral-anon
  /backend
  /dr-connector
  

Ignoring the last one, those other 7 containers are things we expect
to exist in *every* system emulator.

Second, every single one of them is a single level deep. IOW, the for()
loop in container_get is effectively pointless.

We can fix this by having a single method:

 void container_create_builtin(Object *root)

which creates the 7 built-in standard containers we expect
everywhere, with open coded object_new + add_child calls. 

Then all current users of container_get() can switch over
to object_resolve_path, and container_get() can be eliminated.

The 'dr-connector' creation can just be open-coded using
object_new() in the spapr code.

>  Object *container_get(Object *root, const char *path)
>  {
>      Object *obj, *child;
> @@ -31,6 +45,7 @@ Object *container_get(Object *root, const char *path)
>      int i;
>  
>      parts = g_strsplit(path, "/", 0);
> +    /* "path" must be an non-empty string starting with "/" */
>      assert(parts != NULL && parts[0] != NULL && !parts[0][0]);
>      obj = root;
>  
> @@ -40,6 +55,12 @@ Object *container_get(Object *root, const char *path)
>              child = object_new(TYPE_CONTAINER);
>              object_property_add_child(obj, parts[i], child);
>              object_unref(child);
> +        } else {
> +            /*
> +             * Each object within the path must be a container object
> +             * itself, including the object to be returned.
> +             */
> +            assert(object_dynamic_cast(child, TYPE_CONTAINER));
>          }
>      }
>  
> -- 
> 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 5/5] qom: Make container_get() strict to always walk or return container

[Peter Xu]

On Tue, Nov 19, 2024 at 10:03:22AM +0000, Daniel P. Berrangé wrote:
> The docs are a welcome addition, but at the same time the docs won't get
> read most of the time.
> 
> With this in mind, IMHO, it is a conceptually *terrible* design for us to
> have a method called "get" which magically *creates* stuff as a side-effect
> of its calling. We'd be well served by fixing that design problem.
> 
> If I look in the code at what calls we have to container_get, and more
> specifically what "path" values we pass, there are not actually that many:
> 
>   /objects
>   /chardevs
>   /unattached
>   /machine
>   /peripheral
>   /peripheral-anon
>   /backend
>   /dr-connector
>   
> 
> Ignoring the last one, those other 7 containers are things we expect
> to exist in *every* system emulator.
> 
> Second, every single one of them is a single level deep. IOW, the for()
> loop in container_get is effectively pointless.
> 
> We can fix this by having a single method:
> 
>  void container_create_builtin(Object *root)
> 
> which creates the 7 built-in standard containers we expect
> everywhere, with open coded object_new + add_child calls. 
> 
> Then all current users of container_get() can switch over
> to object_resolve_path, and container_get() can be eliminated.
> 
> The 'dr-connector' creation can just be open-coded using
> object_new() in the spapr code.

Yes I think this could make sense, also after I noticed that the assert I
added may not always work..  Please ignore this series then, I'll prepare
something else soon.

-- 
Peter Xu