Re: [PATCH 07/12] ppc/e500: Avoid abuse of container_get()

[Peter Xu]

On Thu, Nov 21, 2024 at 10:48:43AM +0100, Cédric Le Goater wrote:
> On 11/21/24 10:38, Cédric Le Goater wrote:
> > On 11/20/24 22:56, Peter Xu wrote:
> > > 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"));
> > 
> > 
> > why not simply use :
> > 
> >        CCSR(object_resolve_path("/machine/e500-ccsr", NULL));
> 
> 
> I guess we want to avoid the absolute paths. If so,

It wasn't my intention, but what you said actually makes sense to me to
avoid hard-coded "/machine" if possible.

OTOH, object_resolve_path_component() was actually tiny little faster when
we know the depth of the path.

> 
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> 
> 
> We might want to convert these lookups to object_resolve_path_component
> too, not in this patchset.
> 
> hw/i386/acpi-build.c:    host = 
> PCI_HOST_BRIDGE(object_resolve_path("/machine/i440fx", NULL));
> hw/i386/acpi-build.c:        host = 
> PCI_HOST_BRIDGE(object_resolve_path("/machine/q35", NULL));
> target/i386/kvm/kvm.c:        (MemoryRegion *) 
> object_resolve_path("/machine/smram", NULL);
> target/i386/tcg/sysemu/tcg-cpu.c:        (MemoryRegion *) 
> object_resolve_path("/machine/smram", NULL);

Sounds reasonable to me to use the same style.  I'll stick with this patch
as of now in the current series.

Thanks,

-- 
Peter Xu