[PATCH 0/3] Fix up sam460ex fixes

[PATCH 0/3] Fix up sam460ex fixes

[BALATON Zoltan via]

Accidentally the wrong version of this series was committed, this
series fixes that up to the last version that was meant to be merged.

BALATON Zoltan (3):
  Revert "sam460ex: Remove FDT_PPC dependency from KConfig"
  Revert "ppc4xx: Move common dependency on serial to common option"
  sam460ex: Use type cast macro instead of simple cast

 hw/ppc/Kconfig    | 6 +++++-
 hw/ppc/sam460ex.c | 7 ++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

-- 
2.21.3

[PATCH 2/3] Revert "ppc4xx: Move common dependency on serial to common option"

[BALATON Zoltan via]

This reverts commit e6d5106786 which was added mistakenly. While this
change works it was suggested during review that keeping dependencies
explicit for each board may be better than listing them in a common
option so keep the previous version and revert this change.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/Kconfig | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index d2329edbab..d11dc30509 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -36,6 +36,7 @@ config PPC405
     select M48T59
     select PFLASH_CFI02
     select PPC4XX
+    select SERIAL
 
 config PPC440
     bool
@@ -44,6 +45,7 @@ config PPC440
     imply E1000_PCI
     select PCI_EXPRESS
     select PPC4XX
+    select SERIAL
     select FDT_PPC
 
 config PPC4XX
@@ -51,7 +53,6 @@ config PPC4XX
     select BITBANG_I2C
     select PCI
     select PPC_UIC
-    select SERIAL
 
 config SAM460EX
     bool
@@ -60,6 +61,7 @@ config SAM460EX
     select IDE_SII3112
     select M41T80
     select PPC440
+    select SERIAL
     select SM501
     select SMBUS_EEPROM
     select USB_EHCI_SYSBUS
@@ -121,6 +123,7 @@ config VIRTEX
     bool
     select PPC4XX
     select PFLASH_CFI01
+    select SERIAL
     select XILINX
     select XILINX_ETHLITE
     select FDT_PPC
-- 
2.21.3

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

[BALATON Zoltan via]

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

[PATCH 1/3] Revert "sam460ex: Remove FDT_PPC dependency from KConfig"

[BALATON Zoltan via]

This reverts commit 038da2adf that was mistakenly added, this
dependency is still needed to get libfdt dependencies even if fdt.o is
not needed by sam460ex.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 7e267d94a1..d2329edbab 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -64,6 +64,7 @@ config SAM460EX
     select SMBUS_EEPROM
     select USB_EHCI_SYSBUS
     select USB_OHCI
+    select FDT_PPC
 
 config PREP
     bool
-- 
2.21.3

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

[Greg Kurz]

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

[BALATON Zoltan]

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

[Greg Kurz]

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

[BALATON Zoltan]

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