Re: [PATCH 06/12] hw/ppc: Explicitly create the drc container

[Philippe Mathieu-Daudé]

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,