1. Patchew
  2. QEMU
  3. [PATCH 0/3] Fix up sam460ex fixes
  4. View patch

[PATCH 3/3] sam460ex: Use type cast macro instead of simple cast

BALATON Zoltan via posted 3 patches 4 years, 10 months ago
Maintainers: Greg Kurz <groug@kaod.org>, BALATON Zoltan <balaton@eik.bme.hu>, David Gibson <david@gibson.dropbear.id.au>
There is a newer version of this series
[PATCH 3/3] sam460ex: Use type cast macro instead of simple cast
Posted by BALATON Zoltan via 4 years, 10 months ago 
Use the PCI_BUS type cast macro to convert result of
qdev_get_child_bus(). Also remove the check for NULL afterwards which
should not be needed because sysbus_create_simple() uses error_abort
and PCI_BUS macro also checks its argument by default so this
shouldn't fail here.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/sam460ex.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 14e6583eb0..cc67e9c39b 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -384,11 +384,8 @@ static void sam460ex_init(MachineState *machine)
     ppc460ex_pcie_init(env);
     /* All PCI irqs are connected to the same UIC pin (cf. UBoot source) */
     dev = sysbus_create_simple("ppc440-pcix-host", 0xc0ec00000, uic[1][0]);
-    pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
-    if (!pci_bus) {
-        error_report("couldn't create PCI controller!");
-        exit(1);
-    }
+    pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
+
     memory_region_init_alias(isa, NULL, "isa_mmio", get_system_io(),
                              0, 0x10000);
     memory_region_add_subregion(get_system_memory(), 0xc08000000, isa);
-- 
2.21.3
Re: [PATCH 3/3] sam460ex: Use type cast macro instead of simple cast
Posted by Greg Kurz 4 years, 10 months ago 
On Wed, 6 Jan 2021 16:24:01 +0100
BALATON Zoltan via <qemu-ppc@nongnu.org> wrote:

> Use the PCI_BUS type cast macro to convert result of
> qdev_get_child_bus(). Also remove the check for NULL afterwards which
> should not be needed because sysbus_create_simple() uses error_abort

It seems to me that sysbus_create_simple() doesn't return NULL because
it ends up calling object_new_with_type(). This allocates the object
with either g_malloc() or qemu_memalign(), both of which abort on
failure.

> and PCI_BUS macro also checks its argument by default so this

AFAICT, PCI_BUS() and all other instance type checking macros are
happy with a NULL argument. They simply return NULL in this case.

> shouldn't fail here.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/ppc/sam460ex.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 14e6583eb0..cc67e9c39b 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -384,11 +384,8 @@ static void sam460ex_init(MachineState *machine)
>      ppc460ex_pcie_init(env);
>      /* All PCI irqs are connected to the same UIC pin (cf. UBoot source) */
>      dev = sysbus_create_simple("ppc440-pcix-host", 0xc0ec00000, uic[1][0]);
> -    pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
> -    if (!pci_bus) {
> -        error_report("couldn't create PCI controller!");
> -        exit(1);
> -    }
> +    pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
> +

But PCI_BUS() is being passed qdev_get_child_bus(dev, "pci.0"), not
dev... so the real question here is whether this can return NULL
or not. And if this happens, is this a (1) user or (2) programming
error ?

If (1) then the "if (!pci_bus) { }" should be kept. If (2) then
it should be converted to an assert().

>      memory_region_init_alias(isa, NULL, "isa_mmio", get_system_io(),
>                               0, 0x10000);
>      memory_region_add_subregion(get_system_memory(), 0xc08000000, isa);
Re: [PATCH 3/3] sam460ex: Use type cast macro instead of simple cast
Posted by BALATON Zoltan 4 years, 10 months ago 
On Thu, 7 Jan 2021, Greg Kurz wrote:
> On Wed, 6 Jan 2021 16:24:01 +0100
> BALATON Zoltan via <qemu-ppc@nongnu.org> wrote:
>
>> Use the PCI_BUS type cast macro to convert result of
>> qdev_get_child_bus(). Also remove the check for NULL afterwards which
>> should not be needed because sysbus_create_simple() uses error_abort
>
> It seems to me that sysbus_create_simple() doesn't return NULL because
> it ends up calling object_new_with_type(). This allocates the object
> with either g_malloc() or qemu_memalign(), both of which abort on
> failure.
>
>> and PCI_BUS macro also checks its argument by default so this
>
> AFAICT, PCI_BUS() and all other instance type checking macros are
> happy with a NULL argument. They simply return NULL in this case.

This wasn't my experience when I've got an error in code and got a NULL 
pointer here (on pegasos2 board but same situation). At least with 
qom-debug enabled (which I think is on by default unless explicitly 
disabled in configure) this will abort if the object is not the right 
type.

>> shouldn't fail here.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ppc/sam460ex.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>> index 14e6583eb0..cc67e9c39b 100644
>> --- a/hw/ppc/sam460ex.c
>> +++ b/hw/ppc/sam460ex.c
>> @@ -384,11 +384,8 @@ static void sam460ex_init(MachineState *machine)
>>      ppc460ex_pcie_init(env);
>>      /* All PCI irqs are connected to the same UIC pin (cf. UBoot source) */
>>      dev = sysbus_create_simple("ppc440-pcix-host", 0xc0ec00000, uic[1][0]);
>> -    pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
>> -    if (!pci_bus) {
>> -        error_report("couldn't create PCI controller!");
>> -        exit(1);
>> -    }
>> +    pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
>> +
>
> But PCI_BUS() is being passed qdev_get_child_bus(dev, "pci.0"), not
> dev... so the real question here is whether this can return NULL
> or not. And if this happens, is this a (1) user or (2) programming
> error ?
>
> If (1) then the "if (!pci_bus) { }" should be kept. If (2) then
> it should be converted to an assert().

I think it can only fail if the ppc440-pcix-host type is changed to not 
have a pci.0 child any more which is a programming error that's very 
unlikely to happen but if needed an assert could be added but I don't 
think that's really necessary. The error_report was definitely not needed 
as it's not a user error in any case.

Regards,
BALATON Zoltan

>>      memory_region_init_alias(isa, NULL, "isa_mmio", get_system_io(),
>>                               0, 0x10000);
>>      memory_region_add_subregion(get_system_memory(), 0xc08000000, isa);
>
>
Re: [PATCH 3/3] sam460ex: Use type cast macro instead of simple cast
Posted by Greg Kurz 4 years, 10 months ago 
On Thu, 7 Jan 2021 10:45:26 +0100
BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Thu, 7 Jan 2021, Greg Kurz wrote:
> > On Wed, 6 Jan 2021 16:24:01 +0100
> > BALATON Zoltan via <qemu-ppc@nongnu.org> wrote:
> >
> >> Use the PCI_BUS type cast macro to convert result of
> >> qdev_get_child_bus(). Also remove the check for NULL afterwards which
> >> should not be needed because sysbus_create_simple() uses error_abort
> >
> > It seems to me that sysbus_create_simple() doesn't return NULL because
> > it ends up calling object_new_with_type(). This allocates the object
> > with either g_malloc() or qemu_memalign(), both of which abort on
> > failure.
> >
> >> and PCI_BUS macro also checks its argument by default so this
> >
> > AFAICT, PCI_BUS() and all other instance type checking macros are
> > happy with a NULL argument. They simply return NULL in this case.
> 
> This wasn't my experience when I've got an error in code and got a NULL 
> pointer here (on pegasos2 board but same situation). At least with 
> qom-debug enabled (which I think is on by default unless explicitly 
> disabled in configure) this will abort if the object is not the right 
> type.
> 

You're right that qom-cast-debug is enabled by default and that it
causes object_dynamic_cast_assert() to abort on type mismatch, but
definitely not with a NULL value, as mentioned in this very old
commit:

commit b7f43fe46029d8fd0594cd599fa2599dcce0f553
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Fri Nov 23 16:56:17 2012 +0100

    qom: dynamic_cast of NULL is always NULL
    
    Trying to cast a NULL value will cause a crash.  Returning
    NULL is also sensible, and it is also what the type-unsafe
    DO_UPCAST macro does.
    
    Reported-by: Markus Armbruster <armbru@redhat.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Maybe this should be documented in the function header in "qom/object.h".

> >> shouldn't fail here.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >>  hw/ppc/sam460ex.c | 7 ++-----
> >>  1 file changed, 2 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> >> index 14e6583eb0..cc67e9c39b 100644
> >> --- a/hw/ppc/sam460ex.c
> >> +++ b/hw/ppc/sam460ex.c
> >> @@ -384,11 +384,8 @@ static void sam460ex_init(MachineState *machine)
> >>      ppc460ex_pcie_init(env);
> >>      /* All PCI irqs are connected to the same UIC pin (cf. UBoot source) */
> >>      dev = sysbus_create_simple("ppc440-pcix-host", 0xc0ec00000, uic[1][0]);
> >> -    pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
> >> -    if (!pci_bus) {
> >> -        error_report("couldn't create PCI controller!");
> >> -        exit(1);
> >> -    }
> >> +    pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
> >> +
> >
> > But PCI_BUS() is being passed qdev_get_child_bus(dev, "pci.0"), not
> > dev... so the real question here is whether this can return NULL
> > or not. And if this happens, is this a (1) user or (2) programming
> > error ?
> >
> > If (1) then the "if (!pci_bus) { }" should be kept. If (2) then
> > it should be converted to an assert().
> 
> I think it can only fail if the ppc440-pcix-host type is changed to not 
> have a pci.0 child any more which is a programming error that's very 
> unlikely to happen but if needed an assert could be added but I don't 
> think that's really necessary. The error_report was definitely not needed 
> as it's not a user error in any case.
> 

I was also thinking about a programming error. Whether to add an assert()
or not is up to you, you're the maintainer for this code :)

> Regards,
> BALATON Zoltan
> 
> >>      memory_region_init_alias(isa, NULL, "isa_mmio", get_system_io(),
> >>                               0, 0x10000);
> >>      memory_region_add_subregion(get_system_memory(), 0xc08000000, isa);
> >
> >
Re: [PATCH 3/3] sam460ex: Use type cast macro instead of simple cast
Posted by BALATON Zoltan 4 years, 10 months ago 
On Thu, 7 Jan 2021, Greg Kurz wrote:
> On Thu, 7 Jan 2021 10:45:26 +0100
> BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
>> On Thu, 7 Jan 2021, Greg Kurz wrote:
>>> On Wed, 6 Jan 2021 16:24:01 +0100
>>> BALATON Zoltan via <qemu-ppc@nongnu.org> wrote:
>>>
>>>> Use the PCI_BUS type cast macro to convert result of
>>>> qdev_get_child_bus(). Also remove the check for NULL afterwards which
>>>> should not be needed because sysbus_create_simple() uses error_abort
>>>
>>> It seems to me that sysbus_create_simple() doesn't return NULL because
>>> it ends up calling object_new_with_type(). This allocates the object
>>> with either g_malloc() or qemu_memalign(), both of which abort on
>>> failure.
>>>
>>>> and PCI_BUS macro also checks its argument by default so this
>>>
>>> AFAICT, PCI_BUS() and all other instance type checking macros are
>>> happy with a NULL argument. They simply return NULL in this case.
>>
>> This wasn't my experience when I've got an error in code and got a NULL
>> pointer here (on pegasos2 board but same situation). At least with
>> qom-debug enabled (which I think is on by default unless explicitly
>> disabled in configure) this will abort if the object is not the right
>> type.
>>
>
> You're right that qom-cast-debug is enabled by default and that it
> causes object_dynamic_cast_assert() to abort on type mismatch, but
> definitely not with a NULL value, as mentioned in this very old
> commit:

Indeed, PCI_BUS(NULL) does not abort just returns NULL. I think I 
remembered wrong and had dev==NULL so qdev_get_child_bus() was aborting.

> commit b7f43fe46029d8fd0594cd599fa2599dcce0f553
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Fri Nov 23 16:56:17 2012 +0100
>
>    qom: dynamic_cast of NULL is always NULL
>
>    Trying to cast a NULL value will cause a crash.  Returning
>    NULL is also sensible, and it is also what the type-unsafe
>    DO_UPCAST macro does.
>
>    Reported-by: Markus Armbruster <armbru@redhat.com>
>    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> Maybe this should be documented in the function header in "qom/object.h".
>
>>>> shouldn't fail here.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/ppc/sam460ex.c | 7 ++-----
>>>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>>>> index 14e6583eb0..cc67e9c39b 100644
>>>> --- a/hw/ppc/sam460ex.c
>>>> +++ b/hw/ppc/sam460ex.c
>>>> @@ -384,11 +384,8 @@ static void sam460ex_init(MachineState *machine)
>>>>      ppc460ex_pcie_init(env);
>>>>      /* All PCI irqs are connected to the same UIC pin (cf. UBoot source) */
>>>>      dev = sysbus_create_simple("ppc440-pcix-host", 0xc0ec00000, uic[1][0]);
>>>> -    pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
>>>> -    if (!pci_bus) {
>>>> -        error_report("couldn't create PCI controller!");
>>>> -        exit(1);
>>>> -    }
>>>> +    pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
>>>> +
>>>
>>> But PCI_BUS() is being passed qdev_get_child_bus(dev, "pci.0"), not
>>> dev... so the real question here is whether this can return NULL
>>> or not. And if this happens, is this a (1) user or (2) programming
>>> error ?
>>>
>>> If (1) then the "if (!pci_bus) { }" should be kept. If (2) then
>>> it should be converted to an assert().
>>
>> I think it can only fail if the ppc440-pcix-host type is changed to not
>> have a pci.0 child any more which is a programming error that's very
>> unlikely to happen but if needed an assert could be added but I don't
>> think that's really necessary. The error_report was definitely not needed
>> as it's not a user error in any case.
>>
>
> I was also thinking about a programming error. Whether to add an assert()
> or not is up to you, you're the maintainer for this code :)

In that case I think I keep it simple and don't add an assert because I 
think this error is highly unlikely (we create a pci host object that 
should have a pci bus child) and it would crash anyway shortly when trying 
to add devices so an additional assert here does not seem to help much 
catching a bug.

Regards,
BALATON Zoltan

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.