QEMU will start to not rely on implicit creations of containers soon.  Make
PPC drc devices follow by explicitly create the container whenever a drc
device is realized, dropping container_get() calls.

No functional change intended.

Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/ppc/spapr_drc.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 1484e3209d..3d6ef26b38 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -27,7 +27,7 @@
 #include "sysemu/reset.h"
 #include "trace.h"
 
-#define DRC_CONTAINER_PATH "/dr-connector"
+#define DRC_CONTAINER_PATH "dr-connector"
 #define DRC_INDEX_TYPE_SHIFT 28
 #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1)
 
@@ -514,6 +514,26 @@ static const VMStateDescription vmstate_spapr_drc = {
     }
 };
 
+static GOnce drc_container_created = G_ONCE_INIT;
+
+static gpointer drc_container_create(gpointer unused G_GNUC_UNUSED)
+{
+    container_create(object_get_root(), DRC_CONTAINER_PATH);
+    return NULL;
+}
+
+static Object *drc_container_get(void)
+{
+    return object_resolve_path_component(
+        object_get_root(), DRC_CONTAINER_PATH);
+}
+
+/* TODO: create the container in an ppc init function */
+static void drc_container_create_once(void)
+{
+    g_once(&drc_container_created, drc_container_create, NULL);
+}
+
 static void drc_realize(DeviceState *d, Error **errp)
 {
     SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
@@ -521,6 +541,9 @@ static void drc_realize(DeviceState *d, Error **errp)
     Object *root_container;
     const char *child_name;
 
+    /* Whenever a DRC device is realized, create the container */
+    drc_container_create_once();
+
     trace_spapr_drc_realize(spapr_drc_index(drc));
     /* NOTE: we do this as part of realize/unrealize due to the fact
      * that the guest will communicate with the DRC via RTAS calls
@@ -529,7 +552,7 @@ static void drc_realize(DeviceState *d, Error **errp)
      * inaccessible by the guest, since lookups rely on this path
      * existing in the composition tree
      */
-    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
+    root_container = drc_container_get();
     child_name = object_get_canonical_path_component(OBJECT(drc));
     trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
     object_property_add_alias(root_container, link_name,
@@ -543,12 +566,10 @@ static void drc_unrealize(DeviceState *d)
 {
     SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
     g_autofree gchar *name = g_strdup_printf("%x", spapr_drc_index(drc));
-    Object *root_container;
 
     trace_spapr_drc_unrealize(spapr_drc_index(drc));
     vmstate_unregister(VMSTATE_IF(drc), &vmstate_spapr_drc, drc);
-    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
-    object_property_del(root_container, name);
+    object_property_del(drc_container_get(), name);
 }
 
 SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
@@ -796,9 +817,8 @@ static const TypeInfo spapr_drc_pmem_info = {
 SpaprDrc *spapr_drc_by_index(uint32_t index)
 {
     Object *obj;
-    g_autofree gchar *name = g_strdup_printf("%s/%x", DRC_CONTAINER_PATH,
-                                             index);
-    obj = object_resolve_path(name, NULL);
+    g_autofree gchar *name = g_strdup_printf("%x", index);
+    obj = object_resolve_path_component(drc_container_get(), name);
 
     return !obj ? NULL : SPAPR_DR_CONNECTOR(obj);
 }
@@ -860,7 +880,7 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
     /* aliases for all DRConnector objects will be rooted in QOM
      * composition tree at DRC_CONTAINER_PATH
      */
-    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
+    root_container = drc_container_get();
 
     object_property_iter_init(&iter, root_container);
     while ((prop = object_property_iter_next(&iter))) {
@@ -953,7 +973,7 @@ void spapr_drc_reset_all(SpaprMachineState *spapr)
     ObjectProperty *prop;
     ObjectPropertyIterator iter;
 
-    drc_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
+    drc_container = drc_container_get();
 restart:
     object_property_iter_init(&iter, drc_container);
     while ((prop = object_property_iter_next(&iter))) {
-- 
2.45.0

Hi Peter,

On 20/11/24 22:56, Peter Xu wrote:
> QEMU will start to not rely on implicit creations of containers soon.  Make
> PPC drc devices follow by explicitly create the container whenever a drc
> device is realized, dropping container_get() calls.
> 
> No functional change intended.
> 
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/ppc/spapr_drc.c | 40 ++++++++++++++++++++++++++++++----------
>   1 file changed, 30 insertions(+), 10 deletions(-)


> +static GOnce drc_container_created = G_ONCE_INIT;
> +
> +static gpointer drc_container_create(gpointer unused G_GNUC_UNUSED)
> +{
> +    container_create(object_get_root(), DRC_CONTAINER_PATH);
> +    return NULL;
> +}
> +
> +static Object *drc_container_get(void)
> +{
> +    return object_resolve_path_component(
> +        object_get_root(), DRC_CONTAINER_PATH);
> +}
> +
> +/* TODO: create the container in an ppc init function */
> +static void drc_container_create_once(void)
> +{
> +    g_once(&drc_container_created, drc_container_create, NULL);
> +}
> +
>   static void drc_realize(DeviceState *d, Error **errp)
>   {
>       SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
> @@ -521,6 +541,9 @@ static void drc_realize(DeviceState *d, Error **errp)
>       Object *root_container;
>       const char *child_name;
>   
> +    /* Whenever a DRC device is realized, create the container */
> +    drc_container_create_once();

Can't we just create it once in spapr_dr_connector_class_init(),
removing the G_ONCE_INIT need?

>       trace_spapr_drc_realize(spapr_drc_index(drc));
>       /* NOTE: we do this as part of realize/unrealize due to the fact
>        * that the guest will communicate with the DRC via RTAS calls
> @@ -529,7 +552,7 @@ static void drc_realize(DeviceState *d, Error **errp)
>        * inaccessible by the guest, since lookups rely on this path
>        * existing in the composition tree
>        */
> -    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
> +    root_container = drc_container_get();
>       child_name = object_get_canonical_path_component(OBJECT(drc));
>       trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
>       object_property_add_alias(root_container, link_name,

On Thu, Nov 21, 2024 at 10:35:39AM +0100, Philippe Mathieu-Daudé wrote:
> Hi Peter,
> 
> On 20/11/24 22:56, Peter Xu wrote:
> > QEMU will start to not rely on implicit creations of containers soon.  Make
> > PPC drc devices follow by explicitly create the container whenever a drc
> > device is realized, dropping container_get() calls.
> > 
> > No functional change intended.
> > 
> > Cc: Nicholas Piggin <npiggin@gmail.com>
> > Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
> > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > Cc: qemu-ppc@nongnu.org
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   hw/ppc/spapr_drc.c | 40 ++++++++++++++++++++++++++++++----------
> >   1 file changed, 30 insertions(+), 10 deletions(-)
> 
> 
> > +static GOnce drc_container_created = G_ONCE_INIT;
> > +
> > +static gpointer drc_container_create(gpointer unused G_GNUC_UNUSED)
> > +{
> > +    container_create(object_get_root(), DRC_CONTAINER_PATH);
> > +    return NULL;
> > +}
> > +
> > +static Object *drc_container_get(void)
> > +{
> > +    return object_resolve_path_component(
> > +        object_get_root(), DRC_CONTAINER_PATH);
> > +}
> > +
> > +/* TODO: create the container in an ppc init function */
> > +static void drc_container_create_once(void)
> > +{
> > +    g_once(&drc_container_created, drc_container_create, NULL);
> > +}
> > +
> >   static void drc_realize(DeviceState *d, Error **errp)
> >   {
> >       SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
> > @@ -521,6 +541,9 @@ static void drc_realize(DeviceState *d, Error **errp)
> >       Object *root_container;
> >       const char *child_name;
> > +    /* Whenever a DRC device is realized, create the container */
> > +    drc_container_create_once();
> 
> Can't we just create it once in spapr_dr_connector_class_init(),
> removing the G_ONCE_INIT need?

IIUC it's a matter of whether there's case where we have this file compiled
in, but never create any DRC device.  When that happens, the patch can
change the QEMU qom-tree slightly, in that there'll be an empty drc
container while we used to not have it.

I'm trying to be on the safe side in case something would detect the
container's existance to know whether drc devices are present.  lazy create
it in realize() is the safest way to behave 100% identical like before,
considering my ppc knowledge is merely zero (even if I have drc tests
covered in ppc64's qtest..).

However I also doubt whether it matters much.  It'll be great if I can get
an ACK from anyone familiar with this device (Phil, are you?), then I can
move it over.

> 
> >       trace_spapr_drc_realize(spapr_drc_index(drc));
> >       /* NOTE: we do this as part of realize/unrealize due to the fact
> >        * that the guest will communicate with the DRC via RTAS calls
> > @@ -529,7 +552,7 @@ static void drc_realize(DeviceState *d, Error **errp)
> >        * inaccessible by the guest, since lookups rely on this path
> >        * existing in the composition tree
> >        */
> > -    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
> > +    root_container = drc_container_get();
> >       child_name = object_get_canonical_path_component(OBJECT(drc));
> >       trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
> >       object_property_add_alias(root_container, link_name,
> 

-- 
Peter Xu

On 21/11/24 17:36, Peter Xu wrote:
> On Thu, Nov 21, 2024 at 10:35:39AM +0100, Philippe Mathieu-Daudé wrote:
>> Hi Peter,
>>
>> On 20/11/24 22:56, Peter Xu wrote:
>>> QEMU will start to not rely on implicit creations of containers soon.  Make
>>> PPC drc devices follow by explicitly create the container whenever a drc
>>> device is realized, dropping container_get() calls.
>>>
>>> No functional change intended.
>>>
>>> Cc: Nicholas Piggin <npiggin@gmail.com>
>>> Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
>>> Cc: qemu-ppc@nongnu.org
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>    hw/ppc/spapr_drc.c | 40 ++++++++++++++++++++++++++++++----------
>>>    1 file changed, 30 insertions(+), 10 deletions(-)
>>
>>
>>> +static GOnce drc_container_created = G_ONCE_INIT;
>>> +
>>> +static gpointer drc_container_create(gpointer unused G_GNUC_UNUSED)
>>> +{
>>> +    container_create(object_get_root(), DRC_CONTAINER_PATH);
>>> +    return NULL;
>>> +}
>>> +
>>> +static Object *drc_container_get(void)
>>> +{
>>> +    return object_resolve_path_component(
>>> +        object_get_root(), DRC_CONTAINER_PATH);
>>> +}
>>> +
>>> +/* TODO: create the container in an ppc init function */
>>> +static void drc_container_create_once(void)
>>> +{
>>> +    g_once(&drc_container_created, drc_container_create, NULL);
>>> +}
>>> +
>>>    static void drc_realize(DeviceState *d, Error **errp)
>>>    {
>>>        SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
>>> @@ -521,6 +541,9 @@ static void drc_realize(DeviceState *d, Error **errp)
>>>        Object *root_container;
>>>        const char *child_name;
>>> +    /* Whenever a DRC device is realized, create the container */
>>> +    drc_container_create_once();
>>
>> Can't we just create it once in spapr_dr_connector_class_init(),
>> removing the G_ONCE_INIT need?
> 
> IIUC it's a matter of whether there's case where we have this file compiled
> in, but never create any DRC device.  When that happens, the patch can
> change the QEMU qom-tree slightly, in that there'll be an empty drc
> container while we used to not have it.
> 
> I'm trying to be on the safe side in case something would detect the
> container's existance to know whether drc devices are present.  lazy create
> it in realize() is the safest way to behave 100% identical like before,

Class singleton must be taken care in class_init, not once in the
first instance realize(). I'd rather fix this bad QOM pattern.

> considering my ppc knowledge is merely zero (even if I have drc tests
> covered in ppc64's qtest..).
> 
> However I also doubt whether it matters much.  It'll be great if I can get
> an ACK from anyone familiar with this device (Phil, are you?), then I can
> move it over.

I'm not, but having to care about G_ONCE_INIT seems cumbersome.

> 
>>
>>>        trace_spapr_drc_realize(spapr_drc_index(drc));
>>>        /* NOTE: we do this as part of realize/unrealize due to the fact
>>>         * that the guest will communicate with the DRC via RTAS calls
>>> @@ -529,7 +552,7 @@ static void drc_realize(DeviceState *d, Error **errp)
>>>         * inaccessible by the guest, since lookups rely on this path
>>>         * existing in the composition tree
>>>         */
>>> -    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
>>> +    root_container = drc_container_get();
>>>        child_name = object_get_canonical_path_component(OBJECT(drc));
>>>        trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
>>>        object_property_add_alias(root_container, link_name,
>>
>