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.0On Wed, Nov 20, 2024 at 04:56:58PM -0500, 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(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
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));
Thanks,
C.
>
> memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0", &ccsr->ccsr_space,
> 0, int128_get64(ccsr->ccsr_space.size));
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,
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);
Thanks,
C.
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
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,
>
© 2016 - 2024 Red Hat, Inc.