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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 21/11/24 17:41, Peter Xu wrote:
> 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.

Long term (heterogeneous emulation & dynamic machines in mind) I'm
unsure about qdev_get_machine() future. Not to take care now, but
I'm expecting it to evolve. So not using hardcoded path is better
IMHO.

> 
> 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,
>